Add sso token provider #4853

wty-Bryant · 2023-05-24T03:13:57Z

Add sso token provider to enable sso credential provider retrieving access token from it. Will resolve #4649 with following PRs on this branch

# Conflicts: # CHANGELOG_PENDING.md

…rom main branch

aajtodd

Overall looks good, some suggestions and few things to address.

Also this PR should be made to a feature branch not to main. Create a new branch from main and target that as the destination branch. You will then use that to create additional feature branches from until we are ready to merge the entire thing back to main

e.g.

git fetch
git checkout -b feat-sso-session origin/main
git push -u origin feat-sso-session

feat-sso-session is your "main" branch to work from now and target PRs to. As you merge feature branches into.

aajtodd · 2023-05-25T13:23:45Z

aws/credentials/ssocreds/bearer_token.go

+package ssocreds
+
+import (
+	"context"


fix: Use aws.Context in place of context.Context, the former is a shim for context.Context that works on older Go versions

aajtodd · 2023-05-25T13:29:25Z

aws/credentials/ssocreds/sso_cached_token.go

+	return nil
+}
+
+func loadCachedAccessToken(filename string) (cachedToken, error) {


Question Given this comes from gov2 is there any reason we changed the name from loadCachedToken to loadCachedAccessToken?

I will change it back. Previously I thought it is mainly used to retrieve access token from cached file.

aajtodd · 2023-05-25T13:32:11Z

aws/credentials/ssocreds/sso_cached_token.go

+		return err
+	}
+
+	parse, err := time.Parse(time.RFC3339, value)


fix: This duplicates the logic below, just call parseRFC3339

e.g.

*r, err = parseRFC3339(value) return err

aajtodd · 2023-05-25T13:37:52Z

aws/credentials/ssocreds/bearer_token.go

@@ -0,0 +1,49 @@
+package ssocreds


fix: This should either be renamed to remove the notion of bearer OR move it to a new package. I think I'd go with the latter (moving it) should we decide we want/need to implement bearer token auth support.

e.g.

aws/credentials/bearer token.go

aajtodd · 2023-05-25T13:43:05Z

aws/credentials/ssocreds/token_provider.go

+// create token.
+//
+// The SSOTokenProvider is not safe to use concurrently. The SDK's
+// config.LoadDefaultConfig will automatically wrap the SSOTokenProvider with


fix: This documentation needs to remove smithy-go and aws-sdk-go-v2 references (e.g. LoadDefaultConfig is not part of Go v1)

aajtodd · 2023-05-25T13:48:07Z

aws/credentials/ssocreds/token_provider.go

+	return token, nil
+}
+
+func ToTime(p *time.Time) (v time.Time) {


fix: No reason to be public

aajtodd · 2023-05-25T13:58:42Z

CHANGELOG_PENDING.md

@@ -3,3 +3,5 @@
 ### SDK Enhancements

 ### SDK Bugs
+* `ssocreds`: Add sso token provider logic.


There will only be one changelog entry for this entire body of work so this could be better.

e.g.

* `aws/credentials/ssocreds`: Implement SSO token provider and support for `sso-session` in AWS shared config. * Fixes [4649](https://github.com/aws/aws-sdk-go/issues/4649)

# Conflicts: # CHANGELOG_PENDING.md

isaiahvita · 2023-05-26T16:25:33Z

based on @aajtodd comments here

Also this PR should be made to a feature branch not to main. Create a new branch from main and target that as the destination branch. You will then use that to create additional feature branches from until we are ready to merge the entire thing back to main

Right now, the PR description sounds like you are solving the whole feature in this PR. is that the case?

based on what Aaron said, it sounds like you should have a feat-sso-session branch and you will continually make new PRs off of other branches feat-sso-session-pt1 to merge into feat-sso-session.

but it looks like you are creating one big PR and continually adding commits to it, which isnt the recommendation.

so please clarify what your PR strategy is here and update the PR description appropriately.

wty-Bryant · 2023-05-26T16:40:43Z

so please clarify what your PR strategy is here and update the PR description appropriately.

So the current PR branch only includes sso token provider code and its changelog entry, the last several commits just try to refresh the PR's check by changing a word in changelog content.

isaiahvita · 2023-05-26T16:18:58Z

aws/credentials/bearer/token.go

@@ -0,0 +1,49 @@
+package bearer


to echo one of @aajtodd comments earlier, its good that you removed this out of the ssocreds package. but i think this tokenprovider should be out of the credentials package completely.

it should be aws/bearer/token.go. this is because a token provider is not necessarily a "credentials" based auth, its its own thing. we just happen to be using it as a utility mechanism: use the token provider as a means of getting an access token that we then send to the SSO service to get aws credentials.

for example, StaticTokenProvider has no business being under the credentials package

isaiahvita · 2023-05-26T16:35:48Z

aws/credentials/ssocreds/sso_cached_token.go

+	"time"
+)
+
+var osUserHomeDur = shareddefaults.UserHomeDir


i get what youre doing here: you seem to be saving it out to a variable so you can inject over it in the test case. but id bias towards descriptive difference in naming rather than intentional misspelling. something like resolvedOsUserHomeDir or injectableOsUserHomeDir

aajtodd · 2023-05-31T12:54:14Z

aws/credentials/ssocreds/sso_cached_token_test.go

@@ -0,0 +1,191 @@
+//go:build go1.16


fix: Use the minimum version you need to here, I would think that is just 1.7 like the other files. Why is this so high?

1.7 still doesn't support testing fields like Name, I will change it to 1.9 which is the same as credential provider test

aajtodd · 2023-05-31T13:03:23Z

aws/bearer/token.go

@@ -0,0 +1,49 @@
+package bearer


Discuss: Is this where we want this to live? I can understand @isaiahvita point about it not being AWS Credentials (although if you take "credentials" more generically to mean any kind of credential then aws/credentials/bearer would be an ok package). This new package hierarchy doesn't make sense either though (aws/bearer). Perhaps a new top level auth package (auth/bearer)?

ah ok, i see were split on this, lets discuss thiss offline.

when i think of "credentials", i dont think of a "token" as a type of credentials, it is its own medium of authNZ: a token. but i do admit my concept of "credentials" is heavily influenced by aws' usage of the concept "credentials". but, if were thinking of "credentials" as the all-encompassing medium of authNZ, then that would work.

also, to clarify my preference was more for aws/token and not so much aws/bearer

additionally, if we look at this file
https://github.com/aws/aws-sdk-go/blob/main/aws/credentials/credentials.go#L74

the naming of Value seems to imply that its definition is what a "credentials" is. if it were to leave open the possibility of other types of credentials we would need a name for the credentials that is composed of "awsaccesskey,awssecretkey,awssessiontoken": so it would be something like AWSCredentials and TokenCredentials

Tianyi Wang added 10 commits May 23, 2023 17:56

Squash and Merge sso token provider code and unit test

8855191

Squash and Merge ssocred rfc3339 code location change

3bf12a6

Squash and Merge pending changelog content

9e666ea

Deprecate smithy go dependency and Merge bearer token code

e3644bf

Deprecate smithy go dependency and Merge bearer token code

22968a0

Deprecate go-cmp dependency

6354a34

Merge changed sso token provider error format

3e9df76

Merge branch 'main' into feature-token-provider

df54980

# Conflicts: # CHANGELOG_PENDING.md

Merge modified pending changelog

14a703c

Deleted sms smoke json model that is generated from unknown merging f…

6f16394

…rom main branch

aajtodd requested changes May 25, 2023

View reviewed changes

Tianyi Wang added 2 commits May 25, 2023 14:21

Modify and merge sso token provider logic

d0894cf

Merge branch 'feat-sso-session' into feature-token-provider

d5e1072

# Conflicts: # CHANGELOG_PENDING.md

wty-Bryant closed this May 25, 2023

wty-Bryant reopened this May 25, 2023

wty-Bryant changed the base branch from main to feat-sso-session May 25, 2023 18:48

Tianyi Wang added 3 commits May 25, 2023 14:53

Modify and Merge changelog entry

45be7cf

Merge branch 'feat-sso-session' into feature-token-provider

7b47ebb

# Conflicts: # CHANGELOG_PENDING.md

Modify and Merge changelog entry

e35aaee

wty-Bryant deleted the branch feat-sso-session May 25, 2023 19:18

wty-Bryant closed this May 25, 2023

wty-Bryant reopened this May 25, 2023

Modify and Merge changelog entry

3154ff0

wty-Bryant changed the base branch from feat-sso-session to main May 25, 2023 20:14

wty-Bryant changed the base branch from main to feat-sso-session May 25, 2023 20:15

Modify and Merge changelog entry

3c021ea

isaiahvita reviewed May 26, 2023

View reviewed changes

Modify and Merge bearer token directory and sso token provider var name

359ee1e

Modify and Merge changelog entry

19a523b

wty-Bryant closed this May 26, 2023

wty-Bryant deleted the feature-token-provider branch May 26, 2023 18:55

wty-Bryant restored the feature-token-provider branch May 26, 2023 19:00

wty-Bryant reopened this May 26, 2023

Modify and Merge changelog entry

da6f599

wty-Bryant changed the base branch from feat-sso-session to main May 26, 2023 19:28

wty-Bryant changed the base branch from main to feat-sso-session May 26, 2023 19:28

wty-Bryant and others added 5 commits May 26, 2023 15:28

Merge branch 'feat-sso-session' into feature-token-provider

8cf9168

Modify and Merge context used in token provider

8affb62

Modify and Merge ssocreds test tag

2ffc7e2

Modify and Merge ssocreds token provider test tag

6e4eb05

Modify and Merge ssocreds cached token test tag

b2ca51c

aajtodd requested changes May 31, 2023

View reviewed changes

Tianyi Wang added 4 commits May 31, 2023 11:08

Modify and Merge ssocreds cached token test tag

9b2a2a1

Modify and Merge ssocreds token provider test tag

7564841

Modify and Merge ssocreds token provider test tag

1558f1a

Modify and Merge bearer token directory

89c821a

aajtodd approved these changes May 31, 2023

View reviewed changes

wty-Bryant merged commit 1077388 into feat-sso-session May 31, 2023
31 checks passed

wty-Bryant mentioned this pull request May 31, 2023

Revert "Add sso token provider" #4867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sso token provider #4853

Add sso token provider #4853

wty-Bryant commented May 24, 2023

aajtodd left a comment

aajtodd May 25, 2023

aajtodd May 25, 2023

wty-Bryant May 25, 2023

aajtodd May 25, 2023

aajtodd May 25, 2023

aajtodd May 25, 2023

aajtodd May 25, 2023

aajtodd May 25, 2023

isaiahvita commented May 26, 2023 •

edited

wty-Bryant commented May 26, 2023 •

edited

isaiahvita May 26, 2023

isaiahvita May 26, 2023

aajtodd May 31, 2023

wty-Bryant May 31, 2023

aajtodd May 31, 2023

isaiahvita May 31, 2023 •

edited

isaiahvita May 31, 2023

Add sso token provider #4853

Add sso token provider #4853

Conversation

wty-Bryant commented May 24, 2023

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaiahvita commented May 26, 2023 • edited

wty-Bryant commented May 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaiahvita May 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaiahvita commented May 26, 2023 •

edited

wty-Bryant commented May 26, 2023 •

edited

isaiahvita May 31, 2023 •

edited