adding support to use a kms key for s3 buckets data encryption #2802

fabio-rizzo-01 · 2025-10-13T07:34:36Z

What changes were proposed in this pull request?

The addition of a kms key used to encrypt data in the s3 bucket

Why are the changes needed?

For anyone wish to use data encryption.

Does this PR introduce any user-facing change?

Yes, the addition of new param in the catalog creation : kms key arn

How was this patch tested?

Tested in AWS EKS

CHANGELOG.md

Update catalogs creation to include AWS kms key to be used in S3 data encryption

collado-mike · 2025-10-13T16:36:48Z

...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java

+      IamStatement.Builder allowKms =
+          IamStatement.builder()
+              .effect(IamEffect.ALLOW)
+              .addAction("kms:GenerateDataKeyWithoutPlaintext")
+              .addAction("kms:Encrypt")
+              .addAction("kms:DescribeKey")
+              .addAction("kms:Decrypt")
+              .addAction("kms:GenerateDataKey");


can we add the kms encryption context to this statement?

are you suggesting to allow the user to specify a new param during catalog creation for the encryption context condition?

No, I mean for SSE, the encryption context is always the path to the file. In our IAM policy statement, we can specify that the key can be used only to en/decrypt files in the encrpytion context pattern we specify - e.g., see https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingKMSEncryption.html and https://docs.aws.amazon.com/kms/latest/developerguide/encrypt_context.html#encryption-context-authorization . The user doesn't need to specify this parameter - it's automatic.

I don't think it is needed, the policy for the key and S3 bucket is restrictive enough and adding more restriction would cause the GenerateDataKey operation during the assume role to probably fail.

This is an example of a policy generated while creating a table:

IamPolicy(version=2012-10-17, statements=[ IamStatement(effect=IamEffect(value=Allow), actions=[IamAction(value=s3:PutObject), IamAction(value=s3:DeleteObject)], resources=[IamResource(value=arn:aws:s3:::app-id-bucket/fns/t3/*)]), IamStatement(effect=IamEffect(value=Allow), actions=[IamAction(value=kms:GenerateDataKeyWithoutPlaintext), IamAction(value=kms:DescribeKey), IamAction(value=kms:Decrypt), IamAction(value=kms:GenerateDataKey), IamAction(value=kms:Encrypt)], resources=[IamResource(value=arn:aws:kms:us-east-1:*********:key/id)]), IamStatement(effect=IamEffect(value=Allow), actions=[IamAction(value=s3:ListBucket)], resources=[IamResource(value=arn:aws:s3:::app-id-bucket)], conditions=[IamCondition(operator=StringLike, key=s3:prefix, value=fns/t3/*)]), IamStatement(effect=IamEffect(value=Allow), actions=[IamAction(value=s3:GetBucketLocation)], resources=[IamResource(value=arn:aws:s3:::app-id-bucket)]), IamStatement(effect=IamEffect(value=Allow), actions=[IamAction(value=s3:GetObject), IamAction(value=s3:GetObjectVersion)], resources=[IamResource(value=arn:aws:s3:::app-id-bucket/fns/t3/*)])])

Also in https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingKMSEncryption.html#encryption-context
it says that the bucket path is added by default in the encryption context.

Checking the encryption context in CloudTrail for the GenerateDataKey request I can see:

"encryptionContext": { "aws:s3:arn": "arn:aws:s3:::app-id-*****" }

singhpk234 · 2025-10-13T18:24:17Z

...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java

+              .addAction("kms:Encrypt")
+              .addAction("kms:DescribeKey")
+              .addAction("kms:Decrypt")


should we give encrypt or decrypt based on the priviledge ? for example TABLE_READ should get decrypt and TABLE_READ_AND_WRITE should both encrypt and decrypt ?

singhpk234 · 2025-10-13T18:29:11Z

spec/polaris-management-service.yml

              example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
+            kmsKeyArn:
+              type: string
+              description: the aws kms key arn used to encrypt s3 data


by s3 data we mean data stored in s3 or iceberg data files ?
if its just data stored in s3, a part of iceberg metadata is generated by polaris itself, do we need to plumb the encryption to the fileIO polaris uses ?

if its just iceberg data files how are we making sure its not used against iceberg metadata files such as manifest lists ?

the key is used by s3 to do encryption of any data in the bucket at rest, so as long as the IAM role has got the right entitlements on the key everything will work fine. I don't think anything needs to be changed on the polaris side.

I am talking of setting these properties in the FileIO which polaris uses - https://iceberg.apache.org/docs/nightly/aws/#s3-server-side-encryption or are you relying on these catalog properties seeping automatically to the FileIO

I'm not sure I follow I might be missing something, how would you specify the kms key in Polaris without adding in the storage config, also we need to add the policy in there to be able to use right? thx

dimas-b · 2025-10-14T15:47:20Z

What's the relationship of this PR and #1424 ?

dimas-b · 2025-10-14T15:49:25Z

spec/polaris-management-service.yml

              type: string
              description: the aws user arn used to assume the aws role
              example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
+            kmsKeyArn:


By convention, adding REST API parameters needs to be discussed on the dev ML. Could you start a thread there and reference this PR?

There is already a thread for this PR

My mistake... I did not see it 🤦

For ref: https://lists.apache.org/thread/to7lbdw1k6dym5c6gnk1nm32vqdw24qk

fabio-rizzo-01 · 2025-10-14T15:52:34Z

What's the relationship of this PR and #1424 ?

sorry I didn't see that one.

CHANGELOG.md

dimas-b · 2025-10-14T15:53:58Z

Re: #1424 - that PR appears to be dormant, so it's fine to offer an alternative. I was just trying to understand whether the feature is the same or not :)

Co-authored-by: Dmitri Bourlatchkov <[email protected]>

dimas-b · 2025-10-14T16:35:23Z

...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java


-    String arnPrefix = arnPrefixForPartition(awsPartition);
+    String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition());
+    String kmsKeyArn = storageConfigurationInfo.getKmsKeyArn();


Can / will this work with MinIO KMS? Cf. https://github.com/minio/minio/blob/master/docs/kms/README.md

I will need to test that.

It's fine either way, but if it did not work with MinIO it would be good to mention AWS (only) in the PR description.

Also, I'm pretty sure we'll have to support KMS in MinIO at some point, so if current changes were reusable it would be great (but not required).

adding support to use a kms key for s3 buckets

67fdaca

github-project-automation bot added this to Basic Kanban Board Oct 13, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Oct 13, 2025

fabio-rizzo-01 added 5 commits October 13, 2025 08:40

Update CHANGELOG.md

5cd4c4d

Merge branch 'main' into feature/s3-kms-key

7db0a87

Update CHANGELOG.md

cf4ec3e

Update CHANGELOG.md

ffbe979

fixing indentation issue

708630c

jbonofre self-requested a review October 13, 2025 14:27

collado-mike reviewed Oct 13, 2025

View reviewed changes

singhpk234 reviewed Oct 13, 2025

View reviewed changes

dimas-b reviewed Oct 14, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

5234672

Co-authored-by: Dmitri Bourlatchkov <[email protected]>

dimas-b reviewed Oct 14, 2025

View reviewed changes

adding support to use a kms key for s3 buckets data encryption #2802

Are you sure you want to change the base?

adding support to use a kms key for s3 buckets data encryption #2802

Uh oh!

Conversation

fabio-rizzo-01 commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

CHANGELOG.md

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabio-rizzo-01 commented Oct 14, 2025

Uh oh!

Uh oh!

dimas-b commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fabio-rizzo-01 commented Oct 13, 2025 •

edited

Loading

dimas-b commented Oct 14, 2025 •

edited

Loading

dimas-b Oct 14, 2025 •

edited

Loading

dimas-b Oct 15, 2025 •

edited

Loading