AWS: KeyManagementClient implementation that works with AWS KMS #13136
Conversation
|
I will take a look as well. Thanks ! |
|
Sorry for the delay, I'm taking a look |
| * encrypting/decrypting keys with a KMS-managed master key, (by referencing its key ID), and for | ||
| * the generation of new encryption keys. | ||
| */ | ||
| public class AwsKeyManagementClient implements KeyManagementClient { |
There was a problem hiding this comment.
I think this will need to be serializable so that it can be used in a distributed setting (since an EncryptionManager instance can also be serialized, and generally we'd expect an EncryptionManager to also contain an instance of KeyManagementClient
There was a problem hiding this comment.
Yeah KeyManagementClient does implement Serializable as is (though looking at the implementation, wonder how necessary that is - it's transient in the StandardEncryptionmanager both now and after #7770, cc @ggershinsky)
There was a problem hiding this comment.
As @smaheshwar-pltr points it out the class implements Serializable by interface but I also have doubts whether that's actually needed. KMS interactions for table encryption only concerns the master key which I'd expect to take place on driver / coordinator nodes, encrypting/decrypting the metadata json or manifest lists. Everything else down to the data files will not require a KMS.
| @@ -24,7 +24,7 @@ | |||
| import java.util.Map; | |||
|
|
|||
| /** A minimum client interface to connect to a key management service (KMS). */ | |||
| interface KeyManagementClient extends Serializable, Closeable { | |||
| public interface KeyManagementClient extends Serializable, Closeable { | |||
There was a problem hiding this comment.
I think this will necessarily need to be public so that implementations can reside in other modules, so I do agree with this change.
It'd be good if @rdblue can confirm that or if some other implementation pattern was intended here.
There was a problem hiding this comment.
Definitely looks like something which needs to be public
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java
Outdated
Show resolved
Hide resolved
To be used in table encryption for: - wrapping/unwrapping encryption keys - generating data keys (available specs: AES_256, AES_128) Added integration test for verification.
There was a problem hiding this comment.
Thanks @szlta looks good to me. Thanks @smaheshwar-pltr @pvary @nastra for reviewing!
|
Thanks everyone for the reviews! |
To be used in table encryption for:
Added integration test for verification.