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

delay WebIdentityRoleOptions validation #2030

Merged
merged 2 commits into from Mar 16, 2023
Merged

delay WebIdentityRoleOptions validation #2030

merged 2 commits into from Mar 16, 2023

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Feb 17, 2023

When loading the default config and AWS_WEB_IDENTITY_TOKEN_FILE is set it triggers the assume role with web identity provider. This provider requires it's configuration either entirely in environment variables or shared config. The Go SDK offers an API to configure this provider further via code which was being ignored or causing early validation issues. This change allows AWS_WEB_IDENTITY_TOKEN_FILE to be set and all other options to be supplied by Go code relaxing the constraint that everything has to come from the environment.

fixes #2015

For changes to files under the /codegen/aws-models folder, and manual edits to autogenerated code (e.g. /service/s3/api.go) please create an Issue instead of a PR for those type of changes.

If there is an existing bug or feature this PR is answers please reference it here.

To help speed up the process and reduce the time to merge please ensure that Allow edits and access to secrets by maintainers is checked before submitting your PR. This will allow the project maintainers to make minor adjustments or improvements to the submitted PR, allow us to reduce the roundtrip time for merging your request.

When loading the default config and `AWS_WEB_IDENTITY_TOKEN_FILE` is set it
triggers the assume role with web identity provider. This provider
requires it's configuration either entirely in environment variables or
shared config. The Go SDK offers an API to configure this provider
further via code which was being ignored or causing early validation
issues. This change allows `AWS_WEB_IDENTITY_TOKEN_FILE` to be set
and all other options to be supplied by Go code relaxing the constraint
that everything has to come from the environment.

fixes #2015
@aajtodd aajtodd requested a review from a team as a code owner February 17, 2023 20:53
Comment on lines 398 to 401
opts := stscreds.WebIdentityRoleOptions{
RoleARN: roleARN,
RoleSessionName: sessionName,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The opts value isn't passed to stscreds.NewWebIdentityRoleProvider, so the RoleSessionName set here is lost.

The original declaration of optFns should be restored, i.e.

optFns := []func(*stscreds.WebIdentityRoleOptions){
	func(options *stscreds.WebIdentityRoleOptions) {
		options.RoleSessionName = sessionName
	},
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I've restored the way RoleSessionName was being set.

@gdavison
Copy link
Contributor

gdavison commented Feb 27, 2023

HI @aajtodd, I've been doing some more testing. To clarify, when AWS_WEB_IDENTITY_TOKEN_FILE isn't set, should calling config.WithWebIdentityRoleCredentialOptions with RoleARN be sufficient to trigger assuming a web identity role, or does the stscreds.NewWebIdentityRoleProvider still have to be configured manually in that case?

@aajtodd
Copy link
Contributor Author

aajtodd commented Feb 28, 2023

@gdavison

HI @aajtodd, I've been doing some more testing. To clarify, when AWS_WEB_IDENTITY_TOKEN_FILE isn't set, should calling config.WithWebIdentityRoleCredentialOptions with RoleARN be sufficient to trigger assuming a web identity role, or does the stscreds.NewWebIdentityRoleProvider still have to be configured manually in that case?

No. The config.LoadDefaultConfig attempts to resolve a credential provider from what we refer to as the "default chain". This is a prioritized list of predefined (possible) credential providers with a fairly consistent behavior across AWS SDKs. The web identity credential provider is only chosen IFF no provider with a higher precedence is found first AND either AWS_WEB_IDENTITY_TOKEN_FILE or web_identity_token_file is set in the active profile/profile chain.

The config.WithWebIdentityRoleCredentialOptions API is confusing and honestly probably shouldn't exist. It is only consulted if the web identity provider is chosen and even then not all of the options are valid as you discovered (it likely should have defined a different options struct rather than re-use the existing one for stscreds.WebIdentityRoleProvider).

Since the API does exist though this PR attempts to make more of the options valid, in particular role ARN and explicit stscreds.WebIdentityRoleProvider client.


My 2 cents, if you know the provider explicitly you may not want to go through the default chain to get at it. Configure the credential provider explicitly if you know what it should be. This can even be your own custom defined chain. NOTE: If you configure your own credential provider you likely will want to wrap it with aws.NewCredentialsCache.

@gdavison
Copy link
Contributor

In our current setup, we have to do (among other setup), the following:

func getCredentialsProvider(ctx context.Context, c *Config) (aws.CredentialsProvider, string, error) {
  ...
  
	envConfig, err := config.NewEnvConfig()
  
  ...
  
  // Set up the WebIdentityRoleCredentialOptions here, in case the envvar `AWS_WEB_IDENTITY_TOKEN_FILE` is set
  // Otherwise, the AWS SDK for Go expects that there will be configured credentials and will return an error.
  if web := c.AssumeRoleWithWebIdentity; web != nil {
  	loadOptions = append(loadOptions, config.WithWebIdentityRoleCredentialOptions(func(opts *stscreds.WebIdentityRoleOptions) {
  		opts.RoleARN = web.RoleARN
  		opts.RoleSessionName = web.SessionName
  		opts.Duration = web.Duration
  		if web.Policy != "" {
  			opts.Policy = aws.String(web.Policy)
  		}
  		if len(web.PolicyARNs) > 0 {
  			opts.PolicyARNs = getPolicyDescriptorTypes(web.PolicyARNs)
  		}
  		if web.HasValidTokenSource() {
  			opts.TokenRetriever = web
  		}
  	}))
  }

  cfg, err := config.LoadDefaultConfig(ctx, loadOptions...)
  if err != nil {
  	return nil, "", fmt.Errorf("loading configuration: %w", err)
  }

  // If the envvar `AWS_WEB_IDENTITY_TOKEN_FILE` has not been set, and we want to assume a web role, we now have
  // to handle it manually. Creating the STS client for assuming the role needs a populated `aws.Config`, so we have
  // to do this after calling `config.LoadDefaultConfig`
  if c.AssumeRoleWithWebIdentity != nil && envConfig.WebIdentityTokenFilePath == "" {
  	provider, err := webIdentityCredentialsProvider(ctx, cfg, c)
  	if err != nil {
  		return nil, "", err
  	}
  	cfg.Credentials = provider
  }

  ...
}

func webIdentityCredentialsProvider(ctx context.Context, awsConfig aws.Config, c *Config) (aws.CredentialsProvider, error) {
	web := c.AssumeRoleWithWebIdentity
	client := stsClient(ctx, awsConfig, c)

	appCreds := stscreds.NewWebIdentityRoleProvider(client, web.RoleARN, web, func(opts *stscreds.WebIdentityRoleOptions) {
		opts.RoleSessionName = web.SessionName
		opts.Duration = web.Duration

		if web.Policy != "" {
			opts.Policy = aws.String(web.Policy)
		}

		if len(web.PolicyARNs) > 0 {
			opts.PolicyARNs = getPolicyDescriptorTypes(web.PolicyARNs)
		}
	})

	_, err := appCreds.Retrieve(ctx)
	if err != nil {
		return nil, c.NewCannotAssumeRoleWithWebIdentityError(err)
	}
	return aws.NewCredentialsCache(appCreds), nil
}

func stsClient(ctx context.Context, awsConfig aws.Config, c *Config) *sts.Client {
	return sts.NewFromConfig(awsConfig, func(opts *sts.Options) {
		if c.StsRegion != "" {
			opts.Region = c.StsRegion
		}
		if c.StsEndpoint != "" {
			opts.EndpointResolver = sts.EndpointResolverFromURL(c.StsEndpoint)
		}
	})
}

We need to resolve all of the configuration first so that we can get an aws.Config that we can pass to sts.NewFromConfig so that we can do a "manual" assume role with web credentials. It would be great if config.LoadDefaultConfig would see the web identity options and assume the role.

@aajtodd
Copy link
Contributor Author

aajtodd commented Mar 1, 2023

@gdavison

From the code provided:

If the envvar AWS_WEB_IDENTITY_TOKEN_FILE has not been set, and we want to assume a web role, we now have
// to handle it manually.

This is correct and expected.


It would be great if config.LoadDefaultConfig would see the web identity options and assume the role.

This is a misunderstanding of both how config.LoadDefaultConfig works and it's purpose. As it's name suggests it's attempting to load/resolve the SDK configuration by looking in the most common (default) places. Each of the credential providers it's looking for are used in specific/comomon deployment scenarios (e.g. local development, EC2, lambda, code build, EKS, etc). Once a valid credential source is found the search stops. All the options provided do is influence the web identity provider IFF it is the provider that is selected. Admittedly this is a confusing API (config.WithWebIdentityRoleCredentialOptions) and probably shouldn't exist.

Attempting to hook into this default behavior is understandable but also probably not optimal for your use case. It uses heuristics and a prioritized list to attempt to resolve configuration stopping at the first valid source. When you know the credential provider to choose already you'd be better off handling your own configuration with custom code as you are doing for the web identity provider in the code shared).


All that being said this PR enables more of the options from config.WithWebIdentityRoleCredentialOptions to be valid/merged with environment config including role ARN and explicitly set sts client (previously only the session name was actually used). It would not remove the need to explicitly configure a web identity provider if the environment processed by config.LoadDefaultConfig isn't configured for one.

@gdavison
Copy link
Contributor

gdavison commented Mar 15, 2023

Thanks, @aajtodd. Because of the purpose of config.LoadDefaultConfig, this meets our needs because it at least does not error out, and allows us to do our own handling with resolved configurations to get a valid STS client

@aajtodd aajtodd merged commit 601e6aa into main Mar 16, 2023
@aajtodd aajtodd deleted the fix-2015 branch March 16, 2023 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants