rusackas commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3407952971
##########
superset/cli/update.py:
##########
@@ -110,17 +110,37 @@ def update_api_docs() -> None:
help="An optional previous secret key, if PREVIOUS_SECRET_KEY "
"is not set on the config",
)
-def re_encrypt_secrets(previous_secret_key: Optional[str] = None) -> None:
[email protected](
+ "--engine",
+ "-e",
+ "target_engine_name",
+ required=False,
+ type=click.Choice(sorted(ENCRYPTION_ENGINES), case_sensitive=False),
+ help="Re-encrypt all app-encrypted fields with this encryption engine "
+ "(e.g. 'aes-gcm' for authenticated encryption). The SECRET_KEY is "
+ "unchanged. Take a metadata-DB backup first, then set "
+ "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE to the same value and restart.",
+)
+def re_encrypt_secrets(
+ previous_secret_key: Optional[str] = None,
+ target_engine_name: Optional[str] = None,
+) -> None:
Review Comment:
Good call, added a docstring.
##########
superset/utils/encrypt.py:
##########
@@ -101,10 +179,40 @@ def created_by_enc_field_factory(field: TypeDecorator) ->
bool:
class SecretsMigrator:
- def __init__(self, previous_secret_key: str) -> None:
+ """Re-encrypts every app-encrypted column in the ORM.
+
+ Two modes, which can also be combined:
+
+ - **Key rotation** — pass ``previous_secret_key``. Values are decrypted
+ under the previous key and re-encrypted under the current ``SECRET_KEY``
+ using the currently-configured engine.
+ - **Engine migration** — pass ``target_engine`` (e.g. ``AesGcmEngine``).
+ Values are decrypted under the current ``SECRET_KEY`` with the source
+ engine and re-encrypted with the target engine. This is how an existing
+ install moves from AES-CBC to authenticated AES-GCM without bricking
+ stored secrets; the ``SECRET_KEY`` itself is unchanged.
+
+ Both modes share the same all-or-nothing transaction and per-column
+ idempotency: a value already readable in the target form is left untouched,
+ so a run can be safely repeated or resumed.
+ """
+
+ def __init__(
+ self,
+ previous_secret_key: Optional[str] = None,
+ target_engine: Optional[type[Any]] = None,
+ ) -> None:
from superset import db # pylint: disable=import-outside-toplevel
Review Comment:
Added a docstring to the initializer.
##########
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
Review Comment:
Added a docstring here.
##########
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:
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:
Review Comment:
Annotated `app_context` as `AppContext`.
--
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]
