Take in an admin's edits of the state files while running (closes #68)

smallwebwaf watches SWWAF_STATE_DIR with fsnotify and takes in a saved
edit of a state file in place of what it held. It knows its own writes
by the SHA-256 of what it last read or wrote; each write first takes in
an edit made since. An edit that does not parse is renamed to
<name>.bad at the next write. Each edit taken in or set aside is logged
and counted. Every ban on a netblock is checked, and the next ban is
worked out from the one that ended last. README.md says how to add and
lift a ban.

Judgement call: a broken edit is set aside at the next write, since an
editor's file can be read half written.

Model: opus-5-5

README.md

@@ -13,23 +13,21 @@ JSON log line for every request.
 
 Status: the first two milestones are built
 (https://git.eeqj.de/sneak/smallwebwaf/issues/13 and
-https://git.eeqj.de/sneak/smallwebwaf/issues/14), and so are six parts of
+https://git.eeqj.de/sneak/smallwebwaf/issues/14), and so are five parts of
 milestone 3: the static lists, the bans that broken rate limits lead to and the
 JSON state files with your edits taken in while it runs, which come next in the
-build order, `observe` mode, which comes a little later, and the metrics
+build order, and the metrics endpoint and the header size and the idle time as
-endpoint and the header size and the idle time as settings, which come last in
+settings, which come last in it. `smallwebwaf` passes each request to the app
-it. `smallwebwaf` passes each request to the app and the app's answer back,
+and the app's answer back, unchanged, within its timeouts and size limits, works
-unchanged, within its timeouts and size limits, works out each client's address,
+out each client's address, bans a client that sends too many requests, refuses a
-bans a client that sends too many requests, refuses a client that comes from a
+client that comes from a country you refuse or from a network you refuse, lets
-country you refuse or from a network you refuse, lets the networks you choose
+the networks you choose through, keeps its bans, each client's counters and
-through, keeps its bans, each client's counters and history, and GeoJS's answers
+history, and GeoJS's answers in JSON files across restarts, takes in your edits
-in JSON files across restarts, takes in your edits of those files while it runs,
+of those files while it runs, writes a JSON log line for every request, and
-writes a JSON log line for every request, serves Prometheus metrics to a scraper
+serves Prometheus metrics to a scraper that holds the metrics token. It comes as
-that holds the metrics token, and in `observe` mode passes on the requests it
+the image the app's own image is built on. The rest of the design comes after
-would refuse, logging what it would have done with them. It comes as the image
+that, in the order of the build order in [`SPEC.md`](SPEC.md). The survey of
-the app's own image is built on. The rest of the design comes after that, in the
+existing tools that led to the design is in [`EVALUATION.md`](EVALUATION.md).
-order of the build order in [`SPEC.md`](SPEC.md). The survey of existing tools
-that led to the design is in [`EVALUATION.md`](EVALUATION.md).
 
 ## Getting started
 
@@ -121,18 +119,6 @@ in `bin/state` unless `SWWAF_STATE_DIR` is set.
   `SWWAF_ALLOW_NETS` too is let through. A client in
   `SWWAF_RATE_LIMIT_EXEMPT_NETS` is neither counted nor refused by the rate
   limits; the country lists and bans still apply to it.
-- In `observe` mode, with `SWWAF_MODE=observe`, refuses none of the requests
-  that `SWWAF_DENY_NETS`, a ban, the country lists or a rate limit would refuse:
-  it passes them to the app, and their log lines name what `enforce` mode would
-  have done (see `would_action` in "Request log" below). The checks run, and
-  requests are counted, as in `enforce` mode, but a broken rate limit makes no
-  ban and does not set the client's counters back to zero, so each request over
-  the limit is logged as one that would be refused. The bans in `bans.json` are
-  kept, and refuse requests again when `smallwebwaf` next runs in `enforce`
-  mode, as long as they last. The timeouts and size limits still apply, since
-  they protect `smallwebwaf` and the app themselves, and a request for the
-  metrics without the token is still answered `401`. It is for trying a
-  configuration before enforcing it.
 - Answers `GET /_smallwebwaf/healthz` itself with `200` and `ok`, before any
   check and without asking the app, for the image's health check.
 - Answers `GET /_smallwebwaf/metrics` with its metrics (see "Metrics" below) for
@@ -154,9 +140,6 @@ it, and the effective settings are logged at start.
 - `SWWAF_LISTEN_ADDR` (default `:8080`): where `smallwebwaf` listens.
 - `SWWAF_UPSTREAM_URL` (default `http://127.0.0.1:8081`): the app, as `http` or
   `https`, a host and an optional port, and nothing more.
-- `SWWAF_MODE` (default `enforce`): `enforce`, or `observe` to pass on the
-  requests `smallwebwaf` would refuse and log what it would have done (see "What
-  it does so far" above).
 - `SWWAF_TRUSTED_PROXIES` (default `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16`,
   the private address ranges): the netblocks whose `X-Forwarded-For` is
   believed. A list given replaces the default; set but empty, it trusts nothing.
@@ -258,8 +241,8 @@ refused ones included:
 - `country` is the client's country as GeoJS places it. It is empty with neither
   country list set, for a client in `SWWAF_ALLOW_NETS` or `SWWAF_DENY_NETS`, for
   a client on a private, loopback or link-local address, when GeoJS cannot place
-  the client or has not answered in time, and for a request whose client a ban
+  the client or has not answered in time, and for a request refused because a
-  covers, even when the client's country is known.
+  ban covers its client, even when the client's country is known.
 - `status` is what the client was sent, `0` if nothing was; `upstream_status` is
   what the app answered, and is left out when the app did not answer.
 - `request_bytes` and `response_bytes` count body bytes.
@@ -271,18 +254,11 @@ refused ones included:
   `timed_out` for one that ran out of time, `upstream_error` when the app could
   not be reached or its answer broke off, and `admin` for one `smallwebwaf`
   answered at its own endpoint.
-- `would_action` is there in `observe` mode for a request that
-  `SWWAF_DENY_NETS`, a ban, the country lists or a rate limit would have refused
-  in `enforce` mode, and names the action that refusal would have had: `denied`,
-  `banned`, `country_denied` or `rate_limited`. `action` then names what was
-  done: `forward` for a request passed to the app, and another action, such as
-  `too_large`, for one a size or time limit refused.
 - `limit_hit` is there for a request that broke a rate limit, and names the
   window whose limit it went over: `minute`, `hour` or `day`, the shortest if it
   went over several. `offence` is then `limit`.
 - `ban_expires` is there for a request that made a ban or was refused under one,
-  or in `observe` mode would have been refused under one, and gives when the ban
+  and gives when the ban ends, in the same form as `time`, or `permanent`.
-  ends, in the same form as `time`, or `permanent`.
 - `aborted` is there, and true, when the client went away early.
 - `duration_total` and `duration_upstream_total` are in milliseconds.
 
@@ -642,16 +618,17 @@ the metrics, failure behaviour and the build order.
 So far `smallwebwaf` looks up only the country, only through GeoJS, and only
 while `SWWAF_DENIED_COUNTRIES` or `SWWAF_EXCLUSIVELY_ALLOWED_COUNTRIES` is set:
 then the address of every new visitor is sent to GeoJS, except a visitor in
-`SWWAF_ALLOW_NETS` or `SWWAF_DENY_NETS` and one whose netblock a ban covers, and
+`SWWAF_ALLOW_NETS` or `SWWAF_DENY_NETS` and one refused because a ban covers its
-with neither set, none is. An IPv6 visitor is asked about by the first address
+netblock, and with neither set, none is. An IPv6 visitor is asked about by the
-of its /64. A new visitor waits at most a second for its answer, and without one
+first address of its /64. A new visitor waits at most a second for its answer,
-counts as coming from an unknown country until the answer arrives. The addresses
+and without one counts as coming from an unknown country until the answer
-waiting are asked about together, up to 200 in one request, one request at a
+arrives. The addresses waiting are asked about together, up to 200 in one
-time; at most 10,000 visitors wait, and one more counts as coming from an
+request, one request at a time; at most 10,000 visitors wait, and one more
-unknown country until there is room. While GeoJS fails, visitors with a kept
+counts as coming from an unknown country until there is room. While GeoJS fails,
-answer are unaffected and new ones count as coming from an unknown country.
+visitors with a kept answer are unaffected and new ones count as coming from an
-GeoJS is then left alone for a second, twice as long after each further failure
+unknown country. GeoJS is then left alone for a second, twice as long after each
-up to five minutes, and asked again by the next request that needs it.
+further failure up to five minutes, and asked again by the next request that
+needs it.
 
 In the full design, `smallwebwaf` looks up the AS number and country of every
 client, for the request log, the metrics and the ban notes, and for the country
@@ -703,10 +680,8 @@ addresses are never sent to GeoJS.
   limits, and writes the request's log line. Its `check` method is where a
   request is refused before anything reaches the app: for `SWWAF_DENY_NETS`, for
   a ban, for the country lists, for a rate limit, which bans the client, and for
-  an announced body over the size limit; in `observe` mode, only for the size
+  an announced body over the size limit. A request under `/_smallwebwaf/` that
-  limit, with what it would have refused for noted in the log line. A request
+  `check` lets through is answered by `answerAdmin` instead of reaching the app.
-  under `/_smallwebwaf/` that `check` lets through is answered by `answerAdmin`
-  instead of reaching the app.
 - `internal/metrics`: the metrics, counted as the other parts tell it what
   happened, and served in the Prometheus text format.
 - `internal/bans`: the ban ledger: each netblock's bans with their notes, how
@@ -773,8 +748,8 @@ so that they run in minimal containers.
 
 ## TODO
 
-- The rest of milestone 3: exemptions and the rest of the request log's fields;
+- The rest of milestone 3, from exemptions up to the rest of the request log's
-  then the rest of the design, in the order of the build order in
+  fields, and the rest of the design, in the order of the build order in
   [`SPEC.md`](SPEC.md).
 
 ## Documents

internal/bans/bans.go

@@ -107,8 +107,8 @@ type Ledger struct {
 	changed chan struct{}
 
 	mu sync.Mutex
-	// netblocks holds each banned netblock's bans, oldest first. Check and
+	// netblocks holds each banned netblock's bans, oldest first. Check
-	// Find make each netblock they find the most recently seen.
+	// makes each netblock it finds the most recently seen.
 	netblocks *simplelru.LRU[netip.Prefix, *[]Ban]
 	// held is how many bans netblocks holds, at most rules.MaxBans.
 	held int
@@ -144,36 +144,34 @@ func (l *Ledger) Changed() <-chan struct{} {
 	return l.changed
 }
 
-// Check is called for a request from client, at now. It reports whether
+// Check is called for each request from client, at now. It reports
-// a ban on a netblock client is in is active, and returns that ban, with
+// whether a ban on a netblock client is in is active, and returns that
-// the request counted among those it refused.
+// ban, with the request counted among those it refused.
 func (l *Ledger) Check(client netip.Addr, now time.Time) (Ban, bool) {
 	l.mu.Lock()
 	defer l.mu.Unlock()
 
-	ban := l.active(client, now)
+	lengths := l.v6Lengths
-	if ban == nil {
+	if client.Is4() {
-		return Ban{}, false
+		lengths = l.v4Lengths
 	}
 
+	for _, length := range lengths {
+		bans, found := l.netblocks.Get(netip.PrefixFrom(client, length).Masked())
+		if !found {
+			continue
+		}
+
+		ban := activeBan(*bans, now)
+		if ban != nil {
 			ban.Notes.Requests++
 			ban.Notes.Refused++
 
 			return *ban, true
 		}
-
-// Find is Check without counting the request among those the ban
-// refused: in observe mode a ban refuses nothing.
-func (l *Ledger) Find(client netip.Addr, now time.Time) (Ban, bool) {
-	l.mu.Lock()
-	defer l.mu.Unlock()
-
-	ban := l.active(client, now)
-	if ban == nil {
-		return Ban{}, false
 	}
 
-	return *ban, true
+	return Ban{}, false
 }
 
 // activeBan returns the ban in bans, a netblock's bans oldest first, that
@@ -332,29 +330,6 @@ func (l *Ledger) Load(bans []Ban) {
 	}
 }
 
-// active returns the ban active at now on a netblock client is in, or
-// nil.
-func (l *Ledger) active(client netip.Addr, now time.Time) *Ban {
-	lengths := l.v6Lengths
-	if client.Is4() {
-		lengths = l.v4Lengths
-	}
-
-	for _, length := range lengths {
-		bans, found := l.netblocks.Get(netip.PrefixFrom(client, length).Masked())
-		if !found {
-			continue
-		}
-
-		ban := activeBan(*bans, now)
-		if ban != nil {
-			return ban
-		}
-	}
-
-	return nil
-}
-
 // add adds ban to its netblock's bans, after the last, and makes its
 // netblock the most recently seen. With MaxBans held, it drops one first.
 func (l *Ledger) add(ban Ban) {

internal/bans/bans_test.go

@@ -162,28 +162,6 @@ func TestCheckRefusesWhileTheBanLastsAndCountsTheRefusals(t *testing.T) {
 	}
 }
 
-func TestFindCountsNothing(t *testing.T) {
-	t.Parallel()
-
-	ledger := bans.New(defaultRules())
-	netblock := netip.MustParsePrefix("203.0.113.9/32")
-	ban := ledger.BanForLimit(netblock, midnight(), bans.Notes{Requests: 5})
-
-	got, banned := ledger.Find(netblock.Addr(), ban.Expires.Add(-time.Nanosecond))
-	if !banned || got != ban {
-		t.Errorf("find during the ban gives %+v and %t, want %+v", got, banned, ban)
-	}
-
-	_, banned = ledger.Find(netblock.Addr(), ban.Expires)
-	if banned {
-		t.Error("the ban did not end")
-	}
-
-	if notes := ledger.Bans(netblock)[0].Notes; notes != ban.Notes {
-		t.Errorf("the notes are %+v, want them unchanged, %+v", notes, ban.Notes)
-	}
-}
-
 func TestMaxBansDropsTheEarliestBanOfTheNetblockSeenLongestAgo(t *testing.T) {
 	t.Parallel()
 

internal/bans/snapshot_test.go

@@ -147,14 +147,8 @@ func TestPermanentBanStartedBeforeAnEndedOneRefuses(t *testing.T) {
 	ledger.Load([]bans.Ban{permanent, ended})
 
 	now := midnight().Add(2 * time.Hour)
-	client := netip.MustParseAddr("203.0.113.9")
 
-	ban, banned := ledger.Find(client, now)
+	ban, banned := ledger.Check(netip.MustParseAddr("203.0.113.9"), now)
-	if !banned || !ban.Permanent() {
-		t.Errorf("find gives %+v and %t, want the permanent ban", ban, banned)
-	}
-
-	ban, banned = ledger.Check(client, now)
 	if !banned || !ban.Permanent() {
 		t.Errorf("the client is refused: %t, under %+v, want under the permanent ban",
 			banned, ban)

internal/config/config.go

@@ -27,11 +27,6 @@ type Config struct {
 	ListenAddr string
 	// UpstreamURL is the app (SWWAF_UPSTREAM_URL).
 	UpstreamURL *url.URL
-	// Observe is true in observe mode, when SWWAF_MODE is observe rather
-	// than enforce: a request that SWWAF_DENY_NETS, a ban, the country
-	// lists or a rate limit would refuse is passed to the app instead, and
-	// no ban is made.
-	Observe bool
 	// TrustedProxies are the netblocks whose X-Forwarded-For is
 	// believed (SWWAF_TRUSTED_PROXIES).
 	TrustedProxies []netip.Prefix
@@ -167,7 +162,6 @@ var (
 	errNotAbsolutePath = errors.New(
 		"is not an absolute path, such as /var/lib/smallwebwaf")
 	errShortToken = errors.New("is shorter than 32 characters")
-	errNotMode    = errors.New("is not enforce or observe")
 )
 
 // FromEnvironment reads the settings with lookupEnv, normally
@@ -178,7 +172,6 @@ func FromEnvironment(lookupEnv func(string) (string, bool)) (*Config, error) {
 	cfg := &Config{
 		ListenAddr:           env.address("SWWAF_LISTEN_ADDR", ":8080"),
 		UpstreamURL:          env.appURL("SWWAF_UPSTREAM_URL", "http://127.0.0.1:8081"),
-		Observe:              env.observe("SWWAF_MODE", "enforce"),
 		TrustedProxies:       env.netblocks("SWWAF_TRUSTED_PROXIES", privateRanges),
 		ClientRequestTimeout: env.duration("SWWAF_CLIENT_REQUEST_TIMEOUT", "60s"),
 		ClientRequestHeaderMaxBytes: env.headerSize(
@@ -281,17 +274,6 @@ func (e *environment) appURL(name, defaultValue string) *url.URL {
 	return upstream
 }
 
-// observe reads the setting that is the mode, enforce or observe, and
-// reports whether it is observe.
-func (e *environment) observe(name, defaultValue string) bool {
-	mode := e.value(name, defaultValue)
-	if mode != "enforce" && mode != "observe" {
-		e.check(name, fmt.Errorf("%q %w", mode, errNotMode))
-	}
-
-	return mode == "observe"
-}
-
 // netblocks reads a setting that is a list of netblocks.
 func (e *environment) netblocks(name, defaultValue string) []netip.Prefix {
 	netblocks, err := parseNetblocks(e.value(name, defaultValue))

internal/config/config_test.go

@@ -18,7 +18,6 @@ import (
 const (
 	listenAddr              = "SWWAF_LISTEN_ADDR"
 	upstreamURL             = "SWWAF_UPSTREAM_URL"
-	mode                    = "SWWAF_MODE"
 	trustedProxies          = "SWWAF_TRUSTED_PROXIES"
 	clientRequestTimeout    = "SWWAF_CLIENT_REQUEST_TIMEOUT"
 	clientHeaderMaxBytes    = "SWWAF_CLIENT_REQUEST_HEADER_MAX_BYTES"
@@ -84,7 +83,6 @@ func TestDefaults(t *testing.T) {
 
 	wantSettings(t, cfg, config.Config{
 		ListenAddr:                  ":8080",
-		Observe:                     false,
 		ClientRequestTimeout:        time.Minute,
 		ClientRequestHeaderMaxBytes: 32 << 10,
 		ClientIdleTimeout:           2 * time.Minute,
@@ -128,7 +126,6 @@ func TestValuesAsSet(t *testing.T) {
 	cfg := fromEnvironment(t, environment{
 		listenAddr:              "127.0.0.1:9000",
 		upstreamURL:             "https://app.internal:8443/",
-		mode:                    "observe",
 		trustedProxies:          " 192.0.2.1, 10.1.2.3/8 ,2001:db8::/32",
 		clientRequestTimeout:    "90s",
 		clientHeaderMaxBytes:    "8K",
@@ -161,7 +158,6 @@ func TestValuesAsSet(t *testing.T) {
 
 	wantSettings(t, cfg, config.Config{
 		ListenAddr:                  "127.0.0.1:9000",
-		Observe:                     true,
 		ClientRequestTimeout:        90 * time.Second,
 		ClientRequestHeaderMaxBytes: 8 << 10,
 		ClientIdleTimeout:           5 * time.Minute,
@@ -311,7 +307,6 @@ func TestInvalidValueStopsTheStart(t *testing.T) {
 		{upstreamURL, "http://127.0.0.1:8081/app"},
 		{upstreamURL, "http://127.0.0.1:8081/?a=1"},
 		{upstreamURL, "http://user:secret@127.0.0.1:8081"},
-		{mode, "Observe"}, {mode, "block"}, {mode, ""},
 		{trustedProxies, "10.0.0.0/33"},
 		{trustedProxies, "traefik"},
 		{trustedProxies, "10.0.0.0/8,,192.168.0.0/16"},
@@ -431,7 +426,6 @@ func TestLogsEachSettingWithItsValue(t *testing.T) {
 	want := map[string]string{
 		listenAddr:              ":8080",
 		upstreamURL:             "http://127.0.0.1:8081",
-		mode:                    "enforce",
 		trustedProxies:          "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16",
 		clientRequestTimeout:    "45s",
 		clientHeaderMaxBytes:    "32K",
@@ -471,7 +465,6 @@ func wantSettings(t *testing.T, got *config.Config, want config.Config) {
 	t.Helper()
 
 	if got.ListenAddr != want.ListenAddr ||
-		got.Observe != want.Observe ||
 		got.ClientRequestTimeout != want.ClientRequestTimeout ||
 		got.ClientRequestHeaderMaxBytes != want.ClientRequestHeaderMaxBytes ||
 		got.ClientIdleTimeout != want.ClientIdleTimeout ||

internal/proxy/bans.go

@@ -14,15 +14,10 @@ func (rq *request) banResponse(action string) *refusal {
 	return &refusal{status: rq.h.config.BanResponse, action: action}
 }
 
-// banned reports whether a ban on a netblock the client is in covers the
+// banned reports whether a ban on a netblock the client is in refuses
-// request at now, and notes for the log line when that ban ends.
+// the request at now, and notes for the log line when that ban ends.
 func (rq *request) banned(now time.Time) bool {
-	check := rq.h.ledger.Check
+	ban, banned := rq.h.ledger.Check(rq.client, now)
-	if rq.h.config.Observe {
-		check = rq.h.ledger.Find // in observe mode the ban refuses nothing
-	}
-
-	ban, banned := check(rq.client, now)
 	if banned {
 		rq.line.BanExpires = banExpires(ban)
 	}
@@ -31,9 +26,8 @@ func (rq *request) banned(now time.Time) bool {
 }
 
 // limitBroken counts the request for the rate limits at now, and reports
-// whether it takes the client over one. In enforce mode such a request
+// whether it takes the client over one. Such a request bans the client's
-// bans the client's netblock, and sets the client's counters back to
+// netblock, and sets the client's counters back to zero.
-// zero; in observe mode it does neither.
 func (rq *request) limitBroken(now time.Time) bool {
 	group := clientGroup(rq.client)
 
@@ -42,13 +36,6 @@ func (rq *request) limitBroken(now time.Time) bool {
 		return false
 	}
 
-	rq.line.LimitHit = hit.Window
-	rq.line.Offence = requestlog.OffenceLimit
-
-	if rq.h.config.Observe {
-		return true
-	}
-
 	netblock := rq.netblock()
 	ban := rq.h.ledger.BanForLimit(netblock, now, bans.Notes{
 		Country: rq.line.Country,
@@ -67,6 +54,9 @@ func (rq *request) limitBroken(now time.Time) bool {
 		Requests: rq.h.limiter.Requests(netblock) + 1,
 	})
 	rq.h.limiter.Reset(group)
+
+	rq.line.LimitHit = hit.Window
+	rq.line.Offence = requestlog.OffenceLimit
 	rq.line.BanExpires = banExpires(ban)
 
 	return true

internal/proxy/observe_test.go

@@ -1,202 +0,0 @@
-package proxy_test
-
-import (
-	"bytes"
-	"io"
-	"net/http"
-	"net/netip"
-	"sync/atomic"
-	"testing"
-	"time"
-
-	"sneak.berlin/go/smallwebwaf/internal/bans"
-	"sneak.berlin/go/smallwebwaf/internal/proxy"
-	"sneak.berlin/go/smallwebwaf/internal/requestlog"
-)
-
-// observe is the value of SWWAF_MODE for observe mode.
-const observe = "observe"
-
-func TestObserveModeForwardsWhatEnforceModeRefuses(t *testing.T) {
-	t.Parallel()
-
-	const (
-		denied = "192.0.2.50" // in SWWAF_DENY_NETS
-		banned = otherClient  // under a ban read from bans.json
-	)
-
-	for _, tc := range []struct {
-		setting string // "" leaves SWWAF_MODE at its default
-		observe bool
-	}{
-		{"", false},
-		{"enforce", false},
-		{observe, true},
-	} {
-		t.Run(mode+"="+tc.setting, func(t *testing.T) {
-			t.Parallel()
-
-			geojsURL, _ := startGeoJS(t)
-			env := map[string]string{
-				rateLimitPerMinute: "1",
-				denyNets:           denied,
-				deniedCountries:    "kp",
-			}
-
-			if tc.setting != "" {
-				env[mode] = tc.setting
-			}
-
-			s, clk, server := startWithClock(t, geojsURL, env)
-			server.Ledger.Load([]bans.Ban{{
-				Netblock: netip.MustParsePrefix(banned + "/32"),
-				Start:    clk.Now(),
-				Expires:  clk.Now().Add(time.Hour),
-			}})
-
-			// fromDE's first request is within the limit of one a minute,
-			// and its second breaks it.
-			s.get(fromDE, http.StatusOK, requestlog.ActionForward)
-
-			for _, sent := range []struct{ from, refusal string }{
-				{denied, requestlog.ActionDenied},
-				{banned, requestlog.ActionBanned},
-				{fromKP, requestlog.ActionCountryDenied},
-				{fromDE, requestlog.ActionRateLimited},
-			} {
-				if !tc.observe {
-					line := s.get(sent.from, http.StatusForbidden, sent.refusal)
-					wantWouldAction(t, line, "")
-
-					continue
-				}
-
-				// Passed to the app, which answered it.
-				line := s.get(sent.from, http.StatusOK, requestlog.ActionForward)
-				wantWouldAction(t, line, sent.refusal)
-
-				if line.UpstreamStatus != http.StatusOK {
-					t.Errorf("log line has upstream_status %d, want 200",
-						line.UpstreamStatus)
-				}
-			}
-		})
-	}
-}
-
-func TestObserveModeMakesNoBanAndKeepsTheBansItHas(t *testing.T) {
-	t.Parallel()
-
-	s, clk, server := startWithClock(t, "", map[string]string{
-		mode:               observe,
-		rateLimitPerMinute: "1",
-	})
-	kept := bans.Ban{
-		Netblock: netip.MustParsePrefix(otherClient + "/32"),
-		Start:    clk.Now(),
-		Expires:  clk.Now().Add(time.Hour),
-	}
-	server.Ledger.Load([]bans.Ban{kept})
-
-	// No ban sets client's counters back to zero, so each request after
-	// the first breaks the limit of one a minute.
-	s.get(client, http.StatusOK, requestlog.ActionForward)
-
-	for range 2 {
-		line := s.get(client, http.StatusOK, requestlog.ActionForward)
-		wantWouldAction(t, line, requestlog.ActionRateLimited)
-
-		if line.LimitHit != minute || line.Offence != requestlog.OffenceLimit ||
-			line.BanExpires != "" {
-			t.Errorf("log line has limit_hit %q, offence %q and ban_expires %q, "+
-				"want minute, limit and none", line.LimitHit, line.Offence,
-				line.BanExpires)
-		}
-	}
-
-	// The ban read from bans.json refuses nothing, and so counts no
-	// refusal in its notes, but is kept.
-	line := s.get(otherClient, http.StatusOK, requestlog.ActionForward)
-	wantWouldAction(t, line, requestlog.ActionBanned)
-
-	if line.BanExpires != requestlog.FormatTime(kept.Expires) {
-		t.Errorf("log line has ban_expires %q, want %s", line.BanExpires,
-			requestlog.FormatTime(kept.Expires))
-	}
-
-	got := server.Ledger.Snapshot()
-	if len(got) != 1 || got[0] != kept {
-		t.Errorf("bans\n%+v\nwant only\n%+v", got, kept)
-	}
-}
-
-func TestObserveModeKeepsTheSizeLimitsAndTheToken(t *testing.T) {
-	t.Parallel()
-
-	const denied = "192.0.2.50" // in SWWAF_DENY_NETS
-
-	var calls atomic.Int32
-
-	app := startApp(t, func(w http.ResponseWriter, _ *http.Request) {
-		calls.Add(1)
-		answerWithSize(w, 2*sizeLimit, true)
-	})
-	addr, out := startProxy(t, app.URL, map[string]string{
-		mode:             observe,
-		trustedProxies:   trustLocalhost,
-		denyNets:         denied,
-		requestMaxBytes:  sizeLimitSetting,
-		responseMaxBytes: sizeLimitSetting,
-		metricsToken:     token,
-	})
-
-	// SWWAF_DENY_NETS would refuse each request; instead a size limit or
-	// the missing token does.
-	for i, tc := range []struct {
-		method, path string
-		body         io.Reader
-		status       int
-		action       string
-	}{
-		{
-			http.MethodPost, "/upload", bytes.NewReader(make([]byte, 2*sizeLimit)),
-			http.StatusRequestEntityTooLarge, requestlog.ActionTooLarge,
-		},
-		{
-			http.MethodGet, "/download", http.NoBody,
-			http.StatusBadGateway, requestlog.ActionTooLarge,
-		},
-		{
-			http.MethodGet, proxy.MetricsPath, http.NoBody,
-			http.StatusUnauthorized, requestlog.ActionAdmin,
-		},
-	} {
-		req := newRequest(t, tc.method, addr, tc.path, tc.body)
-		req.Header.Set(forwardedFor, denied)
-		wantStatus(t, do(t, req), tc.status)
-
-		line := out.requestLines(t, i+1)[i]
-		wantLine(t, line, tc.status, tc.action)
-		wantWouldAction(t, line, requestlog.ActionDenied)
-	}
-
-	// The upload was refused before it reached the app.
-	if calls.Load() != 1 {
-		t.Errorf("the app was called %d times, want once", calls.Load())
-	}
-}
-
-// wantWouldAction checks the request log line's would_action, and that a
-// line that should have none has no such field.
-func wantWouldAction(t *testing.T, line logLine, want string) {
-	t.Helper()
-
-	got, present := line.fields["would_action"]
-
-	switch {
-	case want == "" && present:
-		t.Errorf("log line has would_action %v, want none", got)
-	case want != "" && got != want:
-		t.Errorf("log line has would_action %v, want %s", got, want)
-	}
-}

internal/proxy/proxy_test.go

@@ -50,7 +50,6 @@ const (
 	clientResponseTimeout   = "SWWAF_CLIENT_RESPONSE_TIMEOUT"
 	upstreamRequestTimeout  = "SWWAF_UPSTREAM_REQUEST_TIMEOUT"
 	upstreamResponseTimeout = "SWWAF_UPSTREAM_RESPONSE_TIMEOUT"
-	mode                    = "SWWAF_MODE"
 	requestMaxBytes         = "SWWAF_REQUEST_MAX_BYTES"
 	responseMaxBytes        = "SWWAF_RESPONSE_MAX_BYTES"
 	trustedProxies          = "SWWAF_TRUSTED_PROXIES"

internal/proxy/request.go

@@ -109,24 +109,38 @@ func (h *handler) newRequest(w http.ResponseWriter, r *http.Request) *request {
 
 // check is the one place where a request can be refused once its client
 // is known, before its body is read or anything reaches the app. It
-// returns nil to let the request through. The checks of checkClient come
+// returns nil to let the request through. A client in SWWAF_ALLOW_NETS
-// first, answered with SWWAF_BAN_RESPONSE, and then the size limit, so
+// skips every check but the size limit. For any other client,
-// that a request the rate limits count is counted even when it is
+// SWWAF_DENY_NETS comes first, then a ban on its netblock, so that a
-// refused for its size. In observe mode a request checkClient refuses
+// client either refuses is not looked up, and then the country lists; a
-// goes on to the size limit like any other. ctx is the request's own
+// request any of them refuses is not counted for the rate limits. Then
-// context.
+// come the rate limits, unless the client is in
+// SWWAF_RATE_LIMIT_EXEMPT_NETS, so that every other request is counted,
+// one refused for its size too. Every refusal but the size limit's is
+// answered with SWWAF_BAN_RESPONSE. ctx is the request's own context.
 func (rq *request) check(ctx context.Context) *refusal {
-	action := rq.checkClient(ctx)
+	cfg := rq.h.config
-	if action != "" {
+	allowed := isInside(rq.client, cfg.AllowNets)
-		if !rq.h.config.Observe {
+	exempt := isInside(rq.client, cfg.RateLimitExemptNets)
-			return rq.banResponse(action)
+	now := rq.h.now()
+
+	if !allowed && isInside(rq.client, cfg.DenyNets) {
+		return rq.banResponse(requestlog.ActionDenied)
 	}
 
-		// The log line names what enforce mode would have done.
+	if !allowed && rq.banned(now) {
-		rq.line.WouldAction = action
+		return rq.banResponse(requestlog.ActionBanned)
 	}
 
-	maxBytes := rq.h.config.RequestMaxBytes
+	if !allowed && rq.countryDenied(ctx) {
+		return rq.banResponse(requestlog.ActionCountryDenied)
+	}
+
+	if !allowed && !exempt && rq.limitBroken(now) {
+		return rq.banResponse(requestlog.ActionRateLimited)
+	}
+
+	maxBytes := cfg.RequestMaxBytes
 	if maxBytes > 0 && rq.in.ContentLength > maxBytes {
 		return &refusal{
 			status: http.StatusRequestEntityTooLarge,
@@ -138,42 +152,6 @@ func (rq *request) check(ctx context.Context) *refusal {
 	return nil
 }
 
-// checkClient runs the checks on the request's client, and returns the
-// action of the first that refuses the request, or "" when none does. A
-// client in SWWAF_ALLOW_NETS skips them. For any other client,
-// SWWAF_DENY_NETS comes first, then a ban on its netblock, so that a
-// client either refuses is not looked up, and then the country lists; a
-// request any of them refuses is not counted for the rate limits. Then
-// come the rate limits, unless the client is in
-// SWWAF_RATE_LIMIT_EXEMPT_NETS, so that every other request is counted.
-// 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) {
-		return ""
-	}
-
-	now := rq.h.now()
-
-	if isInside(rq.client, cfg.DenyNets) {
-		return requestlog.ActionDenied
-	}
-
-	if rq.banned(now) {
-		return requestlog.ActionBanned
-	}
-
-	if rq.countryDenied(ctx) {
-		return requestlog.ActionCountryDenied
-	}
-
-	if !isInside(rq.client, cfg.RateLimitExemptNets) && rq.limitBroken(now) {
-		return requestlog.ActionRateLimited
-	}
-
-	return ""
-}
-
 // forward passes the request to the app and the app's answer back. ctx
 // is the request's own context.
 func (rq *request) forward(ctx context.Context) {

internal/requestlog/requestlog.go

@@ -67,10 +67,6 @@ type Line struct {
 	Referer        string `json:"referer"`
 	UserAgent      string `json:"user_agent"`
 	Action         string `json:"action"`
-	// WouldAction is, in observe mode, the action enforce mode would have
-	// taken with a request it would have refused: ActionDenied,
-	// ActionBanned, ActionCountryDenied or ActionRateLimited.
-	WouldAction string `json:"would_action,omitempty"`
 	// LimitHit is the window whose rate limit the request went over:
 	// minute, hour or day.
 	LimitHit string `json:"limit_hit,omitempty"`

internal/smallwebwaf/smallwebwaf_test.go

@@ -418,7 +418,6 @@ func wantStartingLine(t *testing.T, line map[string]any, appURL, dir string) {
 		listenAddr:                              localhost + ":0",
 		upstreamURL:                             appURL,
 		stateDir:                                dir,
-		"SWWAF_MODE":                            "enforce",
 		stateWriteDelay:                         "10s",
 		stateCounterInterval:                    "15m",
 		trustedProxies:                          "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16",