Merge branch 'master' into add-tests

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

modules/caddyhttp/reverseproxy/caddyfile.go

@@ -923,13 +923,8 @@ func (h *Handler) FinalizeUnmarshalCaddyfile(helper httpcaddyfile.Helper) error
 		d.Next()
 		args := d.RemainingArgs()
 
-		// TODO: Remove this check at some point in the future
-		if len(args) == 2 {
-			return d.Errf("configuring 'handle_response' for status code replacement is no longer supported. Use 'replace_status' instead.")
-		}
-
 		if len(args) > 1 {
-			return d.Errf("too many arguments for 'handle_response': %s", args)
+			return d.Errf("too many arguments for 'handle_response': only a single response matcher name is allowed, but got: %s", args)
 		}
 
 		var matcher *caddyhttp.ResponseMatcher

modules/caddyhttp/reverseproxy/client_disconnect_test.go

@@ -0,0 +1,54 @@
+package reverseproxy
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"sync"
+	"testing"
+
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
+)
+
+// TestClientDisconnectRecordsStatus verifies that when the downstream client
+// disconnects (its request context is canceled) before the upstream sends any
+// response headers, the recorded status is 499 ("client closed request")
+// rather than 0.
+func TestClientDisconnectRecordsStatus(t *testing.T) {
+	// backend that blocks until the client goes away, so it never gets
+	// the chance to send response headers
+	gotRequest := make(chan struct{})
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		close(gotRequest)
+		<-r.Context().Done()
+	}))
+	defer backend.Close()
+
+	h := minimalHandler(0, &Upstream{
+		Host: new(Host),
+		Dial: backend.Listener.Addr().String(),
+	})
+
+	ctx, cancel := context.WithCancel(context.Background())
+	req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil).WithContext(ctx)
+	req = prepareTestRequest(req)
+
+	rec := caddyhttp.NewResponseRecorder(httptest.NewRecorder(), nil, nil)
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		_ = h.ServeHTTP(rec, req, caddyhttp.HandlerFunc(func(http.ResponseWriter, *http.Request) error {
+			return nil
+		}))
+	}()
+
+	<-gotRequest
+	cancel()
+	wg.Wait()
+
+	if got := rec.Status(); got != 499 {
+		t.Errorf("expected status 499 after client disconnect, got %d", got)
+	}
+}

modules/caddyhttp/reverseproxy/forwardauth/caddyfile.go

@@ -129,7 +129,11 @@ func parseCaddyfile(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
 				if !dispenser.NextArg() {
 					return nil, dispenser.ArgErr()
 				}
-				rpHandler.Rewrite.URI = dispenser.Val()
+				uri := dispenser.Val()
+				if rpHandler.Rewrite.URI != "" {
+					return nil, dispenser.Errf("cannot re-declare uri: %s", uri)
+				}
+				rpHandler.Rewrite.URI = uri
 				dispenser.DeleteN(2)
 
 			case "copy_headers":

modules/caddyhttp/reverseproxy/healthchecks.go

@@ -438,9 +438,7 @@ func (h *Handler) doActiveHealthCheck(dialInfo DialInfo, hostAddr string, networ
 	// may be expected by handlers of this request
 	ctx := h.ctx.Context
 	ctx = context.WithValue(ctx, caddy.ReplacerCtxKey, caddy.NewReplacer())
-	ctx = context.WithValue(ctx, caddyhttp.VarsCtxKey, map[string]any{
-		dialInfoVarKey: dialInfo,
-	})
+	ctx = context.WithValue(ctx, dialInfoCtxKey, dialInfo)
 	req, err := http.NewRequestWithContext(ctx, h.HealthChecks.Active.Method, u.String(), requestBody)
 	if err != nil {
 		return fmt.Errorf("making request: %v", err)

modules/caddyhttp/reverseproxy/hopheaders_test.go

@@ -0,0 +1,191 @@
+package reverseproxy
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/caddyserver/caddy/v2"
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
+)
+
+// 101 responses strip hop-by-hop headers (Alt-Svc, Keep-Alive, etc.) but preserve Upgrade and Connection
+func TestFinalizeResponse_101_StripsHopByHopHeaders(t *testing.T) {
+	h := &Handler{logger: caddy.Log()}
+
+	req := httptest.NewRequest(http.MethodGet, "/ws", nil)
+	req.Header.Set("Upgrade", "websocket")
+	req.Header.Set("Connection", "Upgrade")
+	req.Header.Set("Sec-WebSocket-Version", "13")
+	req.Header.Set("Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ==")
+
+	vars := map[string]any{}
+	ctx := context.WithValue(req.Context(), caddyhttp.VarsCtxKey, vars)
+	req = req.WithContext(ctx)
+
+	res := &http.Response{
+		StatusCode: http.StatusSwitchingProtocols,
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header: http.Header{
+			"Upgrade":              {"websocket"},
+			"Connection":           {"Upgrade"},
+			"Sec-Websocket-Accept": {"s3pPLMBiTxaQ9kYGzzhZRbK+xOo="},
+			"Alt-Svc":              {"h2=\"evil.com:443\"; ma=86400"},
+			"Keep-Alive":           {"timeout=999"},
+			"Proxy-Authenticate":   {"Basic realm=\"phish\""},
+		},
+		Body: fakeRWC{strings.NewReader("")},
+	}
+
+	repl := caddy.NewReplacer()
+	rw := httptest.NewRecorder()
+
+	err := h.finalizeResponse(rw, req, res, repl, fakeStart, caddy.Log())
+	if err != nil {
+		t.Logf("finalizeResponse returned error (expected, no real conn): %v", err)
+	}
+
+	if got := rw.Header().Get("Upgrade"); got != "websocket" {
+		t.Errorf("Upgrade = %q, want %q", got, "websocket")
+	}
+	if got := rw.Header().Get("Connection"); got != "Upgrade" {
+		t.Errorf("Connection = %q, want %q", got, "Upgrade")
+	}
+	for _, hdr := range []string{"Alt-Svc", "Keep-Alive", "Proxy-Authenticate"} {
+		if got := rw.Header().Get(hdr); got != "" {
+			t.Errorf("%s = %q, want empty (should be stripped)", hdr, got)
+		}
+	}
+}
+
+// Headers named in the upstream Connection value are stripped
+func TestFinalizeResponse_101_StripsConnectionNamedHeaders(t *testing.T) {
+	h := &Handler{logger: caddy.Log()}
+
+	req := httptest.NewRequest(http.MethodGet, "/ws", nil)
+	req.Header.Set("Upgrade", "websocket")
+	req.Header.Set("Connection", "Upgrade")
+	req.Header.Set("Sec-WebSocket-Version", "13")
+	req.Header.Set("Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ==")
+
+	vars := map[string]any{}
+	ctx := context.WithValue(req.Context(), caddyhttp.VarsCtxKey, vars)
+	req = req.WithContext(ctx)
+
+	res := &http.Response{
+		StatusCode: http.StatusSwitchingProtocols,
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header: http.Header{
+			"Upgrade":              {"websocket"},
+			"Connection":           {"Upgrade, X-Custom-ID"},
+			"Sec-Websocket-Accept": {"s3pPLMBiTxaQ9kYGzzhZRbK+xOo="},
+			"X-Custom-Id":          {"should-be-stripped"},
+		},
+		Body: fakeRWC{strings.NewReader("")},
+	}
+
+	repl := caddy.NewReplacer()
+	rw := httptest.NewRecorder()
+
+	err := h.finalizeResponse(rw, req, res, repl, fakeStart, caddy.Log())
+	if err != nil {
+		t.Logf("finalizeResponse returned error (no real connection): %v", err)
+	}
+
+	if got := rw.Header().Get("Upgrade"); got != "websocket" {
+		t.Errorf("Upgrade = %q, want %q", got, "websocket")
+	}
+	if got := rw.Header().Get("X-Custom-Id"); got != "" {
+		t.Errorf("X-Custom-Id = %q, want empty (named in Connection, should be stripped)", got)
+	}
+	if got := rw.Header().Get("Connection"); got != "Upgrade" {
+		t.Errorf("Connection = %q, want %q", got, "Upgrade")
+	}
+}
+
+// Normal 200 responses strip hop-by-hop headers
+func TestFinalizeResponse_200_StillStripsHopByHop(t *testing.T) {
+	h := &Handler{logger: caddy.Log()}
+
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	vars := map[string]any{}
+	ctx := context.WithValue(req.Context(), caddyhttp.VarsCtxKey, vars)
+	req = req.WithContext(ctx)
+
+	res := &http.Response{
+		StatusCode: http.StatusOK,
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header: http.Header{
+			"Content-Type": {"text/plain"},
+			"Alt-Svc":      {"h2=\"evil.com:443\""},
+			"Keep-Alive":   {"timeout=999"},
+		},
+		Body: io.NopCloser(strings.NewReader("ok")),
+	}
+
+	repl := caddy.NewReplacer()
+	rw := httptest.NewRecorder()
+
+	err := h.finalizeResponse(rw, req, res, repl, fakeStart, caddy.Log())
+	if err != nil {
+		t.Fatalf("finalizeResponse returned error: %v", err)
+	}
+
+	for _, hdr := range []string{"Alt-Svc", "Keep-Alive"} {
+		if got := rw.Header().Get(hdr); got != "" {
+			t.Errorf("%s = %q on 200 response, want empty (stripped)", hdr, got)
+		}
+	}
+}
+
+// Stripping still runs when the client didn't request an upgrade
+func TestFinalizeResponse_101_NoUpgradeRequest(t *testing.T) {
+	h := &Handler{logger: caddy.Log()}
+
+	req := httptest.NewRequest(http.MethodGet, "/ws", nil)
+
+	vars := map[string]any{}
+	ctx := context.WithValue(req.Context(), caddyhttp.VarsCtxKey, vars)
+	req = req.WithContext(ctx)
+
+	res := &http.Response{
+		StatusCode: http.StatusSwitchingProtocols,
+		ProtoMajor: 1,
+		ProtoMinor: 1,
+		Header: http.Header{
+			"Upgrade":    {"websocket"},
+			"Connection": {"Upgrade"},
+			"Alt-Svc":    {"h2=\"evil.com:443\""},
+		},
+		Body: io.NopCloser(strings.NewReader("")),
+	}
+
+	repl := caddy.NewReplacer()
+	rw := httptest.NewRecorder()
+
+	err := h.finalizeResponse(rw, req, res, repl, fakeStart, caddy.Log())
+	if err != nil {
+		t.Fatalf("finalizeResponse returned error: %v", err)
+	}
+
+	if got := rw.Header().Get("Alt-Svc"); got != "" {
+		t.Errorf("Alt-Svc = %q, want empty (stripped)", got)
+	}
+}
+
+// fakeRWC lets handleUpgradeResponse proceed past the body type assertion in tests
+type fakeRWC struct {
+	io.Reader
+}
+
+func (f fakeRWC) Write(p []byte) (n int, err error) { return len(p), nil }
+func (f fakeRWC) Close() error                      { return nil }
+
+var fakeStart = time.Time{}

modules/caddyhttp/reverseproxy/hosts.go

@@ -24,7 +24,6 @@ import (
 	"time"
 
 	"github.com/caddyserver/caddy/v2"
-	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 )
 
 // UpstreamPool is a collection of upstreams.
@@ -297,7 +296,7 @@ func (di DialInfo) String() string {
 // GetDialInfo gets the upstream dialing info out of the context,
 // and returns true if there was a valid value; false otherwise.
 func GetDialInfo(ctx context.Context) (DialInfo, bool) {
-	dialInfo, ok := caddyhttp.GetVar(ctx, dialInfoVarKey).(DialInfo)
+	dialInfo, ok := ctx.Value(dialInfoCtxKey).(DialInfo)
 	return dialInfo, ok
 }
 
@@ -329,9 +328,9 @@ type dynamicHostEntry struct {
 	lastSeen time.Time
 }
 
-// dialInfoVarKey is the key used for the variable that holds
+// dialInfoCtxKey is the context key used for the variable that holds
 // the dial info for the upstream connection.
-const dialInfoVarKey = "reverse_proxy.dial_info"
+const dialInfoCtxKey caddy.CtxKey = "reverse_proxy.dial_info"
 
 // proxyProtocolInfoVarKey is the key used for the variable that holds
 // the proxy protocol info for the upstream connection.

modules/caddyhttp/reverseproxy/httptransport_test.go

@@ -260,7 +260,7 @@ func TestHTTPTransport_DialContext_DialInfoOverride(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			dialCtx := context.WithValue(context.Background(), caddyhttp.VarsCtxKey, make(map[string]any))
-			caddyhttp.SetVar(dialCtx, dialInfoVarKey, DialInfo{
+			dialCtx = context.WithValue(dialCtx, dialInfoCtxKey, DialInfo{
 				Network: "tcp4",
 				Address: tt.dialInfo,
 			})

modules/caddyhttp/reverseproxy/reverseproxy.go

@@ -660,11 +660,6 @@ func (h *Handler) proxyLoopIteration(r *http.Request, origReq *http.Request, w h
 		)
 	}
 
-	// attach to the request information about how to dial the upstream;
-	// this is necessary because the information cannot be sufficiently
-	// or satisfactorily represented in a URL
-	caddyhttp.SetVar(r.Context(), dialInfoVarKey, dialInfo)
-
 	// set placeholders with information about this upstream
 	repl.Set("http.reverse_proxy.upstream.address", dialInfo.String())
 	repl.Set("http.reverse_proxy.upstream.hostport", dialInfo.Address)
@@ -699,9 +694,15 @@ func (h *Handler) proxyLoopIteration(r *http.Request, origReq *http.Request, w h
 
 	// proxy the request to that upstream
 	proxyErr = h.reverseProxy(w, r, origReq, repl, dialInfo, next)
-	if proxyErr == nil || errors.Is(proxyErr, context.Canceled) {
-		// context.Canceled happens when the downstream client
-		// cancels the request, which is not our failure
+	if proxyErr == nil {
+		return true, nil
+	}
+	if errors.Is(proxyErr, context.Canceled) {
+		// context.Canceled happens when the downstream client cancels the
+		// request, which is not our failure; don't retry or ding the upstream.
+		// Record a 499 (client closed request) so the access log reflects the
+		// disconnect instead of a misleading 0 status (see #7396).
+		w.WriteHeader(499)
 		return true, nil
 	}
 
@@ -1031,7 +1032,14 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, origRe
 			return nil
 		},
 	}
-	req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace))
+	// attach to the request information about how to dial the upstream;
+	// this is necessary because the information cannot be sufficiently
+	// or satisfactorily represented in a URL
+	// it's set before request is roundtripped to avoid a race condition when
+	// http.Transport reads a newer value to dial a new connection when that new
+	// value is updated by another reverse proxy handler, typically forward_auth.
+	ctx := context.WithValue(req.Context(), dialInfoCtxKey, di)
+	req = req.WithContext(httptrace.WithClientTrace(ctx, trace))
 
 	// do the round-trip
 	start := time.Now()
@@ -1215,6 +1223,22 @@ func (h *Handler) finalizeResponse(
 	start time.Time,
 	logger *zap.Logger,
 ) error {
+	// Strip hop-by-hop headers from the upstream response.
+	// For 101 Switching Protocols, save the Upgrade value
+	// first (handleUpgradeResponse needs it for validation)
+	// and restore it with a canonical Connection header.
+	upgradeVal := res.Header.Get("Upgrade")
+
+	removeConnectionHeaders(res.Header)
+	for _, h := range hopHeaders {
+		res.Header.Del(h)
+	}
+
+	if res.StatusCode == http.StatusSwitchingProtocols && upgradeVal != "" {
+		res.Header.Set("Upgrade", upgradeVal)
+		res.Header.Set("Connection", "Upgrade")
+	}
+
 	// deal with 101 Switching Protocols responses: (WebSocket, h2c, etc)
 	if res.StatusCode == http.StatusSwitchingProtocols {
 		var wg sync.WaitGroup
@@ -1223,12 +1247,6 @@ func (h *Handler) finalizeResponse(
 		return nil
 	}
 
-	removeConnectionHeaders(res.Header)
-
-	for _, h := range hopHeaders {
-		res.Header.Del(h)
-	}
-
 	// delete our Server header and use Via instead (see #6275)
 	rw.Header().Del("Server")
 	var protoPrefix string

modules/caddyhttp/reverseproxy/selectionpolicies.go

@@ -233,12 +233,23 @@ func (r RandomChoiceSelection) Validate() error {
 // Select returns an available host, if any.
 func (r RandomChoiceSelection) Select(pool UpstreamPool, _ *http.Request, _ http.ResponseWriter) *Upstream {
 	k := min(r.Choose, len(pool))
-	choices := make([]*Upstream, k)
-	for i, upstream := range pool {
+
+	// reservoir sampling (Algorithm R) over the available upstreams:
+	// the first k available upstreams fill the reservoir, then each
+	// subsequent one replaces a random reservoir entry with probability
+	// k/n, so every available upstream is sampled uniformly
+	choices := make([]*Upstream, 0, k)
+	var available int
+	for _, upstream := range pool {
 		if !upstream.Available() {
 			continue
 		}
-		j := weakrand.IntN(i + 1) //nolint:gosec
+		available++
+		if len(choices) < k {
+			choices = append(choices, upstream)
+			continue
+		}
+		j := weakrand.IntN(available) //nolint:gosec
 		if j < k {
 			choices[j] = upstream
 		}
@@ -712,7 +723,7 @@ func (s CookieHashSelection) Select(pool UpstreamPool, req *http.Request, w http
 			continue
 		}
 		sha, err := hashCookie(s.Secret, upstream.Dial)
-		if err == nil && sha == cookieValue {
+		if err == nil && hmac.Equal([]byte(sha), []byte(cookieValue)) {
 			return upstream
 		}
 	}

modules/caddyhttp/reverseproxy/selectionpolicies_test.go

@@ -725,6 +725,33 @@ func TestRandomChoicePolicy(t *testing.T) {
 	}
 }
 
+func TestRandomChoicePolicyLeastLoaded(t *testing.T) {
+	// when the number of available upstreams does not exceed the choose
+	// count, all of them must be candidates, so the least-loaded one
+	// must always be selected; the pool intentionally starts with an
+	// unavailable upstream to verify that reservoir sampling counts
+	// available upstreams rather than pool indices
+	pool := testPool()
+	pool[0].Dial = "localhost:8080"
+	pool[1].Dial = "localhost:8081"
+	pool[2].Dial = "localhost:8082"
+	pool[0].setHealthy(false)
+	pool[1].setHealthy(true)
+	pool[2].setHealthy(true)
+	pool[1].countRequest(30)
+	// pool[2] has no active requests
+
+	request := httptest.NewRequest(http.MethodGet, "/test", nil)
+	randomChoicePolicy := RandomChoiceSelection{Choose: 2}
+
+	for i := 0; i < 100; i++ {
+		h := randomChoicePolicy.Select(pool, request, nil)
+		if h != pool[2] {
+			t.Fatalf("with 2 available upstreams and choose=2, the least-loaded upstream (pool[2]) must always be selected; got %v on iteration %d", h, i)
+		}
+	}
+}
+
 func TestCookieHashPolicy(t *testing.T) {
 	ctx, cancel := caddy.NewContext(caddy.Context{Context: context.Background()})
 	defer cancel()
@@ -890,3 +917,46 @@ func TestCookieHashPolicyWithFirstFallback(t *testing.T) {
 		t.Error("Expected cookieHashPolicy to set a new cookie.")
 	}
 }
+
+func TestCookieHashPolicyWithSecret(t *testing.T) {
+	ctx, cancel := caddy.NewContext(caddy.Context{Context: context.Background()})
+	defer cancel()
+	cookieHashPolicy := CookieHashSelection{Secret: "hunter2"}
+	if err := cookieHashPolicy.Provision(ctx); err != nil {
+		t.Errorf("Provision error: %v", err)
+		t.FailNow()
+	}
+
+	pool := testPool()
+	pool[0].Dial = "localhost:8080"
+	pool[1].Dial = "localhost:8081"
+	pool[2].Dial = "localhost:8082"
+	pool[0].setHealthy(true)
+	pool[1].setHealthy(true)
+	pool[2].setHealthy(true)
+
+	request := httptest.NewRequest(http.MethodGet, "/test", nil)
+	w := httptest.NewRecorder()
+	h := cookieHashPolicy.Select(pool, request, w)
+	cookie := w.Result().Cookies()[0]
+
+	// a matching cookie sticks to the same host
+	request = httptest.NewRequest(http.MethodGet, "/test", nil)
+	w = httptest.NewRecorder()
+	request.AddCookie(cookie)
+	if got := cookieHashPolicy.Select(pool, request, w); got != h {
+		t.Errorf("Expected to stick to host %s, got %s", h, got)
+	}
+	if len(w.Result().Cookies()) != 0 {
+		t.Error("Expected no new cookie for a matching value")
+	}
+
+	// a tampered cookie value must not match any host and gets a fresh cookie
+	request = httptest.NewRequest(http.MethodGet, "/test", nil)
+	w = httptest.NewRecorder()
+	request.AddCookie(&http.Cookie{Name: cookie.Name, Value: cookie.Value[:len(cookie.Value)-1] + "0"})
+	cookieHashPolicy.Select(pool, request, w)
+	if len(w.Result().Cookies()) == 0 {
+		t.Error("Expected a new cookie to be set for a non-matching value")
+	}
+}