Repository navigation
Add pluggable native Geo providers and protected CloudFront ingress - #439
ChristianPavilonis wants to merge 18 commits into
Conversation
72ceed7 to
c8ee48f
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Reviewed at 3a0e5eb4. The portable Geo outcome and socket-only peer admission hold up well. Requesting changes for one new question: the credential header ordering from add18fc3 contradicts the Geo guide and the design spec. The earlier CI-guard finding is also still open.
😃 Praise
- I couldn't find any way to forge the Geo outcome. No setter, extension, state or header path replaces it, and the core and contract tests check this for every adapter.
- Invalid Geo selections are rejected in
run_production_selectedbefore any listener, runtime or store is created. - Peer admission uses only the owned socket peer, and IPv4-mapped peers and networks are normalized correctly.
/0and whole-family unions are rejected.
Findings
Blocking
- ❓ Credential header is visible to the received-head preparer before it is stripped (
crates/edgezero-adapter-axum/src/service.rs:313). See the inline comment. - 🔧 Still open: the Geo CI steps in
.github/workflows/test.ymlstill lack thematrix.suite == 'workspace'guard. See the earlier comment on line 189.
Non-blocking
- 🤔 No test shows that forwarding trust can't admit Geo.
docs/guide/native-geo.mdsays that trusted-proxy configuration and forwarding headers can't create Geo source admission. The code is correct:ProtectedCloudFrontAlb::admitonly sees the socket peer.- However, every Geo unit and hosting test runs with
TrustedProxyPolicy::no_trustand sends noX-Forwarded-For/Forwarded. An implementation that admitted on the effective client IP would pass the whole suite. - Suggested test: trust
127.0.0.1as a proxy, set the Geo policy to192.0.2.1/32, sendX-Forwarded-For: 192.0.2.1, and expectUntrustedIngress.
- ⛏ The reserved credential names don't match their doc comment (
crates/edgezero-adapter-axum/src/geo.rs:144). See the inline comment. - ⛏ Nothing pins the detailed JSON error on the
Defaultpath (crates/edgezero-core/src/app.rs:337). See the inline comment. - 📝 The
#[non_exhaustive]follow-up covers more types than reported. In addition toGeoInput/GeoSource/GeoTrustBasis, these public types are also exhaustive:GeoOutcome,GeoStatus,GeoResolution,GeoError,GeoValueErrorandGeoUnavailableReason. - ⛏ The PR description is stale again. It names
414e52aeas the current head; the head is now3a0e5eb4. The earlier duplicatedep:ipnet/dep:getrandomnit inCargo.tomlalso remains.
CI Status
These checks ran locally at 3a0e5eb4 with Rust 1.98.1:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS, 2902 passed and 0 failed - Hosted checks: all green except
cache-restore-seed. That job fails the same way on the basebb56cfe1(a GHA cache write error), so it isn't caused by this PR.
The Geo feature-matrix commands, the WASM runtime tests and the dependency script were not rerun locally.
| let received = | ||
| validate_normalized_ingress_parts_with_summary(parts, self.app.ingress_head_limits()); | ||
| if received.is_ok() { | ||
| self.app |
There was a problem hiding this comment.
❓ Credential header is visible to application code before it is stripped
sanitize_received_head calls prepare_received_head(&parts.headers, ..), and only afterwards calls self.geo.strip_credential(..). The preparer is the application-supplied closure registered with RouterBuilder::received_head_preparer. In protected-ALB mode it therefore receives the origin credential header and its value.
This contradicts the documented guarantee:
docs/guide/native-geo.md:253: "strips every occurrence before application callbacks".docs/guide/native-geo.md:303: "without exposing name or value to application code".- The design spec, line 118: "Remove every occurrence of the configured credential header before application admission callbacks, request preparers or middleware run".
The router.rs rustdoc says the opposite ("before stripping forwarding fields or provider credentials"), so this looks deliberate in add18fc3. Which contract is intended?
Scenario: an app registers a preparer that logs or clones received headers for auditing. The origin-verify secret then ends up in the application's logs or extensions.
Fix: if the preparer has no need to see the credential, strip it first and leave forwarding-field stripping where it is:
self.geo.strip_credential(&mut parts.headers);
let received =
validate_normalized_ingress_parts_with_summary(parts, self.app.ingress_head_limits());
if received.is_ok() {
self.app
.prepare_received_head(&parts.headers, &mut parts.extensions);
}
receivedMoving the strip before validation also keeps the credential out of the error path. Please check that the original header bytes still count toward the head limits, as the PR intends.
Otherwise, update the guide and the spec to say that preparers see the credential and must not keep it. Either way, add a test that sets a preparer together with a Geo selection. No current test covers that ordering.
| /// Validate a nonempty bounded literal peer allowlist and dedicated header name. | ||
| /// | ||
| /// Names must be literal HTTP tokens of at most 128 ASCII bytes beginning with | ||
| /// `x-`. Forwarding, AWS, `EdgeZero`, real-IP and correlation names are reserved. |
There was a problem hiding this comment.
⛏ The doc comment says EdgeZero names are reserved, but the code only rejects the x-edge- prefix
x-edgezero-* doesn't start with x-edge- (the next character is z, not -). So a name like x-edgezero-proxy, which edgezero-core/src/outbound.rs:80 uses, is accepted as a credential name and would then be stripped from inbound requests. Either add x-edgezero- to the reserved prefixes or reword the comment.
| .dispatch_resolved_outcome( | ||
| resolved, | ||
| request, | ||
| admitted.with_request_context(geo, detail == FrameworkErrorDetail::CategoryOnly), |
There was a problem hiding this comment.
⛏ Nothing pins the detailed JSON error message on the Default path
The redaction flag is only set for CategoryOnly (Axum). Fastly, Cloudflare and Spin go through dispatch_admitted/Default and should keep invalid JSON payload: <serde detail>.
No test asserts that suffix. The existing assertion around context.rs:2211 uses .contains("invalid JSON payload"), which also matches the redacted message. An inverted or always-true flag would silently remove error detail on all three WASM adapters with the suite still green.
The code is correct today; this is a coverage gap. One Default-path assertion that the message keeps the ": " detail would close it.
|
CREATOR: From the server-side identity plan (IABTechLab/trusted-server#1559, |
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.
Current review readiness
Ready for review. Required current-head CI passed at
414e52ae41ee95880fd11238a22ad2dc944f44c2: test run 42924133045.Minimal fixes originate in #414 and are merged upward:
Deadline; non-timeouts remainUnavailable. The previously failing hosted timeout test and all 78 provider cases now pass without assertion, retry or duration changes.aws-types =1.6.0andaws-smithy-runtime-api =1.19.0. This prevents the schema mismatch and preserves the explicit older behavior version instead of switching retry policy or suppressing deprecations. A fresh consumer passes with compiler and Clippy warnings denied throughout.Local final-top Axum checks pass 488 unit, 28 contract, six AWS startup, four celld startup and 36 hosting cases. Final-top workspace strict Clippy and formatting pass. The prior stable checkpoint passed all six mandatory commands; unchanged inputs retain their receipts, while fresh current-head CI checks the combined state. The current ready state supersedes historical draft-only prerequisites below; unexecuted deployment acceptance is not claimed complete. An additional Fastly WASM runtime job was still running at transition and is not counted as passed. No TS work or deployment qualification is a review prerequisite. Integration #462 remains draft; reviewers
aram356,prk-Jranddhruv8shremain requested. Stack465 and PR bases are unchanged; #453 is excluded.Current review-stack checkpoint
Publication head:
414e52ae41ee95880fd11238a22ad2dc944f44c2. This cleanup preserves the existing feature unit and PR identity. The additional change is the completed generated-native build correction: image-only code now follows the scaffold's existing Fastly-example selection, with no request-policy change. Its focused generator/native proof is retained; fresh per-branch hosted checks are not assumed green.Review order: #275 → #406 → #407 → #409 → #414 → #422 → #439 → #454. Draft integration checkpoint: #462. New SDK/group/codec/phase development and unrelated TS work are not prerequisites or additions to this PR. Earlier notes below retain their original tested revisions and limits; they are not a whole-tree pass for this new head.
Earlier implementation and proof notes
RC refresh checkpoint, 2026-11-10
Current head:
c8ee48fcea9828095640a0fa8e593fb59c6d57fa.Geo-only behavior is ported onto current #409/#422 instead of restoring repeated older dispatch/body code. Built-ins remain synchronous and local. One private outcome is captured from owned ingress and retained through the managed pre-dispatch seam. Native
trusted_proxynaming keeps the outbound hard-cut guard byte-unchanged, without the old exemption.Feature core/native/all-feature, Geo-wire and disabled-feature checks, dependency isolation, strict Clippy, docs/actionlint/format pass. Independent source review found no credible introduced defect. Lower CI/fixture fixes are merged; all five locked fetches and 35 CI tooling cases pass.
Protected CloudFront is a composition: ALB validates the credential value externally; EdgeZero checks the actual socket-source policy and strips the selected credential header, including normalized-head errors, before relevant application callbacks. No live ALB/CloudFront source qualification is claimed. Arbitrary unmanaged
ctx.into_requestplus a fresh context deliberately resets Geo to NoSelection. This adds no production KV Geo provider or local MaxMind implementation. The admitted-error finalizer is a separate follow-on.Draft pending fresh hosted checks and the combined RC audit. Two unchanged native default-parallel deadline misses remain recorded and unclassified; serial passes and untouched-foundation parallel passes do not clear them. No timing bound was relaxed. No deployment, main merge, security-alert dismissal or review-thread resolution occurred. Historical design/review context follows.
Summary
Stack and prerequisite reconciliation
Base branch:
feat/native-celld-stores, draft #422 at5426ea65. This PR stays on top of the existing native celld stack.Although the GitHub stack names #409 as an ancestor, this base did not contain its checked-ingress implementation. This diff therefore also reconciles the pinned
66a05e25checked-ingress/proxy/diagnostics contract. It adapts that commit's own delta while preserving newer native stores, local non-Send initialization,Connections/FuturesUnordered, bounded ingress/config snapshots and asynchronous response EOF validation. The existing forwarding parser supplies the shared bounded peer-network matcher.The distinct #407 buffered JSON-cap/managed Body feature is not imported or claimed. Native JSON error privacy is retained through private admitted/context state. The plan records exactly which #407-dependent cap/carrier checks are excluded. No old branch tree replaced newer files.
Changes
edgezero-core::geoctx.geo(), default NoSelection on legacy/unmanaged paths, preserved framework error privacy and egress classification.AxumRunOptions, capability inspection through finalized options, startup rejection before runner resources, native-only Arc thread bounds.Issue scope
Closes #432. Implements the CloudFront and synchronous Geo contract under #391. Local GeoIP databases, checked-reader-IP input and database lifecycle are tracked separately in #438 and are not included in this PR.
Trusted Server adoption and jurisdiction/consent interpretation remain separate. Geo is an estimate, not proof of residence, identity or consent. Country-only and unrecognized subdivisions stay unknown to application policy.
Test plan
cargo test --locked --workspace --all-targets: 2,115 passed, 6 existing generated-consumer/real-service tests ignored.cargo test --locked -p edgezero-adapter-axum --all-targets --all-features: 422 passed, including mixed AWS/celld startup, preserved lifecycle/EOF tests and 32 hosting tests. Provider service fixtures are loopback-only.cargo clippy --locked --workspace --all-targets --all-features -- -D warnings, disabled-native Clippy,cargo fmt --all -- --check,git diff --check.fixture-axumchecked against its updated lockfile.edgezero serve --adapter axum. Programmatic production-runner and real-listener tests were used instead; no Geo CLI selector is introduced.Tested implementation is
56eab85f; the later commit records evidence/status only. Exact commands, tested feature sets, timings, local logs and ignored-test limits are in the implementation plan. No failing local gate is excluded.Trust and acceptance limits
The protected policy declares an externally credential-gated and isolated ALB path. A matching subnet does not authenticate a distribution or prove that its listener rules are correct. EdgeZero does not receive the configured credential value or call AWS to inspect/provision infrastructure.
Production use still requires deployed proof of the credential gate, target isolation, viewer-header replacement including AWS-network missing-subdivision spoofing, value rotation and shared-cache/error-TTL isolation. The guide requires HTTPS, a default-deny ALB, CloudFront origin-facing prefix-list restrictions, private target access, explicitly generated geographic headers and disabled shared caching. No deployment was attempted.
The six existing ignored generated/service harnesses were not counted as passes. Unchanged docs dependencies report seven npm audit vulnerabilities; no unrelated dependency upgrades were attempted. ShellCheck was unavailable; Bash syntax, behavioral failure cases and real graph checks passed.
Checklist
CLAUDE.mdconventions and scoped ownership.Owned follow-up review checkpoint
6365062820d924b314fc52930ccbdbe252fb6b49. Source treee229fa851df5edc0ec751bb523eab18623191d1eis byte-identical to previous head3a0e5eb43cdb9e20076b47fcede31d3e1b52f862.