diff --git a/hack/prepare-supervisor-on-kind.sh b/hack/prepare-supervisor-on-kind.sh index e02646f0..ba644388 100755 --- a/hack/prepare-supervisor-on-kind.sh +++ b/hack/prepare-supervisor-on-kind.sh @@ -442,8 +442,11 @@ else fi # Test that the federation domain is working before we proceed. -echo "Fetching FederationDomain discovery info via command: ${proxy_env_vars}curl -fLsS --cacert \"$root_ca_crt_path\" \"$issuer/.well-known/openid-configuration\"" -https_proxy="$proxy_server" no_proxy="$proxy_except" curl -fLsS --cacert "$root_ca_crt_path" "$issuer/.well-known/openid-configuration" | jq . +echo "Fetching FederationDomain discovery info via command:" \ + "${proxy_env_vars}curl -fLsS --retry-all-errors --retry 5 --cacert \"$root_ca_crt_path\" \"$issuer/.well-known/openid-configuration\"" +https_proxy="$proxy_server" no_proxy="$proxy_except" curl -fLsS \ + --retry-all-errors --retry 5 --cacert "$root_ca_crt_path" \ + "$issuer/.well-known/openid-configuration" | jq . if [[ "$OSTYPE" == "darwin"* ]]; then certificateAuthorityData=$(cat "$root_ca_crt_path" | base64) diff --git a/internal/federationdomain/formposthtml/form_post.css b/internal/federationdomain/formposthtml/form_post.css index 90f15b3b..13550ce0 100644 --- a/internal/federationdomain/formposthtml/form_post.css +++ b/internal/federationdomain/formposthtml/form_post.css @@ -67,6 +67,10 @@ code { background-image: url("data:image/svg+xml,%3Csvg version='1.1' width='36' height='36' viewBox='0 0 36 36' preserveAspectRatio='xMidYMid meet' xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink'%3E%3Ctitle%3Ecopy-to-clipboard-line%3C/title%3E%3Cpath d='M22.6,4H21.55a3.89,3.89,0,0,0-7.31,0H13.4A2.41,2.41,0,0,0,11,6.4V10H25V6.4A2.41,2.41,0,0,0,22.6,4ZM23,8H13V6.25A.25.25,0,0,1,13.25,6h2.69l.12-1.11A1.24,1.24,0,0,1,16.61,4a2,2,0,0,1,3.15,1.18l.09.84h2.9a.25.25,0,0,1,.25.25Z' class='clr-i-outline clr-i-outline-path-1'%3E%3C/path%3E%3Cpath d='M33.25,18.06H21.33l2.84-2.83a1,1,0,1,0-1.42-1.42L17.5,19.06l5.25,5.25a1,1,0,0,0,.71.29,1,1,0,0,0,.71-1.7l-2.84-2.84H33.25a1,1,0,0,0,0-2Z' class='clr-i-outline clr-i-outline-path-2'%3E%3C/path%3E%3Cpath d='M29,16h2V6.68A1.66,1.66,0,0,0,29.35,5H27.08V7H29Z' class='clr-i-outline clr-i-outline-path-3'%3E%3C/path%3E%3Cpath d='M29,31H7V7H9V5H6.64A1.66,1.66,0,0,0,5,6.67V31.32A1.66,1.66,0,0,0,6.65,33H29.36A1.66,1.66,0,0,0,31,31.33V22.06H29Z' class='clr-i-outline clr-i-outline-path-4'%3E%3C/path%3E%3Crect x='0' y='0' width='36' height='36' fill-opacity='0'/%3E%3C/svg%3E"); } +.error { + font-family: monospace; +} + @keyframes loader { to { transform: rotate(360deg); diff --git a/internal/federationdomain/formposthtml/form_post.gohtml b/internal/federationdomain/formposthtml/form_post.gohtml index 1fd0a70d..9117dc56 100644 --- a/internal/federationdomain/formposthtml/form_post.gohtml +++ b/internal/federationdomain/formposthtml/form_post.gohtml @@ -30,5 +30,10 @@ SPDX-License-Identifier: Apache-2.0 {{ .Parameters.Get "code" }} + diff --git a/internal/federationdomain/formposthtml/form_post.js b/internal/federationdomain/formposthtml/form_post.js index dcf86275..05b3bbea 100644 --- a/internal/federationdomain/formposthtml/form_post.js +++ b/internal/federationdomain/formposthtml/form_post.js @@ -2,7 +2,12 @@ // SPDX-License-Identifier: Apache-2.0 window.onload = () => { - const transitionToState = (id) => { + const transitionToState = (id, message) => { + // For the error state, there is also a message to show. + if (id === 'error') { + document.getElementById('message').innerText = message + } + // Hide all the other ".state"
s. Array.from(document.querySelectorAll('.state')).forEach(e => e.hidden = true); @@ -44,22 +49,31 @@ window.onload = () => { responseParams['redirect_uri'].value, { method: 'POST', - mode: 'no-cors', // in the future, we could change this to "cors" (see comment below) + mode: 'cors', // Using 'cors' is required to get actual response status codes. headers: {'Content-Type': 'application/x-www-form-urlencoded;charset=UTF-8'}, body: responseParams['encoded_params'].value, }) .then(response => { clearTimeout(timeout); - // Requests made using "no-cors" mode will hide the real response.status by making it 0 - // and the real response.ok by making it false. - // If the real response was success, then we would like to show the success state. - // If the real response was an error, then we wish we could do something else (maybe show the error?), - // but we have no way to know the real response as long as we are making "no-cors" requests. - // For now, show the success status for all responses. - // In the future, we could make this request in "cors" mode once old versions of our CLI - // which did not handle CORS are upgraded out by our users. That would allow us to use - // a conditional statement based on response.ok here to decide which state to transition into. - transitionToState('success'); + if (response.ok) { + // Got 2XX http response status, so the user has logged in successfully. + transitionToState('success'); + } else { + // Got non-2XX http response status. Show the error after trying to read the response body. + // These are not recoverable errors. The CLI stop listening and is no longer prompting for authcode. + response.text() + .then(function (text) { + transitionToState('error', response.status + ": " + text); + }) + .catch((reason) => { + console.error("error while reading response.text()", reason); + transitionToState('error', response.status + ": [could not read response body]"); + }) + } }) + // A network error is encountered or CORS is misconfigured on the server-side. + // This could happen in the case where the CLI is running on a different machine (e.g. ssh jumphost). + // This always happens in Safari because that browser always prevents an https (TLS) web site from making + // fetch calls to an http (non-TLS) localhost site (see https://bugs.webkit.org/show_bug.cgi?id=171934). .catch(() => transitionToState('manual')); }; diff --git a/internal/federationdomain/formposthtml/formposthtml_test.go b/internal/federationdomain/formposthtml/formposthtml_test.go index 3ab3a49a..db1fd02d 100644 --- a/internal/federationdomain/formposthtml/formposthtml_test.go +++ b/internal/federationdomain/formposthtml/formposthtml_test.go @@ -29,8 +29,8 @@ var ( - - + + @@ -54,6 +54,11 @@ var ( test-S629KHsCCBYV0PQ6FDSrn6iEXtVImQRBh7NCAk.JezyUSdCiSslYjtUmv7V5VAgiCz3ZkES9mYldg9GhqU
+ `) @@ -61,8 +66,8 @@ 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-uIWC0J7wd7tWtcXmugZCkKsQpqOsQzqBI/mfQMtUde0='; ` + - `style-src 'sha256-kXh6OrB2z7wkx7v1N3ay9deQhV5edwuogARaUtvNYN4='; ` + + `script-src 'sha256-fiAdxAQHPoodG4cbENki/1TI+cjBOXxw+ADCoCtepQo='; ` + + `style-src 'sha256-p+fPKq5SYyVeT46EkDVZx28MRQ6wlWHdDm3o3qZFGTA='; ` + `img-src data:; ` + `connect-src *; ` + `frame-ancestors 'none'` diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index f31592c2..73a9d729 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -922,7 +922,7 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req w.Header().Set("Access-Control-Allow-Origin", allowOrigin) w.Header().Set("Vary", "*") // supposed to use Vary when Access-Control-Allow-Origin is a specific host } else { - // Return HTTP 405 for anything that's not a GET. + // When we are not using form_post, then return HTTP 405 for anything that's not a GET. if r.Method != http.MethodGet { h.logger.V(plog.KlogLevelDebug).Info("Pinniped: Got unexpected request on callback listener", "method", r.Method) w.WriteHeader(http.StatusMethodNotAllowed) @@ -933,6 +933,9 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req params = r.URL.Query() } + // At this point, it doesn't matter if we got the params from a form_post POST request or a regular GET request. + // Next, validate the params, and if we got an authcode then try to use it to complete the login. + // Validate OAuth2 state and fail if it's incorrect (to block CSRF). if err := h.state.Validate(params.Get("state")); err != nil { return httperr.New(http.StatusForbidden, "missing or invalid state parameter") diff --git a/test/integration/formposthtml_test.go b/test/integration/formposthtml_test.go index 9056e065..bbfb8c05 100644 --- a/test/integration/formposthtml_test.go +++ b/test/integration/formposthtml_test.go @@ -48,25 +48,15 @@ func TestFormPostHTML_Browser_Parallel(t *testing.T) { t.Run("callback server error", func(t *testing.T) { browser := browsertest.OpenBrowser(t) - // Serve the form_post template with a redirect URI that will return an HTTP 500 response. + // Serve the form_post template with a redirect URI that will return an HTTP 400 response. responseParams := formpostRandomParams(t) - formpostInitiate(t, browser, formpostTemplateServer(t, callbackURL+"?fail=500", responseParams)) + formpostInitiate(t, browser, formpostTemplateServer(t, callbackURL+"?fail=400", responseParams)) // Now we handle the callback and assert that we got what we expected. expectCallback(t, responseParams) - // This is not 100% the behavior we'd like, but because our JS is making - // a cross-origin fetch() without CORS, we don't get to know anything - // about the response (even whether it is 200 vs. 500), so this case - // is the same as the success case. - // - // This case is fairly unlikely in practice, and if the CLI encounters - // an error it can also expose it via stderr anyway. - // - // In the future, we could change the Javascript code to use mode 'cors' - // because we have upgraded our CLI callback endpoint to handle CORS, - // and then we could change this to formpostExpectManualState(). - formpostExpectSuccessState(t, browser) + // This failure should cause the UI to enter the "error" state. + formpostExpectErrorState(t, browser) }) t.Run("network failure", func(t *testing.T) { @@ -113,7 +103,7 @@ func TestFormPostHTML_Browser_Parallel(t *testing.T) { // It returns the URL of the running test server and a function for fetching the next // received form POST parameters. // -// The test server supports special `?fail=close` and `?fail=500` to force error cases. +// The test server supports special `?fail=close` and `?fail=400` to force error cases. func formpostCallbackServer(t *testing.T) (string, func(*testing.T, url.Values)) { t.Helper() results := make(chan url.Values) @@ -156,8 +146,9 @@ func formpostCallbackServer(t *testing.T) (string, func(*testing.T, url.Values)) _ = conn.Close() } return - case "500": // If "fail=500" is passed, return a 500 error. - w.WriteHeader(http.StatusInternalServerError) + case "400": // If "fail=400" is passed, return a 400 error. + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte("this is the text of the bad request error response")) return } })) @@ -265,6 +256,19 @@ func formpostExpectSuccessState(t *testing.T, b *browsertest.Browser) { formpostExpectFavicon(t, b, "✅") } +// formpostExpectErrorState asserts that the page is in the "error" state. +func formpostExpectErrorState(t *testing.T, b *browsertest.Browser) { + t.Helper() + t.Logf("expecting to see error message become visible...") + b.WaitForVisibleElements(t, "div#error") + errorDivText := b.TextOfFirstMatch(t, "div#error") + require.Contains(t, errorDivText, "Error during login") + require.Contains(t, errorDivText, "400: this is the text of the bad request error response") + require.Contains(t, errorDivText, "Please try again.") + require.Equal(t, "Error during login", b.Title(t)) + formpostExpectFavicon(t, b, "⛔") +} + // formpostExpectManualState asserts that the page is in the "manual" state and returns the auth code. func formpostExpectManualState(t *testing.T, b *browsertest.Browser) string { t.Helper()