rusackas commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3342896414
##########
superset/utils/encrypt.py:
##########
@@ -177,6 +226,42 @@ def _select_columns_from_table(
cols = ",".join(pk_columns + column_names)
return conn.execute(f"SELECT {cols} FROM {table_name}") # noqa: S608
+ def _target_type(self, encrypted_type: EncryptedType) -> EncryptedType:
+ """The EncryptedType to re-encrypt a value *into*.
+
+ For a key rotation this is the column's own configured type (current
+ engine + lazily-resolved current key). For an engine migration it is a
+ type pinned to the requested target engine under the current key.
+ """
+ if self._target_engine is None:
+ return encrypted_type
+ return EncryptedType(
+ type_in=encrypted_type.underlying_type,
+ key=self._secret_key,
+ engine=self._target_engine,
+ )
+
+ def _source_decryptors(self, encrypted_type: EncryptedType) ->
list[EncryptedType]:
+ """Candidate decryptors, tried in order, to recover a value's
plaintext.
+
+ 1. The column's configured type — current key + currently-configured
+ engine. During an engine migration this reads the source ciphertext
+ while the config still points at the source engine.
+ 2. The previous key with the historical AES-CBC engine — covers
+ ``SECRET_KEY`` rotation, and (since the previous key defaults to the
+ current one in engine-migration mode) reading AES-CBC source data
+ after the config has already been flipped to the target engine.
+ """
+ decryptors = [encrypted_type]
+ if self._previous_secret_key:
+ decryptors.append(
+ EncryptedType(
+ type_in=encrypted_type.underlying_type,
+ key=self._previous_secret_key,
+ )
Review Comment:
Good catch, this is a real bug for AES-GCM deployments. Fixed in fc28e60:
`_source_decryptors` now tries the previous key under the column's own engine
(so GCM ciphertext from the old key decrypts on rotation) as well as the
historical AES-CBC engine (which preserves the engine-flipped-first migration
path). Added a unit test, `test_key_rotation_for_aes_gcm_column`, that fails
without the fix.
##########
superset/utils/encrypt.py:
##########
@@ -177,6 +226,42 @@ def _select_columns_from_table(
cols = ",".join(pk_columns + column_names)
return conn.execute(f"SELECT {cols} FROM {table_name}") # noqa: S608
+ def _target_type(self, encrypted_type: EncryptedType) -> EncryptedType:
+ """The EncryptedType to re-encrypt a value *into*.
+
+ For a key rotation this is the column's own configured type (current
+ engine + lazily-resolved current key). For an engine migration it is a
+ type pinned to the requested target engine under the current key.
+ """
+ if self._target_engine is None:
+ return encrypted_type
+ return EncryptedType(
+ type_in=encrypted_type.underlying_type,
+ key=self._secret_key,
+ engine=self._target_engine,
+ )
+
+ def _source_decryptors(self, encrypted_type: EncryptedType) ->
list[EncryptedType]:
+ """Candidate decryptors, tried in order, to recover a value's
plaintext.
+
+ 1. The column's configured type — current key + currently-configured
+ engine. During an engine migration this reads the source ciphertext
+ while the config still points at the source engine.
+ 2. The previous key with the historical AES-CBC engine — covers
+ ``SECRET_KEY`` rotation, and (since the previous key defaults to the
+ current one in engine-migration mode) reading AES-CBC source data
+ after the config has already been flipped to the target engine.
+ """
+ decryptors = [encrypted_type]
+ if self._previous_secret_key:
+ decryptors.append(
+ EncryptedType(
+ type_in=encrypted_type.underlying_type,
+ key=self._previous_secret_key,
+ )
+ )
+ return decryptors
Review Comment:
Same root cause as the other thread, and you're right. Fixed in fc28e60: the
previous-key fallback now uses the column's configured engine (plus AES-CBC as
a second attempt), so AES-GCM key rotation works. Covered by the new
`test_key_rotation_for_aes_gcm_column` unit test.
##########
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)),
Review Comment:
Agreed, applied in fc28e60 — `click.Choice(..., case_sensitive=False)`. The
engine keys are lowercase so the lookup still resolves correctly after Click
normalizes the input.
--
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]
