feat(encryption) [4/N] AES-GCM encryption implementation - #3963
Conversation
# Conflicts: # pyiceberg/utils/lazy_import.py # tests/utils/test_lazy_import.py
| | hf | Support for Hugging Face Hub | | ||
| | gcp-auth | Support for Google Cloud authentication | | ||
| | entra-auth | Support for Azure Entra authentication | | ||
| | encryption | Support for table encryption | |
There was a problem hiding this comment.
Missed this in the first PR
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Couple questions, but this looks great. Thanks so much for doing this!
| from pyiceberg.exceptions import NotInstalledError | ||
|
|
||
|
|
||
| def not_installed(module_name: str, extras_name: str | None = None) -> NotInstalledError: |
There was a problem hiding this comment.
Why do we need this? try_import should be able to take in the full name (try_import("cryptography.hazmat.primitives.ciphers.aead")).
Are you doing this for the sake of keeping the types? The way we've handled that in the past is with a if TYPE_CHECKING block. This works great for this particular use case in an init function, but otherwise just adds some confusion for which import function we should use.
|
|
||
| AES128_KEY = b"0123456789012345" | ||
| PLAINTEXT = b"the quick brown fox" | ||
|
|
There was a problem hiding this comment.
Are we able to add a fixture that was written by a different implementation (java or rust?). That would go a long way to making sure that our decryption logic is correct.
There was a problem hiding this comment.
I did test it locally against rust which I tested at the time against Java, I’ll track a cross client test as a follow up if that’s works for you?
| try: | ||
| return self._aes_gcm.decrypt(nonce, encrypted, aad) | ||
| except self._invalid_tag as e: | ||
| raise ValueError("AES-GCM decryption failed") from e |
There was a problem hiding this comment.
It looks like Java's error message is ""wrong decryption key; or corrupt/tampered data".
What would you think about changing this error message? This error message makes it unclear if the system failed (and should just be retried?) or if the user inputs are incorrect.
There was a problem hiding this comment.
Happy to align with Java errr message here
There was a problem hiding this comment.
I think that'll be a nice touch
| raise not_installed("cryptography", extras_name="encryption") from None | ||
|
|
||
| self._aes_gcm: AESGCM = AESGCM(key.key) | ||
| self._invalid_tag: type[InvalidTag] = InvalidTag |
There was a problem hiding this comment.
nit: codex suggested this strange logic instead of not_installed
self._aes_gcm: AESGCM = try_import("cryptography.hazmat.primitives.ciphers.aead", extras_name="encryption").AESGCM(
key.key
)
self._invalid_tag: type[InvalidTag] = try_import("cryptography.exceptions", extras_name="encryption").InvalidTag
| assert ciphertext != PLAINTEXT | ||
| assert cipher.decrypt(ciphertext, aad) == PLAINTEXT | ||
|
|
||
|
|
There was a problem hiding this comment.
something like this would be good. just as a regression test
def test_aes128_gcm_known_answer(mocker: MockFixture) -> None:
# NIST CAVS gcmEncryptExtIV128.rsp vector from RustCrypto's aes-gcm tests.
# https://github.com/RustCrypto/AEADs/blob/aes-gcm-v0.10.3/aes-gcm/tests/aes128gcm.rs#L737-L744
key = bytes.fromhex("c939cc13397c1d37de6ae0e1cb7c423c")
nonce = bytes.fromhex("b3d8cc017cbb89b39e0f67e2")
plaintext = bytes.fromhex("c3b3c41f113a31b73d9a5cd432103069")
aad = bytes.fromhex("24825602bd12a984e0092d3e448eda5f")
ciphertext = bytes.fromhex("93fe7d9e9bfd10348a5606e5cafa7354")
tag = bytes.fromhex("0032a1dc85f1c9786925a2e71d8272dd")
expected = nonce + ciphertext + tag
cipher = AesGcmCipher(SecureKey(key))
mocker.patch("pyiceberg.encryption.ciphers.os.urandom", return_value=nonce)
assert cipher.encrypt(plaintext, aad) == expected
assert cipher.decrypt(expected, aad) == plaintext
There was a problem hiding this comment.
Test cases I added are verified against iceberg-rust
| try: | ||
| return self._aes_gcm.decrypt(nonce, encrypted, aad) | ||
| except self._invalid_tag as e: | ||
| raise ValueError("AES-GCM decryption failed") from e |
There was a problem hiding this comment.
I think that'll be a nice touch
|
When we eventually use the rust bindings, we can still keep |
The existing cipher tests all round-trip through the same code, so a symmetric change to the nonce || ciphertext || tag layout would pass them while breaking interoperability with the Java and iceberg-rust clients. Pin encryption and decryption against fixed GCM-spec vectors covering AES-128 and AES-256, with and without AAD.
Report the same cause as the Java client when the GCM tag check fails, so the message reads consistently across implementations and makes clear the failure is bad input rather than a retryable system error.
Import the cryptography modules with try_import rather than a spelled-out try/except, and drop not_installed now that nothing calls it. The TYPE_CHECKING block already keeps AESGCM and InvalidTag statically typed, so lazy_import is back to a single entry point.
|
Thanks both for the reviews, should be good now |
3 participants
Working towards: #3222
Rationale for this change
Direct port of iceberg-rust AEC-GCM encryption apache/iceberg-rust#2026
Are these changes tested?
Are there any user-facing changes?