flyrain commented on code in PR #1424:
URL: https://github.com/apache/polaris/pull/1424#discussion_r2241219483
##########
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:
It seems more straightforward to generate the KMS policy separately directly
using `readLocations` and `writeLocations`, combining with the comment here:
https://github.com/apache/polaris/pull/1424/files#r2237156985. This approach
should allow us to support table-level KMS without additional complexity.
With that in place, we could simplify `KMS_SUPPORT_LEVEL_S3` to a simple
boolean (`enabled / disabled`). `PackedPolicyTooLargeException` is still a
valid concern, but we can surface a clear error if it ever occurs assuming it's
a edge case.
--
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
