diff --git a/README.md b/README.md index 43b9565..abe505f 100644 --- a/README.md +++ b/README.md @@ -104,6 +104,7 @@ TTY detection, and security headers are always applied. | ----------------------- | ----------------------------------- | -------- | | `WEBHOOKER_ENVIRONMENT` | `dev` or `prod` | `dev` | | `PORT` | HTTP listen port | `8080` | +| `BIND_ADDRESS` | IP address the HTTP listener binds. Loopback by default, so the cleartext listener is not published on every interface. Containers must set `0.0.0.0`. See [Bind address](#bind-address) | `127.0.0.1` | | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | | `DEBUG` | Enable debug logging | `false` | | `MAINTENANCE_MODE` | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false` | @@ -225,6 +226,58 @@ A set but unparseable value aborts startup. When the list is non-empty webhooker logs it at startup, blocks and all, so the hole is visible in the log of any deployment that has one. +#### Bind address + +`BIND_ADDRESS` is the IP address the HTTP listener binds. It defaults +to `127.0.0.1`, so out of the box webhooker is reachable only from the +host it runs on. + +That listener speaks **cleartext**, and it serves both the admin UI and +the unauthenticated webhook receiver. webhooker terminates no TLS +itself; a production deployment puts a reverse proxy in front of it +(see +[Deployment behind a reverse proxy](#deployment-behind-a-reverse-proxy)), +and the proxy reaches it over loopback. A default that bound every +interface would leave that cleartext port answering the internet +alongside the proxy — the admin login form and the receiver, in the +clear, on a port nobody chose to publish. Reaching webhooker from +another host is therefore something you configure, not something you +get by default. + +**A container must set `BIND_ADDRESS=0.0.0.0`.** A process bound to +loopback inside a container is unreachable from outside its network +namespace even with `-p`, because the published port maps to the +container's external address and nothing is listening there. The +container's namespace is its own boundary, and `-p` is the exposure +decision. The `docker run` command in +[Running with Docker](#running-with-docker) sets it. + +The value must be an IP address literal: + +- `127.0.0.1` — loopback only (the default). Use this with a reverse + proxy on the same host. +- `0.0.0.0` — every IPv4 address. Required in a container; on a bare + host, only with a firewall in front of the port. +- `::` — every address, IPv6 and (on Linux, with the default + `net.ipv6.bindv6only=0`) IPv4 as well. +- A specific address such as `10.0.0.5` — that interface only. + +Hostnames are **not** accepted. `localhost` aborts startup rather than +being resolved: which of `127.0.0.1` and `::1` it means differs by +host, a name can resolve to several addresses of which only one could +be bound, and the answer can change under a running process. A value +carrying a port (`127.0.0.1:8080`) is likewise rejected — the port is +`PORT`'s business. Any unparseable value aborts startup; see +[Invalid values abort startup](#invalid-values-abort-startup). + +An address that parses but is not assigned to this host — say +`10.0.0.5` on a machine that has no such interface — is a valid +literal, so it reaches the listener and fails there. The process logs +the bind error and exits non-zero rather than staying up with nothing +listening. The effective value is in the `bindAddress` field of the +startup log line, which is the way to check what a running deployment +actually bound. + #### Metrics credentials `METRICS_USERNAME` and `METRICS_PASSWORD` are set together or not at @@ -382,8 +435,11 @@ additionally be a number in the range 1–65535, `RECEIVER_RATE_LIMIT` must be at least 1, `RETENTION_SWEEP_INTERVAL` must be greater than zero (it is a ticker period, so `0s` or a negative value would crash the reaper after -startup), and every entry in `TRUSTED_PROXIES` and -`ALLOWED_EGRESS_CIDRS` must be a CIDR block or a bare IP address. +startup), every entry in `TRUSTED_PROXIES` and +`ALLOWED_EGRESS_CIDRS` must be a CIDR block or a bare IP address, and +`BIND_ADDRESS` must be an IP address literal — `localhost`, +`127.0.0.1:8080` and `10.0.0.0/8` are each rejected rather than +resolved, split, or narrowed to something they do not say. `SESSION_IDLE_TIMEOUT` is the exception: a non-positive value there means idle expiry is disabled, not invalid. @@ -528,12 +584,32 @@ decision: ```bash docker run -d \ - -p 8080:8080 \ + -p 127.0.0.1:8080:8080 \ -v /path/to/data:/var/lib/webhooker \ -e WEBHOOKER_ENVIRONMENT=prod \ + -e BIND_ADDRESS=0.0.0.0 \ webhooker:latest ``` +`BIND_ADDRESS=0.0.0.0` is **required** in a container and is not the +default. webhooker binds loopback unless told otherwise (see +[Bind address](#bind-address)), and a loopback-bound process inside a +container is unreachable from outside its network namespace even with +`-p`: the port is published and nothing answers on it. The container's +health check fails too — it requests `http://localhost:8080`, and +`localhost` resolves to `::1` first, which a `127.0.0.1` bind is not +listening on — so the container goes `unhealthy` about 95 seconds +after start. A container that is `unhealthy` with `connection refused` +in its health log, or a published port that resets connections, is +this. + +Publishing to `127.0.0.1:8080` rather than `8080` keeps Docker from +opening the cleartext port on every interface of the host, which is +what a bare `-p 8080:8080` does — including through firewall rules, +since Docker's forwarding rules are inserted ahead of most host +firewalls. Bind it to the host address your reverse proxy connects +from, and nothing wider. + The container runs as a non-root user (`webhooker`, UID 1000), exposes port 8080, and includes a health check against `/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all @@ -543,6 +619,137 @@ databases written by `database` targets (`archive-{uuid}.db`). Mount this as a persistent volume to preserve data across container restarts. +## Deployment behind a reverse proxy + +webhooker terminates no TLS of its own. It serves plaintext HTTP and +expects a reverse proxy in front of it, which is the deployment it is +built for: the proxy holds the certificate, and webhooker binds +loopback where only the proxy can reach it. + +Five things have to be right. Each one is silent when it is wrong — +the service comes up, serves pages, and is broken in a way nothing +reports. + +1. **Bind or firewall the app port.** `BIND_ADDRESS` defaults to + `127.0.0.1` so the cleartext listener is not published beside the + proxy. If you must widen it — a container, or a proxy on another + host — firewall the port to the proxy's address. A cleartext port + answering the internet serves the admin login form and the + unauthenticated receiver with no TLS at all, and the proxy in front + of it changes nothing about that. +2. **Set `WEBHOOKER_ENVIRONMENT=prod`.** It defaults to `dev`, and the + session cookie's `Secure` flag depends on it — see the table in + [Configuration](#configuration). Left at the default, a session + cookie can be sent over plaintext HTTP. +3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate + limiter keys on the connecting peer, which behind a proxy is the + proxy on every request: all clients collapse into one global bucket + per limit and the receiver's per-IP limits become service-wide + ceilings. See [Trusted proxies](#trusted-proxies). List the proxy + and nothing else. +4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the + port. webhooker's Origin/Referer check compares against the host it + was given, so on any port other than 443 `$host` makes every form + POST — including login — fail with `403 origin invalid`, with + nothing in the error naming the cause. +5. **Keep the proxy's access log.** webhooker's own access log records + the peer address, which behind a proxy is always the proxy. The + proxy's log is the only record of which client sent what. nginx's + default `combined` format already logs `$remote_addr`; do not + replace it with one that drops the client address, and retain those + logs as long as you would want to answer a question about traffic. + +### nginx + +Complete server block. Replace the `server_name` and the two +certificate paths. + +```nginx +server { + listen 443 ssl; + listen [::]:443 ssl; + http2 on; # nginx 1.25.1+; older: listen 443 ssl http2; + + server_name webhooker.example.com; + + ssl_certificate /etc/ssl/certs/webhooker.example.com.crt; + ssl_certificate_key /etc/ssl/private/webhooker.example.com.key; + ssl_protocols TLSv1.2 TLSv1.3; + + # webhooker caps form POST bodies at 1 MB. nginx's default happens + # to match, so leaving this out breaks nothing today — but if you + # ever raise webhooker's cap, this is the limit you will still be + # hitting, and the rejection is nginx's HTML page rather than + # webhooker's message. + client_max_body_size 1m; + + # $remote_addr is the client. webhooker's own log records this + # proxy and nothing else, so this file is the only place the + # client's address is written down. + access_log /var/log/nginx/webhooker.access.log combined; + + location / { + # A literal address, not localhost: with BIND_ADDRESS at its + # 127.0.0.1 default, a localhost that resolves to ::1 first + # gets connection refused. + proxy_pass http://127.0.0.1:8080; + + # $http_host, NOT $host. $host drops the port and every form + # POST fails with 403 origin invalid on any port but 443. + proxy_set_header Host $http_host; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + + # Above webhooker's own 60s request timeout, so its 503 + # reaches the client instead of nginx cutting the connection + # first and answering 504. + proxy_read_timeout 70s; + } +} + +server { + listen 80; + listen [::]:80; + server_name webhooker.example.com; + return 308 https://$host$request_uri; +} +``` + +`X-Forwarded-For` must be **appended** to, which +`$proxy_add_x_forwarded_for` does. webhooker reads no other forwarded +client header: `X-Real-IP` and `True-Client-IP` are ignored from every +peer, so setting them has no effect. See +[Trusted proxies](#trusted-proxies) for how the chain is walked. + +`X-Forwarded-Proto: https` is what tells webhooker the request arrived +over TLS, which decides the CSRF cookie's `Secure` flag and the strict +Origin/Referer mode. Without it, requests are treated as plaintext. + +With that block, webhooker's environment is: + +```sh +WEBHOOKER_ENVIRONMENT=prod +BIND_ADDRESS=127.0.0.1 # the default; stated here to be explicit +TRUSTED_PROXIES=127.0.0.1 +``` + +If nginx runs on another host, `BIND_ADDRESS` becomes the address it +connects to, `TRUSTED_PROXIES` becomes nginx's address, and the port +must be firewalled to that address — the traffic between them is +cleartext. + +### HSTS is always sent, and is not configurable + +Every response carries +`Strict-Transport-Security: max-age=63072000; includeSubDomains; preload`. +Two years, every subdomain, and a `preload` token. There is no setting +that changes or suppresses it. + +This is worth knowing before the first request reaches a browser: a +client that sees it once will refuse plaintext HTTP to that hostname — +and to every subdomain of it — for two years, whatever else is served +there. Terminate TLS on a hostname you are prepared to keep on HTTPS. + ## Backup, Restore, and Upgrades ### What to back up @@ -678,6 +885,16 @@ Upgrade procedure: 4. Confirm `database migrations completed` in the logs before putting traffic back on it. +**Upgrading past the introduction of `BIND_ADDRESS`:** earlier versions +always bound every interface. The listener now binds `127.0.0.1` unless +`BIND_ADDRESS` says otherwise, so a deployment that relied on the old +behaviour becomes unreachable from other hosts until it sets one. In a +container that means `BIND_ADDRESS=0.0.0.0`; without it the container +also goes `unhealthy`, since its health check reaches the app over +`localhost`, which resolves to `::1` before `127.0.0.1`. On a bare +host, set the address the proxy connects to. See +[Bind address](#bind-address). + **Downgrade is unsupported.** Once a newer binary has migrated the files there is no way to move them back. `AutoMigrate` is additive — it adds tables, columns and indexes and never drops or rewrites them — so an diff --git a/internal/config/config.go b/internal/config/config.go index 9c886a1..4b25320 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -33,6 +33,27 @@ const ( // defaultPort is the default HTTP listen port. defaultPort = 8080 + // defaultBindAddress is the interface the plaintext HTTP + // listener claims when BIND_ADDRESS is unset. + // + // Loopback, because the listener speaks cleartext and serves + // both the admin UI and the unauthenticated receiver: a + // wildcard default publishes them on every interface of every + // host that never configured anything, which is the failure + // this default exists to prevent. Reaching webhooker from off + // the host is then a deliberate act — a reverse proxy in front + // of it, or an explicit BIND_ADDRESS. + // + // A container needs BIND_ADDRESS=0.0.0.0 set explicitly: a + // loopback-bound process is unreachable from outside its + // network namespace even with -p. That is deliberate. The + // container fails its healthcheck immediately and visibly, + // where the wildcard default fails silently in the direction of + // exposure. There is no container auto-detection here, because + // a heuristic that guesses wrong opens the cleartext port + // exactly where nobody is looking. + defaultBindAddress = "127.0.0.1" + // defaultRetentionSweepInterval is how often the retention // reaper deletes events older than each webhook's RetentionDays. defaultRetentionSweepInterval = time.Hour @@ -75,6 +96,10 @@ var ErrInvalidPort = errors.New("invalid port") // nor a bare IP address. var ErrInvalidCIDR = errors.New("invalid CIDR") +// ErrInvalidBindAddress is returned when BIND_ADDRESS is set to +// something that is not an IP address literal. +var ErrInvalidBindAddress = errors.New("invalid bind address") + // ErrIncompleteMetricsAuth is returned when exactly one of // METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither // fallback is acceptable: serving /metrics on the username alone @@ -105,6 +130,13 @@ type Config struct { Port int SentryDSN string + // BindAddress is the IP address the plaintext HTTP listener + // binds, as an address literal. It defaults to + // defaultBindAddress and is never empty: an empty string would + // mean the wildcard to net.Listen, which is the opposite of the + // default this ships. + BindAddress string + // RetentionSweepInterval is how often the retention reaper runs. // Always positive: it becomes a time.NewTicker period. RetentionSweepInterval time.Duration @@ -387,6 +419,42 @@ func envPrefixList(key string) ([]netip.Prefix, error) { return prefixes, nil } +// envBindAddress returns the value of the named environment variable +// parsed as an IP address literal. An unset (or empty, or +// whitespace-only) value yields defaultValue. +// +// Only literals are accepted: no hostname is resolved, so `localhost` +// is an error rather than a DNS lookup at startup whose answer could +// be either loopback family, could change under the process, and +// could return several addresses of which only one would be bound. A +// value with a port in it (`127.0.0.1:8080`) is likewise an error — +// the port is PORT's business, and silently accepting it would bind +// something other than what was asked for. +// +// A set value that is not a literal is a hard error naming the key +// and the bad value, so startup fails loudly rather than falling back +// to a default the operator plainly did not want. A literal that is +// not an address of this host parses here and fails at listen time +// instead, which ends the process non-zero. +func envBindAddress(key, defaultValue string) (string, error) { + v := strings.TrimSpace(os.Getenv(key)) + if v == "" { + return defaultValue, nil + } + + addr, err := netip.ParseAddr(v) + if err != nil { + return "", fmt.Errorf( + "%w: %s: %q must be an IP address literal such as "+ + "127.0.0.1, 0.0.0.0 or ::, not a hostname and not "+ + "host:port: %w", + ErrInvalidBindAddress, key, v, err, + ) + } + + return addr.String(), nil +} + // resolveMetricsAuth reads the /metrics basic-auth credentials and // rejects a half-set pair, naming both variables either way. The // error carries neither value: the password is a secret. @@ -431,6 +499,27 @@ func resolveEnvironment() (string, error) { return environment, nil } +// resolveListener reads the two variables that describe the HTTP +// listener: which port it claims and which address it claims it on. +// They are read together because neither is meaningful alone, and +// because a validation failure in either has to abort startup before +// anything binds. +func resolveListener() (int, string, error) { + port, err := envPort("PORT", defaultPort) + if err != nil { + return 0, "", err + } + + bindAddress, err := envBindAddress( + "BIND_ADDRESS", defaultBindAddress, + ) + if err != nil { + return 0, "", err + } + + return port, bindAddress, nil +} + // loadFromEnv builds a Config from the environment. Every value that // needs parsing fails loudly when it is set but unparseable: the // documented defaults apply only to variables that are unset (or @@ -442,7 +531,7 @@ func loadFromEnv() (*Config, error) { return nil, err } - port, err := envPort("PORT", defaultPort) + port, bindAddress, err := resolveListener() if err != nil { return nil, err } @@ -506,6 +595,7 @@ func loadFromEnv() (*Config, error) { MetricsUsername: metricsUsername, MetricsPassword: metricsPassword, Port: port, + BindAddress: bindAddress, SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, SessionIdleTimeout: sessionIdleTimeout, @@ -625,6 +715,11 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { log.Info("Configuration loaded", "environment", s.Environment, "port", s.Port, + // Logged because which interfaces the cleartext listener + // answers on is not otherwise observable from inside a + // container, and it decides whether anything but the local + // host can reach the admin UI. + "bindAddress", s.BindAddress, "debug", s.Debug, "maintenanceMode", s.MaintenanceMode, "dataDir", s.DataDir, diff --git a/internal/config/env_test.go b/internal/config/env_test.go index a127abe..bc67167 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -21,6 +21,22 @@ const ( envKeyPort = "PORT" envKeyDebug = "DEBUG" envKeyMaintenanceMode = "MAINTENANCE_MODE" + envKeyBindAddress = "BIND_ADDRESS" +) + +// Sample BIND_ADDRESS values used by the tables below. +const ( + // bindAddressDefault is the shipped default. It is asserted + // against the package's own constant in + // TestNewUsesDefaultsWhenUnset, so the two cannot drift. + bindAddressDefault = "127.0.0.1" + + // bindAddressWildcard is the value a container deployment sets. + bindAddressWildcard = "0.0.0.0" + + // bindAddressSample is an arbitrary specific address, standing + // for "one interface of several". + bindAddressSample = "10.1.2.3" ) // envBoolCase is one row of the envBool table. @@ -291,6 +307,160 @@ func TestEnvPort(t *testing.T) { } } +// TestEnvBindAddress covers BIND_ADDRESS parsing. +// +// Only IP address literals are accepted. Every rejection below is a +// value an operator plausibly writes — a hostname, a host:port, a +// CIDR block — and each has to abort startup rather than fall back to +// the default, because falling back would bind an address other than +// the one asked for and, in the wildcard-default case this setting +// exists to end, publish cleartext on every interface. +func TestEnvBindAddress(t *testing.T) { + for _, tt := range envBindAddressCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvBindAddressForTest( + testEnvKey, bindAddressDefault, + ) + + if tt.expectError { + require.Error(t, err) + require.ErrorIs(t, err, config.ErrInvalidBindAddress) + assert.Contains(t, err.Error(), testEnvKey) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +// envBindAddressCase is one row of the envBindAddress table. +type envBindAddressCase struct { + name string + set bool + value string + expectError bool + expected string +} + +// envBindAddressCases is the envBindAddress table, kept out of the +// test body so the test itself stays readable. +func envBindAddressCases() []envBindAddressCase { + return append( + envBindAddressAcceptedCases(), + envBindAddressRejectedCases()..., + ) +} + +// envBindAddressAcceptedCases are the values that parse: the three +// spellings of "unset" that take the default, and the literals. +func envBindAddressAcceptedCases() []envBindAddressCase { + return []envBindAddressCase{ + { + name: "unset returns the default", + expected: bindAddressDefault, + }, + { + name: "empty returns the default", + set: true, + value: "", + expected: bindAddressDefault, + }, + { + name: "whitespace returns the default", + set: true, + value: " ", + expected: bindAddressDefault, + }, + { + name: "ipv4 wildcard is parsed", + set: true, + value: bindAddressWildcard, + expected: bindAddressWildcard, + }, + { + name: "ipv4 literal is parsed", + set: true, + value: bindAddressSample, + expected: bindAddressSample, + }, + { + name: "surrounding whitespace is trimmed", + set: true, + value: " " + bindAddressSample + " ", + expected: bindAddressSample, + }, + { + name: "ipv6 wildcard is parsed", + set: true, + value: "::", + expected: "::", + }, + { + name: "ipv6 literal is parsed", + set: true, + value: "2001:db8::5", + expected: "2001:db8::5", + }, + } +} + +// envBindAddressRejectedCases are the values that abort startup. +// Each is something an operator plausibly writes, and none may fall +// back to the default: the default is loopback, so a silent fallback +// would bind somewhere other than what was asked for. +func envBindAddressRejectedCases() []envBindAddressCase { + return []envBindAddressCase{ + { + name: "garbage is rejected", + set: true, + value: "not-an-address", + expectError: true, + }, + { + name: "hostname is rejected", + set: true, + value: "localhost", + expectError: true, + }, + { + name: "unresolvable hostname is rejected", + set: true, + value: "no-such-host.invalid", + expectError: true, + }, + { + name: "host and port is rejected", + set: true, + value: bindAddressDefault + ":8080", + expectError: true, + }, + { + name: "bracketed ipv6 is rejected", + set: true, + value: "[::1]", + expectError: true, + }, + { + name: "CIDR block is rejected", + set: true, + value: "10.0.0.0/8", + expectError: true, + }, + } +} + // buildConfig constructs a Config through fx exactly as the // application does, returning the config and any construction error. func buildConfig(t *testing.T) (*config.Config, error) { @@ -312,13 +482,45 @@ func buildConfig(t *testing.T) (*config.Config, error) { } func TestNewRejectsBadEnvValues(t *testing.T) { - tests := []struct { - name string - key string - value string - expectError bool - check func(t *testing.T, cfg *config.Config) - }{ + for _, tt := range badEnvValueCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + t.Setenv(tt.key, tt.value) + + cfg, err := buildConfig(t) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.key) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + require.NotNil(t, cfg) + tt.check(t, cfg) + }) + } +} + +// badEnvValueCase is one row of the config.New table: a variable, the +// value it is set to, and either the assertion that startup fails +// naming both, or a check on the Config that resulted. +type badEnvValueCase struct { + name string + key string + value string + expectError bool + check func(t *testing.T, cfg *config.Config) +} + +// badEnvValueCases is the config.New table, kept out of the test body +// so the test itself stays readable. +func badEnvValueCases() []badEnvValueCase { + return []badEnvValueCase{ { name: "valid PORT is used", key: envKeyPort, @@ -361,29 +563,35 @@ func TestNewRejectsBadEnvValues(t *testing.T) { value: "sometimes", expectError: true, }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Cannot use t.Parallel() here because t.Setenv - // is incompatible with parallel subtests. - t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") - t.Setenv(tt.key, tt.value) - - cfg, err := buildConfig(t) - - if tt.expectError { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.key) - assert.Contains(t, err.Error(), tt.value) - - return - } - - require.NoError(t, err) - require.NotNil(t, cfg) - tt.check(t, cfg) - }) + { + name: "valid BIND_ADDRESS is used", + key: envKeyBindAddress, + value: bindAddressWildcard, + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.Equal( + t, bindAddressWildcard, cfg.BindAddress, + ) + }, + }, + { + name: "unparseable BIND_ADDRESS aborts startup", + key: envKeyBindAddress, + value: "not-an-address", + expectError: true, + }, + { + name: "hostname BIND_ADDRESS aborts startup", + key: envKeyBindAddress, + value: "localhost", + expectError: true, + }, + { + name: "BIND_ADDRESS with a port aborts startup", + key: envKeyBindAddress, + value: bindAddressDefault + ":8080", + expectError: true, + }, } } @@ -395,6 +603,7 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) { for _, key := range []string{ envKeyPort, envKeyDebug, envKeyMaintenanceMode, + envKeyBindAddress, } { require.NoError(t, os.Unsetenv(key)) } @@ -406,4 +615,15 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) { assert.Equal(t, 8080, cfg.Port) assert.False(t, cfg.Debug) assert.False(t, cfg.MaintenanceMode) + + // Loopback, not the wildcard: the default must not publish the + // cleartext admin UI and the unauthenticated receiver on every + // interface of a host that configured nothing. The value is read + // from the package rather than repeated, so the README's + // documented default and the compiled-in one are pinned to the + // same constant. + assert.Equal( + t, config.DefaultBindAddressForTest, cfg.BindAddress, + ) + assert.Equal(t, bindAddressDefault, cfg.BindAddress) } diff --git a/internal/config/export_test.go b/internal/config/export_test.go index 710e680..f952bd2 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -50,3 +50,13 @@ func EnvPositiveIntForTest(key string, defaultValue int) (int, error) { func EnvPortForTest(key string, defaultValue int) (int, error) { return envPort(key, defaultValue) } + +// EnvBindAddressForTest exposes envBindAddress. +func EnvBindAddressForTest(key, defaultValue string) (string, error) { + return envBindAddress(key, defaultValue) +} + +// DefaultBindAddressForTest exposes the compiled-in BIND_ADDRESS +// default, so a test pins the documented value rather than repeating +// a literal that could drift from it. +const DefaultBindAddressForTest = defaultBindAddress diff --git a/internal/server/bind_address_test.go b/internal/server/bind_address_test.go new file mode 100644 index 0000000..9329180 --- /dev/null +++ b/internal/server/bind_address_test.go @@ -0,0 +1,336 @@ +package server_test + +import ( + "context" + "net" + "net/http" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx" + "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/globals" + "sneak.berlin/go/webhooker/internal/server" +) + +const ( + // loopbackV4 is the shipped BIND_ADDRESS default. + loopbackV4 = "127.0.0.1" + + // wildcardV4 is the value a container deployment must set, + // where a loopback-bound process is unreachable from outside + // its network namespace even with a published port. + wildcardV4 = "0.0.0.0" + + // unavailableAddr is a TEST-NET-1 address (RFC 5737). It is a + // well-formed literal that no host is assigned, so binding it + // fails with EADDRNOTAVAIL rather than succeeding somewhere + // unexpected. + unavailableAddr = "192.0.2.1" + + // listenReadyTimeout bounds the wait for the listener to accept + // connections. The bind itself is immediate; this only covers + // goroutine scheduling. + listenReadyTimeout = 3 * time.Second + + // listenPollInterval is how often the readiness wait retries. + listenPollInterval = 10 * time.Millisecond + + // dialTimeout bounds a single connection attempt in these + // tests. Everything dialled here is on this host, so a dial + // that is not answered immediately is a failure, not slowness. + dialTimeout = time.Second +) + +// freePort returns a TCP port that is free on every local address at +// the moment it returns, by taking one on the wildcard and releasing +// it. The window between release and re-bind is the standard one +// every "pick a free port" helper carries. +func freePort(t *testing.T) int { + t.Helper() + + var listenCfg net.ListenConfig + + l, err := listenCfg.Listen(t.Context(), "tcp", "0.0.0.0:0") + require.NoError(t, err) + + addr, ok := l.Addr().(*net.TCPAddr) + require.True(t, ok, "listener is not TCP") + require.NoError(t, l.Close()) + + return addr.Port +} + +// otherLocalAddr returns a local IPv4 address that is not +// loopbackV4, or skips the test when the host has none. +// +// The bind-address tests need a second address of this host to stand +// in for "another interface": what a wildcard bind claims and a +// loopback bind does not. 127.0.0.2 is that address on Linux, where +// the whole 127.0.0.0/8 is local; elsewhere an interface address is +// used instead. Each candidate is proven bindable before it is +// returned, so a host that offers neither skips rather than fails on +// something that was never about the code under test. +func otherLocalAddr(t *testing.T) string { + t.Helper() + + candidates := []string{"127.0.0.2"} + + ifaceAddrs, err := net.InterfaceAddrs() + require.NoError(t, err) + + for _, a := range ifaceAddrs { + ipNet, ok := a.(*net.IPNet) + if !ok { + continue + } + + ip4 := ipNet.IP.To4() + if ip4 == nil || ip4.String() == loopbackV4 { + continue + } + + candidates = append(candidates, ip4.String()) + } + + var listenCfg net.ListenConfig + + for _, candidate := range candidates { + l, listenErr := listenCfg.Listen( + t.Context(), "tcp", net.JoinHostPort(candidate, "0"), + ) + if listenErr != nil { + continue + } + + require.NoError(t, l.Close()) + + return candidate + } + + t.Skip("host has no second local IPv4 address to bind") + + return "" +} + +// startBoundServer starts the wired app with the given bind address +// on a free port and returns that port. The app is stopped on +// cleanup. +func startBoundServer(t *testing.T, bindAddress string) int { + t.Helper() + + port := freePort(t) + + env := newTestEnv(t) + env.cfg.BindAddress = bindAddress + env.cfg.Port = port + + app := fx.New( + fx.NopLogger, + fx.Supply(env.log, env.cfg, env.mw, env.hnd), + fx.Provide(globals.New, server.New), + fx.Invoke(func(*server.Server) {}), + ) + + startCtx, cancelStart := context.WithTimeout( + context.Background(), lifecycleTimeout, + ) + defer cancelStart() + + require.NoError(t, app.Start(startCtx)) + + t.Cleanup(func() { + stopCtx, cancelStop := context.WithTimeout( + context.Background(), lifecycleTimeout, + ) + defer cancelStop() + + require.NoError(t, app.Stop(stopCtx)) + }) + + return port +} + +// dialable reports whether a TCP connection to addr succeeds. +func dialable(ctx context.Context, addr string) bool { + dialer := net.Dialer{Timeout: dialTimeout} + + conn, err := dialer.DialContext(ctx, "tcp", addr) + if err != nil { + return false + } + + _ = conn.Close() + + return true +} + +// requireDialable waits for addr to accept connections, failing the +// test if it never does. +func requireDialable(t *testing.T, addr string) { + t.Helper() + + deadline := time.Now().Add(listenReadyTimeout) + for time.Now().Before(deadline) { + if dialable(t.Context(), addr) { + return + } + + time.Sleep(listenPollInterval) + } + + t.Fatalf("nothing accepted connections on %s", addr) +} + +// TestListenAddr pins how BindAddress and Port are rendered into the +// listen address. +// +// The defect this covers was a bare fmt.Sprintf(":%d", port), which +// binds every interface with no way to say otherwise. The IPv6 rows +// are here because an unbracketed IPv6 host would produce an address +// net.Listen rejects, turning a valid configuration into a startup +// failure. +func TestListenAddr(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + bindAddress string + port int + expected string + }{ + { + name: "loopback default", + bindAddress: loopbackV4, + port: 8080, + expected: "127.0.0.1:8080", + }, + { + name: "ipv4 wildcard", + bindAddress: wildcardV4, + port: 8080, + expected: "0.0.0.0:8080", + }, + { + name: "ipv6 wildcard is bracketed", + bindAddress: "::", + port: 8080, + expected: "[::]:8080", + }, + { + name: "ipv6 literal is bracketed", + bindAddress: "2001:db8::5", + port: 9001, + expected: "[2001:db8::5]:9001", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.expected, server.ListenAddrForTest( + &config.Config{ + BindAddress: tt.bindAddress, + Port: tt.port, + }, + )) + }) + } +} + +// TestBindAddress_LoopbackIsNotOnOtherAddresses proves the fix end to +// end: with BIND_ADDRESS at its loopback default, the cleartext +// listener answers on loopback and has not claimed any other address +// of this host. +// +// The second address is proven free by binding it on the same port +// while the server runs. That is the assertion that fails against the +// old wildcard bind — a wildcard listener owns the port on every +// address, so this bind would return EADDRINUSE. Dialling from +// another machine is what the operator cares about, and this is the +// in-process form of it: the socket the remote host would connect to +// does not exist. +func TestBindAddress_LoopbackIsNotOnOtherAddresses(t *testing.T) { + t.Parallel() + + other := otherLocalAddr(t) + port := startBoundServer(t, loopbackV4) + + // Positive control: the service really is up and serving. + requireDialable(t, net.JoinHostPort(loopbackV4, strconv.Itoa(port))) + + var listenCfg net.ListenConfig + + l, err := listenCfg.Listen( + t.Context(), "tcp", + net.JoinHostPort(other, strconv.Itoa(port)), + ) + require.NoError( + t, err, + "port %d on %s is taken while bound to %s: the listener "+ + "claimed more than its configured address", + port, other, loopbackV4, + ) + + require.NoError(t, l.Close()) +} + +// TestBindAddress_WildcardReachesOtherAddresses is the counterpart: +// the value a container deployment sets does reach the addresses the +// default withholds. Without this, a loopback-only bind would pass +// the test above by never listening at all. +func TestBindAddress_WildcardReachesOtherAddresses(t *testing.T) { + t.Parallel() + + other := otherLocalAddr(t) + port := startBoundServer(t, wildcardV4) + + requireDialable(t, net.JoinHostPort(other, strconv.Itoa(port))) +} + +// TestBindAddress_ServesRequestsOnConfiguredAddress proves the bound +// listener serves the application rather than merely accepting TCP, +// so a bind address that is honoured cannot be mistaken for one that +// is honoured and broken. +func TestBindAddress_ServesRequestsOnConfiguredAddress(t *testing.T) { + t.Parallel() + + port := startBoundServer(t, loopbackV4) + addr := net.JoinHostPort(loopbackV4, strconv.Itoa(port)) + requireDialable(t, addr) + + req, err := http.NewRequestWithContext( + t.Context(), http.MethodGet, + "http://"+addr+"/.well-known/healthcheck", nil, + ) + require.NoError(t, err) + + client := &http.Client{Timeout: dialTimeout} + + resp, err := client.Do(req) + require.NoError(t, err) + + defer func() { _ = resp.Body.Close() }() + + assert.Equal(t, http.StatusOK, resp.StatusCode) +} + +// TestBindAddress_UnavailableAddressShutsDownTheApp covers the half +// of the fail-loud rule that configuration parsing cannot reach. A +// syntactically valid address that is not assigned to this host +// parses fine and fails at bind time, after fx has already reported +// RUNNING. It must end the process non-zero rather than leave it +// alive with nothing listening. +func TestBindAddress_UnavailableAddressShutsDownTheApp(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + env.cfg.BindAddress = unavailableAddr + env.cfg.Port = freePort(t) + + requireListenFailureExit(t, env) +} diff --git a/internal/server/export_test.go b/internal/server/export_test.go index d5b05be..02c0b77 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -55,6 +55,16 @@ func NewRouterForTest( return s.router } +// ListenAddrForTest exposes the address the HTTP listener binds for +// a given Config, so the rendering of host and port — IPv6 +// bracketing above all — can be pinned without standing up a +// listener. +func ListenAddrForTest(cfg *config.Config) string { + s := &Server{params: ServerParams{Config: cfg}} + + return s.listenAddr() +} + // ProbePattern is the route NewRouterWithProbeForTest adds to the // production route tree. const ProbePattern = "/probe" diff --git a/internal/server/http.go b/internal/server/http.go index 5413dd9..040197f 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -2,8 +2,9 @@ package server import ( "errors" - "fmt" + "net" "net/http" + "strconv" "time" ) @@ -24,21 +25,47 @@ const ( httpMaxHeaderBytes = 1 << 20 ) -func (s *Server) serveUntilShutdown() { - listenAddr := fmt.Sprintf(":%d", s.params.Config.Port) - s.httpServer = &http.Server{ - Addr: listenAddr, +// listenAddr renders the address the HTTP listener binds. +// +// The host half is always present: an empty host would be the +// wildcard, and the whole point of BIND_ADDRESS is that binding every +// interface is a choice the operator makes rather than one the +// process makes for them. Config guarantees a literal, so +// JoinHostPort's bracketing is enough to make IPv6 well formed. +func (s *Server) listenAddr() string { + return net.JoinHostPort( + s.params.Config.BindAddress, + strconv.Itoa(s.params.Config.Port), + ) +} + +// newHTTPServer builds the HTTP server for this Server's +// configuration. +// +// It is called from New, on the constructing goroutine, rather than +// from the serving goroutine that used to assign s.httpServer +// directly. Two goroutines reach that field — the serving goroutine +// and the fx stop hook, which calls Shutdown on it — with nothing +// ordering them. Constructing it during New puts the write before +// every hook fx will later run, which both removes the race and rules +// out the nil dereference a stop that arrived before the serving +// goroutine had run would have caused. +func (s *Server) newHTTPServer() *http.Server { + return &http.Server{ + Addr: s.listenAddr(), ReadTimeout: httpReadTimeout, WriteTimeout: httpWriteTimeout, MaxHeaderBytes: httpMaxHeaderBytes, Handler: s, } +} +func (s *Server) serveUntilShutdown() { // add routes // this does any necessary setup in each handler s.SetupRoutes() - s.log.Info("http begin listen", "listenaddr", listenAddr) + s.log.Info("http begin listen", "listenaddr", s.httpServer.Addr) err := s.httpServer.ListenAndServe() if err != nil && !errors.Is(err, http.ErrServerClosed) { diff --git a/internal/server/listen_failure_test.go b/internal/server/listen_failure_test.go index 6b71185..3508626 100644 --- a/internal/server/listen_failure_test.go +++ b/internal/server/listen_failure_test.go @@ -36,9 +36,9 @@ const lifecycleTimeout = 15 * time.Second // // The port is occupied by a listener this test holds open, on a // kernel-chosen port, so the failure is the real EADDRINUSE the -// operator hits when a second instance starts. Loopback is enough to -// collide with the server's wildcard bind: a listening socket on a -// specific address blocks the wildcard from claiming the same port. +// operator hits when a second instance starts. The server is pointed +// at the same loopback address, so the collision is a direct one on +// the exact address it asks the kernel for. func TestListenFailure_ShutsDownTheApp(t *testing.T) { t.Parallel() @@ -55,11 +55,27 @@ func TestListenFailure_ShutsDownTheApp(t *testing.T) { require.True(t, ok, "listener is not TCP") // The collaborators come from the wired graph rather than stubs, - // so the Server under test is the one that ships. Only the port - // is test-specific. + // so the Server under test is the one that ships. Only the + // listen address is test-specific. env := newTestEnv(t) + env.cfg.BindAddress = loopbackV4 env.cfg.Port = addr.Port + requireListenFailureExit(t, env) +} + +// requireListenFailureExit starts the wired app over env and asserts +// that it gives up on its own with the listen-failure status, then +// completes its stop sequence. +// +// Two different listen failures share it — a port already in use and +// an address that is not on this host — because what has to hold for +// both is the same: the failure is discovered after fx has already +// reported RUNNING, so the only thing that can turn it into a visible +// exit is the shutdown path under test. +func requireListenFailureExit(t *testing.T, env *testEnv) { + t.Helper() + app := fx.New( fx.NopLogger, fx.Supply(env.log, env.cfg, env.mw, env.hnd), diff --git a/internal/server/server.go b/internal/server/server.go index 721df5b..d6a93d5 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -107,6 +107,7 @@ func New(lc fx.Lifecycle, params ServerParams) (*Server, error) { s.mw = params.Middleware s.h = params.Handlers s.log = params.Logger.Get() + s.httpServer = s.newHTTPServer() lc.Append(fx.Hook{ OnStart: func(_ context.Context) error {