Adds accountId to STS credentials providers #4040

cenedhryn · 2023-05-25T23:33:32Z

Motivation and Context

STS provides accountId in the ARN of the assumedRoleUser included in its response objects. This PR extracts that accountId, if present, and adds it to the credentials identity.

Resolving credentials end-to-end should work once credentials are resolved before the endpoint.

Modifications

Added accountId to AwsBasicCredentials and AwsSessionCredentials since the credentials providers use those classes.
Modified STS providers to find accountId in the response and add it to the returned credentials

Testing

Added codegen classes test to verify that the accountId is available to the endpoint resolution logic. Because we currently resolve the endpoint before we resolve credentials, this test is not working.

.../java/software/amazon/awssdk/services/sts/auth/StsGetFederationTokenCredentialsProvider.java

...ces/sts/src/main/java/software/amazon/awssdk/services/sts/auth/SessionCredentialsHolder.java

...ava/software/amazon/awssdk/services/sts/auth/StsWebIdentityTokenFileCredentialsProvider.java

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsBasicCredentials.java

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsSessionCredentials.java

...ces/sts/src/main/java/software/amazon/awssdk/services/sts/auth/SessionCredentialsHolder.java

pom.xml

millems · 2023-06-01T19:57:29Z

services/sts/src/main/java/software/amazon/awssdk/services/sts/auth/StsCredentialsProvider.java

@@ -123,7 +124,16 @@ public Duration prefetchTime() {
    /**
     * Implemented by a child class to call STS and get a new set of credentials to be used by this provider.
     */
-    protected abstract Credentials getUpdatedCredentials(StsClient stsClient);
+    protected abstract SessionCredentialsHolder getUpdatedCredentials(StsClient stsClient);


This is technically backwards-incompatible since this is a public class. That said, it was only recently made public so it's probably not a problem.

SessionCredentialsHolder isn't a public type, though, so we probably shouldn't expose it on a protected method of a public class. Is there a better way to get the credential information up to this type? Maybe just have the subtypes return AwsSessionCredentials directly? Add the optional expiration to AwsSessionCredentials as well?

I was a bit confused by the public part, because that change went out at the same time that this PR was created.
Changing signature to AwsSessionCredentials - note that the code does not use SessionCredentialsHolder anymore, the RefreshResult uses . If this change is approved, we can remove SessionCredentialsHolder from the public PR (the non-identity non-accountId changes should probably be punted to master asap before customers have more time to use the newly public class)

services/sts/src/main/java/software/amazon/awssdk/services/sts/auth/StsCredentialsProvider.java

gosar · 2023-06-01T19:58:47Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsBasicCredentials.java

     * This should be accessed via {@link AnonymousCredentialsProvider#resolveCredentials()}.
     */
    @SdkInternalApi
-    static final AwsBasicCredentials ANONYMOUS_CREDENTIALS = new AwsBasicCredentials(null, null, false);
+    static final AwsBasicCredentials ANONYMOUS_CREDENTIALS = builder().validateCredentials(Boolean.FALSE).build();


nit: s/Boolean.FALSE/false

gosar · 2023-06-01T19:59:48Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsBasicCredentials.java


-        if (validateCredentials) {
+        if (Boolean.TRUE.equals(builder.validateCredentials)) {


nit: can be if (builder.validateCredentials) { if it is changed to boolean

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsBasicCredentials.java

millems · 2023-06-01T23:02:31Z

...ces/sts/src/main/java/software/amazon/awssdk/services/sts/auth/SessionCredentialsHolder.java

@@ -27,22 +27,48 @@
 @SdkInternalApi
 @ThreadSafe
 final class SessionCredentialsHolder {


Do we even need this anymore, since the expiration is in the session credentials now?

It will be removed in subsequent PRs.

sonarqubecloud · 2023-06-02T00:54:06Z

SonarCloud Quality Gate failed.

4 Bugs
0 Vulnerabilities
0 Security Hotspots
123 Code Smells

65.3% Coverage
2.6% Duplication

gosar · 2023-06-02T08:10:23Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsBasicCredentials.java

+        /**
+         * Whether this class should verify that accessKeyId and secretAccessKey are not null.
+         */
+        Builder validateCredentials(Boolean validateCredentials) {


nit: I think this can be boolean too.

gosar · 2023-06-02T08:22:48Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/AwsSessionCredentials.java

+        return Optional.ofNullable(accountId);
+    }
+
+    public Optional<Instant> expiration() {


If this makes sense to add to this interface, why not add it to AwsSessionCredentialsIdentity and @Override here.

Oh, the top Identity interface already defines expirationTime(), which is what we should use.

Adds accountId to STS credentials providers

…5119) * Adds account ID as a built-in endpoint parameter (#4016) * Refactors credential identity with separate implementing classes (#4024) (#4028) * Adds accountId as a parameter to AWS credentials identity (#4029) * Adds accountId to STS credentials providers (#4040) * Adding accountId support to environment variable credential provider (#4327) * Adds accountId support to process credentials provider (#4332) * Adds account ID support for profile credentials provider sources (#4340) * Adds accountId endpoint mode (#4984) * Adding back existing endpoint files (#5120) * changelog (#5121)

…ws#5119) * Adds account ID as a built-in endpoint parameter (aws#4016) * Refactors credential identity with separate implementing classes (aws#4024) (aws#4028) * Adds accountId as a parameter to AWS credentials identity (aws#4029) * Adds accountId to STS credentials providers (aws#4040) * Adding accountId support to environment variable credential provider (aws#4327) * Adds accountId support to process credentials provider (aws#4332) * Adds account ID support for profile credentials provider sources (aws#4340) * Adds accountId endpoint mode (aws#4984) * Adding back existing endpoint files (aws#5120) * changelog (aws#5121)

cenedhryn requested a review from a team as a code owner May 25, 2023 23:33

Adds accountId to STS credentials providers

d9a4675

cenedhryn force-pushed the salande/accountid-add-sts branch from 540597a to d9a4675 Compare May 25, 2023 23:36

gosar reviewed May 26, 2023

View reviewed changes

millems reviewed May 26, 2023

View reviewed changes

cenedhryn added 2 commits May 26, 2023 16:58

changes to AwsBasicCredentials and minor STS updates

23a9f8b

removes expiration from SessionCredentialsHolder and minor fixes

c341bc7

millems reviewed Jun 1, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

millems reviewed Jun 1, 2023

View reviewed changes

gosar reviewed Jun 1, 2023

View reviewed changes

Changes due to StsCredentialsProvider becoming public

cfb4013

millems reviewed Jun 1, 2023

View reviewed changes

millems approved these changes Jun 1, 2023

View reviewed changes

gosar reviewed Jun 2, 2023

View reviewed changes

cenedhryn merged commit f7df929 into feature/master/accountid-endpoint-routing Jun 2, 2023

cenedhryn deleted the salande/accountid-add-sts branch June 2, 2023 15:36

cenedhryn added a commit that referenced this pull request Apr 17, 2024

Adds accountId to STS credentials providers (#4040)

b5512db

Adds accountId to STS credentials providers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adds accountId to STS credentials providers #4040

Adds accountId to STS credentials providers #4040

Uh oh!

cenedhryn commented May 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

millems Jun 1, 2023

Uh oh!

cenedhryn Jun 1, 2023

Uh oh!

Uh oh!

gosar Jun 1, 2023

Uh oh!

gosar Jun 1, 2023

Uh oh!

Uh oh!

millems Jun 1, 2023

Uh oh!

cenedhryn Jun 2, 2023

Uh oh!

sonarqubecloud bot commented Jun 2, 2023

Uh oh!

gosar Jun 2, 2023

Uh oh!

gosar Jun 2, 2023

Uh oh!

gosar Jun 2, 2023

Uh oh!

Uh oh!


		if (validateCredentials) {
		if (Boolean.TRUE.equals(builder.validateCredentials)) {

Adds accountId to STS credentials providers #4040

Adds accountId to STS credentials providers #4040

Uh oh!

Conversation

cenedhryn commented May 25, 2023

Motivation and Context

Modifications

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!