fix(sso): keep the OIDC authorize request when Kerberos SSO fails

internal/handler/auth.go

@@ -1358,29 +1358,35 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) {
 		keytabPath := h.getKeytabPath()
 		if keytabPath == "" {
 			log.Printf("[sso] Kerberos not configured ip=%s", ip)
-			h.redirectToLoginError(w, r, redirectURI, "Kerberos not configured")
+			h.ssoLoginError(w, r, redirectURI, "Kerberos not configured")
 			return
 		}
 
 		// If we already sent the challenge (sso_attempt=1) and the browser
 		// still didn't respond with a Negotiate token, SSO has failed.
 		if r.URL.Query().Get("sso_attempt") == "1" {
 			log.Printf("[sso] Browser did not provide Kerberos credentials ip=%s", ip)
-			h.redirectToLoginError(w, r, redirectURI, "SSO authentication failed — your browser did not provide Kerberos credentials")
+			h.ssoLoginError(w, r, redirectURI, "SSO authentication failed — your browser did not provide Kerberos credentials")
 			return
 		}
 
 		log.Printf("[sso] Sending Negotiate challenge ip=%s", ip)
-		// First visit — send the 401 challenge
-		retryURL := h.url("/login/sso") + "?sso_attempt=1"
-		if redirectURI != "" {
-			retryURL += "&redirect_uri=" + url.QueryEscape(redirectURI)
+		// First visit — send the 401 challenge. The retry must carry the same
+		// request (client_id, oidc and the whole authorize request): with only
+		// redirect_uri, the retry resolved the default app and an OIDC failure lost
+		// its state, nonce and PKCE challenge on the way back.
+		retry := parseOIDCAuthzRequest(r).values()
+		if r.URL.Query().Get("oidc") == "1" {
+			retry.Set("oidc", "1")
 		}
+		retry.Set("sso_attempt", "1")
+		retryURL := h.url("/login/sso") + "?" + retry.Encode()
 		w.Header().Set("WWW-Authenticate", "Negotiate")
 		w.WriteHeader(http.StatusUnauthorized)
 		// Redirect to self with sso_attempt=1 so we can detect failure
-		// retryURL is built server-side from h.url() + url.QueryEscape'd values —
-		// it is never echoed from the request. Escaped here for defence in depth.
+		// retryURL is built server-side from h.url() + the typed oidcAuthzRequest
+		// allowlist, every value percent-encoded by url.Values.Encode — no raw
+		// request text reaches it. Escaped here for defence in depth.
 		// NOTE: html/template treats <meta content> as contentTypeUnsafe (it
 		// attribute-escapes but does NOT URL-filter), so a template rewrite would
 		// not add a guarantee here; the safety rests on the server-side
@@ -1394,33 +1400,33 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) {
 	tokenB64 := authHeader[10:]
 	tokenBytes, err := base64.StdEncoding.DecodeString(tokenB64)
 	if err != nil {
-		h.redirectToLoginError(w, r, redirectURI, "Invalid Negotiate token")
+		h.ssoLoginError(w, r, redirectURI, "Invalid Negotiate token")
 		return
 	}
 
 	if isNTLMToken(tokenBytes) {
 		log.Printf("[sso] NTLM token rejected (Kerberos required) ip=%s", ip)
-		h.redirectToLoginError(w, r, redirectURI, "NTLM is not supported, Kerberos required")
+		h.ssoLoginError(w, r, redirectURI, "NTLM is not supported, Kerberos required")
 		return
 	}
 
 	keytabPath := h.getKeytabPath()
 	if keytabPath == "" {
-		h.redirectToLoginError(w, r, redirectURI, "Kerberos not configured")
+		h.ssoLoginError(w, r, redirectURI, "Kerberos not configured")
 		return
 	}
 
 	kt, err := keytab.Load(keytabPath)
 	if err != nil {
-		h.redirectToLoginError(w, r, redirectURI, "Kerberos configuration error")
+		h.ssoLoginError(w, r, redirectURI, "Kerberos configuration error")
 		return
 	}
 
 	// Extract username from SPNEGO/Kerberos token
 	username, err := h.extractKerberosUsername(tokenBytes, kt)
 	if err != nil {
 		log.Printf("[sso] Kerberos auth failed: %v", err)
-		h.redirectToLoginError(w, r, redirectURI, "Kerberos authentication failed")
+		h.ssoLoginError(w, r, redirectURI, "Kerberos authentication failed")
 		return
 	}
 
@@ -1429,32 +1435,32 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		log.Printf("[sso] User resolution failed for %q: %v", username, err)
 		if errors.Is(err, auth.ErrLDAPAccountDisabled) {
-			h.redirectToLoginError(w, r, redirectURI, "Account disabled")
+			h.ssoLoginError(w, r, redirectURI, "Account disabled")
 			return
 		}
-		h.redirectToLoginError(w, r, redirectURI, "User not found in directory")
+		h.ssoLoginError(w, r, redirectURI, "User not found in directory")
 		return
 	}
 
 	user, err := h.store.ResolveUser(userGUID)
 	if err != nil {
-		h.redirectToLoginError(w, r, redirectURI, "User not found")
+		h.ssoLoginError(w, r, redirectURI, "User not found")
 		return
 	}
 	if user.Disabled {
-		h.redirectToLoginError(w, r, redirectURI, "Account disabled")
+		h.ssoLoginError(w, r, redirectURI, "Account disabled")
 		return
 	}
 
 	// app was resolved + redirect validated at the top of the handler (L1).
 	roles, perms, denied := h.resolveTokenRoles(app, user)
 	if denied {
-		h.redirectToLoginError(w, r, redirectURI, "Access denied: not assigned to this app")
+		h.ssoLoginError(w, r, redirectURI, "Access denied: not assigned to this app")
 		return
 	}
 	accessToken, refreshToken, expiresIn, err := h.issueTokenPair(user, roles, perms, ldapGroups, app)
 	if err != nil {
-		h.redirectToLoginError(w, r, redirectURI, "Token generation failed")
+		h.ssoLoginError(w, r, redirectURI, "Token generation failed")
 		return
 	}
 
@@ -1476,7 +1482,7 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) {
 
 		codeBytes := make([]byte, 32)
 		if _, err := rand.Read(codeBytes); err != nil {
-			h.redirectToLoginError(w, r, redirectURI, "Internal error")
+			h.ssoLoginError(w, r, redirectURI, "Internal error")
 			return
 		}
 		code := hex.EncodeToString(codeBytes)
@@ -1493,7 +1499,7 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) {
 			CreatedAt:           time.Now(),
 		}
 		if err := h.store.SaveOIDCAuthCode(ac); err != nil {
-			h.redirectToLoginError(w, r, redirectURI, "Internal error")
+			h.ssoLoginError(w, r, redirectURI, "Internal error")
 			return
 		}
 
@@ -1527,6 +1533,21 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, h.url("/account")+"#"+fragment, http.StatusFound)
 }
 
+// ssoLoginError ends a failed /login/sso attempt. An OIDC attempt (oidc=1)
+// returns to SimpleAuth's own authorize page with the whole authorize request,
+// exactly like a failed password attempt (renderOIDCLoginError), so the user can
+// fall back to their password and the client still receives its state, nonce and
+// PKCE challenge. Redirecting to the client's redirect_uri with only ?error=
+// dropped state, which OIDC clients reject as a CSRF mismatch before the error is
+// ever shown. Non-OIDC flows keep redirectToLoginError.
+func (h *Handler) ssoLoginError(w http.ResponseWriter, r *http.Request, redirectURI, msg string) {
+	if r.URL.Query().Get("oidc") == "1" {
+		h.renderOIDCLoginError(w, r, msg)
+		return
+	}
+	h.redirectToLoginError(w, r, redirectURI, msg)
+}
+
 // extractKerberosUsername validates a SPNEGO/Kerberos token (full AP-REQ
 // verification — authenticator, clock skew, replay cache; C1) and returns the
 // realm-stripped username.

internal/handler/sso_failure_test.go

@@ -0,0 +1,143 @@
+package handler
+
+import (
+	"html"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+var metaRefreshURL = regexp.MustCompile(`content="0;url=([^"]+)"`)
+
+func ssoGet(h *Handler, target string) *httptest.ResponseRecorder {
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, httptest.NewRequest("GET", target, nil))
+	return rec
+}
+
+func oidcSSOQuery(appID, cb string) url.Values {
+	q := url.Values{}
+	q.Set("oidc", "1")
+	q.Set("client_id", appID)
+	q.Set("redirect_uri", cb)
+	q.Set("state", "the-state")
+	q.Set("nonce", "the-nonce")
+	q.Set("scope", "openid email")
+	q.Set("code_challenge", s256("v"))
+	q.Set("code_challenge_method", "S256")
+	return q
+}
+
+// negotiateRetryURL sends the first, header-less /login/sso request and returns
+// the meta-refresh URL a browser without Kerberos credentials follows.
+func negotiateRetryURL(t *testing.T, h *Handler, q url.Values) *url.URL {
+	t.Helper()
+	rec := ssoGet(h, "/login/sso?"+q.Encode())
+	if rec.Code != http.StatusUnauthorized || rec.Header().Get("WWW-Authenticate") != "Negotiate" {
+		t.Fatalf("expected a 401 Negotiate challenge, got %d %q", rec.Code, rec.Header().Get("WWW-Authenticate"))
+	}
+	m := metaRefreshURL.FindStringSubmatch(rec.Body.String())
+	if m == nil {
+		t.Fatalf("challenge body has no meta-refresh retry: %s", rec.Body.String())
+	}
+	u, err := url.Parse(html.UnescapeString(m[1]))
+	if err != nil {
+		t.Fatalf("parse retry URL: %v", err)
+	}
+	return u
+}
+
+func withKeytab(t *testing.T, h *Handler) {
+	t.Helper()
+	h.cfg.KRB5Keytab = filepath.Join(t.TempDir(), "krb5.keytab")
+}
+
+// The retry used to keep only redirect_uri, so it resolved the default app and an
+// OIDC failure lost state, nonce and the PKCE challenge on the way back.
+func TestSSONegotiateRetryKeepsAuthorizeRequest(t *testing.T) {
+	h, _ := testSetup(t)
+	withKeytab(t, h)
+	cb := mkPKCEApp(t, h, "shop9")
+	sent := oidcSSOQuery("shop9", cb)
+
+	retry := negotiateRetryURL(t, h, sent).Query()
+	for _, k := range []string{"oidc", "client_id", "redirect_uri", "state", "nonce", "scope", "code_challenge", "code_challenge_method"} {
+		if retry.Get(k) != sent.Get(k) {
+			t.Errorf("retry dropped or altered %q: want %q, got %q", k, sent.Get(k), retry.Get(k))
+		}
+	}
+	if retry.Get("sso_attempt") != "1" {
+		t.Error("retry must mark sso_attempt=1 so the failure is detected")
+	}
+}
+
+// A failed OIDC SSO attempt must come back to SimpleAuth's authorize page with the
+// whole request — not to the client's redirect_uri with a bare ?error=, which has
+// no state and is rejected by OIDC clients as a CSRF mismatch.
+func TestSSOFailureReturnsToAuthorizePageWithState(t *testing.T) {
+	h, _ := testSetup(t)
+	withKeytab(t, h)
+	cb := mkPKCEApp(t, h, "shop10")
+	sent := oidcSSOQuery("shop10", cb)
+
+	retry := negotiateRetryURL(t, h, sent)
+	rec := ssoGet(h, retry.RequestURI())
+	if rec.Code != http.StatusFound {
+		t.Fatalf("failed SSO must redirect, got %d %s", rec.Code, rec.Body.String())
+	}
+	lu, err := url.Parse(rec.Header().Get("Location"))
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if strings.HasPrefix(lu.String(), cb) || !strings.Contains(lu.Path, "/protocol/openid-connect/auth") {
+		t.Fatalf("failed SSO must return to SimpleAuth's authorize page, got %q", lu.String())
+	}
+	got := lu.Query()
+	for _, k := range []string{"client_id", "redirect_uri", "state", "nonce", "scope", "code_challenge", "code_challenge_method"} {
+		if got.Get(k) != sent.Get(k) {
+			t.Errorf("failure redirect dropped or altered %q: want %q, got %q", k, sent.Get(k), got.Get(k))
+		}
+	}
+	if got.Get("error") == "" {
+		t.Error("failure redirect must carry the error message for the login page")
+	}
+}
+
+func TestSSOFailureWithoutKerberosKeepsAuthorizeRequest(t *testing.T) {
+	h, _ := testSetup(t)
+	cb := mkPKCEApp(t, h, "shop11")
+	sent := oidcSSOQuery("shop11", cb)
+
+	rec := ssoGet(h, "/login/sso?"+sent.Encode())
+	lu, _ := url.Parse(rec.Header().Get("Location"))
+	if rec.Code != http.StatusFound || !strings.Contains(lu.Path, "/protocol/openid-connect/auth") {
+		t.Fatalf("expected a redirect to the authorize page, got %d %q", rec.Code, lu)
+	}
+	if lu.Query().Get("state") != "the-state" || lu.Query().Get("code_challenge") != sent.Get("code_challenge") {
+		t.Errorf("Kerberos-not-configured failure dropped the authorize request: %q", lu.RawQuery)
+	}
+}
+
+// Without oidc=1 the hosted flow keeps its redirect to the app with ?error=. The
+// retry now carries client_id, so an app whose redirect_uri is only in its own
+// allowlist gets that error instead of a 400 from the default app.
+func TestSSOFailureHostedFlowStillRedirectsToApp(t *testing.T) {
+	h, _ := testSetup(t)
+	withKeytab(t, h)
+	cb := mkPKCEApp(t, h, "shop12")
+	sent := url.Values{"client_id": {"shop12"}, "redirect_uri": {cb}}
+
+	retry := negotiateRetryURL(t, h, sent)
+	if retry.Query().Get("client_id") != "shop12" {
+		t.Fatalf("hosted retry dropped client_id: %q", retry.RawQuery)
+	}
+	rec := ssoGet(h, retry.RequestURI())
+	loc := rec.Header().Get("Location")
+	if rec.Code != http.StatusFound || !strings.HasPrefix(loc, cb+"?error=") {
+		t.Fatalf("hosted SSO failure must redirect to the app with ?error=, got %d %q %s", rec.Code, loc, rec.Body.String())
+	}
+}