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

Support getting tokens from an IAM Role requiring MFA #42

Merged
merged 2 commits into from Apr 12, 2018

Conversation

gwkunze
Copy link
Contributor

@gwkunze gwkunze commented Jan 16, 2018

Adding the AssumeRoleTokenProvider allows accounts with MFA to also request a token. Instead of getting an error:

heptio-authenticator-aws token -i my-cluster -r arn:aws:iam::123456789012:role/KubernetesAdmin
could not get token: could not create session: AssumeRoleTokenProviderNotSetError: assume role with MFA enabled, but AssumeRoleTokenProvider session option not set.

The app asks the user for an mfa code:

heptio-authenticator-aws token -i my-cluster -r arn:aws:iam::123456789012:role/KubernetesAdmin
Assume Role MFA token code: 123456
k8s-aws-v1.abcdefg......

Signed-off-by: Gijs Kunze <gijs@usabilla.com>
@nckturner
Copy link
Contributor

Great! LGTM

@gwkunze
Copy link
Contributor Author

gwkunze commented Jan 25, 2018

I should note that since stscreds.StdinTokenProvider prompts on stdout and reads from stdin the kubectl --kubeconfig /path/to/kubeconfig --token "$(heptio-authenticator-aws token -i CLUSTER_ID -r ROLE_ARN)" [...]-command as described in the readme doesn't work as the prompt will be part of the --token argument. Switching to custom prompt written to stderr would probably resolve this.

Another issue is that this will mean that the user will need to enter an MFA code for every kubectl command they want to execute. Including having to wait 30 seconds between commands for a new code to be generated as you can't use the same one twice. This is of course not the best user experience. It might be better if there was some caching of the pre-signed url on the client side machine, only generating a new token when the url is about to expire...

@nckturner
Copy link
Contributor

nckturner commented Jan 25, 2018

@gwkunze Noted, the implementation of this proposal kubernetes/community#1503 kubernetes/enhancements#541 which is how heptio authenticator will be exec'd by kubectl, will have to make sure the exec'd process properly inherits stdin from kubectl. Its a similar situation for binaries that have a login functionality.

We should take care that interactive stderr and stdin are correctly inherited by the sub-process to enable this kind of interaction. The plugin will still be responsible for prompting the user, receiving user feedback, and timeouts.

Caching is also be handled by that proposal, but perhaps we can wait until the proposal is finalized before moving forward with this.

Plugins will be called on client-go initialization, and again when the API server returns a 401 HTTP status code indicating expired credentials. Plugins can indicate their credentials explicit expiry using the Expiry field on the returned ExecCredentials object, otherwise credentials will be cached hroughout the lifetime of a program.

@mattmoyer
Copy link
Contributor

@gwkunze thanks for this! We run with MFA-restricted IAM roles here too so this is very welcome.

I'd still like to merge this, but I agree with @nckturner re:

wait until the proposal is finalized before moving forward with this.

If it's helpful, I've been using https://github.com/99designs/aws-vault to do the MFA dance.

@mattmoyer
Copy link
Contributor

If you don't mind, do a git --amend --signoff to add a Signed-Off-By: line and make our DCO bot happy.

Signed-off-by: Gijs Kunze <gijs@usabilla.com>
@gwkunze
Copy link
Contributor Author

gwkunze commented Feb 13, 2018

Apologies for the late update.

@mumoshu
Copy link
Contributor

mumoshu commented Mar 19, 2018

@mattlandis @mattmoyer @nckturner Hi, this is a friendly reminder towards merging this great work.

Given the commit is now signed-off and it is already LGTMed, probably you want to merge this?

@mattlandis
Copy link
Contributor

I want to check that this works properly with the new 1.10 auth provider changes in client-go before we merge this change in.

@nckturner
Copy link
Contributor

nckturner commented Apr 12, 2018

Rereading @gwkunze's comment, all the issues he mentioned apply for the 1.10 auth provider because the aws sdk prints the prompt to enter your MFA code on stdout, which in the 1.10 exec based system is reserved for the formatted json token output. Also, I'm not sure why I missed this initially but prompting to stderr is exactly what we should do.

I propose we merge this change, tag and branch for kubernetes 1.9, then merge 1.10 support and create an issue in this repository for the broken 1.10 MFA functionality. We can then look into fixing that with a custom AssumeRoleProvider or something similar. Note that we will need the fix in the 1.9 branch as well.

@nckturner nckturner closed this Apr 12, 2018
@nckturner nckturner reopened this Apr 12, 2018
@nckturner
Copy link
Contributor

And prompting every kubectl invocation would be annoying, you're right, we should look into caching client side beyond the lifetime of the authenticator.

@mattmoyer
Copy link
Contributor

It looks like providing a custom TokenProvider function for stderr vs. stdout would be pretty easy:
https://github.com/aws/aws-sdk-go/blob/master/aws/credentials/stscreds/assume_role_provider.go#L103-L109

It looks like if we just replace the fmt.Printf("...") with fmt.Fprint(os.Stderr, "...") we'll be good to go:

func StdinStderrTokenProvider() (string, error) {
	var v string
	fmt.Fprint(os.Stderr, "Assume Role MFA token code: ")
	_, err := fmt.Scanln(&v)
	return v, err
}

@nckturner
Copy link
Contributor

Great, let's merge this PR and get a fix in.

@nckturner nckturner merged commit 43dfa3e into kubernetes-sigs:master Apr 12, 2018
@gwkunze gwkunze deleted the patch-1 branch April 13, 2018 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants