Rule files, and bans for a clear sign of attack (closes #24)

Every *.rules file in SWWAF_RULES_DIR is read at start and on each
change. Each request is checked against the rules after the rate
limits: log notes a match, block refuses with 403, ban refuses and bans
the netblock for SWWAF_ATTACK_BAN_DURATION, made permanent by its next
request or attack. path, query and uri are matched as the request line
sent them; header:Host and header:Transfer-Encoding are refused. Bans
gain a cause. The image ships 00-default.rules.

Judgement call: a header sent twice is matched with its values joined
by ", ".
Judgement call: SWWAF_MAX_BAN_DURATION does not cap a ban for an attack.
Not in this unit: offences for rule matches, with the error burst.

Model: opus-5-5

README.md

@@ -490,11 +490,9 @@ not make the netblock's next ban longer.
 `smallwebwaf` reads every `*.rules` file in `SWWAF_RULES_DIR`,
 `/etc/smallwebwaf/rules.d` by default, in the order of their names, and checks
 each request against their rules in that order, as "Rule files" in
-[`SPEC.md`](SPEC.md) describes. A file whose name starts with `.`, such as an
-editor's lock file `.#50-app.rules`, is not a rule file, as a shell's `*.rules`
-would not match it. A rule is a line of four fields separated by spaces or tabs:
-an id, a target, an action and a regex, which runs to the end of the line. Blank
-lines and lines that start with `#` are ignored.
+[`SPEC.md`](SPEC.md) describes. A rule is a line of four fields separated by
+spaces or tabs: an id, a target, an action and a regex, which runs to the end of
+the line. Blank lines and lines that start with `#` are ignored.
 
 ```
 # id           target      action  regex
@@ -525,13 +523,10 @@ header, a regex that does not compile or an id used twice stops the start with a
 message naming the file and the line, and so does a `SWWAF_RULES_DIR` that does
 not exist. An empty directory is no error, and the log says that it holds no
 rules. While it runs, `smallwebwaf` watches the directory, and reads the rule
-files again once the directory has had no change for 2 seconds after one is
-edited, added or removed, so that a file saved in place, appended to or copied
-in with `scp` is read only once whole, unless its writing stops for longer. It
-also reads them 2 seconds after it starts watching, so that an edit saved while
-it started is not missed. If they then hold one of those errors, the rules stay
-as they were, the earlier version of the edited file included, the log names the
-file and the line, and the files are read again after the next change.
+files again whenever one is edited, added or removed. If they then hold one of
+those errors, the rules stay as they were, the earlier version of the edited
+file included, the log names the file and the line, and the files are read again
+at the next change.
 
 The image ships one rule file, `share/rules.d/00-default.rules` here: rules that
 ban probes no real visitor sends, for secrets, version control directories,

internal/rules/rules.go

@@ -1,8 +1,7 @@
 // Package rules reads the rule files: the plain text files in
 // SWWAF_RULES_DIR, one rule to a line, that each request is checked
 // against, as the "Rule files" section of SPEC.md describes. They are read
-// at start, and again once the directory has had no change for a short
-// time after one is edited, added or removed.
+// at start, and again whenever one is edited, added or removed.
 package rules
 
 import (
@@ -18,7 +17,6 @@ import (
 	"slices"
 	"strings"
 	"sync/atomic"
-	"time"
 
 	"github.com/fsnotify/fsnotify"
 )
@@ -37,12 +35,6 @@ const (
 // extension ends the name of every rule file.
 const extension = ".rules"
 
-// quietTime is how long SWWAF_RULES_DIR must go without a change before
-// the rule files are read again, so that a file still being written, such
-// as one saved in place, appended to or copied in with scp, is read only
-// once whole.
-const quietTime = 2 * time.Second
-
 // headerTarget starts the target that is one request header,
 // header:<Name>.
 const headerTarget = "header:"
@@ -158,12 +150,11 @@ func (f *Files) Len() int {
 }
 
 // Watch watches Dir until ctx is done, and reads the rule files again
-// once Dir has had no change for quietTime, after one is edited, added or
-// removed, and after Watch starts watching. If they then hold an error,
+// whenever one is edited, added or removed. If they then hold an error,
 // the rules stay as they were, the error is logged with its file and
-// line, and the files are read again after the next change. If Dir cannot
-// be watched, that is logged, and the rules stay as they were loaded.
-// While Enabled is false, Watch returns at once.
+// line, and the files are read again at the next change. If Dir cannot be
+// watched, that is logged, and the rules stay as they were loaded. While
+// Enabled is false, Watch returns at once.
 func (f *Files) Watch(ctx context.Context) {
 	if !f.params.Enabled {
 		return
@@ -188,29 +179,15 @@ func (f *Files) Watch(ctx context.Context) {
 	f.params.ProcessLog.Info("watching the rule files for edits",
 		"directory", f.params.Dir)
 
-	f.readAfterChanges(ctx, watcher.Events, watcher.Errors)
-}
-
-// readAfterChanges reads the rule files again once quietTime has passed
-// without a change from events, until ctx is done, and logs the errors
-// from errs. The wait starts at once, as if for a change, so that an edit
-// saved after Load read the files, and before Dir was watched, is taken
-// in too.
-func (f *Files) readAfterChanges(
-	ctx context.Context, events <-chan fsnotify.Event, errs <-chan error,
-) {
-	quiet := time.NewTimer(quietTime)
-	defer quiet.Stop()
-
 	for {
 		select {
 		case <-ctx.Done():
 			return
-		case <-events:
-			quiet.Reset(quietTime)
-		case <-quiet.C:
-			f.readAgain()
-		case err := <-errs:
+		case event := <-watcher.Events:
+			if filepath.Ext(event.Name) == extension {
+				f.readAgain()
+			}
+		case err = <-watcher.Errors:
 			f.params.ProcessLog.Warn("watching the rule files failed",
 				"error", err.Error())
 		}
@@ -241,9 +218,7 @@ func (f *Files) logRead(count int) {
 }
 
 // read returns the rules of every rule file in dir, in the order of the
-// files' names, and then of their lines. A file whose name starts with a
-// dot, such as an editor's lock file .#50-app.rules, is not a rule file,
-// as a shell's *.rules would not match it.
+// files' names, and then of their lines.
 func read(dir string) ([]Rule, error) {
 	entries, err := os.ReadDir(dir)
 	if err != nil {
@@ -256,12 +231,11 @@ func read(dir string) ([]Rule, error) {
 	places := map[string]string{}
 
 	for _, entry := range entries {
-		name := entry.Name()
-		if entry.IsDir() || strings.HasPrefix(name, ".") || filepath.Ext(name) != extension {
+		if entry.IsDir() || filepath.Ext(entry.Name()) != extension {
 			continue
 		}
 
-		rules, err = readFile(filepath.Join(dir, name), rules, places)
+		rules, err = readFile(filepath.Join(dir, entry.Name()), rules, places)
 		if err != nil {
 			return nil, err
 		}
@@ -399,14 +373,15 @@ func value(target string, r *http.Request) string {
 	}
 }
 
-// pathAndQuery returns the target of r's request line, r.RequestURI, as
-// the client sent it, less any scheme and host: a target with a scheme
-// gives what follows the scheme and its :, and the host when // follows.
-// So http://host/path, as a client sends it to a proxy, gives /path, and
-// so does http:/path, which Go reads as a target with a scheme and no
-// host. r.URL is not used: when the path holds a character it escapes,
-// such as \ or a non-ASCII byte, it decodes the whole path and escapes it
-// again, so that \ becomes %5C and %2e a dot.
+// pathAndQuery returns the path and the query of r as the client sent
+// them, as the app is sent them: the target of its request line,
+// r.RequestURI, of which a target with a scheme gives what follows the
+// scheme and its :, and the host when // follows. So http://host/path, as
+// a client sends it to a proxy, gives /path, and so does http:/path,
+// which Go reads as a target with a scheme and no host. r.URL is not
+// used: when the path holds a character it escapes, such as \ or a
+// non-ASCII byte, it decodes the whole path and escapes it again, so that
+// \ becomes %5C and %2e a dot.
 func pathAndQuery(r *http.Request) string {
 	if !r.URL.IsAbs() {
 		return r.RequestURI

internal/rules/rules_test.go

@@ -164,28 +164,6 @@ func TestFilesReadInNameOrderThenLineOrder(t *testing.T) {
 	}
 }
 
-func TestFileWhoseNameStartsWithADotIsNotARuleFile(t *testing.T) {
-	t.Parallel()
-
-	dir := writeFiles(t, ruleFiles{firstFile: "probe path block ^/probe\n"})
-
-	// The lock file Emacs makes beside a file while it is edited: a link to
-	// nothing, which cannot be read.
-	err := os.Symlink("user@host.1234:1700000000", filepath.Join(dir, ".#"+firstFile))
-	if err != nil {
-		t.Fatalf("symlink: %v", err)
-	}
-
-	params, _ := newParams(dir)
-
-	files, err := rules.Load(params)
-	if err != nil {
-		t.Fatalf("load: %v", err)
-	}
-
-	wantMatched(t, files, get(t, "/probe"), "probe")
-}
-
 func TestFaultStopsTheStartNamingTheFileAndLine(t *testing.T) {
 	t.Parallel()
 
@@ -622,8 +600,8 @@ func (l processLog) waitFor(t *testing.T, msg string) map[string]any {
 }
 
 // waitUntil waits for the rule files to be read until done reports true,
-// as it does once they have been read after the test's last change. They
-// can be read before then too, as they are once Watch starts watching.
+// as it does once they have been read after the test's last change. One
+// change can be seen more than once, and so read more than once.
 func (l processLog) waitUntil(t *testing.T, done func() bool) {
 	t.Helper()
 

internal/rules/watch_internal_test.go

@@ -1,162 +0,0 @@
-package rules
-
-import (
-	"context"
-	"log/slog"
-	"net/http"
-	"net/http/httptest"
-	"os"
-	"path/filepath"
-	"slices"
-	"testing"
-	"testing/synctest"
-	"time"
-
-	"github.com/fsnotify/fsnotify"
-)
-
-// The tests below run readAfterChanges in a synctest bubble, where time is
-// a clock of the test's own: time.Sleep moves it on at once, and
-// synctest.Wait returns once readAfterChanges waits again, so that every
-// reading due by then is done. The test sends the changes itself, as the
-// watch of a directory cannot run in a bubble.
-
-func TestFileWrittenInTwoPartsTakenInOnlyWhole(t *testing.T) {
-	t.Parallel()
-
-	synctest.Test(t, func(t *testing.T) {
-		dir := t.TempDir()
-		path := filepath.Join(dir, "50-app.rules")
-		writeFile(t, path, "first path block ^/first\n")
-		files := load(t, dir)
-		changes := run(t, files)
-
-		file, err := os.Create(path) //nolint:gosec // a file the test wrote
-		if err != nil {
-			t.Fatalf("create: %v", err)
-		}
-
-		defer func() {
-			_ = file.Close()
-		}()
-
-		// The first part ends in the middle of a ban rule's regex, which,
-		// read then, would ban every request.
-		write(t, file, "first path block ^/first\nprobe path ban ^/")
-
-		changes <- fsnotify.Event{Name: path, Op: fsnotify.Write}
-
-		time.Sleep(quietTime - time.Nanosecond)
-		synctest.Wait()
-		wantMatched(t, files, "/anything")
-
-		// The second part starts the wait again.
-		write(t, file, `\.env$`+"\n")
-
-		changes <- fsnotify.Event{Name: path, Op: fsnotify.Write}
-
-		time.Sleep(quietTime - time.Nanosecond)
-		synctest.Wait()
-		wantMatched(t, files, "/.env")
-
-		time.Sleep(time.Nanosecond)
-		synctest.Wait()
-		wantMatched(t, files, "/.env", "probe")
-		wantMatched(t, files, "/anything")
-	})
-}
-
-func TestEditSavedBeforeTheWatchStartsTakenIn(t *testing.T) {
-	t.Parallel()
-
-	synctest.Test(t, func(t *testing.T) {
-		dir := t.TempDir()
-		path := filepath.Join(dir, "50-app.rules")
-		writeFile(t, path, "first path block ^/first\n")
-		files := load(t, dir)
-
-		// Saved after Load read the files, and before the directory was
-		// watched, so that no change is seen for it.
-		writeFile(t, path, "first path block ^/edited\n")
-		run(t, files)
-		time.Sleep(quietTime)
-		synctest.Wait()
-		wantMatched(t, files, "/edited", "first")
-	})
-}
-
-// load loads the rules in dir.
-func load(t *testing.T, dir string) *Files {
-	t.Helper()
-
-	files, err := Load(Params{
-		Dir: dir, Enabled: true, ProcessLog: slog.New(slog.DiscardHandler),
-	})
-	if err != nil {
-		t.Fatalf("load: %v", err)
-	}
-
-	return files
-}
-
-// run runs files' readAfterChanges until the test ends, and returns the
-// channel that sends it changes.
-func run(t *testing.T, files *Files) chan<- fsnotify.Event {
-	t.Helper()
-
-	changes := make(chan fsnotify.Event)
-	ctx, stop := context.WithCancel(t.Context())
-	stopped := make(chan struct{})
-
-	go func() {
-		files.readAfterChanges(ctx, changes, nil)
-		close(stopped)
-	}()
-
-	t.Cleanup(func() {
-		stop()
-		<-stopped
-	})
-
-	return changes
-}
-
-// writeFile writes content to the file at path.
-func writeFile(t *testing.T, path, content string) {
-	t.Helper()
-
-	err := os.WriteFile(path, []byte(content), 0o600)
-	if err != nil {
-		t.Fatalf("write %s: %v", path, err)
-	}
-}
-
-// write writes text to the end of file.
-func write(t *testing.T, file *os.File, text string) {
-	t.Helper()
-
-	_, err := file.WriteString(text)
-	if err != nil {
-		t.Fatalf("write: %v", err)
-	}
-}
-
-// wantMatched checks the ids of the rules that a GET request for path
-// matches, in order.
-func wantMatched(t *testing.T, files *Files, path string, want ...string) {
-	t.Helper()
-
-	r := httptest.NewRequestWithContext(t.Context(), http.MethodGet,
-		"http://app.example"+path, nil)
-
-	matched := files.Match(r)
-
-	got := make([]string, 0, len(matched))
-	for _, rule := range matched {
-		got = append(got, rule.ID)
-	}
-
-	if !slices.Equal(got, want) {
-		t.Errorf("%s matched %v, want %v", path, got, want)
-	}
-}

internal/smallwebwaf/smallwebwaf_test.go

@@ -343,21 +343,8 @@ func TestRuleFileAddedWhileRunningTakesEffect(t *testing.T) {
 
 	out := runUntilStopped(t, env, func(url string) {
 		wantGreeting(t, url)
-
-		// Written once: each change would start the rule files' wait
-		// again. A file written before smallwebwaf watches the directory is
-		// read once it does.
-		err := os.WriteFile(filepath.Join(dir, "50-app.rules"),
-			[]byte("everything path block ^/\n"), 0o600)
-		if err != nil {
-			t.Fatalf("write the rule file: %v", err)
-		}
-
-		// As long as that takes, so that a slow test process cannot fail
-		// the test.
-		for statusFrom(t, url, "203.0.113.9") != http.StatusForbidden {
-			time.Sleep(pollInterval)
-		}
+		saveUntilAnswered(t, filepath.Join(dir, "50-app.rules"),
+			"everything path block ^/\n", url, "203.0.113.9", http.StatusForbidden)
 	})
 	out.line(t, "action", "rule_blocked")
 }