rusackas commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3407953153
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +366,94 @@ def
test_re_encrypt_secrets_failure_exits_nonzero(app_context):
# The failure path must be handled by the CLI, not leaked as an
# uncaught exception.
assert response.exception is None or isinstance(response.exception,
SystemExit)
+
+
+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.
+ """
+ current_app.config.pop("PREVIOUS_SECRET_KEY", None)
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update,
+ "SecretsMigrator",
+ ) as migrator_mock:
+ migrator_mock.return_value.run.return_value = (
+ superset.utils.encrypt.ReEncryptStats()
+ )
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "aes-gcm"],
+ )
+
+ assert response.exit_code == 0
+ call_kwargs = migrator_mock.call_args.kwargs
+ assert call_kwargs.get("target_engine") is AesGcmEngine
+ assert call_kwargs.get("previous_secret_key") is None
+
+
+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).
+ """
+ current_app.config.pop("PREVIOUS_SECRET_KEY", None)
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update,
+ "SecretsMigrator",
+ ) as migrator_mock:
+ migrator_mock.return_value.run.return_value = (
+ superset.utils.encrypt.ReEncryptStats()
+ )
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "AES-GCM"],
+ )
+
+ assert response.exit_code == 0
+ assert migrator_mock.call_args.kwargs.get("target_engine") is AesGcmEngine
+
+
+def test_re_encrypt_secrets_combined_key_rotation_and_engine(app_context) ->
None:
Review Comment:
Annotated `app_context` as `AppContext`.
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +366,94 @@ def
test_re_encrypt_secrets_failure_exits_nonzero(app_context):
# The failure path must be handled by the CLI, not leaked as an
# uncaught exception.
assert response.exception is None or isinstance(response.exception,
SystemExit)
+
+
+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.
+ """
+ current_app.config.pop("PREVIOUS_SECRET_KEY", None)
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update,
+ "SecretsMigrator",
+ ) as migrator_mock:
+ migrator_mock.return_value.run.return_value = (
+ superset.utils.encrypt.ReEncryptStats()
+ )
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "aes-gcm"],
+ )
+
+ assert response.exit_code == 0
+ call_kwargs = migrator_mock.call_args.kwargs
+ assert call_kwargs.get("target_engine") is AesGcmEngine
+ assert call_kwargs.get("previous_secret_key") is None
+
+
+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).
+ """
+ current_app.config.pop("PREVIOUS_SECRET_KEY", None)
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update,
+ "SecretsMigrator",
+ ) as migrator_mock:
+ migrator_mock.return_value.run.return_value = (
+ superset.utils.encrypt.ReEncryptStats()
+ )
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "AES-GCM"],
+ )
+
+ assert response.exit_code == 0
+ assert migrator_mock.call_args.kwargs.get("target_engine") is AesGcmEngine
+
+
+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
+ (for re-encryption). This is the mode most likely to regress, since the
+ single-option tests each pin only the other's variable.
+ """
+ current_app.config.pop("PREVIOUS_SECRET_KEY", None)
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update,
+ "SecretsMigrator",
+ ) as migrator_mock:
+ migrator_mock.return_value.run.return_value = (
+ superset.utils.encrypt.ReEncryptStats()
+ )
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--previous_secret_key", "old-key", "--engine", "aes-gcm"],
+ )
+
+ assert response.exit_code == 0
+ call_kwargs = migrator_mock.call_args.kwargs
+ assert call_kwargs.get("target_engine") is AesGcmEngine
+ assert call_kwargs.get("previous_secret_key") == "old-key"
+
+
+def test_re_encrypt_secrets_engine_option_invalid_raises_usage(app_context) ->
None:
Review Comment:
Annotated `app_context` as `AppContext`.
##########
superset/utils/encrypt.py:
##########
@@ -19,17 +19,76 @@
from dataclasses import dataclass
from typing import Any, Optional
-from flask import Flask
+from flask import current_app, Flask
from flask_babel import lazy_gettext as _
from sqlalchemy import Table, text, TypeDecorator
from sqlalchemy.engine import Connection, Dialect, Row
from sqlalchemy_utils import EncryptedType as SqlaEncryptedType
+from sqlalchemy_utils.types.encrypted.encrypted_type import (
+ AesEngine,
+ AesGcmEngine,
+ EncryptionDecryptionBaseEngine,
+)
class EncryptedType(SqlaEncryptedType):
cache_ok = True
+# Named encryption engines selectable via the
``SQLALCHEMY_ENCRYPTED_FIELD_ENGINE``
+# config. "aes" (AES-CBC) is the historical default; "aes-gcm" is authenticated
+# encryption (recommended for new deployments). NOTE: switching an existing
+# deployment from "aes" to "aes-gcm" requires re-encrypting all stored secrets
+# first — see the SIP referenced in the docs. Changing this on a populated
+# database without that migration will make existing secrets undecryptable.
+ENCRYPTION_ENGINES: dict[str, type[EncryptionDecryptionBaseEngine]] = {
+ "aes": AesEngine,
+ "aes-gcm": AesGcmEngine,
+}
+
+# The historical fallback engine when the config does not name one.
+DEFAULT_ENCRYPTION_ENGINE_NAME = "aes"
+
+# Engines whose ciphertext is authenticated: a successful decrypt is
+# cryptographic proof the value is genuinely in that form. AES-GCM carries an
+# authentication tag; AES-CBC does not, so a CBC "success" can be coincidental.
+# Classification logic (the migrator's idempotency fast path) must let an
+# authenticated decrypt win over an unauthenticated one, never the reverse.
+AUTHENTICATED_ENGINES: frozenset[type[EncryptionDecryptionBaseEngine]] =
frozenset(
+ {AesGcmEngine}
+)
+
+
+def _is_authenticated_engine(engine: type[EncryptionDecryptionBaseEngine]) ->
bool:
+ return engine in AUTHENTICATED_ENGINES
+
+
+def resolve_encryption_engine(engine_name: str) ->
type[EncryptionDecryptionBaseEngine]:
+ """Resolve a configured engine name to its engine class, fail-closed.
+
+ The value is normalized (trimmed, lower-cased, underscores → hyphens) so it
+ matches the case-insensitive CLI ``click.Choice``. An unrecognized name
+ raises so a misconfiguration fails at field-construction (startup) rather
+ than silently degrading to unauthenticated AES-CBC — which would let an
+ operator who typo'd ``"aes_gcm"`` believe they had authenticated
encryption,
+ and, after a GCM migration, write new secrets as CBC into a GCM database.
+
+ The offending value is deliberately kept out of the error message: it comes
+ from the app config (which also holds ``SECRET_KEY``), and static analysis
+ flags interpolating config-sourced values into logs/errors as potential
+ clear-text secret exposure. The set of valid engine names is enough to
+ diagnose a typo.
+ """
+ normalized = engine_name.strip().lower().replace("_", "-")
Review Comment:
Good catch, `resolve_encryption_engine` now type-checks the value first so a
non-string fails closed with the same `ValueError` instead of an
`AttributeError`. Added a unit test.
--
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]
