Repository navigation
feat(encryption): [8/N] encryption manager - #4149
xanderbailey wants to merge 2 commits into
Conversation
|
|
||
| AES_KEY_LENGTHS = (16, 24, 32) | ||
|
|
||
| AAD_PREFIX_LENGTH = 16 |
There was a problem hiding this comment.
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
some review comments, pairing with my agent
| kms_client (KeyManagementClient | None): Wraps and unwraps KEKs with the table's master key. | ||
| metadata (TableMetadata): The table's metadata, holding `encryption.key-id` and its encryption keys. | ||
| """ | ||
| if not isinstance(metadata, TableMetadataV3): |
There was a problem hiding this comment.
Encryption silently turns off on v1/v2 tables. If a v1/v2 table has encryption.key-id set, from_table_metadata returns None, so the table is treated as unencrypted and plaintext gets written.
Java's checkCompatibility rejects encryption properties on tables below v3. This should raise an error instead of returning None.
Rust might have the same issue
| from pyiceberg.encryption.ciphers import AesGcmCipher, AesKeySize, SecureKey | ||
| from pyiceberg.encryption.key_metadata import StandardKeyMetadata | ||
| from pyiceberg.encryption.kms import KeyManagementClient | ||
| from pyiceberg.table import TableProperties |
There was a problem hiding this comment.
Circular import. manager.py imports TableProperties from pyiceberg.table at module level.
Importing the manager from pyiceberg/table/__init__.py, which a later step will need, raises ImportError. Moving the constants or using a lazy import would fix it.
| if (key := self._encryption_keys.get(key_id)) is None: | ||
| raise ValueError(f"Encryption key not found: {key_id}") | ||
|
|
||
| if key.encrypted_by_id is None: |
There was a problem hiding this comment.
we should guard against passing a KEK id here. right now it gets past the lookup, then fails with KEK not found in encryption keys: <master-key-id>, which is misleading.
java rejects this up front:
https://github.com/apache/iceberg/blob/fb58513787756dedab6550d6695df36a27acb64e/core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java#L171-L183
if key.encrypted_by_id == self._table_key_id:
raise ValueError(f"{key_id} is a key encryption key, not manifest list key metadata")EDIT: this should be in L119 😄
Working towards #3222
Rationale for this change
Have chosen to go with a structure that matches iceberg-rust rather than java's inheritance, I think this will make it easier in the future to move parts of this down to rust if we choose to. I also think it's a little bit more clear than how java does it.
encrypt(output_file)is missing because we don't have a format for the output file yet so this will come later as we continue to build out this feature.Are these changes tested?
Are there any user-facing changes?