codeant-ai-for-open-source[bot] commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3338482244
##########
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:
**Suggestion:** In engine-migration mode, always forwarding
`previous_secret_key` into `SecretsMigrator` can override the migrator's
built-in fallback to the current `SECRET_KEY`. If `PREVIOUS_SECRET_KEY` is
still set in config, this can make `--engine` runs fail to decrypt AES-CBC rows
(especially if config was already flipped), because decryption is attempted
with a stale key instead of the current one. Pass `previous_secret_key` only
when explicitly provided for key rotation, or ignore config
`PREVIOUS_SECRET_KEY` when running engine-only migration so the migrator can
default correctly. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β `re_encrypt_secrets --engine` fails when PREVIOUS_SECRET_KEY set.
- β AES-CBC secrets cannot be migrated to AES-GCM.
- β οΈ Authenticated-encryption adoption blocked for affected deployments.
- β οΈ Operators see unexplained decryption failures during migration.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure a Superset instance so that app-encrypted fields use AES-CBC
under the
current `SECRET_KEY` but the engine config has already been flipped to
AES-GCM: set
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE = "aes-gcm"` in config (used by
`SQLAlchemyUtilsAdapter.create()` at `superset/utils/encrypt.py:71-88`)
while the
underlying database still holds AES-CBC ciphertext from the historical
default engine.
2. Ensure that `PREVIOUS_SECRET_KEY` remains set in config to an older key
(different from
the current `SECRET_KEY`), e.g. after a prior key rotation, so
`current_app.config["PREVIOUS_SECRET_KEY"]` is non-empty when
`re_encrypt_secrets()` runs
at `superset/cli/update.py:104-124`.
3. Run the CLI command `superset re-encrypt-secrets --engine aes-gcm` (wired
to
`re_encrypt_secrets` at `superset/cli/update.py:104-124`) without providing
`--previous_secret_key`; inside this function, `previous_secret_key` is
populated from
`current_app.config.get("PREVIOUS_SECRET_KEY")` at `update.py:128-130`, and
`SecretsMigrator` is constructed with `previous_secret_key=<stale config
value>` and
`target_engine=AesGcmEngine` at `update.py:140-143`.
4. In `SecretsMigrator.__init__` at `superset/utils/encrypt.py:141-157`, the
engine-migration fallback `if target_engine is not None and not
previous_secret_key:
previous_secret_key = self._secret_key` is bypassed because
`previous_secret_key` is
already set from config; `_previous_secret_key` is thus the stale key
instead of the
current `SECRET_KEY`. When `run()` (at `encrypt.py:376-385`) iterates over
rows and calls
`_re_encrypt_row()` (`encrypt.py:265-361`), `_source_decryptors()`
(`encrypt.py:244-263`)
builds decryptors `[encrypted_type(current_engine=AES-GCM, current key),
AES-CBC with
stale previous key]`. Both fail to decrypt AES-CBC ciphertext encrypted with
the current
key, so each value is counted as `failed` and logged, and `run()` raises once
`stats.failed` is non-zero (`encrypt.py:387-385`), causing
`re_encrypt_secrets()` to print
`Re-encryption failed: ...` and exit with code 1 at
`superset/cli/update.py:145-148`,
blocking AES-CBCβAES-GCM migration for deployments where
`PREVIOUS_SECRET_KEY` is still
configured.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0a77a939b12e48b2868183aca5dcea5b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0a77a939b12e48b2868183aca5dcea5b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/cli/update.py
**Line:** 140:143
**Comment:**
*Logic Error: In engine-migration mode, always forwarding
`previous_secret_key` into `SecretsMigrator` can override the migrator's
built-in fallback to the current `SECRET_KEY`. If `PREVIOUS_SECRET_KEY` is
still set in config, this can make `--engine` runs fail to decrypt AES-CBC rows
(especially if config was already flipped), because decryption is attempted
with a stale key instead of the current one. Pass `previous_secret_key` only
when explicitly provided for key rotation, or ignore config
`PREVIOUS_SECRET_KEY` when running engine-only migration so the migrator can
default correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40654&comment_hash=057fc5941ab247e3c6c451e9f34efd1fb7ea6add4080b3e1e2122924251e2fa5&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40654&comment_hash=057fc5941ab247e3c6c451e9f34efd1fb7ea6add4080b3e1e2122924251e2fa5&reaction=dislike'>π</a>
##########
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:
**Suggestion:** The previous-key fallback decryptor is built without an
`engine`, so it always uses the default AES-CBC engine. That breaks
`SECRET_KEY` rotation for deployments configured with AES-GCM, because
ciphertext encrypted with the old key and GCM cannot be decrypted by this
fallback and is incorrectly counted as failed. Build the previous-key decryptor
with the same engine as the column's `encrypted_type` (or include both relevant
engines in order) so key rotation works for non-CBC configurations. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β re_encrypt_secrets CLI fails rotating AES-GCM deployments.
- β AES-GCM secrets remain unreadable under new SECRET_KEY.
- β οΈ Operators cannot complete documented AES-GCM key rotation.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure Superset with AES-GCM as the encrypted field engine by setting
`SQLALCHEMY_ENCRYPTED_FIELD_ENGINE = "aes-gcm"`, which
`SQLAlchemyUtilsAdapter.create()`
in `superset/utils/encrypt.py:71-87` reads to pass `engine=AesGcmEngine` into
`EncryptedType`.
2. Store app-encrypted secrets (e.g. DB passwords) so they are written using
`EncryptedType` instances discovered by
`SecretsMigrator.discover_encrypted_fields()` in
`superset/utils/encrypt.py:160-198`; these ciphertexts are produced with
`SECRET_KEY=KEY_A` and the AES-GCM engine, as also validated by the
integration tests that
ensure encrypted fields exist on the `dbs` table in
`tests/integration_tests/utils/encrypt_tests.py:34-48`.
3. Rotate the secret key by changing `current_app.config["SECRET_KEY"]` to
`KEY_B` and
configuring `PREVIOUS_SECRET_KEY=KEY_A`, then run the CLI command bound to
`re_encrypt_secrets()` in `superset/cli/update.py:25-48` (invoked via
`superset
re-encrypt-secrets --previous-secret-key KEY_A` or using
`PREVIOUS_SECRET_KEY`), which
constructs `SecretsMigrator(previous_secret_key=KEY_A, target_engine=None)`
at
`superset/cli/update.py:61-64`.
4. During `SecretsMigrator.run()` in `superset/utils/encrypt.py:117-163`,
each row is
processed by `_re_encrypt_row()` which calls `_source_decryptors()` at
`superset/utils/encrypt.py:244-260`; this builds two decryptors: (a) the
column's
configured AES-GCM + KEY_B type, and (b) a previous-key fallback
`EncryptedType(type_in=encrypted_type.underlying_type,
key=self._previous_secret_key)`
without an `engine`, which defaults to AES-CBC. Both decryptors fail on
AES-GCM+KEY_A
ciphertext, causing `_re_encrypt_row()` to mark the column as failed and
`run()` to raise
an exception when `stats.failed` is non-zero at
`superset/utils/encrypt.py:151-163`, which
`re_encrypt_secrets()` catches and reports as a failed re-encryption in
`superset/cli/update.py:65-69`, leaving AES-GCM deployments unable to
complete key
rotation.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=86676e1926fe404bb871eb03e9f3591a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=86676e1926fe404bb871eb03e9f3591a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/encrypt.py
**Line:** 255:263
**Comment:**
*Logic Error: The previous-key fallback decryptor is built without an
`engine`, so it always uses the default AES-CBC engine. That breaks
`SECRET_KEY` rotation for deployments configured with AES-GCM, because
ciphertext encrypted with the old key and GCM cannot be decrypted by this
fallback and is incorrectly counted as failed. Build the previous-key decryptor
with the same engine as the column's `encrypted_type` (or include both relevant
engines in order) so key rotation works for non-CBC configurations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40654&comment_hash=99a0cedb63f76d9e9f2e27f7e16cbdad3c0c296c6c37007e7b1d014be618932c&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40654&comment_hash=99a0cedb63f76d9e9f2e27f7e16cbdad3c0c296c6c37007e7b1d014be618932c&reaction=dislike'>π</a>
--
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]
