rusackas commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3342898692
##########
superset/utils/encrypt.py:
##########
@@ -65,6 +78,12 @@ def create(
**kwargs: Optional[dict[str, Any]],
) -> TypeDecorator:
if app_config:
+ # Select the encryption engine from config, defaulting to the
+ # historical AES-CBC engine for backward compatibility. 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", "aes")
+ kwargs["engine"] = ENCRYPTION_ENGINES.get(engine_name,
AesEngine)
return EncryptedType(*args, lambda: app_config["SECRET_KEY"],
**kwargs)
Review Comment:
On the warning: good idea, added in fc28e60 — the adapter now logs a warning
naming the valid engines before falling back to AES-CBC when
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` is unrecognized. On the mypy concern:
`pre-commit run mypy` passes as-is on this assignment, so no change was needed
there.
##########
superset/config.py:
##########
@@ -295,6 +295,16 @@ def _try_json_readsha(filepath: str, length: int) -> str |
None:
SQLAlchemyUtilsAdapter
)
+# Encryption engine used by the default SQLAlchemyUtilsAdapter for
app-encrypted
+# fields. Options:
+# "aes" - AES-CBC (historical default; unauthenticated, queryable)
+# "aes-gcm" - AES-GCM (authenticated encryption; recommended for NEW
installs)
+# WARNING: changing this on a database that already holds encrypted secrets
+# (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.
+SQLALCHEMY_ENCRYPTED_FIELD_ENGINE: Literal["aes", "aes-gcm"] = "aes"
Review Comment:
Fair point, fixed in fc28e60 — the comment block above
`SQLALCHEMY_ENCRYPTED_FIELD_TYPE_ADAPTER` now points operators at the
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` knob as the preferred way to switch
engines, keeping the custom-adapter example as the advanced option.
##########
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)),
+ 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:
previous_secret_key = previous_secret_key or current_app.config.get(
"PREVIOUS_SECRET_KEY"
)
- if previous_secret_key is None:
+ target_engine = (
+ ENCRYPTION_ENGINES[target_engine_name] if target_engine_name else None
+ )
+ if previous_secret_key is None and target_engine is None:
click.secho(
- "No previous secret key provided; nothing to re-encrypt.",
+ "No previous secret key or target engine provided; nothing to
re-encrypt.",
fg="yellow",
)
return
- secrets_migrator = SecretsMigrator(previous_secret_key=previous_secret_key)
+ secrets_migrator = SecretsMigrator(
+ previous_secret_key=previous_secret_key,
+ target_engine=target_engine,
+ )
Review Comment:
I don't think this one holds as stated. The repro requires
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` to be flipped to GCM *and* a stale
`PREVIOUS_SECRET_KEY` left in config while the data is still CBC under the
current key — but the documented runbook (and the `--engine` help text) says to
run the migrator *before* flipping the config. More importantly, after the
`_source_decryptors` fix in fc28e60 the previous-key fallback now also tries
AES-CBC, so even the config-flipped-first case decrypts CBC source data under
the previous key. Forwarding `PREVIOUS_SECRET_KEY` during an engine migration
is intentional for operators doing a combined key-rotation + engine change.
Leaving the CLI as-is.
--
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]
