Skip to content

AWS: KeyManagementClient implementation that works with AWS KMS - #13136

Merged
amogh-jahagirdar merged 4 commits into
apache:mainfrom
szlta:aws-kms
Jul 19, 2025
Merged

AWS: KeyManagementClient implementation that works with AWS KMS#13136
amogh-jahagirdar merged 4 commits into
apache:mainfrom
szlta:aws-kms

Conversation

@szlta

@szltaszlta commented May 23, 2025

Copy link
Copy Markdown
Contributor

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.

@szlta

szlta commented May 23, 2025

Copy link
Copy Markdown
ContributorAuthor

cc: @amogh-jahagirdar

@jbonofre

jbonofre commented May 28, 2025

Copy link
Copy Markdown
Member

@nastra@Fokko ^^

@jbonofre

jbonofre commented May 28, 2025

Copy link
Copy Markdown
Member

I will take a look as well. Thanks !

@amogh-jahagirdar
amogh-jahagirdar self-requested a review June 8, 2025 02:19
@amogh-jahagirdar

amogh-jahagirdar commented Jun 8, 2025

Copy link
Copy Markdown
Contributor

Sorry for the delay, I'm taking a look

amogh-jahagirdar
amogh-jahagirdar reviewed Jun 8, 2025
View reviewed changes

@amogh-jahagirdaramogh-jahagirdar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to take a pass over tests but left some comments!

Comment threadaws/src/main/java/org/apache/iceberg/aws/AwsProperties.java Outdated
Comment threadaws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java
* 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 {

@amogh-jahagirdaramogh-jahagirdarJun 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@smaheshwar-pltrsmaheshwar-pltrJun 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@szltaszltaJun 10, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadcore/src/main/java/org/apache/iceberg/encryption/KeyManagementClient.java

/** A minimum client interface to connect to a key management service (KMS). */
interface KeyManagementClient extends Serializable, Closeable {
public interface KeyManagementClient extends Serializable, Closeable {

@amogh-jahagirdaramogh-jahagirdarJun 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@smaheshwar-pltrsmaheshwar-pltrJun 8, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I believe that it being public lets catalogs / table operations in other packages use the KMS client - e.g. #13066 / #13225.

@pvarypvaryJul 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely looks like something which needs to be public

amogh-jahagirdar
amogh-jahagirdar reviewed Jun 17, 2025
View reviewed changes
Comment threadaws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java Outdated
Comment threadaws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java Outdated
pvary
pvary approved these changes Jul 18, 2025
View reviewed changes

@pvarypvary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amogh-jahagirdar: Any more comments?

nastra
nastra reviewed Jul 18, 2025
View reviewed changes
Comment threadaws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java Outdated
Comment threadaws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java Outdated
Comment threadaws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java Outdated
Comment threadaws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java Outdated
szlta added 4 commits July 18, 2025 17:32
@szlta
AWS: KeyManagementClient implementation that works with AWS KMS
c0d6510
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.
@szlta
Using AWS types in AwsProperties instead of Strings
351fd5b
@szlta
Refactors TestKeyManagementClient.java
10a5c85
@szlta
addressing comments from nastra
9507ab0
amogh-jahagirdar
amogh-jahagirdar approved these changes Jul 19, 2025
View reviewed changes

@amogh-jahagirdaramogh-jahagirdar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @szlta looks good to me. Thanks @smaheshwar-pltr@pvary@nastra for reviewing!

@amogh-jahagirdar
amogh-jahagirdar merged commit 77f1f5b into apache:mainJul 19, 2025
@szlta

szlta commented Jul 19, 2025

Copy link
Copy Markdown
ContributorAuthor

Thanks everyone for the reviews!

@malhotrashivammalhotrashivam mentioned this pull request Sep 11, 2025
Merged
@singhpk234singhpk234 mentioned this pull request Sep 16, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nastranastranastra left review comments
@pvarypvarypvary approved these changes
@amogh-jahagirdaramogh-jahagirdaramogh-jahagirdar approved these changes
+1 more reviewer
@smaheshwar-pltrsmaheshwar-pltrsmaheshwar-pltr left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AWScore

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@szlta@jbonofre@amogh-jahagirdar@nastra@pvary@smaheshwar-pltr