Copilot commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3338104890
##########
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:
`kwargs` is annotated such that each kwarg value is an `Optional[dict[str,
Any]]`, so assigning `AesEngine`/`AesGcmEngine` into `kwargs["engine"]` will
likely fail mypy. Also, silently falling back to AES-CBC on an unknown
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` makes misconfiguration hard to
detect—consider logging a warning when the value is unrecognized (while still
defaulting to AES-CBC for backwards compatibility).
##########
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:
This new config knob contradicts the earlier comment block above
`SQLALCHEMY_ENCRYPTED_FIELD_TYPE_ADAPTER` which says operators must write a
custom adapter to switch engines. To avoid confusing operators, that earlier
guidance should be updated to mention `SQLALCHEMY_ENCRYPTED_FIELD_ENGINE` as
the preferred mechanism (and keep the custom-adapter example as an advanced
option).
--
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]
