fix(encrypt): stop naive padding from truncating secrets ending in '*' - #43074
Conversation
#32664 (reopened): the default AES-CBC EncryptedType field (used for Database.password, among other secrets) relies on sqlalchemy_utils' "naive" padding scheme, which pads short plaintext with literal '*' bytes and unpads on decrypt by unconditionally stripping every trailing '*' (NaivePadding.unpad -> value.rstrip(b"*")). A password or token whose real value happens to end in '*' gets that character stripped right along with the padding, silently corrupting the secret the next time it's read back. This is a different root cause than the original report (fixed by #30532, which addressed the connection-string encoding used at database-creation time). The issue was closed assuming that fix covered it, but the underlying storage-layer corruption was never addressed; two independent users confirmed reproducing it on 6.1.0 after the close. Verified directly against Superset's own field factory (SQLAlchemyUtilsAdapter, not just raw sqlalchemy_utils): 'mypassword*' round-trips through process_bind_param/process_result_value as 'mypassword'. Test-only: a real fix needs care. Naive padding is sqlalchemy_utils' historical default (kept for backwards compatibility with existing ciphertext), and switching padding schemes on an existing deployment has the same re-encryption hazard already documented in superset/utils/encrypt.py for the AES-CBC -> AES-GCM engine switch -- old encrypted values could become unreadable if the padding scheme just changes under them without a migration path. Fixes #32664
rusackas
added
the
tdd
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #85c853
Actionable Suggestions - 1
-
tests/unit_tests/utils/encrypt_test.py - 1
- Test will always fail without fix · Line 78-99
Review Details
-
Files reviewed - 1 · Commit Range:
66d4119..66d4119- tests/unit_tests/utils/encrypt_test.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43074 +/- ##
==========================================
- Coverage 66.64% 66.61% -0.04%
==========================================
Files 2866 2866
Lines 162896 162808 -88
Branches 37525 37477 -48
==========================================
- Hits 108568 108456 -112
- Misses 52203 52224 +21
- Partials 2125 2128 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes the bug the previous commit's test pinned. The default AES-CBC field padded new writes with sqlalchemy_utils' "naive" scheme, which pads short plaintext with literal '*' bytes and unpads by unconditionally stripping every trailing '*' -- silently eating a real trailing '*' in any password/token right along with the padding. Adds BackwardCompatibleAesEngine, a drop-in AesEngine subclass that pads new writes with PKCS5 (the standard, self-validating scheme sqlalchemy_utils ships for exactly this reason) while still decrypting values already stored under naive padding: decrypt tries PKCS5 unpad first -- which raises InvalidPaddingError on anything that isn't validly PKCS5-padded -- and falls back to naive unpad only when that fails. That makes the fix safe to ship with no data migration: existing ciphertext is never rewritten, and old secrets keep decrypting correctly. ENCRYPTION_ENGINES["aes"] now resolves to this subclass instead of the raw upstream AesEngine, so it applies wherever the default engine is used: Database.password and every other secret stored via the default EncryptedType field, not just the specific case in the bug report. Added regression coverage: multiple trailing asterisks, a legacy naive-padded value read back correctly under the new engine (the no-migration-needed property), and a check that new ciphertext is actually PKCS5-padded. Updated one characterization test (test_encrypt_cbc_iv_reuse.py) that pinned the exact previous engine class identity -- it now pins BackwardCompatibleAesEngine instead, with an assertion that the deterministic-IV behavior it exists to test is unchanged (this engine doesn't touch IV derivation, only padding). Fixes #32664
rusackas
changed the title
…dCompatibleAesEngine check_encryption_engine() compared the resolved engine class against the raw sqlalchemy_utils AesEngine, but ENCRYPTION_ENGINES["aes"] now resolves to BackwardCompatibleAesEngine, so the legacy-engine startup warning silently stopped firing. The key-rotation integration test decrypted under the previous key with the default (NaivePadding) engine, which no longer matches what values are actually written under -- fix it to decrypt with BackwardCompatibleAesEngine, mirroring SecretsMigrator._source_decryptors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #525c0aActionable 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 AI Code Review powered by |
| """ | ||
|
|
||
| def _set_padding_mechanism(self, padding_mechanism: str | None = None) -> None: | ||
| super()._set_padding_mechanism("pkcs5") |
There was a problem hiding this comment.
This changes the stored CBC format in a way that old Superset processes cannot read: the previous AesEngine only removes *, so a value written here during a rolling upgrade or rollback is returned with its PKCS5 bytes appended and the password/token fails. Can we keep a write format old readers accept until this transition is coordinated?
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
Reviewed the padding fix — PKCS5 with a naive-unpad fallback preserves backward-compat with existing stored values, and the cipher/key handling is unchanged. LGTM.
sadpandajoe
added
the
merge-if-green
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
SUMMARY
Fixes #32664 (reopened): any secret stored via the default AES-CBC
EncryptedTypefield (this includesDatabase.password, and every other secret using the default encryption engine) silently loses a trailing*character the next time it's decrypted after being saved.Root cause: the default field padded new writes with sqlalchemy_utils' "naive" padding scheme, which pads short plaintext with literal
*bytes and unpads by unconditionally stripping every trailing*(NaivePadding.unpad→value.rstrip(b"*"), no length or count encoded anywhere). If the real secret happens to end in*, that character is indistinguishable from padding and gets stripped along with it.This is a different root cause than the original report, which was fixed by #30532 (a connection-string encoding issue at database-creation time, not this storage-layer corruption). The issue was closed on the assumption that fix covered it, but two independent users (@CamiloCarvajalPensemos and @ajunior) confirmed reproducing this on 6.1.0 well after the close, with @CamiloCarvajalPensemos providing the exact root-cause diagnosis this fix is built on.
The fix:
BackwardCompatibleAesEngine, a drop-inAesEnginesubclass that pads new writes with PKCS5 (the standard, self-validating padding scheme sqlalchemy_utils ships specifically as the safe alternative to naive padding) while still correctly decrypting values already stored under naive padding.decrypttries PKCS5 unpad first — which raisesInvalidPaddingErroron anything that isn't validly PKCS5-padded — and falls back to naive unpad only when that fails.This makes the fix safe to ship without a data migration: existing ciphertext is never rewritten in place, so already-stored secrets keep decrypting correctly, while every new write is safe going forward.
ENCRYPTION_ENGINES["aes"]now resolves to this subclass, so the fix applies everywhere the default engine is used, not justDatabase.password.TESTING INSTRUCTIONS
22 tests pass, including:
test_trailing_asterisk_survives_round_trip) and a multi-asterisk varianttest_legacy_naive_padded_secret_still_decrypts— the load-bearing proof that this needs no migration: encrypts a value with the raw upstreamAesEngine(naive padding) directly, then decrypts it withBackwardCompatibleAesEngine, confirming it still reads back correctlytest_new_writes_use_pkcs5_padding— confirms new ciphertext actually uses the safer schemeBackwardCompatibleAesEnginewith an explicit assertion that the deterministic-IV property it exists to test is unchanged (this class doesn't touch IV derivation, only padding)Also ran the full
tests/unit_tests/utils/directory (782 passed, 1 pre-existing unrelated failure in a date-parsing test, nothing to do with this change).ADDITIONAL INFORMATION