diff --git a/modules/caddyhttp/caddyauth/caddyauth.go b/modules/caddyhttp/caddyauth/caddyauth.go index 30bcdf66b..f3becb268 100644 --- a/modules/caddyhttp/caddyauth/caddyauth.go +++ b/modules/caddyhttp/caddyauth/caddyauth.go @@ -15,7 +15,12 @@ package caddyauth import ( + "bufio" + "bytes" "fmt" + "io" + "maps" + "net" "net/http" "go.uber.org/zap" @@ -88,8 +93,26 @@ func (a Authentication) ServeHTTP(w http.ResponseWriter, r *http.Request, next c var hasCandidate bool var authed bool var err error + + // With a single provider there is nothing to isolate it from, so it is + // given the real ResponseWriter directly — no buffering, and every writer + // capability (Flusher, Hijacker, Pusher, ReaderFrom, ...) is preserved + // exactly as before. Only when MULTIPLE providers are configured can a + // failing provider's response clobber another provider's or the successful + // handler chain's, so there each provider gets its own bounded buffered + // writer. See https://github.com/caddyserver/caddy/issues/5190. + isolate := len(a.Providers) > 1 + + var winner *bufferedResponseWriter + var failed []*bufferedResponseWriter for provName, prov := range a.Providers { - user, authed, err = prov.Authenticate(w, r) + pw := w + var bw *bufferedResponseWriter + if isolate { + bw = newBufferedResponseWriter(w) + pw = bw + } + user, authed, err = prov.Authenticate(pw, r) if err != nil { if c := a.logger.Check(zapcore.ErrorLevel, "auth provider returned error"); c != nil { c.Write(zap.String("provider", provName), zap.Error(err)) @@ -100,8 +123,12 @@ func (a Authentication) ServeHTTP(w http.ResponseWriter, r *http.Request, next c continue } if authed { + winner = bw break } + if isolate { + failed = append(failed, bw) + } if userHasInfo(user) { candidate = user hasCandidate = true @@ -111,18 +138,157 @@ func (a Authentication) ServeHTTP(w http.ResponseWriter, r *http.Request, next c if hasCandidate { setAuthUserPlaceholders(repl, "http.auth.candidate", candidate) } + // When isolating, no failed provider's response reached the real + // writer, so apply one provider's challenge headers (e.g. a + // WWW-Authenticate, or a Location); a redirecting provider takes + // precedence and is sent as a full response. A single provider already + // wrote its challenge directly to the real writer. Either way, fall + // through to the auth error so handle_errors runs and a challenge that + // set only headers (like basic auth) still returns 401, not 200. + if isolate { + if replay := pickReplay(failed); replay != nil { + maps.Copy(w.Header(), replay.header) + if replay.statusCode >= 300 && replay.statusCode < 400 { + w.WriteHeader(replay.statusCode) + _, _ = w.Write(replay.buf.Bytes()) + return nil + } + } + } return caddyhttp.Error(http.StatusUnauthorized, fmt.Errorf("not authenticated")) } + // When isolating, the winning provider wrote to its buffer; copy the + // headers it set (e.g. a Set-Cookie establishing a new session) onto the + // real writer. Its status/body are NOT replayed: the request is + // authenticated and continues down the handler chain, which produces the + // actual response. (A single provider already wrote its headers directly.) + if winner != nil { + maps.Copy(w.Header(), winner.header) + } + setAuthUserPlaceholders(repl, "http.auth.user", user) return next.ServeHTTP(w, r) } +// pickReplay chooses which failed provider's buffered response to send when +// no provider authenticated: a redirect (3xx) wins, otherwise the first +// provider that wrote anything (status, body, or headers). +func pickReplay(failed []*bufferedResponseWriter) *bufferedResponseWriter { + var replay *bufferedResponseWriter + for _, bw := range failed { + if bw.statusCode >= 300 && bw.statusCode < 400 { + return bw + } + if replay == nil && (bw.statusCode != 0 || bw.buf.Len() > 0 || len(bw.header) > 0) { + replay = bw + } + } + return replay +} + func userHasInfo(user User) bool { return user.ID != "" || len(user.Metadata) > 0 } +// maxBufferedAuthResponse caps how much of a single provider's response body +// is buffered during multi-provider authentication, so a misbehaving provider +// cannot cause unbounded memory use. Authentication challenges are tiny; this +// limit is far larger than any legitimate challenge. +const maxBufferedAuthResponse = 1 << 20 // 1 MiB + +// bufferedResponseWriter captures a single provider's response — headers, +// status, and a size-capped body — so it can be discarded or replayed once +// the outcome of the whole provider set is known (only used when more than +// one provider is configured). +// +// It implements every capability interface a provider may hold on the real +// writer, so both plain type assertions (w.(http.Flusher)) and +// http.ResponseController keep working exactly as they did before isolation: +// Pusher and ReaderFrom come from the embedded caddyhttp.ResponseWriterWrapper, +// Hijacker and Flusher are declared below. Body writes are captured rather +// than streamed, and flushing is suppressed while buffering. A provider that +// hijacks the connection mid-authentication escapes to the real writer +// (isolation cannot apply once hijacked); auth providers are not expected to +// stream or hijack during Authenticate. +type bufferedResponseWriter struct { + *caddyhttp.ResponseWriterWrapper + header http.Header + statusCode int + buf bytes.Buffer + overflowed bool +} + +func newBufferedResponseWriter(w http.ResponseWriter) *bufferedResponseWriter { + return &bufferedResponseWriter{ + ResponseWriterWrapper: &caddyhttp.ResponseWriterWrapper{ResponseWriter: w}, + header: make(http.Header), + } +} + +func (bw *bufferedResponseWriter) Header() http.Header { return bw.header } + +func (bw *bufferedResponseWriter) WriteHeader(statusCode int) { + if bw.statusCode == 0 { + bw.statusCode = statusCode + } +} + +func (bw *bufferedResponseWriter) Write(data []byte) (int, error) { + bw.WriteHeader(http.StatusOK) + if room := maxBufferedAuthResponse - bw.buf.Len(); room < len(data) { + bw.overflowed = true + if room > 0 { + bw.buf.Write(data[:room]) + } + // Report a full write so the provider does not error; the body is a + // discardable challenge and only its capped prefix is retained. + return len(data), nil + } + return bw.buf.Write(data) +} + +// ReadFrom captures the body (respecting the size cap) instead of the +// embedded wrapper's pass-through to the real writer, which would leak the +// provider's body past isolation. It returns the total number of bytes +// consumed from r — retained and drained alike — and any read error, +// per the io.ReaderFrom contract. +func (bw *bufferedResponseWriter) ReadFrom(r io.Reader) (int64, error) { + bw.WriteHeader(http.StatusOK) + room := int64(maxBufferedAuthResponse - bw.buf.Len()) + n, err := bw.buf.ReadFrom(io.LimitReader(r, room)) + if err != nil { + return n, err + } + extra, err := io.Copy(io.Discard, r) + if extra > 0 { + bw.overflowed = true + } + return n + extra, err +} + +// Flush implements http.Flusher. Flushing is suppressed while a provider's +// response is buffered, so a provider that flushes during Authenticate cannot +// prematurely commit the response to the client. It is declared explicitly +// (rather than left to FlushError) so that providers doing a plain +// w.(http.Flusher) assertion still find one, as they do on the real writer. +func (bw *bufferedResponseWriter) Flush() {} + +// FlushError is the same suppression for http.ResponseController.Flush, which +// prefers FlushError over Flush. See https://github.com/caddyserver/caddy/issues/6144. +func (bw *bufferedResponseWriter) FlushError() error { return nil } + +// Hijack implements http.Hijacker by delegating to the real writer: once a +// provider takes over the connection there is no response left to isolate. +// This mirrors responseRecorder.Hijack in the caddyhttp package. The +// controller is built on the embedded wrapper, whose Unwrap reaches the real +// writer, and returns http.ErrNotSupported if that writer cannot hijack. +func (bw *bufferedResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { + //nolint:bodyclose + return http.NewResponseController(bw.ResponseWriterWrapper).Hijack() +} + func setAuthUserPlaceholders(repl *caddy.Replacer, namespace string, user User) { repl.Set(namespace+".id", user.ID) for k, v := range user.Metadata { diff --git a/modules/caddyhttp/caddyauth/caddyauth_test.go b/modules/caddyhttp/caddyauth/caddyauth_test.go index 708dcedf0..774ffe4aa 100644 --- a/modules/caddyhttp/caddyauth/caddyauth_test.go +++ b/modules/caddyhttp/caddyauth/caddyauth_test.go @@ -15,8 +15,12 @@ package caddyauth import ( + "bufio" + "bytes" "context" "errors" + "io" + "net" "net/http" "net/http/httptest" "testing" @@ -195,3 +199,304 @@ type staticAuthenticator struct { func (a staticAuthenticator) Authenticate(http.ResponseWriter, *http.Request) (User, bool, error) { return a.user, a.authed, a.err } + +// writingAuthenticator writes to the response during Authenticate (as a real +// provider might: a challenge/redirect on failure, a Set-Cookie on success). +type writingAuthenticator struct { + authed bool + write func(w http.ResponseWriter) +} + +func (a writingAuthenticator) Authenticate(w http.ResponseWriter, _ *http.Request) (User, bool, error) { + if a.write != nil { + a.write(w) + } + if a.authed { + return User{ID: "u"}, true, nil + } + return User{}, false, nil +} + +func serveAuth(providers map[string]Authenticator, next caddyhttp.Handler) *httptest.ResponseRecorder { + a := Authentication{Providers: providers, logger: zap.NewNop()} + req, _ := newRequestWithReplacer() + rr := httptest.NewRecorder() + _ = a.ServeHTTP(rr, req, next) + return rr +} + +// A failing provider that writes a redirect must not clobber the response +// when another provider authenticates the request. Provider map iteration +// order is randomized, so exercise both orderings. #5190 +func TestFailingProviderDoesNotClobberSuccess(t *testing.T) { + redirecter := writingAuthenticator{write: func(w http.ResponseWriter) { + w.Header().Set("Location", "/login") + w.WriteHeader(http.StatusFound) + }} + succeeder := writingAuthenticator{authed: true} + + for i := 0; i < 20; i++ { + reached := false + rr := serveAuth(map[string]Authenticator{ + "redirect": redirecter, "succeed": succeeder, + }, caddyhttp.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) error { + reached = true + w.WriteHeader(http.StatusOK) + return nil + })) + if !reached { + t.Fatalf("run %d: handler chain not reached despite successful auth", i) + } + if rr.Code != http.StatusOK { + t.Fatalf("run %d: got status %d, want 200 (failing provider leaked its redirect)", i, rr.Code) + } + if loc := rr.Header().Get("Location"); loc != "" { + t.Fatalf("run %d: failing provider's Location header leaked: %q", i, loc) + } + } +} + +// The successful provider's headers (e.g. a Set-Cookie for a new session) +// must reach the client even though its response is otherwise buffered. #5190 +func TestSuccessfulProviderHeadersPreserved(t *testing.T) { + succeeder := writingAuthenticator{authed: true, write: func(w http.ResponseWriter) { + w.Header().Set("Set-Cookie", "session=abc; Path=/") + }} + rr := serveAuth(map[string]Authenticator{"succeed": succeeder}, + caddyhttp.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) error { + w.WriteHeader(http.StatusOK) + return nil + })) + if got := rr.Header().Get("Set-Cookie"); got != "session=abc; Path=/" { + t.Fatalf("successful provider's Set-Cookie not preserved: got %q", got) + } + if rr.Code != http.StatusOK { + t.Fatalf("got status %d, want 200", rr.Code) + } +} + +// When every provider fails, a challenge still reaches the client and a +// redirect takes precedence over a plain failure. #5190 +func TestAllFailReplaysRedirect(t *testing.T) { + plain := writingAuthenticator{write: func(w http.ResponseWriter) { + w.WriteHeader(http.StatusUnauthorized) + }} + redirecter := writingAuthenticator{write: func(w http.ResponseWriter) { + w.Header().Set("Location", "/login") + w.WriteHeader(http.StatusFound) + }} + rr := serveAuth(map[string]Authenticator{ + "plain": plain, "redirect": redirecter, + }, caddyhttp.HandlerFunc(func(http.ResponseWriter, *http.Request) error { + t.Fatal("handler chain must not run when auth fails") + return nil + })) + if rr.Code != http.StatusFound { + t.Fatalf("got status %d, want 302 (redirect should win the replay)", rr.Code) + } + if loc := rr.Header().Get("Location"); loc != "/login" { + t.Fatalf("redirect Location not replayed: %q", loc) + } +} + +// A provider that fails by setting only a challenge header (no status/body) — +// which is exactly what basic auth does (WWW-Authenticate) — must still result +// in a 401 with that header, not a 200. #5190 +func TestHeaderOnlyChallengeStillReturns401(t *testing.T) { + basicish := writingAuthenticator{write: func(w http.ResponseWriter) { + w.Header().Set("WWW-Authenticate", `Basic realm="test"`) + }} + err := (func() error { + a := Authentication{Providers: map[string]Authenticator{"basic": basicish}, logger: zap.NewNop()} + req, _ := newRequestWithReplacer() + return a.ServeHTTP(&recordingStatusWriter{ResponseWriter: httptest.NewRecorder()}, req, + caddyhttp.HandlerFunc(func(http.ResponseWriter, *http.Request) error { + t.Fatal("handler chain must not run when auth fails") + return nil + })) + })() + var handlerErr caddyhttp.HandlerError + if !errors.As(err, &handlerErr) { + t.Fatalf("expected a 401 HandlerError, got %v (a header-only challenge must not return nil/200)", err) + } + if handlerErr.StatusCode != http.StatusUnauthorized { + t.Fatalf("expected status 401, got %d", handlerErr.StatusCode) + } +} + +// recordingStatusWriter records whether WriteHeader was called and with what +// status, to prove a header-only challenge does not emit a 200. +type recordingStatusWriter struct { + http.ResponseWriter + wroteHeader bool + status int +} + +func (rw *recordingStatusWriter) WriteHeader(status int) { + rw.wroteHeader = true + rw.status = status + rw.ResponseWriter.WriteHeader(status) +} + +// capabilityWriter implements the writer capabilities providers may rely on, +// recording whether each was actually invoked. +type capabilityWriter struct { + http.ResponseWriter + flushed bool + hijacked bool +} + +func (cw *capabilityWriter) Flush() { cw.flushed = true } +func (cw *capabilityWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { + cw.hijacked = true + return nil, nil, nil +} + +// With a SINGLE provider there is nothing to isolate, so the provider must +// receive the real writer untouched — all capabilities preserved and writes +// streamed directly. This is the no-regression guarantee for the common case. +func TestSingleProviderGetsRealWriter(t *testing.T) { + var gotReal bool + prov := writingAuthenticator{authed: true, write: func(w http.ResponseWriter) { + _, gotReal = w.(*capabilityWriter) // the real writer, not a buffered wrapper + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + }} + cw := &capabilityWriter{ResponseWriter: httptest.NewRecorder()} + a := Authentication{Providers: map[string]Authenticator{"only": prov}, logger: zap.NewNop()} + req, _ := newRequestWithReplacer() + if err := a.ServeHTTP(cw, req, caddyhttp.HandlerFunc(func(http.ResponseWriter, *http.Request) error { + return nil + })); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !gotReal { + t.Error("single provider did not receive the real ResponseWriter") + } + if !cw.flushed { + t.Error("single provider's Flush did not reach the real writer") + } +} + +// With MULTIPLE providers the buffered writer must still satisfy every +// capability interface the real writer satisfies, so external providers that +// check for Flusher/Hijacker/Pusher/ReaderFrom do not change behavior. Both +// access paths are checked: a plain type assertion, which is what most +// existing providers do, and http.ResponseController. +func TestBufferedWriterPreservesCapabilities(t *testing.T) { + var ( + ran bool + assertFlusher, assertHijacker, assertPusher, assertReaderFrom bool + ctrlFlush, ctrlHijack bool + ) + probe := writingAuthenticator{write: func(w http.ResponseWriter) { + ran = true + flusher, ok := w.(http.Flusher) + assertFlusher = ok + if ok { + flusher.Flush() // must be suppressed, not forwarded + } + _, assertHijacker = w.(http.Hijacker) + _, assertPusher = w.(http.Pusher) + _, assertReaderFrom = w.(io.ReaderFrom) + + ctrl := http.NewResponseController(w) + ctrlFlush = ctrl.Flush() == nil // FlushError suppressed → nil while buffering + _, _, herr := ctrl.Hijack() + ctrlHijack = herr == nil + }} + // Neither provider authenticates: providers are iterated from a map, so a + // succeeding one could break out of the loop before the probe ever runs. + // Two non-authenticating providers still enable isolation (len > 1) and + // guarantee the probe is exercised on every run. + other := writingAuthenticator{} + cw := &capabilityWriter{ResponseWriter: httptest.NewRecorder()} + a := Authentication{Providers: map[string]Authenticator{"probe": probe, "other": other}, logger: zap.NewNop()} + req, _ := newRequestWithReplacer() + _ = a.ServeHTTP(cw, req, caddyhttp.HandlerFunc(func(http.ResponseWriter, *http.Request) error { return nil })) + + if !ran { + t.Fatal("probe provider never ran; the test asserts nothing") + } + for _, tc := range []struct { + name string + got bool + }{ + {"w.(http.Flusher)", assertFlusher}, + {"w.(http.Hijacker)", assertHijacker}, + {"w.(http.Pusher)", assertPusher}, + {"w.(io.ReaderFrom)", assertReaderFrom}, + {"http.ResponseController.Flush", ctrlFlush}, + {"http.ResponseController.Hijack", ctrlHijack}, + } { + if !tc.got { + t.Errorf("%s failed on the buffered writer but succeeds on the real writer", tc.name) + } + } + // Flush during buffering must be suppressed — it must NOT reach the real writer. + if cw.flushed { + t.Error("a provider's Flush leaked to the real writer during buffering") + } +} + +// A provider that writes an enormous body must not be buffered without bound. +func TestBufferedResponseIsSizeCapped(t *testing.T) { + huge := 4 * maxBufferedAuthResponse + flood := writingAuthenticator{write: func(w http.ResponseWriter) { + n, err := w.Write(make([]byte, huge)) + if err != nil || n != huge { + t.Errorf("provider write short/errored: n=%d err=%v", n, err) + } + }} + succeeder := writingAuthenticator{authed: true} + bw := newBufferedResponseWriter(httptest.NewRecorder()) + // drive the flood provider through a buffered writer directly to inspect the cap + flood.Authenticate(bw, httptest.NewRequest(http.MethodGet, "/", nil)) + if bw.buf.Len() > maxBufferedAuthResponse { + t.Fatalf("buffered %d bytes, exceeds cap %d", bw.buf.Len(), maxBufferedAuthResponse) + } + if !bw.overflowed { + t.Error("expected overflow to be flagged when a provider exceeds the buffer cap") + } + _ = succeeder +} + +// ReadFrom must honor the io.ReaderFrom contract even when the size cap is +// exceeded: report the total bytes consumed from the source (retained and +// drained alike) and propagate any error encountered while draining. +func TestBufferedWriterReadFromReportsTotalAndDrainError(t *testing.T) { + extra := int64(1234) + total := int64(maxBufferedAuthResponse) + extra + bw := newBufferedResponseWriter(httptest.NewRecorder()) + n, err := bw.ReadFrom(bytes.NewReader(make([]byte, total))) + if err != nil { + t.Fatalf("ReadFrom() error = %v", err) + } + if n != total { + t.Errorf("ReadFrom() = %d, want total consumed %d", n, total) + } + if bw.buf.Len() != maxBufferedAuthResponse { + t.Errorf("retained %d bytes, want cap %d", bw.buf.Len(), maxBufferedAuthResponse) + } + if !bw.overflowed { + t.Error("expected overflow to be flagged when the source exceeds the buffer cap") + } + + drainErr := errors.New("drain failure") + bw = newBufferedResponseWriter(httptest.NewRecorder()) + n, err = bw.ReadFrom(io.MultiReader( + bytes.NewReader(make([]byte, maxBufferedAuthResponse)), + errorReader{err: drainErr}, + )) + if !errors.Is(err, drainErr) { + t.Errorf("ReadFrom() error = %v, want the drain error %v", err, drainErr) + } + if n != int64(maxBufferedAuthResponse) { + t.Errorf("ReadFrom() = %d, want %d bytes consumed before the error", n, maxBufferedAuthResponse) + } +} + +type errorReader struct{ err error } + +func (er errorReader) Read([]byte) (int, error) { return 0, er.err }