From 75dcb6e39448ef4bd0eb7caba566d5780029a4eb Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Mon, 27 Jul 2026 10:35:16 +0200 Subject: [PATCH] Redact credential form values instead of dropping the body, and bound AppSec settings --- proxy/cmd/proxy/cmd/root.go | 75 ++++++++-------- proxy/internal/appsec/body.go | 59 +++++++++---- proxy/internal/appsec/client.go | 73 ++++++++++++---- proxy/internal/appsec/client_test.go | 124 ++++++++++++++++++++++++--- proxy/internal/auth/appsec_test.go | 5 +- proxy/internal/auth/middleware.go | 16 ++-- 6 files changed, 260 insertions(+), 92 deletions(-) diff --git a/proxy/cmd/proxy/cmd/root.go b/proxy/cmd/proxy/cmd/root.go index abe68d0c3..2cff54b55 100644 --- a/proxy/cmd/proxy/cmd/root.go +++ b/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, }) diff --git a/proxy/internal/appsec/body.go b/proxy/internal/appsec/body.go index 9cb9fde06..3b383e627 100644 --- a/proxy/internal/appsec/body.go +++ b/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()) } diff --git a/proxy/internal/appsec/client.go b/proxy/internal/appsec/client.go index bb9af41e1..bb9b57b91 100644 --- a/proxy/internal/appsec/client.go +++ b/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) diff --git a/proxy/internal/appsec/client_test.go b/proxy/internal/appsec/client_test.go index f1ca4a33a..9254e5fab 100644 --- a/proxy/internal/appsec/client_test.go +++ b/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") + }) + } +} diff --git a/proxy/internal/auth/appsec_test.go b/proxy/internal/auth/appsec_test.go index 0de73fbea..22dc2b448 100644 --- a/proxy/internal/auth/appsec_test.go +++ b/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)) diff --git a/proxy/internal/auth/middleware.go b/proxy/internal/auth/middleware.go index b7a22e5a4..787f41625 100644 --- a/proxy/internal/auth/middleware.go +++ b/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 {