Honor X-Forwarded-Proto for cookie Secure, not just r.TLS
Every auth cookie loginhandler.go sets (OIDC state, SAML request, pending-login, session) decided Secure from r.TLS != nil alone -- correct only if enterprise-auth terminates TLS itself, which it never does (it's a plain http.Server, same as every other service here). In any real deployment, TLS is terminated at a reverse proxy/ingress in front of it, so r.TLS is nil at this process even over a genuinely HTTPS client connection. Found live: SAML's request-tracking cookie is SameSite=None (required, since the ACS POST is cross-site from the IdP's origin), which the cookie spec requires to be paired with Secure. Behind a real TLS-terminating nginx proxy, the cookie came back without Secure and Chrome silently dropped it -- breaking the SAML login flow entirely, not just weakening it. Fixed with isSecureRequest(r), which also checks X-Forwarded-Proto: https -- not a new trust boundary, since this handler already assumes it sits behind exactly this kind of proxy, never directly internet-facing.
This commit is contained in:
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user