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 token refresh support to SSOCredentialProvider #1903

Merged
merged 22 commits into from
Nov 11, 2022

Conversation

isaiahvita
Copy link
Contributor

@isaiahvita isaiahvita commented Nov 2, 2022

This PR adds support for token refresh to the SSOCredentialProvider (by using the SSOTokenProvider) while maintaining support for legacy SSO configurations (i.e. SSO configurations that do not use an sso-session but specify sso fields directly in a profile section of a shared config file)

Testing procedure:
Followed testing instructions by SDKs feature owner @alextwoods which included:

  • running the sso login flow with the AWS CLI
  • using the access token, calling S3 List Buckets on the configured AWS account
  • when the access token expires, re-calling S3 List buckets and see it succeed with an updated expiration in the SSO cache

Additonally:
Fixes #1845
Fixes #1846

@isaiahvita isaiahvita requested a review from a team as a code owner November 2, 2022 22:34
@isaiahvita isaiahvita assigned isaiahvita and sbiscigl and unassigned isaiahvita Nov 2, 2022
@@ -3,6 +3,7 @@ package config
import (
"context"
"fmt"
"github.com/aws/aws-sdk-go-v2/service/ssooidc"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick: move sdk import with other sdk imports

Copy link
Contributor

Choose a reason for hiding this comment

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

goimports is a good tool to do this automatically for you. https://pkg.go.dev/golang.org/x/tools/cmd/goimports

Copy link
Contributor

@syall syall left a comment

Choose a reason for hiding this comment

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

Minor notes about the PR, add:

  • More context in the description
  • How the changes was tested (commands, etc.)
  • How changes will impact customers (only due to the possible break/fix)

Also, try to keep the same style of checking for the empty string throughout the PR.

if err := c.validateCredentialsConfig(profile); err != nil {
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove newline

if len(missing) > 0 {
return fmt.Errorf("profile %q is configured to use SSO but is missing required configuration: %s",
c.Profile, strings.Join(missing, ", "))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add back newline

@@ -45,6 +45,8 @@ type Options struct {
// If custom cached token filepath is used, the Provider's startUrl
// parameter will be ignored.
CachedTokenFilepath string

TokenClient CreateTokenAPIClient
Copy link
Contributor

Choose a reason for hiding this comment

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

CreateTokenAPIClient needs documentation comment

Comment on lines +186 to +190
cachedPath, err := ssocreds.StandardCachedTokenFilepath(sharedConfig.SSOSession.Name)
if err != nil {
return err
}
oidcClient := ssooidc.NewFromConfig(cfgCopy)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: put oidcClient := ssooidc.NewFromConfig(cfgCopy) at the top of this comment block to keep with other validation done for TokenClient

Suggested change
cachedPath, err := ssocreds.StandardCachedTokenFilepath(sharedConfig.SSOSession.Name)
if err != nil {
return err
}
oidcClient := ssooidc.NewFromConfig(cfgCopy)
oidcClient := ssooidc.NewFromConfig(cfgCopy)
cachedPath, err := ssocreds.StandardCachedTokenFilepath(sharedConfig.SSOSession.Name)
if err != nil {
return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm im a bit confused at this requested change.

this validation:

		cachedPath, err := ssocreds.StandardCachedTokenFilepath(sharedConfig.SSOSession.Name)
		if err != nil {
			return err
		}

isnt operating on any input related to oidcClient := ssooidc.NewFromConfig(cfgCopy)

additionally, the cached path conditional checks to see if the existing token is valid before creating an oidc client. if the existing token is not valid, then there is not reason to create an oidc client, so it should happen after

Comment on lines +181 to +184
var optFns []func(*ssocreds.SSOTokenProviderOptions)
if found {
optFns = append(optFns, ssoTokenProviderOptionsFn)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

optFns does not seem to be used anywhere in this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this should of bee used as input for ssoidc.NewFromConfig?

Copy link
Member

Choose a reason for hiding this comment

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

I believe these are function options to be passed to the token provider constructor function actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good catch. Because optFns are *ssocreds.SSOTokenProviderOptions, it cannot be passed into the ssooidc constructor and must be passed into the token provider constructor. This means that instead of adding a new field to the sso Options and passing the ssoidc client as an option into the sso constructor:

		options = append(options, func(o *ssocreds.Options) {
			o.TokenClient = oidcClient
			o.CachedTokenFilepath = cachedPath
		})

I must make the added field a *SSOTokenProvider and pass that as an option into the sso constructor
Ill push a new commit to this PR with that change.

if p.cachedTokenFilepath == "" {
cachedTokenFilepath, err := StandardCachedTokenFilepath(p.options.StartURL)
var accessToken *string
if p.options.TokenClient != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a comment about the new TokenClient workflow.

}
accessToken = &token.Value
} else {
if p.cachedTokenFilepath == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a comment about cachedTokenFilepath workflow.

config/resolve_bearer_token.go Show resolved Hide resolved
}

return nil
}

func getSSOSession(name string, sections ini.Sections, logger logging.Logger) (*SSOSession, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure this function that was removed is not referenced anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Since this is package private there is no concern for this being removed. If something else requires it within the package, then it wouldn't compile or pass tests.

@@ -1088,17 +1069,66 @@ func (c *SharedConfig) validateCredentialType() error {
len(c.CredentialProcess) != 0,
len(c.WebIdentityTokenFile) != 0,
) {
return fmt.Errorf("only one credential type may be specified per profile: source profile, credential source, credential process, web identity token, or sso")
return fmt.Errorf("only one credential type may be specified per profile: source profile, credential source, credential process, web identity token")
}

return nil
}

func (c *SharedConfig) validateSSOConfiguration() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

note: validation in general has become stricter for both the legacy and current format.

@@ -3,6 +3,7 @@ package config
import (
"context"
"fmt"
"github.com/aws/aws-sdk-go-v2/service/ssooidc"
Copy link
Contributor

Choose a reason for hiding this comment

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

goimports is a good tool to do this automatically for you. https://pkg.go.dev/golang.org/x/tools/cmd/goimports

config/resolve_bearer_token.go Show resolved Hide resolved
Comment on lines +181 to +184
var optFns []func(*ssocreds.SSOTokenProviderOptions)
if found {
optFns = append(optFns, ssoTokenProviderOptionsFn)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this should of bee used as input for ssoidc.NewFromConfig?

Comment on lines 1107 to 1111
missing = append(missing, fmt.Sprintf("%s", ssoRegionKey))
}

if len(c.SSOSession.SSOStartURL) == 0 {
missing = append(missing, fmt.Sprintf("%s", ssoStartURLKey))
Copy link
Contributor

Choose a reason for hiding this comment

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

are the fmt.Sprintf needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah right, its not. will remove

Comment on lines +181 to +184
var optFns []func(*ssocreds.SSOTokenProviderOptions)
if found {
optFns = append(optFns, ssoTokenProviderOptionsFn)
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe these are function options to be passed to the token provider constructor function actually.

}

return nil
}

func getSSOSession(name string, sections ini.Sections, logger logging.Logger) (*SSOSession, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Since this is package private there is no concern for this being removed. If something else requires it within the package, then it wouldn't compile or pass tests.

Comment on lines +1079 to +1092
if c.hasSSOTokenProviderConfiguration() {
err := c.validateSSOTokenProviderConfiguration()
if err != nil {
return err
}
return nil
}

if c.hasLegacySSOConfiguration() {
err := c.validateLegacySSOConfiguration()
if err != nil {
return err
}
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Can both exist within the same profile, and if so is there precedence? As that would change how you would want this validation to work.

Choose a reason for hiding this comment

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

Yes - both could exist in one profile (its unlikely, but someone who is migrating from legacy to new could have such a setup). If so - the new format should be used, and the sso_start_url and sso_region MUST match (ie, if there is a mismatch between the profile's sso_region and the sso-session's, then we should raise an error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skmcgrail yes both can exist in the same profile. i intentionally check to see if the new configuration is present and valid. if it is valid, then it should be used and if it is not valid, then it should error (even if the legacy configuration is present and valid). is there another scenario in mind you might be thinking about thats not covered here?

also, regarding @alextwoods comment about matching sso_start_url and sso_region, i do that additional check here https://github.com/aws/aws-sdk-go-v2/pull/1903/files#diff-ad732dd8295a0ce04a495056007062e13a140b7f97f173004d0d6523545805d2R1120

Comment on lines 48 to 51

// Used by the SSOCredentialProvider if a token configuration
// profile is used in the shared config
TokenProvider *SSOTokenProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should prefix this member with SSO in case there are other kinds of TokenProvider in the future. CachedTokenFilepath probably should of been prefixed with SSO as well, but that ship has passed.

Copy link
Contributor Author

@isaiahvita isaiahvita Nov 10, 2022

Choose a reason for hiding this comment

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

yah i was debating with myself on this and then just defaulted to the existing style in that file. but i agree adding SSO makes sense and i just made the change with a new commit

@isaiahvita isaiahvita merged commit 3d79564 into main Nov 11, 2022
@isaiahvita isaiahvita deleted the isvita/token-refresh branch November 15, 2022 17:30
danielrbradley added a commit to pulumi/pulumi-aws-native that referenced this pull request Jul 3, 2023
Closes #954

This pulls in this fix to the config module: aws/aws-sdk-go-v2#1903
danielrbradley added a commit to pulumi/pulumi-aws-native that referenced this pull request Jul 6, 2023
Closes #954

This pulls in this fix to the config module: aws/aws-sdk-go-v2#1903
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.

Should not backfill session-name for SSO Token Provider Need to check for sso-session is present or not.
6 participants