Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 clawbot
|
2ae6f4be30 | ||
|
clawbot
|
95499e2a59 |
@@ -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. 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) |
|
||||
|
||||
#### Allowing egress to your own network
|
||||
@@ -157,11 +157,6 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and
|
||||
the rest — are refused, which stops a target from being used to make
|
||||
webhooker probe the network it sits in.
|
||||
|
||||
Besides the private and reserved ranges, the default blocklist refuses
|
||||
public cloud metadata addresses: currently only `168.63.129.16`, Azure's
|
||||
WireServer, which serves an Azure VM its credentials. Because it is a
|
||||
public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
|
||||
|
||||
That default is also inconvenient for the thing webhooker is mostly
|
||||
for: taking a public webhook and forwarding it to something on your own
|
||||
network. A container on the same Docker network, a box on `10.x`, a
|
||||
@@ -200,16 +195,15 @@ Two things this setting cannot do:
|
||||
the list is always an allowlist; an empty list (the default) means
|
||||
every private and reserved range stays refused. Note that
|
||||
`0.0.0.0/0` gets you most of the way there anyway, per above.
|
||||
- **It cannot open link-local, or a cloud metadata endpoint at a
|
||||
non-public address that discloses credentials or user data.** An
|
||||
address is on the list below when it is not a public address and both
|
||||
of these hold: the provider fixes it, so it cannot collide with
|
||||
anything you run; and reaching it hands out credentials, user data or
|
||||
bootstrap material. Those stay blocked no matter what you list,
|
||||
including when you list them outright or list a supernet such as
|
||||
`0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
|
||||
effort rather than a guarantee — it is a hand-maintained list and the
|
||||
caveat below the table applies:
|
||||
- **It cannot open link-local, or a cloud metadata endpoint that
|
||||
discloses credentials or user data.** An address is on the list below
|
||||
when both of these hold: the provider fixes it, so it cannot collide
|
||||
with anything you run; and reaching it hands out credentials, user
|
||||
data or bootstrap material. Those stay blocked no matter what you
|
||||
list, including when you list them outright or list a supernet such
|
||||
as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
|
||||
best effort rather than a guarantee — it is a hand-maintained list
|
||||
and the caveat below the table applies:
|
||||
|
||||
| Blocked unconditionally | What it is |
|
||||
| ----------------------- | ---------- |
|
||||
@@ -248,8 +242,7 @@ Two things this setting cannot do:
|
||||
encodings, which the default blocklist does not match. A publicly
|
||||
routable metadata address is not listed here, because nothing on this
|
||||
list can be reopened and blocking one that way would leave you no
|
||||
escape hatch at all; Azure's `168.63.129.16` is refused by the default
|
||||
blocklist instead, as described above.
|
||||
escape hatch at all.
|
||||
|
||||
This list is not exhaustive of every cloud's metadata address — if
|
||||
yours is not here, do not allowlist the block that contains it.
|
||||
@@ -379,48 +372,41 @@ 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 cover
|
||||
the addresses of your reverse proxies.
|
||||
`X-Forwarded-For` header the rate limiters believe, so it should name
|
||||
the addresses of your reverse proxies and nothing else.
|
||||
|
||||
`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. 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.
|
||||
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.
|
||||
|
||||
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
|
||||
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
|
||||
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. Set `TRUSTED_PROXIES` to that proxy's address to restore
|
||||
per-client buckets.
|
||||
passwords.
|
||||
|
||||
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
|
||||
@@ -442,14 +428,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.
|
||||
- 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
|
||||
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. 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.
|
||||
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.
|
||||
|
||||
#### Sessions
|
||||
|
||||
@@ -771,16 +757,10 @@ repository's `Dockerfile` and runs it. The app needs:
|
||||
|
||||
- **Environment variables:**
|
||||
- `WEBHOOKER_ENVIRONMENT=prod`
|
||||
- `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).
|
||||
- `TRUSTED_PROXIES`: your reverse proxy's address on that Docker
|
||||
network. The `remoteIP` field of the `http request` log line for a
|
||||
request that came through the proxy shows it; the health check's
|
||||
own lines show `::1`. See [Trusted proxies](#trusted-proxies).
|
||||
- Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets
|
||||
`BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to
|
||||
`/var/lib/webhooker`.
|
||||
@@ -843,14 +823,12 @@ 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. **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.
|
||||
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.
|
||||
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
|
||||
@@ -990,10 +968,15 @@ scratch file**: it holds committed transactions that are not yet in the
|
||||
have no readable schema at all. `-shm` is regenerable, but there is no
|
||||
reason to separate the two — copy the directory and you have them.
|
||||
|
||||
A clean shutdown closes every database, which checkpoints and removes
|
||||
its sidecars; a killed or crashed instance leaves them, and they must be
|
||||
carried with the `.db`. An archive the service has not opened since a
|
||||
crash keeps that crash's sidecars, even across a later clean stop.
|
||||
A clean shutdown closes `webhooker.db` and every `events-*.db`, which
|
||||
checkpoints and removes their sidecars; a killed or crashed instance
|
||||
leaves them, and they must be carried with the `.db`. **Archive
|
||||
databases are different**: their handle is not closed at shutdown, so
|
||||
`archive-*.db-wal` and `-shm` normally survive a clean stop and the
|
||||
`-wal` can hold every row the archive has. Measured on a stopped
|
||||
instance: `archive-….db` 4096 bytes with no table, its `-wal` 157 KB
|
||||
holding all 8 archived events. Copying `DATA_DIR` in full is what makes
|
||||
this a non-issue; copying `.db` files out of it by name is not.
|
||||
|
||||
Configuration is **not** in `DATA_DIR` — it comes from the environment
|
||||
and from a `.env` file read out of the process working directory. Back
|
||||
@@ -1068,9 +1051,10 @@ The file becomes self-contained again when the handle closes, which
|
||||
happens on the next write past the debounce window, when the connection
|
||||
pool retires the idle connection (about a minute after the last write),
|
||||
or at the idle archive sweep — measured, the same file was a complete
|
||||
20 KB `.db` with no sidecars about a minute after its last write. A
|
||||
clean stop closes it too. So either move `archive-{uuid}.db` together
|
||||
with any `-wal`/`-shm` beside it, or wait until there are none.
|
||||
20 KB `.db` with no sidecars about a minute after its last write.
|
||||
Shutdown is **not** on that list: the archive handle is not closed when
|
||||
the service stops. So either move `archive-{uuid}.db` together with any
|
||||
`-wal`/`-shm` beside it, or wait until there are none.
|
||||
|
||||
### Restore
|
||||
|
||||
@@ -1089,10 +1073,12 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
|
||||
They are part of the database, and dropping a `-wal` silently
|
||||
discards every transaction it still holds. An `.backup` set will not
|
||||
contain any: it writes a single consolidated file per database. A
|
||||
stop-and-copy set normally has none, because a clean stop closes
|
||||
every database and checkpoints its sidecars away; the exception is an
|
||||
archive not opened since a crash. A copy salvaged from a crashed
|
||||
instance has them for everything, and needs all of them.
|
||||
stop-and-copy set has none for `webhooker.db` or the `events-*.db`,
|
||||
because a clean stop closes those and checkpoints their sidecars
|
||||
away — but it will normally have them for `archive-*.db`, whose
|
||||
handle stays open across shutdown, and those carry the archive's
|
||||
rows. A copy salvaged from a crashed instance has them for
|
||||
everything, and needs all of them.
|
||||
|
||||
4. **Fix ownership.** The container runs as the non-root `webhooker`
|
||||
user, UID 1000 / GID 1000. Restored files must be owned by (or
|
||||
@@ -1411,11 +1397,10 @@ 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` 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))
|
||||
`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))
|
||||
- **[Prometheus](https://prometheus.io)** for metrics, served at
|
||||
`/metrics` behind basic auth
|
||||
- **[Sentry](https://sentry.io)** for optional error reporting
|
||||
@@ -2596,30 +2581,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). 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
|
||||
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
|
||||
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: 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.
|
||||
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`.
|
||||
- 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 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
|
||||
|
||||
@@ -2722,10 +2707,8 @@ 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. `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)).
|
||||
real client address. Setting `TRUSTED_PROXIES` does not stop the
|
||||
saturation, but it makes the source visible in the failure logs.
|
||||
|
||||
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
|
||||
@@ -3082,9 +3065,10 @@ 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` 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
|
||||
(see [Rate Limiting](#rate-limiting))
|
||||
(see [Rate Limiting](#rate-limiting)). webhooker warns at startup
|
||||
whenever `TRUSTED_PROXIES` is empty
|
||||
- Prometheus metrics behind basic auth
|
||||
- Static assets embedded in binary (no filesystem access needed at
|
||||
runtime)
|
||||
@@ -3104,8 +3088,7 @@ each hook. The order, read off the fx stop-hook log:
|
||||
3. `server` — the HTTP drain, bounded separately by
|
||||
`server.ShutdownTimeout` (**3 seconds**), then a Sentry flush if
|
||||
`SENTRY_DSN` is set
|
||||
4. `delivery.Engine` — waits for its workers, then closes the archive
|
||||
databases
|
||||
4. `delivery.Engine`
|
||||
5. `healthcheck`
|
||||
6. `WebhookDBManager`
|
||||
7. the database close
|
||||
|
||||
+69
-34
@@ -75,11 +75,6 @@ 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
|
||||
@@ -177,14 +172,13 @@ 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. 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.
|
||||
// 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.
|
||||
TrustedProxies []netip.Prefix
|
||||
|
||||
// AllowedEgressCIDRs is the set of networks a delivery target
|
||||
@@ -198,10 +192,9 @@ type Config struct {
|
||||
// alwaysBlockedNetworks stays blocked no matter what is listed
|
||||
// here. That set is link-local plus the cloud metadata
|
||||
// endpoints outside it that disclose credentials or user data
|
||||
// at a provider-fixed, non-public address; it is not
|
||||
// exhaustive of every cloud's metadata address. See
|
||||
// alwaysBlockedNetworks for the authoritative list and the
|
||||
// criterion it is built from.
|
||||
// at a provider-fixed address; it is not exhaustive of every
|
||||
// cloud's metadata address. See alwaysBlockedNetworks for the
|
||||
// authoritative list and the criterion it is built from.
|
||||
AllowedEgressCIDRs []netip.Prefix
|
||||
|
||||
params *ConfigParams
|
||||
@@ -466,15 +459,14 @@ 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 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) {
|
||||
// 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) {
|
||||
v := strings.TrimSpace(os.Getenv(key))
|
||||
if v == "" {
|
||||
v = defaultValue
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
var prefixes []netip.Prefix
|
||||
@@ -688,12 +680,12 @@ func loadFromEnv() (*Config, error) {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
trustedProxies, err := envPrefixList("TRUSTED_PROXIES", defaultTrustedProxies)
|
||||
trustedProxies, err := envPrefixList("TRUSTED_PROXIES")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS", "")
|
||||
allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -754,19 +746,61 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
|
||||
|
||||
log.Warn(
|
||||
"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
|
||||
"otherwise-blocked networks. Anyone who can create a "+
|
||||
"delivery target can now make this process issue "+
|
||||
"requests into them, and read back the response. Only "+
|
||||
"the addresses the README lists as blocked "+
|
||||
"unconditionally stay blocked regardless of what is "+
|
||||
"listed here; a public cloud metadata address such as "+
|
||||
"168.63.129.16 is reachable once it, or a block "+
|
||||
"covering it, is listed.",
|
||||
"otherwise-blocked private/reserved networks. Anyone "+
|
||||
"who can create a delivery target can now make this "+
|
||||
"process issue requests into them, and read back the "+
|
||||
"response. Link-local and the known cloud instance "+
|
||||
"metadata endpoints outside it stay blocked "+
|
||||
"regardless of what is listed here.",
|
||||
"allowedEgressCIDRs",
|
||||
strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
|
||||
)
|
||||
}
|
||||
|
||||
// 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.
|
||||
@@ -812,6 +846,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
"hasMetricsAuth", s.MetricsAuthEnabled(),
|
||||
)
|
||||
|
||||
s.warnSharedRateLimitBucket(log)
|
||||
s.warnEgressAllowlist(log)
|
||||
|
||||
return s, nil
|
||||
|
||||
+107
-21
@@ -551,11 +551,6 @@ 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
|
||||
@@ -564,21 +559,18 @@ 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: defaultProxies,
|
||||
expected: []string{},
|
||||
},
|
||||
{
|
||||
name: "blank value uses default",
|
||||
name: "blank value trusts nothing",
|
||||
set: true,
|
||||
value: " ",
|
||||
expected: defaultProxies,
|
||||
},
|
||||
{
|
||||
name: "set value replaces the default entirely",
|
||||
set: true,
|
||||
value: "203.0.113.7",
|
||||
expected: []string{"203.0.113.7/32"},
|
||||
expected: []string{},
|
||||
},
|
||||
{
|
||||
name: caseValidValueParsed,
|
||||
@@ -842,13 +834,107 @@ func TestEgressAllowlistWarning(t *testing.T) {
|
||||
// to be able to read back which networks are open.
|
||||
assert.Contains(t, logged, "10.0.0.0/8")
|
||||
assert.Contains(t, logged, "127.0.0.0/8")
|
||||
// What stays shut is the whole unconditional set, not
|
||||
// link-local alone; a public metadata address is not in
|
||||
// it, so a listed block covering it opens it.
|
||||
assert.Contains(t, logged, "blocked unconditionally")
|
||||
assert.Contains(t, logged, "168.63.129.16 is reachable")
|
||||
// The listed blocks need not be private or reserved.
|
||||
assert.NotContains(t, logged, "private/reserved")
|
||||
// What stays shut. Asserted on the clause naming the
|
||||
// wider set rather than on "Link-local" alone, so the
|
||||
// string cannot narrow back to link-local only while
|
||||
// the always-blocked set covers ULA, CGNAT and two
|
||||
// public metadata addresses as well.
|
||||
assert.Contains(t, logged, "metadata endpoints outside it")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// 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",
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -6,6 +6,21 @@ 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
|
||||
|
||||
+23
-70
@@ -362,15 +362,6 @@ func (e *Engine) start() {
|
||||
// stop cancels the worker pool's context and waits for the pool
|
||||
// to drain, bounded by the stop hook's context: a wedged worker
|
||||
// must not hang the process past fx's stop timeout.
|
||||
//
|
||||
// Once the pool has drained it closes the archive writers, so a
|
||||
// clean stop leaves no archive -wal behind. Nothing else holds a
|
||||
// writer for long by then: the archive sweeper stops before the
|
||||
// engine, and deleting a webhook only closes one. If the pool did
|
||||
// not drain in time, the writers are left open, as a kill would
|
||||
// leave them. Closing them would wait for any write in progress,
|
||||
// and a worker still running would then open new writers that
|
||||
// nothing closes, so it gains nothing over a kill.
|
||||
func (e *Engine) stop(ctx context.Context) error {
|
||||
e.log.Info("delivery engine stopping")
|
||||
|
||||
@@ -385,8 +376,6 @@ func (e *Engine) stop(ctx context.Context) error {
|
||||
return err
|
||||
}
|
||||
|
||||
e.dbTarget.evictAll()
|
||||
|
||||
e.log.Info("delivery engine stopped")
|
||||
|
||||
return nil
|
||||
@@ -739,7 +728,9 @@ func (e *Engine) recoverSingleRetry(
|
||||
// webhook on one bad read would be a far larger fault than
|
||||
// the strand it is meant to clear.
|
||||
if errors.Is(err, gorm.ErrRecordNotFound) {
|
||||
e.failMissingTarget(webhookDB, webhookID, d)
|
||||
e.failMissingTargetRetry(
|
||||
webhookDB, webhookID, d,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
@@ -1142,7 +1133,9 @@ func (e *Engine) sweepSingleRetry(
|
||||
// Deleted is terminal, unreadable is not; see
|
||||
// recoverSingleRetry.
|
||||
if errors.Is(err, gorm.ErrRecordNotFound) {
|
||||
e.failMissingTarget(webhookDB, webhookID, d)
|
||||
e.failMissingTargetRetry(
|
||||
webhookDB, webhookID, d,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
@@ -1256,19 +1249,19 @@ func (e *Engine) failUnretryableRetry(
|
||||
e.failDelivery(webhookDB, d, target.Type, reason)
|
||||
}
|
||||
|
||||
// failMissingTarget terminally fails a recovered delivery, pending or
|
||||
// retrying, whose target row is gone. Restart recovery and the periodic
|
||||
// sweep call it for both statuses, so the transition exists once.
|
||||
// failMissingTargetRetry terminally fails an orphaned retrying
|
||||
// delivery whose target row is gone. Both restart recovery and the
|
||||
// periodic sweep call it, so the transition exists once.
|
||||
//
|
||||
// Until it existed those paths logged the failed lookup and moved on,
|
||||
// which left the delivery where it was for the life of the database and
|
||||
// Until it existed both paths logged the failed lookup and returned,
|
||||
// which left the delivery retrying for the life of the database and
|
||||
// the sweep repeating the same error every minute forever. Failing it
|
||||
// with a recorded reason is the treatment the other orphaned-retry
|
||||
// cases already get, so all of them read alike in the event log.
|
||||
//
|
||||
// Logged at warn rather than error: a deleted target is an operator
|
||||
// action, not a system fault.
|
||||
func (e *Engine) failMissingTarget(
|
||||
func (e *Engine) failMissingTargetRetry(
|
||||
webhookDB *gorm.DB,
|
||||
webhookID string,
|
||||
d *database.Delivery,
|
||||
@@ -1281,37 +1274,13 @@ func (e *Engine) failMissingTarget(
|
||||
|
||||
defer e.inflight.release(d.ID)
|
||||
|
||||
// The batch was read before ownership was taken, and a worker may
|
||||
// have settled the delivery and let it go in between. Only a row
|
||||
// still in the status the batch read is failed.
|
||||
row, err := e.loadDelivery(webhookDB, d.ID)
|
||||
if err != nil {
|
||||
e.log.Error(
|
||||
"failed to load delivery",
|
||||
"delivery_id", d.ID,
|
||||
"error", err,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
if row.Status != d.Status {
|
||||
e.log.Debug(
|
||||
"delivery already handled, not failed",
|
||||
"delivery_id", d.ID,
|
||||
"status", row.Status,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
targetType, reason := e.missingTargetReason(d.TargetID)
|
||||
|
||||
e.log.Warn(
|
||||
"failing recovered delivery: its target no longer exists",
|
||||
"failing orphaned retrying delivery: "+
|
||||
"its target no longer exists",
|
||||
"webhook_id", webhookID,
|
||||
"delivery_id", d.ID,
|
||||
"status", d.Status,
|
||||
"target_id", d.TargetID,
|
||||
"target_type", targetType,
|
||||
)
|
||||
@@ -1345,14 +1314,15 @@ func (e *Engine) missingTargetReason(
|
||||
if err != nil {
|
||||
return "", fmt.Sprintf(
|
||||
"target %s no longer exists; the delivery "+
|
||||
"has been failed terminally",
|
||||
"cannot be retried and has been failed "+
|
||||
"terminally",
|
||||
targetID,
|
||||
)
|
||||
}
|
||||
|
||||
return target.Type, fmt.Sprintf(
|
||||
"target %q (type %s) was deleted; the delivery "+
|
||||
"has been failed terminally",
|
||||
"cannot be retried and has been failed terminally",
|
||||
target.Name, target.Type,
|
||||
)
|
||||
}
|
||||
@@ -2051,30 +2021,13 @@ func (e *Engine) sendRecoveredDeliveries(
|
||||
|
||||
target, ok := targetMap[deliveries[i].TargetID]
|
||||
if !ok {
|
||||
// A missing entry does not mean the target is gone: the
|
||||
// map is also empty when its query failed. Only a lookup
|
||||
// that finds no row ends the delivery; any other error
|
||||
// leaves it pending for the next sweep. See
|
||||
// recoverSingleRetry.
|
||||
var err error
|
||||
e.log.Error(
|
||||
"target not found for delivery",
|
||||
"delivery_id", deliveries[i].ID,
|
||||
"target_id", deliveries[i].TargetID,
|
||||
)
|
||||
|
||||
target, err = e.loadTarget(deliveries[i].TargetID)
|
||||
if errors.Is(err, gorm.ErrRecordNotFound) {
|
||||
e.failMissingTarget(webhookDB, webhookID, &deliveries[i])
|
||||
|
||||
continue
|
||||
}
|
||||
|
||||
if err != nil {
|
||||
e.log.Error(
|
||||
"failed to load target for recovered delivery",
|
||||
"delivery_id", deliveries[i].ID,
|
||||
"target_id", deliveries[i].TargetID,
|
||||
"error", err,
|
||||
)
|
||||
|
||||
continue
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
if !e.takeForRedispatch(
|
||||
|
||||
@@ -2,8 +2,6 @@ package delivery_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -271,88 +269,3 @@ func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
|
||||
|
||||
requireStopHookExpires(t, lc.hooks[0], "delivery engine")
|
||||
}
|
||||
|
||||
// deliverToArchive runs one delivery to a database target through
|
||||
// the running engine and returns the webhook's archive file path.
|
||||
// The archive writer holds the file open afterwards.
|
||||
func deliverToArchive(t *testing.T, s iSetup) string {
|
||||
t.Helper()
|
||||
|
||||
deliveryID, task := seedLogTask(t, s)
|
||||
task.TargetType = database.TargetTypeDatabase
|
||||
|
||||
s.Engine.Notify([]delivery.Task{task})
|
||||
|
||||
iWaitForDelivered(t, s.WebhookDB, deliveryID)
|
||||
|
||||
return filepath.Join(
|
||||
filepath.Dir(s.DBMgr.DBPath(s.WebhookID)),
|
||||
fmt.Sprintf("archive-%s.db", s.WebhookID),
|
||||
)
|
||||
}
|
||||
|
||||
// TestEngine_StopHookClosesArchives is the regression test for an
|
||||
// archive split across two files by a clean stop. The engine never
|
||||
// closed its archive writers, so after a stop the archived rows
|
||||
// could sit in archive-{id}.db-wal while archive-{id}.db held no
|
||||
// table at all, and copying the .db on its own gave an empty
|
||||
// database.
|
||||
func TestEngine_StopHookClosesArchives(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
lc := startEngineViaHook(t, s.Engine)
|
||||
|
||||
path := deliverToArchive(t, s)
|
||||
require.FileExists(
|
||||
t, path+"-wal",
|
||||
"an open archive should have a -wal for the stop to remove",
|
||||
)
|
||||
|
||||
require.NoError(t, lc.hooks[0].OnStop(context.Background()))
|
||||
|
||||
wals, err := filepath.Glob(
|
||||
filepath.Join(filepath.Dir(path), "archive-*.db-wal"),
|
||||
)
|
||||
require.NoError(t, err)
|
||||
require.Empty(
|
||||
t, wals, "a clean stop must leave no archive -wal behind",
|
||||
)
|
||||
|
||||
// With no -wal beside it, the row can only be in the .db.
|
||||
count, err := countArchivedRows(path)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, int64(1), count)
|
||||
}
|
||||
|
||||
// TestEngine_StopHookTimeoutLeavesArchivesOpen covers a stop whose
|
||||
// budget runs out while a worker is still running. The archive
|
||||
// writers are left open, as a kill would leave them: closing them
|
||||
// would wait for any write in progress, and that worker would then
|
||||
// open new writers that nothing closes.
|
||||
func TestEngine_StopHookTimeoutLeavesArchivesOpen(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
lc := startEngineViaHook(t, s.Engine)
|
||||
|
||||
deliverToArchive(t, s)
|
||||
|
||||
release := make(chan struct{})
|
||||
|
||||
t.Cleanup(func() {
|
||||
close(release)
|
||||
s.Engine.EvictWebhook(s.WebhookID)
|
||||
})
|
||||
|
||||
s.Engine.ExportWedgeWorker(release)
|
||||
|
||||
requireStopHookExpires(t, lc.hooks[0], "delivery engine")
|
||||
|
||||
require.True(
|
||||
t, s.Engine.ExportArchiveHandleOpen(s.WebhookID),
|
||||
"a stop that timed out must not close archive writers",
|
||||
)
|
||||
}
|
||||
|
||||
@@ -342,31 +342,6 @@ func (e *Engine) ExportRecoverRetryingDeliveries(
|
||||
e.recoverRetryingDeliveries(webhookDB, webhookID)
|
||||
}
|
||||
|
||||
// ExportFailMissingTarget exposes failMissingTarget, so a test can hand
|
||||
// it a delivery as a batch read it earlier.
|
||||
func (e *Engine) ExportFailMissingTarget(
|
||||
webhookDB *gorm.DB,
|
||||
webhookID string,
|
||||
d *database.Delivery,
|
||||
) {
|
||||
e.failMissingTarget(webhookDB, webhookID, d)
|
||||
}
|
||||
|
||||
// ExportSendRecoveredDeliveries exposes sendRecoveredDeliveries, so a
|
||||
// test can hand it a target map that lacks a delivery's target.
|
||||
func (e *Engine) ExportSendRecoveredDeliveries(
|
||||
ctx context.Context,
|
||||
webhookDB *gorm.DB,
|
||||
deliveries []database.Delivery,
|
||||
webhookID string,
|
||||
targetMap map[string]database.Target,
|
||||
settled map[string]struct{},
|
||||
) {
|
||||
e.sendRecoveredDeliveries(
|
||||
ctx, webhookDB, deliveries, webhookID, targetMap, settled,
|
||||
)
|
||||
}
|
||||
|
||||
// ExportDeliveryCh returns the delivery channel.
|
||||
func (e *Engine) ExportDeliveryCh() chan Task {
|
||||
return e.deliveryCh
|
||||
|
||||
@@ -26,7 +26,7 @@ var (
|
||||
"hostname resolved to no IP addresses",
|
||||
)
|
||||
errBlockedIP = errors.New(
|
||||
"blocked private, reserved or cloud metadata address",
|
||||
"blocked private/reserved IP range",
|
||||
)
|
||||
errBlockedMetadata = errors.New(
|
||||
"blocked link-local or cloud instance metadata " +
|
||||
@@ -37,10 +37,9 @@ var (
|
||||
)
|
||||
)
|
||||
|
||||
// blockedNetworks is the default blocklist: the private and
|
||||
// reserved IP ranges, plus the public cloud metadata addresses,
|
||||
// that are blocked to prevent SSRF attacks. An operator can
|
||||
// permit specific blocks out of this set with
|
||||
// blockedNetworks contains all private/reserved IP ranges
|
||||
// that should be blocked to prevent SSRF attacks. An operator
|
||||
// can permit specific blocks out of this set with
|
||||
// ALLOWED_EGRESS_CIDRS; see Guard.
|
||||
//
|
||||
//nolint:gochecknoglobals // package-level network list is appropriate here
|
||||
@@ -123,8 +122,6 @@ func init() {
|
||||
"::1/128",
|
||||
"fc00::/7",
|
||||
"fe80::/10",
|
||||
// Azure WireServer, a public address that serves VM credentials.
|
||||
"168.63.129.16/32",
|
||||
})
|
||||
|
||||
// Every entry is named. The set must not grow or shrink
|
||||
@@ -219,8 +216,8 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
|
||||
}
|
||||
|
||||
// isBlockedIP checks whether an IP address falls within
|
||||
// the default blocklist, before any operator allowlist is
|
||||
// considered.
|
||||
// any blocked private/reserved network range, before any
|
||||
// operator allowlist is considered.
|
||||
func isBlockedIP(ip net.IP) bool {
|
||||
return matchesAny(blockedNetworks, ip)
|
||||
}
|
||||
@@ -323,7 +320,7 @@ func (g *Guard) allows(ip net.IP) bool {
|
||||
//
|
||||
// 1. alwaysBlockedNetworks is refused before the allowlist is
|
||||
// consulted, so no configured CIDR reaches link-local or a
|
||||
// cloud metadata endpoint at a non-public address.
|
||||
// cloud instance metadata endpoint.
|
||||
// 2. The allowlist is consulted next, so a listed private
|
||||
// network becomes reachable.
|
||||
// 3. Everything else keeps the default blocklist's answer.
|
||||
|
||||
@@ -390,41 +390,6 @@ func TestGuardAllowlist_PublicUnaffected(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestGuardAllowlist_AzureWireServerReopenable covers Azure's
|
||||
// WireServer, a public address that serves VM credentials. The
|
||||
// default guard refuses it, but because it is public it sits in
|
||||
// the default blocklist rather than the unconditional set, so an
|
||||
// operator who lists it can reach it.
|
||||
func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const wireServerIP = "168.63.129.16"
|
||||
|
||||
target := "http://" + wireServerIP + "/?comp=versions"
|
||||
|
||||
defaultGuard := delivery.NewTestGuard()
|
||||
|
||||
err := defaultGuard.ValidateTargetURL(context.Background(), target)
|
||||
require.Error(t, err,
|
||||
"WireServer must be refused with no allowlist set",
|
||||
)
|
||||
assert.NotContains(t, err.Error(), metadataRefusalClause,
|
||||
"WireServer must be refused by the default blocklist, "+
|
||||
"which an allowlist can override",
|
||||
)
|
||||
|
||||
assertDialRefused(t, defaultGuard, target)
|
||||
|
||||
listed := delivery.NewTestGuard(
|
||||
netip.MustParsePrefix(wireServerIP + "/32"),
|
||||
)
|
||||
|
||||
assert.NoError(t,
|
||||
listed.ValidateTargetURL(context.Background(), target),
|
||||
"an operator who lists WireServer must be able to reach it",
|
||||
)
|
||||
}
|
||||
|
||||
// TestGuardCheckIP_BothPathsShareOneDecision asserts that the
|
||||
// validator and the dialer are not two policies that happen to
|
||||
// agree: both are defined in terms of checkIP, so the exported
|
||||
|
||||
@@ -277,24 +277,6 @@ func (t *databaseTarget) evict(webhookID string) {
|
||||
)
|
||||
}
|
||||
|
||||
// evictAll evicts every cached archive writer, exactly as evict
|
||||
// does for one webhook. The engine calls it at shutdown, once its
|
||||
// workers have returned. Closing the last handle on an archive
|
||||
// moves the contents of its -wal into the .db and removes the
|
||||
// -wal, so a clean stop leaves each archive as a single file.
|
||||
func (t *databaseTarget) evictAll() {
|
||||
t.mu.Lock()
|
||||
|
||||
writers := t.writers
|
||||
t.writers = nil
|
||||
|
||||
t.mu.Unlock()
|
||||
|
||||
for _, w := range writers {
|
||||
w.evict()
|
||||
}
|
||||
}
|
||||
|
||||
// sweepWebhook prunes one webhook's archive of rows older than
|
||||
// expiry, without requiring a write. It returns nil (nothing to
|
||||
// do) when the archive file does not exist, so a sweep never
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
package delivery_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
@@ -362,46 +361,3 @@ func TestEvictWebhook_LaterDeliveryRecreatesWriter(t *testing.T) {
|
||||
"a later delivery should recreate the writer",
|
||||
)
|
||||
}
|
||||
|
||||
// TestEngineStop_WriteAfterStopIsRefused proves the engine's stop
|
||||
// closes each archive writer the way deleting its webhook does: a
|
||||
// write that reaches a writer after the stop is refused, reopens
|
||||
// nothing and adds no row.
|
||||
func TestEngineStop_WriteAfterStopIsRefused(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
eng, _ := evictTestEngine(t)
|
||||
|
||||
webhookDB := testWebhookDB(t)
|
||||
event := seedEvent(t, webhookDB, `{"archived":true}`)
|
||||
d := seedDatabaseTargetDelivery(t, webhookDB, event, "")
|
||||
|
||||
eng.ExportDeliverDatabase(webhookDB, d)
|
||||
|
||||
w := eng.ExportArchiveWriterFor(event.WebhookID)
|
||||
require.NotNil(t, w)
|
||||
require.True(t, w.HandleOpen())
|
||||
|
||||
require.NoError(t, eng.ExportStop(context.Background()))
|
||||
|
||||
err := w.Write(evictTestRow("ev-after-stop"), 0)
|
||||
|
||||
require.ErrorIs(
|
||||
t, err, delivery.ErrExportArchiveWriterEvicted,
|
||||
"a write after the stop must be refused",
|
||||
)
|
||||
assert.False(
|
||||
t, w.HandleOpen(),
|
||||
"a refused write must not reopen the archive",
|
||||
)
|
||||
assert.False(
|
||||
t, eng.ExportHasArchiveWriter(event.WebhookID),
|
||||
"the stop should empty the registry",
|
||||
)
|
||||
|
||||
count, err := countArchivedRows(w.Path())
|
||||
require.NoError(t, err)
|
||||
assert.Equal(
|
||||
t, int64(1), count, "the refused row must not be written",
|
||||
)
|
||||
}
|
||||
|
||||
@@ -18,17 +18,16 @@ import (
|
||||
// https://git.eeqj.de/sneak/webhooker/issues/107: a delivery failed
|
||||
// with nothing in its event log to say why, and a retrying delivery
|
||||
// whose target was deleted, which used to keep sending and then never
|
||||
// terminalise. Section 4 is the same deleted-target gap for a pending
|
||||
// delivery: https://git.eeqj.de/sneak/webhooker/issues/293.
|
||||
// terminalise.
|
||||
|
||||
// tUnknownType is a target type no build implements. It stands in for
|
||||
// a target whose type was written by a build that knew a type this one
|
||||
// does not.
|
||||
const tUnknownType = database.TargetType("pubsub")
|
||||
|
||||
// tSeedDeletedTarget creates a target, a delivery against it at the
|
||||
// given status with one recorded failed attempt, and then deletes the
|
||||
// target the way the source page does.
|
||||
// tSeedDeletedTarget creates a target, a retrying delivery against it
|
||||
// with one recorded failed attempt, and then deletes the target the
|
||||
// way the source page does.
|
||||
//
|
||||
// It asserts the delete is soft, because that is the whole reason the
|
||||
// engine could not tell a deleted target from a target id that never
|
||||
@@ -37,7 +36,6 @@ func tSeedDeletedTarget(
|
||||
t *testing.T,
|
||||
s iSetup,
|
||||
name, url string,
|
||||
status database.DeliveryStatus,
|
||||
) string {
|
||||
t.Helper()
|
||||
|
||||
@@ -53,7 +51,8 @@ func tSeedDeletedTarget(
|
||||
)
|
||||
|
||||
d := iSeedDelivery(
|
||||
t, s.WebhookDB, event.ID, targetID, status,
|
||||
t, s.WebhookDB, event.ID, targetID,
|
||||
database.DeliveryStatusRetrying,
|
||||
)
|
||||
|
||||
iSeedFailedResult(t, s.WebhookDB, d.ID)
|
||||
@@ -174,7 +173,6 @@ func TestRecoverSingleRetry_TargetDeleted(t *testing.T) {
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "gone-on-recovery", "http://example.com/hook",
|
||||
database.DeliveryStatusRetrying,
|
||||
)
|
||||
|
||||
s.Engine.ExportRecoverWebhookDeliveries(
|
||||
@@ -212,7 +210,6 @@ func TestSweepSingleRetry_TargetDeleted(t *testing.T) {
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "gone-on-sweep", "http://example.com/hook",
|
||||
database.DeliveryStatusRetrying,
|
||||
)
|
||||
|
||||
// Twice, because the bug was an error the sweep repeated every
|
||||
@@ -281,11 +278,11 @@ func TestSweepSingleRetry_TargetNeverExisted(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// TestFailMissingTarget_WritesNoTargetRow holds the new terminal path
|
||||
// to the same rule as the existing one: no target row, and so no
|
||||
// TestFailMissingTargetRetry_WritesNoTargetRow holds the new terminal
|
||||
// path to the same rule as the existing one: no target row, and so no
|
||||
// plaintext target config, may be written into the per-webhook event
|
||||
// database. See https://git.eeqj.de/sneak/webhooker/issues/206.
|
||||
func TestFailMissingTarget_WritesNoTargetRow(
|
||||
func TestFailMissingTargetRetry_WritesNoTargetRow(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
@@ -300,7 +297,6 @@ func TestFailMissingTarget_WritesNoTargetRow(
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "credential-bearing", hookURL,
|
||||
database.DeliveryStatusRetrying,
|
||||
)
|
||||
|
||||
s.Engine.ExportSweepWebhookRetries(
|
||||
@@ -533,260 +529,3 @@ func TestRecoverSingleRetry_TargetUnreadable_LeavesDeliveryAlone(
|
||||
|
||||
assert.Zero(t, s.Engine.ExportInflightHeld())
|
||||
}
|
||||
|
||||
// --- 4. A pending delivery whose target is gone ---
|
||||
|
||||
func TestRecoverPending_TargetDeleted(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "gone-while-pending", "http://example.com/hook",
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
s.Engine.ExportRecoverWebhookDeliveries(
|
||||
context.Background(), s.WebhookID,
|
||||
)
|
||||
|
||||
iAssertStatus(
|
||||
t, s.WebhookDB, deliveryID,
|
||||
database.DeliveryStatusFailed,
|
||||
)
|
||||
|
||||
last := tLastResult(t, s, deliveryID, 2)
|
||||
|
||||
assert.False(t, last.Success)
|
||||
assert.Equal(t, 2, last.AttemptNum)
|
||||
assert.Contains(t, last.Error, "gone-while-pending")
|
||||
assert.Contains(t, last.Error, "was deleted")
|
||||
|
||||
assert.Empty(t, fDrain(s.Engine),
|
||||
"a delivery whose target is gone was sent",
|
||||
)
|
||||
assert.Zero(t, s.Engine.ExportInflightHeld(),
|
||||
"the terminal path leaked its ownership reference",
|
||||
)
|
||||
}
|
||||
|
||||
// TestRecoverPending_TargetDeleted_LeavesAnOwnedDeliveryAlone: the
|
||||
// terminal write takes ownership like every other recovery write, so a
|
||||
// delivery the engine still holds is not failed underneath its worker.
|
||||
func TestRecoverPending_TargetDeleted_LeavesAnOwnedDeliveryAlone(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "gone-but-owned", "http://example.com/hook",
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
require.True(t, s.Engine.ExportRetainDelivery(deliveryID))
|
||||
|
||||
s.Engine.ExportRecoverWebhookDeliveries(
|
||||
context.Background(), s.WebhookID,
|
||||
)
|
||||
|
||||
iAssertStatus(
|
||||
t, s.WebhookDB, deliveryID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
assert.Len(t, iResults(t, s.WebhookDB, deliveryID), 1,
|
||||
"a delivery the engine owns was failed underneath it",
|
||||
)
|
||||
}
|
||||
|
||||
// TestFailMissingTarget_LeavesASettledDeliveryAlone: the recovery paths
|
||||
// read their batch before taking ownership, and a worker may send a
|
||||
// delivery and let it go in between. The terminal write goes by the row
|
||||
// as it is now, not as the batch read it.
|
||||
func TestFailMissingTarget_LeavesASettledDeliveryAlone(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "gone-after-sending", "http://example.com/hook",
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
var batch database.Delivery
|
||||
|
||||
require.NoError(t, s.WebhookDB.First(
|
||||
&batch, "id = ?", deliveryID,
|
||||
).Error)
|
||||
|
||||
// A worker settles the delivery after the batch was read.
|
||||
require.NoError(t, s.WebhookDB.Model(&database.Delivery{}).
|
||||
Where("id = ?", deliveryID).
|
||||
Update("status", database.DeliveryStatusDelivered).Error)
|
||||
|
||||
s.Engine.ExportFailMissingTarget(
|
||||
s.WebhookDB, s.WebhookID, &batch,
|
||||
)
|
||||
|
||||
iAssertStatus(
|
||||
t, s.WebhookDB, deliveryID,
|
||||
database.DeliveryStatusDelivered,
|
||||
)
|
||||
|
||||
assert.Len(t, iResults(t, s.WebhookDB, deliveryID), 1,
|
||||
"a delivery settled after the batch read was then failed",
|
||||
)
|
||||
assert.Zero(t, s.Engine.ExportInflightHeld())
|
||||
}
|
||||
|
||||
// TestSweepPending_TargetDeleted sweeps twice over a batch that also
|
||||
// holds a healthy stranded delivery. The one whose target is gone is
|
||||
// failed once and then left alone; the healthy one is queued by the
|
||||
// first sweep and not again by the second.
|
||||
func TestSweepPending_TargetDeleted(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
liveTargetID := uuid.New().String()
|
||||
s := fSweepSetup(t, liveTargetID, "still-there")
|
||||
|
||||
deliveryID := tSeedDeletedTarget(
|
||||
t, s, "gone-on-pending-sweep", "http://example.com/hook",
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
rAgePending(t, s.WebhookDB, deliveryID)
|
||||
|
||||
event := iSeedEvent(
|
||||
t, s.WebhookDB, s.WebhookID, `{"target":"live"}`,
|
||||
)
|
||||
|
||||
healthy := iSeedDelivery(
|
||||
t, s.WebhookDB, event.ID, liveTargetID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
rAgePending(t, s.WebhookDB, healthy.ID)
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
s.Engine.ExportSweepWebhookRetries(ctx, s.WebhookID)
|
||||
|
||||
tasks := fDrain(s.Engine)
|
||||
require.Len(t, tasks, 1,
|
||||
"the first sweep did not queue the healthy delivery",
|
||||
)
|
||||
assert.Equal(t, healthy.ID, tasks[0].DeliveryID)
|
||||
|
||||
s.Engine.ExportSweepWebhookRetries(ctx, s.WebhookID)
|
||||
|
||||
assert.Empty(t, fDrain(s.Engine),
|
||||
"the second sweep queued a delivery again",
|
||||
)
|
||||
|
||||
iAssertStatus(
|
||||
t, s.WebhookDB, deliveryID,
|
||||
database.DeliveryStatusFailed,
|
||||
)
|
||||
|
||||
last := tLastResult(t, s, deliveryID, 2)
|
||||
|
||||
assert.Contains(t, last.Error, "gone-on-pending-sweep")
|
||||
assert.Contains(t, last.Error, "was deleted")
|
||||
}
|
||||
|
||||
// TestSendRecoveredDeliveries_TargetMissingFromMap: the batch's target
|
||||
// map is empty when its query failed, so every delivery in the batch is
|
||||
// looked up on its own. A healthy one is sent to the target that lookup
|
||||
// finds.
|
||||
func TestSendRecoveredDeliveries_TargetMissingFromMap(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
targetID := uuid.New().String()
|
||||
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "found-on-lookup",
|
||||
database.TargetTypeLog, "", 0,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
t, s.WebhookDB, s.WebhookID, `{"map":"empty"}`,
|
||||
)
|
||||
|
||||
d := iSeedDelivery(
|
||||
t, s.WebhookDB, event.ID, targetID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
s.Engine.ExportSendRecoveredDeliveries(
|
||||
context.Background(), s.WebhookDB,
|
||||
[]database.Delivery{d}, s.WebhookID,
|
||||
map[string]database.Target{}, nil,
|
||||
)
|
||||
|
||||
tasks := fDrain(s.Engine)
|
||||
require.Len(t, tasks, 1,
|
||||
"the healthy delivery was not queued exactly once",
|
||||
)
|
||||
assert.Equal(t, d.ID, tasks[0].DeliveryID)
|
||||
assert.Equal(t, targetID, tasks[0].TargetID)
|
||||
assert.Equal(t, database.TargetTypeLog, tasks[0].TargetType)
|
||||
|
||||
iAssertStatus(
|
||||
t, s.WebhookDB, d.ID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
}
|
||||
|
||||
// TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone: a failed
|
||||
// read of the main database is not a deleted target. Restart recovery
|
||||
// holds every pending delivery of the webhook in one batch, so failing
|
||||
// on this would fail all of them.
|
||||
func TestRecoverPending_TargetUnreadable_LeavesDeliveryAlone(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
s := newISetup(t)
|
||||
|
||||
targetID := uuid.New().String()
|
||||
|
||||
iCreateTarget(
|
||||
t, s.MainDB, targetID, s.WebhookID, "healthy",
|
||||
database.TargetTypeLog, "", 0,
|
||||
)
|
||||
|
||||
event := iSeedEvent(
|
||||
t, s.WebhookDB, s.WebhookID, `{"still":"pending"}`,
|
||||
)
|
||||
|
||||
d := iSeedDelivery(
|
||||
t, s.WebhookDB, event.ID, targetID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
sqlDB, err := s.MainDB.DB()
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, sqlDB.Close())
|
||||
|
||||
s.Engine.ExportRecoverPendingDeliveries(
|
||||
context.Background(), s.WebhookDB, s.WebhookID,
|
||||
)
|
||||
|
||||
iAssertStatus(
|
||||
t, s.WebhookDB, d.ID,
|
||||
database.DeliveryStatusPending,
|
||||
)
|
||||
|
||||
assert.Empty(t, iResults(t, s.WebhookDB, d.ID),
|
||||
"an unreadable main database produced a terminal "+
|
||||
"failure row",
|
||||
)
|
||||
assert.Empty(t, fDrain(s.Engine))
|
||||
assert.Zero(t, s.Engine.ExportInflightHeld())
|
||||
}
|
||||
|
||||
@@ -103,10 +103,9 @@ 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, 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.
|
||||
// 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.
|
||||
//
|
||||
// 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
|
||||
// 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
|
||||
// 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, 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.
|
||||
// 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.
|
||||
//
|
||||
// This fails if credentials stop being verified ahead of the limiter.
|
||||
func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) {
|
||||
|
||||
@@ -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, 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.
|
||||
// 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.
|
||||
//
|
||||
// So budget is spent only by a FAILED verification. A correct
|
||||
// password is never throttled, whatever the counters say, which is
|
||||
|
||||
@@ -123,8 +123,9 @@ func bucketKey(addr netip.Addr) string {
|
||||
return prefix.String()
|
||||
}
|
||||
|
||||
// isTrustedProxy reports whether addr belongs to a network in
|
||||
// TRUSTED_PROXIES, which by default is the RFC 1918 private ranges.
|
||||
// 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.
|
||||
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: from a peer that is not a trusted
|
||||
// proxy, a client that rotates a forwarded header on every
|
||||
// this gating exists for: with no trusted proxies configured (the
|
||||
// default), 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.
|
||||
|
||||
@@ -133,11 +133,6 @@ func (w *recoverResponseWriter) Unwrap() http.ResponseWriter {
|
||||
// what the access log records and the metrics count, and outside the
|
||||
// sentryhttp handler, whose Repanic option depends on something
|
||||
// further out recovering what it re-raises.
|
||||
//
|
||||
// Unlike http.Error on its own, it deletes any Set-Cookie the handler
|
||||
// set before panicking, because a request that failed must not hand
|
||||
// the client a credential; every other header is left to http.Error.
|
||||
// See https://git.eeqj.de/sneak/webhooker/issues/193.
|
||||
func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
|
||||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(
|
||||
@@ -169,8 +164,6 @@ func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
|
||||
return
|
||||
}
|
||||
|
||||
rw.Header().Del("Set-Cookie")
|
||||
|
||||
http.Error(
|
||||
rw,
|
||||
http.StatusText(
|
||||
|
||||
@@ -304,44 +304,16 @@ func TestRecovererRepanicsErrAbortHandler(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// TestRecovererDropsSetCookieFromTheRecovered500 covers a handler that
|
||||
// sets a cookie and a redirect target and then panics before sending
|
||||
// anything. A request that failed must not hand the client a
|
||||
// credential, so the 500 carries no cookie; Location is left alone.
|
||||
func TestRecovererDropsSetCookieFromTheRecovered500(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
probe := newRecovererProbe(
|
||||
t, false,
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.Header().Set("Set-Cookie", "session=x")
|
||||
w.Header().Set("Location", "/after")
|
||||
|
||||
panic(panicMarker)
|
||||
},
|
||||
)
|
||||
|
||||
resp, err := probe.get(t)
|
||||
require.NoError(t, err)
|
||||
require.NoError(t, resp.Body.Close())
|
||||
|
||||
assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
|
||||
assert.Empty(t, resp.Cookies())
|
||||
assert.Equal(t, "/after", resp.Header.Get("Location"))
|
||||
}
|
||||
|
||||
// TestRecovererKeepsAnAlreadyCommittedResponse covers a handler that
|
||||
// panics after sending its status. The bytes are already on the wire,
|
||||
// cookie included, so a second WriteHeader would change nothing the
|
||||
// client sees and would draw net/http's "superfluous
|
||||
// response.WriteHeader" report.
|
||||
// so a second WriteHeader would change nothing the client sees and
|
||||
// would draw net/http's "superfluous response.WriteHeader" report.
|
||||
func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
probe := newRecovererProbe(
|
||||
t, false,
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.Header().Set("Set-Cookie", "session=x")
|
||||
w.WriteHeader(committedStatus)
|
||||
_, _ = w.Write([]byte("partial"))
|
||||
|
||||
@@ -359,7 +331,6 @@ func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
|
||||
|
||||
assert.Equal(t, committedStatus, resp.StatusCode)
|
||||
assert.Equal(t, "partial", string(body))
|
||||
assert.Len(t, resp.Cookies(), 1)
|
||||
|
||||
record := probe.panicRecord(t)
|
||||
assert.Equal(t, panicMarker, record["panic"])
|
||||
|
||||
@@ -140,12 +140,11 @@ func (s *Server) setupPageRoutes() {
|
||||
r.Use(s.mw.NoCache())
|
||||
|
||||
// The login POST carries no pre-emptive rate limiter. Behind
|
||||
// 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.
|
||||
// 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.
|
||||
r.Get("/login", s.h.HandleLoginPage())
|
||||
r.Post("/login", s.h.HandleLoginSubmit())
|
||||
|
||||
|
||||
Reference in New Issue
Block a user