vmware-tanzu · cfryanr · Oct 4, 2023 · Sep 28, 2023
diff --git 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
@@ -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
@@ -30,5 +30,10 @@ SPDX-License-Identifier: Apache-2.0
         <code id="manual-auth-code">{{ .Parameters.Get "code" }}</code>
     </button>
 </div>
+<div id="error" class="state" data-favicon="⛔" data-title="Error during login" hidden>
+    <h1>Error during login</h1>
+    <p id="message" class="error"></p>
+    <p>Please try again.</p>
+</div>
 </body>
 </html>
diff --git 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" <div>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
@@ -29,8 +29,8 @@ var (
         <html lang="en">
         <head>
             <meta charset="UTF-8">
-            <style>body{font-family:metropolis-light,Helvetica,sans-serif}h1{font-size:20px}.state{position:absolute;top:100px;left:50%;width:400px;height:80px;margin-top:-40px;margin-left:-200px;font-size:14px;line-height:24px}button{margin:-10px;padding:10px;text-align:left;width:100%;display:inline;border:none;background:0 0;cursor:pointer;transition:all .1s}button:hover{background-color:#eee;transform:scale(1.01)}button:active{background-color:#ddd;transform:scale(.99)}code{display:block;word-wrap:break-word;word-break:break-all;font-size:12px;font-family:monospace;color:#333}.copy-icon{float:left;width:36px;height:36px;margin-top:-3px;margin-right:10px;background-size:contain;background-repeat:no-repeat;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")}@keyframes loader{to{transform:rotate(360deg)}}#loading{content:'';box-sizing:border-box;width:80px;height:80px;margin-top:-40px;margin-left:-40px;border-radius:50%;border:2px solid #fff;border-top-color:#1b3951;animation:loader .6s linear infinite}</style>
-            <script>window.onload=()=>{const e=e=>{Array.from(document.querySelectorAll(".state")).forEach(e=>e.hidden=!0);const t=document.getElementById(e);t.hidden=!1,document.title=t.dataset.title,document.getElementById("favicon").setAttribute("href","data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 100 100%22><text y=%22.9em%22 font-size=%2290%22>"+t.dataset.favicon+"</text></svg>")};e("loading"),window.history.replaceState(null,"","./"),document.getElementById("manual-copy-button").onclick=()=>{const e=document.getElementById("manual-copy-button").innerText;navigator.clipboard.writeText(e).then(()=>console.info("copied authorization code "+e+" to clipboard")).catch(t=>console.error("failed to copy code "+e+" to clipboard: "+t))};const n=setTimeout(()=>e("manual"),2e3),t=document.forms[0].elements;fetch(t.redirect_uri.value,{method:"POST",mode:"no-cors",headers:{"Content-Type":"application/x-www-form-urlencoded;charset=UTF-8"},body:t.encoded_params.value}).then(t=>{clearTimeout(n),e("success")}).catch(()=>e("manual"))}</script>
+            <style>body{font-family:metropolis-light,Helvetica,sans-serif}h1{font-size:20px}.state{position:absolute;top:100px;left:50%;width:400px;height:80px;margin-top:-40px;margin-left:-200px;font-size:14px;line-height:24px}button{margin:-10px;padding:10px;text-align:left;width:100%;display:inline;border:none;background:0 0;cursor:pointer;transition:all .1s}button:hover{background-color:#eee;transform:scale(1.01)}button:active{background-color:#ddd;transform:scale(.99)}code{display:block;word-wrap:break-word;word-break:break-all;font-size:12px;font-family:monospace;color:#333}.copy-icon{float:left;width:36px;height:36px;margin-top:-3px;margin-right:10px;background-size:contain;background-repeat:no-repeat;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)}}#loading{content:'';box-sizing:border-box;width:80px;height:80px;margin-top:-40px;margin-left:-40px;border-radius:50%;border:2px solid #fff;border-top-color:#1b3951;animation:loader .6s linear infinite}</style>
+            <script>window.onload=()=>{const e=(e,t)=>{e==="error"&&(document.getElementById("message").innerText=t),Array.from(document.querySelectorAll(".state")).forEach(e=>e.hidden=!0);const n=document.getElementById(e);n.hidden=!1,document.title=n.dataset.title,document.getElementById("favicon").setAttribute("href","data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 100 100%22><text y=%22.9em%22 font-size=%2290%22>"+n.dataset.favicon+"</text></svg>")};e("loading"),window.history.replaceState(null,"","./"),document.getElementById("manual-copy-button").onclick=()=>{const e=document.getElementById("manual-copy-button").innerText;navigator.clipboard.writeText(e).then(()=>console.info("copied authorization code "+e+" to clipboard")).catch(t=>console.error("failed to copy code "+e+" to clipboard: "+t))};const n=setTimeout(()=>e("manual"),2e3),t=document.forms[0].elements;fetch(t.redirect_uri.value,{method:"POST",mode:"cors",headers:{"Content-Type":"application/x-www-form-urlencoded;charset=UTF-8"},body:t.encoded_params.value}).then(t=>{clearTimeout(n),t.ok?e("success"):t.text().then(function(n){e("error",t.status+": "+n)}).catch(n=>{console.error("error while reading response.text()",n),e("error",t.status+": [could not read response body]")})}).catch(()=>e("manual"))}</script>
             <link id="favicon" rel="icon"/>
         </head>
         <body>
@@ -54,15 +54,20 @@ var (
                 <code id="manual-auth-code">test-S629KHsCCBYV0PQ6FDSrn6iEXtVImQRBh7NCAk.JezyUSdCiSslYjtUmv7V5VAgiCz3ZkES9mYldg9GhqU</code>
             </button>
         </div>
+        <div id="error" class="state" data-favicon="⛔" data-title="Error during login" hidden>
+            <h1>Error during login</h1>
+            <p id="message" class="error"></p>
+            <p>Please try again.</p>
+        </div>
         </body>
         </html>
 	`)
 
 	// 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
@@ -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
@@ -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()