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
WIP - Add Login Command, and add OIDC implementation. #25758
Conversation
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
1 similar comment
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
@cjcullen - uses your new AuthProvider framework - not sure if I'm getting it the right way inside the |
@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 |
@bobbyrullo Does the login() command work as you'd expect? If no credentials are present for version negotiation, does that mean that the |
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.
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 |
@smarterclayton @liggitt @fabianofranz probably have opinions about the shape of the command too. |
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.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 instructkubectl login --oidc
to execkubectl-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
.
@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.
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 |
I think I see 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?). |
@bobbyrullo PR needs rebase |
Sounds reasonable. So in that case, if you provide an Also, do you think
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 |
Thing is, it's not using the plugin mechanism. It's using the native fields we provide. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
4 similar comments
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". 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. |
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). |
@deads2k just to clarify, would it be enough to provide a flag for logging in:
And a flag for persisting provider config values?
Just trying to catch back up on this thread. |
No idea how I just unassigned @brendandburns btw (since I don't have write access). Fear my wizardry. |
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. |
cc @kubernetes/kubectl |
ok to test |
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. |
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
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
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.