From 3dee6bc59aab8a6c42b4ba6dd24da7213b487e62 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 21:06:52 +0000 Subject: [PATCH 1/7] fix(security): authenticated AES-GCM payload encryption + per-trust keys (FLIP-PT-004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-), 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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- docs/aes-payload-keys.md | 80 ++++++ .../cohort_services/submit_cohort_query.py | 16 +- .../flip_api/scripts/generate_trust_key.py | 15 ++ .../src/flip_api/scripts/register_trust.py | 6 + .../services/register_trust.py | 12 +- flip-api/src/flip_api/utils/encryption.py | 254 ++++++++++++++---- .../unit/scripts/test_register_trust_cli.py | 6 + flip-api/tests/unit/utils/test_encryption.py | 159 +++++++++-- .../data_access_api/utils/encryption.py | 240 +++++++++++++---- .../tests/utils/test_encryption.py | 138 +++++++--- .../imaging_api/utils/encryption.py | 240 +++++++++++++---- .../tests/utils/test_encryption.py | 168 ++++++++---- .../trust-api/tests/utils/test_encryption.py | 157 ++++++----- trust/trust-api/trust_api/utils/encryption.py | 226 +++++++++++++--- 14 files changed, 1341 insertions(+), 376 deletions(-) create mode 100644 docs/aes-payload-keys.md diff --git a/docs/aes-payload-keys.md b/docs/aes-payload-keys.md new file mode 100644 index 000000000..2e4ea9e1e --- /dev/null +++ b/docs/aes-payload-keys.md @@ -0,0 +1,80 @@ +# Cross-trust payload encryption — keys and rotation + +Payloads exchanged between the hub and trusts (task payloads carrying cohort SQL, +`encrypted_project_id`, XNAT credentials) are encrypted with **AES-256-GCM** by +`utils/encryption.py` in `flip-api`, `trust-api`, `imaging-api` and +`data-access-api` (FLIP-PT-004). + +GCM is authenticated: a tampered ciphertext raises `InvalidTag` instead of +decrypting to attacker-influenced bytes, which the previous AES-CBC scheme +allowed. The envelope's key-id is bound into the authentication tag, so it cannot +be relabelled either. + +## Envelope + +Base64 of `{"v": 1, "kid": ..., "iv": ..., "ct": ...}` — `iv` is a 12-byte GCM +nonce, `ct` is ciphertext‖tag. The algorithm is fixed rather than negotiated +(no algorithm-confusion surface); `v` exists so the envelope can be changed later. + +## Keys + +| Variable | Where | Purpose | +|---|---|---| +| `AES_KEY_BASE64` | hub + all trusts | Shared key, registered under kid `shared`. Used when nothing else is set. | +| `TRUST_AES_KID` + `TRUST_AES_KEY_BASE64` | one trust | That trust's own key. When set, it is used for everything that service encrypts. | +| `AES_TRUST_KEYS` | hub | JSON `{kid: base64_key}` of per-trust keys, so the hub can encrypt to a specific trust. | + +`register_trust` mints a per-trust key and kid (`trust-`) at +registration, returned in the kit as `trust_aes_key` / `trust_aes_kid`. + +## Giving a trust its own key + +Per-trust keys contain the blast radius: with one shared key, a single +compromised trust yields the key for the whole federation. + +1. Put the trust's key in the hub's `AES_TRUST_KEYS` under `trust-` + (source it from a secret store — **not** the application database, or a DB + compromise becomes a federation-wide key compromise). +2. Set `TRUST_AES_KID` / `TRUST_AES_KEY_BASE64` in that trust's kit + (`trust/.env..`), distributed out-of-band like + `TRUST_INTERNAL_SERVICE_KEY`. + +`kid_for_trust()` then selects that key automatically; trusts without one keep +using the shared key, so trusts can be moved over one at a time. + +> The kit distributor (`scripts/distribute_trust_kits.py` / `trust_kit_lib.py`) +> does not yet write these two variables into the kit file — that wiring is the +> remaining task before per-trust keys can be switched on. + +## Rolling this out to an existing deployment + +`decrypt()` also accepts the previous **AES-CBC** format, so a hub and a trust on +different builds still understand each other while hosts are upgraded — which +matters for on-prem trusts that cannot all be restarted at once. `encrypt()` is +GCM-only, so no new unauthenticated ciphertext is produced, and nothing is stored +encrypted at rest (task payloads are held as plaintext in the hub database and +encrypted at dispatch), so there is no data to migrate. + +There is also no key change: `AES_KEY_BASE64` stays as it is, and per-trust keys +are inert until provisioned. A trust operator changes no configuration — this is +a code deploy only. + +1. Deploy the hub and each trust, in any order and at any pace. +2. When every host is upgraded, set `AES_ACCEPT_LEGACY_CBC=false` and confirm + nothing breaks — that proves no peer is still sending CBC. +3. Delete the shim (`_decrypt_legacy_cbc`, `_accept_legacy_cbc`, the fallback + branch in `decrypt`, and the CBC imports) from all four services. + +One caveat during the upgrade: the hub marks a task `IN_PROGRESS` when it +dispatches it, so a task collected by a peer that cannot read it stays +`IN_PROGRESS`. Reset any such rows to `PENDING` afterwards. + +## Rotation + +Because every payload names its key, receivers can hold several keys at once: +add the new key to the keyring, switch the sender to it, then drop the old one. +No simultaneous change across the hub and every trust. + +Note that some ciphertext is stored, not just in flight — queued `TrustTask` +payloads and `User.encrypted_password` — so re-encrypt or drain those before +removing a key they were encrypted under. diff --git a/flip-api/src/flip_api/cohort_services/submit_cohort_query.py b/flip-api/src/flip_api/cohort_services/submit_cohort_query.py index 300a6fc08..3e1a196b6 100644 --- a/flip-api/src/flip_api/cohort_services/submit_cohort_query.py +++ b/flip-api/src/flip_api/cohort_services/submit_cohort_query.py @@ -31,7 +31,7 @@ TrustDetails, ) from flip_api.domain.schemas.status import TaskType -from flip_api.utils.encryption import encrypt +from flip_api.utils.encryption import encrypt, kid_for_trust from flip_api.utils.logger import logger router = APIRouter(prefix="/cohort", tags=["cohort_services"]) @@ -146,13 +146,17 @@ def submit_cohort_query( result: list[TrustDetails] = [] queried_trust_ids: list[UUID] = [] - # Encrypt project_id before sending to trusts - encrypted_project_id = encrypt(str(cohort_query.project_id)) - logger.debug("Checking if project_id is encrypted: %s", encrypted_project_id) - - # Queue a task for each trust (instead of direct HTTP calls) + # Queue a task for each trust (instead of direct HTTP calls). The + # project_id is encrypted per trust under that trust's key-id — a single + # ciphertext reused across trusts could not be bound to a per-trust key + # (FLIP-PT-004, docs/aes-payload-keys.md). Trusts without their own key + # fall back to the shared kid. for trust in trusts: try: + encrypted_project_id = encrypt( + str(cohort_query.project_id), + kid=kid_for_trust(trust_id=str(trust.id), trust_code=getattr(trust, "code", None)), + ) task_payload = SubmitCohortQueryBody( query_name=cohort_query.name, query=cohort_query.query, diff --git a/flip-api/src/flip_api/scripts/generate_trust_key.py b/flip-api/src/flip_api/scripts/generate_trust_key.py index c8f3df65e..fcbd53867 100644 --- a/flip-api/src/flip_api/scripts/generate_trust_key.py +++ b/flip-api/src/flip_api/scripts/generate_trust_key.py @@ -17,6 +17,7 @@ are never added by hand; ``register_trust`` is the sole writer of the registry. """ +import base64 import hashlib import secrets @@ -30,3 +31,17 @@ def generate_trust_key() -> tuple[str, str]: key = secrets.token_urlsafe(32) key_hash = hashlib.sha256(key.encode()).hexdigest() return key, key_hash + + +def generate_aes_key() -> str: + """Generate a fresh 256-bit AES key, base64-encoded. + + Used by ``register_trust`` to mint a per-trust payload-encryption key + (FLIP-PT-004 step 2). Unlike the API key, this is a symmetric key the hub + must also hold to encrypt to / decrypt from the trust, so — like + ``AES_KEY_BASE64`` — only the plaintext is meaningful (there is no hash form). + + Returns: + str: Base64-encoded 32-byte (AES-256) key. + """ + return base64.b64encode(secrets.token_bytes(32)).decode() diff --git a/flip-api/src/flip_api/scripts/register_trust.py b/flip-api/src/flip_api/scripts/register_trust.py index 84c2e3256..b495628ec 100644 --- a/flip-api/src/flip_api/scripts/register_trust.py +++ b/flip-api/src/flip_api/scripts/register_trust.py @@ -175,6 +175,12 @@ def register_one_trust( "trust_name": kit.trust.name, "trust_api_key": kit.trust_api_key, "trust_internal_service_key": kit.trust_internal_service_key, + # Per-trust payload-encryption key + kid (FLIP-PT-004). Belongs in the + # trust's kit as TRUST_AES_KEY_BASE64 / TRUST_AES_KID; inert until the + # hub also holds it (docs/aes-payload-keys.md), so until then payloads + # keep using the shared key. + "trust_aes_key": kit.trust_aes_key, + "trust_aes_kid": kit.trust_aes_kid, "fl_kit_slot": kit.fl_kit_slot.slot_name, "fl_kit_slot_number": kit.fl_kit_slot.slot_number, "hub_shared": _hub_shared_from_env(), diff --git a/flip-api/src/flip_api/trusts_services/services/register_trust.py b/flip-api/src/flip_api/trusts_services/services/register_trust.py index 2b4db4df3..a5343bd17 100644 --- a/flip-api/src/flip_api/trusts_services/services/register_trust.py +++ b/flip-api/src/flip_api/trusts_services/services/register_trust.py @@ -36,7 +36,7 @@ from flip_api.db.models.main_models import FLKitSlot, Trust from flip_api.db.seed.fl_kit_slots import insert_missing_slots, resolve_fl_kit_slot_names from flip_api.domain.schemas.actions import TrustAuditAction -from flip_api.scripts.generate_trust_key import generate_trust_key +from flip_api.scripts.generate_trust_key import generate_aes_key, generate_trust_key from flip_api.trusts_services.utils.audit_helper import audit_trust_action from flip_api.utils.logger import logger @@ -68,12 +68,20 @@ class RegisteredTrust: Plaintext ``trust_api_key`` and ``trust_internal_service_key`` are returned exactly once: the hub stores only the api-key's SHA-256 hash, and the internal-service key is never persisted hub-side. + + ``trust_aes_key`` / ``trust_aes_kid`` are the per-trust payload-encryption key + and its key-id (FLIP-PT-004). This is a symmetric key the hub must also hold to + encrypt to / decrypt from this trust, so — unlike the api key — it cannot be + reduced to a hash. Until it is provisioned on both sides + (``docs/aes-payload-keys.md``) payloads keep using the shared key. """ trust: Trust fl_kit_slot: FLKitSlot trust_api_key: str trust_internal_service_key: str + trust_aes_key: str + trust_aes_kid: str def _claim_free_slot(session: Session) -> FLKitSlot | None: @@ -216,4 +224,6 @@ def register_trust( fl_kit_slot=slot, trust_api_key=api_key, trust_internal_service_key=internal_key, + trust_aes_key=generate_aes_key(), + trust_aes_kid=f"trust-{trust.id}", ) diff --git a/flip-api/src/flip_api/utils/encryption.py b/flip-api/src/flip_api/utils/encryption.py index 260549aed..7d3d334ce 100644 --- a/flip-api/src/flip_api/utils/encryption.py +++ b/flip-api/src/flip_api/utils/encryption.py @@ -10,88 +10,248 @@ # limitations under the License. # +"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). + +AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC +scheme, which had no authentication: CBC ciphertexts were malleable, so a +tampered payload decrypted to attacker-influenced plaintext instead of failing. +GCM authenticates the ciphertext and the envelope's ``kid``, so any tampering +raises ``InvalidTag``. + +Every ciphertext carries a **key id** (``kid``) and every service resolves keys +through a small keyring. That is what makes key rotation and **per-trust keys** +possible: receivers can hold several keys at once, so a key can be introduced or +retired without every participant changing over at the same moment. + +Wire format — base64 of ``{"v": 1, "kid": ..., "iv": ..., "ct": ...}``, where +``iv`` is a 12-byte GCM nonce and ``ct`` is ciphertext‖tag. ``v`` is a format +discriminator so the envelope itself can be changed later without ambiguity; +the algorithm is fixed (AES-256-GCM) rather than negotiated, which removes any +algorithm-confusion surface. + +Keys (all optional except the shared key): + +- ``AES_KEY_BASE64`` / Secrets Manager ``aes_key`` — the shared key, always + registered under :data:`SHARED_KID`. Used when nothing else is configured. +- ``TRUST_AES_KID`` + ``TRUST_AES_KEY_BASE64`` — a trust's own key. When set, + that key is used for anything this service encrypts. +- ``AES_TRUST_KEYS`` — hub-side JSON map ``{kid: base64_key}`` of per-trust keys, + so the hub can encrypt to (and decrypt from) a specific trust by ``kid``. + +.. rubric:: Temporary compatibility shim — DELETE AFTER ROLL-OUT + +:func:`decrypt` also accepts the previous **AES-CBC** payload format, so a hub and +a trust running different builds can still talk to each other while every host is +upgraded (on-prem trusts cannot all be restarted at once). Nothing ever *writes* +CBC — :func:`encrypt` is GCM-only — so no new unauthenticated ciphertext is +created, and no payload is stored at rest in either format. + +Once every hub and trust runs this build: set ``AES_ACCEPT_LEGACY_CBC=false`` to +verify nothing still sends CBC, then delete :func:`_decrypt_legacy_cbc`, +:func:`_accept_legacy_cbc`, the fallback branch in :func:`decrypt`, and the CBC +imports below. +""" + import base64 +import json import os -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import padding -from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes +from cryptography.hazmat.primitives import padding # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers.aead import AESGCM from flip_api.config import get_settings from flip_api.utils.get_secrets import get_secret -_aes_key_cache: bytes | None = None +#: Key-id under which the shared ``AES_KEY_BASE64`` is registered. +SHARED_KID = "shared" +_VERSION = 1 +_NONCE_BYTES = 12 # NIST SP 800-38D recommended nonce length for AES-GCM -def get_aes_key() -> bytes: - """Retrieve the AES key and return it as bytes. +_keyring_cache: dict[str, bytes] | None = None - In production, fetches from AWS Secrets Manager. In dev, uses the environment variable directly. - Cached after first call — the key does not change during the lifetime of a process. - Returns: - bytes: The decoded AES key. +def _shared_key() -> bytes: + """Return the shared AES key (Secrets Manager in prod, settings in dev).""" + stt = get_settings() + key_b64 = get_secret("aes_key") if stt.ENV == "production" else stt.AES_KEY_BASE64 + return base64.b64decode(key_b64) + + +def _keyring() -> dict[str, bytes]: + """Return the ``kid -> key`` keyring: the shared key plus any configured extras. + + Cached — keys do not change during a process lifetime. """ - global _aes_key_cache # noqa: PLW0603 - if _aes_key_cache is not None: - return _aes_key_cache + global _keyring_cache # noqa: PLW0603 + if _keyring_cache is not None: + return _keyring_cache + + ring: dict[str, bytes] = {SHARED_KID: _shared_key()} + + own_kid, own_key = os.environ.get("TRUST_AES_KID"), os.environ.get("TRUST_AES_KEY_BASE64") + if own_kid and own_key: + ring[own_kid] = base64.b64decode(own_key) + + trust_keys = os.environ.get("AES_TRUST_KEYS") + if trust_keys: + ring.update({kid: base64.b64decode(key) for kid, key in json.loads(trust_keys).items()}) + + _keyring_cache = ring + return ring + + +def _default_kid() -> str: + """Return the kid used when the caller does not name one. + + A service configured with its own key (``TRUST_AES_KID``) uses it; otherwise + the shared key. + """ + return os.environ.get("TRUST_AES_KID") or SHARED_KID + + +def _reset_caches() -> None: + """Clear the keyring cache. Test-only helper.""" + global _keyring_cache # noqa: PLW0603 + _keyring_cache = None - stt = get_settings() - aes_key_b64 = get_secret("aes_key") if stt.ENV == "production" else stt.AES_KEY_BASE64 - _aes_key_cache = base64.b64decode(aes_key_b64) - return _aes_key_cache +def _aad(kid: str) -> bytes: + """Associated data bound into the GCM tag, so the ``kid`` cannot be swapped.""" + return f"FLIP|v{_VERSION}|{kid}".encode() -def encrypt(plaintext: str, key: bytes | None = None) -> str: - """Encrypt plaintext using AES-CBC with PKCS7 padding. Returns Base64-encoded ciphertext. + +def encrypt(plaintext: str, key: bytes | None = None, kid: str | None = None) -> str: + """Encrypt ``plaintext`` with AES-256-GCM. Args: - plaintext (str): The plaintext string to encrypt. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + plaintext (str): The text to encrypt. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by ``kid``. + kid (str | None): Key id. Selects the key when ``key`` is ``None`` + (default: :func:`_default_kid`), and is recorded in the envelope. Returns: - str: Base64-encoded ciphertext, with the random IV prepended to the ciphertext bytes - before encoding. + str: Base64-encoded envelope. + + Raises: + KeyError: ``key`` is ``None`` and ``kid`` is not in the keyring. + ValueError: The key is not a valid AES key length. """ if key is None: - key = get_aes_key() + kid = kid or _default_kid() + key = _keyring()[kid] + else: + kid = kid or SHARED_KID + + nonce = os.urandom(_NONCE_BYTES) + ciphertext = AESGCM(key).encrypt(nonce, plaintext.encode(), _aad(kid)) + envelope = { + "v": _VERSION, + "kid": kid, + "iv": base64.b64encode(nonce).decode(), + "ct": base64.b64encode(ciphertext).decode(), + } + return base64.b64encode(json.dumps(envelope).encode()).decode() + - iv = os.urandom(16) +def _accept_legacy_cbc() -> bool: + """Whether pre-GCM AES-CBC payloads are still accepted. SHIM — delete after roll-out.""" + return os.environ.get("AES_ACCEPT_LEGACY_CBC", "true").strip().lower() not in ("false", "0", "no") - padder = padding.PKCS7(128).padder() - padded_data = padder.update(plaintext.encode()) + padder.finalize() - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - encryptor = cipher.encryptor() - ciphertext = encryptor.update(padded_data) + encryptor.finalize() +def _decrypt_legacy_cbc(raw: bytes, key: bytes | None) -> str: + """Decrypt a pre-GCM ``iv[16] ‖ ciphertext`` AES-CBC payload. - return base64.b64encode(iv + ciphertext).decode() + SHIM — delete after roll-out. Unauthenticated by construction: this is only + here so a not-yet-upgraded peer can still be understood during the upgrade. + + Raises: + ValueError: When ``AES_ACCEPT_LEGACY_CBC`` is disabled. + """ + if not _accept_legacy_cbc(): + raise ValueError("Legacy AES-CBC payload rejected (AES_ACCEPT_LEGACY_CBC is disabled)") + if key is None: + key = _keyring()[SHARED_KID] + + decryptor = Cipher(algorithms.AES(key), modes.CBC(raw[:16])).decryptor() + padded_plaintext = decryptor.update(raw[16:]) + decryptor.finalize() + unpadder = padding.PKCS7(128).unpadder() + return (unpadder.update(padded_plaintext) + unpadder.finalize()).decode() + + +def _parse_envelope(raw: bytes) -> dict | None: + """Return the decoded GCM envelope, or ``None`` if these are not envelope bytes. + + CBC ciphertext is indistinguishable from random, so it never parses as a JSON + object carrying ``kid`` — which makes this a safe way to tell the two formats + apart without putting a marker in the envelope. + """ + try: + envelope = json.loads(raw) + except (UnicodeDecodeError, json.JSONDecodeError): + return None + return envelope if isinstance(envelope, dict) and "kid" in envelope else None def decrypt(encoded_payload: str, key: bytes | None = None) -> str: - """Decrypt Base64-encoded ciphertext using AES-CBC with PKCS7 padding. Returns the original plaintext. + """Decrypt a payload produced by :func:`encrypt`. + + Also accepts the pre-GCM AES-CBC format while the estate is being upgraded — + see the module docstring's shim note. Args: - encoded_payload (str): Base64-encoded payload where the first 16 bytes are the IV and the - remaining bytes are the ciphertext. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + encoded_payload (str): The base64-encoded envelope. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by the envelope's ``kid``. Returns: str: The decrypted plaintext. + + Raises: + cryptography.exceptions.InvalidTag: The payload failed authentication + (tampered ciphertext, wrong key, or altered ``kid``). + KeyError: The envelope's ``kid`` is not in the keyring. + ValueError: The envelope is malformed, an unsupported version, or a legacy + CBC payload arrived while ``AES_ACCEPT_LEGACY_CBC`` is disabled. """ + raw = base64.b64decode(encoded_payload) + + envelope = _parse_envelope(raw) + if envelope is None: + return _decrypt_legacy_cbc(raw, key) # SHIM — delete after roll-out + + if envelope.get("v") != _VERSION: + raise ValueError(f"Unsupported payload version: {envelope.get('v')!r}") + + kid = envelope["kid"] if key is None: - key = get_aes_key() + key = _keyring().get(kid) + if key is None: + raise KeyError(f"No key registered for kid {kid!r}") - encrypted_data = base64.b64decode(encoded_payload) - iv = encrypted_data[:16] - ciphertext = encrypted_data[16:] + nonce = base64.b64decode(envelope["iv"]) + ciphertext = base64.b64decode(envelope["ct"]) + return AESGCM(key).decrypt(nonce, ciphertext, _aad(kid)).decode() - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - decryptor = cipher.decryptor() - padded_plaintext = decryptor.update(ciphertext) + decryptor.finalize() - unpadder = padding.PKCS7(128).unpadder() - plaintext = unpadder.update(padded_plaintext) + unpadder.finalize() - return plaintext.decode() +def kid_for_trust(trust_id: str | None = None, trust_code: str | None = None) -> str: + """Return the ``kid`` to encrypt with for a given trust. + + Per-trust keys are registered as ``trust-``. Falls back to the + default kid when that trust has no key yet, so a trust can be given its own + key independently of the others. + + Args: + trust_id (str | None): The trust's UUID (preferred, stable). + trust_code (str | None): The trust's short code (fallback label). + + Returns: + str: A ``kid`` present in the keyring. + """ + ring = _keyring() + for candidate in (f"trust-{trust_id}" if trust_id else None, f"trust-{trust_code}" if trust_code else None): + if candidate and candidate in ring: + return candidate + return _default_kid() diff --git a/flip-api/tests/unit/scripts/test_register_trust_cli.py b/flip-api/tests/unit/scripts/test_register_trust_cli.py index a16858123..b9d0b9586 100644 --- a/flip-api/tests/unit/scripts/test_register_trust_cli.py +++ b/flip-api/tests/unit/scripts/test_register_trust_cli.py @@ -37,6 +37,8 @@ def _kit(name: str, slot_name: str = "Trust_007", slot_number: int = 7) -> Regis fl_kit_slot=FLKitSlot(slot_name=slot_name, slot_number=slot_number), trust_api_key=f"plain-api-{name}", trust_internal_service_key=f"plain-internal-{name}", + trust_aes_key=f"plain-aes-{name}", + trust_aes_kid=f"trust-{name}", ) @@ -66,11 +68,15 @@ def fake_register_trust(*, name, code, region, session): # noqa: ARG001 assert kit["trust_internal_service_key"] == "plain-internal-Open Trust (EC2)" assert kit["fl_kit_slot"] == "Trust_007" assert kit["fl_kit_slot_number"] == 7 + assert kit["trust_aes_key"] == "plain-aes-Open Trust (EC2)" + assert kit["trust_aes_kid"] == "trust-Open Trust (EC2)" assert set(kit) == { "trust_id", "trust_name", "trust_api_key", "trust_internal_service_key", + "trust_aes_key", + "trust_aes_kid", "fl_kit_slot", "fl_kit_slot_number", "hub_shared", diff --git a/flip-api/tests/unit/utils/test_encryption.py b/flip-api/tests/unit/utils/test_encryption.py index 564108a27..6ab4a970a 100644 --- a/flip-api/tests/unit/utils/test_encryption.py +++ b/flip-api/tests/unit/utils/test_encryption.py @@ -11,49 +11,152 @@ # import base64 -import binascii -from unittest.mock import patch +import json import pytest +from cryptography.exceptions import InvalidTag -from flip_api.utils.encryption import decrypt, encrypt, get_aes_key +from flip_api.utils import encryption +from flip_api.utils.encryption import SHARED_KID, _reset_caches, decrypt, encrypt, kid_for_trust -# Must be exactly 32 bytes (AES-256) +# Must be exactly 32 bytes (AES-256). RAW_KEY_BYTES = b"ThisIsExactly32BytesLongKey!!!!1" ENCODED_KEY = base64.b64encode(RAW_KEY_BYTES).decode() +TRUST_KEY_B64 = base64.b64encode(b"AnotherExactly32ByteAESKey!!!!!2").decode() -@pytest.fixture -def mock_settings(): - """Mock settings for AWS region.""" - with ( - patch("flip_api.utils.encryption.get_settings") as mock_get_settings, - patch("flip_api.utils.encryption._aes_key_cache", None), - ): - mock_get_settings.return_value.ENV = "production" - mock_get_settings.return_value.AES_KEY_BASE64 = ENCODED_KEY - yield mock_get_settings +@pytest.fixture(autouse=True) +def _shared_key_settings(monkeypatch): + """Point the shared key at ``RAW_KEY_BYTES`` and clear key env/caches per test.""" + class _S: + ENV = "development" + AES_KEY_BASE64 = ENCODED_KEY -def test_encryption_decryption_roundtrip(): - plaintext = "This is a test message" - encrypted = encrypt(plaintext, RAW_KEY_BYTES) - decrypted = decrypt(encrypted, RAW_KEY_BYTES) - assert decrypted == plaintext + monkeypatch.setattr(encryption, "get_settings", lambda: _S()) + for var in ("AES_TRUST_KEYS", "TRUST_AES_KID", "TRUST_AES_KEY_BASE64"): + monkeypatch.delenv(var, raising=False) + _reset_caches() + yield + _reset_caches() -def test_get_aes_key_returns_decoded_bytes(mock_settings): - with patch("flip_api.utils.encryption.get_secret", return_value=ENCODED_KEY): - key = get_aes_key() - assert key == RAW_KEY_BYTES +def _envelope(payload: str) -> dict: + return json.loads(base64.b64decode(payload)) -def test_get_aes_key_raises_if_secret_invalid_base64(mock_settings): - with patch("flip_api.utils.encryption.get_secret", return_value="not-base64"): - with pytest.raises(binascii.Error, match="Invalid base64-encoded string"): - get_aes_key() +def _reseal(envelope: dict) -> str: + return base64.b64encode(json.dumps(envelope).encode()).decode() + + +def test_roundtrip_via_keyring(): + assert decrypt(encrypt("hello world")) == "hello world" + + +def test_roundtrip_explicit_key(): + assert decrypt(encrypt("secret", RAW_KEY_BYTES), RAW_KEY_BYTES) == "secret" + + +def test_default_kid_is_shared(): + envelope = _envelope(encrypt("x")) + assert envelope["kid"] == SHARED_KID + assert envelope["v"] == 1 + + +def test_tampered_ciphertext_fails_closed(): + envelope = _envelope(encrypt("do not tamper")) + raw = bytearray(base64.b64decode(envelope["ct"])) + raw[0] ^= 0x01 # flip one bit + envelope["ct"] = base64.b64encode(bytes(raw)).decode() + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope)) + + +def test_kid_swap_fails_closed(): + """The kid is bound into the AAD, so relabelling the envelope must not decrypt.""" + envelope = _envelope(encrypt("bound to kid", RAW_KEY_BYTES, kid="kid-a")) + envelope["kid"] = "kid-b" + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope), RAW_KEY_BYTES) + + +def test_unknown_kid_raises(): + with pytest.raises(KeyError): + decrypt(encrypt("x", RAW_KEY_BYTES, kid="does-not-exist")) + + +def test_unsupported_version_raises(): + envelope = _envelope(encrypt("x")) + envelope["v"] = 99 + with pytest.raises(ValueError, match="Unsupported payload version"): + decrypt(_reseal(envelope)) + + +def test_own_trust_key_is_used_by_default(monkeypatch): + """A service configured with its own key encrypts under that kid, no extra flag.""" + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + payload = encrypt("per-trust") + assert _envelope(payload)["kid"] == "trust-GSTT" + assert decrypt(payload) == "per-trust" + + +def test_hub_encrypts_to_a_specific_trust(monkeypatch): + monkeypatch.setenv("AES_TRUST_KEYS", json.dumps({"trust-abc": TRUST_KEY_B64})) + _reset_caches() + assert decrypt(encrypt("to a trust", kid="trust-abc")) == "to a trust" + + +def test_kid_for_trust_uses_per_trust_key_when_present(monkeypatch): + monkeypatch.setenv("AES_TRUST_KEYS", json.dumps({"trust-abc": TRUST_KEY_B64})) + _reset_caches() + assert kid_for_trust(trust_id="abc") == "trust-abc" + + +def test_kid_for_trust_falls_back_when_unprovisioned(): + assert kid_for_trust(trust_id="abc") == SHARED_KID def test_invalid_key_length_raises(): - with pytest.raises(ValueError, match="Invalid key size"): + with pytest.raises(ValueError, match="key must be 128, 192, or 256 bits"): encrypt("data", b"short") + + +# --- temporary AES-CBC compatibility shim (delete with the shim) --------------- + + +def _legacy_cbc_payload(plaintext: str, key: bytes = RAW_KEY_BYTES) -> str: + """Produce a payload in the pre-GCM format, exactly as an un-upgraded peer would.""" + import os + + from cryptography.hazmat.primitives import padding + from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes + + iv = os.urandom(16) + padder = padding.PKCS7(128).padder() + padded = padder.update(plaintext.encode()) + padder.finalize() + encryptor = Cipher(algorithms.AES(key), modes.CBC(iv)).encryptor() + return base64.b64encode(iv + encryptor.update(padded) + encryptor.finalize()).decode() + + +def test_legacy_cbc_from_unupgraded_peer_still_decrypts(): + assert decrypt(_legacy_cbc_payload("task payload")) == "task payload" + + +def test_legacy_cbc_accepted_with_explicit_key(): + assert decrypt(_legacy_cbc_payload("x"), RAW_KEY_BYTES) == "x" + + +def test_legacy_cbc_rejected_once_disabled(monkeypatch): + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + with pytest.raises(ValueError, match="Legacy AES-CBC payload rejected"): + decrypt(_legacy_cbc_payload("x")) + + +def test_gcm_still_works_when_legacy_disabled(monkeypatch): + """Turning the shim off must not affect the real format.""" + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + assert decrypt(encrypt("still fine")) == "still fine" diff --git a/trust/data-access-api/data_access_api/utils/encryption.py b/trust/data-access-api/data_access_api/utils/encryption.py index 705ac063e..2cf2b0771 100644 --- a/trust/data-access-api/data_access_api/utils/encryption.py +++ b/trust/data-access-api/data_access_api/utils/encryption.py @@ -10,92 +10,224 @@ # limitations under the License. # +"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). + +AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC +scheme, which had no authentication: CBC ciphertexts were malleable, so a +tampered payload decrypted to attacker-influenced plaintext instead of failing. +GCM authenticates the ciphertext and the envelope's ``kid``, so any tampering +raises ``InvalidTag``. + +Every ciphertext carries a **key id** (``kid``) and every service resolves keys +through a small keyring. That is what makes key rotation and **per-trust keys** +possible: receivers can hold several keys at once, so a key can be introduced or +retired without every participant changing over at the same moment. + +Wire format — base64 of ``{"v": 1, "kid": ..., "iv": ..., "ct": ...}``, where +``iv`` is a 12-byte GCM nonce and ``ct`` is ciphertext‖tag. ``v`` is a format +discriminator so the envelope itself can be changed later without ambiguity; +the algorithm is fixed (AES-256-GCM) rather than negotiated, which removes any +algorithm-confusion surface. + +Keys (all optional except the shared key): + +- ``AES_KEY_BASE64`` — the shared key, always registered under + :data:`SHARED_KID`. Used when nothing else is configured. +- ``TRUST_AES_KID`` + ``TRUST_AES_KEY_BASE64`` — a trust's own key. When set, + that key is used for anything this service encrypts. +- ``AES_TRUST_KEYS`` — hub-side JSON map ``{kid: base64_key}`` of per-trust keys, + so the hub can encrypt to (and decrypt from) a specific trust by ``kid``. + +.. rubric:: Temporary compatibility shim — DELETE AFTER ROLL-OUT + +:func:`decrypt` also accepts the previous **AES-CBC** payload format, so a hub and +a trust running different builds can still talk to each other while every host is +upgraded (on-prem trusts cannot all be restarted at once). Nothing ever *writes* +CBC — :func:`encrypt` is GCM-only — so no new unauthenticated ciphertext is +created, and no payload is stored at rest in either format. + +Once every hub and trust runs this build: set ``AES_ACCEPT_LEGACY_CBC=false`` to +verify nothing still sends CBC, then delete :func:`_decrypt_legacy_cbc`, +:func:`_accept_legacy_cbc`, the fallback branch in :func:`decrypt`, and the CBC +imports below. +""" + import base64 +import json import os -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import padding -from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes +from cryptography.hazmat.primitives import padding # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers.aead import AESGCM from data_access_api.config import get_settings +#: Key-id under which the shared ``AES_KEY_BASE64`` is registered. +SHARED_KID = "shared" -# --- Step 1: Load AES key from environment file --- -def get_aes_key() -> bytes: - """Retrieve the AES key from the environment file and return it as bytes. +_VERSION = 1 +_NONCE_BYTES = 12 # NIST SP 800-38D recommended nonce length for AES-GCM - Returns: - bytes: The decoded AES key (16, 24, or 32 bytes). +_keyring_cache: dict[str, bytes] | None = None - Raises: - ValueError: If the AES key is missing from configuration or has an invalid length. + +def _shared_key() -> bytes: + """Return the shared AES key from settings (``AES_KEY_BASE64``).""" + return base64.b64decode(get_settings().AES_KEY_BASE64) + + +def _keyring() -> dict[str, bytes]: + """Return the ``kid -> key`` keyring: the shared key plus any configured extras. + + Cached — keys do not change during a process lifetime. + """ + global _keyring_cache # noqa: PLW0603 + if _keyring_cache is not None: + return _keyring_cache + + ring: dict[str, bytes] = {SHARED_KID: _shared_key()} + + own_kid, own_key = os.environ.get("TRUST_AES_KID"), os.environ.get("TRUST_AES_KEY_BASE64") + if own_kid and own_key: + ring[own_kid] = base64.b64decode(own_key) + + trust_keys = os.environ.get("AES_TRUST_KEYS") + if trust_keys: + ring.update({kid: base64.b64decode(key) for kid, key in json.loads(trust_keys).items()}) + + _keyring_cache = ring + return ring + + +def _default_kid() -> str: + """Return the kid used when the caller does not name one. + + A service configured with its own key (``TRUST_AES_KID``) uses it; otherwise + the shared key. """ - key_b64 = get_settings().AES_KEY_BASE64 - if not key_b64: - raise ValueError("AES key not found in environment file") + return os.environ.get("TRUST_AES_KID") or SHARED_KID - key = base64.b64decode(key_b64) - if len(key) not in (16, 24, 32): - raise ValueError("Invalid AES key length") - return key +def _reset_caches() -> None: + """Clear the keyring cache. Test-only helper.""" + global _keyring_cache # noqa: PLW0603 + _keyring_cache = None -# --- Step 2: AES-CBC encryption --- -def encrypt(plaintext: str, key: bytes | None = None) -> str: - """Encrypt plaintext using AES-CBC with PKCS7 padding. Returns Base64-encoded ciphertext. + +def _aad(kid: str) -> bytes: + """Associated data bound into the GCM tag, so the ``kid`` cannot be swapped.""" + return f"FLIP|v{_VERSION}|{kid}".encode() + + +def encrypt(plaintext: str, key: bytes | None = None, kid: str | None = None) -> str: + """Encrypt ``plaintext`` with AES-256-GCM. Args: - plaintext (str): The plaintext string to encrypt. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + plaintext (str): The text to encrypt. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by ``kid``. + kid (str | None): Key id. Selects the key when ``key`` is ``None`` + (default: :func:`_default_kid`), and is recorded in the envelope. Returns: - str: Base64-encoded ciphertext with the random 16-byte IV prepended to the ciphertext - bytes before encoding. + str: Base64-encoded envelope. + + Raises: + KeyError: ``key`` is ``None`` and ``kid`` is not in the keyring. + ValueError: The key is not a valid AES key length. """ if key is None: - key = get_aes_key() + kid = kid or _default_kid() + key = _keyring()[kid] + else: + kid = kid or SHARED_KID - iv = os.urandom(16) + nonce = os.urandom(_NONCE_BYTES) + ciphertext = AESGCM(key).encrypt(nonce, plaintext.encode(), _aad(kid)) + envelope = { + "v": _VERSION, + "kid": kid, + "iv": base64.b64encode(nonce).decode(), + "ct": base64.b64encode(ciphertext).decode(), + } + return base64.b64encode(json.dumps(envelope).encode()).decode() - # Pad plaintext to 128-bit (16-byte) blocks - padder = padding.PKCS7(128).padder() - padded_data = padder.update(plaintext.encode()) + padder.finalize() - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - encryptor = cipher.encryptor() - ciphertext = encryptor.update(padded_data) + encryptor.finalize() +def _accept_legacy_cbc() -> bool: + """Whether pre-GCM AES-CBC payloads are still accepted. SHIM — delete after roll-out.""" + return os.environ.get("AES_ACCEPT_LEGACY_CBC", "true").strip().lower() not in ("false", "0", "no") - # Combine IV and ciphertext, then encode for storage/transmission - encrypted_payload = iv + ciphertext - return base64.b64encode(encrypted_payload).decode() + +def _decrypt_legacy_cbc(raw: bytes, key: bytes | None) -> str: + """Decrypt a pre-GCM ``iv[16] ‖ ciphertext`` AES-CBC payload. + + SHIM — delete after roll-out. Unauthenticated by construction: this is only + here so a not-yet-upgraded peer can still be understood during the upgrade. + + Raises: + ValueError: When ``AES_ACCEPT_LEGACY_CBC`` is disabled. + """ + if not _accept_legacy_cbc(): + raise ValueError("Legacy AES-CBC payload rejected (AES_ACCEPT_LEGACY_CBC is disabled)") + if key is None: + key = _keyring()[SHARED_KID] + + decryptor = Cipher(algorithms.AES(key), modes.CBC(raw[:16])).decryptor() + padded_plaintext = decryptor.update(raw[16:]) + decryptor.finalize() + unpadder = padding.PKCS7(128).unpadder() + return (unpadder.update(padded_plaintext) + unpadder.finalize()).decode() + + +def _parse_envelope(raw: bytes) -> dict | None: + """Return the decoded GCM envelope, or ``None`` if these are not envelope bytes. + + CBC ciphertext is indistinguishable from random, so it never parses as a JSON + object carrying ``kid`` — which makes this a safe way to tell the two formats + apart without putting a marker in the envelope. + """ + try: + envelope = json.loads(raw) + except (UnicodeDecodeError, json.JSONDecodeError): + return None + return envelope if isinstance(envelope, dict) and "kid" in envelope else None -# --- Step 3: AES-CBC decryption --- def decrypt(encoded_payload: str, key: bytes | None = None) -> str: - """Decrypt Base64-encoded ciphertext using AES-CBC with PKCS7 padding. Returns the original plaintext. + """Decrypt a payload produced by :func:`encrypt`. + + Also accepts the pre-GCM AES-CBC format while the estate is being upgraded — + see the module docstring's shim note. Args: - encoded_payload (str): Base64-encoded payload where the first 16 bytes are the IV and the - remaining bytes are the ciphertext. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + encoded_payload (str): The base64-encoded envelope. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by the envelope's ``kid``. Returns: str: The decrypted plaintext. + + Raises: + cryptography.exceptions.InvalidTag: The payload failed authentication + (tampered ciphertext, wrong key, or altered ``kid``). + KeyError: The envelope's ``kid`` is not in the keyring. + ValueError: The envelope is malformed, an unsupported version, or a legacy + CBC payload arrived while ``AES_ACCEPT_LEGACY_CBC`` is disabled. """ - if key is None: - key = get_aes_key() + raw = base64.b64decode(encoded_payload) - encrypted_data = base64.b64decode(encoded_payload) - iv = encrypted_data[:16] - ciphertext = encrypted_data[16:] + envelope = _parse_envelope(raw) + if envelope is None: + return _decrypt_legacy_cbc(raw, key) # SHIM — delete after roll-out - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - decryptor = cipher.decryptor() - padded_plaintext = decryptor.update(ciphertext) + decryptor.finalize() + if envelope.get("v") != _VERSION: + raise ValueError(f"Unsupported payload version: {envelope.get('v')!r}") - # Unpad to recover original plaintext - unpadder = padding.PKCS7(128).unpadder() - plaintext = unpadder.update(padded_plaintext) + unpadder.finalize() - return plaintext.decode() + kid = envelope["kid"] + if key is None: + key = _keyring().get(kid) + if key is None: + raise KeyError(f"No key registered for kid {kid!r}") + + nonce = base64.b64decode(envelope["iv"]) + ciphertext = base64.b64decode(envelope["ct"]) + return AESGCM(key).decrypt(nonce, ciphertext, _aad(kid)).decode() diff --git a/trust/data-access-api/tests/utils/test_encryption.py b/trust/data-access-api/tests/utils/test_encryption.py index 0df9fd9f4..41202bc4f 100644 --- a/trust/data-access-api/tests/utils/test_encryption.py +++ b/trust/data-access-api/tests/utils/test_encryption.py @@ -11,56 +11,120 @@ # import base64 -from unittest.mock import patch +import json import pytest +from cryptography.exceptions import InvalidTag -from data_access_api.utils.encryption import decrypt, encrypt, get_aes_key +from data_access_api.utils import encryption +from data_access_api.utils.encryption import SHARED_KID, _reset_caches, decrypt, encrypt -# Helper: generate valid 32-byte key (256-bit AES) -VALID_KEY = b"thisisaverysecurekey123456789012" # 32 bytes -VALID_KEY_B64 = base64.b64encode(VALID_KEY).decode() +RAW_KEY_BYTES = b"ThisIsExactly32BytesLongKey!!!!1" +ENCODED_KEY = base64.b64encode(RAW_KEY_BYTES).decode() +TRUST_KEY_B64 = base64.b64encode(b"AnotherExactly32ByteAESKey!!!!!2").decode() -@patch("data_access_api.utils.encryption.get_settings") -def test_get_aes_key_valid(mock_get_settings): - mock_get_settings.return_value.AES_KEY_BASE64 = VALID_KEY_B64 - key = get_aes_key() - assert key == VALID_KEY +@pytest.fixture(autouse=True) +def _shared_key_settings(monkeypatch): + monkeypatch.setattr(encryption, "get_settings", lambda: type("S", (), {"AES_KEY_BASE64": ENCODED_KEY})()) + for var in ("AES_TRUST_KEYS", "TRUST_AES_KID", "TRUST_AES_KEY_BASE64"): + monkeypatch.delenv(var, raising=False) + _reset_caches() + yield + _reset_caches() -@patch("data_access_api.utils.encryption.get_settings") -def test_get_aes_key_missing(mock_get_settings): - mock_get_settings.return_value.AES_KEY_BASE64 = None - with pytest.raises(ValueError, match="AES key not found in environment file"): - get_aes_key() +def _envelope(payload: str) -> dict: + return json.loads(base64.b64decode(payload)) -@patch("data_access_api.utils.encryption.get_settings") -def test_get_aes_key_invalid_length(mock_get_settings): - short_key = base64.b64encode(b"shortkey").decode() - mock_get_settings.return_value.AES_KEY_BASE64 = short_key - with pytest.raises(ValueError, match="Invalid AES key length"): - get_aes_key() +def _reseal(envelope: dict) -> str: + return base64.b64encode(json.dumps(envelope).encode()).decode() -def test_encrypt_decrypt_roundtrip(): - plaintext = "Sensitive data payload" - encrypted = encrypt(plaintext, key=VALID_KEY) - decrypted = decrypt(encrypted, key=VALID_KEY) - assert decrypted == plaintext +def test_roundtrip_via_keyring(): + assert decrypt(encrypt("hello")) == "hello" -def test_encrypt_decrypt_roundtrip_with_None(): - plaintext = "Sensitive data payload" - encrypted = encrypt(plaintext, key=None) - decrypted = decrypt(encrypted, key=None) - assert decrypted == plaintext +def test_roundtrip_explicit_key(): + assert decrypt(encrypt("secret", key=RAW_KEY_BYTES), key=RAW_KEY_BYTES) == "secret" -def test_encrypt_output_is_base64(): - encrypted = encrypt("some text", key=VALID_KEY) - # This will raise if it's not valid base64 - decoded = base64.b64decode(encrypted) - assert isinstance(decoded, bytes) - assert len(decoded) > 16 # IV + ciphertext +def test_default_kid_is_shared(): + assert _envelope(encrypt("x"))["kid"] == SHARED_KID + + +def test_tampered_ciphertext_fails_closed(): + envelope = _envelope(encrypt("do not tamper")) + raw = bytearray(base64.b64decode(envelope["ct"])) + raw[0] ^= 0x01 + envelope["ct"] = base64.b64encode(bytes(raw)).decode() + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope)) + + +def test_kid_swap_fails_closed(): + envelope = _envelope(encrypt("bound", key=RAW_KEY_BYTES, kid="kid-a")) + envelope["kid"] = "kid-b" + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope), key=RAW_KEY_BYTES) + + +def test_unsupported_version_raises(): + envelope = _envelope(encrypt("x")) + envelope["v"] = 99 + with pytest.raises(ValueError, match="Unsupported payload version"): + decrypt(_reseal(envelope)) + + +def test_own_trust_key_is_used_by_default(monkeypatch): + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + payload = encrypt("per-trust") + assert _envelope(payload)["kid"] == "trust-GSTT" + assert decrypt(payload) == "per-trust" + + +def test_decrypts_payload_addressed_to_this_trust(monkeypatch): + """Hub encrypts to this trust's kid; the trust decrypts it with its own key.""" + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + from_hub = encrypt("task payload", key=base64.b64decode(TRUST_KEY_B64), kid="trust-GSTT") + assert decrypt(from_hub) == "task payload" + + +# --- temporary AES-CBC compatibility shim (delete with the shim) --------------- + + +def _legacy_cbc_payload(plaintext: str, key: bytes = RAW_KEY_BYTES) -> str: + """Produce a payload in the pre-GCM format, exactly as an un-upgraded peer would.""" + import os + + from cryptography.hazmat.primitives import padding + from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes + + iv = os.urandom(16) + padder = padding.PKCS7(128).padder() + padded = padder.update(plaintext.encode()) + padder.finalize() + encryptor = Cipher(algorithms.AES(key), modes.CBC(iv)).encryptor() + return base64.b64encode(iv + encryptor.update(padded) + encryptor.finalize()).decode() + + +def test_legacy_cbc_from_unupgraded_hub_still_decrypts(): + assert decrypt(_legacy_cbc_payload("task payload")) == "task payload" + + +def test_legacy_cbc_rejected_once_disabled(monkeypatch): + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + with pytest.raises(ValueError, match="Legacy AES-CBC payload rejected"): + decrypt(_legacy_cbc_payload("x")) + + +def test_gcm_still_works_when_legacy_disabled(monkeypatch): + """Turning the shim off must not affect the real format.""" + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + assert decrypt(encrypt("still fine")) == "still fine" diff --git a/trust/imaging-api/imaging_api/utils/encryption.py b/trust/imaging-api/imaging_api/utils/encryption.py index c0a37ff69..da25a3d80 100644 --- a/trust/imaging-api/imaging_api/utils/encryption.py +++ b/trust/imaging-api/imaging_api/utils/encryption.py @@ -10,92 +10,224 @@ # limitations under the License. # +"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). + +AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC +scheme, which had no authentication: CBC ciphertexts were malleable, so a +tampered payload decrypted to attacker-influenced plaintext instead of failing. +GCM authenticates the ciphertext and the envelope's ``kid``, so any tampering +raises ``InvalidTag``. + +Every ciphertext carries a **key id** (``kid``) and every service resolves keys +through a small keyring. That is what makes key rotation and **per-trust keys** +possible: receivers can hold several keys at once, so a key can be introduced or +retired without every participant changing over at the same moment. + +Wire format — base64 of ``{"v": 1, "kid": ..., "iv": ..., "ct": ...}``, where +``iv`` is a 12-byte GCM nonce and ``ct`` is ciphertext‖tag. ``v`` is a format +discriminator so the envelope itself can be changed later without ambiguity; +the algorithm is fixed (AES-256-GCM) rather than negotiated, which removes any +algorithm-confusion surface. + +Keys (all optional except the shared key): + +- ``AES_KEY_BASE64`` — the shared key, always registered under + :data:`SHARED_KID`. Used when nothing else is configured. +- ``TRUST_AES_KID`` + ``TRUST_AES_KEY_BASE64`` — a trust's own key. When set, + that key is used for anything this service encrypts. +- ``AES_TRUST_KEYS`` — hub-side JSON map ``{kid: base64_key}`` of per-trust keys, + so the hub can encrypt to (and decrypt from) a specific trust by ``kid``. + +.. rubric:: Temporary compatibility shim — DELETE AFTER ROLL-OUT + +:func:`decrypt` also accepts the previous **AES-CBC** payload format, so a hub and +a trust running different builds can still talk to each other while every host is +upgraded (on-prem trusts cannot all be restarted at once). Nothing ever *writes* +CBC — :func:`encrypt` is GCM-only — so no new unauthenticated ciphertext is +created, and no payload is stored at rest in either format. + +Once every hub and trust runs this build: set ``AES_ACCEPT_LEGACY_CBC=false`` to +verify nothing still sends CBC, then delete :func:`_decrypt_legacy_cbc`, +:func:`_accept_legacy_cbc`, the fallback branch in :func:`decrypt`, and the CBC +imports below. +""" + import base64 +import json import os -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import padding -from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes +from cryptography.hazmat.primitives import padding # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers.aead import AESGCM from imaging_api.config import get_settings +#: Key-id under which the shared ``AES_KEY_BASE64`` is registered. +SHARED_KID = "shared" -# --- Step 1: Load AES key from environment file --- -def get_aes_key() -> bytes: - """Retrieve the AES key from the environment file and return it as bytes. +_VERSION = 1 +_NONCE_BYTES = 12 # NIST SP 800-38D recommended nonce length for AES-GCM - Returns: - bytes: The decoded AES key (16, 24, or 32 bytes). +_keyring_cache: dict[str, bytes] | None = None - Raises: - ValueError: If the AES key is missing from configuration or has an invalid length. + +def _shared_key() -> bytes: + """Return the shared AES key from settings (``AES_KEY_BASE64``).""" + return base64.b64decode(get_settings().AES_KEY_BASE64) + + +def _keyring() -> dict[str, bytes]: + """Return the ``kid -> key`` keyring: the shared key plus any configured extras. + + Cached — keys do not change during a process lifetime. + """ + global _keyring_cache # noqa: PLW0603 + if _keyring_cache is not None: + return _keyring_cache + + ring: dict[str, bytes] = {SHARED_KID: _shared_key()} + + own_kid, own_key = os.environ.get("TRUST_AES_KID"), os.environ.get("TRUST_AES_KEY_BASE64") + if own_kid and own_key: + ring[own_kid] = base64.b64decode(own_key) + + trust_keys = os.environ.get("AES_TRUST_KEYS") + if trust_keys: + ring.update({kid: base64.b64decode(key) for kid, key in json.loads(trust_keys).items()}) + + _keyring_cache = ring + return ring + + +def _default_kid() -> str: + """Return the kid used when the caller does not name one. + + A service configured with its own key (``TRUST_AES_KID``) uses it; otherwise + the shared key. """ - key_b64 = get_settings().AES_KEY_BASE64 - if not key_b64: - raise ValueError("AES key not found in environment file") + return os.environ.get("TRUST_AES_KID") or SHARED_KID - key = base64.b64decode(key_b64) - if len(key) not in (16, 24, 32): - raise ValueError("Invalid AES key length") - return key +def _reset_caches() -> None: + """Clear the keyring cache. Test-only helper.""" + global _keyring_cache # noqa: PLW0603 + _keyring_cache = None -# --- Step 2: AES-CBC encryption --- -def encrypt(plaintext: str, key: bytes | None = None) -> str: - """Encrypt plaintext using AES-CBC with PKCS7 padding. Returns Base64-encoded ciphertext. + +def _aad(kid: str) -> bytes: + """Associated data bound into the GCM tag, so the ``kid`` cannot be swapped.""" + return f"FLIP|v{_VERSION}|{kid}".encode() + + +def encrypt(plaintext: str, key: bytes | None = None, kid: str | None = None) -> str: + """Encrypt ``plaintext`` with AES-256-GCM. Args: - plaintext (str): The plaintext string to encrypt. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + plaintext (str): The text to encrypt. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by ``kid``. + kid (str | None): Key id. Selects the key when ``key`` is ``None`` + (default: :func:`_default_kid`), and is recorded in the envelope. Returns: - str: Base64-encoded ciphertext with the random 16-byte IV prepended to the ciphertext - bytes before encoding. + str: Base64-encoded envelope. + + Raises: + KeyError: ``key`` is ``None`` and ``kid`` is not in the keyring. + ValueError: The key is not a valid AES key length. """ if key is None: - key = get_aes_key() + kid = kid or _default_kid() + key = _keyring()[kid] + else: + kid = kid or SHARED_KID - iv = os.urandom(16) + nonce = os.urandom(_NONCE_BYTES) + ciphertext = AESGCM(key).encrypt(nonce, plaintext.encode(), _aad(kid)) + envelope = { + "v": _VERSION, + "kid": kid, + "iv": base64.b64encode(nonce).decode(), + "ct": base64.b64encode(ciphertext).decode(), + } + return base64.b64encode(json.dumps(envelope).encode()).decode() - # Pad plaintext to 128-bit (16-byte) blocks - padder = padding.PKCS7(128).padder() - padded_data = padder.update(plaintext.encode()) + padder.finalize() - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - encryptor = cipher.encryptor() - ciphertext = encryptor.update(padded_data) + encryptor.finalize() +def _accept_legacy_cbc() -> bool: + """Whether pre-GCM AES-CBC payloads are still accepted. SHIM — delete after roll-out.""" + return os.environ.get("AES_ACCEPT_LEGACY_CBC", "true").strip().lower() not in ("false", "0", "no") - # Combine IV and ciphertext, then encode for storage/transmission - encrypted_payload = iv + ciphertext - return base64.b64encode(encrypted_payload).decode() + +def _decrypt_legacy_cbc(raw: bytes, key: bytes | None) -> str: + """Decrypt a pre-GCM ``iv[16] ‖ ciphertext`` AES-CBC payload. + + SHIM — delete after roll-out. Unauthenticated by construction: this is only + here so a not-yet-upgraded peer can still be understood during the upgrade. + + Raises: + ValueError: When ``AES_ACCEPT_LEGACY_CBC`` is disabled. + """ + if not _accept_legacy_cbc(): + raise ValueError("Legacy AES-CBC payload rejected (AES_ACCEPT_LEGACY_CBC is disabled)") + if key is None: + key = _keyring()[SHARED_KID] + + decryptor = Cipher(algorithms.AES(key), modes.CBC(raw[:16])).decryptor() + padded_plaintext = decryptor.update(raw[16:]) + decryptor.finalize() + unpadder = padding.PKCS7(128).unpadder() + return (unpadder.update(padded_plaintext) + unpadder.finalize()).decode() + + +def _parse_envelope(raw: bytes) -> dict | None: + """Return the decoded GCM envelope, or ``None`` if these are not envelope bytes. + + CBC ciphertext is indistinguishable from random, so it never parses as a JSON + object carrying ``kid`` — which makes this a safe way to tell the two formats + apart without putting a marker in the envelope. + """ + try: + envelope = json.loads(raw) + except (UnicodeDecodeError, json.JSONDecodeError): + return None + return envelope if isinstance(envelope, dict) and "kid" in envelope else None -# --- Step 3: AES-CBC decryption --- def decrypt(encoded_payload: str, key: bytes | None = None) -> str: - """Decrypt Base64-encoded ciphertext using AES-CBC with PKCS7 padding. Returns the original plaintext. + """Decrypt a payload produced by :func:`encrypt`. + + Also accepts the pre-GCM AES-CBC format while the estate is being upgraded — + see the module docstring's shim note. Args: - encoded_payload (str): Base64-encoded payload where the first 16 bytes are the IV and the - remaining bytes are the ciphertext. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + encoded_payload (str): The base64-encoded envelope. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by the envelope's ``kid``. Returns: str: The decrypted plaintext. + + Raises: + cryptography.exceptions.InvalidTag: The payload failed authentication + (tampered ciphertext, wrong key, or altered ``kid``). + KeyError: The envelope's ``kid`` is not in the keyring. + ValueError: The envelope is malformed, an unsupported version, or a legacy + CBC payload arrived while ``AES_ACCEPT_LEGACY_CBC`` is disabled. """ - if key is None: - key = get_aes_key() + raw = base64.b64decode(encoded_payload) - encrypted_data = base64.b64decode(encoded_payload) - iv = encrypted_data[:16] - ciphertext = encrypted_data[16:] + envelope = _parse_envelope(raw) + if envelope is None: + return _decrypt_legacy_cbc(raw, key) # SHIM — delete after roll-out - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - decryptor = cipher.decryptor() - padded_plaintext = decryptor.update(ciphertext) + decryptor.finalize() + if envelope.get("v") != _VERSION: + raise ValueError(f"Unsupported payload version: {envelope.get('v')!r}") - # Unpad to recover original plaintext - unpadder = padding.PKCS7(128).unpadder() - plaintext = unpadder.update(padded_plaintext) + unpadder.finalize() - return plaintext.decode() + kid = envelope["kid"] + if key is None: + key = _keyring().get(kid) + if key is None: + raise KeyError(f"No key registered for kid {kid!r}") + + nonce = base64.b64decode(envelope["iv"]) + ciphertext = base64.b64decode(envelope["ct"]) + return AESGCM(key).decrypt(nonce, ciphertext, _aad(kid)).decode() diff --git a/trust/imaging-api/tests/utils/test_encryption.py b/trust/imaging-api/tests/utils/test_encryption.py index a1543f893..56bc6c8b9 100644 --- a/trust/imaging-api/tests/utils/test_encryption.py +++ b/trust/imaging-api/tests/utils/test_encryption.py @@ -11,60 +11,120 @@ # import base64 -from unittest.mock import MagicMock, patch +import json import pytest +from cryptography.exceptions import InvalidTag -from imaging_api.utils.encryption import decrypt, encrypt, get_aes_key - - -class TestGetAesKey: - def test_valid_key(self): - key = get_aes_key() - assert isinstance(key, bytes) - assert len(key) in (16, 24, 32) - - @patch("imaging_api.utils.encryption.get_settings") - def test_missing_key_raises_value_error(self, mock_settings): - mock_settings.return_value = MagicMock(AES_KEY_BASE64="") - with pytest.raises(ValueError, match="AES key not found"): - get_aes_key() - - @patch("imaging_api.utils.encryption.get_settings") - def test_invalid_key_length_raises_value_error(self, mock_settings): - # 10 bytes → invalid AES key length - mock_settings.return_value = MagicMock(AES_KEY_BASE64=base64.b64encode(b"x" * 10).decode()) - with pytest.raises(ValueError, match="Invalid AES key length"): - get_aes_key() - - -class TestEncryptDecrypt: - def test_roundtrip(self): - plaintext = "hello-project-id-12345" - key = get_aes_key() - encrypted = encrypt(plaintext, key) - decrypted = decrypt(encrypted, key) - assert decrypted == plaintext - - def test_encrypt_produces_different_ciphertexts(self): - plaintext = "same-text" - key = get_aes_key() - c1 = encrypt(plaintext, key) - c2 = encrypt(plaintext, key) - assert c1 != c2 # random IV → different ciphertexts - - def test_encrypt_uses_default_key(self): - plaintext = "auto-key-test" - encrypted = encrypt(plaintext) - decrypted = decrypt(encrypted) - assert decrypted == plaintext - - def test_roundtrip_with_unicode(self): - plaintext = "patient-name-日本��" - key = get_aes_key() - assert decrypt(encrypt(plaintext, key), key) == plaintext - - def test_roundtrip_with_long_text(self): - plaintext = "a" * 1000 - key = get_aes_key() - assert decrypt(encrypt(plaintext, key), key) == plaintext +from imaging_api.utils import encryption +from imaging_api.utils.encryption import SHARED_KID, _reset_caches, decrypt, encrypt + +RAW_KEY_BYTES = b"ThisIsExactly32BytesLongKey!!!!1" +ENCODED_KEY = base64.b64encode(RAW_KEY_BYTES).decode() +TRUST_KEY_B64 = base64.b64encode(b"AnotherExactly32ByteAESKey!!!!!2").decode() + + +@pytest.fixture(autouse=True) +def _shared_key_settings(monkeypatch): + monkeypatch.setattr(encryption, "get_settings", lambda: type("S", (), {"AES_KEY_BASE64": ENCODED_KEY})()) + for var in ("AES_TRUST_KEYS", "TRUST_AES_KID", "TRUST_AES_KEY_BASE64"): + monkeypatch.delenv(var, raising=False) + _reset_caches() + yield + _reset_caches() + + +def _envelope(payload: str) -> dict: + return json.loads(base64.b64decode(payload)) + + +def _reseal(envelope: dict) -> str: + return base64.b64encode(json.dumps(envelope).encode()).decode() + + +def test_roundtrip_via_keyring(): + assert decrypt(encrypt("hello")) == "hello" + + +def test_roundtrip_explicit_key(): + assert decrypt(encrypt("secret", key=RAW_KEY_BYTES), key=RAW_KEY_BYTES) == "secret" + + +def test_default_kid_is_shared(): + assert _envelope(encrypt("x"))["kid"] == SHARED_KID + + +def test_tampered_ciphertext_fails_closed(): + envelope = _envelope(encrypt("do not tamper")) + raw = bytearray(base64.b64decode(envelope["ct"])) + raw[0] ^= 0x01 + envelope["ct"] = base64.b64encode(bytes(raw)).decode() + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope)) + + +def test_kid_swap_fails_closed(): + envelope = _envelope(encrypt("bound", key=RAW_KEY_BYTES, kid="kid-a")) + envelope["kid"] = "kid-b" + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope), key=RAW_KEY_BYTES) + + +def test_unsupported_version_raises(): + envelope = _envelope(encrypt("x")) + envelope["v"] = 99 + with pytest.raises(ValueError, match="Unsupported payload version"): + decrypt(_reseal(envelope)) + + +def test_own_trust_key_is_used_by_default(monkeypatch): + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + payload = encrypt("per-trust") + assert _envelope(payload)["kid"] == "trust-GSTT" + assert decrypt(payload) == "per-trust" + + +def test_decrypts_payload_addressed_to_this_trust(monkeypatch): + """Hub encrypts to this trust's kid; the trust decrypts it with its own key.""" + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + from_hub = encrypt("task payload", key=base64.b64decode(TRUST_KEY_B64), kid="trust-GSTT") + assert decrypt(from_hub) == "task payload" + + +# --- temporary AES-CBC compatibility shim (delete with the shim) --------------- + + +def _legacy_cbc_payload(plaintext: str, key: bytes = RAW_KEY_BYTES) -> str: + """Produce a payload in the pre-GCM format, exactly as an un-upgraded peer would.""" + import os + + from cryptography.hazmat.primitives import padding + from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes + + iv = os.urandom(16) + padder = padding.PKCS7(128).padder() + padded = padder.update(plaintext.encode()) + padder.finalize() + encryptor = Cipher(algorithms.AES(key), modes.CBC(iv)).encryptor() + return base64.b64encode(iv + encryptor.update(padded) + encryptor.finalize()).decode() + + +def test_legacy_cbc_from_unupgraded_hub_still_decrypts(): + assert decrypt(_legacy_cbc_payload("task payload")) == "task payload" + + +def test_legacy_cbc_rejected_once_disabled(monkeypatch): + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + with pytest.raises(ValueError, match="Legacy AES-CBC payload rejected"): + decrypt(_legacy_cbc_payload("x")) + + +def test_gcm_still_works_when_legacy_disabled(monkeypatch): + """Turning the shim off must not affect the real format.""" + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + assert decrypt(encrypt("still fine")) == "still fine" diff --git a/trust/trust-api/tests/utils/test_encryption.py b/trust/trust-api/tests/utils/test_encryption.py index 6ce0309c3..45df60736 100644 --- a/trust/trust-api/tests/utils/test_encryption.py +++ b/trust/trust-api/tests/utils/test_encryption.py @@ -10,94 +10,121 @@ # limitations under the License. # -"""Tests for AES-CBC decryption utility.""" - import base64 -import os -from unittest.mock import patch +import json -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import padding -from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes +import pytest +from cryptography.exceptions import InvalidTag -import trust_api.utils.encryption as encryption_module -from trust_api.utils.encryption import decrypt, get_aes_key +from trust_api.utils import encryption +from trust_api.utils.encryption import SHARED_KID, _reset_caches, decrypt, encrypt +RAW_KEY_BYTES = b"ThisIsExactly32BytesLongKey!!!!1" +ENCODED_KEY = base64.b64encode(RAW_KEY_BYTES).decode() +TRUST_KEY_B64 = base64.b64encode(b"AnotherExactly32ByteAESKey!!!!!2").decode() + + +@pytest.fixture(autouse=True) +def _shared_key_settings(monkeypatch): + monkeypatch.setattr(encryption, "get_settings", lambda: type("S", (), {"AES_KEY_BASE64": ENCODED_KEY})()) + for var in ("AES_TRUST_KEYS", "TRUST_AES_KID", "TRUST_AES_KEY_BASE64"): + monkeypatch.delenv(var, raising=False) + _reset_caches() + yield + _reset_caches() + + +def _envelope(payload: str) -> dict: + return json.loads(base64.b64decode(payload)) + + +def _reseal(envelope: dict) -> str: + return base64.b64encode(json.dumps(envelope).encode()).decode() -def _encrypt(plaintext: str, key: bytes) -> str: - """Encrypt plaintext using AES-CBC with PKCS7 padding (test helper).""" - iv = os.urandom(16) - padder = padding.PKCS7(128).padder() - padded = padder.update(plaintext.encode()) + padder.finalize() - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - encryptor = cipher.encryptor() - ciphertext = encryptor.update(padded) + encryptor.finalize() - return base64.b64encode(iv + ciphertext).decode() +def test_roundtrip_via_keyring(): + assert decrypt(encrypt("hello")) == "hello" -class TestGetAesKey: - def setup_method(self): - encryption_module._aes_key_cache = None - def test_returns_decoded_bytes(self): - raw_key = os.urandom(32) - b64_key = base64.b64encode(raw_key).decode() - mock_settings = type("S", (), {"AES_KEY_BASE64": b64_key})() +def test_roundtrip_explicit_key(): + assert decrypt(encrypt("secret", key=RAW_KEY_BYTES), key=RAW_KEY_BYTES) == "secret" - with patch("trust_api.utils.encryption.get_settings", return_value=mock_settings): - result = get_aes_key() - assert result == raw_key +def test_default_kid_is_shared(): + assert _envelope(encrypt("x"))["kid"] == SHARED_KID - def test_caches_after_first_call(self): - raw_key = os.urandom(32) - b64_key = base64.b64encode(raw_key).decode() - mock_settings = type("S", (), {"AES_KEY_BASE64": b64_key})() - with patch("trust_api.utils.encryption.get_settings", return_value=mock_settings) as mock_get: - first = get_aes_key() - second = get_aes_key() +def test_tampered_ciphertext_fails_closed(): + envelope = _envelope(encrypt("do not tamper")) + raw = bytearray(base64.b64decode(envelope["ct"])) + raw[0] ^= 0x01 + envelope["ct"] = base64.b64encode(bytes(raw)).decode() + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope)) - assert first is second - mock_get.assert_called_once() +def test_kid_swap_fails_closed(): + envelope = _envelope(encrypt("bound", key=RAW_KEY_BYTES, kid="kid-a")) + envelope["kid"] = "kid-b" + with pytest.raises(InvalidTag): + decrypt(_reseal(envelope), key=RAW_KEY_BYTES) -class TestDecrypt: - def test_decrypts_valid_payload(self): - key = os.urandom(32) - plaintext = "hello, trust!" - encrypted = _encrypt(plaintext, key) - result = decrypt(encrypted, key=key) +def test_unsupported_version_raises(): + envelope = _envelope(encrypt("x")) + envelope["v"] = 99 + with pytest.raises(ValueError, match="Unsupported payload version"): + decrypt(_reseal(envelope)) - assert result == plaintext - def test_decrypts_empty_string(self): - key = os.urandom(32) - encrypted = _encrypt("", key) +def test_own_trust_key_is_used_by_default(monkeypatch): + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + payload = encrypt("per-trust") + assert _envelope(payload)["kid"] == "trust-GSTT" + assert decrypt(payload) == "per-trust" - assert decrypt(encrypted, key=key) == "" - def test_decrypts_unicode_content(self): - key = os.urandom(32) - plaintext = '{"patient_id": 42, "name": "Test"}' - encrypted = _encrypt(plaintext, key) +def test_decrypts_payload_addressed_to_this_trust(monkeypatch): + """Hub encrypts to this trust's kid; the trust decrypts it with its own key.""" + monkeypatch.setenv("TRUST_AES_KID", "trust-GSTT") + monkeypatch.setenv("TRUST_AES_KEY_BASE64", TRUST_KEY_B64) + _reset_caches() + from_hub = encrypt("task payload", key=base64.b64decode(TRUST_KEY_B64), kid="trust-GSTT") + assert decrypt(from_hub) == "task payload" + + +# --- temporary AES-CBC compatibility shim (delete with the shim) --------------- + + +def _legacy_cbc_payload(plaintext: str, key: bytes = RAW_KEY_BYTES) -> str: + """Produce a payload in the pre-GCM format, exactly as an un-upgraded peer would.""" + import os + + from cryptography.hazmat.primitives import padding + from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes + + iv = os.urandom(16) + padder = padding.PKCS7(128).padder() + padded = padder.update(plaintext.encode()) + padder.finalize() + encryptor = Cipher(algorithms.AES(key), modes.CBC(iv)).encryptor() + return base64.b64encode(iv + encryptor.update(padded) + encryptor.finalize()).decode() - assert decrypt(encrypted, key=key) == plaintext - def test_uses_get_aes_key_when_no_key_provided(self): - key = os.urandom(32) - plaintext = "auto-key test" - encrypted = _encrypt(plaintext, key) +def test_legacy_cbc_from_unupgraded_hub_still_decrypts(): + assert decrypt(_legacy_cbc_payload("task payload")) == "task payload" - with patch("trust_api.utils.encryption.get_aes_key", return_value=key): - result = decrypt(encrypted) - assert result == plaintext +def test_legacy_cbc_rejected_once_disabled(monkeypatch): + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + with pytest.raises(ValueError, match="Legacy AES-CBC payload rejected"): + decrypt(_legacy_cbc_payload("x")) - def test_decrypts_long_payload(self): - key = os.urandom(32) - plaintext = "x" * 10000 - encrypted = _encrypt(plaintext, key) - assert decrypt(encrypted, key=key) == plaintext +def test_gcm_still_works_when_legacy_disabled(monkeypatch): + """Turning the shim off must not affect the real format.""" + monkeypatch.setenv("AES_ACCEPT_LEGACY_CBC", "false") + _reset_caches() + assert decrypt(encrypt("still fine")) == "still fine" diff --git a/trust/trust-api/trust_api/utils/encryption.py b/trust/trust-api/trust_api/utils/encryption.py index 18bb32774..9726b40f3 100644 --- a/trust/trust-api/trust_api/utils/encryption.py +++ b/trust/trust-api/trust_api/utils/encryption.py @@ -10,58 +10,224 @@ # limitations under the License. # -"""AES-CBC decryption for task payloads received from the central hub.""" +"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). + +AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC +scheme, which had no authentication: CBC ciphertexts were malleable, so a +tampered payload decrypted to attacker-influenced plaintext instead of failing. +GCM authenticates the ciphertext and the envelope's ``kid``, so any tampering +raises ``InvalidTag``. + +Every ciphertext carries a **key id** (``kid``) and every service resolves keys +through a small keyring. That is what makes key rotation and **per-trust keys** +possible: receivers can hold several keys at once, so a key can be introduced or +retired without every participant changing over at the same moment. + +Wire format — base64 of ``{"v": 1, "kid": ..., "iv": ..., "ct": ...}``, where +``iv`` is a 12-byte GCM nonce and ``ct`` is ciphertext‖tag. ``v`` is a format +discriminator so the envelope itself can be changed later without ambiguity; +the algorithm is fixed (AES-256-GCM) rather than negotiated, which removes any +algorithm-confusion surface. + +Keys (all optional except the shared key): + +- ``AES_KEY_BASE64`` — the shared key, always registered under + :data:`SHARED_KID`. Used when nothing else is configured. +- ``TRUST_AES_KID`` + ``TRUST_AES_KEY_BASE64`` — a trust's own key. When set, + that key is used for anything this service encrypts. +- ``AES_TRUST_KEYS`` — hub-side JSON map ``{kid: base64_key}`` of per-trust keys, + so the hub can encrypt to (and decrypt from) a specific trust by ``kid``. + +.. rubric:: Temporary compatibility shim — DELETE AFTER ROLL-OUT + +:func:`decrypt` also accepts the previous **AES-CBC** payload format, so a hub and +a trust running different builds can still talk to each other while every host is +upgraded (on-prem trusts cannot all be restarted at once). Nothing ever *writes* +CBC — :func:`encrypt` is GCM-only — so no new unauthenticated ciphertext is +created, and no payload is stored at rest in either format. + +Once every hub and trust runs this build: set ``AES_ACCEPT_LEGACY_CBC=false`` to +verify nothing still sends CBC, then delete :func:`_decrypt_legacy_cbc`, +:func:`_accept_legacy_cbc`, the fallback branch in :func:`decrypt`, and the CBC +imports below. +""" import base64 +import json +import os -from cryptography.hazmat.backends import default_backend -from cryptography.hazmat.primitives import padding -from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes +from cryptography.hazmat.primitives import padding # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes # legacy-CBC shim +from cryptography.hazmat.primitives.ciphers.aead import AESGCM from trust_api.config import get_settings -_aes_key_cache: bytes | None = None +#: Key-id under which the shared ``AES_KEY_BASE64`` is registered. +SHARED_KID = "shared" +_VERSION = 1 +_NONCE_BYTES = 12 # NIST SP 800-38D recommended nonce length for AES-GCM -def get_aes_key() -> bytes: - """Retrieve the AES key from the environment and return it as bytes. +_keyring_cache: dict[str, bytes] | None = None - Cached after first call — the key does not change during the lifetime of a process. + +def _shared_key() -> bytes: + """Return the shared AES key from settings (``AES_KEY_BASE64``).""" + return base64.b64decode(get_settings().AES_KEY_BASE64) + + +def _keyring() -> dict[str, bytes]: + """Return the ``kid -> key`` keyring: the shared key plus any configured extras. + + Cached — keys do not change during a process lifetime. + """ + global _keyring_cache # noqa: PLW0603 + if _keyring_cache is not None: + return _keyring_cache + + ring: dict[str, bytes] = {SHARED_KID: _shared_key()} + + own_kid, own_key = os.environ.get("TRUST_AES_KID"), os.environ.get("TRUST_AES_KEY_BASE64") + if own_kid and own_key: + ring[own_kid] = base64.b64decode(own_key) + + trust_keys = os.environ.get("AES_TRUST_KEYS") + if trust_keys: + ring.update({kid: base64.b64decode(key) for kid, key in json.loads(trust_keys).items()}) + + _keyring_cache = ring + return ring + + +def _default_kid() -> str: + """Return the kid used when the caller does not name one. + + A service configured with its own key (``TRUST_AES_KID``) uses it; otherwise + the shared key. + """ + return os.environ.get("TRUST_AES_KID") or SHARED_KID + + +def _reset_caches() -> None: + """Clear the keyring cache. Test-only helper.""" + global _keyring_cache # noqa: PLW0603 + _keyring_cache = None + + +def _aad(kid: str) -> bytes: + """Associated data bound into the GCM tag, so the ``kid`` cannot be swapped.""" + return f"FLIP|v{_VERSION}|{kid}".encode() + + +def encrypt(plaintext: str, key: bytes | None = None, kid: str | None = None) -> str: + """Encrypt ``plaintext`` with AES-256-GCM. + + Args: + plaintext (str): The text to encrypt. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by ``kid``. + kid (str | None): Key id. Selects the key when ``key`` is ``None`` + (default: :func:`_default_kid`), and is recorded in the envelope. Returns: - bytes: The decoded AES key. + str: Base64-encoded envelope. + + Raises: + KeyError: ``key`` is ``None`` and ``kid`` is not in the keyring. + ValueError: The key is not a valid AES key length. """ - global _aes_key_cache # noqa: PLW0603 - if _aes_key_cache is not None: - return _aes_key_cache + if key is None: + kid = kid or _default_kid() + key = _keyring()[kid] + else: + kid = kid or SHARED_KID + + nonce = os.urandom(_NONCE_BYTES) + ciphertext = AESGCM(key).encrypt(nonce, plaintext.encode(), _aad(kid)) + envelope = { + "v": _VERSION, + "kid": kid, + "iv": base64.b64encode(nonce).decode(), + "ct": base64.b64encode(ciphertext).decode(), + } + return base64.b64encode(json.dumps(envelope).encode()).decode() + + +def _accept_legacy_cbc() -> bool: + """Whether pre-GCM AES-CBC payloads are still accepted. SHIM — delete after roll-out.""" + return os.environ.get("AES_ACCEPT_LEGACY_CBC", "true").strip().lower() not in ("false", "0", "no") + + +def _decrypt_legacy_cbc(raw: bytes, key: bytes | None) -> str: + """Decrypt a pre-GCM ``iv[16] ‖ ciphertext`` AES-CBC payload. - _aes_key_cache = base64.b64decode(get_settings().AES_KEY_BASE64) - return _aes_key_cache + SHIM — delete after roll-out. Unauthenticated by construction: this is only + here so a not-yet-upgraded peer can still be understood during the upgrade. + + Raises: + ValueError: When ``AES_ACCEPT_LEGACY_CBC`` is disabled. + """ + if not _accept_legacy_cbc(): + raise ValueError("Legacy AES-CBC payload rejected (AES_ACCEPT_LEGACY_CBC is disabled)") + if key is None: + key = _keyring()[SHARED_KID] + + decryptor = Cipher(algorithms.AES(key), modes.CBC(raw[:16])).decryptor() + padded_plaintext = decryptor.update(raw[16:]) + decryptor.finalize() + unpadder = padding.PKCS7(128).unpadder() + return (unpadder.update(padded_plaintext) + unpadder.finalize()).decode() + + +def _parse_envelope(raw: bytes) -> dict | None: + """Return the decoded GCM envelope, or ``None`` if these are not envelope bytes. + + CBC ciphertext is indistinguishable from random, so it never parses as a JSON + object carrying ``kid`` — which makes this a safe way to tell the two formats + apart without putting a marker in the envelope. + """ + try: + envelope = json.loads(raw) + except (UnicodeDecodeError, json.JSONDecodeError): + return None + return envelope if isinstance(envelope, dict) and "kid" in envelope else None def decrypt(encoded_payload: str, key: bytes | None = None) -> str: - """Decrypt Base64-encoded ciphertext using AES-CBC with PKCS7 padding. + """Decrypt a payload produced by :func:`encrypt`. + + Also accepts the pre-GCM AES-CBC format while the estate is being upgraded — + see the module docstring's shim note. Args: - encoded_payload (str): Base64-encoded payload where the first 16 bytes are the IV and the - remaining bytes are the ciphertext. - key (bytes | None): The AES key to use. If None, the shared AES key is retrieved via - :func:`get_aes_key`. + encoded_payload (str): The base64-encoded envelope. + key (bytes | None): Explicit key. If ``None``, resolved from the keyring + by the envelope's ``kid``. Returns: str: The decrypted plaintext. + + Raises: + cryptography.exceptions.InvalidTag: The payload failed authentication + (tampered ciphertext, wrong key, or altered ``kid``). + KeyError: The envelope's ``kid`` is not in the keyring. + ValueError: The envelope is malformed, an unsupported version, or a legacy + CBC payload arrived while ``AES_ACCEPT_LEGACY_CBC`` is disabled. """ - if key is None: - key = get_aes_key() + raw = base64.b64decode(encoded_payload) - encrypted_data = base64.b64decode(encoded_payload) - iv = encrypted_data[:16] - ciphertext = encrypted_data[16:] + envelope = _parse_envelope(raw) + if envelope is None: + return _decrypt_legacy_cbc(raw, key) # SHIM — delete after roll-out - cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=default_backend()) - decryptor = cipher.decryptor() - padded_plaintext = decryptor.update(ciphertext) + decryptor.finalize() + if envelope.get("v") != _VERSION: + raise ValueError(f"Unsupported payload version: {envelope.get('v')!r}") - unpadder = padding.PKCS7(128).unpadder() - plaintext = unpadder.update(padded_plaintext) + unpadder.finalize() - return plaintext.decode() + kid = envelope["kid"] + if key is None: + key = _keyring().get(kid) + if key is None: + raise KeyError(f"No key registered for kid {kid!r}") + + nonce = base64.b64decode(envelope["iv"]) + ciphertext = base64.b64decode(envelope["ct"]) + return AESGCM(key).decrypt(nonce, ciphertext, _aad(kid)).decode() From 20e5eaf7bf4c5282e5bf3b345073991a4093c455 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 04:34:59 +0000 Subject: [PATCH 2/7] chore: drop the internal finding-ID scheme from source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- .../src/flip_api/cohort_services/submit_cohort_query.py | 2 +- flip-api/src/flip_api/scripts/generate_trust_key.py | 8 ++++---- flip-api/src/flip_api/scripts/register_trust.py | 2 +- .../flip_api/trusts_services/services/register_trust.py | 2 +- flip-api/src/flip_api/utils/encryption.py | 2 +- flip-api/tests/unit/utils/test_s3_client.py | 2 +- trust/data-access-api/data_access_api/utils/encryption.py | 2 +- trust/imaging-api/imaging_api/utils/encryption.py | 2 +- trust/trust-api/trust_api/utils/encryption.py | 2 +- 9 files changed, 12 insertions(+), 12 deletions(-) diff --git a/flip-api/src/flip_api/cohort_services/submit_cohort_query.py b/flip-api/src/flip_api/cohort_services/submit_cohort_query.py index 3e1a196b6..faafbd901 100644 --- a/flip-api/src/flip_api/cohort_services/submit_cohort_query.py +++ b/flip-api/src/flip_api/cohort_services/submit_cohort_query.py @@ -149,7 +149,7 @@ def submit_cohort_query( # Queue a task for each trust (instead of direct HTTP calls). The # project_id is encrypted per trust under that trust's key-id — a single # ciphertext reused across trusts could not be bound to a per-trust key - # (FLIP-PT-004, docs/aes-payload-keys.md). Trusts without their own key + # (see docs/aes-payload-keys.md). Trusts without their own key # fall back to the shared kid. for trust in trusts: try: diff --git a/flip-api/src/flip_api/scripts/generate_trust_key.py b/flip-api/src/flip_api/scripts/generate_trust_key.py index fcbd53867..9f4a8d6ec 100644 --- a/flip-api/src/flip_api/scripts/generate_trust_key.py +++ b/flip-api/src/flip_api/scripts/generate_trust_key.py @@ -36,10 +36,10 @@ def generate_trust_key() -> tuple[str, str]: def generate_aes_key() -> str: """Generate a fresh 256-bit AES key, base64-encoded. - Used by ``register_trust`` to mint a per-trust payload-encryption key - (FLIP-PT-004 step 2). Unlike the API key, this is a symmetric key the hub - must also hold to encrypt to / decrypt from the trust, so — like - ``AES_KEY_BASE64`` — only the plaintext is meaningful (there is no hash form). + Used by ``register_trust`` to mint a per-trust payload-encryption key. + Unlike the API key, this is a symmetric key the hub must also hold to + encrypt to / decrypt from the trust, so — like ``AES_KEY_BASE64`` — only + the plaintext is meaningful (there is no hash form). Returns: str: Base64-encoded 32-byte (AES-256) key. diff --git a/flip-api/src/flip_api/scripts/register_trust.py b/flip-api/src/flip_api/scripts/register_trust.py index b495628ec..7a5a7aaeb 100644 --- a/flip-api/src/flip_api/scripts/register_trust.py +++ b/flip-api/src/flip_api/scripts/register_trust.py @@ -175,7 +175,7 @@ def register_one_trust( "trust_name": kit.trust.name, "trust_api_key": kit.trust_api_key, "trust_internal_service_key": kit.trust_internal_service_key, - # Per-trust payload-encryption key + kid (FLIP-PT-004). Belongs in the + # Per-trust payload-encryption key + kid. Belongs in the # trust's kit as TRUST_AES_KEY_BASE64 / TRUST_AES_KID; inert until the # hub also holds it (docs/aes-payload-keys.md), so until then payloads # keep using the shared key. diff --git a/flip-api/src/flip_api/trusts_services/services/register_trust.py b/flip-api/src/flip_api/trusts_services/services/register_trust.py index a5343bd17..037d770b1 100644 --- a/flip-api/src/flip_api/trusts_services/services/register_trust.py +++ b/flip-api/src/flip_api/trusts_services/services/register_trust.py @@ -70,7 +70,7 @@ class RegisteredTrust: internal-service key is never persisted hub-side. ``trust_aes_key`` / ``trust_aes_kid`` are the per-trust payload-encryption key - and its key-id (FLIP-PT-004). This is a symmetric key the hub must also hold to + and its key-id. This is a symmetric key the hub must also hold to encrypt to / decrypt from this trust, so — unlike the api key — it cannot be reduced to a hash. Until it is provisioned on both sides (``docs/aes-payload-keys.md``) payloads keep using the shared key. diff --git a/flip-api/src/flip_api/utils/encryption.py b/flip-api/src/flip_api/utils/encryption.py index 7d3d334ce..8943640e6 100644 --- a/flip-api/src/flip_api/utils/encryption.py +++ b/flip-api/src/flip_api/utils/encryption.py @@ -10,7 +10,7 @@ # limitations under the License. # -"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). +"""Authenticated encryption for cross-trust payloads. AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC scheme, which had no authentication: CBC ciphertexts were malleable, so a diff --git a/flip-api/tests/unit/utils/test_s3_client.py b/flip-api/tests/unit/utils/test_s3_client.py index fe47420cc..99b2ea366 100644 --- a/flip-api/tests/unit/utils/test_s3_client.py +++ b/flip-api/tests/unit/utils/test_s3_client.py @@ -24,7 +24,7 @@ ``tests/unit/file_services/test_presigned_url_for_upload.py``, ``tests/unit/file_services/test_download_file.py``, and ``tests/unit/file_services/test_retrieve_federated_results.py``, this -module forms the policy retest required by the FLIP-PT review brief: +module forms the policy retest required by the external security review: no log line may contain ``X-Amz-Signature=``, ``X-Amz-Credential=``, or any ``s3.amazonaws.com/...?...`` URL. """ diff --git a/trust/data-access-api/data_access_api/utils/encryption.py b/trust/data-access-api/data_access_api/utils/encryption.py index 2cf2b0771..b6a209653 100644 --- a/trust/data-access-api/data_access_api/utils/encryption.py +++ b/trust/data-access-api/data_access_api/utils/encryption.py @@ -10,7 +10,7 @@ # limitations under the License. # -"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). +"""Authenticated encryption for cross-trust payloads. AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC scheme, which had no authentication: CBC ciphertexts were malleable, so a diff --git a/trust/imaging-api/imaging_api/utils/encryption.py b/trust/imaging-api/imaging_api/utils/encryption.py index da25a3d80..6232fcda8 100644 --- a/trust/imaging-api/imaging_api/utils/encryption.py +++ b/trust/imaging-api/imaging_api/utils/encryption.py @@ -10,7 +10,7 @@ # limitations under the License. # -"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). +"""Authenticated encryption for cross-trust payloads. AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC scheme, which had no authentication: CBC ciphertexts were malleable, so a diff --git a/trust/trust-api/trust_api/utils/encryption.py b/trust/trust-api/trust_api/utils/encryption.py index 9726b40f3..1ee25f29c 100644 --- a/trust/trust-api/trust_api/utils/encryption.py +++ b/trust/trust-api/trust_api/utils/encryption.py @@ -10,7 +10,7 @@ # limitations under the License. # -"""Authenticated encryption for cross-trust payloads (FLIP-PT-004). +"""Authenticated encryption for cross-trust payloads. AES-256-GCM (AEAD) in a small key-id'd envelope. Replaces the previous AES-CBC scheme, which had no authentication: CBC ciphertexts were malleable, so a From 609a7eb51d958879fba710e5d17af79d5dc2c9e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 04:35:00 +0000 Subject: [PATCH 3/7] fix(test): update admin_create_trust for the two new RegisteredTrust fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- .../unit/trusts_services/test_admin_create_trust.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/flip-api/tests/unit/trusts_services/test_admin_create_trust.py b/flip-api/tests/unit/trusts_services/test_admin_create_trust.py index 430c0ba4c..3466ab22c 100644 --- a/flip-api/tests/unit/trusts_services/test_admin_create_trust.py +++ b/flip-api/tests/unit/trusts_services/test_admin_create_trust.py @@ -51,6 +51,10 @@ def _registered(name: str = "GSTT", slot_name: str = "Trust_007", slot_number: i fl_kit_slot=FLKitSlot(slot_name=slot_name, slot_number=slot_number), trust_api_key="plain-api", trust_internal_service_key="plain-internal", + # Minted by register_trust alongside the other keys. Not surfaced by this + # endpoint's response model — see the note in test_admin_create_trust_returns_registered_kit. + trust_aes_key="plain-aes", # pragma: allowlist secret + trust_aes_kid="trust-gstt-1", ) @@ -74,6 +78,13 @@ def test_admin_create_trust_returns_registered_kit(mock_register, mock_perms, ad assert result.fl_kit_slot_number == 7 assert result.trust_api_key == "plain-api" assert result.trust_internal_service_key == "plain-internal" + # register_trust mints a per-trust AES key, but ICreatedTrust deliberately does not + # carry it: this endpoint backs the admin UI, which has no kit file to write it into. + # A trust registered here therefore keeps encrypting under the shared kid until an + # operator provisions its own key on both sides (docs/aes-payload-keys.md). Pinned so + # the key cannot start leaking into an HTTP response without this assertion failing. + assert not hasattr(result, "trust_aes_key") + assert not hasattr(result, "trust_aes_kid") db.rollback.assert_not_called() mock_perms.assert_called_once() From 8172c0985c872541c68ecbfef1cde2f446747882 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:12:18 +0000 Subject: [PATCH 4/7] fix: deliver the per-trust AES key to the trust on both registration paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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=` writes them into `trust/.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/.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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- docs/aes-payload-keys.md | 13 ++++++-- .../src/flip_api/domain/interfaces/trust.py | 11 +++++++ .../trusts_services/admin_create_trust.py | 2 ++ .../test_admin_create_trust.py | 14 +++----- flip-ui/src/partials/trusts/TrustKitModal.vue | 10 ++++-- .../trusts/__tests__/TrustKitModal.spec.ts | 10 +++++- flip-ui/src/services/admin-trusts-service.ts | 5 +++ scripts/tests/test_trust_kit_lib.py | 32 +++++++++++++++++-- scripts/trust_kit_lib.py | 18 ++++++++--- 9 files changed, 91 insertions(+), 24 deletions(-) diff --git a/docs/aes-payload-keys.md b/docs/aes-payload-keys.md index 2e4ea9e1e..d1b4e8792 100644 --- a/docs/aes-payload-keys.md +++ b/docs/aes-payload-keys.md @@ -42,9 +42,16 @@ compromised trust yields the key for the whole federation. `kid_for_trust()` then selects that key automatically; trusts without one keep using the shared key, so trusts can be moved over one at a time. -> The kit distributor (`scripts/distribute_trust_kits.py` / `trust_kit_lib.py`) -> does not yet write these two variables into the kit file — that wiring is the -> remaining task before per-trust keys can be switched on. +Both registration paths now deliver step 2 automatically: `register-trust +KIT=` writes the two variables into the kit file (they are credential keys +in `scripts/trust_kit_lib.py`, so they are written once on a new registration and +never clobbered on the idempotent skip path), and the admin UI's kit modal +includes them in its copy-all block. + +> Step 1 is still manual. The hub does not persist the AES key anywhere — a +> symmetric key cannot be reduced to a hash the way the api key can — so the value +> shown at registration is the only copy. Record it into `AES_TRUST_KEYS` at the +> same time, or the trust has to be re-registered to obtain another. ## Rolling this out to an existing deployment diff --git a/flip-api/src/flip_api/domain/interfaces/trust.py b/flip-api/src/flip_api/domain/interfaces/trust.py index 9e43f5290..699ec96d7 100644 --- a/flip-api/src/flip_api/domain/interfaces/trust.py +++ b/flip-api/src/flip_api/domain/interfaces/trust.py @@ -56,6 +56,15 @@ class ICreatedTrust(BaseModel): exactly once. The hub only stores the SHA-256 of the api key; the internal service key is not persisted (only used by trust-internal services). + `trust_aes_key` / `trust_aes_kid` are the trust's payload-encryption key and its + key-id. Returned exactly once and stored nowhere hub-side — unlike the api key a + symmetric key cannot be reduced to a hash, so an admin who does not record it here + has to re-register the trust to get another. Carried for the same reason as the + other credentials: this response is the source for the Kit-credentials block of + ``trust/.env..``, and the key is one of those credentials. It stays + inert until an operator also adds it to the hub's ``AES_TRUST_KEYS`` map + (``docs/aes-payload-keys.md``); until then payloads use the shared key. + `fl_kit_slot` is the pre-provisioned FL participant identity assigned to this trust from the shared pool. The operator's containers mount the matching ``workspace/net-N/services//`` provisioned kit dirs; this is the @@ -69,6 +78,8 @@ class ICreatedTrust(BaseModel): created_at: datetime | None = None trust_api_key: str trust_internal_service_key: str + trust_aes_key: str + trust_aes_kid: str fl_kit_slot: str fl_kit_slot_number: int diff --git a/flip-api/src/flip_api/trusts_services/admin_create_trust.py b/flip-api/src/flip_api/trusts_services/admin_create_trust.py index c5b0a78ed..8523e6e7c 100644 --- a/flip-api/src/flip_api/trusts_services/admin_create_trust.py +++ b/flip-api/src/flip_api/trusts_services/admin_create_trust.py @@ -103,6 +103,8 @@ def admin_create_trust( created_at=registered.trust.created_at, trust_api_key=registered.trust_api_key, trust_internal_service_key=registered.trust_internal_service_key, + trust_aes_key=registered.trust_aes_key, + trust_aes_kid=registered.trust_aes_kid, fl_kit_slot=registered.fl_kit_slot.slot_name, fl_kit_slot_number=registered.fl_kit_slot.slot_number, ) diff --git a/flip-api/tests/unit/trusts_services/test_admin_create_trust.py b/flip-api/tests/unit/trusts_services/test_admin_create_trust.py index 3466ab22c..6685bb101 100644 --- a/flip-api/tests/unit/trusts_services/test_admin_create_trust.py +++ b/flip-api/tests/unit/trusts_services/test_admin_create_trust.py @@ -51,8 +51,6 @@ def _registered(name: str = "GSTT", slot_name: str = "Trust_007", slot_number: i fl_kit_slot=FLKitSlot(slot_name=slot_name, slot_number=slot_number), trust_api_key="plain-api", trust_internal_service_key="plain-internal", - # Minted by register_trust alongside the other keys. Not surfaced by this - # endpoint's response model — see the note in test_admin_create_trust_returns_registered_kit. trust_aes_key="plain-aes", # pragma: allowlist secret trust_aes_kid="trust-gstt-1", ) @@ -78,13 +76,11 @@ def test_admin_create_trust_returns_registered_kit(mock_register, mock_perms, ad assert result.fl_kit_slot_number == 7 assert result.trust_api_key == "plain-api" assert result.trust_internal_service_key == "plain-internal" - # register_trust mints a per-trust AES key, but ICreatedTrust deliberately does not - # carry it: this endpoint backs the admin UI, which has no kit file to write it into. - # A trust registered here therefore keeps encrypting under the shared kid until an - # operator provisions its own key on both sides (docs/aes-payload-keys.md). Pinned so - # the key cannot start leaking into an HTTP response without this assertion failing. - assert not hasattr(result, "trust_aes_key") - assert not hasattr(result, "trust_aes_kid") + # The AES key is minted per trust and stored nowhere hub-side, so this response is + # the only place it ever exists. Dropping it here would mint a key and discard it, + # leaving a UI-registered trust unable to use one without re-registering. + assert result.trust_aes_key == "plain-aes" + assert result.trust_aes_kid == "trust-gstt-1" db.rollback.assert_not_called() mock_perms.assert_called_once() diff --git a/flip-ui/src/partials/trusts/TrustKitModal.vue b/flip-ui/src/partials/trusts/TrustKitModal.vue index 13096134f..b72504d85 100644 --- a/flip-ui/src/partials/trusts/TrustKitModal.vue +++ b/flip-ui/src/partials/trusts/TrustKitModal.vue @@ -90,6 +90,8 @@

FL_KIT_SLOT is the FL identity clients register under; EXPECTED_TRUST_ID is an optional startup self-check. + TRUST_AES_KID stays inert until the same key is added to + the hub's AES_TRUST_KEYS map.

@@ -159,12 +161,14 @@ const close = () => { emit("closeModal"); }; -// All five managed Kit-credential lines as one paste-ready block, so the admin -// can copy the whole set into trust/.env.. with a single click -// (EXPECTED_TRUST_ID is the trust id; the hub only ever stores credential hashes). +// Every managed Kit-credential line as one paste-ready block, so the admin can copy +// the whole set into trust/.env.. with a single click (EXPECTED_TRUST_ID +// is the trust id; the hub only ever stores credential hashes). const allCredentialsBlock = computed(() => [ `TRUST_API_KEY=${props.trust?.trust_api_key ?? ""}`, `TRUST_INTERNAL_SERVICE_KEY=${props.trust?.trust_internal_service_key ?? ""}`, + `TRUST_AES_KEY_BASE64=${props.trust?.trust_aes_key ?? ""}`, + `TRUST_AES_KID=${props.trust?.trust_aes_kid ?? ""}`, `FL_KIT_SLOT=${props.trust?.fl_kit_slot ?? ""}`, `FL_KIT_SLOT_NUMBER=${props.trust?.fl_kit_slot_number ?? ""}`, `EXPECTED_TRUST_ID=${props.trust?.id ?? ""}` diff --git a/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts b/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts index 26e64abe2..a64d1612f 100644 --- a/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts +++ b/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts @@ -31,6 +31,8 @@ const trust = { created_at: null, trust_api_key: "api-key-abc", trust_internal_service_key: "internal-key-def", + trust_aes_key: "aes-key-ghi", + trust_aes_kid: "trust-uuid-123", fl_kit_slot: "Trust_1", fl_kit_slot_number: 1 }; @@ -69,6 +71,8 @@ describe("TrustKitModal — copy all credentials", () => { const text = block.text(); expect(text).toContain("TRUST_API_KEY=api-key-abc"); expect(text).toContain("TRUST_INTERNAL_SERVICE_KEY=internal-key-def"); + expect(text).toContain("TRUST_AES_KEY_BASE64=aes-key-ghi"); + expect(text).toContain("TRUST_AES_KID=trust-uuid-123"); expect(text).toContain("FL_KIT_SLOT=Trust_1"); expect(text).toContain("FL_KIT_SLOT_NUMBER=1"); expect(text).toContain("EXPECTED_TRUST_ID=trust-uuid-123"); @@ -88,7 +92,7 @@ describe("TrustKitModal — copy all credentials", () => { expect(wrapper.find("[data-test='copy-all-credentials-btn']").exists()).toBe(true); }); - it("copies all five credential lines with a single button click", async () => { + it("copies every credential line with a single button click", async () => { const wrapper = mountModal(); const btn = wrapper.find("[data-test='copy-all-credentials-btn']"); expect(btn.exists()).toBe(true); @@ -97,6 +101,10 @@ describe("TrustKitModal — copy all credentials", () => { const copied = writeText.mock.calls[0][0] as string; expect(copied).toContain("TRUST_API_KEY=api-key-abc"); expect(copied).toContain("TRUST_INTERNAL_SERVICE_KEY=internal-key-def"); + // The AES key exists only in this response — if the modal stops rendering it, + // the key is minted and lost, and the trust can never use a per-trust key. + expect(copied).toContain("TRUST_AES_KEY_BASE64=aes-key-ghi"); + expect(copied).toContain("TRUST_AES_KID=trust-uuid-123"); expect(copied).toContain("FL_KIT_SLOT=Trust_1"); expect(copied).toContain("FL_KIT_SLOT_NUMBER=1"); expect(copied).toContain("EXPECTED_TRUST_ID=trust-uuid-123"); diff --git a/flip-ui/src/services/admin-trusts-service.ts b/flip-ui/src/services/admin-trusts-service.ts index 347a74369..f5dc75dea 100644 --- a/flip-ui/src/services/admin-trusts-service.ts +++ b/flip-ui/src/services/admin-trusts-service.ts @@ -23,6 +23,11 @@ export interface ICreatedTrust { // the hub past the response. Surface to the admin in a one-time modal. trust_api_key: string; trust_internal_service_key: string; + // Payload-encryption key + key-id for this trust. Also returned once and stored + // nowhere hub-side: unlike the api key, a symmetric key cannot be reduced to a + // hash, so an admin who does not record it here must re-register to get another. + trust_aes_key: string; + trust_aes_kid: string; // FL kit slot the hub claimed for this trust from the pre-provisioned pool. // The operator's fl-clients mount the matching workspace/net-N/services/ // provisioned kit dirs; fl_kit_slot_number picks the Flower supernode key. diff --git a/scripts/tests/test_trust_kit_lib.py b/scripts/tests/test_trust_kit_lib.py index a5dea2765..fa4acb5d7 100644 --- a/scripts/tests/test_trust_kit_lib.py +++ b/scripts/tests/test_trust_kit_lib.py @@ -51,12 +51,29 @@ def _assert(condition: bool, label: str, detail: str = "") -> None: FAIL += 1 +def _count_key(content: str, key: str) -> int: + """Count live ``KEY=`` lines, matching the key exactly. + + A substring count is wrong here: ``AES_KEY_BASE64`` is a suffix of the + per-trust ``TRUST_AES_KEY_BASE64``, so ``content.count("AES_KEY_BASE64=")`` + silently counts both and a duplicate-detection assertion stops meaning + anything. + """ + return sum( + 1 + for ln in content.splitlines() + if not ln.lstrip().startswith("#") and ln.split("=", 1)[0] == key + ) + + def _full_kit(**overrides: object) -> dict: kit = { "trust_id": "11111111-1111-1111-1111-111111111111", "trust_name": "GSTT Hospital", "trust_api_key": "plain-api-key", "trust_internal_service_key": "plain-internal-key", + "trust_aes_key": "plain-aes-key==", + "trust_aes_kid": "trust-11111111-1111-1111-1111-111111111111", "fl_kit_slot": "Trust_1", "fl_kit_slot_number": 1, "hub_shared": {"AES_KEY_BASE64": "v1==", "FL_BACKEND": "flower"}, @@ -79,6 +96,12 @@ def test_new_kit_writes_creds_meta_and_hub_shared() -> None: _assert("OMOP_DB_PORT=5436" in content, "host-local profile seeded from example") _assert("TRUST_API_KEY=plain-api-key" in content, "TRUST_API_KEY written") _assert("TRUST_INTERNAL_SERVICE_KEY=plain-internal-key" in content, "internal key written") + # The hub keeps no copy of the AES key, so if it is not written here it is lost. + _assert("TRUST_AES_KEY_BASE64=plain-aes-key==" in content, "AES key written (trailing == preserved)") + _assert( + "TRUST_AES_KID=trust-11111111-1111-1111-1111-111111111111" in content, + "AES kid written", + ) _assert("EXPECTED_TRUST_ID=11111111-1111-1111-1111-111111111111" in content, "EXPECTED_TRUST_ID written") _assert("FL_KIT_SLOT=Trust_1" in content, "FL_KIT_SLOT written") _assert("FL_KIT_SLOT_NUMBER=1" in content, "FL_KIT_SLOT_NUMBER written") @@ -95,6 +118,7 @@ def test_skip_path_preserves_existing_creds() -> None: target.write_text( "TRUST_API_KEY=preserved-key\n" "TRUST_INTERNAL_SERVICE_KEY=preserved-internal\n" + "TRUST_AES_KEY_BASE64=preserved-aes==\n" "FL_KIT_SLOT=Trust_1\n" "FL_KIT_SLOT_NUMBER=1\n" ) @@ -110,6 +134,8 @@ def test_skip_path_preserves_existing_creds() -> None: content = target.read_text() _assert("TRUST_API_KEY=preserved-key" in content, "TRUST_API_KEY preserved on skip path") _assert("TRUST_INTERNAL_SERVICE_KEY=preserved-internal" in content, "internal key preserved on skip path") + # An unrecoverable credential: a skip-path clobber would strand the trust. + _assert("TRUST_AES_KEY_BASE64=preserved-aes==" in content, "AES key preserved on skip path") _assert("AES_KEY_BASE64=rotated==" in content, "hub-shared refreshed on skip path") @@ -121,11 +147,11 @@ def test_idempotent_rotation_no_dupes() -> None: tkl.write_kit(target, _full_kit(hub_shared={"AES_KEY_BASE64": "v2==", "FL_BACKEND": "nvflare"})) content = target.read_text() - _assert(content.count("AES_KEY_BASE64=") == 1, "no duplicate AES_KEY_BASE64") + _assert(_count_key(content, "AES_KEY_BASE64") == 1, "no duplicate AES_KEY_BASE64") _assert("AES_KEY_BASE64=v2==" in content, "AES key rotated to v2") _assert("FL_BACKEND=nvflare" in content, "FL_BACKEND rotated") _assert(content.count(tkl.SENTINEL) == 1, "exactly one sentinel") - _assert(content.count("TRUST_API_KEY=") == 1, "no duplicate TRUST_API_KEY") + _assert(_count_key(content, "TRUST_API_KEY") == 1, "no duplicate TRUST_API_KEY") def test_absent_target_no_example_creates_file() -> None: @@ -165,7 +191,7 @@ def test_ec2_rerun_preserves_host_local_profile() -> None: _assert("GRAFANA_PORT=3301" in content, "host-local GRAFANA_PORT preserved") _assert("TRUST_API_KEY=existing-prod-key" in content, "existing prod creds preserved") _assert("AES_KEY_BASE64=new==" in content, "hub-shared rotated") - _assert(content.count("AES_KEY_BASE64=") == 1, "no duplicate AES_KEY_BASE64 after re-run") + _assert(_count_key(content, "AES_KEY_BASE64") == 1, "no duplicate AES_KEY_BASE64 after re-run") def test_dev_commented_hub_shared() -> None: diff --git a/scripts/trust_kit_lib.py b/scripts/trust_kit_lib.py index 0b52994af..e3507ae1d 100644 --- a/scripts/trust_kit_lib.py +++ b/scripts/trust_kit_lib.py @@ -15,10 +15,11 @@ dict (as emitted by ``flip_api.scripts.register_trust``) into a kit file while preserving the operator's host-local edits: -- Credentials (``TRUST_API_KEY`` / ``TRUST_INTERNAL_SERVICE_KEY``) are written - only when present in the kit (a new registration). The idempotent skip path - omits them, so an existing kit's credentials are never clobbered (the hub - stores only hashes and cannot re-emit plaintext). +- Credentials (``TRUST_API_KEY`` / ``TRUST_INTERNAL_SERVICE_KEY`` / + ``TRUST_AES_KEY_BASE64`` / ``TRUST_AES_KID``) are written only when present in + the kit (a new registration). The idempotent skip path omits them, so an + existing kit's credentials are never clobbered (the hub stores only hashes, + and does not retain the AES key at all, so neither can be re-emitted). - Metadata (``EXPECTED_TRUST_ID`` / ``FL_KIT_SLOT`` / ``FL_KIT_SLOT_NUMBER``) is present on both paths and upserted unconditionally. - The hub-shared block is upserted under a sentinel header; the header is added @@ -66,7 +67,12 @@ ) # Plaintext credentials — written once on a new registration, never on skip. -CREDENTIAL_KEYS: tuple[str, ...] = ("TRUST_API_KEY", "TRUST_INTERNAL_SERVICE_KEY") +CREDENTIAL_KEYS: tuple[str, ...] = ( + "TRUST_API_KEY", + "TRUST_INTERNAL_SERVICE_KEY", + "TRUST_AES_KEY_BASE64", + "TRUST_AES_KID", +) # Metadata — present on both the new-registration and idempotent-skip paths. METADATA_KEYS: tuple[str, ...] = ("EXPECTED_TRUST_ID", "FL_KIT_SLOT", "FL_KIT_SLOT_NUMBER") @@ -75,6 +81,8 @@ _FIELD_BY_ENV_KEY: dict[str, str] = { "TRUST_API_KEY": "trust_api_key", "TRUST_INTERNAL_SERVICE_KEY": "trust_internal_service_key", + "TRUST_AES_KEY_BASE64": "trust_aes_key", + "TRUST_AES_KID": "trust_aes_kid", "EXPECTED_TRUST_ID": "trust_id", "FL_KIT_SLOT": "fl_kit_slot", "FL_KIT_SLOT_NUMBER": "fl_kit_slot_number", From 9a9d1d292d2c76522fbaeb76ab6b00d64611a440 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 05:22:27 +0000 Subject: [PATCH 5/7] docs(trust): describe both registration paths accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keeping the admin-UI registration flow means the kit docs have to describe it, and two statements about it were wrong. `register-trust KIT=` 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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- trust/AGENTS.md | 25 +++++++++++++++++++++---- trust/CLAUDE.md | 25 +++++++++++++++++++++---- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/trust/AGENTS.md b/trust/AGENTS.md index 51613ad3a..26b5cab0f 100644 --- a/trust/AGENTS.md +++ b/trust/AGENTS.md @@ -38,7 +38,7 @@ the commented dev form): | Host-local profile | Operator | hand-edit (ports, bind dirs) | | Trust-local credentials | Operator | hand-edit (passwords, service URLs) | | Hub-shared (managed) | Hub admin | `register-trust KIT=` (live in prod, commented in dev) / `sync-trust-kit KIT=` (prod refresh) | -| Kit credentials (managed) | Hub | `register-trust` only — write-once; hub keeps only the hash | +| Kit credentials (managed) | Hub | `register-trust` or the Add-Trust admin UI — write-once. The hub keeps only the api key's hash, and keeps nothing at all for `TRUST_AES_KEY_BASE64`, so a lost value means re-registering | The Hub-shared block is delimited by a sentinel comment (`# ── Hub-shared (managed by register-trust / sync-trust-kits — do not edit) ──`) @@ -65,14 +65,31 @@ etc., then re-transmit the refreshed kit file to the remote operator encrypted channel for on-prem). For the on-prem flow, the admin scaffolds and fills the kit on their -workstation in two commands (prod AWS creds required): +workstation in three commands (prod AWS creds required): 1. `make new-trust TRUST_CODE= TRUST_NAME="..." PROD=true` — scaffolds `trust/.env..production` from the base template (`trust/.env.example`). 2. `make register-trust KIT= PROD=true` — registers on the prod hub and - fills BOTH the Kit credentials AND the Hub-shared block in one step - (replaces the old "paste 5 UI lines + separate `sync-trust-kit`"). + fills the Kit credentials plus the part of the Hub-shared block that the hub + itself knows. +3. `make sync-trust-kit KIT= PROD=true` — **required on prod.** `register_trust` + runs inside the flip-api container, so it can only emit the `HUB_SHARED_ENV_KEYS` + present in that task's environment (`AES_KEY_BASE64`, `TRUST_API_KEY_HEADER`, + `FL_BACKEND`). The deploy-side rest — `DOCKER_TAG`, `DOCKER_REGISTRY`, + `DOCKER_FL_*`, `NLB_SUBDOMAIN`, `FL_SERVER_PORT`, the kit dates — comes from the + admin's local `$(MAIN_ENV_FILE)` here. Skip it and the operator's kit ships with + no image tags and no NLB subdomain. + +**Registering from the admin UI instead.** `POST /admin/trusts` (the Add-Trust +flow) is a supported alternative to step 2, not a legacy path. Its one-time kit +modal renders the same Kit-credentials block as paste-ready lines — +`TRUST_API_KEY`, `TRUST_INTERNAL_SERVICE_KEY`, `TRUST_AES_KEY_BASE64`, +`TRUST_AES_KID`, `FL_KIT_SLOT`, `FL_KIT_SLOT_NUMBER`, `EXPECTED_TRUST_ID` — which +the admin pastes into a kit scaffolded by step 1. Step 3 still applies, and on the +UI path it supplies the *whole* Hub-shared block rather than just the deploy-side +remainder, since nothing hub-side was written into the kit. The modal is the only +place the AES key ever appears, so it has to be recorded before the modal closes. Then `make -C deploy/providers/AWS package-onprem-trust-kit KIT= PROD=true` tarballs the populated kit file as-is + the operator's slice of the FL diff --git a/trust/CLAUDE.md b/trust/CLAUDE.md index 726f72cad..56183ebce 100644 --- a/trust/CLAUDE.md +++ b/trust/CLAUDE.md @@ -38,7 +38,7 @@ the commented dev form): | Host-local profile | Operator | hand-edit (ports, bind dirs) | | Trust-local credentials | Operator | hand-edit (passwords, service URLs) | | Hub-shared (managed) | Hub admin | `register-trust KIT=` (live in prod, commented in dev) / `sync-trust-kit KIT=` (prod refresh) | -| Kit credentials (managed) | Hub | `register-trust` only — write-once; hub keeps only the hash | +| Kit credentials (managed) | Hub | `register-trust` or the Add-Trust admin UI — write-once. The hub keeps only the api key's hash, and keeps nothing at all for `TRUST_AES_KEY_BASE64`, so a lost value means re-registering | The Hub-shared block is delimited by a sentinel comment (`# ── Hub-shared (managed by register-trust / sync-trust-kits — do not edit) ──`) @@ -65,14 +65,31 @@ etc., then re-transmit the refreshed kit file to the remote operator encrypted channel for on-prem). For the on-prem flow, the admin scaffolds and fills the kit on their -workstation in two commands (prod AWS creds required): +workstation in three commands (prod AWS creds required): 1. `make new-trust TRUST_CODE= TRUST_NAME="..." PROD=true` — scaffolds `trust/.env..production` from the base template (`trust/.env.example`). 2. `make register-trust KIT= PROD=true` — registers on the prod hub and - fills BOTH the Kit credentials AND the Hub-shared block in one step - (replaces the old "paste 5 UI lines + separate `sync-trust-kit`"). + fills the Kit credentials plus the part of the Hub-shared block that the hub + itself knows. +3. `make sync-trust-kit KIT= PROD=true` — **required on prod.** `register_trust` + runs inside the flip-api container, so it can only emit the `HUB_SHARED_ENV_KEYS` + present in that task's environment (`AES_KEY_BASE64`, `TRUST_API_KEY_HEADER`, + `FL_BACKEND`). The deploy-side rest — `DOCKER_TAG`, `DOCKER_REGISTRY`, + `DOCKER_FL_*`, `NLB_SUBDOMAIN`, `FL_SERVER_PORT`, the kit dates — comes from the + admin's local `$(MAIN_ENV_FILE)` here. Skip it and the operator's kit ships with + no image tags and no NLB subdomain. + +**Registering from the admin UI instead.** `POST /admin/trusts` (the Add-Trust +flow) is a supported alternative to step 2, not a legacy path. Its one-time kit +modal renders the same Kit-credentials block as paste-ready lines — +`TRUST_API_KEY`, `TRUST_INTERNAL_SERVICE_KEY`, `TRUST_AES_KEY_BASE64`, +`TRUST_AES_KID`, `FL_KIT_SLOT`, `FL_KIT_SLOT_NUMBER`, `EXPECTED_TRUST_ID` — which +the admin pastes into a kit scaffolded by step 1. Step 3 still applies, and on the +UI path it supplies the *whole* Hub-shared block rather than just the deploy-side +remainder, since nothing hub-side was written into the kit. The modal is the only +place the AES key ever appears, so it has to be recorded before the modal closes. Then `make -C deploy/providers/AWS package-onprem-trust-kit KIT= PROD=true` tarballs the populated kit file as-is + the operator's slice of the FL From 8b3ff860167daf795fc284dc708dbbf26fbe54c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 06:12:33 +0000 Subject: [PATCH 6/7] feat(security): key hub->trust task payloads per trust, audit the keyring at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- docs/aes-payload-keys.md | 37 ++++ .../fl_services/services/fl_service.py | 4 + flip-api/src/flip_api/main.py | 12 ++ .../flip_api/private_services/trust_tasks.py | 8 +- .../services/trust_key_config.py | 164 ++++++++++++++++++ flip-api/src/flip_api/utils/encryption.py | 15 ++ .../unit/private_services/test_trust_tasks.py | 27 +++ .../services/test_trust_key_config.py | 151 ++++++++++++++++ 8 files changed, 416 insertions(+), 2 deletions(-) create mode 100644 flip-api/src/flip_api/trusts_services/services/trust_key_config.py create mode 100644 flip-api/tests/unit/trusts_services/services/test_trust_key_config.py diff --git a/docs/aes-payload-keys.md b/docs/aes-payload-keys.md index d1b4e8792..8c5f7226b 100644 --- a/docs/aes-payload-keys.md +++ b/docs/aes-payload-keys.md @@ -42,6 +42,43 @@ compromised trust yields the key for the whole federation. `kid_for_trust()` then selects that key automatically; trusts without one keep using the shared key, so trusts can be moved over one at a time. +### What a per-trust key does and does not cover + +Hub → trust payloads are per-trust keyed only where one ciphertext has exactly one +reader. That is the task-dispatch path (`GET /tasks/pending`, whose whole batch +belongs to the polling trust) and the cohort-query submission, which encrypts the +project id once per trust. + +The FL training payload is **deliberately left on the shared key**: +`fl_service.start_training` hands a single ciphertext to the FL server, which fans it +out to every participating client. Encrypting under one trust's key would make it +undecryptable for the rest. Narrowing it needs the FL server to carry a per-client +payload — a protocol change, not a different `kid` at the call site. + +Trust-internal encryption (imaging-api → data-access-api) needs nothing special: on a +trust, `_default_kid()` already resolves to that trust's own `TRUST_AES_KID`. + +### Getting only half of it configured + +The two halves are provisioned separately and nothing joins them up, so a partial +rollout fails asymmetrically: + +* **hub → trust degrades silently.** `kid_for_trust()` does not find the trust's kid + in the hub's keyring and falls back to the shared key. The trust decrypts it fine. + Everything works, without the isolation you configured, and nothing says so. +* **trust → hub fails loudly**, with `KeyError: No key registered for kid + 'trust-'`. + +Because the quiet direction is the dangerous one, flip-api audits the keyring at +startup (`trusts_services/services/trust_key_config.py`) and logs per-trust key +coverage. Two operator errors are logged at error level: a kid in `AES_TRUST_KEYS` +matching no registered trust (a typo or a deleted trust — the key is loaded but never +selected, which is indistinguishable at runtime from "no key yet"), and a key that is +not a valid AES length. A malformed `AES_TRUST_KEYS` now fails at boot rather than on +the first request that encrypts. + +`_keyring()` is memoised for the process lifetime, so adding a key needs a restart. + Both registration paths now deliver step 2 automatically: `register-trust KIT=` writes the two variables into the kit file (they are credential keys in `scripts/trust_kit_lib.py`, so they are written once on a new registration and diff --git a/flip-api/src/flip_api/fl_services/services/fl_service.py b/flip-api/src/flip_api/fl_services/services/fl_service.py index ab87a1e63..e93372e94 100644 --- a/flip-api/src/flip_api/fl_services/services/fl_service.py +++ b/flip-api/src/flip_api/fl_services/services/fl_service.py @@ -357,6 +357,10 @@ def start_training( from flip_api.fl_services.services import fl_scheduler_service required_info = fl_scheduler_service.get_required_training_details(model_id, session) + # Deliberately the shared key, not a per-trust one: this single ciphertext is handed + # to the FL server, which fans it out to every participating client. Encrypting it + # under one trust's key would make it undecryptable for all the others. Narrowing + # this needs the FL server to carry a per-client payload, not a different kid here. encrypted_project_id = encrypt(required_info.project_id) training_details = IStartTrainingBody( diff --git a/flip-api/src/flip_api/main.py b/flip-api/src/flip_api/main.py index 7df1acbd5..1ebf60ea8 100644 --- a/flip-api/src/flip_api/main.py +++ b/flip-api/src/flip_api/main.py @@ -18,6 +18,7 @@ from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse from slowapi.errors import RateLimitExceeded +from sqlmodel import Session from flip_api.cohort_services import ( get_cohort_query_results, @@ -25,6 +26,7 @@ submit_cohort_query, ) from flip_api.config import get_settings +from flip_api.db.database import get_engine from flip_api.file_services import ( delete_file, download_file, @@ -88,6 +90,7 @@ trusts_health_check, update_trust_status, ) +from flip_api.trusts_services.services.trust_key_config import log_trust_key_config from flip_api.user_services import ( access_request, delete_user, @@ -101,6 +104,7 @@ update_user, ) from flip_api.utils.cognito_helpers import get_cors_allowed_origins +from flip_api.utils.logger import logger from flip_api.utils.rate_limiter import limiter from flip_api.utils.security_headers import SecurityHeadersMiddleware @@ -120,6 +124,14 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: app (FastAPI): The FastAPI application instance being started. """ _cors_allowed_origins.extend(get_cors_allowed_origins()) + # Report per-trust payload-key coverage, and fail fast on a malformed + # AES_TRUST_KEYS rather than midway through the first request that encrypts. + # A DB outage must not stop the app booting — the audit is diagnostics. + try: + with Session(get_engine()) as session: + log_trust_key_config(session) + except Exception: + logger.exception("Could not audit the per-trust payload-key configuration at startup") start_scheduler() print("Starting up the app...") yield diff --git a/flip-api/src/flip_api/private_services/trust_tasks.py b/flip-api/src/flip_api/private_services/trust_tasks.py index a5314d8d9..bc9ae889f 100644 --- a/flip-api/src/flip_api/private_services/trust_tasks.py +++ b/flip-api/src/flip_api/private_services/trust_tasks.py @@ -31,7 +31,7 @@ from flip_api.domain.schemas.private import TaskResultInput, TrustTaskResponse from flip_api.domain.schemas.status import TaskStatus, TaskType from flip_api.private_services.imaging_notifications import handle_imaging_task_completed -from flip_api.utils.encryption import encrypt +from flip_api.utils.encryption import encrypt, kid_for_trust from flip_api.utils.logger import logger from flip_api.utils.rate_limiter import limiter @@ -87,6 +87,10 @@ def _get_pending_tasks(trust: Trust, db: Session) -> dict[str, object]: return {**_trust_identity(trust), "tasks": []} now = datetime.now(timezone.utc) + # Every task in this batch goes to one trust, so the whole batch is encrypted + # under that trust's key when it has one. Resolved once rather than per task: + # the lookup is keyring-only, and the answer cannot change within a batch. + kid = kid_for_trust(trust_id=str(trust.id), trust_code=trust.code) response: list[TrustTaskResponse] = [] for task in tasks: task.status = TaskStatus.IN_PROGRESS @@ -95,7 +99,7 @@ def _get_pending_tasks(trust: Trust, db: Session) -> dict[str, object]: TrustTaskResponse( id=task.id, task_type=task.task_type, - payload=encrypt(task.payload), + payload=encrypt(task.payload, kid=kid), created_at=task.created_at, ) ) diff --git a/flip-api/src/flip_api/trusts_services/services/trust_key_config.py b/flip-api/src/flip_api/trusts_services/services/trust_key_config.py new file mode 100644 index 000000000..f83c632f1 --- /dev/null +++ b/flip-api/src/flip_api/trusts_services/services/trust_key_config.py @@ -0,0 +1,164 @@ +# Copyright (c) Guy's and St Thomas' NHS Foundation Trust & King's College London +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# http://www.apache.org/licenses/LICENSE-2.0 +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""Startup audit of the hub's per-trust payload-encryption key configuration. + +Per-trust keys are provisioned in two halves that nothing joins up: the trust gets +``TRUST_AES_KID`` / ``TRUST_AES_KEY_BASE64`` in its kit file, and the hub gets the +same key in its ``AES_TRUST_KEYS`` map. Miss the hub half and the two directions +disagree in a way that is easy to misread: + +* **hub → trust** degrades **silently**. ``kid_for_trust()`` looks the trust's kid up + in the keyring, does not find it, and falls back to the shared key. The trust + decrypts it fine, because its keyring also holds the shared key. Everything works — + with none of the isolation the configuration was supposed to buy, and no signal. +* **trust → hub** fails **loudly**, with ``KeyError: No key registered for kid + 'trust-'`` at decrypt time. + +The quiet direction is the dangerous one, so this reports at boot what the hub can +actually see: which trusts have a key, which are on the shared key, and — the usual +cause of a silent fallback — which configured kids match no trust at all, because a +typo'd or stale kid is indistinguishable at runtime from "this trust has no key yet". + +What the hub 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. +""" + +from dataclasses import dataclass, field +from uuid import UUID + +from sqlmodel import Session, select + +from flip_api.db.models.main_models import Trust +from flip_api.utils.encryption import SHARED_KID, registered_kids +from flip_api.utils.logger import logger + +#: Prefix every per-trust kid carries — see ``kid_for_trust``. +_KID_PREFIX = "trust-" + +#: Key lengths AES accepts, in bytes (AES-128 / AES-192 / AES-256). +_VALID_KEY_LENGTHS = (16, 24, 32) + + +@dataclass +class TrustKeyReport: + """What the hub's keyring says about per-trust key coverage.""" + + #: Names of trusts with a per-trust key loaded. + covered: list[str] = field(default_factory=list) + #: Names of trusts falling back to the shared key. + on_shared_key: list[str] = field(default_factory=list) + #: Configured kids matching no trust — a typo, or a deleted trust. + orphan_kids: list[str] = field(default_factory=list) + #: ``kid`` values whose key is not a valid AES length. + bad_length_kids: list[str] = field(default_factory=list) + + @property + def total_trusts(self) -> int: + return len(self.covered) + len(self.on_shared_key) + + +def audit_trust_key_config(session: Session) -> TrustKeyReport: + """Compare the hub's keyring against the registered trusts. + + Args: + session (Session): Database session used to read the trust roster. + + Returns: + TrustKeyReport: Coverage plus any misconfiguration found. + + Raises: + json.JSONDecodeError: If ``AES_TRUST_KEYS`` is not valid JSON. + binascii.Error: If a configured key is not valid base64. + """ + # Also forces the lazy keyring parse, so a malformed AES_TRUST_KEYS raises here + # (at boot) instead of on the first request that needs to encrypt. + kids = registered_kids() + + trusts = session.exec(select(Trust)).all() + report = TrustKeyReport() + + matched_kids: set[str] = {SHARED_KID} + for trust in trusts: + # Mirrors kid_for_trust's lookup order: id first, then code. + candidates = [f"{_KID_PREFIX}{trust.id}"] + if trust.code: + candidates.append(f"{_KID_PREFIX}{trust.code}") + hit = next((c for c in candidates if c in kids), None) + if hit: + matched_kids.add(hit) + report.covered.append(trust.name) + else: + report.on_shared_key.append(trust.name) + + for kid, length in kids.items(): + if kid not in matched_kids: + report.orphan_kids.append(kid) + if length not in _VALID_KEY_LENGTHS: + report.bad_length_kids.append(kid) + + return report + + +def log_trust_key_config(session: Session) -> TrustKeyReport: + """Run the audit and log it. Never raises on a misconfiguration — it reports. + + A missing per-trust key is the normal state during a staged rollout, so coverage + is informational. An orphan kid or a bad key length is an operator error that + would otherwise only surface as a silent shared-key fallback, so both are logged + at error level. + + Args: + session (Session): Database session used to read the trust roster. + + Returns: + TrustKeyReport: The report that was logged, for callers that want to assert on it. + """ + report = audit_trust_key_config(session) + + logger.info( + "Per-trust payload keys: %d/%d trusts have their own key; %d using the shared key.", + len(report.covered), + report.total_trusts, + len(report.on_shared_key), + ) + + if report.orphan_kids: + logger.error( + "AES_TRUST_KEYS contains %d kid(s) matching no registered trust: %s. " + "Those keys are never selected, so the trusts they were meant for keep using " + "the shared key with no other symptom. Check for a typo or a deleted trust.", + len(report.orphan_kids), + ", ".join(sorted(report.orphan_kids)), + ) + + if report.bad_length_kids: + logger.error( + "AES_TRUST_KEYS contains %d key(s) that are not a valid AES length (16/24/32 bytes): %s. " + "Encrypting to these trusts will fail at request time.", + len(report.bad_length_kids), + ", ".join(sorted(report.bad_length_kids)), + ) + + return report + + +def kid_for_trust_id(trust_id: UUID) -> str: + """Return the kid a trust's key would be registered under. + + Args: + trust_id (UUID): The trust's id. + + Returns: + str: The ``trust-`` kid, matching what ``register_trust`` mints. + """ + return f"{_KID_PREFIX}{trust_id}" diff --git a/flip-api/src/flip_api/utils/encryption.py b/flip-api/src/flip_api/utils/encryption.py index 8943640e6..71e01a398 100644 --- a/flip-api/src/flip_api/utils/encryption.py +++ b/flip-api/src/flip_api/utils/encryption.py @@ -102,6 +102,21 @@ def _keyring() -> dict[str, bytes]: return ring +def registered_kids() -> dict[str, int]: + """Return ``kid -> key length in bytes`` for every key in the keyring. + + Exposes the *shape* of the keyring without handing out key material, so a + configuration audit can report what is loaded and check key lengths without + ever holding a key. Calling this also forces the lazy parse of + ``AES_TRUST_KEYS``, which is why startup calls it: a malformed value then + fails at boot rather than midway through a request. + + Returns: + dict[str, int]: Each registered kid mapped to its key length in bytes. + """ + return {kid: len(key) for kid, key in _keyring().items()} + + def _default_kid() -> str: """Return the kid used when the caller does not name one. diff --git a/flip-api/tests/unit/private_services/test_trust_tasks.py b/flip-api/tests/unit/private_services/test_trust_tasks.py index cc1651d95..4d0215c6c 100644 --- a/flip-api/tests/unit/private_services/test_trust_tasks.py +++ b/flip-api/tests/unit/private_services/test_trust_tasks.py @@ -119,6 +119,33 @@ def test_get_pending_tasks_returns_tasks_with_identity(mock_pending_tasks, mock_ app.dependency_overrides.pop(get_session, None) +def test_pending_tasks_are_encrypted_under_the_trusts_own_key(mock_pending_tasks, mock_auth, mock_trust): + """The batch must be encrypted with the polling trust's kid, not the shared one. + + Every task in a batch belongs to one trust, so this is the one hub->trust path that + can be narrowed to a per-trust key. Falling back to the shared kid is correct only + when that trust has no key of its own; if this regresses to an unconditional + ``encrypt(...)`` nothing fails, the payloads just quietly lose their isolation. + """ + mock_db = MagicMock() + mock_db.exec.return_value.all.return_value = mock_pending_tasks + app.dependency_overrides[get_session] = lambda: mock_db + + with patch("flip_api.private_services.trust_tasks.encrypt", return_value="ct") as mock_encrypt: + with patch( + "flip_api.private_services.trust_tasks.kid_for_trust", return_value="trust-abc" + ) as mock_kid: + response = client.get("/api/tasks/pending") + + assert response.status_code == 200 + mock_kid.assert_called_once_with(trust_id=str(mock_trust.id), trust_code=mock_trust.code) + assert mock_encrypt.call_count == len(mock_pending_tasks) + for call in mock_encrypt.call_args_list: + assert call.kwargs["kid"] == "trust-abc" + + app.dependency_overrides.pop(get_session, None) + + def test_get_pending_tasks_empty(mock_auth, mock_trust): """No queued tasks — handler still returns the identity block.""" mock_db = MagicMock() diff --git a/flip-api/tests/unit/trusts_services/services/test_trust_key_config.py b/flip-api/tests/unit/trusts_services/services/test_trust_key_config.py new file mode 100644 index 000000000..38c0d2455 --- /dev/null +++ b/flip-api/tests/unit/trusts_services/services/test_trust_key_config.py @@ -0,0 +1,151 @@ +# Copyright (c) Guy's and St Thomas' NHS Foundation Trust & King's College London +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# http://www.apache.org/licenses/LICENSE-2.0 +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +"""Tests for the startup audit of per-trust payload-encryption keys.""" + +import base64 +import json +import logging +from unittest.mock import MagicMock +from uuid import uuid4 + +import pytest + +from flip_api.trusts_services.services.trust_key_config import ( + audit_trust_key_config, + kid_for_trust_id, + log_trust_key_config, +) +from flip_api.utils import encryption + +SHARED = base64.b64encode(b"0" * 32).decode() + + +def _trust(name: str, code: str | None = None): + trust = MagicMock() + trust.id = uuid4() + trust.name = name + trust.code = code + return trust + + +def _session(trusts): + session = MagicMock() + session.exec.return_value.all.return_value = trusts + return session + + +@pytest.fixture(autouse=True) +def _clean_keyring(monkeypatch): + """Reset the memoised keyring around each test, and pin a known shared key.""" + monkeypatch.setattr(encryption, "_shared_key", lambda: base64.b64decode(SHARED)) + monkeypatch.delenv("AES_TRUST_KEYS", raising=False) + monkeypatch.delenv("TRUST_AES_KID", raising=False) + monkeypatch.delenv("TRUST_AES_KEY_BASE64", raising=False) + encryption._reset_caches() + yield + encryption._reset_caches() + + +def _set_trust_keys(monkeypatch, mapping: dict[str, str]) -> None: + monkeypatch.setenv("AES_TRUST_KEYS", json.dumps(mapping)) + encryption._reset_caches() + + +def test_no_per_trust_keys_puts_every_trust_on_the_shared_key(): + trusts = [_trust("GSTT"), _trust("KCH")] + report = audit_trust_key_config(_session(trusts)) + + assert report.covered == [] + assert sorted(report.on_shared_key) == ["GSTT", "KCH"] + assert report.orphan_kids == [] + assert report.total_trusts == 2 + + +def test_trust_with_a_key_is_reported_as_covered(monkeypatch): + gstt, kch = _trust("GSTT"), _trust("KCH") + _set_trust_keys(monkeypatch, {kid_for_trust_id(gstt.id): base64.b64encode(b"1" * 32).decode()}) + + report = audit_trust_key_config(_session([gstt, kch])) + + assert report.covered == ["GSTT"] + assert report.on_shared_key == ["KCH"] + assert report.orphan_kids == [] + + +def test_kid_by_trust_code_also_counts_as_covered(monkeypatch): + """``kid_for_trust`` falls back to the code, so the audit must match on it too.""" + gstt = _trust("GSTT", code="GSTT") + _set_trust_keys(monkeypatch, {"trust-GSTT": base64.b64encode(b"1" * 32).decode()}) + + report = audit_trust_key_config(_session([gstt])) + + assert report.covered == ["GSTT"] + assert report.orphan_kids == [] + + +def test_kid_matching_no_trust_is_reported_as_an_orphan(monkeypatch): + """The typo case: the key is loaded but is never selected, so the trust it was + meant for silently keeps using the shared key. Nothing else in the system says so. + """ + gstt = _trust("GSTT") + _set_trust_keys(monkeypatch, {f"trust-{uuid4()}": base64.b64encode(b"1" * 32).decode()}) + + report = audit_trust_key_config(_session([gstt])) + + assert report.covered == [] + assert report.on_shared_key == ["GSTT"] + assert len(report.orphan_kids) == 1 + + +def test_shared_kid_is_never_an_orphan(): + report = audit_trust_key_config(_session([_trust("GSTT")])) + assert encryption.SHARED_KID not in report.orphan_kids + + +def test_key_of_the_wrong_length_is_flagged(monkeypatch): + gstt = _trust("GSTT") + _set_trust_keys(monkeypatch, {kid_for_trust_id(gstt.id): base64.b64encode(b"too-short").decode()}) + + report = audit_trust_key_config(_session([gstt])) + + assert report.covered == ["GSTT"] # it is loaded... + assert report.bad_length_kids == [kid_for_trust_id(gstt.id)] # ...but unusable + + +def test_malformed_trust_keys_raises_so_boot_fails(monkeypatch): + """A bad AES_TRUST_KEYS must fail at startup, not on the first request to encrypt.""" + monkeypatch.setenv("AES_TRUST_KEYS", "{not json") + encryption._reset_caches() + + with pytest.raises(json.JSONDecodeError): + audit_trust_key_config(_session([_trust("GSTT")])) + + +def test_log_reports_orphans_at_error_level(monkeypatch, caplog): + _set_trust_keys(monkeypatch, {"trust-typo": base64.b64encode(b"1" * 32).decode()}) + + with caplog.at_level(logging.INFO, logger="uvicorn"): + report = log_trust_key_config(_session([_trust("GSTT")])) + + assert report.orphan_kids == ["trust-typo"] + errors = [r for r in caplog.records if r.levelno >= logging.ERROR] + assert errors, "an orphan kid must be logged at error level" + assert "trust-typo" in errors[0].getMessage() + + +def test_log_does_not_raise_when_everything_is_on_the_shared_key(caplog): + """A staged rollout is the normal state — coverage alone is not an error.""" + with caplog.at_level(logging.INFO, logger="uvicorn"): + log_trust_key_config(_session([_trust("GSTT"), _trust("KCH")])) + + assert not [r for r in caplog.records if r.levelno >= logging.ERROR] From 1f4c35a60ac6cb1d7b5bbb7fe84f0646f4d13d6c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 06:18:36 +0000 Subject: [PATCH 7/7] fix: ship the per-trust AES kid inert, so registration cannot break decryption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delivering the key (8172c098) 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-' 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 8172c098 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 Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU Signed-off-by: Claude --- docs/aes-payload-keys.md | 30 ++++++++++-- flip-ui/src/partials/trusts/TrustKitModal.vue | 7 +-- .../trusts/__tests__/TrustKitModal.spec.ts | 6 ++- scripts/tests/test_trust_kit_lib.py | 24 ++++++++- scripts/trust_kit_lib.py | 49 ++++++++++++++++++- 5 files changed, 104 insertions(+), 12 deletions(-) diff --git a/docs/aes-payload-keys.md b/docs/aes-payload-keys.md index 8c5f7226b..27a878628 100644 --- a/docs/aes-payload-keys.md +++ b/docs/aes-payload-keys.md @@ -32,12 +32,34 @@ registration, returned in the kit as `trust_aes_key` / `trust_aes_kid`. Per-trust keys contain the blast radius: with one shared key, a single compromised trust yields the key for the whole federation. +Registration delivers the key to the trust but leaves it **inert**: the kit gets +`TRUST_AES_KEY_BASE64` live and `TRUST_AES_KID` commented out. Nothing is stored +hub-side — the hub has no column for it and cannot re-emit it — so the value shown +at registration is the only copy, and `AES_TRUST_KEYS` is populated by hand. + +**The order matters, and there is no safe partial state.** `TRUST_AES_KID` is the +switch: setting it live makes the trust encrypt under its own key *and* is what lets +the trust decrypt anything the hub encrypts to it. Enable the two halves out of step +and one direction breaks: + +* **Trust first, hub not yet:** imaging-api encrypts the new XNAT user's password + under the trust's kid, the hub cannot resolve it, and the credentials email is + never sent. The failure is swallowed by a broad handler in + `private_services/imaging_notifications.py`, so the only symptom is a log line. +* **Hub first, trust not yet:** `kid_for_trust()` starts selecting the trust's kid + for task payloads, but the trust has not loaded that kid, so its poller cannot + decrypt them. + +So enable both sides and restart both, in one window: + 1. Put the trust's key in the hub's `AES_TRUST_KEYS` under `trust-` (source it from a secret store — **not** the application database, or a DB - compromise becomes a federation-wide key compromise). -2. Set `TRUST_AES_KID` / `TRUST_AES_KEY_BASE64` in that trust's kit - (`trust/.env..`), distributed out-of-band like - `TRUST_INTERNAL_SERVICE_KEY`. + compromise becomes a federation-wide key compromise), and restart the hub: + `_keyring()` is memoised for the process lifetime. +2. Uncomment `TRUST_AES_KID` in that trust's kit (`trust/.env..`) and + restart the trust stack. + +Re-registering a trust never re-comments a kid an operator has enabled. `kid_for_trust()` then selects that key automatically; trusts without one keep using the shared key, so trusts can be moved over one at a time. diff --git a/flip-ui/src/partials/trusts/TrustKitModal.vue b/flip-ui/src/partials/trusts/TrustKitModal.vue index b72504d85..00af442c5 100644 --- a/flip-ui/src/partials/trusts/TrustKitModal.vue +++ b/flip-ui/src/partials/trusts/TrustKitModal.vue @@ -90,8 +90,9 @@

FL_KIT_SLOT is the FL identity clients register under; EXPECTED_TRUST_ID is an optional startup self-check. - TRUST_AES_KID stays inert until the same key is added to - the hub's AES_TRUST_KEYS map. + TRUST_AES_KID is commented on purpose — uncomment it only + after the same key is in the hub's AES_TRUST_KEYS map, + or payloads stop decrypting in both directions.

@@ -168,7 +169,7 @@ const allCredentialsBlock = computed(() => [ `TRUST_API_KEY=${props.trust?.trust_api_key ?? ""}`, `TRUST_INTERNAL_SERVICE_KEY=${props.trust?.trust_internal_service_key ?? ""}`, `TRUST_AES_KEY_BASE64=${props.trust?.trust_aes_key ?? ""}`, - `TRUST_AES_KID=${props.trust?.trust_aes_kid ?? ""}`, + `# TRUST_AES_KID=${props.trust?.trust_aes_kid ?? ""}`, `FL_KIT_SLOT=${props.trust?.fl_kit_slot ?? ""}`, `FL_KIT_SLOT_NUMBER=${props.trust?.fl_kit_slot_number ?? ""}`, `EXPECTED_TRUST_ID=${props.trust?.id ?? ""}` diff --git a/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts b/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts index a64d1612f..dc31cbf51 100644 --- a/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts +++ b/flip-ui/src/partials/trusts/__tests__/TrustKitModal.spec.ts @@ -72,7 +72,7 @@ describe("TrustKitModal — copy all credentials", () => { expect(text).toContain("TRUST_API_KEY=api-key-abc"); expect(text).toContain("TRUST_INTERNAL_SERVICE_KEY=internal-key-def"); expect(text).toContain("TRUST_AES_KEY_BASE64=aes-key-ghi"); - expect(text).toContain("TRUST_AES_KID=trust-uuid-123"); + expect(text).toContain("# TRUST_AES_KID=trust-uuid-123"); expect(text).toContain("FL_KIT_SLOT=Trust_1"); expect(text).toContain("FL_KIT_SLOT_NUMBER=1"); expect(text).toContain("EXPECTED_TRUST_ID=trust-uuid-123"); @@ -104,7 +104,9 @@ describe("TrustKitModal — copy all credentials", () => { // The AES key exists only in this response — if the modal stops rendering it, // the key is minted and lost, and the trust can never use a per-trust key. expect(copied).toContain("TRUST_AES_KEY_BASE64=aes-key-ghi"); - expect(copied).toContain("TRUST_AES_KID=trust-uuid-123"); + // Commented on purpose: enabling it before the hub holds the key breaks + // decryption in both directions. + expect(copied).toContain("# TRUST_AES_KID=trust-uuid-123"); expect(copied).toContain("FL_KIT_SLOT=Trust_1"); expect(copied).toContain("FL_KIT_SLOT_NUMBER=1"); expect(copied).toContain("EXPECTED_TRUST_ID=trust-uuid-123"); diff --git a/scripts/tests/test_trust_kit_lib.py b/scripts/tests/test_trust_kit_lib.py index fa4acb5d7..c16831b9d 100644 --- a/scripts/tests/test_trust_kit_lib.py +++ b/scripts/tests/test_trust_kit_lib.py @@ -98,10 +98,13 @@ def test_new_kit_writes_creds_meta_and_hub_shared() -> None: _assert("TRUST_INTERNAL_SERVICE_KEY=plain-internal-key" in content, "internal key written") # The hub keeps no copy of the AES key, so if it is not written here it is lost. _assert("TRUST_AES_KEY_BASE64=plain-aes-key==" in content, "AES key written (trailing == preserved)") + # The kid ships commented: setting it live before the hub holds the same key + # breaks decryption in both directions, so enabling it must stay deliberate. _assert( - "TRUST_AES_KID=trust-11111111-1111-1111-1111-111111111111" in content, - "AES kid written", + "# TRUST_AES_KID=trust-11111111-1111-1111-1111-111111111111" in content, + "AES kid written commented (inert until enabled)", ) + _assert(_count_key(content, "TRUST_AES_KID") == 0, "AES kid has no live line") _assert("EXPECTED_TRUST_ID=11111111-1111-1111-1111-111111111111" in content, "EXPECTED_TRUST_ID written") _assert("FL_KIT_SLOT=Trust_1" in content, "FL_KIT_SLOT written") _assert("FL_KIT_SLOT_NUMBER=1" in content, "FL_KIT_SLOT_NUMBER written") @@ -220,6 +223,22 @@ def test_dev_commented_hub_shared() -> None: _assert(content2.count("# AES_KEY_BASE64=") == 1, "no duplicate commented key on re-run") +def test_enabled_aes_kid_is_not_switched_off_by_re_registration() -> None: + print("▶ a live TRUST_AES_KID set by the operator survives a re-run") + with tempfile.TemporaryDirectory() as td: + target = Path(td) / ".env.GSTT.development" + target.write_text("TRUST_AES_KID=trust-enabled-by-operator\n") + + tkl.write_kit(target, _full_kit()) + + content = target.read_text() + _assert( + "TRUST_AES_KID=trust-enabled-by-operator" in content, + "operator's live kid preserved", + ) + _assert(_count_key(content, "TRUST_AES_KID") == 1, "no second kid line added") + + def main() -> None: test_new_kit_writes_creds_meta_and_hub_shared() test_dev_commented_hub_shared() @@ -227,6 +246,7 @@ def main() -> None: test_idempotent_rotation_no_dupes() test_absent_target_no_example_creates_file() test_ec2_rerun_preserves_host_local_profile() + test_enabled_aes_kid_is_not_switched_off_by_re_registration() print("—") print(f"PASS={PASS} FAIL={FAIL}") sys.exit(0 if FAIL == 0 else 1) diff --git a/scripts/trust_kit_lib.py b/scripts/trust_kit_lib.py index e3507ae1d..5346a7b5f 100644 --- a/scripts/trust_kit_lib.py +++ b/scripts/trust_kit_lib.py @@ -71,9 +71,16 @@ "TRUST_API_KEY", "TRUST_INTERNAL_SERVICE_KEY", "TRUST_AES_KEY_BASE64", - "TRUST_AES_KID", ) +# Written commented-out, with its value preserved. Setting TRUST_AES_KID live is what +# switches a trust onto its own key, and doing that before the hub holds the same key +# breaks both directions: the trust encrypts to the hub under a kid the hub cannot +# resolve, and the hub encrypts to the trust under a kid the trust has not loaded. +# The kit therefore ships the value ready to use but inert — see +# docs/aes-payload-keys.md for the enablement order. +COMMENTED_CREDENTIAL_KEYS: tuple[str, ...] = ("TRUST_AES_KID",) + # Metadata — present on both the new-registration and idempotent-skip paths. METADATA_KEYS: tuple[str, ...] = ("EXPECTED_TRUST_ID", "FL_KIT_SLOT", "FL_KIT_SLOT_NUMBER") @@ -116,6 +123,41 @@ def upsert(lines: list[str], key: str, value: str) -> list[str]: return out +def upsert_commented(lines: list[str], key: str, value: str) -> list[str]: + """Upsert ``# KEY=value`` — the value is delivered, but inert until uncommented. + + Distinct from :func:`comment_out`, which discards the value. Here the operator + needs the value in the file to enable it later, so it must survive; and distinct + from :func:`upsert`, which would make it live immediately. + + An existing *live* ``KEY=`` line is left alone: an operator who has deliberately + enabled the key must not have it silently switched off by a re-registration. + + Args: + lines (list[str]): Current file lines (no trailing newlines). + key (str): Env var name to write commented. + value (str): Value to preserve behind the comment. + + Returns: + list[str]: A new list of lines with the key present as ``# KEY=value``. + """ + if any(not ln.lstrip().startswith("#") and ln.split("=", 1)[0] == key for ln in lines): + return lines + + out: list[str] = [] + replaced = False + for line in lines: + bare = line.lstrip("#").lstrip() + if not replaced and line.lstrip().startswith("#") and bare.split("=", 1)[0] == key: + out.append(f"# {key}={value}") + replaced = True + else: + out.append(line) + if not replaced: + out.append(f"# {key}={value}") + return out + + def comment_out(lines: list[str], key: str) -> list[str]: """Replace the first ``KEY=...`` *or* ``# KEY=...`` line with ``# KEY=`` (no value). @@ -174,6 +216,11 @@ def write_kit(target: Path, kit: dict, example: Path | None = None, hub_shared_c if value is not None: lines = upsert(lines, env_key, str(value)) + for env_key in COMMENTED_CREDENTIAL_KEYS: + value = kit.get(_FIELD_BY_ENV_KEY[env_key]) + if value is not None: + lines = upsert_commented(lines, env_key, str(value)) + if hub_shared_commented: # Dev kit: document the hub-shared keys under the sentinel but keep them # inert (commented). Their values come from the hub's .env.development,