Repository navigation
Add pluggable native Geo providers and protected CloudFront ingress - #439
ChristianPavilonis wants to merge 10 commits into
Conversation
72ceed7 to
c8ee48f
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds portable Geo outcome types in core, protected CloudFront/ALB source admission in native Axum, and a deployment guide. Requesting changes for one design question (raw geographic headers stay readable to application code) and a CI guard mistake. The rest is cleanup, including two pieces of core plumbing that I confirmed are unneeded by disabling each one and re-running the tests.
Findings
Blocking
- ❓ Raw
CloudFront-Viewer-*headers stay readable from untrusted peers (crates/edgezero-adapter-axum/src/service.rs:473) - 🔧 Geo CI steps run in all four host-test legs (
.github/workflows/test.yml:189)
Non-blocking
-
♻️
category_only_json_errorsis redundant for every in-tree caller (crates/edgezero-core/src/context.rs:677) -
♻️ The pre-dispatch Geo copy is never read (
crates/edgezero-core/src/context.rs:783) -
♻️
geo_hosting_invalid_policy_never_initializes_or_listensnever reaches the runner (crates/edgezero-adapter-axum/tests/production_hosting.rs:2350) -
🤔 Does
geo-cloudfrontpay for itself? (crates/edgezero-adapter-axum/Cargo.toml:40) -
⛏ Duplicate
dep:getrandom/dep:ipnetin theaxumfeature (crates/edgezero-adapter-axum/Cargo.toml:22) -
⛏ The PR description is stale relative to the head:
- It cites tested implementation
56eab85f, which is not on this branch; the head is414e52ae. - It names the base as #422 at
5426ea65; the base is nowf9b0f20c. - The Lockfiles row claims dependency edges in the root/demo/reusable locks; the diff changes no lockfile.
- The reconciliation section says this diff brings in the #409 checked-ingress contract, but that is already in the base. Here the diff only extracts
peer_networks.rs. - One section says "Ready for review", another "Draft pending fresh hosted checks".
Trimming it to the current head's change set and evidence would make the review surface clear.
- It cites tested implementation
-
🌱 Public Geo types will need breaking changes for #438 and VPC origins (
crates/edgezero-core/src/geo.rs:280) -
🌱 Hidden
dispatch_admitted_with_*chain (crates/edgezero-core/src/app.rs:321) -
🌱 Guide snippets are hand copies of the doctests: the Rust blocks in
docs/guide/native-geo.md(lines 30-174) duplicate the four doctests incrates/edgezero-adapter-axum/src/geo.rs:7-88, and nothing checks that they stay identical. Moving the examples into compiled files and pulling them into the guide with VitePress<<<snippet imports would let the existing doctest CI step cover the guide. -
🌱 Kosovo (
XK) acceptance check (docs/guide/native-geo.md:218)
📌 Out of Scope
- Pre-existing macOS-only failures, untouched by this PR (Linux CI is green): the four
production_hostingproxy/TLS tests need127.0.0.2/127.0.0.3on loopback, andnative_aws_startup::production_runner_composes_celld_config_aws_secret_and_local_secretfails withWouldBlock(os error 35) because its fixture's accepted stream inherits non-blocking mode.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (workspace; feature check and Spin wasm32 check also pass)
- Geo steps run locally: dependency script, core/Axum Geo unit tests,
service::tests, doctests, and Geo hosting tests with the built-in disabled and enabled (serial and parallel): PASS - GitHub checks at
414e52ae: all green
| .await; | ||
| } | ||
| }; | ||
| let geo = self.geo.resolve(remote_addr, &parts.headers); |
There was a problem hiding this comment.
❓ Raw CloudFront-Viewer-* headers stay readable, including from untrusted peers
When a Geo selection is active, both headers are read into GeoInput here, then left on parts.headers. A request from a peer outside the protected ranges gets UntrustedIngress from ctx.geo(), while ctx.headers()["cloudfront-viewer-country"] still carries whatever the client sent. Two lines above, strip_forwarding_headers removes raw forwarding headers unconditionally so application code cannot misread them. Geo has no equivalent, and docs/guide/native-geo.md doesn't tell applications not to read these headers.
Was keeping them intentional? If not, remove both names once they have been consumed. Off stays unchanged:
let geo = self.geo.resolve(remote_addr, &parts.headers);
self.geo.strip_assertions(&mut parts.headers);// geo.rs
pub(crate) fn strip_assertions(&self, headers: &mut HeaderMap) {
if matches!(self, Self::Selected { .. }) {
let _country = headers.remove("cloudfront-viewer-country");
let _region = headers.remove("cloudfront-viewer-country-region");
}
}The fixture's mutated_country == "GB" assertion in production_hosting.rs would change accordingly. If keeping them is deliberate, please add a warning to the guide's "Read the immutable request outcome" section that the raw headers are unauthenticated and ctx.geo() is the only trusted read.
| if: matrix.suite == 'workspace' | ||
| run: bash scripts/check_native_store_dependency_contract.sh | ||
|
|
||
| - name: Geo dependency isolation |
There was a problem hiding this comment.
🔧 Geo steps run in every host-test matrix leg
Every neighbouring step here is guarded with if: matrix.suite == 'workspace', but the nine new Geo steps (lines 189-219) have no guard. Run 42924133045 shows all nine executed in the workspace, cli, axum and app-demo legs, so the dependency script and four distinct Axum feature builds run four times per push.
Add the guard to each step:
- name: Geo dependency isolation
if: matrix.suite == 'workspace'
run: bash scripts/check_geo_dependency_contract.sh(or matrix.suite == 'axum' if they are meant to live with the Axum leg).
| let bytes = self.buffered_bytes(max, true).await?; | ||
| serde_json::from_slice(bytes.as_ref()).map_err(|err| { | ||
| if self.body_policy.is_some() { | ||
| if self.category_only_json_errors || self.body_policy.is_some() { |
There was a problem hiding this comment.
♻️ category_only_json_errors duplicates the existing body_policy privacy
The only caller that passes FrameworkErrorDetail::CategoryOnly is managed Axum dispatch, and Axum attaches with_buffered_json_policy to every managed body (crates/edgezero-adapter-axum/src/request.rs:84). So self.body_policy.is_some() already makes this path category-only, and the base already asserts "invalid JSON payload" in production_hosting.rs (native_default_errors_json_cap_and_logs_keep_sentinels_private).
I checked this locally by forcing the flag off: all 5 Axum hosting JSON tests and all 436 Axum lib tests still pass. The only failure is app::tests::geo_owned_outcome_survives_public_request_mutation, which builds a body without a JSON policy.
Suggest dropping the flag and the bool half of the (GeoOutcome, bool) plumbing (AdmittedIngress::{request_context, take_request_context, with_request_context} and app.rs:337), and removing the JSON assertions from the Geo test. That also keeps a non-Geo change to core JSON error text out of this PR.
| } | ||
| } | ||
| pub(crate) fn new_pre_dispatch(request: Request, ingress: Option<&AdmittedIngress>) -> Self { | ||
| let (geo, category_only_json_errors) = ingress.map_or( |
There was a problem hiding this comment.
♻️ The pre-dispatch Geo copy is never read
new_pre_dispatch clones the outcome through AdmittedIngress::request_context(), which costs a String allocation for the provider ID on every selected request. PreDispatchRequest has no geo() accessor, and every path that builds the final RequestContext passes Some(ingress) to finish_pre_dispatch, which overwrites the copy from take_request_context().
Locally, defaulting this to (GeoOutcome::NoSelection, false) passes core (1144), Axum lib (436) and the Geo hosting tests, and leaves request_context unused. Either drop it and delete AdmittedIngress::request_context, or, if pre-dispatch hooks should see Geo (design section 3, step 4 says the outcome is stored before request preparers run), add PreDispatchRequest::geo() with a test.
| } | ||
|
|
||
| #[test] | ||
| fn geo_hosting_invalid_policy_never_initializes_or_listens() { |
There was a problem hiding this comment.
♻️ This test never reaches the runner
In geo-invalid-policy mode the fixture passes an empty network list, so ProtectedCloudFrontAlb::new(...)? (tests/fixtures/production_host.rs:1002) fails in main before run_app_with_options_and_initializer is called. The assertions (no INITIALIZED, no bind, no roots) hold by construction. Empty-list rejection is already covered by geo_protected_policy_reuses_network_bounds, and with geo-cloudfront enabled no hosting test exercises runner-side Geo rejection.
Build a valid policy plus a selection that only NativeGeo::validate rejects. This works in both feature states:
let selection = if selected == "geo-reserved-identity" {
NativeGeoSelection::Supplied {
policy,
id: GeoProviderId::cloudfront(),
provider: Arc::new(FixtureGeoProvider(false)),
}
} else if selected == "geo-off" {and rename the mode and test to match.
| aws-dynamodb = ["axum", "edgezero-store-aws/dynamodb-kv"] | ||
| celld-config = ["axum", "edgezero-store-celld/config"] | ||
| celld-kv = ["axum", "edgezero-store-celld/kv"] | ||
| geo-cloudfront = ["axum"] |
There was a problem hiding this comment.
🤔 Does the geo-cloudfront feature pay for itself?
It gates geo/cloudfront.rs, about 45 non-test lines with no dependencies; check_geo_dependency_contract.sh proves the package set is identical with it on or off. Geo is already off at runtime until with_geo selects it, so the feature is a second off switch. It adds NativeGeoConfigError::UnavailableCloudFront, the cfg-split tests in dev_server.rs and production_hosting.rs, the cfg_attr on the module, and four of the nine new CI steps.
Design section 7 asks for the built-in to be default-off, and NativeGeoSelection::Off already provides that. Consider compiling the built-in under axum and dropping the feature. Non-blocking, since this is a spec-level choice.
| "dep:tokio", | ||
| "dep:tower", | ||
| "dep:futures-util", | ||
| "dep:getrandom", |
There was a problem hiding this comment.
⛏ Duplicate feature entries
axum already lists dep:ipnet and dep:getrandom at lines 27-28 (from the base), so these two additions do nothing. Cargo deduplicates them silently, but the diff reads as if Geo added dependencies. Remove lines 22-23.
| /// | ||
| /// No request, reader IP, service handle or arbitrary headers are available. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct GeoInput { |
There was a problem hiding this comment.
🌱 Planned follow-ups will break these public types
GeoInput has public fields and no constructor, and GeoSource/GeoTrustBasis have a single variant each. None are #[non_exhaustive]. #438 (reader-IP input, GeoIP source) and VPC origins (a new trust basis) are already planned, and each will break every provider test that builds GeoInput { .. } and every exhaustive match on these enums. That is allowed at 0.1.0, but worth deciding on purpose. For example: #[non_exhaustive] plus GeoInput::new(country, subdivision) for the input, while keeping GeoStatus/GeoOutcome exhaustive so consumers are forced to handle new statuses.
| /// Dispatches with an adapter-owned outcome, independent of public request extensions. | ||
| #[doc(hidden)] | ||
| #[inline] | ||
| pub async fn dispatch_admitted_with_geo( |
There was a problem hiding this comment.
🌱 Hidden dispatch entry points grow by one per adapter-provided fact
dispatch_admitted -> dispatch_admitted_with_framework_error_detail -> dispatch_admitted_with_geo, each a #[doc(hidden)] pub method. The next adapter-supplied request fact (for example #438's checked reader IP) would add a fourth. A single hidden parameter struct with Default, such as AdmittedDispatch { detail, geo }, would stop the chain.
| | `US` and `01` | `Located` with syntax-checked numeric subdivision | | ||
| | `US` and `ZZ` | `Located` with a syntax-valid assertion, not verified subdivision membership | | ||
| | Subdivision without country | `InvalidAssertion` | | ||
| | Unsupported/reserved country, including `XX`, `ZZ`, `UK`, or `XK` | `InvalidAssertion`, no aliases | |
There was a problem hiding this comment.
🌱 Check what CloudFront sends for Kosovo
AWS documents CloudFront-Viewer-Country only as "ISO 3166-1 alpha-2" and lists no user-assigned codes, so rejecting XK matches the documented contract. MaxMind and CLDR do use XK for Kosovo, though. If CloudFront sends it, every Kosovo viewer becomes InvalidAssertion, indistinguishable from forged input. Suggest adding "observe the country value for a Kosovo client" to this guide's deployment acceptance list.
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.