Repository navigation
Replies: 2 comments 4 replies
|
Thanks for writing this up so carefully, @amasolov. We checked it and as expected changing A few points we'd like the design to cover:
On question 1, Looking forward to proceed 🙏 |
|
Thanks @mbakgun. I checked each point against
On I've taken your answers to the open questions: I'd suggest four PRs, in order: the key list ( |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Changes in revision 4
Following @mbakgun's review of revision 3, as implemented in #871. Where the sections below still say otherwise, this list wins.
key_rotationsrecords those keys. Refusing anything but the last completed target would have blocked phase 2 of the next rotation.enabledand the weight don't apply. A main kept at weight 0 can still lead, and so can a worker while main is down.HEYM_ACCEPT_CURRENT_ENCRYPTION_KEY=true.heym-keys-fingerprintHMAC already onmain. The primary is its first half, anddecrypt_fingerprintsholds the same HMAC of each key.Changes in revision 3
Following @mbakgun's second review (reply):
k8s:rows and tries up to 20 rows. The check is a module that the worker entrypoints run too, and the docs cover updating existing worker setups (Failing early, Docs).Changes in revision 2
Following @mbakgun's review:
SECRET_KEYstays a strict match (Instance compatibility).ON CONFLICT DO NOTHING(Failing early).updated_at, skipk8s:rows, and keep their progress in the database.audit()is called from the admin action (Re-encryption).Problem
ENCRYPTION_KEYcan't be rotated.services/encryption.pybuilds a singleFernetfromsha256(ENCRYPTION_KEY), and everything Heym encrypts at rest goes through it:credentials.encrypted_config, for every credentialservices/sso_settings.py)api/codex_oauth.py), which are short-livedChanging the value makes everything already stored unreadable. So a leaked key, a staff change or a compliance rule can't be acted on short of re-entering every credential. A key set wrong also fails late and quietly: on a single instance a run just loses
$credentials.x, with no error that names the key.Proposal:
MultiFernetwith online rotationcryptography'sMultiFernettakes an ordered list of keys. It encrypts with the first and decrypts with any of them, androtate()re-encrypts a token under the first key while keeping its original timestamp.ENCRYPTION_KEYENCRYPTION_KEYS_EXTRA(new, optional)Each key goes through the same
sha256derivation as today, so existing data decrypts unchanged, and an install that never setsENCRYPTION_KEYS_EXTRAbehaves exactly as now.ENCRYPTION_KEYS_EXTRAis split on commas and each entry is trimmed. Empty entries are dropped: otherwise a trailing comma would addsha256("")as a valid key. Each remaining entry goes through the same placeholder check asENCRYPTION_KEY, and the settings validator refuses to start on a failure.Rollout order for online rotation
With several processes, or several instances in a cluster, a rolling restart means old and new configuration run side by side for a while. If a process encrypts with the new key before every other process can decrypt it, those other processes fail. So rotation takes two configuration changes, not one:
ENCRYPTION_KEY=old,ENCRYPTION_KEYS_EXTRA=new. Roll every instance.newyet, so a mixed fleet is fine.ENCRYPTION_KEY=new,ENCRYPTION_KEYS_EXTRA=old. Roll every instance.new.newas its primarynewin batches; reads keep working throughout.oldfromENCRYPTION_KEYS_EXTRAA single Docker container restarts all its workers together, so it can go straight to phase 2. The phases matter for multi-instance clusters and for anyone doing rolling updates on Kubernetes.
Instance compatibility
Today
cluster/identity.py:keys_fingerprint()concatenatessha256(ENCRYPTION_KEY)[:16]andsha256(SECRET_KEY)[:16], andcluster/registry.py:is_compatible_with()compares each instance only with main. Two problems with keeping that:new, for example during an OAuth refresh.sha256(ENCRYPTION_KEY)is also the Fernet key material. Publishing a prefix of it puts 64 bits of the actual key into the instances table.Proposed:
Fingerprints are domain-separated:
HMAC-SHA256(key, "heym:fingerprint:v1"), truncated. That is unrelated to the Fernet key bytes.Each instance publishes its primary fingerprint, the set of fingerprints it can decrypt with, and a
SECRET_KEYfingerprint.Eligibility is anchored on main, in two steps:
SECRET_KEYfingerprint match main's exactly.A symmetric rule across all live instances would let one bad instance push out good ones: a worker joining with a typo in its key can't read main's primary, so main itself would drop out of the pool, which can't happen today. Anchoring on main means a bad instance only excludes itself.
The second step can drop two instances that each fail on the other, for example with three different primaries live at once. That only costs capacity and main can't drop out, so the rule stays simple rather than looking for the largest consistent subset. The rollout order never produces that state.
The admin Instances view shows each instance's primary and decrypt set, so a stuck rotation is visible.
Leadership
Dispatch eligibility doesn't apply to leader election today.
lock_service.start()is the first call inlifespan, beforeheartbeat_service.start(), and_try_acquire_leader_lockchecks only the advisory lock. So any process can win leadership, even before its instance has registered and whatever its version or keys. The leader runs cron, the Google and IMAP pollers and the RabbitMQ consumers, which read credentials and write them on OAuth refresh.Two checks cover the two windows:
pg_try_advisory_lock. The leader re-checks on each 5-second loop and releases the lock if it fails.lifespandoesn't matter..envon a single instance, where there's no cluster to exclude it from.Re-encryption (phase 3)
audit()like every other admin route. It is refused unless every live instance reports the same primary fingerprint.MultiFernet.rotate()only if the primary key alone can't decrypt it.credentialsandsso_settings. OAuth state tokens expire on their own and are skipped. Rows holding ak8s:pointer (RFC: Kubernetes Secret-backed credentials #861) are skipped; they store no ciphertext.updated_at. The rewrite is a CoreUPDATE … SET encrypted_config = :new, updated_at = credentials.updated_at, soonupdatedoesn't fire. A re-encryption isn't an edit, and it shouldn't show as one or change anything keyed onupdated_at. This is the one Core-level write tocredentials, and RFC: Kubernetes Secret-backed credentials #861's guard test allow-lists it.SELECT … FOR UPDATE), so an OAuth token refresh at the same moment can't be overwritten with stale data.key_rotationsrow: who started it, target primary, done, remaining, unreadable, state). Progress survives a leader change, every instance reports the same numbers, and the completed row's target is what the key check holds instances to.Failing early on a wrong key
Store a small canary value in the database, encrypted with the primary key.
python -m app.key_check, run before uvicorn starts.release-entrypoint.shruns it afteralembic upgrade head. The worker definitions, which start uvicorn directly, run it first (… key_check && exec uvicorn …). With--workers 8, a failure inside the app's lifespan only makes uvicorn respawn the process, so the check has to run before uvicorn.ENCRYPTION_KEY does not match the key this database was encrypted with;updated_atfirst and skippingk8s:rows, plus the SSO secret. On an install that changed the key before, the rows written since are the ones the current key can read. If one decrypts, or the database holds none, the canary is written. If values exist but none decrypt, startup fails without writing a canary, so a wrong key at first boot is never recorded as the right one.ON CONFLICT DO NOTHING.Docs
deploy/kubernetesand the Compose files gain the extra key as an optional entry in the existing secret (an optionalsecretKeyRefon Kubernetes). The community Helm chart will follow.docker-compose.worker.yml, theworkerservice indocker-compose.cluster.ymlanddeploy/kubernetes/overlays/orbstack-cluster/worker.yaml. All three gain the key check, and the note tells operators to make the same change in their own copies. A worker left on the old entrypoint still runs, without the check, so it falls back on the leadership and dispatch rules. The community Helm chart's worker StatefulSet gets the same change.Out of scope
Decided
ENCRYPTION_KEYS_EXTRA. In phase 1 it holds the new key, soPREVIOUSwould mislead.deploy/kubernetesand Compose, with the extra key optional.Delivery
One PR, not four. On its own, the key list lets an operator add a key but never drop the old one, so a leaked key stays required until the job lands, and dropping it is what a rotation is for. One PR also means one migration and one pass over the docs.
key_rotations, and the per-instance primary and decrypt-set fingerprint columns.MultiFernetinencryption.pyandENCRYPTION_KEYS_EXTRAparsing and validation); the key check module and the entrypoints; compatibility and leadership; the re-encryption job and the admin action; docs and deployment entries.The RabbitMQ config-hash fix from #861 lands with #861's own single PR, as its first commit, as agreed there.
All reactions