feat(identity): envelope encryption for per-user secret values (#876 machinery) - #1644
Open
DmitriyG228 wants to merge 2 commits into
Open
DmitriyG228 wants to merge 2 commits into
DmitriyG228 wants to merge 2 commits into
Conversation
…machinery) KEK (operator env, never in the DB) wraps a per-user DEK (stored wrapped in users.data), which encrypts each secret value to enc:v1:<nonce>:<ct>. Rotating the KEK re-wraps N small DEKs instead of re-encrypting every secret — the whole reason for the middle key. JSONB structure is untouched: keys stay keys, only values change shape. Every ciphertext is bound to (user_id, field) through AES-GCM additional data, so a value lifted between users or between fields fails authentication instead of decrypting somewhere it does not belong. Both are tested. Dual-read, never dual-write: an unprefixed value is pre-migration plaintext and reads back unchanged, so a database mid-migration keeps working; there is no matching passthrough on encrypt. And #876 A4 — require_readable() refuses to continue when a row holds ciphertext and no KEK is configured, because the failure this exists to prevent is a key going missing in one environment and the service serving, or rewriting, secrets in the clear. FINDING, and it moves this file: #876 proposes putting this 'behind the existing SecretsPort seam' in identity_core. Nothing imports identity_core — grep across core/ and services/ returns no consumer outside its own tests, and the admin-api Dockerfile copies only admin-api's own src. That seam is dead code, so encryption placed there would never have run. This lives in admin_api/app/ next to main.py and calendars.py, which is where users.data is actually written. Adds cryptography (Apache-2.0 OR BSD-3, Category A; 50.0.1, matching the pin already resolved in core/meetings/services/mcp/uv.lock). The stdlib has no AES. Import is proven at configure time so a KEK set into an image without the wheel fails at boot, not at the first user. Scope: machinery only. No call site is wired, no migration sweep, no KEK provisioned in any environment. Behaviour is identical until a caller opts in. [19 new tests]
uv.lock regenerated: cryptography 50.0.1 + cffi + pycparser. The Dockerfile runs uv sync --frozen, so the lock must carry it or the image build fails.
🃏 Merge card — #1644
Not mergeable yet — every row above must be accepted before merge (choke point 1). Fill in what's ❌ above, then this clears automatically. How a PR reaches merge: the merge bar. |
Member
Author
|
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Machinery for #876, built now because the calendar-OAuth connect flow will store Google refresh
tokens and those must not land in
users.datain the clear. An ICS URL leaking exposes onecalendar's contents; a refresh token leaking is ongoing, silent read access until revoked.
COMMITMENTS IN THIS PR
None. No published terms, no dates, no rights, no spend. Additive code that nothing calls; a new
runtime dependency (
cryptography) is declared and locked. Behaviour is byte-identical until a callsite opts in.
The shape
Rotating the KEK re-wraps N small DEKs instead of re-encrypting every secret — that is the entire
reason for the middle key, and it is tested (
test_rotating_the_kek_is_a_rewrap_not_a_reencrypt).The JSONB structure is untouched: keys stay keys, only values change shape, so nothing querying this
column learns anything new.
Three properties worth reviewing
Ciphertexts are bound to (user, field) through AES-GCM additional data. A value lifted from one
user's row into another's, or from
webhook_secretintostt_token, fails authentication ratherthan decrypting somewhere it does not belong. Both directions tested.
Dual-read, never dual-write. An unprefixed value is pre-migration plaintext and reads back
unchanged, so a database mid-migration keeps working. There is deliberately no matching passthrough
on
encrypt— once a cipher is configured everything written is encrypted, which makes migration aread-then-write sweep rather than a mode that can be left half-on.
A missing KEK stops the service (#876 A4).
require_readable()raises when a row holdsciphertext and no key is configured. The failure this exists to prevent is a KEK quietly absent in
one environment and a service carrying on — serving, or worse re-writing, values in the clear.
SecretCipher.from_env()also proves thecryptographyimport at configure time, so a KEK set intoan image without the wheel fails at boot rather than when the first user connects a calendar.
Finding: #876's proposed seam is dead code
The issue says to put this "behind the existing
SecretsPortseam" inidentity_core. Nothingimports
identity_core—grep -rl identity_coreacrosscore/andservices/returns noconsumer outside its own tests, and
core/identity/services/admin-api/Dockerfilecopies onlyadmin-api's own
src. Encryption placed there would never have executed in the running service.So this lives in
admin_api/app/field_crypto.py, next tomain.pyandcalendars.py— whereusers.datais actually written.SecretsPortis a real idea (audited brokering of named operatorsecrets) but it is a different concern from encrypting user-owned values at rest, and it is not
currently wired to anything. Worth deciding separately whether to wire or retire it.
Dependency
cryptography>=45,<51— Apache-2.0 OR BSD-3, FINOS Category A. Resolves to 50.0.1, the same versionalready locked in
core/meetings/services/mcp/uv.lock. The stdlib has no AES, so a dependency isunavoidable for AES-256-GCM.
uv.lockregenerated (addscffi,pycparser); the Dockerfile runsuv sync --frozen, so the lock had to carry it.Scope — deliberately narrow
In: the cipher, its tests, the dependency.
Out: every call site. The four existing plaintext fields (
stt_token, LLMapi_key,webhook_secret,ics_url) are untouched, there is no migration sweep, and no KEK is provisionedin any environment.
This is the narrow reading of "encrypt first": build the machinery and use it for new secrets —
the OAuth tokens — from day one, then migrate the existing four on the same machinery. Full #876
stays open, and becomes "wire four call sites and run the sweep" rather than a from-scratch design.
Tests
tests/test_field_crypto.py, 19 cases, pure — no containers, no database.Not run: the rest of the admin-api suite.
tests/conftest.pystarts Postgres and Redis viatestcontainers, and container workloads do not run on the founder's laptop — that belongs on
bbbor in CI. No file in this PR is imported by any existing test.
Contribution rights
under Apache-2.0, and it is not owned or controlled by an employer, client, or other entity.
may control this contribution. I am requesting Vexa's private corporate-authorization process.
Every commit must also carry the contributor's own DCO
Signed-off-byline. Selecting theindependent path means no individual CLA is required.