rusackas opened a new pull request, #43074:
URL: https://github.com/apache/superset/pull/43074
### SUMMARY
Test-only PR pinning a real, currently-reproducing bug: any secret stored
via the default AES-CBC `EncryptedType` field (this includes
`Database.password`) silently loses a trailing `*` character on the next read
after it's saved.
The default field uses sqlalchemy_utils' "naive" padding scheme, which pads
short plaintext with literal `*` bytes and unpads on decrypt via
`value.rstrip(b"*")` (unconditional, no length/count encoded anywhere). If the
real secret happens to end in `*`, that character is indistinguishable from
padding and gets stripped along with it.
```python
field = SQLAlchemyUtilsAdapter().create(SECRET, String(1024))
encrypted = field.process_bind_param("mypassword*", DIALECT)
field.process_result_value(encrypted, DIALECT)
# 'mypassword' -- the trailing '*' is gone
```
This is #32664, reopened. It's a different root cause than the original
report, which was fixed by #30532 (a connection-string encoding issue at
database-creation time, not a 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 PR's test is built on. I verified it independently against
Superset's own field factory before reopening, not just the report.
**Why test-only:** the actual fix needs care. Naive padding is
sqlalchemy_utils' historical default, kept for backwards compatibility with
already-stored ciphertext. Switching the padding scheme (e.g. to PKCS5, which
is unambiguous) has the same re-encryption hazard that
`superset/utils/encrypt.py` already documents for the AES-CBC to AES-GCM engine
switch: existing encrypted secrets could become unreadable if the scheme just
changes underneath them without a migration path. That's a bigger, separate
piece of work.
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/utils/encrypt_test.py -v` —
`test_trailing_asterisk_survives_round_trip` currently fails (red) on `master`,
confirming the reproduction. The other 15 existing tests in the file are
unaffected.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #32664
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
