From bc436bacad20765cc74183eda8d1dd9e2eb4b91d Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 11 Aug 2026 09:35:46 -0700 Subject: [PATCH] chore(security): warn when legacy encryption engine is configured (#42937) Co-authored-by: Amin Ghadersohi --- superset/config.py | 3 + superset/initialization/__init__.py | 59 +++++++++++++ .../check_encryption_engine_test.py | 88 +++++++++++++++++++ .../utils/test_encrypt_cbc_iv_reuse.py | 80 +++++++++++++++++ 4 files changed, 230 insertions(+) create mode 100644 tests/unit_tests/initialization/check_encryption_engine_test.py create mode 100644 tests/unit_tests/utils/test_encrypt_cbc_iv_reuse.py diff --git a/superset/config.py b/superset/config.py index fa87c7d8a42..56ac684c743 100644 --- a/superset/config.py +++ b/superset/config.py @@ -350,6 +350,9 @@ SQLALCHEMY_ENCRYPTED_FIELD_TYPE_ADAPTER = ( # pylint: disable=invalid-name # (database passwords, SSH tunnel credentials, OAuth tokens, ...) will make # those values undecryptable unless they are re-encrypted first. See the # authenticated-encryption SIP/migration before switching an existing install. +# Leaving this at "aes" logs a startup warning +# (SupersetAppInitializer.check_encryption_engine) pointing at the +# `superset re-encrypt-secrets --engine aes-gcm` migration path. SQLALCHEMY_ENCRYPTED_FIELD_ENGINE: Literal["aes", "aes-gcm"] = "aes" # Extends the default SQLGlot dialects with additional dialects diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 09e852c1b91..6fc6713dac5 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -1147,6 +1147,64 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods ) sys.exit(1) + def check_encryption_engine(self) -> None: + """Warn when app-encrypted fields use the legacy AES-CBC engine. + + ``SQLALCHEMY_ENCRYPTED_FIELD_ENGINE`` defaults to ``"aes"`` for backward + compatibility: every secret an existing install has ever written through + this mechanism (database passwords, SSH tunnel credentials, OAuth2 + tokens, and similar) is stored in that engine's ciphertext format, and + there is no per-value marker recording which engine produced it — the + engine is a single, global setting shared by every encrypted column. + + Unlike ``check_secret_key`` and its siblings, this never refuses to + start. ``"aes"`` is a working, still-supported configuration, not a + known-bad placeholder value: blocking startup on it would turn an + opt-in hardening step into a forced-migration outage for every + deployment that has not yet run the engine migration. It only warns, + on every boot, so operators have a documented path to the + authenticated ``"aes-gcm"`` engine (see ``superset re-encrypt-secrets`` + and ``docs/sip/authenticated-encryption-at-rest.md``). + """ + # pylint: disable=import-outside-toplevel + from sqlalchemy_utils.types.encrypted.encrypted_type import AesEngine + + from superset.utils.encrypt import ( + DEFAULT_ENCRYPTION_ENGINE_NAME, + resolve_encryption_engine, + ) + + engine_name = self.config.get( + "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE", DEFAULT_ENCRYPTION_ENGINE_NAME + ) + try: + engine_cls = resolve_encryption_engine(engine_name) + except ValueError: + # An unrecognized value already fails closed at field construction + # (see ``resolve_encryption_engine``); nothing more to warn about. + return + if engine_cls is not AesEngine: + return + self._log_config_warning( + "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE is set to the legacy 'aes' " + "engine (AES-CBC, unauthenticated). App-encrypted fields — " + "database passwords, SSH tunnel credentials, OAuth2 tokens, and " + "similar — would benefit from the authenticated 'aes-gcm' engine " + "instead.\n" + "Switching engines on a populated database requires " + "re-encrypting existing values first, since the two ciphertext " + "formats are not interchangeable:\n" + " 1. Back up the metadata database.\n" + " 2. superset re-encrypt-secrets --engine aes-gcm\n" + " 3. Set SQLALCHEMY_ENCRYPTED_FIELD_ENGINE = 'aes-gcm' in " + "superset_config.py.\n" + " 4. Restart Superset, then re-run the command above once more " + "to sweep up any values written during the cutover.\n" + "See UPDATING.md and " + "docs/sip/authenticated-encryption-at-rest.md for the full " + "runbook." + ) + def configure_session(self) -> None: if self.config["SESSION_SERVER_SIDE"]: Session(self.superset_app) @@ -1269,6 +1327,7 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods self.configure_feature_flags() self.check_guest_token_secret() self.check_async_query_secret() + self.check_encryption_engine() self.configure_db_encrypt() self.setup_db() diff --git a/tests/unit_tests/initialization/check_encryption_engine_test.py b/tests/unit_tests/initialization/check_encryption_engine_test.py new file mode 100644 index 00000000000..ff3ce93d585 --- /dev/null +++ b/tests/unit_tests/initialization/check_encryption_engine_test.py @@ -0,0 +1,88 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Unit tests for the legacy-encryption-engine startup warning.""" + +from typing import Any +from unittest.mock import patch + +from superset.initialization import SupersetAppInitializer + + +def _make_initializer(config: dict[str, Any]) -> SupersetAppInitializer: + """Build a bare initializer with just the attributes the check needs.""" + initializer = SupersetAppInitializer.__new__(SupersetAppInitializer) + initializer.config = config + return initializer + + +def test_warns_when_engine_unset_defaults_to_legacy_aes() -> None: + """An absent config value resolves to the legacy engine and warns.""" + initializer = _make_initializer({}) + + with patch.object(initializer, "_log_config_warning") as log_warning: + initializer.check_encryption_engine() + + log_warning.assert_called_once() + message = log_warning.call_args.args[0] + assert "aes-gcm" in message + assert "re-encrypt-secrets" in message + + +def test_warns_when_engine_explicitly_set_to_aes() -> None: + """An explicit 'aes' value warns the same as the implicit default.""" + initializer = _make_initializer({"SQLALCHEMY_ENCRYPTED_FIELD_ENGINE": "aes"}) + + with patch.object(initializer, "_log_config_warning") as log_warning: + initializer.check_encryption_engine() + + log_warning.assert_called_once() + + +def test_silent_when_engine_is_gcm() -> None: + """An operator who has already opted into 'aes-gcm' gets no warning.""" + initializer = _make_initializer({"SQLALCHEMY_ENCRYPTED_FIELD_ENGINE": "aes-gcm"}) + + with patch.object(initializer, "_log_config_warning") as log_warning: + initializer.check_encryption_engine() + + log_warning.assert_not_called() + + +def test_silent_when_engine_value_is_unrecognized() -> None: + """An unrecognized value already fails closed at field construction + (``resolve_encryption_engine``); this check does not pile on a second, + redundant warning for the same misconfiguration. + """ + initializer = _make_initializer({"SQLALCHEMY_ENCRYPTED_FIELD_ENGINE": "bogus"}) + + with patch.object(initializer, "_log_config_warning") as log_warning: + initializer.check_encryption_engine() + + log_warning.assert_not_called() + + +def test_never_raises_system_exit() -> None: + """Unlike check_secret_key/check_guest_token_secret/check_async_query_secret, + this check must never refuse to start: the legacy engine is a supported + configuration, not a known-bad placeholder, so blocking startup on it + would turn an opt-in hardening step into a forced-migration outage. + """ + initializer = _make_initializer({}) + + with patch.object(initializer, "_log_config_warning"): + # Should not raise SystemExit. + initializer.check_encryption_engine() diff --git a/tests/unit_tests/utils/test_encrypt_cbc_iv_reuse.py b/tests/unit_tests/utils/test_encrypt_cbc_iv_reuse.py new file mode 100644 index 00000000000..ad208813755 --- /dev/null +++ b/tests/unit_tests/utils/test_encrypt_cbc_iv_reuse.py @@ -0,0 +1,80 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Characterization tests for the IV behavior of Superset's default +app-encryption engine (``sqlalchemy_utils`` ``AesEngine``, AES-CBC). + +``AesEngine._initialize_engine`` (see +``sqlalchemy_utils.types.encrypted.encrypted_type``) derives its IV as the +first 16 bytes of the SHA-256 digest of the configured key: ``self.iv = +self.secret_key[:16]``. That derivation is a pure function of the key alone, +so every encryption performed under the same key reuses the same IV, unlike +``AesGcmEngine``, which samples a fresh random IV (``os.urandom``) on every +call and embeds it in the output. These tests pin that observable difference: +same-plaintext encryptions are identical under the default engine and distinct +under the GCM engine. +""" + +from sqlalchemy import String +from sqlalchemy.engine import make_url + +from superset.utils.encrypt import ( + DEFAULT_ENCRYPTION_ENGINE_NAME, + EncryptedType, + resolve_encryption_engine, +) + +DIALECT = make_url("sqlite://").get_dialect() +SECRET_KEY = "k" * 32 + + +def _field(engine: type) -> EncryptedType: + return EncryptedType(String(1024), key=lambda: SECRET_KEY, engine=engine) + + +def test_default_encryption_engine_name_resolves_to_cbc() -> None: + """The engine name the codebase falls back to when config is unset resolves + to the unauthenticated AES-CBC engine, not the authenticated AES-GCM one. + """ + from sqlalchemy_utils.types.encrypted.encrypted_type import AesEngine + + assert resolve_encryption_engine(DEFAULT_ENCRYPTION_ENGINE_NAME) is AesEngine + + +def test_default_engine_repeats_ciphertext_for_repeated_plaintext() -> None: + """Encrypting the same plaintext twice under one key produces identical + ciphertext with the default engine, because its IV is a deterministic + function of the key rather than freshly sampled per call. + """ + field = _field(resolve_encryption_engine(DEFAULT_ENCRYPTION_ENGINE_NAME)) + + first = field.process_bind_param("hunter2", DIALECT) + second = field.process_bind_param("hunter2", DIALECT) + + assert first == second + + +def test_gcm_engine_varies_ciphertext_for_repeated_plaintext() -> None: + """Contrast case: the authenticated engine samples a fresh random IV each + call, so encrypting the same plaintext twice under one key produces + different ciphertext. + """ + field = _field(resolve_encryption_engine("aes-gcm")) + + first = field.process_bind_param("hunter2", DIALECT) + second = field.process_bind_param("hunter2", DIALECT) + + assert first != second