133 lines
6.9 KiB
Markdown
133 lines
6.9 KiB
Markdown
---
|
|
id: review-001-client-timeout-retry
|
|
name: Client redirect/Retry-After policy — idempotency, timeouts, caps (FWD-03, FWD-04, FWD-05, FWD-11, FWD-09)
|
|
status: completed
|
|
depends_on: []
|
|
scope: moderate
|
|
risk: medium
|
|
impact: component
|
|
level: implementation
|
|
tags: [client, review-001]
|
|
---
|
|
|
|
## Description
|
|
|
|
Review 001 findings on the outbound client host
|
|
(`src/client/http_client.rs`, `retry_after.rs`), grouped because they all
|
|
shape the shared client's request policy:
|
|
|
|
- **FWD-03**: no explicit redirect policy → reqwest's default
|
|
cross-host redirect scrub removes only `Authorization`/`Cookie`/etc;
|
|
`HttpAuthScheme::ApiKey { header_name }` credentials and
|
|
`default_headers` follow a 302 to an attacker host intact (verified
|
|
against reqwest 0.13 source). Fix: explicit policy — none, or limited
|
|
same-host.
|
|
- **FWD-04**: `RetryTransientMiddleware` retries POSTs (5xx/408/429/
|
|
timeout classified retryable regardless of method — verified in
|
|
reqwest-retry 0.9.1) → duplicate upstream side effects; backoff has no
|
|
total-duration cap. Fix: skip retries for non-idempotent methods (or
|
|
make idempotency a per-adapter policy) and cap total retry wall time.
|
|
- **FWD-05**: `HttpClientConfig::default()` sets `request_timeout: None`
|
|
and no connect timeout anywhere; `retry_after.rs:27-37` accepts any u64
|
|
`Retry-After` with no maximum — a hostile backend's 10-year deadline
|
|
stalls calls to that URL indefinitely. Fix: default request + connect
|
|
timeouts (the gateway's 30 s deadline is the natural anchor) and a
|
|
`Retry-After` ceiling (e.g. 300 s, configurable).
|
|
- **FWD-11**: `Retry-After` is keyed on the pre-redirect URL, eviction
|
|
drops the *earliest* deadline (keeping year-long entries), and all
|
|
waiters wake together with no jitter. Key on the effective URL, evict
|
|
sensibly, add jitter.
|
|
- **FWD-09**: blocking `std::fs::read` in `build_client`, reachable via
|
|
the public documented hot-reload path `SharedHttpClient::reload` —
|
|
`tokio::fs`/`spawn_blocking` the reads.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [x] Cross-host redirect test with an API-key credential header — key must not reach the redirect target
|
|
- [x] Non-idempotent method is never retried (test); total retry duration bounded (test)
|
|
- [x] Default request + connect timeouts exist in `HttpClientConfig::default()`; `Retry-After` capped (tests)
|
|
- [x] Retry-After keyed post-redirect; eviction and wake behavior fixed or documented (tests)
|
|
- [x] No blocking fs reads on the async path (FWD-09)
|
|
- [x] `HttpClientConfig` defaults documented; review-001-forward-url-safety and this task together close the deployment-facing gate
|
|
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
|
|
|
## References
|
|
|
|
- docs/reviews/001-initial-implementation-review.md (Part D, FWD-03, FWD-04, FWD-05, FWD-09, FWD-11)
|
|
- docs/architecture/decisions/039-http-server-and-client-host-colocated.md
|
|
|
|
## Notes
|
|
|
|
> Agent fills during implementation. Independent of
|
|
> review-001-forward-url-safety (different file); they form the
|
|
> deployment gate together.
|
|
|
|
Policy values chosen (documented in the `http_client.rs` module docs and
|
|
`HttpClientConfig` field docs):
|
|
|
|
- Redirect policy: same-host only (scheme + host + port must match), 10-hop
|
|
cap; cross-host redirects surface the 302 response untouched — headers
|
|
never travel to another host. Rationale: same-host hops preserve the
|
|
legitimate redirect-following convenience (oauth-ish flows behind one
|
|
origin) while structurally ruling out credential exfiltration; the
|
|
alternative (redirects none) is a one-line config swap for callers who
|
|
want stricter behavior.
|
|
- Retries: method-gated (GET/HEAD/PUT/DELETE/OPTIONS only; POST/PATCH/
|
|
CONNECT/TRACE bypass the retry middleware entirely) + attempt count 3
|
|
+ backoff bounds [100 ms, 2 s] with `Jitter::Bounded` + wall-clock
|
|
budget `max_total_retry_duration` (default 10 s) enforced by
|
|
`TotalRetryBudget` (stops retries and clamps scheduled retries to the
|
|
budget deadline).
|
|
- Default timeouts: request 30 s (gateway deadline anchor), connect
|
|
10 s, read 30 s; all configurable, `None` disables.
|
|
- Retry-After ceiling: 300 s default, configurable per client
|
|
(`retry_after_ceiling`) / per middleware instance; applies to both
|
|
seconds and HTTP-date forms; past deadlines still rejected.
|
|
- FWD-11: deadlines recorded under the effective (post-redirect) URL
|
|
(`response.url()`); eviction prefers expired entries, then the
|
|
farthest-future deadline (least actionable first); wakes jittered by
|
|
25% of the remaining wait, capped at 2 s.
|
|
- FWD-09: `SharedHttpClient::reload` is now async (`tokio::fs::read` —
|
|
`spawn_blocking`-backed); PEM reads factored into read-first + pure
|
|
builder. `SharedHttpClient::new` remains sync and is documented as
|
|
one-shot blocking construction at assembly time (never on a request or
|
|
hot-reload path) — kept sync deliberately to avoid churning ~15
|
|
constructor call sites in adapter tests; reload (the documented
|
|
hot-reload path) is fully non-blocking.
|
|
|
|
## Summary
|
|
|
|
Implemented in `src/client/http_client.rs` + `src/client/retry_after.rs`
|
|
(commits 015b241, b1529dd, 4a557a0):
|
|
|
|
- **FWD-03**: `same_host_redirect_policy()` — reqwest `Policy::custom`
|
|
following only scheme+host+port matches (10-hop cap via
|
|
`attempt.error`); everything else `attempt.stop()`. Tests:
|
|
`cross_host_redirect_does_not_leak_api_key_header` (raw-TCP attacker +
|
|
redirector listeners assert the key never reaches the target and no
|
|
request arrives at all), `same_host_redirect_is_still_followed`.
|
|
- **FWD-04**: `RetryGateMiddleware` gates idempotent methods into
|
|
`RetryTransientMiddleware` wrapped in `TotalRetryBudget` (wall-clock
|
|
cap; also clamps scheduled retry times into the budget). Tests:
|
|
`non_idempotent_post_gets_one_attempt_then_the_429_is_surfaced` (500 on
|
|
POST → exactly 1 upstream hit), `idempotent_get_is_retried_on_a_
|
|
transient_failure` (500,500,200 → 3 hits), policy unit tests.
|
|
- **FWD-05**: `HttpClientConfig::default()` → request 30 s / connect
|
|
10 s / read 30 s; `retry_after_ceiling` (default 300 s) clamps both
|
|
numeric and HTTP-date `Retry-After` values in
|
|
`parse_retry_after_with_ceiling`. Tests: default-config assertions,
|
|
10-year Retry-After clamped, custom 5 s ceiling honored.
|
|
- **FWD-11**: record under `response.url()` (effective URL); eviction =
|
|
expired-first then farthest-future; wake jitter (25% capped 2 s).
|
|
Tests: `middleware_records_under_the_effective_url`,
|
|
`record_evicts_expired_entries_first`,
|
|
`record_evicts_the_farthest_deadline_when_none_expired`,
|
|
`sleep_wakes_before_the_deadline_within_the_jitter_bound`,
|
|
`jitter_is_bounded_by_a_fraction_of_the_remaining_wait`.
|
|
- **FWD-09**: `build_client` split into read (async `tokio::fs` for
|
|
`reload`, sync for one-shot `new`) + pure `build_client_with_pems`;
|
|
`reload` documented and implemented as non-blocking.
|
|
|
|
36 client unit tests (was 20) + full-suite green; `cargo clippy
|
|
--all-targets -- -D warnings` and `cargo fmt --check` clean;
|
|
`cargo test --all-features` green (289 tests). |