glm-5.1
80d1fd0fb3
Add three ADRs (025-027) and update five spec documents to close gaps identified in the security and bug review: - ADR-025: Rate limiter IP source must be ConnectInfo only (C1 fix) - ADR-026: Connector timeout ceiling of 30s for per-site timeouts (C3 fix) - ADR-027: Admin socket resource limits — 5s timeout, 4096 byte line limit (W4 fix) Spec changes: - proxy.md: add rate limiter IP source section, URI error handling constraint, connector ceiling description, renumber sections - operations.md: add ConnectInfo-only IP source, in-flight counter architectural requirement (C2), JSON format guarantee (C4), admin socket resource limits, 100ms drain polling interval - config.md: fix http_port type u32→u16 (W12), tighten upstream host validation (W1), tighten ACME contact validation (W2), add X-Forwarded-Proto cross-reference, clarify alknet ADR-030 reference - overview.md: fix ambiguous C1 reference, add ADR/OQ cross-references - open-questions.md: update OQ-09 resolution, add OQ-13 (acme_contact Vec) and OQ-14 (eviction configurability) - README.md: add ADR-025/026/027 and OQ-13/14, update doc statuses to draft Also fix reviewer findings: alknet ADR-030 scope clarification, RFC 2616 reference updated to RFC 7230.
93 lines
4.0 KiB
Markdown
93 lines
4.0 KiB
Markdown
# ADR-025: Rate Limiter IP Source Must Be ConnectInfo Only
|
|
|
|
## Status
|
|
|
|
Accepted
|
|
|
|
## Context
|
|
|
|
The rate limiter identifies clients by IP address to enforce per-IP token bucket
|
|
limits. The question is: what is the authoritative source of the client IP for
|
|
rate limiting?
|
|
|
|
Two potential sources exist:
|
|
|
|
1. **`ConnectInfo<SocketAddr>`**: The TCP peer address, extracted from
|
|
`TcpStream::peer_addr()` before TLS handshake and propagated to axum via
|
|
`ConnectInfoService`. This is the real client IP at the TCP level.
|
|
|
|
2. **`X-Forwarded-For` header**: A client-supplied HTTP header that may contain
|
|
an IP address. This header is set by the proxy handler *after* rate limiting
|
|
(the rate limiter runs as middleware before the handler), so the value
|
|
present during rate limit checks is whatever the client sent — completely
|
|
untrusted.
|
|
|
|
ADR-021 already establishes that the proxy is an edge proxy and client-supplied
|
|
`X-Forwarded-For` headers are untrusted. The proxy handler replaces
|
|
`X-Forwarded-For` with the `ConnectInfo` IP before forwarding upstream
|
|
specifically to prevent spoofing. However, ADR-021 only addresses the proxy
|
|
handler's header injection — it does not specify the rate limiter's IP source.
|
|
|
|
The current implementation checks `X-Forwarded-For` *first* and falls back to
|
|
`ConnectInfo`, which creates two attack vectors:
|
|
|
|
- **Rate limit bypass**: A client sends each request with a different random
|
|
`X-Forwarded-For` value. Every request appears to come from a different IP,
|
|
evading the per-IP token bucket entirely.
|
|
- **Denial-of-service via IP spoofing**: A client sends requests with
|
|
`X-Forwarded-For: <victim IP>`. The victim's bucket is depleted and their
|
|
legitimate requests receive 429 responses.
|
|
|
|
## Decision
|
|
|
|
The rate limiter must use `ConnectInfo<SocketAddr>` as the **sole** source of
|
|
client IP addresses. `X-Forwarded-For` must not be consulted for rate limiting
|
|
purposes.
|
|
|
|
The rate limiter runs as middleware before the proxy handler. At that point in
|
|
the request lifecycle, no proxy headers have been injected — any `X-Forwarded-For`
|
|
header present is from the client and is untrusted (ADR-021).
|
|
|
|
`ConnectInfo<SocketAddr>` is always present because each listener populates it
|
|
via `into_make_service_with_connect_info::<SocketAddr>()`. If `ConnectInfo` is
|
|
absent (which should not happen in normal operation), the request should be
|
|
rejected rather than falling back to an untrusted header.
|
|
|
|
## Rationale
|
|
|
|
- The proxy is the edge proxy (ADR-021). Client-supplied headers are
|
|
untrusted at the edge.
|
|
- Rate limiting is a security mechanism — it must use the most trustworthy
|
|
IP source available. `ConnectInfo` is set by the kernel's TCP stack, not by
|
|
the client.
|
|
- The rate limiter's position in the middleware stack (before the handler)
|
|
means it sees raw client headers, not the replaced `X-Forwarded-For` that the
|
|
proxy handler injects.
|
|
- Falling back to `X-Forwarded-For` when `ConnectInfo` is absent creates a
|
|
downgrade attack — an attacker could potentially strip `ConnectInfo` from
|
|
extensions to force the fallback path. Better to reject than to accept an
|
|
untrusted IP.
|
|
|
|
## Consequences
|
|
|
|
**Positive:**
|
|
- Rate limiting cannot be bypassed via header spoofing
|
|
- Rate limiting cannot be weaponized to DoS legitimate clients
|
|
- Consistent with ADR-021's edge proxy trust model
|
|
- No ambiguity about the IP source — one source, one code path
|
|
|
|
**Negative:**
|
|
- If the proxy is ever placed behind a trusted CDN or load balancer,
|
|
`ConnectInfo` will reflect the CDN's IP, not the end client's. This would
|
|
require a "trusted proxies" configuration (already noted as a Phase 2
|
|
consideration in ADR-021).
|
|
- Requests without `ConnectInfo` are rejected. This should not happen in
|
|
normal operation but adds a hard failure mode.
|
|
|
|
## References
|
|
|
|
- [proxy.md](../proxy.md) — Proxy header injection, request flow
|
|
- [operations.md](../operations.md) — Rate limiting design
|
|
- ADR-006 — Token bucket rate limiting
|
|
- ADR-021 — X-Forwarded-For edge proxy model
|
|
- Security Review C1 — Rate limiter X-Forwarded-For spoofing vulnerability |