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.
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.
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Could we parameterize this with @ParameterizedTest?
|
|
||
| assertThat(keyManagementClient.unwrapKey(result.wrappedKey(), keyId)).isEqualTo(result.key()); | ||
| assertThat(result.key().limit()) | ||
| .isEqualTo(DataKeySpec.AES_128.equals(dataKeySpec) ? 128 / 8 : 256 / 8); |
There was a problem hiding this comment.
Can we move this to a helper function expectedBytes(DataKeySpec spec)? I think it'd be easier to read that way
To be used in table encryption for:
Added integration test for verification.