From 4a4874ae2fa03becc31f944630bf41837b9cce1e Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 5 Nov 2026 05:48:50 +0100 Subject: [PATCH] servewaf always answers rproxy well within its time limit (closes #182) A question waits on GeoJS at most 800ms, the new default of SWWAF_LOOKUP_TIMEOUT. In enforce mode it does not wait at all when it is over the header limits, on a fetched blocklist with deny, on the CrowdSec decision list, or over a whole rate limit. Not waiting never lowers a client's limit. Just before the Core Rule Set, servewaf refuses more than 50 cookies or 100 header fields with 431. Shared default changed: SWWAF_LOOKUP_TIMEOUT also applies to smallwebwaf with no command. Judgement call: SWWAF_REPUTATION_TIMEOUT stays 2s, since no request waits on the DNS blocklists or AbuseIPDB. Model: opus-5-5 --- SPEC.md | 70 +++- docs/metrics.md | 3 +- docs/request-log.md | 3 +- docs/settings.md | 13 +- internal/config/config.go | 2 +- internal/config/config_test.go | 6 +- internal/lookup/lookup.go | 16 +- internal/lookup/lookup_test.go | 37 +- internal/proxy/answertime_test.go | 600 +++++++++++++++++++++++++++ internal/proxy/biased.go | 16 +- internal/proxy/biased_test.go | 2 +- internal/proxy/countries_test.go | 6 +- internal/proxy/lookup.go | 22 +- internal/proxy/lookup_test.go | 6 +- internal/proxy/metrics_test.go | 4 +- internal/proxy/proxy_test.go | 3 + internal/proxy/request.go | 44 +- internal/proxy/servewaf.go | 63 +++ internal/proxy/servewaf_test.go | 2 +- internal/ratelimit/ratelimit.go | 24 ++ internal/ratelimit/ratelimit_test.go | 44 ++ internal/requestlog/requestlog.go | 6 +- 22 files changed, 937 insertions(+), 55 deletions(-) create mode 100644 internal/proxy/answertime_test.go diff --git a/SPEC.md b/SPEC.md index 03b382a..6843758 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1102,8 +1102,9 @@ metrics. ### The answer -`servewaf` runs steps 3 to 12 of "What happens to a request", in that order. -Every answer but the last below carries `X-Smallwebwaf-Disposition`. +`servewaf` runs steps 3 to 12 of "What happens to a request", in that order, +with the header limits just before step 12 (see "Answering in time"). Every +answer but the last below carries `X-Smallwebwaf-Disposition`. - `X-Smallwebwaf-Disposition: allow`, status `200`, no body. `rproxy` passes the request to the app. With `SWWAF_ADD_LOOKUP_HEADERS` the answer also carries @@ -1114,6 +1115,7 @@ Every answer but the last below carries `X-Smallwebwaf-Disposition`. status is one of: - `SWWAF_BAN_RESPONSE`, for a refusal; - `403`, for a `block` rule or the Core Rule Set; + - `431`, for a request over the header limits; - `404`, for a path under `/_smallwebwaf/` that passes the checks, so that the app never sees that prefix. - `X-Smallwebwaf-Disposition: close`, status `200`, no body. This takes the @@ -1131,6 +1133,70 @@ The Core Rule Set inspects the body a question carries only while does not inspect a JSON or XML body of 64 KiB or more, which may have been cut short. +### Answering in time + +`rproxy` stops waiting for `servewaf` after `SWRP_TIMEOUT_MS`, by default 2 +seconds, and `SWRP_DEFAULT_DISPOSITION` then decides. So at the defaults, +`servewaf` answers a request well within that time, whatever the request and +whatever its outside sources do. + +GeoJS is the only outside source a question can wait on. A question waits only +when step 5 says so, and for at most `SWWAF_LOOKUP_TIMEOUT`, by default 800 +milliseconds. A client without an answer by then counts as unknown. No question +waits on these, which run in the background: + +- the DNS blocklists and AbuseIPDB; +- fetching the blocklists, the CrowdSec decision list and the file + `SWWAF_ASN_LIMIT_PERCENT_URL` names. + +In `enforce` mode, a question does not wait for GeoJS when it is a request: + +- over the header limits; +- from a client on a fetched blocklist, while `SWWAF_BLOCKLIST_ACTION` is + `deny`; +- from a client on the CrowdSec decision list; +- that would take its client over a rate limit at 100% of it, since a percentage + can only lower a limit. + +Such a request still goes through the steps in their order. Its client's AS +number and country stay unknown until GeoJS has answered. So while +`SWWAF_EXCLUSIVELY_ALLOWED_COUNTRIES` is set, that list can refuse it first. +`SWWAF_UNKNOWN_LIMIT_PERCENT` does not apply to it until then, so not waiting +never lowers its limit. + +The static lists and bans come before the lookup, so they never wait. These can +still wait for GeoJS: + +- a country list, which needs the answer; +- a rate limit a percentage has lowered, which needs the answer; +- a DNS blocklist or AbuseIPDB, while `SWWAF_REPUTATION_ACTION` is `deny`; +- a trap path; +- a rule file; +- the Core Rule Set. + +The header limits are fixed, with no setting. `servewaf` refuses a request over +either one: + +- more than 50 cookies. Each part of its `Cookie` headers between semicolons + counts as one, unless it is empty or only spaces and tabs. +- more than 100 header fields. A header sent twice counts twice. `Host` and + `Transfer-Encoding` do not count. `X-Request-ID` and `X-Forwarded-Proto` do. + +Real clients never reach these limits. The answer is `deny` with `431`, and the +log line's action is `too_large`. The refusal makes no ban and is no offence. +The rate limits count the request as usual, so a client that keeps sending such +requests breaks a limit. + +The limits apply on every path and in every `SWWAF_WAF_MODE`. A client in +`SWWAF_ALLOW_NETS` skips them, as it skips the Core Rule Set. In `observe` mode +such a request is answered `allow` with `would_action`, and the Core Rule Set +does not read it. + +The Core Rule Set runs each of its rules on each cookie, so thousands of small +cookies would keep it busy for over a second. Within the limits, one long cookie +or query string can still keep it busy for a fraction of a second. So enough +such requests at once can overload `servewaf` past `SWRP_TIMEOUT_MS`. + ### The report Once the visitor's answer has ended, `rproxy` sends `POST /_smallwebwaf/report` diff --git a/docs/metrics.md b/docs/metrics.md index 3bb111a..e2bff8c 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -17,7 +17,8 @@ label as `exported_instance` unless the scrape sets `honor_labels: true`. - `smallwebwaf_rate_limit_hits_total`, by `window` and by `kind` (`requests`, `bytes` or `refusals`); -- `smallwebwaf_size_and_time_limit_hits_total`, by `limit`, the setting passed; +- `smallwebwaf_size_and_time_limit_hits_total`, by `limit`, the setting passed. + `servewaf`'s header limits have no setting, so it leaves them out; - `smallwebwaf_offences_total`, by `kind`; - `smallwebwaf_bans_made_total`, by `cause`; - `smallwebwaf_active_bans` and `smallwebwaf_permanent_bans`. diff --git a/docs/request-log.md b/docs/request-log.md index 1d584db..1689319 100644 --- a/docs/request-log.md +++ b/docs/request-log.md @@ -67,7 +67,8 @@ Every line has: CrowdSec decision, each of which bans the client; - `country_denied`, `rate_limited`, `rule_blocked` and `waf_blocked`: refused by that check; - - `too_large` and `timed_out`: a size or time limit was passed; + - `too_large`: a size limit was passed, or a header limit in `servewaf`; + - `timed_out`: a time limit was passed; - `upstream_error`: the app could not be reached, or its answer broke off; - `admin`: answered at one of `smallwebwaf`'s own endpoints. - `would_action`: in `observe` mode, what `enforce` mode would have done with a diff --git a/docs/settings.md b/docs/settings.md index a59e4df..9c39e58 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -157,7 +157,7 @@ that take `off`, the `_FILE` form, and what each group does and why. - an anomaly threshold per AS number. - `SWWAF_LOOKUP_DB_PATH` (default empty): the IPinfo Lite database file, in its `.mmdb` form, for `SWWAF_LOOKUP_SOURCE=file`. -- `SWWAF_LOOKUP_TIMEOUT` (default `1s`): how long a request waits for its +- `SWWAF_LOOKUP_TIMEOUT` (default `800ms`): how long a request waits for its client's first answer from GeoJS, and how long a request to GeoJS may take. A duration; never `off`. - `SWWAF_ADD_LOOKUP_HEADERS` (default `false`): `true` passes the app the @@ -340,7 +340,10 @@ A few limits are fixed rather than settings: - at most 1,000 DNS blocklist queries are under way at once; - at most 20,000 anomaly counters are kept; - each alert destination's queue holds at most 1,000 alerts; -- `servewaf` holds at most 10,000 answered requests awaiting their reports. +- `servewaf` holds at most 10,000 answered requests awaiting their reports; +- `servewaf` refuses a request with more than 50 cookies or more than 100 header + fields (see "Answering in time" under "`servewaf`" in + [`SPEC.md`](../SPEC.md)). ## `servewaf` @@ -358,3 +361,9 @@ at start still shows them. request to an app. `SWWAF_WAF_BODY_LIMIT` has an effect up to 64 KiB less one byte, whatever its value. A question carries no more of a body than 64 KiB (see "`servewaf`" in [`SPEC.md`](../SPEC.md)). + +`SWWAF_LOOKUP_TIMEOUT` is the longest a question waits on an outside source. +Keep it well under `rproxy`'s `SWRP_TIMEOUT_MS`, as the default is. No question +waits for `SWWAF_REPUTATION_TIMEOUT`, since the DNS blocklists and AbuseIPDB are +asked in the background (see "Answering in time" under "`servewaf`" in +[`SPEC.md`](../SPEC.md)). diff --git a/internal/config/config.go b/internal/config/config.go index 5f01c82..914e3de 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -516,7 +516,7 @@ func FromEnvironment(lookupEnv func(string) (string, bool)) (*Config, error) { BytesCount: env.bytesCount("SWWAF_BYTES_COUNT", "both"), LookupSource: env.lookupSource("SWWAF_LOOKUP_SOURCE", "geojs"), LookupDBPath: env.value("SWWAF_LOOKUP_DB_PATH", ""), - LookupTimeout: env.durationNotOff("SWWAF_LOOKUP_TIMEOUT", "1s"), + LookupTimeout: env.durationNotOff("SWWAF_LOOKUP_TIMEOUT", "800ms"), AddLookupHeaders: env.boolean("SWWAF_ADD_LOOKUP_HEADERS", "false"), DeniedCountries: env.countries("SWWAF_DENIED_COUNTRIES", ""), ExclusivelyAllowedCountries: env.countries( diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 032a3e0..67c3f33 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -281,7 +281,7 @@ func TestDefaults(t *testing.T) { RulesEnabled: true, }) wantLookupSettings(t, cfg, config.Config{ - LookupSource: defaultLookupSource, LookupTimeout: time.Second, + LookupSource: defaultLookupSource, LookupTimeout: 800 * time.Millisecond, }) wantByteLimitSettings(t, cfg, config.Config{ BytesLimitPerMinute: 4 << 30, BytesLimitPerHour: 20 << 30, @@ -1165,7 +1165,7 @@ func TestLookupSettingsAsSet(t *testing.T) { cfg = fromEnvironment(t, environment{lookupSource: off}) wantLookupSettings(t, cfg, config.Config{ - LookupSource: off, LookupTimeout: time.Second, + LookupSource: off, LookupTimeout: 800 * time.Millisecond, }) } @@ -2385,7 +2385,7 @@ func TestLogsEachSettingWithItsValue(t *testing.T) { bytesCount: "both", lookupSource: defaultLookupSource, lookupDBPath: "", - lookupTimeout: "1s", + lookupTimeout: "800ms", addLookupHeaders: "false", deniedCountries: "", allowedCountries: "", diff --git a/internal/lookup/lookup.go b/internal/lookup/lookup.go index cdd86cb..c483529 100644 --- a/internal/lookup/lookup.go +++ b/internal/lookup/lookup.go @@ -179,7 +179,7 @@ func New(params Params) *GeoJS { // own address for IPv4, and an address in the same place for an IPv6 // netblock. func (g *GeoJS) LookUp(ctx context.Context, client netip.Prefix) Answer { - answer, asked := g.answerOrWait(ctx, client) + answer, asked := g.answerOrWait(ctx, client, g.wait) if asked == nil { return answer } @@ -209,6 +209,14 @@ func (g *GeoJS) LookUp(ctx context.Context, client netip.Prefix) Answer { return answer } +// LookUpWithoutWaiting is LookUp for a request that does not need the answer, +// whatever Wait says: it returns the zero Answer at once when none is kept. +func (g *GeoJS) LookUpWithoutWaiting(ctx context.Context, client netip.Prefix) Answer { + answer, _ := g.answerOrWait(ctx, client, false) + + return answer +} + // Kept returns client's answer, if one is kept, without asking GeoJS. func (g *GeoJS) Kept(client netip.Prefix) (Answer, bool) { g.mu.Lock() @@ -263,9 +271,9 @@ func (g *GeoJS) Load(answers []Answer) { // answerOrWait returns client's kept answer if it has one. Otherwise it // puts the client among those waiting if there is room, has GeoJS asked // about them if it can be, and returns what to wait on for the answer, or -// nil when there is nothing to wait for. +// nil when there is nothing to wait for or mustWait is false. func (g *GeoJS) answerOrWait( - ctx context.Context, client netip.Prefix, + ctx context.Context, client netip.Prefix, mustWait bool, ) (Answer, <-chan struct{}) { g.mu.Lock() defer g.mu.Unlock() @@ -283,7 +291,7 @@ func (g *GeoJS) answerOrWait( g.ask(ctx) - if !g.wait { + if !mustWait { return Answer{}, nil // the answer is not needed before the request goes on } diff --git a/internal/lookup/lookup_test.go b/internal/lookup/lookup_test.go index e62d5a1..32d3497 100644 --- a/internal/lookup/lookup_test.go +++ b/internal/lookup/lookup_test.go @@ -72,6 +72,39 @@ func TestKeptAnswerIsUsedFor7DaysThenAskedAgain(t *testing.T) { }) } +func TestLookUpWithoutWaitingAsksGeoJSAndGoesOnAtOnce(t *testing.T) { + t.Parallel() + + synctest.Test(t, func(t *testing.T) { + geojs, clock, g := start() + client := netip.MustParsePrefix("203.0.113.9/32") + + // Wait is set, and GeoJS does not answer. + geojs.set(hanging) + + began := time.Now() + answer := g.LookUpWithoutWaiting(t.Context(), client) + + if waited := time.Since(began); waited != 0 || answer != (lookup.Answer{}) { + t.Errorf("got %+v after %s, want no answer at once", answer, waited) + } + + waitForRequests(t, geojs, 1) + wantAsked(t, geojs, 0, "203.0.113.9") + + // That request is abandoned, and has failed once this returns. GeoJS + // answers when it is asked again, and the answer is kept. + time.Sleep(timeout) + synctest.Wait() + geojs.set(answering) + waitForCountry(t, g, clock, client, germany) + + if got := g.LookUpWithoutWaiting(t.Context(), client).Country; got != germany { + t.Errorf("the kept answer gives the country %q, want %q", got, germany) + } + }) +} + func TestNewClientWaitsAtMostOneSecondThenCountsAsNotFound(t *testing.T) { t.Parallel() @@ -123,8 +156,8 @@ func TestRequestWaitsAsLongAsTheTimeoutSaysAndGeoJSIsAbandonedAfterIt(t *testing t.Parallel() synctest.Test(t, func(t *testing.T) { - // A timeout longer than the default second, and a GeoJS that does - // not answer. + // A timeout longer than the default, and a GeoJS that does not + // answer. const longerTimeout = 3 * time.Second m := metrics.New(1, "app") diff --git a/internal/proxy/answertime_test.go b/internal/proxy/answertime_test.go new file mode 100644 index 0000000..8520eae --- /dev/null +++ b/internal/proxy/answertime_test.go @@ -0,0 +1,600 @@ +package proxy_test + +import ( + "fmt" + "maps" + "net" + "net/http" + "net/http/httptest" + "net/netip" + "slices" + "strconv" + "strings" + "testing" + "testing/synctest" + "time" + + "sneak.berlin/go/smallwebwaf/internal/bans" + "sneak.berlin/go/smallwebwaf/internal/lookup" + "sneak.berlin/go/smallwebwaf/internal/proxy" + "sneak.berlin/go/smallwebwaf/internal/ratelimit" + "sneak.berlin/go/smallwebwaf/internal/reputation" + "sneak.berlin/go/smallwebwaf/internal/requestlog" +) + +const ( + // answerTimeLimit is the default of rproxy's SWRP_TIMEOUT_MS, the time + // servewaf has to answer. + answerTimeLimit = 2 * time.Second + // defaultLookupWait is the default of SWWAF_LOOKUP_TIMEOUT. + defaultLookupWait = 800 * time.Millisecond + // The header limits SPEC.md gives. + cookieLimit = 50 + headerFieldLimit = 100 + // cookieHeader is the header the cookie limit counts. + cookieHeader = "Cookie" + // tooLarge is how servewaf refuses a request over the header limits. + tooLarge = http.StatusRequestHeaderFieldsTooLarge + // silentAbuseIPDBURL is where AbuseIPDB never answers: at the stand-in + // that unansweredGeoJSURL names too. + silentAbuseIPDBURL = "unanswered://abuseipdb/api/v2/check" + // wholeLimit is the percentage of a rate limit that lowers it not at all. + wholeLimit = 100 + // clientsDecision is the CrowdSec decision list holding client alone. + clientsDecision = `[{"duration": "4h0m0s", "scenario": "crowdsecurity/ssh-bf", ` + + `"scope": "Ip", "type": "ban", "value": "` + client + `"}]` +) + +// cookies returns a Cookie header's value with count cookies. +func cookies(count int) string { + parts := make([]string, 0, count) + for i := range count { + parts = append(parts, "c"+strconv.Itoa(i)+"=1") + } + + return strings.Join(parts, "; ") +} + +// fillHeaders adds headers to a question until the checks see total header +// fields: all but ClientHeader and PeerHeader, which servewaf removes. Go's +// client sends nothing else but Host, which the limit does not count, once +// User-Agent is set. +func fillHeaders(req *http.Request, total int) { + req.Header.Set("User-Agent", "test") + + for i := len(req.Header) - 2; i < total; i++ { + req.Header.Set("X-Filler-"+strconv.Itoa(i), "1") + } +} + +func TestSixteenThousandCookiesAreRefusedWellWithinTheTimeLimit(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", map[string]string{wafMode: block}) + req := s.question(siteA, client, "/") + req.Header.Set(cookieHeader, strings.Repeat("a;", 16000)) + + began := time.Now() + got := do(t, req) + + if took := time.Since(began); took >= answerTimeLimit/2 { + t.Errorf("answered after %s, want well within %s", took, answerTimeLimit) + } + + wantDisposition(t, got, tooLarge, proxy.DispositionDeny) + + line := s.out.requestLine(t) + wantLine(t, line, tooLarge, requestlog.ActionTooLarge) + + if line.DurationWAF != nil { + t.Errorf("the Core Rule Set read the request, for %f ms", *line.DurationWAF) + } +} + +func TestEachHeaderLimitRefusesARequestOverIt(t *testing.T) { + t.Parallel() + + repeated := make([]string, headerFieldLimit) + for i := range repeated { + repeated[i] = "1" + } + + for _, tc := range []struct { + name string + make func(*http.Request) + // refused is true for a request over a limit, and false for one at it. + refused bool + }{ + {"cookies at the limit", func(req *http.Request) { + req.Header.Set(cookieHeader, cookies(cookieLimit)) + }, false}, + {"cookies over the limit", func(req *http.Request) { + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + }, true}, + {"parts empty or only spaces and tabs are no cookies", func(req *http.Request) { + req.Header.Set(cookieHeader, cookies(cookieLimit)+"; \t;;") + }, false}, + {"parts of no-break spaces count", func(req *http.Request) { + req.Header.Set(cookieHeader, strings.Repeat(" ;", cookieLimit+1)) + }, true}, + {"cookies over the limit across two headers", func(req *http.Request) { + req.Header[cookieHeader] = []string{cookies(cookieLimit), "last=1"} + }, true}, + {"cookies without a value count", func(req *http.Request) { + req.Header.Set(cookieHeader, strings.Repeat("a;", cookieLimit+1)) + }, true}, + {"header fields at the limit", func(req *http.Request) { + fillHeaders(req, headerFieldLimit) + }, false}, + {"header fields over the limit", func(req *http.Request) { + fillHeaders(req, headerFieldLimit+1) + }, true}, + {"a header sent many times counts each time", func(req *http.Request) { + req.Header["X-Filler"] = repeated + }, true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", map[string]string{wafMode: block}) + req := s.question(siteA, client, "/") + tc.make(req) + + got := do(t, req) + line := s.out.requestLine(t) + + if tc.refused { + wantDisposition(t, got, tooLarge, proxy.DispositionDeny) + wantLine(t, line, tooLarge, requestlog.ActionTooLarge) + + return + } + + wantDisposition(t, got, http.StatusOK, proxy.DispositionAllow) + wantLine(t, line, http.StatusOK, requestlog.ActionForward) + + if line.DurationWAF == nil { + t.Error("the Core Rule Set did not read a request within the limits") + } + }) + } +} + +func TestHeaderLimitsHoldWhateverTheCoreRuleSetSettingsSay(t *testing.T) { + t.Parallel() + + for _, env := range []map[string]string{ + {wafMode: off}, + {wafMode: "detect"}, + {wafMode: block, "SWWAF_WAF_EXEMPT_PATHS": "/"}, + } { + t.Run(fmt.Sprint(env), func(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", env) + req := s.question(siteA, client, "/") + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + wantDisposition(t, do(t, req), tooLarge, proxy.DispositionDeny) + }) + } +} + +func TestClientInTheAllowNetsSkipsTheHeaderLimits(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", map[string]string{wafMode: block, allowNets: client}) + req := s.question(siteA, client, "/") + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + wantDisposition(t, do(t, req), http.StatusOK, proxy.DispositionAllow) +} + +func TestObserveModeAllowsARequestOverTheHeaderLimitsUnread(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", map[string]string{mode: observe, wafMode: block}) + req := s.question(siteA, client, "/") + req.Header.Set(cookieHeader, strings.Repeat("a;", 16000)) + + began := time.Now() + got := do(t, req) + + if took := time.Since(began); took >= answerTimeLimit/2 { + t.Errorf("answered after %s, want well within %s", took, answerTimeLimit) + } + + wantDisposition(t, got, http.StatusOK, proxy.DispositionAllow) + + line := s.out.requestLine(t) + if line.WouldAction != requestlog.ActionTooLarge || line.DurationWAF != nil { + t.Errorf("would_action %q and duration_waf %v, want %q and none", + line.WouldAction, line.DurationWAF, requestlog.ActionTooLarge) + } +} + +func TestRequestsOverTheHeaderLimitsCountTowardTheRateLimits(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", map[string]string{rateLimitPerMinute: "1"}) + ask := func() answer { + req := s.question(siteA, client, "/") + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + return do(t, req) + } + + wantDisposition(t, ask(), tooLarge, proxy.DispositionDeny) + wantDisposition(t, ask(), http.StatusForbidden, proxy.DispositionDeny) + + lines := s.out.requestLines(t, 2) + wantLine(t, lines[1], http.StatusForbidden, requestlog.ActionRateLimited) +} + +func TestTheHeaderLimitsComeAfterTheTrapPathsAndTheRuleFiles(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name, path, action string + }{ + {"a trap path", "/wp-login.php", requestlog.ActionBanned}, + {"a block rule", "/blocked", requestlog.ActionRuleBlocked}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + s := startServeWAF(t, "", map[string]string{ + trapPaths: trapPathList, rulesDir: writeRules(t, testRules), + }) + req := s.question(siteA, client, tc.path) + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + wantDisposition(t, do(t, req), http.StatusForbidden, proxy.DispositionDeny) + wantLine(t, s.out.requestLine(t), http.StatusForbidden, tc.action) + }) + } +} + +func TestNotWaitingForGeoJSNeverLowersAClientsLimit(t *testing.T) { + t.Parallel() + + geojsURL, _ := startGeoJS(t) + s := startServeWAF(t, geojsURL, map[string]string{ + unknownLimitPercent: "0", lookupTimeout: "1h", + }) + req := s.question(siteA, fromDE, "/") + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + wantDisposition(t, do(t, req), tooLarge, proxy.DispositionDeny) + + line := s.out.requestLine(t) + wantLine(t, line, tooLarge, requestlog.ActionTooLarge) + + if line.LimitPercent != nil { + t.Errorf("limit percentage %d, want none", *line.LimitPercent) + } + + // The client is under no ban, and GeoJS places it in a country. + wantDisposition(t, s.ask(fromDE, "/"), http.StatusOK, proxy.DispositionAllow) +} + +func TestAClientGeoJSPlacedInNoCountryCountsAsUnknownWithoutWaiting(t *testing.T) { + t.Parallel() + + geojsURL, _ := startGeoJS(t) + s := startServeWAF(t, geojsURL, map[string]string{unknownLimitPercent: "0"}) + s.server.GeoJS.Load([]lookup.Answer{{ + Client: netip.MustParsePrefix(client + "/32"), ASN: "AS64512", + Answered: s.clock.Now(), Used: s.clock.Now(), + }}) + + req := s.question(siteA, client, "/") + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + wantDisposition(t, do(t, req), http.StatusForbidden, proxy.DispositionDeny) + wantLine(t, s.out.requestLine(t), http.StatusForbidden, requestlog.ActionRateLimited) +} + +func TestObserveModeWaitsForGeoJSForARequestOverTheHeaderLimits(t *testing.T) { + t.Parallel() + + geojsURL, _ := startGeoJS(t) + s := startServeWAF(t, geojsURL, map[string]string{ + mode: observe, addLookupHeaders: on, lookupTimeout: "1h", + }) + req := s.question(siteA, fromDE, "/") + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + + got := do(t, req) + wantDisposition(t, got, http.StatusOK, proxy.DispositionAllow) + + asn, country := got.header.Get("X-Client-Asn"), got.header.Get("X-Client-Country") + if asn != asnDE || country != "DE" { + t.Errorf("answered with the AS number %q and the country %q, want %q and DE", + asn, country, asnDE) + } +} + +func TestWithoutACommandTheHeaderLimitsDoNotApply(t *testing.T) { + t.Parallel() + + app := startApp(t, func(http.ResponseWriter, *http.Request) {}) + addr, out := startProxy(t, app.URL, nil) + + req := newRequest(t, http.MethodGet, addr, "/", http.NoBody) + req.Header.Set(cookieHeader, cookies(cookieLimit+1)) + fillHeaders(req, headerFieldLimit+1) + + wantStatus(t, do(t, req), http.StatusOK) + wantLine(t, out.requestLine(t), http.StatusOK, requestlog.ActionForward) +} + +// newServeWAFInBubble makes a servewaf for a synctest bubble: GeoJS never +// answers, a setting needs its answer, and everything is at its default but +// for env. No request to it may use the network. +func newServeWAFInBubble(t *testing.T, env map[string]string) (*proxy.Server, *output) { + t.Helper() + + settings := map[string]string{deniedCountries: "KP"} + maps.Copy(settings, env) + + server, out, _ := newServerWith(t, proxy.NewServeWAF, "http://app.invalid", + unansweredGeoJSURL, time.Now, settings) + + return server, out +} + +// askDirectly hands a question about a request from client straight to server, +// as change leaves it, and returns the answer's status and how long it took. +func askDirectly( + t *testing.T, server *proxy.Server, change func(*http.Request), +) (int, time.Duration) { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", http.NoBody) + req.RemoteAddr = net.JoinHostPort(rproxyPeer, "1234") + req.Header.Set(proxy.ClientHeader, client) + req.Header.Set(requestIDHeader, "the-id") + change(req) + + recorder := httptest.NewRecorder() + began := time.Now() + + server.Handler.ServeHTTP(recorder, req) + + return recorder.Code, time.Since(began) +} + +// banClient bans client at now. +func banClient(_ *testing.T, server *proxy.Server, now time.Time) { + server.Ledger.BanForLimit(netip.MustParsePrefix(client+"/32"), now, bans.Notes{}) +} + +// countClient counts one request from client at now for the rate limits. +func countClient(_ *testing.T, server *proxy.Server, now time.Time) { + server.Limiter.Count(netip.MustParsePrefix(client+"/32"), now, wholeLimit) +} + +// listClient puts client on the blocklist at dropURL. +func listClient(t *testing.T, server *proxy.Server, now time.Time) { + t.Helper() + + err := server.Lists.Load([]reputation.List{{ + URL: dropURL, Tried: now, Fetched: now, Lines: []string{client}, + }}) + if err != nil { + t.Fatalf("load the blocklist: %v", err) + } +} + +// decideOnClient puts client on the CrowdSec decision list. +func decideOnClient(t *testing.T, server *proxy.Server, now time.Time) { + t.Helper() + loadDecisions(t, server, now, clientsDecision) +} + +// leaveAsItIs prepares nothing. +func leaveAsItIs(*testing.T, *proxy.Server, time.Time) {} + +func TestARefusedRequestDoesNotWaitForASlowGeoJS(t *testing.T) { + t.Parallel() + + const forbidden = http.StatusForbidden + + for _, tc := range []struct { + name string + env map[string]string + // prepare gets the server ready, at now, before the question. + prepare func(t *testing.T, server *proxy.Server, now time.Time) + // cookie is the request's Cookie header. + cookie string + status int + action string + }{ + { + name: "SWWAF_DENY_NETS", env: map[string]string{denyNets: client}, + prepare: leaveAsItIs, status: forbidden, action: requestlog.ActionDenied, + }, + { + name: "a ban", + prepare: banClient, status: forbidden, action: requestlog.ActionBanned, + }, + { + name: "the rate limits", env: map[string]string{rateLimitPerMinute: "1"}, + prepare: countClient, status: forbidden, action: requestlog.ActionRateLimited, + }, + { + name: "the rate limits, with a biased threshold", + env: map[string]string{rateLimitPerMinute: "1", unknownLimitPercent: "50"}, + prepare: countClient, status: forbidden, action: requestlog.ActionRateLimited, + }, + { + name: "a blocklist", env: map[string]string{blocklistURLs: dropURL}, + prepare: listClient, status: forbidden, action: requestlog.ActionDenied, + }, + { + name: "the CrowdSec decision list", + env: map[string]string{crowdSecURL: lapi, crowdSecKey: bouncerKey}, + prepare: decideOnClient, status: forbidden, action: requestlog.ActionBanned, + }, + { + name: "the header limits", cookie: cookies(cookieLimit + 1), + prepare: leaveAsItIs, status: tooLarge, action: requestlog.ActionTooLarge, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // In a synctest bubble, as TestRequestWaitsAsLongAsTheLookupTimeoutSays + // says, so that a request that does not wait takes no time at all. + synctest.Test(t, func(t *testing.T) { + server, out := newServeWAFInBubble(t, tc.env) + tc.prepare(t, server, time.Now()) + + status, took := askDirectly(t, server, func(req *http.Request) { + req.Header.Set(cookieHeader, tc.cookie) + }) + + if status != tc.status || took != 0 { + t.Errorf("answered %d after %s, want %d at once; output:\n%s", + status, took, tc.status, out.text()) + } + + wantLine(t, out.requestLine(t), tc.status, tc.action) + + // GeoJS's request is abandoned before the bubble ends. + time.Sleep(defaultLookupWait) + }) + }) + } +} + +func TestAQuestionWaitsForASlowGeoJSOnlyAsLongAsTheDefaultSays(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + env map[string]string + // prepare gets the server ready, at now, before the question. + prepare func(t *testing.T, server *proxy.Server, now time.Time) + status int + action string + }{ + {"nothing refuses it", nil, leaveAsItIs, http.StatusOK, requestlog.ActionForward}, + { + "a country list needs the answer", map[string]string{allowedCountries: "DE"}, + leaveAsItIs, http.StatusForbidden, requestlog.ActionCountryDenied, + }, + { + "a blocklist that only logs", + map[string]string{blocklistURLs: dropURL, blocklistAction: actionLog}, + listClient, http.StatusOK, requestlog.ActionForward, + }, + { + "a blocklist that only limits", + map[string]string{blocklistURLs: dropURL, blocklistAction: limitQuarter}, + listClient, http.StatusOK, requestlog.ActionForward, + }, + { + "over a rate limit on an exempt path", + map[string]string{rateLimitPerMinute: "1", rateLimitExemptPaths: "/"}, + countClient, http.StatusOK, requestlog.ActionForward, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + synctest.Test(t, func(t *testing.T) { + server, out := newServeWAFInBubble(t, tc.env) + tc.prepare(t, server, time.Now()) + + status, took := askDirectly(t, server, func(*http.Request) {}) + + if status != tc.status || took != defaultLookupWait { + t.Errorf("answered %d after %s, want %d after %s; output:\n%s", + status, took, tc.status, defaultLookupWait, out.text()) + } + + wantLine(t, out.requestLine(t), tc.status, tc.action) + }) + }) + } +} + +func TestWithoutACommandARefusedRequestStillWaitsForGeoJS(t *testing.T) { + t.Parallel() + + synctest.Test(t, func(t *testing.T) { + server, out, _ := newProxy(t, "http://app.invalid", unansweredGeoJSURL, + time.Now, map[string]string{deniedCountries: "KP", blocklistURLs: dropURL}) + listClient(t, server, time.Now()) + + status, took := askDirectly(t, server, func(req *http.Request) { + req.RemoteAddr = net.JoinHostPort(client, "1234") + }) + + if status != http.StatusForbidden || took != defaultLookupWait { + t.Errorf("answered %d after %s, want %d after %s; output:\n%s", + status, took, http.StatusForbidden, defaultLookupWait, out.text()) + } + + wantLine(t, out.requestLine(t), http.StatusForbidden, requestlog.ActionDenied) + }) +} + +func TestServeWAFAnswersWithinTheTimeLimitWhileEveryOutsideSourceHangs(t *testing.T) { + t.Parallel() + + held, askedGeoJS, _ := startHeldGeoJS(t) + + // A resolver that takes every query and answers none. + resolver, err := (&net.ListenConfig{}).ListenPacket(t.Context(), "udp", localhost+":0") + if err != nil { + t.Fatalf("listen: %v", err) + } + + t.Cleanup(func() { _ = resolver.Close() }) + + server, out, _ := newServerWith(t, func(params proxy.Params) *proxy.Server { + params.AbuseIPDBURL = silentAbuseIPDBURL + + return proxy.NewServeWAF(params) + }, "http://app.invalid", held, time.Now, map[string]string{ + deniedCountries: "KP", wafMode: block, + dnsblZones: dnsblZone, dnsblResolver: resolver.LocalAddr().String(), + abuseIPDBKey: accountKey, + blocklistURLs: held + "/drop.txt", asnLimitPercentURL: held + "/asn.txt", + crowdSecURL: held, crowdSecKey: bouncerKey, + }) + + // AbuseIPDB is asked only about a client with an offence. + group := netip.MustParsePrefix(client + "/32") + server.Limiter.AddToHistory(group, time.Now(), ratelimit.Request{Attack: true}) + + go server.Lists.Run(t.Context()) + + status, took := askDirectly(t, server, func(req *http.Request) { + req.Header.Set("User-Agent", "test") + req.Header.Set("Accept", "*/*") + req.Host = siteA + }) + + if status != http.StatusOK || took >= answerTimeLimit { + t.Errorf("answered %d after %s, want %d within %s; output:\n%s", + status, took, http.StatusOK, answerTimeLimit, out.text()) + } + + // Each outside source was asked, and none has answered. The stand-in for + // GeoJS is the lists' too, which name no address. + if !slices.Contains(askedGeoJS(), client) { + t.Errorf("GeoJS was asked about %q, not %s", askedGeoJS(), client) + } + + if queries := server.DNSBL.Queries(dnsblZone); queries != 1 { + t.Errorf("%d queries to the DNS blocklist, want 1", queries) + } + + if checked := server.AbuseIPDB.Checked(); checked != 1 { + t.Errorf("%d checks with AbuseIPDB, want 1", checked) + } +} diff --git a/internal/proxy/biased.go b/internal/proxy/biased.go index 5370ff0..294cae9 100644 --- a/internal/proxy/biased.go +++ b/internal/proxy/biased.go @@ -34,19 +34,19 @@ func biasedThresholdsSet(cfg *config.Config) bool { // percentage SWWAF_ASN_LIMIT_PERCENT gives its AS number, the one the file // SWWAF_ASN_LIMIT_PERCENT_URL names gives it, the one // SWWAF_COUNTRY_LIMIT_PERCENT gives its country, for a client without a -// country, SWWAF_UNKNOWN_LIMIT_PERCENT, for a client a blocklist lists, -// the percentage of SWWAF_BLOCKLIST_ACTION while it is limit, and for a -// client a DNSBL zone's verdict lists, or whose AbuseIPDB score is a hit, -// the percentage of SWWAF_REPUTATION_ACTION while it is limit. For the -// byte limits, SWWAF_ASN_BYTES_PERCENT and SWWAF_COUNTRY_BYTES_PERCENT -// take the place of the first three for an AS number or a country they -// list. +// country, unless its request skipped the wait for GeoJS, +// SWWAF_UNKNOWN_LIMIT_PERCENT, for a client a blocklist lists, the +// percentage of SWWAF_BLOCKLIST_ACTION while it is limit, and for a client +// a DNSBL zone's verdict lists, or whose AbuseIPDB score is a hit, the +// percentage of SWWAF_REPUTATION_ACTION while it is limit. For the byte +// limits, SWWAF_ASN_BYTES_PERCENT and SWWAF_COUNTRY_BYTES_PERCENT take the +// place of the first three for an AS number or a country they list. func (rq *request) limitPercentages() (percentage, percentage) { cfg := rq.h.config asn, country := rq.line.ASN, rq.line.Country unknown := percentage{percent: whole} - if country == "" { + if country == "" && !rq.skippedLookupWait { unknown = percentage{cfg.UnknownLimitPercent, "SWWAF_UNKNOWN_LIMIT_PERCENT"} } diff --git a/internal/proxy/biased_test.go b/internal/proxy/biased_test.go index 03d4c33..cf1e5ab 100644 --- a/internal/proxy/biased_test.go +++ b/internal/proxy/biased_test.go @@ -284,7 +284,7 @@ func TestClientWithoutAnAnswerInTimeHasTheUnknownLimitPercent(t *testing.T) { server, out, _ := newProxy(t, "http://app.invalid", unansweredGeoJSURL, time.Now, map[string]string{unknownLimitPercent: "0"}) - // Once the second the request waits for its answer is up, the client + // Once the time the request waits for its answer is up, the client // counts as without a country, and its zero allowance refuses the // request before it reaches the app. serveFromDE(t, server, http.MethodGet, http.NoBody) diff --git a/internal/proxy/countries_test.go b/internal/proxy/countries_test.go index a5e558f..0725070 100644 --- a/internal/proxy/countries_test.go +++ b/internal/proxy/countries_test.go @@ -284,9 +284,9 @@ func TestExclusiveListRefusesAPrivateAddressUnlessAllowed(t *testing.T) { // each in an AS of its own, and no other address. It returns its URL, and // what returns the addresses it has been asked about. A test that needs // the stand-in to be asked or to answer sets SWWAF_LOOKUP_TIMEOUT to an -// hour, whether or not a request waits for the answer: on the default -// second, a hold-up of the test process can abandon the request to the -// stand-in, and leave the client unknown. +// hour, whether or not a request waits for the answer: on the default, a +// hold-up of the test process can abandon the request to the stand-in, and +// leave the client unknown. func startGeoJS(t *testing.T) (string, func() []string) { t.Helper() diff --git a/internal/proxy/lookup.go b/internal/proxy/lookup.go index 33190e4..1bf1fac 100644 --- a/internal/proxy/lookup.go +++ b/internal/proxy/lookup.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/netip" + "time" "sneak.berlin/go/smallwebwaf/internal/lookup" ) @@ -23,17 +24,24 @@ const ( // SWWAF_LOOKUP_SOURCE is off or the client is on a private, loopback or // link-local address, which no lookup can place. The lookup database // answers at once. With GeoJS, while a setting needs the answer, such as a -// country list or a biased threshold, a new client's request waits for it. -// ctx is the request's own context. -func (rq *request) lookUp(ctx context.Context) { +// country list or a biased threshold, a new client's request waits for it, +// unless refusedWhateverTheAnswer reports true. ctx is the request's own +// context. +func (rq *request) lookUp(ctx context.Context, now time.Time) { if rq.h.config.LookupSource == "off" || !canBePlaced(rq.client) { return } - if rq.h.config.LookupSource == "file" { - rq.lookupAnswer = rq.h.lookupFile.LookUp(rq.h.clientGroup(rq.client)) - } else { - rq.lookupAnswer = rq.h.geojs.LookUp(ctx, rq.h.clientGroup(rq.client)) + client := rq.h.clientGroup(rq.client) + + switch { + case rq.h.config.LookupSource == "file": + rq.lookupAnswer = rq.h.lookupFile.LookUp(client) + case rq.h.answersQuestions() && rq.refusedWhateverTheAnswer(now): + rq.lookupAnswer = rq.h.geojs.LookUpWithoutWaiting(ctx, client) + rq.skippedLookupWait = rq.lookupAnswer.Answered.IsZero() + default: + rq.lookupAnswer = rq.h.geojs.LookUp(ctx, client) } rq.lookedUp = true diff --git a/internal/proxy/lookup_test.go b/internal/proxy/lookup_test.go index 695c657..9ea14eb 100644 --- a/internal/proxy/lookup_test.go +++ b/internal/proxy/lookup_test.go @@ -319,7 +319,7 @@ func TestLookupHeadersArePassedToTheAppAndTheClientsOwnRemoved(t *testing.T) { addr, out, _ := startProxyWithClock(t, app.URL, geojsURL, time.Now, map[string]string{ trustedProxies: trustLocalhost, lookupTimeout: "1h", - addLookupHeaders: "true", + addLookupHeaders: on, }) s := &sender{t: t, addr: addr, out: out} @@ -383,8 +383,8 @@ func TestRequestWaitsAsLongAsTheLookupTimeoutSays(t *testing.T) { // network, which would keep that clock from moving on: the request is // handed to the proxy's handler, and GeoJS is one that never answers. synctest.Test(t, func(t *testing.T) { - // Not the default second. The exclusive list needs the answer, and - // the app is never reached. + // Not the default. The exclusive list needs the answer, and the app + // is never reached. const timeout = 3 * time.Second server, out, _ := newProxy(t, "http://app.invalid", unansweredGeoJSURL, diff --git a/internal/proxy/metrics_test.go b/internal/proxy/metrics_test.go index 401c102..c4d1634 100644 --- a/internal/proxy/metrics_test.go +++ b/internal/proxy/metrics_test.go @@ -424,8 +424,8 @@ func TestMetricsCountGeoJSRequestsAndFailures(t *testing.T) { req.Header.Set(forwardedFor, fromDE) wantStatus(t, do(t, req), http.StatusOK) - // The client stops waiting for GeoJS after a second, so GeoJS's - // failure can come after its request has ended. + // The client stops waiting for GeoJS once SWWAF_LOOKUP_TIMEOUT is up, so + // GeoJS's failure can come after its request has ended. deadline := time.Now().Add(waitLimit) metrics := scrape(t, addr) diff --git a/internal/proxy/proxy_test.go b/internal/proxy/proxy_test.go index 6af3e76..0613933 100644 --- a/internal/proxy/proxy_test.go +++ b/internal/proxy/proxy_test.go @@ -91,6 +91,9 @@ const ( // off is the value that switches a setting off. const off = "off" +// on is the value that switches a setting that takes true or false on. +const on = "true" + // output collects what smallwebwaf writes on stdout. type output struct { mu sync.Mutex diff --git a/internal/proxy/request.go b/internal/proxy/request.go index 99c38d0..d5721e8 100644 --- a/internal/proxy/request.go +++ b/internal/proxy/request.go @@ -58,6 +58,10 @@ type decision struct { // what the lookup gave then, the zero Answer while GeoJS had given none. lookedUp bool lookupAnswer lookup.Answer + // skippedLookupWait is true for a request that did not wait for GeoJS and + // found no answer kept. SWWAF_UNKNOWN_LIMIT_PERCENT does not apply to it, + // so that not waiting never lowers its client's limits. + skippedLookupWait bool // counted is true for a request the rate limits counted, whose bytes // the byte limits count once it has ended. limitPercent and // bytesPercent are then its client's limit percentages for the rate @@ -222,12 +226,12 @@ func requestHeaders(r *http.Request, names []string) map[string]string { // is known, before anything reaches the app, and before its body is read, // but for the part the Core Rule Set reads. It returns nil to let the // request through. The checks of checkClient come first, answered with -// SWWAF_BAN_RESPONSE, or 403 for a block rule or the Core Rule Set, and -// then the size limit, so that a request the rate limits count is counted -// even when it is refused for its size. In observe mode a request -// checkClient refuses goes on to the size limit like any other. A size or -// time limit the Core Rule Set's reading of the body meets ends the -// request in either mode. ctx is the request's own context. +// SWWAF_BAN_RESPONSE, 403 for a block rule or the Core Rule Set, or 431 for +// servewaf's header limits, and then the size limit, so that a request the +// rate limits count is counted even when it is refused for its size. In +// observe mode a request checkClient refuses goes on to the size limit like +// any other. A size or time limit the Core Rule Set's reading of the body +// meets ends the request in either mode. ctx is the request's own context. func (rq *request) check(ctx context.Context) *refusal { action := rq.checkClient(ctx) @@ -243,6 +247,8 @@ func (rq *request) check(ctx context.Context) *refusal { rq.line.WouldAction = action case action == requestlog.ActionRuleBlocked || action == requestlog.ActionWAFBlocked: return &refusal{status: http.StatusForbidden, action: action} + case action == requestlog.ActionTooLarge: + return &refusal{status: http.StatusRequestHeaderFieldsTooLarge, action: action} default: return rq.banResponse(action) } @@ -271,9 +277,10 @@ func (rq *request) check(ctx context.Context) *refusal { // rate limits, unless the client is in SWWAF_RATE_LIMIT_EXEMPT_NETS or the // request's path is exempt under SWWAF_RATE_LIMIT_EXEMPT_PATHS, so that // every other request is counted, each of them by the client's limit -// percentages, then SWWAF_TRAP_PATHS, then the rule files, and last the -// Core Rule Set. A request exempt from the rate limits is exempt from the -// byte limits too. ctx is the request's own context. +// percentages, then SWWAF_TRAP_PATHS, then the rule files, then, in +// servewaf, the header limits, and last the Core Rule Set. A request exempt +// from the rate limits is exempt from the byte limits too. ctx is the +// request's own context. func (rq *request) checkClient(ctx context.Context) string { cfg := rq.h.config if isInside(rq.client, cfg.AllowNets) { @@ -290,7 +297,7 @@ func (rq *request) checkClient(ctx context.Context) string { return requestlog.ActionBanned } - rq.lookUp(ctx) + rq.lookUp(ctx, now) // The lookup can wait for GeoJS, so the checks after it read the time // again: a request counted at the time it arrived could be counted @@ -314,8 +321,7 @@ func (rq *request) checkClient(ctx context.Context) string { return requestlog.ActionDenied } - rq.counted = !isInside(rq.client, cfg.RateLimitExemptNets) && - !pathExempt(rq.in, cfg.RateLimitExemptPaths) + rq.counted = rq.countedByRateLimits() if rq.counted { rq.limitPercent, rq.bytesPercent = rq.limitPercentages() rq.line.LimitPercent, rq.line.LimitPercentSetting = rq.limitPercent.logged() @@ -335,9 +341,23 @@ func (rq *request) checkClient(ctx context.Context) string { return action } + if rq.overHeaderLimits() { + return requestlog.ActionTooLarge + } + return rq.checkCoreRuleSet() } +// countedByRateLimits reports whether the rate limits count the request: its +// client is not in SWWAF_RATE_LIMIT_EXEMPT_NETS, and its path is not exempt +// under SWWAF_RATE_LIMIT_EXEMPT_PATHS. +func (rq *request) countedByRateLimits() bool { + cfg := rq.h.config + + return !isInside(rq.client, cfg.RateLimitExemptNets) && + !pathExempt(rq.in, cfg.RateLimitExemptPaths) +} + // pathExempt reports whether r is exempt under prefixes, // SWWAF_RATE_LIMIT_EXEMPT_PATHS or SWWAF_WAF_EXEMPT_PATHS: whether its // path as sent, the path the app receives, not percent-decoded, starts diff --git a/internal/proxy/servewaf.go b/internal/proxy/servewaf.go index 9fe3923..61d7aa0 100644 --- a/internal/proxy/servewaf.go +++ b/internal/proxy/servewaf.go @@ -53,6 +53,13 @@ const ReportPath = "/_smallwebwaf/report" // QuestionBodyMaxBytes is the most of a visitor's body a question carries. const QuestionBodyMaxBytes = 64 << 10 +// The header limits: the most cookies and header fields a request may have +// when the checks reach the Core Rule Set. Real clients never reach them. +const ( + maxCookies = 50 + maxHeaderFields = 100 +) + const ( // maxAwaitingReport is the most answered requests held until their // reports come. Past it, the request answered longest ago is dropped. @@ -146,6 +153,62 @@ func (h *handler) serveWAF(w http.ResponseWriter, r *http.Request) { h.answerOwnEndpoint(w, r) } +// answersQuestions reports whether h is servewaf, which alone holds requests +// for their reports. +func (h *handler) answersQuestions() bool { + return h.awaitingReport != nil +} + +// refusedWhateverTheAnswer reports whether the request is one that does not wait +// for GeoJS, since the checks will refuse it at now whatever GeoJS answers about +// its client: it is over the header limits, a blocklist lists its client while +// SWWAF_BLOCKLIST_ACTION is deny, the CrowdSec decision list lists it, or it is +// over a whole rate limit. In observe mode, which refuses nothing, none is. +// rproxy lets a request through when servewaf answers too late. +func (rq *request) refusedWhateverTheAnswer(now time.Time) bool { + if rq.h.config.Observe { + return false + } + + lists := rq.h.lists + _, decided := lists.CrowdSecDecision(rq.client, now) + + return decided || rq.overHeaderLimits() || + (rq.h.config.BlocklistAction == deny && len(lists.ListedBy(rq.client)) > 0) || + (rq.countedByRateLimits() && + rq.h.limiter.OverARateLimit(rq.h.clientGroup(rq.client), now)) +} + +// overHeaderLimits reports whether servewaf refuses the request for its +// headers: it has more than maxHeaderFields fields, each value of a header sent +// twice being one, or more than maxCookies cookies, each part of a Cookie header +// between semicolons being one unless it is empty or only spaces and tabs. The +// Core Rule Set runs each of its rules on each cookie, so that thousands of small +// cookies keep it busy for over a second. It trims only spaces and control +// characters, of which a header value can hold only tabs, so a part of Unicode +// spaces is a cookie to it. +func (rq *request) overHeaderLimits() bool { + if !rq.h.answersQuestions() { + return false + } + + fields, cookies := 0, 0 + + for _, values := range rq.in.Header { + fields += len(values) + } + + for _, value := range rq.in.Header.Values("Cookie") { + for part := range strings.SplitSeq(value, ";") { + if strings.Trim(part, " \t") != "" { + cookies++ + } + } + } + + return fields > maxHeaderFields || cookies > maxCookies +} + // answerQuestion runs the checks on the visitor's request a question // carries, and answers rproxy what to do with it. A denied request's counting // is finished here, and an allowed one's by its report. diff --git a/internal/proxy/servewaf_test.go b/internal/proxy/servewaf_test.go index b618493..2081dd7 100644 --- a/internal/proxy/servewaf_test.go +++ b/internal/proxy/servewaf_test.go @@ -767,7 +767,7 @@ func TestAllowCarriesTheLookupHeadersWhileTheSettingIsSet(t *testing.T) { setting string wantASN, wantCountry string }{ - {"true", "AS64496", "DE"}, + {on, "AS64496", "DE"}, {"false", "", ""}, } { t.Run(tc.setting, func(t *testing.T) { diff --git a/internal/ratelimit/ratelimit.go b/internal/ratelimit/ratelimit.go index 2318102..73e35ee 100644 --- a/internal/ratelimit/ratelimit.go +++ b/internal/ratelimit/ratelimit.go @@ -228,6 +228,30 @@ func (l *Limiter) Count( return l.count(client, now, 1, 0, percent) } +// OverARateLimit reports whether one more request from client at now would +// take it over a whole rate limit. A client's percentage can only lower a +// limit, so that request is over the limit at any percentage. It counts +// nothing, and does not make the client the most recently seen. +func (l *Limiter) OverARateLimit(client netip.Prefix, now time.Time) bool { + l.mu.Lock() + defer l.mu.Unlock() + + c, seen := l.clients.Peek(client) + if !seen { + return false + } + + buckets := c.buckets() + + for i, w := range l.windows { + if w.limit > 0 && buckets[i].Add(now, w.length, 0)+1 > float64(w.limit) { + return true + } + } + + return false +} + // CountBytes counts bytes, those of a request from client that has ended, // at now, in every window, and returns the client's counts in each window. // It reports whether the bytes take the client over a byte limit, of which diff --git a/internal/ratelimit/ratelimit_test.go b/internal/ratelimit/ratelimit_test.go index 79f3b59..5791f48 100644 --- a/internal/ratelimit/ratelimit_test.go +++ b/internal/ratelimit/ratelimit_test.go @@ -26,6 +26,50 @@ const ( hour = "hour" ) +func TestOverARateLimitSaysWhatTheNextRequestWouldDoAndCountsNothing(t *testing.T) { + t.Parallel() + + client := netip.MustParsePrefix("203.0.113.9/32") + now := midnight() + limiter := ratelimit.New(ratelimit.Limits{PerMinute: limit, PerDay: limit + 1}, + tableSize) + + if limiter.OverARateLimit(client, now) { + t.Error("a client never seen is over a rate limit") + } + + if limiter.Len() != 0 { + t.Error("asking about a client never seen added it to the table") + } + + for range limit { + if limiter.OverARateLimit(client, now) { + t.Error("a client with room for another request is over a rate limit") + } + + wantCount(t, limiter, client, now, "") + } + + if !limiter.OverARateLimit(client, now) { + t.Error("a client at its limit is not over a rate limit") + } + + held, _ := limiter.Client(client) + if held.Minute.Current != limit { + t.Errorf("the client's minute holds %d requests, want %d", held.Minute.Current, limit) + } + + wantCount(t, limiter, client, now, minute) + + // A limit that is off is never gone over. + off := ratelimit.New(ratelimit.Limits{}, tableSize) + off.Count(client, now, whole) + + if off.OverARateLimit(client, now) { + t.Error("a client is over a rate limit that is off") + } +} + func TestEachWindowRefusesAtItsLimitAndLetsTheClientBack(t *testing.T) { t.Parallel() diff --git a/internal/requestlog/requestlog.go b/internal/requestlog/requestlog.go index c05c0c2..6ddf8e7 100644 --- a/internal/requestlog/requestlog.go +++ b/internal/requestlog/requestlog.go @@ -18,7 +18,8 @@ import ( const ( // ActionForward is a request passed to the app. ActionForward = "forward" - // ActionTooLarge is a request or response over its size limit. + // ActionTooLarge is a request or response over its size limit, or, in + // servewaf, a request over the header limits. ActionTooLarge = "too_large" // ActionTimedOut is a request or response that ran out of time. ActionTimedOut = "timed_out" @@ -139,7 +140,8 @@ type Line struct { // WouldAction is, in observe mode, the action enforce mode would have // taken with a request it would have refused: ActionDenied, // ActionBanned, ActionCountryDenied, ActionRateLimited, - // ActionRuleBlocked or ActionWAFBlocked. + // ActionRuleBlocked, ActionWAFBlocked, or, in servewaf, ActionTooLarge + // for the header limits. WouldAction string `json:"would_action,omitempty"` // LimitPercent and LimitPercentSetting are, for a request the rate // limits counted whose client a biased threshold gives a percentage of