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

Configuring Assume Role With Web Identity doesn't allow directly setting parameters when setting AWS_WEB_IDENTITY_TOKEN_FILE #2015

Closed
gdavison opened this issue Feb 8, 2023 · 9 comments · Fixed by #2030
Assignees
Labels
bug This issue is a bug.

Comments

@gdavison
Copy link
Contributor

gdavison commented Feb 8, 2023

Describe the bug

When assuming a web identity role, we're not able to point to the token file using AWS_WEB_IDENTITY_TOKEN_FILE and set the role ARN and other parameters by using config.WithWebIdentityRoleCredentialOptions. For example, what we'd like to do is:

loadOptions = append(loadOptions, config.WithWebIdentityRoleCredentialOptionsFunc(opts *stscreds.WebIdentityRoleOptions) {
  opts.RoleARN = "<...>"
 })
// ...
cfg, err := config.LoadDefaultConfig(ctx, loadOptions...)

If configuration is set using only a combination of environment variables and/or named profiles, it works as intended.

Expected Behavior

All config sources, i.e. config.LoadOptions, config.EnvConfig, and config.SharedConfig should be used when resolving the web identity settings.

Current Behavior

Because AWS_WEB_IDENTITY_TOKEN_FILE is set, config.resolveCredentialChain assumes that the token file location and role ARN will be sourced from environment variables. This causes the checks in config.assumeWebIdentity to fail, since there is no value set for the file path.

If a dummy value is set in AWS_WEB_IDENTITY_TOKEN_FILE, the check in config.assumeWebIdentity will pass, and then the function passed to config.WithWebIdentityRoleCredentialOptionsFunc will be called and override the token retriever.

Reproduction Steps

Run the following with AWS_WEB_IDENTITY_TOKEN_FILE and opts.RoleARN set

package main

import (
	"context"
	"log"

	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/credentials/stscreds"
)

func main() {
	ctx := context.Background()

	var loadOptions []func(*config.LoadOptions) error

	loadOptions = append(loadOptions,
		config.WithWebIdentityRoleCredentialOptions(func(opts *stscreds.WebIdentityRoleOptions) {
			opts.RoleARN = "..."
		}),
	)

	_, err := config.LoadDefaultConfig(ctx, loadOptions...)
	if err != nil {
		log.Fatalf("failed: %s", err)
	}
}

Possible Solution

In config.assumeWebIdentity, validation of filepath and roleARN should either be deferred or should apply values from all config sources before validating.

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

go 1.19

require (
github.com/aws/aws-sdk-go-v2/config v1.18.12
github.com/aws/aws-sdk-go-v2/credentials v1.13.12
)

require (
github.com/aws/aws-sdk-go-v2 v1.17.4 // indirect
github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.12.22 // indirect
github.com/aws/aws-sdk-go-v2/internal/configsources v1.1.28 // indirect
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.4.22 // indirect
github.com/aws/aws-sdk-go-v2/internal/ini v1.3.29 // indirect
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.9.22 // indirect
github.com/aws/aws-sdk-go-v2/service/sso v1.12.1 // indirect
github.com/aws/aws-sdk-go-v2/service/ssooidc v1.14.1 // indirect
github.com/aws/aws-sdk-go-v2/service/sts v1.18.3 // indirect
github.com/aws/smithy-go v1.13.5 // indirect
)

Compiler and Version used

go version go1.19.4 darwin/arm64

Operating System and version

N/A

@gdavison gdavison added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 8, 2023
@isaiahvita isaiahvita self-assigned this Feb 10, 2023
@aajtodd aajtodd assigned aajtodd and unassigned isaiahvita Feb 13, 2023
aajtodd added a commit that referenced this issue 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
@aajtodd aajtodd removed the needs-triage This issue or PR still needs to be triaged. label Feb 17, 2023
@aajtodd
Copy link
Contributor

aajtodd commented Feb 17, 2023

Thanks for the report. I've been able to recreate this.

We had a bit of back and forth on what to do about this one because this is working as intended from a cross SDK consistency standpoint. AWS SDKs are supposed to consume this credential provider when loading the default chain either from environment OR from the shared config file, not a combination of both. The Go SDK has offered a third option with config.WithWebIdentityRoleCredentialOptions which muddies the water a bit. It would appear the intention was to allow setting only role session name with this functional option but we re-used the stscreds.WebIdentityRoleOptions which allows you to specify a whole lot more than just that resulting in a confusing API (and this issue).

We've decided to relax the validation for environment variables and validate role ARN after processing the functional options. Shared config profiles will still be required to specify both properties (role ARN and web identity token file path). Relaxing shared config parsing would result in too large of a consistency gap between SDKs, those profiles would be considered invalid from e.g. Java, Kotlin, Rust, etc.

I believe this will meet your ask but I wanted level set what combination is expected to work after the change.

@joe-a-t
Copy link

joe-a-t commented Feb 17, 2023

Can you please explicitly clarify if hashicorp/terraform-provider-aws#27019 (comment) will work after the change you are discussing get made?

@aajtodd
Copy link
Contributor

aajtodd commented Feb 17, 2023

@joe-a-t I can't speak for terraform, your question would me more appropriately directed at them (or if they want to answer here as to whether the proposed change is sufficient that would be fine).

@joe-a-t
Copy link

joe-a-t commented Feb 17, 2023

@aajtodd If I'm following correctly, I think that it would mean the SDK is receiving the role_arn directly from the Terraform provider but would have to pick up the AWS_WEB_IDENTITY_TOKEN_FILE from the environment variables. Is that something that would work with your proposed change?

@gdavison could you please confirm/clarify ^?

@gdavison
Copy link
Contributor Author

gdavison commented Feb 28, 2023

Hi @joe-a-t, I've been doing some testing. As-is, this doesn't completely address the AWS Provider issue. It does prevent an error, but we still need to make a number of workarounds.

Based on #2030 (comment), I may have misunderstood in what conditions config.WithWebIdentityRoleCredentialOptions would trigger trying to assume these credentials. I'll have to try some more tests.

@aajtodd
Copy link
Contributor

aajtodd commented Feb 28, 2023

I understand the ask to merge all sources of configuration (environment, profile, explicit) but that is unfortunately not how it is supposed to work. The linked PR is a small compromise because the Go SDK contains the config.WithWebIdentityRoleCredentialOptions API.


See response on PR: #2030 (comment) that gives more details and a suggestion.

I'm not sure I really understand the desired behavior at this point so I'll be interested to see what your thoughts are.

@joe-a-t
Copy link

joe-a-t commented Feb 28, 2023

I think the desired behavior is the merging of different sources of configuration, in particular explicitly passing in role_arn and setting AWS_WEB_IDENTITY_TOKEN_FILE in the environment. It was a while ago that we tested this but I seem to recall this desired configuration merging behavior being possible in the AWS CLI.

If meeting that use case is something that you are rejecting, then for my personal use case, I don't think there is a compromise that would provide value to me and might as well leave all the functionality as it currently is.

@aajtodd
Copy link
Contributor

aajtodd commented Feb 28, 2023

but I seem to recall this desired configuration merging behavior being possible in the AWS CLI.

The CLI only has environment and config file so they may very well merge those two sources in some scenarios. The Go SDK has a (confusing) third option via explicit code when loading the default chain. The CLI and the SDKs share many behaviors but not all (and that is on purpose as the are built for different use cases). The config.LoadDefaultConfig is a convenience API to handle the most common development/deployment scenarios such that the SDK "works out of the box". You can achieve what you are asking for by writing your own code around loading credentials.

in particular explicitly passing in role_arn and setting AWS_WEB_IDENTITY_TOKEN_FILE in the environment

The linked PR is a compromise that allows for this. Between your comments and @gdavison I'm no longer sure you are asking for the same things.

aajtodd added a commit that referenced this issue Mar 16, 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
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants