diff --git a/Dockerfile b/Dockerfile index 94b5b57..b4f7273 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,6 +2,7 @@ # The linter is invoked directly rather than through `make lint`: that # target shells out to `docker build -f Dockerfile.lint`, and there is # no docker daemon inside a docker build. +# script/cibuild and script/docker name this stage in --no-cache-filter. # golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-10 FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint @@ -15,6 +16,7 @@ RUN make fmt-check RUN golangci-lint run --config .golangci.yml ./... # Build stage +# script/cibuild and script/docker name this stage in --no-cache-filter. # golang 1.25-alpine, 2026-02-28 FROM golang@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS builder @@ -41,15 +43,33 @@ FROM alpine@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4 RUN apk add --no-cache ca-certificates tzdata -WORKDIR /app +COPY --from=builder /src/bin/dnswatcher /usr/local/bin/dnswatcher -COPY --from=builder /src/bin/dnswatcher /app/dnswatcher - -# Create data directory -RUN mkdir -p /var/lib/dnswatcher +# Run as an unprivileged user that owns the data directory. A fresh named +# volume inherits this ownership; a bind-mounted host directory must be +# owned by uid 10001 (see "Running under upaas" in README.md), or startup +# fails. +RUN addgroup -S -g 10001 dnswatcher \ + && adduser -S -G dnswatcher -u 10001 dnswatcher \ + && mkdir -p /var/lib/dnswatcher \ + && chown dnswatcher:dnswatcher /var/lib/dnswatcher ENV DNSWATCHER_DATA_DIR=/var/lib/dnswatcher +# Config loading also reads a `.env` file and a file named `dnswatcher` +# (any config extension, or none) from the working directory. `/` holds +# neither, so every setting comes from the environment. Do not make the +# data directory, or the binary's directory, the working directory. +WORKDIR / + +USER dnswatcher + EXPOSE 8080 -ENTRYPOINT ["/app/dnswatcher"] +# busybox wget (already in alpine) probes the health endpoint every 10 +# seconds, so the container is healthy well before upaas reads its health +# 60 seconds after a deploy and fails the deploy unless it is healthy. +HEALTHCHECK --interval=10s --timeout=5s --start-period=10s --retries=3 \ + CMD wget -q -O /dev/null "http://127.0.0.1:${PORT:-8080}/.well-known/healthcheck" || exit 1 + +ENTRYPOINT ["/usr/local/bin/dnswatcher"] diff --git a/README.md b/README.md index 9e854ff..8e51883 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,10 @@ rejected. record types: A, AAAA, CNAME, MX, TXT, SRV, CAA, NS. - Stores results **per nameserver**. The state for a hostname is not a merged view — it is a map from nameserver to record set. +- DNS names inside record values (CNAME, MX, SRV and NS targets) are + stored in lower case, because names are case-insensitive and + nameservers may answer in any letter case. TXT and CAA values keep + their letter case; they are not lower-cased. - Any observable change in any nameserver's response triggers a notification. This includes: - **Record change**: A nameserver returns different records than it @@ -182,6 +186,67 @@ dnswatcher exposes a lightweight HTTP API for operational visibility: | `GET /api/v1/status` | Current monitoring state | | `GET /metrics` | Prometheus metrics (optional) | +#### Server timeouts + +The HTTP server sets all four socket-level timeouts. These are compile-time +constants in `internal/server/server.go`, not configurable via environment +variables. + +| Timeout | Value | Purpose | +|---------------------|-------|-----------------------------------------------| +| `ReadHeaderTimeout` | 10s | Bounds the request header read (slowloris) | +| `ReadTimeout` | 15s | Bounds the whole request read, headers + body | +| `WriteTimeout` | 75s | Bounds handler execution plus response flush | +| `IdleTimeout` | 120s | Reaps idle keep-alive connections | + +These are distinct from the 60s per-request handler budget applied by +`chimw.Timeout` in `internal/server/routes.go`, which cancels the request +context but does not touch the socket. `WriteTimeout` is deliberately +larger than that budget: the write deadline is armed once request headers +are read, so a smaller value would sever the connection before a handler +using its full budget could respond. `IdleTimeout` exceeds common +Prometheus scrape intervals so the scraper reuses its connection. + +### Security Headers + +Every response — the dashboard, the static assets under `/s/...`, the +healthchecks, the JSON API, and `/metrics` — carries the following +headers, set by a global middleware: + +| Header | Value | +|-----------------------------|---------------------------------------| +| `Strict-Transport-Security` | `max-age=31536000; includeSubDomains` | +| `Content-Security-Policy` | see below | +| `X-Frame-Options` | `DENY` | +| `X-Content-Type-Options` | `nosniff` | +| `Referrer-Policy` | `no-referrer` | +| `Permissions-Policy` | all unused browser features denied | + +The content security policy is: + +``` +default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self'; +font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none'; +form-action 'none'; frame-ancestors 'none' +``` + +The dashboard ships no JavaScript (the 30-second refresh is a +``), no inline styles, no inline event +handlers, and no images; its only subresource is the embedded stylesheet +at `/s/css/tailwind.min.css`, which `style-src 'self'` permits. The +policy therefore needs neither `unsafe-inline` nor `unsafe-eval`. +`frame-ancestors 'none'` is the primary anti-framing control, with +`X-Frame-Options: DENY` retained as the legacy fallback. + +HSTS is emitted unconditionally, including over plain HTTP. dnswatcher is +expected to run behind a TLS-terminating reverse proxy, and the browser +must still be told to enforce HTTPS end to end, so the header is never +gated on whether the request itself arrived over TLS. + +`Referrer-Policy: no-referrer` is stricter than the +`strict-origin-when-cross-origin` baseline: the dashboard has no +cross-origin navigation needs, and its URL may name internal hosts. + --- ## Architecture @@ -194,7 +259,8 @@ internal/ globals/globals.go Build-time variables (version) logger/logger.go slog structured logging (TTY detection) healthcheck/healthcheck.go Health check service - middleware/middleware.go HTTP middleware (logging, CORS, metrics auth) + middleware/middleware.go HTTP middleware (logging, CORS, security + headers, metrics auth) handlers/handlers.go HTTP request handlers server/ server.go HTTP server lifecycle @@ -403,9 +469,13 @@ them. We provide: - `script/fmt` — format all code (gofmt -s, goimports) - `script/fmt-check` — check formatting (read-only) - `script/check` — run test, lint, and fmt-check -- `script/docker` — build the Docker image tagged via - `script/projectname` -- `script/cibuild` — CI entrypoint: plain `docker build .` +- `script/docker` — build the Docker image tagged via `script/projectname`, with + `--no-cache-filter=lint,builder` so the lint stage and the builder stage, + which runs the tests, run on every invocation +- `script/cibuild` — CI entrypoint: `docker build` with + `--no-cache-filter=lint,builder`, so the lint stage and the builder stage, + which runs the tests, run on every invocation, because a cached build lints + nothing and queries no DNS - `script/precommit` — run by the git pre-commit hook; `go mod tidy` guard, then `script/check` - `script/install-precommit` — install the git pre-commit hook @@ -446,11 +516,57 @@ docker run -d \ --- +## Running under upaas + +[upaas](https://git.eeqj.de/sneak/upaas) builds the image from this +repository's `Dockerfile` and runs it. The app needs: + +- **Branch:** `prod`. `prod` is cut from `main`, and merging a `main` to + `prod` pull request is a deploy. +- **Volume:** one host directory mounted at `/var/lib/dnswatcher`, where + the state file lives. upaas bind-mounts the host path it is given and + does not create it. The container runs as uid 10001 and does not start + unless it can write there. Create the directory before the first + deploy: + + ```sh + mkdir -p /path/to/data + chown 10001:10001 /path/to/data + chmod 700 /path/to/data + ``` + +- **Network and port:** the dashboard is unauthenticated and shows every + watched name and recent alert, and upaas publishes every mapped port on + all interfaces of the host + ([upaas issue 113](https://git.eeqj.de/sneak/upaas/issues/113)). Add a + port mapping to container port `8080` only if the dashboard should be + public. Otherwise add none: set the app's Docker network in upaas to + your reverse proxy's Docker network, and the proxy reaches the app at + `upaas-` followed by the app name, port `8080`. +- **Required environment:** `DNSWATCHER_TARGETS`, a comma-separated list + of the domains and hostnames to watch. dnswatcher refuses to start + without it. +- **Recommended environment:** at least one notification endpoint + (`DNSWATCHER_SLACK_WEBHOOK`, `DNSWATCHER_MATTERMOST_WEBHOOK`, + `DNSWATCHER_NTFY_TOPIC`); without one, changes show only on the + dashboard. `DNSWATCHER_METRICS_USERNAME` and + `DNSWATCHER_METRICS_PASSWORD` serve `/metrics` behind basic auth. +- **Leave unset:** `DNSWATCHER_DATA_DIR`, which the image sets to + `/var/lib/dnswatcher`, and `PORT`, which defaults to `8080`. Every + setting comes from the environment; the image holds no config file. +- **Health check:** the image's own, which requests + `/.well-known/healthcheck` every 10 seconds. upaas reads the + container's health 60 seconds after a deploy and marks the deploy + failed unless it is `healthy`. + +--- + ## Monitoring Lifecycle -1. **Startup**: Load state from disk. If no state file exists, start - with empty state (first check will establish baseline without - triggering change notifications). +1. **Startup**: Check that the data directory can be written, and exit + with an error naming it if not. Load state from disk. If no state + file exists, start with empty state (first check will establish + baseline without triggering change notifications). 2. **Initial check**: Immediately perform all DNS, port, and TLS checks on startup. 3. **Periodic checks** (DNS always runs first): diff --git a/TODO.md b/TODO.md index 91ea4e5..36a1c9a 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,22 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-28: DNS names in record values (CNAME, MX, SRV and NS targets) are + lower-cased, so nameservers that answer in different letter case no longer + count as inconsistent or as a record change (closes #157). +- 2026-09-28: `script/cibuild` and `script/docker` now pass + `--no-cache-filter=lint,builder` so lint and tests run every build (closes + #115). +- 2026-09-28: the server timeout test now drives `Run` and checks the + `http.Server` it serves carries the timeouts; corrected the `ReadTimeout` + note in that test (closes #120). +- 2026-09-28: upaas deploy readiness — runtime image runs as unprivileged + `dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is + not writable, README "Running under upaas" (closes #147). +- 2026-09-21: added behavioural tests for `internal/globals`, + `internal/healthcheck`, and `internal/logger` (closes #110). +- 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync` + `// indirect` line so `script/bootstrap` leaves a clean tree (#132) - 2026-08-10: comment-only corrections to `script/bootstrap`, `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in `script/bootstrap` was justified by a claim that `script/fmt-check` @@ -101,6 +117,24 @@ Rationale, Design, TODO, License, Author) if any are still missing. so shutdown cannot be extended indefinitely; an `OnStop` context that is already expired on entry with nothing outstanding drains quietly rather than warning about deliveries that were never abandoned +- 2026-08-09: `http.Server` now sets all four socket-level timeouts + (`ReadTimeout` 15s, `ReadHeaderTimeout` 10s, `WriteTimeout` 75s, + `IdleTimeout` 120s) as named constants in `internal/server/server.go`, + closing the slowloris / unreaped-keep-alive exposure required by + `REPO_POLICIES.md` before 1.0; `WriteTimeout` is deliberately greater + than the 60s `chimw.Timeout` handler budget so that budget stays + reachable, and tests in `internal/server` pin both the non-zero + values and that relationship (#99) +- 2026-08-09: security response headers middleware + (`SecurityHeaders()` in `internal/middleware/middleware.go`) + registered globally in `internal/server/routes.go`, so HSTS, CSP, + `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, and + `Permissions-Policy` are set on every response including `/s/...` and + `/metrics`; the CSP needs no `unsafe-inline`/`unsafe-eval` because the + dashboard ships no JavaScript and no inline styles; HSTS is emitted + unconditionally per policy (TLS-terminating proxy in front). Remaining + 1.0 hardening items — `http.Server` timeouts, request body limits, + rate limiting, CORS scoping — are tracked separately - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the org-standard v2-schema config used across the org's repos diff --git a/go.mod b/go.mod index 53638c9..0078dcf 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,6 @@ require ( go.yaml.in/yaml/v2 v2.4.2 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/mod v0.32.0 // indirect - golang.org/x/sync v0.19.0 // indirect golang.org/x/sys v0.41.0 // indirect golang.org/x/text v0.34.0 // indirect golang.org/x/tools v0.41.0 // indirect diff --git a/internal/globals/globals_test.go b/internal/globals/globals_test.go new file mode 100644 index 0000000..f44174e --- /dev/null +++ b/internal/globals/globals_test.go @@ -0,0 +1,50 @@ +package globals_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/globals" +) + +// TestGlobals exercises the package-level version and appname +// variables through their setters and read-back via New. These are +// shared package state, so the test mutates a global and must run +// sequentially; it cannot use t.Parallel(). +// +//nolint:paralleltest // mutates shared package-level globals, must run sequentially +func TestGlobals(t *testing.T) { + versions := []string{"v1.2.3", "dev", "", "v1.2.3-4-gabcdef"} + for _, want := range versions { + globals.SetVersion(want) + + g, err := globals.New(nil) + require.NoError(t, err) + assert.Equal(t, want, g.Version, + "New must surface the version set by SetVersion") + } + + names := []string{"dnswatcher", "other", ""} + for _, want := range names { + globals.SetAppname(want) + + g, err := globals.New(nil) + require.NoError(t, err) + assert.Equal(t, want, g.Appname, + "New must surface the appname set by SetAppname") + } + + // New returns a snapshot: a later SetVersion must not mutate a + // Globals handed out earlier. + globals.SetVersion("first") + + g, err := globals.New(nil) + require.NoError(t, err) + + globals.SetVersion("second") + assert.Equal(t, "first", g.Version, + "a Globals returned by New must not change when the "+ + "package variable is set again") +} diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go new file mode 100644 index 0000000..b87c3d1 --- /dev/null +++ b/internal/healthcheck/healthcheck_test.go @@ -0,0 +1,134 @@ +package healthcheck_test + +import ( + "context" + "encoding/json" + "testing" + "time" + + "go.uber.org/fx" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/healthcheck" + "sneak.berlin/go/dnswatcher/internal/logger" +) + +// recordingLifecycle is a minimal fx.Lifecycle that records the hooks +// appended to it, so healthcheck.New can be exercised through its real +// constructor without standing up a whole fx application. +type recordingLifecycle struct { + hooks []fx.Hook +} + +func (l *recordingLifecycle) Append(hook fx.Hook) { + l.hooks = append(l.hooks, hook) +} + +// newHealthcheck builds a Healthcheck through the real constructor and +// runs the registered OnStart hook so StartupTime is set the same way +// the fx lifecycle would set it. +func newHealthcheck( + t *testing.T, + maintenance bool, + version string, +) *healthcheck.Healthcheck { + t.Helper() + + g := &globals.Globals{Appname: "dnswatcher", Version: version} + + log, err := logger.New(nil, logger.Params{Globals: g}) + require.NoError(t, err) + + lifecycle := &recordingLifecycle{} + + hc, err := healthcheck.New(lifecycle, healthcheck.Params{ + Globals: g, + Config: &config.Config{MaintenanceMode: maintenance}, + Logger: log, + }) + require.NoError(t, err) + + require.Len(t, lifecycle.hooks, 1, + "New must register exactly one lifecycle hook") + require.NotNil(t, lifecycle.hooks[0].OnStart) + require.NoError(t, lifecycle.hooks[0].OnStart(context.Background())) + + return hc +} + +func TestCheckStatusAndPayloadShape(t *testing.T) { + t.Parallel() + + hc := newHealthcheck(t, false, "v9.9.9") + resp := hc.Check() + + assert.Equal(t, "ok", resp.Status) + + // The JSON shape and field names are part of the contract for the + // /health and /.well-known/healthcheck routes, so assert on the + // exact set of keys the response marshals to. + raw, err := json.Marshal(resp) + require.NoError(t, err) + + var fields map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &fields)) + + wantKeys := []string{ + "status", + "now", + "uptimeSeconds", + "uptimeHuman", + "version", + "appname", + "maintenanceMode", + } + assert.Len(t, fields, len(wantKeys), + "response must marshal to exactly the documented fields") + + for _, key := range wantKeys { + assert.Contains(t, fields, key, "missing JSON field %q", key) + } + + // The Now field is documented as RFC3339Nano; a change to the + // format constant should turn this red. + _, err = time.Parse(time.RFC3339Nano, resp.Now) + assert.NoError(t, err, "Now must be RFC3339Nano") +} + +func TestCheckMaintenanceModeReflectsConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + maintenance bool + }{ + {"maintenance off", false}, + {"maintenance on", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + hc := newHealthcheck(t, tt.maintenance, "test") + resp := hc.Check() + assert.Equal(t, tt.maintenance, resp.Maintenance, + "maintenanceMode must mirror Config.MaintenanceMode") + }) + } +} + +func TestCheckSurfacesVersionAndAppname(t *testing.T) { + t.Parallel() + + hc := newHealthcheck(t, false, "surfaced-version-123") + resp := hc.Check() + + assert.Equal(t, "surfaced-version-123", resp.Version, + "version from globals must appear in the payload") + assert.Equal(t, "dnswatcher", resp.Appname) +} diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go new file mode 100644 index 0000000..80ec8c9 --- /dev/null +++ b/internal/logger/logger_test.go @@ -0,0 +1,66 @@ +package logger_test + +import ( + "context" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/logger" +) + +func newTestLogger(t *testing.T) *logger.Logger { + t.Helper() + + g := &globals.Globals{Appname: "dnswatcher", Version: "test"} + + l, err := logger.New(nil, logger.Params{Globals: g}) + require.NoError(t, err) + + return l +} + +// TestNewReturnsUsableLogger checks that the constructor yields a +// working *slog.Logger. +func TestNewReturnsUsableLogger(t *testing.T) { + t.Parallel() + + l := newTestLogger(t) + require.NotNil(t, l.Get(), "Get must return a non-nil logger") +} + +// TestDefaultLevelExcludesDebug verifies the default configuration +// logs at info: debug records are suppressed, info records pass. +func TestDefaultLevelExcludesDebug(t *testing.T) { + t.Parallel() + + log := newTestLogger(t).Get() + ctx := context.Background() + + assert.False(t, log.Enabled(ctx, slog.LevelDebug), + "debug must be suppressed at the default level") + assert.True(t, log.Enabled(ctx, slog.LevelInfo), + "info must be enabled at the default level") +} + +// TestEnableDebugLoggingChangesLevel verifies the debug and non-debug +// configurations differ as intended: enabling debug makes debug +// records pass where they previously did not. +func TestEnableDebugLoggingChangesLevel(t *testing.T) { + t.Parallel() + + l := newTestLogger(t) + log := l.Get() + ctx := context.Background() + + require.False(t, log.Enabled(ctx, slog.LevelDebug), + "debug must start disabled") + + l.EnableDebugLogging() + + assert.True(t, log.Enabled(ctx, slog.LevelDebug), + "debug must be enabled after EnableDebugLogging") +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 0a05dd5..03f435e 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -21,6 +21,60 @@ import ( // corsMaxAge is the maximum age for CORS preflight responses. const corsMaxAge = 300 +// Security response header values applied to every response. +// +// The CSP is as strict as the dashboard allows: the template ships no +// JavaScript, no inline styles, no inline event handlers and no images, +// and its only subresource is the embedded stylesheet at +// /s/css/tailwind.min.css, which style-src 'self' permits. Neither +// unsafe-inline nor unsafe-eval is used. frame-ancestors 'none' is the +// primary anti-framing control; X-Frame-Options is the legacy fallback. +const ( + // hstsValue is emitted unconditionally, including over plain HTTP, + // because the service runs behind a TLS-terminating proxy and the + // browser must still enforce HTTPS end to end. + hstsValue = "max-age=31536000; includeSubDomains" + + cspValue = "default-src 'self'; " + + "script-src 'none'; " + + "style-src 'self'; " + + "img-src 'self'; " + + "font-src 'none'; " + + "connect-src 'none'; " + + "object-src 'none'; " + + "base-uri 'none'; " + + "form-action 'none'; " + + "frame-ancestors 'none'" + + frameOptionsValue = "DENY" + + contentTypeOptionsValue = "nosniff" + + // referrerPolicyValue is stricter than the policy minimum of + // strict-origin-when-cross-origin: the dashboard has no + // cross-origin navigation needs and its URL may name internal + // hosts. + referrerPolicyValue = "no-referrer" + + permissionsPolicyValue = "accelerometer=(), " + + "autoplay=(), " + + "camera=(), " + + "display-capture=(), " + + "encrypted-media=(), " + + "fullscreen=(), " + + "geolocation=(), " + + "gyroscope=(), " + + "magnetometer=(), " + + "microphone=(), " + + "midi=(), " + + "payment=(), " + + "picture-in-picture=(), " + + "publickey-credentials-get=(), " + + "screen-wake-lock=(), " + + "usb=(), " + + "xr-spatial-tracking=()" +) + // Params contains dependencies for Middleware. type Params struct { fx.In @@ -186,6 +240,37 @@ func (m *Middleware) CORS() func(http.Handler) http.Handler { }) } +// SecurityHeaders returns middleware that sets the security response +// headers required for production internet exposure on every response. +// +// The headers are set before the request reaches the next handler so +// that they are present on every response, including panics recovered +// by chi's Recoverer and timeouts produced by chi's Timeout. +func (m *Middleware) SecurityHeaders() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func( + writer http.ResponseWriter, + request *http.Request, + ) { + header := writer.Header() + header.Set("Strict-Transport-Security", hstsValue) + header.Set("Content-Security-Policy", cspValue) + header.Set("X-Frame-Options", frameOptionsValue) + header.Set( + "X-Content-Type-Options", + contentTypeOptionsValue, + ) + header.Set("Referrer-Policy", referrerPolicyValue) + header.Set( + "Permissions-Policy", + permissionsPolicyValue, + ) + + next.ServeHTTP(writer, request) + }) + } +} + // MetricsAuth returns basic auth middleware for /metrics. func (m *Middleware) MetricsAuth() func(http.Handler) http.Handler { if m.params.Config.MetricsUsername == "" { diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go new file mode 100644 index 0000000..598f476 --- /dev/null +++ b/internal/middleware/middleware_test.go @@ -0,0 +1,334 @@ +package middleware_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "go.uber.org/fx/fxtest" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/handlers" + "sneak.berlin/go/dnswatcher/internal/logger" + "sneak.berlin/go/dnswatcher/internal/middleware" + "sneak.berlin/go/dnswatcher/internal/notify" + "sneak.berlin/go/dnswatcher/internal/state" +) + +// Expected security header values, spelled out literally so that any +// change to the middleware has to be made deliberately here as well. +const ( + wantHSTS = "max-age=31536000; includeSubDomains" + + wantCSP = "default-src 'self'; " + + "script-src 'none'; " + + "style-src 'self'; " + + "img-src 'self'; " + + "font-src 'none'; " + + "connect-src 'none'; " + + "object-src 'none'; " + + "base-uri 'none'; " + + "form-action 'none'; " + + "frame-ancestors 'none'" + + wantFrameOptions = "DENY" + + wantContentTypeOptions = "nosniff" + + wantReferrerPolicy = "no-referrer" + + wantPermissionsPolicy = "accelerometer=(), " + + "autoplay=(), " + + "camera=(), " + + "display-capture=(), " + + "encrypted-media=(), " + + "fullscreen=(), " + + "geolocation=(), " + + "gyroscope=(), " + + "magnetometer=(), " + + "microphone=(), " + + "midi=(), " + + "payment=(), " + + "picture-in-picture=(), " + + "publickey-credentials-get=(), " + + "screen-wake-lock=(), " + + "usb=(), " + + "xr-spatial-tracking=()" +) + +// stylesheetPath is the only subresource the dashboard loads. +const stylesheetPath = "/s/css/tailwind.min.css" + +// newTestLogger builds a logger for direct component construction. +func newTestLogger(t *testing.T) *logger.Logger { + t.Helper() + + glob, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log, err := logger.New(nil, logger.Params{Globals: glob}) + if err != nil { + t.Fatalf("logger.New: %v", err) + } + + return log +} + +// newTestMiddleware builds a Middleware without an fx application. +func newTestMiddleware(t *testing.T) *middleware.Middleware { + t.Helper() + + glob, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + mw, err := middleware.New(nil, middleware.Params{ + Logger: newTestLogger(t), + Globals: glob, + Config: &config.Config{}, + }) + if err != nil { + t.Fatalf("middleware.New: %v", err) + } + + return mw +} + +// serveWithSecurityHeaders runs a GET through SecurityHeaders and +// returns the recorded response. +func serveWithSecurityHeaders( + t *testing.T, + target string, + handler http.Handler, +) *httptest.ResponseRecorder { + t.Helper() + + mw := newTestMiddleware(t) + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, target, nil, + ) + + mw.SecurityHeaders()(handler).ServeHTTP(rec, req) + + return rec +} + +// okHandler writes a trivial 200 response. +func okHandler() http.Handler { + return http.HandlerFunc(func( + writer http.ResponseWriter, + _ *http.Request, + ) { + writer.WriteHeader(http.StatusOK) + }) +} + +func TestSecurityHeaders(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + header string + want string + }{ + { + "hsts", + "Strict-Transport-Security", + wantHSTS, + }, + { + "csp", + "Content-Security-Policy", + wantCSP, + }, + { + "frame options", + "X-Frame-Options", + wantFrameOptions, + }, + { + "content type options", + "X-Content-Type-Options", + wantContentTypeOptions, + }, + { + "referrer policy", + "Referrer-Policy", + wantReferrerPolicy, + }, + { + "permissions policy", + "Permissions-Policy", + wantPermissionsPolicy, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := serveWithSecurityHeaders(t, "/", okHandler()) + + got := rec.Header().Get(tt.header) + if got != tt.want { + t.Errorf( + "%s = %q, want %q", + tt.header, got, tt.want, + ) + } + }) + } +} + +// TestSecurityHeadersCSPDirectives guards the properties the repo +// policy requires of the content security policy itself. +func TestSecurityHeadersCSPDirectives(t *testing.T) { + t.Parallel() + + rec := serveWithSecurityHeaders(t, "/", okHandler()) + csp := rec.Header().Get("Content-Security-Policy") + + forbidden := []string{"unsafe-inline", "unsafe-eval"} + for _, directive := range forbidden { + if strings.Contains(csp, directive) { + t.Errorf("CSP must not contain %q: %q", directive, csp) + } + } + + required := []string{ + "default-src 'self'", + "script-src 'none'", + "style-src 'self'", + "frame-ancestors 'none'", + } + for _, directive := range required { + if !strings.Contains(csp, directive) { + t.Errorf("CSP must contain %q: %q", directive, csp) + } + } +} + +// TestSecurityHeadersOnErrorResponse verifies the headers are emitted +// even when the wrapped handler fails, since they are set before the +// handler runs. +func TestSecurityHeadersOnErrorResponse(t *testing.T) { + t.Parallel() + + failing := http.HandlerFunc(func( + writer http.ResponseWriter, + _ *http.Request, + ) { + http.Error( + writer, + "boom", + http.StatusInternalServerError, + ) + }) + + rec := serveWithSecurityHeaders(t, "/api/v1/status", failing) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } + + if got := rec.Header().Get( + "X-Content-Type-Options", + ); got != wantContentTypeOptions { + t.Errorf( + "X-Content-Type-Options = %q, want %q", + got, wantContentTypeOptions, + ) + } + + if got := rec.Header().Get( + "Strict-Transport-Security", + ); got != wantHSTS { + t.Errorf( + "Strict-Transport-Security = %q, want %q", + got, wantHSTS, + ) + } +} + +// newTestHandlers builds real Handlers with empty monitoring state. +func newTestHandlers(t *testing.T) *handlers.Handlers { + t.Helper() + + glob, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log := newTestLogger(t) + + notifier, err := notify.New(fxtest.NewLifecycle(t), notify.Params{ + Logger: log, + Config: &config.Config{}, + }) + if err != nil { + t.Fatalf("notify.New: %v", err) + } + + hnd, err := handlers.New(nil, handlers.Params{ + Logger: log, + Globals: glob, + State: state.NewForTest(), + Notify: notifier, + }) + if err != nil { + t.Fatalf("handlers.New: %v", err) + } + + return hnd +} + +// TestDashboardRendersWithSecurityHeaders renders the real dashboard +// through the middleware and checks that the policy still permits the +// one stylesheet the page loads. +func TestDashboardRendersWithSecurityHeaders(t *testing.T) { + t.Parallel() + + mw := newTestMiddleware(t) + hnd := newTestHandlers(t) + + router := chi.NewRouter() + router.Use(mw.SecurityHeaders()) + router.Get("/", hnd.HandleDashboard()) + + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) + + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + + body := rec.Body.String() + if !strings.Contains(body, stylesheetPath) { + t.Errorf("dashboard does not reference %q", stylesheetPath) + } + + if !strings.Contains(body, "dnswatcher") { + t.Errorf("dashboard body looks empty: %d bytes", len(body)) + } + + csp := rec.Header().Get("Content-Security-Policy") + if csp != wantCSP { + t.Errorf("CSP = %q, want %q", csp, wantCSP) + } + + // The stylesheet is same-origin, so style-src 'self' allows it. + if !strings.Contains(csp, "style-src 'self'") { + t.Errorf("CSP would block %q: %q", stylesheetPath, csp) + } +} diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go new file mode 100644 index 0000000..6f4f9f1 --- /dev/null +++ b/internal/resolver/export_test.go @@ -0,0 +1,8 @@ +package resolver + +import "github.com/miekg/dns" + +// ExtractRecordValue exports extractRecordValue for testing. +func ExtractRecordValue(rr dns.RR) string { + return extractRecordValue(rr) +} diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index eebab39..f89d73d 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -608,7 +608,10 @@ func classifyResponse(resp *NameserverResponse, state queryState) { } } -// extractRecordValue formats a DNS RR value as a string. +// extractRecordValue formats a DNS RR value as a string. DNS names +// are case-insensitive and nameservers may answer in any letter case, +// so names are lower-cased to compare equal. TXT and CAA values keep +// their letter case. func extractRecordValue(rr dns.RR) string { switch r := rr.(type) { case *dns.A: @@ -616,22 +619,22 @@ func extractRecordValue(rr dns.RR) string { case *dns.AAAA: return r.AAAA.String() case *dns.CNAME: - return r.Target + return strings.ToLower(r.Target) case *dns.MX: - return fmt.Sprintf("%d %s", r.Preference, r.Mx) + return fmt.Sprintf("%d %s", r.Preference, strings.ToLower(r.Mx)) case *dns.TXT: return strings.Join(r.Txt, "") case *dns.SRV: return fmt.Sprintf( "%d %d %d %s", - r.Priority, r.Weight, r.Port, r.Target, + r.Priority, r.Weight, r.Port, strings.ToLower(r.Target), ) case *dns.CAA: return fmt.Sprintf( "%d %s \"%s\"", r.Flag, r.Tag, r.Value, ) case *dns.NS: - return r.Ns + return strings.ToLower(r.Ns) default: return "" } diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go new file mode 100644 index 0000000..781e90b --- /dev/null +++ b/internal/resolver/iterative_test.go @@ -0,0 +1,62 @@ +package resolver_test + +import ( + "testing" + + "github.com/miekg/dns" + "github.com/stretchr/testify/assert" + + "sneak.berlin/go/dnswatcher/internal/resolver" +) + +func TestExtractRecordValue_LetterCase(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rr dns.RR + want string + }{ + { + name: "MX target lower-cased", + rr: &dns.MX{Preference: 1, Mx: "ASPMX.L.GOOGLE.COM."}, + want: "1 aspmx.l.google.com.", + }, + { + name: "NS target lower-cased", + rr: &dns.NS{Ns: "x.ns.joker.COM."}, + want: "x.ns.joker.com.", + }, + { + name: "CNAME target lower-cased", + rr: &dns.CNAME{Target: "WWW.Example.Com."}, + want: "www.example.com.", + }, + { + name: "SRV target lower-cased", + rr: &dns.SRV{ + Priority: 10, Weight: 5, Port: 443, + Target: "SIP.Example.Com.", + }, + want: "10 5 443 sip.example.com.", + }, + { + name: "TXT value keeps its case", + rr: &dns.TXT{Txt: []string{"Verify=AbC123"}}, + want: "Verify=AbC123", + }, + { + name: "CAA value keeps its case", + rr: &dns.CAA{Flag: 0, Tag: "issue", Value: "LetsEncrypt.org"}, + want: `0 issue "LetsEncrypt.org"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, resolver.ExtractRecordValue(tt.rr)) + }) + } +} diff --git a/internal/server/export_test.go b/internal/server/export_test.go new file mode 100644 index 0000000..5a1bec7 --- /dev/null +++ b/internal/server/export_test.go @@ -0,0 +1,24 @@ +package server + +import ( + "net/http" + "time" +) + +// RequestTimeout exports the handler execution budget applied by +// chimw.Timeout in SetupRoutes, so tests can assert the relationship +// between it and the server's WriteTimeout. +const RequestTimeout time.Duration = requestTimeout + +// SetListenPort overrides the port Run binds. A test uses it to hand +// Run an unbindable port so ListenAndServe fails immediately and Run +// returns after storing its http.Server. +func SetListenPort(s *Server, port int) { + s.port = port +} + +// HTTPServerOf returns the http.Server that Run built and stored, so a +// test can inspect the timeouts the running server actually carries. +func HTTPServerOf(s *Server) *http.Server { + return s.httpServer +} diff --git a/internal/server/routes.go b/internal/server/routes.go index fa99177..5c71d84 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -21,6 +21,7 @@ func (s *Server) SetupRoutes() { // Global middleware s.router.Use(chimw.Recoverer) s.router.Use(chimw.RequestID) + s.router.Use(s.mw.SecurityHeaders()) s.router.Use(s.mw.Logging()) s.router.Use(s.mw.CORS()) s.router.Use(chimw.Timeout(requestTimeout)) diff --git a/internal/server/server.go b/internal/server/server.go index 4cb30a2..f9385eb 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -33,8 +33,52 @@ type Params struct { // shutdownTimeout is how long to wait for graceful shutdown. const shutdownTimeout = 30 * time.Second -// readHeaderTimeout is the max duration for reading request headers. -const readHeaderTimeout = 10 * time.Second +// Socket-level timeouts for the HTTP server. +// +// These bound time spent on the connection itself and are a distinct +// control from the per-request handler budget enforced by +// chimw.Timeout(requestTimeout) in routes.go: that one cancels the +// request context after requestTimeout but never touches the socket, +// so without the values below a peer can hold a connection open +// forever (slowloris, unreaped keep-alives). +// +// The one hard constraint between the two controls is +// writeTimeout > requestTimeout. net/http arms the write deadline +// once the request headers have been read, so on a plaintext +// connection it covers handler execution AND the response flush. If +// writeTimeout were <= requestTimeout the server would sever the +// connection before a handler that legitimately consumed its full +// budget could emit anything, making the 60s budget unreachable in +// practice. The margin between them is the response-flush allowance. +// +// The only clients of this service are browsers loading the dashboard +// and a Prometheus scraper; the values are sized for those. +const ( + // readHeaderTimeout is the max duration for reading request + // headers. + readHeaderTimeout = 10 * time.Second + + // readTimeout bounds reading the entire request, headers plus + // body. Every route here is a GET with no body, so this only + // ever needs to cover headers; the extra 5s over + // readHeaderTimeout is slack, not a real allowance, and keeps a + // body dribbled one byte at a time from holding the read side + // open indefinitely. + readTimeout = 15 * time.Second + + // writeTimeout must exceed the requestTimeout handler budget + // (60s) per the note above. The 15s difference is the allowance + // for flushing a completed response to a slow client. + writeTimeout = 75 * time.Second + + // idleTimeout reaps keep-alive connections between requests. It + // is deliberately longer than the common Prometheus scrape + // intervals (15s/30s/60s) so the scraper reuses its connection + // rather than reconnecting every cycle, while a browser tab + // left open on the dashboard stops occupying a connection + // within two minutes of going quiet. + idleTimeout = 120 * time.Second +) // Server is the HTTP server. type Server struct { @@ -76,16 +120,29 @@ func New( return srv, nil } +// newHTTPServer builds the listening http.Server with every +// socket-level timeout set. All four are set deliberately: a zero +// value in net/http means "no limit", not "some default". +func newHTTPServer( + listenAddr string, + handler http.Handler, +) *http.Server { + return &http.Server{ + Addr: listenAddr, + Handler: handler, + ReadTimeout: readTimeout, + ReadHeaderTimeout: readHeaderTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + } +} + // Run starts the HTTP server. func (s *Server) Run() { s.SetupRoutes() listenAddr := fmt.Sprintf(":%d", s.port) - s.httpServer = &http.Server{ - Addr: listenAddr, - Handler: s, - ReadHeaderTimeout: readHeaderTimeout, - } + s.httpServer = newHTTPServer(listenAddr, s) s.log.Info("http server starting", "addr", listenAddr) diff --git a/internal/server/server_test.go b/internal/server/server_test.go new file mode 100644 index 0000000..6cd036c --- /dev/null +++ b/internal/server/server_test.go @@ -0,0 +1,130 @@ +package server_test + +import ( + "testing" + + "github.com/spf13/viper" + "go.uber.org/fx" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/handlers" + "sneak.berlin/go/dnswatcher/internal/healthcheck" + "sneak.berlin/go/dnswatcher/internal/logger" + "sneak.berlin/go/dnswatcher/internal/middleware" + "sneak.berlin/go/dnswatcher/internal/notify" + "sneak.berlin/go/dnswatcher/internal/server" + "sneak.berlin/go/dnswatcher/internal/state" +) + +// buildServer wires a *server.Server exactly as cmd/dnswatcher does, +// minus the watcher/resolver subtree that would touch live DNS. fx +// builds the object graph but the lifecycle is never started, so no +// OnStart hook runs and nothing listens or resolves. The caller must +// first configure viper (config.New reads it), which is also why the +// caller cannot run in parallel. +func buildServer(t *testing.T) *server.Server { + t.Helper() + + var srv *server.Server + + app := fx.New( + fx.NopLogger, + fx.Provide( + globals.New, + logger.New, + config.New, + state.New, + healthcheck.New, + notify.New, + middleware.New, + handlers.New, + server.New, + ), + fx.Populate(&srv), + ) + + err := app.Err() + if err != nil { + t.Fatalf("building server graph: %v", err) + } + + return srv +} + +// TestRunWiresSocketTimeouts pins that the http.Server the running +// server actually serves — the one Run builds and hands to +// ListenAndServe — carries every socket-level timeout, plus the two +// relationships the values must satisfy. +// +// Run is driven to completion with an unbindable port: it builds and +// stores s.httpServer, then ListenAndServe fails at once and Run +// returns without ever listening. The assertions run in the same +// goroutine after Run returns, so reading s.httpServer is free of any +// data race. Nothing here measures elapsed time. +// +// ReadTimeout must be at least ReadHeaderTimeout. net/http reads the +// headers under ReadHeaderTimeout, then sets the read deadline for the +// rest of the request to ReadTimeout, counted from when it started +// reading the request. If ReadTimeout were smaller, a request whose +// headers arrived after ReadTimeout but within ReadHeaderTimeout would +// get a read deadline that had already passed, so reading its body +// would fail at once. +func TestRunWiresSocketTimeouts(t *testing.T) { + // Sets an env var and touches viper global state, so like the + // config tests it cannot use t.Parallel. + viper.Reset() + t.Setenv("DNSWATCHER_TARGETS", "example.com") + + srv := buildServer(t) + server.SetListenPort(srv, -1) + + srv.Run() + + hs := server.HTTPServerOf(srv) + if hs == nil { + t.Fatal("Run did not build an http.Server") + } + + if hs.ReadTimeout <= 0 { + t.Errorf("ReadTimeout must be non-zero, got %v", hs.ReadTimeout) + } + + if hs.ReadHeaderTimeout <= 0 { + t.Errorf( + "ReadHeaderTimeout must be non-zero, got %v", + hs.ReadHeaderTimeout, + ) + } + + if hs.WriteTimeout <= 0 { + t.Errorf("WriteTimeout must be non-zero, got %v", hs.WriteTimeout) + } + + if hs.IdleTimeout <= 0 { + t.Errorf("IdleTimeout must be non-zero, got %v", hs.IdleTimeout) + } + + if hs.WriteTimeout <= server.RequestTimeout { + t.Errorf( + "WriteTimeout (%v) must exceed handler budget (%v)", + hs.WriteTimeout, + server.RequestTimeout, + ) + } + + if hs.ReadTimeout < hs.ReadHeaderTimeout { + t.Errorf( + "ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)", + hs.ReadTimeout, + hs.ReadHeaderTimeout, + ) + } + + if hs.Handler != srv { + t.Errorf( + "Run wired handler %T, want the *server.Server", + hs.Handler, + ) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index efe681c..417a4d7 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -148,6 +148,11 @@ func New( lifecycle.Append(fx.Hook{ OnStart: func(_ context.Context) error { + err := state.checkDataDirWritable() + if err != nil { + return err + } + return state.Load() }, OnStop: func(_ context.Context) error { @@ -345,3 +350,27 @@ func (s *State) GetCertificateState( return cs, ok } + +// checkDataDirWritable creates the data directory if needed, then writes +// and removes the temp file that Save uses. It runs at startup so that an +// unwritable directory stops the process, instead of the process running +// with every save failing and only logged. +func (s *State) checkDataDirWritable() error { + dir := s.config.DataDir + tmpPath := s.config.StatePath() + ".tmp" + + err := os.MkdirAll(dir, dirPermissions) + if err == nil { + err = os.WriteFile(tmpPath, nil, filePermissions) + } + + if err == nil { + err = os.Remove(tmpPath) + } + + if err != nil { + return fmt.Errorf("data directory %s is not writable: %w", dir, err) + } + + return nil +} diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 699d17c..3fbca93 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -4,10 +4,16 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "sync" "testing" "time" + "go.uber.org/fx/fxtest" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/logger" "sneak.berlin/go/dnswatcher/internal/state" ) @@ -493,6 +499,107 @@ func TestSaveWritePermissionError(t *testing.T) { } } +// startState builds a State through the real constructor and runs its +// startup hook against dataDir, returning the startup error. +func startState(t *testing.T, dataDir string) error { + t.Helper() + + g, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log, err := logger.New(nil, logger.Params{Globals: g}) + if err != nil { + t.Fatalf("logger.New: %v", err) + } + + lifecycle := fxtest.NewLifecycle(t) + + _, err = state.New(lifecycle, state.Params{ + Logger: log, + Config: &config.Config{DataDir: dataDir}, + }) + if err != nil { + t.Fatalf("state.New: %v", err) + } + + return lifecycle.Start(t.Context()) +} + +// TestStartupFailsWhenDataDirNotWritable verifies that startup stops +// with an error naming the data directory when it cannot be written. +// The directory's parent is a regular file, which also fails as root. +func TestStartupFailsWhenDataDirNotWritable(t *testing.T) { + t.Parallel() + + parent := filepath.Join(t.TempDir(), "file") + + err := os.WriteFile(parent, nil, 0o600) + if err != nil { + t.Fatalf("writing file: %v", err) + } + + dataDir := filepath.Join(parent, "data") + + err = startState(t, dataDir) + if err == nil { + t.Fatal("startup should fail when the data directory is not writable") + } + + want := "data directory " + dataDir + " is not writable" + if !strings.Contains(err.Error(), want) { + t.Errorf("startup error %q does not contain %q", err, want) + } +} + +// TestStartupFailsWhenExistingDataDirNotWritable verifies that startup +// stops when the data directory exists but the temp file that saving uses +// cannot be written in it. A directory sitting at the temp file's path +// makes that write fail, which also holds as root. +func TestStartupFailsWhenExistingDataDirNotWritable(t *testing.T) { + t.Parallel() + + dataDir := t.TempDir() + + err := os.Mkdir(filepath.Join(dataDir, "state.json.tmp"), 0o700) + if err != nil { + t.Fatalf("creating directory: %v", err) + } + + err = startState(t, dataDir) + if err == nil { + t.Fatal("startup should fail when the data directory is not writable") + } + + want := "data directory " + dataDir + " is not writable" + if !strings.Contains(err.Error(), want) { + t.Errorf("startup error %q does not contain %q", err, want) + } +} + +// TestStartupCreatesDataDir verifies that startup creates a missing +// data directory and leaves nothing behind in it. +func TestStartupCreatesDataDir(t *testing.T) { + t.Parallel() + + dataDir := filepath.Join(t.TempDir(), "data") + + err := startState(t, dataDir) + if err != nil { + t.Fatalf("startup error: %v", err) + } + + entries, err := os.ReadDir(dataDir) + if err != nil { + t.Fatalf("reading data directory: %v", err) + } + + if len(entries) != 0 { + t.Errorf("startup left %d entries in the data directory", len(entries)) + } +} + // TestPortStateUnmarshalJSON_NewFormat verifies deserialization of the // current multi-hostname format. func TestPortStateUnmarshalJSON_NewFormat(t *testing.T) { diff --git a/script/cibuild b/script/cibuild index 1b9e57d..29bea03 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,14 +1,18 @@ #!/bin/sh # script/cibuild: run the CI build. The Dockerfile's lint stage runs # make fmt-check and golangci-lint; its builder stage runs make test -# and make build. A successful build implies all of those passed. +# and make build. +# +# --no-cache-filter=lint,builder runs both stages on every invocation; +# otherwise an unchanged tree is served from the layer cache and passes +# without linting or querying live DNS. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - docker build . + docker build --no-cache-filter=lint,builder . } main "$@" diff --git a/script/docker b/script/docker index 9b9ea86..4f1fd14 100755 --- a/script/docker +++ b/script/docker @@ -1,6 +1,10 @@ #!/bin/sh # script/docker: build the Docker image tagged with the project name. -# Identical in all repos; the tag comes from script/projectname. +# The tag comes from script/projectname. +# +# --no-cache-filter=lint,builder runs the lint stage and the builder +# stage (make test) on every invocation; otherwise an unchanged tree is +# served from the layer cache without linting or querying live DNS. set -eu SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" @@ -8,7 +12,7 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" - docker build -t "$("$SCRIPT_DIR/projectname")" . + docker build --no-cache-filter=lint,builder -t "$("$SCRIPT_DIR/projectname")" . } main "$@"