Author SHA1 Message Date
clawbot ef8e8cf398 Serve /metrics from a registry of its own (closes #227)
check / check (push) Successful in 4m2s
The HTTP metrics recorder, the delivery collectors and the Go and
process collectors now register on one prometheus.Registry that fx
provides, instead of Prometheus's global default registry, and
/metrics serves that registry. A second metrics-enabled router in one
process, or the server tests run with -count=2, no longer panics on a
duplicate registration.

The middleware builds its recorder once, in New, so installing
Metrics() on more than one router over the same graph is also safe.
The scrape keeps the same series and labels, including go_*,
process_* and promhttp_metric_handler_*.

Model: opus-5-5
2026-09-29 09:20:29 +00:00
clawbot ab63b5f777 Restrict /s/* to GET and HEAD (closes #169)
check / check (push) Successful in 3m37s
The static file server was attached with Mount, which registers every
method, so POST, PUT and DELETE on an asset were answered 200 with the
file. It is now registered for GET and HEAD only, inside a /s group
whose method-not-allowed handler answers 405 with Allow: GET, HEAD.
A method chi does not route at all, such as PROPFIND, still gets 405
from the top-level router, without Allow. The inverted test and the
README route table say the same.

Model: opus-5-5
2026-09-29 11:10:27 +02:00
sneak 9cf9cdd8eb 1.0.0 milestone: next into main (#321)
check / check (push) Successful in 8s
Reviewed-on: #321
2026-09-29 11:04:58 +02:00
24 changed files with 458 additions and 214 deletions
+61 -78
View File
@@ -147,7 +147,7 @@ TTY detection, and security headers are always applied.
| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` | | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
| `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` |
| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted. A set value replaces the default. Under the default, any client with a private address, whether it connects directly or through the proxy, can choose its own rate-limit key by sending its own `X-Forwarded-For`; if any clients have private addresses, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) |
| `ALLOWED_EGRESS_CIDRS` | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) | | `ALLOWED_EGRESS_CIDRS` | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) |
#### Allowing egress to your own network #### Allowing egress to your own network
@@ -379,48 +379,41 @@ unlocked.
`TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare
address such as `192.168.1.7` is accepted and treated as a single address such as `192.168.1.7` is accepted and treated as a single
host), for example `192.168.1.7, 2001:db8::5`. It decides whose host), for example `192.168.1.7, 2001:db8::5`. It decides whose
`X-Forwarded-For` header the rate limiters believe, so it should cover `X-Forwarded-For` header the rate limiters believe, so it should name
the addresses of your reverse proxies. the addresses of your reverse proxies and nothing else.
`X-Forwarded-For` is honoured **only** when the connecting peer is `X-Forwarded-For` is honoured **only** when the connecting peer is
inside one of these blocks; for every other peer the client identity is inside one of these blocks; for every other peer the client identity is
the connection's own address and the header is ignored. Unset (or the connection's own address and the header is ignored. The default is
empty), the list is the RFC 1918 private ranges: `10.0.0.0/8`, the empty list, which trusts nobody — anything else would let any
`172.16.0.0/12` and `192.168.0.0/16`. That covers a reverse proxy client pick its own rate limit bucket, minting a fresh one per request
reaching webhooker over a Docker network or a private LAN without or draining someone else's. Set it to the address of your reverse
anything set. A set value replaces the default entirely. A set but proxy, and to nothing wider. A set but unparseable value aborts
unparseable value aborts startup. startup.
Trusting those ranges has two consequences for clients with private That default is safe against forged headers, but leaving it unset in
addresses: production has a cost you must know about. Production runs behind a
TLS-terminating reverse proxy, so with `TRUSTED_PROXIES` unset every
- Any such client, whether it connects directly or through the proxy, request keys on the proxy's own address and all clients share a single
can choose its own rate-limit key by sending its own
`X-Forwarded-For`. A direct client's header is walked because the
client is itself trusted; behind the proxy, the client's own address
is skipped as a trusted hop when the chain is walked (below), so the
entry it wrote is taken as the client. If any of your clients have
private addresses, you must set `TRUSTED_PROXIES` to the proxy's
address alone.
- A client behind the proxy that sends no `X-Forwarded-For` of its own
shares the proxy's bucket, because its own address is skipped too.
Setting the list to the proxy's address alone gives each its own
bucket.
A proxy the list does not cover, such as nginx on the same host
reaching webhooker over loopback, is not trusted: every request through
it keys on the proxy's own address and all clients share a single
bucket per limit. The receiver limits become service-wide ceilings, bucket per limit. The receiver limits become service-wide ceilings,
and the login endpoint's failure counting collapses onto one key, so a and the login endpoint's failure counting collapses onto one key, so a
stranger's wrong passwords throttle every other client's wrong stranger's wrong passwords throttle every other client's wrong
passwords. Set `TRUSTED_PROXIES` to that proxy's address to restore passwords.
per-client buckets.
What it cannot do is lock the operator out. The login endpoint What it cannot do is lock the operator out. The login endpoint
verifies credentials **before** it consults any limit and charges only verifies credentials **before** it consults any limit and charges only
failures, so a correct password is never throttled no matter how full failures, so a correct password is never throttled no matter how full
the bucket is. See [Rate Limiting](#rate-limiting). 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,
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.
`X-Real-IP` and `True-Client-IP` are **never** read, from any peer. `X-Real-IP` and `True-Client-IP` are **never** read, from any peer.
Reverse proxies append to `X-Forwarded-For` but forward other client Reverse proxies append to `X-Forwarded-For` but forward other client
headers verbatim, so a single-valued header is client-controlled even headers verbatim, so a single-valued header is client-controlled even
@@ -442,14 +435,14 @@ Two operator requirements follow:
(nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`, (nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`,
Caddy and AWS ALB by default), and must append a bare address with Caddy and AWS ALB by default), and must append a bare address with
no port. no port.
- Keep clients out of the list. Any address inside `TRUSTED_PROXIES` - List proxy hosts **only**. Any address inside `TRUSTED_PROXIES`
chooses its own rate-limit key: its `X-Forwarded-For` is walked, so chooses its own rate-limit key: its `X-Forwarded-For` is walked, so
it can name a different address on every request to get a fresh it can name a different address on every request to get a fresh
bucket each time, or name another client's address to drain that bucket each time, or name another client's address to drain that
client's bucket. A block that also covers clients — the default, on client's bucket. Never list a block that also covers clients — a
a network where clients have private addresses — makes all three broad `10.0.0.0/8` on a network where clients live in the same range
limits, including the unauthenticated webhook receiver, silently makes all three limits, including the unauthenticated webhook
bypassable by every client in the block. receiver, silently bypassable by every client in the block.
#### Sessions #### Sessions
@@ -771,16 +764,10 @@ repository's `Dockerfile` and runs it. The app needs:
- **Environment variables:** - **Environment variables:**
- `WEBHOOKER_ENVIRONMENT=prod` - `WEBHOOKER_ENVIRONMENT=prod`
- `TRUSTED_PROXIES`: Docker networks use private addresses, so the - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker
default covers your reverse proxy on that network. Under the network. The `remoteIP` field of the `http request` log line for a
default, any client with a private address, whether it connects request that came through the proxy shows it; the health check's
directly or through the proxy, can choose its own rate-limit key own lines show `::1`. See [Trusted proxies](#trusted-proxies).
by sending its own `X-Forwarded-For`. If any clients have private
addresses, or the network's addresses are outside the RFC 1918
ranges, set it to the proxy's address there. 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 - Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets
`BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to `BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to
`/var/lib/webhooker`. `/var/lib/webhooker`.
@@ -843,14 +830,12 @@ reports.
behind a proxy means the `X-Forwarded-Proto` header. The block below behind a proxy means the `X-Forwarded-Proto` header. The block below
sets it; without it every request is read as plaintext and cookies sets it; without it every request is read as plaintext and cookies
ship without `Secure`. See [Configuration](#configuration). ship without `Secure`. See [Configuration](#configuration).
3. **Make sure `TRUSTED_PROXIES` covers the proxy's address.** Unset, 3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate
it covers the RFC 1918 private ranges, so a proxy on a Docker limiter keys on the connecting peer, which behind a proxy is the
network or a private LAN is covered and one on loopback is not. For proxy on every request: all clients collapse into one global bucket
a proxy it does not cover, every rate limiter keys on the connecting per limit and the receiver's per-IP limits become service-wide
peer, which is the proxy on every request: all clients collapse into ceilings. See [Trusted proxies](#trusted-proxies). List the proxy
one global bucket per limit and the receiver's per-IP limits become and nothing else.
service-wide ceilings. See [Trusted proxies](#trusted-proxies). If
any clients have private addresses, list the proxy and nothing else.
4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the 4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the
port. webhooker's Origin/Referer check compares against the host it port. webhooker's Origin/Referer check compares against the host it
was given, so on any port other than 443 `$host` makes every form was given, so on any port other than 443 `$host` makes every form
@@ -1411,11 +1396,10 @@ It uses:
- **[go-chi/httprate](https://github.com/go-chi/httprate)** for - **[go-chi/httprate](https://github.com/go-chi/httprate)** for
sliding-window rate limiting of the password-change and webhook sliding-window rate limiting of the password-change and webhook
receiver endpoints. The bucket is per client IP only when receiver endpoints. The bucket is per client IP only when
`TRUSTED_PROXIES` covers the reverse proxy (by default it covers the `TRUSTED_PROXIES` names the reverse proxy; unset, every client
RFC 1918 private ranges); otherwise every client behind that proxy behind that proxy shares one bucket per limit. The login endpoint
shares one bucket per limit. The login endpoint counts failed counts failed attempts itself instead, so that a correct password is
attempts itself instead, so that a correct password is never never throttled (see [Rate Limiting](#rate-limiting))
throttled (see [Rate Limiting](#rate-limiting))
- **[Prometheus](https://prometheus.io)** for metrics, served at - **[Prometheus](https://prometheus.io)** for metrics, served at
`/metrics` behind basic auth `/metrics` behind basic auth
- **[Sentry](https://sentry.io)** for optional error reporting - **[Sentry](https://sentry.io)** for optional error reporting
@@ -2596,30 +2580,30 @@ let one subscriber rotate source addresses and mint a fresh bucket per
request, evading these limits at the network layer without spoofing request, evading these limits at the network layer without spoofing
anything; the cost is that distinct clients inside one `/64` share a anything; the cost is that distinct clients inside one `/64` share a
bucket. IPv4-mapped addresses (`::ffff:1.2.3.4`) key as the IPv4 address bucket. IPv4-mapped addresses (`::ffff:1.2.3.4`) key as the IPv4 address
they carry. See [Trusted proxies](#trusted-proxies). When that variable they carry. See [Trusted proxies](#trusted-proxies). Deployed without that
does not cover the reverse proxy, a client behind it shares one bucket variable set, a client behind a reverse proxy shares one bucket with
with every other client behind the same proxy. Set `TRUSTED_PROXIES` to every other client behind the same proxy. Set `TRUSTED_PROXIES` to the
the proxy's address to get per-client limits back. What the shared bucket proxy's address to get per-client limits back. What the shared bucket
costs is not the same for every limiter, and the two cases pull in costs is not the same for every limiter, and the two cases pull in
opposite directions: opposite directions:
- For the **receiver** limits it costs throughput, which is the safe - For the **receiver** limits it costs throughput, which is the safe
direction to be wrong in: sharing can only make a limit bind sooner, direction to be wrong in: sharing can only make a limit bind sooner,
never let a sender past it. It matters more for the aggregate limit never let a sender past it. It matters more for the aggregate limit
than for the per-entrypoint one: when `TRUSTED_PROXIES` does not than for the per-entrypoint one: with `TRUSTED_PROXIES` unset behind
cover the reverse proxy a production deployment is required to run the reverse proxy a production deployment is required to run behind,
behind, every request keys on the proxy, so the aggregate limit every request keys on the proxy, so the aggregate limit becomes a
becomes a service-wide ceiling of 1200 requests per minute across all service-wide ceiling of 1200 requests per minute across all senders
senders and all entrypoints, where the per-entrypoint limit's and all entrypoints, where the per-entrypoint limit's capacity still
capacity still grows with the number of entrypoints. Any deployment grows with the number of entrypoints. Any deployment with more than a
with more than a handful of busy entrypoints must make sure handful of busy entrypoints must set `TRUSTED_PROXIES`.
`TRUSTED_PROXIES` covers its proxy.
- For the **login and password-change** limits it costs precision, not - For the **login and password-change** limits it costs precision, not
availability. Login failures from every client land in one counter, availability. Login failures from every client land in one counter,
so a stranger's wrong passwords make the operator's own wrong so a stranger's wrong passwords make the operator's own wrong
passwords answer `429` sooner; the operator's _correct_ password is passwords answer `429` sooner; the operator's _correct_ password is
never affected, because it is never counted. Production deployments never affected, because it is never counted. Production deployments
should still make sure `TRUSTED_PROXIES` covers their proxy. should still set `TRUSTED_PROXIES`; webhooker warns at startup
whenever it is empty, in any environment.
#### The login endpoint #### The login endpoint
@@ -2722,10 +2706,8 @@ re-fills both verification slots on its first two requests. The
remedies are to block the source at the reverse proxy, or to remedies are to block the source at the reverse proxy, or to
rate-limit `POST /pages/login` there — the one place a limit can be rate-limit `POST /pages/login` there — the one place a limit can be
applied without reintroducing the lockout, because the proxy sees the applied without reintroducing the lockout, because the proxy sees the
real client address. `TRUSTED_PROXIES` does not stop the saturation. real client address. Setting `TRUSTED_PROXIES` does not stop the
The flood's source is in the proxy's access log: webhooker's own logs saturation, but it makes the source visible in the failure logs.
record the proxy's address, not the client's (see
[Deployment behind a reverse proxy](#deployment-behind-a-reverse-proxy)).
Finer-grained per-webhook rate limits (configured in the web UI and Finer-grained per-webhook rate limits (configured in the web UI and
enforced in the webhook handler) can layer on top of this env-level enforced in the webhook handler) can layer on top of this env-level
@@ -2739,7 +2721,7 @@ abuse limit later; they are tracked as future work.
| ------ | --------------------------- | ----------- | | ------ | --------------------------- | ----------- |
| `GET` | `/` | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) | | `GET` | `/` | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
| `GET` | `/.well-known/healthcheck` | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) | | `GET` | `/.well-known/healthcheck` | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
| any | `/s/*` | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` | | `GET`, `HEAD` | `/s/*` | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
| `POST` | `/webhook/{uuid}` | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) | | `POST` | `/webhook/{uuid}` | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
#### Authentication Endpoints #### Authentication Endpoints
@@ -3082,9 +3064,10 @@ check, see [The login endpoint](#the-login-endpoint).
It runs behind session auth, so only a client already holding a It runs behind session auth, so only a client already holding a
valid session reaches it, and an operator throttled out of changing valid session reaches it, and an operator throttled out of changing
a password can still log in. The bucket is per client IP only when a password can still log in. The bucket is per client IP only when
`TRUSTED_PROXIES` covers the reverse proxy; otherwise every client `TRUSTED_PROXIES` names the reverse proxy; unset, every client
shares one bucket, which costs precision rather than availability shares one bucket, which costs precision rather than availability
(see [Rate Limiting](#rate-limiting)) (see [Rate Limiting](#rate-limiting)). webhooker warns at startup
whenever `TRUSTED_PROXIES` is empty
- 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)
+5
View File
@@ -16,6 +16,7 @@ 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"
@@ -177,6 +178,10 @@ 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
+60 -22
View File
@@ -75,11 +75,6 @@ const (
// internet-exposed endpoint. // internet-exposed endpoint.
defaultReceiverRateLimit = 120 defaultReceiverRateLimit = 120
// defaultTrustedProxies is TRUSTED_PROXIES when it is unset: the
// RFC 1918 private ranges, which a reverse proxy reaching the
// process over a Docker network or a private LAN connects from.
defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
// maxPort is the highest valid TCP port number. The lower // maxPort is the highest valid TCP port number. The lower
// bound (at least 1) is enforced by envPositiveInt. // bound (at least 1) is enforced by envPositiveInt.
maxPort = 65535 maxPort = 65535
@@ -177,14 +172,13 @@ type Config struct {
// TrustedProxies is the set of networks whose members are // TrustedProxies is the set of networks whose members are
// allowed to speak for the client with X-Forwarded-For, the // allowed to speak for the client with X-Forwarded-For, the
// only forwarded header read. Unless TRUSTED_PROXIES is set it // only forwarded header read. It is empty unless
// is the RFC 1918 private ranges (defaultTrustedProxies). // TRUSTED_PROXIES is set, and empty means no peer is
// Other peers' forwarded headers are ignored and they are // trusted: forwarded headers are then ignored entirely and
// identified by the connection's own address. Under the // clients are identified by the connection's own address.
// default any client with a private address, directly or // Members can choose their own rate-limit key, so this must
// through a proxy, can choose its own rate-limit key, so // name proxy hosts only, never a block that also covers
// where any clients have private addresses this must be set // clients.
// to the proxy hosts alone.
TrustedProxies []netip.Prefix TrustedProxies []netip.Prefix
// AllowedEgressCIDRs is the set of networks a delivery target // AllowedEgressCIDRs is the set of networks a delivery target
@@ -466,15 +460,14 @@ func parseCIDR(entry string) (netip.Prefix, error) {
// envPrefixList returns the value of the named environment variable // envPrefixList returns the value of the named environment variable
// parsed as a comma-separated list of CIDR blocks (bare addresses // parsed as a comma-separated list of CIDR blocks (bare addresses
// allowed). An unset, empty, or blank value is read as defaultValue // allowed). An unset, empty, or blank value yields an empty list. A
// instead. A set value containing an unparseable entry is a hard // set value containing an unparseable entry is a hard error naming
// error naming the key and the bad entry, so startup fails loudly // the key and the bad entry, so startup fails loudly rather than
// rather than silently running with a list the operator did not // silently running with a list the operator did not intend.
// intend. func envPrefixList(key string) ([]netip.Prefix, error) {
func envPrefixList(key, defaultValue string) ([]netip.Prefix, error) {
v := strings.TrimSpace(os.Getenv(key)) v := strings.TrimSpace(os.Getenv(key))
if v == "" { if v == "" {
v = defaultValue return nil, nil
} }
var prefixes []netip.Prefix var prefixes []netip.Prefix
@@ -688,12 +681,12 @@ func loadFromEnv() (*Config, error) {
return nil, err return nil, err
} }
trustedProxies, err := envPrefixList("TRUSTED_PROXIES", defaultTrustedProxies) trustedProxies, err := envPrefixList("TRUSTED_PROXIES")
if err != nil { if err != nil {
return nil, err return nil, err
} }
allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS", "") allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS")
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -767,6 +760,50 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
) )
} }
// warnSharedRateLimitBucket logs a startup warning whenever
// TRUSTED_PROXIES is empty, in any environment.
//
// With no trusted proxies every rate limiter keys on the connecting
// peer's address. Whether that is harmless or dangerous depends on
// what is in front of the process, which this code cannot observe:
// with nothing in front, the peer is the client and the limits are
// per-client as intended; behind a reverse proxy the peer is the proxy
// for every request, so all clients share one bucket per limiter.
//
// The login endpoint no longer spends budget on arrival — it verifies
// credentials first and charges only failures — so a shared bucket
// cannot deny the operator a correct password. What it does collapse
// is the failure counting: one client's wrong passwords throttle
// everyone else's wrong passwords, and the receiver's limits become
// service-wide ceilings.
//
// 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 —
// so this warns rather than failing startup or changing the key.
func (c *Config) warnSharedRateLimitBucket(log *slog.Logger) {
if len(c.TrustedProxies) > 0 {
return
}
log.Warn(
"TRUSTED_PROXIES is empty: every rate limit keys on the "+
"connecting peer's address. With nothing proxying to "+
"this process that is the client itself and the limits "+
"are per-client as intended. Behind a reverse proxy the "+
"peer is the proxy on every request, so all clients "+
"share one bucket per limit: the receiver limits become "+
"service-wide ceilings, and one client's failed logins "+
"throttle every other client's failed logins — a "+
"correct password still gets in. If anything proxies to "+
"this process, set TRUSTED_PROXIES to its address.",
"environment", c.Environment,
"trustedProxies", len(c.TrustedProxies),
)
}
// New creates a Config by reading environment variables. // New creates a Config by reading environment variables.
// //
//nolint:revive // lc parameter is required by fx even if unused. //nolint:revive // lc parameter is required by fx even if unused.
@@ -812,6 +849,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
"hasMetricsAuth", s.MetricsAuthEnabled(), "hasMetricsAuth", s.MetricsAuthEnabled(),
) )
s.warnSharedRateLimitBucket(log)
s.warnEgressAllowlist(log) s.warnEgressAllowlist(log)
return s, nil return s, nil
+101 -14
View File
@@ -551,11 +551,6 @@ func testReceiverRateLimitSuccess(
} }
func TestTrustedProxies(t *testing.T) { func TestTrustedProxies(t *testing.T) {
// Unset, the RFC 1918 private ranges are trusted, so a reverse
// proxy on a Docker network or a private LAN is covered without
// configuration.
defaultProxies := []string{cidrPrivateV4, "172.16.0.0/12", "192.168.0.0/16"}
tests := []struct { tests := []struct {
name string name string
set bool set bool
@@ -564,21 +559,18 @@ func TestTrustedProxies(t *testing.T) {
expected []string expected []string
}{ }{
{ {
// The default must be "trust nobody": an empty list
// means forwarded headers are ignored, never that
// every peer may speak for the client.
name: caseUnsetUsesDefault, name: caseUnsetUsesDefault,
set: false, set: false,
expected: defaultProxies, expected: []string{},
}, },
{ {
name: "blank value uses default", name: "blank value trusts nothing",
set: true, set: true,
value: " ", value: " ",
expected: defaultProxies, expected: []string{},
},
{
name: "set value replaces the default entirely",
set: true,
value: "203.0.113.7",
expected: []string{"203.0.113.7/32"},
}, },
{ {
name: caseValidValueParsed, name: caseValidValueParsed,
@@ -853,6 +845,101 @@ func TestEgressAllowlistWarning(t *testing.T) {
} }
} }
// TestSharedRateLimitBucketWarning covers the startup warning that
// 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, 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
environment string
trustedProxies string
expectWarning bool
}{
{
name: "prod without trusted proxies warns",
environment: config.EnvironmentProd,
expectWarning: true,
},
{
name: "prod with trusted proxies is quiet",
environment: config.EnvironmentProd,
trustedProxies: cidrPrivateV4,
expectWarning: false,
},
{
name: "dev without trusted proxies warns",
environment: config.EnvironmentDev,
expectWarning: true,
},
{
name: "dev with trusted proxies is quiet",
environment: config.EnvironmentDev,
trustedProxies: cidrPrivateV4,
expectWarning: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Cannot use t.Parallel() here because t.Setenv
// is incompatible with parallel subtests.
t.Setenv("WEBHOOKER_ENVIRONMENT", tt.environment)
if tt.trustedProxies == "" {
require.NoError(
t, os.Unsetenv("TRUSTED_PROXIES"),
)
} else {
t.Setenv("TRUSTED_PROXIES", tt.trustedProxies)
}
var buf bytes.Buffer
log := slog.New(slog.NewJSONHandler(
&buf, &slog.HandlerOptions{
Level: slog.LevelDebug,
},
))
require.NoError(
t,
config.WarnSharedRateLimitBucketForTest(log),
)
if !tt.expectWarning {
assert.Empty(t, buf.String())
return
}
logged := buf.String()
assert.Contains(t, logged, `"level":"WARN"`)
assert.Contains(t, logged, "TRUSTED_PROXIES")
assert.Contains(t, logged, "share one bucket")
assert.Contains(
t, logged, "throttle every other client's failed logins",
)
// The warning must not claim a lockout the login
// endpoint no longer permits: credentials are verified
// before any budget is spent.
assert.Contains(
t, logged, "a correct password still gets in",
)
// The text must stay accurate for a developer with
// nothing in front of the process, where an empty
// list costs nothing.
assert.Contains(
t, logged, "nothing proxying to this process",
)
})
}
}
// metricsEnv describes what one subtest below puts in the // metricsEnv describes what one subtest below puts in the
// environment for a single METRICS_ variable. A variable that is // environment for a single METRICS_ variable. A variable that is
// set to the empty string and one that is not set at all are // set to the empty string and one that is not set at all are
+15
View File
@@ -6,6 +6,21 @@ import "log/slog"
// the external config_test package so each helper can be covered by // the external config_test package so each helper can be covered by
// its own table-driven test without weakening the package API. // its own table-driven test without weakening the package API.
// WarnSharedRateLimitBucketForTest loads a Config from the current
// environment and emits its startup warnings to log. The real logger
// writes to stdout, so this lets the warning's firing condition be
// asserted against a handler the test controls.
func WarnSharedRateLimitBucketForTest(log *slog.Logger) error {
c, err := loadFromEnv()
if err != nil {
return err
}
c.warnSharedRateLimitBucket(log)
return nil
}
// WarnEgressAllowlistForTest loads a Config from the current // WarnEgressAllowlistForTest loads a Config from the current
// environment and emits its egress-allowlist startup warning to // environment and emits its egress-allowlist startup warning to
// log, so a test can assert both that the warning fires only when // log, so a test can assert both that the warning fires only when
+6 -5
View File
@@ -148,6 +148,7 @@ 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
@@ -167,10 +168,10 @@ type Engine struct {
retryCh chan Task retryCh chan Task
workers int workers int
// mtr is the delivery metric set. Production wires the // mtr is the delivery metric set. Production wires the one
// process-wide one; a test can substitute a set registered on // registered on the registry /metrics serves; a test can
// a private registry so its assertions are not disturbed by // substitute a set registered on a registry it holds, so it can
// deliveries other tests are making at the same time. // gather what its own deliveries recorded.
mtr *metrics.Set mtr *metrics.Set
// targets maps each target type to its implementation. // targets maps each target type to its implementation.
@@ -204,7 +205,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: metrics.Default(), mtr: params.Metrics,
} }
e.initTargets(&http.Client{ e.initTargets(&http.Client{
+5 -5
View File
@@ -9,6 +9,7 @@ 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"
@@ -389,7 +390,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.Default(), mtr: metrics.New(prometheus.NewRegistry()),
} }
e.initTargets(client) e.initTargets(client)
@@ -404,7 +405,7 @@ func NewTestEngineSmallRetry(
e := &Engine{ e := &Engine{
log: log, log: log,
retryCh: make(chan Task, 1), retryCh: make(chan Task, 1),
mtr: metrics.Default(), mtr: metrics.New(prometheus.NewRegistry()),
} }
e.initTargets(nil) e.initTargets(nil)
@@ -427,7 +428,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.Default(), mtr: metrics.New(prometheus.NewRegistry()),
} }
e.initTargets(client) e.initTargets(client)
@@ -435,8 +436,7 @@ 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 private registry instead of // assert on collectors registered on a registry it holds.
// 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
} }
+2 -3
View File
@@ -35,9 +35,8 @@ 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
// private registry. The process-wide collectors are moved by every // registry this test holds, so its exact assertions can gather from
// other delivery test running in parallel, so exact assertions are // it.
// 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 {
+3 -4
View File
@@ -103,10 +103,9 @@ func (h *Handlers) renderLoginError(
// The credential check runs BEFORE any rate-limit budget is // The credential check runs BEFORE any rate-limit budget is
// consulted, and only a failed check spends budget. That is what // consulted, and only a failed check spends budget. That is what
// keeps the single administrative path reachable: behind the reverse // keeps the single administrative path reachable: behind the reverse
// proxy this deployment requires, when TRUSTED_PROXIES does not cover // proxy this deployment requires, with TRUSTED_PROXIES unset, every
// it, every client shares one bucket, so a limiter spent on arrival // client shares one bucket, so a limiter spent on arrival lets any
// lets any stranger deny the operator's own correct password // stranger deny the operator's own correct password indefinitely.
// indefinitely.
// //
// Verifying first means every login POST costs an Argon2id hash, so // Verifying first means every login POST costs an Argon2id hash, so
// the work is taken under a bounded number of verification slots. // the work is taken under a bounded number of verification slots.
+6 -6
View File
@@ -25,7 +25,7 @@ const (
// sharedProxyPeer is the whole point of this file. Production is // sharedProxyPeer is the whole point of this file. Production is
// required to run behind a TLS-terminating reverse proxy, and // required to run behind a TLS-terminating reverse proxy, and
// when TRUSTED_PROXIES does not cover it every client — attacker // TRUSTED_PROXIES defaults to empty, so every client — attacker
// and operator alike — reaches the process from the proxy's // and operator alike — reaches the process from the proxy's
// address and shares one rate-limit bucket. Both parties in // address and shares one rate-limit bucket. Both parties in
// these tests therefore use the same RemoteAddr. // these tests therefore use the same RemoteAddr.
@@ -115,11 +115,11 @@ func floodFailures(
// done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150. // done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150.
// //
// The attacker and the operator share one rate-limit bucket, because // The attacker and the operator share one rate-limit bucket, because
// behind the mandated reverse proxy, when TRUSTED_PROXIES does not // behind the mandated reverse proxy with TRUSTED_PROXIES unset every
// cover it, every client keys on the proxy's address. The attacker // client keys on the proxy's address. The attacker floods the
// floods the operator's own username — a single-admin product has a // operator's own username — a single-admin product has a predictable
// predictable one — far past the failure limit. The operator must // one — far past the failure limit. The operator must still be able
// still be able to log in with the correct password. // to log in with the correct password.
// //
// This fails if credentials stop being verified ahead of the limiter. // This fails if credentials stop being verified ahead of the limiter.
func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) { func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) {
+4 -1
View File
@@ -12,6 +12,7 @@ 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"
@@ -61,6 +62,8 @@ 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
@@ -122,7 +125,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 = metrics.Default() s.mtr = params.Metrics
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,6 +20,7 @@ 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"
) )
@@ -109,6 +110,8 @@ 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
@@ -0,0 +1,20 @@
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
}
+26 -20
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.
// //
// The inbound HTTP metrics come from the go-http-metrics recorder in // It also builds the registry the authenticated /metrics route
// internal/middleware and land on prometheus.DefaultRegisterer. These // serves. These collectors, the inbound HTTP metrics recorded in
// collectors register there too, so both surfaces are gathered by the // internal/middleware, and the Go runtime and process collectors all
// one promhttp handler mounted on the authenticated /metrics route. // register on that one registry, never on Prometheus's global default.
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,25 +57,31 @@ var knownTargetTypes = []database.TargetType{
database.TargetTypeSlack, database.TargetTypeSlack,
} }
// defaultSet is the process-wide metric set, registered on the same // NewRegistry returns the registry /metrics serves, carrying the Go
// registry the HTTP middleware and the /metrics handler already use. // runtime and process collectors that Prometheus's global default
// It is built on first use rather than in an init so that a test // registry carries, so the go_* and process_* series stay in the
// binary that never touches metrics never registers them. // scrape.
// //
//nolint:gochecknoglobals // one process-wide registration, by design // A registry of its own, rather than the global default, is what lets
var defaultSet = sync.OnceValue(func() *Set { // two dependency graphs in one process — two tests, say — each
return New(prometheus.DefaultRegisterer) // register their collectors without the second registration
}) // panicking.
func NewRegistry() *prometheus.Registry {
reg := prometheus.NewRegistry()
reg.MustRegister(
collectors.NewGoCollector(),
collectors.NewProcessCollector(
collectors.ProcessCollectorOpts{},
),
)
// Default returns the process-wide metric set. return reg
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 uses the single Default set; tests build their own // Production builds one on the registry /metrics serves; tests build
// against a private registry so assertions are not disturbed by // their own against a private registry so assertions are not
// deliveries other tests are making concurrently. // disturbed by deliveries other tests are making concurrently.
type Set struct { type Set struct {
eventsReceived prometheus.Counter eventsReceived prometheus.Counter
deliveryAttempts *prometheus.CounterVec deliveryAttempts *prometheus.CounterVec
@@ -93,7 +99,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.Registerer) *Set { func New(reg *prometheus.Registry) *Set {
factory := promauto.With(reg) factory := promauto.With(reg)
s := &Set{ s := &Set{
+1 -2
View File
@@ -10,8 +10,7 @@ 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 rather than the process-wide default one // own Prometheus registry without building a whole Middleware.
// that Middleware.Metrics uses.
func MetricsMiddlewareForTest( func MetricsMiddlewareForTest(
rec httpmetrics.Recorder, rec httpmetrics.Recorder,
) func(http.Handler) http.Handler { ) func(http.Handler) http.Handler {
+4 -4
View File
@@ -108,10 +108,10 @@ type failureWindow struct {
// //
// A limiter that spends budget on arrival cannot protect a // A limiter that spends budget on arrival cannot protect a
// single-admin product: behind the reverse proxy the deployment // single-admin product: behind the reverse proxy the deployment
// requires, when TRUSTED_PROXIES does not cover it, every client // requires, with TRUSTED_PROXIES unset, every client keys on the
// keys on the proxy, so a stranger trickling five POSTs a minute // proxy, so a stranger trickling five POSTs a minute keeps the one
// keeps the one bucket full and the operator's own correct password // bucket full and the operator's own correct password is answered 429
// is answered 429 forever. There is no second administrative path. // forever. There is no second administrative path.
// //
// So budget is spent only by a FAILED verification. A correct // So budget is spent only by a FAILED verification. A correct
// password is never throttled, whatever the counters say, which is // password is never throttled, whatever the counters say, which is
+4 -7
View File
@@ -7,7 +7,6 @@ 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"
) )
@@ -152,16 +151,14 @@ 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 default registry, which is the one the /metrics route gathers. // the registry the /metrics route serves. Every call shares the one
// 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( return metricsMiddleware(s.metricsRecorder)
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 instead // recorder, so tests can gather from a registry of their own.
// 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 {
+2 -3
View File
@@ -57,9 +57,8 @@ 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 rather than the // The recorder writes to a registry of the test's own, so each test
// process-wide default one, so each test observes only its own // observes only its own traffic.
// traffic.
func metricsTestRouter( func metricsTestRouter(
t *testing.T, t *testing.T,
receiverLimit int, receiverLimit int,
+17 -4
View File
@@ -13,6 +13,9 @@ 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"
@@ -148,10 +151,11 @@ 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
@@ -161,6 +165,12 @@ 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().
@@ -179,6 +189,9 @@ 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 -2
View File
@@ -123,8 +123,9 @@ func bucketKey(addr netip.Addr) string {
return prefix.String() return prefix.String()
} }
// isTrustedProxy reports whether addr belongs to a network in // isTrustedProxy reports whether addr belongs to a network the
// TRUSTED_PROXIES, which by default is the RFC 1918 private ranges. // operator listed in TRUSTED_PROXIES. The list is empty by default,
// so by default nothing is trusted.
func (m *Middleware) isTrustedProxy(addr netip.Addr) bool { func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
for _, prefix := range m.params.Config.TrustedProxies { for _, prefix := range m.params.Config.TrustedProxies {
if prefix.Contains(addr) { if prefix.Contains(addr) {
+2 -2
View File
@@ -426,8 +426,8 @@ func assertSharedBucket(
} }
// TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test // TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test
// this gating exists for: from a peer that is not a trusted // this gating exists for: with no trusted proxies configured (the
// proxy, a client that rotates a forwarded header on every // default), a client that rotates a forwarded header on every
// request must stay in one bucket. If forwarded headers were // request must stay in one bucket. If forwarded headers were
// trusted unconditionally, each spoofed value would mint a fresh // trusted unconditionally, each spoofed value would mint a fresh
// bucket and the limit would stop no one. // bucket and the limit would stop no one.
+3
View File
@@ -24,6 +24,7 @@ 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"
@@ -163,6 +164,8 @@ 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,
+23 -16
View File
@@ -7,7 +7,6 @@ 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"
) )
@@ -92,11 +91,25 @@ func (s *Server) setupGlobalMiddleware() {
func (s *Server) setupRoutes() { func (s *Server) setupRoutes() {
s.router.Get("/", s.h.HandleIndex()) s.router.Get("/", s.h.HandleIndex())
s.router.Mount( // Static assets answer GET and HEAD only. chi's default 405
"/s", // carries no Allow header, so this group supplies its own.
http.StripPrefix("/s", http.FileServer(http.FS(static.Static))), staticFiles := http.StripPrefix(
"/s", http.FileServer(http.FS(static.Static)),
) )
s.router.Route("/s", func(r chi.Router) {
r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Allow", "GET, HEAD")
http.Error(
w,
"Method Not Allowed",
http.StatusMethodNotAllowed,
)
})
r.Method(http.MethodGet, "/*", staticFiles)
r.Method(http.MethodHead, "/*", staticFiles)
})
s.router.Route("/api/v1", func(_ chi.Router) { s.router.Route("/api/v1", func(_ chi.Router) {
// API routes will be added here. // API routes will be added here.
}) })
@@ -116,12 +129,7 @@ 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( r.Get("/metrics", s.h.HandleMetrics())
"/metrics",
http.HandlerFunc(
promhttp.Handler().ServeHTTP,
),
)
}) })
} }
@@ -140,12 +148,11 @@ func (s *Server) setupPageRoutes() {
r.Use(s.mw.NoCache()) r.Use(s.mw.NoCache())
// The login POST carries no pre-emptive rate limiter. Behind // The login POST carries no pre-emptive rate limiter. Behind
// the reverse proxy production requires, when TRUSTED_PROXIES // the reverse proxy production requires, with TRUSTED_PROXIES
// does not cover it, every client shares one bucket, so a // unset, every client shares one bucket, so a limiter spent
// limiter spent on arrival lets any stranger deny the operator // on arrival lets any stranger deny the operator the only
// the only administrative path. The handler verifies // administrative path. The handler verifies credentials first
// credentials first and charges only failures; see // and charges only failures; see Handlers.authenticateUser.
// Handlers.authenticateUser.
r.Get("/login", s.h.HandleLoginPage()) r.Get("/login", s.h.HandleLoginPage())
r.Post("/login", s.h.HandleLoginSubmit()) r.Post("/login", s.h.HandleLoginSubmit())
+82 -16
View File
@@ -23,6 +23,7 @@ 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"
@@ -112,6 +113,8 @@ 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,
@@ -396,13 +399,15 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
// --- /s static group --- // --- /s static group ---
// TestStaticServesEveryMethod pins what the static mount actually // TestStaticServesOnlyGetAndHead pins the methods the static group
// answers. chi's Mount registers the handler for all methods and // answers: GET and HEAD are served the asset, and the other methods
// http.FileServer only special-cases HEAD (by suppressing the body), // chi routes (POST, PUT, DELETE and the rest) are refused with 405
// so a POST or a DELETE to an asset is served the file rather than // and an Allow header naming those two. A method chi does not route,
// refused. The README documents this; the test is what keeps the two // such as PROPFIND, is refused with 405 by the top-level router
// from drifting. // before it reaches the static group, so it gets no Allow header.
func TestStaticServesEveryMethod(t *testing.T) { // The README documents this; the test is what keeps the two from
// drifting.
func TestStaticServesOnlyGetAndHead(t *testing.T) {
t.Parallel() t.Parallel()
env := newTestEnv(t) env := newTestEnv(t)
@@ -417,6 +422,7 @@ func TestStaticServesEveryMethod(t *testing.T) {
http.MethodPost, http.MethodPost,
http.MethodPut, http.MethodPut,
http.MethodDelete, http.MethodDelete,
"PROPFIND",
} { } {
t.Run(method, func(t *testing.T) { t.Run(method, func(t *testing.T) {
t.Parallel() t.Parallel()
@@ -428,18 +434,38 @@ func TestStaticServesEveryMethod(t *testing.T) {
w := httptest.NewRecorder() w := httptest.NewRecorder()
env.router.ServeHTTP(w, req) env.router.ServeHTTP(w, req)
assert.Equal(t, http.StatusOK, w.Code, switch method {
"static mount answers every method") case http.MethodGet:
assert.Equal(t, http.StatusOK, w.Code)
if method == http.MethodHead { assert.Equal(t, body, w.Body.Bytes(),
"the asset itself is returned")
case http.MethodHead:
assert.Equal(t, http.StatusOK, w.Code)
assert.Empty(t, w.Body.Bytes(), assert.Empty(t, w.Body.Bytes(),
"HEAD must not carry a body") "HEAD must not carry a body")
case "PROPFIND":
return assert.Equal(
t, http.StatusMethodNotAllowed, w.Code,
)
assert.Empty(t, w.Header().Get("Allow"),
"chi refuses a method it does not route "+
"before the static group runs")
assert.NotContains(
t, w.Body.String(), string(body),
"a refused method must not get the asset",
)
default:
assert.Equal(
t, http.StatusMethodNotAllowed, w.Code,
)
assert.Equal(
t, "GET, HEAD", w.Header().Get("Allow"),
)
assert.NotContains(
t, w.Body.String(), string(body),
"a refused method must not get the asset",
)
} }
assert.Equal(t, body, w.Body.Bytes(),
"the asset itself is returned")
}) })
} }
} }
@@ -938,3 +964,43 @@ 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)
}
}
}