From 86f2bea8c58d25c220deb5a692398ea1660c3aaf Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 14 Dec 2021 15:55:35 -0500 Subject: [PATCH] pinniped CLI: allow all forms of http redirects For password based login on the CLI (i.e. no browser), this change relaxes the response code check to allow for any redirect code handled by the Go standard library. In the future, we can drop the rewriteStatusSeeOtherToStatusFoundForBrowserless logic from the server side code. Signed-off-by: Monis Khan --- .../oidc/callback/callback_handler_test.go | 2 +- pkg/oidcclient/login.go | 6 +- pkg/oidcclient/login_test.go | 113 +++++++++++++++++- 3 files changed, 117 insertions(+), 4 deletions(-) diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 79c4afb3..48e04afd 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -174,7 +174,7 @@ func TestCallbackEndpoint(t *testing.T) { }, }, { - name: "GET with good state and cookie and successful upstream token exchange returns 302 to downstream client callback with its state and code", + name: "GET with good state and cookie and successful upstream token exchange returns 303 to downstream client callback with its state and code", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyState).String(), diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 2046d047..470ed3dd 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -435,7 +435,9 @@ func (h *handlerState) cliBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) ( authorizeURL := h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...) // Don't follow redirects automatically because we want to handle redirects here. + var sawRedirect bool h.httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { + sawRedirect = true return http.ErrUseLastResponse } @@ -454,8 +456,8 @@ func (h *handlerState) cliBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) ( } _ = authRes.Body.Close() // don't need the response body, and okay if it fails to close - // A successful authorization always results in a 302. - if authRes.StatusCode != http.StatusFound { + // A successful authorization always results in a redirect (we are flexible on the exact status code). + if !sawRedirect { return nil, fmt.Errorf( "error getting authorization: expected to be redirected, but response status was %s", authRes.Status) } diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 8929c5ef..2bf4620c 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -900,7 +900,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo `/authorize?access_type=offline&client_id=test-client-id&code_challenge=VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g&code_challenge_method=S256&nonce=test-nonce&pinniped_idp_name=some-upstream-name&pinniped_idp_type=ldap&redirect_uri=http%3A%2F%2F127.0.0.1%3A0%2Fcallback&response_type=code&scope=test-scope&state=test-state": some error fetching authorize endpoint`, }, { - name: "ldap login when the OIDC provider authorization endpoint returns something other than a 302 redirect", + name: "ldap login when the OIDC provider authorization endpoint returns something other than a redirect", clientID: "test-client-id", opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -1215,6 +1215,117 @@ func TestLogin(t *testing.T) { // nolint:gocyclo }, wantToken: &testToken, }, + { + name: "successful ldap login with env vars for username and password, http.StatusSeeOther redirect", + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + fakeAuthCode := "test-authcode-value" + + h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) provider.UpstreamOIDCIdentityProviderI { + mock := mockUpstream(t) + mock.EXPECT(). + ExchangeAuthcodeAndValidateTokens( + gomock.Any(), fakeAuthCode, pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), "http://127.0.0.1:0/callback"). + Return(&testToken, nil) + return mock + } + + h.generateState = func() (state.State, error) { return "test-state", nil } + h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } + h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } + h.getEnv = func(key string) string { + switch key { + case "PINNIPED_USERNAME": + return "some-upstream-username" + case "PINNIPED_PASSWORD": + return "some-upstream-password" + default: + return "" // all other env vars are treated as if they are unset + } + } + h.promptForValue = func(_ context.Context, promptLabel string) (string, error) { + require.FailNow(t, fmt.Sprintf("saw unexpected prompt from the CLI: %q", promptLabel)) + return "", nil + } + h.promptForSecret = func(promptLabel string) (string, error) { + require.FailNow(t, fmt.Sprintf("saw unexpected prompt from the CLI: %q", promptLabel)) + return "", nil + } + + cache := &mockSessionCache{t: t, getReturnsToken: nil} + cacheKey := SessionCacheKey{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + } + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawPutKeys) + require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) + }) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithCLISendingCredentials()(h)) + require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(h)) + + discoveryRequestWasMade := false + authorizeRequestWasMade := false + t.Cleanup(func() { + require.True(t, discoveryRequestWasMade, "should have made an discovery request") + require.True(t, authorizeRequestWasMade, "should have made an authorize request") + }) + + require.NoError(t, WithClient(&http.Client{ + Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) { + switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path { + case "http://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + case "http://" + successServer.Listener.Addr().String() + "/authorize": + authorizeRequestWasMade = true + require.Equal(t, "some-upstream-username", req.Header.Get("Pinniped-Username")) + require.Equal(t, "some-upstream-password", req.Header.Get("Pinniped-Password")) + require.Equal(t, url.Values{ + // This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example: + // $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1 + // VVaezYqum7reIhoavCHD1n2d+piN3r/mywoYj7fCR7g + "code_challenge": []string{"VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"}, + "code_challenge_method": []string{"S256"}, + "response_type": []string{"code"}, + "scope": []string{"test-scope"}, + "nonce": []string{"test-nonce"}, + "state": []string{"test-state"}, + "access_type": []string{"offline"}, + "client_id": []string{"test-client-id"}, + "redirect_uri": []string{"http://127.0.0.1:0/callback"}, + "pinniped_idp_name": []string{"some-upstream-name"}, + "pinniped_idp_type": []string{"ldap"}, + }, req.URL.Query()) + return &http.Response{ + StatusCode: http.StatusSeeOther, + Header: http.Header{"Location": []string{ + fmt.Sprintf("http://127.0.0.1:0/callback?code=%s&state=test-state", fakeAuthCode), + }}, + }, nil + default: + // Note that "/token" requests should not be made. They are mocked by mocking calls to ExchangeAuthcodeAndValidateTokens(). + require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String())) + return nil, nil + } + }), + })(h)) + return nil + } + }, + issuer: successServer.URL, + wantLogs: []string{ + "\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\" \"issuer\"=\"" + successServer.URL + "\"", + "\"level\"=4 \"msg\"=\"Pinniped: Read username from environment variable\" \"name\"=\"PINNIPED_USERNAME\"", + "\"level\"=4 \"msg\"=\"Pinniped: Read password from environment variable\" \"name\"=\"PINNIPED_PASSWORD\"", + }, + wantToken: &testToken, + }, { name: "with requested audience, session cache hit with valid token, but discovery fails", clientID: "test-client-id",