dimas-b commented on code in PR #2672:
URL: https://github.com/apache/polaris/pull/2672#discussion_r2383287183
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -79,43 +79,46 @@ public AccessConfig getSubscopedCreds(
int storageCredentialDurationSeconds =
realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS);
AwsStorageConfigurationInfo storageConfig = config();
- AssumeRoleRequest.Builder request =
- AssumeRoleRequest.builder()
- .externalId(storageConfig.getExternalId())
- .roleArn(storageConfig.getRoleARN())
- .roleSessionName("PolarisAwsCredentialsStorageIntegration")
- .policy(
- policyString(
- storageConfig.getAwsPartition(),
- allowListOperation,
- allowedReadLocations,
- allowedWriteLocations)
- .toJson())
- .durationSeconds(storageCredentialDurationSeconds);
- credentialsProvider.ifPresent(
- cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp)));
-
String region = storageConfig.getRegion();
- @SuppressWarnings("resource")
- // Note: stsClientProvider returns "thin" clients that do not need closing
- StsClient stsClient =
-
stsClientProvider.stsClient(StsDestination.of(storageConfig.getStsEndpointUri(),
region));
-
- AssumeRoleResponse response = stsClient.assumeRole(request.build());
AccessConfig.Builder accessConfig = AccessConfig.builder();
- accessConfig.put(StorageAccessProperty.AWS_KEY_ID,
response.credentials().accessKeyId());
- accessConfig.put(
- StorageAccessProperty.AWS_SECRET_KEY,
response.credentials().secretAccessKey());
- accessConfig.put(StorageAccessProperty.AWS_TOKEN,
response.credentials().sessionToken());
- Optional.ofNullable(response.credentials().expiration())
- .ifPresent(
- i -> {
- accessConfig.put(
- StorageAccessProperty.EXPIRATION_TIME,
String.valueOf(i.toEpochMilli()));
- accessConfig.put(
- StorageAccessProperty.AWS_SESSION_TOKEN_EXPIRES_AT_MS,
- String.valueOf(i.toEpochMilli()));
- });
+
+ if (storageConfig.shouldUseSts()) {
Review Comment:
Ideally yes, and that was in my first revision of this PR. Unfortunately
many existing use cases excessively request credential vending for `FILE`
storage and would be broken if we were to enforce this logic "in general".
We cannot enforce it here, because "storage integration" code has valid call
paths without credential vending, where it exposes non-credential config to
clients (e.g. S3 endpoints).
I'd like to enforce stricter checks as you suggested, but due to risk of
breaking existing clients (I saw that in our reg. tests), I propose to defer
it. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
