Skip to content
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

Add sso token provider #4853

Merged
merged 29 commits into from
May 31, 2023
Merged

Add sso token provider #4853

merged 29 commits into from
May 31, 2023

Conversation

wty-Bryant
Copy link
Contributor

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

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

package ssocreds

import (
"context"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return nil
}

func loadCachedAccessToken(filename string) (cachedToken, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return err
}

parse, err := time.Parse(time.RFC3339, value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: This duplicates the logic below, just call parseRFC3339

e.g.

*r, err = parseRFC3339(value)
return err

@@ -0,0 +1,49 @@
package ssocreds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return token, nil
}

func ToTime(p *time.Time) (v time.Time) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: No reason to be public

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

### SDK Bugs
* `ssocreds`: Add sso token provider logic.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@wty-Bryant wty-Bryant closed this May 25, 2023
@wty-Bryant wty-Bryant reopened this May 25, 2023
@wty-Bryant wty-Bryant changed the base branch from main to feat-sso-session May 25, 2023 18:48
@wty-Bryant wty-Bryant deleted the branch feat-sso-session May 25, 2023 19:18
@wty-Bryant wty-Bryant closed this May 25, 2023
@wty-Bryant wty-Bryant reopened this May 25, 2023
@wty-Bryant wty-Bryant changed the base branch from feat-sso-session to main May 25, 2023 20:14
@wty-Bryant wty-Bryant changed the base branch from main to feat-sso-session May 25, 2023 20:15
@isaiahvita
Copy link
Contributor

isaiahvita commented May 26, 2023

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
Copy link
Contributor Author

wty-Bryant commented May 26, 2023

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.

@@ -0,0 +1,49 @@
package bearer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"time"
)

var osUserHomeDur = shareddefaults.UserHomeDir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wty-Bryant wty-Bryant closed this May 26, 2023
@wty-Bryant wty-Bryant deleted the feature-token-provider branch May 26, 2023 18:55
@wty-Bryant wty-Bryant restored the feature-token-provider branch May 26, 2023 19:00
@wty-Bryant wty-Bryant reopened this May 26, 2023
@wty-Bryant wty-Bryant changed the base branch from feat-sso-session to main May 26, 2023 19:28
@wty-Bryant wty-Bryant changed the base branch from main to feat-sso-session May 26, 2023 19:28
@@ -0,0 +1,191 @@
//go:build go1.16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -0,0 +1,49 @@
package bearer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

@isaiahvita isaiahvita May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wty-Bryant wty-Bryant merged commit 1077388 into feat-sso-session May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-sdk-go doens't support new sso-session in a shared config
3 participants