-
Notifications
You must be signed in to change notification settings - Fork 340
Support Aws STS AssumeRoleWithWebIdentity #3170
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.
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
Changes from all commits
8940129
0acbb64
95e0566
2a78172
c9097f7
ef22353
5d0b0d5
a5e9d37
d39abc4
0215cac
44b039b
1849ab0
e6677d3
236f65b
ff0c342
c41a23e
b5362fb
8498ffb
cfe0851
9b9552e
380de92
f3eaf23
dcacc5c
ebee75c
e4618ca
0260b44
fa7b282
c340ab3
bd2c3f8
79fd424
ec4eb9a
9dc3aee
101fcf3
2e0a1b9
022f4da
5a93a5f
98d008e
1d2727c
7d21203
e549edd
f67f03d
9f7ccde
6100407
81df525
ddae40e
09f71fc
0776e59
3c1b8b5
82bc103
a053b23
0cb4607
6271bc9
165c461
465c4b5
9fc4d76
4c6b96f
d42eaf1
230ca71
44848f8
907234a
13f935a
f3e1e85
a68225c
ea6afe5
f3763e6
7ef2de0
d1fdb38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,8 @@ | |
| import software.amazon.awssdk.policybuilder.iam.IamStatement; | ||
| import software.amazon.awssdk.services.sts.StsClient; | ||
| import software.amazon.awssdk.services.sts.model.AssumeRoleRequest; | ||
| import software.amazon.awssdk.services.sts.model.AssumeRoleResponse; | ||
| import software.amazon.awssdk.services.sts.model.AssumeRoleWithWebIdentityRequest; | ||
| import software.amazon.awssdk.services.sts.model.Credentials; | ||
|
|
||
| /** Credential vendor that supports generating */ | ||
| public class AwsCredentialsStorageIntegration | ||
|
|
@@ -81,7 +82,8 @@ public StorageAccessConfig getSubscopedCreds( | |
| boolean allowListOperation, | ||
| @Nonnull Set<String> allowedReadLocations, | ||
| @Nonnull Set<String> allowedWriteLocations, | ||
| Optional<String> refreshCredentialsEndpoint) { | ||
| Optional<String> refreshCredentialsEndpoint, | ||
| Optional<String> token) { | ||
| int storageCredentialDurationSeconds = | ||
| realmConfig.getConfig(STORAGE_CREDENTIAL_DURATION_SECONDS); | ||
| AwsStorageConfigurationInfo storageConfig = config(); | ||
|
|
@@ -90,35 +92,58 @@ public StorageAccessConfig getSubscopedCreds( | |
| StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder(); | ||
|
|
||
| if (shouldUseSts(storageConfig)) { | ||
| AssumeRoleRequest.Builder request = | ||
| AssumeRoleRequest.builder() | ||
| .externalId(storageConfig.getExternalId()) | ||
| .roleArn(storageConfig.getRoleARN()) | ||
| .roleSessionName("PolarisAwsCredentialsStorageIntegration") | ||
| .policy( | ||
| policyString( | ||
| storageConfig, | ||
| allowListOperation, | ||
| allowedReadLocations, | ||
| allowedWriteLocations, | ||
| region, | ||
| accountId) | ||
| .toJson()) | ||
| .durationSeconds(storageCredentialDurationSeconds); | ||
| credentialsProvider.ifPresent( | ||
| cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp))); | ||
|
|
||
| @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.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()) | ||
| Credentials credentials; | ||
| if (Boolean.TRUE.equals(storageConfig.getPropagateApiUserIdentity())) { | ||
| AssumeRoleWithWebIdentityRequest.Builder request = | ||
| AssumeRoleWithWebIdentityRequest.builder() | ||
| .webIdentityToken( | ||
| token.orElseThrow( | ||
| () -> | ||
| new IllegalArgumentException( | ||
| "Token must be provided when PROPAGATE_API_USER_IDENTITY is true"))) | ||
| .roleArn(storageConfig.getRoleARN()) | ||
| .roleSessionName("PolarisAwsCredentialsStorageIntegration") | ||
| .policy( | ||
| policyString( | ||
| storageConfig, | ||
| allowListOperation, | ||
| allowedReadLocations, | ||
| allowedWriteLocations, | ||
| region, | ||
| accountId) | ||
| .toJson()) | ||
| .durationSeconds(storageCredentialDurationSeconds); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't see a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from what I gather with this flow, we don't need to provide a policy string. Seems that the STS reads the users token and provides you with all the credentials you have permission for. I'm not an expert in S3 or STS so I could be way off and feel free to correct me if I am, but from testing with our appliance this is the way it seems to be.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Access down-scoping (policy-based restrictions) makes more sense in the case when Polaris has its own "service" credential because we do not want to delegate more rights to the user than what is necessary for the API request. In this case the STS session is based in the unmodified API user identity. The user can gain the same access by talking to STS directly. The only reason for restricting access might be to enforce catalog-level location settings. However, that would be mostly about preventing accidental mistakes on the client side rather than a true security control. That said, for the sake of narrowing Polaris behaviour variations it might still be worth applying the same policy as in the "service" account case... just for ease of reasoning about how the system works. Taking this one step further, it might be worth controlling the policy application with yet another config property, if there are use cases when the extra policy is not desirable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, I'll test out the policy string on our storage before committing it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from my POV, it would be ok to do the policy change in a follow-up PR. Current change is already under a dedicated config, so existing clients and servers will not be not affected even if we merge "as is". |
||
|
|
||
| credentials = stsClient.assumeRoleWithWebIdentity(request.build()).credentials(); | ||
| } else { | ||
| AssumeRoleRequest.Builder request = | ||
| AssumeRoleRequest.builder() | ||
| .externalId(storageConfig.getExternalId()) | ||
| .roleArn(storageConfig.getRoleARN()) | ||
| .roleSessionName("PolarisAwsCredentialsStorageIntegration") | ||
| .policy( | ||
| policyString( | ||
| storageConfig, | ||
| allowListOperation, | ||
| allowedReadLocations, | ||
| allowedWriteLocations, | ||
| region, | ||
| accountId) | ||
| .toJson()) | ||
| .durationSeconds(storageCredentialDurationSeconds); | ||
| credentialsProvider.ifPresent( | ||
| cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp))); | ||
| credentials = stsClient.assumeRole(request.build()).credentials(); | ||
| } | ||
| accessConfig.put(StorageAccessProperty.AWS_KEY_ID, credentials.accessKeyId()); | ||
| accessConfig.put(StorageAccessProperty.AWS_SECRET_KEY, credentials.secretAccessKey()); | ||
| accessConfig.put(StorageAccessProperty.AWS_TOKEN, credentials.sessionToken()); | ||
| Optional.ofNullable(credentials.expiration()) | ||
| .ifPresent( | ||
| i -> { | ||
| accessConfig.put( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: WDYT about
Optional<String>here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the same to me, what's bad about using
@Nullable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a "nit" comment :)
Optionalis slightly preferable IMHO because it's an essential piece of information for making theStorageAccessConfig, but the content of the token is optional... really fine point :) I'm ok either way.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to hold this until you deal with
StorageCredentialCacheKey, then use whatever is more convenient on the cache side.