From a201ecd13296b718e86129bd5026842c7d87d33f Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 11:46:50 +0200 Subject: [PATCH] Deploy: main into prod (#343) Brings `prod`, which upaas deploys, up to `main` at `9cf9cdd`, the merge of https://git.eeqj.de/sneak/webhooker/pulls/321. `prod` was cut from `main` at `251cb3d` (1.0.0b1). What it deploys is everything listed in https://git.eeqj.de/sneak/webhooker/pulls/321. For running it: - With `WEBHOOKER_ENVIRONMENT` unset, the instance runs as `prod` and sends no `Access-Control-Allow-Origin: *`. - Each event database gains its new indexes the first time it is opened after the upgrade. - `webhooker_delivery_retries_total` no longer counts a circuit breaker holding back a delivery that is already `retrying`. Not in this PR yet: https://git.eeqj.de/sneak/webhooker/issues/340, in which the container sets its own data directory owner and mode before start. It is in progress on `next`. Once it reaches `main`, this PR carries it, because the PR follows `main`. Model: opus-5-5 Co-authored-by: Jeffrey Paul <1+sneak@noreply.example.org> Reviewed-on: https://git.eeqj.de/sneak/webhooker/pulls/343 Co-authored-by: clawbot <35+clawbot@noreply.example.org> --- README.md | 266 ++++++++++++----- internal/config/config.go | 35 ++- internal/config/config_test.go | 30 +- internal/database/database.go | 6 +- internal/database/event_tier_indexes_test.go | 166 +++++++++++ internal/database/model_delivery.go | 15 +- internal/database/model_delivery_result.go | 13 +- internal/database/model_event.go | 18 ++ internal/database/webhook_db_manager.go | 71 +++-- internal/database/webhook_db_manager_test.go | 52 ++++ internal/datadir/lock.go | 10 +- internal/delivery/circuit_breaker.go | 12 +- internal/delivery/circuit_breaker_test.go | 8 +- internal/delivery/engine.go | 122 ++++++-- internal/delivery/engine_lifecycle_test.go | 87 ++++++ internal/delivery/engine_test.go | 175 +++++++++++ internal/delivery/export_test.go | 46 +++ internal/delivery/inflight_test.go | 67 +++++ internal/delivery/metrics_test.go | 7 +- internal/delivery/redirect_test.go | 21 +- internal/delivery/ssrf.go | 17 +- internal/delivery/ssrf_allowlist_test.go | 35 +++ internal/delivery/target_database.go | 18 ++ .../delivery/target_database_evict_test.go | 44 +++ internal/delivery/target_headers_test.go | 3 +- internal/delivery/target_http.go | 29 +- internal/delivery/terminal_state_test.go | 279 +++++++++++++++++- internal/handlers/ui_copy_test.go | 77 +++++ internal/middleware/csrf_test.go | 5 +- internal/middleware/recoverer.go | 7 + internal/middleware/recoverer_test.go | 33 ++- internal/reqtls/reqtls.go | 2 +- internal/session/session.go | 4 +- internal/session/session_test.go | 4 +- templates/source_detail.html | 9 +- templates/target_edit.html | 2 +- 36 files changed, 1574 insertions(+), 221 deletions(-) create mode 100644 internal/database/event_tier_indexes_test.go diff --git a/README.md b/README.md index c66e5ca..5729606 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,13 @@ services, durably stores them, and delivers them to configured targets with retry support, logging, and observability. Category: infrastructure / web service. License: MIT. +Each entrypoint is a version 4 UUID served at `/webhook/{uuid}`, and +that UUID is the entrypoint's only credential. webhooker does not use +shared secrets, HMAC signatures or token headers on the receiver, and +will not add them — read +[The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret) +before deploying one. + ## Getting Started ### Prerequisites @@ -37,9 +44,9 @@ make bootstrap # Run all checks (test, lint, format check) make check -# Run in development mode. DATA_DIR defaults to /var/lib/webhooker in -# every environment, so set it (in .env or the shell) to a writable -# directory when running from a clone. +# Run the server from the clone. DATA_DIR defaults to +# /var/lib/webhooker in every environment, so set it (in .env or the +# shell) to a writable directory. DATA_DIR=./data make dev # Build Docker image @@ -85,7 +92,8 @@ them at once. A variable already present in the real environment wins over the file's value for the same name. The environment is selected by setting `WEBHOOKER_ENVIRONMENT` to `dev` -or `prod` (default: `dev`). The setting controls exactly one behavior: +or `prod` (default: `prod`; `dev` must be set explicitly). The setting +controls exactly one behavior: | Behavior | `dev` | `prod` | | -------- | ----------------------- | ---------------- | @@ -127,7 +135,7 @@ TTY detection, and security headers are always applied. | Variable | Description | Default | | ----------------------- | ----------------------------------- | -------- | -| `WEBHOOKER_ENVIRONMENT` | `dev` or `prod` | `dev` | +| `WEBHOOKER_ENVIRONMENT` | `dev` or `prod` | `prod` | | `PORT` | HTTP listen port | `8080` | | `BIND_ADDRESS` | IP address the HTTP listener binds. Loopback by default, so the cleartext listener is not published on every interface. The Docker image ships `0.0.0.0` instead. See [Bind address](#bind-address) | `127.0.0.1` (image: `0.0.0.0`) | | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | @@ -149,6 +157,11 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and the rest — are refused, which stops a target from being used to make webhooker probe the network it sits in. +Besides the private and reserved ranges, the default blocklist refuses +public cloud metadata addresses: currently only `168.63.129.16`, Azure's +WireServer, which serves an Azure VM its credentials. Because it is a +public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it. + That default is also inconvenient for the thing webhooker is mostly for: taking a public webhook and forwarding it to something on your own network. A container on the same Docker network, a box on `10.x`, a @@ -187,15 +200,16 @@ Two things this setting cannot do: the list is always an allowlist; an empty list (the default) means every private and reserved range stays refused. Note that `0.0.0.0/0` gets you most of the way there anyway, per above. -- **It cannot open link-local, or a cloud metadata endpoint that - discloses credentials or user data.** An address is on the list below - when both of these hold: the provider fixes it, so it cannot collide - with anything you run; and reaching it hands out credentials, user - data or bootstrap material. Those stay blocked no matter what you - list, including when you list them outright or list a supernet such - as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as - best effort rather than a guarantee — it is a hand-maintained list - and the caveat below the table applies: +- **It cannot open link-local, or a cloud metadata endpoint at a + non-public address that discloses credentials or user data.** An + address is on the list below when it is not a public address and both + of these hold: the provider fixes it, so it cannot collide with + anything you run; and reaching it hands out credentials, user data or + bootstrap material. Those stay blocked no matter what you list, + including when you list them outright or list a supernet such as + `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best + effort rather than a guarantee — it is a hand-maintained list and the + caveat below the table applies: | Blocked unconditionally | What it is | | ----------------------- | ---------- | @@ -234,7 +248,8 @@ Two things this setting cannot do: encodings, which the default blocklist does not match. A publicly routable metadata address is not listed here, because nothing on this list can be reopened and blocking one that way would leave you no - escape hatch at all. + escape hatch at all; Azure's `168.63.129.16` is refused by the default + blocklist instead, as described above. This list is not exhaustive of every cloud's metadata address — if yours is not here, do not allowlist the block that contains it. @@ -392,10 +407,9 @@ the bucket is. See [Rate Limiting](#rate-limiting). The remedy is to set `TRUSTED_PROXIES` to your reverse proxy's address, which restores per-client buckets. webhooker logs a warning -at startup whenever `TRUSTED_PROXIES` is empty, in every environment — -not only when `WEBHOOKER_ENVIRONMENT=prod`, because that variable -defaults to `dev` and an operator who never set it is precisely the -one at risk. The warning is informational when nothing proxies to the +at startup whenever `TRUSTED_PROXIES` is empty, in every environment, +because behind a proxy every client shares one bucket in `dev` and +`prod` alike. The warning is informational when nothing proxies to the process: with no proxy in front, the peer address is the client's own and the buckets are already per-client. See [Rate Limiting](#rate-limiting) for what each limit shares. @@ -631,7 +645,6 @@ decision: docker run -d \ -p 127.0.0.1:8080:8080 \ -v /path/to/data:/var/lib/webhooker \ - -e WEBHOOKER_ENVIRONMENT=prod \ -e BIND_ADDRESS=0.0.0.0 \ webhooker:latest ``` @@ -724,6 +737,66 @@ listing the directory and learning your webhook UUIDs from the `events-{uuid}.db` filenames — not the barrier protecting the credentials. +### Running under upaas + +[upaas](https://git.eeqj.de/sneak/upaas) builds the image from this +repository's `Dockerfile` and runs it. The app needs: + +- **Network and port:** add no port mapping in upaas. upaas publishes + every mapped port on all interfaces of the host + ([upaas issue 113](https://git.eeqj.de/sneak/upaas/issues/113)), + which would put the plain-HTTP admin UI and receiver there. Instead, + set the app's Docker Network in upaas to your reverse proxy's Docker + network; the proxy then reaches the app at `upaas-` followed by the + app name, port `8080`. Leave `PORT` unset: the image's health check + probes `8080`. +- **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:** + - `WEBHOOKER_ENVIRONMENT=prod` + - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker + network. The `remoteIP` field of the `http request` log line for a + request that came through the proxy shows it; the health check's + own lines show `::1`. See [Trusted proxies](#trusted-proxies). + - Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets + `BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to + `/var/lib/webhooker`. + - Everything else is optional; see [Configuration](#configuration). +- **Health check:** the image's own, which requests + `/.well-known/healthcheck`. upaas reads the container's health 60 + seconds after a deploy and marks the deploy failed unless it is + `healthy`. +- **First run:** the first start prints the `admin` password once, in + the banner described under [The admin account](#the-admin-account), + to the container's log. upaas names the container `upaas-` followed + by the app name, so for an app named `webhooker`: + + ```bash + docker logs upaas-webhooker + ``` + + If the password is lost, stop the container, set a new password with + the app's own image and volume, and start it again (see + [Recovering a lost admin password](#recovering-a-lost-admin-password)): + + ```bash + docker stop upaas-webhooker + docker run --rm --volumes-from upaas-webhooker \ + "$(docker inspect -f '{{.Image}}' upaas-webhooker)" \ + /app/webhooker resetpw -generate admin + docker start upaas-webhooker + ``` + ## Deployment behind a reverse proxy webhooker terminates no TLS of its own. It serves plaintext HTTP and @@ -745,17 +818,18 @@ reports. serves the admin login form and the unauthenticated receiver with no TLS at all, and the proxy in front of it changes nothing about that. -2. **Set `WEBHOOKER_ENVIRONMENT=prod`, and make sure the proxy sends - `X-Forwarded-Proto`.** These are two requirements, not one. The - environment setting decides CORS and nothing else: the default +2. **Make sure the environment is not `dev` (leave + `WEBHOOKER_ENVIRONMENT` unset or set it to `prod`), and make sure + the proxy sends `X-Forwarded-Proto`.** These are two requirements, + not one. The environment setting decides CORS and nothing else: `dev` answers every origin with `Access-Control-Allow-Origin: *` (without credentials), which a server-rendered production - deployment has no use for. Cookie `Secure` and the strict - Origin/Referer mode are **not** tied to it — they are decided per - request from the transport, which behind a proxy means the - `X-Forwarded-Proto` header. The block below sets it; without it - every request is read as plaintext and cookies ship without - `Secure`. See [Configuration](#configuration). + deployment has no use for, and `prod` — the default — disables it. + Cookie `Secure` and the strict Origin/Referer mode are **not** tied + to it — they are decided per request from the transport, which + behind a proxy means the `X-Forwarded-Proto` header. The block below + sets it; without it every request is read as plaintext and cookies + ship without `Secure`. See [Configuration](#configuration). 3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate limiter keys on the connecting peer, which behind a proxy is the proxy on every request: all clients collapse into one global bucket @@ -847,7 +921,6 @@ sent — `$scheme` above does. With that block, webhooker's environment is: ```sh -WEBHOOKER_ENVIRONMENT=prod BIND_ADDRESS=127.0.0.1 # the default; stated here to be explicit TRUSTED_PROXIES=127.0.0.1 ``` @@ -902,15 +975,10 @@ scratch file**: it holds committed transactions that are not yet in the have no readable schema at all. `-shm` is regenerable, but there is no reason to separate the two — copy the directory and you have them. -A clean shutdown closes `webhooker.db` and every `events-*.db`, which -checkpoints and removes their sidecars; a killed or crashed instance -leaves them, and they must be carried with the `.db`. **Archive -databases are different**: their handle is not closed at shutdown, so -`archive-*.db-wal` and `-shm` normally survive a clean stop and the -`-wal` can hold every row the archive has. Measured on a stopped -instance: `archive-….db` 4096 bytes with no table, its `-wal` 157 KB -holding all 8 archived events. Copying `DATA_DIR` in full is what makes -this a non-issue; copying `.db` files out of it by name is not. +A clean shutdown closes every database, which checkpoints and removes +its sidecars; a killed or crashed instance leaves them, and they must be +carried with the `.db`. An archive the service has not opened since a +crash keeps that crash's sidecars, even across a later clean stop. Configuration is **not** in `DATA_DIR` — it comes from the environment and from a `.env` file read out of the process working directory. Back @@ -985,10 +1053,9 @@ The file becomes self-contained again when the handle closes, which happens on the next write past the debounce window, when the connection pool retires the idle connection (about a minute after the last write), or at the idle archive sweep — measured, the same file was a complete -20 KB `.db` with no sidecars about a minute after its last write. -Shutdown is **not** on that list: the archive handle is not closed when -the service stops. So either move `archive-{uuid}.db` together with any -`-wal`/`-shm` beside it, or wait until there are none. +20 KB `.db` with no sidecars about a minute after its last write. A +clean stop closes it too. So either move `archive-{uuid}.db` together +with any `-wal`/`-shm` beside it, or wait until there are none. ### Restore @@ -1007,12 +1074,10 @@ the service stops. So either move `archive-{uuid}.db` together with any They are part of the database, and dropping a `-wal` silently discards every transaction it still holds. An `.backup` set will not contain any: it writes a single consolidated file per database. A - stop-and-copy set has none for `webhooker.db` or the `events-*.db`, - because a clean stop closes those and checkpoints their sidecars - away — but it will normally have them for `archive-*.db`, whose - handle stays open across shutdown, and those carry the archive's - rows. A copy salvaged from a crashed instance has them for - everything, and needs all of them. + stop-and-copy set normally has none, because a clean stop closes + every database and checkpoints its sidecars away; the exception is an + archive not opened since a crash. A copy salvaged from a crashed + instance has them for everything, and needs all of them. 4. **Fix ownership.** The container runs as the non-root `webhooker` user, UID 1000 / GID 1000. Restored files must be owned by (or @@ -1149,14 +1214,38 @@ backups at rest and restrict who can read them. ## The entrypoint URL is the authentication secret -The receiver verifies nothing about an inbound request. The UUID in an -entrypoint's URL is its credential: anyone who holds that URL can -submit events to it, and the receiver checks nothing else about the -sender. Treat an entrypoint URL the way you would treat an API token. +**The entrypoint UUID is the credential, and it is the only one.** +webhooker mints a version 4 UUID per entrypoint and serves it at +`/webhook/{uuid}`. Possession of that URL is the authentication: +anyone who holds it can submit events to the entrypoint, and the +receiver verifies nothing else about the sender. -There is no way to rotate the UUID in place. To retire one, delete the -entrypoint (or deactivate it, which answers `410`) and create a new -one, then point the sender at the new URL. +There is no shared secret, no HMAC signature, no bearer token and no +second factor on the receiver, and none will be added. This was +considered and rejected; the implementation that existed was removed +in [PR #279](https://git.eeqj.de/sneak/webhooker/pulls/279), closing +[issue #67](https://git.eeqj.de/sneak/webhooker/issues/67) and +[issue #241](https://git.eeqj.de/sneak/webhooker/issues/241). A +proposal to reintroduce any of them — including as "defence in depth" +alongside the UUID — is answered by this section. Inbound signature +headers a sender sends anyway (`X-Hub-Signature` and its +per-provider equivalents) are stored and forwarded as ordinary +headers; nothing checks them. + +What that means for an operator: + +- **The URL is a capability, so treat it as a secret.** Keep it out of + logs, ticket bodies, chat messages and screenshots. Anyone who reads + it anywhere can post events as that sender. +- **Rotating means minting a new entrypoint, not changing a key.** + There is no way to rotate the UUID in place. To retire one, delete + the entrypoint (or deactivate it, which answers `410`) and create a + new one, then point the sender at the new URL. +- **A sender that cannot be given a secret URL is a constraint on that + integration, not a reason to change this.** If a service only + supports signed payloads to a well-known URL, raise it as its own + problem — pick a different integration path, or accept that it + cannot be used. It is not grounds to reintroduce shared secrets. ## Entrypoints @@ -1166,8 +1255,15 @@ standard: normalized scripts in `script/` are the entrypoints for the development workflow. Ten of the Makefile's seventeen targets are thin shims that call them; `build`, `run`, `dev`, `deps`, `clean`, `css` and `version` are inline commands with no script behind them, though -`build` and `version` both take their value from `script/version`. We -provide: +`build` and `version` both take their value from `script/version`. + +`make check` needs the third-party browser assets in `static/`, which +are not committed, so run `make bootstrap` (or just `make assets`) once +after cloning. Without them the tests fail with a message naming that +remedy. `make check` does not fetch them itself because it must not +change any files in the repo. + +We provide: - `script/bootstrap` — install all dependencies (idempotent) - `script/setup` — make a fresh clone ready for development @@ -1476,7 +1572,7 @@ events should be forwarded. | `type` | TargetType | One of: `http`, `slack`, `database`, `log` | | `active` | boolean | Whether deliveries are enabled (default: true) | | `config` | JSON text | Type-specific configuration | -| `max_retries` | integer | Maximum retry attempts for `http` and `slack` targets (0 = fire-and-forget, >0 = retries with backoff and a circuit breaker). Ignored by `database` and `log` targets | +| `max_retries` | integer | Total delivery attempts for `http` and `slack` targets, not retries on top of the first: 0 is a single fire-and-forget attempt with no retries and no circuit breaker, and a value of N makes N attempts in all, with exponential backoff and a per-target circuit breaker. Ignored by `database` and `log` targets | | `max_queue_size` | integer | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels | **Relations:** Belongs to Webhook. Has many Deliveries. @@ -1484,12 +1580,12 @@ events should be forwarded. **Target types:** - **`http`** — Forward the event as an HTTP POST to a configured URL. - Behavior depends on `max_retries`: when `max_retries` is 0 (the - default), the target operates in fire-and-forget mode — a single - attempt with no retries and no circuit breaker. When `max_retries` is - greater than 0, failed deliveries are retried with exponential backoff - up to `max_retries` attempts, protected by a per-target circuit - breaker. + `max_retries` is the total number of delivery attempts, not retries on + top of the first: when `max_retries` is 0 (the default), the target + operates in fire-and-forget mode, a single attempt with no retries and + no circuit breaker; a value of N makes up to N attempts in all, + retrying failed deliveries with exponential backoff and protecting them + with a per-target circuit breaker. - **`slack`** — Post the event as a formatted message to a Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It is built on the same HTTP core as `http` and honours `max_retries` @@ -1669,6 +1765,28 @@ retries) is individually logged for full observability. **Relations:** Belongs to Delivery. +#### Event-tier indexes + +These indexes on the per-webhook event databases are declared in the model +tags, so `AutoMigrate` creates them on a fresh and on an existing database: + +| Table | Columns | Serves | +| ------------------ | --------------------------- | ------ | +| `deliveries` | `status`, `deleted_at` | Startup recovery, the retry and pending sweeps every 60 seconds and the queue-depth sampler every 30 seconds, which select deliveries by status | +| `deliveries` | `event_id`, `deleted_at` | The event log, which loads each event's deliveries, and retention, which selects and deletes the deliveries of expired events | +| `delivery_results` | `delivery_id`, `deleted_at` | The event log, which loads the attempts of a page's deliveries, and retention, which deletes the attempts of expired events | +| `events` | `deleted_at`, `created_at` | Retention, which selects expired events by age | +| `events` | `created_at` | Retention's delete of the expired events themselves | + +GORM's soft delete adds `deleted_at IS NULL` to these queries; retention's +deletes leave it out, but their lookups of expired rows keep it. SQLite keeps +no statistics on these tables, and without them it rates the `deleted_at` +index, which every live row matches, above an index on a column matched +against several values or compared with `<`. So every index but the last also +covers `deleted_at`. It comes second, so that retention's deletes can use the +index without it, except in `events`, where `created_at` is compared with `<` +and SQLite narrows by a `<` only on the last column it uses. + #### Common Fields Every entity except `Setting` includes these fields from `BaseModel`. @@ -1932,7 +2050,7 @@ rescans the database anyway). | ----------- | -------- | | **Closed** | Normal operation. Deliveries flow through. Consecutive failures are counted. | | **Open** | Target appears down. Deliveries are skipped and rescheduled for after the cooldown. | -| **Half-Open** | Cooldown expired. One probe delivery is allowed to test if the target has recovered. | +| **Half-Open** | Cooldown expired. One probe delivery is allowed to test if the target has recovered. Other deliveries are rescheduled for one whole cooldown later. | **Transitions:** @@ -1970,7 +2088,9 @@ operations), and log targets (stdout) do not use circuit breakers. When a circuit is open and a new delivery arrives, the engine marks the delivery as `retrying` and schedules a retry timer for after the remaining cooldown period. This ensures no deliveries are lost — they're -just delayed until the target is healthy again. +just delayed until the target is healthy again. A delivery already in +`retrying` keeps that status without another database write each time +the breaker turns it away. ### Metrics @@ -1984,7 +2104,7 @@ arriving and being stored, they are just not getting anywhere. | Metric | Type | Meaning | | ------ | ---- | ------- | | `webhooker_events_received_total` | counter | Events received and durably stored. Compare against the delivery counters on one dashboard | -| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery an open circuit breaker refused is not one: it is counted as a retry instead | +| `webhooker_delivery_attempts_total` | counter | Delivery attempts actually dispatched to a target. A delivery a circuit breaker refused is not one: it is counted as a retry instead, but only when the refusal moves it into `retrying` | | `webhooker_deliveries_succeeded_total` | counter | Deliveries that reached `delivered` | | `webhooker_deliveries_failed_total` | counter | Deliveries that failed terminally and will not be retried | | `webhooker_delivery_retries_total` | counter | Deliveries put back into `retrying` | @@ -2867,6 +2987,10 @@ check, see [The login endpoint](#the-login-endpoint). ### Authentication +- **Webhook receiver:** the entrypoint UUID in the URL, and nothing + else. No shared secret, no HMAC signature, no token header, and none + will be added — see + [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret). - **Web UI:** Cookie-based sessions using gorilla/sessions with encrypted cookies. Sessions are configured with HttpOnly, SameSite Lax, and Secure whenever the request is on TLS — the flag follows the @@ -2906,7 +3030,8 @@ check, see [The login endpoint](#the-login-endpoint). mode - **The entrypoint URL is the receiver's only credential.** Nothing about an inbound request is verified; possession of the UUID - authorises submission (see + authorises submission, and no shared secret or signature check will + be added alongside it (see [The entrypoint URL is the authentication secret](#the-entrypoint-url-is-the-authentication-secret)) - **SSRF prevention** for HTTP delivery targets: private/reserved IP ranges (RFC 1918, loopback, link-local, cloud metadata) are blocked @@ -2962,7 +3087,8 @@ each hook. The order, read off the fx stop-hook log: 3. `server` — the HTTP drain, bounded separately by `server.ShutdownTimeout` (**3 seconds**), then a Sentry flush if `SENTRY_DSN` is set -4. `delivery.Engine` +4. `delivery.Engine` — waits for its workers, then closes the archive + databases 5. `healthcheck` 6. `WebhookDBManager` 7. the database close diff --git a/internal/config/config.go b/internal/config/config.go index 6a7a628..1b9a4e7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -192,9 +192,10 @@ type Config struct { // alwaysBlockedNetworks stays blocked no matter what is listed // here. That set is link-local plus the cloud metadata // endpoints outside it that disclose credentials or user data - // at a provider-fixed address; it is not exhaustive of every - // cloud's metadata address. See alwaysBlockedNetworks for the - // authoritative list and the criterion it is built from. + // at a provider-fixed, non-public address; it is not + // exhaustive of every cloud's metadata address. See + // alwaysBlockedNetworks for the authoritative list and the + // criterion it is built from. AllowedEgressCIDRs []netip.Prefix params *ConfigParams @@ -585,12 +586,14 @@ func resolveMetricsAuth() (string, string, error) { ) } -// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to -// dev, and rejects unrecognised values. +// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to prod +// when it is unset so a deployment that forgets the variable is not +// silently permissive; dev must be set explicitly. It rejects +// unrecognised values. func resolveEnvironment() (string, error) { environment := os.Getenv("WEBHOOKER_ENVIRONMENT") if environment == "" { - environment = EnvironmentDev + environment = EnvironmentProd } if environment != EnvironmentDev && @@ -744,12 +747,14 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) { log.Warn( "ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+ - "otherwise-blocked private/reserved networks. Anyone "+ - "who can create a delivery target can now make this "+ - "process issue requests into them, and read back the "+ - "response. Link-local and the known cloud instance "+ - "metadata endpoints outside it stay blocked "+ - "regardless of what is listed here.", + "otherwise-blocked networks. Anyone who can create a "+ + "delivery target can now make this process issue "+ + "requests into them, and read back the response. Only "+ + "the addresses the README lists as blocked "+ + "unconditionally stay blocked regardless of what is "+ + "listed here; a public cloud metadata address such as "+ + "168.63.129.16 is reachable once it, or a block "+ + "covering it, is listed.", "allowedEgressCIDRs", strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","), ) @@ -772,10 +777,8 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) { // everyone else's wrong passwords, and the receiver's limits become // service-wide ceilings. // -// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT. That -// variable defaults to dev, so gating on it would silence the warning -// for exactly the operator who forgot to configure the deployment — -// the case it exists to catch. +// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT: +// behind a proxy every client shares one bucket in dev and prod alike. // // The default of trusting nobody is deliberate — trusting forwarded // headers from arbitrary peers lets any client choose its own bucket — diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f38f7fd..a7cc76e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -44,9 +44,9 @@ func TestEnvironmentConfig(t *testing.T) { isProd bool }{ { - name: "default is dev", - isDev: true, - isProd: false, + name: "default is prod", + isDev: false, + isProd: true, }, { name: "explicit dev", @@ -834,12 +834,13 @@ func TestEgressAllowlistWarning(t *testing.T) { // to be able to read back which networks are open. assert.Contains(t, logged, "10.0.0.0/8") assert.Contains(t, logged, "127.0.0.0/8") - // What stays shut. Asserted on the clause naming the - // wider set rather than on "Link-local" alone, so the - // string cannot narrow back to link-local only while - // the always-blocked set covers ULA, CGNAT and two - // public metadata addresses as well. - assert.Contains(t, logged, "metadata endpoints outside it") + // What stays shut is the whole unconditional set, not + // link-local alone; a public metadata address is not in + // it, so a listed block covering it opens it. + assert.Contains(t, logged, "blocked unconditionally") + assert.Contains(t, logged, "168.63.129.16 is reachable") + // The listed blocks need not be private or reserved. + assert.NotContains(t, logged, "private/reserved") }) } } @@ -848,10 +849,9 @@ func TestEgressAllowlistWarning(t *testing.T) { // tells an operator a deployment behind a reverse proxy shares one // rate-limit bucket between every client, which turns the receiver // limits into service-wide ceilings and collapses login failure -// counting. It must fire whenever TRUSTED_PROXIES is empty, -// in any environment: WEBHOOKER_ENVIRONMENT defaults to dev, so gating -// on it would silence the warning for exactly the operator who never -// configured the deployment. It stays quiet once proxies are named. +// counting. It must fire whenever TRUSTED_PROXIES is empty, in any +// environment, because behind a proxy every client shares one bucket +// in dev and prod alike. It stays quiet once proxies are named. func TestSharedRateLimitBucketWarning(t *testing.T) { tests := []struct { name string @@ -871,10 +871,6 @@ func TestSharedRateLimitBucketWarning(t *testing.T) { expectWarning: false, }, { - // The default environment. An internet-exposed - // deployment whose operator never set - // WEBHOOKER_ENVIRONMENT lands here and has exactly - // the exposure the warning announces. name: "dev without trusted proxies warns", environment: config.EnvironmentDev, expectWarning: true, diff --git a/internal/database/database.go b/internal/database/database.go index f2880e8..ba28bae 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -17,12 +17,12 @@ import ( "gorm.io/gorm" "sneak.berlin/go/webhooker/internal/banner" "sneak.berlin/go/webhooker/internal/config" + "sneak.berlin/go/webhooker/internal/datadir" "sneak.berlin/go/webhooker/internal/gormlog" "sneak.berlin/go/webhooker/internal/logger" ) const ( - dataDirPerm = 0750 randomPasswordLen = 16 sessionKeyLen = 32 ) @@ -185,7 +185,9 @@ func (d *Database) connect() error { // caller's decision. func (d *Database) connectTo(dataDir string) error { // Ensure the data directory exists before opening the database. - err := os.MkdirAll(dataDir, dataDirPerm) + // datadir.DirPerm is the single source of the directory mode; this + // package creates the directory too, since either may run first. + err := os.MkdirAll(dataDir, datadir.DirPerm) if err != nil { return fmt.Errorf( "creating data directory %s: %w", diff --git a/internal/database/event_tier_indexes_test.go b/internal/database/event_tier_indexes_test.go new file mode 100644 index 0000000..d25dca5 --- /dev/null +++ b/internal/database/event_tier_indexes_test.go @@ -0,0 +1,166 @@ +package database_test + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + "sneak.berlin/go/webhooker/internal/database" +) + +// TestWebhookDBManager_OpenAddsEventTierIndexes verifies that opening a +// per-webhook database that predates these indexes creates them. It +// stands in for an older database file by dropping the indexes +// AutoMigrate just created, then reopening the same file. +func TestWebhookDBManager_OpenAddsEventTierIndexes(t *testing.T) { + t.Parallel() + + indexes := []struct { + model any + name string + }{ + {&database.Delivery{}, "idx_deliveries_status"}, + {&database.Delivery{}, "idx_deliveries_event_id"}, + {&database.DeliveryResult{}, "idx_delivery_results_delivery_id"}, + {&database.Event{}, "idx_events_deleted_at_created_at"}, + {&database.Event{}, "idx_events_created_at"}, + } + + mgr, lc := setupTestWebhookDBManager(t) + ctx := context.Background() + require.NoError(t, lc.Start(ctx)) + + defer func() { require.NoError(t, lc.Stop(ctx)) }() + + webhookID := uuid.New().String() + + db, err := mgr.GetDB(webhookID) + require.NoError(t, err) + + // A fresh database has them. + for _, ix := range indexes { + require.True(t, db.Migrator().HasIndex(ix.model, ix.name)) + } + + // Stand in for a database file created before the indexes existed. + for _, ix := range indexes { + require.NoError(t, db.Migrator().DropIndex(ix.model, ix.name)) + require.False(t, db.Migrator().HasIndex(ix.model, ix.name)) + } + + // Drop the cached connection so the next open reopens the file and + // runs AutoMigrate against it, as a restart would. + require.NoError(t, mgr.CloseAll()) + + db, err = mgr.GetDB(webhookID) + require.NoError(t, err) + + for _, ix := range indexes { + assert.True(t, db.Migrator().HasIndex(ix.model, ix.name), + "opening the existing database should create %s", ix.name) + } +} + +// TestEventTierQueriesUseTheirIndexes verifies that the statements the +// indexes are for use them. GORM builds each statement in a dry run as +// the code named above it does, soft-delete condition included, and +// SQLite, which keeps no statistics on these tables, must plan to seek +// on each index listed by the columns in parentheses. +func TestEventTierQueriesUseTheirIndexes(t *testing.T) { + t.Parallel() + + mgr, lc := setupTestWebhookDBManager(t) + ctx := context.Background() + require.NoError(t, lc.Start(ctx)) + + defer func() { require.NoError(t, lc.Stop(ctx)) }() + + db, err := mgr.GetDB(uuid.New().String()) + require.NoError(t, err) + + dry := db.Session(&gorm.Session{DryRun: true}) + ids := []string{ + uuid.New().String(), uuid.New().String(), uuid.New().String(), + } + cutoff := time.Now() + + var ( + deliveries []database.Delivery + results []database.DeliveryResult + depths []struct{ Depth int } + ) + + byStatus := "idx_deliveries_status (status=? AND deleted_at=?)" + byEvent := "idx_deliveries_event_id (event_id=? AND deleted_at=?)" + byAge := "idx_events_deleted_at_created_at (deleted_at=? AND created_atTimeout (seconds, blank = default): -
- - +
+
+ + +
+

This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.

diff --git a/templates/target_edit.html b/templates/target_edit.html index 9194721..a2ff56c 100644 --- a/templates/target_edit.html +++ b/templates/target_edit.html @@ -69,7 +69,7 @@
-

0 is fire-and-forget: one attempt, no circuit breaker.

+

This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.

{{end}}