From d15ba7d216c440aefb6298a218ea450f7b712273 Mon Sep 17 00:00:00 2001 From: Your Name Date: Fri, 9 Oct 2026 22:49:46 +0300 Subject: [PATCH] fix(sso): keep the OIDC authorize request when Kerberos SSO fails Two hops of the /login/sso flow dropped the authorize request that oidcAuthzRequest exists to carry: - The Negotiate fallback. A browser that cannot answer the 401 challenge follows the page's meta-refresh, whose URL kept only redirect_uri. client_id, oidc, state, nonce and the PKCE challenge were lost, so the retry resolved the DEFAULT app: an app whose redirect_uri is only in its own allowlist got 400 "redirect_uri not allowed", and an OIDC attempt continued as a non-OIDC one. - The failure redirect. Every /login/sso failure called redirectToLoginError, which sends the browser to the client's redirect_uri with only ?error=. That callback has no state, so OIDC clients reject it as a CSRF/state mismatch and the user never sees the real reason or gets a chance to use their password. The retry URL is now built from the same typed oidcAuthzRequest allowlist (plus oidc and sso_attempt), every value percent-encoded -- still no raw request text in the meta-refresh. Failures in an OIDC attempt go through renderOIDCLoginError, exactly like a failed password: back to SimpleAuth's authorize page with the error banner and the whole request, so the user can fall back to their password and the client still receives state, nonce and the PKCE challenge. Non-OIDC (hosted) flows keep redirecting to the app with ?error=, now for the right app. Tests fail on master and pass here: the retry keeps every authorize parameter, an OIDC failure returns to the authorize page with state and PKCE, a Kerberos-not-configured failure does the same, and the hosted flow still redirects an app-allowlisted redirect_uri with ?error=. Verified against a Samba AD DC behind nginx: a browser without Kerberos lands on the login page with the reason shown, and the AD password then completes the flow with code and state. Co-Authored-By: Claude Opus 5.5 --- internal/handler/auth.go | 63 ++++++++---- internal/handler/sso_failure_test.go | 143 +++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 21 deletions(-) create mode 100644 internal/handler/sso_failure_test.go diff --git a/internal/handler/auth.go b/internal/handler/auth.go index a0e5048..c5241d5 100644 --- a/internal/handler/auth.go +++ b/internal/handler/auth.go @@ -1358,7 +1358,7 @@ 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 } @@ -1366,21 +1366,27 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) { // 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 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,25 +1400,25 @@ 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 } @@ -1420,7 +1426,7 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) { 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. diff --git a/internal/handler/sso_failure_test.go b/internal/handler/sso_failure_test.go new file mode 100644 index 0000000..ca227ae --- /dev/null +++ b/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()) + } +}