waf: the header limits are the first check (closes #311)

A request over 50 cookies or 100 header fields is now refused by the first
check after SWW_ALLOW_NETS. It causes no lookup, no query to RouteWatch, a DNS
blocklist or AbuseIPDB, no rule file match and no Core Rule Set. The rate
limits no longer count it, and a banned or denied client gets 431 for it. It
makes no ban, makes no ban permanent and is not counted in a ban's notes. In
observe mode no lookup is made for it. Requests within the limits go through
the same checks in the same order.

Model: opus-5-5

README.md

@@ -509,7 +509,8 @@ request reaches the app.
 
 `waf` takes the checks:
 
-3. The tag lists.
+3. The header limits, then the tag lists. A client in `SWW_ALLOW_NETS` skips the
+   header limits.
 4. The static lists, `SWW_ALLOW_NETS` and `SWW_DENY_NETS`.
 5. The bans.
 6. The lookup of the client's AS number and country.
@@ -521,7 +522,7 @@ request reaches the app.
 11. The rate limits.
 12. The trap paths.
 13. The rule files.
-14. The header limits, and then the Core Rule Set.
+14. The Core Rule Set.
 15. The tag refusals again, so that `exploitsignature` can refuse too.
 16. A path under `/_sww/` is answered `404`, so that the app never sees that
     prefix.
@@ -537,7 +538,7 @@ request reaches the app.
 A refused visitor is answered with `SWW_BAN_RESPONSE`. A `block` rule or a Core
 Rule Set match answers `403` and bans nobody, since the Core Rule Set has false
 positives; such refusals count toward `SWW_ERROR_BURST_THRESHOLD` instead. A
-request over the header limits of step 14 is answered `431`, and bans nobody
+request over the header limits of step 3 is answered `431`, and bans nobody
 either. `rproxy` itself answers `405` to a method it does not pass on, and the
 status for a size or time limit, such as `413` for a body too large.
 

SPEC.md

@@ -145,10 +145,12 @@ field gives that name. `refusal_limit` and `byte_limits` give theirs in
     - An IPv6 client is its group: the netblock of `SWW_IPV6_GROUP_PREFIX`
       around its address, a /64 by default, since one abuser usually controls a
       whole /64. A client is therefore one IPv4 address or one IPv6 group.
-3. The tag lists (`tag_lists`; see "Request tags"). Each list that holds the
+3. The header limits (`header_limits`; see "Answering in time"). They come first
-   client adds its tag. Next, `routewatch_tags` adds the client's `asn=` and
+   after `SWW_ALLOW_NETS`, since they only count header fields and cookies.
+   Next, the tag lists (`tag_lists`; see "Request tags"). Each list that holds
+   the client adds its tag. Next, `routewatch_tags` adds the client's `asn=` and
    `prefix=` tags while `SWW_ROUTEWATCH_URL` is set. A client in
-   `SWW_ALLOW_NETS` gets no tag.
+   `SWW_ALLOW_NETS` skips the header limits and gets no tag.
 4. The static lists. A client in `SWW_ALLOW_NETS` skips steps 5 to 15, the byte
    limits and the error burst. A client in `SWW_DENY_NETS` is refused
    (`deny_nets`); a client in both lists is let through.
@@ -181,10 +183,9 @@ field gives that name. `refusal_limit` and `byte_limits` give theirs in
     refused and bans the client.
 13. The rule files (`rule_files`; see "Rule files"). Matching stops at the first
     rule that refuses.
-14. The header limits (`header_limits`; see "Answering in time"), and then the
+14. The Core Rule Set (`core_rule_set`; see "Attack detection"). A match is
-    Core Rule Set (`core_rule_set`; see "Attack detection"). A Core Rule Set
+    refused with `403` while `SWW_WAF_MODE` is `block`, and only logged and
-    match is refused with `403` while `SWW_WAF_MODE` is `block`, and only logged
+    alerted while it is `detect`.
-    and alerted while it is `detect`.
 15. The tag refusals again (`tag_deny`), so that `exploitsignature` can refuse
     too.
 16. A path under `/_sww/` is answered `404`, so that the app never sees that
@@ -237,11 +238,16 @@ apply.
 ## Bans
 
 Until a ban ends, every request from its netblock is refused with
-`SWW_BAN_RESPONSE`. The netblock is the client: its IPv4 address, widened to
+`SWW_BAN_RESPONSE`, unless it is over the header limits. The netblock is the
-`SWW_BAN_SCOPE_V4_PREFIX`, or its IPv6 group. At the defaults a ban covers one
+client: its IPv4 address, widened to `SWW_BAN_SCOPE_V4_PREFIX`, or its IPv6
-address or one /64, so a permanent ban does not reach neighbours who did
+group. At the defaults a ban covers one address or one /64, so a permanent ban
-nothing. Requests refused under a ban are not counted against limits. They are
+does not reach neighbours who did nothing. Requests refused under a ban are not
-counted in the client's history and in the ban's notes.
+counted against limits. They are counted in the client's history and in the
+ban's notes.
+
+A request over the header limits is refused with `431` before the bans are
+checked (see "Answering in time"). It makes no ban, makes no ban permanent and
+is not counted in a ban's notes.
 
 An offence is a request `waf` holds against the client. It is one of:
 
@@ -2020,7 +2026,6 @@ waits on these, which run in the background:
 
 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 `SWW_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
@@ -2032,8 +2037,8 @@ number and country stay unknown until GeoJS has answered. So while
 `SWW_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
+The header limits, the static lists and bans come before the lookup, so they
-still wait for GeoJS:
+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;
@@ -2052,13 +2057,17 @@ either one:
 
 Real clients never reach these limits. The answer is `deny` with `431`, and the
 log line's action is `request_header_too_large`. The refusal makes no ban and is
-no offence. The rate limits count the request as usual, so a client that keeps
+no offence.
-sending such requests breaks a limit.
+
+The header limits are the first check after `SWW_ALLOW_NETS`. A request over
+them is refused before any later check runs. So it causes no lookup, no query to
+an outside source and no rule match, and the rate limits do not count it. A
+client under a ban or in `SWW_DENY_NETS` also gets `431` for such a request.
 
 The limits apply on every path and in every `SWW_WAF_MODE`. A client in
 `SWW_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
+such a request is answered `allow` with `would_action`, and no other check runs
-does not read it.
+on 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

internal/waf/answertime_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/netip"
+	"path/filepath"
 	"slices"
 	"strconv"
 	"strings"
@@ -16,6 +17,7 @@ import (
 
 	"sneak.berlin/go/smallwebwaf/internal/bans"
 	"sneak.berlin/go/smallwebwaf/internal/lookup"
+	"sneak.berlin/go/smallwebwaf/internal/lookup/lookuptest"
 	"sneak.berlin/go/smallwebwaf/internal/ratelimit"
 	"sneak.berlin/go/smallwebwaf/internal/reputation"
 	"sneak.berlin/go/smallwebwaf/internal/requestlog"
@@ -216,93 +218,164 @@ func TestObserveModeAllowsARequestOverTheHeaderLimitsUnread(t *testing.T) {
 	}
 }
 
-func TestRequestsOverTheHeaderLimitsCountTowardTheRateLimits(t *testing.T) {
+func TestTheRateLimitsDoNotCountRequestsOverTheHeaderLimits(t *testing.T) {
 	t.Parallel()
 
 	s := startWAF(t, "", map[string]string{rateLimitPerMinute: "1"})
-	ask := func() answer {
+
+	for range 2 {
 		req := s.question(siteA, client, "/")
 		req.Header.Set(cookieHeader, cookies(cookieLimit+1))
-
+		wantDisposition(t, do(t, req), tooLarge, waf.DispositionDeny)
-		return do(t, req)
 	}
 
-	wantDisposition(t, ask(), tooLarge, waf.DispositionDeny)
+	wantDisposition(t, s.ask(client, "/"), http.StatusOK, waf.DispositionAllow)
-	wantDisposition(t, ask(), http.StatusForbidden, waf.DispositionDeny)
-
-	lines := s.out.requestLines(t, 2)
-	wantLine(t, lines[1], http.StatusForbidden, requestlog.ActionClientIsRateLimited)
 }
 
-func TestTheHeaderLimitsComeAfterTheTrapPathsAndTheRuleFiles(t *testing.T) {
+func TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses(t *testing.T) {
 	t.Parallel()
 
 	for _, tc := range []struct {
-		name, path, action string
+		name string
+		env  map[string]string
+		path string
+		// prepare gets the server ready, at now, before the question.
+		prepare func(t *testing.T, server *waf.Server, now time.Time)
 	}{
-		{"a trap path", "/wp-login.php", requestlog.ActionTemporarilyBanned},
+		{"SWW_DENY_NETS", map[string]string{denyNets: client}, "/", leaveAsItIs},
-		{"a block rule", "/blocked", requestlog.ActionBlockedByBlockRule},
+		{"a ban", nil, "/", banClient},
+		{"a blocklist", map[string]string{blocklistURLs: dropURL}, "/", listClient},
+		{
+			"the CrowdSec decision list",
+			map[string]string{crowdSecURL: lapi, crowdSecKey: bouncerKey}, "/", decideOnClient,
+		},
+		{"the rate limits", map[string]string{rateLimitPerMinute: "1"}, "/", countClient},
+		{
+			"a trap path", map[string]string{trapPaths: trapPathList}, "/wp-login.php",
+			leaveAsItIs,
+		},
+		{
+			"a block rule", map[string]string{rulesDir: writeRules(t, testRules)}, "/blocked",
+			leaveAsItIs,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			s := startWAF(t, "", map[string]string{
+			s := startWAF(t, "", tc.env)
-				trapPaths: trapPathList, rulesDir: writeRules(t, testRules),
+			tc.prepare(t, s.server, s.clock.Now())
-			})
+
 			req := s.question(siteA, client, tc.path)
 			req.Header.Set(cookieHeader, cookies(cookieLimit+1))
 
-			wantDisposition(t, do(t, req), http.StatusForbidden, waf.DispositionDeny)
+			wantDisposition(t, do(t, req), tooLarge, waf.DispositionDeny)
-			wantLine(t, s.out.requestLine(t), http.StatusForbidden, tc.action)
+			wantLine(t, s.out.requestLine(t), tooLarge, requestlog.ActionRequestHeaderTooLarge)
 		})
 	}
 }
 
+func TestARequestOverTheHeaderLimitsRunsNoOtherCheck(t *testing.T) {
+	t.Parallel()
+
+	// Each check after the header limits leaves a mark on the log line of a
+	// request it reads: a tag, routewatch_tags_missing, a rule id, the
+	// counts, or the Core Rule Set's time.
+	path := filepath.Join(t.TempDir(), "ipinfo_lite.mmdb")
+	lookuptest.Write(t, path, map[string]lookuptest.Network{
+		client + "/32": {ASN: asnDE, ASName: asNameDE, Country: "DE"},
+	})
+	s := startWAF(t, "", map[string]string{
+		lookupSource: fileSource, lookupDBPath: path,
+		tagLists:      "vpn=" + vpnList,
+		routeWatchURL: startRouteWatch(t, map[string]standInRoute{client: {prefixDE, 64496}}),
+		dnsblZones:    dnsblZone, reputationAction: actionLog,
+		rulesDir: writeRules(t, "noted path log ^/\n"),
+		wafMode:  block,
+	})
+	loadLists(t, s.server, map[string][]string{vpnList: {client}})
+	loadVerdicts(s.server, map[string][]string{client: {dnsblZone}})
+
+	over := s.question(siteA, client, "/")
+	over.Header.Set(cookieHeader, cookies(cookieLimit+1))
+	wantDisposition(t, do(t, over), tooLarge, waf.DispositionDeny)
+
+	within := s.question(siteA, client, "/")
+	within.Header.Set(cookieHeader, cookies(cookieLimit))
+	wantDisposition(t, do(t, within), http.StatusOK, waf.DispositionAllow)
+
+	lines := s.out.requestLines(t, 2)
+
+	wantTags(t, lines[0])
+	wantTagsMissing(t, lines[0], "")
+
+	if lines[0].RuleIDs != nil || lines[0].Counts != (ratelimit.Counts{}) ||
+		lines[0].DurationCoreRuleSetMS != nil {
+		t.Errorf("rule ids %v, counts %+v and Core Rule Set time %v, want none",
+			lines[0].RuleIDs, lines[0].Counts, lines[0].DurationCoreRuleSetMS)
+	}
+
+	// The request within the limits shows that each check leaves its mark.
+	wantTags(t, lines[1],
+		"asn=64496", "country=DE", "dnsbl="+dnsblZone, "exploitsignature", "vpn")
+	wantTagsMissing(t, lines[1], "no_answer_yet")
+
+	if lines[1].RuleIDs == nil || lines[1].Counts.Minute != 1 ||
+		lines[1].DurationCoreRuleSetMS == nil {
+		t.Errorf("rule ids %v, counts %+v and Core Rule Set time %v, want each",
+			lines[1].RuleIDs, lines[1].Counts, lines[1].DurationCoreRuleSetMS)
+	}
+}
+
 func TestNotWaitingForGeoJSNeverLowersAClientsLimit(t *testing.T) {
 	t.Parallel()
 
+	// The client's next request takes it over the whole limit, so it does
+	// not wait.
 	geojsURL, _ := startGeoJS(t)
 	s := startWAF(t, geojsURL, map[string]string{
-		unknownLimitPercent: "0", lookupTimeout: "1h",
+		unknownLimitPercent: "0", lookupTimeout: "1h", rateLimitPerMinute: "1",
 	})
-	req := s.question(siteA, fromDE, "/")
+	countClient(t, s.server, s.clock.Now())
-	req.Header.Set(cookieHeader, cookies(cookieLimit+1))
 
-	wantDisposition(t, do(t, req), tooLarge, waf.DispositionDeny)
+	wantDisposition(t, s.ask(fromDE, "/"), http.StatusForbidden, waf.DispositionDeny)
 
 	line := s.out.requestLine(t)
-	wantLine(t, line, tooLarge, requestlog.ActionRequestHeaderTooLarge)
+	wantLine(t, line, http.StatusForbidden, requestlog.ActionClientIsRateLimited)
 
 	if line.RateLimitPercent != nil {
 		t.Errorf("limit percentage %d, want none", *line.RateLimitPercent)
 	}
-
-	// The client is under no ban, and GeoJS places it in a country.
-	wantDisposition(t, s.ask(fromDE, "/"), http.StatusOK, waf.DispositionAllow)
 }
 
 func TestAClientGeoJSPlacedInNoCountryCountsAsUnknownWithoutWaiting(t *testing.T) {
 	t.Parallel()
 
+	// The client's next request takes it over the whole limit, so it does
+	// not wait.
 	geojsURL, _ := startGeoJS(t)
-	s := startWAF(t, geojsURL, map[string]string{unknownLimitPercent: "0"})
+	s := startWAF(t, geojsURL, map[string]string{
+		unknownLimitPercent: "0", rateLimitPerMinute: "1",
+	})
 	s.server.GeoJS.Load([]lookup.Answer{{
 		Client: netip.MustParsePrefix(client + "/32"), ASN: "AS64512",
 		Answered: s.clock.Now(), Used: s.clock.Now(),
 	}})
+	countClient(t, s.server, s.clock.Now())
 
-	req := s.question(siteA, client, "/")
+	wantDisposition(t, s.ask(client, "/"), http.StatusForbidden, waf.DispositionDeny)
-	req.Header.Set(cookieHeader, cookies(cookieLimit+1))
 
-	wantDisposition(t, do(t, req), http.StatusForbidden, waf.DispositionDeny)
+	line := s.out.requestLine(t)
-	wantLine(t, s.out.requestLine(t), http.StatusForbidden,
+	wantLine(t, line, http.StatusForbidden, requestlog.ActionClientIsRateLimited)
-		requestlog.ActionClientIsRateLimited)
+
+	if line.RateLimitPercent == nil || *line.RateLimitPercent != 0 {
+		t.Errorf("limit percentage %v, want 0", line.RateLimitPercent)
+	}
 }
 
-func TestObserveModeWaitsForGeoJSForARequestOverTheHeaderLimits(t *testing.T) {
+func TestObserveModeAsksGeoJSNothingForARequestOverTheHeaderLimits(t *testing.T) {
 	t.Parallel()
 
-	geojsURL, _ := startGeoJS(t)
+	geojsURL, asked := startGeoJS(t)
 	s := startWAF(t, geojsURL, map[string]string{
 		mode: observe, addLookupHeaders: on, lookupTimeout: "1h",
 	})
@@ -312,10 +385,9 @@ func TestObserveModeWaitsForGeoJSForARequestOverTheHeaderLimits(t *testing.T) {
 	got := do(t, req)
 	wantDisposition(t, got, http.StatusOK, waf.DispositionAllow)
 
-	asn, country := got.header.Get("X-Client-Asn"), got.header.Get("X-Client-Country")
+	if len(asked()) != 0 || got.header.Get("X-Client-Asn") != "" {
-	if asn != asnDE || country != "DE" {
+		t.Errorf("GeoJS was asked about %v, and the answer gave the AS number %q, "+
-		t.Errorf("answered with the AS number %q and the country %q, want %q and DE",
+			"want neither", asked(), got.header.Get("X-Client-Asn"))
-			asn, country, asnDE)
 	}
 }
 

internal/waf/headerlimits.go

@@ -10,22 +10,23 @@ import (
 	"sneak.berlin/go/smallwebwaf/internal/requestlog"
 )
 
-// The header limits: the most cookies and header fields a request may have
+// 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.
+// Real clients never reach them.
 const (
 	maxCookies      = 50
 	maxHeaderFields = 100
 )
 
-// headerLimits refuses a request over the header limits, with 431. Its figures
+// headerLimits refuses a request with more than maxHeaderFields header fields
-// are the request's header fields and cookies, and the limit of each.
+// or more than maxCookies cookies, with 431. Its figures are the request's
+// header fields and cookies, and the limit of each.
 type headerLimits struct{}
 
 func (headerLimits) name() string { return "header_limits" }
 
 func (headerLimits) check(_ context.Context, rq *request, _ time.Time) *verdict {
 	fields, cookies := rq.headerCounts()
-	if !overHeaderLimits(fields, cookies) {
+	if fields <= maxHeaderFields && cookies <= maxCookies {
 		return nil
 	}
 
@@ -38,13 +39,6 @@ func (headerLimits) check(_ context.Context, rq *request, _ time.Time) *verdict
 	}
 }
 
-// overHeaderLimits reports whether a request with these counts of header fields
-// and cookies, as headerCounts counts them, is over the header limits: it has
-// more than maxHeaderFields fields or more than maxCookies cookies.
-func overHeaderLimits(fields, cookies int) bool {
-	return fields > maxHeaderFields || cookies > maxCookies
-}
-
 // headerCounts returns the request's header fields, each value of a header sent
 // twice being one, and its cookies, each part of a Cookie header between
 // semicolons being one unless it is empty or only spaces and tabs. The Core

internal/waf/heuristics.go

@@ -41,8 +41,11 @@ const (
 
 // beforeLookup are the heuristics that run before the client's AS number and
 // country are looked up, so that a client they refuse is not looked up.
+// header_limits comes first: it only counts, and a request it refuses causes no
+// other work.
 func beforeLookup() []heuristic {
 	return []heuristic{
+		headerLimits{},
 		tagLists{},
 		routeWatchTags{},
 		denyNets{},
@@ -64,7 +67,6 @@ func afterLookup() []heuristic {
 		rateLimits{},
 		trapPaths{},
 		ruleFiles{},
-		headerLimits{},
 		coreRuleSet{},
 		tagDeny{},
 	}

internal/waf/waf.go

@@ -143,10 +143,9 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 // 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: the heuristic crowdsec, header_limits, blocklists or rate_limits
+// its client: the heuristic crowdsec, blocklists or rate_limits will refuse it,
-// will refuse it, as a function in that heuristic's file says. In observe mode,
+// as a function in that heuristic's file says. In observe mode, which refuses
-// which refuses nothing, none is. rproxy lets a request through when
+// nothing, none is. rproxy lets a request through when waf answers too late.
-// waf answers too late.
 func (rq *request) refusedWhateverTheAnswer(now time.Time) bool {
 	if rq.h.config.Observe {
 		return false
@@ -154,8 +153,7 @@ func (rq *request) refusedWhateverTheAnswer(now time.Time) bool {
 
 	_, decided := rq.crowdSecDecisionInForce(now)
 
-	return decided || overHeaderLimits(rq.headerCounts()) ||
+	return decided || rq.refusedByBlocklists(rq.h.lists.ListedBy(rq.client)) ||
-		rq.refusedByBlocklists(rq.h.lists.ListedBy(rq.client)) ||
 		rq.overAWholeRateLimit(now)
 }