Repository navigation
Conversation
Specify a provider-neutral JA4 accessor (ctx.ja4()) backed by adapter-owned RequestIngress metadata: Fastly and Cloudflare populate it from their trusted runtime APIs, Spin and Axum report NotExposed, and no adapter derives JA4 from request headers. Refs #283
Per the user, ID-7 relies on a trusted JA4 where the adapter supplies one and otherwise does not block record creation (DEC-18), and JA4 is an EdgeZero abstraction, ctx.ja4(), filled per adapter: Fastly from runtime TLS, Cloudflare from Bot Management, Axum from a trusted proxy header such as CloudFront's, Spin never. Trusted Server reads JA4 only through it, so T12 deletes the direct Fastly read. IABTechLab/edgezero#469's draft leaves Axum NotExposed and defers the trusted proxy header to its section 13.1; the plan asks for that section in R2.
|
CREATOR: From the server-side identity plan (IABTechLab/trusted-server#1559, Part of #283 |
Add edgezero_core::ja4 with a validated, Debug-redacted Ja4Fingerprint and CapturedJa4 provenance/absence states. RequestIngress carries an optional capture and RequestContext::ja4() reads it. Fastly and Cloudflare populate it from their runtime TLS APIs; Spin and Axum report NotExposed. No adapter derives JA4 from request headers. Includes the implementation plan and adapter documentation. Refs #283
Add a fixed ja4_state label to the request-fidelity fixture hook and send spoofed ja4 / cloudfront-viewer-ja4-fingerprint headers in its spoof case. The runner asserts Fastly and Cloudflare report missing and Spin reports not_exposed on the real local runtimes, covering Spin's into_core_request path that the plain-wasmtime contract suite cannot construct. Replace magic section offsets in ja4.rs with named constants shared by parse and the accessors, use matches! byte ranges, share one FoxIO sample constant across core tests, and move test imports to module level. Refs #283
dhruv8sh
changed the title
Axum never sees the TLS handshake, but a TLS-terminating proxy in front of it (FoxIO's nginx module, HAProxy, ATS, CloudFront) can compute JA4. Add Ja4Source::TrustedProxyHeader and CapturedJa4::from_trusted_proxy_header, and an Axum TrustedJa4Header that records the configured header only when the direct TCP peer is inside an operator CIDR allowlist. Any other peer stays NotExposed, so clients cannot forge it. Configured with EDGEZERO__ADAPTER__JA4_HEADER and EDGEZERO__ADAPTER__JA4_TRUSTED_PROXY_CIDRS, or programmatically via AxumDevServer / EdgeZeroAxumService::with_trusted_ja4_header. Config fails closed on partial settings, invalid entries and /0 networks. Default behaviour is unchanged. Spec §5.8, plan task 6 and the Axum guide document the design, the operator requirement to overwrite the header, and an nginx example. Refs #283
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
A well-structured design: one adapter-owned ctx.ja4() with explicit provenance and explicit "no fingerprint" states, and request headers are never trusted by default. The new Axum trusted-proxy path is careful and fails closed.
I'm requesting changes for three reasons:
- The docs present CloudFront as a source for a plain IP allowlist, and any CloudFront distribution can pass that check.
- The Fastly/Cloudflare capture path still has no test that could fail if it broke.
- The separate JA4 proxy allowlist departs from the requested #409/#439 trust check, and that needs a maintainer decision.
😃 Praise
- The Axum trust check is well protected. I applied five deliberate breaks, and each was caught:
- Trust any peer: caught by the peer-matrix test and the real-TCP dev-server test.
- Trust an unknown peer: caught.
- Skip IPv4-mapped canonicalization: caught.
- Allow
/0networks: caught. - Ignore the config in
into_core_request: caught by the real-TCP test.
- Config fails closed on partial settings, invalid entries and
/0. A repeated header field is treated asMalformedrather than picking one. Ja4Fingerprint::parseis tight. It uses named offsets, never echoes its input, and has good rejection cases.- The request-fidelity spoof case is strong real-runtime evidence. At
a3afa231, the CI logs showja4_stateasserted in 16 Fastly, 12 Cloudflare and 13 Spin results, checked against the committedobservations.json. - On every production Fastly path, the ingress snapshot is inserted after hook values are merged (
request.rs:330,:453).
Findings
Blocking
- 🔧 CloudFront is listed as a source for the IP allowlist, which any CloudFront distribution can pass. See inline on
docs/guide/adapters/axum.md. - 🔧 The tests can't tell real Fastly/Cloudflare JA4 wiring from a constant
Missing. See inline oncrates/edgezero-adapter-cloudflare/tests/contract.rs. - ❓ A separate JA4-only proxy allowlist. aram356 asked for JA4 "from a hop that #409 or #439 checks". #409 adds
EDGEZERO__ADAPTER__TRUSTED_PROXY_CIDRS, and this PR deliberately adds a secondEDGEZERO__ADAPTER__JA4_TRUSTED_PROXY_CIDRS. Spec §5.8 explains why: it's a narrower claim, and it avoids depending on when #409 merges. That's reasonable, but operators would maintain two IP lists for the same proxies, and the lists can drift. It also departs from the stated requirement. @aram356, is the separate setting acceptable, or should this path wait for #409'sCheckedProxyMetadata?
Non-blocking
- 🤔 Every Fastly request now loads the full client TLS metadata bundle. See inline.
- 🤔 An empty provider value is classified as
Malformed. See inline. - ⛏ Some parser rules and the redaction of section a are not protected by tests. See inline.
- 🤔 Spec §10 lists Fastly and Spin tests that weren't added. See inline.
- ⛏ Nothing tests that
run_appreads the JA4 environment variables. See inline. - 📝 The nginx warning misses the
proxy_set_headerinheritance trap. See inline. - ⛏ The overview matrix is slightly wrong for configured Axum. See inline.
- 📝 The PR description says "app hooks can't overwrite the value". That holds for the Fastly raw-request hook only. Pre-dispatch hooks receive
&mut Requestand can replace extensions. The overview doc and spec §5.7 already state this correctly.
CI Status
- Hosted checks for
304f5a42were still queued when this review was written. - I ran these locally on
304f5a42:cargo test -p edgezero-core --lib: 511 passed.cargo test -p edgezero-adapter-axum --all-targets --features axum: 131 passed.- My local Rust is 1.99.1; the repo pins 1.98.1.
- I did not re-run the wasm suites locally. Their code is unchanged since
a3afa231, where they passed in CI. - No live Fastly or Cloudflare
Capturedresult has been verified yet.
|
|
||
| When a TLS-terminating proxy in front of Axum computes JA4 (FoxIO's | ||
| [ja4-nginx-module](https://github.com/FoxIO-LLC/ja4-nginx-module), HAProxy's | ||
| ja4 module, the Apache Traffic Server JA4 plugin, CloudFront, …), Axum can read |
There was a problem hiding this comment.
🔧 CloudFront can't be safely identified by an IP allowlist.
CloudFront's IP ranges are shared by every AWS customer. Anyone can create their own distribution that points at this origin. Its requests arrive from an allowlisted CloudFront IP, so they pass the peer check. That distribution can also add arbitrary headers through its "origin custom headers" setting.
With CLOUDFRONT ranges in JA4_TRUSTED_PROXY_CIDRS, an attacker could therefore get any value they like recorded as Captured { source: TrustedProxyHeader }. This is why #439 has a separate protected-CloudFront check. I haven't checked whether CloudFront rejects a custom header named CloudFront-Viewer-*, but the operator can choose any header name.
Suggested fix: remove CloudFront from the allowlist-path examples here, in the ja4.rs module doc, in spec §5.8 and in the PR description. Alternatively, state that CloudFront needs #439's protected-CloudFront check, which this path doesn't provide. nginx, HAProxy and ATS are fine examples because the operator owns them.
| } | ||
|
|
||
| #[wasm_bindgen_test] | ||
| async fn ingress_ja4_is_missing_without_cf_despite_header() { |
There was a problem hiding this comment.
🔧 These tests can't distinguish real JA4 wiring from a constant Missing.
This test (and the Fastly one, ingress_ja4_is_missing_for_non_client_request_despite_header) uses input where the provider can't supply a fingerprint:
- Cloudflare: there is no
cfobject. - Fastly:
FastlyRequest::get(...)is not a client request, and fastly 0.13.1'sget_client_tls_metadata()returnsNoneimmediately (if !self.is_from_client()). - Real-runtime fixture: it serves plain HTTP, so both providers report
missingthere as well.
So replacing the adapter wiring with CapturedJa4::from_runtime_tls(None) still passes the whole suite. The cf → bot_management → ja4 chain and the adapter-level Captured/Malformed outcomes are never executed.
Suggested fix (Cloudflare): worker::Request::from(web_sys::Request) reads cf through a JS getter (worker-sys 0.8.5, ext/request.rs). That means a test can inject one before converting the request:
let bot = js_sys::Object::new();
js_sys::Reflect::set(&bot, &"ja4".into(), &"t13d1516h2_8daaf6152771_e5627efa2ab1".into()).unwrap();
let cf = js_sys::Object::new();
js_sys::Reflect::set(&cf, &"botManagement".into(), &bot).unwrap();
js_sys::Reflect::set(&raw, &"cf".into(), &cf).unwrap();
// expect Some(Captured { source: RuntimeTls, .. }), then repeat with an invalid value → MalformedI traced this from the source but haven't run it, so it's worth confirming in the browser runner.
Fastly: this path can't be exercised without a TLS client. Please either note that limit explicitly or complete the live-provider check before merge.
| None | ||
| }; | ||
| RequestIngress::new(target, origin, HeaderFidelity::default()) | ||
| .with_ja4(CapturedJa4::from_runtime_tls(req.get_tls_ja4())) |
There was a problem hiding this comment.
🤔 This now loads the whole client TLS metadata bundle on every Fastly request.
In fastly 0.13.1, get_tls_ja4() goes through get_client_tls_metadata(), and its lazy load fetches all of these together:
- the raw ClientHello
- SNI, JA3 and JA4
- the raw client certificate and its verification result
- the cipher name and protocol
On main, the adapter never touched TLS metadata. Because capture_request_ingress runs on every dispatch path, every app now pays for those host calls and buffer copies, even if it never calls ctx.ja4(). Every app also now runs the SDK's panic!s in that bundle (for example, "must be valid UTF-8" on SNI and JA4).
The cost is probably small, but the spec doesn't mention it. Could you note this trade-off in the spec, or measure it, so it's a deliberate decision?
| /// Map a runtime TLS accessor's result. Invalid values are discarded. | ||
| #[inline] | ||
| #[must_use] | ||
| pub fn from_runtime_tls(value: Option<&str>) -> Self { |
There was a problem hiding this comment.
🤔 An empty provider value becomes Malformed, not Missing.
Cloudflare's JA3/JA4 docs say the value "can be null or empty" in some cases, for example when Bot Management is skipped or a TLS session is resumed. That statement is about logs; I haven't confirmed what Workers expose. worker-sys maps a JS "" to Some(""), so validated("") fails and the result is Unavailable(Malformed). An empty trusted-proxy header field behaves the same way.
The spec presents Malformed as a signal that the provider changed its format. Apps that treat it as suspicious would then flag ordinary requests.
Suggestion: treat an empty value as Missing, for example by filtering value.filter(|v| !v.is_empty()) before validating. Please also add a test for from_runtime_tls(Some("")).
| // Cipher and extension counts. | ||
| 4..=7 => byte.is_ascii_digit(), | ||
| // First and last ALPN characters. | ||
| 8 | 9 => byte.is_ascii_alphanumeric(), |
There was a problem hiding this comment.
⛏ Some parser rules and the redaction of section a aren't protected by tests.
I applied each of these one at a time and ran cargo test -p edgezero-core --lib. All passed:
- ALPN positions
8 | 9 => true - version positions
1 | 2 => true - a
Debugthat printsJa4Fingerprint([redacted] t13d1516h2)
The protocol, SNI, count and uppercase-hex mutations were caught. The redaction tests only check that the full string and 8daaf6152771 are absent, but the spec says Debug never contains the fingerprint.
Suggestion:
- Add rejection cases such as
t13d1516h_…(ALPN) andt_3d…/tX3d…(version). - Assert that
!output.contains("t13d1516h2")inja4_never_prints_fingerprint_or_inputand in theRequestIngressdebug test.
| | ----------------------- | ---------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Core `ja4.rs` | native `cargo test -p edgezero-core` | • Accepts FoxIO's example `t13d1516h2_8daaf6152771_e5627efa2ab1`, a `q` value, a `d` value, `i` SNI, `000000000000` sections and uppercase ALPN characters.<br>• Section accessors return `t13d1516h2`, `8daaf6152771` and `e5627efa2ab1`.<br>• Rejects: empty, 35 and 37 bytes, leading or trailing whitespace, uppercase hex in `b` or `c`, wrong or missing separators, an unknown protocol or SNI letter, non-digit counts, a non-ASCII value of the right byte length, a 32-character JA3 hash and a `ja4_r` string.<br>• `from_runtime_tls` for `None`, valid and invalid input.<br>• `Debug` output of `Ja4Fingerprint`, `CapturedJa4` and `RequestIngress` never contains the fingerprint, and `parse` errors never contain the input. | | ||
| | Core `ingress.rs`, `context.rs` | native | • `RequestIngress::new(…).ja4()` is `None`; `with_ja4` round-trips.<br>• `RequestContext::ja4()` is `None` without `RequestIngress` and returns the stored value when present. | | ||
| | Fastly | Viceroy (`wasm32-wasip1`) | • `capture_request_ingress(&FastlyRequest::new(…))` gives `Some(Unavailable(Missing))`.<br>• Through `dispatch_with_registries`, a raw-request hook that inserts a `RequestIngress` with a fake captured JA4 does not change what the handler sees (modelled on the existing hook-override test at `crates/edgezero-adapter-fastly/src/request.rs:1153`).<br>• A client `ja4` header does not produce `Captured`. | |
There was a problem hiding this comment.
🤔 This row lists tests that aren't in the PR.
The Fastly row describes a raw-request hook that inserts a RequestIngress with a fake captured JA4 through dispatch_with_registries. The Spin row describes a wasmtime contract test with a ja4 header. Neither was added.
The behaviour is covered elsewhere:
- Fastly: the generic
ingress_registry_dispatch_wins_scratch_collisiontest protects the replacement order. - Spin: the real-runtime fixture covers
into_core_request.
R5's acceptance points at this table, so please either add the Fastly test or update these rows to name the coverage that exists.
| log::warn!("{warning}"); | ||
| } | ||
| let addr = resolution.addr; | ||
| let trusted_ja4_header = TrustedJa4Header::from_env(&env)?; |
There was a problem hiding this comment.
⛏ Nothing tests that run_app reads the environment variables.
I replaced this line with let trusted_ja4_header: Option<TrustedJa4Header> = None;, and the Axum suite (131 tests) and the CLI suite still passed. from_env itself is unit-tested, and the PR description mentions a manual edgezero demo check.
What isn't tested automatically is the documented operator path: set both variables and capture works, or set only one and startup fails. A small test through run_app or serve_with_stores with an EnvConfig would cover it.
| omits it when the value is empty: | ||
|
|
||
| ```nginx | ||
| proxy_set_header X-JA4 $http_ssl_ja4; |
There was a problem hiding this comment.
📝 Worth adding the nginx inheritance trap to this warning.
proxy_set_header directives are inherited from an enclosing block only when the current block defines none. So a location that sets its own proxy_set_header Host … silently drops an X-JA4 defined at the server or http level. The client's own X-JA4 then reaches Axum from a trusted peer.
I'm going by nginx's documented inheritance rule here; I haven't run it. Suggested addition: "Set it in every block that has its own proxy_set_header, and in every server block that proxies to Axum, including plain-HTTP ones."
| | Fastly | `Captured { RuntimeTls }` | `Unavailable(Missing)` | `Unavailable(Malformed)` | | ||
| | Cloudflare | `Captured { RuntimeTls }` with Enterprise Bot Management; otherwise `Unavailable(Missing)` | `Unavailable(Missing)` | `Unavailable(Malformed)` | | ||
| | Spin | `Unavailable(NotExposed)` | `Unavailable(NotExposed)` | n/a | | ||
| | Axum | `Unavailable(NotExposed)`; with a [trusted proxy](./axum#ja4-from-a-trusted-proxy), `Captured { TrustedProxyHeader }` | `Unavailable(NotExposed)` | `Unavailable(Malformed)` | |
There was a problem hiding this comment.
⛏ The Axum row is slightly wrong when a proxy is configured.
With a trusted proxy configured, a request from that proxy without the header gets Unavailable(Missing), not NotExposed. The "Plain HTTP" column (Axum is always plain HTTP) only shows NotExposed. Spec §7 states this correctly, as "From a trusted peer: … or Missing without the header".
Both workspaces depend on edgezero-adapter-axum by path, so adding ipnet to it requires their lockfiles to record the new edge. CI runs them with --locked. No new packages are resolved. Refs #283
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
One item, found while bringing this PR into rc/identity (#495). It is the only reason #495's CI is red.
The inline comment carries a one-click GitHub
suggestion. Use Commit suggestion to apply it.
Blocking
- 🔧 The Axum guide names
EdgeZeroAxumService, which #275 removes (docs/guide/adapters/axum.md:350-351)
| pass it to `AxumDevServer::with_trusted_ja4_header` or | ||
| `EdgeZeroAxumService::with_trusted_ja4_header`. |
There was a problem hiding this comment.
🔧 wrench: EdgeZeroAxumService does not exist once this meets #275. #275 removes the type (downstream response delivery is adapter-owned) and adds scripts/check_outbound_legacy_api.sh, which rejects the name anywhere in docs/guide. This PR targets main, where the type still exists, so its own CI has no such check and stays green.
Merging this PR into rc/identity (0ea3e7bc) turned #495's workspace tests job red at the "No legacy outbound API" step, on exactly this line (job log). The previous rc/identity head, 3964365e, was green. In that merge the builder moved to the crate-private AxumServiceState (service.rs:268 on rc/identity), so AxumDevServer::with_trusted_ja4_header is the only public way to set it there.
Dropping the second name is safe on main too: AxumDevServer::with_trusted_ja4_header is public on both bases (dev_server.rs, through pub mod dev_server). When this rebases onto #275, move the service.rs builder off EdgeZeroAxumService the same way the rc/identity merge did.
| pass it to `AxumDevServer::with_trusted_ja4_header` or | |
| `EdgeZeroAxumService::with_trusted_ja4_header`. | |
| pass it to `AxumDevServer::with_trusted_ja4_header`. |
There was a problem hiding this comment.
Applied the suggestion in 52f4a09f.
Brings #275 at e9ffaa8 and the refreshed heads of #499, #460, #406, #407, #409, #414, #422 and #439. Each PR merged the one below it, so this merge has no conflicts. Two edits fit #469 to #275, as #469's open review asks: - #469's ingress JA4 test calls into_core_request with one argument. #275 now passes the Axum outbound executor through that function, so the test gets an executor handle the way the other ingress tests do. - The Axum guide names EdgeZeroAxumService, which #275 removes and check_outbound_legacy_api.sh rejects. The guide now names only AxumDevServer::with_trusted_ja4_header.
Summary
ctx.ja4() -> Option<&CapturedJa4>, backed by the adapter-ownedRequestIngressmetadata from Add pre-dispatch hooks and explicit request ingress fidelity #403. Today the only way to get JA4 is the Fastly-only raw-request hook, and apps can't tell "this platform never supplies JA4" from "this request has none".get_tls_ja4()) and Cloudflare (Bot Managementja4) populate it. Spin reportsNotExposedbecause it never sees the TLS handshake. Axum reportsNotExposedby default. It can instead read JA4 from a TLS-terminating proxy's header (FoxIO's nginx module, HAProxy, ATS, CloudFront), but only when the direct TCP peer is in an operator-configured CIDR allowlist. Client-supplied headers are never trusted, and app hooks can't overwrite the value.Changes
crates/edgezero-core/src/ja4.rs(new)Ja4Fingerprint(strict 36-bytea_b_cvalidation,summary/cipher_hash/extension_hash, redactedDebug, noDisplay/Serialize),CapturedJa4(Captured { fingerprint, source }orUnavailable(reason)),Ja4Source::RuntimeTls,Ja4Unavailable::{Malformed, Missing, NotExposed},CapturedJa4::from_runtime_tls,Ja4Source::TrustedProxyHeader,CapturedJa4::from_trusted_proxy_header.crates/edgezero-core/src/ingress.rsRequestIngressgainswith_ja4/ja4().Nonemeans the adapter made no claim.Debugshows the state, never the value.crates/edgezero-core/src/context.rsRequestContext::ja4().edgezero-adapter-fastlycapture_request_ingressrecordsget_tls_ja4().edgezero-adapter-cloudflareinto_core_requestrecordscf().bot_management().ja4().edgezero-adapter-spinUnavailable(NotExposed).edgezero-adapter-axumja4.rs:TrustedJa4Header(header name + CIDR allowlist viaipnet2, already in the lockfile). Configured byEDGEZERO__ADAPTER__JA4_HEADER+EDGEZERO__ADAPTER__JA4_TRUSTED_PROXY_CIDRS, orAxumDevServer/EdgeZeroAxumService::with_trusted_ja4_header. Fails closed on partial config, invalid entries and/0. Without config it recordsNotExposedas before.tests/fixtures/request-fidelity{,-runner}ja4_statelabel. Thespoofcase sendsja4andcloudfront-viewer-ja4-fingerprintheaders. The runner asserts the per-platform state. Observations re-recorded; the only change is the new field.docs/guide/adapters/*.mddocs/superpowers/{specs,plans}/Closes
Closes #283
Test plan
cargo test --workspace --all-targets: 1566 passedcargo clippy --workspace --all-targets --all-features -- -D warningson the pinned Rust 1.98.1, plus clippy onwasm32-unknown-unknown(Cloudflare),wasm32-wasip1(Fastly) andwasm32-wasip2(Spin)cargo fmt --all -- --check, and for both fixture workspacescargo check --workspace --all-targets --features "fastly cloudflare spin"wasm-bindgen-test-runner+ Chromium (14 passed); Spin contract under wasmtime 48.0.1 (12 passed)examples/app-demoworkspace: 33 passedcd docs && npm run lint && npm run format && npm run buildMissing/Malformed, and config rejection; a real-TCP dev-server test where a loopback client is captured under127.0.0.0/8andNotExposedunder192.0.2.0/24edgezero demo(Axum) servesGET /→ 200 with and without the JA4 config, and refuses to start with only one JA4 variable set or with0.0.0.0/0./scripts/test-request-fidelity.sh allagainst real Spin 4.1.0, Viceroy 0.21.0 and workerd. JA4 state ismissingon Fastly and Cloudflare (local runtimes serve plain HTTP) andnot_exposedon Spin, including the spoofed-header case. This covers Spin'sinto_core_requestpath, which the plain-wasmtime suite can't construct.Captured, over HTTP →Missing; Cloudflare zone with Bot Management →Captured, without →Missing. Also confirms providers emit lowercase hex.Checklist
{id}syntax (not:id)edgezero_core(nothttpcrate)KvRegistry/ConfigRegistry/SecretRegistry(not the legacy single-handle setters) — see spec §6.6