WIP - Add Login Command, and add OIDC implementation. #25758

bobbyrullo · 2016-05-17T17:06:56Z

OIDC implementation of login command.
Spawns a browser, and sends user to the OIDC IdP to authenticate, after
which they are redirected back to a server running on localhost, which
performs the auth code exchange and persists the ID Token and possibly
refresh token.

NOTE: this is a WIP, rebased off of #25270, which this is dependent on.

k8s-bot · 2016-05-17T17:07:12Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-17T17:08:15Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-05-17T17:08:37Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

bobbyrullo · 2016-05-17T17:12:06Z

@cjcullen - uses your new AuthProvider framework - not sure if I'm getting it the right way inside the cmd though.

bobbyrullo · 2016-05-19T15:52:23Z

@deads2k, @cjcullen, - The PR that this depends on is in the submit queue so I would really I appreciate a review - we have folks who can review the OIDC aspect of things so it's really just this tiny commit here that I need help with - namely, I'm not sure if I'm getting the AuthProvider correctly; I tried getting it from the factory.ClientConfig() but that has the unfortunate effect of trying to actually use the AuthProvider's transport to do version negotiation, and of course that fails when you don't have any credentials yet (as would be the case for anyone logging in)

cjcullen · 2016-05-19T16:34:50Z

@bobbyrullo Does the login() command work as you'd expect?

If no credentials are present for version negotiation, does that mean that the /api and /apis endpoints are open to anybody?

bobbyrullo · 2016-05-19T16:41:13Z

@bobbyrullo Does the login() command work as you'd expect?

As implemented it does work...sort of. I realize that the way I have it implemented it's ignoring the "--user" flag, so no matter what it uses the user set in your kubeconfig's current-context.

If no credentials are present for version negotiation, does that mean that the /api and /apis endpoints are open to anybody?

No, the opposite of that is my problem; if there's no credentials, you cannot do version negotiation because all endpoints are protected. I don't need version negotiation for login() but I can't call ClientConfig() without the side effect of that happening.

deads2k · 2016-05-19T16:45:57Z

@smarterclayton @liggitt @fabianofranz probably have opinions about the shape of the command too.

deads2k · 2016-05-19T16:48:42Z

pkg/kubectl/cmd/login.go

+	cmd := &cobra.Command{
+		Use:   "login",
+		Short: "login Log in to the cluster.",
+		Long:  `login obtains the neccesary credentials to make authenticated requests to the cluster. The type of log in depends on the 'auth-provider' type in your configuration, and will likely require some type of user interaction.`,


Seems like you'll want to support a bootstrapping case where the user actually specifies the mechanism he'd like to use to login. Something like --auth-provider=oidc. It also seems like you'd want a flag to provide some initial bootstrapping for the auth provider if none exists.

Isn't that case is supported by --user=XXX ? An AuthProvider belongs to a AuthInfo (i.e. within user in the .kube/config), they don't exist on their own in in the Config.

As for an initial bootstrapping flag, what did you have in mind there? Each AuthProvider has its own configuration, so maybe a flag which takes a bunch of key/value properties separated by some delimiter?

Isn't that case is supported by --user=XXX ? An AuthProvider belongs to a AuthInfo (i.e. within user in the .kube/config), they don't exist on their own in in the Config.

--user is a different flag, which in kube means "present this user as part of my basic auth credential to the API server. Since you won't be using that in future calls to the API server, I'm not sure--user` makes sense.

Maybe I could see the re-use, but I don't see it as automatically correct.

Each AuthProvider has its own configuration, so maybe a flag which takes a bunch of key/value properties separated by some delimiter?

Yeah, I think that would do.

Isn't that case is supported by --user=XXX ? An AuthProvider belongs to a AuthInfo (i.e. within user in the .kube/config), they don't exist on their own in in the Config.

--user is a different flag, which in kube means "present this user as part of my basic auth credential to the API server. Since you won't be using that in future calls to the API server, I'm not sure--user` makes sense.

Maybe I could see the re-use, but I don't see it as automatically correct.

That change has already happened - changing your --user already changes the AuthProvider that is being used to WrapTransport

Isn't that case is supported by --user=XXX ? An AuthProvider belongs to a AuthInfo (i.e. within user in the .kube/config), they don't exist on their own in in the Config.

I don't think so. That pre-supposes a .kube/config file that has a stubbed user stanza. I don't think that the should be the assumption kubectl login. I think kubectl login is where I go to get from nothing to working .kube/config in a single command.

There are two parts of login extension.

Baked in login code that could init the auth provider if a user could indicate it - i.e. a flag like --oidc

A set of code that is not baked into login, but which could be added via a package installer such that kubectl login could delegate - i.e. a binary kubectl-login-oidc in the path that the flag --oidc would instruct kubectl login --oidc to exec kubectl-login-oidc with any shim logic in place.

I'd like to see both - bootstrapping a kube config initially is ok as a starting point, but I'd much rather be able to say kubectl login --kerberos or kubectl login --gcloud.

bobbyrullo · 2016-05-19T23:11:34Z

@brendandburns - could you please tag this as p1 and 1.3 as per the Effort tracking Spreadsheet?

Login gets the current AuthProvider and calls Login(); some user interaction is assumed to have happen after.

Spawns a browser, and sends user to the OIDC IdP to authenticate, after which they are redirected back to a server running on localhost, which performs the auth code exchange and persists the ID Token and possibly refresh token.

bobbyrullo · 2016-05-25T17:19:53Z

@bobbyrullo: Does that keep to the spirit of what you want out of the login command?

Sure, but are all those features necessary for an initial implementation? Can we not do this incrementally?

I'm happy to be on the hook for doing it even; my only problem is that all this extra interactive stuff with flags and whatnot requires some UX design work which will take time. Adding just the case where login handles only the fully-configured .kube/config is much simpler. I don't see the disadvantage here to an incremental approach, which is all that I'm advocating.

deads2k · 2016-05-25T18:18:01Z

Sure, but are all those features necessary for an initial implementation? Can we not do this incrementally?

I think I see --auth-provider= and --auth-provider-args= as required for an initial implementation.

I also think that an implementation that doesn't consider a no auth-provider case (basic auth against kube api server) isn't one I'd want to merge. I think I'd like to see that stubbed (detect the challenge and fail maybe?).

k8s-github-robot · 2016-05-29T17:53:31Z

@bobbyrullo PR needs rebase

bobbyrullo · 2016-05-31T16:46:27Z

I think I see --auth-provider= and --auth-provider-args= as required for an initial implementation.

Sounds reasonable.

So in that case, if you provide an --auth-provider, you must also provide an --auth-provider-args as well right (unless your auth-provider somehow works without config?

Also, do you think --auth-provider-config is a better name?

I also think that an implementation that doesn't consider a no auth-provider case (basic auth against kube api server) isn't one I'd want to merge. I think I'd like to see that stubbed (detect the challenge and fail maybe?)

Is basic auth what most people are using to authenticate to k8s with? If not, why is it a reasonable default? Why not simply "please configure an auth-provider before using the login command"?

Either way, it seems that the right way to do basic auth in this scenario would be to go ahead and implement a BasicAuthAuthProvider, right?

deads2k · 2016-05-31T20:11:11Z

Either way, it seems that the right way to do basic auth in this scenario would be to go ahead and implement a BasicAuthAuthProvider, right?

Thing is, it's not using the plugin mechanism. It's using the native fields we provide.

k8s-bot · 2016-06-02T19:21:19Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-14T23:06:25Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-15T00:34:19Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-28T12:57:10Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-29T16:28:48Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-30T01:48:19Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

ericchiang · 2016-06-30T21:36:03Z

FYI: I'll be taking over this PR for the 1.4 release cycle. Will be opening a PR in about a week (will be on vacation).

ericchiang · 2016-07-11T23:17:22Z

I think I see --auth-provider= and --auth-provider-args= as required for an initial implementation.

@deads2k just to clarify, would it be enough to provide a flag for logging in:

kubectl login --auth-provider=( foo )

And a flag for persisting provider config values?

kubectl login --auth-provider=( foo ) --auth-provider-args=( key )=( value )

Just trying to catch back up on this thread.

ericchiang · 2016-07-11T23:19:43Z

No idea how I just unassigned @brendandburns btw (since I don't have write access). Fear my wizardry.

deads2k · 2016-07-12T11:37:31Z

@deads2k just to clarify, would it be enough to provide a flag for logging in:

I think those two arguments (both options) and working with the "normal" kube basic auth would do it. That would cover how people would plug in via the auth provider mechanism and how we could handle challenges for the built-in fields.

bgrant0607 · 2016-07-19T03:30:34Z

cc @kubernetes/kubectl

bgrant0607 · 2016-07-19T03:33:17Z

ok to test

k8s-bot · 2016-07-19T03:35:43Z

GCE e2e build/test failed for commit 56820e6.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@deads2k

Automatic merge from submit-queue docs/proposal: add proposal for kubectl login This PR updates kubernetes/enhancements#32 and #25758 by adding a proposal for a "kubectl login" command. It's a bit more involved than the implementation discussed with @deads2k in #25758, by proposing a long term goal for the overall subcommand. cc @kubernetes/sig-auth @kubernetes/kubectl

@deads2k

Automatic merge from submit-queue docs/proposal: add proposal for kubectl login This PR updates kubernetes/enhancements#32 and kubernetes/kubernetes#25758 by adding a proposal for a "kubectl login" command. It's a bit more involved than the implementation discussed with @deads2k in #25758, by proposing a long term goal for the overall subcommand. cc @kubernetes/sig-auth @kubernetes/kubectl

googlebot added the cla: yes label May 17, 2016

bobbyrullo force-pushed the login branch from 1331642 to 4b81699 Compare May 17, 2016 17:07

k8s-github-robot assigned brendandburns May 17, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 17, 2016

bobbyrullo mentioned this pull request May 17, 2016

Implement OIDC client AuthProvider #25270

Merged

deads2k reviewed May 19, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2016

Bobby Rullo added 2 commits May 25, 2016 13:11

Add new Login command.

1bc481b

Login gets the current AuthProvider and calls Login(); some user interaction is assumed to have happen after.

Add OIDC implementation of login command.

56820e6

Spawns a browser, and sends user to the OIDC IdP to authenticate, after which they are redirected back to a server running on localhost, which performs the auth code exchange and persists the ID Token and possibly refresh token.

bobbyrullo force-pushed the login branch from 4b81699 to 56820e6 Compare May 25, 2016 17:14

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 25, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2016

erictune self-assigned this Jun 6, 2016

ericchiang unassigned brendandburns Jul 11, 2016

ericchiang mentioned this pull request Jul 18, 2016

Kubectl login subcommand kubernetes/enhancements#32

Closed

20 tasks

ericchiang mentioned this pull request Jul 21, 2016

docs/proposal: add proposal for kubectl login #29350

Merged

bobbyrullo closed this Aug 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Add Login Command, and add OIDC implementation. #25758

WIP - Add Login Command, and add OIDC implementation. #25758

bobbyrullo commented May 17, 2016 •

edited

k8s-bot commented May 17, 2016

k8s-bot commented May 17, 2016

k8s-bot commented May 17, 2016

bobbyrullo commented May 17, 2016

bobbyrullo commented May 19, 2016

cjcullen commented May 19, 2016

bobbyrullo commented May 19, 2016

deads2k commented May 19, 2016

deads2k May 19, 2016

bobbyrullo May 19, 2016

deads2k May 19, 2016

deads2k May 19, 2016

bobbyrullo May 19, 2016

deads2k May 19, 2016

smarterclayton May 19, 2016 •

edited

bobbyrullo commented May 19, 2016

bobbyrullo commented May 25, 2016

deads2k commented May 25, 2016

k8s-github-robot commented May 29, 2016

bobbyrullo commented May 31, 2016

deads2k commented May 31, 2016

k8s-bot commented Jun 2, 2016

k8s-bot commented Jun 14, 2016

k8s-bot commented Jun 15, 2016

k8s-bot commented Jun 28, 2016

k8s-bot commented Jun 29, 2016

k8s-bot commented Jun 30, 2016

ericchiang commented Jun 30, 2016

ericchiang commented Jul 11, 2016

ericchiang commented Jul 11, 2016

deads2k commented Jul 12, 2016

bgrant0607 commented Jul 19, 2016

bgrant0607 commented Jul 19, 2016

k8s-bot commented Jul 19, 2016

WIP - Add Login Command, and add OIDC implementation. #25758

WIP - Add Login Command, and add OIDC implementation. #25758

Conversation

bobbyrullo commented May 17, 2016 • edited

k8s-bot commented May 17, 2016

k8s-bot commented May 17, 2016

k8s-bot commented May 17, 2016

bobbyrullo commented May 17, 2016

bobbyrullo commented May 19, 2016

cjcullen commented May 19, 2016

bobbyrullo commented May 19, 2016

deads2k commented May 19, 2016

deads2k May 19, 2016

Choose a reason for hiding this comment

bobbyrullo May 19, 2016

Choose a reason for hiding this comment

deads2k May 19, 2016

Choose a reason for hiding this comment

deads2k May 19, 2016

Choose a reason for hiding this comment

bobbyrullo May 19, 2016

Choose a reason for hiding this comment

deads2k May 19, 2016

Choose a reason for hiding this comment

smarterclayton May 19, 2016 • edited

Choose a reason for hiding this comment

bobbyrullo commented May 19, 2016

bobbyrullo commented May 25, 2016

deads2k commented May 25, 2016

k8s-github-robot commented May 29, 2016

bobbyrullo commented May 31, 2016

deads2k commented May 31, 2016

k8s-bot commented Jun 2, 2016

k8s-bot commented Jun 14, 2016

k8s-bot commented Jun 15, 2016

k8s-bot commented Jun 28, 2016

k8s-bot commented Jun 29, 2016

k8s-bot commented Jun 30, 2016

ericchiang commented Jun 30, 2016

ericchiang commented Jul 11, 2016

ericchiang commented Jul 11, 2016

deads2k commented Jul 12, 2016

bgrant0607 commented Jul 19, 2016

bgrant0607 commented Jul 19, 2016

k8s-bot commented Jul 19, 2016

bobbyrullo commented May 17, 2016 •

edited

smarterclayton May 19, 2016 •

edited