adding support to use a kms key for s3 buckets data encryption (AWS only) #2802
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<AwsCredentialsProvider> 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<String> allowedReadLocations, | ||
| @Nonnull Set<String> allowedWriteLocations, | ||
| Optional<String> 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)) { | ||
|
|
@@ -90,10 +99,13 @@ public AccessConfig getSubscopedCreds( | |
| .roleSessionName("PolarisAwsCredentialsStorageIntegration") | ||
| .policy( | ||
| policyString( | ||
| storageConfig, | ||
| allowListOperation, | ||
| allowedReadLocations, | ||
| allowedWriteLocations, | ||
| kmsKey, | ||
| region, | ||
| accountId) | ||
| .toJson()) | ||
| .durationSeconds(storageCredentialDurationSeconds); | ||
| credentialsProvider.ifPresent( | ||
|
|
@@ -163,12 +175,14 @@ 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. | ||
| */ | ||
| private IamPolicy policyString( | ||
| AwsStorageConfigurationInfo storageConfigurationInfo, | ||
| boolean allowList, | ||
| Set<String> readLocations, | ||
| Set<String> writeLocations, | ||
| String kmsKey, | ||
| String region, | ||
| String accountId) { | ||
| IamPolicy.Builder policyBuilder = IamPolicy.builder(); | ||
| IamStatement.Builder allowGetObjectStatementBuilder = | ||
| IamStatement.builder() | ||
|
|
@@ -178,7 +192,9 @@ private IamPolicy policyString( | |
| Map<String, IamStatement.Builder> bucketListStatementBuilder = new HashMap<>(); | ||
| Map<String, IamStatement.Builder> bucketGetLocationStatementBuilder = new HashMap<>(); | ||
|
|
||
| String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition()); | ||
| String kmsKeyArn = storageConfigurationInfo.getKmsKeyArn(); | ||
|
There was a problem hiding this comment. Can / will this work with MinIO KMS? Cf. https://github.com/minio/minio/blob/master/docs/kms/README.md There was a problem hiding this comment. I will need to test that. There was a problem hiding this comment. It's fine either way, but if it did not work with MinIO it would be good to mention AWS (only) in the PR description. Also, I'm pretty sure we'll have to support KMS in MinIO at some point, so if current changes were reusable it would be great (but not required). |
||
|
|
||
| Stream.concat(readLocations.stream(), writeLocations.stream()) | ||
| .distinct() | ||
| .forEach( | ||
|
|
@@ -225,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 | ||
|
|
@@ -239,7 +258,45 @@ private IamPolicy policyString( | |
| bucketGetLocationStatementBuilder | ||
| .values() | ||
| .forEach(statementBuilder -> policyBuilder.addStatement(statementBuilder.build())); | ||
| var r = policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build(); | ||
| LOGGER.info("Policies {}", r); | ||
| return r; | ||
| } | ||
|
|
||
| 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) { | ||
| LOGGER.info("Adding KMS key policy for key {}", kmsKeyArn); | ||
| allowKms.addResource(IamResource.create(kmsKeyArn)); | ||
| } | ||
| 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) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that "storage integration" code should work independently of table properties. In other words Polaris should own storage integration and
AccessConfig. Table properties are settable by the end users outside the context of a catalog (e.g. via registerTable). Mixing both sources of integration config is confusing IMHO.Is the intention here to support different KMS config per table?
Polaris may need to be able to get
AccessConfigin order to load table metadata (which has table properties)... Does this not create a chicken-and-egg problem?That said, I'm open to a wider discussion about this on the
devML.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is to support the usage of the s3 properties defined in iceberg, we don't really need to pass down props or have that code in there as long as we can agree on the code https://github.com/apache/polaris/pull/2802/files#diff-d305f7a426a7690c576722c114257792b3fcee726624655d15893b71499827f8R274.
If no kms key was defined in the polaris AWS storage then we allow the usage of any keys owned by that account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we discuss
arnKeyAll(and I do not have any objections there), I think we need to resolve the conceptual issue of using metadata properties for generatingAccessConfig. Polaris may needAccessConfigin order to read metadata JSON files (which have table properties)... How can this work in principle?