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

@@ -1149,8 +1149,7 @@ waits on these, which run in the background:
 - fetching the blocklists, the CrowdSec decision list and the file
   `SWWAF_ASN_LIMIT_PERCENT_URL` names.
 
-A question does not wait for GeoJS when the checks will refuse it whatever GeoJS
-answers. That is a request:
+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
@@ -1162,19 +1161,24 @@ answers. That is a request:
 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 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.
+  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.
 

internal/proxy/answertime_test.go

@@ -15,6 +15,7 @@ import (
 	"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"
@@ -110,9 +111,12 @@ func TestEachHeaderLimitRefusesARequestOverIt(t *testing.T) {
 		{"cookies over the limit", func(req *http.Request) {
 			req.Header.Set(cookieHeader, cookies(cookieLimit+1))
 		}, true},
-		{"empty parts are no cookies", func(req *http.Request) {
-			req.Header.Set(cookieHeader, cookies(cookieLimit)+"; ;;")
+		{"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},
@@ -227,6 +231,90 @@ func TestRequestsOverTheHeaderLimitsCountTowardTheRateLimits(t *testing.T) {
 	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()
 
@@ -385,15 +473,32 @@ func TestAQuestionWaitsForASlowGeoJSOnlyAsLongAsTheDefaultSays(t *testing.T) {
 	t.Parallel()
 
 	for _, tc := range []struct {
-		name   string
-		env    map[string]string
-		status int
-		action string
+		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, http.StatusOK, requestlog.ActionForward},
+		{"nothing refuses it", nil, leaveAsItIs, http.StatusOK, requestlog.ActionForward},
 		{
 			"a country list needs the answer", map[string]string{allowedCountries: "DE"},
-			http.StatusForbidden, requestlog.ActionCountryDenied,
+			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) {
@@ -401,6 +506,8 @@ func TestAQuestionWaitsForASlowGeoJSOnlyAsLongAsTheDefaultSays(t *testing.T) {
 
 			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 {
@@ -414,6 +521,27 @@ func TestAQuestionWaitsForASlowGeoJSOnlyAsLongAsTheDefaultSays(t *testing.T) {
 	}
 }
 
+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()
 

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"}
 	}
 

internal/proxy/lookup.go

@@ -25,8 +25,8 @@ const (
 // 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,
-// unless servewaf will refuse the request whatever the answer says. ctx is
-// the request's own context.
+// 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
@@ -39,6 +39,7 @@ func (rq *request) lookUp(ctx context.Context, now time.Time) {
 		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)
 	}

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}
 

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

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

internal/proxy/servewaf.go

@@ -159,13 +159,17 @@ func (h *handler) answersQuestions() bool {
 	return h.awaitingReport != nil
 }
 
-// refusedWhateverTheAnswer reports whether the checks will refuse the request
-// 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. rproxy
-// lets a request through when servewaf answers too late, so such a request
-// does not wait for GeoJS.
+// 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)
 
@@ -178,9 +182,11 @@ func (rq *request) refusedWhateverTheAnswer(now time.Time) bool {
 // 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 that is not empty being one. 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.
+// 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
@@ -194,7 +200,7 @@ func (rq *request) overHeaderLimits() bool {
 
 	for _, value := range rq.in.Header.Values("Cookie") {
 		for part := range strings.SplitSeq(value, ";") {
-			if strings.TrimSpace(part) != "" {
+			if strings.Trim(part, " \t") != "" {
 				cookies++
 			}
 		}

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) {