From 67fdacaf628366df33e99031f110a598cfb49576 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 10 Oct 2025 16:52:53 +0100 Subject: [PATCH 01/21] adding support to use a kms key for s3 buckets --- .../polaris/core/entity/CatalogEntity.java | 2 + .../aws/AwsCredentialsStorageIntegration.java | 25 +++++-- .../aws/AwsStorageConfigurationInfo.java | 4 ++ .../aws/AwsStorageConfigurationInfoTest.java | 3 +- .../AwsCredentialsStorageIntegrationTest.java | 68 +++++++++++++++++++ .../service/entity/CatalogEntityTest.java | 4 ++ spec/polaris-management-service.yml | 4 ++ 7 files changed, 105 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 055ccd8959..3b3a656801 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,6 +161,7 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) + .setKmsKeyArn(awsConfig.getKmsKeyArn()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) @@ -308,6 +309,7 @@ public Builder setStorageConfigurationInfo( AwsStorageConfigurationInfo.builder() .allowedLocations(allowedLocations) .roleARN(awsConfigModel.getRoleArn()) + .kmsKeyArn(awsConfigModel.getKmsKeyArn()) .externalId(awsConfigModel.getExternalId()) .region(awsConfigModel.getRegion()) .endpoint(awsConfigModel.getEndpoint()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 8023f7a607..2d5e685995 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -90,7 +90,7 @@ public AccessConfig getSubscopedCreds( .roleSessionName("PolarisAwsCredentialsStorageIntegration") .policy( policyString( - storageConfig.getAwsPartition(), + storageConfig, allowListOperation, allowedReadLocations, allowedWriteLocations) @@ -163,9 +163,8 @@ private boolean shouldUseSts(AwsStorageConfigurationInfo storageConfig) { * ListBucket privileges with no resources. This prevents us from sending an empty policy to AWS * and just assuming the role with full privileges. */ - // TODO - add KMS key access private IamPolicy policyString( - String awsPartition, + AwsStorageConfigurationInfo storageConfigurationInfo, boolean allowList, Set readLocations, Set writeLocations) { @@ -178,7 +177,10 @@ private IamPolicy policyString( Map bucketListStatementBuilder = new HashMap<>(); Map bucketGetLocationStatementBuilder = new HashMap<>(); - String arnPrefix = arnPrefixForPartition(awsPartition); + String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); + String kmsKeyArn = storageConfigurationInfo.getKmsKeyArn(); + addKmsKeyPolicy(kmsKeyArn, policyBuilder); + Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -242,6 +244,21 @@ private IamPolicy policyString( return policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); } + private static void addKmsKeyPolicy(String kmsKeyArn, IamPolicy.Builder policyBuilder) { + if (kmsKeyArn != null) { + IamStatement.Builder allowKms = + IamStatement.builder() + .effect(IamEffect.ALLOW) + .addAction("kms:GenerateDataKeyWithoutPlaintext") + .addAction("kms:Encrypt") + .addAction("kms:DescribeKey") + .addAction("kms:Decrypt") + .addAction("kms:GenerateDataKey"); + allowKms.addResource(IamResource.create(kmsKeyArn)); + policyBuilder.addStatement(allowKms.build()); + } + } + private static String arnPrefixForPartition(String awsPartition) { return String.format("arn:%s:s3:::", awsPartition != null ? awsPartition : "aws"); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index b3d7d60790..05692e85e5 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -63,6 +63,10 @@ public String getFileIoImplClassName() { @Nullable public abstract String getRoleARN(); + /** KMS Key ARN for server-side encryption, optional */ + @Nullable + public abstract String getKmsKeyArn(); + /** AWS external ID, optional */ @Nullable public abstract String getExternalId(); diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 3460dc23f7..e5f0e79e1d 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/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") + .kmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java index 7b4b50dece..267073e12c 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java @@ -542,6 +542,74 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { String.valueOf(EXPIRE_TIME.toEpochMilli())); } + @Test + public void testGetSubscopedCredsInlinePolicyWithKmsKey() { + StsClient stsClient = Mockito.mock(StsClient.class); + String roleARN = "arn:aws:iam::012345678901:role/jdoe"; + String externalId = "externalId"; + String bucket = "bucket"; + String warehouseKeyPrefix = "path/to/warehouse"; + String firstPath = warehouseKeyPrefix + "/namespace/table"; + String kmsKeyArn = "arn:aws:kms:us-east-1:012345678901:key/444343245"; + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + assertThat(invocation.getArguments()[0]) + .isInstanceOf(AssumeRoleRequest.class) + .asInstanceOf(InstanceOfAssertFactories.type(AssumeRoleRequest.class)) + .extracting(AssumeRoleRequest::policy) + .extracting(IamPolicy::fromJson) + .satisfies( + policy -> { + assertThat(policy) + .extracting(IamPolicy::statements) + .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) + .hasSize(5) + .anySatisfy( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:Encrypt"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of(IamResource.create(kmsKeyArn)), + IamStatement::resources)); + }); + return ASSUME_ROLE_RESPONSE; + }); + AccessConfig accessConfig = + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region("us-east-1") + .kmsKeyArn(kmsKeyArn) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, firstPath)), + Set.of(s3Path(bucket, firstPath)), + Optional.empty()); + assertThat(accessConfig.credentials()) + .isNotEmpty() + .containsEntry(StorageAccessProperty.AWS_TOKEN.getPropertyName(), "sess") + .containsEntry(StorageAccessProperty.AWS_KEY_ID.getPropertyName(), "accessKey") + .containsEntry(StorageAccessProperty.AWS_SECRET_KEY.getPropertyName(), "secretKey") + .containsEntry( + StorageAccessProperty.AWS_SESSION_TOKEN_EXPIRES_AT_MS.getPropertyName(), + String.valueOf(EXPIRE_TIME.toEpochMilli())); + } + @ParameterizedTest @ValueSource(strings = {AWS_PARTITION, "aws-cn", "aws-us-gov"}) public void testClientRegion(String awsPartition) { diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index 47e61f5734..6ef467fc78 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -292,6 +292,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -305,6 +306,7 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + assertThat(((AwsStorageConfigInfo)catalog.getStorageConfigInfo()).getKmsKeyArn()).isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test @@ -313,6 +315,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -328,6 +331,7 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); + assertThat(((AwsStorageConfigInfo)catalog.getStorageConfigInfo()).getKmsKeyArn()).isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index 59baaf99d8..ed22cd95d2 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1103,6 +1103,10 @@ components: type: string description: the aws user arn used to assume the aws role example: "arn:aws:iam::123456789001:user/abc1-b-self1234" + kmsKeyArn: + type: string + description: the aws kms key arn used to encrypt s3 data + example: "arn:aws:kms::123456789001:key/01234578" region: type: string description: the aws region where data is stored From 5cd4c4da50f9eb4c807ee282f6b28a6cf17af404 Mon Sep 17 00:00:00 2001 From: fabio-rizzo-01 Date: Mon, 13 Oct 2025 08:40:05 +0100 Subject: [PATCH 02/21] Update CHANGELOG.md --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfeb0f861f..99f32dc47e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### New Features +- Updated catalogs creation to include AWS kms key ,as an extra param in the storage config info, to be used in S3 data encryption - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. @@ -167,4 +168,4 @@ Apache Polaris 0.9.0 was released on March 11, 2025 as the first Polaris release [Unreleased]: https://github.com/apache/polaris/commits [1.0.1-incubating]: https://github.com/apache/polaris/releases/tag/apache-polaris-1.0.1-incubating [1.0.0-incubating]: https://github.com/apache/polaris/releases/tag/apache-polaris-1.0.0-incubating-rc2 -[0.9.0-incubating]: https://github.com/apache/polaris/releases/tag/apache-polaris-0.9.0-incubating \ No newline at end of file +[0.9.0-incubating]: https://github.com/apache/polaris/releases/tag/apache-polaris-0.9.0-incubating From cf4ec3eb1736a4dd5d3fad57df82c60d2dad5f05 Mon Sep 17 00:00:00 2001 From: fabio-rizzo-01 Date: Mon, 13 Oct 2025 09:01:16 +0100 Subject: [PATCH 03/21] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75ddffff8c..67acaee352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -193,4 +193,4 @@ Apache Polaris 0.9.0 was released on March 11, 2025 as the first Polaris release [1.1.0-incubating]: https://github.com/apache/polaris/compare/apache-polaris-1.0.1-incubating...apache-polaris-1.1.0-incubating [1.0.1-incubating]: https://github.com/apache/polaris/compare/apache-polaris-1.0.0-incubating...apache-polaris-1.0.1-incubating [1.0.0-incubating]: https://github.com/apache/polaris/compare/apache-polaris-0.9.0-incubating...apache-polaris-1.0.0-incubating -[0.9.0-incubating]: https://github.com/apache/polaris/commits/apache-polaris-0.9.0-incubating \ No newline at end of file +[0.9.0-incubating]: https://github.com/apache/polaris/commits/apache-polaris-0.9.0-incubating From ffbe979e33fe6b01fd221da4677c7df642efa5b5 Mon Sep 17 00:00:00 2001 From: fabio-rizzo-01 Date: Mon, 13 Oct 2025 09:02:43 +0100 Subject: [PATCH 04/21] Update CHANGELOG.md From 5a0f3eb3941f092c1f61e53a53883651541dfe80 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Mon, 13 Oct 2025 09:19:38 +0100 Subject: [PATCH 05/21] fixing indentation issue --- .../apache/polaris/service/entity/CatalogEntityTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index 6ef467fc78..344a2764d8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -306,7 +306,8 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); - assertThat(((AwsStorageConfigInfo)catalog.getStorageConfigInfo()).getKmsKeyArn()).isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test @@ -331,7 +332,8 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); - assertThat(((AwsStorageConfigInfo)catalog.getStorageConfigInfo()).getKmsKeyArn()).isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test From 708630c25f44f6fe7652346a449d8bfb65f63eeb Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Mon, 13 Oct 2025 09:19:38 +0100 Subject: [PATCH 06/21] fixing indentation issue --- .../apache/polaris/service/entity/CatalogEntityTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index 6ef467fc78..344a2764d8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -306,7 +306,8 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); - assertThat(((AwsStorageConfigInfo)catalog.getStorageConfigInfo()).getKmsKeyArn()).isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test @@ -331,7 +332,8 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); - assertThat(((AwsStorageConfigInfo)catalog.getStorageConfigInfo()).getKmsKeyArn()).isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test From 3a10f7dfe2abc9c13cfe6058d20d0dded81dd85e Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Tue, 21 Oct 2025 09:33:33 +0100 Subject: [PATCH 07/21] temporary changes to handle iceberg s3 properties --- .../AtomicOperationMetaStoreManager.java | 6 +- .../TransactionWorkspaceMetaStoreManager.java | 6 +- .../TransactionalMetaStoreManagerImpl.java | 6 +- .../core/storage/PolarisCredentialVendor.java | 4 +- .../storage/PolarisStorageIntegration.java | 3 +- .../aws/AwsCredentialsStorageIntegration.java | 70 +++++++++++++++---- .../AzureCredentialsStorageIntegration.java | 4 +- .../storage/cache/StorageCredentialCache.java | 6 +- .../gcp/GcpCredentialsStorageIntegration.java | 3 +- .../AwsCredentialsStorageIntegrationTest.java | 4 +- .../catalog/io/AccessConfigProvider.java | 3 +- .../catalog/io/DefaultFileIOFactory.java | 10 ++- .../service/catalog/io/FileIOUtil.java | 7 +- ...PolarisStorageIntegrationProviderImpl.java | 3 +- .../iceberg/AbstractIcebergCatalogTest.java | 4 +- 15 files changed, 104 insertions(+), 35 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java index e2c46c1515..5725244746 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java @@ -1601,7 +1601,8 @@ private void revokeGrantRecord( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { // get meta store session we should be using BasePersistence ms = callCtx.getMetaStore(); @@ -1642,7 +1643,8 @@ private void revokeGrantRecord( allowListOperation, allowedReadLocations, allowedWriteLocations, - refreshCredentialsEndpoint); + refreshCredentialsEndpoint, + props); return new ScopedCredentialsResult(accessConfig); } catch (Exception ex) { return new ScopedCredentialsResult( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java index 8729558935..408b070747 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java @@ -346,7 +346,8 @@ public ScopedCredentialsResult getSubscopedCredsForEntity( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { return delegate.getSubscopedCredsForEntity( callCtx, catalogId, @@ -355,7 +356,8 @@ public ScopedCredentialsResult getSubscopedCredsForEntity( allowListOperation, allowedReadLocations, allowedWriteLocations, - refreshCredentialsEndpoint); + refreshCredentialsEndpoint, + props); } @Override diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index 402cdc280e..1e744399bb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -2095,7 +2095,8 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { // get meta store session we should be using TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore()); @@ -2131,7 +2132,8 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( allowListOperation, allowedReadLocations, allowedWriteLocations, - refreshCredentialsEndpoint); + refreshCredentialsEndpoint, + props); return new ScopedCredentialsResult(accessConfig); } catch (Exception ex) { return new ScopedCredentialsResult( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java index d64e9ad88c..461dec28ff 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java @@ -19,6 +19,7 @@ package org.apache.polaris.core.storage; import jakarta.annotation.Nonnull; +import java.util.Map; import java.util.Optional; import java.util.Set; import org.apache.polaris.core.PolarisCallContext; @@ -53,5 +54,6 @@ ScopedCredentialsResult getSubscopedCredsForEntity( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint); + Optional refreshCredentialsEndpoint, + Map props); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java index 1828d01c81..5b0e34fce2 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java @@ -67,7 +67,8 @@ public abstract AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint); + Optional refreshCredentialsEndpoint, + Map props); /** * Validate access for the provided operation actions and locations. diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 2d5e685995..f755d860bc 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -33,6 +33,8 @@ import org.apache.polaris.core.storage.StorageAccessProperty; import org.apache.polaris.core.storage.StorageUtil; import org.apache.polaris.core.storage.aws.StsClientProvider.StsDestination; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.policybuilder.iam.IamConditionOperator; import software.amazon.awssdk.policybuilder.iam.IamEffect; @@ -49,6 +51,9 @@ public class AwsCredentialsStorageIntegration private final StsClientProvider stsClientProvider; private final Optional credentialsProvider; + private static final Logger LOGGER = + LoggerFactory.getLogger(AwsCredentialsStorageIntegration.class); + public AwsCredentialsStorageIntegration( AwsStorageConfigurationInfo config, StsClient fixedClient) { this(config, (destination) -> fixedClient); @@ -75,11 +80,15 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { + LOGGER.info("Getting subscoped creds props: {}", props); + String kmsKey = props.get("s3.sse.key") != null ? props.get("s3.sse.key").toString() : null; int storageCredentialDurationSeconds = realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); AwsStorageConfigurationInfo storageConfig = config(); String region = storageConfig.getRegion(); + String accountId = storageConfig.getAwsAccountId(); AccessConfig.Builder accessConfig = AccessConfig.builder(); if (shouldUseSts(storageConfig)) { @@ -93,7 +102,10 @@ public AccessConfig getSubscopedCreds( storageConfig, allowListOperation, allowedReadLocations, - allowedWriteLocations) + allowedWriteLocations, + kmsKey, + region, + accountId) .toJson()) .durationSeconds(storageCredentialDurationSeconds); credentialsProvider.ifPresent( @@ -167,7 +179,10 @@ private IamPolicy policyString( AwsStorageConfigurationInfo storageConfigurationInfo, boolean allowList, Set readLocations, - Set writeLocations) { + Set writeLocations, + String kmsKey, + String region, + String accountId) { IamPolicy.Builder policyBuilder = IamPolicy.builder(); IamStatement.Builder allowGetObjectStatementBuilder = IamStatement.builder() @@ -179,7 +194,6 @@ private IamPolicy policyString( String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); String kmsKeyArn = storageConfigurationInfo.getKmsKeyArn(); - addKmsKeyPolicy(kmsKeyArn, policyBuilder); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() @@ -227,6 +241,9 @@ private IamPolicy policyString( arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/"))); }); policyBuilder.addStatement(allowPutObjectStatementBuilder.build()); + addKmsKeyPolicy(kmsKey, kmsKeyArn, policyBuilder, true, region, accountId); + } else { + addKmsKeyPolicy(kmsKey, kmsKeyArn, policyBuilder, false, region, accountId); } if (!bucketListStatementBuilder.isEmpty()) { bucketListStatementBuilder @@ -241,22 +258,45 @@ 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, IamPolicy.Builder policyBuilder) { + private static void addKmsKeyPolicy( + String kmsKeyArnOverride, + String kmsKeyArn, + IamPolicy.Builder policyBuilder, + boolean canEncrypt, + String region, + String accountId) { + if (kmsKeyArn == null && kmsKeyArnOverride == null) { + kmsKeyArn = arnKeyAll(region, accountId); + } + 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"); + } + if (kmsKeyArnOverride != null) { + LOGGER.info("Adding KMS key policy for key {}", kmsKeyArnOverride); + allowKms.addResource(IamResource.create(kmsKeyArnOverride)); + } if (kmsKeyArn != null) { - IamStatement.Builder allowKms = - IamStatement.builder() - .effect(IamEffect.ALLOW) - .addAction("kms:GenerateDataKeyWithoutPlaintext") - .addAction("kms:Encrypt") - .addAction("kms:DescribeKey") - .addAction("kms:Decrypt") - .addAction("kms:GenerateDataKey"); + LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn); allowKms.addResource(IamResource.create(kmsKeyArn)); - policyBuilder.addStatement(allowKms.build()); } + policyBuilder.addStatement(allowKms.build()); + } + + private static String arnKeyAll(String region, String accountId) { + return String.format("arn:aws:kms:%s:%s:key/*", region, accountId); } private static String arnPrefixForPartition(String awsPartition) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java index a043a7daa5..af1948fb57 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java @@ -45,6 +45,7 @@ import java.time.ZoneOffset; import java.time.temporal.ChronoUnit; import java.util.HashSet; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -78,7 +79,8 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { String loc = !allowedWriteLocations.isEmpty() ? allowedWriteLocations.stream().findAny().orElse(null) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java index 82de799152..f6e2699138 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java @@ -110,7 +110,8 @@ public AccessConfig getOrGenerateSubScopeCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { if (!isTypeSupported(polarisEntity.getType())) { diagnostics.fail( "entity_type_not_suppported_to_scope_creds", "type={}", polarisEntity.getType()); @@ -136,7 +137,8 @@ public AccessConfig getOrGenerateSubScopeCreds( k.allowedListAction(), k.allowedReadLocations(), k.allowedWriteLocations(), - k.refreshCredentialsEndpoint()); + k.refreshCredentialsEndpoint(), + props); if (scopedCredentialsResult.isSuccess()) { long maxCacheDurationMs = maxCacheDurationMs(callCtx.getRealmConfig()); return new StorageCredentialCacheEntry( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java index c0568cc9b5..eddb67a834 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java @@ -77,7 +77,8 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { try { sourceCredentials.refresh(); } catch (IOException e) { diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java index 267073e12c..279dbf026c 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java @@ -573,10 +573,10 @@ public void testGetSubscopedCredsInlinePolicyWithKmsKey() { List.of( IamAction.create( "kms:GenerateDataKeyWithoutPlaintext"), - IamAction.create("kms:Encrypt"), IamAction.create("kms:DescribeKey"), IamAction.create("kms:Decrypt"), - IamAction.create("kms:GenerateDataKey")), + IamAction.create("kms:GenerateDataKey"), + IamAction.create("kms:Encrypt")), IamStatement::actions) .returns( List.of(IamResource.create(kmsKeyArn)), diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java index d336040273..d33c08af34 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java @@ -99,6 +99,7 @@ public AccessConfig getAccessConfig( tableLocations, storageActions, storageInfo.get(), - refreshCredentialsEndpoint); + refreshCredentialsEndpoint, + new java.util.HashMap<>()); } } diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java index 44f038d72f..4f1068d4ee 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java @@ -39,6 +39,8 @@ import org.apache.polaris.core.storage.PolarisCredentialVendor; import org.apache.polaris.core.storage.PolarisStorageActions; import org.apache.polaris.core.storage.cache.StorageCredentialCache; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A default FileIO factory implementation for creating Iceberg {@link FileIO} instances with @@ -54,6 +56,7 @@ public class DefaultFileIOFactory implements FileIOFactory { private final StorageCredentialCache storageCredentialCache; private final MetaStoreManagerFactory metaStoreManagerFactory; + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultFileIOFactory.class); @Inject public DefaultFileIOFactory( @@ -77,7 +80,10 @@ public FileIO loadFileIO( metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext); // Get subcoped creds + + LOGGER.info("Properties before adding scoped credentials: {}", properties); properties = new HashMap<>(properties); + final Map newProps = new HashMap<>(properties); Optional storageInfoEntity = FileIOUtil.findStorageInfoFromHierarchy(resolvedEntityPath); Optional accessConfig = @@ -91,12 +97,14 @@ public FileIO loadFileIO( tableLocations, storageActions, storageInfo, - Optional.empty())); + Optional.empty(), + newProps)); // Update the FileIO with the subscoped credentials // Update with properties in case there are table-level overrides the credentials should // always override table-level properties, since storage configuration will be found at // whatever entity defines it + if (accessConfig.isPresent()) { properties.putAll(accessConfig.get().credentials()); properties.putAll(accessConfig.get().extraProperties()); diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java index f4a6320d67..00815b7b58 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java @@ -18,6 +18,7 @@ */ package org.apache.polaris.service.catalog.io; +import java.util.Map; import java.util.Optional; import java.util.Set; import org.apache.iceberg.catalog.TableIdentifier; @@ -82,7 +83,8 @@ public static AccessConfig refreshAccessConfig( Set tableLocations, Set storageActions, PolarisEntity entity, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { boolean skipCredentialSubscopingIndirection = callContext @@ -113,7 +115,8 @@ public static AccessConfig refreshAccessConfig( allowList, tableLocations, writeLocations, - refreshCredentialsEndpoint); + refreshCredentialsEndpoint, + props); LOGGER .atDebug() .addKeyValue("tableIdentifier", tableIdentifier) diff --git a/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java b/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java index 23ec20abc3..c2653a6857 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java @@ -114,7 +114,8 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint) { + Optional refreshCredentialsEndpoint, + Map props) { return AccessConfig.builder().supportsCredentialVending(false).build(); } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java index 369a672520..3484e1c83c 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java @@ -44,6 +44,7 @@ import java.lang.reflect.Method; import java.time.Clock; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -1904,7 +1905,8 @@ public void testDropTableWithPurge() { true, Set.of(tableMetadata.location()), Set.of(tableMetadata.location()), - Optional.empty()) + Optional.empty(), + Collections.emptyMap()) .getAccessConfig() .credentials(); Assertions.assertThat(credentials) From d0907a17ef271cb279ec6d17276f7fbee51d91da Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 11:39:53 +0000 Subject: [PATCH 08/21] removed s3 key table changes. added new properties for AwsStorageConfigurationInfo, currentKmsKey and allowedKmsKeys --- .../admin/model/CatalogSerializationTest.java | 4 + .../polaris/core/entity/CatalogEntity.java | 6 +- .../AtomicOperationMetaStoreManager.java | 6 +- .../TransactionWorkspaceMetaStoreManager.java | 6 +- .../TransactionalMetaStoreManagerImpl.java | 6 +- .../core/storage/PolarisCredentialVendor.java | 4 +- .../storage/PolarisStorageIntegration.java | 3 +- .../aws/AwsCredentialsStorageIntegration.java | 72 +++-- .../aws/AwsStorageConfigurationInfo.java | 9 +- .../AzureCredentialsStorageIntegration.java | 4 +- .../storage/cache/StorageCredentialCache.java | 6 +- .../gcp/GcpCredentialsStorageIntegration.java | 3 +- .../aws/AwsStorageConfigurationInfoTest.java | 2 +- .../AwsCredentialsStorageIntegrationTest.java | 289 +++++++++++++----- .../catalog/io/AccessConfigProvider.java | 3 +- .../service/catalog/io/FileIOUtil.java | 7 +- ...PolarisStorageIntegrationProviderImpl.java | 3 +- spec/polaris-management-service.yml | 8 +- 18 files changed, 310 insertions(+), 131 deletions(-) diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c4210486ba..c645f086cb 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -35,6 +35,7 @@ public class CatalogSerializationTest { private static final String TEST_LOCATION = "s3://test/"; private static final String TEST_CATALOG_NAME = "test-catalog"; private static final String TEST_ROLE_ARN = "arn:aws:iam::123456789012:role/test-role"; + private static final String KMS_KEY = "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1"; @BeforeEach public void setUp() { @@ -59,6 +60,7 @@ public void testJsonFormat() throws JsonProcessingException { new CatalogProperties(TEST_LOCATION), AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) .setRoleArn(TEST_ROLE_ARN) + .setCurrentKmsKey(KMS_KEY) .build()); String json = mapper.writeValueAsString(catalog); @@ -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\":[]," + "\"pathStyleAccess\":false," + "\"storageType\":\"S3\"," + "\"allowedLocations\":[]" diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 4d78ddf778..095c950aeb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,7 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) - .setKmsKeyArn(awsConfig.getKmsKeyArn()) + .setCurrentKmsKey(awsConfig.currentKmsKey()) + .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) @@ -309,7 +310,8 @@ public Builder setStorageConfigurationInfo( AwsStorageConfigurationInfo.builder() .allowedLocations(allowedLocations) .roleARN(awsConfigModel.getRoleArn()) - .kmsKeyArn(awsConfigModel.getKmsKeyArn()) + .currentKmsKey(awsConfigModel.getCurrentKmsKey()) + .allowedKmsKeys(awsConfigModel.getAllowedKmsKeys()) .externalId(awsConfigModel.getExternalId()) .region(awsConfigModel.getRegion()) .endpoint(awsConfigModel.getEndpoint()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java index 67a2486214..c3841486a7 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java @@ -1598,8 +1598,7 @@ private void revokeGrantRecord( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { // get meta store session we should be using BasePersistence ms = callCtx.getMetaStore(); @@ -1640,8 +1639,7 @@ private void revokeGrantRecord( allowListOperation, allowedReadLocations, allowedWriteLocations, - refreshCredentialsEndpoint, - props); + refreshCredentialsEndpoint); return new ScopedCredentialsResult(accessConfig); } catch (Exception ex) { return new ScopedCredentialsResult( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java index 408b070747..8729558935 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java @@ -346,8 +346,7 @@ public ScopedCredentialsResult getSubscopedCredsForEntity( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { return delegate.getSubscopedCredsForEntity( callCtx, catalogId, @@ -356,8 +355,7 @@ public ScopedCredentialsResult getSubscopedCredsForEntity( allowListOperation, allowedReadLocations, allowedWriteLocations, - refreshCredentialsEndpoint, - props); + refreshCredentialsEndpoint); } @Override diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java index 1e744399bb..402cdc280e 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java @@ -2095,8 +2095,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { // get meta store session we should be using TransactionalPersistence ms = ((TransactionalPersistence) callCtx.getMetaStore()); @@ -2132,8 +2131,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant( allowListOperation, allowedReadLocations, allowedWriteLocations, - refreshCredentialsEndpoint, - props); + refreshCredentialsEndpoint); return new ScopedCredentialsResult(accessConfig); } catch (Exception ex) { return new ScopedCredentialsResult( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java index 461dec28ff..d64e9ad88c 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisCredentialVendor.java @@ -19,7 +19,6 @@ package org.apache.polaris.core.storage; import jakarta.annotation.Nonnull; -import java.util.Map; import java.util.Optional; import java.util.Set; import org.apache.polaris.core.PolarisCallContext; @@ -54,6 +53,5 @@ ScopedCredentialsResult getSubscopedCredsForEntity( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props); + Optional refreshCredentialsEndpoint); } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java index 5b0e34fce2..1828d01c81 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java @@ -67,8 +67,7 @@ public abstract AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props); + Optional refreshCredentialsEndpoint); /** * Validate access for the provided operation actions and locations. diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index f755d860bc..0c114f5b16 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -23,6 +23,7 @@ import jakarta.annotation.Nonnull; import java.net.URI; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -80,10 +81,7 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { - LOGGER.info("Getting subscoped creds props: {}", props); - String kmsKey = props.get("s3.sse.key") != null ? props.get("s3.sse.key").toString() : null; + Optional refreshCredentialsEndpoint) { int storageCredentialDurationSeconds = realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); AwsStorageConfigurationInfo storageConfig = config(); @@ -103,7 +101,6 @@ public AccessConfig getSubscopedCreds( allowListOperation, allowedReadLocations, allowedWriteLocations, - kmsKey, region, accountId) .toJson()) @@ -180,7 +177,6 @@ private IamPolicy policyString( boolean allowList, Set readLocations, Set writeLocations, - String kmsKey, String region, String accountId) { IamPolicy.Builder policyBuilder = IamPolicy.builder(); @@ -193,7 +189,8 @@ private IamPolicy policyString( Map bucketGetLocationStatementBuilder = new HashMap<>(); String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); - String kmsKeyArn = storageConfigurationInfo.getKmsKeyArn(); + String currentKmsKey = storageConfigurationInfo.currentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() @@ -241,9 +238,9 @@ private IamPolicy policyString( arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/"))); }); policyBuilder.addStatement(allowPutObjectStatementBuilder.build()); - addKmsKeyPolicy(kmsKey, kmsKeyArn, policyBuilder, true, region, accountId); + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, true, region, accountId); } else { - addKmsKeyPolicy(kmsKey, kmsKeyArn, policyBuilder, false, region, accountId); + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, false, region, accountId); } if (!bucketListStatementBuilder.isEmpty()) { bucketListStatementBuilder @@ -264,15 +261,36 @@ private IamPolicy policyString( } private static void addKmsKeyPolicy( - String kmsKeyArnOverride, String kmsKeyArn, + List allowedKmsKeys, IamPolicy.Builder policyBuilder, boolean canEncrypt, String region, String accountId) { - if (kmsKeyArn == null && kmsKeyArnOverride == null) { - kmsKeyArn = arnKeyAll(region, 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) @@ -284,15 +302,35 @@ private static void addKmsKeyPolicy( if (canEncrypt) { allowKms.addAction("kms:Encrypt"); } - if (kmsKeyArnOverride != null) { - LOGGER.info("Adding KMS key policy for key {}", kmsKeyArnOverride); - allowKms.addResource(IamResource.create(kmsKeyArnOverride)); - } + + return allowKms; + } + + private static void addKmsKeyResource(String kmsKeyArn, IamStatement.Builder allowKms) { if (kmsKeyArn != null) { LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn); allowKms.addResource(IamResource.create(kmsKeyArn)); } - policyBuilder.addStatement(allowKms.build()); + } + + private static boolean hasAllowedKmsKeys(List allowedKmsKeys) { + return allowedKmsKeys != null && !allowedKmsKeys.isEmpty(); + } + + private static void addAllowedKmsKeyResources( + List allowedKmsKeys, IamStatement.Builder allowKms) { + allowedKmsKeys.forEach( + keyArn -> { + LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + allowKms.addResource(IamResource.create(keyArn)); + }); + } + + private static void addAllKeysResource( + String region, String accountId, IamStatement.Builder allowKms) { + String allKeysArn = arnKeyAll(region, accountId); + allowKms.addResource(IamResource.create(allKeysArn)); + LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); } private static String arnKeyAll(String region, String accountId) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 05692e85e5..f087e2fca4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -27,6 +27,7 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import jakarta.annotation.Nullable; import java.net.URI; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; @@ -63,9 +64,13 @@ public String getFileIoImplClassName() { @Nullable public abstract String getRoleARN(); - /** KMS Key ARN for server-side encryption, optional */ + /** KMS Key ARN for server-side encryption,used for writes, optional */ @Nullable - public abstract String getKmsKeyArn(); + public abstract String currentKmsKey(); + + /** Comma-separated list of allowed KMS Key ARNs, optional */ + @Nullable + public abstract List allowedKmsKeys(); /** AWS external ID, optional */ @Nullable diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java index af1948fb57..a043a7daa5 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java @@ -45,7 +45,6 @@ import java.time.ZoneOffset; import java.time.temporal.ChronoUnit; import java.util.HashSet; -import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -79,8 +78,7 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { String loc = !allowedWriteLocations.isEmpty() ? allowedWriteLocations.stream().findAny().orElse(null) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java index f6e2699138..82de799152 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java @@ -110,8 +110,7 @@ public AccessConfig getOrGenerateSubScopeCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { if (!isTypeSupported(polarisEntity.getType())) { diagnostics.fail( "entity_type_not_suppported_to_scope_creds", "type={}", polarisEntity.getType()); @@ -137,8 +136,7 @@ public AccessConfig getOrGenerateSubScopeCreds( k.allowedListAction(), k.allowedReadLocations(), k.allowedWriteLocations(), - k.refreshCredentialsEndpoint(), - props); + k.refreshCredentialsEndpoint()); if (scopedCredentialsResult.isSuccess()) { long maxCacheDurationMs = maxCacheDurationMs(callCtx.getRealmConfig()); return new StorageCredentialCacheEntry( diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java index eddb67a834..c0568cc9b5 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/gcp/GcpCredentialsStorageIntegration.java @@ -77,8 +77,7 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { try { sourceCredentials.refresh(); } catch (IOException e) { diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index e5f0e79e1d..96e69ad1f3 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -71,7 +71,7 @@ public void testStsEndpoint() { private static ImmutableAwsStorageConfigurationInfo.Builder newBuilder() { return AwsStorageConfigurationInfo.builder() .roleARN("arn:aws:iam::123456789012:role/polaris-test") - .kmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245"); + .currentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java index 05d8127086..1a0349e77d 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java @@ -389,6 +389,8 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { String warehouseKeyPrefix = "path/to/warehouse"; String firstPath = warehouseKeyPrefix + "/namespace/table"; String secondPath = warehouseKeyPrefix + "/oldnamespace/table"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -402,8 +404,26 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(3) + .hasSize(4) .satisfiesExactly( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -456,7 +476,7 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -482,6 +502,8 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { String externalId = "externalId"; String bucket = "bucket"; String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -495,8 +517,26 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(2) - .satisfiesExactly( + .hasSize(3) + .satisfiesExactlyInAnyOrder( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -523,7 +563,7 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -542,74 +582,6 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { String.valueOf(EXPIRE_TIME.toEpochMilli())); } - @Test - public void testGetSubscopedCredsInlinePolicyWithKmsKey() { - StsClient stsClient = Mockito.mock(StsClient.class); - String roleARN = "arn:aws:iam::012345678901:role/jdoe"; - String externalId = "externalId"; - String bucket = "bucket"; - String warehouseKeyPrefix = "path/to/warehouse"; - String firstPath = warehouseKeyPrefix + "/namespace/table"; - String kmsKeyArn = "arn:aws:kms:us-east-1:012345678901:key/444343245"; - Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) - .thenAnswer( - invocation -> { - assertThat(invocation.getArguments()[0]) - .isInstanceOf(AssumeRoleRequest.class) - .asInstanceOf(InstanceOfAssertFactories.type(AssumeRoleRequest.class)) - .extracting(AssumeRoleRequest::policy) - .extracting(IamPolicy::fromJson) - .satisfies( - policy -> { - assertThat(policy) - .extracting(IamPolicy::statements) - .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(5) - .anySatisfy( - statement -> - assertThat(statement) - .returns(IamEffect.ALLOW, IamStatement::effect) - .returns( - List.of( - IamAction.create( - "kms:GenerateDataKeyWithoutPlaintext"), - IamAction.create("kms:DescribeKey"), - IamAction.create("kms:Decrypt"), - IamAction.create("kms:GenerateDataKey"), - IamAction.create("kms:Encrypt")), - IamStatement::actions) - .returns( - List.of(IamResource.create(kmsKeyArn)), - IamStatement::resources)); - }); - return ASSUME_ROLE_RESPONSE; - }); - AccessConfig accessConfig = - new AwsCredentialsStorageIntegration( - AwsStorageConfigurationInfo.builder() - .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) - .roleARN(roleARN) - .externalId(externalId) - .region("us-east-1") - .kmsKeyArn(kmsKeyArn) - .build(), - stsClient) - .getSubscopedCreds( - EMPTY_REALM_CONFIG, - true, - Set.of(s3Path(bucket, firstPath)), - Set.of(s3Path(bucket, firstPath)), - Optional.empty()); - assertThat(accessConfig.credentials()) - .isNotEmpty() - .containsEntry(StorageAccessProperty.AWS_TOKEN.getPropertyName(), "sess") - .containsEntry(StorageAccessProperty.AWS_KEY_ID.getPropertyName(), "accessKey") - .containsEntry(StorageAccessProperty.AWS_SECRET_KEY.getPropertyName(), "secretKey") - .containsEntry( - StorageAccessProperty.AWS_SESSION_TOKEN_EXPIRES_AT_MS.getPropertyName(), - String.valueOf(EXPIRE_TIME.toEpochMilli())); - } - @ParameterizedTest @ValueSource(strings = {AWS_PARTITION, "aws-cn", "aws-us-gov"}) public void testClientRegion(String awsPartition) { @@ -730,6 +702,177 @@ public void testNoClientRegion(String awsPartition) { ; } + @Test + public void testKmsKeyPolicyLogic() { + StsClient stsClient = Mockito.mock(StsClient.class); + String roleARN = "arn:aws:iam::012345678901:role/jdoe"; + String externalId = "externalId"; + String bucket = "bucket"; + String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-1"; + String accountId = "012345678901"; + String currentKmsKey = "arn:aws:kms:us-east-1:012345678901:key/current-key"; + List allowedKmsKeys = + List.of( + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1", + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-2"); + + // Test with current KMS key and write permissions + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with write permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"), + IamAction.create("kms:Encrypt"))); + assertThat(stmt.resources()).contains(IamResource.create(currentKmsKey)); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .currentKmsKey(currentKmsKey) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + + // Test with allowed KMS keys and read-only permissions + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with read-only permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"))); + assertThat(stmt.actions()).doesNotContain(IamAction.create("kms:Encrypt")); + assertThat(stmt.resources()) + .containsExactlyInAnyOrder( + IamResource.create(allowedKmsKeys.get(0)), + IamResource.create(allowedKmsKeys.get(1))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .allowedKmsKeys(allowedKmsKeys) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and read-only (should add wildcard KMS access) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify wildcard KMS statement exists + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.resources()) + .contains( + IamResource.create( + String.format("arn:aws:kms:%s:%s:key/*", region, accountId))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and write permissions (should not add KMS statement) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify no KMS statement exists + assertThat(policy.statements()) + .noneMatch( + stmt -> + stmt.actions().stream() + .anyMatch(action -> action.value().startsWith("kms:"))); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + } + private static @Nonnull String s3Arn(String partition, String bucket, String keyPrefix) { String bucketArn = "arn:" + partition + ":s3:::" + bucket; if (keyPrefix == null) { diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java index d33c08af34..d336040273 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/AccessConfigProvider.java @@ -99,7 +99,6 @@ public AccessConfig getAccessConfig( tableLocations, storageActions, storageInfo.get(), - refreshCredentialsEndpoint, - new java.util.HashMap<>()); + refreshCredentialsEndpoint); } } diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java index 00815b7b58..f4a6320d67 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/io/FileIOUtil.java @@ -18,7 +18,6 @@ */ package org.apache.polaris.service.catalog.io; -import java.util.Map; import java.util.Optional; import java.util.Set; import org.apache.iceberg.catalog.TableIdentifier; @@ -83,8 +82,7 @@ public static AccessConfig refreshAccessConfig( Set tableLocations, Set storageActions, PolarisEntity entity, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { boolean skipCredentialSubscopingIndirection = callContext @@ -115,8 +113,7 @@ public static AccessConfig refreshAccessConfig( allowList, tableLocations, writeLocations, - refreshCredentialsEndpoint, - props); + refreshCredentialsEndpoint); LOGGER .atDebug() .addKeyValue("tableIdentifier", tableIdentifier) diff --git a/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java b/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java index c2653a6857..23ec20abc3 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java @@ -114,8 +114,7 @@ public AccessConfig getSubscopedCreds( boolean allowListOperation, @Nonnull Set allowedReadLocations, @Nonnull Set allowedWriteLocations, - Optional refreshCredentialsEndpoint, - Map props) { + Optional refreshCredentialsEndpoint) { return AccessConfig.builder().supportsCredentialVending(false).build(); } diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index b629bfd748..f762bcf43d 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1103,10 +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" - kmsKeyArn: + 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 + items: + type: string + example: ["arn:aws:kms::123456789001:key/01234578"] region: type: string description: the aws region where data is stored From 7c33b5575bd34923ca1de779d21ac20f19eb3579 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 11:50:27 +0000 Subject: [PATCH 09/21] removed s3 key table changes. added new properties for AwsStorageConfigurationInfo, currentKmsKey and allowedKmsKeys --- .../storage/aws/AwsCredentialsStorageIntegration.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 1b27ea29e3..1a576c26ea 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -86,6 +86,7 @@ public StorageAccessConfig getSubscopedCreds( realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); AwsStorageConfigurationInfo storageConfig = config(); String region = storageConfig.getRegion(); + String accountId = storageConfig.getAwsAccountId(); StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder(); if (shouldUseSts(storageConfig)) { @@ -96,7 +97,7 @@ public StorageAccessConfig getSubscopedCreds( .roleSessionName("PolarisAwsCredentialsStorageIntegration") .policy( policyString( - storageConfig.getAwsPartition(), + storageConfig, allowListOperation, allowedReadLocations, allowedWriteLocations, @@ -172,7 +173,7 @@ private boolean shouldUseSts(AwsStorageConfigurationInfo storageConfig) { * and just assuming the role with full privileges. */ private IamPolicy policyString( - String awsPartition, + AwsStorageConfigurationInfo storageConfigurationInfo, boolean allowList, Set readLocations, Set writeLocations, @@ -187,9 +188,9 @@ private IamPolicy policyString( Map bucketListStatementBuilder = new HashMap<>(); Map bucketGetLocationStatementBuilder = new HashMap<>(); - String arnPrefix = arnPrefixForPartition(awsPartition); - String currentKmsKey = storageConfigurationInfo.currentKmsKey(); - List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); + String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); + String currentKmsKey = storageConfigurationInfo.currentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( From 65a84fb77a493058c8d208c46a06fd5959edef85 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 11:52:58 +0000 Subject: [PATCH 10/21] removed s3 key table changes. added new properties for AwsStorageConfigurationInfo, currentKmsKey and allowedKmsKeys --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bb90c6a13..fc2dbec6a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,7 +74,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### New Features -- Updated catalogs creation to include AWS kms key ,as an extra param in the storage config info, to be used in S3 data encryption +- 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 - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. From b8bacb98a1e80ba967eb9ed1f6c23b074bf7fa16 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:38:22 +0000 Subject: [PATCH 11/21] adding support to use a kms key for s3 buckets (#17) * removed s3 key table changes. added new properties for AwsStorageConfigurationInfo, currentKmsKey and allowedKmsKeys --- CHANGELOG.md | 1 + .../admin/model/CatalogSerializationTest.java | 4 + .../polaris/core/entity/CatalogEntity.java | 4 + .../aws/AwsCredentialsStorageIntegration.java | 108 ++++++++- .../aws/AwsStorageConfigurationInfo.java | 9 + .../aws/AwsStorageConfigurationInfoTest.java | 3 +- .../AwsCredentialsStorageIntegrationTest.java | 221 +++++++++++++++++- .../service/entity/CatalogEntityTest.java | 6 + spec/polaris-management-service.yml | 10 + 9 files changed, 353 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 493e0c60db..fc2dbec6a9 100644 --- a/CHANGELOG.md +++ b/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 - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c4210486ba..c645f086cb 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -35,6 +35,7 @@ public class CatalogSerializationTest { private static final String TEST_LOCATION = "s3://test/"; private static final String TEST_CATALOG_NAME = "test-catalog"; private static final String TEST_ROLE_ARN = "arn:aws:iam::123456789012:role/test-role"; + private static final String KMS_KEY = "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1"; @BeforeEach public void setUp() { @@ -59,6 +60,7 @@ public void testJsonFormat() throws JsonProcessingException { new CatalogProperties(TEST_LOCATION), AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) .setRoleArn(TEST_ROLE_ARN) + .setCurrentKmsKey(KMS_KEY) .build()); String json = mapper.writeValueAsString(catalog); @@ -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\":[]," + "\"pathStyleAccess\":false," + "\"storageType\":\"S3\"," + "\"allowedLocations\":[]" diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 40cf1969d6..095c950aeb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,6 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) + .setCurrentKmsKey(awsConfig.currentKmsKey()) + .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) @@ -308,6 +310,8 @@ public Builder setStorageConfigurationInfo( AwsStorageConfigurationInfo.builder() .allowedLocations(allowedLocations) .roleARN(awsConfigModel.getRoleArn()) + .currentKmsKey(awsConfigModel.getCurrentKmsKey()) + .allowedKmsKeys(awsConfigModel.getAllowedKmsKeys()) .externalId(awsConfigModel.getExternalId()) .region(awsConfigModel.getRegion()) .endpoint(awsConfigModel.getEndpoint()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index e393911f71..1a576c26ea 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -23,6 +23,7 @@ import jakarta.annotation.Nonnull; import java.net.URI; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -33,6 +34,8 @@ import org.apache.polaris.core.storage.StorageAccessProperty; import org.apache.polaris.core.storage.StorageUtil; import org.apache.polaris.core.storage.aws.StsClientProvider.StsDestination; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.policybuilder.iam.IamConditionOperator; import software.amazon.awssdk.policybuilder.iam.IamEffect; @@ -49,6 +52,9 @@ public class AwsCredentialsStorageIntegration private final StsClientProvider stsClientProvider; private final Optional credentialsProvider; + private static final Logger LOGGER = + LoggerFactory.getLogger(AwsCredentialsStorageIntegration.class); + public AwsCredentialsStorageIntegration( AwsStorageConfigurationInfo config, StsClient fixedClient) { this(config, (destination) -> fixedClient); @@ -80,6 +86,7 @@ public StorageAccessConfig getSubscopedCreds( realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); AwsStorageConfigurationInfo storageConfig = config(); String region = storageConfig.getRegion(); + String accountId = storageConfig.getAwsAccountId(); StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder(); if (shouldUseSts(storageConfig)) { @@ -90,10 +97,12 @@ public StorageAccessConfig getSubscopedCreds( .roleSessionName("PolarisAwsCredentialsStorageIntegration") .policy( policyString( - storageConfig.getAwsPartition(), + storageConfig, allowListOperation, allowedReadLocations, - allowedWriteLocations) + allowedWriteLocations, + region, + accountId) .toJson()) .durationSeconds(storageCredentialDurationSeconds); credentialsProvider.ifPresent( @@ -163,12 +172,13 @@ private boolean shouldUseSts(AwsStorageConfigurationInfo storageConfig) { * ListBucket privileges with no resources. This prevents us from sending an empty policy to AWS * and just assuming the role with full privileges. */ - // TODO - add KMS key access private IamPolicy policyString( - String awsPartition, + AwsStorageConfigurationInfo storageConfigurationInfo, boolean allowList, Set readLocations, - Set writeLocations) { + Set writeLocations, + String region, + String accountId) { IamPolicy.Builder policyBuilder = IamPolicy.builder(); IamStatement.Builder allowGetObjectStatementBuilder = IamStatement.builder() @@ -178,7 +188,9 @@ private IamPolicy policyString( Map bucketListStatementBuilder = new HashMap<>(); Map bucketGetLocationStatementBuilder = new HashMap<>(); - String arnPrefix = arnPrefixForPartition(awsPartition); + String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); + String currentKmsKey = storageConfigurationInfo.currentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -225,6 +237,9 @@ private IamPolicy policyString( arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/"))); }); policyBuilder.addStatement(allowPutObjectStatementBuilder.build()); + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, true, region, accountId); + } else { + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, false, region, accountId); } if (!bucketListStatementBuilder.isEmpty()) { bucketListStatementBuilder @@ -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 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); + allowKms.addResource(IamResource.create(kmsKeyArn)); + } + } + + private static boolean hasAllowedKmsKeys(List allowedKmsKeys) { + return allowedKmsKeys != null && !allowedKmsKeys.isEmpty(); + } + + private static void addAllowedKmsKeyResources( + List allowedKmsKeys, IamStatement.Builder allowKms) { + allowedKmsKeys.forEach( + keyArn -> { + LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + allowKms.addResource(IamResource.create(keyArn)); + }); + } + + private static void addAllKeysResource( + String region, String accountId, IamStatement.Builder allowKms) { + String allKeysArn = arnKeyAll(region, accountId); + allowKms.addResource(IamResource.create(allKeysArn)); + LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); + } + + private static String arnKeyAll(String region, String accountId) { + return String.format("arn:aws:kms:%s:%s:key/*", region, accountId); } private static String arnPrefixForPartition(String awsPartition) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 69c669222d..34ab547b48 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -27,6 +27,7 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import jakarta.annotation.Nullable; import java.net.URI; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; @@ -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(); + + /** Comma-separated list of allowed KMS Key ARNs, optional */ + @Nullable + public abstract List allowedKmsKeys(); + /** AWS external ID, optional */ @Nullable public abstract String getExternalId(); diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 3460dc23f7..96e69ad1f3 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/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"); } @Test diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java index fb0c63c403..2732577489 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java @@ -389,6 +389,8 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { String warehouseKeyPrefix = "path/to/warehouse"; String firstPath = warehouseKeyPrefix + "/namespace/table"; String secondPath = warehouseKeyPrefix + "/oldnamespace/table"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -402,8 +404,26 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(3) + .hasSize(4) .satisfiesExactly( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -456,7 +476,7 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -482,6 +502,8 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { String externalId = "externalId"; String bucket = "bucket"; String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -495,8 +517,26 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(2) - .satisfiesExactly( + .hasSize(3) + .satisfiesExactlyInAnyOrder( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -523,7 +563,7 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -662,6 +702,177 @@ public void testNoClientRegion(String awsPartition) { ; } + @Test + public void testKmsKeyPolicyLogic() { + StsClient stsClient = Mockito.mock(StsClient.class); + String roleARN = "arn:aws:iam::012345678901:role/jdoe"; + String externalId = "externalId"; + String bucket = "bucket"; + String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-1"; + String accountId = "012345678901"; + String currentKmsKey = "arn:aws:kms:us-east-1:012345678901:key/current-key"; + List allowedKmsKeys = + List.of( + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1", + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-2"); + + // Test with current KMS key and write permissions + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with write permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"), + IamAction.create("kms:Encrypt"))); + assertThat(stmt.resources()).contains(IamResource.create(currentKmsKey)); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .currentKmsKey(currentKmsKey) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + + // Test with allowed KMS keys and read-only permissions + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with read-only permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"))); + assertThat(stmt.actions()).doesNotContain(IamAction.create("kms:Encrypt")); + assertThat(stmt.resources()) + .containsExactlyInAnyOrder( + IamResource.create(allowedKmsKeys.get(0)), + IamResource.create(allowedKmsKeys.get(1))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .allowedKmsKeys(allowedKmsKeys) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and read-only (should add wildcard KMS access) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify wildcard KMS statement exists + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.resources()) + .contains( + IamResource.create( + String.format("arn:aws:kms:%s:%s:key/*", region, accountId))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and write permissions (should not add KMS statement) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify no KMS statement exists + assertThat(policy.statements()) + .noneMatch( + stmt -> + stmt.actions().stream() + .anyMatch(action -> action.value().startsWith("kms:"))); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + } + private static @Nonnull String s3Arn(String partition, String bucket, String keyPrefix) { String bucketArn = "arn:" + partition + ":s3:::" + bucket; if (keyPrefix == null) { diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index 5c3c220c82..d611be8a58 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,6 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -334,6 +335,8 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test @@ -342,6 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -357,6 +361,8 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index d1775b7598..f762bcf43d 100644 --- a/spec/polaris-management-service.yml +++ b/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 + items: + type: string + example: ["arn:aws:kms::123456789001:key/01234578"] region: type: string description: the aws region where data is stored From 8850df122c0b7eaf805fa5b572fab8051e39434c Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:50:00 +0000 Subject: [PATCH 12/21] fixed merge issue --- .../apache/polaris/service/entity/CatalogEntityTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index d611be8a58..24c7814ce8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,7 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -335,7 +335,7 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @@ -345,7 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -361,7 +361,7 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } From b604568a58a3505f21c2943061ff88a084807b51 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:57:18 +0000 Subject: [PATCH 13/21] Feature/s3 kms key (#18) * fixed merge issue --- .../apache/polaris/service/entity/CatalogEntityTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index d611be8a58..24c7814ce8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,7 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -335,7 +335,7 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @@ -345,7 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -361,7 +361,7 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } From d85e4cf5c0a4c985b03279914b5369c0288a67e2 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Thu, 20 Nov 2025 10:20:02 +0000 Subject: [PATCH 14/21] fixed issue that broke integration tests and updated code based on PR comments --- CHANGELOG.md | 2 +- .../admin/model/CatalogSerializationTest.java | 27 +++++++++++++++++++ .../polaris/core/entity/CatalogEntity.java | 4 +-- .../aws/AwsCredentialsStorageIntegration.java | 27 ++++++++++--------- .../aws/AwsStorageConfigurationInfo.java | 4 +-- .../aws/AwsStorageConfigurationInfoTest.java | 3 +-- spec/polaris-management-service.yml | 2 +- .../test/minio/Dockerfile-minio-version | 3 ++- 8 files changed, 50 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc2dbec6a9..d6dd97e301 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,7 +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 +- Added KMS properties (optional) to catalog storage config to enable S3 data encryption. - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c645f086cb..3244f14732 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -53,6 +53,33 @@ public void testCatalogSerialization(String description, Catalog catalog) @Test public void testJsonFormat() throws JsonProcessingException { + Catalog catalog = + new Catalog( + Catalog.TypeEnum.INTERNAL, + TEST_CATALOG_NAME, + new CatalogProperties(TEST_LOCATION), + AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) + .setRoleArn(TEST_ROLE_ARN) + .build()); + + String json = mapper.writeValueAsString(catalog); + + assertThat(json) + .isEqualTo( + "{\"type\":\"INTERNAL\"," + + "\"name\":\"test-catalog\"," + + "\"properties\":{\"default-base-location\":\"s3://test/\"}," + + "\"storageConfigInfo\":{" + + "\"roleArn\":\"arn:aws:iam::123456789012:role/test-role\"," + + "\"allowedKmsKeys\":[]," + + "\"pathStyleAccess\":false," + + "\"storageType\":\"S3\"," + + "\"allowedLocations\":[]" + + "}}"); + } + + @Test + public void testJsonFormatWithKmsProperties() throws JsonProcessingException { Catalog catalog = new Catalog( Catalog.TypeEnum.INTERNAL, diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 095c950aeb..4607728b35 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,8 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) - .setCurrentKmsKey(awsConfig.currentKmsKey()) - .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) + .setCurrentKmsKey(awsConfig.getCurrentKmsKey()) + .setAllowedKmsKeys(awsConfig.getAllowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 1a576c26ea..2996006954 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -189,8 +189,8 @@ private IamPolicy policyString( Map bucketGetLocationStatementBuilder = new HashMap<>(); String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); - String currentKmsKey = storageConfigurationInfo.currentKmsKey(); - List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); + String currentKmsKey = storageConfigurationInfo.getCurrentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.getAllowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -254,20 +254,18 @@ private IamPolicy policyString( bucketGetLocationStatementBuilder .values() .forEach(statementBuilder -> policyBuilder.addStatement(statementBuilder.build())); - var r = policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); - LOGGER.info("Policies {}", r); - return r; + return policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); } private static void addKmsKeyPolicy( String kmsKeyArn, List allowedKmsKeys, IamPolicy.Builder policyBuilder, - boolean canEncrypt, + boolean canWrite, String region, String accountId) { - IamStatement.Builder allowKms = buildBaseKmsStatement(canEncrypt); + IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite); boolean hasCurrentKey = kmsKeyArn != null; boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys); @@ -282,10 +280,13 @@ private static void addKmsKeyPolicy( // Add KMS statement if we have any KMS key configuration if (hasCurrentKey || hasAllowedKeys) { policyBuilder.addStatement(allowKms.build()); - } else if (!canEncrypt) { + } else if (!canWrite) { // Only add wildcard KMS access for read-only operations when no specific keys are configured - addAllKeysResource(region, accountId, allowKms); - policyBuilder.addStatement(allowKms.build()); + // this check is for minio because it doesn't have region or account id + if (region != null && accountId != null) { + addAllKeysResource(region, accountId, allowKms); + policyBuilder.addStatement(allowKms.build()); + } } } @@ -307,7 +308,7 @@ private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) { private static void addKmsKeyResource(String kmsKeyArn, IamStatement.Builder allowKms) { if (kmsKeyArn != null) { - LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn); + LOGGER.debug("Adding KMS key policy for key {}", kmsKeyArn); allowKms.addResource(IamResource.create(kmsKeyArn)); } } @@ -320,7 +321,7 @@ private static void addAllowedKmsKeyResources( List allowedKmsKeys, IamStatement.Builder allowKms) { allowedKmsKeys.forEach( keyArn -> { - LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + LOGGER.debug("Adding allowed KMS key policy for key {}", keyArn); allowKms.addResource(IamResource.create(keyArn)); }); } @@ -329,7 +330,7 @@ private static void addAllKeysResource( String region, String accountId, IamStatement.Builder allowKms) { String allKeysArn = arnKeyAll(region, accountId); allowKms.addResource(IamResource.create(allKeysArn)); - LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); + LOGGER.debug("Adding KMS key policy for all keys in account {}", accountId); } private static String arnKeyAll(String region, String accountId) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 34ab547b48..b62265f929 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -66,11 +66,11 @@ public String getFileIoImplClassName() { /** KMS Key ARN for server-side encryption,used for writes, optional */ @Nullable - public abstract String currentKmsKey(); + public abstract String getCurrentKmsKey(); /** Comma-separated list of allowed KMS Key ARNs, optional */ @Nullable - public abstract List allowedKmsKeys(); + public abstract List getAllowedKmsKeys(); /** AWS external ID, optional */ @Nullable diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 96e69ad1f3..3460dc23f7 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -70,8 +70,7 @@ public void testStsEndpoint() { private static ImmutableAwsStorageConfigurationInfo.Builder newBuilder() { return AwsStorageConfigurationInfo.builder() - .roleARN("arn:aws:iam::123456789012:role/polaris-test") - .currentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245"); + .roleARN("arn:aws:iam::123456789012:role/polaris-test"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index f762bcf43d..03c894b6e4 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1109,7 +1109,7 @@ components: example: "arn:aws:kms::123456789001:key/01234578" allowedKmsKeys: type: array - description: the aws kms keys arn used to encrypt s3 data + description: The list of kms keys that this catalog and its clients are allow to use for reading s3 data items: type: string example: ["arn:aws:kms::123456789001:key/01234578"] diff --git a/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version b/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version index 1dd0f4e2f5..2865b0640a 100644 --- a/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version +++ b/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version @@ -19,4 +19,5 @@ # Dockerfile to provide the image name and tag to a test. # Version is managed by Renovate - do not edit. -FROM quay.io/minio/minio:RELEASE.2025-04-08T15-41-24Z +#FROM quay.io/minio/minio:RELEASE.2025-04-08T15-41-24Z +FROM jetae-publish.prod.aws.jpmchase.net/container-external/docker.io/minio/minio:RELEASE.2024-03-07T00-43-48Z From a7d6413de458e138f859f4c1d69a6a0c08a29ec4 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:38:22 +0000 Subject: [PATCH 15/21] adding support to use a kms key for s3 buckets (#17) * removed s3 key table changes. added new properties for AwsStorageConfigurationInfo, currentKmsKey and allowedKmsKeys --- CHANGELOG.md | 1 + .../admin/model/CatalogSerializationTest.java | 4 + .../polaris/core/entity/CatalogEntity.java | 4 + .../aws/AwsCredentialsStorageIntegration.java | 108 ++++++++- .../aws/AwsStorageConfigurationInfo.java | 9 + .../aws/AwsStorageConfigurationInfoTest.java | 3 +- .../AwsCredentialsStorageIntegrationTest.java | 221 +++++++++++++++++- .../service/entity/CatalogEntityTest.java | 6 + spec/polaris-management-service.yml | 10 + 9 files changed, 353 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e715a2a2cb..6af985bc4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,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 - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c4210486ba..c645f086cb 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -35,6 +35,7 @@ public class CatalogSerializationTest { private static final String TEST_LOCATION = "s3://test/"; private static final String TEST_CATALOG_NAME = "test-catalog"; private static final String TEST_ROLE_ARN = "arn:aws:iam::123456789012:role/test-role"; + private static final String KMS_KEY = "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1"; @BeforeEach public void setUp() { @@ -59,6 +60,7 @@ public void testJsonFormat() throws JsonProcessingException { new CatalogProperties(TEST_LOCATION), AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) .setRoleArn(TEST_ROLE_ARN) + .setCurrentKmsKey(KMS_KEY) .build()); String json = mapper.writeValueAsString(catalog); @@ -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\":[]," + "\"pathStyleAccess\":false," + "\"storageType\":\"S3\"," + "\"allowedLocations\":[]" diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 40cf1969d6..095c950aeb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,6 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) + .setCurrentKmsKey(awsConfig.currentKmsKey()) + .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) @@ -308,6 +310,8 @@ public Builder setStorageConfigurationInfo( AwsStorageConfigurationInfo.builder() .allowedLocations(allowedLocations) .roleARN(awsConfigModel.getRoleArn()) + .currentKmsKey(awsConfigModel.getCurrentKmsKey()) + .allowedKmsKeys(awsConfigModel.getAllowedKmsKeys()) .externalId(awsConfigModel.getExternalId()) .region(awsConfigModel.getRegion()) .endpoint(awsConfigModel.getEndpoint()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index e393911f71..1a576c26ea 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -23,6 +23,7 @@ import jakarta.annotation.Nonnull; import java.net.URI; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -33,6 +34,8 @@ import org.apache.polaris.core.storage.StorageAccessProperty; import org.apache.polaris.core.storage.StorageUtil; import org.apache.polaris.core.storage.aws.StsClientProvider.StsDestination; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.policybuilder.iam.IamConditionOperator; import software.amazon.awssdk.policybuilder.iam.IamEffect; @@ -49,6 +52,9 @@ public class AwsCredentialsStorageIntegration private final StsClientProvider stsClientProvider; private final Optional credentialsProvider; + private static final Logger LOGGER = + LoggerFactory.getLogger(AwsCredentialsStorageIntegration.class); + public AwsCredentialsStorageIntegration( AwsStorageConfigurationInfo config, StsClient fixedClient) { this(config, (destination) -> fixedClient); @@ -80,6 +86,7 @@ public StorageAccessConfig getSubscopedCreds( realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); AwsStorageConfigurationInfo storageConfig = config(); String region = storageConfig.getRegion(); + String accountId = storageConfig.getAwsAccountId(); StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder(); if (shouldUseSts(storageConfig)) { @@ -90,10 +97,12 @@ public StorageAccessConfig getSubscopedCreds( .roleSessionName("PolarisAwsCredentialsStorageIntegration") .policy( policyString( - storageConfig.getAwsPartition(), + storageConfig, allowListOperation, allowedReadLocations, - allowedWriteLocations) + allowedWriteLocations, + region, + accountId) .toJson()) .durationSeconds(storageCredentialDurationSeconds); credentialsProvider.ifPresent( @@ -163,12 +172,13 @@ private boolean shouldUseSts(AwsStorageConfigurationInfo storageConfig) { * ListBucket privileges with no resources. This prevents us from sending an empty policy to AWS * and just assuming the role with full privileges. */ - // TODO - add KMS key access private IamPolicy policyString( - String awsPartition, + AwsStorageConfigurationInfo storageConfigurationInfo, boolean allowList, Set readLocations, - Set writeLocations) { + Set writeLocations, + String region, + String accountId) { IamPolicy.Builder policyBuilder = IamPolicy.builder(); IamStatement.Builder allowGetObjectStatementBuilder = IamStatement.builder() @@ -178,7 +188,9 @@ private IamPolicy policyString( Map bucketListStatementBuilder = new HashMap<>(); Map bucketGetLocationStatementBuilder = new HashMap<>(); - String arnPrefix = arnPrefixForPartition(awsPartition); + String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); + String currentKmsKey = storageConfigurationInfo.currentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -225,6 +237,9 @@ private IamPolicy policyString( arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/"))); }); policyBuilder.addStatement(allowPutObjectStatementBuilder.build()); + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, true, region, accountId); + } else { + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, false, region, accountId); } if (!bucketListStatementBuilder.isEmpty()) { bucketListStatementBuilder @@ -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 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); + allowKms.addResource(IamResource.create(kmsKeyArn)); + } + } + + private static boolean hasAllowedKmsKeys(List allowedKmsKeys) { + return allowedKmsKeys != null && !allowedKmsKeys.isEmpty(); + } + + private static void addAllowedKmsKeyResources( + List allowedKmsKeys, IamStatement.Builder allowKms) { + allowedKmsKeys.forEach( + keyArn -> { + LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + allowKms.addResource(IamResource.create(keyArn)); + }); + } + + private static void addAllKeysResource( + String region, String accountId, IamStatement.Builder allowKms) { + String allKeysArn = arnKeyAll(region, accountId); + allowKms.addResource(IamResource.create(allKeysArn)); + LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); + } + + private static String arnKeyAll(String region, String accountId) { + return String.format("arn:aws:kms:%s:%s:key/*", region, accountId); } private static String arnPrefixForPartition(String awsPartition) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 69c669222d..34ab547b48 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -27,6 +27,7 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import jakarta.annotation.Nullable; import java.net.URI; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; @@ -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(); + + /** Comma-separated list of allowed KMS Key ARNs, optional */ + @Nullable + public abstract List allowedKmsKeys(); + /** AWS external ID, optional */ @Nullable public abstract String getExternalId(); diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 3460dc23f7..96e69ad1f3 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/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"); } @Test diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java index fb0c63c403..2732577489 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java @@ -389,6 +389,8 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { String warehouseKeyPrefix = "path/to/warehouse"; String firstPath = warehouseKeyPrefix + "/namespace/table"; String secondPath = warehouseKeyPrefix + "/oldnamespace/table"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -402,8 +404,26 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(3) + .hasSize(4) .satisfiesExactly( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -456,7 +476,7 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -482,6 +502,8 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { String externalId = "externalId"; String bucket = "bucket"; String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -495,8 +517,26 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(2) - .satisfiesExactly( + .hasSize(3) + .satisfiesExactlyInAnyOrder( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -523,7 +563,7 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -662,6 +702,177 @@ public void testNoClientRegion(String awsPartition) { ; } + @Test + public void testKmsKeyPolicyLogic() { + StsClient stsClient = Mockito.mock(StsClient.class); + String roleARN = "arn:aws:iam::012345678901:role/jdoe"; + String externalId = "externalId"; + String bucket = "bucket"; + String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-1"; + String accountId = "012345678901"; + String currentKmsKey = "arn:aws:kms:us-east-1:012345678901:key/current-key"; + List allowedKmsKeys = + List.of( + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1", + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-2"); + + // Test with current KMS key and write permissions + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with write permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"), + IamAction.create("kms:Encrypt"))); + assertThat(stmt.resources()).contains(IamResource.create(currentKmsKey)); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .currentKmsKey(currentKmsKey) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + + // Test with allowed KMS keys and read-only permissions + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with read-only permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"))); + assertThat(stmt.actions()).doesNotContain(IamAction.create("kms:Encrypt")); + assertThat(stmt.resources()) + .containsExactlyInAnyOrder( + IamResource.create(allowedKmsKeys.get(0)), + IamResource.create(allowedKmsKeys.get(1))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .allowedKmsKeys(allowedKmsKeys) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and read-only (should add wildcard KMS access) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify wildcard KMS statement exists + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.resources()) + .contains( + IamResource.create( + String.format("arn:aws:kms:%s:%s:key/*", region, accountId))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and write permissions (should not add KMS statement) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify no KMS statement exists + assertThat(policy.statements()) + .noneMatch( + stmt -> + stmt.actions().stream() + .anyMatch(action -> action.value().startsWith("kms:"))); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + } + private static @Nonnull String s3Arn(String partition, String bucket, String keyPrefix) { String bucketArn = "arn:" + partition + ":s3:::" + bucket; if (keyPrefix == null) { diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index 5c3c220c82..d611be8a58 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,6 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -334,6 +335,8 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test @@ -342,6 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -357,6 +361,8 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index d1775b7598..f762bcf43d 100644 --- a/spec/polaris-management-service.yml +++ b/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 + items: + type: string + example: ["arn:aws:kms::123456789001:key/01234578"] region: type: string description: the aws region where data is stored From e4c8260a2e1b69db6c87d1db57d418897e6590b6 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:57:18 +0000 Subject: [PATCH 16/21] Feature/s3 kms key (#18) * fixed merge issue --- .../apache/polaris/service/entity/CatalogEntityTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index d611be8a58..24c7814ce8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,7 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -335,7 +335,7 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @@ -345,7 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -361,7 +361,7 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } From 4b2dcd38cfb5a69892397b0df6e1c30ac1b93c03 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Thu, 20 Nov 2025 10:27:38 +0000 Subject: [PATCH 17/21] fixed issue that broke integration tests and updated code based on PR comments --- .../org/apache/polaris/test/minio/Dockerfile-minio-version | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version b/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version index 2865b0640a..1dd0f4e2f5 100644 --- a/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version +++ b/tools/minio-testcontainer/src/main/resources/org/apache/polaris/test/minio/Dockerfile-minio-version @@ -19,5 +19,4 @@ # Dockerfile to provide the image name and tag to a test. # Version is managed by Renovate - do not edit. -#FROM quay.io/minio/minio:RELEASE.2025-04-08T15-41-24Z -FROM jetae-publish.prod.aws.jpmchase.net/container-external/docker.io/minio/minio:RELEASE.2024-03-07T00-43-48Z +FROM quay.io/minio/minio:RELEASE.2025-04-08T15-41-24Z From 5da097ade6fad49cdec0278e52e6d4707d1d4a37 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Thu, 20 Nov 2025 10:34:38 +0000 Subject: [PATCH 18/21] Feature/s3 kms key (#19) fixed issue that broke integration tests and updated code based on PR comments --- CHANGELOG.md | 2 +- .../admin/model/CatalogSerializationTest.java | 27 +++++++++++++++++++ .../polaris/core/entity/CatalogEntity.java | 4 +-- .../aws/AwsCredentialsStorageIntegration.java | 27 ++++++++++--------- .../aws/AwsStorageConfigurationInfo.java | 4 +-- .../aws/AwsStorageConfigurationInfoTest.java | 3 +-- spec/polaris-management-service.yml | 2 +- 7 files changed, 48 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6af985bc4e..8d60ba4cee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,7 +82,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 +- Added KMS properties (optional) to catalog storage config to enable S3 data encryption. - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c645f086cb..3244f14732 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -53,6 +53,33 @@ public void testCatalogSerialization(String description, Catalog catalog) @Test public void testJsonFormat() throws JsonProcessingException { + Catalog catalog = + new Catalog( + Catalog.TypeEnum.INTERNAL, + TEST_CATALOG_NAME, + new CatalogProperties(TEST_LOCATION), + AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) + .setRoleArn(TEST_ROLE_ARN) + .build()); + + String json = mapper.writeValueAsString(catalog); + + assertThat(json) + .isEqualTo( + "{\"type\":\"INTERNAL\"," + + "\"name\":\"test-catalog\"," + + "\"properties\":{\"default-base-location\":\"s3://test/\"}," + + "\"storageConfigInfo\":{" + + "\"roleArn\":\"arn:aws:iam::123456789012:role/test-role\"," + + "\"allowedKmsKeys\":[]," + + "\"pathStyleAccess\":false," + + "\"storageType\":\"S3\"," + + "\"allowedLocations\":[]" + + "}}"); + } + + @Test + public void testJsonFormatWithKmsProperties() throws JsonProcessingException { Catalog catalog = new Catalog( Catalog.TypeEnum.INTERNAL, diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 095c950aeb..4607728b35 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,8 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) - .setCurrentKmsKey(awsConfig.currentKmsKey()) - .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) + .setCurrentKmsKey(awsConfig.getCurrentKmsKey()) + .setAllowedKmsKeys(awsConfig.getAllowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 1a576c26ea..2996006954 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -189,8 +189,8 @@ private IamPolicy policyString( Map bucketGetLocationStatementBuilder = new HashMap<>(); String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); - String currentKmsKey = storageConfigurationInfo.currentKmsKey(); - List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); + String currentKmsKey = storageConfigurationInfo.getCurrentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.getAllowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -254,20 +254,18 @@ private IamPolicy policyString( bucketGetLocationStatementBuilder .values() .forEach(statementBuilder -> policyBuilder.addStatement(statementBuilder.build())); - var r = policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); - LOGGER.info("Policies {}", r); - return r; + return policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); } private static void addKmsKeyPolicy( String kmsKeyArn, List allowedKmsKeys, IamPolicy.Builder policyBuilder, - boolean canEncrypt, + boolean canWrite, String region, String accountId) { - IamStatement.Builder allowKms = buildBaseKmsStatement(canEncrypt); + IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite); boolean hasCurrentKey = kmsKeyArn != null; boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys); @@ -282,10 +280,13 @@ private static void addKmsKeyPolicy( // Add KMS statement if we have any KMS key configuration if (hasCurrentKey || hasAllowedKeys) { policyBuilder.addStatement(allowKms.build()); - } else if (!canEncrypt) { + } else if (!canWrite) { // Only add wildcard KMS access for read-only operations when no specific keys are configured - addAllKeysResource(region, accountId, allowKms); - policyBuilder.addStatement(allowKms.build()); + // this check is for minio because it doesn't have region or account id + if (region != null && accountId != null) { + addAllKeysResource(region, accountId, allowKms); + policyBuilder.addStatement(allowKms.build()); + } } } @@ -307,7 +308,7 @@ private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) { private static void addKmsKeyResource(String kmsKeyArn, IamStatement.Builder allowKms) { if (kmsKeyArn != null) { - LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn); + LOGGER.debug("Adding KMS key policy for key {}", kmsKeyArn); allowKms.addResource(IamResource.create(kmsKeyArn)); } } @@ -320,7 +321,7 @@ private static void addAllowedKmsKeyResources( List allowedKmsKeys, IamStatement.Builder allowKms) { allowedKmsKeys.forEach( keyArn -> { - LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + LOGGER.debug("Adding allowed KMS key policy for key {}", keyArn); allowKms.addResource(IamResource.create(keyArn)); }); } @@ -329,7 +330,7 @@ private static void addAllKeysResource( String region, String accountId, IamStatement.Builder allowKms) { String allKeysArn = arnKeyAll(region, accountId); allowKms.addResource(IamResource.create(allKeysArn)); - LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); + LOGGER.debug("Adding KMS key policy for all keys in account {}", accountId); } private static String arnKeyAll(String region, String accountId) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 34ab547b48..b62265f929 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -66,11 +66,11 @@ public String getFileIoImplClassName() { /** KMS Key ARN for server-side encryption,used for writes, optional */ @Nullable - public abstract String currentKmsKey(); + public abstract String getCurrentKmsKey(); /** Comma-separated list of allowed KMS Key ARNs, optional */ @Nullable - public abstract List allowedKmsKeys(); + public abstract List getAllowedKmsKeys(); /** AWS external ID, optional */ @Nullable diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 96e69ad1f3..3460dc23f7 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -70,8 +70,7 @@ public void testStsEndpoint() { private static ImmutableAwsStorageConfigurationInfo.Builder newBuilder() { return AwsStorageConfigurationInfo.builder() - .roleARN("arn:aws:iam::123456789012:role/polaris-test") - .currentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245"); + .roleARN("arn:aws:iam::123456789012:role/polaris-test"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index f762bcf43d..03c894b6e4 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1109,7 +1109,7 @@ components: example: "arn:aws:kms::123456789001:key/01234578" allowedKmsKeys: type: array - description: the aws kms keys arn used to encrypt s3 data + description: The list of kms keys that this catalog and its clients are allow to use for reading s3 data items: type: string example: ["arn:aws:kms::123456789001:key/01234578"] From 8bd5b74982159be40727746851af523284ed668a Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:38:22 +0000 Subject: [PATCH 19/21] adding support to use a kms key for s3 buckets (#17) * removed s3 key table changes. added new properties for AwsStorageConfigurationInfo, currentKmsKey and allowedKmsKeys --- CHANGELOG.md | 1 + .../admin/model/CatalogSerializationTest.java | 4 + .../polaris/core/entity/CatalogEntity.java | 4 + .../aws/AwsCredentialsStorageIntegration.java | 108 ++++++++- .../aws/AwsStorageConfigurationInfo.java | 9 + .../aws/AwsStorageConfigurationInfoTest.java | 3 +- .../AwsCredentialsStorageIntegrationTest.java | 221 +++++++++++++++++- .../service/entity/CatalogEntityTest.java | 6 + spec/polaris-management-service.yml | 10 + 9 files changed, 353 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ad9d759b1..a0abd73c50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,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 - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c4210486ba..c645f086cb 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -35,6 +35,7 @@ public class CatalogSerializationTest { private static final String TEST_LOCATION = "s3://test/"; private static final String TEST_CATALOG_NAME = "test-catalog"; private static final String TEST_ROLE_ARN = "arn:aws:iam::123456789012:role/test-role"; + private static final String KMS_KEY = "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1"; @BeforeEach public void setUp() { @@ -59,6 +60,7 @@ public void testJsonFormat() throws JsonProcessingException { new CatalogProperties(TEST_LOCATION), AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) .setRoleArn(TEST_ROLE_ARN) + .setCurrentKmsKey(KMS_KEY) .build()); String json = mapper.writeValueAsString(catalog); @@ -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\":[]," + "\"pathStyleAccess\":false," + "\"storageType\":\"S3\"," + "\"allowedLocations\":[]" diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 40cf1969d6..095c950aeb 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,6 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) + .setCurrentKmsKey(awsConfig.currentKmsKey()) + .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) @@ -308,6 +310,8 @@ public Builder setStorageConfigurationInfo( AwsStorageConfigurationInfo.builder() .allowedLocations(allowedLocations) .roleARN(awsConfigModel.getRoleArn()) + .currentKmsKey(awsConfigModel.getCurrentKmsKey()) + .allowedKmsKeys(awsConfigModel.getAllowedKmsKeys()) .externalId(awsConfigModel.getExternalId()) .region(awsConfigModel.getRegion()) .endpoint(awsConfigModel.getEndpoint()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index e393911f71..1a576c26ea 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -23,6 +23,7 @@ import jakarta.annotation.Nonnull; import java.net.URI; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -33,6 +34,8 @@ import org.apache.polaris.core.storage.StorageAccessProperty; import org.apache.polaris.core.storage.StorageUtil; import org.apache.polaris.core.storage.aws.StsClientProvider.StsDestination; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.policybuilder.iam.IamConditionOperator; import software.amazon.awssdk.policybuilder.iam.IamEffect; @@ -49,6 +52,9 @@ public class AwsCredentialsStorageIntegration private final StsClientProvider stsClientProvider; private final Optional credentialsProvider; + private static final Logger LOGGER = + LoggerFactory.getLogger(AwsCredentialsStorageIntegration.class); + public AwsCredentialsStorageIntegration( AwsStorageConfigurationInfo config, StsClient fixedClient) { this(config, (destination) -> fixedClient); @@ -80,6 +86,7 @@ public StorageAccessConfig getSubscopedCreds( realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); AwsStorageConfigurationInfo storageConfig = config(); String region = storageConfig.getRegion(); + String accountId = storageConfig.getAwsAccountId(); StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder(); if (shouldUseSts(storageConfig)) { @@ -90,10 +97,12 @@ public StorageAccessConfig getSubscopedCreds( .roleSessionName("PolarisAwsCredentialsStorageIntegration") .policy( policyString( - storageConfig.getAwsPartition(), + storageConfig, allowListOperation, allowedReadLocations, - allowedWriteLocations) + allowedWriteLocations, + region, + accountId) .toJson()) .durationSeconds(storageCredentialDurationSeconds); credentialsProvider.ifPresent( @@ -163,12 +172,13 @@ private boolean shouldUseSts(AwsStorageConfigurationInfo storageConfig) { * ListBucket privileges with no resources. This prevents us from sending an empty policy to AWS * and just assuming the role with full privileges. */ - // TODO - add KMS key access private IamPolicy policyString( - String awsPartition, + AwsStorageConfigurationInfo storageConfigurationInfo, boolean allowList, Set readLocations, - Set writeLocations) { + Set writeLocations, + String region, + String accountId) { IamPolicy.Builder policyBuilder = IamPolicy.builder(); IamStatement.Builder allowGetObjectStatementBuilder = IamStatement.builder() @@ -178,7 +188,9 @@ private IamPolicy policyString( Map bucketListStatementBuilder = new HashMap<>(); Map bucketGetLocationStatementBuilder = new HashMap<>(); - String arnPrefix = arnPrefixForPartition(awsPartition); + String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); + String currentKmsKey = storageConfigurationInfo.currentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -225,6 +237,9 @@ private IamPolicy policyString( arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/"))); }); policyBuilder.addStatement(allowPutObjectStatementBuilder.build()); + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, true, region, accountId); + } else { + addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, false, region, accountId); } if (!bucketListStatementBuilder.isEmpty()) { bucketListStatementBuilder @@ -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 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); + allowKms.addResource(IamResource.create(kmsKeyArn)); + } + } + + private static boolean hasAllowedKmsKeys(List allowedKmsKeys) { + return allowedKmsKeys != null && !allowedKmsKeys.isEmpty(); + } + + private static void addAllowedKmsKeyResources( + List allowedKmsKeys, IamStatement.Builder allowKms) { + allowedKmsKeys.forEach( + keyArn -> { + LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + allowKms.addResource(IamResource.create(keyArn)); + }); + } + + private static void addAllKeysResource( + String region, String accountId, IamStatement.Builder allowKms) { + String allKeysArn = arnKeyAll(region, accountId); + allowKms.addResource(IamResource.create(allKeysArn)); + LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); + } + + private static String arnKeyAll(String region, String accountId) { + return String.format("arn:aws:kms:%s:%s:key/*", region, accountId); } private static String arnPrefixForPartition(String awsPartition) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 69c669222d..34ab547b48 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -27,6 +27,7 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import jakarta.annotation.Nullable; import java.net.URI; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; @@ -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(); + + /** Comma-separated list of allowed KMS Key ARNs, optional */ + @Nullable + public abstract List allowedKmsKeys(); + /** AWS external ID, optional */ @Nullable public abstract String getExternalId(); diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 3460dc23f7..96e69ad1f3 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/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"); } @Test diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java index fb0c63c403..2732577489 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java @@ -389,6 +389,8 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { String warehouseKeyPrefix = "path/to/warehouse"; String firstPath = warehouseKeyPrefix + "/namespace/table"; String secondPath = warehouseKeyPrefix + "/oldnamespace/table"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -402,8 +404,26 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(3) + .hasSize(4) .satisfiesExactly( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -456,7 +476,7 @@ public void testGetSubscopedCredsInlinePolicyWithoutWrites() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -482,6 +502,8 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { String externalId = "externalId"; String bucket = "bucket"; String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-2"; + String accountId = "012345678901"; Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) .thenAnswer( invocation -> { @@ -495,8 +517,26 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { assertThat(policy) .extracting(IamPolicy::statements) .asInstanceOf(InstanceOfAssertFactories.list(IamStatement.class)) - .hasSize(2) - .satisfiesExactly( + .hasSize(3) + .satisfiesExactlyInAnyOrder( + statement -> + assertThat(statement) + .returns(IamEffect.ALLOW, IamStatement::effect) + .returns( + List.of( + IamAction.create( + "kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey")), + IamStatement::actions) + .returns( + List.of( + IamResource.create( + String.format( + "arn:aws:kms:%s:%s:key/*", + region, accountId))), + IamStatement::resources), statement -> assertThat(statement) .returns(IamEffect.ALLOW, IamStatement::effect) @@ -523,7 +563,7 @@ public void testGetSubscopedCredsInlinePolicyWithEmptyReadAndWrite() { .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) .roleARN(roleARN) .externalId(externalId) - .region("us-east-2") + .region(region) .build(), stsClient) .getSubscopedCreds( @@ -662,6 +702,177 @@ public void testNoClientRegion(String awsPartition) { ; } + @Test + public void testKmsKeyPolicyLogic() { + StsClient stsClient = Mockito.mock(StsClient.class); + String roleARN = "arn:aws:iam::012345678901:role/jdoe"; + String externalId = "externalId"; + String bucket = "bucket"; + String warehouseKeyPrefix = "path/to/warehouse"; + String region = "us-east-1"; + String accountId = "012345678901"; + String currentKmsKey = "arn:aws:kms:us-east-1:012345678901:key/current-key"; + List allowedKmsKeys = + List.of( + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-1", + "arn:aws:kms:us-east-1:012345678901:key/allowed-key-2"); + + // Test with current KMS key and write permissions + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with write permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"), + IamAction.create("kms:Encrypt"))); + assertThat(stmt.resources()).contains(IamResource.create(currentKmsKey)); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .currentKmsKey(currentKmsKey) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + + // Test with allowed KMS keys and read-only permissions + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify KMS statement exists with read-only permissions + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.actions()) + .containsAll( + List.of( + IamAction.create("kms:GenerateDataKeyWithoutPlaintext"), + IamAction.create("kms:DescribeKey"), + IamAction.create("kms:Decrypt"), + IamAction.create("kms:GenerateDataKey"))); + assertThat(stmt.actions()).doesNotContain(IamAction.create("kms:Encrypt")); + assertThat(stmt.resources()) + .containsExactlyInAnyOrder( + IamResource.create(allowedKmsKeys.get(0)), + IamResource.create(allowedKmsKeys.get(1))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .allowedKmsKeys(allowedKmsKeys) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and read-only (should add wildcard KMS access) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify wildcard KMS statement exists + assertThat(policy.statements()) + .anySatisfy( + stmt -> { + assertThat(stmt.resources()) + .contains( + IamResource.create( + String.format("arn:aws:kms:%s:%s:key/*", region, accountId))); + }); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(), + Optional.empty()); + + // Test with no KMS keys and write permissions (should not add KMS statement) + Mockito.reset(stsClient); + Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class))) + .thenAnswer( + invocation -> { + AssumeRoleRequest request = invocation.getArgument(0); + IamPolicy policy = IamPolicy.fromJson(request.policy()); + + // Verify no KMS statement exists + assertThat(policy.statements()) + .noneMatch( + stmt -> + stmt.actions().stream() + .anyMatch(action -> action.value().startsWith("kms:"))); + + return ASSUME_ROLE_RESPONSE; + }); + + new AwsCredentialsStorageIntegration( + AwsStorageConfigurationInfo.builder() + .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix)) + .roleARN(roleARN) + .externalId(externalId) + .region(region) + .build(), + stsClient) + .getSubscopedCreds( + EMPTY_REALM_CONFIG, + true, + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Set.of(s3Path(bucket, warehouseKeyPrefix + "/table")), + Optional.empty()); + } + private static @Nonnull String s3Arn(String partition, String bucket, String keyPrefix) { String bucketArn = "arn:" + partition + ":s3:::" + bucket; if (keyPrefix == null) { diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index 5c3c220c82..d611be8a58 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,6 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -334,6 +335,8 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test @@ -342,6 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") + .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -357,6 +361,8 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index d1775b7598..f762bcf43d 100644 --- a/spec/polaris-management-service.yml +++ b/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 + items: + type: string + example: ["arn:aws:kms::123456789001:key/01234578"] region: type: string description: the aws region where data is stored From 8a1fe7eeed21803e79c25cbd940cbffac977414a Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Fri, 14 Nov 2025 13:57:18 +0000 Subject: [PATCH 20/21] Feature/s3 kms key (#18) * fixed merge issue --- .../apache/polaris/service/entity/CatalogEntityTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java index d611be8a58..24c7814ce8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/entity/CatalogEntityTest.java @@ -321,7 +321,7 @@ public void testCatalogTypeDefaultsToInternal() { AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") .setExternalId("externalId") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(List.of(baseLocation)) @@ -335,7 +335,7 @@ public void testCatalogTypeDefaultsToInternal() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.INTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } @@ -345,7 +345,7 @@ public void testCatalogTypeExternalPreserved() { AwsStorageConfigInfo storageConfigModel = AwsStorageConfigInfo.builder() .setRoleArn("arn:aws:iam::012345678901:role/test-role") - .setKmsKeyArn("arn:aws:kms:us-east-1:012345678901:key/444343245") + .setCurrentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245") .setExternalId("externalId") .setUserArn("aws::a:user:arn") .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) @@ -361,7 +361,7 @@ public void testCatalogTypeExternalPreserved() { Catalog catalog = catalogEntity.asCatalog(serviceIdentityProvider); assertThat(catalog.getType()).isEqualTo(Catalog.TypeEnum.EXTERNAL); - assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getKmsKeyArn()) + assertThat(((AwsStorageConfigInfo) catalog.getStorageConfigInfo()).getCurrentKmsKey()) .isEqualTo("arn:aws:kms:us-east-1:012345678901:key/444343245"); } From 5a60a69d0a2637d33b49941ca240d3ea68410759 Mon Sep 17 00:00:00 2001 From: "Rizzo Cascio, Fabio" Date: Thu, 20 Nov 2025 10:34:38 +0000 Subject: [PATCH 21/21] Feature/s3 kms key (#19) fixed issue that broke integration tests and updated code based on PR comments --- CHANGELOG.md | 2 +- .../admin/model/CatalogSerializationTest.java | 27 +++++++++++++++++++ .../polaris/core/entity/CatalogEntity.java | 4 +-- .../aws/AwsCredentialsStorageIntegration.java | 27 ++++++++++--------- .../aws/AwsStorageConfigurationInfo.java | 4 +-- .../aws/AwsStorageConfigurationInfoTest.java | 3 +-- spec/polaris-management-service.yml | 2 +- 7 files changed, 48 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0abd73c50..0ef1645c5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,7 +83,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 +- Added KMS properties (optional) to catalog storage config to enable S3 data encryption. - Added a finer grained authorization model for UpdateTable requests. Existing privileges continue to work for granting UpdateTable, such as `TABLE_WRITE_PROPERTIES`. However, you can now instead grant privileges just for specific operations, such as `TABLE_ADD_SNAPSHOT` - Added a Management API endpoint to reset principal credentials, controlled by the `ENABLE_CREDENTIAL_RESET` (default: true) feature flag. diff --git a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java index c645f086cb..3244f14732 100644 --- a/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java +++ b/api/management-model/src/test/java/org/apache/polaris/core/admin/model/CatalogSerializationTest.java @@ -53,6 +53,33 @@ public void testCatalogSerialization(String description, Catalog catalog) @Test public void testJsonFormat() throws JsonProcessingException { + Catalog catalog = + new Catalog( + Catalog.TypeEnum.INTERNAL, + TEST_CATALOG_NAME, + new CatalogProperties(TEST_LOCATION), + AwsStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.S3) + .setRoleArn(TEST_ROLE_ARN) + .build()); + + String json = mapper.writeValueAsString(catalog); + + assertThat(json) + .isEqualTo( + "{\"type\":\"INTERNAL\"," + + "\"name\":\"test-catalog\"," + + "\"properties\":{\"default-base-location\":\"s3://test/\"}," + + "\"storageConfigInfo\":{" + + "\"roleArn\":\"arn:aws:iam::123456789012:role/test-role\"," + + "\"allowedKmsKeys\":[]," + + "\"pathStyleAccess\":false," + + "\"storageType\":\"S3\"," + + "\"allowedLocations\":[]" + + "}}"); + } + + @Test + public void testJsonFormatWithKmsProperties() throws JsonProcessingException { Catalog catalog = new Catalog( Catalog.TypeEnum.INTERNAL, diff --git a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java index 095c950aeb..4607728b35 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java @@ -161,8 +161,8 @@ private StorageConfigInfo getStorageInfo(Map internalProperties) .setRoleArn(awsConfig.getRoleARN()) .setExternalId(awsConfig.getExternalId()) .setUserArn(awsConfig.getUserARN()) - .setCurrentKmsKey(awsConfig.currentKmsKey()) - .setAllowedKmsKeys(awsConfig.allowedKmsKeys()) + .setCurrentKmsKey(awsConfig.getCurrentKmsKey()) + .setAllowedKmsKeys(awsConfig.getAllowedKmsKeys()) .setStorageType(StorageConfigInfo.StorageTypeEnum.S3) .setAllowedLocations(awsConfig.getAllowedLocations()) .setRegion(awsConfig.getRegion()) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java index 1a576c26ea..2996006954 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java @@ -189,8 +189,8 @@ private IamPolicy policyString( Map bucketGetLocationStatementBuilder = new HashMap<>(); String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); - String currentKmsKey = storageConfigurationInfo.currentKmsKey(); - List allowedKmsKeys = storageConfigurationInfo.allowedKmsKeys(); + String currentKmsKey = storageConfigurationInfo.getCurrentKmsKey(); + List allowedKmsKeys = storageConfigurationInfo.getAllowedKmsKeys(); Stream.concat(readLocations.stream(), writeLocations.stream()) .distinct() .forEach( @@ -254,20 +254,18 @@ private IamPolicy policyString( bucketGetLocationStatementBuilder .values() .forEach(statementBuilder -> policyBuilder.addStatement(statementBuilder.build())); - var r = policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); - LOGGER.info("Policies {}", r); - return r; + return policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); } private static void addKmsKeyPolicy( String kmsKeyArn, List allowedKmsKeys, IamPolicy.Builder policyBuilder, - boolean canEncrypt, + boolean canWrite, String region, String accountId) { - IamStatement.Builder allowKms = buildBaseKmsStatement(canEncrypt); + IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite); boolean hasCurrentKey = kmsKeyArn != null; boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys); @@ -282,10 +280,13 @@ private static void addKmsKeyPolicy( // Add KMS statement if we have any KMS key configuration if (hasCurrentKey || hasAllowedKeys) { policyBuilder.addStatement(allowKms.build()); - } else if (!canEncrypt) { + } else if (!canWrite) { // Only add wildcard KMS access for read-only operations when no specific keys are configured - addAllKeysResource(region, accountId, allowKms); - policyBuilder.addStatement(allowKms.build()); + // this check is for minio because it doesn't have region or account id + if (region != null && accountId != null) { + addAllKeysResource(region, accountId, allowKms); + policyBuilder.addStatement(allowKms.build()); + } } } @@ -307,7 +308,7 @@ private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) { private static void addKmsKeyResource(String kmsKeyArn, IamStatement.Builder allowKms) { if (kmsKeyArn != null) { - LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn); + LOGGER.debug("Adding KMS key policy for key {}", kmsKeyArn); allowKms.addResource(IamResource.create(kmsKeyArn)); } } @@ -320,7 +321,7 @@ private static void addAllowedKmsKeyResources( List allowedKmsKeys, IamStatement.Builder allowKms) { allowedKmsKeys.forEach( keyArn -> { - LOGGER.info("Adding allowed KMS key policy for key {}", keyArn); + LOGGER.debug("Adding allowed KMS key policy for key {}", keyArn); allowKms.addResource(IamResource.create(keyArn)); }); } @@ -329,7 +330,7 @@ private static void addAllKeysResource( String region, String accountId, IamStatement.Builder allowKms) { String allKeysArn = arnKeyAll(region, accountId); allowKms.addResource(IamResource.create(allKeysArn)); - LOGGER.info("Adding KMS key policy for all keys in account {}", accountId); + LOGGER.debug("Adding KMS key policy for all keys in account {}", accountId); } private static String arnKeyAll(String region, String accountId) { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java index 34ab547b48..b62265f929 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java @@ -66,11 +66,11 @@ public String getFileIoImplClassName() { /** KMS Key ARN for server-side encryption,used for writes, optional */ @Nullable - public abstract String currentKmsKey(); + public abstract String getCurrentKmsKey(); /** Comma-separated list of allowed KMS Key ARNs, optional */ @Nullable - public abstract List allowedKmsKeys(); + public abstract List getAllowedKmsKeys(); /** AWS external ID, optional */ @Nullable diff --git a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java index 96e69ad1f3..3460dc23f7 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java @@ -70,8 +70,7 @@ public void testStsEndpoint() { private static ImmutableAwsStorageConfigurationInfo.Builder newBuilder() { return AwsStorageConfigurationInfo.builder() - .roleARN("arn:aws:iam::123456789012:role/polaris-test") - .currentKmsKey("arn:aws:kms:us-east-1:012345678901:key/444343245"); + .roleARN("arn:aws:iam::123456789012:role/polaris-test"); } @Test diff --git a/spec/polaris-management-service.yml b/spec/polaris-management-service.yml index f762bcf43d..03c894b6e4 100644 --- a/spec/polaris-management-service.yml +++ b/spec/polaris-management-service.yml @@ -1109,7 +1109,7 @@ components: example: "arn:aws:kms::123456789001:key/01234578" allowedKmsKeys: type: array - description: the aws kms keys arn used to encrypt s3 data + description: The list of kms keys that this catalog and its clients are allow to use for reading s3 data items: type: string example: ["arn:aws:kms::123456789001:key/01234578"]