Add decrypt only access for legacy AWS KMS keys by iting0321 · Pull Request #5162 · apache/polaris

iting0321 · 2026-07-26T10:30:57Z

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:

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:

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

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes Reduce permissions on historical KMS keys #3338
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

adutra · 2026-07-27T08:45:33Z


-  /** 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


Suggested change

@Nullable

@Deprecated

@Nullable

added, thanks you!

flyrain

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?

flyrain · 2026-07-27T22:46:20Z

+
+      List<String> allowedKmsKeys = getAllowedKmsKeys();
+      checkArgument(
+          allowedKmsKeys == null || allowedKmsKeys.stream().noneMatch(legacyKmsKeys::contains),


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?

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.

flyrain · 2026-07-27T23:39:36Z

              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.


nit: arn -> ARN, aws -> AWS

flyrain · 2026-07-28T00:18:14Z

            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


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.

polaris/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java

Lines 431 to 471 in 0d164ef

private static boolean addKmsKeyPolicy(

String kmsKeyArn,

List<String> allowedKmsKeys,

IamPolicy.Builder policyBuilder,

boolean canWrite,

String region,

String accountId) {

boolean hasCurrentKey = kmsKeyArn != null;

boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);

boolean isAwsS3 = region != null && accountId != null;

// Nothing to do if no keys are configured and not AWS S3

if (!hasCurrentKey && !hasAllowedKeys && !isAwsS3) {

return false;

}

IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);

if (hasCurrentKey) {

addKmsKeyResource(kmsKeyArn, allowKms);

}

if (hasAllowedKeys) {

addAllowedKmsKeyResources(allowedKmsKeys, allowKms);

}

// 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;

if (shouldAddWildcard) {

addAllKeysResource(region, accountId, allowKms);

}

if (hasCurrentKey || hasAllowedKeys || shouldAddWildcard) {

policyBuilder.addStatement(allowKms.build());

return true;

}

return false;

}

.

Yes, exactly. This is only a documentation correction. allowedKmsKeys already receives encryption and data-key permissions.

flyrain · 2026-07-28T00:20:51Z

+              items:
+                type: string
+              example: ["arn:aws:kms::123456789001:key/01234578"]
+            legacyKmsKeys:


Maybe we could give a more descriptive name, like decryptOnlyKmsKeys

I followed the name proposed in #3338. @dimas-b, WDYT about renaming it to decryptOnlyKmsKeys?

iting0321 · 2026-07-28T16:42:57Z

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

Copilot AI review requested due to automatic review settings July 26, 2026 10:30

github-project-automation Bot added this to Basic Kanban Board Jul 26, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 26, 2026

Copilot AI reviewed Jul 26, 2026

reduce permissions for legacy AWS KMS keys

d6d4c53

iting0321 force-pushed the fix/legacy-kms-key-permissions branch from 3ddb516 to d6d4c53 Compare July 26, 2026 11:47

iting0321 changed the title ~~Add decrypt-only access for legacy AWS KMS keys~~ Add decrypt only access for legacy AWS KMS keys Jul 26, 2026

fix format

e403444

adutra reviewed Jul 27, 2026

View reviewed changes

flyrain reviewed Jul 28, 2026

View reviewed changes

fix alias ARN problem

e0044f3

	private static boolean addKmsKeyPolicy(
	String kmsKeyArn,
	List<String> allowedKmsKeys,
	IamPolicy.Builder policyBuilder,
	boolean canWrite,
	String region,
	String accountId) {

	boolean hasCurrentKey = kmsKeyArn != null;
	boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
	boolean isAwsS3 = region != null && accountId != null;

	// Nothing to do if no keys are configured and not AWS S3
	if (!hasCurrentKey && !hasAllowedKeys && !isAwsS3) {
	return false;
	}

	IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);

	if (hasCurrentKey) {
	addKmsKeyResource(kmsKeyArn, allowKms);
	}

	if (hasAllowedKeys) {
	addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
	}

	// 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;
	if (shouldAddWildcard) {
	addAllKeysResource(region, accountId, allowKms);
	}

	if (hasCurrentKey \|\| hasAllowedKeys \|\| shouldAddWildcard) {
	policyBuilder.addStatement(allowKms.build());
	return true;
	}
	return false;
	}

Uh oh!

Conversation

iting0321 commented Jul 26, 2026

Summary

Changes

Key rotation workflow

Validation

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iting0321 commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants