[WIP] Add "duration" option for token command. #75
Conversation
Signed-off-by: Shinichi Ishimura <shiketaudonko41@gmail.com>
|
Allowing the user to control the length of the validity is a great contribution. We may want to coordinate this with the request.Presign() duration as well. We may also want to think if there is a way to have the server restrict how long a token can be valid for. An administrator may want to prevent users from using tokens that are too long lived. |
There was a problem hiding this comment.
Looks great, thanks for adding this! I had a few inline comments.
@mattlandis is right that an administrator might want to require shorter validity, since it's basically a cache of an authorization decision and you might not want that to last too long.
There is already a check on the validation side that won't accept tokens with validity longer than 60 seconds, so we'll need to patch that with a maxSessionValidity configuration or something to make this work:
https://github.com/heptio/authenticator/blob/3b5322c58a2345d330eb5cbdfa003e4ebf329ee4/pkg/token/token.go#L284-L287
| @@ -67,4 +68,6 @@ func init() { | |||
| tokenCmd.Flags().StringP("role", "r", "", "Assume an IAM Role ARN before signing this token") | |||
| viper.BindPFlag("role", tokenCmd.Flags().Lookup("role")) | |||
| viper.BindEnv("role", "DEFAULT_ROLE") | |||
| tokenCmd.Flags().IntP("duration", "d", 15, "Seconds that the token is valid for.") | |||
There was a problem hiding this comment.
I think this flag could be a DurationP which should be a bit cleaner.
| @@ -172,8 +172,11 @@ func (g generator) GetWithRole(clusterID string, roleARN string) (string, error) | |||
| // if a roleARN was specified, replace the STS client with one that uses | |||
| // temporary credentials from that role. | |||
| if roleARN != "" { | |||
| opt := func(p *stscreds.AssumeRoleProvider) { | |||
| p.Duration = time.Duration(duration) * time.Minute | |||
There was a problem hiding this comment.
The usage message above says "Seconds" but this looks like the argument is interpreted as minutes? I think passing a time.Duration all the way through (rather than int) might be better to avoid this type of confusion.
There was a problem hiding this comment.
The usage is wrong. I intended "Minutes" 🙇
|
I have not had a chance to test this but according to the sigv4 signing documentation the maximum length of a signed request being good for is 15 minutes. We could look at having the authenticator cache the credentials if they are longer than 15 minutes a store some where and when the 1.10 api sends us a request we can reuse the local credentials and make another token good for another 15 minutes. This separates the life time of the MFA session from the life of the token. The downside is the client now needs to manage some sort of state storage. |
kamatama41
changed the title
|
@mattlandis @mattmoyer |
|
This is very annoying when using eks with iam roles and logging into k8 dashboard. |
|
Yeah, we use the command below to give users a token to log into the kube dashboard with a token. Works great minus that every 15 minutes their browser session closes on them and they have to get a new token. For kubectl people don't notice that sometimes a command takes longer, but they sure do notice in the web ui when it says session expired/unauthenticated. |
…puted-worker-group-count Use static not computed worker group count
This addresses #63
It would make users happy who feel like the 15 mins is too short :)