caddyauth: isolate provider responses to prevent cross-provider clobbering (#7904)

* caddyauth: isolate provider responses to prevent cross-provider clobbering

When multiple authentication providers are configured, each was handed the
real ResponseWriter, so a failing provider that wrote to the response (a
401 challenge or a login redirect) could clobber the response of another
provider or of the successful handler chain. Because provider map iteration
order is randomized, which provider's side effects won was nondeterministic.

A single provider now receives the real ResponseWriter unchanged — no
buffering, and Flusher/Hijacker/Pusher/ReaderFrom preserved exactly as
before. Only with multiple providers does each get its own buffered writer;
those writers embed caddyhttp.ResponseWriterWrapper so the underlying
capabilities remain type-assertable via http.ResponseController (Flush is
suppressed while buffering so a provider cannot prematurely commit the
response), and the buffered body is size-capped to avoid unbounded memory.

On success the winning provider's headers (e.g. a Set-Cookie) are copied to
the real writer and the chain proceeds. On total failure one provider's
challenge headers are applied (a redirect is sent as a full response),
otherwise the auth error is returned so handle_errors runs and a
header-only challenge (like basic auth) still returns 401.

Fixes #5190

* caddyauth: return total consumed bytes and drain error from ReadFrom

ReadFrom drained a source that exceeded the buffer cap but reported only
the bytes retained in the buffer and dropped any error from the drain,
violating the io.ReaderFrom contract: a caller such as io.Copy would see
fewer bytes than were actually consumed from the source, and a read
failure during the drain was silently swallowed. Return the
retained-plus-drained total and propagate the drain error.

* caddyauth: preserve Flusher and Hijacker on the buffered writer

The buffered writer used for multi-provider isolation embeds
caddyhttp.ResponseWriterWrapper, which promotes only Header, Write and
WriteHeader from the wrapped ResponseWriter and adds Push, ReadFrom and
Unwrap. Flush and Hijack were therefore reachable only through
http.ResponseController; a provider doing a plain w.(http.Flusher) or
w.(http.Hijacker) assertion silently lost them once a second provider was
configured.

Declare both on bufferedResponseWriter. Flush is a no-op so a provider
cannot prematurely commit a buffered response, and FlushError keeps the
same suppression for ResponseController, which prefers it over Flush.
Hijack delegates through the embedded wrapper, mirroring
responseRecorder.Hijack in the caddyhttp package.

The existing capability test only probed via http.ResponseController,
which is why this went unnoticed. It now asserts each capability both by
direct type assertion and through the controller, and uses two
non-authenticating providers so the probe is guaranteed to run — map
iteration order previously allowed a succeeding provider to break out of
the loop before the probe executed.

Also replace two header copy loops with maps.Copy, fixing the mapsloop
lint failures.
pull/7910/head
SillyZir 2026-07-31 22:19:27 -04:00 committed by GitHub
parent 2adc763020
commit e096ca9503
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 472 additions and 1 deletions

View File

@ -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 {

View File

@ -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 }