Repository navigation
feat(encryption): [8/N] encryption manager - #4149
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
There was a problem hiding this comment.
This came up apache/iceberg-rust#3402 and was discussed here apache/iceberg-rust#2453 (comment). Happy to change this in python and follow-up in rust
There was a problem hiding this comment.
I think we should raise here instead of returning None. Encryption is a v3 feature, and java rejects encryption.key-id on pre-v3 tables at creation. Once this is wired into writes, returning None would mean writes silently go out as plaintext even though the user asked for encryption.
We should make sure the raise doesn't block loading the table or metadata-only commits, otherwise users can't remove_properties("encryption.key-id") to fix it. Pre-v3 tables have no encryption-keys, so nothing in them can be encrypted and reads don't need the manager. When we wire this up, the error should only surface when producing a snapshot (write side).
Follow-up: we should reject encryption.key-id / encryption.data-key-length on pre-v3 tables at create / property update, like java's checkCompatibility.
| 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.
|
Thanks for the review! |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM!
i had my agent did a small poc against java and rust implementation and everything worked as expected
Once we wire this up, we should double check the behavior for v2 table and make sure that we are not allowing encryption for v2 tables. This is a more generalized problem and I created #4156 to track 😄
| if (table_key_id := metadata.properties.get(TableProperties.ENCRYPTION_KEY_ID)) is None: | ||
| return None | ||
|
|
||
| if not isinstance(metadata, TableMetadataV3): |
There was a problem hiding this comment.
nit: format_version < 3 might be better since we have v4 coming up soon. we can make this a follow up fix and refactor other places in the repo (if any)
| reader.decrypt_manifest_list_key_metadata(key_id) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("key_size", list(AesKeySize)) |
There was a problem hiding this comment.
nit: i think this and the other pytest.mark.parametrize can be simple for loops inside the function 😄
i added this rule in the AGENTS.md (see #4150)
|
cc @mbutrovich if you would like to give it a look |
|
Thanks @xanderbailey ! |
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?