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. It does not wait at all when the checks will refuse
it whatever GeoJS answers. 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,7 +1149,8 @@ waits on these, which run in the background:
 - 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:
+A question does not wait for GeoJS when the checks will refuse it whatever GeoJS
+answers. That is a request:
 
 - over the header limits;
 - from a client on a fetched blocklist, while `SWWAF_BLOCKLIST_ACTION` is
@@ -1161,24 +1162,19 @@ In `enforce` mode, a question does not wait for GeoJS when it 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 trap path;
-- a rule file;
-- the Core Rule Set.
+- a DNS blocklist or AbuseIPDB, while `SWWAF_REPUTATION_ACTION` is `deny`.
 
 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.
+  counts as one, unless it is empty.
 - 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,7 +15,6 @@ 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"
@@ -111,12 +110,9 @@ func TestEachHeaderLimitRefusesARequestOverIt(t *testing.T) {
 		{"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;;")
+		{"empty parts are no cookies", func(req *http.Request) {
+			req.Header.Set(cookieHeader, cookies(cookieLimit)+"; ;;")
 		}, 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},
@@ -231,90 +227,6 @@ 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()
 
@@ -473,32 +385,15 @@ 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
+		name   string
+		env    map[string]string
+		status int
+		action string
 	}{
-		{"nothing refuses it", nil, leaveAsItIs, http.StatusOK, requestlog.ActionForward},
+		{"nothing refuses it", nil, 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,
+			http.StatusForbidden, requestlog.ActionCountryDenied,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -506,8 +401,6 @@ 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 {
@@ -521,27 +414,6 @@ 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, 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.
+// 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.
 func (rq *request) limitPercentages() (percentage, percentage) {
 	cfg := rq.h.config
 	asn, country := rq.line.ASN, rq.line.Country
 
 	unknown := percentage{percent: whole}
-	if country == "" && !rq.skippedLookupWait {
+	if country == "" {
 		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 refusedWhateverTheAnswer reports true. ctx is the request's own
-// context.
+// unless servewaf will refuse the request whatever the answer says. 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,7 +39,6 @@ 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: on,
+		addLookupHeaders: "true",
 	})
 	s := &sender{t: t, addr: addr, out: out}
 

internal/proxy/proxy_test.go

@@ -91,9 +91,6 @@ 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,10 +58,6 @@ 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,17 +159,13 @@ 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.
+// 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.
 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)
 
@@ -182,11 +178,9 @@ 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 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.
+// 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.
 func (rq *request) overHeaderLimits() bool {
 	if !rq.h.answersQuestions() {
 		return false
@@ -200,7 +194,7 @@ func (rq *request) overHeaderLimits() bool {
 
 	for _, value := range rq.in.Header.Values("Cookie") {
 		for part := range strings.SplitSeq(value, ";") {
-			if strings.Trim(part, " \t") != "" {
+			if strings.TrimSpace(part) != "" {
 				cookies++
 			}
 		}

internal/proxy/servewaf_test.go

@@ -767,7 +767,7 @@ func TestAllowCarriesTheLookupHeadersWhileTheSettingIsSet(t *testing.T) {
 		setting              string
 		wantASN, wantCountry string
 	}{
-		{on, "AS64496", "DE"},
+		{"true", "AS64496", "DE"},
 		{"false", "", ""},
 	} {
 		t.Run(tc.setting, func(t *testing.T) {