feat(encryption) [5/N] KMS - #3968
xanderbailey wants to merge 3 commits into
Conversation
|
@kevinjqliu this is good for a review when you have a moment |
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
@xanderbailey thanks for porting this. The tests cover the failure paths I'd want from a KMS mock: a wrong master key, a tampered wrapped key, an unknown key id, and wrapping without AAD, which keeps it compatible with Java's MemoryMockKMS. The redacted reprs and the master_key_size name also carry over what we settled on the Rust side.
My main question is about how a KeyManagementClient gets constructed. It's worth settling that now, before third-party clients start subclassing it.
On #3963 the cross-client fixture for the cipher tests was deferred to a follow-up, but I can't find an issue for it under #3222. Could you file one and link it here? This PR's test master key uses the same bytes as Java's UnitestKMS.MASTER_KEY1, so a key that Java wraps under keyA would make a good first fixture.
| class KeyManagementClient(ABC): | ||
| """A base class for key management service implementations. | ||
|
|
||
| Wraps and unwraps table encryption keys using master keys that the service holds. | ||
| """ |
There was a problem hiding this comment.
How will a client be constructed once the catalog wiring lands? Java's KeyManagementClient has initialize(Map<String, String> properties). EncryptionUtil.createKmsClient builds the class named by encryption.kms-impl with a no-arg constructor and then calls initialize with the catalog properties. This ABC has no equivalent yet, so a custom client has no construction contract to code against.
PyIceberg already has a pattern for classes loaded by name. FileIO.__init__ takes properties, and _import_file_io calls class_(properties). Could KeyManagementClient define __init__(self, properties: Properties = EMPTY_DICT) the same way? If the loader starts calling cls(properties) in a later PR, any subclass written against this version with a different constructor will fail to load. MemoryKeyManagementClient would need master_key_size to come from a property in that model. If you'd rather mirror iceberg-rust, its KmsClientFactory solves the same problem, but either way I'd like the contract in this PR.
There was a problem hiding this comment.
Ah yes okay so I was actually deliberately deferring that choice but I forget python is all public...
| class MemoryKeyManagementClient(KeyManagementClient): | ||
| """A key management service that holds its master keys in memory, for testing and demonstration. | ||
|
|
||
| Master keys live only in this process, with no durability or access control, so this is | ||
| not for production use. Mirrors Java's `MemoryMockKMS` and iceberg-rust's | ||
| `MemoryKeyManagementClient`. |
There was a problem hiding this comment.
Does MemoryKeyManagementClient need to ship in the pyiceberg package? Java keeps MemoryMockKMS under core/src/test, so it's not public API there. iceberg-rust does export its version. Once this is released, removing it needs a @deprecated cycle. If PyIceberg's own tests are the only consumers, moving it under tests/ keeps that option open. A UnitestKMS-style subclass with Java's fixed keys could live next to it for the cross-client tests. If you expect users to run it for demos, could you say so in the PR description?
This branch has not been deployed
2 participants
Rationale for this change
Encryption needs a KMS interface or class that we can implement for different key management service providers like AWS, Vault etc. This is directly modeled off rusts KMS
Are these changes tested?
Are there any user-facing changes?