From 7b97f1533e3010f278b14dac823b294e5f746969 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 4 Feb 2022 16:57:37 -0800 Subject: [PATCH 1/3] Add CORS request handling to CLI's localhost listener This is to support the new changes in Google Chrome v98 which now performs CORS preflight requests for the Javascript form submission on the Supervisor's login page, even though the form is being submitted to a localhost listener. --- pkg/oidcclient/login.go | 37 +++++++++- pkg/oidcclient/login_test.go | 132 +++++++++++++++++++++++++++++------ 2 files changed, 143 insertions(+), 26 deletions(-) diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index d9364689e..223e7fb2a 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -834,10 +834,41 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req }() var params url.Values - if h.useFormPost { + if h.useFormPost { // nolint:nestif + if r.Method == http.MethodOptions { + // Google Chrome decided that it should do CORS preflight checks for this Javascript form submission POST request. + // See https://developer.chrome.com/blog/private-network-access-preflight/ + origin := r.Header.Get("Origin") + if origin == "" { + // The CORS preflight request should have an origin. + h.logger.V(debugLogLevel).Info("Pinniped: Got OPTIONS request without origin header") + w.WriteHeader(http.StatusBadRequest) + return nil // keep listening for more requests + } + h.logger.V(debugLogLevel).Info("Pinniped: Got CORS preflight request from browser", "origin", origin) + issuerURL, parseErr := url.Parse(h.issuer) + if parseErr != nil { + return httperr.Wrap(http.StatusInternalServerError, "invalid issuer url", parseErr) + } + // To tell the browser that it is okay to make the real POST request, return the following response. + w.Header().Set("Access-Control-Allow-Origin", issuerURL.Scheme+"://"+issuerURL.Host) + w.Header().Set("Access-Control-Allow-Credentials", "false") + w.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS") + w.Header().Set("Access-Control-Allow-Private-Network", "true") + // If the browser would like to send some headers on the real request, allow them. Chrome doesn't + // currently send this header at the moment. This is in case some browser in the future decides to + // request to be allowed to send specific headers by using Access-Control-Request-Headers. + requestedHeaders := r.Header.Get("Access-Control-Request-Headers") + if requestedHeaders != "" { + w.Header().Set("Access-Control-Allow-Headers", requestedHeaders) + } + w.WriteHeader(http.StatusNoContent) + return nil // keep listening for more requests + } + // Return HTTP 405 for anything that's not a POST. if r.Method != http.MethodPost { - return httperr.Newf(http.StatusMethodNotAllowed, "wanted POST") + return httperr.Newf(http.StatusMethodNotAllowed, "wanted POST but got %s", r.Method) } // Parse and pull the response parameters from a application/x-www-form-urlencoded request body. @@ -848,7 +879,7 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req } else { // Return HTTP 405 for anything that's not a GET. if r.Method != http.MethodGet { - return httperr.Newf(http.StatusMethodNotAllowed, "wanted GET") + return httperr.Newf(http.StatusMethodNotAllowed, "wanted GET but got %s", r.Method) } // Pull response parameters from the URL query string. diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 8ee920d72..bae18b49b 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -1825,6 +1825,8 @@ func TestHandlePasteCallback(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { + t.Parallel() + h := &handlerState{ callbacks: make(chan callbackResult, 1), state: state.State("test-state"), @@ -1866,35 +1868,38 @@ func TestHandleAuthCodeCallback(t *testing.T) { } } tests := []struct { - name string - method string - query string - body []byte - contentType string - opt func(t *testing.T) Option - wantErr string - wantHTTPStatus int + name string + method string + query string + body []byte + headers http.Header + opt func(t *testing.T) Option + + wantErr string + wantHTTPStatus int + wantNoCallbacks bool + wantHeaders http.Header }{ { name: "wrong method", - method: "POST", + method: http.MethodPost, query: "", - wantErr: "wanted GET", + wantErr: "wanted GET but got POST", wantHTTPStatus: http.StatusMethodNotAllowed, }, { name: "wrong method for form_post", - method: "GET", + method: http.MethodGet, query: "", opt: withFormPostMode, - wantErr: "wanted POST", + wantErr: "wanted POST but got GET", wantHTTPStatus: http.StatusMethodNotAllowed, }, { name: "invalid form for form_post", - method: "POST", + method: http.MethodPost, query: "", - contentType: "application/x-www-form-urlencoded", + headers: map[string][]string{"Content-Type": {"application/x-www-form-urlencoded"}}, body: []byte(`%`), opt: withFormPostMode, wantErr: `invalid form: invalid URL escape "%"`, @@ -1918,6 +1923,75 @@ func TestHandleAuthCodeCallback(t *testing.T) { wantErr: `login failed with code "some_error": optional error description`, wantHTTPStatus: http.StatusBadRequest, }, + { + name: "in form post mode, invalid issuer url config during CORS preflight request returns an error", + method: http.MethodOptions, + query: "", + headers: map[string][]string{"Origin": {"https://some-origin.com"}}, + wantErr: `invalid issuer url: parse "://bad-url": missing protocol scheme`, + wantHTTPStatus: http.StatusInternalServerError, + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.useFormPost = true + h.issuer = "://bad-url" + return nil + } + }, + }, + { + name: "in form post mode, options request is missing origin header results in 400 and keeps listener running", + method: http.MethodOptions, + query: "", + opt: withFormPostMode, + wantNoCallbacks: true, + wantHTTPStatus: http.StatusBadRequest, + }, + { + name: "in form post mode, valid CORS request responds with 402 and CORS headers and keeps listener running", + method: http.MethodOptions, + query: "", + headers: map[string][]string{"Origin": {"https://some-origin.com"}}, + wantNoCallbacks: true, + wantHTTPStatus: http.StatusNoContent, + wantHeaders: map[string][]string{ + "Access-Control-Allow-Credentials": {"false"}, + "Access-Control-Allow-Methods": {"POST, OPTIONS"}, + "Access-Control-Allow-Origin": {"https://valid-issuer.com"}, + "Access-Control-Allow-Private-Network": {"true"}, + }, + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.useFormPost = true + h.issuer = "https://valid-issuer.com/with/some/path" + return nil + } + }, + }, + { + name: "in form post mode, valid CORS request with Access-Control-Request-Headers responds with 402 and CORS headers including Access-Control-Allow-Headers and keeps listener running", + method: http.MethodOptions, + query: "", + headers: map[string][]string{ + "Origin": {"https://some-origin.com"}, + "Access-Control-Request-Headers": {"header1, header2, header3"}, + }, + wantNoCallbacks: true, + wantHTTPStatus: http.StatusNoContent, + wantHeaders: map[string][]string{ + "Access-Control-Allow-Credentials": {"false"}, + "Access-Control-Allow-Methods": {"POST, OPTIONS"}, + "Access-Control-Allow-Origin": {"https://valid-issuer.com"}, + "Access-Control-Allow-Private-Network": {"true"}, + "Access-Control-Allow-Headers": {"header1, header2, header3"}, + }, + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.useFormPost = true + h.issuer = "https://valid-issuer.com/with/some/path" + return nil + } + }, + }, { name: "invalid code", query: "state=test-state&code=invalid", @@ -1938,8 +2012,9 @@ func TestHandleAuthCodeCallback(t *testing.T) { }, }, { - name: "valid", - query: "state=test-state&code=valid", + name: "valid", + query: "state=test-state&code=valid", + wantHTTPStatus: http.StatusOK, opt: func(t *testing.T) Option { return func(h *handlerState) error { h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} @@ -1955,10 +2030,11 @@ func TestHandleAuthCodeCallback(t *testing.T) { }, }, { - name: "valid form_post", - method: http.MethodPost, - contentType: "application/x-www-form-urlencoded", - body: []byte(`state=test-state&code=valid`), + name: "valid form_post", + method: http.MethodPost, + headers: map[string][]string{"Content-Type": {"application/x-www-form-urlencoded"}}, + body: []byte(`state=test-state&code=valid`), + wantHTTPStatus: http.StatusOK, opt: func(t *testing.T) Option { return func(h *handlerState) error { h.useFormPost = true @@ -1978,11 +2054,14 @@ func TestHandleAuthCodeCallback(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { + t.Parallel() + h := &handlerState{ callbacks: make(chan callbackResult, 1), state: state.State("test-state"), pkce: pkce.Code("test-pkce"), nonce: nonce.Nonce("test-nonce"), + logger: testlogger.New(t).Logger, } if tt.opt != nil { require.NoError(t, tt.opt(t)(h)) @@ -1998,8 +2077,8 @@ func TestHandleAuthCodeCallback(t *testing.T) { if tt.method != "" { req.Method = tt.method } - if tt.contentType != "" { - req.Header.Set("Content-Type", tt.contentType) + if tt.headers != nil { + req.Header = tt.headers } err = h.handleAuthCodeCallback(resp, req) @@ -2012,11 +2091,18 @@ func TestHandleAuthCodeCallback(t *testing.T) { } } else { require.NoError(t, err) + require.Equal(t, tt.wantHTTPStatus, resp.Code) + } + + if tt.wantHeaders != nil { + require.Equal(t, tt.wantHeaders, resp.Header()) } select { case <-time.After(1 * time.Second): - require.Fail(t, "timed out waiting to receive from callbacks channel") + if !tt.wantNoCallbacks { + require.Fail(t, "timed out waiting to receive from callbacks channel") + } case result := <-h.callbacks: if tt.wantErr != "" { require.EqualError(t, result.err, tt.wantErr) From 3c7e387137a880b3cffed05083e467ec6f4f6719 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 7 Feb 2022 13:32:31 -0800 Subject: [PATCH 2/3] Keep the CLI localhost listener running after requests with wrong verb Just in case some future browser change sends some new kind of request to our CLI, just ignore them by returning StatusMethodNotAllowed and continuing to listen. --- pkg/oidcclient/login.go | 10 +++++++--- pkg/oidcclient/login_test.go | 25 ++++++++++++++----------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 223e7fb2a..ca2335bcc 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -868,10 +868,12 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req // Return HTTP 405 for anything that's not a POST. if r.Method != http.MethodPost { - return httperr.Newf(http.StatusMethodNotAllowed, "wanted POST but got %s", r.Method) + h.logger.V(debugLogLevel).Info("Pinniped: Got unexpected request on callback listener", "method", r.Method) + w.WriteHeader(http.StatusMethodNotAllowed) + return nil // keep listening for more requests } - // Parse and pull the response parameters from a application/x-www-form-urlencoded request body. + // Parse and pull the response parameters from an application/x-www-form-urlencoded request body. if err := r.ParseForm(); err != nil { return httperr.Wrap(http.StatusBadRequest, "invalid form", err) } @@ -879,7 +881,9 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req } else { // Return HTTP 405 for anything that's not a GET. if r.Method != http.MethodGet { - return httperr.Newf(http.StatusMethodNotAllowed, "wanted GET but got %s", r.Method) + h.logger.V(debugLogLevel).Info("Pinniped: Got unexpected request on callback listener", "method", r.Method) + w.WriteHeader(http.StatusMethodNotAllowed) + return nil // keep listening for more requests } // Pull response parameters from the URL query string. diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index bae18b49b..a7c765c85 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -1881,19 +1881,19 @@ func TestHandleAuthCodeCallback(t *testing.T) { wantHeaders http.Header }{ { - name: "wrong method", - method: http.MethodPost, - query: "", - wantErr: "wanted GET but got POST", - wantHTTPStatus: http.StatusMethodNotAllowed, + name: "wrong method returns an error but keeps listening", + method: http.MethodPost, + query: "", + wantNoCallbacks: true, + wantHTTPStatus: http.StatusMethodNotAllowed, }, { - name: "wrong method for form_post", - method: http.MethodGet, - query: "", - opt: withFormPostMode, - wantErr: "wanted POST but got GET", - wantHTTPStatus: http.StatusMethodNotAllowed, + name: "wrong method for form_post returns an error but keeps listening", + method: http.MethodGet, + query: "", + opt: withFormPostMode, + wantNoCallbacks: true, + wantHTTPStatus: http.StatusMethodNotAllowed, }, { name: "invalid form for form_post", @@ -2098,6 +2098,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { require.Equal(t, tt.wantHeaders, resp.Header()) } + gotCallback := false select { case <-time.After(1 * time.Second): if !tt.wantNoCallbacks { @@ -2111,7 +2112,9 @@ func TestHandleAuthCodeCallback(t *testing.T) { require.NoError(t, result.err) require.NotNil(t, result.token) require.Equal(t, result.token.IDToken.Token, "test-id-token") + gotCallback = true } + require.Equal(t, tt.wantNoCallbacks, !gotCallback) }) } } From 6781bfd7d8d017190e83441c6365e8bbd0434888 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 7 Feb 2022 16:21:23 -0800 Subject: [PATCH 3/3] Fix JS bug: form post UI shows manual copy/paste UI upon failed callback When the POST to the CLI's localhost callback endpoint results in a non-2XX status code, then treat that as a failed login attempt and automatically show the manual copy/paste UI. --- internal/oidc/provider/formposthtml/form_post.js | 13 ++++++++++--- .../oidc/provider/formposthtml/formposthtml_test.go | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/oidc/provider/formposthtml/form_post.js b/internal/oidc/provider/formposthtml/form_post.js index 4c0eb7df2..57a187257 100644 --- a/internal/oidc/provider/formposthtml/form_post.js +++ b/internal/oidc/provider/formposthtml/form_post.js @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 window.onload = () => { @@ -48,7 +48,14 @@ window.onload = () => { headers: {'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8'}, body: responseParams['encoded_params'].value, }) - .then(() => clearTimeout(timeout)) - .then(() => transitionToState('success')) + .then(response => { + clearTimeout(timeout); + if (response.ok) { + transitionToState('success'); + } else { + // Got non-2XX http response status. + transitionToState('manual'); + } + }) .catch(() => transitionToState('manual')); }; diff --git a/internal/oidc/provider/formposthtml/formposthtml_test.go b/internal/oidc/provider/formposthtml/formposthtml_test.go index b09c0d7b3..0a6a30ec9 100644 --- a/internal/oidc/provider/formposthtml/formposthtml_test.go +++ b/internal/oidc/provider/formposthtml/formposthtml_test.go @@ -30,7 +30,7 @@ var ( - + @@ -61,7 +61,7 @@ var ( // It's okay if this changes in the future, but this gives us a chance to eyeball the formatting. // Our browser-based integration tests should find any incompatibilities. testExpectedCSP = `default-src 'none'; ` + - `script-src 'sha256-cjTdJmRvuz5EHNb/cw6pFk9iWyjegU9Ihx7Fb9tlqRg='; ` + + `script-src 'sha256-Lon+X41NoXuVGPqi3LsAPmBqlDmwbu3lGhQii7/Zjrc='; ` + `style-src 'sha256-CtfkX7m8x2UdGYvGgDq+6b6yIAQsASW9pbQK+sG8fNA='; ` + `img-src data:; ` + `connect-src *; ` +