AWS: Use assumed-role credentials for REST SigV4 signing#16794
Conversation
GabrielBBaldez
force-pushed
the
aws-assume-role-rest-credentials
branch
from
c17cfe7 to
c64c9eb
Compare
RESTSigV4AuthSession signs requests with AwsProperties#restCredentialsProvider(), whose decision chain only handled static rest.* keys, a custom client.credentials-provider, and otherwise the default credential chain. When the catalog is configured with AssumeRoleAwsClientFactory, the assumed role is applied to the S3/Glue/KMS/DynamoDB clients but never to the SigV4 REST signing path, so REST calls silently fall back to the default credential chain and diverge from the other AWS clients. Add an assume-role branch between the custom-provider and default-chain steps that returns an StsAssumeRoleCredentialsProvider built from the existing client.assume-role.* properties, mirroring AssumeRoleAwsClientFactory#createCredentialsProvider.
GabrielBBaldez
force-pushed
the
aws-assume-role-rest-credentials
branch
from
c64c9eb to
2c13a21
Compare
| !Strings.isNullOrEmpty(this.clientAssumeRoleRegion), | ||
| "Cannot assume role %s to sign REST requests: %s is required", | ||
| this.clientAssumeRoleArn, | ||
| CLIENT_ASSUME_ROLE_REGION); |
There was a problem hiding this comment.
I think we should return here and fall back to DefaultCredentialsProvider. We can print out a warning message here
There was a problem hiding this comment.
Done in the latest push, thanks!
…ing for REST signing
NathanCYee
force-pushed
the
aws-assume-role-rest-credentials
branch
3 times, most recently
from
754a2ac to
0a331df
Compare
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Just took a pass, generally lgtm! left some feedbacks
| return DefaultCredentialsProvider.builder().build(); | ||
| } | ||
|
|
||
| protected StsAssumeRoleCredentialsProvider assumeRoleCredentialsProvider() { |
There was a problem hiding this comment.
This should be package private/no accessibility modifier?
StsAssumeRoleCredentialsProvider assumeRoleCredentialsProvider() {
There was a problem hiding this comment.
Changed to package private
| this.clientAssumeRoleSessionName = properties.get(CLIENT_ASSUME_ROLE_SESSION_NAME); | ||
| this.clientAssumeRoleSessionName = | ||
| properties.getOrDefault( | ||
| CLIENT_ASSUME_ROLE_SESSION_NAME, String.format("iceberg-aws-%s", UUID.randomUUID())); |
There was a problem hiding this comment.
I think we should still initialize this lazily. Auto-gen this is actually a behavior change.
We can check and generate this in createAssumeRoleRequest
There was a problem hiding this comment.
I reverted back to lazily generating the session name in the latest change, but I created a private UUID so that the randomized session name is stable across lazy generations for the lifetime of AwsProperties.
This matches the random uuid used in other places like BaseMetadataTable and the test for AliyunOSSExtension.
NathanCYee
force-pushed
the
aws-assume-role-rest-credentials
branch
2 times, most recently
from
0df785f to
ae521e6
Compare
|
Awesome! Thanks, @NathanCYee and @CTTY, for all the teamwork and collaboration. It was a pleasure working with you guys! |
NathanCYee
force-pushed
the
aws-assume-role-rest-credentials
branch
from
ae521e6 to
a744bea
Compare
… aws to refer to rest sigv4 catalog configuration
…andom session name
…ate. Update AssumeRoleAwsClientFactory to use shared function to reduce code duplication
NathanCYee
force-pushed
the
aws-assume-role-rest-credentials
branch
from
a744bea to
72aaa35
Compare
3 participants
What changed
RESTSigV4AuthSessionsigns catalog requests withAwsProperties#restCredentialsProvider(), whose decision chain only handled three cases:rest.*keys →StaticCredentialsProviderclient.credentials-provider→ that providerDefaultCredentialsProviderThere was no branch for
client.assume-role.*. So when the catalog is configured withAssumeRoleAwsClientFactory, the assumed role is applied to the S3/Glue/KMS/DynamoDB clients (viaapplyAssumeRoleConfigurations) but never to the SigV4 REST signing path — REST calls silently fall back to the default credential chain and diverge from the other AWS clients.This adds an assume-role branch between the custom-provider and default-chain steps that returns an
StsAssumeRoleCredentialsProviderbuilt from the existingclient.assume-role.*properties (arn, region, session name, timeout, external id, tags), mirroringAssumeRoleAwsClientFactory#createCredentialsProvider. Explicit staticrest.*keys andclient.credentials-providerkeep precedence.This is the "fastest path" described in the issue. Happy to instead extract the STS provider construction into a shared utility (option 3 in the issue) if reviewers prefer to avoid the small duplication with
AssumeRoleAwsClientFactory.Closes #16667
Testing
New unit tests in
TestAwsProperties:restCredentialsProvider()returns anStsAssumeRoleCredentialsProviderDefaultCredentialsProviderrest.*keys → still returnsStaticCredentialsProvider(precedence preserved)./gradlew :iceberg-aws:test :iceberg-aws:spotlessJavaCheck :iceberg-aws:checkstyleMain :iceberg-aws:checkstyleTestpasses locally.