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

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

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

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

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