Redact credential form values instead of dropping the body, and bound AppSec settings

proxy/cmd/proxy/cmd/root.go

@@ -225,44 +225,43 @@ func runServer(cmd *cobra.Command, args []string) error {
 	defer stop()
 
 	srv := proxy.New(ctx, proxy.Config{
-		ListenAddr:               addr,
-		Logger:                   logger,
-		Version:                  Version,
-		ManagementAddress:        mgmtAddr,
-		ProxyURL:                 proxyDomain,
-		ProxyToken:               proxyToken,
-		CertificateDirectory:     certDir,
-		CertificateFile:          certFile,
-		CertificateKeyFile:       certKeyFile,
-		GenerateACMECertificates: acmeCerts,
-		ACMEChallengeAddress:     acmeAddr,
-		ACMEDirectory:            acmeDir,
-		ACMEEABKID:               acmeEABKID,
-		ACMEEABHMACKey:           acmeEABHMACKey,
-		ACMEChallengeType:        acmeChallengeType,
-		DebugEndpointEnabled:     debugEndpoint,
-		DebugEndpointAddress:     debugEndpointAddr,
-		HealthAddr:               healthAddr,
-		ForwardedProto:           forwardedProto,
-		TrustedProxies:           parsedTrustedProxies,
-		CertLockMethod:           nbacme.CertLockMethod(certLockMethod),
-		WildcardCertDir:          wildcardCertDir,
-		WireguardPort:            wgPort,
-		Performance:              perf,
-		ProxyProtocol:            proxyProtocol,
-		PreSharedKey:             preSharedKey,
-		SupportsCustomPorts:      supportsCustomPorts,
-		RequireSubdomain:         requireSubdomain,
-		Private:                  private,
-		MaxDialTimeout:           maxDialTimeout,
-		MaxSessionIdleTimeout:    maxSessionIdleTimeout,
-		MappingBatchWatchdog:     envDurationOrDefault("NB_PROXY_MAPPING_BATCH_WATCHDOG", 0),
-		GeoDataDir:               geoDataDir,
-		CrowdSecAPIURL:           crowdsecAPIURL,
-		CrowdSecAPIKey:           crowdsecAPIKey,
-		CrowdSecAppSecURL:        appsecURL,
-		CrowdSecAppSecTimeout:    appsecTimeout,
-
+		ListenAddr:                 addr,
+		Logger:                     logger,
+		Version:                    Version,
+		ManagementAddress:          mgmtAddr,
+		ProxyURL:                   proxyDomain,
+		ProxyToken:                 proxyToken,
+		CertificateDirectory:       certDir,
+		CertificateFile:            certFile,
+		CertificateKeyFile:         certKeyFile,
+		GenerateACMECertificates:   acmeCerts,
+		ACMEChallengeAddress:       acmeAddr,
+		ACMEDirectory:              acmeDir,
+		ACMEEABKID:                 acmeEABKID,
+		ACMEEABHMACKey:             acmeEABHMACKey,
+		ACMEChallengeType:          acmeChallengeType,
+		DebugEndpointEnabled:       debugEndpoint,
+		DebugEndpointAddress:       debugEndpointAddr,
+		HealthAddr:                 healthAddr,
+		ForwardedProto:             forwardedProto,
+		TrustedProxies:             parsedTrustedProxies,
+		CertLockMethod:             nbacme.CertLockMethod(certLockMethod),
+		WildcardCertDir:            wildcardCertDir,
+		WireguardPort:              wgPort,
+		Performance:                perf,
+		ProxyProtocol:              proxyProtocol,
+		PreSharedKey:               preSharedKey,
+		SupportsCustomPorts:        supportsCustomPorts,
+		RequireSubdomain:           requireSubdomain,
+		Private:                    private,
+		MaxDialTimeout:             maxDialTimeout,
+		MaxSessionIdleTimeout:      maxSessionIdleTimeout,
+		MappingBatchWatchdog:       envDurationOrDefault("NB_PROXY_MAPPING_BATCH_WATCHDOG", 0),
+		GeoDataDir:                 geoDataDir,
+		CrowdSecAPIURL:             crowdsecAPIURL,
+		CrowdSecAPIKey:             crowdsecAPIKey,
+		CrowdSecAppSecURL:          appsecURL,
+		CrowdSecAppSecTimeout:      appsecTimeout,
 		CrowdSecAppSecMaxBodyBytes: appsecMaxBodyBytes,
 	})
 

proxy/internal/appsec/body.go

@@ -57,27 +57,56 @@ func replay(prefix []byte, rest io.ReadCloser) io.ReadCloser {
 	}
 }
 
-// formHasField reports whether body is a URL-encoded form carrying any of the
-// named fields. Used to keep credential submissions out of the mirrored
-// request: the proxy's own password / PIN login form posts to the service path,
-// so without this the plaintext credential would reach the Security Engine.
-func formHasField(contentType string, body []byte, fields []string) bool {
+// redactedPlaceholder replaces a credential value in the mirrored body. It is
+// inert for rule matching, and its fixed length leaks nothing about the secret.
+const redactedPlaceholder = "redacted"
+
+// redactFormFields returns the body to mirror for a URL-encoded form, with the
+// values of the named fields replaced. The proxy's own password / PIN login
+// form posts to the service path itself, so without this the plaintext
+// credential would reach the Security Engine.
+//
+// Only the credential values are removed, never the whole body: dropping the
+// body outright would let a caller exempt any payload from inspection just by
+// appending a field named "password". Everything else in the form stays
+// inspectable, which is the point.
+//
+// Returns body unchanged when it is not a URL-encoded form or carries none of
+// the fields. Note that url.ParseQuery returns whatever it could parse
+// alongside an error, and the login handler reads the form with the same
+// tolerance, so a partially parsable body carrying a credential is still
+// redacted (and re-encoded from the parsed subset) rather than sent raw.
+//
+// Field names match case-sensitively, on purpose: the caller passes the exact
+// names the login handler reads via r.FormValue, and that lookup is itself
+// case-sensitive. A "Password" field is therefore never a credential as far as
+// the proxy is concerned, and redacting it would only blind the WAF to a value
+// the proxy does not own.
+func redactFormFields(contentType string, body []byte, fields []string) []byte {
 	if len(fields) == 0 || len(body) == 0 {
-		return false
+		return body
 	}
 	media, _, err := mime.ParseMediaType(contentType)
 	if err != nil || media != "application/x-www-form-urlencoded" {
-		return false
-	}
-	values, err := url.ParseQuery(string(body))
-	if err != nil {
-		// An unparsable form cannot be cleared as credential-free.
-		return true
+		return body
 	}
+
+	values, parseErr := url.ParseQuery(string(body))
+	redacted := false
 	for field := range values {
-		if slices.Contains(fields, field) {
-			return true
+		if !slices.Contains(fields, field) {
+			continue
 		}
+		for i := range values[field] {
+			values[field][i] = redactedPlaceholder
+		}
+		redacted = true
 	}
-	return false
+	if !redacted {
+		// Nothing to protect. A body that failed to parse is forwarded as-is:
+		// it carries no credential the login handler could read either.
+		_ = parseErr
+		return body
+	}
+	return []byte(values.Encode())
 }

proxy/internal/appsec/client.go

@@ -55,9 +55,26 @@ const (
 	// DefaultTimeout matches the 200ms budget CrowdSec's remediation component
 	// spec sets for the blocking AppSec call.
 	DefaultTimeout = 200 * time.Millisecond
+	// MinTimeout and MaxTimeout bound the configured inspection timeout.
+	// Inspection is synchronous, so the upper bound is what keeps a
+	// mis-set value from parking every request to an inspected service on a
+	// slow engine; the lower bound keeps the call from timing out before the
+	// engine can realistically answer. Mirrors the per-middleware bounds the
+	// proxy already applies to in-path calls.
+	MinTimeout = 10 * time.Millisecond
+	MaxTimeout = 5 * time.Second
 	// DefaultMaxBodyBytes caps the request body mirrored to the engine.
 	// Requests with a larger body are inspected on headers and URI only.
 	DefaultMaxBodyBytes int64 = 64 << 10
+	// MaxBodyBytesLimit is the ceiling for that cap. Each in-flight inspected
+	// request holds its buffered body in memory with no shared budget, so the
+	// worst case is this value times the concurrent request count. Matches the
+	// proxy-wide body-capture ceiling.
+	MaxBodyBytesLimit int64 = 8 << 20
+	// maxResponseBytes bounds how much of a verdict response is read. The
+	// engine answers with a two-field JSON object, so anything beyond this is
+	// not a response we can act on.
+	maxResponseBytes int64 = 4 << 10
 )
 
 // ErrUnavailable reports that the engine could not produce a verdict: the call
@@ -111,19 +128,33 @@ func New(cfg Config) (*Client, error) {
 		return nil, errors.New("appsec url has no host")
 	}
 
-	timeout := cfg.Timeout
-	if timeout <= 0 {
-		timeout = DefaultTimeout
-	}
-	maxBody := cfg.MaxBodyBytes
-	if maxBody == 0 {
-		maxBody = DefaultMaxBodyBytes
-	}
 	logger := cfg.Logger
 	if logger == nil {
 		logger = log.NewEntry(log.StandardLogger())
 	}
 
+	timeout := cfg.Timeout
+	switch {
+	case timeout <= 0:
+		timeout = DefaultTimeout
+	case timeout < MinTimeout:
+		logger.Warnf("appsec timeout %s is below the minimum, using %s", timeout, MinTimeout)
+		timeout = MinTimeout
+	case timeout > MaxTimeout:
+		logger.Warnf("appsec timeout %s exceeds the maximum, using %s", timeout, MaxTimeout)
+		timeout = MaxTimeout
+	}
+
+	// A negative cap is meaningful: forward no body at all.
+	maxBody := cfg.MaxBodyBytes
+	switch {
+	case maxBody == 0:
+		maxBody = DefaultMaxBodyBytes
+	case maxBody > MaxBodyBytesLimit:
+		logger.Warnf("appsec max body %d exceeds the maximum, using %d", maxBody, MaxBodyBytesLimit)
+		maxBody = MaxBodyBytesLimit
+	}
+
 	return &Client{
 		url:          cfg.URL,
 		apiKey:       cfg.APIKey,
@@ -150,10 +181,10 @@ type Request struct {
 	// TransactionID correlates the engine's alert with the proxy's access log
 	// entry. Empty lets the engine generate its own UUID.
 	TransactionID string
-	// OmitBodyFields suppresses body forwarding when the body is a form
-	// containing any of these field names. Used to keep credentials submitted
-	// to the proxy's own login form out of the engine.
-	OmitBodyFields []string
+	// RedactBodyFields lists form fields whose values are replaced before the
+	// body is mirrored. Used to keep credentials submitted to the proxy's own
+	// login form out of the engine while still inspecting the rest.
+	RedactBodyFields []string
 }
 
 // Inspect mirrors r to the AppSec engine and returns its verdict. A nil error
@@ -183,6 +214,14 @@ func (c *Client) Inspect(ctx context.Context, req Request) (restrict.Verdict, er
 		return restrict.DenyAppSecUnavailable, fmt.Errorf("%w: %w", ErrUnavailable, err)
 	}
 	defer func() {
+		// Drain before closing. net/http only returns a connection to the idle
+		// pool once its body is read to EOF; closing with bytes outstanding
+		// discards it. Every verdict carries a JSON body, so skipping this
+		// would cost a fresh handshake per inspected request, inside the
+		// timeout budget.
+		if _, err := io.Copy(io.Discard, io.LimitReader(resp.Body, maxResponseBytes)); err != nil {
+			c.logger.Tracef("drain appsec response body: %v", err)
+		}
 		if err := resp.Body.Close(); err != nil {
 			c.logger.Tracef("close appsec response body: %v", err)
 		}
@@ -193,7 +232,8 @@ func (c *Client) Inspect(ctx context.Context, req Request) (restrict.Verdict, er
 
 // readBody buffers the body so it can be mirrored, always restoring it on the
 // original request. Returns nil when there is no body to forward: no body at
-// all, an upgrade request, a body over the cap, or a credential form.
+// all, an upgrade request, or a body over the cap. A login form is forwarded
+// with its credential values redacted rather than suppressed.
 func (c *Client) readBody(req Request) ([]byte, error) {
 	r := req.HTTP
 	if c.maxBodyBytes < 0 || r.Body == nil || r.Body == http.NoBody {
@@ -218,10 +258,7 @@ func (c *Client) readBody(req Request) ([]byte, error) {
 	if oversize {
 		return nil, nil
 	}
-	if formHasField(r.Header.Get("Content-Type"), body, req.OmitBodyFields) {
-		return nil, nil
-	}
-	return body, nil
+	return redactFormFields(r.Header.Get("Content-Type"), body, req.RedactBodyFields), nil
 }
 
 // buildRequest assembles the mirrored request. Per the protocol it is a GET
@@ -276,7 +313,7 @@ func (c *Client) verdict(resp *http.Response) (restrict.Verdict, error) {
 	var decoded struct {
 		Action string `json:"action"`
 	}
-	if err := json.NewDecoder(io.LimitReader(resp.Body, 4<<10)).Decode(&decoded); err != nil {
+	if err := json.NewDecoder(io.LimitReader(resp.Body, maxResponseBytes)).Decode(&decoded); err != nil {
 		// A non-2xx status with an unreadable body is still a block: the engine
 		// answered, we just cannot tell which remediation it chose.
 		c.logger.Debugf("failed to decode appsec response (status %d): %v", resp.StatusCode, err)

proxy/internal/appsec/client_test.go

@@ -4,10 +4,12 @@ import (
 	"context"
 	"errors"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/netip"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -235,7 +237,7 @@ func TestInspect_ChunkedOversizeBodyIsReplayed(t *testing.T) {
 	assert.Equal(t, payload, string(restored), "bytes read to detect the cap must be replayed")
 }
 
-func TestInspect_SkipsCredentialForm(t *testing.T) {
+func TestInspect_RedactsCredentialForm(t *testing.T) {
 	eng := &engine{body: `{"action":"allow"}`}
 	srv := eng.start(t)
 	client := newClient(t, srv.URL)
@@ -244,21 +246,63 @@ func TestInspect_SkipsCredentialForm(t *testing.T) {
 	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
 	_, err := client.Inspect(context.Background(), Request{
-		HTTP:           r,
-		ClientIP:       netip.MustParseAddr("203.0.113.7"),
-		OmitBodyFields: []string{"password", "pin"},
+		HTTP:             r,
+		ClientIP:         netip.MustParseAddr("203.0.113.7"),
+		RedactBodyFields: []string{"password", "pin"},
 	})
 	require.NoError(t, err)
 
-	assert.Empty(t, eng.gotBody, "a login form body must not reach the engine")
-	assert.NotContains(t, string(eng.gotBody), "hunter2")
-	// The request is still inspected on headers and URI.
+	assert.NotContains(t, string(eng.gotBody), "hunter2", "the credential must not reach the engine")
+	assert.Contains(t, string(eng.gotBody), "next=%2Fhome", "the rest of the form stays inspectable")
 	assert.Equal(t, 1, eng.requests)
 
 	restored, err := io.ReadAll(r.Body)
 	require.NoError(t, err)
 	assert.Equal(t, "password=hunter2&next=%2Fhome", string(restored),
-		"the login handler still needs to read the form")
+		"the login handler still needs to read the real form")
+}
+
+// A caller must not be able to exempt a payload from inspection by naming one
+// of its fields "password": only the credential value is dropped.
+func TestInspect_RedactionIsNotABodyBypass(t *testing.T) {
+	eng := &engine{body: `{"action":"allow"}`}
+	srv := eng.start(t)
+	client := newClient(t, srv.URL)
+
+	const payload = "q=%27+OR+1%3D1--&password=x"
+	r := inbound(http.MethodPost, "http://svc.example.com/search", payload)
+	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+	_, err := client.Inspect(context.Background(), Request{
+		HTTP:             r,
+		ClientIP:         netip.MustParseAddr("203.0.113.7"),
+		RedactBodyFields: []string{"password", "pin"},
+	})
+	require.NoError(t, err)
+
+	forwarded := string(eng.gotBody)
+	assert.Contains(t, forwarded, "OR+1%3D1", "the attack payload must still be inspected")
+	assert.NotContains(t, forwarded, "password=x")
+}
+
+// A body that fails to parse carries no credential the login handler could
+// read either, so it is inspected as-is rather than withheld.
+func TestInspect_MalformedFormIsStillInspected(t *testing.T) {
+	eng := &engine{body: `{"action":"allow"}`}
+	srv := eng.start(t)
+	client := newClient(t, srv.URL)
+
+	const payload = "q=%zz&evil=%3Cscript%3E"
+	r := inbound(http.MethodPost, "http://svc.example.com/search", payload)
+	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+	_, err := client.Inspect(context.Background(), Request{
+		HTTP:             r,
+		ClientIP:         netip.MustParseAddr("203.0.113.7"),
+		RedactBodyFields: []string{"password", "pin"},
+	})
+	require.NoError(t, err)
+	assert.Equal(t, payload, string(eng.gotBody))
 }
 
 func TestInspect_ForwardsNonCredentialForm(t *testing.T) {
@@ -270,9 +314,9 @@ func TestInspect_ForwardsNonCredentialForm(t *testing.T) {
 	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
 	_, err := client.Inspect(context.Background(), Request{
-		HTTP:           r,
-		ClientIP:       netip.MustParseAddr("203.0.113.7"),
-		OmitBodyFields: []string{"password", "pin"},
+		HTTP:             r,
+		ClientIP:         netip.MustParseAddr("203.0.113.7"),
+		RedactBodyFields: []string{"password", "pin"},
 	})
 	require.NoError(t, err)
 	assert.Equal(t, "q=%3Cscript%3E", string(eng.gotBody), "ordinary form bodies must be inspected")
@@ -414,3 +458,61 @@ func TestInspect_MapsV4MappedClientIP(t *testing.T) {
 	assert.Equal(t, "203.0.113.7", eng.gotHeader.Get(headerIP),
 		"v4-mapped addresses must be unmapped so engine allowlists and rules match")
 }
+
+// Inspection is a blocking call on every request, so the connection to the
+// engine must be reused. net/http only pools a connection whose body was read
+// to EOF, which a verdict path returning early would skip.
+func TestInspect_ReusesConnections(t *testing.T) {
+	var conns atomic.Int64
+	srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(`{"action":"allow","http_status":200}`))
+	}))
+	srv.Config.ConnState = func(_ net.Conn, state http.ConnState) {
+		if state == http.StateNew {
+			conns.Add(1)
+		}
+	}
+	srv.Start()
+	t.Cleanup(srv.Close)
+
+	client := newClient(t, srv.URL)
+	for range 5 {
+		verdict, err := client.Inspect(context.Background(), Request{
+			HTTP:     inbound(http.MethodGet, "http://svc.example.com/", ""),
+			ClientIP: netip.MustParseAddr("203.0.113.7"),
+		})
+		require.NoError(t, err)
+		require.Equal(t, restrict.Allow, verdict)
+	}
+
+	assert.Equal(t, int64(1), conns.Load(),
+		"all five inspections must share one connection; a fresh handshake per request would eat the timeout budget")
+}
+
+func TestNew_ClampsOperatorValues(t *testing.T) {
+	tests := []struct {
+		name        string
+		cfg         Config
+		wantTimeout time.Duration
+		wantMaxBody int64
+	}{
+		{"defaults", Config{}, DefaultTimeout, DefaultMaxBodyBytes},
+		{"timeout below minimum", Config{Timeout: time.Microsecond}, MinTimeout, DefaultMaxBodyBytes},
+		{"timeout above maximum", Config{Timeout: time.Hour}, MaxTimeout, DefaultMaxBodyBytes},
+		{"body above maximum", Config{MaxBodyBytes: 1 << 30}, DefaultTimeout, MaxBodyBytesLimit},
+		// A negative cap means "forward no body" and must survive clamping.
+		{"negative body preserved", Config{MaxBodyBytes: -1}, DefaultTimeout, -1},
+		{"in-range values kept", Config{Timeout: time.Second, MaxBodyBytes: 1 << 10}, time.Second, 1 << 10},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.cfg.URL = "http://127.0.0.1:7422/"
+			tt.cfg.APIKey = "k"
+			client, err := New(tt.cfg)
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantTimeout, client.http.Timeout, "inspection is in the request path, so the timeout must stay bounded")
+			assert.Equal(t, tt.wantMaxBody, client.maxBodyBytes, "buffered bodies are held per in-flight request")
+		})
+	}
+}

proxy/internal/auth/appsec_test.go

@@ -3,7 +3,6 @@ package auth
 import (
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"testing"
 
 	log "github.com/sirupsen/logrus"
@@ -19,9 +18,7 @@ import (
 // appsecEngine is a stub AppSec component returning a fixed remediation.
 func appsecEngine(t *testing.T, status int, body string) *httptest.Server {
 	t.Helper()
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		_, _ = strings.NewReader("").Read(nil)
-		defer func() { _ = r.Body.Close() }()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(status)
 		if body != "" {
 			_, _ = w.Write([]byte(body))

proxy/internal/auth/middleware.go

@@ -282,6 +282,10 @@ func (mw *Middleware) checkIPRestrictions(w http.ResponseWriter, r *http.Request
 // enables inspection. Returns false when the request was blocked and a response
 // has been written.
 //
+// Every non-allow remediation blocks with 403, captcha included: the proxy has
+// no challenge flow to serve. The distinct verdict is still recorded so the
+// access log shows which remediation the engine actually chose.
+//
 // Unlike the geo and IP-reputation checks, this runs for overlay traffic too:
 // AppSec inspects request content, which is just as meaningful when the caller
 // reached the proxy through the WireGuard tunnel.
@@ -334,9 +338,9 @@ func (mw *Middleware) inspectAppSec(r *http.Request, config DomainConfig) restri
 	}
 
 	req := appsec.Request{
-		HTTP:           r,
-		ClientIP:       clientIP,
-		OmitBodyFields: credentialFormFields(config.Schemes),
+		HTTP:             r,
+		ClientIP:         clientIP,
+		RedactBodyFields: credentialFormFields(config.Schemes),
 	}
 	if cd := proxy.CapturedDataFromContext(r.Context()); cd != nil {
 		req.TransactionID = cd.GetRequestID()
@@ -349,9 +353,9 @@ func (mw *Middleware) inspectAppSec(r *http.Request, config DomainConfig) restri
 	return verdict
 }
 
-// credentialFormFields lists the login form fields whose presence suppresses
-// body forwarding, so a password or PIN submitted to the proxy's own login form
-// never reaches the Security Engine.
+// credentialFormFields lists the login form fields whose values are redacted
+// from the mirrored body, so a password or PIN submitted to the proxy's own
+// login form never reaches the Security Engine.
 func credentialFormFields(schemes []Scheme) []string {
 	var fields []string
 	for _, s := range schemes {