diff --git a/docs/sip/authenticated-encryption-at-rest.md b/docs/sip/authenticated-encryption-at-rest.md index ea6cc8daf79..c95caf25255 100644 --- a/docs/sip/authenticated-encryption-at-rest.md +++ b/docs/sip/authenticated-encryption-at-rest.md @@ -110,7 +110,10 @@ Once the migrator and docs are in place, change the default to `"aes-gcm"` for the config comment and must be in `UPDATING.md`. - Recommended operator runbook: take a metadata-DB backup → run `re-encrypt-secrets --engine aes-gcm` → set - `SQLALCHEMY_ENCRYPTED_FIELD_ENGINE = "aes-gcm"` → restart. + `SQLALCHEMY_ENCRYPTED_FIELD_ENGINE = "aes-gcm"` → restart → re-run + `re-encrypt-secrets --engine aes-gcm` once more to sweep up any secrets a live + instance wrote as AES-CBC during the cutover window. The canonical, more + detailed version of this runbook lives in `UPDATING.md`; this is a summary. - `AesEngine` allows queryability over encrypted fields; AES-GCM does not. Any code that filters/queries on an encrypted column directly must be audited before Phase 3 (none is expected, but it must be verified). diff --git a/superset/utils/encrypt.py b/superset/utils/encrypt.py index ba60e2424d2..4043b7a99f4 100644 --- a/superset/utils/encrypt.py +++ b/superset/utils/encrypt.py @@ -131,9 +131,13 @@ class SQLAlchemyUtilsAdapter( # pylint: disable=too-few-public-methods # AES-CBC. An explicit ``engine`` kwarg (e.g. from the migrator) # always takes precedence. if "engine" not in kwargs: - engine_name = ( - app_config.get("SQLALCHEMY_ENCRYPTED_FIELD_ENGINE") - or DEFAULT_ENCRYPTION_ENGINE_NAME + # Only an *absent* key defaults to AES-CBC; a present value + # (even an empty string) is routed through the fail-closed + # resolver so a blanked-out config does not silently degrade to + # unauthenticated encryption. + engine_name = app_config.get( + "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE", + DEFAULT_ENCRYPTION_ENGINE_NAME, ) # ``**kwargs`` is loosely annotated as ``Optional[dict]`` here, so # route the resolved engine class through an ``Any`` local. diff --git a/tests/integration_tests/cli_tests.py b/tests/integration_tests/cli_tests.py index ee96bc0a384..e54d6152cd6 100644 --- a/tests/integration_tests/cli_tests.py +++ b/tests/integration_tests/cli_tests.py @@ -368,7 +368,7 @@ def test_re_encrypt_secrets_failure_exits_nonzero(app_context): assert response.exception is None or isinstance(response.exception, SystemExit) -def test_re_encrypt_secrets_engine_option_invokes_migrator(app_context): +def test_re_encrypt_secrets_engine_option_invokes_migrator(app_context) -> None: """ When --engine is provided, the CLI must resolve the engine name to the correct engine class and pass it to SecretsMigrator as target_engine. @@ -393,7 +393,7 @@ def test_re_encrypt_secrets_engine_option_invokes_migrator(app_context): assert call_kwargs.get("previous_secret_key") is None -def test_re_encrypt_secrets_engine_option_case_insensitive(app_context): +def test_re_encrypt_secrets_engine_option_case_insensitive(app_context) -> None: """ The --engine option must be case-insensitive per click.Choice(..., case_sensitive=False). @@ -416,7 +416,7 @@ def test_re_encrypt_secrets_engine_option_case_insensitive(app_context): assert migrator_mock.call_args.kwargs.get("target_engine") is AesGcmEngine -def test_re_encrypt_secrets_combined_key_rotation_and_engine(app_context): +def test_re_encrypt_secrets_combined_key_rotation_and_engine(app_context) -> None: """ --previous_secret_key and --engine combine in a single run: the migrator must receive both the previous key (for decryption) and the target engine @@ -443,7 +443,7 @@ def test_re_encrypt_secrets_combined_key_rotation_and_engine(app_context): assert call_kwargs.get("previous_secret_key") == "old-key" -def test_re_encrypt_secrets_engine_option_invalid_raises_usage(app_context): +def test_re_encrypt_secrets_engine_option_invalid_raises_usage(app_context) -> None: """ An unrecognized engine name must produce a click usage error, not a traceback or silent failure. diff --git a/tests/unit_tests/utils/encrypt_test.py b/tests/unit_tests/utils/encrypt_test.py index 2a191c6922c..ab1a67fb02f 100644 --- a/tests/unit_tests/utils/encrypt_test.py +++ b/tests/unit_tests/utils/encrypt_test.py @@ -93,6 +93,23 @@ def test_unknown_engine_raises_fail_closed() -> None: assert "aes-gcm" in message +def test_empty_engine_value_raises_fail_closed() -> None: + """A present-but-empty engine value fails closed instead of defaulting. + + Only an *absent* key falls back to AES-CBC. An empty string (e.g. a + blanked-out env var) must not silently degrade to unauthenticated CBC after + a GCM migration — it routes through the same fail-closed resolver as any + other unrecognized value. + """ + with pytest.raises( + ValueError, match="Unrecognized SQLALCHEMY_ENCRYPTED_FIELD_ENGINE" + ): + SQLAlchemyUtilsAdapter().create( + {**SECRET, "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE": ""}, + String(128), + ) + + def test_engine_name_is_normalized() -> None: """Engine names are case/separator-normalized to match the CLI's Choice.""" for name in ("AES-GCM", "aes_gcm", " Aes-Gcm "):