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

OIDC User Login flow #55514

Closed
wants to merge 2 commits into from
Closed

Conversation

easeway
Copy link

@easeway easeway commented Nov 11, 2017

What this PR does / why we need it:

Add a new login command to kubectl: kubectl alpha login which orchestrates interactive user login flow with a local web browser. It provides a convenient way without figuring out id-token and refresh-token with some other mechanisms and copies the values into kubeconfig manually.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #54201

Special notes for your reviewer:

This PR is submitted initially as a proposal. Please provide feedback, and I will help to follow up.

Release note:

The following additional config properties are added in kubeconfig under auth-provider:

...
user:
- name: oidc-user
  user:
    auth-provider:
      client-id: client-id-from-provider
      client-secret: client-secret-from-provider
      idp-issuer-url: provider-IDP-URL
      callback-port: '8172'
      callback-path: /authcode
      extra-scopes: profile,email
      auth-options: prompt=consent

The additional properties are all optional:

- callback-port: specifies which port to listen on the local web server launched by kubectl for OIDC callback, must be consistent with the service registered in provider
- callback-path: the path in callback url, must be consistent with service registered in provider
- extra-scopes: scopes in addition to openid
- auth-options: parameters in the initial auth request, semi-comma separated

With this command, initially we don't need to specify id-token and refresh-token in the config.
Run this command, and once the login completes, id-token/refresh-token will be filled in.

One known issue with https://accounts.google.com, as google only issue refresh-token the first time user logins with the service, auth-options: prompt=consent is needed if refresh-token is expected to be updated in config every time.

Yisui Hu added 2 commits November 9, 2017 13:48
Adding full github.com/coreos/go-oidc and github.com/skratchdot/open-golang
to vendor pkgs, for "kubectl alpha login" to perform OIDC user login flow.

A minor fix is needed in hack/update-godep-licenses.sh where max_depth should
be 2 for finding license files as 1 is not enough for packages like
gopkg.in/square/go-jose.v2
kubectl alpha login

will open browser to login users and save tokens back to kubeconfig file

The AuthProvider defined in k8s.io/client-go/rest/plugin.go has a method
called "Login" however it's not used anywhere in the code.
It's a good place the implement the interactive login flow. So this proposal
changes the definition of the method a little to make it for interactive flow.

Only OIDC provider implements the interactive login flow in this commit.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 11, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @easeway. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: easeway
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue: 54201

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@tamalsaha
Copy link
Member

tamalsaha commented Nov 11, 2017

Thank you for doing this @easeway !

@tamalsaha
Copy link
Member

@kubernetes/sig-cli-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Nov 11, 2017
@k8s-ci-robot
Copy link
Contributor

@tamalsaha: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cli-pr-reviews

In response to this:

@kubernetes/sig-cli-pr-reviews

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

http.Error(w, "failed to exchange token: "+err.Error(), http.StatusInternalServerError)
return
}
idToken, ok := token.Extra("id_token").(string)
Copy link
Member

@tamalsaha tamalsaha Nov 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to say that OIDC should not be required for this. Since kubectl can work with --token, this should work for any OAuth2 provider (not just the ones who also send id token).

  • If an id_token, then it can behave like a OIDC auth provider.
  • Otherwise, kubectl just sends the auth token to kube api server. How the auth token gets associated with an identity should be up to the webhook authN server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I agree the implementation will be mostly the same. My implementation goes in the OIDC auth plugin in kubectl, so that will be OIDC specific. Regarding OAuth2, it's a different route performed by kubectl. And currently there's no specific design for an interactive flow. I feel we need more discussion on that before we can consolidate the implementation for both flows.

@liggitt liggitt assigned liggitt, ericchiang and pwittrock and unassigned lavalamp and thockin Nov 11, 2017
@liggitt
Copy link
Member

liggitt commented Nov 11, 2017

@kubernetes/sig-cli-proposals @kubernetes/sig-auth-proposals

@k8s-ci-robot k8s-ci-robot added kind/design Categorizes issue or PR as related to design. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Nov 11, 2017
@liggitt
Copy link
Member

liggitt commented Nov 11, 2017

cc @mikedanese

@mikedanese mikedanese self-assigned this Nov 11, 2017
@@ -2997,6 +3013,21 @@
"Rev": "20b71e5b60d756d3d2f80def009790325acc2b23"
},
{
"ImportPath": "gopkg.in/square/go-jose.v2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go-jose is already vendored at github.com/square/go-jose. Can you just update that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We import gopkg.in/square/go-jose.v2 in go-oidc which I think is the recommended import. https://github.com/square/go-jose#versions

Maybe we can switch the import in k8s.io/kubernetes ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, my preference would be to update existing code to use gopkg.in... and avoid vendoring two of the same library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! I can check the places using go-jose.

@@ -30,8 +30,8 @@ type AuthProvider interface {
// WrapTransport allows the plugin to create a modified RoundTripper that
// attaches authorization headers (or other info) to requests.
WrapTransport(http.RoundTripper) http.RoundTripper
// Login allows the plugin to initialize its configuration. It must not
// require direct user interaction.
// Login allows the plugin to initialize its configuration. It may involve
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a major change that hasn't been discussed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other auth providers already require user interaction. e.g. the azure auth-n plugin can prompt you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the comment on the interface wrong or the azure implementation wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Login command has never been implemented, the Azure transport wrapper does this, so I'd call the comment is outdated.

Conversation about interactive prompts here #43987 (comment)

@pwittrock
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2017
@pwittrock
Copy link
Member

cc @monopole

@pwittrock
Copy link
Member

@easeway Thanks for your contribution. Have you read the SIG cli contributing guide?

@ericchiang
Copy link
Contributor

Beyond the sig-cli concerns I have a couple comments:

  • @tamalsaha's right that this is really close to a pure OAuth2 login (we just grab the "id_token" instead of the "access_token" in the response). We might consider adding a mode for a regular access_token.
  • @liggitt's brought up concerns in the past about transmitting tokens over HTTP, even via localhost. This is something we should at the very least call out.

Going to add this to tomorrow's SIG auth meeting so we can discuss this.

@easeway
Copy link
Author

easeway commented Nov 16, 2017

@pwittrock I will submit a design proposal soon. The issue already exists #54201

@ericchiang
Copy link
Contributor

ericchiang commented Nov 16, 2017

There was some discussion of this in sig-auth today. https://docs.google.com/document/d/1woLGRoONE3EBVx-wTb4pvp4CI7tmLZ6lS26VTbosLKM/view#heading=h.8jbewrjpj9jq

Biggest concerns are:

  • This login mechanism should work with any rotation mechanism.
  • Auth providers need to be pluggable and we should have a plan to add an extension mechanism before adding more.
    • Use cases include kubelet bootstrapping and the many other kind of IdPs we should support.
  • The login mechanism should probably work with generic OAuth2.
  • Syncing with sig-cli to determine what kind of things are okay to compile into kubectl.

Basically, before we start committing to an OpenID Connect login, we probably want to do some planning around client auth providers in general.

I'd like to take on some of this planning over the 1.10 release cycle.

@k8s-github-robot
Copy link

@easeway PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2017
k8s-github-robot pushed a commit that referenced this pull request Dec 15, 2017
Automatic merge from submit-queue (batch tested with PRs 56161, 56324, 55685, 56409, 55296). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

bootstrap: use gopkg.in import for square/go-jose

xref #55514

For 1.10. Ignore while 1.9 code freeze is in effect.

```release-note
NONE
```
@pwittrock
Copy link
Member

/unassign

@ericchiang
Copy link
Contributor

Can people interested in this PR take a look at my proposal for external kubectl auth providers? Specifically the "Login" section?

kubernetes/community#1503

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 15, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 15, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 14, 2018
@dims
Copy link
Member

dims commented May 7, 2018

@easeway let's please close this. please reopen if necessary

/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/design Categorizes issue or PR as related to design. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Support Oauth2 authorization flow in kubectl