Add a pluggable Edge Cookie provider seam with the built-in HMAC provider - #1043
jwrosewell wants to merge 46 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
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
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.
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.
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
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
|
Wanted to flag a few process things that are making it hard for us to keep up with the review, rather than any issue with the work itself. Force pushes. The branches are being force-pushed, which means the previous commits go away and GitHub can no longer show us what changed since our last look. On a PR this size that costs us a lot: we end up re-reading the whole diff rather than just the delta. Could you push follow-up commits on top instead? We're happy to squash at merge time, so nothing needs to stay messy in the final history. Resolving review threads. When a comment has been addressed, hitting Resolve conversation on the thread makes a real difference. Right now we can't tell at a glance which points are done and which are still open, so we re-check all of them. Your written responses have been thorough, and this just makes them easier to track against the code. Re-requesting review. After you've pushed changes, please click the re-request review button next to our names. That's what puts the PR back in our queue. Without it we don't get notified, so a PR can sit longer than it should while we assume you're still working on it. None of this is a comment on the substance, which we're working through separately. It's just that the current shape makes each pass more expensive than it needs to be, and these three things would speed us up considerably. |
|
@aram356 Taking these in turn. Force pushes. Agreed. Follow-up commits from here, and squash at merge. Resolving threads. All six of Christian's now have replies in place and Re-requesting review. We cannot. Our access to this repository is read Something that explains more of the delay than any button. We asked for this Status, so it is written down somewhere you can point at. Everything from your Note on how this was written. An AI assistant drafted this comment and checked |
Brings main at 066ea3c into split/1-ec-provider. Twelve files conflicted and one more needed a change without a textual conflict. Most resolutions keep both sides. Five needed a decision, recorded here so that no behavior changes silently. Edge Cookie generation, in ec/mod.rs. Main's IABTechLab#885 creates a row only when no row holds the key, retries on a collision and binds the request snapshot to the new row. This branch creates identifiers through the selected provider. Both are kept. Each attempt asks the provider for a candidate through the new EcContext::candidate_id, which runs the reserved-header and identifier-bound checks, then creates the row with create_if_absent under the provider's canonical key and binds the snapshot to that key. Orphan recovery, in ec/finalize.rs. Main's IABTechLab#885 rotates an orphaned cookie by generating an HMAC identifier directly, which would give a vendor-provider deployment a built-in identifier. Recovery now asks the selected provider through candidate_id, keeps main's proof of absence and retry limit, and does nothing when no provider is selected. Any headers the provider asks for while creating the replacement are applied to the response. Withdrawal, in ec/finalize.rs. Main's IABTechLab#1113 hardening is kept whole, being finalize_unusable_consent, the existence check inside write_withdrawal_tombstone, the snapshot write-back and log_tombstone_outcome. The tombstones are keyed by this branch's withdrawal_kv_keys, the canonical keys of the cookie and the active identifier, in place of withdrawal_ec_ids, and the write-back compares against the active identifier's canonical key. EID ingestion on the returning and generated paths uses main's collect-then-upsert form under the canonical key. Secret references, in config.rs. Main's IABTechLab#1036 resolves secret settings from the secret store by the paths listed in secret_fields. This branch makes the legacy ec.passphrase optional and adds the [ec.providers.hmac] passphrase, which was not listed, so it would have been read as a literal value. Both are now listed as optional paths and checked as key references when the configuration is pushed, and the integration fixture and the example configuration name the secret key rather than a value. Spin start-up, in the Spin adapter. Main's IABTechLab#1036 loads settings from the config store with secret resolution, which fixes the same start-up failure this branch's Spin commit fixed. Main's loader is taken and SpinPlatformConfigStore, which nothing else uses, is removed. The four adapters keep this branch's composition-root provider check, and Axum, Cloudflare and Spin keep its error propagation from build_ec_context, alongside main's compiled auction plan from IABTechLab#1016. Tests. Main's collision-retry tests and its orphan-rotation tests built their context with no provider selected, which main's code did not need. They now select the built-in HMAC provider, as an HMAC deployment does, because with no provider there is nothing to create or rotate. No other test reaches identifier creation or orphan recovery without a provider selected. Tests from each side were moved onto the other side's interfaces, being this branch's six-parameter context helpers, main's mutable context in ec_finalize_response, and the [ec.providers.hmac] passphrase in main's two secret validation tests.
Main's IABTechLab#1016 builds the orchestrator and the integration registry from a compiled auction plan, and limits IntegrationRegistry::new to core's own tests. The merge of main moved the Axum adapter's state building onto the plan and took main's imports, but missed the test helper state_with_uninjected_provider, which still called build_orchestrator and IntegrationRegistry::new, so the adapter's library tests stopped compiling. The helper now builds both from the plan, the same way build_state_with_settings does.
Christian Pavilonis's review thread on provider response effects asked core to validate them against the managed ts- cookies, the x-ts- namespace and framing headers. The check is in place, but several comments said a rejected effect "fails the request", which is not what happens. EcContext::generate_if_needed returns the error, and its only two callers outside tests, the publisher fallback in the Fastly adapter and IntegrationRegistry::handle_proxy, log it and serve the response without an Edge Cookie. The reserved_response_effect and apply_provider_response_headers docs, the comment in EcContext::candidate_id and the reserved-surface test now say that. The candidate_id comment no longer implies the rejection matches the identifier-bounds check in every respect, because that check runs after the provider's headers are captured. A new test, a_rejected_provider_effect_never_reaches_the_finalized_response, proves a rejected header never reaches the response. For each reserved effect it lets generation fail, runs EC finalization on the same context, and checks the response carries no forged ts-ec cookie, no x-ts-ec header and no transfer-encoding. It failed when the header capture in candidate_id was moved ahead of the reserved check, and passes with the code as it is. The test helper now hands back the context even when generation fails, so the test can finalize on it. The review thread is IABTechLab#1043 (comment)
Christian Pavilonis's review thread on partner paths asked for validation and KV normalization to be dispatched by provider code. Validation already was, but several paths still read or wrote identity graph rows under the identifier as issued, while generation stores each row under the owning provider's canonical form. For a provider whose canonical form differs from the cookie value those paths found no row. - Pull sync validated the identifier but kept only the raw value. It looked the request snapshot up under that value, while generation binds the snapshot to the canonical key and every read EC finalization makes uses that key, so it skipped every partner. Its revalidation read and write-back used the raw value too. - The admin lookup answered 404 for a row that exists. - The /auction, publisher navigation and /_ts/page-bids preloads loaded a miss, and resolve_auction_eids matched the snapshot by the raw identifier, so auctions carried no server-side EIDs. - The navigation preload also replaced the snapshot generation had just bound to the canonical key with that miss, so a newly created identifier got no ts-ec cookie on a navigation with no EID cookies to ingest. Pull sync now carries the canonical key and uses it for the request snapshot lookup, the revalidation read and the write-back. Partners still receive the identifier as issued, and the pull rate limit key still hashes the issued identifier, which leaves rate limiting unchanged for every provider. The admin lookup reads under the key, reports the requested identifier as ec_id, and adds the key it read as kv_key, which the API reference now describes. The three preloads load under EcContext::ec_kv_key and resolve_auction_eids looks the entry up under the key. EcContext::accepts_id lost its only caller and is removed. For the built-in HMAC provider the canonical key is the identifier itself for every identifier read-back accepts, so these paths behave as before for HMAC cookies. Apart from the new kv_key field, the one change an HMAC deployment can see is that an admin lookup given an identifier with an uppercase hash now finds the row stored under the lowercase key instead of answering 404. Each path has a test using CanonicalizingProvider, whose identifier t0ca~MiXeD.CaseId is stored under t0ca~mixed.caseid, and all seven new tests failed before the fix. The shared constants for that identifier now live beside the provider, so the identify, finalization and new tests use one definition. The known-gap note on EcContext::kv_key_for, which cited commit 343ac3e from outside this branch, and the matching note on AcceptedProviders now state which paths key rows through the canonical form. The review thread is IABTechLab#1043 (comment)
Some comments added for the two review threads on provider response effects and canonical keying claimed more than the code does, and one changed comparison had no test that depended on it. The AcceptedProviders doc said pull sync, batch sync and the admin lookup all read and write rows, but the admin lookup only reads. The comment in the test a_rejected_provider_effect_never_reaches_the_finalized_response said a rejected header kept anywhere on the context would reach the browser, when EC finalization applies only the response headers the context holds. The admin lookup docs and the API reference entry for kv_key named the owning provider as the source of the row key, which does not hold for a deployment with no provider selected, where the built-in HMAC identifier format supplies the key. Three other comments now say precisely what they mean. The EcContext::kv_key_for doc names the function each reference points at, the EcContext::candidate_id comment no longer credits an unnamed caller with serving the response, and the dispatch_pull_sync doc names ec_hash as the input to the rate limit key. Before replacing the snapshot that generation bound, the publisher navigation preload compares that snapshot with its fresh read. The comparison is keyed by the canonical key, yet the navigation tests for a provider whose canonical key differs from the cookie value passed with it keyed by the identifier as issued, because their stores returned the row on the first read. The test for a newly created identifier's cookie now also runs against a store whose first point read misses the row generation just wrote. With the comparison keyed by the identifier as issued, the preload replaced the snapshot with that miss, EC finalization skipped the cookie and the test failed. With the canonical key the test passes.
A provider's own response headers, such as an evidence cookie, reached the browser even when generation discarded the candidate they came with. EcContext::candidate_id kept the headers before checking the identifier against the cookie bounds, and generate_with_provider left them on the context when the candidate collided with an existing row or its row could not be written. EC finalization applies whatever headers the context holds, and the publisher and integration proxies log a generation error and still serve the response, so the cookie went out with no identifier stored for it. candidate_id now keeps the headers only once the identifier has passed the bounds check, or when the provider produced no identifier at all, and generate_with_provider drops them with a colliding or unpersisted candidate. The reserved-surface check asked for in the review thread on provider response effects already ran before any header was kept, so the same rule now also holds when the identifier is rejected, when it collides and when its row cannot be written. A test covers each of those three cases, and all three failed before the change with the provider's cookie on the finalized response. HeaderSettingProvider now takes the identifier it returns, so a test can pair a permitted header with an identifier outside the cookie-safe alphabet. The finalization comment on applying provider headers now names candidate_id as the place they are checked, where it named generate_with_provider. The review thread is IABTechLab#1043 (comment)
The API reference said the explicit admin EC lookup route accepts an EC
ID in the bare {64 lowercase hex}.{6 alphanumeric} form. The route
accepts whatever AcceptedProviders::canonical_kv_key accepts, which is
an identifier created by the selected provider, such as the built-in
HMAC provider's hmac~ form, and the bare legacy form that provider
still reads, with both HMAC forms accepted when no provider is
selected. The hash may be given in either case, because
canonical_kv_key lowercases it before the check.
The note on retiring the legacy bare reader said a page view with
ts-eids or sharedId cookies runs ingest_eid_cookies in
ec_finalize_response and so restarts the row's one-year clock. Main's
change threading the EC KV read through the request (IABTechLab#885) moved
finalization to collect_eid_cookie_updates and
upsert_partner_ids_from_snapshot, which writes nothing unless a partner
ID is added or changed. The note now names that function and says only
such a view restarts the clock.
|
@aram356 @jevansnyc could one of you press re-run on the The release build finished before that step, the same job passed on #1084 twenty minutes earlier, and the workflow has succeeded on 23 of its last 25 runs. Because that job failed, Failed run: https://github.com/IABTechLab/trusted-server/actions/runs/34855712510 What this push changed, in short. The branch is merged up to current |
Brings upd/split/1-ec-provider at 1b88e42 into split/2-device-geo. The five commits this branch lacked, on top of the cb28ad3 it already had, answer two review threads on IABTechLab#1043 and correct what reviewing those answers found. - Pull sync, the admin lookup, /auction, the publisher navigation preload and /_ts/page-bids read and write identity-graph rows under the owning provider's canonical key (0e7f7eb and fb1bc28). - The docs on a rejected provider effect say generation returns an error and the page is served without an Edge Cookie, with a test that a rejected header never reaches the finalized response (0980b73). - A provider response's headers are kept only for a candidate that generation commits (cb6f717). - The API reference lists the admin lookup's accepted EC ID forms, and the bare reader note names the function that writes EID updates (1b88e42). The merge had no conflicts. The merged tree passes a native all-targets check, 2,763 core tests, the Axum, Cloudflare and Spin tests and the three wasm checks.
This pull request is the first of six stacked pull requests and the only one with no parent, so it depends on nothing beyond
mainand can merge on its own. The stack is #1043, #1044, #1045, #1046, #1047 and #1094, each targetingmainand each branch building on the one before. The first five decompose #838 as requested in the #986 review, and #1094 opens the integration seam on top of them. A later pull request's own change shows when its head branch is compared with the previous pull request's head branch.The design specs for the series are carried by #1084. The spec for this pull request is
2026-07-30-pluggable-providers-design.md, which covers both this pull request (the Edge Cookie seam) and #1044 (device and geo selection). The spec is revised to match this implementation, with a revision record listing every divergence from the earlier draft and why.What this pull request 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>]. Leaving[ec] providerout means stateless operation 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 a 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. In either form the passphrase names a secret-store key, as[ec] passphrasedoes onmain, and the resolved value is held to the same 32-byte minimum.[A-Za-z0-9._~-], enforced at creation, 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.provider-code-registry.md, the registry the spec set defines. Core creates{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 createshmac~<hash>.<suffix>and still reads its earlier bare form so deployed cookies keep working, andec/provider.rsdocuments when that reader can be retired.AcceptedProviders, which applies the global bounds and then asks the provider that owns the identifier's code, so a code no configured provider owns is refused. Identify, EC finalization, pull sync, batch sync, the admin lookup,/auction, the publisher navigation preload and/_ts/page-bidsall use the canonical key, so a provider whose canonical form differs from its cookie value still reaches the row it created. The admin lookup reports the key it read askv_key.is_valid_ec_idstays the built-in HMAC grammar, accepting thehmac~envelope and the legacy bare form.Set-Cookiefor ats-cookie, anyx-ts-header, and the framing, hop-by-hop andcache-controlheaders. A refused effect makes generation return an error, and the page is served without an Edge Cookie. Headers are kept only from a provider response whose identifier generation commits, or that produced no identifier, and finalization appends them rather than replacing the origin's.How it was verified
Every job
main's CI runs was run locally against this head,1b88e42f7, on Windows and under WSL, and all of them passed. That coverscargo fmt --all --check, Clippy with warnings denied on the Axum, Cloudflare, Spin and Fastly adapters and on both wasm targets, the Axum, Cloudflare and Spin adapter suites, the cross-adapter parity suite, the benchmark smoke run, the release wasm builds for Spin and Fastly, and the CLI, OpenRTB codegen,format-docsand template cache harness jobs that only run on Linux. The core suite passes with 2,747 tests natively and 2,741 under Viceroy, where 6 are ignored.CI on this head is green across all 19 checks, being Run Tests, Run Format, Integration Tests and CodeQL Advanced. The integration run needed a re-run because
prepare integration artifactsfirst failed downloading wasm-opt from GitHub releases, which skipped the three integration jobs behind it. Thank you to whoever pressed it.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.