dimas-b commented on code in PR #2802:
URL: https://github.com/apache/polaris/pull/2802#discussion_r2535070103
##########
api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java:
##########
@@ -70,6 +72,8 @@ public void testJsonFormat() throws JsonProcessingException {
+ "\"properties\":{\"default-base-location\":\"s3://test/\"},"
+ "\"storageConfigInfo\":{"
+ "\"roleArn\":\"arn:aws:iam::123456789012:role/test-role\","
+ +
"\"currentKmsKey\":\"arn:aws:kms:us-east-1:012345678901:key/allowed-key-1\","
+ + "\"allowedKmsKeys\":[],"
Review Comment:
Could you make a separate test case for this? This "minimal" config is
intended to test that older clients do not get new optional properties.
##########
spec/polaris-management-service.yml:
##########
@@ -1103,6 +1103,16 @@ components:
type: string
description: the aws user arn used to assume the aws role
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
+ currentKmsKey:
+ type: string
+ description: the aws kms key arn used to encrypt s3 data
+ example: "arn:aws:kms::123456789001:key/01234578"
+ allowedKmsKeys:
+ type: array
+ description: the aws kms keys arn used to encrypt s3 data
Review Comment:
```suggestion
description: The list of kms keys that this catalog and its
clients are allow to use for reading s3 data
```
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -63,6 +64,14 @@ public String getFileIoImplClassName() {
@Nullable
public abstract String getRoleARN();
+ /** KMS Key ARN for server-side encryption,used for writes, optional */
+ @Nullable
+ public abstract String currentKmsKey();
Review Comment:
it might be preferable to follow the existing `get*` method name pattern for
JSON attributes in this class.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -239,7 +254,86 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+ var r =
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+ LOGGER.info("Policies {}", r);
Review Comment:
do we really need to log this all the time? Maybe `.debug()`?
nit: double space char in the message.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -239,7 +254,86 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+ var r =
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+ LOGGER.info("Policies {}", r);
+ return r;
+ }
+
+ private static void addKmsKeyPolicy(
+ String kmsKeyArn,
+ List<String> allowedKmsKeys,
+ IamPolicy.Builder policyBuilder,
+ boolean canEncrypt,
+ String region,
+ String accountId) {
+
+ IamStatement.Builder allowKms = buildBaseKmsStatement(canEncrypt);
+ boolean hasCurrentKey = kmsKeyArn != null;
+ boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+
+ if (hasCurrentKey) {
+ addKmsKeyResource(kmsKeyArn, allowKms);
+ }
+
+ if (hasAllowedKeys) {
+ addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
+ }
+
+ // Add KMS statement if we have any KMS key configuration
+ if (hasCurrentKey || hasAllowedKeys) {
+ policyBuilder.addStatement(allowKms.build());
+ } else if (!canEncrypt) {
+ // Only add wildcard KMS access for read-only operations when no
specific keys are configured
+ addAllKeysResource(region, accountId, allowKms);
+ policyBuilder.addStatement(allowKms.build());
+ }
+ }
+
+ private static IamStatement.Builder buildBaseKmsStatement(boolean
canEncrypt) {
+ IamStatement.Builder allowKms =
+ IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("kms:GenerateDataKeyWithoutPlaintext")
+ .addAction("kms:DescribeKey")
+ .addAction("kms:Decrypt")
+ .addAction("kms:GenerateDataKey");
+
+ if (canEncrypt) {
+ allowKms.addAction("kms:Encrypt");
+ }
+
+ return allowKms;
+ }
+
+ private static void addKmsKeyResource(String kmsKeyArn, IamStatement.Builder
allowKms) {
+ if (kmsKeyArn != null) {
+ LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn);
Review Comment:
too verbose? why not `.debug()` (in other places too)?
##########
CHANGELOG.md:
##########
@@ -74,6 +74,7 @@ request adding CHANGELOG notes for breaking (!) changes and
possibly other secti
### New Features
+- Updated catalogs creation to include AWS current kms key and allowed kms
key,as extra params in the storage config info, to be used for S3 data
encryption
Review Comment:
```suggestion
- Added KMS properties (optional) to catalog storage config to enable S3
data encryption.
```
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -239,7 +254,86 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+ var r =
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+ LOGGER.info("Policies {}", r);
+ return r;
+ }
+
+ private static void addKmsKeyPolicy(
+ String kmsKeyArn,
+ List<String> allowedKmsKeys,
+ IamPolicy.Builder policyBuilder,
+ boolean canEncrypt,
+ String region,
+ String accountId) {
+
+ IamStatement.Builder allowKms = buildBaseKmsStatement(canEncrypt);
+ boolean hasCurrentKey = kmsKeyArn != null;
+ boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+
+ if (hasCurrentKey) {
+ addKmsKeyResource(kmsKeyArn, allowKms);
+ }
+
+ if (hasAllowedKeys) {
+ addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
+ }
+
+ // Add KMS statement if we have any KMS key configuration
+ if (hasCurrentKey || hasAllowedKeys) {
+ policyBuilder.addStatement(allowKms.build());
+ } else if (!canEncrypt) {
Review Comment:
the `canEncrypt` really means `canWrite`, does it not? 🤔
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -70,7 +70,8 @@ public void testStsEndpoint() {
private static ImmutableAwsStorageConfigurationInfo.Builder newBuilder() {
return AwsStorageConfigurationInfo.builder()
- .roleARN("arn:aws:iam::123456789012:role/polaris-test");
+ .roleARN("arn:aws:iam::123456789012:role/polaris-test")
+ .currentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245");
Review Comment:
Is KMS key required in this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
