aws/credentials/stscreds: adding web identity role provider #2193

xibz · 2018-10-08T23:24:55Z

This update allows for OIDC token files to be used by specifying the token path on AWS_WEB_IDENTITY_TOKEN_FILE. In addition, the role ARN must be set via AWS_IAM_ROLE_ARN

jasdel · 2018-10-08T23:29:53Z

aws/credentials/stscreds/web_identity_provider.go

+}
+
+var emptyTokenFilePathErr = awserr.New(ErrCodeWebIdentityRetrievalErr, "'WebIdentityTokenFilePath' environment variable is empty", nil)
+var emptyRoleARNErr = awserr.New(ErrCodeWebIdentityRetrievalErr, "'WebIdentityRoleARN' environment variable is empty", nil)


The error messages here are too specific, probably should be more generic since these are passed in values.

Also Might be good to make the Error codes be InvalidParameter or something similar to differentiate between that and the API error failed.

jasdel · 2018-10-08T23:30:21Z

aws/credentials/stscreds/web_identity_provider.go

+
+// NewWebIdentityCredentials will return a new set of credentials with a given
+// configuration, role arn, and token file path.
+func NewWebIdentityCredentials(c client.ConfigProvider, roleARN, path string) *credentials.Credentials {


nit path would be better as tokenFilePath

jasdel · 2018-10-08T23:30:25Z

aws/credentials/stscreds/web_identity_provider.go

+
+// NewWebIdentityRoleProvider will return a new WebIdentityRoleProvider with the
+// provided stsiface.STSAPI
+func NewWebIdentityRoleProvider(svc stsiface.STSAPI, roleARN, path string) *WebIdentityRoleProvider {


nit path would be better as tokenFilePath

jasdel · 2018-10-08T23:32:21Z

aws/credentials/stscreds/web_identity_provider.go

+
+	// session name is used to uniquely identify a session. This simply
+	// uses unix time in nanoseconds to uniquely identify sessions.
+	sessionName := strconv.FormatInt(now().UTC().UnixNano(), 10)


This is probably pretty unique, but not sure if it will be unique enough. Since field is required, might need more specific value, and optionally allow user provided.

This is similar to what the CLI does. Would the overhead of adding the hostname (which would be fairly unique for Kubernetes pods) be too much? Could that be done once initially?

jasdel · 2018-10-08T23:35:36Z

aws/credentials/stscreds/web_identity_provider.go

+		return credentials.Value{}, awserr.New(ErrCodeWebIdentityRetrievalErr, "failed to retrieve credentials", err)
+	}
+
+	p.SetExpiration(aws.TimeValue(resp.Credentials.Expiration), p.ExpiryWindow)


Is it possible for the Expiration to be nil? If so these credentials will expire immediately since the TimeValue will be zero time. If no Expiration is a valid use case might need to only SetExpiration if Expiration isn't nil.

This should never be nil, an assumed role credential will always have an expiration and a token. https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithWebIdentity.html

jasdel · 2018-10-08T23:37:29Z

aws/credentials/stscreds/web_identity_provider.go

+
+	value := credentials.Value{
+		AccessKeyID:     aws.StringValue(resp.Credentials.AccessKeyId),
+		SecretAccessKey: aws.StringValue(resp.Credentials.SecretAccessKey),


Probably want to set the SessionToken value as well incase the AssumeRoleWithWebIdentity call returns it.

As stated above, a SessionToken is a required field and should be added

jasdel · 2018-10-08T23:43:08Z

aws/session/session.go

@@ -440,6 +440,15 @@ func mergeConfigSrcs(cfg, userCfg *aws.Config, envCfg envConfig, sharedCfg share
 			cfg.Credentials = credentials.NewStaticCredentialsFromCreds(
 				envCfg.Creds,
 			)
+
+		} else if len(envCfg.WebIdentityTokenFilePath) > 0 {


Should this also validate that the RoleArn env var is provided, or just let the creds fail?

The role ARN is a required field, I'd say it should be required to use this provider

This will return an error, if role ARN is empty, when Retrieve is called. I figure it'd be best to add it to the credential chain and if it is empty return an error signaling an issue back to the user rather than ignoring it silently.

I am okay with changing it to behaving the other way, if it seems more preferable.

How about this, return an error if only one is set at the session create level rather than at the credential retrieval. That way it fails quickly and let's the user know that they have misconfigured the application

Ah ok that makes sense. I like an error being reported if only one is present

micahhausler · 2018-10-09T20:38:54Z

aws/credentials/stscreds/web_identity_provider.go

+	value := credentials.Value{
+		AccessKeyID:     aws.StringValue(resp.Credentials.AccessKeyId),
+		SecretAccessKey: aws.StringValue(resp.Credentials.SecretAccessKey),
+		SessionToken:    aws.StringValue(resp.Credentials.SessionToken),


Should you add the Value.ProviderName to the credentials?

…ntity

jasdel · 2019-06-10T23:31:11Z

When this change is accepted, the web identity should be added as a valid value to credential_source

jasdel · 2019-06-25T23:55:34Z

Replacing this PR with, #2667

…2667) Adds support for assuming an role via the Web Identity Token. Allows for OIDC token files to be used by specifying the token path through the AWS_WEB_IDENTITY_TOKEN_FILE, and AWS_ROLE_ARN environment variables. Replaces PR #2193

xibz requested a review from jasdel October 8, 2018 23:24

jasdel suggested changes Oct 8, 2018

View reviewed changes

xibz force-pushed the webidentity branch from 9531f6f to 4b852f9 Compare October 9, 2018 18:08

Adding web identity role provider

9ca78b7

xibz force-pushed the webidentity branch from 4b852f9 to 9ca78b7 Compare October 9, 2018 20:33

micahhausler reviewed Oct 9, 2018

View reviewed changes

xibz force-pushed the webidentity branch from f8c9e95 to 774cd1e Compare October 9, 2018 20:46

adding session error if values aren't properly configured for web ide…

1168ec5

…ntity

xibz force-pushed the webidentity branch from 774cd1e to 1168ec5 Compare October 9, 2018 20:56

micahhausler mentioned this pull request Oct 30, 2018

Is it possible to use with AssumeRoleWithWebIdentity? kubernetes-sigs/aws-iam-authenticator#154

Closed

pioneerit mentioned this pull request Apr 4, 2019

aws/credentials: Add missing Expiration field to the credentials.Value #2546

Closed

jasdel added the pr/work-in-progress This PR is a draft and needs further work. label Jun 3, 2019

jasdel mentioned this pull request Jun 25, 2019

aws/session: Add support for assuming role via Web Identity Tokens #2667

Merged

jasdel closed this Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws/credentials/stscreds: adding web identity role provider #2193

aws/credentials/stscreds: adding web identity role provider #2193

xibz commented Oct 8, 2018

jasdel Oct 8, 2018

jasdel Oct 8, 2018

jasdel Oct 8, 2018

jasdel Oct 8, 2018

jasdel Oct 8, 2018

micahhausler Oct 9, 2018

jasdel Oct 8, 2018

micahhausler Oct 9, 2018

jasdel Oct 8, 2018

micahhausler Oct 9, 2018

jasdel Oct 8, 2018

micahhausler Oct 9, 2018 •

edited

Loading

xibz Oct 9, 2018 •

edited

Loading

xibz Oct 9, 2018

xibz Oct 9, 2018

micahhausler Oct 9, 2018

micahhausler Oct 9, 2018

jasdel commented Jun 10, 2019

jasdel commented Jun 25, 2019

aws/credentials/stscreds: adding web identity role provider #2193

aws/credentials/stscreds: adding web identity role provider #2193

Conversation

xibz commented Oct 8, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahhausler Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

xibz Oct 9, 2018 • edited Loading

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

jasdel commented Jun 10, 2019

jasdel commented Jun 25, 2019

micahhausler Oct 9, 2018 •

edited

Loading

xibz Oct 9, 2018 •

edited

Loading