FLIP-PT-004: authenticated AES-GCM payload encryption + per-trust keys - #845
Draft
atriaybagur wants to merge 8 commits into
Draft
FLIP-PT-004: authenticated AES-GCM payload encryption + per-trust keys#845atriaybagur wants to merge 8 commits into
atriaybagur wants to merge 8 commits into
Conversation
…eys (FLIP-PT-004)
Cross-trust payloads were encrypted with AES-CBC and no authentication, so
ciphertexts were malleable: a tampered payload decrypted to attacker-influenced
plaintext rather than failing. That matters most for the hub->trust task payload,
which carries the cohort SQL a trust then executes against OMOP.
Replace it with AES-256-GCM (AEAD) in a small key-id'd envelope, in flip-api,
trust-api, imaging-api and data-access-api:
- Tampering now raises InvalidTag. The envelope's kid is bound into the
authentication tag, so it cannot be relabelled either.
- Wire format: base64 of {"v":1,"kid","iv","ct"} — 12-byte GCM nonce, and a
fixed algorithm rather than a negotiated one (no algorithm-confusion surface).
- Keys resolve through a keyring, so a service can hold several at once. This is
what makes rotation and per-trust keys possible without every participant
changing over at the same moment.
Per-trust keys (blast radius: one shared key means one compromised trust yields
the key for the whole federation):
- register_trust mints a per-trust key and kid (trust-<id>), returned in the kit
as trust_aes_key / trust_aes_kid.
- submit_cohort_query encrypts project_id per destination trust; a single
ciphertext reused across trusts could not be bound to a per-trust key.
- Trusts without their own key keep using the shared key, so they can be moved
over one at a time.
Roll-out: decrypt() also accepts the old AES-CBC format so a hub and a trust on
different builds still understand each other while hosts are upgraded (on-prem
trusts cannot all be restarted at once). encrypt() is GCM-only, so no new
unauthenticated ciphertext is written, and nothing is stored encrypted at rest —
task payloads are held as plaintext and encrypted at dispatch — so there is no
data to migrate and no key change. Once every host is upgraded, set
AES_ACCEPT_LEGACY_CBC=false to confirm nothing still sends CBC, then delete the
shim; it is marked for removal in each module.
Wiring the kit distributor to write TRUST_AES_KID / TRUST_AES_KEY_BASE64 is the
remaining task before per-trust keys can be switched on; see
docs/aes-payload-keys.md.
Tests, ruff and mypy green across all four services.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Signed-off-by: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Finding IDs belong in commit messages and PR titles, where they link a change to its tracker entry. In source they date the code to one review and mean nothing to a reader outside the organisation, on an Apache-2.0 repo. Every reference carried a real explanation alongside it; only the identifier is removed. The four `encryption.py` module docstrings still say why GCM replaced CBC, `generate_trust_key` still says why a symmetric key has no hash form, and the `submit_cohort_query` comment still points at `docs/aes-payload-keys.md` — which is the durable reference the ID was standing in for. Also covers one pre-existing reference in `test_s3_client.py` that predates this branch. `docs/aes-payload-keys.md` keeps its reference: it is documentation, not source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude <noreply@anthropic.com>
…fields `RegisteredTrust` gained required `trust_aes_key` / `trust_aes_kid` fields earlier on this branch, but this test's `_registered()` helper was not updated. It was failing with a TypeError, and mypy was flagging the same call — so the branch was red before this commit. While pinning it: `ICreatedTrust` deliberately does not carry the AES key. This endpoint backs the admin UI, which has no kit file to write it into, so a trust registered here keeps encrypting under the shared kid until an operator provisions its own key on both sides. That asymmetry with the CLI path is now asserted rather than merely true, so the key cannot start appearing in an HTTP response unnoticed. Verified: 1346 flip-api unit + step-function tests, 55 trust-api, 261 imaging-api, 156 data-access-api; ruff and mypy clean across all four. The remaining failures are the pre-existing `test_mfa_gate` and an imaging download test that assumes a non-root user; the integration errors are Docker-gated and unavailable here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude <noreply@anthropic.com>
…paths `register_trust` mints a per-trust payload-encryption key, and until now neither path handed it over. The admin UI's `ICreatedTrust` dropped it from the response, and the kit distributor never learned to write it — a gap `docs/aes-payload-keys.md` flagged as "the remaining task before per-trust keys can be switched on". So the key was generated and discarded on every registration, and per-trust keys could not actually be turned on for anyone. The hub stores it nowhere. Unlike the api key, a symmetric key the hub must also hold cannot be reduced to a hash, so the value produced at registration is the only copy that will ever exist — dropping it means re-registering the trust to get another. Both paths now deliver it: - `ICreatedTrust` carries `trust_aes_key` / `trust_aes_kid`, and the kit modal includes `TRUST_AES_KEY_BASE64` / `TRUST_AES_KID` in its copy-all block, so the admin pastes seven credential lines instead of five. - `trust_kit_lib` treats them as credential keys, so `register-trust KIT=<CODE>` writes them into `trust/.env.<CODE>.<env>` on a new registration and leaves them untouched on the idempotent skip path — the same write-once discipline the api key already has, and load-bearing here in a way it is not for a hashable credential. Step 1 of the rollout (adding the key to the hub's `AES_TRUST_KEYS`) stays manual; the doc now says so, and says why recording the value at registration matters. Tests pin the property that actually bites: that the key survives to a place the operator can read it. The kit-lib suite asserts it is written on registration and preserved on the skip path, the modal spec asserts it reaches the copy-all block, and the endpoint test asserts it reaches the response. While adding those, one existing assertion needed anchoring. `content.count( "AES_KEY_BASE64=")` was counting a substring, and `AES_KEY_BASE64` is a suffix of `TRUST_AES_KEY_BASE64` — so the new credential silently inflated the count and the duplicate-detection assertion stopped meaning anything. Replaced with an exact-key line count. Note for anyone running these: `scripts/tests/*.py` are standalone harnesses that print ✅/❌ and exit non-zero via `main()`. Under `pytest` the test functions run but never assert, so failures print and the run still reports green — run them as `python scripts/tests/<file>.py`. Verified: 1346 flip-api unit + step-function tests, 1080 flip-ui unit tests, and all six scripts harnesses (PASS=58 FAIL=0 for trust_kit_lib); ruff, mypy and eslint clean. The one failure is the pre-existing `test_mfa_gate`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude <noreply@anthropic.com>
Keeping the admin-UI registration flow means the kit docs have to describe it, and
two statements about it were wrong.
`register-trust KIT=<CODE>` was documented as filling "BOTH the Kit credentials AND
the Hub-shared block in one step", which the Makefile's own comment on
`sync-trust-kit` contradicts: the script runs inside the flip-api container, so
`_hub_shared_from_env()` can only emit the `HUB_SHARED_ENV_KEYS` present in that
task's environment. The deploy-side rest — image tags, registries, NLB subdomain,
FL server port, kit dates — comes from the admin's workstation, which is why sync is
required on prod rather than a rotation-only convenience. Documented as step 3, with
what breaks if it is skipped.
The UI path was described as replaced by the CLI ("the old paste 5 UI lines"). It is
a supported alternative, the count is now seven lines rather than five, and the
`docs/source/deploy-flip/` on-prem guide already lists `POST /admin/trusts` as a way
to satisfy the hub-side prerequisite. Written up as its own paragraph, including the
part that is easy to get wrong: on the UI path sync supplies the *whole* Hub-shared
block, not just the deploy-side remainder, because nothing hub-side was written into
the kit.
Also corrected the Kit-credentials table row. "Hub keeps only the hash" holds for the
api key but not for `TRUST_AES_KEY_BASE64` — the hub retains nothing for that one, so
a lost value costs a re-registration.
`AGENTS.md` regenerated from `CLAUDE.md` with the documented sed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Signed-off-by: Claude <noreply@anthropic.com>
…ring at startup Two gaps found while tracing what happens to the AES key after registration. **The task-dispatch path was still on the shared key.** `GET /tasks/pending` called `encrypt(task.payload)` with no kid, so the main hub->trust channel used the shared key even for a trust that had its own. Every task in a batch belongs to the polling trust and `trust` is already in scope, so the kid resolves once per batch and the whole batch goes under it. Cohort submission already did this; now the two agree. `fl_service.start_training` stays on the shared key, and now says why: it hands one ciphertext to the FL server, which fans it out to every participating client, so encrypting under one trust's key would make it undecryptable for the others. Narrowing that needs a per-client payload from the FL server, not a different kid at the call site. Trust-internal encryption needs no change — on a trust, `_default_kid()` already resolves to that trust's own kid. **A half-configured rollout was invisible.** The trust's kit and the hub's `AES_TRUST_KEYS` are provisioned separately with nothing joining them up, and the two directions disagree asymmetrically when only one is done: trust->hub raises `KeyError: No key registered for kid ...`, but hub->trust silently falls back to the shared key and keeps working, so the isolation is absent with no symptom. `trusts_services/services/trust_key_config.py` audits what the hub can actually see and logs it during startup: coverage (how many trusts have their own key) at info, and at error the two operator mistakes that produce a silent fallback — a kid matching no registered trust (typo, or a trust since deleted; the key loads but is never selected, which at runtime looks exactly like "no key yet"), and a key that is not a valid AES length. It also forces the lazy `AES_TRUST_KEYS` parse, so a malformed value fails at boot instead of on the first request that encrypts. The audit is diagnostics, so a DB error during it is logged and does not stop the app. What the hub still cannot know is whether a trust has `TRUST_AES_KID` set in its kit — that lives on the trust host. The orphan-kid check is the closest hub-side proxy, and the doc says so rather than implying the check is complete. `registered_kids()` exposes the keyring's shape (kid -> key length) so the audit can report and length-check without ever holding key material. Verified: 1356 flip-api unit + step-function tests; ruff and mypy clean. The dispatch test was checked against a reverted `encrypt(task.payload)` to confirm it actually fails, rather than passing vacuously. The one failure is the pre-existing `test_mfa_gate`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude <noreply@anthropic.com>
…ecryption Delivering the key (8172c09) also wrote `TRUST_AES_KID` live into every new kit. That flips the trust onto its own key immediately, while the hub still knows nothing about it — and there is a trust->hub encrypted path that then fails: imaging-api creates an XNAT user and encrypts its password -> services/users.py:171, under the trust's own kid the hub decrypts it to email the credentials -> private_services/imaging_notifications.py:93 -> KeyError: No key registered for kid 'trust-<uuid>' The KeyError lands in a broad `except Exception` that logs and moves on, so the user is created on XNAT and simply never receives their password. The only trace is one log line — the exact silent-failure shape the startup audit was added to surface, introduced by the commit before it. The kit now ships `TRUST_AES_KEY_BASE64` live and `TRUST_AES_KID` commented, so the key is delivered (the problem 8172c09 set out to fix) without changing behaviour. Uncommenting it is the deliberate act that switches the trust over. The admin UI's copy-all block matches, and says why the line is commented. `upsert_commented` writes a commented key while preserving its value — distinct from `comment_out`, which discards the value, and from `upsert`, which would make it live. It leaves an existing *live* line alone, so a re-registration cannot silently switch off a key an operator has already enabled. There is no safe partial state here, which the doc now spells out with both failure modes and the order to avoid them: hub's `AES_TRUST_KEYS` plus a hub restart first (the keyring is memoised for the process lifetime), then uncomment on the trust and restart it. Enabling the trust first breaks the password path above; enabling the hub first makes `kid_for_trust()` select a kid the trust has not loaded, so task payloads stop decrypting. Verified: 1356 flip-api unit + step-function tests, 1080 flip-ui unit tests, all six scripts harnesses (trust_kit_lib now PASS=61); ruff, mypy and eslint clean. Two new kit-lib assertions cover the inert write and the operator-override case. The one failure is the pre-existing `test_mfa_gate`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude <noreply@anthropic.com>
This was referenced Aug 3, 2026
Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cross-trust payloads were encrypted with AES-CBC and no authentication, so ciphertexts were malleable — a tampered payload decrypted to attacker-influenced plaintext rather than failing. That matters most for the hub→trust task payload, which carries the cohort SQL a trust then executes against OMOP.
This replaces the scheme with AES-256-GCM (AEAD) in a small key-id'd envelope, in
flip-api,trust-api,imaging-apianddata-access-api:InvalidTag. The envelope'skidis bound into the authentication tag, so it cannot be relabelled either.{"v":1,"kid","iv","ct"}— 12-byte GCM nonce, and a fixed algorithm rather than a negotiated one (no algorithm-confusion surface).Per-trust keys. With one shared key, a single compromised trust yields the key for the whole federation. So:
register_trustmints a per-trust key and kid (trust-<id>), returned in the kit astrust_aes_key/trust_aes_kid.submit_cohort_queryencryptsproject_idper destination trust — a single ciphertext reused across trusts could not be bound to a per-trust key.Wiring the kit distributor to write
TRUST_AES_KID/TRUST_AES_KEY_BASE64into the kit file is the remaining task before per-trust keys can be switched on — seedocs/aes-payload-keys.md.Roll-out (no coordination, no key change)
decrypt()also accepts the old AES-CBC format, so a hub and a trust on different builds still understand each other while hosts are upgraded — on-prem trusts cannot all be restarted at once.encrypt()is GCM-only, so no new unauthenticated ciphertext is written.Nothing is stored encrypted at rest (task payloads are held as plaintext in the hub DB and encrypted at dispatch;
encrypted_project_idandencrypted_passwordare in-flight only), so there is no data to migrate and no key change —AES_KEY_BASE64is unchanged and a trust operator edits no configuration.AES_ACCEPT_LEGACY_CBC=falseand confirm nothing breaks — that proves no peer still sends CBC.DELETE AFTER ROLL-OUTin each module) in a follow-up PR.One caveat during the upgrade: the hub marks a task
IN_PROGRESSat dispatch, so a task collected by a peer that cannot read it staysIN_PROGRESS— reset any such rows toPENDINGafterwards.Linked Issues
Refs #429
Checklist
Type of Change
make -C docs/ docs.Testing
I did the following tests to verify my changes:
make unit-test.make integration-test.49 unit tests green across the four services (
ruff+mypyclean on every changed file):flip-apiregister_trustCLI kit shape,submit_cohort_querytrust-api/imaging-api/data-access-apiCoverage of the security properties specifically:
InvalidTag(bit-flip onct).kidswap →InvalidTag(the AAD binding).kid→KeyError; unsupportedv→ValueError.kid_for_trustfalls back when a trust has no key yet.AES_ACCEPT_LEGACY_CBC=false; and turning the flag off does not disturb GCM.I ran the affected suites rather than each service's full suite — worth a complete
make testper service in CI before this leaves draft.Additional Notes
make testin CI, and DCO sign-off under a human author — the commit is currently signed off by the automation identity._parse_envelope: CBC ciphertext is indistinguishable from random so it never parses as a JSON object carryingkid, which is why the GCM envelope needed no version prefix.🤖 Generated with Claude Code
https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Generated by Claude Code