rusackas commented on PR #40654: URL: https://github.com/apache/superset/pull/40654#issuecomment-4674956404
Thanks @richardfogaca — really sharp review. Addressed the design items in 10c49b6 under one posture: **authenticated checks gate decisions; unauthenticated paths never silently win.** **Unknown engine → fail-closed** (`encrypt.py:86-97`). Agreed this was the wrong default for a crypto selector. An unrecognized `SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` now raises at field construction instead of degrading to CBC, and the value is case/separator-normalized (`aes_gcm`/`AES-GCM` → `aes-gcm`) to match the CLI's `click.Choice`. Kept the offending value out of the error message (same clear-text-logging rationale as the CodeQL fix in 731a476) but list the valid engines. Absent key still defaults to `aes` so existing installs are untouched. Mirrors house precedent (refusing to boot on a default SECRET_KEY) and kills the post-migration split-brain you flagged. **Rollback spurious-skip → fixed, not documented** (`encrypt.py:343-349`). You're right that an unauthenticated CBC "success" shouldn't classify a value as migrated. When the target engine is unauthenticated, the migrator now probes the value under any authenticated engine (current + previous key) *first*; an authenticated decrypt wins and the value is re-encrypted rather than skipped. Generalized on *is the target authenticated* rather than hardcoding engine names. Since the coincidental CBC-decrypt-of-GCM can't be crafted deterministically, the new test pins the **ordering invariant** (forces the target read to succeed, asserts re-encrypt not skip). Explicitly declined the "document as best-effort" alternative — a run reporting success while leaving rows that brick on the next flip violates the migrator's own transactional/idempotent contract. **Runbook** (`UPDATING.md`): added the post-restart re-run step (sweeps secrets written CBC during the migration window) and a sentence that multi-worker cutover has a brief decrypt-outage and isn't zero-downtime. **Smaller items**: combined key-rotation + engine-migration is now covered at both the migrator (`encrypt_test.py`) and CLI (`cli_tests.py`) levels; de-duped the redundant previous-key decryptor when `prev_key == SECRET_KEY`; tightened the engine-list type hint; hoisted the function-local `AesGcmEngine` imports. Two I left as follow-ups rather than widen this PR, l-mk if you'd rather I fold them in: - **`discover_encrypted_fields` custom-adapter no-op** (`encrypt.py:204`): a non-default `SQLALCHEMY_ENCRYPTED_FIELD_TYPE_ADAPTER` yields a silent 0-column migration. Pre-existing, but worth a warn/refuse — happy to file an issue. - The `app_config[...]` vs `.get(...,"aes")` default: kept `.get` so configs that omit the key still work, but the silent `ENCRYPTION_ENGINES.get(..., AesEngine)` fallback is gone, so the default now lives in just config.py + the documented `DEFAULT_ENCRYPTION_ENGINE_NAME`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
