Skip to content

DO-NOT-MERGE (fix) Refined AWS vs non-AWS detection for S3-compatible storage #3496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MonkeyCanCode wants to merge 2 commits into from
Closed

DO-NOT-MERGE (fix) Refined AWS vs non-AWS detection for S3-compatible storage #3496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f36b8ae
Refined AWS vs non-AWS detection for S3-compatible storage
MonkeyCanCode Jan 21, 2026
43499bd
Refined AWS vs non-AWS detection for S3-compatible storage
MonkeyCanCode Jan 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
- (Before/After)UpdateTableEvent is emitted for all table updates within a transaction.
- Added KMS options to Polaris CLI
- Changed from Poetry to UV for Python package management
- Refined AWS vs non-AWS detection for S3-compatible storage.

### Deprecations

Expand Down
24 changes: 20 additions & 4 deletions ...e/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,23 @@ private IamPolicy policyString(
arnPrefix + StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/")));
});
policyBuilder.addStatement(allowPutObjectStatementBuilder.build());
addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, true, region, accountId);
addKmsKeyPolicy(
storageConfigurationInfo,
currentKmsKey,
allowedKmsKeys,
policyBuilder,
true,
region,
accountId);
} else {
addKmsKeyPolicy(currentKmsKey, allowedKmsKeys, policyBuilder, false, region, accountId);
addKmsKeyPolicy(
storageConfigurationInfo,
currentKmsKey,
allowedKmsKeys,
policyBuilder,
false,
region,
accountId);
}
if (!bucketListStatementBuilder.isEmpty()) {
bucketListStatementBuilder
Expand All @@ -292,6 +306,7 @@ private IamPolicy policyString(
}

private static void addKmsKeyPolicy(
AwsStorageConfigurationInfo storageConfigurationInfo,
String kmsKeyArn,
List<String> allowedKmsKeys,
IamPolicy.Builder policyBuilder,
Expand All @@ -302,6 +317,7 @@ private static void addKmsKeyPolicy(
IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
boolean hasCurrentKey = kmsKeyArn != null;
boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
boolean isAwsS3 = storageConfigurationInfo.isAwsS3();

if (hasCurrentKey) {
addKmsKeyResource(kmsKeyArn, allowKms);
Expand All @@ -316,8 +332,8 @@ private static void addKmsKeyPolicy(
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) {
// this check is for non-AWS S3-compatible storage as policy statements may not be compatible
if (isAwsS3) {
addAllKeysResource(region, accountId, allowKms);
policyBuilder.addStatement(allowKms.build());
}
Expand Down
7 changes: 7 additions & 0 deletions ...s-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@ public URI getStsEndpointUri() {
return getStsEndpoint() == null ? getInternalEndpointUri() : URI.create(getStsEndpoint());
}

@JsonIgnore
public boolean isAwsS3() {
String endpoint = getEndpoint();
// AWS S3 if no endpoint is specified or if it uses an amazonaws.com endpoint
return endpoint == null || endpoint.contains(".amazonaws.com");
Copy link
Contributor

@evindj evindj Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the endpoint be an Ip address rather than than a FQDN ?

Copy link
Contributor Author

@MonkeyCanCode MonkeyCanCode Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually AWS endpoint will be a pretty wide set of IPs and those IP can changed too as far as I know. I can't think about a reason on why we would ever want to pin a specific IP for using AWS endpoint as they all have geo routing already. But that is fair if somehow a user really wants to pined to a specific AWS IP address, this won't add wildcard KMS policy (as it will then get classified as non-AWS S3). But if user did specified KMS key on the catalog property, this will then work normally again with more detailed KMS policies.

Copy link
Contributor

@evindj evindj Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although this is generally fair, and will work for most of the cases, I was thinking if it would make sense to enable KMS addition as a configuration rather than something that is tightly coupled with whether or not it the underlying storage is AWS. Not a blocker.

Copy link
Member

@snazy snazy Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for .amazonaws.com works for the current public regions and zones. Technically, it would be better to check for the string only in the host part of the endpoint URI though. I do not know the endpoints of the relatively new AWS local regions/zones. Whether those are underneath the amazonaws.com domain. Amazon promised a "EU sovereign cloud", and I think to make it completely sovereign, the endpoints would be in a different DNS domain.

OTOH this check wouldn't work for localstack, which supports KMS, or any other KMS implementation.

I think, it would be better to gate this check on a different condition/setting rather than looking for .amazonaws.com in the whole endpoint URI.

}

@JsonIgnore
Copy link
Contributor

@evindj evindj Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not this change but is weird that the class is called AwsStorageConfigurationInfo and it implements a method with this name, I think eventually we want to refactor this class and may be call it S3StorageConfigurationInfo instead.

Copy link
Contributor

@evindj evindj Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also a question if we want a single class for all S3 compatible storage or if we want to have a model where there are many subclasses based on the actual storage backend we are dealing with.

Copy link
Contributor Author

@MonkeyCanCode MonkeyCanCode Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, refactor this maybe wise. Base on what I recalled, S3-compatible was added later on and it was added on top of the AWS one. Thus current state.

@Nullable
public String getAwsAccountId() {
Expand Down
24 changes: 24 additions & 0 deletions ...re/src/test/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,30 @@ public void testStsUnavailable() {
assertThat(newBuilder().stsUnavailable(true).build().getStsUnavailable()).isTrue();
}

@Test
public void testIsAwsS3() {
// Not defined, detail to AWS
assertThat(newBuilder().build().isAwsS3()).isTrue();
// Valid AWS S3 endpoints
assertThat(newBuilder().endpoint("https://s3.us-west-2.amazonaws.com").build().isAwsS3())
.isTrue();
assertThat(
newBuilder().endpoint("https://s3.dualstack.us-west-2.amazonaws.com").build().isAwsS3())
.isTrue();
assertThat(
newBuilder()
.endpoint("https://s3-fips.dualstack.us-west-2.amazonaws.com")
.build()
.isAwsS3())
.isTrue();
assertThat(newBuilder().endpoint("https://s3-fips.us-west-2.amazonaws.com").build().isAwsS3())
.isTrue();
// Custom S3-compatible endpoints
assertThat(newBuilder().endpoint("http://minio:9000").build().isAwsS3()).isFalse();
assertThat(newBuilder().endpoint("https://minio:9000").build().isAwsS3()).isFalse();
assertThat(newBuilder().endpoint("http://localhost:9000").build().isAwsS3()).isFalse();
}

@Test
public void testRoleArnParsing() {
AwsStorageConfigurationInfo awsConfig =
Expand Down