Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
3050c2e3b5 | ||
|
|
dcb26a239f | ||
|
|
b12e204d1f |
@@ -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` |
|
||||
| `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` |
|
||||
| `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) |
|
||||
| `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) |
|
||||
| `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
|
||||
@@ -379,41 +379,48 @@ unlocked.
|
||||
`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
|
||||
host), for example `192.168.1.7, 2001:db8::5`. It decides whose
|
||||
`X-Forwarded-For` header the rate limiters believe, so it should name
|
||||
the addresses of your reverse proxies and nothing else.
|
||||
`X-Forwarded-For` header the rate limiters believe, so it should cover
|
||||
the addresses of your reverse proxies.
|
||||
|
||||
`X-Forwarded-For` is honoured **only** when the connecting peer is
|
||||
inside one of these blocks; for every other peer the client identity is
|
||||
the connection's own address and the header is ignored. The default is
|
||||
the empty list, which trusts nobody — anything else would let any
|
||||
client pick its own rate limit bucket, minting a fresh one per request
|
||||
or draining someone else's. Set it to the address of your reverse
|
||||
proxy, and to nothing wider. A set but unparseable value aborts
|
||||
startup.
|
||||
the connection's own address and the header is ignored. Unset (or
|
||||
empty), the list is the RFC 1918 private ranges: `10.0.0.0/8`,
|
||||
`172.16.0.0/12` and `192.168.0.0/16`. That covers a reverse proxy
|
||||
reaching webhooker over a Docker network or a private LAN without
|
||||
anything set. A set value replaces the default entirely. A set but
|
||||
unparseable value aborts startup.
|
||||
|
||||
That default is safe against forged headers, but leaving it unset in
|
||||
production has a cost you must know about. Production runs behind a
|
||||
TLS-terminating reverse proxy, so with `TRUSTED_PROXIES` unset every
|
||||
request keys on the proxy's own address and all clients share a single
|
||||
Trusting those ranges has two consequences for clients with private
|
||||
addresses:
|
||||
|
||||
- Any such client, whether it connects directly or through the proxy,
|
||||
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,
|
||||
and the login endpoint's failure counting collapses onto one key, so a
|
||||
stranger's wrong passwords throttle every other client's wrong
|
||||
passwords.
|
||||
passwords. Set `TRUSTED_PROXIES` to that proxy's address to restore
|
||||
per-client buckets.
|
||||
|
||||
What it cannot do is lock the operator out. The login endpoint
|
||||
verifies credentials **before** it consults any limit and charges only
|
||||
failures, so a correct password is never throttled no matter how full
|
||||
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.
|
||||
Reverse proxies append to `X-Forwarded-For` but forward other client
|
||||
headers verbatim, so a single-valued header is client-controlled even
|
||||
@@ -435,14 +442,14 @@ Two operator requirements follow:
|
||||
(nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`,
|
||||
Caddy and AWS ALB by default), and must append a bare address with
|
||||
no port.
|
||||
- List proxy hosts **only**. Any address inside `TRUSTED_PROXIES`
|
||||
- Keep clients out of the list. Any address inside `TRUSTED_PROXIES`
|
||||
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
|
||||
bucket each time, or name another client's address to drain that
|
||||
client's bucket. Never list a block that also covers clients — a
|
||||
broad `10.0.0.0/8` on a network where clients live in the same range
|
||||
makes all three limits, including the unauthenticated webhook
|
||||
receiver, silently bypassable by every client in the block.
|
||||
client's bucket. A block that also covers clients — the default, on
|
||||
a network where clients have private addresses — makes all three
|
||||
limits, including the unauthenticated webhook receiver, silently
|
||||
bypassable by every client in the block.
|
||||
|
||||
#### Sessions
|
||||
|
||||
@@ -764,10 +771,16 @@ repository's `Dockerfile` and runs it. The app needs:
|
||||
|
||||
- **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).
|
||||
- `TRUSTED_PROXIES`: Docker networks use private addresses, so the
|
||||
default covers your reverse proxy on that network. 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, 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
|
||||
`BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to
|
||||
`/var/lib/webhooker`.
|
||||
@@ -830,12 +843,14 @@ reports.
|
||||
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
|
||||
per limit and the receiver's per-IP limits become service-wide
|
||||
ceilings. See [Trusted proxies](#trusted-proxies). List the proxy
|
||||
and nothing else.
|
||||
3. **Make sure `TRUSTED_PROXIES` covers the proxy's address.** Unset,
|
||||
it covers the RFC 1918 private ranges, so a proxy on a Docker
|
||||
network or a private LAN is covered and one on loopback is not. For
|
||||
a proxy it does not cover, every rate limiter keys on the connecting
|
||||
peer, which is the proxy on every request: all clients collapse into
|
||||
one global bucket per limit and the receiver's per-IP limits become
|
||||
service-wide ceilings. See [Trusted proxies](#trusted-proxies). If
|
||||
any clients have private addresses, list the proxy and nothing else.
|
||||
4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the
|
||||
port. webhooker's Origin/Referer check compares against the host it
|
||||
was given, so on any port other than 443 `$host` makes every form
|
||||
@@ -1396,10 +1411,11 @@ It uses:
|
||||
- **[go-chi/httprate](https://github.com/go-chi/httprate)** for
|
||||
sliding-window rate limiting of the password-change and webhook
|
||||
receiver endpoints. The bucket is per client IP only when
|
||||
`TRUSTED_PROXIES` names the reverse proxy; unset, every client
|
||||
behind that proxy shares one bucket per limit. The login endpoint
|
||||
counts failed attempts itself instead, so that a correct password is
|
||||
never throttled (see [Rate Limiting](#rate-limiting))
|
||||
`TRUSTED_PROXIES` covers the reverse proxy (by default it covers the
|
||||
RFC 1918 private ranges); otherwise every client behind that proxy
|
||||
shares one bucket per limit. The login endpoint counts failed
|
||||
attempts itself instead, so that a correct password is never
|
||||
throttled (see [Rate Limiting](#rate-limiting))
|
||||
- **[Prometheus](https://prometheus.io)** for metrics, served at
|
||||
`/metrics` behind basic auth
|
||||
- **[Sentry](https://sentry.io)** for optional error reporting
|
||||
@@ -2580,30 +2596,30 @@ let one subscriber rotate source addresses and mint a fresh bucket per
|
||||
request, evading these limits at the network layer without spoofing
|
||||
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
|
||||
they carry. See [Trusted proxies](#trusted-proxies). Deployed without that
|
||||
variable set, a client behind a reverse proxy shares one bucket with
|
||||
every other client behind the same proxy. Set `TRUSTED_PROXIES` to the
|
||||
proxy's address to get per-client limits back. What the shared bucket
|
||||
they carry. See [Trusted proxies](#trusted-proxies). When that variable
|
||||
does not cover the reverse proxy, a client behind it shares one bucket
|
||||
with every other client behind the same proxy. Set `TRUSTED_PROXIES` to
|
||||
the 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
|
||||
opposite directions:
|
||||
|
||||
- For the **receiver** limits it costs throughput, which is the safe
|
||||
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
|
||||
than for the per-entrypoint one: with `TRUSTED_PROXIES` unset behind
|
||||
the reverse proxy a production deployment is required to run behind,
|
||||
every request keys on the proxy, so the aggregate limit becomes a
|
||||
service-wide ceiling of 1200 requests per minute across all senders
|
||||
and all entrypoints, where the per-entrypoint limit's capacity still
|
||||
grows with the number of entrypoints. Any deployment with more than a
|
||||
handful of busy entrypoints must set `TRUSTED_PROXIES`.
|
||||
than for the per-entrypoint one: when `TRUSTED_PROXIES` does not
|
||||
cover the reverse proxy a production deployment is required to run
|
||||
behind, every request keys on the proxy, so the aggregate limit
|
||||
becomes a service-wide ceiling of 1200 requests per minute across all
|
||||
senders and all entrypoints, where the per-entrypoint limit's
|
||||
capacity still grows with the number of entrypoints. Any deployment
|
||||
with more than a handful of busy entrypoints must make sure
|
||||
`TRUSTED_PROXIES` covers its proxy.
|
||||
- For the **login and password-change** limits it costs precision, not
|
||||
availability. Login failures from every client land in one counter,
|
||||
so a stranger's wrong passwords make the operator's own wrong
|
||||
passwords answer `429` sooner; the operator's _correct_ password is
|
||||
never affected, because it is never counted. Production deployments
|
||||
should still set `TRUSTED_PROXIES`; webhooker warns at startup
|
||||
whenever it is empty, in any environment.
|
||||
should still make sure `TRUSTED_PROXIES` covers their proxy.
|
||||
|
||||
#### The login endpoint
|
||||
|
||||
@@ -2706,8 +2722,10 @@ re-fills both verification slots on its first two requests. The
|
||||
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
|
||||
applied without reintroducing the lockout, because the proxy sees the
|
||||
real client address. Setting `TRUSTED_PROXIES` does not stop the
|
||||
saturation, but it makes the source visible in the failure logs.
|
||||
real client address. `TRUSTED_PROXIES` does not stop the saturation.
|
||||
The flood's source is in the proxy's access log: webhooker's own 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
|
||||
enforced in the webhook handler) can layer on top of this env-level
|
||||
@@ -2721,7 +2739,7 @@ abuse limit later; they are tracked as future work.
|
||||
| ------ | --------------------------- | ----------- |
|
||||
| `GET` | `/` | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
|
||||
| `GET` | `/.well-known/healthcheck` | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
|
||||
| `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` |
|
||||
| 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` |
|
||||
| `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
|
||||
@@ -3064,10 +3082,9 @@ check, see [The login endpoint](#the-login-endpoint).
|
||||
It runs behind session auth, so only a client already holding a
|
||||
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
|
||||
`TRUSTED_PROXIES` names the reverse proxy; unset, every client
|
||||
`TRUSTED_PROXIES` covers the reverse proxy; otherwise every client
|
||||
shares one bucket, which costs precision rather than availability
|
||||
(see [Rate Limiting](#rate-limiting)). webhooker warns at startup
|
||||
whenever `TRUSTED_PROXIES` is empty
|
||||
(see [Rate Limiting](#rate-limiting))
|
||||
- Prometheus metrics behind basic auth
|
||||
- Static assets embedded in binary (no filesystem access needed at
|
||||
runtime)
|
||||
|
||||
@@ -16,7 +16,6 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
"sneak.berlin/go/webhooker/internal/healthcheck"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
"sneak.berlin/go/webhooker/internal/metrics"
|
||||
"sneak.berlin/go/webhooker/internal/middleware"
|
||||
"sneak.berlin/go/webhooker/internal/resetpw"
|
||||
"sneak.berlin/go/webhooker/internal/server"
|
||||
@@ -178,10 +177,6 @@ func newApp() *fx.App {
|
||||
healthcheck.New,
|
||||
session.New,
|
||||
handlers.New,
|
||||
// The registry /metrics serves, and the delivery
|
||||
// collectors registered on it.
|
||||
metrics.NewRegistry,
|
||||
metrics.New,
|
||||
middleware.New,
|
||||
// The one SSRF guard both target-creation validation
|
||||
// and the delivery dialer consult, so they cannot
|
||||
|
||||
+22
-60
@@ -75,6 +75,11 @@ const (
|
||||
// internet-exposed endpoint.
|
||||
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
|
||||
// bound (at least 1) is enforced by envPositiveInt.
|
||||
maxPort = 65535
|
||||
@@ -172,13 +177,14 @@ type Config struct {
|
||||
|
||||
// TrustedProxies is the set of networks whose members are
|
||||
// allowed to speak for the client with X-Forwarded-For, the
|
||||
// only forwarded header read. It is empty unless
|
||||
// TRUSTED_PROXIES is set, and empty means no peer is
|
||||
// trusted: forwarded headers are then ignored entirely and
|
||||
// clients are identified by the connection's own address.
|
||||
// Members can choose their own rate-limit key, so this must
|
||||
// name proxy hosts only, never a block that also covers
|
||||
// clients.
|
||||
// only forwarded header read. Unless TRUSTED_PROXIES is set it
|
||||
// is the RFC 1918 private ranges (defaultTrustedProxies).
|
||||
// Other peers' forwarded headers are ignored and they are
|
||||
// identified by the connection's own address. Under the
|
||||
// default any client with a private address, directly or
|
||||
// through a proxy, can choose its own rate-limit key, so
|
||||
// where any clients have private addresses this must be set
|
||||
// to the proxy hosts alone.
|
||||
TrustedProxies []netip.Prefix
|
||||
|
||||
// AllowedEgressCIDRs is the set of networks a delivery target
|
||||
@@ -460,14 +466,15 @@ func parseCIDR(entry string) (netip.Prefix, error) {
|
||||
|
||||
// envPrefixList returns the value of the named environment variable
|
||||
// parsed as a comma-separated list of CIDR blocks (bare addresses
|
||||
// allowed). An unset, empty, or blank value yields an empty list. A
|
||||
// set value containing an unparseable entry is a hard error naming
|
||||
// the key and the bad entry, so startup fails loudly rather than
|
||||
// silently running with a list the operator did not intend.
|
||||
func envPrefixList(key string) ([]netip.Prefix, error) {
|
||||
// allowed). An unset, empty, or blank value is read as defaultValue
|
||||
// instead. A set value containing an unparseable entry is a hard
|
||||
// error naming the key and the bad entry, so startup fails loudly
|
||||
// rather than silently running with a list the operator did not
|
||||
// intend.
|
||||
func envPrefixList(key, defaultValue string) ([]netip.Prefix, error) {
|
||||
v := strings.TrimSpace(os.Getenv(key))
|
||||
if v == "" {
|
||||
return nil, nil
|
||||
v = defaultValue
|
||||
}
|
||||
|
||||
var prefixes []netip.Prefix
|
||||
@@ -681,12 +688,12 @@ func loadFromEnv() (*Config, error) {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
trustedProxies, err := envPrefixList("TRUSTED_PROXIES")
|
||||
trustedProxies, err := envPrefixList("TRUSTED_PROXIES", defaultTrustedProxies)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS")
|
||||
allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS", "")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -760,50 +767,6 @@ 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.
|
||||
//
|
||||
//nolint:revive // lc parameter is required by fx even if unused.
|
||||
@@ -849,7 +812,6 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
"hasMetricsAuth", s.MetricsAuthEnabled(),
|
||||
)
|
||||
|
||||
s.warnSharedRateLimitBucket(log)
|
||||
s.warnEgressAllowlist(log)
|
||||
|
||||
return s, nil
|
||||
|
||||
+14
-101
@@ -551,6 +551,11 @@ func testReceiverRateLimitSuccess(
|
||||
}
|
||||
|
||||
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 {
|
||||
name string
|
||||
set bool
|
||||
@@ -559,18 +564,21 @@ func TestTrustedProxies(t *testing.T) {
|
||||
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,
|
||||
set: false,
|
||||
expected: []string{},
|
||||
expected: defaultProxies,
|
||||
},
|
||||
{
|
||||
name: "blank value trusts nothing",
|
||||
name: "blank value uses default",
|
||||
set: true,
|
||||
value: " ",
|
||||
expected: []string{},
|
||||
expected: defaultProxies,
|
||||
},
|
||||
{
|
||||
name: "set value replaces the default entirely",
|
||||
set: true,
|
||||
value: "203.0.113.7",
|
||||
expected: []string{"203.0.113.7/32"},
|
||||
},
|
||||
{
|
||||
name: caseValidValueParsed,
|
||||
@@ -845,101 +853,6 @@ 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
|
||||
// environment for a single METRICS_ variable. A variable that is
|
||||
// set to the empty string and one that is not set at all are
|
||||
|
||||
@@ -6,21 +6,6 @@ import "log/slog"
|
||||
// the external config_test package so each helper can be covered by
|
||||
// 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
|
||||
// environment and emits its egress-allowlist startup warning to
|
||||
// log, so a test can assert both that the warning fires only when
|
||||
|
||||
@@ -148,7 +148,6 @@ type EngineParams struct {
|
||||
DBManager *database.WebhookDBManager
|
||||
Logger *logger.Logger
|
||||
SSRFGuard *Guard
|
||||
Metrics *metrics.Set
|
||||
}
|
||||
|
||||
// Engine processes queued deliveries in the background
|
||||
@@ -168,10 +167,10 @@ type Engine struct {
|
||||
retryCh chan Task
|
||||
workers int
|
||||
|
||||
// mtr is the delivery metric set. Production wires the one
|
||||
// registered on the registry /metrics serves; a test can
|
||||
// substitute a set registered on a registry it holds, so it can
|
||||
// gather what its own deliveries recorded.
|
||||
// mtr is the delivery metric set. Production wires the
|
||||
// process-wide one; a test can substitute a set registered on
|
||||
// a private registry so its assertions are not disturbed by
|
||||
// deliveries other tests are making at the same time.
|
||||
mtr *metrics.Set
|
||||
|
||||
// targets maps each target type to its implementation.
|
||||
@@ -205,7 +204,7 @@ func New(
|
||||
deliveryCh: make(chan Task, deliveryChannelSize),
|
||||
retryCh: make(chan Task, retryChannelSize),
|
||||
workers: defaultWorkers,
|
||||
mtr: params.Metrics,
|
||||
mtr: metrics.Default(),
|
||||
}
|
||||
|
||||
e.initTargets(&http.Client{
|
||||
|
||||
@@ -9,7 +9,6 @@ import (
|
||||
"net/url"
|
||||
"time"
|
||||
|
||||
"github.com/prometheus/client_golang/prometheus"
|
||||
"go.uber.org/fx"
|
||||
"gorm.io/gorm"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
@@ -390,7 +389,7 @@ func NewTestEngine(
|
||||
deliveryCh: make(chan Task, deliveryChannelSize),
|
||||
retryCh: make(chan Task, retryChannelSize),
|
||||
workers: workers,
|
||||
mtr: metrics.New(prometheus.NewRegistry()),
|
||||
mtr: metrics.Default(),
|
||||
}
|
||||
e.initTargets(client)
|
||||
|
||||
@@ -405,7 +404,7 @@ func NewTestEngineSmallRetry(
|
||||
e := &Engine{
|
||||
log: log,
|
||||
retryCh: make(chan Task, 1),
|
||||
mtr: metrics.New(prometheus.NewRegistry()),
|
||||
mtr: metrics.Default(),
|
||||
}
|
||||
e.initTargets(nil)
|
||||
|
||||
@@ -428,7 +427,7 @@ func NewTestEngineWithDB(
|
||||
deliveryCh: make(chan Task, deliveryChannelSize),
|
||||
retryCh: make(chan Task, retryChannelSize),
|
||||
workers: workers,
|
||||
mtr: metrics.New(prometheus.NewRegistry()),
|
||||
mtr: metrics.Default(),
|
||||
}
|
||||
e.initTargets(client)
|
||||
|
||||
@@ -436,7 +435,8 @@ func NewTestEngineWithDB(
|
||||
}
|
||||
|
||||
// ExportSetMetrics substitutes the engine's metric set, so a test can
|
||||
// assert on collectors registered on a registry it holds.
|
||||
// assert on collectors registered on a private registry instead of
|
||||
// the process-wide ones every other test is also moving.
|
||||
func (e *Engine) ExportSetMetrics(mtr *metrics.Set) {
|
||||
e.mtr = mtr
|
||||
}
|
||||
|
||||
@@ -35,8 +35,9 @@ const (
|
||||
)
|
||||
|
||||
// mIsolate gives the setup's engine a metric set registered on a
|
||||
// registry this test holds, so its exact assertions can gather from
|
||||
// it.
|
||||
// private registry. The process-wide collectors are moved by every
|
||||
// other delivery test running in parallel, so exact assertions are
|
||||
// only possible against a registry this test owns.
|
||||
func mIsolate(
|
||||
t *testing.T, s iSetup,
|
||||
) *prometheus.Registry {
|
||||
|
||||
@@ -103,9 +103,10 @@ func (h *Handlers) renderLoginError(
|
||||
// The credential check runs BEFORE any rate-limit budget is
|
||||
// consulted, and only a failed check spends budget. That is what
|
||||
// keeps the single administrative path reachable: behind the reverse
|
||||
// proxy this deployment requires, with TRUSTED_PROXIES unset, every
|
||||
// client shares one bucket, so a limiter spent on arrival lets any
|
||||
// stranger deny the operator's own correct password indefinitely.
|
||||
// proxy this deployment requires, when TRUSTED_PROXIES does not cover
|
||||
// it, every client shares one bucket, so a limiter spent on arrival
|
||||
// lets any stranger deny the operator's own correct password
|
||||
// indefinitely.
|
||||
//
|
||||
// Verifying first means every login POST costs an Argon2id hash, so
|
||||
// the work is taken under a bounded number of verification slots.
|
||||
|
||||
@@ -25,7 +25,7 @@ const (
|
||||
|
||||
// sharedProxyPeer is the whole point of this file. Production is
|
||||
// required to run behind a TLS-terminating reverse proxy, and
|
||||
// TRUSTED_PROXIES defaults to empty, so every client — attacker
|
||||
// when TRUSTED_PROXIES does not cover it every client — attacker
|
||||
// and operator alike — reaches the process from the proxy's
|
||||
// address and shares one rate-limit bucket. Both parties in
|
||||
// these tests therefore use the same RemoteAddr.
|
||||
@@ -115,11 +115,11 @@ func floodFailures(
|
||||
// done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150.
|
||||
//
|
||||
// The attacker and the operator share one rate-limit bucket, because
|
||||
// behind the mandated reverse proxy with TRUSTED_PROXIES unset every
|
||||
// client keys on the proxy's address. The attacker floods the
|
||||
// operator's own username — a single-admin product has a predictable
|
||||
// one — far past the failure limit. The operator must still be able
|
||||
// to log in with the correct password.
|
||||
// behind the mandated reverse proxy, when TRUSTED_PROXIES does not
|
||||
// cover it, every client keys on the proxy's address. The attacker
|
||||
// floods the operator's own username — a single-admin product has a
|
||||
// predictable one — far past the failure limit. The operator must
|
||||
// still be able to log in with the correct password.
|
||||
//
|
||||
// This fails if credentials stop being verified ahead of the limiter.
|
||||
func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) {
|
||||
|
||||
@@ -12,7 +12,6 @@ import (
|
||||
"net/http"
|
||||
"sync/atomic"
|
||||
|
||||
"github.com/prometheus/client_golang/prometheus"
|
||||
"go.uber.org/fx"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
"sneak.berlin/go/webhooker/internal/delivery"
|
||||
@@ -62,8 +61,6 @@ type HandlersParams struct {
|
||||
Notifier delivery.Notifier
|
||||
Evictor delivery.WebhookEvictor
|
||||
SSRFGuard *delivery.Guard
|
||||
Metrics *metrics.Set
|
||||
Registry *prometheus.Registry
|
||||
}
|
||||
|
||||
// Handlers provides HTTP handler methods for all application
|
||||
@@ -125,7 +122,7 @@ func New(
|
||||
s.mw = params.Middleware
|
||||
s.notifier = params.Notifier
|
||||
s.evictor = params.Evictor
|
||||
s.mtr = params.Metrics
|
||||
s.mtr = metrics.Default()
|
||||
s.ssrf = params.SSRFGuard
|
||||
|
||||
// Parse all page templates once at startup
|
||||
|
||||
@@ -20,7 +20,6 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
"sneak.berlin/go/webhooker/internal/healthcheck"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
"sneak.berlin/go/webhooker/internal/metrics"
|
||||
"sneak.berlin/go/webhooker/internal/middleware"
|
||||
"sneak.berlin/go/webhooker/internal/session"
|
||||
)
|
||||
@@ -110,8 +109,6 @@ func newTestApp(
|
||||
func(r *recordingEvictor) delivery.WebhookEvictor {
|
||||
return r
|
||||
},
|
||||
metrics.NewRegistry,
|
||||
metrics.New,
|
||||
middleware.New,
|
||||
delivery.NewGuard,
|
||||
handlers.New,
|
||||
|
||||
@@ -1,20 +0,0 @@
|
||||
package handlers
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
|
||||
"github.com/prometheus/client_golang/prometheus/promhttp"
|
||||
)
|
||||
|
||||
// HandleMetrics returns the Prometheus scrape handler for the
|
||||
// registry every collector in this process registers on. It is what
|
||||
// promhttp.Handler builds for the global default registry, including
|
||||
// the promhttp_metric_handler_* series that count scrapes, pointed at
|
||||
// that registry instead.
|
||||
func (s *Handlers) HandleMetrics() http.HandlerFunc {
|
||||
reg := s.params.Registry
|
||||
|
||||
return promhttp.InstrumentMetricHandler(
|
||||
reg, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
|
||||
).ServeHTTP
|
||||
}
|
||||
+20
-26
@@ -3,17 +3,17 @@
|
||||
// deliveries are attempted, how they end, how long they take, how
|
||||
// deep the queues are, and how many circuit breakers are open.
|
||||
//
|
||||
// It also builds the registry the authenticated /metrics route
|
||||
// serves. These collectors, the inbound HTTP metrics recorded in
|
||||
// internal/middleware, and the Go runtime and process collectors all
|
||||
// register on that one registry, never on Prometheus's global default.
|
||||
// The inbound HTTP metrics come from the go-http-metrics recorder in
|
||||
// internal/middleware and land on prometheus.DefaultRegisterer. These
|
||||
// collectors register there too, so both surfaces are gathered by the
|
||||
// one promhttp handler mounted on the authenticated /metrics route.
|
||||
package metrics
|
||||
|
||||
import (
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/prometheus/client_golang/prometheus"
|
||||
"github.com/prometheus/client_golang/prometheus/collectors"
|
||||
"github.com/prometheus/client_golang/prometheus/promauto"
|
||||
"sneak.berlin/go/webhooker/internal/database"
|
||||
)
|
||||
@@ -57,31 +57,25 @@ var knownTargetTypes = []database.TargetType{
|
||||
database.TargetTypeSlack,
|
||||
}
|
||||
|
||||
// NewRegistry returns the registry /metrics serves, carrying the Go
|
||||
// runtime and process collectors that Prometheus's global default
|
||||
// registry carries, so the go_* and process_* series stay in the
|
||||
// scrape.
|
||||
// defaultSet is the process-wide metric set, registered on the same
|
||||
// registry the HTTP middleware and the /metrics handler already use.
|
||||
// It is built on first use rather than in an init so that a test
|
||||
// binary that never touches metrics never registers them.
|
||||
//
|
||||
// A registry of its own, rather than the global default, is what lets
|
||||
// two dependency graphs in one process — two tests, say — each
|
||||
// register their collectors without the second registration
|
||||
// panicking.
|
||||
func NewRegistry() *prometheus.Registry {
|
||||
reg := prometheus.NewRegistry()
|
||||
reg.MustRegister(
|
||||
collectors.NewGoCollector(),
|
||||
collectors.NewProcessCollector(
|
||||
collectors.ProcessCollectorOpts{},
|
||||
),
|
||||
)
|
||||
//nolint:gochecknoglobals // one process-wide registration, by design
|
||||
var defaultSet = sync.OnceValue(func() *Set {
|
||||
return New(prometheus.DefaultRegisterer)
|
||||
})
|
||||
|
||||
return reg
|
||||
// Default returns the process-wide metric set.
|
||||
func Default() *Set {
|
||||
return defaultSet()
|
||||
}
|
||||
|
||||
// Set is one registered group of webhooker's delivery collectors.
|
||||
// Production builds one on the registry /metrics serves; tests build
|
||||
// their own against a private registry so assertions are not
|
||||
// disturbed by deliveries other tests are making concurrently.
|
||||
// Production uses the single Default set; tests build their own
|
||||
// against a private registry so assertions are not disturbed by
|
||||
// deliveries other tests are making concurrently.
|
||||
type Set struct {
|
||||
eventsReceived prometheus.Counter
|
||||
deliveryAttempts *prometheus.CounterVec
|
||||
@@ -99,7 +93,7 @@ type Set struct {
|
||||
// New registers a full set of delivery collectors on reg and returns
|
||||
// it. It panics if reg already holds them, which is the intended
|
||||
// behaviour for a duplicate registration.
|
||||
func New(reg *prometheus.Registry) *Set {
|
||||
func New(reg prometheus.Registerer) *Set {
|
||||
factory := promauto.With(reg)
|
||||
|
||||
s := &Set{
|
||||
|
||||
@@ -10,7 +10,8 @@ import (
|
||||
|
||||
// MetricsMiddlewareForTest builds the metrics recording middleware
|
||||
// against a caller-supplied recorder, so a test can gather from its
|
||||
// own Prometheus registry without building a whole Middleware.
|
||||
// own Prometheus registry rather than the process-wide default one
|
||||
// that Middleware.Metrics uses.
|
||||
func MetricsMiddlewareForTest(
|
||||
rec httpmetrics.Recorder,
|
||||
) func(http.Handler) http.Handler {
|
||||
|
||||
@@ -108,10 +108,10 @@ type failureWindow struct {
|
||||
//
|
||||
// A limiter that spends budget on arrival cannot protect a
|
||||
// single-admin product: behind the reverse proxy the deployment
|
||||
// requires, with TRUSTED_PROXIES unset, every client keys on the
|
||||
// proxy, so a stranger trickling five POSTs a minute keeps the one
|
||||
// bucket full and the operator's own correct password is answered 429
|
||||
// forever. There is no second administrative path.
|
||||
// requires, when TRUSTED_PROXIES does not cover it, every client
|
||||
// keys on the proxy, so a stranger trickling five POSTs a minute
|
||||
// keeps the one bucket full and the operator's own correct password
|
||||
// is answered 429 forever. There is no second administrative path.
|
||||
//
|
||||
// So budget is spent only by a FAILED verification. A correct
|
||||
// password is never throttled, whatever the counters say, which is
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
|
||||
"github.com/go-chi/chi"
|
||||
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"
|
||||
"github.com/slok/go-http-metrics/middleware/std"
|
||||
)
|
||||
@@ -151,14 +152,16 @@ func (r boundedLabelRecorder) AddInflightRequests(
|
||||
var _ httpmetrics.Recorder = boundedLabelRecorder{}
|
||||
|
||||
// Metrics returns middleware that records Prometheus HTTP metrics on
|
||||
// the registry the /metrics route serves. Every call shares the one
|
||||
// recorder New built, so any number of routers can install it.
|
||||
// the default registry, which is the one the /metrics route gathers.
|
||||
func (s *Middleware) Metrics() func(http.Handler) http.Handler {
|
||||
return metricsMiddleware(s.metricsRecorder)
|
||||
return metricsMiddleware(
|
||||
prommetrics.NewRecorder(prommetrics.Config{}),
|
||||
)
|
||||
}
|
||||
|
||||
// metricsMiddleware builds the recording middleware against a given
|
||||
// recorder, so tests can gather from a registry of their own.
|
||||
// recorder, so tests can gather from a registry of their own instead
|
||||
// of the process-wide default.
|
||||
func metricsMiddleware(
|
||||
rec httpmetrics.Recorder,
|
||||
) func(http.Handler) http.Handler {
|
||||
|
||||
@@ -57,8 +57,9 @@ const (
|
||||
// Server.setupWebhookRoutes inside it. That ordering is the whole
|
||||
// defect, so a test that flattens it would prove nothing.
|
||||
//
|
||||
// The recorder writes to a registry of the test's own, so each test
|
||||
// observes only its own traffic.
|
||||
// The recorder writes to a registry of the test's own rather than the
|
||||
// process-wide default one, so each test observes only its own
|
||||
// traffic.
|
||||
func metricsTestRouter(
|
||||
t *testing.T,
|
||||
receiverLimit int,
|
||||
|
||||
@@ -13,9 +13,6 @@ import (
|
||||
"github.com/go-chi/chi"
|
||||
"github.com/go-chi/chi/middleware"
|
||||
"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"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
"sneak.berlin/go/webhooker/internal/globals"
|
||||
@@ -151,11 +148,10 @@ const (
|
||||
type MiddlewareParams struct {
|
||||
fx.In
|
||||
|
||||
Logger *logger.Logger
|
||||
Globals *globals.Globals
|
||||
Config *config.Config
|
||||
Session *session.Session
|
||||
Registry *prometheus.Registry
|
||||
Logger *logger.Logger
|
||||
Globals *globals.Globals
|
||||
Config *config.Config
|
||||
Session *session.Session
|
||||
}
|
||||
|
||||
// Middleware provides HTTP middleware for logging, CORS, auth, and
|
||||
@@ -165,12 +161,6 @@ type Middleware struct {
|
||||
params *MiddlewareParams
|
||||
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
|
||||
// concurrent password hashing. It is built on first use so that
|
||||
// every construction path gets one; see guard().
|
||||
@@ -189,9 +179,6 @@ func New(
|
||||
s.params = ¶ms
|
||||
s.log = params.Logger.Get()
|
||||
s.session = params.Session
|
||||
s.metricsRecorder = prommetrics.NewRecorder(
|
||||
prommetrics.Config{Registry: params.Registry},
|
||||
)
|
||||
|
||||
return s, nil
|
||||
}
|
||||
|
||||
@@ -123,9 +123,8 @@ func bucketKey(addr netip.Addr) string {
|
||||
return prefix.String()
|
||||
}
|
||||
|
||||
// isTrustedProxy reports whether addr belongs to a network the
|
||||
// operator listed in TRUSTED_PROXIES. The list is empty by default,
|
||||
// so by default nothing is trusted.
|
||||
// isTrustedProxy reports whether addr belongs to a network in
|
||||
// TRUSTED_PROXIES, which by default is the RFC 1918 private ranges.
|
||||
func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
|
||||
for _, prefix := range m.params.Config.TrustedProxies {
|
||||
if prefix.Contains(addr) {
|
||||
|
||||
@@ -426,8 +426,8 @@ func assertSharedBucket(
|
||||
}
|
||||
|
||||
// TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test
|
||||
// this gating exists for: with no trusted proxies configured (the
|
||||
// default), a client that rotates a forwarded header on every
|
||||
// this gating exists for: from a peer that is not a trusted
|
||||
// proxy, a client that rotates a forwarded header on every
|
||||
// request must stay in one bucket. If forwarded headers were
|
||||
// trusted unconditionally, each spoofed value would mint a fresh
|
||||
// bucket and the limit would stop no one.
|
||||
|
||||
@@ -24,7 +24,6 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
"sneak.berlin/go/webhooker/internal/healthcheck"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
"sneak.berlin/go/webhooker/internal/metrics"
|
||||
"sneak.berlin/go/webhooker/internal/middleware"
|
||||
"sneak.berlin/go/webhooker/internal/resetpw"
|
||||
"sneak.berlin/go/webhooker/internal/session"
|
||||
@@ -164,8 +163,6 @@ func newServerApp(
|
||||
session.New,
|
||||
func() delivery.Notifier { return &noopNotifier{} },
|
||||
func() delivery.WebhookEvictor { return &noopEvictor{} },
|
||||
metrics.NewRegistry,
|
||||
metrics.New,
|
||||
middleware.New,
|
||||
delivery.NewGuard,
|
||||
handlers.New,
|
||||
|
||||
+16
-23
@@ -7,6 +7,7 @@ import (
|
||||
sentryhttp "github.com/getsentry/sentry-go/http"
|
||||
"github.com/go-chi/chi"
|
||||
"github.com/go-chi/chi/middleware"
|
||||
"github.com/prometheus/client_golang/prometheus/promhttp"
|
||||
"sneak.berlin/go/webhooker/static"
|
||||
)
|
||||
|
||||
@@ -91,25 +92,11 @@ func (s *Server) setupGlobalMiddleware() {
|
||||
func (s *Server) setupRoutes() {
|
||||
s.router.Get("/", s.h.HandleIndex())
|
||||
|
||||
// Static assets answer GET and HEAD only. chi's default 405
|
||||
// carries no Allow header, so this group supplies its own.
|
||||
staticFiles := http.StripPrefix(
|
||||
"/s", http.FileServer(http.FS(static.Static)),
|
||||
s.router.Mount(
|
||||
"/s",
|
||||
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) {
|
||||
// API routes will be added here.
|
||||
})
|
||||
@@ -129,7 +116,12 @@ func (s *Server) setupRoutes() {
|
||||
if s.params.Config.MetricsAuthEnabled() {
|
||||
s.router.Group(func(r chi.Router) {
|
||||
r.Use(s.mw.MetricsAuth())
|
||||
r.Get("/metrics", s.h.HandleMetrics())
|
||||
r.Get(
|
||||
"/metrics",
|
||||
http.HandlerFunc(
|
||||
promhttp.Handler().ServeHTTP,
|
||||
),
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -148,11 +140,12 @@ func (s *Server) setupPageRoutes() {
|
||||
r.Use(s.mw.NoCache())
|
||||
|
||||
// The login POST carries no pre-emptive rate limiter. Behind
|
||||
// the reverse proxy production requires, with TRUSTED_PROXIES
|
||||
// unset, every client shares one bucket, so a limiter spent
|
||||
// on arrival lets any stranger deny the operator the only
|
||||
// administrative path. The handler verifies credentials first
|
||||
// and charges only failures; see Handlers.authenticateUser.
|
||||
// the reverse proxy production requires, when TRUSTED_PROXIES
|
||||
// does not cover it, every client shares one bucket, so a
|
||||
// limiter spent on arrival lets any stranger deny the operator
|
||||
// the only administrative path. The handler verifies
|
||||
// credentials first and charges only failures; see
|
||||
// Handlers.authenticateUser.
|
||||
r.Get("/login", s.h.HandleLoginPage())
|
||||
r.Post("/login", s.h.HandleLoginSubmit())
|
||||
|
||||
|
||||
@@ -23,7 +23,6 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/handlers"
|
||||
"sneak.berlin/go/webhooker/internal/healthcheck"
|
||||
"sneak.berlin/go/webhooker/internal/logger"
|
||||
"sneak.berlin/go/webhooker/internal/metrics"
|
||||
"sneak.berlin/go/webhooker/internal/middleware"
|
||||
"sneak.berlin/go/webhooker/internal/server"
|
||||
"sneak.berlin/go/webhooker/internal/session"
|
||||
@@ -113,8 +112,6 @@ func newTestEnvWithConfig(
|
||||
session.New,
|
||||
func() delivery.Notifier { return &noopNotifier{} },
|
||||
func() delivery.WebhookEvictor { return &noopEvictor{} },
|
||||
metrics.NewRegistry,
|
||||
metrics.New,
|
||||
middleware.New,
|
||||
delivery.NewGuard,
|
||||
handlers.New,
|
||||
@@ -399,15 +396,13 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
|
||||
|
||||
// --- /s static group ---
|
||||
|
||||
// TestStaticServesOnlyGetAndHead pins the methods the static group
|
||||
// answers: GET and HEAD are served the asset, and the other methods
|
||||
// chi routes (POST, PUT, DELETE and the rest) are refused with 405
|
||||
// and an Allow header naming those two. A method chi does not route,
|
||||
// such as PROPFIND, is refused with 405 by the top-level router
|
||||
// before it reaches the static group, so it gets no Allow header.
|
||||
// The README documents this; the test is what keeps the two from
|
||||
// drifting.
|
||||
func TestStaticServesOnlyGetAndHead(t *testing.T) {
|
||||
// TestStaticServesEveryMethod pins what the static mount actually
|
||||
// answers. chi's Mount registers the handler for all methods and
|
||||
// http.FileServer only special-cases HEAD (by suppressing the body),
|
||||
// so a POST or a DELETE to an asset is served the file rather than
|
||||
// refused. The README documents this; the test is what keeps the two
|
||||
// from drifting.
|
||||
func TestStaticServesEveryMethod(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := newTestEnv(t)
|
||||
@@ -422,7 +417,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
|
||||
http.MethodPost,
|
||||
http.MethodPut,
|
||||
http.MethodDelete,
|
||||
"PROPFIND",
|
||||
} {
|
||||
t.Run(method, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
@@ -434,38 +428,18 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
|
||||
w := httptest.NewRecorder()
|
||||
env.router.ServeHTTP(w, req)
|
||||
|
||||
switch method {
|
||||
case http.MethodGet:
|
||||
assert.Equal(t, http.StatusOK, w.Code)
|
||||
assert.Equal(t, body, w.Body.Bytes(),
|
||||
"the asset itself is returned")
|
||||
case http.MethodHead:
|
||||
assert.Equal(t, http.StatusOK, w.Code)
|
||||
assert.Equal(t, http.StatusOK, w.Code,
|
||||
"static mount answers every method")
|
||||
|
||||
if method == http.MethodHead {
|
||||
assert.Empty(t, w.Body.Bytes(),
|
||||
"HEAD must not carry a body")
|
||||
case "PROPFIND":
|
||||
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",
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
assert.Equal(t, body, w.Body.Bytes(),
|
||||
"the asset itself is returned")
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -964,43 +938,3 @@ func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestTwoMetricsRoutersInOneProcess pins
|
||||
// https://git.eeqj.de/sneak/webhooker/issues/227: a second
|
||||
// metrics-enabled router in one process used to panic, because the
|
||||
// HTTP metrics registered on Prometheus's global default registry.
|
||||
// Two routers are built over separate dependency graphs and a third
|
||||
// over the first graph again, and each must still serve the HTTP,
|
||||
// delivery and Go runtime series.
|
||||
func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
first := newTestEnvWithConfig(
|
||||
t, metricsConfig(t, metricsUser, metricsAuthValue),
|
||||
)
|
||||
second := newTestEnvWithConfig(
|
||||
t, metricsConfig(t, metricsUser, metricsAuthValue),
|
||||
)
|
||||
third := &testEnv{
|
||||
router: server.NewRouterForTest(
|
||||
first.log.Get(), first.cfg, first.mw, first.hnd,
|
||||
),
|
||||
}
|
||||
|
||||
for _, env := range []*testEnv{first, second, third} {
|
||||
env.get("/", nil)
|
||||
|
||||
scrape := env.metricsRequest(metricsUser, metricsAuthValue)
|
||||
require.Equal(t, http.StatusOK, scrape.Code)
|
||||
|
||||
for _, series := range []string{
|
||||
"http_request_duration_seconds",
|
||||
"http_response_size_bytes",
|
||||
"http_requests_inflight",
|
||||
"webhooker_events_received_total",
|
||||
"go_goroutines",
|
||||
} {
|
||||
assert.Contains(t, scrape.Body.String(), series)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user