Add a pluggable Edge Cookie provider seam with the built-in HMAC provider - #1043
Add a pluggable Edge Cookie provider seam with the built-in HMAC provider#1043jwrosewell wants to merge 38 commits into
Conversation
312a4fc to
73b40b9
Compare
83e551d to
e278981
Compare
e278981 to
4529151
Compare
The five-PR series (IABTechLab#1043 to IABTechLab#1047) opens the identity, device and geo seams. The nine vendor integrations already in core sit behind the integration registry instead, which is a private table, so none of them can move out until that table is opened. This spec defines the one core change that opens it: public registration builders with a second input on IntegrationRegistry, browser JavaScript carried on the registration, startup validation as a hook, the same treatment for auction providers and the bid renderer contract, and neutral replacements for the two places where a vendor reaches into core. It then sets out the migration of all nine existing integrations, one PR each. The change is complete in itself: after it, no vendor move needs a core change. Written against the series' tree with the file and line references for every claim about the current code. Documentation only.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR lands the Edge Cookie provider seam with the built-in HMAC provider, per the pluggable-providers design spec carried in the same change. The lifecycle contract (mint, recognition, KV keying), the global identifier bounds, startup validation, the deprecated-passphrase migration, and the partner-path envelope fix are substantially implemented, with strong test coverage, and CI is fully green.
The major blocker is architectural: vendor extensibility should lean on the existing integration system rather than introduce a parallel "provider" mechanism. The codebase has one established home for vendor code (the integration registry), and this PR adds a second seam, a second config namespace, and a second nomenclature for what a vendor ships. We want that resolved at spec level before PRs 2-5 of the series build on the current shape - see the first cross-cutting finding below.
Beyond that, changes are requested on: a reproduced bypass of the advertised 32-byte passphrase minimum on the deprecated configuration form, two points where the implementation does not do what the spec states (unknown-key rejection in the hmac block; canonical-key routing on identity-graph reads and withdrawals), and an egress guarantee the proxy paths do not honor.
4 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 the fix in prose because the change spans multiple files or non-contiguous lines and cannot be auto-applied.
Blocking
🔧 wrench
- Vendor identity should lean on the integration system, not a second extension mechanism - the major blocker; cross-cutting, below
- Legacy
[ec] passphrasebypasses the new 32-byte minimum - see inline atcrates/trusted-server-core/src/settings.rs:658(suggestion) [ec.providers.hmac]silently accepts unknown keys - see inline atcrates/trusted-server-core/src/settings.rs:726(suggestion)- Identity-graph reads and withdrawal tombstones bypass the provider's canonical key - cross-cutting, below
❓ question
- Spec says an unrecognized cookie value is "never used or egressed", but the proxy forwarding paths egress it - cross-cutting, below
Non-blocking
♻️ refactor / 🤔 thinking / ⛏ nitpick / 📌 out of scope
- ♻️
build_providersilently returnsOk(None)forprovider = "hmac"with no block - see inline atcrates/trusted-server-core/src/ec/provider.rs:304(suggestion) - ⛏ 22-space run inside the mint-rejection error message - see inline at
crates/trusted-server-core/src/ec/mod.rs:444(suggestion) - ♻️
EdgeCookieProvider's doc comment is fused intoProviderCode's, leaving the trait undocumented - see inline atcrates/trusted-server-core/src/ec/provider.rs:177 - ⛏
ec::get_ec_idis dead code, yet was modified to accept any provider code - see inline atcrates/trusted-server-core/src/ec/mod.rs:137 - ⛏ Module docs describe constructor injection that is not how
RequestInfoflows - see inline atcrates/trusted-server-core/src/ec/provider.rs:4 - 🤔 Cluster prefix listing splits across the envelope migration - cross-cutting, below
- ♻️ Magic strings
"hmac"/"none"scattered across four call sites - cross-cutting, below - 🤔
RequestInfoaccessors have no production consumer in this PR - cross-cutting, below - 🤔 Spec revision followed the implementation - cross-cutting, below
- 📌 Operator guides still document
[ec] passphraseas the current form - cross-cutting, below
Cross-cutting / body-level findings
-
🔧 Vendor identity should lean on the integration system, not a second extension mechanism (the major blocker). The codebase already has one home for vendor code: the integration registry (
IntegrationRegistration::builder(ID).with_proxy().with_head_injector()...), capability-based and config-namespaced under[integrations.<id>]. This PR adds a second vendor seam -RuntimeServices::ec_provider, a single-slotOption<Arc<dyn EdgeCookieProvider>>matched byid(), configured under[ec.providers.<key>]- and a second nomenclature ("providers").RuntimeServicesis otherwise the platform composition surface (KV store, geo, HTTP client, client info: things the host supplies); a vendor identity module is not a host capability, and a vendor realistically ships a JS integration and an identity function together, which this split forces into two mechanisms. Please rework the vendor seam onto the integration system: identity provision as a registration capability (for example.with_ec_provider(...)), with[ec] provider = "<integration id>"still supplying the select-exactly-one semantics; the built-in HMAC provider can stay hard-wired in core as the default, and geo/device rightly remain platform services. If there is a reason this cannot work, the spec should defend the separate provider mechanism against this alternative explicitly - and we want that settled at spec level before PRs 2-5 of the series build on the current shape. -
🔧 Identity-graph reads and withdrawal tombstones bypass the provider's canonical key. The spec's lifecycle table (section 3) routes identity-graph row reads and writes through
normalize_id_for_kv. Mint honors that:EcContext::generate_with_providerkeys the row withprovider_kv_key(ec/mod.rs:476). Buthandle_identifyreads with the raw cookie value (kv.get(ec_id),ec/identify.rs:89), withdrawal tombstones are written under the raw value (ec/finalize.rs, thewrite_withdrawal_tombstoneloop), and EID ingestion keys by the raw value. For the built-in HMAC provider raw and canonical coincide, so nothing misbehaves today; for the first provider whose canonical form differs from the cookie value (exactly theCanonicalizingProvidercase this PR's own test proves at mint), identify misses the row written at mint, and a withdrawal tombstone lands on a key no live row uses, so the revocation never takes effect. Proposed fix: compute the canonical key once inEcContext(for example anec_kv_key()accessor derived from the selected provider) and use it in identify, the finalize tombstones, and EID ingestion - or amend the spec to state that reads and withdrawals become canonical-form-routed only when the first canonicalizing provider ships, and track that as a follow-up. -
❓ Spec says an unrecognized cookie value is "never used or egressed", but the proxy forwarding paths egress it. Section 3's Recognize row states that a value the selected provider does not recognize "is never used or egressed."
append_ec_id(proxy.rs:1263) andhandle_first_party_click(proxy.rs:1609) forward the rawts-eccookie /x-ts-echeader value to origin and click-target URLs throughedge_cookie::get_ec_id, which checks only the character/length allowlist - so a foreign-coded value (zz00~...), or any cookie in a stateless (no-provider) deployment, is egressed on those paths. The looseness predates this PR, but the PR introduces the spec claim. Which should change - the spec (scope the guarantee to the EC lifecycle paths and note the proxy forwarding exception) or the code (route those call sites through provider ownership)? -
🤔 Cluster prefix listing splits across the envelope migration. Section 3 says the pre-epic IP-cluster prefix listing "continues unchanged." Fresh mints are now keyed
hmac~<hash>.<suffix>, soevaluate_cluster's prefix (ec_hash,ec/kv.rs:715) becomeshmac~<hash>for coded rows while legacy rows still list under the bare<hash>. Two rows for the same client IP that straddle the envelope migration therefore never count each other, andcluster_size(a NAT/fraud signal in identify responses) undercounts while both populations coexist. Worth a sentence in the spec, and possibly a follow-up to bridge the count during the migration window. -
♻️ Magic strings
"hmac"/"none"are scattered across four call sites (Ec::validate_provider_selection,build_provider,provider_owns_id'sprovider.id() == "hmac", andHMAC_PROVIDER_CODEinec/generation.rs). A typed selector, for exampleenum EcProviderSelection { None, Hmac, Vendor(String) }with a custom deserializer (vendor keys are open-ended, so a catch-all variant is needed), would centralize the vocabulary before #1044 adds more built-ins. Non-blocking: the string form works and is startup-validated. -
🤔
RequestInfoaccessors have no production consumer in this PR.path(),query(),query_param(),header_names(),user_agent(), andheader()are supplied by production code but consumed only by tests in this PR (the HMAC provider reads onlyclient_ip()). The spec's own minimalism rule (section 4) requires a production caller in the same change that introduces a method; the consumers arrive later in the stack. For a stacked series this can be acceptable, but the spec should say which PR consumes each accessor, or the accessors should land with their consumers. -
🤔 Spec revision followed the implementation. The spec is commendably candid that it is the 2026-07-31 draft "revised against the implementation" with a revision-record table, and that table is genuinely useful. The process consequence is worth naming, though: when the normative spec is restated to match landed code, divergences become ratifications rather than decisions, and questions like the extension-model one above surface at review time instead of design time. For the remaining PRs in the series, it would serve the spec-first intent better to land spec changes ahead of the implementing PR and let review happen against the spec before the code exists.
-
📌 Operator guides still document
[ec] passphraseas the current form.docs/guide/configuration.md:1933,docs/guide/key-rotation.md:31,docs/guide/error-reference.md:72, plusec-setup-guide.md/edge-cookies.md/fastly.mdpredate the provider layout, the deprecation, and the new stateless default intrusted-server.example.toml. A docs pass is needed in this series; a follow-up PR is fine.
CI Status
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- format-docs: PASS (required)
- format-typescript: PASS (required)
- cargo test (axum native): PASS
- cargo test (cloudflare native + wasm32-unknown-unknown check/build): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- vitest: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CodeQL: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
| log::warn!( | ||
| "[ec] passphrase is deprecated; move it to [ec.providers.hmac] passphrase and \ | ||
| set [ec] provider = \"hmac\"" | ||
| ); | ||
| self.provider = Some("hmac".to_owned()); | ||
| self.providers.hmac = Some(HmacProviderConfig { passphrase }); | ||
| Ok(()) |
There was a problem hiding this comment.
🔧 wrench - The advertised 32-byte passphrase minimum is bypassed on the deprecated form. finalize_deserialized runs derive validation before migrate_legacy_ec_layout(), and this deprecated field no longer carries a #[validate] attribute, so [ec] passphrase = "short" (or an empty value) migrates and starts successfully. Reproduced with a scratch test: Settings::from_toml returns Ok for a legacy 5-byte passphrase. That contradicts the PR description ("enforced wherever the passphrase is configured") and the commit message. Validating inside the migration keeps the enforcement self-contained for every construction path:
| log::warn!( | |
| "[ec] passphrase is deprecated; move it to [ec.providers.hmac] passphrase and \ | |
| set [ec] provider = \"hmac\"" | |
| ); | |
| self.provider = Some("hmac".to_owned()); | |
| self.providers.hmac = Some(HmacProviderConfig { passphrase }); | |
| Ok(()) | |
| Self::validate_passphrase(&passphrase).map_err(|err| { | |
| Report::new(TrustedServerError::Configuration { | |
| message: format!( | |
| "[ec] passphrase (deprecated) is invalid ({err}): use a random secret \ | |
| of at least {} bytes, placed in [ec.providers.hmac]", | |
| Self::MIN_PASSPHRASE_LENGTH, | |
| ), | |
| }) | |
| })?; | |
| log::warn!( | |
| "[ec] passphrase is deprecated; move it to [ec.providers.hmac] passphrase and \ | |
| set [ec] provider = \"hmac\"" | |
| ); | |
| self.provider = Some("hmac".to_owned()); | |
| self.providers.hmac = Some(HmacProviderConfig { passphrase }); | |
| Ok(()) |
Verified: with this applied, the legacy short passphrase fails startup, and the full local gate passes.
| #[derive(Debug, Default, Clone, Deserialize, Serialize, Validate)] | ||
| pub struct HmacProviderConfig { |
There was a problem hiding this comment.
🔧 wrench - The spec (section 6) states deny_unknown_fields is on "both built-in provider config structs", but this struct has no such attribute, so [ec.providers.hmac] passphrase = "..." typo_key = "x" is silently accepted. (A typo'd block name is caught by the stray-block rule; a typo'd key inside the block is not.)
| #[derive(Debug, Default, Clone, Deserialize, Serialize, Validate)] | |
| pub struct HmacProviderConfig { | |
| #[derive(Debug, Default, Clone, Deserialize, Serialize, Validate)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct HmacProviderConfig { |
Verified: with this applied, an unknown key in the hmac block fails startup, and the full local gate passes.
| "hmac" => ec | ||
| .providers | ||
| .hmac | ||
| .as_ref() | ||
| .map(|config| Box::new(HmacProvider::new(config.passphrase.clone())) as _), |
There was a problem hiding this comment.
♻️ refactor - When provider = "hmac" is selected but the block is absent, this arm returns Ok(None) and the deployment silently runs stateless. Settings validation rejects that configuration at startup, but if this seam is ever reached with such a config (programmatic Settings, a future construction path), the result is the exact "silent identity outage" the spec's failure-mode table exists to prevent. The vendor arm fails loudly; this arm should too:
| "hmac" => ec | |
| .providers | |
| .hmac | |
| .as_ref() | |
| .map(|config| Box::new(HmacProvider::new(config.passphrase.clone())) as _), | |
| "hmac" => match ec.providers.hmac.as_ref() { | |
| Some(config) => Some(Box::new(HmacProvider::new(config.passphrase.clone())) as _), | |
| // Settings validation rejects a selected provider with no block; | |
| // if that is bypassed, fail loudly rather than silently running | |
| // stateless. | |
| None => { | |
| return Err(Report::new(TrustedServerError::EdgeCookie { | |
| message: "Edge Cookie provider `hmac` is selected but [ec.providers.hmac] \ | |
| is not configured" | |
| .to_owned(), | |
| })); | |
| } | |
| }, |
| if !ec_id_has_only_allowed_chars(&ec_id) { | ||
| return Err(Report::new(TrustedServerError::EdgeCookie { | ||
| message: format!( | ||
| "Provider `{}` produced an identifier that is empty, over {} bytes, or outside the cookie-safe alphabet", |
There was a problem hiding this comment.
⛏ nitpick - This string carries a 22-space run (a missing \ line continuation), so the logged error reads "...bytes, or outside the cookie-safe alphabet".
| "Provider `{}` produced an identifier that is empty, over {} bytes, or outside the cookie-safe alphabet", | |
| "Provider `{}` produced an identifier that is empty, over {} bytes, or \ | |
| outside the cookie-safe alphabet", |
| } | ||
| } | ||
|
|
||
| pub trait EdgeCookieProvider: Send + Sync + core::fmt::Debug { |
There was a problem hiding this comment.
♻️ refactor - This trait has no doc comment: the paragraph written for it ("A strategy for deriving an Edge Cookie identifier...") is fused into the doc block of ProviderCode above (lines 58-77), where it reads as part of that struct's documentation. The stray text is also stale: it says a provider returns Ok(None) from generate, but generate returns GeneratedEdgeCookie { id: None }. Apply manually (two non-contiguous edit sites, so this cannot be a single suggestion): move the strategy paragraph here, reword the Ok(None) sentence to the id: None semantics, and leave ProviderCode with only its own registry-code doc.
| // Accept the coded form (any provider's `{code}~value` within the global | ||
| // identifier bounds) and the legacy bare HMAC form. Provider-aware | ||
| // ownership lives in `EcContext`; this helper only reads the string. | ||
| let ec_id = parsed |
There was a problem hiding this comment.
⛏ nitpick - This pub fn get_ec_id has no callers anywhere in the workspace (proxy.rs and testlight.rs use edge_cookie::get_ec_id), yet this PR loosened its filter to accept any {code}~ value without an ownership check against the selected provider. A future caller picking it up would adopt foreign-coded identifiers that EcContext deliberately treats as absent. Either delete the function or align its filter with provider_owns_id.
| //! Edge Cookie identity providers. | ||
| //! | ||
| //! An [`EdgeCookieProvider`] derives an Edge Cookie identifier. Providers are | ||
| //! wired by dependency injection: a provider's constructor takes the services it |
There was a problem hiding this comment.
⛏ nitpick - The module doc says a provider's constructor takes the services it needs, with RequestInfo as the example, but RequestInfo is passed at generate call time, not at construction; the opening sentence is also garbled ("...for the client IP) (the adapter, through [build_provider]) supplies instances per request."). Same constructor-injection claim in evidence.rs lines 3-7. Worth a small rewrite so the first thing a vendor implementer reads matches the trait signature.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Reviewed the pluggable Edge Cookie provider changes at 0f5c063214ba1d46478311851f08fe9b10c2ccf8. I am requesting changes based on the inline findings. This review includes one P1, three P2s, and one non-blocking migration clarification. cargo test-fastly and all 18 GitHub checks passed at the reviewed head; these findings concern runtime and provider-contract behavior rather than test failures.
| .filter(|provider| provider.id() == other) | ||
| .map(|provider| Box::new(SharedProvider(provider)) as _); | ||
| if provider.is_none() { | ||
| return Err(Report::new(TrustedServerError::EdgeCookie { |
There was a problem hiding this comment.
P1: Portability adapters swallow unavailable-provider errors
build_provider correctly returns an error here, but Axum, Cloudflare, and Spin catch it from read_from_request_with_geo and replace the EC context with EcContext::default(), so the request continues without identity. That contradicts the provider contract, which says an unavailable required service or injected provider stops the request. Fastly already propagates the error. Please return an error response in those adapters or reject the selection while building adapter state, and add a regression test for an uninjected provider on each adapter.
| // generation (for example to request more client evidence). This is empty | ||
| // unless a provider produced headers, so it is safe on every path. | ||
| for (name, value) in ec_context.response_headers() { | ||
| response.headers_mut().insert(name, value.clone()); |
There was a problem hiding this comment.
P2: Provider response effects can overwrite core-managed state
These headers are inserted without checking names or cookie ownership. A provider can return Set-Cookie: ts-ec=..., including when it returns no identifier, and bypass core's identifier validation, graph-write requirement, and managed EC cookie code. It can also overwrite reserved x-ts-* or response-framing headers. Providers may legitimately need their own evidence cookies, so banning every Set-Cookie would be too broad. Please validate these effects or expose a typed response API that reserves the managed ts-* cookie names, the x-ts-* namespace, and framing or hop-by-hop headers while allowing provider-owned cookies.
| let mut parts = value.split('.'); | ||
| let bare = match split_provider_code(value) { | ||
| (Some(code), bare) if code == HMAC_PROVIDER_CODE => bare, | ||
| (Some(_), _) => return false, |
There was a problem hiding this comment.
P2: Partner paths reject identifiers from the next provider
is_valid_ec_id explicitly rejects every provider code except hmac, and pull sync, batch sync, and the admin lookup all call it. This is already concrete in the stacked work: PR #1044 adds the hs00~ host-signal provider without changing these consumers, so its valid identifiers work in organic read and write paths but are skipped or rejected by all three partner and diagnostic paths. Please separate global cookie bounds from provider-specific validation, dispatch validation and KV normalization by provider code, and cover a non-HMAC identifier in pull sync, batch sync, and admin lookup tests.
| // guards so a stateless deployment on a host with no client IP does not | ||
| // log spurious errors. The provider reads it borrowed at generate time | ||
| // (see [`generate_with_provider`]), so nothing is cloned here. | ||
| if self.client_ip.is_none() { |
There was a problem hiding this comment.
P2: Generic generation requires client IP before calling the provider
This check rejects the request before the selected provider can decide whether it needs client IP. RequestInfo::client_ip already defines an empty string as the unavailable state, and providers are meant to read only the request evidence they need. The generic check therefore blocks header-, cookie-, query-, and client-derived providers that can operate without IP. Please move the requirement into HmacProvider and any other provider that uses IP, then pass the documented empty value to providers that do not.
| /// carries, with the value part accepted by that provider's | ||
| /// [`accepts_id`](EdgeCookieProvider::accepts_id). A legacy bare identifier | ||
| /// (no code prefix) belongs only to the built-in HMAC provider, which | ||
| /// dual-reads its pre-envelope form for one release cycle so deployed cookies |
There was a problem hiding this comment.
Non-blocking: Define when the bare HMAC reader can be removed
This comment promises one release cycle of bare-HMAC compatibility, but returning users do not have their bare cookie rewritten and the cookie lifetime is one year. The current code is safe while this reader remains. Before scheduling its removal, please define a retirement condition based on the maximum cookie and graph-row lifetime plus rollout skew and observed legacy-reader traffic. Otherwise, remove the one-release wording and keep the reader.
0f5c063 to
11cc575
Compare
The five-PR series (IABTechLab#1043 to IABTechLab#1047) opens the identity, device and geo seams. The nine vendor integrations already in core sit behind the integration registry instead, which is a private table, so none of them can move out until that table is opened. This spec defines the one core change that opens it: public registration builders with a second input on IntegrationRegistry, browser JavaScript carried on the registration, startup validation as a hook, the same treatment for auction providers and the bid renderer contract, and neutral replacements for the two places where a vendor reaches into core. It then sets out the migration of all nine existing integrations, one PR each. The change is complete in itself: after it, no vendor move needs a core change. Written against the series' tree with the file and line references for every claim about the current code. Documentation only.
The review of IABTechLab#1043 asked that spec changes land before the code that implements them, so a divergence is a decision taken in review rather than a ratification of something already merged. PRs IABTechLab#1043 to IABTechLab#1047 each carried the design document for their own step, and IABTechLab#1043 carried a 607-line spec describing device providers, geo providers, the permission model and the browser resolve endpoint, none of which is in that PR. Move all six series documents here, so this PR carries the complete normative set and no code: - 2026-07-30-pluggable-providers-design.md (from IABTechLab#1043) - provider-code-registry.md (from IABTechLab#1043) - 2026-07-30-permission-model-design.md (from IABTechLab#1045) - 2026-07-30-client-cycle-ec-resolve-design.md (from IABTechLab#1046, later revised by IABTechLab#1047) - 2026-07-30-integration-response-header-hook-design.md (from IABTechLab#1047) - 2026-07-30-provider-migration-rollout-design.md (from IABTechLab#1047) Each file is taken verbatim at the tip of the stack, so the later revisions are preserved: the provider-switching continuity section, the geo requires-signal floor, and the code-envelope paragraph IABTechLab#1047 added to the client-cycle spec. The revision-record tables are unchanged. No document's substance was edited. The only edits are to this spec's own status line, which said the PR adds one document and that the series specs land with IABTechLab#1047, and a revision-record row recording the move.
|
This response was drafted with AI assistance and checked against the branches before posting. Thank you both. Twenty observations across the two reviews. Seventeen are answered in code on this branch and three are answered in the pull request of the chain where the answer belongs, named in the Addressed elsewhere table. Each fix is separately committed, so any one can be confirmed without reading a combined diff. Two further rows in the Addressed table are not yours, being things we found while answering and fixed in the same pass. A note on scope. Some of your observations reach past this PR into the ones before and after it, which is unavoidable because the work was split into a chain. Answering only within #1043 would be more confusing, not less, so this comment answers for the whole chain and says where each answer lives. #1043 is simply the PR the review happened on. Each commit's message ends with an The branch is rebased onto We also run the core library suite natively, at 2,463 tests, and #1047 adds that run to One CI note, and a small ask. CodeQL flags "Cleartext logging of sensitive information" on #1044 to #1047 and #1094. It is a false positive and we would ask you to dismiss it, since the alerts belong to this repository and we cannot. The passphrase it traces is held in a Where each piece is, and what changed between the PRsTwo things moved since Aram's review on 27 August that are not visible from this PR alone. The seam that the architectural finding asks this work to lean on now These are one block, and the order below is the order they should merge in. Splitting them is what creates the legacy this work exists to stop, because each one on its own leaves the core carrying a shape the next one removes. The last item is the point of the whole exercise, an unmerged vendor change landing without adding to the core, so no further legacy is added rather than removed later.
Rowena asked on 27 August whether #1044 must follow #1043, or whether #1045 could follow #1043 instead. The answer is that #1045 cannot move ahead of #1044, and here is the reason rather than the assertion. The permission model needs a jurisdiction baseline, which is the country whose rules apply when the geo lookup returns nothing. That baseline lives on the The rest of the order is the same kind of dependency rather than preference. #1046 is the browser-set path for an identity #1043 defines, and #1047 documents behavior the four before it introduce, so documenting it earlier would describe code that is not there. If a different order would help you, tell us what you need and we will say honestly whether it can be done, because we would rather rework the split than have the whole thing wait on the shape we happened to choose. Items 2 to 7 are one ordered chain, not six branches beside each other. #1043 is against The order we suggest is #1084 first, since it settles the design question and costs nothing, then #1043 to #1047 in sequence, then the implementation of #1084. That implementation is where identity, geo and device all become capabilities a registration declares, which is the architectural finding answered rather than deferred. It lands there and not here because a registration can only carry an Edge Cookie provider once that trait exists, and #1043 is what adds it, so the seam PR is the first point in the chain where both exist together. We would rather do it once, against a seam that exists, than rewrite five reviewed PRs onto a seam that did not exist when the review was written. Addressed
Addressed elsewhere in the chainEach is answered in the PR of the chain where the answer belongs rather than in this one.
Behavior changesFour, each called out deliberately rather than left to be found. Every one of them is necessary rather than incidental, and every one moves in the direction this project has already chosen, which is a core that is neutral between vendors and does nothing on a deployment's behalf that the deployment did not ask for. The last is the one an operator will feel most, so it is worth reading even if the rest are skimmed.
How providers see the requestApplying the minimalism rule to the evidence interface was the wrong call, and we are reversing it. Here is the design we are implementing instead, so the reasoning is on the record rather than arriving as a surprise in a later PR. A provider is given everything the request carries. The client IP, the User Restricting what a provider can see is the wrong lever. The right one is
That guards against a badly behaved provider twice over, without the interface deciding in advance what a vendor is allowed to look at. A permission describes what, not how, and that is the whole reason this boundary is the right one. A permission names a data use, being storage on the device, or personalized marketing. It never names a technology. There is no permission saying the User-Agent header may be read, or that a cookie may be used but local storage may not. Data protection works the same way round, because it governs the purpose data is put to rather than the mechanism used to achieve it. So restricting what a provider sees regulates the how, not the what. A provider blocked from one header can often reach the same purpose another way, and one allowed to see a header still may not use it for a purpose nobody granted. What stops the purpose is not running the provider at all, which is the first layer above. Drawing the boundary on the purpose rather than the mechanism also buys something we would like to build on. Every provider already declares the permissions its data use requires, so a build can be asked what it will do before it serves a single request. The core can emit a manifest for a given deployment listing every permission every module in it requires, derived from the modules themselves rather than from someone's notes. That is a machine-readable statement of what a deployment does with data, which is most of the work of writing a privacy notice, and it can be generated and kept current rather than maintained by hand and quietly going stale 🙂 And the claim can be checked, which is what makes it useful. A provider declaring the permissions its data use requires is, on its own, only a claim. What turns a claim into something a publisher can rely on is that the code is open, so anyone can read what a module actually does and hold it against what the module said it would do. That is a large part of what the word trusted in Trusted Server has to mean, because a trust nobody can verify is only a reputation. Checking used to be expensive enough that almost nobody did it. That has changed. An AI agent can read a module, read its declared permissions and report the difference in minutes, for very little, and can do it again on every release rather than once at onboarding. So a false declaration, or a module quietly doing more than it declared, moves from something findable in principle to something that will be found in practice. The consequence should follow the finding, and it should be plain. A vendor whose modules repeatedly do not do what they say should not have modules in this project, and should not remain a member of the organization that publishes it. Simple. That is the enforcement this model needs behind it, and it is available only because the code is open and the declarations are machine-readable. The caller is us, and it is the next step rather than part of this stack. We will use all of it, to the extent permissions allow, across the geo, device and Edge Cookie providers. We are deliberately not raising that pull request alongside these, because this stack is already a large change and a vendor module on top would make it harder to review. What that work needs is specific rather than speculative. The evidence interface #1043 carries already exposes the client IP, the User-Agent, headers read by name, header enumeration so a module sends a complete evidence set rather than working from an allowlist compiled into it, the path, and the query and its parameters, and it is whole on #1043 as of So the evidence interface is whole on #1043 as of We will prove the evidence actually arrives. A loopback provider that Two notes on sequencing. The advertise-and-gate half needs The gap this leaves, which we would like to fillThere is no conformance suite a provider can be run through. Core defends Found in
|
| The change | What it gives a publisher |
|---|---|
| A core that is neutral between vendors | No vendor's code sits inside the core everyone depends on, so no vendor's interests are built into it |
| Vendor modules owned and maintained by the vendor, with a maintainer recorded | You can see who stands behind the code carrying a vendor's name, and hold them to it |
| Permissions expressed as what data is used for, not which technology is allowed | The rule survives the next technology, because it never named one. It is also the way data protection law is written |
| A permissions manifest for a build | A deployment can state what it will do with data before it serves a single request, which is most of a privacy notice, generated from the modules rather than written by hand |
| The same evidence available to every vendor | Nobody gets a better view of the request than anybody else, so vendors compete on what they do rather than on access |
| Declarations that are machine-readable, in code that is open | A claim can be checked against actual behavior cheaply, by anyone, on every release rather than once at onboarding |
| A conformance suite any provider can be run through | A vendor can show their module behaves before shipping it, and the project can show it too |
| Startup that fails rather than falls back quietly | A misconfigured deployment stops instead of doing something nobody asked for and nobody notices |
Those are reasons for the wider ecosystem to engage with Trusted Server, not just reasons for us to like it. We would much rather arrive in New York with them shipped and running than describe them as a plan.
Section 3's Recognize row says a value the selected provider does not recognize "is never used or egressed". Three paths egressed one anyway. `append_ec_id` put the raw `ts-ec` cookie or `x-ts-ec` header on the outbound origin URL, `handle_first_party_click` put it on the click target's redirect URL, and the testlight integration put it in the proxied body as `user.id`. All three read through `edge_cookie::get_ec_id`, which checks the cookie-safe alphabet and the length cap and nothing else, so a value carrying another deployment's provider code (`zz00~...`), and any cookie at all in a deployment with no provider selected, was handed on. The code changes rather than the claim. `edge_cookie::recognized_ec_id` reads the value and then asks the selected provider whether it owns it, through `provider_owns_id`, which is the same test `EcContext` applies when it reads the cookie back, so the egress paths and the EC lifecycle agree on what this deployment issued. All three call sites use it. Behavior change: a deployment with no Edge Cookie provider selected now forwards no `ts-ec` value at all, on any of the three paths. It previously forwarded whatever the browser sent. An operator running stateless and relying on the raw cookie reaching the origin, the click target, or the testlight upstream will see that value stop arriving, and testlight, which requires an identifier, will fail the request rather than proxy it. The fix for such a deployment is to select a provider, which is what makes the value this deployment's to hand on. The testlight call site is in scope on the evidence rather than by assumption: its value is written into the request body as `user.id` by `rewrite_request_body` and that body is POSTed to the operator-configured endpoint, so the identifier leaves the edge even though the integration sets `forward_ec_id = false` (which suppresses only the query-parameter copy on the same request). The spec's Recognize row now names the three egress paths in the column that says where core applies recognition, and records that a stateless deployment recognizes nothing and so egresses nothing. The claim is left as strong as it was. Tests: each of the three paths, with a foreign-coded value and with a stateless deployment, plus a positive control on each that the deployment's own identifier still gets through. The testlight cases assert no upstream call is made at all. `click_appends_ec_id_when_present` used `ec-123`, which no provider issues, and now uses an identifier the built-in HMAC provider owns. Every new test was run against the unfixed code and failed there. Addresses: Aram Grigoryan review of PR 1043, crates/trusted-server-core/src/proxy.rs:1263 (question)
Section 3 said the pre-epic IP-cluster prefix listing "continues unchanged". The listing does, but the key space it lists over does not. A fresh mint is keyed `hmac~<hash>.<suffix>`, so the prefix `evaluate_cluster` derives is `hmac~<hash>` for a coded row while a legacy bare row still lists under `<hash>` alone. Prefix matching is anchored at the start of the key, so two rows for the same client IP that straddle the envelope never count each other and `cluster_size` under-reports while both populations coexist. The decision is to accept the undercount rather than bridge it, and the spec now says so along with the bound and the reasoning, and the prefix derivation in `evaluate_cluster` carries the same note so the next reader of that line is not surprised by it. The "gates nothing" half of the reasoning was checked rather than assumed. Every read of `cluster_size` in the workspace is a store, a log line, or the optional field in the identify response. The single read that reaches a branch is the cache short circuit in `evaluate_cluster` itself, which tests whether a value is stored, not what it is, so `Some(1)` and `Some(100000)` take the same path. There are no matches at all in the TypeScript or the integration tests. Two settings look like they gate on it and do not: `cluster_trust_threshold` (whose doc comment says entries at or below it "are treated as individual users for identity resolution") and `cluster_recheck_secs` are parsed and defaulted but have no readers anywhere in the code. They are noted here because they are what would make a reader believe the count is a control. They are outside this change; the unimplemented threshold wants an issue of its own. Addresses: Aram Grigoryan review of PR 1043, crates/trusted-server-core/src/ec/kv.rs:715 (thinking)
The two egress tests added in the previous commit dropped the `Report<TrustedServerError>` that `expect_err` returns on the floor. `Report` is `#[must_use]`, so building the library's test target warned, and clippy runs with `--all-targets -- -D warnings`, which would have failed the CI gate rather than only warning.
The spec's minimalism rule wants a production caller in the same change that introduces a method. `RequestInfo` arrived with seven accessors and only one of them, `client_ip`, is read by production code, in `HmacProvider::generate` at crates/trusted-server-core/src/ec/provider.rs. The other six had no non-test caller anywhere in the workspace on this branch as it stands: - `user_agent()` and `header_names()` had no caller at all, test or otherwise, beyond their two implementations. - `header()` and `query_param()` were called only by two test doubles in `ec/mod.rs`, both inside `#[cfg(test)]`, plus evidence.rs's own tests. - `path()` and `query()` were called only by evidence.rs's own tests, and `query()` by the default body of `query_param()`, which nothing called. Note the file is crates/trusted-server-core/src/evidence.rs; there is no `ec/evidence.rs`. The many `.path()`, `.query()` and `.header()` hits elsewhere in the workspace are `http::Uri`, `http::request::Builder` and the unrelated `http_util::RequestInfo` struct, which has `host` and `scheme` fields and none of these methods. All six are removed, along with everything that existed only to feed them: the `headers`, `path` and `query` fields and the `with_request_target` builder on both `OwnedRequestInfo` and `BorrowedRequestInfo`, the header snapshot argument of `OwnedRequestInfo::new`, `BorrowedRequestInfo::new` and the test-only `edge_cookie::generate_ec_id`, and the `request_headers` / `request_path` / `request_query` snapshot `EcContext` took at read time to fill them. Leaving state a provider can no longer read would be worse than the accessors themselves. Two test doubles went with them, `CookieCapturingProvider` and `EvidenceCapturingProvider`, along with the two tests that existed to prove the removed accessors carried cookies and query parameters. The third test that used `EvidenceCapturingProvider`, `a_provider_that_reads_no_client_ip_mints_when_the_host_has_none`, tests something else (a provider that needs no client IP still mints on a host that has none), so it stays, now with a `NoClientIpProvider` double that also asserts such a host passes the documented empty string rather than failing. The trait keeps its role as the seam. Its docs, the provider module docs and section 4 of the design now say that further evidence arrives as a defaulted accessor in the change that first reads it, rather than claiming a provider can already read headers, cookies, client hints and the URL. Probe: removing `client_ip` from the trait fails the library build at ec/provider.rs, where `HmacProvider::generate` reads it. Removing the other six failed nothing outside the tests deleted with them, which is the asymmetry this commit is about. Addresses: Aram Grigoryan review of PR 1043, crates/trusted-server-core/src/evidence.rs:27 (thinking)
Every Edge Cookie provider goes through one mechanism and none of them is special, so the selector no longer carries a variant for the built-in HMAC provider. `EcProviderSelection` is now `None` for explicit statelessness and `Named(String)` for a provider chosen by name, and `hmac` is an ordinary name in the same open-ended namespace a vendor crate names its own provider from. A variant per provider wrote the special case into the type, so every match on it had to know that one provider was different, and the built-in provider is due to become a vendor-supplied module rather than living in core. The one place that still knows `hmac` is built into core is the resolution in `build_provider`, lifted into `resolve_named_provider` and commented to say that it goes when the built-in provider becomes a module, after which `hmac` resolves through the injected path like any other name. Nothing else branches on whether a name is built in. `Ec::validate_provider_selection` is now a name lookup through the new `EcProviders::has_block`, and the unreferenced-block check reads the new `EcProviders::configured_keys` rather than pushing `hmac` in by hand, which also drops `has_vendor` and `vendor_keys`, both of which only answered for names that are not built in. `EcProviderSelection::HMAC_KEY` becomes the module-level `HMAC_PROVIDER_KEY` beside `HMAC_PROVIDER_CODE`, because the selection type should not name any one provider. The configuration surface does not change. `[ec] provider = "none"`, `"hmac"` and any vendor key parse to the same behavior and are written back as exactly the same string, which the round-trip test now proves on the serialized scalar itself rather than only on the surrounding document text. This is not the reviewer's finding about scattered string literals, which the typed selector already fixed. It is the project's own rule that no provider is special.
The response EC finalization edits is the finished one, so it already carries whatever the publisher's origin returned. The provider-header loop used `HeaderMap::insert`, which drops every existing value for that name, so a provider setting one evidence cookie deleted every `Set-Cookie` the origin had written, a publisher's session and sign-in cookies included, and a provider setting `Vary` deleted the origin's. `response_headers` is a list of pairs precisely so a provider can set more than one cookie, and `insert` collapsed those too. The rule, written out on the new `apply_provider_response_headers`, is that this seam is additive. A provider only ever adds evidence about the request, it never corrects the origin's output, so core has no grounds to discard a value it did not write. `Set-Cookie` can never be folded into one field line, the list-valued headers a provider realistically sets (`Vary` above all) mean the union of their field lines, and the single-valued headers where replacing would be right are exactly the ones `reserved_response_effect` already fails the request for. So nothing a provider may set here needs to replace, and appending is the direction that cannot silently destroy someone else's header. No test anywhere covered a provider header reaching a response. The new one drives a provider that sets its own cookie and its own `Vary` through the real mint path onto a response the origin has already written to, and asserts the origin's cookie, the provider's cookie, core's own `ts-ec` and both `Vary` entries all survive.
The pluggable-providers spec required, in its provider-switching section,
that switching must not strand the identities the previous provider
minted and "above all must not make a later opt-out unable to revoke
them". It then claimed old cookies stay recognized after a switch
whenever the newly selected provider accepts their shape.
That claim is false and cannot be made true here. Ownership is decided on
the `{code}~` prefix before any provider is asked about shape, and the
check is enforced twice: `AcceptedProviders::owner` dispatches on the
code, and `canonical_kv_key` re-checks the derived key through
`provider_owns_id`. So a newly selected provider rejects every identifier
the previous one minted, whatever its shape. The new test drives this and
shows the result: after a switch the retired identifier is never adopted,
withdrawal still expires the browser cookie, but the retired provider's
identity-graph row keeps `consent.ok = true` and is never tombstoned. It
then sits for the one-year entry TTL.
I changed the spec rather than the code. The mechanism the spec itself
names for carrying identities across a switch is the `legacy_providers`
reader list, which the same section marks as deferred to the migration
spec, and `AcceptedProviders` is already built as the seam for it. Even
once it lands the requirement would not hold on its own, because it would
depend on the operator listing the retired provider, so an unconditional
guarantee was never something the code could provide. The old wording
also contradicted section 5 of the same document, which already states
the true rule that a cookie carrying another provider's code is treated
as absent.
The replacement says plainly what a switch does to read-back, to the
browser cookie and to the graph rows, and what an operator must do about
revocation: deal with the retired provider's rows at the switch, since
they are identifiable by that provider's `{code}~` key prefix, or accept
that later withdrawals are recorded only in the browser until the TTL
expires. The `cookie_ec_kv_key` doc comment claimed the same reach the
spec did and is corrected to match.
The provider series design specs move to the spec-only PR (IABTechLab#1084) so they can be reviewed before the code that implements them. Three doc comments cited those files by repository path, which no longer resolves from this branch. Refer to each document by name instead, so the comment stays true whichever PR is read first.
The mint-rejection fix restored one collapsed continuation in ec/mod.rs and said the rest of that file was clean, which it was. The same fault exists in four more places on this branch, so fixing only the reported one leaves the pattern half addressed. Each was written across two source lines without the trailing backslash, so the source indentation became a run of spaces inside the message: ec/admin.rs:373 the invalid-EC-ID response an operator sees ec/finalize.rs:125 the skipped-response-write log line ec/provider.rs:635 the missing-client-IP error from the HMAC provider ec/pull_sync.rs:72 the skipped-dispatch log line integrations/testlight.rs:196 the no-recognized-EC-ID error The continuation is restored in each, so every message reads as one sentence. The whole of trusted-server-core was scanned for the same shape, matching runs of five or more spaces inside a string literal. The only remaining matches are TOML fixtures in settings.rs tests, where the embedded newlines are deliberate. Addresses: ec/mod.rs:444 follow-up, the same fault outside the file first reported
The commit that rewrote the module docs to match the trait signature left two item-level doc comments in the same file still describing constructor injection, so the claim that nothing passes evidence by constructor was contradicted three declarations further down. IdentityInput's doc said request data reaches a provider "through the services injected into its constructor". EdgeCookieProvider::generate's doc said the identifier is derived "from the provider's injected services". Neither matches the signature, which takes request_info: &dyn RequestInfo as a parameter and reads evidence from it. The built-in HMAC provider does exactly that at ec/provider.rs:632. Both now describe the parameter the evidence actually arrives on. The crate was searched for the same wording; the only other mention is in ec/mod.rs on a test-only helper, where it correctly describes how the provider itself is constructed rather than how request evidence reaches it. Addresses: ec/provider.rs:4 follow-up, item docs still describing constructor injection
`ProviderCode::new` is public and validated its argument with `assert!`, so any caller outside this workspace could take down a live request by passing a code that was not exactly four characters of [a-z0-9]. The doc comment claimed the panic "never" fires on a request path, which held only for as long as every caller happened to pass a literal, and nothing enforced that. A vendor Edge Cookie provider is exactly the caller the claim could not cover. `new` now returns `Option<ProviderCode>`, so it cannot panic whatever it is given, and a caller outside core has to handle a malformed code. The compile-time guarantee the codes in this workspace relied on moves into a new `provider_code!` macro, which runs the same check inside a `const` block, so a bad literal fails the build and the value it yields needs no unwrapping. Every code in the workspace, the built-in HMAC code included, now goes through the macro. Addresses: crates/trusted-server-core/src/ec/provider.rs, where `ProviderCode::new` could panic at run time while its documentation said it could not.
`resolve_named_provider` looked for a built-in provider before the one the adapter injects, so a vendor provider whose id is `hmac` was dropped in favour of core's own and nothing said so. Nothing reserved the name and nothing warned, which left an operator with a configured vendor provider that never ran and no way to see why. This is not only a missing warning. Once this work merges, IAB Tech Lab is itself a vendor shipping an HMAC provider while core still ships one, so two suppliers really can arrive under one name in a single deployment, and there is no correct way to pick between them. `build_provider` now refuses that pair through `ensure_no_name_collision` and the error names both claimants, core and the deployment's adapter, along with the contested name. The check runs before the selector is read, so selecting a different provider does not hide the clash, and because the adapters call it through `ensure_provider_available` while they build application state, an operator is told at startup rather than on the first request that happens to select the name. Addresses: crates/trusted-server-core/src/ec/provider.rs, where `resolve_named_provider` silently preferred the built-in `hmac` provider over an injected one of the same name.
`EC_RESPONSE_HEADERS` in the EC finalization module and the first four entries of `INTERNAL_HEADERS` in the constants module were the same four header names written out twice, in two files, with nothing keeping them in step. The two lists do different jobs, one is stripped from a response the request may not carry an identity on and the other is never forwarded to a third party, but every Edge Cookie output header has to be in both, so adding a fifth to one and forgetting the other would send Edge Cookie output to an origin that should never see it. `EC_RESPONSE_HEADERS` now lives once, in the constants module, and `INTERNAL_HEADERS` is assembled from it and the remaining internal names while the crate is compiled, so the Edge Cookie half cannot be edited in one place and missed in the other. EC finalization reads the same constant instead of keeping a copy. The new test in the constants module asserts the containment, the total, and that no name appears twice, so going back to two hand-written lists fails the build. Addresses: crates/trusted-server-core/src/ec/finalize.rs and crates/trusted-server-core/src/constants.rs, where one list of Edge Cookie response headers was maintained by hand in two places.
The composition root resolved `[ec] provider` and threw the provider away, keeping only the knowledge that the selection could be satisfied, and then the request path resolved the same settings again to get a provider it could use. On the Fastly, Cloudflare and Spin adapters that is twice for every request, because those three run a fresh instance per request and rebuild application state each time, which was confirmed by reading `run_app` in the matching edgezero adapters. The composition root now keeps what it resolved, in `AppState`, and hands the same instance to every request through the new `RuntimeServices::resolved_ec_provider`. Core reads it through `request_provider`, which returns the threaded instance when there is one and otherwise resolves exactly as before, so an adapter that threads nothing, the core tests and any embedder driving core directly included, keeps today's behaviour, the loud failure on a selected but uninjected provider included. Nothing about which provider is chosen changes, only how many times the choosing happens. The Axum adapter is deliberately left checking rather than keeping, because it is a long-lived process whose application state is built once at start-up, so it has no second resolution to save. Addresses: crates/trusted-server-core/src/ec/provider.rs and the Fastly, Cloudflare and Spin adapters, where `ensure_provider_available` and `EcContext::read_from_request` each built the provider once per request.
The Spin adapter cannot start on upstream/main today, and this fixes it here. `build_state` compiled `trusted-server.example.toml` into the binary and parsed it, but that template ships placeholder secrets by design and its placeholder admin password is the first entry in `PASSWORD_PLACEHOLDERS`, so `validate_admin_handler_passwords` refused it every time. `build_state` therefore never returned `Ok`, the router fell back to the start-up error handler, and the component answered 503 to every request. The failure is "Handler `^/_ts/admin` uses a placeholder password; configure a strong secret". Nothing caught it because nothing called `build_state`. Every Spin test enters through the `routes_with_settings` parity seam and supplies its own settings, so the one path a deployed component actually takes was the one path never exercised. Settings now come from the platform config store at run time, which is what the Fastly, Axum and Cloudflare adapters already do, so an operator publishes one with `ts config push` and the component reads it. The new `SpinPlatformConfigStore` reads Spin component variables directly rather than through the per-request handle, because application state is built before any request context exists. Component variables are ambient, which is how the secret store already reads them, and both paths map keys through `spin_variable_name` so start-up and the request path read the same variable for the same key. The new test calls `build_state` and requires any failure to be the absence of a config store. Outside the Spin runtime there are no component variables, so it cannot return `Ok` under `cargo test`, but a configuration compiled into the binary would fail for a different reason and the test says so. Restoring the old body fails it with the placeholder-password message. Addresses: crates/trusted-server-adapter-spin/src/app.rs, where `build_state` parsed a baked example template whose placeholder admin password made every request fail.
`get_ec_id` is public on upstream/main today and this fixes it here. It
reads the `x-ts-ec` request header and then the `ts-ec` cookie, and
checks the result only with `ec_id_has_only_allowed_chars`. That function
is the global cookie backstop, the length cap and the cookie-safe
alphabet, and its own documentation in `ec/cookies.rs` says the strict
check is the one used to reject untrusted request values. On its own it
accepts any run of `[A-Za-z0-9._~-]` up to the cap, so it cannot tell an
identifier this deployment minted from one an attacker typed. `x-ts-ec`
is stripped from responses but not from inbound requests, so the header
really is the client's to set, and the raw reader prefers it over the
cookie. This is the inbound twin of the egress fault this branch already
fixes, which is why it belongs here.
The right check is not the built-in strict format validator. A vendor
provider's identifier is not required to match the HMAC
`<64 hex>.<6 alphanumeric>` shape, so holding every deployment to it
would drop exactly the opaque identifiers the provider model exists to
carry. The right check is provider ownership, where the `{code}~` prefix
is dispatched to the provider that owns it and that provider's
`accepts_id` decides, which is what `recognized_ec_id` already does and
what the EC lifecycle applies on read-back.
The raw reader cannot make that check, because it has neither settings
nor the selected provider, so it stops being a public entry point. It is
now `pub(crate)` and named `unvalidated_ec_id_from_request`, so no caller
can read it as returning a validated identifier, and `recognized_ec_id`
is the only way in from outside the module. Nothing outside the crate
called the old name.
The new test drives three identifiers this deployment could never have
issued through both readers, shows the bounds alone accept all three,
and requires the public path to recognize none of them, while an
identifier the selected provider does own is still returned. Replacing
the ownership check with the bounds fails it on the first one.
Addresses: crates/trusted-server-core/src/edge_cookie.rs, where
`get_ec_id` was public and validated client-supplied identifiers with the
outbound backstop list.
Reverts the removal of the request-evidence accessors, so RequestInfo carries the client IP, the User-Agent, headers by name, header names, the path, the query and its parameters again. They were removed to satisfy a rule in our own specification, which says every trait method needs a production caller in the change that introduces it. That rule is right for a behavioural trait, where a method nothing calls is dead weight. It is wrong for an evidence interface, and applying it here was our mistake rather than anyone else's. An evidence interface describes what a request carries, not what today's code happens to read. Held to the caller rule it grows a method every time a vendor arrives, so no vendor can write against it and it cannot stay stable across a release. It also puts the boundary in the wrong place, because what a provider may see was never the control. What a provider may do with what it sees is the control, and that is the permission model. The specification is amended in the same series rather than quietly ignored. Two test provider codes restored with the revert predate ProviderCode::new returning an Option, so they now build through the macro that cannot fail. Addresses: crates/trusted-server-core/src/evidence.rs, an evidence interface narrowed to today's callers
Fixes doc comments, error strings and TOML comments on split/1 so they match the code they describe, and applies house-style wording rules to every added line touched. Continues and completes work a prior agent started (which stopped partway through the B3 item list), reviewed against the run books at .claude/pr1/runbook-track-1-code-chain.md and .claude/pr1/comments-docs-runbook.md in the trusted-server repo. F2 (verified, already done by the prior agent): added "cache-control" to FRAMING_OR_HOP_BY_HOP_HEADERS in ec/provider.rs with a regression test, and removed the false reference to a per-adapter is_hop_by_hop_response_header function. B3.9 (verified, already done by the prior agent): recorded, rather than fixed, the gap where pull sync (ec/pull_sync.rs) and the admin lookup (ec/admin.rs) key identity-graph rows by the raw identifier instead of the canonical form the three organic paths use. A doc comment on EcContext::kv_key_for and AcceptedProviders now names the gap and points at commit 343ac3e, which fixed the three organic paths. Recorded as a known issue for a later change rather than changed now, because routing these two paths through the canonical key this late changes behavior. Wrong-claim and stale-reference fixes (runbook Part B3, items 1-25): verified each item against the current tree. Most were already corrected by the prior agent (HmacProvider failure handling, the environment-variable override claim, EcProviders selection docs, provider construction timing, BorrowedRequestInfo allocation, the generate_if_needed and validate_provider_selection # Errors lists, the retirement-arithmetic doc, kv_key_for, ec_allowed, request_headers, edge_cookie.rs recognized_ec_id, admin.rs and cookies.rs identifier grammar, IdentityInput gating, and the plural "built-in providers" wording in platform/types.rs). This pass added the one remaining fix: crates/trusted-server-adapter-spin/src/app.rs no longer claims Cloudflare reads settings the same way as Fastly and Axum (Cloudflare also reads a JSON binding and compiles in the example TOML natively). House-style sweeps (runbook Part C), restricted to lines split/1 added over upstream/main d516a9e, verified per line via git diff before editing: - "mint"/"minted"/"mints" -> "create"/"created"/"creates" (or "issue"/ "derive" by sense) in doc comments, inline comments and expect()/ assert messages across ec/provider.rs, ec/mod.rs, ec/admin.rs, ec/batch_sync.rs, ec/cookies.rs, ec/finalize.rs, ec/generation.rs, ec/identify.rs, ec/pull_sync.rs, edge_cookie.rs, integrations/testlight.rs, platform/test_support.rs, proxy.rs, and crates/trusted-server-adapter-fastly/src/app.rs. Left the `mint: bool` test-builder struct field name alone (an identifier, not prose), and left every "mint" occurrence that predates split/1 alone (confirmed against upstream per file before editing; two lines were edited by mistake and then reverted once the upstream check showed they were pre-existing text, not split/1 additions). - "initialise" -> "initialize" in the four adapters' build_state / build_state_with_settings doc comments. - "several" -> "multiple" in ec/generation.rs. - "HTTP/2 fingerprint" -> "TLS and HTTP/2 signal" in ec/generation.rs. - "built-in HMAC default" -> "built-in HMAC provider" (there is no default provider) in crates/edgecookie/README.md. - Environment variable casing: TRUSTED_SERVER__ec__provider -> TRUSTED_SERVER__EC__PROVIDER in trusted-server.example.toml, to match settings.rs and the documented upper-case form. - Bytes vs characters: trusted-server.example.toml's new [ec.providers.hmac] block comment now says ">= 32 bytes" to match the startup error message, which counts bytes. - A8 cargo-feature overclaim: trusted-server.example.toml no longer says a vendor provider "needs its own cargo feature" (none exists); it now says a vendor provider ships in its own crate the adapter composes in. - A15 environment-loading overclaim: trusted-server.example.toml's [ec] block comment now matches settings.rs, saying deployment tooling can merge an environment value into the published configuration before load, and that the running server itself reads settings from the platform config store, not the environment. Also fixed two doc-comment line-wrap glitches left by the prior agent's edits (a stray single-word line in ec/provider.rs's build_provider doc and in settings.rs's Ec::provider doc), where a mid-sentence line break had been left in place after wording changed. Left for a documented human decision rather than changed, per the runbook's own "James decides" note: two em dashes in trusted-server.example.toml:76 and crates/trusted-server-core/README.md that follow an existing dash-separated heading/bullet convention used throughout each file; and the one new "test-publisher.com" test URI in ec/identify.rs, which matches roughly twenty pre-existing (not split/1-introduced) occurrences of the same fixture domain already in that file, so changing only the new one would be inconsistent and changing the rest is outside split/1's introduced lines. Out of scope for this branch, so not touched: the "no host-specific call" overclaim (B1 item 1, on split/2/3), the geo-default docs (A1, on split/6/7), and the drafted GitHub text fixes (A2-A4, A10-A16), all of which live on later branches or in .claude/pr1/ review artifacts. Not built or tested per instructions; verification is deferred to the full-stack gate run after every branch in the chain is rebased. AI assistance note: this commit was produced by an AI coding session that continued a prior AI session's partially completed edits, reading both against the run books named above. A human should review the "James decides" items before the stack is pushed.
The provider reached the request path two ways, through the adapter-resolved resolved_ec_provider and through a raw ec_provider slot on RuntimeServices that only the core test helpers ever set. Section 3.6 of the integration provider seam design specifies one path, so the raw slot goes and every caller now reaches a provider through the resolved seam. request_provider builds from [ec] settings alone when nothing was threaded, and the test helpers thread their provider the way a production adapter does, so the tests exercise the path production uses.
CI compiles the test build with -D warnings, where NoClientIpProvider is an error because nothing constructs it. The test it was written for had been rewritten around the evidence-capturing provider, leaving the fixture behind. The seam branch already removes it for the same reason, so remove it here where it first appears and the whole chain stays consistent.
Answers the architectural finding on IABTechLab#1043 rather than deferring it. Vendor identity reached core through RuntimeServices injection while everything else a module supplies was declared on its integration registration, so there were two extension mechanisms and identity was on the second one. A registration can now declare an Edge Cookie provider and a device provider the same way it declares a geo provider. The registry resolves each against its selector, warns when a module declares a capability the selector does not choose, and the adapters apply all three to RuntimeServices in one place. RuntimeServices gains a device slot, which it had no way to carry before, and with_ec_provider and with_device_provider to match with_geo. Identity and device differ from geo in one way that matters. Both have providers built into core, so a selector naming a built-in is not an error at the registry, and resolution returns None for those and lets core resolve them as before. Two closed allowlists had to open, being the same fault in two more places. The device selector accepted only `builtin` and `fastly`, so any module id was rejected before the registry saw it. The jurisdiction check asked whether the geo selector was `platform`, so a deployment selecting a module's geo provider was told it had none. Neither could stand once a module can supply these. Proven by the probe, which now declares all three capabilities from one registration. Two tests select the module for identity and for device and assert the resolved provider is the module's own. The probe's identity provider mints a value the built-in HMAC provider cannot produce, so a passing assertion means the module's provider ran rather than core's. Addresses: Aram, `ec/provider.rs`, vendor identity should lean on the integration system rather than a second extension mechanism
8362aef to
c16c99d
Compare
The five-PR series (IABTechLab#1043 to IABTechLab#1047) opens the identity, device and geo seams. The nine vendor integrations already in core sit behind the integration registry instead, which is a private table, so none of them can move out until that table is opened. This spec defines the one core change that opens it: public registration builders with a second input on IntegrationRegistry, browser JavaScript carried on the registration, startup validation as a hook, the same treatment for auction providers and the bid renderer contract, and neutral replacements for the two places where a vendor reaches into core. It then sets out the migration of all nine existing integrations, one PR each. The change is complete in itself: after it, no vendor move needs a core change. Written against the series' tree with the file and line references for every claim about the current code. Documentation only.
The review of IABTechLab#1043 asked that spec changes land before the code that implements them, so a divergence is a decision taken in review rather than a ratification of something already merged. PRs IABTechLab#1043 to IABTechLab#1047 each carried the design document for their own step, and IABTechLab#1043 carried a 607-line spec describing device providers, geo providers, the permission model and the browser resolve endpoint, none of which is in that PR. Move all six series documents here, so this PR carries the complete normative set and no code: - 2026-07-30-pluggable-providers-design.md (from IABTechLab#1043) - provider-code-registry.md (from IABTechLab#1043) - 2026-07-30-permission-model-design.md (from IABTechLab#1045) - 2026-07-30-client-cycle-ec-resolve-design.md (from IABTechLab#1046, later revised by IABTechLab#1047) - 2026-07-30-integration-response-header-hook-design.md (from IABTechLab#1047) - 2026-07-30-provider-migration-rollout-design.md (from IABTechLab#1047) Each file is taken verbatim at the tip of the stack, so the later revisions are preserved: the provider-switching continuity section, the geo requires-signal floor, and the code-envelope paragraph IABTechLab#1047 added to the client-cycle spec. The revision-record tables are unchanged. No document's substance was edited. The only edits are to this spec's own status line, which said the PR adds one document and that the series specs land with IABTechLab#1047, and a revision-record row recording the move.
The four series specs (client-cycle EC resolve, permission model, pluggable providers, migration and rollout) each carried a Status line saying they were implemented. The code they describe is only in PRs IABTechLab#1043 to IABTechLab#1047 and none of those is merged, so the line read as shipped behavior. Each now says Proposed, names the PR that carries the implementation and states that it is not yet on main, keeping the existing revision dates and notes. The integration provider seam spec carried counts and line references that do not hold on main at d516a9e. Corrected against that commit: - Section 4 said migration_guards.rs embeds "the thirteen vendor files". The directory holds 23 .rs files (2 infrastructure, 6 in nextjs/, 2 in datadome/, 13 top-level integration modules), the guard embeds 20 of them and 9 of those 20 belong to the nine vendors, with osano.rs and the two datadome/ files absent. builders() registers 13 integrations, which is a different 13 from the file count. - Section 3.5 gave no counts for the prepare and finalize calls. There are nine production prepare_request call sites across the four adapters and a tenth in core, and the single production finalize_response call site is in core rather than in any adapter. - Section 8 item 3 described a proxy resolving geo twice, which does not happen on main. The real double resolution is the adapter EC context build against handle_auction on POST /auction. - Section 8 item 5 understated the Spin gap and misdescribed Cloudflare. Cloudflare covers every route it registers and has no health route, while Spin skips its first-party bindings as well as its inline admin stubs. - Line references: settings.rs:166 to :215, auction/mod.rs:49 to the list at :51 to :53, publisher.rs:4361 to :4369. Section 6 now requires the round trip to be proven on the Fastly adapter, the primary deployment target, rather than on any adapter, because Fastly has no library target and the round trip otherwise only runs on the Axum dev server.
Answers the architectural finding on IABTechLab#1043 rather than deferring it. Vendor identity reached core through RuntimeServices injection while everything else a module supplies was declared on its integration registration, so there were two extension mechanisms and identity was on the second one. A registration can now declare an Edge Cookie provider and a device provider the same way it declares a geo provider. The registry resolves each against its selector, warns when a module declares a capability the selector does not choose, and the adapters apply all three to RuntimeServices in one place. RuntimeServices gains a device slot, which it had no way to carry before, and with_ec_provider and with_device_provider to match with_geo. Identity and device differ from geo in one way that matters. Both have providers built into core, so a selector naming a built-in is not an error at the registry, and resolution returns None for those and lets core resolve them as before. Two closed allowlists had to open, being the same fault in two more places. The device selector accepted only `builtin` and `fastly`, so any module id was rejected before the registry saw it. The jurisdiction check asked whether the geo selector was `platform`, so a deployment selecting a module's geo provider was told it had none. Neither could stand once a module can supply these. Proven by the probe, which now declares all three capabilities from one registration. Two tests select the module for identity and for device and assert the resolved provider is the module's own. The probe's identity provider mints a value the built-in HMAC provider cannot produce, so a passing assertion means the module's provider ran rather than core's. Addresses: Aram, `ec/provider.rs`, vendor identity should lean on the integration system rather than a second extension mechanism
Answers the architectural finding on IABTechLab#1043 rather than deferring it. Vendor identity reached core through RuntimeServices injection while everything else a module supplies was declared on its integration registration, so there were two extension mechanisms and identity was on the second one. A registration can now declare an Edge Cookie provider and a device provider the same way it declares a geo provider. The registry resolves each against its selector, warns when a module declares a capability the selector does not choose, and the adapters apply all three to RuntimeServices in one place. RuntimeServices gains a device slot, which it had no way to carry before, and with_ec_provider and with_device_provider to match with_geo. Identity and device differ from geo in one way that matters. Both have providers built into core, so a selector naming a built-in is not an error at the registry, and resolution returns None for those and lets core resolve them as before. Two closed allowlists had to open, being the same fault in two more places. The device selector accepted only `builtin` and `fastly`, so any module id was rejected before the registry saw it. The jurisdiction check asked whether the geo selector was `platform`, so a deployment selecting a module's geo provider was told it had none. Neither could stand once a module can supply these. Proven by the probe, which now declares all three capabilities from one registration. Two tests select the module for identity and for device and assert the resolved provider is the module's own. The probe's identity provider mints a value the built-in HMAC provider cannot produce, so a passing assertion means the module's provider ran rather than core's. Addresses: Aram, `ec/provider.rs`, vendor identity should lean on the integration system rather than a second extension mechanism
Answers the architectural finding on IABTechLab#1043 rather than deferring it. Vendor identity reached core through RuntimeServices injection while everything else a module supplies was declared on its integration registration, so there were two extension mechanisms and identity was on the second one. A registration can now declare an Edge Cookie provider and a device provider the same way it declares a geo provider. The registry resolves each against its selector, warns when a module declares a capability the selector does not choose, and the adapters apply all three to RuntimeServices in one place. RuntimeServices gains a device slot, which it had no way to carry before, and with_ec_provider and with_device_provider to match with_geo. Identity and device differ from geo in one way that matters. Both have providers built into core, so a selector naming a built-in is not an error at the registry, and resolution returns None for those and lets core resolve them as before. Two closed allowlists had to open, being the same fault in two more places. The device selector accepted only `builtin` and `fastly`, so any module id was rejected before the registry saw it. The jurisdiction check asked whether the geo selector was `platform`, so a deployment selecting a module's geo provider was told it had none. Neither could stand once a module can supply these. Proven by the probe, which now declares all three capabilities from one registration. Two tests select the module for identity and for device and assert the resolved provider is the module's own. The probe's identity provider mints a value the built-in HMAC provider cannot produce, so a passing assertion means the module's provider ran rather than core's. Addresses: Aram, `ec/provider.rs`, vendor identity should lean on the integration system rather than a second extension mechanism
Answers the architectural finding on IABTechLab#1043 rather than deferring it. Vendor identity reached core through RuntimeServices injection while everything else a module supplies was declared on its integration registration, so there were two extension mechanisms and identity was on the second one. A registration can now declare an Edge Cookie provider and a device provider the same way it declares a geo provider. The registry resolves each against its selector, warns when a module declares a capability the selector does not choose, and the adapters apply all three to RuntimeServices in one place. RuntimeServices gains a device slot, which it had no way to carry before, and with_ec_provider and with_device_provider to match with_geo. Identity and device differ from geo in one way that matters. Both have providers built into core, so a selector naming a built-in is not an error at the registry, and resolution returns None for those and lets core resolve them as before. Two closed allowlists had to open, being the same fault in two more places. The device selector accepted only `builtin` and `fastly`, so any module id was rejected before the registry saw it. The jurisdiction check asked whether the geo selector was `platform`, so a deployment selecting a module's geo provider was told it had none. Neither could stand once a module can supply these. Proven by the probe, which now declares all three capabilities from one registration. Two tests select the module for identity and for device and assert the resolved provider is the module's own. The probe's identity provider mints a value the built-in HMAC provider cannot produce, so a passing assertion means the module's provider ran rather than core's. Addresses: Aram, `ec/provider.rs`, vendor identity should lean on the integration system rather than a second extension mechanism
3 participants
First of five stacked PRs decomposing #838 as requested in the #986 review, where each PR carries one feature and its design spec. This PR is the Edge Cookie provider seam. The stack order is #1043, #1044, #1045, #1046, #1047. Each PR's own change is visible by comparing its head branch to the previous PR's head branch, and this first PR is independently mergeable to
main.Spec: docs/superpowers/specs/2026-07-30-pluggable-providers-design.md, which is the Tech Lab 2026-07-31 draft revised to match this implementation, with a revision-record table listing every divergence and why. The spec covers both this PR (the EC seam) and #1044 (device and geo selection).
What this PR does
Edge Cookie identity generation becomes a selectable provider behind the
EdgeCookieProvidertrait incrates/trusted-server-core/src/ec/provider.rs, with the existing HMAC implementation as the built-in and configuration selecting it.[ec] providernames a block under[ec.providers.<key>]. Omitted means stateless with no Edge Cookie, andprovider = "none"spells the same choice explicitly (rejected if provider blocks are left configured). A selected provider with no block, an unreferenced stray block, and an unknown key in a block all fail at startup, so misconfiguration is loud.[ec] passphraseform still starts. It migrates toprovider = "hmac"plus[ec.providers.hmac]with a deprecation warning, so a fleet can move configuration and binaries independently. Both forms together are rejected.[A-Za-z0-9._~-], enforced at mint, cookie read-back, and cookie write. A violating identifier is rejected outright and never rewritten, so the cookie value and the identity-graph key can never silently diverge (the previous sanitize-by-stripping path is removed).accepts_id, and the identity-graph key through itsnormalize_id_for_kvcanonical form, so an opaque vendor identifier round-trips byte-for-byte. One test proves a non-default provider round-trips verbatim and another proves the graph is keyed by the canonical form.[ec.providers.<key>]blocks are captured as raw values in core and deserialized by the adapter that injects the vendor provider, so core never names a vendor.{code}~value, checks the code at read-back, and keys the identity graph with it, so identifiers from different providers can never collide and switching providers cannot silently adopt another provider's identities. The built-in provider mintshmac~<hash>.<suffix>and dual-reads its pre-envelope bare form for one release cycle, so deployed cookies keep working.hmac~identifier was skipped by pull sync, refused by batch sync and answered 400 by the admin lookup while CI stayed green on a bare seeded cookie.is_valid_ec_idnow accepts thehmac~envelope as well as the legacy bare form and rejects any other provider's code,normalize_ec_id_for_kvkeeps the envelope so the key matches the one written at mint, and each of the three call sites has a test with a coded identifier (commit6f15e50c0).Breaking change
A minimum HMAC passphrase length of 32 bytes is now enforced wherever the passphrase is configured. A shorter passphrase that previously started will fail startup validation with a direct message.
How it was verified
Full local gate on this branch, all clean.
cargo test-fastly(core plus adapter suites),cargo test-axum,cargo test-cloudflare,cargo test-spin, the integration parity suite,cargo fmt --check, and all six per-target clippy aliases.Framing
Privacy is a spectrum, and this change is neutral infrastructure. It does not decide whether identity is created, it makes that decision configurable and inspectable, and the deployer selects a provider (or none) according to the laws and policies that apply to them. Trust comes from that flexibility being respected and visible in configuration rather than hard-coded.
References #777. Decomposes #838 (kept as a draft reference until this series merges). Spec baseline from #986.
Produced with AI assistance under James Rosewell's direction, and flagged here so reviewers know to apply the usual scrutiny.