From 70e558f40111dfe618fc42c4b8fae02f7900a545 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 6 Oct 2026 21:19:43 +0000 Subject: [PATCH] Settings given as files: the _FILE form of every setting (closes #87) Every setting X may instead be given as a file that X_FILE names, read once at start: its contents, less one trailing newline, are the value, checked as X would be. X and X_FILE both set, or a file that cannot be read, stops the start with a message naming the variable. The logged settings name the file, and mask a token read from one. SWWAF_LOG_REMOTE_TLS_CA_FILE, whose value is a file already, has no _FILE form. The health check reads only SWWAF_LISTEN_ADDR and SWWAF_UPSTREAM_URL, so no other setting or file can fail it. Judgement call: an invalid value read from a file is named as X, not X_FILE. Rule suppressed: gosec G304 on reading the named file, as for the CA file. Model: opus-5-5 --- README.md | 42 +++++++- SPEC.md | 5 +- internal/config/config.go | 93 ++++++++++++++--- internal/config/config_test.go | 127 +++++++++++++++++++++++ internal/smallwebwaf/healthcheck.go | 8 +- internal/smallwebwaf/healthcheck_test.go | 32 ++++++ 6 files changed, 283 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index a545085..80f9fa4 100644 --- a/README.md +++ b/README.md @@ -181,9 +181,10 @@ in `bin/state` unless `SWWAF_STATE_DIR` is set, and the default rule file of ## Settings -Each setting is an environment variable, and each has a default, so none has to -be set. A setting that is set but invalid stops the start with a message naming -it, and the effective settings are logged at start. +Each setting is an environment variable, or a file one names (see "Settings +given as files" below), and each has a default, so none has to be set. A setting +that is set but invalid stops the start with a message naming 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 @@ -288,7 +289,8 @@ it, and the effective settings are logged at start. - `SWWAF_METRICS_TOKEN` (default unset): the token a scraper sends for the metrics, a long random value. While it is unset the metrics are off; one shorter than 32 characters stops the start. The settings logged at start show - `********` in its place. + `********` in its place. Given as a file, it can be kept out of the app's + reach (see "Settings given as files" below). - `SWWAF_METRICS_TOP_N` (default `50`): how many countries get series of their own in the metrics by country; the others are counted as `other`. - `SWWAF_RULES_DIR` (default `/etc/smallwebwaf/rules.d`): the directory of the @@ -329,6 +331,38 @@ with their counters and history, and an IPv6 client is counted by its /64. A new client waits at most a second for its country, and at most 100,000 answers from GeoJS are kept, for 7 days each. +### Settings given as files + +Any setting may instead be given as a file that holds its value: the variable +named like the setting with `_FILE` added, such as `SWWAF_METRICS_TOKEN_FILE`, +names the file. `smallwebwaf` reads the file once, at start, and its health +check reads only the files of `SWWAF_LISTEN_ADDR` and `SWWAF_UPSTREAM_URL`, each +time it runs. The file's contents are the value, less one newline at their end +so that a file written with `echo` or an editor works, and are checked as the +setting's own value would be. Setting both the setting and its `_FILE` form, or +naming a file that cannot be read, stops the start with a message naming the +variable. The settings logged at start name the file, and show a token given in +one as `********`, as they show one given directly. +`SWWAF_LOG_REMOTE_TLS_CA_FILE`, whose value names a file already, has no `_FILE` +form. + +The app starts with the same environment variables as `smallwebwaf`, so it can +read a token given as one. A token given as a file is out of the app's reach +only while the `smallwebwaf` user alone can read the file: make it on the host, +owned by uid 65532, the `smallwebwaf` user, with mode `0400`, and mount the +directory that holds it into the container read-only; the container sees the +same owner and mode. For example, on the host: + +```sh +mkdir -p /srv/app/tokens +openssl rand -hex 32 > /srv/app/tokens/metrics +chown 65532:65532 /srv/app/tokens/metrics +chmod 0400 /srv/app/tokens/metrics +``` + +and for the container, `-v /srv/app/tokens:/etc/smallwebwaf/tokens:ro` and +`-e SWWAF_METRICS_TOKEN_FILE=/etc/smallwebwaf/tokens/metrics`. + ## Request log `smallwebwaf` writes one JSON object per line on stdout for every request, diff --git a/SPEC.md b/SPEC.md index 9071387..6edd41a 100644 --- a/SPEC.md +++ b/SPEC.md @@ -32,7 +32,7 @@ from a directory of hand-editable text files. - Defence against traffic floods that saturate the host's network link. That needs help upstream of the host. - A web UI or a configuration file. Settings are environment variables. Apart - from settings given as files (the `_FILE` form of any setting, such as + from settings given as files (the `_FILE` form of a setting, such as `SWWAF_ADMIN_TOKEN_FILE`, and `SWWAF_LOG_REMOTE_TLS_CA_FILE`), its own state files and the lookup database, the only files read are the rule files, which hold one regex per line and nothing more elaborate. @@ -298,7 +298,8 @@ it. - A list set to an empty value is an empty list, and replaces the default. - Every setting may instead be given as a file holding the value, named by the setting's name with `_FILE` added, such as `SWWAF_ADMIN_TOKEN_FILE`, for - secrets and long lists. + secrets and long lists. `SWWAF_LOG_REMOTE_TLS_CA_FILE`, whose value names a + file already, has no `_FILE` form. - Settings, including those given as files, are read once at start; changing one means restarting the container. The files `smallwebwaf` watches while it runs are its state files, its rule files and the lookup database. diff --git a/internal/config/config.go b/internal/config/config.go index 0f0ddc0..e5552ee 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ // Package config reads smallwebwaf's settings. Every setting is an -// environment variable whose name starts with SWWAF_, every setting has a -// default, and this package is the one place they are read. +// environment variable whose name starts with SWWAF_, or a file such a +// variable names, every setting has a default, and this package is the +// one place they are read. package config import ( @@ -154,8 +155,8 @@ type Config struct { LogRemoteFacility int LogRemoteAppName string - // settings are the values read, as given or by default, for the - // log line at start. + // settings are the values read, as given or by default, and the + // files they were read from, for the log line at start. settings []slog.Attr } @@ -173,6 +174,10 @@ const ( minTokenLength = 32 // masked is what the log shows for a token that is set. masked = "********" + // defaultListenAddr and defaultUpstreamURL are the defaults of + // SWWAF_LISTEN_ADDR and SWWAF_UPSTREAM_URL. + defaultListenAddr = ":8080" + defaultUpstreamURL = "http://127.0.0.1:8081" ) var ( @@ -221,17 +226,20 @@ var ( errNotFacility = errors.New("is not a syslog facility such as local0 or daemon") errNotAppName = errors.New( "is not 1 to 48 printable ASCII characters without a space, such as gitea") + errSetTwice = errors.New("set only one of them") ) // FromEnvironment reads the settings with lookupEnv, normally -// os.LookupEnv. A setting that is not set takes its default. A setting -// that is set but invalid is an error that names it. +// os.LookupEnv. A setting may instead be given as a file: the variable +// named by the setting's name with _FILE added names the file, which is +// read now (see lookup). A setting that is not set takes its default. A +// setting that is set but invalid is an error that names it. func FromEnvironment(lookupEnv func(string) (string, bool)) (*Config, error) { env := &environment{lookupEnv: lookupEnv} hostname, _ := os.Hostname() // "" when the host has no name to give cfg := &Config{ - ListenAddr: env.address("SWWAF_LISTEN_ADDR", ":8080"), - UpstreamURL: env.appURL("SWWAF_UPSTREAM_URL", "http://127.0.0.1:8081"), + ListenAddr: env.address("SWWAF_LISTEN_ADDR", defaultListenAddr), + UpstreamURL: env.appURL("SWWAF_UPSTREAM_URL", defaultUpstreamURL), InstanceName: env.value("SWWAF_INSTANCE_NAME", hostname), Observe: env.observe("SWWAF_MODE", "enforce"), TrustedProxies: env.netblocks("SWWAF_TRUSTED_PROXIES", privateRanges), @@ -295,6 +303,24 @@ func FromEnvironment(lookupEnv func(string) (string, bool)) (*Config, error) { return cfg, nil } +// ListenAddrAndUpstreamURL reads only SWWAF_LISTEN_ADDR and +// SWWAF_UPSTREAM_URL, either of which may be given as a file, as +// FromEnvironment does. The health check needs no other setting, so it +// reads no other, nor a file that another names. +func ListenAddrAndUpstreamURL( + lookupEnv func(string) (string, bool), +) (string, *url.URL, error) { + env := &environment{lookupEnv: lookupEnv} + listenAddr := env.address("SWWAF_LISTEN_ADDR", defaultListenAddr) + upstreamURL := env.appURL("SWWAF_UPSTREAM_URL", defaultUpstreamURL) + + if env.err != nil { + return "", nil, env.err + } + + return listenAddr, upstreamURL, nil +} + // privateRanges are the private address ranges, the default trusted // proxies. const privateRanges = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" @@ -316,8 +342,8 @@ type environment struct { // value returns a setting's value, or its default when it is not set, // and notes it for the log. func (e *environment) value(name, defaultValue string) string { - value, ok := e.lookupEnv(name) - if !ok { + value, set := e.lookup(name) + if !set { value = defaultValue } @@ -326,6 +352,37 @@ func (e *environment) value(name, defaultValue string) string { return value } +// lookup returns a setting's value and whether it is set: the value of the +// variable name, or the contents of the file that the variable name_FILE +// names, less one newline at their end. It notes that file's path for the +// log. Both variables set, or a file that cannot be read, is an error. +func (e *environment) lookup(name string) (string, bool) { + value, set := e.lookupEnv(name) + fileName := name + "_FILE" + + path, inFile := e.lookupEnv(fileName) + if !inFile { + return value, set + } + + if set { + e.check(name, fmt.Errorf("is set, and so is %s; %w", fileName, errSetTwice)) + + return value, set + } + + e.settings = append(e.settings, slog.String(fileName, path)) + + contents, err := os.ReadFile(path) //nolint:gosec // a file the admin names + if err != nil { + e.check(fileName, fmt.Errorf("cannot be read: %w", err)) + + return "", false + } + + return strings.TrimSuffix(string(contents), "\n"), true +} + // check keeps the first error, naming the setting it is about. func (e *environment) check(name string, err error) { if err != nil && e.err == nil { @@ -484,7 +541,7 @@ func (e *environment) absolutePath(name, defaultValue string) string { // switches off what it guards; set, it must be at least minTokenLength // characters. Neither the log nor an error shows its value. func (e *environment) token(name string) string { - value, set := e.lookupEnv(name) + value, set := e.lookup(name) if !set { e.settings = append(e.settings, slog.String(name, "")) @@ -515,9 +572,12 @@ func (e *environment) logRemoteURL(name string) *url.URL { } // certificates reads a setting that is the path of a file of PEM -// certificates. Unset or empty, it is nil. +// certificates. Unset or empty, it is nil. Its value names a file +// already, so, unlike the other settings, it has no _FILE form. func (e *environment) certificates(name string) *x509.CertPool { - path := e.value(name, "") + path, _ := e.lookupEnv(name) + e.settings = append(e.settings, slog.String(name, path)) + if path == "" { return nil } @@ -552,9 +612,12 @@ func (e *environment) facility(name, defaultValue string) int { // lines are sent in, by default the instance name. Its value is checked // when it is set, and, while lines are sent, when it is the instance name. func (e *environment) appName(name, instanceName string, sending bool) string { - _, set := e.lookupEnv(name) + value, set := e.lookup(name) + if !set { + value = instanceName + } - value := e.value(name, instanceName) + e.settings = append(e.settings, slog.String(name, value)) switch { case isAppName(value): diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 30a97e1..00cf272 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -722,6 +722,133 @@ func TestTokenIsLoggedMasked(t *testing.T) { } } +func TestSettingFromFileLosesOneNewlineAndNoMore(t *testing.T) { + t.Parallel() + + for contents, want := range map[string]string{ + token: token, + token + "\n": token, + token + "\n\n": token + "\n", + token + " \n": token + " ", + } { + cfg := fromEnvironment(t, environment{ + metricsToken + "_FILE": writeFile(t, contents), + }) + if cfg.MetricsToken != want { + t.Errorf("file holding %q gave %s %q, want %q", contents, metricsToken, + cfg.MetricsToken, want) + } + } +} + +func TestSettingFromFileIsCheckedAsTheSettingItself(t *testing.T) { + t.Parallel() + + _, err := config.FromEnvironment(environment{ + requestMaxBytes + "_FILE": writeFile(t, "lots\n"), + }.lookupEnv) + + want := requestMaxBytes + `: "lots" is not a size such as 512K, 100M or 5G, or off` + if err == nil || err.Error() != want { + t.Errorf("error %v, want %s", err, want) + } + + _, err = config.FromEnvironment(environment{ + logRemoteURL: remoteURL, + instanceName: instance, + logRemoteAppName + "_FILE": writeFile(t, "my app\n"), + }.lookupEnv) + + want = logRemoteAppName + `: "my app" is not 1 to 48 printable ASCII ` + + `characters without a space, such as gitea` + if err == nil || err.Error() != want { + t.Errorf("error %v, want %s", err, want) + } +} + +func TestSettingAndItsFileBothSetStopsTheStart(t *testing.T) { + t.Parallel() + + _, err := config.FromEnvironment(environment{ + metricsToken: token, + metricsToken + "_FILE": writeFile(t, token), + }.lookupEnv) + + want := metricsToken + ": is set, and so is " + metricsToken + + "_FILE; set only one of them" + if err == nil || err.Error() != want { + t.Errorf("error %v, want %s", err, want) + } +} + +func TestUnreadableSettingFileStopsTheStart(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + for _, path := range []string{filepath.Join(dir, "missing"), dir} { + _, err := config.FromEnvironment(environment{metricsToken + "_FILE": path}.lookupEnv) + + want := metricsToken + "_FILE: cannot be read: " + if err == nil || !strings.HasPrefix(err.Error(), want) { + t.Errorf("%s: error %v, want one starting %s", path, err, want) + } + } +} + +func TestTokenFromFileIsLoggedMaskedWithTheFile(t *testing.T) { + t.Parallel() + + path := writeFile(t, token+"\n") + cfg := fromEnvironment(t, environment{metricsToken + "_FILE": path}) + + var out bytes.Buffer + + slog.New(slog.NewJSONHandler(&out, nil)).Info("starting", "settings", cfg) + + var line struct { + Settings map[string]string `json:"settings"` + } + + err := json.Unmarshal(out.Bytes(), &line) + if err != nil { + t.Fatalf("decode %s: %v", out.Bytes(), err) + } + + if strings.Contains(out.String(), token) || + line.Settings[metricsToken] != "********" || + line.Settings[metricsToken+"_FILE"] != path { + t.Errorf("the token is not logged masked, with its file %s: %s", path, + out.String()) + } +} + +func TestRemoteLogCAFileIsNotReadAsAFileInItsTurn(t *testing.T) { + t.Parallel() + + cfg := fromEnvironment(t, environment{ + logRemoteTLSCAFile + "_FILE": writeFile(t, "/nonexistent/ca.pem\n"), + }) + if cfg.LogRemoteTLSCAs != nil { + t.Errorf("%s_FILE gave certificates", logRemoteTLSCAFile) + } +} + +// writeFile writes contents to a file in a directory of its own, removed +// when the test ends, and returns the file's path. +func writeFile(t *testing.T, contents string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "setting") + + err := os.WriteFile(path, []byte(contents), 0o600) + if err != nil { + t.Fatalf("write %s: %v", path, err) + } + + return path +} + func TestLogsEachSettingWithItsValue(t *testing.T) { t.Parallel() diff --git a/internal/smallwebwaf/healthcheck.go b/internal/smallwebwaf/healthcheck.go index 9269f3e..cb5f9e7 100644 --- a/internal/smallwebwaf/healthcheck.go +++ b/internal/smallwebwaf/healthcheck.go @@ -23,6 +23,8 @@ var errHealthEndpoint = errors.New("smallwebwaf's health endpoint answered") // smallwebwaf answers its health endpoint on 127.0.0.1, at the port in // SWWAF_LISTEN_ADDR, and the app accepts connections at the address in // SWWAF_UPSTREAM_URL. Otherwise it writes why to stderr and returns 1. +// It reads no other setting, nor a file that another names, so neither +// can fail it. // args are the arguments after `healthcheck`; it takes none, and given // one it names it on stderr and returns 1 without checking anything. func HealthCheck( @@ -50,13 +52,13 @@ func healthCheck(ctx context.Context, lookupEnv func(string) (string, bool)) err ctx, cancel := context.WithTimeout(ctx, healthCheckTimeout) defer cancel() - cfg, err := config.FromEnvironment(lookupEnv) + listenAddr, upstreamURL, err := config.ListenAddrAndUpstreamURL(lookupEnv) if err != nil { return fmt.Errorf("invalid setting: %w", err) } // The settings have checked that the address has a port. - _, port, _ := net.SplitHostPort(cfg.ListenAddr) + _, port, _ := net.SplitHostPort(listenAddr) health := "http://" + net.JoinHostPort("127.0.0.1", port) + proxy.HealthPath req, err := http.NewRequestWithContext(ctx, http.MethodGet, health, http.NoBody) @@ -75,7 +77,7 @@ func healthCheck(ctx context.Context, lookupEnv func(string) (string, bool)) err return fmt.Errorf("%w %s", errHealthEndpoint, res.Status) } - conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", appAddress(cfg.UpstreamURL)) + conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", appAddress(upstreamURL)) if err != nil { return fmt.Errorf("connect to the app: %w", err) } diff --git a/internal/smallwebwaf/healthcheck_test.go b/internal/smallwebwaf/healthcheck_test.go index 56af193..3089f22 100644 --- a/internal/smallwebwaf/healthcheck_test.go +++ b/internal/smallwebwaf/healthcheck_test.go @@ -6,6 +6,8 @@ import ( "net" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "time" @@ -43,6 +45,21 @@ func TestHealthCheck(t *testing.T) { wantHealthCheck(t, env, 0, "") + // The health check reads those two settings alone, here given as + // files: a removed or invalid token file, or an invalid value of + // another setting, does not fail it. + for _, other := range []struct{ name, value string }{ + {"SWWAF_METRICS_TOKEN_FILE", filepath.Join(t.TempDir(), "removed")}, + {"SWWAF_METRICS_TOKEN_FILE", writeFile(t, "too short\n")}, + {"SWWAF_MODE", "neither"}, + } { + wantHealthCheck(t, map[string]string{ + listenAddr + "_FILE": writeFile(t, ":"+port+"\n"), + upstreamURL + "_FILE": writeFile(t, app.URL+"\n"), + other.name: other.value, + }, 0, "") + } + app.Close() wantHealthCheck(t, env, 1, "unhealthy: connect to the app: ") @@ -98,3 +115,18 @@ func wantHealthCheck(t *testing.T, env map[string]string, status int, message st got, wrote, status, message) } } + +// writeFile writes contents to a file in a directory of its own, removed +// when the test ends, and returns the file's path. +func writeFile(t *testing.T, contents string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "setting") + + err := os.WriteFile(path, []byte(contents), 0o600) + if err != nil { + t.Fatalf("write %s: %v", path, err) + } + + return path +}