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()) + } +}