Add expiration to token creation #160

nckturner · 2018-10-13T15:41:35Z

STS presigned urls expire after 15 minutes, and are not configurable,
so we refresh tokens after 14 minutes to prevent getting occasional
unauthorized responses.

Fixes #133

nckturner · 2018-10-13T15:51:57Z

/hold

Going to change the value of expiration parameter to Presign() back to 60, since changing it to 900 (15 minutes) breaks backwards compatibility of tokens, and it doesn't have any effect :D I think its reasonable to increase the range of accepted values for the x-amz-expires header, in case we want to push STS to support it in the future.

nckturner · 2018-10-15T16:59:19Z

I set the value of the Presign() parameter back to 60, but left the accepted values on the server side from 0-15 minutes. Based on what I've heard, people would use a longer token expiration than 15 minutes, so if STS is willing to support x-amz-expires at some point in the future, then we should decide what the longest expiration we are willing to allow. That can be done in a future change.

wesleyk · 2018-10-17T21:23:30Z

pkg/token/token.go

 	execInput := &clientauthv1alpha1.ExecCredential{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "client.authentication.k8s.io/v1alpha1",
 			Kind:       "ExecCredential",
 		},
 		Status: &clientauthv1alpha1.ExecCredentialStatus{
-			Token: token,
+			ExpirationTimestamp: &expirationTimestamp,


Will this expiration continually be pushed back with each call to FormatJSON, as it's calculated based on now + refreshTokenAfter?

Or is it the case that the authentication layer will understand, based on the initial ExecCredentialStatus, that it a refresh should occur after the provided ExpirationTimestamp?

What happens is the FormatJSON is executed each time a token is generated, and the response is cached in client-go. This will solve the issue where client-go is running continuously, like in the sample program provided in the issue, or in kubelet, etc. For kubectl, we need client caching in a file, for which there is currently another PR open. I did refactor a little based on your comment (no functionality change) where its now generating the expiration next to the place where the token is generated. The expiration of the token is based on the x-amz-date header, so I could try deriving the expiration from that instead, which is more correct, though in practice I doubt there would be a noticeable difference from the two timestamps.

wesleyk

Looks good to me, but I don't have full context on the code. Thanks for the help Nick!

mattlandis

Looks good. Only a few small comments.

pkg/token/token.go

mattlandis · 2018-10-18T18:06:36Z

pkg/token/token.go

@@ -125,15 +140,15 @@ type getCallerIdentityWrapper struct {
 // Generator provides new tokens for the heptio authenticator.
 type Generator interface {
 	// Get a token using credentials in the default credentials chain.
-	Get(string) (string, error)
+	Get(string) (*Token, error)


I would prefer returning an empty struct to nil since we don't use it as a pointer (modify contents etc.)

nckturner · 2018-10-18T20:24:17Z

/hold cancel

STS presigned urls expire after 15 minutes, and its unconfigurable, so we refresh tokens after 14 minutes to prevent getting occasional unauthorized responses.

- Rename expiration variables to reflect their actual use - Create Token struct so that we can generate the expiration time immediately after we presign the token, and return the Token struct from the generate token functions.

mattlandis · 2018-10-18T22:28:09Z

/lgtm
/approve

k8s-ci-robot · 2018-10-18T22:28:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattlandis, nckturner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattlandis,nckturner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nakulpathak3 · 2018-12-19T20:19:17Z

hey I know this went in but I'm getting 403 Forbidden "could not get token: AccessDenied: Access denied" error at random times still. I'm using the Kubernetes Python Client which implemented the usage of aws-iam-authenticator here - https://github.com/kubernetes-client/python-base/blob/master/config/exec_provider.py#L77. This is happening after creating a role and using the -r flag not with normal user accounts.

mlafeldt · 2018-12-20T15:51:08Z

Verified that this is working with kubectl proxy. Good job!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 13, 2018

k8s-ci-robot requested review from mattlandis and mattmoyer October 13, 2018 15:41

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 13, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2018

nckturner force-pushed the expiration branch 2 times, most recently from 2d0671d to 61e7e6d Compare October 15, 2018 16:54

nckturner mentioned this pull request Oct 16, 2018

Add cache functionality for tokens #140

Closed

wesleyk reviewed Oct 17, 2018

View reviewed changes

wesleyk approved these changes Oct 17, 2018

View reviewed changes

mattlandis reviewed Oct 18, 2018

View reviewed changes

mattlandis mentioned this pull request Oct 18, 2018

Long run of "kubectl run" does not refresh IAM token #148

Closed

nckturner force-pushed the expiration branch from 65ba8ee to 7aa0596 Compare October 18, 2018 20:14

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2018

nckturner force-pushed the expiration branch from 7aa0596 to 2f9099f Compare October 18, 2018 22:02

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2018

Add expiration to token creation

981ecbe

STS presigned urls expire after 15 minutes, and its unconfigurable, so we refresh tokens after 14 minutes to prevent getting occasional unauthorized responses.

nckturner force-pushed the expiration branch from 2f9099f to e3ad53b Compare October 18, 2018 22:18

Refactor for clarity

b60fb07

- Rename expiration variables to reflect their actual use - Create Token struct so that we can generate the expiration time immediately after we presign the token, and return the Token struct from the generate token functions.

nckturner force-pushed the expiration branch from e3ad53b to b60fb07 Compare October 18, 2018 22:23

k8s-ci-robot assigned mattlandis Oct 18, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2018

k8s-ci-robot merged commit 4ee748e into kubernetes-sigs:master Oct 18, 2018

tbarrella mentioned this pull request May 22, 2019

aws eks get-token regresses providing token expiration aws/aws-cli#4182

Closed

kyleknap mentioned this pull request Jun 26, 2019

Add expirationTimestamp to get-token CLI aws/aws-cli#4203

Merged

joanayma pushed a commit to joanayma/aws-iam-authenticator that referenced this pull request Aug 11, 2021

Removing aws_iam_service_linked_role from module (kubernetes-sigs#160)

b15e1d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expiration to token creation #160

Add expiration to token creation #160

nckturner commented Oct 13, 2018 •

edited

Loading

nckturner commented Oct 13, 2018

nckturner commented Oct 15, 2018

wesleyk Oct 17, 2018

nckturner Oct 18, 2018 •

edited

Loading

wesleyk left a comment

mattlandis left a comment

mattlandis Oct 18, 2018

nckturner commented Oct 18, 2018

mattlandis commented Oct 18, 2018

k8s-ci-robot commented Oct 18, 2018

nakulpathak3 commented Dec 19, 2018

mlafeldt commented Dec 20, 2018

Add expiration to token creation #160

Add expiration to token creation #160

Conversation

nckturner commented Oct 13, 2018 • edited Loading

nckturner commented Oct 13, 2018

nckturner commented Oct 15, 2018

wesleyk Oct 17, 2018

Choose a reason for hiding this comment

nckturner Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

wesleyk left a comment

Choose a reason for hiding this comment

mattlandis left a comment

Choose a reason for hiding this comment

mattlandis Oct 18, 2018

Choose a reason for hiding this comment

nckturner commented Oct 18, 2018

mattlandis commented Oct 18, 2018

k8s-ci-robot commented Oct 18, 2018

nakulpathak3 commented Dec 19, 2018

mlafeldt commented Dec 20, 2018

nckturner commented Oct 13, 2018 •

edited

Loading

nckturner Oct 18, 2018 •

edited

Loading