diff --git a/README.md b/README.md index 58d2489..d492aa7 100644 --- a/README.md +++ b/README.md @@ -187,24 +187,56 @@ generic JSON body); with it disabled they only log a warning. | `PREST_GUARD_BLACKLIST` | empty | Blocked IPs/CIDRs (403) | | `PREST_GUARD_WHITELIST` | empty | Allowed IPs/CIDRs; when set, everyone else is denied | | `PREST_GUARD_EXEMPT_IPS` | empty | IPs/CIDRs that skip checks (still subject to active bans) | -| `PREST_GUARD_EXCLUDE_PATHS` | empty | Extra paths whose subtree skips all guard checks | +| `PREST_GUARD_EXCLUDE_PATHS` | empty | Extra paths whose subtree the engine scopes as excluded (detection and policy checks skip; IP bans, IP policy and rate limits still run, see below) | | `PREST_GUARD_TRUSTED_PROXIES` | empty | Proxy IPs/CIDRs whose `X-Forwarded-For` is trusted | +| `PREST_GUARD_TRUSTED_PROXY_DEPTH` | engine default (`1`) | How many forwarded hops from the right belong to your own proxies | | `PREST_GUARD_BLOCK_CLOUD_PROVIDERS` | empty | Block datacenter ranges (`AWS,GCP,Azure`) | | `PREST_GUARD_MAX_BODY_BYTES` | `8192` | Body prefix the inspection layer reads | | `PREST_GUARD_REDIS_URL` | empty | Share rate limit and ban state across instances | | `PREST_GUARD_REDIS_PREFIX` | `prest_guard` | Key namespace in Redis | | `PREST_GUARD_REDIS_FAIL_OPEN` | `true` | Keep serving from per-instance memory when Redis is down | +| `PREST_GUARD_ENGINE_CONFIG` | empty | Inline JSON object over the full engine config (see below) | +| `PREST_GUARD_ENGINE_CONFIG_FILE` | empty | Path to a JSON file with the same content; set only one of the two | + +### Full engine config + +The variables above are a subset of the engine's `SecurityConfig`. Everything else (excluded detection headers, params and body fields, `Detection` tuning, `FailSecure`, endpoint rate limits, `EnforceHTTPS`, security headers, CORS, custom error responses, global behavior rules, blocked user agents, country lists, dynamic rules, emergency mode, agent settings, and the rest) is reachable through `PREST_GUARD_ENGINE_CONFIG` or `PREST_GUARD_ENGINE_CONFIG_FILE`: a JSON object whose keys are `guardcore.SecurityConfig` field names. It is decoded over the config the `PREST_GUARD_*` variables produce, so a key named in the JSON wins over the matching variable and the keys it does not name keep their values. Nested structs and maps merge into the engine defaults, slices and scalars replace them, durations are integer nanoseconds, and `ExcludePaths` can only add to the built-in defaults. Unknown keys, malformed JSON, func and interface fields, and values the engine rejects are loud config errors: an enabled guard answers 500 instead of running with a config you did not ask for. + +```bash +export PREST_GUARD_ENGINE_CONFIG='{"BlockedUserAgents":["sqlmap"],"CustomErrorResponses":{"403":"blocked"},"ExcludedDetectionParams":{"_where":true}}' +``` + +Fields JSON cannot carry (`CustomResponseModifier`, `AgentHandler`, `GeoIPHandler`, `CustomRequestCheck`, `OnError`) are set from Go: a plugin built from a fork wraps `Load` with its own `GuardConfig.Configure`, which receives the fully built engine config last. + +```go +func GuardMiddlewareLoad() negroni.Handler { + return Load(func() GuardConfig { + cfg := ParseGuardConfigFromEnv() + cfg.Configure = func(c *guardcore.SecurityConfig) { + c.CustomRequestCheck = myCheck + } + return cfg + }) +} +``` + +`CustomResponseModifier` takes effect once the pinned `nethttp-guard` release applies it (the nethttp adapter wires the engine's response modifier from the release after v1.4.0). ### Defaults that prevent outages Three defaults exist specifically because most pREST deployments sit behind a load balancer: -- `/_health`, `/_ready`, and the docs/static paths are excluded from every - guard check, always. Health probes share the proxy's identity with all - user traffic; rate limiting or banning them takes pods out of rotation. - Operator entries in `EXCLUDE_PATHS` are merged with these, not replacing - them. +- `/_health`, `/_ready`, and the docs/static paths are always excluded, + and operator entries in `EXCLUDE_PATHS` are merged with them, not + replacing them. Exclusion is the engine's own scoping, the same as in the + other adapters and the reference: detection and the route and request + policy checks skip these paths, and their responses still carry the + security headers and count in the metrics. IP bans, the IP blacklist and + whitelist, and rate limits are still enforced on them. Health probes share + the proxy's identity with all user traffic, so list the probe source in + `EXEMPT_IPS` (and set `TRUSTED_PROXIES`) to keep a rate limit from taking + pods out of rotation. - Auto-banning is opt-in. With an empty `TRUSTED_PROXIES`, every client shares the load balancer's IP, so a handful of violations would ban all users at once. Starting the guard without `TRUSTED_PROXIES` logs a loud @@ -214,13 +246,7 @@ a load balancer: ### Trusted proxies -When the direct peer is a trusted proxy, the client IP is resolved by -walking `X-Forwarded-For` from the right and taking the rightmost hop that -is not itself trusted (the leftmost entry when every hop is trusted, every -header line joined). Forwarded headers from untrusted peers are ignored -completely, and a malformed hop voids the whole chain. This mirrors the -reference engine's adapter-side contract; the engine separately flags -untrusted peers that send `X-Forwarded-For` as spoof attempts. +The engine resolves the client identity (the reference `extract_client_ip`), the plugin only hands it the connection's `RemoteAddr`. When the direct peer is in `TRUSTED_PROXIES`, the client is the hop `TRUSTED_PROXY_DEPTH` places from the right of `X-Forwarded-For` (default 1, the entry your proxy appended; set it to the number of proxies in front of pREST). A depth that over-counts the hops falls back to the right-to-left walk past trusted entries, and a chain shorter than the depth keeps the peer. Forwarded headers from untrusted peers are ignored, and the engine flags them as spoof attempts. Earlier versions walked the chain themselves and took the rightmost untrusted hop; with several trusted proxies in a row, set the depth instead. ### Redis modes @@ -254,21 +280,12 @@ inspection on, full middleware path): | Body size | Pass path (prose JSON) | Reject path (threat) | |---|---|---| -| 8 KiB (default) | ~59 ms | ~59 ms | -| 16 KiB | ~140 ms | ~139 ms | -| 64 KiB | ~1.16 s | ~1.25 s | -| 256 KiB | ~13 s | ~16 s | - -Rejection is not cheaper than passing; cost is body-size driven. Keep the -default unless you have measured acceptance for the cost at your bound: -eight concurrent clients posting 64 KiB bodies will saturate one core. - -The numbers above are large because guard-core-go does not yet port the -reference engine's large-value truncation (Python guard-core caps -full-pattern scans at 256 KiB and rebuilds a bounded scan budget around -detected attack regions). Until that lands engine-side, treat -`MAX_BODY_BYTES` as a CPU budget, not a feature dial. Rate limits and IP -policy alone do not read bodies and cost about 4 microseconds per request. +| 8 KiB (default) | ~28 ms | ~20 ms | +| 16 KiB | ~47 ms | ~39 ms | +| 64 KiB | ~0.22 s | ~0.20 s | +| 256 KiB | ~1.2 s | ~1.2 s | + +Cost still grows with body size, so keep the default unless you have measured acceptance for the cost at your bound. The engine caps what it scans (its large-value truncation, `Detection.MaxBodyInspectBytes` and the scan budget), which is why the 1 MiB bound costs about the same as 256 KiB. Treat `MAX_BODY_BYTES` as a CPU budget, not a feature dial. Rate limits and IP policy alone do not read bodies and cost about 9 microseconds per request. ## Verdict codes diff --git a/config.go b/config.go index 1365079..9779b93 100644 --- a/config.go +++ b/config.go @@ -91,8 +91,13 @@ type GuardConfig struct { ExcludePaths []string // TrustedProxies lists proxy IPs/CIDRs whose X-Forwarded-For is trusted // when resolving the real client IP. Empty means forwarded headers are - // ignored and the direct peer is the client. + // ignored and the direct peer is the client. The engine resolves the + // client (the reference extract_client_ip), not this plugin. TrustedProxies []string + // TrustedProxyDepth is the engine's trusted_proxy_depth: how many + // forwarded hops from the right belong to your own proxies. 0 keeps the + // engine default (1). + TrustedProxyDepth int // BlockCloudProviders blocks datacenter ranges ("AWS", "GCP", "Azure"). BlockCloudProviders []string @@ -105,6 +110,19 @@ type GuardConfig struct { // initialization retries in the background, and the guard goes live when // Redis returns. RedisFailOpen bool + + // EngineConfigJSON is a JSON object whose keys are guardcore.SecurityConfig + // field names (see the README "Full engine config" section). It is decoded + // over the config the fields above produce, so it reaches every engine + // setting without a dedicated variable. Unknown keys and non-serializable + // fields are an error, never silently dropped. + EngineConfigJSON string + // Configure is the Go-level hook for a plugin built from a fork: it gets + // the fully built engine config last (after the fields above and + // EngineConfigJSON), and can set the func and interface fields JSON + // cannot carry (CustomResponseModifier, AgentHandler, GeoIPHandler, + // CustomRequestCheck, ...). + Configure func(*guardcore.SecurityConfig) } // ParseGuardConfig reads every PREST_GUARD_* variable through lookup. Each @@ -231,6 +249,32 @@ func ParseGuardConfig(lookup func(string) (string, bool)) GuardConfig { cfg.BlockCloudProviders = splitList(raw) } + if raw, ok := lookup(envPrefix + "TRUSTED_PROXY_DEPTH"); ok { + if parsed, err := parseInt(raw); err != nil { + fail("PREST_GUARD_TRUSTED_PROXY_DEPTH: %q is not a valid integer", raw) + } else if parsed <= 0 { + fail("PREST_GUARD_TRUSTED_PROXY_DEPTH: %d is not positive", parsed) + } else { + cfg.TrustedProxyDepth = parsed + } + } + + inline, _ := lookup(envPrefix + "ENGINE_CONFIG") + file, _ := lookup(envPrefix + "ENGINE_CONFIG_FILE") + inline, file = strings.TrimSpace(inline), strings.TrimSpace(file) + switch { + case inline != "" && file != "": + fail("PREST_GUARD_ENGINE_CONFIG and PREST_GUARD_ENGINE_CONFIG_FILE are both set; use one") + case inline != "": + cfg.EngineConfigJSON = inline + case file != "": + if data, err := os.ReadFile(file); err != nil { + fail("PREST_GUARD_ENGINE_CONFIG_FILE: %v", err) + } else { + cfg.EngineConfigJSON = string(data) + } + } + if raw, ok := lookup(envPrefix + "REDIS_URL"); ok { cfg.RedisURL = strings.TrimSpace(raw) } diff --git a/configure_test.go b/configure_test.go new file mode 100644 index 0000000..f4f3025 --- /dev/null +++ b/configure_test.go @@ -0,0 +1,38 @@ +package main + +import ( + "net/http" + "testing" + + "github.com/rennf93/guard-core-go/v4/guardcore" +) + +// GuardConfig.Configure is the Go-level escape hatch for a plugin built from +// a fork: it receives the fully built engine config (func and interface +// fields such as CustomResponseModifier, AgentHandler or GeoIPHandler +// included) after the env subset and the engine config JSON were applied. +func TestConfigureHookEditsTheEngineConfigLast(t *testing.T) { + cfg := ParseGuardConfig(lookupMap(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_ENGINE_CONFIG": `{"BlockedUserAgents": ["badbot"]}`, + })) + cfg.Configure = func(c *guardcore.SecurityConfig) { + c.BlockedUserAgents = []string{"worsebot"} + } + h := Load(parseConst(cfg)) + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.2", map[string]string{"User-Agent": "badbot/1.0"}); code != http.StatusOK { + t.Fatalf("the hook runs after the JSON and replaces it, got %d", code) + } + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.2", map[string]string{"User-Agent": "worsebot/1.0"}); code != http.StatusForbidden { + t.Fatalf("the hook's own setting must apply, got %d", code) + } +} + +func TestConfigureHookRunsBeforeValidation(t *testing.T) { + cfg := baseEnabled(nil) + cfg.Configure = func(c *guardcore.SecurityConfig) { c.Blacklist = []string{"not-an-ip"} } + h := Load(parseConst(cfg)) + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.2", nil); code != http.StatusInternalServerError { + t.Fatalf("an invalid hook result must fail closed, got %d", code) + } +} diff --git a/exclusions.go b/exclusions.go index 53ce790..2a4a7d0 100644 --- a/exclusions.go +++ b/exclusions.go @@ -1,8 +1,13 @@ package main -import "strings" +import ( + "path" + "strings" +) -// pathExclusions holds normalized exclude_paths entries. +// pathExclusions holds normalized exclude_paths entries. It is only the +// pre-engine matcher of deferredHandler (the 503 window before an engine +// exists); once the engine is up, exclusion scoping is the engine's own. type pathExclusions struct { entries []string } @@ -24,9 +29,11 @@ func newPathExclusions(entries []string) *pathExclusions { // matches reports whether path falls inside one of the exclusion subtrees, // using the same subtree-or-equal semantics as the engine's exclude path // matcher: an entry "/health" covers "/health" and anything under "/health/". -func (p *pathExclusions) matches(path string) bool { +func (p *pathExclusions) matches(urlPath string) bool { + // Clean first: "/_health/../articles" must not ride an excluded prefix. + urlPath = path.Clean("/" + urlPath) for _, entry := range p.entries { - if entry == "/" || path == entry || strings.HasPrefix(path, entry+"/") { + if entry == "/" || urlPath == entry || strings.HasPrefix(urlPath, entry+"/") { return true } } diff --git a/exclusions_test.go b/exclusions_test.go index f0d5c60..f3bc838 100644 --- a/exclusions_test.go +++ b/exclusions_test.go @@ -36,3 +36,13 @@ func TestPathExclusionsSubtreeSemantics(t *testing.T) { t.Fatal("unrelated path must not match") } } + +func TestPathExclusionsDoNotFollowDotDotOutOfASubtree(t *testing.T) { + ex := newPathExclusions([]string{"/_health"}) + if ex.matches("/_health/../articles") { + t.Fatal("a dot-dot path leaving the subtree must not match") + } + if !ex.matches("/_health/./ready") { + t.Fatal("a cleaned path inside the subtree must match") + } +} diff --git a/go.mod b/go.mod index e9f271c..fd1e213 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.26.0 require ( github.com/rennf93/guard-core-go/v4 v4.3.2 - github.com/rennf93/nethttp-guard v1.3.0 + github.com/rennf93/nethttp-guard v1.4.0 github.com/urfave/negroni/v3 v3.1.1 ) diff --git a/go.sum b/go.sum index 3806be1..9096694 100644 --- a/go.sum +++ b/go.sum @@ -14,8 +14,8 @@ github.com/redis/go-redis/v9 v9.23.0 h1:/TTVdJa8BRNsybwwGGnmiYktHNlUXtbOQxlWpHBP github.com/redis/go-redis/v9 v9.23.0/go.mod h1:EWP2UUk+XDaI1s5nQB5mablZN6vccXlmBQTyfnF87D0= github.com/rennf93/guard-core-go/v4 v4.3.2 h1:UpEUaSw1MNIoPcMbikG2hPZr+rhqp8vtCDasNh0m5DQ= github.com/rennf93/guard-core-go/v4 v4.3.2/go.mod h1:zbSvFuYHn8Oo6bMwJ7jJ3iR8CEHBIYUvf1ALd1qMAvU= -github.com/rennf93/nethttp-guard v1.3.0 h1:XdsT31Cg2lrqy/0kiXievANz/KaZM1LdXttK6plfC38= -github.com/rennf93/nethttp-guard v1.3.0/go.mod h1:504XNp7GO3F/ZZ1Keiy9j7SdfYfWCHHqlGpqrdQM76A= +github.com/rennf93/nethttp-guard v1.4.0 h1:7CArpfVqAbfk0I5HYVr1Vvcbb2P9D8RiPbv3wrgiBDE= +github.com/rennf93/nethttp-guard v1.4.0/go.mod h1:DP766KRBdBQXFJ44j08drxWLjV9WvO1vA4gXMJqTu+k= github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= github.com/urfave/negroni/v3 v3.1.1 h1:6MS4nG9Jk/UuCACaUlNXCbiKa0ywF9LXz5dGu09v8hw= diff --git a/middleware.go b/middleware.go index 2651491..f215187 100644 --- a/middleware.go +++ b/middleware.go @@ -1,8 +1,11 @@ package main import ( + "encoding/json" + "fmt" "log/slog" "net/http" + "strings" "sync" "time" @@ -42,12 +45,6 @@ func Load(parse func() GuardConfig) negroni.Handler { return passThroughHandler() } - trusted, err := newTrustedProxySet(cfg.TrustedProxies) - if err != nil { - slog.Error("prest-guard: invalid config, failing closed", "err", err) - return misconfiguredHandler() - } - engineCfg, err := buildEngineConfig(cfg) if err != nil { slog.Error("prest-guard: invalid config, failing closed", "err", err) @@ -56,13 +53,13 @@ func Load(parse func() GuardConfig) negroni.Handler { exclusions := newPathExclusions(mergeExclusions(cfg.ExcludePaths)) - handler, err := engineFactoryFor(engineCfg, cfg, exclusions, trusted)() + handler, err := engineFactoryFor(engineCfg, cfg)() if err == nil { return handler } slog.Error("prest-guard: engine initialization failed; answering 503 until it succeeds", "err", err) return deferredHandler( - engineFactoryFor(engineCfg, cfg, exclusions, trusted), + engineFactoryFor(engineCfg, cfg), exclusions, time.Second, ) @@ -87,7 +84,8 @@ func buildEngineConfig(cfg GuardConfig) (*guardcore.SecurityConfig, error) { cfg.AutoBanDuration = defaultAutoBanDuration } } - return guardcore.NewSecurityConfig(func(c *guardcore.SecurityConfig) { + var overlayErr error + engineCfg, err := guardcore.NewSecurityConfig(func(c *guardcore.SecurityConfig) { c.OnBlock = onBlockLogger() c.PassiveMode = cfg.Passive @@ -132,11 +130,14 @@ func buildEngineConfig(cfg GuardConfig) (*guardcore.SecurityConfig, error) { c.EnabledDetectionCategories = cfg.InspectCategories } if len(cfg.TrustedProxies) > 0 { - // The engine arms spoof detection for untrusted peers carrying - // X-Forwarded-For; the chain walk itself stays in this adapter - // (the engine's own comment pins that split). + // The engine resolves the client identity from X-Forwarded-For + // for trusted peers and flags spoofed headers from untrusted + // ones (extract_client_ip); the adapter passes RemoteAddr as is. c.TrustedProxies = cfg.TrustedProxies } + if cfg.TrustedProxyDepth > 0 { + c.TrustedProxyDepth = cfg.TrustedProxyDepth + } // Pin the Redis fields before the conditional: the engine defaults // read REDIS_URL from the environment, and pREST deployments often @@ -150,7 +151,27 @@ func buildEngineConfig(cfg GuardConfig) (*guardcore.SecurityConfig, error) { c.RedisURL = cfg.RedisURL c.RedisFailOpen = cfg.RedisFailOpen } + + // The full engine config: JSON over the subset above, then the Go + // hook. The default exclusions are merged back afterwards so an + // ExcludePaths list in the JSON can only add to them. + if cfg.EngineConfigJSON != "" { + decoder := json.NewDecoder(strings.NewReader(cfg.EngineConfigJSON)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(c); err != nil { + overlayErr = fmt.Errorf("engine config JSON: %w", err) + return + } + c.ExcludePaths = mergeExclusions(c.ExcludePaths) + } + if cfg.Configure != nil { + cfg.Configure(c) + } }) + if overlayErr != nil { + return nil, overlayErr + } + return engineCfg, err } // engineFactory builds and fully initializes one engine plus its request @@ -158,7 +179,7 @@ func buildEngineConfig(cfg GuardConfig) (*guardcore.SecurityConfig, error) { // the error. type engineFactory func() (negroni.Handler, error) -func engineFactoryFor(engineCfg *guardcore.SecurityConfig, cfg GuardConfig, exclusions *pathExclusions, trusted *trustedProxySet) engineFactory { +func engineFactoryFor(engineCfg *guardcore.SecurityConfig, cfg GuardConfig) engineFactory { return func() (negroni.Handler, error) { engine, err := guardcore.NewEngine(engineCfg) if err != nil { @@ -168,7 +189,7 @@ func engineFactoryFor(engineCfg *guardcore.SecurityConfig, cfg GuardConfig, excl _ = engine.Close() return nil, err } - handler, err := newGuardHandler(engine, cfg, exclusions, trusted) + handler, err := newGuardHandler(engine, cfg) if err != nil { _ = engine.Close() return nil, err @@ -177,11 +198,11 @@ func engineFactoryFor(engineCfg *guardcore.SecurityConfig, cfg GuardConfig, excl } } -// newGuardHandler composes the live request path: exclusion short-circuit, -// trusted-proxy resolution, then the engine (rate limits, IP policy, cloud -// providers, and, when inspect_payloads is on, payload detection including -// the engine's native body scan). -func newGuardHandler(engine *guardcore.Engine, cfg GuardConfig, exclusions *pathExclusions, trusted *trustedProxySet) (negroni.Handler, error) { +// newGuardHandler wraps the engine through the nethttp adapter: exclusion +// scoping, client identity resolution (trusted proxies), rate limits, IP +// policy, cloud providers and, when inspect_payloads is on, payload +// detection all run inside the engine exactly as for every other adapter. +func newGuardHandler(engine *guardcore.Engine, cfg GuardConfig) (negroni.Handler, error) { wrap, err := nethttpguard.New(engine, nethttpguard.WithMaxBodyBytes(cfg.MaxBodyBytes), nethttpguard.WithLogger(bridgeLogger()), @@ -190,20 +211,14 @@ func newGuardHandler(engine *guardcore.Engine, cfg GuardConfig, exclusions *path return nil, err } return negroni.HandlerFunc(func(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) { - // exclude_paths promises "skips guard checks"; the engine still runs - // some checks on excluded paths, so this short-circuits first. - if exclusions.matches(r.URL.Path) { - next(w, r) - return - } - applyTrustedProxy(r, trusted) wrap(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { next(w, r) })).ServeHTTP(w, r) }), nil } -// deferredHandler answers 503 (excluded paths still pass) while a background +// deferredHandler answers 503 (excluded paths still pass, matched by the +// small pre-engine matcher because no engine exists yet) while a background // goroutine retries the factory with capped exponential backoff; the first // success swaps the live guard in, so a startup outage recovers instead of // latching. diff --git a/middleware_test.go b/middleware_test.go index dbe1ab9..c3a794f 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -34,6 +34,22 @@ func serve(tb testing.TB, h negroni.Handler, method, target, clientIP string, bo return rec } +// serveRaw is serve with request headers. +func serveRaw(tb testing.TB, h negroni.Handler, target, clientIP string, headers map[string]string) *httptest.ResponseRecorder { + tb.Helper() + req := httptest.NewRequest(http.MethodGet, target, nil) + req.RemoteAddr = clientIP + ":12345" + for name, value := range headers { + req.Header.Set(name, value) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(okBody)) + }) + return rec +} + func parseConst(cfg GuardConfig) func() GuardConfig { return func() GuardConfig { return cfg } } @@ -154,41 +170,36 @@ func TestRateLimitReturns429(t *testing.T) { func TestHealthEndpointsExcludedByDefault(t *testing.T) { h := Load(parseConst(baseEnabled(func(c *GuardConfig) { - c.RateLimit = 1 - c.RateLimitWindow = 60 - c.Blacklist = []string{"203.0.113.0/24"} + c.InspectPayloads = true }))) - ip := "203.0.113.7" // blacklisted; exclusion short-circuits before the engine + attack := "?q=" + // Excluded paths skip detection (exclusion scoping inside the engine). for _, path := range []string{"/_health", "/_health/ready", "/_ready", "/docs", "/static/app.js"} { - if rec := serve(t, h, http.MethodGet, path, ip, ""); rec.Code != http.StatusOK { - t.Fatalf("%s must be excluded by default even for a blacklisted IP, got %d", path, rec.Code) + if rec := serve(t, h, http.MethodGet, path+attack, "203.0.113.7", ""); rec.Code != http.StatusOK { + t.Fatalf("%s must be excluded from detection by default, got %d", path, rec.Code) } } - if rec := serve(t, h, http.MethodGet, "/healthcheck", ip, ""); rec.Code == http.StatusOK { + if rec := serve(t, h, http.MethodGet, "/healthcheck"+attack, "203.0.113.7", ""); rec.Code == http.StatusOK { t.Fatal("/healthcheck is not a default exclusion and must still be guarded") } } func TestOperatorExclusionsMergeWithDefaults(t *testing.T) { h := Load(parseConst(baseEnabled(func(c *GuardConfig) { - c.RateLimit = 1 + c.InspectPayloads = true c.ExcludePaths = []string{"/reports"} }))) ip := "203.0.113.9" - if rec := serve(t, h, http.MethodGet, "/reports/daily", ip, ""); rec.Code != http.StatusOK { - t.Fatalf("operator exclusion must skip checks, got %d", rec.Code) + attack := "?q=" + if rec := serve(t, h, http.MethodGet, "/reports/daily"+attack, ip, ""); rec.Code != http.StatusOK { + t.Fatalf("operator exclusion must skip detection, got %d", rec.Code) } - if rec := serve(t, h, http.MethodGet, "/_health", ip, ""); rec.Code != http.StatusOK { + if rec := serve(t, h, http.MethodGet, "/_health"+attack, ip, ""); rec.Code != http.StatusOK { t.Fatalf("defaults must survive operator exclusions, got %d", rec.Code) } - // Rate limits key per client and endpoint: the first /articles request - // passes, the second trips the limit of 1. - if rec := serve(t, h, http.MethodGet, "/articles", ip, ""); rec.Code != http.StatusOK { - t.Fatalf("non-excluded path first request must pass, got %d", rec.Code) - } - if rec := serve(t, h, http.MethodGet, "/articles", ip, ""); rec.Code != http.StatusTooManyRequests { - t.Fatalf("non-excluded path must stay rate limited, got %d", rec.Code) + if rec := serve(t, h, http.MethodGet, "/articles"+attack, ip, ""); rec.Code != http.StatusBadRequest { + t.Fatalf("non-excluded path must stay inspected, got %d", rec.Code) } } diff --git a/parity_test.go b/parity_test.go new file mode 100644 index 0000000..8309eab --- /dev/null +++ b/parity_test.go @@ -0,0 +1,214 @@ +package main + +// Parity tests against the reference adapter (fastapi-guard): excluded paths +// still go through the engine (exclusion scoping, not a bypass), the engine +// owns client identity resolution, and the full engine config is reachable +// from the plugin's environment. + +import ( + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/urfave/negroni/v3" +) + +func serveWithHeaders(tb testing.TB, h negroni.Handler, target, clientIP string, headers map[string]string) (int, http.Header, string) { + tb.Helper() + rec := serveRaw(tb, h, target, clientIP, headers) + return rec.Code, rec.Header(), rec.Body.String() +} + +// Excluded paths are scoped by the engine, not short-circuited before it: +// the reference keeps ip_security and rate_limit enforced on them and the +// pass-through response carries the security headers. +func TestExcludedPathsGoThroughTheEngine(t *testing.T) { + h := Load(parseConst(baseEnabled(func(c *GuardConfig) { + c.Blacklist = []string{"203.0.113.0/24"} + }))) + if code, _, _ := serveWithHeaders(t, h, "/_health", "203.0.113.7", nil); code != http.StatusForbidden { + t.Fatalf("a blacklisted IP must be refused on an excluded path (reference ip_security), got %d", code) + } + code, header, _ := serveWithHeaders(t, h, "/_health", "198.51.100.2", nil) + if code != http.StatusOK { + t.Fatalf("a clean client must reach an excluded path, got %d", code) + } + if header.Get("X-Frame-Options") == "" { + t.Fatalf("the engine's security headers must reach excluded-path responses: %v", header) + } +} + +func TestExcludedPathsStillSkipDetection(t *testing.T) { + h := Load(parseConst(baseEnabled(func(c *GuardConfig) { c.InspectPayloads = true }))) + if code, _, _ := serveWithHeaders(t, h, "/_health?q=", "198.51.100.2", nil); code != http.StatusOK { + t.Fatalf("an excluded path must skip payload detection, got %d", code) + } + if code, _, _ := serveWithHeaders(t, h, "/articles?q=", "198.51.100.2", nil); code != http.StatusBadRequest { + t.Fatalf("the same payload must still be blocked off the excluded path, got %d", code) + } +} + +func TestPathTraversalOutOfAnExcludedSubtreeIsNotExcluded(t *testing.T) { + h := Load(parseConst(baseEnabled(func(c *GuardConfig) { + c.Blacklist = []string{"203.0.113.0/24"} + c.InspectPayloads = true + }))) + if code, _, _ := serveWithHeaders(t, h, "/_health/../articles", "203.0.113.7", nil); code != http.StatusForbidden { + t.Fatalf("a dot-dot path leaving the excluded subtree must be guarded, got %d", code) + } +} + +// The engine resolves the client from X-Forwarded-For (reference +// extract_client_ip) under trusted_proxy_depth; the plugin no longer walks +// the chain itself. +func TestTrustedProxyDepthComesFromTheEngine(t *testing.T) { + env := map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_BLACKLIST": "203.0.113.0/24", + "PREST_GUARD_TRUSTED_PROXIES": "10.0.0.0/8", + } + xff := map[string]string{"X-Forwarded-For": "203.0.113.7, 10.0.0.2"} + + h := Load(parseEnv(env)) + if code, _, _ := serveWithHeaders(t, h, "/articles", "10.0.0.1", xff); code != http.StatusOK { + t.Fatalf("with the default depth 1 the last hop is the client (reference semantics), got %d", code) + } + + env["PREST_GUARD_TRUSTED_PROXY_DEPTH"] = "2" + h = Load(parseEnv(env)) + if code, _, _ := serveWithHeaders(t, h, "/articles", "10.0.0.1", xff); code != http.StatusForbidden { + t.Fatalf("depth 2 must resolve the second hop from the right, got %d", code) + } +} + +func TestUntrustedPeerForwardedHeaderIsIgnored(t *testing.T) { + h := Load(parseEnv(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_BLACKLIST": "203.0.113.0/24", + "PREST_GUARD_TRUSTED_PROXIES": "10.0.0.0/8", + })) + xff := map[string]string{"X-Forwarded-For": "203.0.113.7"} + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.9", xff); code != http.StatusOK { + t.Fatalf("a spoofed header from an untrusted peer must not move the identity, got %d", code) + } +} + +func TestInvalidTrustedProxyFailsClosed(t *testing.T) { + h := Load(parseEnv(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_TRUSTED_PROXIES": "not-an-ip", + })) + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.9", nil); code != http.StatusInternalServerError { + t.Fatalf("an unusable trusted proxy entry must fail closed, got %d", code) + } +} + +func TestInvalidTrustedProxyDepthFailsLoud(t *testing.T) { + cfg := ParseGuardConfig(lookupMap(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_TRUSTED_PROXY_DEPTH": "0", + })) + if cfg.Invalid == nil { + t.Fatal("a non-positive trusted proxy depth must be recorded as a config error") + } +} + +// The full engine config: every SecurityConfig field the env subset does not +// cover is reachable through PREST_GUARD_ENGINE_CONFIG (inline JSON) or +// PREST_GUARD_ENGINE_CONFIG_FILE, applied over the env subset. +func TestEngineConfigJSONReachesFieldsOutsideTheEnvSubset(t *testing.T) { + h := Load(parseEnv(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_ENGINE_CONFIG": `{ + "BlockedUserAgents": ["badbot"], + "CustomErrorResponses": {"403": "denied by policy"}, + "EnforceHTTPS": false, + "SecurityHeaders": {"FrameOptions": "DENY"} + }`, + })) + code, header, body := serveWithHeaders(t, h, "/articles", "198.51.100.2", map[string]string{"User-Agent": "badbot/1.0"}) + if code != http.StatusForbidden || !strings.Contains(body, "denied by policy") { + t.Fatalf("a JSON-configured blocked user agent and custom error body must apply, got %d %q", code, body) + } + if header.Get("X-Frame-Options") != "DENY" { + t.Fatalf("JSON-configured security headers must apply, got %v", header) + } +} + +func TestEngineConfigFileMatchesInlineJSON(t *testing.T) { + path := filepath.Join(t.TempDir(), "engine.json") + if err := os.WriteFile(path, []byte(`{"BlockedUserAgents": ["badbot"]}`), 0o600); err != nil { + t.Fatal(err) + } + h := Load(parseEnv(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_ENGINE_CONFIG_FILE": path, + })) + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.2", map[string]string{"User-Agent": "badbot/1.0"}); code != http.StatusForbidden { + t.Fatalf("the engine config file must apply, got %d", code) + } +} + +func TestEngineConfigJSONKeepsDefaultExclusions(t *testing.T) { + // The default /_health exclusion survives the JSON list: detection is + // skipped there, so the payload passes. + h := Load(parseEnv(map[string]string{ + "PREST_GUARD_ENABLED": "true", + "PREST_GUARD_INSPECT_PAYLOADS": "true", + "PREST_GUARD_ENGINE_CONFIG": `{"ExcludePaths": ["/reports"]}`, + })) + for _, path := range []string{"/_health", "/reports"} { + if code, _, _ := serveWithHeaders(t, h, path+"?q=", "198.51.100.2", nil); code != http.StatusOK { + t.Fatalf("%s must stay excluded from detection, got %d", path, code) + } + } +} + +func TestEngineConfigJSONFailsClosed(t *testing.T) { + for name, env := range map[string]map[string]string{ + "unknown field": {"PREST_GUARD_ENGINE_CONFIG": `{"NoSuchField": 1}`}, + "bad json": {"PREST_GUARD_ENGINE_CONFIG": `{`}, + "func field": {"PREST_GUARD_ENGINE_CONFIG": `{"OnBlock": "x"}`}, + "invalid value": {"PREST_GUARD_ENGINE_CONFIG": `{"RateLimit": -5, "EnableRateLimiting": true}`}, + "missing file": {"PREST_GUARD_ENGINE_CONFIG_FILE": "/nonexistent/engine.json"}, + "both sources": {"PREST_GUARD_ENGINE_CONFIG": `{}`, "PREST_GUARD_ENGINE_CONFIG_FILE": "/nonexistent/engine.json"}, + } { + t.Run(name, func(t *testing.T) { + env["PREST_GUARD_ENABLED"] = "true" + h := Load(parseEnv(env)) + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.2", nil); code != http.StatusInternalServerError { + t.Fatalf("a bad engine config must fail closed, got %d", code) + } + }) + } +} + +func TestEngineConfigJSONIgnoredWhileGuardDisabled(t *testing.T) { + h := Load(parseEnv(map[string]string{"PREST_GUARD_ENGINE_CONFIG": `{`})) + if code, _, _ := serveWithHeaders(t, h, "/articles", "198.51.100.2", nil); code != http.StatusOK { + t.Fatalf("a disabled guard must keep serving despite a bad engine config, got %d", code) + } +} + +// Reference behavior: rate_limit and ip_security stay enforced on excluded +// paths, so a probe source that shares an identity must be exempted +// explicitly (PREST_GUARD_EXEMPT_IPS) rather than relying on the path list. +func TestExcludedPathsStayRateLimitedUnlessExempt(t *testing.T) { + h := Load(parseConst(baseEnabled(func(c *GuardConfig) { + c.RateLimit = 1 + c.ExemptIPs = []string{"198.51.100.1"} + }))) + if code, _, _ := serveWithHeaders(t, h, "/_health", "203.0.113.5", nil); code != http.StatusOK { + t.Fatalf("first probe must pass, got %d", code) + } + if code, _, _ := serveWithHeaders(t, h, "/_health", "203.0.113.5", nil); code != http.StatusTooManyRequests { + t.Fatalf("an excluded path is still rate limited by the engine (reference), got %d", code) + } + for i := 0; i < 3; i++ { + if code, _, _ := serveWithHeaders(t, h, "/_health", "198.51.100.1", nil); code != http.StatusOK { + t.Fatalf("an exempt probe source must never be limited, got %d", code) + } + } +} diff --git a/proxy.go b/proxy.go deleted file mode 100644 index a685b14..0000000 --- a/proxy.go +++ /dev/null @@ -1,113 +0,0 @@ -package main - -import ( - "fmt" - "net" - "net/http" - "strings" -) - -// trustedProxySet holds pre-parsed trusted proxy IPs and CIDRs. -type trustedProxySet struct { - ips map[string]bool - nets []*net.IPNet -} - -// newTrustedProxySet parses trusted proxy entries. Any entry that is neither -// an IP nor a CIDR is a config error, so the guard fails closed at load -// rather than mis-resolving clients at request time. -func newTrustedProxySet(entries []string) (*trustedProxySet, error) { - if len(entries) == 0 { - return nil, nil - } - set := &trustedProxySet{ips: make(map[string]bool, len(entries))} - for i, entry := range entries { - entry = strings.TrimSpace(entry) - if entry == "" { - continue - } - if ip := net.ParseIP(entry); ip != nil { - set.ips[ip.String()] = true - continue - } - _, cidr, err := net.ParseCIDR(entry) - if err != nil { - return nil, fmt.Errorf("trusted_proxies[%d]: %q is not an IP or CIDR", i, entry) - } - set.nets = append(set.nets, cidr) - } - return set, nil -} - -// contains reports whether ip belongs to the trusted set. -func (t *trustedProxySet) contains(ip net.IP) bool { - if t == nil || ip == nil { - return false - } - if t.ips[ip.String()] { - return true - } - for _, n := range t.nets { - if n.Contains(ip) { - return true - } - } - return false -} - -// applyTrustedProxy rewrites r.RemoteAddr to the real client IP when the -// direct peer is a trusted proxy and X-Forwarded-For is present. Resolution -// walks the hop list from right to left and picks the rightmost entry that -// is not itself a trusted proxy (the standard rightmost-untrusted rule); -// when every hop is trusted the leftmost entry is used. When the peer is not -// trusted the forwarded header is attacker-controlled and ignored entirely, -// leaving RemoteAddr untouched. The rewritten address keeps a port suffix -// (net.JoinHostPort) so downstream net.SplitHostPort keeps working. -// -// The engine deliberately leaves the chain walk to adapters; it only arms -// spoof detection, so this walk is the plugin's half of that contract. -func applyTrustedProxy(r *http.Request, trusted *trustedProxySet) { - if trusted == nil { - return - } - peer := net.ParseIP(requestClientHost(r)) - if !trusted.contains(peer) { - return - } - // Header.Values joins every X-Forwarded-For line: some proxies append a - // new header line instead of joining their hop into the existing value, - // and taking only the first line would trust client-supplied data. - xff := strings.Join(r.Header.Values("X-Forwarded-For"), ",") - if strings.TrimSpace(xff) == "" { - return - } - hops := strings.Split(xff, ",") - for i := len(hops) - 1; i >= 0; i-- { - hop := net.ParseIP(strings.TrimSpace(hops[i])) - if hop == nil { - // Malformed hop: the chain cannot be trusted, keep the peer. - return - } - if trusted.contains(hop) { - continue - } - r.RemoteAddr = net.JoinHostPort(hop.String(), "0") - return - } - // All hops are trusted proxies: fall back to the leftmost entry. - leftmost := net.ParseIP(strings.TrimSpace(hops[0])) - if leftmost == nil { - return - } - r.RemoteAddr = net.JoinHostPort(leftmost.String(), "0") -} - -// requestClientHost extracts the bare host from RemoteAddr, tolerating -// addresses without a port. -func requestClientHost(r *http.Request) string { - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - return r.RemoteAddr - } - return host -} diff --git a/proxy_test.go b/proxy_test.go deleted file mode 100644 index 6aa1bc0..0000000 --- a/proxy_test.go +++ /dev/null @@ -1,119 +0,0 @@ -package main - -import ( - "net/http" - "net/http/httptest" - "testing" -) - -func clientAddrAfterProxy(req *http.Request) string { - return requestClientHost(req) -} - -func TestTrustedProxyResolvesRightmostUntrusted(t *testing.T) { - trusted, err := newTrustedProxySet([]string{"10.0.0.9"}) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = "10.0.0.9:54321" - req.Header.Set("X-Forwarded-For", "203.0.113.5, 10.0.0.9") - - applyTrustedProxy(req, trusted) - - if got := clientAddrAfterProxy(req); got != "203.0.113.5" { - t.Fatalf("client must resolve to the rightmost untrusted hop, got %q", got) - } -} - -func TestUntrustedPeerKeepsForwardedHeadersIgnored(t *testing.T) { - trusted, err := newTrustedProxySet([]string{"10.0.0.9"}) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = "203.0.113.99:54321" - req.Header.Set("X-Forwarded-For", "198.51.100.1") - - applyTrustedProxy(req, trusted) - - if got := clientAddrAfterProxy(req); got != "203.0.113.99" { - t.Fatalf("forwarded headers from an untrusted peer must be ignored, got %q", got) - } -} - -func TestTrustedProxyJoinsMultipleHeaderLines(t *testing.T) { - trusted, err := newTrustedProxySet([]string{"10.0.0.9"}) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = "10.0.0.9:54321" - // Some proxies append a second header line instead of joining the hop - // into the existing value. - req.Header.Add("X-Forwarded-For", "198.51.100.7") - req.Header.Add("X-Forwarded-For", "10.0.0.9") - - applyTrustedProxy(req, trusted) - - if got := clientAddrAfterProxy(req); got != "198.51.100.7" { - t.Fatalf("every header line must take part in the walk, got %q", got) - } -} - -func TestTrustedProxyAllHopsTrustedFallsBackToLeftmost(t *testing.T) { - trusted, err := newTrustedProxySet([]string{"10.0.0.9", "10.0.0.8"}) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = "10.0.0.9:54321" - req.Header.Set("X-Forwarded-For", "10.0.0.8, 10.0.0.9") - - applyTrustedProxy(req, trusted) - - if got := clientAddrAfterProxy(req); got != "10.0.0.8" { - t.Fatalf("when every hop is trusted the leftmost entry is used, got %q", got) - } -} - -func TestTrustedProxyMalformedHopKeepsPeer(t *testing.T) { - trusted, err := newTrustedProxySet([]string{"10.0.0.9"}) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = "10.0.0.9:54321" - req.Header.Set("X-Forwarded-For", "not-an-ip, 10.0.0.9") - - applyTrustedProxy(req, trusted) - - if got := clientAddrAfterProxy(req); got != "10.0.0.9" { - t.Fatalf("a malformed chain must not rewrite identity, got %q", got) - } -} - -func TestTrustedProxyCIDRRange(t *testing.T) { - trusted, err := newTrustedProxySet([]string{"172.16.0.0/12"}) - if err != nil { - t.Fatal(err) - } - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.RemoteAddr = "172.16.9.9:54321" - req.Header.Set("X-Forwarded-For", "203.0.113.8") - - applyTrustedProxy(req, trusted) - - if got := clientAddrAfterProxy(req); got != "203.0.113.8" { - t.Fatalf("CIDR entries must be honored, got %q", got) - } -} - -func TestNewTrustedProxySetRejectsGarbage(t *testing.T) { - if _, err := newTrustedProxySet([]string{"obviously-not-ip"}); err == nil { - t.Fatal("invalid entries must fail at load, not mis-resolve at request time") - } - if _, err := newTrustedProxySet(nil); err != nil { - t.Fatalf("empty set is valid: %v", err) - } -}