Add configurable trusted client-IP header support for Fastly - #1048
Add configurable trusted client-IP header support for Fastly#1048prk-Jr wants to merge 25 commits into
Conversation
Reject secrets shorter than the 32-character minimum already applied to ec.passphrase, and route the secret through reject_placeholder_secrets so the placeholder published in the example config and guides fails startup. The secret is the only gate on forging the client address that geolocation, EC identity derivation, and bot protection consume.
The shared proxy code forwards an inbound X-Forwarded-For to publisher origins, and no adapter has a trustworthy upstream forwarded-for chain, so a client could choose the address the origin attributes the request to. The Spin adapter already stripped it; moving the rule into the shared spoofable-header list closes the same gap on Fastly without changing Spin behavior. Integrations that need the address keep injecting their own value from the resolved client IP.
Fold resolution and forwarded-header sanitization into resolve_and_sanitize_client_ip so resolution cannot be reordered after the sanitization that removes the headers it reads. Log every fallback taken while a configuration is present at debug level, without the secret or the address, so a rotated secret or renamed header stops failing silently. Document the front door requirement to leave exactly one value per trust header, since duplicates select the peer address.
prk-Jr
changed the title
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for one high-severity config rollout and rollback compatibility issue. The trusted client-IP runtime behavior and validation otherwise looked sound at the reviewed revision.
`#[serde(default)]` only affects deserialization, so an unconfigured `trusted_client_ip` was still written as `"trusted_client_ip": null` into every blob produced by `ts config push`. `Settings` uses `deny_unknown_fields`, so pushing an otherwise unchanged config before upgrading all instances - or rolling back after such a push - made older instances reject the blob even though the feature was never enabled. Skip serialization when the field is `None`, matching the existing `AuctionConfig` rollback-compatibility pattern, and document that a configured section requires restoring a compatible blob before rollback. Add regression tests proving a default payload omits the key and stays readable by a schema matching the base revision.
Resolve two conflicts: - `settings.rs` imports: keep both `std::time::Duration` (cache policy from main) and `subtle::ConstantTimeEq` (shared-secret comparison). - `configuration.md` key-sections table: keep both the `[cache]` row from main and the `[trusted_client_ip]` row from this branch. Also add `cache` to the `BaseRevisionSettings` test schema, since main adds that key to `Settings` and the base revision this branch rolls back to now knows it.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds authenticated trusted client-IP forwarding for Fastly with peer fallback, cross-adapter trust-header stripping, and validated configuration. No blocking defects found in the resolution path: authentication precedes IP parsing, the comparison is constant-time over fixed-size digests, malformed or duplicated input fails closed to the peer address, no secret or header value is logged, and the config-blob rollback hazard is regression-tested against a modeled base-revision schema. The findings below are hardening and maintainability items.
2 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe concerns in prose because the change is design-level or spans files outside the diff.
Non-blocking
♻️ refactor
- Reserve all internal trust headers, not just the TLS bridge pair - see inline at
crates/trusted-server-core/src/settings.rs:2672
⛏ nitpick
- Duplicate
/.worktrees/entry - see inline at.gitignore:55
🤔 thinking
- Trust-header sanitization depends on unstated middleware ordering - see inline at
crates/trusted-server-adapter-axum/src/middleware.rs:40
🌱 seedling
- No operator-visible signal when the trust handshake degrades - see inline at
crates/trusted-server-adapter-fastly/src/platform.rs:730
CI Status
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- cargo test (axum native): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- vitest: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (actions): PASS
| for header in [&ip_header, &auth_header] { | ||
| if matches!(header.as_str(), "x-ts-tls-protocol" | "x-ts-tls-cipher") { | ||
| return Err(ValidationError::new("reserved_trusted_client_ip_header")); | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ refactor - Reserved-header validation covers only the two TLS bridge headers. Every other internal trust header passes the x- prefix rule: ip_header = "x-geo-info-available", "x-forwarded-for", or "x-ts-ec" are all accepted, and the sanitizer then strips platform-injected internal signals before routing. On Cloudflare the finalize middleware reads x-geo-info-available from the request before sanitization, so a colliding trust header would double as a geo signal. INTERNAL_HEADERS in constants.rs is exactly this reserved set and already contains both TLS bridge names, so the existing reserved-header test passes unchanged.
| for header in [&ip_header, &auth_header] { | |
| if matches!(header.as_str(), "x-ts-tls-protocol" | "x-ts-tls-cipher") { | |
| return Err(ValidationError::new("reserved_trusted_client_ip_header")); | |
| } | |
| } | |
| for header in [&ip_header, &auth_header] { | |
| if crate::constants::INTERNAL_HEADERS.contains(&header.as_str()) { | |
| return Err(ValidationError::new("reserved_trusted_client_ip_header")); | |
| } | |
| } |
If you take this, consider hoisting the path into a use crate::constants::INTERNAL_HEADERS; import and adding one validation test case for a non-TLS reserved name; both live outside this hunk, so the suggestion cannot carry them.
|
|
||
| /guest-profiles | ||
| /benchmark-results/** | ||
| /.worktrees/ |
There was a problem hiding this comment.
⛏ nitpick - Duplicate entry: line 41 ("Agent implementation worktrees") already ignores /.worktrees/ and was present before this PR. This second copy looks like a merge artifact; the plan doc records the decision as retaining the existing ignore, not adding another one.
| /.worktrees/ |
| impl Middleware for FinalizeResponseMiddleware { | ||
| async fn handle(&self, ctx: RequestContext, next: Next<'_>) -> Result<Response, EdgeError> { | ||
| async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result<Response, EdgeError> { | ||
| sanitize_trusted_client_ip_headers( |
There was a problem hiding this comment.
🤔 thinking - The "strip trust headers before routing" invariant now lives inside FinalizeResponseMiddleware, a response-finalization middleware, and holds only because this middleware happens to be registered first in each adapter's router. Nothing enforces that ordering: a middleware registered ahead of it later, or a reorder during a refactor, would silently re-expose the auth header to earlier request handling, and the middleware's name gives no hint that it carries a security invariant. Consider a dedicated request-sanitization middleware, or at minimum a comment at each registration site pinning the ordering requirement. The same applies to the Cloudflare and Spin copies of this change.
| return peer_ip; | ||
| }; | ||
| if !config.authenticates(auth_candidate) { | ||
| log::debug!("Trusted client IP: auth header did not match the configured shared secret"); |
There was a problem hiding this comment.
🌱 seedling - Debug-level fallback logging avoids client-driven log volume, but it also means a rotated-but-unsynced secret or a renamed front-door header silently degrades geo, EC derivation, and bot protection to Fastly edge-node addresses. A follow-up counter (for example, a fallback-with-config-present rate in telemetry) or a synthetic check that asserts the forwarded address round-trips would make that failure mode observable in production. Not for this PR.
3 participants
Summary
Changes
.gitignorecrates/trusted-server-core/src/settings.rsreject_placeholder_secrets.crates/trusted-server-core/src/http_util.rsFastly-Client-IPas spoofable after authenticated consumption and provide a shared sanitizer for configured trust headers.crates/trusted-server-adapter-fastly/src/platform.rscrates/trusted-server-adapter-fastly/src/compat.rsresolve_and_sanitize_client_ipso their ordering cannot be reversed.crates/trusted-server-adapter-fastly/src/main.rsClientInfo.crates/trusted-server-adapter-fastly/src/middleware.rsClientInfofor response geolocation, including authoritative absence.crates/trusted-server-adapter-fastly/src/app.rsClientInfoaddress reaches request-scoped services and the EC finalization context.crates/trusted-server-adapter-axum/src/middleware.rscrates/trusted-server-adapter-cloudflare/src/middleware.rscrates/trusted-server-adapter-spin/src/middleware.rstrusted-server.example.tomldocs/guide/configuration.mddocs/guide/fastly.mdunsetrequirement and keeping the front-door copy of the secret out of inline VCL.docs/superpowers/specs/2026-08-19-trusted-client-ip-header-design.mddocs/superpowers/plans/2026-08-19-trusted-client-ip-header.mdSecret storage
Redacted<String>prevents the shared secret from appearing in debug output and validation errors, but it does not move the value into a platform secret store. With the current configuration architecture,ts config pushserializesshared_secretinto the Trusted Server application-config blob.Fastly recommends Secret Store, rather than Config Store, for sensitive values. Migrating this field requires changing the shared configuration schema to hold a secret-store reference and resolving that reference in the Fastly request path. The broader migration of existing Trusted Server passphrases and secrets is tracked by #846 and remains outside this PR.
The fronting copy is a separate concern: a Fastly VCL/CDN service cannot read a Compute Secret Store. That service must obtain the identical value through a VCL-accessible mechanism such as a tightly restricted private/write-only edge dictionary, and must overwrite both incoming trust headers before forwarding.
Fronting VCL deployment requirement
Code configuration alone does not enable trusted client-IP forwarding. The public/fronting VCL service—not the Trusted Server Compute service—must discard client-supplied trust headers and recreate them on the first visit:
This pasteable example is for a dedicated VCL service whose requests all go to Trusted Server. A shared service must apply the setup only on its Trusted Server route, strip client-supplied authentication on other routes, and never send the dictionary value to unrelated backends. If another CDN precedes Fastly, restrict direct access and use that CDN's protected reader-IP value instead of
client.ip. The dictionary value must exactly matchtrusted_client_ip.shared_secret.Scope note:
X-Forwarded-ForThis PR does not change cross-adapter handling of client-supplied
X-Forwarded-For. An earlier partial hardening change was removed after review because it covered Fastly and Spin but not Cloudflare and Axum. Consistent reconstruction ofX-Forwarded-Forfrom authoritativeClientInfoshould be handled separately.Closes
Closes #1041
Fixes #1040
Test plan
cargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spincargo clippy-fastlycargo clippy-axumcargo clippy-cloudflarecargo clippy-cloudflare-wasmcargo clippy-spin-nativecargo clippy-spin-wasmcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runusing repository-pinned Node 24.12.0cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1Checklist
unwrap()in production codelogmacros, notprintln!