fix(encryption): resolve SECRET_KEY lazily to fix silent re-encrypt-secrets failures#37982
fix(encryption): resolve SECRET_KEY lazily to fix silent re-encrypt-secrets failures#37982Ujjwaljain16 wants to merge 4 commits intoapache:masterfrom
Conversation
Code Review Agent Run #e4fcc6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
…nt re-encrypt-secrets failures
1a36a59 to
ebd2f42
Compare
|
@Ujjwaljain16 thanks for making contribution here! However, this PR contains commits from other PRs you've pushed to. Can you clean that up? |
| # If decryption doesn't raise, value should be garbage | ||
| assert bad_decrypt != test_value | ||
| except (ValueError, Exception): | ||
| pass # Expected: old key cannot decrypt new ciphertext |
There was a problem hiding this comment.
there should be an assertion here for the thrown error to ensure that indeed it's the decryption issue
|
Adding an assertion to ensure decryption fails with the wrong key would strengthen the test by confirming the error is due to key mismatch, not just producing incorrect output. This makes the test more robust. tests/integration_tests/utils/encrypt_tests.py |
|
@hainenber yes that's my mistake and i cleaned that up |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #37982 +/- ##
==========================================
+ Coverage 60.48% 66.43% +5.94%
==========================================
Files 1931 668 -1263
Lines 76236 51322 -24914
Branches 8568 5776 -2792
==========================================
- Hits 46114 34096 -12018
+ Misses 28017 15840 -12177
+ Partials 2105 1386 -719
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ix import sort (I001)
|
This has been working out greatly! Having said that, I think @dpgaspar's would have better authority for the full review. |
Code Review Agent Run #11854eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Code Review Agent Run #f386e4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
SUMMARY
Fixes an issue where
superset re-encrypt-secretscould silently fail to rotate encrypted database secrets.Previously,
EncryptedTypewas initialized with a static string value ofSECRET_KEYat model definition time:If configuration overrides (e.g.,
SUPERSET_SECRET_KEY, Helm updates, or runtime changes) occurred after model import, the encryption engine could remain bound to a stale key.During key rotation:
PREVIOUS_SECRET_KEYsucceeded.EncryptedType.After restart with the new
SECRET_KEY, Superset would fail with:Fix
Pass a callable instead of a static string to
EncryptedType:sqlalchemy-utilssupports callable keys and resolves them on every encrypt/decrypt operation. This ensures the currentSECRET_KEYis always used at runtime.No schema changes or data format changes are introduced.
Tests
test_lazy_key_resolutionto verify key changes are reflected after field creation.test_secret_key_rotationto simulate end-to-end key rotation (KEY_A → KEY_B) and verify correct decryption behavior.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable (backend encryption fix).
TESTING INSTRUCTIONS
Manual verification:
Start Superset with
SECRET_KEY = A.Create a database connection with a password.
Rotate keys:
Restart Superset.
Confirm database connections decrypt successfully under
B.Confirm reverting to
Acauses decryption failure.Unit tests cover lazy key resolution and full rotation behavior.
ADDITIONAL INFORMATION