Skip to content

AWS: Support S3 DSSE-KMS encryption#8370

Merged
jackye1995 merged 7 commits into
apache:mainfrom
aajisaka:support-dsse-kms
May 24, 2024
Merged

AWS: Support S3 DSSE-KMS encryption#8370
jackye1995 merged 7 commits into
apache:mainfrom
aajisaka:support-dsse-kms

Conversation

@aajisaka

@aajisaka aajisaka commented Aug 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

This PR is to support S3 Dual-layer Server Side Encryption (DSSE) for Iceberg tables.

https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingDSSEncryption.html

nastra
nastra reviewed Aug 22, 2023
View reviewed changes
Comment thread aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3SignRequestParser.java Outdated
nastra
nastra reviewed Aug 22, 2023
View reviewed changes
Comment thread aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java Outdated
@nastra

nastra commented Aug 22, 2023

Copy link
Copy Markdown
Contributor

I think it would be better to split this into 2 PRs (one that only updates the AWS SDK version)

singhpk234
singhpk234 reviewed Aug 22, 2023
View reviewed changes
Comment thread docs/aws.md Outdated
Comment thread aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java
* If S3 encryption type is SSE-KMS, input is a KMS Key ID or ARN. In case this property is not
* set, default key "aws/s3" is used. If encryption type is SSE-C, input is a custom base-64
* AES256 symmetric key.
* If S3 encryption type is SSE-KMS or DSSE-KMS, input is a KMS Key ID or ARN. In case this

@singhpk234 singhpk234 Aug 22, 2023

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.

can rephrase this

@aajisaka aajisaka Aug 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, what does that exactly mean?

@singhpk234 singhpk234 Sep 27, 2023

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 see what i meant here was the comment describe an if else structure which makes it a bit hard to follow was wondering if we can re-write this comment in such as way that it's easy for a user to understand / read it.

This is just a nit and not a blocking comment, as you are just adding a disjunstion to an already existing if branch. please feel free to ignore.

@aajisaka

aajisaka commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

Thank you @nastra and @singhpk234 for your review.

Let me create a separate PR to upgrade AWS SDK version.

@aajisaka aajisaka mentioned this pull request Aug 23, 2023
Merged
@aajisaka

aajisaka commented Aug 23, 2023

Copy link
Copy Markdown
Member Author

Opened #8379 to upgrade AWS SDK version

nastra
nastra reviewed Aug 25, 2023
View reviewed changes
Comment thread python/dev/Dockerfile Outdated
nastra
nastra reviewed Aug 25, 2023
View reviewed changes
Comment thread aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3SignResponseParser.java Outdated
amogh-jahagirdar
amogh-jahagirdar reviewed Aug 29, 2023
View reviewed changes
Comment thread aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java Outdated
* <p>For more details:
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingDSSEncryption.html
*/
public static final String DSSE_TYPE_KMS = "kms.dsse";

@amogh-jahagirdar amogh-jahagirdar Aug 29, 2023

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.

The naming kms.dsse implies that DSSE will exist as a property namespace under KMS. Are there any other configurations which would actually go under this namespace? If not should we just call it dsse-kms?

@aajisaka aajisaka Aug 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, there isn't any other configuration go under this namespace. Changed to dsse-kms.

amogh-jahagirdar
amogh-jahagirdar approved these changes Aug 29, 2023
View reviewed changes

@amogh-jahagirdar amogh-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.

LGTM, thanks @aajisaka ! Will wait for @singhpk234 @jackye1995 to take a look before merging

singhpk234
singhpk234 approved these changes Sep 27, 2023
View reviewed changes

@singhpk234 singhpk234 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.

LGTM ! Thanks @aajisaka !

@Fokko Fokko removed the python label Oct 2, 2023
@aajisaka

aajisaka commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

From my side, is there something do to make it forward?

@aajisaka aajisaka force-pushed the support-dsse-kms branch from 8e8cf40 to 2598066 Compare May 20, 2024 08:23
@aajisaka

aajisaka commented May 20, 2024

Copy link
Copy Markdown
Member Author

Rebased for the latest main branch

jackye1995
jackye1995 approved these changes May 21, 2024
View reviewed changes

@jackye1995 jackye1995 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.

looks good to me!

@aajisaka

aajisaka commented May 24, 2024

Copy link
Copy Markdown
Member Author

I think this patch is ready to merge:

  • Built Iceberg and ran a Spark job using Glue 4.0. I confirmed the Spark job successfully created an Iceberg table under the S3 prefix which is configured to reject any PutObject request without DSSE-KMS encryption.
  • Ran integration tests and TestS3FileIOIntegration passed.

cc: @jackye1995 @amogh-jahagirdar

@jackye1995

jackye1995 commented May 24, 2024

Copy link
Copy Markdown
Contributor

Was waiting for @nastra , but I agree this is ready to be merged, I will go ahead to do that, thanks for the contribution!

@jackye1995 jackye1995 merged commit 311dbbb into apache:main May 24, 2024
@aajisaka

aajisaka commented May 25, 2024

Copy link
Copy Markdown
Member Author

Thanks!

@aajisaka aajisaka deleted the support-dsse-kms branch May 25, 2024 06:52
sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024
AWS: Support S3 DSSE-KMS encryption (apache#8370)
82c6a4a
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
@zachdisc
AWS: Support S3 DSSE-KMS encryption (apache#8370)
295aa8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nastra nastra nastra left review comments
@jackye1995 jackye1995 jackye1995 approved these changes
@singhpk234 singhpk234 singhpk234 approved these changes
@amogh-jahagirdar amogh-jahagirdar amogh-jahagirdar approved these changes

Assignees

No one assigned

Labels

AWS docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aajisaka @nastra @jackye1995 @singhpk234 @amogh-jahagirdar @Fokko