adnanhemani commented on code in PR #1424:
URL: https://github.com/apache/polaris/pull/1424#discussion_r2105883584
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -185,7 +190,32 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+
+ policyBuilder.addStatement(allowGetObjectStatementBuilder.build());
+
+ 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(
Review Comment:
Technically, URI should not be used with S3 paths. See #1604.
But this requires further changes in the `StorageUtil` class too...
Let us add this to the underlying issue (#1545)
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -185,7 +190,32 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+
+ policyBuilder.addStatement(allowGetObjectStatementBuilder.build());
+
+ 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().iterator().next()))
Review Comment:
Also, shouldn't we be giving one IAM statement for each of the allowed
locations? What if the location that's being accessed isn't the first allowed
location?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -185,7 +190,32 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+
+ policyBuilder.addStatement(allowGetObjectStatementBuilder.build());
+
+ 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().iterator().next()))
Review Comment:
```suggestion
awsStorageConfigurationInfo.getAllowedLocations().getFirst()))
```
--
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
