reverseproxy: more comments about buffering and add new tests (#6778)

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

.github/workflows/ci.yml

@@ -12,6 +12,10 @@ on:
       - master
       - 2.*
 
+env:
+  # https://github.com/actions/setup-go/issues/491
+  GOTOOLCHAIN: local
+
 jobs:
   test:
     strategy:
@@ -95,7 +99,7 @@ jobs:
       env:
         CGO_ENABLED: 0
       run: |
-        go build -tags nobadger -trimpath -ldflags="-w -s" -v
+        go build -tags nobadger,nomysql,nopgx -trimpath -ldflags="-w -s" -v
 
     - name: Smoke test Caddy
       working-directory: ./cmd/caddy
@@ -118,7 +122,7 @@ jobs:
       # continue-on-error: true
       run: |
         # (go test -v -coverprofile=cover-profile.out -race ./... 2>&1) > test-results/test-result.out
-        go test -tags nobadger -v -coverprofile="cover-profile.out" -short -race ./...
+        go test -tags nobadger,nomysql,nopgx -v -coverprofile="cover-profile.out" -short -race ./...
         # echo "status=$?" >> $GITHUB_OUTPUT
 
     # Relevant step if we reinvestigate publishing test/coverage reports
@@ -166,7 +170,7 @@ jobs:
           retries=3
           exit_code=0
           while ((retries > 0)); do
-            CGO_ENABLED=0 go test -p 1 -tags nobadger -v ./...
+            CGO_ENABLED=0 go test -p 1 -tags nobadger,nomysql,nopgx -v ./...
             exit_code=$?
             if ((exit_code == 0)); then
               break

.github/workflows/cross-build.yml

@@ -10,6 +10,10 @@ on:
       - master
       - 2.*
 
+env:
+  # https://github.com/actions/setup-go/issues/491
+  GOTOOLCHAIN: local
+
 jobs:
   build:
     strategy:

.github/workflows/lint.yml

@@ -13,6 +13,10 @@ on:
 permissions:
   contents: read
 
+env:
+  # https://github.com/actions/setup-go/issues/491
+  GOTOOLCHAIN: local
+
 jobs:
   # From https://github.com/golangci/golangci-lint-action
   golangci:

.github/workflows/release.yml

@@ -5,6 +5,10 @@ on:
     tags:
       - 'v*.*.*'
 
+env:
+  # https://github.com/actions/setup-go/issues/491
+  GOTOOLCHAIN: local
+
 jobs:
   release:
     name: Release

caddyconfig/httpcaddyfile/httptype.go

@@ -191,7 +191,7 @@ func (st ServerType) Setup(
 	metrics, _ := options["metrics"].(*caddyhttp.Metrics)
 	for _, s := range servers {
 		if s.Metrics != nil {
-			metrics = cmp.Or[*caddyhttp.Metrics](metrics, &caddyhttp.Metrics{})
+			metrics = cmp.Or(metrics, &caddyhttp.Metrics{})
 			metrics = &caddyhttp.Metrics{
 				PerHost: metrics.PerHost || s.Metrics.PerHost,
 			}
@@ -350,7 +350,7 @@ func (st ServerType) Setup(
 
 				// avoid duplicates by sorting + compacting
 				sort.Strings(defaultLog.Exclude)
-				defaultLog.Exclude = slices.Compact[[]string, string](defaultLog.Exclude)
+				defaultLog.Exclude = slices.Compact(defaultLog.Exclude)
 			}
 		}
 		// we may have not actually added anything, so remove if empty

modules/caddyhttp/app.go

@@ -207,7 +207,7 @@ func (app *App) Provision(ctx caddy.Context) error {
 
 		if srv.Metrics != nil {
 			srv.logger.Warn("per-server 'metrics' is deprecated; use 'metrics' in the root 'http' app instead")
-			app.Metrics = cmp.Or[*Metrics](app.Metrics, &Metrics{
+			app.Metrics = cmp.Or(app.Metrics, &Metrics{
 				init:        sync.Once{},
 				httpMetrics: &httpMetrics{},
 			})

modules/caddyhttp/reverseproxy/buffering_test.go

@@ -0,0 +1,84 @@
+package reverseproxy
+
+import (
+	"io"
+	"testing"
+)
+
+type zeroReader struct{}
+
+func (zeroReader) Read(p []byte) (int, error) {
+	for i := range p {
+		p[i] = 0
+	}
+	return len(p), nil
+}
+
+func TestBuffering(t *testing.T) {
+	var (
+		h  Handler
+		zr zeroReader
+	)
+	type args struct {
+		body  io.ReadCloser
+		limit int64
+	}
+	tests := []struct {
+		name        string
+		args        args
+		resultCheck func(io.ReadCloser, int64, args) bool
+	}{
+		{
+			name: "0 limit, body is returned as is",
+			args: args{
+				body:  io.NopCloser(&zr),
+				limit: 0,
+			},
+			resultCheck: func(res io.ReadCloser, read int64, args args) bool {
+				return res == args.body && read == args.limit && read == 0
+			},
+		},
+		{
+			name: "negative limit, body is read completely",
+			args: args{
+				body:  io.NopCloser(io.LimitReader(&zr, 100)),
+				limit: -1,
+			},
+			resultCheck: func(res io.ReadCloser, read int64, args args) bool {
+				brc, ok := res.(bodyReadCloser)
+				return ok && brc.body == nil && brc.buf.Len() == 100 && read == 100
+			},
+		},
+		{
+			name: "positive limit, body is read partially",
+			args: args{
+				body:  io.NopCloser(io.LimitReader(&zr, 100)),
+				limit: 50,
+			},
+			resultCheck: func(res io.ReadCloser, read int64, args args) bool {
+				brc, ok := res.(bodyReadCloser)
+				return ok && brc.body != nil && brc.buf.Len() == 50 && read == 50
+			},
+		},
+		{
+			name: "positive limit, body is read completely",
+			args: args{
+				body:  io.NopCloser(io.LimitReader(&zr, 100)),
+				limit: 101,
+			},
+			resultCheck: func(res io.ReadCloser, read int64, args args) bool {
+				brc, ok := res.(bodyReadCloser)
+				return ok && brc.body == nil && brc.buf.Len() == 100 && read == 100
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			res, read := h.bufferedBody(tt.args.body, tt.args.limit)
+			if !tt.resultCheck(res, read, tt.args) {
+				t.Error("Handler.bufferedBody() test failed")
+				return
+			}
+		})
+	}
+}

modules/caddyhttp/reverseproxy/reverseproxy.go

@@ -1234,6 +1234,10 @@ func (h Handler) provisionUpstream(upstream *Upstream) {
 // then returns a reader for the buffer along with how many bytes were buffered. Always close
 // the return value when done with it, just like if it was the original body! If limit is 0
 // (which it shouldn't be), this function returns its input; i.e. is a no-op, for safety.
+// Otherwise, it returns bodyReadCloser, the original body will be closed and body will be nil
+// if it's explicitly configured to buffer all or EOF is reached when reading.
+// TODO: the error during reading is discarded if the limit is negative, should the error be propagated
+// to upstream/downstream?
 func (h Handler) bufferedBody(originalBody io.ReadCloser, limit int64) (io.ReadCloser, int64) {
 	if limit == 0 {
 		return originalBody, 0

modules/caddytls/acmeissuer.go

@@ -146,8 +146,8 @@ func (iss *ACMEIssuer) Provision(ctx caddy.Context) error {
 		iss.AccountKey = accountKey
 	}
 
-	// DNS challenge provider
+	// DNS challenge provider, if not already established
-	if iss.Challenges != nil && iss.Challenges.DNS != nil {
+	if iss.Challenges != nil && iss.Challenges.DNS != nil && iss.Challenges.DNS.solver == nil {
 		var prov certmagic.DNSProvider
 		if iss.Challenges.DNS.ProviderRaw != nil {
 			// a challenge provider has been locally configured - use it

modules/caddytls/tls.go

@@ -182,17 +182,6 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 		t.dns = dnsMod
 	}
 
-	// ECH (Encrypted ClientHello) initialization
-	if t.EncryptedClientHello != nil {
-		t.EncryptedClientHello.configs = make(map[string][]echConfig)
-		outerNames, err := t.EncryptedClientHello.Provision(ctx)
-		if err != nil {
-			return fmt.Errorf("provisioning Encrypted ClientHello components: %v", err)
-		}
-		// outer names should have certificates to reduce client brittleness
-		t.automateNames = append(t.automateNames, outerNames...)
-	}
-
 	// set up a new certificate cache; this (re)loads all certificates
 	cacheOpts := certmagic.CacheOptions{
 		GetConfigForCert: func(cert certmagic.Certificate) (*certmagic.Config, error) {
@@ -243,31 +232,34 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 		t.certificateLoaders = append(t.certificateLoaders, modIface.(CertificateLoader))
 	}
 
-	// on-demand permission module
+	// using the certificate loaders we just initialized, load
-	if t.Automation != nil && t.Automation.OnDemand != nil && t.Automation.OnDemand.PermissionRaw != nil {
+	// manual/static (unmanaged) certificates - we do this in
-		if t.Automation.OnDemand.Ask != "" {
+	// provision so that other apps (such as http) can know which
-			return fmt.Errorf("on-demand TLS config conflict: both 'ask' endpoint and a 'permission' module are specified; 'ask' is deprecated, so use only the permission module")
+	// certificates have been manually loaded, and also so that
-		}
+	// commands like validate can be a better test
-		val, err := ctx.LoadModule(t.Automation.OnDemand, "PermissionRaw")
+	certCacheMu.RLock()
+	magic := certmagic.New(certCache, certmagic.Config{
+		Storage: ctx.Storage(),
+		Logger:  t.logger,
+		OnEvent: t.onEvent,
+		OCSP: certmagic.OCSPConfig{
+			DisableStapling: t.DisableOCSPStapling,
+		},
+		DisableStorageCheck: t.DisableStorageCheck,
+	})
+	certCacheMu.RUnlock()
+	for _, loader := range t.certificateLoaders {
+		certs, err := loader.LoadCertificates()
 		if err != nil {
-			return fmt.Errorf("loading on-demand TLS permission module: %v", err)
+			return fmt.Errorf("loading certificates: %v", err)
 		}
-		t.Automation.OnDemand.permission = val.(OnDemandPermission)
+		for _, cert := range certs {
-	}
+			hash, err := magic.CacheUnmanagedTLSCertificate(ctx, cert.Certificate, cert.Tags)
-
+			if err != nil {
-	// run replacer on ask URL (for environment variables) -- return errors to prevent surprises (#5036)
+				return fmt.Errorf("caching unmanaged certificate: %v", err)
-	if t.Automation != nil && t.Automation.OnDemand != nil && t.Automation.OnDemand.Ask != "" {
+			}
-		t.Automation.OnDemand.Ask, err = repl.ReplaceOrErr(t.Automation.OnDemand.Ask, true, true)
+			t.loaded[hash] = ""
-		if err != nil {
-			return fmt.Errorf("preparing 'ask' endpoint: %v", err)
 		}
-		perm := PermissionByHTTP{
-			Endpoint: t.Automation.OnDemand.Ask,
-		}
-		if err := perm.Provision(ctx); err != nil {
-			return fmt.Errorf("provisioning 'ask' module: %v", err)
-		}
-		t.Automation.OnDemand.permission = perm
 	}
 
 	// automation/management policies
@@ -302,6 +294,33 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 		}
 	}
 
+	// on-demand permission module
+	if t.Automation != nil && t.Automation.OnDemand != nil && t.Automation.OnDemand.PermissionRaw != nil {
+		if t.Automation.OnDemand.Ask != "" {
+			return fmt.Errorf("on-demand TLS config conflict: both 'ask' endpoint and a 'permission' module are specified; 'ask' is deprecated, so use only the permission module")
+		}
+		val, err := ctx.LoadModule(t.Automation.OnDemand, "PermissionRaw")
+		if err != nil {
+			return fmt.Errorf("loading on-demand TLS permission module: %v", err)
+		}
+		t.Automation.OnDemand.permission = val.(OnDemandPermission)
+	}
+
+	// run replacer on ask URL (for environment variables) -- return errors to prevent surprises (#5036)
+	if t.Automation != nil && t.Automation.OnDemand != nil && t.Automation.OnDemand.Ask != "" {
+		t.Automation.OnDemand.Ask, err = repl.ReplaceOrErr(t.Automation.OnDemand.Ask, true, true)
+		if err != nil {
+			return fmt.Errorf("preparing 'ask' endpoint: %v", err)
+		}
+		perm := PermissionByHTTP{
+			Endpoint: t.Automation.OnDemand.Ask,
+		}
+		if err := perm.Provision(ctx); err != nil {
+			return fmt.Errorf("provisioning 'ask' module: %v", err)
+		}
+		t.Automation.OnDemand.permission = perm
+	}
+
 	// session ticket ephemeral keys (STEK) service and provider
 	if t.SessionTickets != nil {
 		err := t.SessionTickets.provision(ctx)
@@ -310,32 +329,19 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 		}
 	}
 
-	// load manual/static (unmanaged) certificates - we do this in
+	// ECH (Encrypted ClientHello) initialization
-	// provision so that other apps (such as http) can know which
+	if t.EncryptedClientHello != nil {
-	// certificates have been manually loaded, and also so that
+		t.EncryptedClientHello.configs = make(map[string][]echConfig)
-	// commands like validate can be a better test
+		outerNames, err := t.EncryptedClientHello.Provision(ctx)
-	certCacheMu.RLock()
-	magic := certmagic.New(certCache, certmagic.Config{
-		Storage: ctx.Storage(),
-		Logger:  t.logger,
-		OnEvent: t.onEvent,
-		OCSP: certmagic.OCSPConfig{
-			DisableStapling: t.DisableOCSPStapling,
-		},
-		DisableStorageCheck: t.DisableStorageCheck,
-	})
-	certCacheMu.RUnlock()
-	for _, loader := range t.certificateLoaders {
-		certs, err := loader.LoadCertificates()
 		if err != nil {
-			return fmt.Errorf("loading certificates: %v", err)
+			return fmt.Errorf("provisioning Encrypted ClientHello components: %v", err)
 		}
-		for _, cert := range certs {
+
-			hash, err := magic.CacheUnmanagedTLSCertificate(ctx, cert.Certificate, cert.Tags)
+		// outer names should have certificates to reduce client brittleness
-			if err != nil {
+		for _, outerName := range outerNames {
-				return fmt.Errorf("caching unmanaged certificate: %v", err)
+			if !t.HasCertificateForSubject(outerName) {
+				t.automateNames = append(t.automateNames, outerNames...)
 			}
-			t.loaded[hash] = ""
 		}
 	}