sadpandajoe commented on code in PR #43074:
URL: https://github.com/apache/superset/pull/43074#discussion_r3771957523
##########
superset/utils/encrypt.py:
##########
@@ -29,20 +30,74 @@
AesGcmEngine,
EncryptionDecryptionBaseEngine,
)
+from sqlalchemy_utils.types.encrypted.padding import (
+ InvalidPaddingError,
+ PADDING_MECHANISM,
+)
class EncryptedType(SqlaEncryptedType):
cache_ok = True
+class BackwardCompatibleAesEngine(AesEngine):
+ """AES-CBC engine that pads new writes with PKCS5, not sqlalchemy_utils'
+ default "naive" scheme, while still reading values already stored under
+ naive padding without a data migration.
+
+ ``NaivePadding`` pads short plaintext with literal ``*`` bytes and unpads
+ by unconditionally stripping every trailing ``*`` -- so a secret whose
+ real value ends in ``*`` loses that character the next time it's
+ decrypted (apache/superset#32664). PKCS5 encodes the padding length in
+ the padding bytes themselves and validates it on unpad, so it can't
+ silently eat real data, and it's the standard scheme
+ ``sqlalchemy_utils`` ships specifically as the safe alternative to naive
+ padding.
+
+ ``decrypt`` tries PKCS5 first: ``PKCS5Padding.unpad`` is self-validating
+ and raises ``InvalidPaddingError`` for anything that isn't validly
+ PKCS5-padded, so it only succeeds on values this engine (or another
+ PKCS5-padded source) actually wrote. Naive-padded legacy values fail
+ that validation in practice, so decrypt falls back to naive unpadding
+ for them -- every already-stored secret keeps decrypting correctly, no
+ re-encryption pass required, while every new write is safe going
+ forward.
+ """
+
+ def _set_padding_mechanism(self, padding_mechanism: str | None = None) ->
None:
+ super()._set_padding_mechanism("pkcs5")
Review 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?
--
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]
