You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "iting0321 (via GitHub)" <gi...@apache.org> on 2026/07/26 10:30:59 UTC
[PR] Add decrypt-only access for legacy AWS KMS keys [polaris]
iting0321 opened a new pull request, #5162:
URL: https://github.com/apache/polaris/pull/5162
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Summary
Fixes #3338 .
This change adds support for historical AWS KMS keys that can decrypt existing S3 data without being permitted to encrypt new data.
Previously, every explicitly configured KMS key received encryption permissions when Polaris vended write-capable credentials. During key rotation, this meant that retaining access to data encrypted with an older key also allowed clients to continue using that key for new writes.
## Changes
- Add `legacyKmsKeys` to the AWS catalog storage configuration.
- Grant legacy keys only:
- `kms:DescribeKey`
- `kms:Decrypt`
- Continue granting current and allowed keys encryption, decryption, and data-key generation permissions for write-capable credentials.
- Deprecate `currentKmsKey` in favor of `allowedKmsKeys` while preserving its existing behavior for compatibility.
- Reject configurations where a legacy key is also configured as:
- `currentKmsKey`
- an entry in `allowedKmsKeys`
- Add the repeatable `--legacy-kms-key` CLI option.
- Support `legacy_kms_keys` in setup configuration import and export.
## Key rotation workflow
A key currently used for encryption should be configured as an allowed key:
```yaml
allowed_kms_keys:
- arn:aws:kms:us-east-1:123456789012:key/current-key
```
After rotating to a new key, the previous key can be moved to the legacy list:
```yaml
allowed_kms_keys:
- arn:aws:kms:us-east-1:123456789012:key/new-key
legacy_kms_keys:
- arn:aws:kms:us-east-1:123456789012:key/previous-key
```
Credentials can then decrypt objects encrypted with the previous key, but cannot use it to encrypt new objects or generate data keys.
## Validation
AWS combines permissions from applicable `Allow` statements. Therefore, emitting separate allowed-key and legacy-key statements would not enforce decrypt-only behavior if the same ARN appeared in both categories.
The storage configuration now rejects overlap rather than silently choosing one category. This validation applies when catalog configuration is created and when persisted configuration is deserialized.
Overlap between `currentKmsKey` and `allowedKmsKeys` remains valid because both are encrypt-capable categories.
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #3338
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [x] ๐ก Added comments for complex logic
- [x] ๐งพ Updated `CHANGELOG.md` (if needed)
- [x] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3777760882
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Would that cause correctness issue if we don't make the validation? See more details here, https://github.com/apache/polaris/pull/5162#discussion_r3661448593.
The backward compatibility is also important, but we don't change the existing one unless the new field(decryptOnlyKmsKey) was added, see more details here, https://github.com/apache/polaris/pull/5162#discussion_r3770075051.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3688331017
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
No problem!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #5162:
URL: https://github.com/apache/polaris/pull/5162#issuecomment-5319902762
> @MonkeyCanCode : FYI re: Python CLI changes.
Thanks for tagging. I will take a look later today
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5162:
URL: https://github.com/apache/polaris/pull/5162#issuecomment-5384243244
Thanks for the contribution, @iting0321 ! Thanks everyone for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3661448593
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -166,6 +170,19 @@ protected void check() {
throw new IllegalArgumentException("ARN does not match the expected role ARN pattern");
}
}
+
+ List<String> legacyKmsKeys = getLegacyKmsKeys();
+ if (legacyKmsKeys != null) {
+ String currentKmsKey = getCurrentKmsKey();
+ checkArgument(
+ currentKmsKey == null || !legacyKmsKeys.contains(currentKmsKey),
+ "currentKmsKey must not also be configured in legacyKmsKeys");
+
+ List<String> allowedKmsKeys = getAllowedKmsKeys();
+ checkArgument(
+ allowedKmsKeys == null || allowedKmsKeys.stream().noneMatch(legacyKmsKeys::contains),
Review Comment:
The whole decrypt-only guarantee rides on this overlap check, and it compares ARN strings exactly. If the same key can show up in two forms, say an alias ARN in `allowedKmsKeys` and the underlying key ARN in `legacyKmsKeys`, those are different strings, so the check passes. Since AWS unions Allow statements, the legacy key would then still pick up encrypt from the allowed statement, defeating decrypt-only. Harmless if entries are always canonical key ARNs, but KMS keys aren't format-validated here the way `roleARN` is. Are canonical key ARNs guaranteed, or should this normalize/validate the ARN form before comparing?
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
Review Comment:
nit: `arn` -> `ARN`, `aws` -> `AWS`
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
Maybe we could give a more descriptive name, like `decryptOnlyKmsKeys`
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
Review Comment:
We kind of re-purpose it from "reading" to "encrypting and decrypting", but by checking the existing behavior, the `allowedKmsKeys` are also used for encryption, I think that repurposing is fine.
https://github.com/apache/polaris/blob/0d164efaa4339fafdd8ca8157bd2dccd4f37c78c/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java#L431-L471.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3777128521
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -329,6 +329,7 @@ private static IamPolicy policyString(
String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition());
String currentKmsKey = storageConfigurationInfo.getCurrentKmsKey();
Review Comment:
I agreed with point, and removed `currentKmsKey` from the main call path. `currentKmsKey ` is now only accepted for backward compatibility and converted to allowedKmsKeys.
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -150,6 +150,7 @@ private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties)
.setUserArn(awsConfig.getUserARN())
.setCurrentKmsKey(awsConfig.getCurrentKmsKey())
Review Comment:
I think we do not need to return `currentKmsKey` in responses, and I removed `setCurrentKmsKey` from the response mapping.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -166,6 +182,40 @@ protected void check() {
throw new IllegalArgumentException("ARN does not match the expected role ARN pattern");
}
}
+
+ List<String> decryptOnlyKmsKeys = getDecryptOnlyKmsKeys();
+ if (decryptOnlyKmsKeys != null && !decryptOnlyKmsKeys.isEmpty()) {
+ String currentKmsKey = getCurrentKmsKey();
+ if (currentKmsKey != null) {
+ validateKmsKeyArn("currentKmsKey", currentKmsKey);
+ }
+
+ List<String> allowedKmsKeys = getAllowedKmsKeys();
+ validateKmsKeyArns("allowedKmsKeys", allowedKmsKeys);
Review Comment:
I updated the validation message to explicitly explain that `decryptOnlyKmsKeys` activates this stricter requirement, including when the invalid value comes from `allowedKmsKeys` or `currentKmsKey`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3787651044
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Fine by me. WDYT? @iting0321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3780011191
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Format issues are specific to a particular Polaris admin and can be corrected. Strict pattern affects all users of non-AWS storage and require a code change to alter behaviour.
In any case, I'm not against extra validation. I'm just proposing not to mix it with the config refactoring.
Note that even this RegEx does not guarantee that two equivalent ARNs (per storage's interpretation) may have different forms in Polaris config - the parts after `key/` are not validated.
If the concern is about strong "only" semantics in `decryptOnlyKeys`, I'd propose to use:
* `allowedKmsKey` -> `encryptionKeys`
* Forward `currentKmsKey` to `encryptionKeys`
* Add `decryptionKeys` without guarantees that the same key may not appear in `encryptionKeys` - it's up to the admin user to control.
* Forward all `encryptionKeys` to `decryptionKeys` (with doc notices).
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3676325480
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -166,6 +170,19 @@ protected void check() {
throw new IllegalArgumentException("ARN does not match the expected role ARN pattern");
}
}
+
+ List<String> legacyKmsKeys = getLegacyKmsKeys();
+ if (legacyKmsKeys != null) {
+ String currentKmsKey = getCurrentKmsKey();
+ checkArgument(
+ currentKmsKey == null || !legacyKmsKeys.contains(currentKmsKey),
+ "currentKmsKey must not also be configured in legacyKmsKeys");
+
+ List<String> allowedKmsKeys = getAllowedKmsKeys();
+ checkArgument(
+ allowedKmsKeys == null || allowedKmsKeys.stream().noneMatch(legacyKmsKeys::contains),
Review Comment:
Sounds good to me. Let me know once the change has been done. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3814127766
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -148,8 +149,8 @@ private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties)
.setRoleArn(awsConfig.getRoleARN())
.setExternalId(awsConfig.getExternalId())
.setUserArn(awsConfig.getUserARN())
- .setCurrentKmsKey(awsConfig.getCurrentKmsKey())
- .setAllowedKmsKeys(awsConfig.getAllowedKmsKeys())
+ .setEncryptionKeys(awsConfig.getEffectiveEncryptionKeys())
Review Comment:
I now populate deprecated `allowedKmsKeys` with the effective encryption keys, so older clients still receive all configured keys. And I preserve `currentKmsKey` only when an actual legacy value exists.
Tests were updated accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3768306624
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
@@ -1108,6 +1112,60 @@ public void testKmsKeyPolicyLogic() {
Optional.empty(),
CredentialVendingContext.empty());
+ // Test that legacy KMS keys only receive decrypt permissions for write operations
+ Mockito.reset(stsClient);
+ Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class)))
+ .thenAnswer(
+ invocation -> {
+ AssumeRoleRequest request = invocation.getArgument(0);
+ IamPolicy policy = IamPolicy.fromJson(request.policy());
+
+ assertThat(policy.statements())
+ .anySatisfy(
+ stmt -> {
+ assertThat(stmt.resources())
+ .containsExactlyInAnyOrder(
+ IamResource.create(allowedKmsKeys.get(0)),
+ IamResource.create(allowedKmsKeys.get(1)));
+ assertThat(stmt.actions())
+ .contains(
+ IamAction.create("kms:Decrypt"),
+ IamAction.create("kms:Encrypt"),
+ IamAction.create("kms:GenerateDataKey"));
+ })
+ .anySatisfy(
+ stmt -> {
+ assertThat(stmt.resources())
+ .containsExactlyInAnyOrder(
+ IamResource.create(legacyKmsKeys.get(0)),
+ IamResource.create(legacyKmsKeys.get(1)));
+ assertThat(stmt.actions())
+ .containsExactlyInAnyOrder(
+ IamAction.create("kms:DescribeKey"),
+ IamAction.create("kms:Decrypt"));
+ });
+
+ return ASSUME_ROLE_RESPONSE;
+ });
+
+ AwsStorageConfigurationInfo configWithLegacyKmsKeys =
+ AwsStorageConfigurationInfo.builder()
+ .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix))
+ .roleARN(roleARN)
+ .externalId(externalId)
+ .region(region)
+ .allowedKmsKeys(allowedKmsKeys)
+ .legacyKmsKeys(legacyKmsKeys)
+ .build();
+ new AwsCredentialsStorageIntegration(stsClient, configWithLegacyKmsKeys, EMPTY_REALM_CONFIG)
+ .getStorageAccessConfig(
Review Comment:
I agree with the point and I added ` Mockito.verify(stsClient).assumeRole(Mockito.isA(AssumeRoleRequest.class));` that ensures the thenAnswer policy assertions actually execute and that assumeRole is called exactly once.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
I think the validation should do more than reject blanks, aliases, and `*` or `?` wildcards. I chose to require canonical KMS key ARNs because these values are written directly into the IAM policy `Resource`.
This gives us a clear and valid resource format and makes exact overlap checking reliable. Non-AWS KMS identifiers may have different resource and matching semantics, so supporting them would require a separate provider-specific policy contract rather than relaxing this AWS-specific path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3770286617
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -329,6 +329,7 @@ private static IamPolicy policyString(
String arnPrefix = arnPrefixForPartition(storageConfigurationInfo.getAwsPartition());
String currentKmsKey = storageConfigurationInfo.getCurrentKmsKey();
Review Comment:
If we deprecated this configuration, why do we still use it on the main call path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3810076088
##########
client/python/tests/test_parser_basic.py:
##########
@@ -166,6 +166,17 @@ def test_parse_argparse_valid_commands(self) -> None:
Parser.parse(["catalogs", "create", "catalog_name"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "internal"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "INTERNAL"])
+ Parser.parse(
+ [
+ "catalogs",
+ "create",
+ "catalog_name",
+ "--decryption-key",
+ "arn:aws:kms:us-east-1:012345678901:key/decryption-key",
Review Comment:
added!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3770258437
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
This is similar to Role ARNs, whose validate we had to relax... cf. #3619
However, we can do that when users demand, of course.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3780011191
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Format issues are specific to a particular Polaris admin and can be corrected. Strict pattern affects all users of non-AWS storage and require a code change to alter behaviour.
In any case, I'm not against extra validation. I'm just proposing not to mix it with the config refactoring.
Note that even this RegEx does not guarantee that two equivalent ARNs (per storage's interpretation) may have different forms in Polaris config - the parts after `key/` are not validated.
If the concern is about strong "only" semantics in `decryptOnlyKeys`, I'd propose to use:
* `allowedKmsKey` -> `encryptionKeys`
* Forward `currentKmsKey` to `encryptionKeys`
* Add `decryptionKeys` without guarantees that the same key may not appear in `encryptionKeys` - it's up to the admin user to control.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5162:
URL: https://github.com/apache/polaris/pull/5162#issuecomment-5319867260
@MonkeyCanCode : FYI re: Python CLI changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3806125461
##########
client/python/tests/test_parser_basic.py:
##########
@@ -166,6 +166,17 @@ def test_parse_argparse_valid_commands(self) -> None:
Parser.parse(["catalogs", "create", "catalog_name"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "internal"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "INTERNAL"])
+ Parser.parse(
Review Comment:
I updated the test to assert the parsed values directly and now also covers using `current-kms-key` and `allowed-kms-key` together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3655628996
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -64,14 +64,18 @@ public String getFileIoImplClassName() {
@Nullable
public abstract String getRoleARN();
- /** KMS Key ARN for server-side encryption,used for writes, optional */
+ /** Deprecated KMS Key ARN, retained for compatibility and treated as an allowed KMS key. */
@Nullable
Review Comment:
```suggestion
@Deprecated
@Nullable
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3752483832
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,79 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testLegacyKmsKeysMustNotOverlapEncryptCapableKeys() {
+ String keyArn = "arn:aws:kms:us-east-1:012345678901:key/cccccccc-cccc-cccc-cccc-cccccccccccc";
+
+ assertThatThrownBy(
+ () -> newBuilder().currentKmsKey(keyArn).legacyKmsKeys(List.of(keyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("currentKmsKey must not also be configured in legacyKmsKeys");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder().allowedKmsKeys(List.of(keyArn)).legacyKmsKeys(List.of(keyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("allowedKmsKeys and legacyKmsKeys must not overlap");
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("invalidKmsKeyArns")
+ public void testLegacyKmsKeysRejectNonCanonicalKeyArns(String description, String invalidKeyArn) {
+ assertThatThrownBy(() -> newBuilder().legacyKmsKeys(List.of(invalidKeyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("legacyKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .allowedKmsKeys(List.of(invalidKeyArn))
+ .legacyKmsKeys(List.of(LEGACY_KMS_KEY_ARN))
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("allowedKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .currentKmsKey(invalidKeyArn)
Review Comment:
I'm not sure using the deprecated method for this test is valuable long-term... Why not use `legacyKmsKeys` here?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Could this be too restrictive? Are we sure KMS ARNs for non-AWS systems match this pattern?
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
@@ -1108,6 +1112,60 @@ public void testKmsKeyPolicyLogic() {
Optional.empty(),
CredentialVendingContext.empty());
+ // Test that legacy KMS keys only receive decrypt permissions for write operations
+ Mockito.reset(stsClient);
+ Mockito.when(stsClient.assumeRole(Mockito.isA(AssumeRoleRequest.class)))
+ .thenAnswer(
+ invocation -> {
+ AssumeRoleRequest request = invocation.getArgument(0);
+ IamPolicy policy = IamPolicy.fromJson(request.policy());
+
+ assertThat(policy.statements())
+ .anySatisfy(
+ stmt -> {
+ assertThat(stmt.resources())
+ .containsExactlyInAnyOrder(
+ IamResource.create(allowedKmsKeys.get(0)),
+ IamResource.create(allowedKmsKeys.get(1)));
+ assertThat(stmt.actions())
+ .contains(
+ IamAction.create("kms:Decrypt"),
+ IamAction.create("kms:Encrypt"),
+ IamAction.create("kms:GenerateDataKey"));
+ })
+ .anySatisfy(
+ stmt -> {
+ assertThat(stmt.resources())
+ .containsExactlyInAnyOrder(
+ IamResource.create(legacyKmsKeys.get(0)),
+ IamResource.create(legacyKmsKeys.get(1)));
+ assertThat(stmt.actions())
+ .containsExactlyInAnyOrder(
+ IamAction.create("kms:DescribeKey"),
+ IamAction.create("kms:Decrypt"));
+ });
+
+ return ASSUME_ROLE_RESPONSE;
+ });
+
+ AwsStorageConfigurationInfo configWithLegacyKmsKeys =
+ AwsStorageConfigurationInfo.builder()
+ .addAllowedLocation(s3Path(bucket, warehouseKeyPrefix))
+ .roleARN(roleARN)
+ .externalId(externalId)
+ .region(region)
+ .allowedKmsKeys(allowedKmsKeys)
+ .legacyKmsKeys(legacyKmsKeys)
+ .build();
+ new AwsCredentialsStorageIntegration(stsClient, configWithLegacyKmsKeys, EMPTY_REALM_CONFIG)
+ .getStorageAccessConfig(
Review Comment:
Should we assert that `assumeRole` was actually called?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3794227309
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Agreed! I follow the rule mentioned by @dimas-b. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5162:
URL: https://github.com/apache/polaris/pull/5162#issuecomment-5359480748
Hi folks, I think the PR is in a good sharp. I proposed to merge it by the end of this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3667476429
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
I followed the name proposed in #3338. @dimas-b, WDYT about renaming it to `decryptOnlyKmsKeys`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3671130879
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -64,14 +64,18 @@ public String getFileIoImplClassName() {
@Nullable
public abstract String getRoleARN();
- /** KMS Key ARN for server-side encryption,used for writes, optional */
+ /** Deprecated KMS Key ARN, retained for compatibility and treated as an allowed KMS key. */
@Nullable
Review Comment:
added!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3806775315
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -148,8 +149,8 @@ private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties)
.setRoleArn(awsConfig.getRoleARN())
.setExternalId(awsConfig.getExternalId())
.setUserArn(awsConfig.getUserARN())
- .setCurrentKmsKey(awsConfig.getCurrentKmsKey())
- .setAllowedKmsKeys(awsConfig.getAllowedKmsKeys())
+ .setEncryptionKeys(awsConfig.getEffectiveEncryptionKeys())
Review Comment:
This stops populating `currentKmsKey` and `allowedKmsKeys`, but both are still in the API response, deprecated, not removed. `CatalogEntityTest` pins the result: after a round trip `getCurrentKmsKey()` is null and `getAllowedKmsKeys()` is empty, so a client that still reads `allowedKmsKeys` off a GET sees no keys at all even though they're configured.
Can you mirror the effective encryption keys back into the deprecated fields here? If the empty response is intentional, put a line about it under Upgrade notes in the CHANGELOG.
##########
spec/polaris-management-service.yml:
##########
@@ -1142,14 +1142,28 @@ components:
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"
+ deprecated: true
+ description: Deprecated. Use encryptionKeys instead. The AWS KMS key ARN used to encrypt and decrypt S3 data.
+ example: "arn:aws:kms:us-east-1:123456789001:key/12345678-1234-1234-1234-123456789012"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ deprecated: true
+ description: Deprecated. Use encryptionKeys instead. The list of KMS keys that this catalog and its clients may use to encrypt and decrypt S3 data.
items:
type: string
- example: ["arn:aws:kms::123456789001:key/01234578"]
+ example: ["arn:aws:kms:us-east-1:123456789001:key/12345678-1234-1234-1234-123456789012"]
+ encryptionKeys:
+ type: array
+ description: The list of KMS keys that this catalog and its clients may use to encrypt S3 data. These keys are also granted decryption permissions.
Review Comment:
```suggestion
description: A list of KMS keys that can be used to encrypt S3 data. These keys are also granted decryption permissions.
```
##########
spec/polaris-management-service.yml:
##########
@@ -1142,14 +1142,28 @@ components:
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"
+ deprecated: true
+ description: Deprecated. Use encryptionKeys instead. The AWS KMS key ARN used to encrypt and decrypt S3 data.
+ example: "arn:aws:kms:us-east-1:123456789001:key/12345678-1234-1234-1234-123456789012"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ deprecated: true
+ description: Deprecated. Use encryptionKeys instead. The list of KMS keys that this catalog and its clients may use to encrypt and decrypt S3 data.
items:
type: string
- example: ["arn:aws:kms::123456789001:key/01234578"]
+ example: ["arn:aws:kms:us-east-1:123456789001:key/12345678-1234-1234-1234-123456789012"]
+ encryptionKeys:
+ type: array
+ description: The list of KMS keys that this catalog and its clients may use to encrypt S3 data. These keys are also granted decryption permissions.
+ items:
+ type: string
+ example: ["arn:aws:kms:us-east-1:123456789001:key/12345678-1234-1234-1234-123456789012"]
+ decryptionKeys:
+ type: array
+ description: The list of KMS keys that this catalog and its clients may use to decrypt S3 data. A key may also appear in encryptionKeys.
Review Comment:
```suggestion
description: A list of KMS keys that can be used to decrypt S3 data. Keeping this list separate from encryptionKeys is recommended, though overlapping keys are allowed.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3770075051
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -166,6 +182,40 @@ protected void check() {
throw new IllegalArgumentException("ARN does not match the expected role ARN pattern");
}
}
+
+ List<String> decryptOnlyKmsKeys = getDecryptOnlyKmsKeys();
+ if (decryptOnlyKmsKeys != null && !decryptOnlyKmsKeys.isEmpty()) {
+ String currentKmsKey = getCurrentKmsKey();
+ if (currentKmsKey != null) {
+ validateKmsKeyArn("currentKmsKey", currentKmsKey);
+ }
+
+ List<String> allowedKmsKeys = getAllowedKmsKeys();
+ validateKmsKeyArns("allowedKmsKeys", allowedKmsKeys);
Review Comment:
**Minor Concern:** the whole ARN-format validation block is gated on `decryptOnlyKmsKeys` being non-empty (lines 186-204). So `allowedKmsKeys` / `currentKmsKey` are normally not validated at all, but adding a single decrypt-only key retroactively applies the stricter rule to fields the operator never touched, and the error message points at `allowedKmsKeys`.
This isn't hypothetical: the spec's own examples were non-canonical (`arn:aws:kms::123456789001:key/01234578`) until this PR fixed them, so anyone who copied their config from the docs will hit it. Worth noting `@Value.Check` also runs on Jackson deserialization, so this can surface when loading an already-persisted catalog config, not just on create/update.
Can we either validate the encrypt-capable lists unconditionally (accepting the compat risk), or say in the error message why the rule suddenly applies?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3769369495
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
Thanks @iting0321. Did you push the change? The name is still `legacyKmsKeys`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3793509898
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,97 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testEffectiveAllowedKmsKeysIncludesDeprecatedCurrentKmsKey() {
+ assertThat(
+ newBuilder()
+ .currentKmsKey(ALLOWED_KMS_KEY_ARN)
+ .allowedKmsKeys(List.of(DECRYPT_ONLY_KMS_KEY_ARN))
+ .build()
+ .getEffectiveAllowedKmsKeys())
+ .containsExactly(DECRYPT_ONLY_KMS_KEY_ARN, ALLOWED_KMS_KEY_ARN);
+ }
+
+ @Test
+ public void testDecryptOnlyKmsKeysMustNotOverlapEncryptCapableKeys() {
+ String keyArn = "arn:aws:kms:us-east-1:012345678901:key/cccccccc-cccc-cccc-cccc-cccccccccccc";
+
+ assertThatThrownBy(
+ () -> newBuilder().currentKmsKey(keyArn).decryptOnlyKmsKeys(List.of(keyArn)).build())
Review Comment:
add @SuppressWarnings("deprecation")
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,97 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testEffectiveAllowedKmsKeysIncludesDeprecatedCurrentKmsKey() {
+ assertThat(
+ newBuilder()
+ .currentKmsKey(ALLOWED_KMS_KEY_ARN)
Review Comment:
add @SuppressWarnings("deprecation")
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
Review Comment:
I updated this test to use allowedKmsKeys.
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
Review Comment:
Agree, I replaced currentKmsKey with allowedKmsKeys.
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,97 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testEffectiveAllowedKmsKeysIncludesDeprecatedCurrentKmsKey() {
+ assertThat(
+ newBuilder()
+ .currentKmsKey(ALLOWED_KMS_KEY_ARN)
+ .allowedKmsKeys(List.of(DECRYPT_ONLY_KMS_KEY_ARN))
+ .build()
+ .getEffectiveAllowedKmsKeys())
+ .containsExactly(DECRYPT_ONLY_KMS_KEY_ARN, ALLOWED_KMS_KEY_ARN);
+ }
+
+ @Test
+ public void testDecryptOnlyKmsKeysMustNotOverlapEncryptCapableKeys() {
+ String keyArn = "arn:aws:kms:us-east-1:012345678901:key/cccccccc-cccc-cccc-cccc-cccccccccccc";
+
+ assertThatThrownBy(
+ () -> newBuilder().currentKmsKey(keyArn).decryptOnlyKmsKeys(List.of(keyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("currentKmsKey must not also be configured in decryptOnlyKmsKeys");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .allowedKmsKeys(List.of(keyArn))
+ .decryptOnlyKmsKeys(List.of(keyArn))
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("allowedKmsKeys and decryptOnlyKmsKeys must not overlap");
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("invalidKmsKeyArns")
+ public void testKmsKeysRejectNonCanonicalArnsWhenDecryptOnlyKmsKeysConfigured(
+ String description, String invalidKeyArn) {
+ assertThatThrownBy(() -> newBuilder().decryptOnlyKmsKeys(List.of(invalidKeyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining(
+ "When decryptOnlyKmsKeys is configured, decryptOnlyKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .allowedKmsKeys(List.of(invalidKeyArn))
+ .decryptOnlyKmsKeys(List.of(DECRYPT_ONLY_KMS_KEY_ARN))
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining(
+ "When decryptOnlyKmsKeys is configured, allowedKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .currentKmsKey(invalidKeyArn)
Review Comment:
add @SuppressWarnings("deprecation")
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3806122219
##########
site/content/blog/2025/12/24/aws-kms.md:
##########
@@ -35,7 +35,7 @@ This functionality will be available in the next release _after_ `1.3.0-incubati
KMS settings in Polaris are relevant to S3 buckets that have been configure to use KMS on the AWS side
(e.g. using SSE-KMS).
-Make a note of the KMS keys ARN that the bucket uses and pass it to the `--current-kms-key` CLI option
+Make a note of the KMS key ARN that the bucket uses and pass it to the `--encryption-key` CLI option
Review Comment:
I added myself in the blog and updated the date. Thank you โบ๏ธ.
##########
client/python/tests/test_parser_basic.py:
##########
@@ -174,7 +185,14 @@ def test_parse_argparse_valid_commands(self) -> None:
["--base-url", "https://customservice.com/subpath", "catalogs", "list"]
)
Parser.parse(
- ["--catalog-url", "http://localhost:8181/server1", "namespaces", "list", "--catalog", "polaris"]
+ [
+ "--catalog-url",
+ "http://localhost:8181/server1",
+ "namespaces",
+ "list",
+ "--catalog",
+ "polaris",
Review Comment:
I removed the unrelated formatting change.
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -323,6 +324,7 @@ private void validateDefaultBaseLocation() {
}
}
+ @SuppressWarnings("deprecation")
Review Comment:
I moved the deprecated S3-property handling into a dedicated helper and scoped the suppression to that method.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -429,82 +430,87 @@ private static IamPolicy policyString(
}
private static boolean addKmsKeyPolicy(
- String kmsKeyArn,
- List<String> allowedKmsKeys,
+ List<String> encryptionKeys,
+ List<String> decryptionKeys,
IamPolicy.Builder policyBuilder,
boolean canWrite,
String region,
String accountId) {
- boolean hasCurrentKey = kmsKeyArn != null;
- boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ boolean hasEncryptionKeys = hasKmsKeys(encryptionKeys);
+ boolean hasDecryptionKeys = hasKmsKeys(decryptionKeys);
boolean isAwsS3 = region != null && accountId != null;
// Nothing to do if no keys are configured and not AWS S3
- if (!hasCurrentKey && !hasAllowedKeys && !isAwsS3) {
+ if (!hasEncryptionKeys && !hasDecryptionKeys && !isAwsS3) {
return false;
}
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
-
- if (hasCurrentKey) {
- addKmsKeyResource(kmsKeyArn, allowKms);
+ boolean statementAdded = false;
+ if (hasEncryptionKeys && canWrite) {
+ IamStatement.Builder allowEncryption = buildKmsEncryptionStatement();
+ addKmsKeyResources(encryptionKeys, allowEncryption, "encryption");
+ policyBuilder.addStatement(allowEncryption.build());
+ statementAdded = true;
}
- if (hasAllowedKeys) {
- addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
+ LinkedHashSet<String> effectiveDecryptionKeys = new LinkedHashSet<>();
+ if (hasDecryptionKeys) {
+ effectiveDecryptionKeys.addAll(decryptionKeys);
+ }
+ if (hasEncryptionKeys) {
+ effectiveDecryptionKeys.addAll(encryptionKeys);
+ }
+ if (!effectiveDecryptionKeys.isEmpty()) {
+ IamStatement.Builder allowDecryption = buildKmsDecryptionStatement();
+ addKmsKeyResources(List.copyOf(effectiveDecryptionKeys), allowDecryption, "decryption");
+ policyBuilder.addStatement(allowDecryption.build());
+ statementAdded = true;
}
// Only add wildcard KMS access for read-only operations on AWS S3 when no specific keys are
// configured. This does not apply to services like Minio where region and accountId are not
// available.
- boolean shouldAddWildcard = !hasCurrentKey && !hasAllowedKeys && !canWrite && isAwsS3;
+ boolean shouldAddWildcard = !hasEncryptionKeys && !hasDecryptionKeys && !canWrite && isAwsS3;
if (shouldAddWildcard) {
+ IamStatement.Builder allowKms = buildKmsDecryptionStatement();
addAllKeysResource(region, accountId, allowKms);
- }
-
- if (hasCurrentKey || hasAllowedKeys || shouldAddWildcard) {
policyBuilder.addStatement(allowKms.build());
- return true;
+ statementAdded = true;
}
- return false;
+ return statementAdded;
}
- private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) {
- IamStatement.Builder allowKms =
- IamStatement.builder()
- .effect(IamEffect.ALLOW)
- .addAction("kms:DescribeKey")
- .addAction("kms:Decrypt");
-
- if (canEncrypt) {
- allowKms
- .addAction("kms:Encrypt")
- .addAction("kms:GenerateDataKey")
- .addAction("kms:GenerateDataKeyWithoutPlaintext");
- }
+ private static IamStatement.Builder buildKmsEncryptionStatement() {
+ return IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("kms:Encrypt")
Review Comment:
I combined the decryption actions into the encryption statement for write credentials.
The separate decryption statement now contains only keys not already covered by encryptionKeys, which avoids repeating long ARNs.
I think this approach keeps the implementation simpler.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3799604184
##########
client/python/apache_polaris/cli/command/catalogs.py:
##########
@@ -267,6 +271,8 @@ def _has_aws_storage_info(self) -> bool:
or self.sts_endpoint
or self.current_kms_key
Review Comment:
As `current_kms_key` and `allowed_kms_keys` are not deprecated via client/python/apache_polaris/cli/constants.py, should we add more verbose warning at runtime? For example,at https://github.com/iting0321/polaris/blob/7e1b13d697be5bd4853f0d2435e95feeb92f43a7/client/python/apache_polaris/cli/command/catalogs.py#L215, should we add waring msg if `current_kms_key` and `allowed_kms_keys` are set?
##########
client/python/tests/test_parser_basic.py:
##########
@@ -166,6 +166,17 @@ def test_parse_argparse_valid_commands(self) -> None:
Parser.parse(["catalogs", "create", "catalog_name"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "internal"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "INTERNAL"])
+ Parser.parse(
Review Comment:
These tests only ensure parser didn't fail not but test if parsed values are what we expected. Perhaps add tests around actual parsed values validation?
Also, it appears we lacks of tests when both `current-kms-key` and `allowed-kms-key` are set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3676355200
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
Let's wait for @dimas-b 's input. Thanks for your patience. Spec changes usually take a bit longer. We'd like to be careful to avoid any back-and-force, as they may introduce compatibility issues. Hope you understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3667466667
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -166,6 +170,19 @@ protected void check() {
throw new IllegalArgumentException("ARN does not match the expected role ARN pattern");
}
}
+
+ List<String> legacyKmsKeys = getLegacyKmsKeys();
+ if (legacyKmsKeys != null) {
+ String currentKmsKey = getCurrentKmsKey();
+ checkArgument(
+ currentKmsKey == null || !legacyKmsKeys.contains(currentKmsKey),
+ "currentKmsKey must not also be configured in legacyKmsKeys");
+
+ List<String> allowedKmsKeys = getAllowedKmsKeys();
+ checkArgument(
+ allowedKmsKeys == null || allowedKmsKeys.stream().noneMatch(legacyKmsKeys::contains),
Review Comment:
I agreed with this point, alias ARNs do not grant permissions to the underlying key, but wildcard resources such as `key/*` could bypass the exact-match overlap check and restore encryption access to a legacy key.
Iโll validate all configured values as concrete KMS key ARNs and reject aliases, wildcards, and non key ARN forms before checking overlap. Once inputs are canonical key ARNs, exact comparison is sufficient.
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
Review Comment:
fixed!
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
Review Comment:
Yes, exactly. This is only a documentation correction. allowedKmsKeys already receives encryption and data-key permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3780127232
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
> Note that even this RegEx does not guarantee that two equivalent ARNs (per storage's interpretation) may have different forms in Polaris config - the parts after key/ are not validated.
We need to harden the validation.
> If the concern is about strong "only" semantics in decryptOnlyKeys, I'd propose to use:
Not sure I follow the second point. Are you suggesting that we donโt need to guarantee that every key in `decryptOnlyKeys` is actually decrypt only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3777622508
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,97 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testEffectiveAllowedKmsKeysIncludesDeprecatedCurrentKmsKey() {
+ assertThat(
+ newBuilder()
+ .currentKmsKey(ALLOWED_KMS_KEY_ARN)
Review Comment:
Please suppress deprecation warnings from this expected reference to `currentKmsKey()`.
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
Review Comment:
This test is not specific to KMS key processing (`.kmsUnavailable(true)` on the next line)... Does it have to use the deprecated config?
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java:
##########
Review Comment:
Could we convert this test to use non-deprecated configs (main use cases)?
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,97 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testEffectiveAllowedKmsKeysIncludesDeprecatedCurrentKmsKey() {
+ assertThat(
+ newBuilder()
+ .currentKmsKey(ALLOWED_KMS_KEY_ARN)
+ .allowedKmsKeys(List.of(DECRYPT_ONLY_KMS_KEY_ARN))
+ .build()
+ .getEffectiveAllowedKmsKeys())
+ .containsExactly(DECRYPT_ONLY_KMS_KEY_ARN, ALLOWED_KMS_KEY_ARN);
+ }
+
+ @Test
+ public void testDecryptOnlyKmsKeysMustNotOverlapEncryptCapableKeys() {
+ String keyArn = "arn:aws:kms:us-east-1:012345678901:key/cccccccc-cccc-cccc-cccc-cccccccccccc";
+
+ assertThatThrownBy(
+ () -> newBuilder().currentKmsKey(keyArn).decryptOnlyKmsKeys(List.of(keyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("currentKmsKey must not also be configured in decryptOnlyKmsKeys");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .allowedKmsKeys(List.of(keyArn))
+ .decryptOnlyKmsKeys(List.of(keyArn))
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("allowedKmsKeys and decryptOnlyKmsKeys must not overlap");
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("invalidKmsKeyArns")
+ public void testKmsKeysRejectNonCanonicalArnsWhenDecryptOnlyKmsKeysConfigured(
+ String description, String invalidKeyArn) {
+ assertThatThrownBy(() -> newBuilder().decryptOnlyKmsKeys(List.of(invalidKeyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining(
+ "When decryptOnlyKmsKeys is configured, decryptOnlyKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .allowedKmsKeys(List.of(invalidKeyArn))
+ .decryptOnlyKmsKeys(List.of(DECRYPT_ONLY_KMS_KEY_ARN))
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining(
+ "When decryptOnlyKmsKeys is configured, allowedKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .currentKmsKey(invalidKeyArn)
Review Comment:
Please suppress deprecation warnings from this expected reference to currentKmsKey().
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
I'm still not comfortable adding new config validation in this PR, which aims to refactor properties that did not have this validation before.
Would you mind moving the new validation logic into a separate PR?
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,97 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testEffectiveAllowedKmsKeysIncludesDeprecatedCurrentKmsKey() {
+ assertThat(
+ newBuilder()
+ .currentKmsKey(ALLOWED_KMS_KEY_ARN)
+ .allowedKmsKeys(List.of(DECRYPT_ONLY_KMS_KEY_ARN))
+ .build()
+ .getEffectiveAllowedKmsKeys())
+ .containsExactly(DECRYPT_ONLY_KMS_KEY_ARN, ALLOWED_KMS_KEY_ARN);
+ }
+
+ @Test
+ public void testDecryptOnlyKmsKeysMustNotOverlapEncryptCapableKeys() {
+ String keyArn = "arn:aws:kms:us-east-1:012345678901:key/cccccccc-cccc-cccc-cccc-cccccccccccc";
+
+ assertThatThrownBy(
+ () -> newBuilder().currentKmsKey(keyArn).decryptOnlyKmsKeys(List.of(keyArn)).build())
Review Comment:
Please suppress deprecation warnings from this expected reference to currentKmsKey().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3806123355
##########
client/python/apache_polaris/cli/command/catalogs.py:
##########
@@ -267,6 +271,8 @@ def _has_aws_storage_info(self) -> bool:
or self.sts_endpoint
or self.current_kms_key
Review Comment:
Thank for your feedbacks. I added runtime warnings when `--current-kms-key` or `--allowed-kms-key` is supplied, directing users to `--encryption-key`.
##########
client/python/apache_polaris/cli/command/catalogs.py:
##########
@@ -267,6 +271,8 @@ def _has_aws_storage_info(self) -> bool:
or self.sts_endpoint
or self.current_kms_key
Review Comment:
Thank for your feedbacks. I added runtime warnings when `current-kms-key` or `allowed-kms-key` is supplied, directing users to `encryption-key`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3751870416
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
`decryptOnlyKmsKeys` SGTM... or perhaps `reeadOnlyKmsKeys`
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
`decryptOnlyKmsKeys` SGTM... or perhaps `readOnlyKmsKeys`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on PR #5162:
URL: https://github.com/apache/polaris/pull/5162#issuecomment-5107045963
> LGTM overall. Thanks @iting0321 for the PR. I think it moves in the right direction. It's a spec change and we usually post it in dev mailing list for visibility. Can you start a discussion for this?
Thanks for your feedback! This is the link for dev mailing list : https://lists.apache.org/thread/fb1xjn9wmp5m0gk1yk59hjhzsj6jl8nq
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3688328736
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -166,6 +170,19 @@ protected void check() {
throw new IllegalArgumentException("ARN does not match the expected role ARN pattern");
}
}
+
+ List<String> legacyKmsKeys = getLegacyKmsKeys();
+ if (legacyKmsKeys != null) {
+ String currentKmsKey = getCurrentKmsKey();
+ checkArgument(
+ currentKmsKey == null || !legacyKmsKeys.contains(currentKmsKey),
+ "currentKmsKey must not also be configured in legacyKmsKeys");
+
+ List<String> allowedKmsKeys = getAllowedKmsKeys();
+ checkArgument(
+ allowedKmsKeys == null || allowedKmsKeys.stream().noneMatch(legacyKmsKeys::contains),
Review Comment:
Hi @flyrain, I already changed in https://github.com/apache/polaris/pull/5162/commits/e0044f3ec7ab1a0de21187c437bb66b1ceb89ce6.
Thanks for your review!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3769407763
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
Sorry, I forgot to push the latest version(but I just pushed)๐จ. Thanks you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3780011191
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Format issues are specific to a particular Polaris admin and can be corrected. Strict pattern affects all users of non-AWS storage and require a code change to alter behaviour.
In any case, I'm not against extra validation. I'm just proposing not to mix it with the config refactoring.
Note that even this RegEx does not guarantee that two equivalent ARNs (per storage's interpretation) may have different forms in Polaris config - the parts after `key/` are not validated.
If the concern is about strong "only" semantics in `decryptOnlyKeys`, I'd propose to use:
* `allowedKmsKey` -> `encryptionKeys`
* Forward `currentKmsKey` to `encryptionKeys`
* Add `decryptionKeys` without guarantees that the same key may not appear in `encryptionKeys` - it's up to the admin user to control.
* Copy all `encryptionKeys` to `decryptionKeys` (with doc notices).
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3784375484
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
> Are you suggesting that we donโt need to guarantee that every key in decryptOnlyKeys is actually decrypt only?
I'm proposing to make the processing lighter and leaner and to use the config name of `decryptionKeys`. With that approach Polaris will not have to guarantee that `decryptionKeys` do not (also) have encrypt access and users are not likely to be confused about that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3787651044
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
Fine by me. I think this should meet the key rotation requirement. WDYT? @iting0321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3669660120
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -64,14 +64,18 @@ public String getFileIoImplClassName() {
@Nullable
public abstract String getRoleARN();
- /** KMS Key ARN for server-side encryption,used for writes, optional */
+ /** Deprecated KMS Key ARN, retained for compatibility and treated as an allowed KMS key. */
@Nullable
Review Comment:
Can we also clarify the version it was deprecated? In this case, it should be 1.8.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3667463449
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -64,14 +64,18 @@ public String getFileIoImplClassName() {
@Nullable
public abstract String getRoleARN();
- /** KMS Key ARN for server-side encryption,used for writes, optional */
+ /** Deprecated KMS Key ARN, retained for compatibility and treated as an allowed KMS key. */
@Nullable
Review Comment:
added, thanks you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "iting0321 (via GitHub)" <gi...@apache.org>.
iting0321 commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3768304503
##########
spec/polaris-management-service.yml:
##########
@@ -1142,11 +1142,18 @@ components:
example: "arn:aws:iam::123456789001:user/abc1-b-self1234"
currentKmsKey:
type: string
- description: the aws kms key arn used to encrypt s3 data
+ deprecated: true
+ description: Deprecated. Use allowedKmsKeys instead. The aws kms key arn used to encrypt and decrypt s3 data.
example: "arn:aws:kms::123456789001:key/01234578"
allowedKmsKeys:
type: array
- description: The list of kms keys that this catalog and its clients are allowed to use for reading s3 data
+ description: The list of kms keys that this catalog and its clients are allowed to use for encrypting and decrypting s3 data
+ items:
+ type: string
+ example: ["arn:aws:kms::123456789001:key/01234578"]
+ legacyKmsKeys:
Review Comment:
I renamed to decryptOnlyKmsKeys because it matches the generated IAM permissions: `kms:Decrypt` and
`kms:DescribeKey`.
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java:
##########
@@ -138,6 +145,79 @@ public void testKmsUnavailable() {
assertThat(newBuilder().kmsUnavailable(true).build().getKmsUnavailable()).isTrue();
}
+ @Test
+ public void testLegacyKmsKeysMustNotOverlapEncryptCapableKeys() {
+ String keyArn = "arn:aws:kms:us-east-1:012345678901:key/cccccccc-cccc-cccc-cccc-cccccccccccc";
+
+ assertThatThrownBy(
+ () -> newBuilder().currentKmsKey(keyArn).legacyKmsKeys(List.of(keyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("currentKmsKey must not also be configured in legacyKmsKeys");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder().allowedKmsKeys(List.of(keyArn)).legacyKmsKeys(List.of(keyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("allowedKmsKeys and legacyKmsKeys must not overlap");
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("invalidKmsKeyArns")
+ public void testLegacyKmsKeysRejectNonCanonicalKeyArns(String description, String invalidKeyArn) {
+ assertThatThrownBy(() -> newBuilder().legacyKmsKeys(List.of(invalidKeyArn)).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("legacyKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .allowedKmsKeys(List.of(invalidKeyArn))
+ .legacyKmsKeys(List.of(LEGACY_KMS_KEY_ARN))
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("allowedKmsKeys must contain concrete AWS KMS key ARNs");
+
+ assertThatThrownBy(
+ () ->
+ newBuilder()
+ .currentKmsKey(invalidKeyArn)
Review Comment:
`decryptOnlyKmsKeys` is already covered by the first assertion. This assertion specifically covers the deprecated but still supported currentKmsKey path. So I think we should retain this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3770271694
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -150,6 +150,7 @@ private StorageConfigInfo getStorageInfo(Map<String, String> internalProperties)
.setUserArn(awsConfig.getUserARN())
.setCurrentKmsKey(awsConfig.getCurrentKmsKey())
Review Comment:
Do we have to produce deprecated properties in responses to clients?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3797565286
##########
site/content/blog/2025/12/24/aws-kms.md:
##########
@@ -35,7 +35,7 @@ This functionality will be available in the next release _after_ `1.3.0-incubati
KMS settings in Polaris are relevant to S3 buckets that have been configure to use KMS on the AWS side
(e.g. using SSE-KMS).
-Make a note of the KMS keys ARN that the bucket uses and pass it to the `--current-kms-key` CLI option
+Make a note of the KMS key ARN that the bucket uses and pass it to the `--encryption-key` CLI option
Review Comment:
I'm not sure we need to change old blog posts. Those are works of their respective authors. Even if they become outdated, it's not a problem from my POV. They will remain relevant to the respective Polaris version as of the publication date and users can always refer to up-to-date information in the main docs section.
Since I happen to be the blog author in this case, I'm ok with this change ๐
@iting0321 : you may want to add yourself as a co-author and update the blog date in the header.
##########
client/python/tests/test_parser_basic.py:
##########
@@ -166,6 +166,17 @@ def test_parse_argparse_valid_commands(self) -> None:
Parser.parse(["catalogs", "create", "catalog_name"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "internal"])
Parser.parse(["catalogs", "create", "catalog_name", "--type", "INTERNAL"])
+ Parser.parse(
+ [
+ "catalogs",
+ "create",
+ "catalog_name",
+ "--decryption-key",
+ "arn:aws:kms:us-east-1:012345678901:key/decryption-key",
Review Comment:
Maybe add a test for multiple keys?
##########
client/python/tests/test_parser_basic.py:
##########
@@ -174,7 +185,14 @@ def test_parse_argparse_valid_commands(self) -> None:
["--base-url", "https://customservice.com/subpath", "catalogs", "list"]
)
Parser.parse(
- ["--catalog-url", "http://localhost:8181/server1", "namespaces", "list", "--catalog", "polaris"]
+ [
+ "--catalog-url",
+ "http://localhost:8181/server1",
+ "namespaces",
+ "list",
+ "--catalog",
+ "polaris",
Review Comment:
Spurious change (unnecessary trailing comma)?
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -323,6 +324,7 @@ private void validateDefaultBaseLocation() {
}
}
+ @SuppressWarnings("deprecation")
Review Comment:
Could this suppression be specific only to S3 config?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -429,82 +430,87 @@ private static IamPolicy policyString(
}
private static boolean addKmsKeyPolicy(
- String kmsKeyArn,
- List<String> allowedKmsKeys,
+ List<String> encryptionKeys,
+ List<String> decryptionKeys,
IamPolicy.Builder policyBuilder,
boolean canWrite,
String region,
String accountId) {
- boolean hasCurrentKey = kmsKeyArn != null;
- boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ boolean hasEncryptionKeys = hasKmsKeys(encryptionKeys);
+ boolean hasDecryptionKeys = hasKmsKeys(decryptionKeys);
boolean isAwsS3 = region != null && accountId != null;
// Nothing to do if no keys are configured and not AWS S3
- if (!hasCurrentKey && !hasAllowedKeys && !isAwsS3) {
+ if (!hasEncryptionKeys && !hasDecryptionKeys && !isAwsS3) {
return false;
}
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
-
- if (hasCurrentKey) {
- addKmsKeyResource(kmsKeyArn, allowKms);
+ boolean statementAdded = false;
+ if (hasEncryptionKeys && canWrite) {
+ IamStatement.Builder allowEncryption = buildKmsEncryptionStatement();
+ addKmsKeyResources(encryptionKeys, allowEncryption, "encryption");
+ policyBuilder.addStatement(allowEncryption.build());
+ statementAdded = true;
}
- if (hasAllowedKeys) {
- addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
+ LinkedHashSet<String> effectiveDecryptionKeys = new LinkedHashSet<>();
+ if (hasDecryptionKeys) {
+ effectiveDecryptionKeys.addAll(decryptionKeys);
+ }
+ if (hasEncryptionKeys) {
+ effectiveDecryptionKeys.addAll(encryptionKeys);
+ }
+ if (!effectiveDecryptionKeys.isEmpty()) {
+ IamStatement.Builder allowDecryption = buildKmsDecryptionStatement();
+ addKmsKeyResources(List.copyOf(effectiveDecryptionKeys), allowDecryption, "decryption");
+ policyBuilder.addStatement(allowDecryption.build());
+ statementAdded = true;
}
// Only add wildcard KMS access for read-only operations on AWS S3 when no specific keys are
// configured. This does not apply to services like Minio where region and accountId are not
// available.
- boolean shouldAddWildcard = !hasCurrentKey && !hasAllowedKeys && !canWrite && isAwsS3;
+ boolean shouldAddWildcard = !hasEncryptionKeys && !hasDecryptionKeys && !canWrite && isAwsS3;
if (shouldAddWildcard) {
+ IamStatement.Builder allowKms = buildKmsDecryptionStatement();
addAllKeysResource(region, accountId, allowKms);
- }
-
- if (hasCurrentKey || hasAllowedKeys || shouldAddWildcard) {
policyBuilder.addStatement(allowKms.build());
- return true;
+ statementAdded = true;
}
- return false;
+ return statementAdded;
}
- private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) {
- IamStatement.Builder allowKms =
- IamStatement.builder()
- .effect(IamEffect.ALLOW)
- .addAction("kms:DescribeKey")
- .addAction("kms:Decrypt");
-
- if (canEncrypt) {
- allowKms
- .addAction("kms:Encrypt")
- .addAction("kms:GenerateDataKey")
- .addAction("kms:GenerateDataKeyWithoutPlaintext");
- }
+ private static IamStatement.Builder buildKmsEncryptionStatement() {
+ return IamStatement.builder()
+ .effect(IamEffect.ALLOW)
+ .addAction("kms:Encrypt")
Review Comment:
Since we also grant `Decrypt` to all encryption keys, is it not more efficient to add decryption grant statements to the encryption statement? I suppose for multiple ARNs, repeating the ARN text will take more space that repeating the grants text... WDYT?
Alternatively we could try constructing both policies (repeated ARNs and repeated grants) and use the shortest version. WDYT?
It is not a huge concern, but running out of max policy size is a frequent error in "real life" deployments ๐ค
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #5162:
URL: https://github.com/apache/polaris/pull/5162
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add decrypt only access for legacy AWS KMS keys [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5162:
URL: https://github.com/apache/polaris/pull/5162#discussion_r3780127232
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -50,6 +50,13 @@ public static ImmutableAwsStorageConfigurationInfo.Builder builder() {
@JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):.+:.*:(.*):.+$";
private static final Pattern ROLE_ARN_PATTERN_COMPILED = Pattern.compile(ROLE_ARN_PATTERN);
+ private static final Pattern KMS_KEY_ARN_PATTERN =
+ Pattern.compile(
+ "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:[0-9]{12}:key/"
Review Comment:
> Note that even this RegEx does not guarantee that two equivalent ARNs (per storage's interpretation) may have different forms in Polaris config - the parts after key/ are not validated.
We need to harden the validation if so.
> If the concern is about strong "only" semantics in decryptOnlyKeys, I'd propose to use:
Not sure I follow the second point. Are you suggesting that we donโt need to guarantee that every key in `decryptOnlyKeys` is actually decrypt only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org