vmware-tanzu / pinniped

Pinniped is the easy, secure way to log in to your Kubernetes clusters.
https://pinniped.dev
Apache License 2.0
541 stars 65 forks source link

GitHub upstream authorize redirect url #1929

Closed benjaminapetersen closed 3 months ago

benjaminapetersen commented 4 months ago

Using the Supervisor authorize endpoint with a GitHubIdentityProvider will redirect the user to GitHub's authorize endpoint with the appropriate parameters.

Release note:

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 39.15094% with 129 lines in your changes are missing coverage. Please review.

Project coverage is 29.91%. Comparing base (d9c1b10) to head (0cdbb71). Report is 1 commits behind head on github_identity_provider.

Files Patch % Lines
test/testlib/browsertest/browsertest.go 0.00% 70 Missing :warning:
test/testlib/client.go 0.00% 47 Missing :warning:
test/testlib/env.go 0.00% 8 Missing :warning:
internal/testutil/totp/totp.go 95.23% 2 Missing :warning:
internal/upstreamgithub/upstreamgithub.go 60.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## github_identity_provider #1929 +/- ## ============================================================ + Coverage 29.85% 29.91% +0.05% ============================================================ Files 357 358 +1 Lines 59474 59643 +169 ============================================================ + Hits 17757 17840 +83 - Misses 41190 41277 +87 + Partials 527 526 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

joshuatcasey commented 4 months ago

What is the URI that Pinniped itself sends the user? The URL above https://github.com/login..... looks like a redirect from GitHub.

Would it be possible to have an E2E test that just asserts that the redirect took place? We could even log in with the right use and make assertions about the resulting urls.

benjaminapetersen commented 4 months ago

Given:

To log in using GitHub, run:
PINNIPED_DEBUG=true https_proxy=http://127.0.0.1:12346 no_proxy=127.0.0.1 ./pinniped whoami --kubeconfig ./kubeconfig-github.yaml

Using that kubeconfig:

PINNIPED_DEBUG=true https_proxy=http://127.0.0.1:12346 no_proxy=127.0.0.1 ./pinniped whoami --kubeconfig ./kubeconfig-github.yaml
Tue, 30 Apr 2024 15:49:45 EDT  pinniped-login  cmd/login_oidc.go:260  Performing OIDC login  {"issuer": "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path", "client id": "pinniped-cli"}
Tue, 30 Apr 2024 15:49:45 EDT  oidcclient/login.go:785  Pinniped: Performing OIDC discovery  {"issuer": "https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path"}
Log in by visiting this link:

    https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/oauth2/authorize?access_type=offline&client_id=pinniped-cli&code_challenge=c2pEeFkUjEiwt45R4vAfPm_qDQJiQMFSj3GhQ37bsd4&code_challenge_method=S256&nonce=f686c4da93923aeef0099e91a99047fd&pinniped_idp_name=My+GitHub+IDP+%F0%9F%9A%80&pinniped_idp_type=github&redirect_uri=http%3A%2F%2F127.0.0.1%3A55370%2Fcallback&response_mode=form_post&response_type=code&scope=groups+offline_access+openid+pinniped%3Arequest-audience+username&state=5c95502634ec09916425416ed5afbcd0

    Optionally, paste your authorization code:

Redirects to:

https://github.com/login?client_id=Iv1.220371c36a282659&return_to=%2Flogin%2Foauth%2Fauthorize%3Fclient_id%3DIv1.220371c36a282659%26redirect_uri%3Dhttps%253A%252F%252Fpinniped-supervisor-clusterip.supervisor.svc.cluster.local%252Fsome%252Fpath%252Fcallback%26response_type%3Dcode%26state%ABC---123-G1UcMpCijKnC_4mSEcc3UvxcIxiR5uGF2EWqA%253D
Screenshot 2024-04-30 at 12 19 28 PM

Which, once logged into GitHub, will redirect back to:

https://pinniped-supervisor-clusterip.supervisor.svc.cluster.local/some/path/callback?code=6ce12af26f3848b44565&state=ABC...123-G1UcMpCijKnC_4mSEcc3UvxcIxiR5uGF2EWqA%3D

Which will report:

Internal Server Error # flow is incomplete, redirect back has not yet been implemented.
cfryanr commented 4 months ago

@benjaminapetersen This PR should add some unit tests in auth_handler_test.go for using the web flow with a GitHubIdentityProvider, including the pinniped-cli client and a dynamic client.

joshuatcasey commented 4 months ago

What is the URI that Pinniped itself sends the user? The URL above https://github.com/login..... looks like a redirect from GitHub.

Would it be possible to have an E2E test that just asserts that the redirect took place? We could even log in with the right use and make assertions about the resulting urls.

To clarify, visiting URL <pinniped>/oauth/authorize should 303 with a Location header that has value something like https://github.com/login/oauth/authorize with a handful of parameters (client_id and scope at least). This is the part that should be tested in auth_handler_test.go (thanks for the pointer, @cfryanr !).

I suspect that github is then performing its own redirect to github.com/login but we only care about that to the extent that the E2E test knows how to log in to GitHub to continue the test.

cfryanr commented 3 months ago

This PR should add some unit tests in auth_handler_test.go for using the web flow with a GitHubIdentityProvider, including the pinniped-cli client and a dynamic client.

We added this in the most recent commit.

cfryanr commented 3 months ago

Note: There is an E2E test started in this branch. However, it is always skipped for now. It uses kubectl to start an auth flow, gets redirected to GitHub, fills in the GitHub login page(s), gets redirected back to the Supervisor. However, there is no code yet to implement handling this callback in the Supervisor's callback endpoint, so the Pinniped CLI hangs waiting for the authcode, and therefore kubectl hangs waiting for the the Pinniped CLI. This test is a work in progress and it's okay to merge it to the long-lived feature branch because we will finish it before we merge to main.