flyrain commented on code in PR #1424:
URL: https://github.com/apache/polaris/pull/1424#discussion_r2176420967
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -198,7 +206,32 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+
+ policyBuilder.addStatement(allowGetObjectStatementBuilder.build());
+
+ if (isKMSSupported(callContext)) {
+ policyBuilder.addStatement(
+ IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("kms:GenerateDataKey")
+ .addAction("kms:Decrypt")
+ .addAction("kms:DescribeKey")
+ .addResource("arn:aws:kms:" + region + ":" + awsAccountId +
":key/*")
+ .addCondition(IamConditionOperator.STRING_EQUALS,
"aws:PrincipalArn", roleARN)
+ .addCondition(
+ IamConditionOperator.STRING_LIKE,
+ "kms:EncryptionContext:aws:s3:arn",
+ getArnPrefixFor(roleARN)
+ + StorageUtil.getBucket(
+
URI.create(awsStorageConfigurationInfo.getAllowedLocations().get(0)))
+ + "/*")
+ .addCondition(
+ IamConditionOperator.STRING_EQUALS,
+ "kms:ViaService",
+ "s3." + region + ".amazonaws.com")
Review Comment:
Does `"s3." + region + ".amazonaws.com"` work with GovCloud or CN
partitions? cc @adnanhemani
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -298,4 +298,12 @@ public static void enforceFeatureEnabledOrThrow(
+ "in their snapshot summary")
.defaultValue(false)
.buildFeatureConfiguration();
+
+ public static final FeatureConfiguration<Boolean> ENABLE_KMS_SUPPORT_FOR_S3 =
Review Comment:
Currently, KMS support is implemented at the catalog level, offering a
broader attach surface. We may introduce table-level KMS support in the future
to enhance security. Since users will need to choose one or the other, it makes
sense to reflect this in the configuration.
Instead of using `ENABLE_KMS_SUPPORT_FOR_S3` (a boolean), we could introduce
a new config option, `KMS_SUPPORT_LEVEL`, with possible values:
* `None` (default)
* `Catalog`
* `Table`
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -198,7 +206,32 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+
+ policyBuilder.addStatement(allowGetObjectStatementBuilder.build());
+
+ if (isKMSSupported(callContext)) {
+ policyBuilder.addStatement(
+ IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("kms:GenerateDataKey")
+ .addAction("kms:Decrypt")
+ .addAction("kms:DescribeKey")
+ .addResource("arn:aws:kms:" + region + ":" + awsAccountId +
":key/*")
+ .addCondition(IamConditionOperator.STRING_EQUALS,
"aws:PrincipalArn", roleARN)
+ .addCondition(
+ IamConditionOperator.STRING_LIKE,
+ "kms:EncryptionContext:aws:s3:arn",
+ getArnPrefixFor(roleARN)
+ + StorageUtil.getBucket(
+
URI.create(awsStorageConfigurationInfo.getAllowedLocations().get(0)))
Review Comment:
Does this support any valid S3-object-ARN prefix(e.g.,
`arn:aws:s3:::my-bucket/reports/2025/*`), not just `arn:aws:s3:::my-bucket/*`?
If that's so. We can create a policy for each individual s3 path in
`allowedLocations`, or even better, we can apply to each path in
`readLocations` and `writeLocations`, so that the polices only apply to
table-level locations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
