You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2026/01/15 02:01:04 UTC
[PR] (fix) Remove KMS policies when KMS is not configured [polaris]
MonkeyCanCode opened a new pull request, #3445:
URL: https://github.com/apache/polaris/pull/3445
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
This PR addressed issues reported in https://github.com/apache/polaris/issues/3440 where when end-user is not using KMS encryption for S3, Polaris still enforces in-lines policies which contains KMS related policies. While fixing this issue, I noticed our read-only policy for kMS is a bit too wide where `GenerateDataKey` and `GenerateDataKeyWithoutPlaintext` should be belongs to write operation instead of read. Thus, this PR also addresses this issue.
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2706348762
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1409,6 +1409,22 @@ public void doRefresh() {
Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
return TableMetadataParser.read(fileIO, metadataLocation);
});
+
+ // After a refresh, re-load the FileIO with the new table metadata properties to
+ // ensure the right permissions are present for subsequent file system interactions.
+ if (currentMetadata != null) {
+ tableFileIO =
+ loadFileIOForTableLike(
Review Comment:
How is that related to KMS policies?
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2709611197
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1409,6 +1409,22 @@ public void doRefresh() {
Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
return TableMetadataParser.read(fileIO, metadataLocation);
});
+
+ // After a refresh, re-load the FileIO with the new table metadata properties to
+ // ensure the right permissions are present for subsequent file system interactions.
+ if (currentMetadata != null) {
+ tableFileIO =
+ loadFileIOForTableLike(
Review Comment:
sounds good - thx!
--
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] (WIP) Remove KMS policies when KMS is not configured [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #3445:
URL: https://github.com/apache/polaris/pull/3445#issuecomment-3775976082
Close this one and opened three different PRs for easier review:
1. https://github.com/apache/polaris/pull/3493
2. https://github.com/apache/polaris/pull/3494
3. https://github.com/apache/polaris/pull/3496
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2692700149
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -297,42 +297,42 @@ private static void addKmsKeyPolicy(
String region,
String accountId) {
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
boolean hasCurrentKey = kmsKeyArn != null;
boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ if (!hasCurrentKey && !hasAllowedKeys) {
+ // No KMS keys configured, do not add any KMS-related policies.
+ return;
+ }
+
+ IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
+
if (hasCurrentKey) {
addKmsKeyResource(kmsKeyArn, allowKms);
}
if (hasAllowedKeys) {
addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
- }
-
- // Add KMS statement if we have any KMS key configuration
- if (hasCurrentKey || hasAllowedKeys) {
- policyBuilder.addStatement(allowKms.build());
- } else if (!canWrite) {
+ } else if (!canWrite && region != null && accountId != null) {
Review Comment:
It looks like this `else` condition will now be evaluated when `hasCurrentKey == true` and `hasAllowedKeys == false`, but it was not evaluated in that case before ๐ค 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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2695686230
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -309,30 +316,31 @@ private static void addKmsKeyPolicy(
addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
}
- // Add KMS statement if we have any KMS key configuration
- if (hasCurrentKey || hasAllowedKeys) {
+ // 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());
- } else if (!canWrite) {
- // Only add wildcard KMS access for read-only operations when no specific keys are configured
- // this check is for minio because it doesn't have region or account id
- if (region != null && accountId != null) {
- addAllKeysResource(region, accountId, allowKms);
- policyBuilder.addStatement(allowKms.build());
- }
}
}
private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) {
IamStatement.Builder allowKms =
IamStatement.builder()
.effect(IamEffect.ALLOW)
- .addAction("kms:GenerateDataKeyWithoutPlaintext")
.addAction("kms:DescribeKey")
- .addAction("kms:Decrypt")
- .addAction("kms:GenerateDataKey");
+ .addAction("kms:Decrypt");
if (canEncrypt) {
- allowKms.addAction("kms:Encrypt");
+ allowKms
+ .addAction("kms:Encrypt")
+ .addAction("kms:GenerateDataKey")
Review Comment:
nit: this looks like a valid change to me, but it goes a bit beyond the immediate purpose of this PR as fixing policies when KMS is _not_ in use... Would you mind mentioning this change in the PR description?
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2693003202
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -297,42 +297,42 @@ private static void addKmsKeyPolicy(
String region,
String accountId) {
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
boolean hasCurrentKey = kmsKeyArn != null;
boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ if (!hasCurrentKey && !hasAllowedKeys) {
+ // No KMS keys configured, do not add any KMS-related policies.
+ return;
+ }
+
+ IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
+
if (hasCurrentKey) {
addKmsKeyResource(kmsKeyArn, allowKms);
}
if (hasAllowedKeys) {
addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
- }
-
- // Add KMS statement if we have any KMS key configuration
- if (hasCurrentKey || hasAllowedKeys) {
- policyBuilder.addStatement(allowKms.build());
- } else if (!canWrite) {
+ } else if (!canWrite && region != null && accountId != null) {
Review Comment:
Thanks for the quick review @dimas-b and that is a fair point which I missed. It appears this is really specific for handling s3 and s3-compatible and I am not sure if the above logic will break or not due to lack of infra to test. I had made the needed code changes to preserve this workflow. Please take another look when you get a chance.
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2706404717
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1409,6 +1409,22 @@ public void doRefresh() {
Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
return TableMetadataParser.read(fileIO, metadataLocation);
});
+
+ // After a refresh, re-load the FileIO with the new table metadata properties to
+ // ensure the right permissions are present for subsequent file system interactions.
+ if (currentMetadata != null) {
+ tableFileIO =
+ loadFileIOForTableLike(
Review Comment:
Appear to be a parallel bug. I am waiting for testing from reporter then I can split this into two PRs. Currently if we merged current PR, it will trigger diff error from reporter.
--
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] (WIP) Remove KMS policies when KMS is not configured [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode closed pull request #3445: (WIP) Remove KMS policies when KMS is not configured
URL: https://github.com/apache/polaris/pull/3445
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2695686230
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -309,30 +316,31 @@ private static void addKmsKeyPolicy(
addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
}
- // Add KMS statement if we have any KMS key configuration
- if (hasCurrentKey || hasAllowedKeys) {
+ // 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());
- } else if (!canWrite) {
- // Only add wildcard KMS access for read-only operations when no specific keys are configured
- // this check is for minio because it doesn't have region or account id
- if (region != null && accountId != null) {
- addAllKeysResource(region, accountId, allowKms);
- policyBuilder.addStatement(allowKms.build());
- }
}
}
private static IamStatement.Builder buildBaseKmsStatement(boolean canEncrypt) {
IamStatement.Builder allowKms =
IamStatement.builder()
.effect(IamEffect.ALLOW)
- .addAction("kms:GenerateDataKeyWithoutPlaintext")
.addAction("kms:DescribeKey")
- .addAction("kms:Decrypt")
- .addAction("kms:GenerateDataKey");
+ .addAction("kms:Decrypt");
if (canEncrypt) {
- allowKms.addAction("kms:Encrypt");
+ allowKms
+ .addAction("kms:Encrypt")
+ .addAction("kms:GenerateDataKey")
Review Comment:
nit: this looks like a valid change to me, but it goes a bit beyond the immediate purpose of this PR as fixing policies when KMS is _not_ in use... Would you mind mentioning this change in the PR description?
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2695696389
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -297,42 +297,42 @@ private static void addKmsKeyPolicy(
String region,
String accountId) {
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
boolean hasCurrentKey = kmsKeyArn != null;
boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ if (!hasCurrentKey && !hasAllowedKeys) {
+ // No KMS keys configured, do not add any KMS-related policies.
+ return;
+ }
+
+ IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
+
if (hasCurrentKey) {
addKmsKeyResource(kmsKeyArn, allowKms);
}
if (hasAllowedKeys) {
addAllowedKmsKeyResources(allowedKmsKeys, allowKms);
- }
-
- // Add KMS statement if we have any KMS key configuration
- if (hasCurrentKey || hasAllowedKeys) {
- policyBuilder.addStatement(allowKms.build());
- } else if (!canWrite) {
+ } else if (!canWrite && region != null && accountId != null) {
Review Comment:
thx - LGTM :+1:
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3445:
URL: https://github.com/apache/polaris/pull/3445#issuecomment-3756540047
CC: @fabio-rizzo-01
--
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] (fix) Remove KMS policies when KMS is not configured [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3445:
URL: https://github.com/apache/polaris/pull/3445#discussion_r2706350202
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1409,6 +1409,22 @@ public void doRefresh() {
Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
return TableMetadataParser.read(fileIO, metadataLocation);
});
+
+ // After a refresh, re-load the FileIO with the new table metadata properties to
+ // ensure the right permissions are present for subsequent file system interactions.
+ if (currentMetadata != null) {
+ tableFileIO =
+ loadFileIOForTableLike(
Review Comment:
If it's just a parallel bugfix, I'd prefer to make it in a separate PR for the sake of clarity ๐ค 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