Repository navigation
fix(security): encrypt credential-bearing system_settings rows at rest (ent#435, CWE-312) - #2330
Conversation
…t (ent#435, CWE-312) Six `system_settings` rows held LIVE third-party credentials in cleartext — `anthropic_api_key`, `github_pat`, `google_api_key`, `slack_app_token`, `slack_client_secret`, `slack_signing_secret` — while Architectural Invariant #12's own table read as though everything was covered. Every DB dump, backup, replica and snapshot therefore carried usable tokens (backups being exactly the artifact most likely to travel), and any read path to the database yielded working credentials WITHOUT needing CREDENTIAL_ENCRYPTION_KEY. `platform-settings.md` was worse than silent about it: security consideration #5 advertised the defect as a control ("stored in SQLite ... encrypted at rest if filesystem supports it"), which protects a powered-off disk and nothing that moves. That bullet is rewritten to say what was actually true and why. The key NAME moves, not just the value. Each secret becomes an AES-256-GCM envelope under `<key>_encrypted` and the cleartext row is DELETED. Leaving an encrypted value under the original name would keep "is this install encrypted?" unanswerable by inspection — which IS the reported defect, not a cosmetic detail. With the rename, the reporter's own verification query returning nothing is the proof, and the sink guard keeps it true. The read path lazily migrates. A one-shot migration converts disk once, but a restored pre-fix backup, a rollback-then-roll-forward, or a direct DB write can put cleartext back and only the reader would notice. `_resolve_secret_setting` resolves encrypted -> legacy-encrypted-and-deleted-on-sight -> env -> "", so cleartext is transient by construction rather than merely absent right now. Steady state is unchanged: while the encrypted row exists the legacy key is never read (pinned by a byte-equality test — a rewrite would change the nonce). Fail directions are deliberately opposite. Read fails OPEN to env (an unreadable envelope must not 500 the agent-start path) but never down to a stale legacy row, which would resurrect a credential the operator had replaced. Write fails CLOSED: no encryption key means refuse, never silently store cleartext. The guard is at the SINK, not only on the routes. `db.set_setting` raises `SecretSettingWriteError` (mapped to 422) for a registered secret key or any merely credential-shaped key (`*_api_key`/`*_token`/`*_secret`/`*_pat`/ `*_password`/`*_credentials`). Two reasons it cannot live on the routes alone: `system_settings` has four writers, and the generic `PUT /api/settings/{key}` catch-all can address ANY key — the same door #506, #1609, ent#12, #1644, ent#14 and ent#346 each found standing open. The explicit set gives a precise refusal with a pointer; the shape heuristic catches the NEXT secret nobody registers. Writing a `*_encrypted` key raw is refused too, since a hand-pasted string would land as a row every reader then fails to decrypt (the #736 rationale). `slack_client_id` is a reasoned exemption, not an omission: an OAuth client_id is a public identifier that `slack_service.get_oauth_url` puts verbatim into the browser-visible authorize URL (the `whatsapp_bindings.account_sid` "(public)" precedent). Recorded with its reason in `PUBLIC_CREDENTIAL_SHAPED_KEYS` so a later reader can tell reviewed from overlooked, and pinned by a test. Dual-track per Invariant #9: `secret_settings_encryption` (SQLite) and Alembic `0041_secret_settings_encryption` (PostgreSQL — the backend the defect was reported on, so not the afterthought). Both call ONE `plan_migration`: the two drivers cannot share SQL, but they must not disagree on policy. Hard-fails on a missing encryption key only when there is something to encrypt, so a fresh install still boots (the #453 choice, refined). `downgrade()` is a deliberate no-op — the honest inverse is "write these live credentials back in cleartext". Found in passing and fixed with it: `rotate-credential-key.py` sweeps COLUMNS, so `elevenlabs_api_key_encrypted` (ent#117) and `a2a_outbound_endpoints_encrypted` (#736) — envelope-in-a-ROW — were invisible to every key rotation, leaving them readable only via the secondary key and dead the moment it was removed. It gains a row-keyed `system_settings` pass whose membership derives from the policy module, so a newly-registered credential setting joins rotation automatically. Encryption protects the database going forward only. Historical backups still hold the plaintext, so the runbook's headline instruction is ROTATE: docs/migrations/SECRET_SETTINGS_ENCRYPTION_2026-08.md. Guards: tests/unit/test_ent435_settings_sink_guard.py (AST — every `system_settings` writer is gated or listed with the reason it cannot carry a credential; OSS tree only, the private submodule owns its twin per #1677). Related to Abilityai/trinity-enterprise#435
… empty envelope (ent#435) Found while testing the ent#435 encryption change: `set_secret_setting(key, "")` stored an AES-256-GCM envelope OF an empty string. Resolution was unaffected — the decrypted `""` is falsy, so every reader fell through to the env var exactly as before — but the row's mere existence made `has_secret_setting` true, so the `source: settings|env` field on the admin status endpoints reported "settings" for a credential that actually resolves from the environment. Pre-ent#435 a blank write stored `''`, which readers treated as falsy and fell through the same way, and `bool(db.get_setting_value(...))` on `''` was False — so "settings" was never reported. Clearing instead of storing preserves the original resolution AND the original source reporting, which makes this a divergence introduced by ent#435 rather than a pre-existing behaviour. Related to Abilityai/trinity-enterprise#435
dolho
changed the title
dolho
changed the title
…or (ent#435) The ent#435 migration refuses to boot when `CREDENTIAL_ENCRYPTION_KEY` is unset AND there are cleartext credentials to encrypt. That refusal is correct — the alternative is booting with the credentials still readable in every dump — but it surfaced as the generic `CredentialEncryptionService` message, which says what to set and nothing about why it suddenly matters. Almost no install can hit this: `start.sh` has auto-generated the key via `ensure_hex32_secret` since v0.6.0 (2026-06-01), three weeks BEFORE it started generating `AGENT_AUTH_SECRET`, so any install with a working agent fleet provably has one. The gap is an install provisioned by a bare `docker compose up` — compose defaults the variable to empty and `.env.example` ships it blank — that ALSO configured credentials through the UI. For that operator the platform has always worked, so a bare "encryption key not configured" gives no clue that an upgrade is what changed. `MissingEncryptionKeyError` now names ent#435, states why refusing to start is deliberate, gives the one-line fix, and points at the runbook. It subclasses ValueError, so the migration runner's failure path and any `except ValueError` caller are unchanged; the failing credential's VALUE never appears in the message (asserted). Verified at container level: exit 1, full message in the log, cleartext row preserved. Runbook gains the same provenance so the rare case is diagnosable from the docs as well as the log. Related to Abilityai/trinity-enterprise#435
…ings.py load CI's regression-diff caught this: the 4 `test_telegram_webhook_backfill` tests passed on base and failed on head. That test loads `routers/settings.py` as a standalone module against a hand-built `services.settings_service` stub whose attributes are enumerated explicitly — the stub is extended per-issue by convention (#1310, #1129, #506, #1609, ent#236). ent#435 added `set_secret_setting` / `clear_secret_setting` / `has_secret_setting` to that router's module-level import, so the load raised `ImportError: cannot import name 'set_secret_setting'`. Proven both directions under CI's ordering (a prior test must seed `services.telemetry_sharing_service` into sys.modules for this file to load at all — a pre-existing order-dependency, unrelated): without the stub entries 4 fail with that ImportError, with them 14 pass. Worth recording why this was not caught locally: in a Python 3.12 venv these 4 fail on BOTH base and head for an unrelated reason (`cannot import name 'telemetry_sharing_service'`), so a local base-vs-head comparison showed "identical, pre-existing" and hid a genuine head-only regression. The 3.13 image that matches CI is the environment that distinguishes them. Related to Abilityai/trinity-enterprise#435
ReviewReviewed the diff and the CI signal. Green and ready — with one genuine regression found and fixed during review, plus a correction to something I claimed earlier. CI caught a real regression I had misdiagnosed
I had previously reported these as "pre-existing, identical on clean Root cause: that test loads Fixed in CI status21 success / 4 skipped / 1 cancelled. All three head pytest shards pass; Review findingsNo stragglers. Grepped both trees for any remaining direct read or write of the six legacy keys — Frontend is untouched and slightly better off. No files changed under
Known residual, flagged not hiddenWith the encryption key missing, the admin status endpoint reports Also unchanged and deliberate: |
dolho
changed the title
Re-review (post-fix)Green on this repo, and the re-read turned up one more thing I had stated wrongly — see the merge-order note at the end. CI on
|
…per row Found on re-review: both migration docstrings claimed "write-then-delete per row, so a crash mid-sweep leaves cleartext intact". The safety conclusion was right but the mechanism described was not — the SQLite sweep issues a single `conn.commit()` after the loop, and the Alembic revision runs inside Alembic's own migration transaction. Both are all-or-nothing across every row. That distinction matters more than a wording nit: single-transaction is the STRONGER property (a crash rolls back rather than leaving three of six credentials converted, and the runner never records the migration as applied), so a future reader trusting the old docstring could "fix" the loop into a per-row commit and quietly trade atomicity for a partially-converted state whose only recovery is the read-path lazy migration. Docstrings now describe what the code does and say explicitly not to make that change. `test_sqlite_sweep_is_atomic_across_rows` pins it: failing the DELETE of the second row leaves all three cleartext rows intact and zero encrypted rows, and a clean re-run converges completely — so the property is asserted rather than asserted-in-prose. The per-row, two-transaction description on the SERVICE path (`_migrate_legacy_secret_setting`) is accurate and unchanged — that one really is two separate db calls, deliberately, because it runs on a read path. Related to Abilityai/trinity-enterprise#435
Re-review #3The merge-order fix was documentation only, so the code was unchanged — which meant this pass had to look somewhere it hadn't. It did, and it found one more real thing. Finding: the migration docstrings misdescribed their own transaction semanticsBoth migrations claimed "write-then-delete per row, so a crash mid-sweep leaves cleartext intact". The conclusion was right; the mechanism was not. The SQLite sweep issues a single That is not a wording nit, because single-transaction is the stronger property: a crash rolls back rather than leaving an install with three of six credentials converted, and the runner never records the migration as applied. A future reader trusting the old docstring could "fix" the loop into a per-row commit and quietly trade atomicity for a partially-converted state whose only recovery is the read-path lazy migration. Fixed in The per-row, two-transaction description on the service path ( CI on
|
Two conflicts, both from dev moving ahead by 12 commits: 1. db/migrations.py MIGRATIONS list — both sides appended. Kept BOTH, with dev's two entries first and this branch's last, so the runner applies them in the order they landed. 2. Alembic multi-head — dev added 0039_operator_queue_addressed_to and 0040_rl_events_failure_kind while this branch carried its own 0039 off the same 0038 parent. Two heads make `alembic upgrade head` apply ZERO revisions (Invariant #3), so every revision since the fork silently stops arriving. Renamed 0039_agent_ownership_operator_resume -> 0041_… and re-parented it onto 0040_rl_events_failure_kind. check_alembic_heads now reports one head. NOTE: PR #2330 also claims 0041 (0041_secret_settings_encryption, same 0040 parent). Whichever of the two merges second must re-parent onto the other — scripts/ci/check_alembic_heads.py is a required, unconditional gate, so it fails loudly rather than shipping a silently-inert migration chain.
Heads-up: Alembic
|
vybe
left a comment
vybe
left a comment
There was a problem hiding this comment.
Validated via /validate-pr: security scans clean, dual-track migration present (SQLite secret_settings_encryption + Alembic 0041), docs updated (architecture.md, feature-flows/platform-settings.md, requirements/credentials.md, migration runbook), regression tests present, all CI green incl. pg-migrations and schema-parity.
Merging FIRST of the three PRs that each claim 0041 off 0040_rl_events_failure_kind — #2312 and #2331 will be re-parented onto 0041_secret_settings_encryption before they merge. Cross-tracker ent#435 needs a manual status-in-dev bump.
…in, and a tier it does not reach fails the run (#2888) (#2893) Fixes #2888 The unclassified-directory guard globbed `tests/*/` after the script had already `cd`'d into `tests/`, so it looked for `tests/tests/*/`, matched nothing, stayed a literal `*` under default globbing, failed classification and `exit 1`'d the run at the api boundary. Every full-suite run since a646de8 (2026-08-27) stopped before the `api`, `standalone` and `postgres` tiers — ~1,900 tests — and nothing said so. - The guard is now `tests/harness/check_test_dirs.py`, handed TESTS_DIR explicitly instead of globbing relative to the cwd, and testable: only a directory that HOLDS test files is a finding (`__pycache__`, `reports/`, a stale local checkout with only `node_modules` are on every machine and collected by nothing), and an owner entry with no directory on disk is flagged too (pytest ignores a missing `--ignore=` path silently). - Tier ledger: DECLARED_TIERS is checked against the recorded rows at the end. A declared tier with no row that the caller did not deselect is reported `NEVER RAN` and fails the run; deselected tiers (`--tier`, `--no-pg`) are listed as SKIP and the verdict says `partial`. - Abort trap: any exit before the summary — including SIGINT/SIGTERM — prints an ABORTED banner naming the tiers left unrun and exits non-zero. The disposable postgres container is torn down on that path too (the old per-tier `trap ... EXIT` is folded into it). - The alembic step of the postgres tier now carries the same dummy Redis/encryption env pg-migrations.yml sets: revision 0041 (#2330) imports `services` → `config`, which raises without Redis credentials. Broken since 2026-08-20, a week before the tier stopped being reached. Guards in tests/unit/test_2080_harness_contract.py: the glob is matched as code (not the comment that names it), the guard passes on the real tree with the real owner lists, fires on an unwired directory holding tests, ignores test-less directories, every `run_tier` name is in DECLARED_TIERS, and the NEVER-RAN / abort paths are present. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Fixes the CWE-312 finding in Abilityai/trinity-enterprise#435. Filed on the private tracker per disclosure policy; the write paths are OSS and backend-agnostic, so the fix lands here. OSS-core by decision — no
requires_entitlement, logic stays in the OSS tree (recorded explicitly, since CLAUDE.md's default for an enterprise-tracker issue is gated unless ruled otherwise).The defect
Six
system_settingsrows held live third-party credentials in cleartext:anthropic_api_key,github_pat,google_api_key,slack_app_token,slack_client_secret,slack_signing_secret.Meanwhile Architectural Invariant #12's own table read as though everything was covered. So every DB dump, backup, replica and snapshot carried usable tokens — backups being exactly the artifact most likely to travel — and any read path to the database yielded working credentials without needing
CREDENTIAL_ENCRYPTION_KEY.platform-settings.mdwas worse than silent about it. Security consideration #5 advertised the defect as a control:Filesystem encryption protects a powered-off disk and nothing that moves. That bullet is rewritten to say what was actually true and why.
Design decisions worth reviewing
The key NAME moves, not just the value. Each secret becomes an AES-256-GCM envelope under
<key>_encryptedand the cleartext row is deleted. Leaving an encrypted value under the original name would keep "is this install encrypted?" unanswerable by inspection — which is the reported defect, not a cosmetic detail. With the rename, the reporter's own verification query returning nothing is the proof, and the sink guard keeps it true.The read path lazily migrates. A one-shot migration converts disk once, but a restored pre-fix backup, a rollback-then-roll-forward, or a direct DB write can put cleartext back — and only the reader would notice.
_resolve_secret_settinggoesencrypted → legacy-cleartext (encrypted-and-deleted on sight) → env → '', making cleartext transient by construction rather than merely absent right now. Steady state is unchanged: while the encrypted row exists the legacy key is never read (pinned by a byte-equality test — a rewrite would change the nonce).Fail directions are deliberately opposite. Read fails open to env (an unreadable envelope must not 500 the agent-start path) but never down to a stale legacy row, which would resurrect a credential the operator had replaced. Write fails closed: no encryption key means refuse, never silently store cleartext.
The guard is at the SINK, not only on the routes.
db.set_settingraisesSecretSettingWriteError(mapped to 422) for a registered secret key or any merely credential-shaped key (*_api_key/*_token/*_secret/*_pat/*_password/*_credentials). Two reasons it cannot live on the routes alone:system_settingshas four writers, and the genericPUT /api/settings/{key}catch-all can address any key — the same door #506, #1609, ent#12, #1644, ent#14 and ent#346 each found standing open. The explicit set gives a precise refusal with a pointer; the shape heuristic catches the next secret nobody registers.slack_client_idis a reasoned exemption, not an omission. An OAuth client_id is a public identifier thatslack_service.get_oauth_urlputs verbatim into the browser-visible authorize URL — thewhatsapp_bindings.account_sid"(public)" precedent. Recorded with its reason inPUBLIC_CREDENTIAL_SHAPED_KEYSand pinned by a test, so a later reader can tell reviewed from overlooked. Happy to encrypt it anyway if you disagree.Dual-track per Invariant #9:
secret_settings_encryption(SQLite) + Alembic0041(PostgreSQL — the backend the defect was reported on, so not the afterthought). Both call oneplan_migration: the two drivers cannot share SQL, but they must not disagree on policy. Hard-fails on a missing encryption key only when there is something to encrypt, so a fresh install still boots.downgrade()is a deliberate no-op — the honest inverse is "write these live credentials back in cleartext".Found in passing, fixed with it
scripts/deploy/rotate-credential-key.pysweeps columns, soelevenlabs_api_key_encrypted(ent#117) anda2a_outbound_endpoints_encrypted(#736) — envelope-in-a-row — were invisible to every key rotation. A "completed" rotation left them readable only via the secondary key, and dead the moment it was removed. Pre-existing, unrelated to the six rows, same shape. Now swept, with membership derived from the policy module so a newly-registered credential setting joins rotation automatically.Operator impact — rotation is the remediation
Encryption protects the database going forward only. Historical backups still hold the plaintext, and row deletion does not guarantee byte-scrubbing (SQLite freed pages, PG dead tuples). The runbook's headline instruction is therefore rotate:
docs/migrations/SECRET_SETTINGS_ENCRYPTION_2026-08.md.Verification
Beyond unit tests, exercised as a real system:
src/backendbind-mounted — not the image: migrated 0038 → 0041 across three revisions, reporter's query returns nothing, zero plaintext left, all 13 original settings and all 7 agent rows intact.upgrade_to_head, idempotent re-run, downgrade + re-upgrade safe.CREDENTIAL_ENCRYPTION_KEYwith credentials present → container exits 1 with an actionable message, cleartext preserved, nothing destroyed; supplying the key boots and migrates.pg_dumpand a SQLite backup artifact both contain zero plaintext.GET /api/settingsleaks nothing.Tests: 39 new (36 OSS + 3 enterprise twin). Targeted regressions 278 passed. Full unit suite: 11930 passed, 20 failed — all 20 verified identical on clean
origin/dev(15 are a Python 3.12-vs-3.13str()difference for IPv4-mapped IPv6 addresses; my local venv is 3.12, CI is 3.13. 4 are a pre-existing test-stubImportError. 1 istest_1920flaggingenterprise/backend/siem/exporter.py, present only because I checked the submodule out — public CI does not).Anti-recurrence:
tests/unit/test_ent435_settings_sink_guard.py(AST — everysystem_settingswriter is gated or listed with the reason it cannot carry a credential).Companion PR
The two enterprise
system_settingssinks carry the same guard plus a private twin of the AST test, per the #1677 caller-parity convention: Abilityai/trinity-enterprise#436. Both are inert for their fixed non-credential keys; they exist so the next key added there cannot be a secret.Merge order: this PR FIRST, then ent#436, then the submodule pointer bump. (Corrected — I originally wrote it the other way round.) The enterprise guard imports
services.secret_settings, which this PR introduces, and trinity-enterprise CI checks out OSSdev— so ent#436 is red withModuleNotFoundError: No module named 'services.secret_settings'until this lands, and goes green on a re-run afterwards. That import is deliberately not wrapped in atry/except ImportError: silently degrading would disable the guard on exactly the tree where it matters. This PR does not bump the submodule pointer.Related to Abilityai/trinity-enterprise#435
🤖 Generated with Claude Code