Update architecture docs to address security review #003 findings
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.
This commit is contained in:
93
docs/architecture/decisions/025-rate-limiter-ip-source.md
Normal file
93
docs/architecture/decisions/025-rate-limiter-ip-source.md
Normal file
@@ -0,0 +1,93 @@
|
||||
# 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
|
||||
90
docs/architecture/decisions/026-connector-timeout-ceiling.md
Normal file
90
docs/architecture/decisions/026-connector-timeout-ceiling.md
Normal file
@@ -0,0 +1,90 @@
|
||||
# ADR-026: Connector Timeout Ceiling for Per-Site Timeouts
|
||||
|
||||
## Status
|
||||
|
||||
Accepted
|
||||
|
||||
## Context
|
||||
|
||||
ADR-015 specifies per-site upstream connect timeout configuration with a default
|
||||
of 5 seconds. The proxy enforces connect timeouts using two mechanisms:
|
||||
|
||||
1. **`tokio::time::timeout`**: Wraps the entire `client.request()` call with the
|
||||
per-site `upstream_connect_timeout_secs` value.
|
||||
2. **`HttpConnector::set_connect_timeout()`**: Sets the TCP-level connect timeout
|
||||
on the hyper `HttpConnector` inside the shared client.
|
||||
|
||||
The problem: the HTTP connector's `set_connect_timeout()` is set once at client
|
||||
creation time and applies to all requests through that client. The current
|
||||
implementation hardcodes this to 5 seconds. Since the connector's internal
|
||||
timeout fires before `tokio::time::timeout`, any per-site connect timeout
|
||||
greater than 5 seconds is silently capped — the connector times out at 5s
|
||||
regardless of the configured value.
|
||||
|
||||
Three approaches exist:
|
||||
|
||||
1. **Raise the connector timeout to a high ceiling**: Set the connector's
|
||||
`set_connect_timeout` to a value higher than any reasonable per-site timeout
|
||||
(e.g., 30s). Let `tokio::time::timeout` enforce the actual per-site limit.
|
||||
The connector timeout becomes a safety ceiling, not the primary enforcement
|
||||
mechanism.
|
||||
|
||||
2. **Remove the connector timeout entirely**: Set `set_connect_timeout` to
|
||||
`None` and rely solely on `tokio::time::timeout`. This removes one layer
|
||||
of timeout enforcement but simplifies the model.
|
||||
|
||||
3. **Create per-site client instances**: Each site gets its own hyper Client
|
||||
with its own connector configured with the site's connect timeout. This is
|
||||
the most precise approach but creates many client instances and connection
|
||||
pools, increasing resource usage.
|
||||
|
||||
## Decision
|
||||
|
||||
Use approach 1: set the connector timeout to a high ceiling value (30 seconds)
|
||||
and let `tokio::time::timeout` enforce the actual per-site connect timeout.
|
||||
|
||||
The connector timeout serves as a safety ceiling — it ensures that even if the
|
||||
`tokio::time::timeout` wrapper fails or is misconfigured, TCP connections
|
||||
cannot hang indefinitely. The ceiling of 30s is well above the default 5s and
|
||||
any reasonable per-site override.
|
||||
|
||||
The `tokio::time::timeout` wrapper with the per-site value is the primary
|
||||
enforcement mechanism. It fires at the correct per-site threshold.
|
||||
|
||||
## Rationale
|
||||
|
||||
- The shared client architecture (ADR-017) means one connector timeout for all
|
||||
sites. Creating per-site clients would undermine connection pooling and
|
||||
increase resource usage.
|
||||
- A ceiling approach preserves the defense-in-depth benefit of two timeout
|
||||
layers while allowing per-site values to actually work.
|
||||
- 30s is a reasonable ceiling — no legitimate upstream connect should take
|
||||
longer than 30s. Sites that need a higher connect timeout can set the ceiling
|
||||
even higher if needed, but 30s covers all practical cases.
|
||||
- Removing the connector timeout (approach 2) removes the safety ceiling
|
||||
entirely. If `tokio::time::timeout` has a bug or is misapplied, TCP connects
|
||||
could hang indefinitely. The ceiling provides a backstop.
|
||||
- The HTTPS client uses `HttpsConnector<HttpConnector>`, which wraps the
|
||||
`HttpConnector`. The same ceiling applies to both HTTP and HTTPS clients.
|
||||
|
||||
## Consequences
|
||||
|
||||
**Positive:**
|
||||
- Per-site connect timeouts work as documented (ADR-015)
|
||||
- Maintains defense-in-depth with two timeout layers
|
||||
- No change to the shared client / connection pooling architecture
|
||||
- Simple implementation: change one constant
|
||||
|
||||
**Negative:**
|
||||
- The connector timeout no longer matches the default connect timeout (5s
|
||||
default vs. 30s ceiling). Operators who read the connector timeout might
|
||||
be confused — documentation must make the ceiling role clear.
|
||||
- If a site needs a connect timeout > 30s, the ceiling must be raised. This
|
||||
is unlikely in practice but creates a hidden upper bound.
|
||||
|
||||
## References
|
||||
|
||||
- [proxy.md](../proxy.md) — Upstream connection, per-site timeouts
|
||||
- ADR-015 — Per-site upstream timeouts with defaults
|
||||
- ADR-017 — Upstream connection defaults
|
||||
- Security Review C3 — Connect timeout silently capped at 5s
|
||||
@@ -0,0 +1,77 @@
|
||||
# ADR-027: Admin Socket Resource Limits
|
||||
|
||||
## Status
|
||||
|
||||
Accepted
|
||||
|
||||
## Context
|
||||
|
||||
The admin Unix domain socket (ADR-014) accepts connections from local
|
||||
processes and reads one newline-terminated command per connection. The current
|
||||
implementation has no read timeout and no line length limit.
|
||||
|
||||
Two attack vectors exist for processes with access to the admin socket
|
||||
(controlled by Unix file permissions):
|
||||
|
||||
1. **Connection hold**: A client connects and sends no data, keeping a
|
||||
connection and tokio task open indefinitely. An attacker with socket access
|
||||
can open many such connections to exhaust file descriptors or task slots.
|
||||
|
||||
2. **Unbounded memory allocation**: A client sends data without a newline,
|
||||
causing `read_line` to buffer indefinitely. This allows unbounded memory
|
||||
consumption from a single connection.
|
||||
|
||||
While the admin socket is protected by Unix file permissions (only processes
|
||||
with access to the socket file can connect), defense-in-depth warrants basic
|
||||
resource limits. The socket is a local diagnostic interface, not a
|
||||
high-performance endpoint — strict limits are appropriate.
|
||||
|
||||
## Decision
|
||||
|
||||
Apply two resource limits to admin socket connections:
|
||||
|
||||
1. **Read timeout**: 5 seconds per connection. If no complete command is
|
||||
received within 5 seconds, the connection is closed and a timeout error
|
||||
is logged at `debug` level.
|
||||
|
||||
2. **Line length limit**: 4096 bytes per command. If a client sends more
|
||||
than 4096 bytes without a newline, the connection is closed and the event
|
||||
is logged at `warn` level. The longest valid command (`reload`) is 6 bytes —
|
||||
4096 bytes provides ample room for future commands while preventing abuse.
|
||||
|
||||
Both limits apply per connection. The admin socket creates one tokio task per
|
||||
connection, so the limits also bound per-connection resource usage.
|
||||
|
||||
## Rationale
|
||||
|
||||
- Defense-in-depth: even with Unix permission protection, basic resource
|
||||
limits prevent accidental or deliberate resource exhaustion.
|
||||
- 5 seconds is generous for a local socket — a local process should be able
|
||||
to send a command within milliseconds. The timeout handles stuck clients and
|
||||
network issues on tunnel-mounted sockets.
|
||||
- 4096 bytes is 600x the longest current command. It accommodates future
|
||||
multi-field commands without allowing abuse.
|
||||
- These limits have no impact on legitimate admin socket usage — the current
|
||||
commands (`reload`, `status`) are tiny and complete instantly.
|
||||
|
||||
## Consequences
|
||||
|
||||
**Positive:**
|
||||
- No unbounded memory allocation from admin socket connections
|
||||
- No indefinite connection holding
|
||||
- Simple implementation: `tokio::time::timeout` + `tokio::io::take` (or
|
||||
`read_until` with a byte limit)
|
||||
- No impact on legitimate usage
|
||||
|
||||
**Negative:**
|
||||
- If a future admin command requires more than 4096 bytes (unlikely), the
|
||||
limit must be raised.
|
||||
- A 5-second timeout could cause issues if the admin socket is accessed via
|
||||
a slow network tunnel. In practice, this is unlikely — admin socket access
|
||||
is typically local.
|
||||
|
||||
## References
|
||||
|
||||
- ADR-014 — Unix domain socket config reload API
|
||||
- [operations.md](../operations.md) — Admin socket protocol
|
||||
- Security Review W4 — Admin socket has no read timeout or line length limit
|
||||
Reference in New Issue
Block a user