diff --git a/enterprise/internal/loginhandler/loginhandler.go b/enterprise/internal/loginhandler/loginhandler.go index 6a78144..e147ab8 100644 --- a/enterprise/internal/loginhandler/loginhandler.go +++ b/enterprise/internal/loginhandler/loginhandler.go @@ -157,7 +157,7 @@ func (h *Handler) handleOIDCLogin(w http.ResponseWriter, r *http.Request) { } http.SetCookie(w, &http.Cookie{ Name: oidcStateCookieName, Value: state, Path: "/auth/oidc/callback", - HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + HttpOnly: true, Secure: isSecureRequest(r), SameSite: http.SameSiteLaxMode, MaxAge: int(loginCookieTTL.Seconds()), }) http.Redirect(w, r, h.oidc.AuthCodeURL(state), http.StatusFound) @@ -170,11 +170,30 @@ func (h *Handler) handleOIDCLogin(w http.ResponseWriter, r *http.Request) { func clearCookie(w http.ResponseWriter, r *http.Request, name, path string) { http.SetCookie(w, &http.Cookie{ Name: name, Value: "", Path: path, - HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + HttpOnly: true, Secure: isSecureRequest(r), SameSite: http.SameSiteLaxMode, MaxAge: -1, }) } +// isSecureRequest decides every cookie's Secure attribute in this file. +// r.TLS != nil alone is wrong for the deployment shape this handler +// actually runs in: enterprise-auth doesn't terminate TLS itself (see +// its own README/Dockerfile -- it's a plain http.Server, same as every +// other service here), so in any real deployment TLS is terminated at a +// reverse proxy/ingress in front of it, and r.TLS is nil at this process +// even though the original client connection was HTTPS. Confirmed live: +// the SAML ACS cookie (SameSite=None, which the cookie spec requires to +// be paired with Secure) came back without Secure behind a real +// TLS-terminating nginx proxy, and Chrome silently drops such a cookie +// -- breaking the entire SAML flow, not just weakening it. Trusting +// X-Forwarded-Proto here doesn't introduce a new trust boundary: this +// handler already trusts its network position (it's meant to sit behind +// exactly this kind of proxy, never directly internet-facing -- see +// /docs/security/threat-model.md's deployment/network assumptions). +func isSecureRequest(r *http.Request) bool { + return r.TLS != nil || r.Header.Get("X-Forwarded-Proto") == "https" +} + func (h *Handler) handleOIDCCallback(w http.ResponseWriter, r *http.Request) { defer clearCookie(w, r, oidcStateCookieName, "/auth/oidc/callback") @@ -238,7 +257,7 @@ func (h *Handler) handleSAMLLogin(w http.ResponseWriter, r *http.Request) { // login will not work correctly over plain HTTP. http.SetCookie(w, &http.Cookie{ Name: samlRequestCookieName, Value: requestID, Path: "/auth/saml/acs", - HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteNoneMode, + HttpOnly: true, Secure: isSecureRequest(r), SameSite: http.SameSiteNoneMode, MaxAge: int(loginCookieTTL.Seconds()), }) http.Redirect(w, r, redirectURL, http.StatusFound) @@ -303,7 +322,7 @@ func (h *Handler) issueSessionAndRedirect(w http.ResponseWriter, r *http.Request } http.SetCookie(w, &http.Cookie{ Name: authhandler.SessionCookieName, Value: token, Path: "/", - HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + HttpOnly: true, Secure: isSecureRequest(r), SameSite: http.SameSiteLaxMode, MaxAge: int(session.HumanSessionTTL.Seconds()), }) http.Redirect(w, r, h.postLoginRedirectURL, http.StatusFound) @@ -323,7 +342,7 @@ func (h *Handler) startTenantSelection(w http.ResponseWriter, r *http.Request, u } http.SetCookie(w, &http.Cookie{ Name: pendingLoginCookieName, Value: token, Path: "/auth", - HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + HttpOnly: true, Secure: isSecureRequest(r), SameSite: http.SameSiteLaxMode, MaxAge: int(session.PendingLoginTTL.Seconds()), }) http.Redirect(w, r, h.selectTenantRedirectURL, http.StatusFound) @@ -433,7 +452,7 @@ func (h *Handler) handleSelectTenant(w http.ResponseWriter, r *http.Request) { } http.SetCookie(w, &http.Cookie{ Name: authhandler.SessionCookieName, Value: token, Path: "/", - HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, + HttpOnly: true, Secure: isSecureRequest(r), SameSite: http.SameSiteLaxMode, MaxAge: int(session.HumanSessionTTL.Seconds()), }) w.Header().Set("Content-Type", "application/json") diff --git a/enterprise/internal/loginhandler/saml_test.go b/enterprise/internal/loginhandler/saml_test.go index 59a0ac6..badfaf5 100644 --- a/enterprise/internal/loginhandler/saml_test.go +++ b/enterprise/internal/loginhandler/saml_test.go @@ -267,6 +267,43 @@ func TestHandleSAMLLoginRedirectsAndSetsRequestCookie(t *testing.T) { if requestCookie.SameSite != http.SameSiteNoneMode { t.Fatalf("SameSite = %v, want SameSiteNoneMode -- the acs POST is cross-site from the idp's origin", requestCookie.SameSite) } + if requestCookie.Secure { + t.Fatal("expected Secure=false for a plain-HTTP request with no X-Forwarded-Proto") + } +} + +// TestHandleSAMLLoginSetsSecureCookieBehindATLSProxy is the regression +// test for a real bug found running this against an actual +// TLS-terminating nginx proxy: r.TLS is nil at this process in that +// topology even though the original client connection was HTTPS, so the +// SameSite=None request cookie above came back without Secure -- +// which Chrome silently drops, since the cookie spec requires +// SameSite=None to be paired with Secure. enterprise-auth never +// terminates TLS itself (see isSecureRequest's doc comment), so this is +// the deployment shape that actually matters, not an edge case. +func TestHandleSAMLLoginSetsSecureCookieBehindATLSProxy(t *testing.T) { + idp := newTestSAMLIdP(t) + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, idp.serviceProvider(t), newTestSessionManager(t), newFakeUserStore(), "http://web/", "http://web/select-tenant") + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + req := httptest.NewRequest(http.MethodGet, "/auth/saml/login", nil) + req.Header.Set("X-Forwarded-Proto", "https") + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + var requestCookie *http.Cookie + for _, c := range rec.Result().Cookies() { + if c.Name == samlRequestCookieName { + requestCookie = c + } + } + if requestCookie == nil { + t.Fatal("expected a saml request cookie to be set") + } + if !requestCookie.Secure { + t.Fatal("expected Secure=true when X-Forwarded-Proto: https is present") + } } func TestFullSAMLLoginFlowIssuesSessionForSingleMembership(t *testing.T) {