Author SHA1 Message Date
clawbot b79e4649a1 Container sets its data directory's owner and mode itself (#353)
check / check (push) Waiting to run
Closes #340.

The image no longer sets `USER`. Its new `ENTRYPOINT`, `deploy/docker-entrypoint.sh`, starts as root, creates `DATA_DIR` if missing, gives the directory and anything in it owned by another user to `webhooker` (UID 1000), sets the directory to `0750`, and runs the command as `webhooker` through `su-exec`. An empty root-owned bind mount, or data left by another UID, now works as mounted; the app never runs as root and is still PID 1. `CMD` is still `/app/webhooker`, so the `resetpw` commands are unchanged. Started with `--user`, the script only runs the command. It is in `/usr/local/bin`, not `/app`, which belongs to `webhooker`.

README: the UID 1000 ownership block, the upaas pre-deploy commands and the restore ownership step are gone; the upaas volume bullet names only the path.

- Judgement call: `su-exec` over `setpriv`: Alpine's small tool for this, needing only musl; busybox's `setpriv` cannot change user, and util-linux's adds `libcap-ng`.
- Deviation: `su-exec` is pinned by version (`0.2-r3`), not by hash; `ca-certificates` beside it is unpinned.
- Judgement call: each start reads every entry's owner but changes only entries owned by someone else.
- `docker exec` and the health check now run as root, since the image sets no `USER`.
- No automated test covers the script: the suite runs inside `docker build`, which cannot start a container.
- A missing host directory under upaas is sneak/upaas#235.

Model: opus-5-5
Reviewed-on: #353
Co-authored-by: clawbot <35+clawbot@noreply.example.org>
2026-09-29 13:05:16 +02:00
clawbot 1428154bbd Let a browser with cookies from an earlier database log in (closes #359)
check / check (push) Waiting to run
A new database brings a new session key. A browser still holding the
old session cookie got a 500 on a correct login: Session.Get returned
the cookie's decode error and the login handler answered it with a
500. Get now treats a cookie that does not decode as absent, and
logging in replaces it. gorilla/csrf already did the same for the
CSRF cookie.

A start that creates webhooker.db now logs "created a new, empty
database" at WARN with its path, shortly before the first-boot banner,
so an unexpectedly empty DATA_DIR is noticed.

The codec tests now decode through the store, since Get no longer
reports the codec's reason.

Model: opus-5-5
2026-09-29 12:58:44 +02:00
sneak 8ad2a86e4b Sneak/testdeploy (#356)
check / check (push) Successful in 11s
Reviewed-on: #356
2026-09-29 12:01:33 +02:00
sneak a891b726e5 Milestone: next into main (#342)
check / check (push) Successful in 9s
Reviewed-on: #342
2026-09-29 11:53:19 +02:00
22 changed files with 280 additions and 233 deletions
+9 -2
View File
@@ -88,7 +88,9 @@ RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static
# alpine:3.21, 2026-03-17 # alpine:3.21, 2026-03-17
FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709 FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709
RUN apk --no-cache add ca-certificates # su-exec 0.2-r3 (Alpine 3.21), 2026-09-29: the entrypoint runs the app
# as webhooker with it.
RUN apk --no-cache add ca-certificates su-exec=0.2-r3
# Create non-root user # Create non-root user
RUN addgroup -g 1000 -S webhooker && \ RUN addgroup -g 1000 -S webhooker && \
@@ -99,13 +101,17 @@ WORKDIR /app
# Copy binary from builder # Copy binary from builder
COPY --from=builder /build/bin/webhooker /app/webhooker COPY --from=builder /build/bin/webhooker /app/webhooker
# Not under /app, which belongs to webhooker: this script runs as root.
COPY deploy/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
# Create data directory for all SQLite databases (main app DB + # Create data directory for all SQLite databases (main app DB +
# per-webhook event DBs). DATA_DIR defaults to /var/lib/webhooker. # per-webhook event DBs). DATA_DIR defaults to /var/lib/webhooker.
RUN mkdir -p /var/lib/webhooker RUN mkdir -p /var/lib/webhooker
RUN chown -R webhooker:webhooker /app /var/lib/webhooker RUN chown -R webhooker:webhooker /app /var/lib/webhooker
USER webhooker # No USER: the entrypoint starts as root to make the data directory
# webhooker's, then runs the app as webhooker.
EXPOSE 8080 EXPOSE 8080
@@ -124,4 +130,5 @@ ENV BIND_ADDRESS=0.0.0.0
HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1 CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]
CMD ["/app/webhooker"] CMD ["/app/webhooker"]
+53 -77
View File
@@ -538,6 +538,12 @@ its Argon2id hash. There is no second account and no forgot-password
flow, so the banner and the reset command below are the only two ways flow, so the banner and the reset command below are the only two ways
in. in.
A start that finds no `webhooker.db` in `DATA_DIR` also logs
`created a new, empty database` at `WARN`, with the file's path,
shortly before the banner. On a deployment that has run before, that
line means `DATA_DIR` was empty, most often because its volume is not
mounted.
#### Recovering a lost admin password #### Recovering a lost admin password
`webhooker resetpw` sets an existing account's password from the `webhooker resetpw` sets an existing account's password from the
@@ -552,8 +558,9 @@ printf '%s' "$NEW_PASSWORD" | \
DATA_DIR=/var/lib/webhooker webhooker resetpw admin DATA_DIR=/var/lib/webhooker webhooker resetpw admin
``` ```
In a container it is the same binary, which the image sets as `CMD` In a container it is the same binary. The image's `CMD` is
rather than `ENTRYPOINT`, so the whole command has to be given: `/app/webhooker`, and a command given to `docker run` replaces all of
it, so the whole command has to be given:
```bash ```bash
docker run --rm -v webhooker-data:/var/lib/webhooker \ docker run --rm -v webhooker-data:/var/lib/webhooker \
@@ -690,38 +697,22 @@ those three values rather than trusting the figure. Measured at 65s on
Docker 29.7.2.) A container `unhealthy` with `connection refused` in Docker 29.7.2.) A container `unhealthy` with `connection refused` in
its health log, or a published port that resets connections, is this. its health log, or a published port that resets connections, is this.
The container runs as a non-root user (`webhooker`, UID 1000), exposes The app runs as a non-root user (`webhooker`, UID 1000), exposes port
port 8080, and includes a health check against 8080, and includes a health check against `/.well-known/healthcheck`.
`/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all The `/var/lib/webhooker` volume holds all SQLite databases: the main
SQLite databases: the main application database (`webhooker.db`), the application database (`webhooker.db`), the per-webhook event databases
per-webhook event databases (`events-{uuid}.db`), and any archive (`events-{uuid}.db`), and any archive databases written by `database`
databases written by `database` targets (`archive-{uuid}.db`). Mount targets (`archive-{uuid}.db`). Mount this as a persistent volume to
this as a persistent volume to preserve data across container preserve data across container restarts.
restarts.
**The bind-mounted directory must be owned by UID 1000, or the **The container sets its data directory's owner and mode itself
container does not start.** Docker creates a `-v` source path that before the app starts**, so a host directory can be mounted as it is,
does not exist yet as `root:root`, and the process runs as UID 1000, whoever owns it. The image's `ENTRYPOINT`,
so it cannot take its `DATA_DIR` lock: `deploy/docker-entrypoint.sh`, starts as root, creates `DATA_DIR` if
it is missing, gives the directory and anything in it that belongs to
``` another user to `webhooker`, sets the directory to `0750`, and only
webhooker: locking data directory /var/lib/webhooker: open then runs the app as `webhooker`. Started with `--user`, it changes
/var/lib/webhooker/webhooker.lock: permission denied nothing and runs the app as that user.
```
It exits non-zero at that point, before opening any database. Create
the directory ahead of the first `docker run`:
```bash
mkdir -p /path/to/data
chown 1000:1000 /path/to/data
chmod 750 /path/to/data
```
The same `chown` is what a restore needs — see step 4 of
[Restore](#restore). A **named volume** does not have this problem:
Docker copies the image's ownership onto a volume it initializes, and
the image creates `/var/lib/webhooker` owned by `webhooker`.
**The file modes are not yours to set, and do not depend on the **The file modes are not yours to set, and do not depend on the
directory.** `webhooker.db` holds target configuration in plaintext — directory.** `webhooker.db` holds target configuration in plaintext —
@@ -729,13 +720,10 @@ bearer tokens, API keys, Slack webhook URLs — along with the session
encryption key, so webhooker creates every SQLite file it owns `0600`: encryption key, so webhooker creates every SQLite file it owns `0600`:
each database and both of its `-wal` and `-shm` sidecars, across all each database and both of its `-wal` and `-shm` sidecars, across all
three tiers. Files an earlier build left `0644` are tightened when three tiers. Files an earlier build left `0644` are tightened when
they are opened. A `DATA_DIR` webhooker creates itself is `0750`, but they are opened. The directory's `0750` is defence in depth — it stops
a bind mount supplies its own directory and Docker's default for one other local users listing the directory and learning your webhook
it creates is `0755`; the `0600` files hold there regardless. The UUIDs from the `events-{uuid}.db` filenames — not the barrier
`chmod 750` above is defence in depth — it stops other local users protecting the credentials.
listing the directory and learning your webhook UUIDs from the
`events-{uuid}.db` filenames — not the barrier protecting the
credentials.
### Running under upaas ### Running under upaas
@@ -751,17 +739,6 @@ repository's `Dockerfile` and runs it. The app needs:
app name, port `8080`. Leave `PORT` unset: the image's health check app name, port `8080`. Leave `PORT` unset: the image's health check
probes `8080`. probes `8080`.
- **Volume:** one host directory mounted at `/var/lib/webhooker`. - **Volume:** one host directory mounted at `/var/lib/webhooker`.
upaas bind-mounts the host path it is given and does not create it,
and the container does not start unless UID 1000 owns it (see
[Running with Docker](#running-with-docker)). Create it before the
first deploy:
```bash
mkdir -p /path/to/data
chown 1000:1000 /path/to/data
chmod 750 /path/to/data
```
- **Environment variables:** - **Environment variables:**
- `WEBHOOKER_ENVIRONMENT=prod` - `WEBHOOKER_ENVIRONMENT=prod`
- `TRUSTED_PROXIES`: your reverse proxy's address on that Docker - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker
@@ -1020,12 +997,12 @@ done
`.backup` reads through the WAL and writes a single consistent file with `.backup` reads through the WAL and writes a single consistent file with
no sidecars of its own, so the destination is complete as it stands. no sidecars of its own, so the destination is complete as it stands.
Two caveats. First, the runtime image is `alpine:3.21` with only Two caveats. First, the runtime image is `alpine:3.21` with only
`ca-certificates` added — the `sqlite3` CLI is **not** in it, so run `ca-certificates` and `su-exec` added — the `sqlite3` CLI is **not** in
this on the host against the volume path, or from a throwaway container it, so run this on the host against the volume path, or from a
that mounts the volume. Second, each file is captured at its own throwaway container that mounts the volume. Second, each file is
instant, so a webhook created or an event delivered between two files captured at its own instant, so a webhook created or an event delivered
being copied lands in one and not the other. If you need the whole set between two files being copied lands in one and not the other. If you
coherent as of a single moment, stop the service. need the whole set coherent as of a single moment, stop the service.
Note that `sqlite3 <db> .dump` is **not** one of these procedures: it is Note that `sqlite3 <db> .dump` is **not** one of these procedures: it is
an export, it holds a read transaction open for as long as it runs, and an export, it holds a read transaction open for as long as it runs, and
@@ -1079,21 +1056,11 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
archive not opened since a crash. A copy salvaged from a crashed archive not opened since a crash. A copy salvaged from a crashed
instance has them for everything, and needs all of them. instance has them for everything, and needs all of them.
4. **Fix ownership.** The container runs as the non-root `webhooker` 4. Start the service. The container gives the directory and the
user, UID 1000 / GID 1000. Restored files must be owned by (or restored files to the `webhooker` user before the app starts,
writable by) that UID, and so must the directory itself — SQLite whoever restored them (see
creates the `-wal` and `-shm` sidecars beside the database, so a [Running with Docker](#running-with-docker)). `AutoMigrate` runs
writable file inside a directory it cannot write is not enough: against each restored database as it is opened.
```bash
chown -R 1000:1000 /path/to/data
```
Restoring as `root` on the host and forgetting this step is the
usual way a restore fails.
5. Start the service. `AutoMigrate` runs against each restored database
as it is opened.
### Upgrades ### Upgrades
@@ -3071,7 +3038,11 @@ check, see [The login endpoint](#the-login-endpoint).
- Prometheus metrics behind basic auth - Prometheus metrics behind basic auth
- Static assets embedded in binary (no filesystem access needed at - Static assets embedded in binary (no filesystem access needed at
runtime) runtime)
- Container runs as non-root user (UID 1000) - The app runs as the non-root `webhooker` user (UID 1000) in the
container. The image sets no `USER`, so these run as root: the
`ENTRYPOINT` script, which sets the data directory's owner and mode
before the app starts; the image's health check; and `docker exec`,
unless given `--user`
- GORM soft deletes on every entity that carries `BaseModel`, which is - GORM soft deletes on every entity that carries `BaseModel`, which is
all of them but `Setting` (data preserved for audit) all of them but `Setting` (data preserved for audit)
@@ -3198,10 +3169,13 @@ version is fixed independently of the compiler's:
`GO_LDFLAGS`, so neither can drop the `-X` that stamps the version. `GO_LDFLAGS`, so neither can drop the `-X` that stamps the version.
The version arrives as the `VERSION` build arg, since the context The version arrives as the `VERSION` build arg, since the context
has no `.git` (see [Version stamping](#version-stamping)). has no `.git` (see [Version stamping](#version-stamping)).
3. **Runtime stage** (`alpine:3.21`) — copies the static binary, 3. **Runtime stage** (`alpine:3.21`) — copies the static binary and
creates the `/var/lib/webhooker` directory for all SQLite databases, `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker`
runs as the non-root `webhooker` user (UID 1000), exposes port 8080, directory for all SQLite databases, exposes port 8080, and includes
and includes a health check against `/.well-known/healthcheck`. a health check against `/.well-known/healthcheck`. It sets no
`USER`: the `ENTRYPOINT` script starts as root, sets the data
directory's owner and mode, and runs the app as the non-root
`webhooker` user (UID 1000) through `su-exec`.
The lint stage invokes `golangci-lint` directly rather than `make lint`: The lint stage invokes `golangci-lint` directly rather than `make lint`:
it is already the pinned linter image, and `make lint` builds it is already the pinned linter image, and `make lint` builds
@@ -3280,3 +3254,5 @@ MIT
## Author ## Author
[@sneak](https://sneak.berlin) [@sneak](https://sneak.berlin)
-5
View File
@@ -16,7 +16,6 @@ import (
"sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/healthcheck" "sneak.berlin/go/webhooker/internal/healthcheck"
"sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/metrics"
"sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/middleware"
"sneak.berlin/go/webhooker/internal/resetpw" "sneak.berlin/go/webhooker/internal/resetpw"
"sneak.berlin/go/webhooker/internal/server" "sneak.berlin/go/webhooker/internal/server"
@@ -178,10 +177,6 @@ func newApp() *fx.App {
healthcheck.New, healthcheck.New,
session.New, session.New,
handlers.New, handlers.New,
// The registry /metrics serves, and the delivery
// collectors registered on it.
metrics.NewRegistry,
metrics.New,
middleware.New, middleware.New,
// The one SSRF guard both target-creation validation // The one SSRF guard both target-creation validation
// and the delivery dialer consult, so they cannot // and the delivery dialer consult, so they cannot
+22
View File
@@ -0,0 +1,22 @@
#!/bin/sh
# deploy/docker-entrypoint.sh: the image's ENTRYPOINT. A bind-mounted
# data directory keeps its owner from the host, often root, and the app
# could not write to it. Started as root, this creates DATA_DIR if
# needed, gives it and everything in it to webhooker, sets its mode, and
# runs the command as webhooker, so the app never runs as root. Started
# as another user, it only runs the command.
set -eu
main() {
if [ "$(id -u)" != 0 ]; then
exec "$@"
fi
dir="${DATA_DIR:-/var/lib/webhooker}"
mkdir -p "$dir"
find "$dir" ! -user webhooker -exec chown -h webhooker:webhooker {} +
chmod 750 "$dir"
exec su-exec webhooker "$@"
}
main "$@"
@@ -3,6 +3,8 @@ package database_test
import ( import (
"bytes" "bytes"
"context" "context"
"log/slog"
"path/filepath"
"strings" "strings"
"testing" "testing"
@@ -83,3 +85,37 @@ func TestFirstBoot_PrintsTheAdminPasswordAsABanner(t *testing.T) {
t, ok, "the printed password must open the seeded account", t, ok, "the printed password must open the seeded account",
) )
} }
// TestNewDatabase_IsLoggedWithItsPath is the log half of
// https://git.eeqj.de/sneak/webhooker/issues/359. A DATA_DIR that is
// unexpectedly empty boots exactly like a first start, so the start
// that creates the database must say so, and where. Opening that
// database again must not.
func TestNewDatabase_IsLoggedWithItsPath(t *testing.T) {
t.Parallel()
dir := t.TempDir()
open := func() string {
var out bytes.Buffer
db, err := database.Open(dir, slog.New(slog.NewTextHandler(&out, nil)))
require.NoError(t, err)
require.NoError(t, db.Close())
return out.String()
}
const created = `level=WARN msg="created a new, empty database"`
first := open()
second := open()
assert.Contains(
t, first,
created+" path="+filepath.Join(dir, database.MainDBFileName),
)
assert.NotContains(
t, second, created, "an existing database is not new",
)
}
+13 -1
View File
@@ -8,6 +8,7 @@ import (
"errors" "errors"
"fmt" "fmt"
"io" "io"
"io/fs"
"log/slog" "log/slog"
"os" "os"
"path/filepath" "path/filepath"
@@ -199,6 +200,12 @@ func (d *Database) connectTo(dataDir string) error {
// Construct the main application database path inside DATA_DIR. // Construct the main application database path inside DATA_DIR.
dbPath := filepath.Join(dataDir, MainDBFileName) dbPath := filepath.Join(dataDir, MainDBFileName)
// Checked before opening, which creates the file. A DATA_DIR that
// is unexpectedly empty -- its volume not mounted, say -- looks
// exactly like a first start, so a new database is a warning.
_, statErr := os.Stat(dbPath)
created := errors.Is(statErr, fs.ErrNotExist)
// Opened through OpenSQLite so this handle carries the same WAL // Opened through OpenSQLite so this handle carries the same WAL
// journaling, busy timeout, immediate-transaction locking, and pool // journaling, busy timeout, immediate-transaction locking, and pool
// bounds as every other database file. See sqlite_open.go. // bounds as every other database file. See sqlite_open.go.
@@ -229,7 +236,12 @@ func (d *Database) connectTo(dataDir string) error {
} }
d.db = db d.db = db
d.log.Info("connected to database", "path", dbPath)
if created {
d.log.Warn("created a new, empty database", "path", dbPath)
} else {
d.log.Info("connected to database", "path", dbPath)
}
// Run migrations // Run migrations
return d.migrate() return d.migrate()
+5 -6
View File
@@ -148,7 +148,6 @@ type EngineParams struct {
DBManager *database.WebhookDBManager DBManager *database.WebhookDBManager
Logger *logger.Logger Logger *logger.Logger
SSRFGuard *Guard SSRFGuard *Guard
Metrics *metrics.Set
} }
// Engine processes queued deliveries in the background // Engine processes queued deliveries in the background
@@ -168,10 +167,10 @@ type Engine struct {
retryCh chan Task retryCh chan Task
workers int workers int
// mtr is the delivery metric set. Production wires the one // mtr is the delivery metric set. Production wires the
// registered on the registry /metrics serves; a test can // process-wide one; a test can substitute a set registered on
// substitute a set registered on a registry it holds, so it can // a private registry so its assertions are not disturbed by
// gather what its own deliveries recorded. // deliveries other tests are making at the same time.
mtr *metrics.Set mtr *metrics.Set
// targets maps each target type to its implementation. // targets maps each target type to its implementation.
@@ -205,7 +204,7 @@ func New(
deliveryCh: make(chan Task, deliveryChannelSize), deliveryCh: make(chan Task, deliveryChannelSize),
retryCh: make(chan Task, retryChannelSize), retryCh: make(chan Task, retryChannelSize),
workers: defaultWorkers, workers: defaultWorkers,
mtr: params.Metrics, mtr: metrics.Default(),
} }
e.initTargets(&http.Client{ e.initTargets(&http.Client{
+5 -5
View File
@@ -9,7 +9,6 @@ import (
"net/url" "net/url"
"time" "time"
"github.com/prometheus/client_golang/prometheus"
"go.uber.org/fx" "go.uber.org/fx"
"gorm.io/gorm" "gorm.io/gorm"
"sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/database"
@@ -390,7 +389,7 @@ func NewTestEngine(
deliveryCh: make(chan Task, deliveryChannelSize), deliveryCh: make(chan Task, deliveryChannelSize),
retryCh: make(chan Task, retryChannelSize), retryCh: make(chan Task, retryChannelSize),
workers: workers, workers: workers,
mtr: metrics.New(prometheus.NewRegistry()), mtr: metrics.Default(),
} }
e.initTargets(client) e.initTargets(client)
@@ -405,7 +404,7 @@ func NewTestEngineSmallRetry(
e := &Engine{ e := &Engine{
log: log, log: log,
retryCh: make(chan Task, 1), retryCh: make(chan Task, 1),
mtr: metrics.New(prometheus.NewRegistry()), mtr: metrics.Default(),
} }
e.initTargets(nil) e.initTargets(nil)
@@ -428,7 +427,7 @@ func NewTestEngineWithDB(
deliveryCh: make(chan Task, deliveryChannelSize), deliveryCh: make(chan Task, deliveryChannelSize),
retryCh: make(chan Task, retryChannelSize), retryCh: make(chan Task, retryChannelSize),
workers: workers, workers: workers,
mtr: metrics.New(prometheus.NewRegistry()), mtr: metrics.Default(),
} }
e.initTargets(client) e.initTargets(client)
@@ -436,7 +435,8 @@ func NewTestEngineWithDB(
} }
// ExportSetMetrics substitutes the engine's metric set, so a test can // ExportSetMetrics substitutes the engine's metric set, so a test can
// assert on collectors registered on a registry it holds. // assert on collectors registered on a private registry instead of
// the process-wide ones every other test is also moving.
func (e *Engine) ExportSetMetrics(mtr *metrics.Set) { func (e *Engine) ExportSetMetrics(mtr *metrics.Set) {
e.mtr = mtr e.mtr = mtr
} }
+3 -2
View File
@@ -35,8 +35,9 @@ const (
) )
// mIsolate gives the setup's engine a metric set registered on a // mIsolate gives the setup's engine a metric set registered on a
// registry this test holds, so its exact assertions can gather from // private registry. The process-wide collectors are moved by every
// it. // other delivery test running in parallel, so exact assertions are
// only possible against a registry this test owns.
func mIsolate( func mIsolate(
t *testing.T, s iSetup, t *testing.T, s iSetup,
) *prometheus.Registry { ) *prometheus.Registry {
+1 -4
View File
@@ -12,7 +12,6 @@ import (
"net/http" "net/http"
"sync/atomic" "sync/atomic"
"github.com/prometheus/client_golang/prometheus"
"go.uber.org/fx" "go.uber.org/fx"
"sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/delivery" "sneak.berlin/go/webhooker/internal/delivery"
@@ -62,8 +61,6 @@ type HandlersParams struct {
Notifier delivery.Notifier Notifier delivery.Notifier
Evictor delivery.WebhookEvictor Evictor delivery.WebhookEvictor
SSRFGuard *delivery.Guard SSRFGuard *delivery.Guard
Metrics *metrics.Set
Registry *prometheus.Registry
} }
// Handlers provides HTTP handler methods for all application // Handlers provides HTTP handler methods for all application
@@ -125,7 +122,7 @@ func New(
s.mw = params.Middleware s.mw = params.Middleware
s.notifier = params.Notifier s.notifier = params.Notifier
s.evictor = params.Evictor s.evictor = params.Evictor
s.mtr = params.Metrics s.mtr = metrics.Default()
s.ssrf = params.SSRFGuard s.ssrf = params.SSRFGuard
// Parse all page templates once at startup // Parse all page templates once at startup
-3
View File
@@ -20,7 +20,6 @@ import (
"sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/healthcheck" "sneak.berlin/go/webhooker/internal/healthcheck"
"sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/metrics"
"sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/middleware"
"sneak.berlin/go/webhooker/internal/session" "sneak.berlin/go/webhooker/internal/session"
) )
@@ -110,8 +109,6 @@ func newTestApp(
func(r *recordingEvictor) delivery.WebhookEvictor { func(r *recordingEvictor) delivery.WebhookEvictor {
return r return r
}, },
metrics.NewRegistry,
metrics.New,
middleware.New, middleware.New,
delivery.NewGuard, delivery.NewGuard,
handlers.New, handlers.New,
-20
View File
@@ -1,20 +0,0 @@
package handlers
import (
"net/http"
"github.com/prometheus/client_golang/prometheus/promhttp"
)
// HandleMetrics returns the Prometheus scrape handler for the
// registry every collector in this process registers on. It is what
// promhttp.Handler builds for the global default registry, including
// the promhttp_metric_handler_* series that count scrapes, pointed at
// that registry instead.
func (s *Handlers) HandleMetrics() http.HandlerFunc {
reg := s.params.Registry
return promhttp.InstrumentMetricHandler(
reg, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
).ServeHTTP
}
+20 -26
View File
@@ -3,17 +3,17 @@
// deliveries are attempted, how they end, how long they take, how // deliveries are attempted, how they end, how long they take, how
// deep the queues are, and how many circuit breakers are open. // deep the queues are, and how many circuit breakers are open.
// //
// It also builds the registry the authenticated /metrics route // The inbound HTTP metrics come from the go-http-metrics recorder in
// serves. These collectors, the inbound HTTP metrics recorded in // internal/middleware and land on prometheus.DefaultRegisterer. These
// internal/middleware, and the Go runtime and process collectors all // collectors register there too, so both surfaces are gathered by the
// register on that one registry, never on Prometheus's global default. // one promhttp handler mounted on the authenticated /metrics route.
package metrics package metrics
import ( import (
"sync"
"time" "time"
"github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/collectors"
"github.com/prometheus/client_golang/prometheus/promauto" "github.com/prometheus/client_golang/prometheus/promauto"
"sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/database"
) )
@@ -57,31 +57,25 @@ var knownTargetTypes = []database.TargetType{
database.TargetTypeSlack, database.TargetTypeSlack,
} }
// NewRegistry returns the registry /metrics serves, carrying the Go // defaultSet is the process-wide metric set, registered on the same
// runtime and process collectors that Prometheus's global default // registry the HTTP middleware and the /metrics handler already use.
// registry carries, so the go_* and process_* series stay in the // It is built on first use rather than in an init so that a test
// scrape. // binary that never touches metrics never registers them.
// //
// A registry of its own, rather than the global default, is what lets //nolint:gochecknoglobals // one process-wide registration, by design
// two dependency graphs in one process — two tests, say — each var defaultSet = sync.OnceValue(func() *Set {
// register their collectors without the second registration return New(prometheus.DefaultRegisterer)
// panicking. })
func NewRegistry() *prometheus.Registry {
reg := prometheus.NewRegistry()
reg.MustRegister(
collectors.NewGoCollector(),
collectors.NewProcessCollector(
collectors.ProcessCollectorOpts{},
),
)
return reg // Default returns the process-wide metric set.
func Default() *Set {
return defaultSet()
} }
// Set is one registered group of webhooker's delivery collectors. // Set is one registered group of webhooker's delivery collectors.
// Production builds one on the registry /metrics serves; tests build // Production uses the single Default set; tests build their own
// their own against a private registry so assertions are not // against a private registry so assertions are not disturbed by
// disturbed by deliveries other tests are making concurrently. // deliveries other tests are making concurrently.
type Set struct { type Set struct {
eventsReceived prometheus.Counter eventsReceived prometheus.Counter
deliveryAttempts *prometheus.CounterVec deliveryAttempts *prometheus.CounterVec
@@ -99,7 +93,7 @@ type Set struct {
// New registers a full set of delivery collectors on reg and returns // New registers a full set of delivery collectors on reg and returns
// it. It panics if reg already holds them, which is the intended // it. It panics if reg already holds them, which is the intended
// behaviour for a duplicate registration. // behaviour for a duplicate registration.
func New(reg *prometheus.Registry) *Set { func New(reg prometheus.Registerer) *Set {
factory := promauto.With(reg) factory := promauto.With(reg)
s := &Set{ s := &Set{
+2 -1
View File
@@ -10,7 +10,8 @@ import (
// MetricsMiddlewareForTest builds the metrics recording middleware // MetricsMiddlewareForTest builds the metrics recording middleware
// against a caller-supplied recorder, so a test can gather from its // against a caller-supplied recorder, so a test can gather from its
// own Prometheus registry without building a whole Middleware. // own Prometheus registry rather than the process-wide default one
// that Middleware.Metrics uses.
func MetricsMiddlewareForTest( func MetricsMiddlewareForTest(
rec httpmetrics.Recorder, rec httpmetrics.Recorder,
) func(http.Handler) http.Handler { ) func(http.Handler) http.Handler {
+7 -4
View File
@@ -7,6 +7,7 @@ import (
"github.com/go-chi/chi" "github.com/go-chi/chi"
httpmetrics "github.com/slok/go-http-metrics/metrics" httpmetrics "github.com/slok/go-http-metrics/metrics"
prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
ghmm "github.com/slok/go-http-metrics/middleware" ghmm "github.com/slok/go-http-metrics/middleware"
"github.com/slok/go-http-metrics/middleware/std" "github.com/slok/go-http-metrics/middleware/std"
) )
@@ -151,14 +152,16 @@ func (r boundedLabelRecorder) AddInflightRequests(
var _ httpmetrics.Recorder = boundedLabelRecorder{} var _ httpmetrics.Recorder = boundedLabelRecorder{}
// Metrics returns middleware that records Prometheus HTTP metrics on // Metrics returns middleware that records Prometheus HTTP metrics on
// the registry the /metrics route serves. Every call shares the one // the default registry, which is the one the /metrics route gathers.
// recorder New built, so any number of routers can install it.
func (s *Middleware) Metrics() func(http.Handler) http.Handler { func (s *Middleware) Metrics() func(http.Handler) http.Handler {
return metricsMiddleware(s.metricsRecorder) return metricsMiddleware(
prommetrics.NewRecorder(prommetrics.Config{}),
)
} }
// metricsMiddleware builds the recording middleware against a given // metricsMiddleware builds the recording middleware against a given
// recorder, so tests can gather from a registry of their own. // recorder, so tests can gather from a registry of their own instead
// of the process-wide default.
func metricsMiddleware( func metricsMiddleware(
rec httpmetrics.Recorder, rec httpmetrics.Recorder,
) func(http.Handler) http.Handler { ) func(http.Handler) http.Handler {
+3 -2
View File
@@ -57,8 +57,9 @@ const (
// Server.setupWebhookRoutes inside it. That ordering is the whole // Server.setupWebhookRoutes inside it. That ordering is the whole
// defect, so a test that flattens it would prove nothing. // defect, so a test that flattens it would prove nothing.
// //
// The recorder writes to a registry of the test's own, so each test // The recorder writes to a registry of the test's own rather than the
// observes only its own traffic. // process-wide default one, so each test observes only its own
// traffic.
func metricsTestRouter( func metricsTestRouter(
t *testing.T, t *testing.T,
receiverLimit int, receiverLimit int,
+4 -17
View File
@@ -13,9 +13,6 @@ import (
"github.com/go-chi/chi" "github.com/go-chi/chi"
"github.com/go-chi/chi/middleware" "github.com/go-chi/chi/middleware"
"github.com/go-chi/cors" "github.com/go-chi/cors"
"github.com/prometheus/client_golang/prometheus"
httpmetrics "github.com/slok/go-http-metrics/metrics"
prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
"go.uber.org/fx" "go.uber.org/fx"
"sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/globals"
@@ -151,11 +148,10 @@ const (
type MiddlewareParams struct { type MiddlewareParams struct {
fx.In fx.In
Logger *logger.Logger Logger *logger.Logger
Globals *globals.Globals Globals *globals.Globals
Config *config.Config Config *config.Config
Session *session.Session Session *session.Session
Registry *prometheus.Registry
} }
// Middleware provides HTTP middleware for logging, CORS, auth, and // Middleware provides HTTP middleware for logging, CORS, auth, and
@@ -165,12 +161,6 @@ type Middleware struct {
params *MiddlewareParams params *MiddlewareParams
session *session.Session session *session.Session
// metricsRecorder records the inbound HTTP metrics on the
// registry /metrics serves. It is built once, in New, because
// building it registers its collectors, and a second
// registration on the same registry panics; see Metrics.
metricsRecorder httpmetrics.Recorder
// loginGuard counts failed credential verifications and bounds // loginGuard counts failed credential verifications and bounds
// concurrent password hashing. It is built on first use so that // concurrent password hashing. It is built on first use so that
// every construction path gets one; see guard(). // every construction path gets one; see guard().
@@ -189,9 +179,6 @@ func New(
s.params = &params s.params = &params
s.log = params.Logger.Get() s.log = params.Logger.Get()
s.session = params.Session s.session = params.Session
s.metricsRecorder = prommetrics.NewRecorder(
prommetrics.Config{Registry: params.Registry},
)
return s, nil return s, nil
} }
-3
View File
@@ -24,7 +24,6 @@ import (
"sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/healthcheck" "sneak.berlin/go/webhooker/internal/healthcheck"
"sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/metrics"
"sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/middleware"
"sneak.berlin/go/webhooker/internal/resetpw" "sneak.berlin/go/webhooker/internal/resetpw"
"sneak.berlin/go/webhooker/internal/session" "sneak.berlin/go/webhooker/internal/session"
@@ -164,8 +163,6 @@ func newServerApp(
session.New, session.New,
func() delivery.Notifier { return &noopNotifier{} }, func() delivery.Notifier { return &noopNotifier{} },
func() delivery.WebhookEvictor { return &noopEvictor{} }, func() delivery.WebhookEvictor { return &noopEvictor{} },
metrics.NewRegistry,
metrics.New,
middleware.New, middleware.New,
delivery.NewGuard, delivery.NewGuard,
handlers.New, handlers.New,
+7 -1
View File
@@ -7,6 +7,7 @@ import (
sentryhttp "github.com/getsentry/sentry-go/http" sentryhttp "github.com/getsentry/sentry-go/http"
"github.com/go-chi/chi" "github.com/go-chi/chi"
"github.com/go-chi/chi/middleware" "github.com/go-chi/chi/middleware"
"github.com/prometheus/client_golang/prometheus/promhttp"
"sneak.berlin/go/webhooker/static" "sneak.berlin/go/webhooker/static"
) )
@@ -129,7 +130,12 @@ func (s *Server) setupRoutes() {
if s.params.Config.MetricsAuthEnabled() { if s.params.Config.MetricsAuthEnabled() {
s.router.Group(func(r chi.Router) { s.router.Group(func(r chi.Router) {
r.Use(s.mw.MetricsAuth()) r.Use(s.mw.MetricsAuth())
r.Get("/metrics", s.h.HandleMetrics()) r.Get(
"/metrics",
http.HandlerFunc(
promhttp.Handler().ServeHTTP,
),
)
}) })
} }
+69 -46
View File
@@ -7,6 +7,7 @@ import (
"net/http/httptest" "net/http/httptest"
"net/url" "net/url"
"regexp" "regexp"
"slices"
"strconv" "strconv"
"strings" "strings"
"testing" "testing"
@@ -23,7 +24,6 @@ import (
"sneak.berlin/go/webhooker/internal/handlers" "sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/healthcheck" "sneak.berlin/go/webhooker/internal/healthcheck"
"sneak.berlin/go/webhooker/internal/logger" "sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/metrics"
"sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/middleware"
"sneak.berlin/go/webhooker/internal/server" "sneak.berlin/go/webhooker/internal/server"
"sneak.berlin/go/webhooker/internal/session" "sneak.berlin/go/webhooker/internal/session"
@@ -113,8 +113,6 @@ func newTestEnvWithConfig(
session.New, session.New,
func() delivery.Notifier { return &noopNotifier{} }, func() delivery.Notifier { return &noopNotifier{} },
func() delivery.WebhookEvictor { return &noopEvictor{} }, func() delivery.WebhookEvictor { return &noopEvictor{} },
metrics.NewRegistry,
metrics.New,
middleware.New, middleware.New,
delivery.NewGuard, delivery.NewGuard,
handlers.New, handlers.New,
@@ -223,9 +221,21 @@ func (e *testEnv) csrfFrom(
// out of the markup has to be unescaped before it is submitted. // out of the markup has to be unescaped before it is submitted.
token := html.UnescapeString(match[1]) token := html.UnescapeString(match[1])
combined := make([]*http.Cookie, 0, len(cookies)) // A cookie the page sets replaces the one of the same name, as in
combined = append(combined, cookies...) // a browser. Sent both, the server would read the first, older one.
combined = append(combined, w.Result().Cookies()...) set := w.Result().Cookies()
combined := make([]*http.Cookie, 0, len(cookies)+len(set))
for _, c := range cookies {
replaced := slices.ContainsFunc(set, func(n *http.Cookie) bool {
return n.Name == c.Name
})
if !replaced {
combined = append(combined, c)
}
}
combined = append(combined, set...)
return token, combined return token, combined
} }
@@ -617,6 +627,59 @@ func TestPagesLogin_CorrectPasswordSurvivesASpentBudget(
) )
} }
// TestPagesLogin_CookiesFromAnEarlierDatabase is
// https://git.eeqj.de/sneak/webhooker/issues/359. A new database
// brings a new session key, and the operator's browser still holds
// the session and CSRF cookies signed with the old one. Logging in
// must work as from a fresh browser and leave cookies the new key
// accepts.
func TestPagesLogin_CookiesFromAnEarlierDatabase(t *testing.T) {
t.Parallel()
const (
username = "operator"
password = "correct-horse-battery-staple"
)
earlier := newTestEnv(t)
earlierID, _ := earlier.seedUser(t, username, password)
_, stale := earlier.csrfFrom(t, "/pages/login", nil)
stale = append(stale, earlier.authCookies(t, earlierID, username)...)
env := newTestEnv(t)
env.seedUser(t, username, password)
token, cookies := env.csrfFrom(t, "/pages/login", stale)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("username", username)
form.Set("password", password)
w := env.post("/pages/login", form, cookies)
require.Equal(
t, http.StatusSeeOther, w.Code,
"a session cookie from another key must not fail the login",
)
// The response deletes the old session cookie and then sets the
// new one; a browser keeps the last.
var fresh *http.Cookie
for _, c := range w.Result().Cookies() {
if c.Name == session.SessionName {
fresh = c
}
}
require.NotNil(t, fresh, "login must set a session cookie")
assert.Equal(
t, "/sources",
env.get("/", []*http.Cookie{fresh}).Header().Get("Location"),
"the new session cookie must authenticate",
)
}
// --- /user/{username} group --- // --- /user/{username} group ---
// TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged
@@ -964,43 +1027,3 @@ func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
}) })
} }
} }
// TestTwoMetricsRoutersInOneProcess pins
// https://git.eeqj.de/sneak/webhooker/issues/227: a second
// metrics-enabled router in one process used to panic, because the
// HTTP metrics registered on Prometheus's global default registry.
// Two routers are built over separate dependency graphs and a third
// over the first graph again, and each must still serve the HTTP,
// delivery and Go runtime series.
func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
t.Parallel()
first := newTestEnvWithConfig(
t, metricsConfig(t, metricsUser, metricsAuthValue),
)
second := newTestEnvWithConfig(
t, metricsConfig(t, metricsUser, metricsAuthValue),
)
third := &testEnv{
router: server.NewRouterForTest(
first.log.Get(), first.cfg, first.mw, first.hnd,
),
}
for _, env := range []*testEnv{first, second, third} {
env.get("/", nil)
scrape := env.metricsRequest(metricsUser, metricsAuthValue)
require.Equal(t, http.StatusOK, scrape.Code)
for _, series := range []string{
"http_request_duration_seconds",
"http_response_size_bytes",
"http_requests_inflight",
"webhooker_events_received_total",
"go_goroutines",
} {
assert.Contains(t, scrape.Body.String(), series)
}
}
}
+8 -7
View File
@@ -19,8 +19,8 @@ import (
) )
// The tests below exercise the securecookie codecs underneath the // The tests below exercise the securecookie codecs underneath the
// store and nothing else: Session.Get only decodes, so no server-side // store and nothing else: they decode through the store itself, so no
// expiry check takes part in the result. They exist because // server-side expiry check takes part in the result. They exist because
// NewCookieStore gives its codecs a 30-day max age that assigning // NewCookieStore gives its codecs a 30-day max age that assigning
// store.Options does not override, which would let the codec accept a // store.Options does not override, which would let the codec accept a
// cookie weeks past the cap the cookie attribute advertises. // cookie weeks past the cap the cookie attribute advertises.
@@ -75,10 +75,11 @@ func restamp(
return base64.URLEncoding.EncodeToString(payload) return base64.URLEncoding.EncodeToString(payload)
} }
// decodeCookie feeds value back through the store's decode path. // decodeCookie feeds value back through the store's decode path. It
// asks the store rather than Session.Get, which treats a cookie that
// does not decode as absent and so hides the codec's reason.
func decodeCookie( func decodeCookie(
t *testing.T, t *testing.T,
s *session.Session,
value string, value string,
) (*sessions.Session, error) { ) (*sessions.Session, error) {
t.Helper() t.Helper()
@@ -94,7 +95,7 @@ func decodeCookie(
SameSite: http.SameSiteLaxMode, SameSite: http.SameSiteLaxMode,
}) })
sess, err := s.Get(req) sess, err := session.NewStore(testKey()).Get(req, session.SessionName)
require.NotNil(t, sess) require.NotNil(t, sess)
return sess, err return sess, err
@@ -105,7 +106,7 @@ func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) {
s := testSession(t) s := testSession(t)
sess, err := decodeCookie(t, s, restamp( sess, err := decodeCookie(t, restamp(
t, t,
issuedCookie(t, s), issuedCookie(t, s),
time.Now().Add(-(testAbsoluteMaxAge-time.Hour)), time.Now().Add(-(testAbsoluteMaxAge-time.Hour)),
@@ -126,7 +127,7 @@ func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) {
s := testSession(t) s := testSession(t)
sess, err := decodeCookie(t, s, restamp( sess, err := decodeCookie(t, restamp(
t, t,
issuedCookie(t, s), issuedCookie(t, s),
time.Now().Add(-(testAbsoluteMaxAge+time.Hour)), time.Now().Add(-(testAbsoluteMaxAge+time.Hour)),
+13 -1
View File
@@ -224,10 +224,22 @@ func New(
} }
// Get retrieves a session for the request. // Get retrieves a session for the request.
//
// A session cookie that does not decode -- one signed with an earlier
// session key, say, because the database was made anew -- is treated
// as absent: the caller gets a new, empty session and no error, and
// the next save replaces the cookie.
func (s *Session) Get( func (s *Session) Get(
r *http.Request, r *http.Request,
) (*sessions.Session, error) { ) (*sessions.Session, error) {
return s.store.Get(r, SessionName) sess, err := s.store.Get(r, SessionName)
if sess == nil {
return nil, err
}
// For a cookie that does not decode, gorilla/sessions returns a
// new, empty session alongside the error that is dropped here.
return sess, nil
} }
// GetKey returns the raw 32-byte authentication key used for // GetKey returns the raw 32-byte authentication key used for