Addresses review feedback on the selectable encryption engine PR. Posture:
authenticated checks gate decisions; unauthenticated paths never silently win.
- Unknown engine name fails closed. An unrecognized
SQLALCHEMY_ENCRYPTED_FIELD_ENGINE now raises at field-construction (startup)
instead of silently degrading to unauthenticated AES-CBC. The value is
normalized (trim/lower/underscores→hyphens) to match the CLI's
case-insensitive click.Choice, and the offending value is kept out of the
error message (same clear-text-logging rationale as the CodeQL fix). Absent
key still defaults to "aes" (no behavior change for existing installs).
- Rollback (GCM→CBC) no longer mis-skips. The "already in target form" fast
path trusted any successful target decrypt — but unauthenticated CBC can
coincidentally decrypt a GCM blob, counting it skipped and leaving GCM
ciphertext that bricks on the next config flip while the run reports success.
When the target engine is unauthenticated, the value is now first probed
under any authenticated engine (current + previous key); an authenticated
decrypt wins. Generalized on "is the target authenticated", not engine names.
- De-dup the redundant previous-key decryptor when prev_key == SECRET_KEY (the
column's own engine is already decryptors[0]).
- Runbook: add the post-restart re-run step (sweeps secrets written CBC during
the migration window) and a quiet-window note (multi-worker cutover is not
zero-downtime).
Tests: flip the unknown-engine test to assert the fail-closed raise (and that
the value isn't leaked); add a normalization test; add the rollback
ordering-invariant test (authenticated probe wins over a forced-spurious CBC
skip); add combined key-rotation + engine-migration coverage at both the
migrator and CLI levels; hoist the function-local AesGcmEngine imports.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds a regression test for the reverse engine migration (--engine aes): an
AES-GCM value must be re-encrypted back to AES-CBC, not skipped. This guards
the idempotency fast-path, which decrypts in the target form first — since the
rollback target (AES-CBC) is unauthenticated, the test confirms it does not
mis-read GCM ciphertext and wrongly skip the value.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address bot review on #40654:
- SecretsMigrator now tries the previous key under both the column's
configured engine and the historical AES-CBC engine, so SECRET_KEY
rotation works for AES-GCM deployments (not just CBC) while still
reading CBC source data after an engine-flipped-first migration.
- re-encrypt-secrets --engine accepts case-insensitive engine names.
- Adapter logs a warning when SQLALCHEMY_ENCRYPTED_FIELD_ENGINE is
unrecognized before falling back to AES-CBC.
- config.py points operators at the SQLALCHEMY_ENCRYPTED_FIELD_ENGINE
knob as the preferred way to switch engines.
- Add unit test covering AES-GCM SECRET_KEY rotation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 2 of the authenticated-encryption-at-rest proposal. Teaches the
existing SecretsMigrator an engine-migration mode (decrypt with the source
engine under the current SECRET_KEY, re-encrypt with a target engine) and
exposes it via `superset re-encrypt-secrets --engine aes-gcm`. The run is
transactional and idempotent per column, so existing installs can move from
AES-CBC to authenticated AES-GCM without bricking stored secrets. Default
engine is unchanged ("aes"); behavior is opt-in. Adds UPDATING.md runbook and
updates the SIP to reflect Phases 1–2 shipping here.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
App-encrypted fields (DB passwords, SSH tunnel creds, OAuth tokens) use
sqlalchemy_utils EncryptedType, which defaults to AES-CBC — unauthenticated
encryption vulnerable to bit-flipping/tamper of ciphertext at rest. AES-GCM is
authenticated and fails closed on tampering.
Phase 1 (this change, backward compatible): add a configurable encryption
engine via SQLALCHEMY_ENCRYPTED_FIELD_ENGINE ("aes" | "aes-gcm"), defaulting to
"aes" so existing deployments are unchanged. The default SQLAlchemyUtilsAdapter
now honors it; an explicit engine kwarg still wins. New installs can opt into
AES-GCM with one line instead of writing a custom adapter.
Includes docs/sip/authenticated-encryption-at-rest.md describing the full
proposal: Phase 2 (a CBC->GCM re-encryption migrator built on SecretsMigrator)
and Phase 3 (flip default for fresh installs). The instance-wide re-encryption
migration is intentionally NOT included here — it is the subject of the SIP.
DRAFT: flipping the engine on a populated DB without the Phase 2 migrator makes
existing secrets undecryptable; this PR only adds the safe opt-in plumbing for
discussion.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
