Header limit of 32 KiB, upstream URL checks, build and run scripts

The request line and headers are now refused above 32 KiB exactly: Go's
server reads 4 KiB past MaxHeaderBytes, so that is set to 28 KiB, and
the tests try exactly 32 KiB and one byte more. SWWAF_UPSTREAM_URL now
needs a host, and a given port must be from 1 to 65535. make build and
make run call script/build and script/run.

Model: opus-5-5

Makefile

@@ -4,10 +4,6 @@
 # per the scripts-to-rule-them-all pattern (see the Entrypoints section
 # of README.md). build and run are for working on the code by hand.
 
-# The version the binary reports. It comes from git, so it is computed
-# here on the host; the Dockerfile is passed it as a build argument.
-VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo unknown)
-
 bootstrap:
 	@script/bootstrap
 
@@ -36,8 +32,7 @@ hooks:
 	@script/install-precommit
 
 build:
-	go build -trimpath -ldflags "-X main.Version=$(VERSION)" \
-		-o bin/smallwebwaf ./cmd/smallwebwaf
+	@script/build
 
-run: build
-	./bin/smallwebwaf
+run:
+	@script/run

README.md

@@ -70,7 +70,7 @@ 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 a port, and nothing more.
+  `https`, a host and an optional port, and nothing more.
 - `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.
@@ -125,10 +125,10 @@ settings, stop, errors) share the stream as JSON lines marked
 
 Go's HTTP server, on which `smallwebwaf` is built, reads a request's line and
 headers before `smallwebwaf` sees the request, and some requests end there,
-without a line in the log: headers over 32 KiB, which it answers `431` (reading
-up to 4 KiB past the limit first), headers slower than
-`SWWAF_CLIENT_REQUEST_TIMEOUT`, whose connection it closes without an answer,
-and requests it cannot read at all, which it answers itself, mostly with `400`.
+without a line in the log: headers over 32 KiB, which it answers `431`, headers
+slower than `SWWAF_CLIENT_REQUEST_TIMEOUT`, whose connection it closes without
+an answer, and requests it cannot read at all, which it answers itself, mostly
+with `400`.
 
 ## Why
 
@@ -406,8 +406,10 @@ so that they run in minimal containers.
   image build.
 - `script/precommit`: run by the git pre-commit hook; runs `script/check`.
 - `script/install-precommit`: installs that hook; `make hooks` runs it.
-
-`make build` builds `bin/smallwebwaf`, and `make run` builds and runs it.
+- `script/build`: builds `bin/smallwebwaf` on the host, with Go installed, for
+  working on the code by hand; `make build` runs it.
+- `script/run`: builds `bin/smallwebwaf` with `script/build` and runs it;
+  `make run` runs it.
 
 ## TODO
 

SPEC.md

@@ -442,8 +442,7 @@ The settings, by group:
       longer than `SWWAF_CLIENT_REQUEST_TIMEOUT` to send them gets no answer:
       the server closes its connection. Headers over
       `SWWAF_CLIENT_REQUEST_HEADER_MAX_BYTES` are answered `431` by the server
-      itself, which reads up to 4 KiB past the limit before it refuses. Neither
-      request gets a line in the request log.
+      itself. Neither request gets a line in the request log.
     - A WebSocket connection leaves these limits behind once it is upgraded: it
       stays open until either side closes it.
 - Lookup of AS number and country (R7). On by default through GeoJS, which needs

internal/config/config.go

@@ -71,7 +71,7 @@ var (
 	errNotListenAddr = errors.New(
 		"is not an address to listen on, such as :8080")
 	errNotUpstreamURL = errors.New(
-		"is not a URL with only a scheme, a host and a port, " +
+		"is not a URL with only a scheme, a host and an optional port, " +
 			"such as http://127.0.0.1:8081")
 )
 
@@ -325,8 +325,8 @@ func parseListenAddr(value string) (string, error) {
 }
 
 // parseUpstreamURL reads the app's URL: http or https, a host and an
-// optional port, and nothing else, since the request's own path and
-// query go to the app unchanged.
+// optional port from 1 to 65535, and nothing else, since the request's
+// own path and query go to the app unchanged.
 func parseUpstreamURL(value string) (*url.URL, error) {
 	upstream, err := url.Parse(value)
 	if err != nil {
@@ -334,12 +334,19 @@ func parseUpstreamURL(value string) (*url.URL, error) {
 	}
 
 	onlySchemeAndHost := (upstream.Scheme == "http" || upstream.Scheme == "https") &&
-		upstream.Host != "" && upstream.User == nil && upstream.Opaque == "" &&
+		upstream.Hostname() != "" && upstream.User == nil && upstream.Opaque == "" &&
 		(upstream.Path == "" || upstream.Path == "/") &&
 		upstream.RawQuery == "" && upstream.Fragment == ""
 	if !onlySchemeAndHost {
 		return nil, fmt.Errorf("%q %w", value, errNotUpstreamURL)
 	}
 
+	if upstream.Port() != "" {
+		port, err := strconv.ParseUint(upstream.Port(), 10, 16)
+		if err != nil || port == 0 {
+			return nil, fmt.Errorf("%q %w", value, errNotUpstreamURL)
+		}
+	}
+
 	return upstream, nil
 }

internal/config/config_test.go

@@ -142,6 +142,9 @@ func TestInvalidValueStopsTheStart(t *testing.T) {
 		{upstreamURL, "127.0.0.1:8081"},
 		{upstreamURL, "ftp://127.0.0.1:8081"},
 		{upstreamURL, "http://"},
+		{upstreamURL, "http://:8081"},
+		{upstreamURL, "http://127.0.0.1:0"},
+		{upstreamURL, "http://127.0.0.1:99999"},
 		{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"},

internal/proxy/passthrough_test.go

@@ -309,7 +309,7 @@ func TestServerHasTheFixedLimits(t *testing.T) {
 		ProcessLog: requestlog.NewProcessLogger(io.Discard),
 	})
 
-	if server.Addr != ":8080" || server.MaxHeaderBytes != 32<<10 ||
+	if server.Addr != ":8080" || server.MaxHeaderBytes != 28<<10 ||
 		server.IdleTimeout != 2*time.Minute || server.ReadHeaderTimeout != time.Minute {
 		t.Errorf("server listens on %q with header limit %d, idle time %s and "+
 			"header timeout %s", server.Addr, server.MaxHeaderBytes,
@@ -327,16 +327,23 @@ func TestRefusesHeadersOver32KiB(t *testing.T) {
 	})
 	addr, _ := startProxy(t, app.URL, nil)
 
+	// size counts every byte of the request: the request line, the
+	// headers and the blank line that ends them.
+	const (
+		start = "GET / HTTP/1.1\r\nHost: app\r\nX-Large: "
+		end   = "\r\n\r\n"
+	)
+
 	for _, tc := range []struct {
-		headerSize int
-		want       int
+		size int
+		want int
 	}{
-		{headerSize: 30 << 10, want: http.StatusOK},
-		{headerSize: 40 << 10, want: http.StatusRequestHeaderFieldsTooLarge},
+		{size: 32 << 10, want: http.StatusOK},
+		{size: 32<<10 + 1, want: http.StatusRequestHeaderFieldsTooLarge},
 	} {
-		req := newRequest(t, http.MethodGet, addr, "/", http.NoBody)
-		req.Header.Set("X-Large", strings.Repeat("a", tc.headerSize))
-		wantStatus(t, do(t, req), tc.want)
+		conn := dial(t, addr)
+		send(t, conn, start+strings.Repeat("a", tc.size-len(start)-len(end))+end)
+		wantStatus(t, readResponse(t, conn), tc.want)
 	}
 
 	if calls.Load() != 1 {

internal/proxy/proxy.go

@@ -15,11 +15,14 @@ import (
 
 // The request line and headers a client may send, and how long a
 // kept-open client connection may wait for its next request, are fixed
-// rather than settings. The idle time is longer than the 90 seconds after
-// which traefik closes a connection it is not using, so traefik never
-// sends a request on a connection smallwebwaf is closing.
+// rather than settings. The limit on the request line and headers is
+// 32 KiB, but Go's server reads 4 KiB past its MaxHeaderBytes before it
+// refuses, so MaxHeaderBytes is set 4 KiB lower. The idle time is longer
+// than the 90 seconds after which traefik closes a connection it is not
+// using, so traefik never sends a request on a connection smallwebwaf is
+// closing.
 const (
-	requestHeaderMaxBytes = 32 << 10
+	requestHeaderMaxBytes = 32<<10 - 4<<10
 	clientIdleTimeout     = 120 * time.Second
 )
 

script/build

@@ -0,0 +1,18 @@
+#!/bin/sh
+# script/build: build bin/smallwebwaf on the host, with Go installed, for
+# working on the code by hand. The version it reports comes from git, as
+# in script/docker.
+set -eu
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
+ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
+
+main() {
+    cd "$ROOT"
+    version="$(git describe --tags --always --dirty 2>/dev/null || true)"
+    [ -n "$version" ] || version="unknown"
+    go build -trimpath -ldflags "-X main.Version=$version" \
+        -o bin/smallwebwaf ./cmd/smallwebwaf
+}
+
+main "$@"

script/run

@@ -0,0 +1,14 @@
+#!/bin/sh
+# script/run: build bin/smallwebwaf with script/build and run it, with
+# the settings in the environment.
+set -eu
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
+ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
+
+main() {
+    "$SCRIPT_DIR/build"
+    exec "$ROOT/bin/smallwebwaf"
+}
+
+main "$@"