Add native secret-store config resolution - #1036
Conversation
ChristianPavilonis
changed the title
|
@ChristianPavilonis to test it before merging into #1019 |
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Moves static app-config credentials from plaintext blob values to secret-store key
references resolved after envelope verification, and fixes the logical-to-physical
store mapping that broke the Fastly deployment. The core design is sound: integrity
verification genuinely precedes resolution, resolution is atomic (the blob is left
untouched on failure), the deploy/load validation split keeps value checks on the load
path where PartnerRegistry::from_config still fails closed, and the two end-to-end
payload tests cover both the all-credentials-resolve and inactive-feature-skip arms.
Four blocking items: resolution discards the one diagnostic that would explain a
mis-mapped store, the documented migration order opens a total outage window, the
Fastly Hooks::routes() path reads the store mapping from the wrong source, and the
EdgeZero dependency is pinned to an unmerged upstream commit.
3 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, needs a new import, or adds code outside the diff. No
suggestion in this review was scratch-verified — local runs were skipped for this
pass, so please re-run the matching checks after applying.
Blocking
🔧 wrench
- Secret-store resolution throws away every adapter's diagnostic — see inline at
crates/trusted-server-core/src/secret_resolution.rs:164 - Documented migration order opens a full outage window — see Cross-cutting below
Hooks::routes()reads the wrong source for the store mapping — see inline atcrates/trusted-server-adapter-fastly/src/app.rs:1261
❓ question
- EdgeZero pinned to an unmerged upstream PR — see Cross-cutting below
Non-blocking
♻️ refactor / 🤔 thinking / ⛏ nitpick / 🌱 seedling
- Required S3 secret references still have serde defaults — see inline at
crates/trusted-server-core/src/settings.rs:767 - Feature-enablement logic duplicated in three places — see inline at
crates/trusted-server-core/src/config_payload.rs:63 EchoSecretStoremakes resolution untestable — see inline atcrates/trusted-server-core/src/config_payload.rs:145expect()on the Tinybird token traps the Wasm guest — see inline atcrates/trusted-server-adapter-fastly/src/tinybird.rs:57- Deploy validation misses duplicate partner key names — see inline at
crates/trusted-server-core/src/ec/registry.rs:74 - New docs bullets lost their markdown hard breaks — see inline at
docs/guide/configuration.md:1620 partners = []is redundant and a footgun — see inline attrusted-server.example.toml:17- Two overlapping ways to express leaf optionality — see inline at
crates/trusted-server-core/src/secret_resolution.rs:64 - Spin's five declared secret variables read as a contract — see inline at
crates/trusted-server-adapter-spin/spin.toml:28
Cross-cutting / body-level findings
-
🔧 Documented migration order opens a full outage window —
docs/guide/configuration.md:60-72gives the order: populate store, replace values with key names,ts config validate+ts config push, then "restart/redeploy instances as needed."Step 3 lands the reference-bearing blob while the old binary is still serving. On Fastly each request reads the config store fresh, so from that instant every request runs
Ec::validate_passphrase— which requires at least 32 bytes onmaintoday (MIN_PASSPHRASE_LENGTH = 32,crates/trusted-server-core/src/settings.rs) — againstpassphrase = "ec_passphrase"(13 bytes). That yieldsshort_passphrase, config load fails, and the service returns its startup-error response for all traffic until the redeploy finishes.The reverse mismatch fails too: a new binary reading a plaintext blob resolves each plaintext secret as a key name. There is no safe intermediate state — the binary and the blob have to flip together, and the doc currently puts the break in the middle. Please correct the ordering and add an explicit warning that a mismatched binary/blob pair fails config load outright. The staged Fastly deployment cited in the PR description would not surface this, since no old binary is in play there.
-
❓ EdgeZero pinned to an unmerged upstream PR —
Cargo.toml:57-62moves all six edgezero crates from stable tagv0.0.4to git rev0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34, a commit on the still-openstackpop/edgezero#344. Merging this putsmainon a branch commit of an unmerged PR: if #344 is rebased or force-pushed before it merges, that commit can become unreachable andmainstops building.You disclosed this in the PR description, and the issue comment suggests this lands in #1019 first, so this may already be handled. It still needs an explicit answer because it constrains
main: hold this PR until #344 merges and re-pin to a tag, or is a rev pin onmainacceptable here? -
📝 CI coverage gap on the reviewed head — only
Analyze (javascript-typescript)ran on1315cdb1. The full gate suite (cargo fmt/test/clippy, all four adapters, cross-adapter parity, vitest, format-docs, integration and browser tests) last ran green on the merge commit598f7100, three commits earlier. That leaves070397f1 Resolve static credentials through typed config,b1e967e3, and1315cdb1without Rust, adapter, or lint coverage. Worth re-triggering the suite on the current head before merge, independent of the findings above. -
👍
validation_error_summaryis a real leak fix —crates/trusted-server-core/src/settings.rs:2387-2424walksValidationErrorsemitting onlypath: code, nevervalidator'sparams, which hold the offending value. The previous code formattedValidationErrorswholesale into a config error message. -
👍 Deleting
S3_CREDENTIALS_CACHEremoves a genuinely bad structure —crates/trusted-server-core/src/proxy.rspreviously kept a process-globalHashMapkeyed on the plaintext secret access key, with unbounded growth and a poisoning-proneMutex. Startup-resolved values are strictly better. -
👍
IntegrationSettings's customDebugcloses the DataDome-key leak that the flattenedJsonValuemap would otherwise print. -
👍 The two payload resolution tests are the right pair —
resolves_all_static_credentials_from_the_mapped_default_storeproves every path arm resolves through a mapped physical store, andinactive_optional_features_do_not_resolve_stale_secret_referencesproves disabled features do not demand stale references. Also good: droppinginclude_str!("trusted-server.example.toml")from the Spin and Cloudflare startup paths in favour of a hard error.
CI Status
- Analyze (javascript-typescript): PASS
- cargo fmt: not run on this head (PASS on
598f7100) - cargo test: not run on this head (PASS on
598f7100) - cargo test (axum native): not run on this head (PASS on
598f7100) - cargo test (cross-adapter parity): not run on this head (PASS on
598f7100) - cargo test (ts CLI, native): not run on this head (PASS on
598f7100) - cargo check (cloudflare native + wasm32-unknown-unknown): not run on this head (PASS on
598f7100) - cargo check/build/test (spin native + wasm32-wasip1): not run on this head (PASS on
598f7100) - integration tests: not run on this head (PASS on
598f7100) - integration tests (Fastly EC lifecycle): not run on this head (PASS on
598f7100) - browser integration tests: not run on this head (PASS on
598f7100) - prepare integration artifacts: not run on this head (PASS on
598f7100) - vitest: not run on this head (PASS on
598f7100) - format-typescript: not run on this head (PASS on
598f7100) - format-docs: not run on this head (PASS on
598f7100) - Analyze (rust): not run on this head (PASS on
598f7100) - Analyze (actions): not run on this head (PASS on
598f7100) - CodeQL: not run on this head (PASS on
598f7100)
No check reported a fail or cancel bucket. Branch protection reported no required checks for this PR.
1315cdb to
3e2b3d2
Compare
|
Review follow-up for
Re-requesting review from @prk-Jr. |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Well-executed change: the resolution model (verify envelope, strip inactive references, resolve, validate runtime settings) is fail-closed, the push-time/runtime validation split is coherent across all four adapters, and the test coverage in config_payload.rs and secret_resolution.rs is thorough. Two blocking findings: a secret-exposure path in the resolution-failure error message, and the failed CodeQL check.
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 touches multiple locations and can't be auto-applied.
Blocking
🔧 wrench
- Resolution-failure error can log a plaintext secret from a legacy blob — see inline at
crates/trusted-server-core/src/secret_resolution.rs:169 - CodeQL check failed: 15 high
rust/cleartext-loggingalerts — see Cross-cutting below
Non-blocking
♻️ refactor / 🤔 thinking / ⛏ nitpick / 📝 note / 🌱 seedling
.env.exampleships the Fastly store mapping active, breaking the documented Axum flow — see inline at.env.example:11(suggestion)- Migration guide doesn't warn that previously legal short secrets now fail startup — see inline at
docs/guide/configuration.md:70(suggestion) - Missing required leaf reports "must be a string" instead of "missing" — see inline at
crates/trusted-server-core/src/secret_resolution.rs:151(suggestion) server_side_key_secret_nameholds the resolved key value at runtime — see inline atcrates/trusted-server-core/src/integrations/datadome.rs:184(suggestion)validate_config_for_deployusesHashMap<_, ()>as a set — see inline atcrates/trusted-server-core/src/ec/registry.rs:77Hooks::stores()duplicatesedgezero.toml— see inline atcrates/trusted-server-adapter-fastly/src/app.rs:1332Cargo.lockrewrote prost'sitertoolsedges — see Cross-cutting below
Cross-cutting / body-level findings
- 🔧 CodeQL check failed: 15 high
rust/cleartext-loggingalerts. Not required under branch protection, but a CI gate this repo treats as blocking. I inspected all 15: they are taint over-approximation — CodeQL now treats everything flowing out ofresolve_secret_references/validate_tinybird_secret/validate_admin_handler_passwordsas secret-tainted and flags logs of plainly non-secret fields (asset-route prefixes insettings.rs, DataDome registration flags indatadome.rs:979, consent clamping inconsent_config.rs, header names inresponse_privacy.rs, etc.). No alert is a real value leak — the nearest real vector is the inline finding atsecret_resolution.rs:169. The alerts still need triage: dismiss each in the code-scanning UI with a justification (or add a CodeQL model/sanitizer exclusion), otherwise this check stays red here and re-fires on every future PR touching these paths. - ⛏
Cargo.lockmoved prost'sitertoolsdependency edges from 0.13.0 to 0.10.5. The edgezero pin update also rewroteprost-build/prost-derive'sitertoolsedges down to the already-present 0.10.5 while 0.13.0 stays in the graph for other consumers — unintended churn from edge unification during the scoped update. Consider hand-restoring the 0.13.0 edges so the lock diff stays scoped to the edgezero bump.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: FAIL
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- format-docs: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- format-typescript: PASS (required)
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (actions): PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Second pass, reviewing head 76f6f13. The feedback commit addresses every finding from the previous review: the resolution-failure error now drops both the key name and the underlying platform error (with a regression test asserting a legacy plaintext value never reaches diagnostics), the new 32-byte minimums were removed in favor of pre-PR behavior (bypass-credential strength enforcement moved back to request time, with a request-level test), .env.example no longer ships the Fastly mapping active, and the stores() metadata is now pinned to edgezero.toml by a manifest-parsing test. What remains blocking is the open CodeQL alert set; one stale doc claim and the lockfile nit round out the list.
1 of the inline comments below carries a one-click GitHub
suggestion— use Commit suggestion to apply it as a commit on the PR branch.
Blocking
🔧 wrench
- CodeQL: 15 high
rust/cleartext-loggingalerts still open — see Cross-cutting below
Non-blocking
⛏ nitpick
- Stale "must be at least 32 bytes" claim for
proxy_secret— see inline atdocs/guide/configuration.md:361(suggestion) Cargo.lockprostitertoolsedges still rewritten — see Cross-cutting below
Cross-cutting / body-level findings
- 🔧 CodeQL: 15 high
rust/cleartext-loggingalerts still open. Carried over from the previous review round. The code fix in76f6f13does not clear them — all 15 are taint over-approximation (CodeQL treats everything flowing out ofresolve_secret_references/validate_tinybird_secret/validate_admin_handler_passwordsas secret-tainted and flags logs of plainly non-secret fields), and all 15 remain open on this PR, so the CodeQL check will fail again once analysis reruns on this head. They need triage: dismiss each in the code-scanning UI with a justification, or add a CodeQL suppression/model exclusion — otherwise this check stays red here and re-fires on every future PR touching these paths. - ⛏
Cargo.lockstill carries the rewritten prostitertoolsedges (0.13.0 → 0.10.5 while 0.13.0 stays in the graph for other consumers) — unaddressed nit from the previous review; the earlier inline thread on this stays open, so no new inline comment here. Hand-restoring the 0.13.0 edges keeps the lock diff scoped to the edgezero bump.
CI Status
GitHub checks have not yet run for head 76f6f13 — only one check has reported; everything else is pending/not started. Local verification was run in the reviewer worktree at this head instead: cargo fmt --all -- --check, cargo clippy-fastly, targeted cargo test-fastly for the modules this head touches (10 secret_resolution + 15 config_payload + 67 datadome + 43 registry + the fastly manifest-metadata test), and prettier for the changed docs — all pass.
- Analyze (javascript-typescript): PASS
- CodeQL: not run on this head (15 alerts from the prior analysis remain open)
- browser integration tests: not run
- integration tests (Fastly EC lifecycle): not run
- integration tests: not run
- cargo test (ts CLI, native): not run
- cargo test (cross-adapter parity): not run
- cargo check/build/test (spin native + wasm32-wasip1): not run
- cargo check (cloudflare native + wasm32-unknown-unknown): not run
- format-docs: not run (required; passes locally)
- cargo test: not run (required; touched modules pass locally)
- cargo test (axum native): not run
- format-typescript: not run (required)
- Analyze (rust): not run
- cargo fmt: not run (required; passes locally)
- prepare integration artifacts: not run
- vitest: not run
- Analyze (actions): not run
# Conflicts: # .env.example # Cargo.lock # crates/trusted-server-adapter-axum/src/app.rs # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-core/src/config.rs # crates/trusted-server-core/src/config_payload.rs # crates/trusted-server-core/src/ec/registry.rs # crates/trusted-server-core/src/integrations/datadome.rs # crates/trusted-server-core/src/integrations/datadome/protection.rs # crates/trusted-server-core/src/proxy.rs # crates/trusted-server-core/src/secret_resolution.rs # crates/trusted-server-core/src/settings.rs # docs/guide/configuration.md # docs/guide/ec-setup-guide.md # docs/guide/getting-started.md # docs/guide/integrations/datadome.md # docs/guide/proxy-signing.md # scripts/template-cache-local-test.sh # trusted-server.example.toml
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds secret-store reference resolution to app config: the blob carries key names, and each adapter resolves them into redacted runtime values after integrity verification. The core mechanism is well built and genuinely fail-closed — one central resolution point, envelope.verify() strictly before resolution on all four adapters, clone-then-swap so a partial resolution never reaches Settings, empty resolved values rejected centrally, no secret in any error message, and zero per-request app-config secret reads left anywhere. The api_token: Option auth change is fail-closed and verified from both directions. The Fastly logical→physical bug is genuinely fixed on the startup path and all three reload paths.
The blockers are concentrated in the operator-facing surface rather than the resolution logic. Three of them produce an outage or a dead local environment for anyone following the instructions as written.
10 of the inline comments below carry a one-click
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them. Every suggestion was applied in a scratch worktree at this head and verified:cargo fmt --all -- --checkclean,clippy-fastly/clippy-axum/clippy-cloudflare/clippy-spin-nativeall clean at-D warnings, 2,477 tests passing acrosstest-fastly(including 2,284 core tests),test-axum,test-cloudflareandtest-spin, pinned prettier clean on both changed docs, and both changed TOML files re-parsed. The remaining comments describe the fix in prose because the change spans multiple files, needs a validator split first, or targets lines outside a RIGHT-side diff hunk.
Blocking
🔧 wrench
KeyInNamedStorefields would silently resolve from the default store — see inline atcrates/trusted-server-core/src/secret_resolution.rs:29trusted_client_ip.shared_secretis the one credential left plaintext in the blob — see inline atcrates/trusted-server-core/src/config.rs:135- Local
fastly compute servecannot start — no app-config secrets seeded — see inline atfastly.toml:62 - Migration runbook never creates or links the physical store — see inline at
docs/guide/configuration.md:70 - Deploy path repeats the same omission — see inline at
docs/guide/getting-started.md:160 - Axum quick-start cannot complete — starter config ships reserved placeholder domains — see inline at
.env.dev:5anddocs/guide/getting-started.md:73 - Stale guardrail claim: deploy validation no longer rejects a placeholder handler password — see inline at
trusted-server.example.toml:41 - CodeQL gate is red — see Cross-cutting below
- PR is
CONFLICTING; two conflicts are competing designs — see Cross-cutting below
❓ question
- Spin hardcodes the config-store name, contradicting the docs this PR adds — see inline at
crates/trusted-server-adapter-spin/src/app.rs:61 - Unrelated
Cargo.lockchurn: prost's itertools 0.13.0 → 0.10.5 — see inline atCargo.lock:3679 - PR description is inaccurate in two places — see Cross-cutting below
Non-blocking
🤔 thinking / ♻️ refactor / 🏕 camp site / ⛏ nitpick
- Rollback window: an old binary uses the documented key name as a live HMAC key — see inline at
crates/trusted-server-core/src/config.rs:136 - A suppressed telemetry error became a per-request 500 — see inline at
crates/trusted-server-adapter-fastly/src/tinybird.rs:60 validate_config_for_startup/_for_deployare byte-identical, soresolved_secretsis a no-op — see inline atcrates/trusted-server-core/src/config.rs:284pull_sync_enabledread withas_bool()but deserialized withfrom_value_or_str— see inline atcrates/trusted-server-core/src/config_payload.rs:84resolve_leafdiscards thePlatformErrorcause, losing the primary triage signal — see inline atcrates/trusted-server-core/src/secret_resolution.rs:167- No test that a required reference failing lookup fails the load — see inline at
crates/trusted-server-core/src/config_payload.rs:455 ts_pull_token's requirement has zero coverage on either side — see inline atcrates/trusted-server-core/src/ec/registry.rs:403require_nonempty_tokenis a flag that earns nothing — see inline atcrates/trusted-server-core/src/ec/registry.rs:348- Cloudflare re-resolves every secret per request, against request #1's
Env— see inline atcrates/trusted-server-adapter-cloudflare/src/app.rs:51 - Cloudflare is the only adapter with no store mapping and no comment saying why — see inline at
crates/trusted-server-adapter-cloudflare/src/app.rs:130 spin.toml's request-signing config variables are now silently inert — see inline atcrates/trusted-server-adapter-spin/src/platform.rs:127EDGEZERO__STORES__SECRETS__…__NAMEsilently redirects the Axum lookups — see inline atdocs/guide/getting-started.md:104- Only worked example puts the passphrase in
argv— see inline atdocs/guide/fastly.md:326 - Five secret key names live in five files with nothing keeping them in sync — see inline at
crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml:4 validation_error_summarydrops the validatormessage— see inline atcrates/trusted-server-core/src/settings.rs:3403- Dead deprecation branches in
try_new— see inline atcrates/trusted-server-core/src/integrations/datadome.rs:390 S3Credentialsrebuilt with three allocations per signing call — see inline atcrates/trusted-server-core/src/proxy.rs:878- Axum reaches for a fully-qualified path instead of the import above it — see inline at
crates/trusted-server-adapter-axum/src/app.rs:68 pub mod secret_resolutionshould bepub(crate)— see inline atcrates/trusted-server-core/src/lib.rs:67tinybird.remove("access_token_secret")is dead — see inline atcrates/trusted-server-core/src/config_payload.rs:73- Duplicate
uselines — see inline atcrates/trusted-server-core/src/settings_data.rs:6 - Two assertions without messages — see inline at
crates/trusted-server-adapter-fastly/src/app.rs:1432 EcPartner::api_tokendoc still says "Plaintext API token" — see inline atcrates/trusted-server-core/src/settings.rs:377.env.exampleordering reads backwards — see inline at.env.example:9- Real deployed Fastly hostname retained — see inline at
docs/guide/ec-setup-guide.md:31
📝 note
- Severity change: DataDome went from request-time fail-open to boot-time fail-closed — see inline at
crates/trusted-server-core/src/integrations/datadome/protection.rs:86 - Trimming the resolved bypass credential invalidates whitespace-carrying values — see inline at
crates/trusted-server-core/src/integrations/datadome.rs:408 - Every Fastly request now opens a config store and makes ~10 dictionary reads — see inline at
crates/trusted-server-adapter-fastly/src/main.rs:93 stores().kvis declared but nothing on the Fastly path consumes it — see inline atcrates/trusted-server-adapter-fastly/src/app.rs:1338jqdependency not in Prerequisites — see inline atdocs/guide/getting-started.md:80configuration.mdS3 table vs example key mismatch — see inline atdocs/guide/configuration.md:1099
👍 praise
- Hand-written
IntegrationSettingsDebug closes a real leak — see inline atcrates/trusted-server-core/src/settings.rs:220 hooks_store_metadata_matches_edgezero_manifestpins exactly the right invariant — see inline atcrates/trusted-server-adapter-fastly/src/app.rs:1394- Order of operations is right and centrally enforced — see inline at
crates/trusted-server-core/src/config_payload.rs:38
Cross-cutting / body-level findings
-
🔧 CodeQL gate is red — 15 open
rust/cleartext-logginghigh alerts. Not in branch protection's required set, so not merge-blocking mechanically. All 15 read as false positives:validate_tinybird_secret(settings.rs:1971) formats only"{setting} must be non-empty after secret resolution"— the setting name, no value; the flagged sinks logroute.prefix, a cache ruleid, apath, and DataDome'ssdk_origin/rewrite_sdk/enable_protection; the named sourcetry_new_with_secret_validationdoes not exist anywhere in the tree; and 9 of the 15 sinks are in files this PR does not touch (consent_config.rs,storage/kv_store.rs,response_privacy.rs,auction/orchestrator.rs,management_api.rs,axum/src/platform.rs). Root cause is field-insensitive taint:Settingsnow carries resolved plaintext secrets, so every log of anySettings-derived field lands in a taint path. The architectural signal is real and new even though each alert is not — before this PR, "log aSettingsfield" could not leak a credential. Resolve by dismissing the 15 with a written justification, so the next true positive in this rule is visible again; or better, give resolved secrets a wrapper whoseDebug/Display/Serializecannot emit the value. NoteRedactedis serde-transparent, so it does not close the serialize half. -
🔧 PR is
CONFLICTING; two of the six conflicts are competing designs.git merge-tree origin/mainconflicts inCargo.lock,Cargo.toml,crates/trusted-server-adapter-fastly/src/app.rs,crates/trusted-server-adapter-fastly/src/main.rs,crates/trusted-server-core/src/config.rsanddocs/guide/configuration.md. Inconfig.rs,mainalready carries asecret_fields()stub returningVec::new()whose comment defers secret-store references "plus operator migration work tracked separately" — worth confirming that deferred work is what this PR delivers. In the Fastly adapter,mainthreads&EnvConfigwithconfig_store_name(env)/config_key(env), while this PR replaces that seam withRuntimeStoreConfig/DEFAULT_CONFIG_STORE_ID/env_config_from_runtime_dictionary. Separately, this PR rewrites "EdgeZero's env overlay" → "The pinned EdgeZero loader" in three places inconfiguration.md; that phrasing only holds while pinned to a rev and goes stale as soon as the pin returns to a tag. -
❓ The PR description is inaccurate in two places. (1) It says
generate-viceroy-config.rs"Generate[s] local secret-store data for references found in integration configuration." It does not —build_app_config_envelope(lines 113-134) only swapsSettings::from_toml+validate_settings_for_deployfortoml::from_str::<TrustedServerAppConfig>+TrustedServerAppConfig::new, andgenerated_config_store_blocks(148-156) still emits only the config-store block. Every secret value is hand-maintained. (2) It describes a "signed configuration blob";envelope.verify()is a self-computed canonical SHA-256 (edgezero-core/src/blob_envelope.rs:87-99), not a signature. The ordering is right and it is the correct primitive for the chunked Fastly path, but it defends against truncation and corruption — not against someone with config-store write access.config_payload.rs:25-26already says "integrity verification", which is the accurate wording. -
📝 The EdgeZero pin is a commit that exists on no upstream branch or tag.
Cargo.toml:57-62pins all six edgezero crates to rev0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34. Verified againststackpop/edgezero: it is not an ancestor oforigin/main,git branch -a --containsandgit tag --containsare both empty, and it is reachable only as a barerefs/commit/<sha>. It is 140 commits behindv0.0.7(5c9886e5), whichmainnow uses — so merging as-is downgrades EdgeZero acrossadapter-{axum,cloudflare,fastly,spin},cliandcore. Upstreamstackpop/edgezero#344is still open withmergeCommit: null, and its current head is055f7e94, from which the pinned rev has also diverged (behind 140, ahead 12) — the branch was rebased since. If #344 lands squashed or rebased, this SHA becomes unreferenced and can be garbage-collected, at which pointcargo fetchfails on every cold cache including CI. All seven edgezero packages inCargo.lockdo move consistently, so nothing is left behind on the old tag. Flagged as informational — the merge-ordering call belongs to the author and the release owner. -
🤔 Viceroy "Option A" in
getting-started.md:46-62gained a required setup step documented nowhere. That section is untouched by this PR and still readsfastly compute servewith no config or secret preparation, but local serve now requires hand-seeded secrets (see thefastly.tomlcomment).grep -rn "local_server.secret_stores" docs/returns onlykey-rotation.mdandrequest-signing.md, both aboutsigning_keys. Option B got a full rewrite; Option A got nothing. The recipe already exists atscripts/template-cache-local-test.sh:227-243. Body-level because line 46 is outside every RIGHT-side hunk. -
♻️
resolve_secret_referencesdeep-clones the whole config for an atomicity guarantee no caller uses (secret_resolution.rs:29and:43). The only production caller isconfig_payload.rs:53, which passes a localdatathat is dropped on the error path anyway; the clone's sole beneficiary isdoes_not_mutate_data_when_resolution_fails(:396-407). It costs a full deep copy of the config JSON per instance boot inside Wasm. Resolving in place is equivalent for every real caller. Body-level because it targets the same lines as theKeyInNamedStoresuggestion. -
⛏ Stale
S3Credentialsdoc claims a runtime store read and a deleted cache (crates/trusted-server-core/src/s3_sigv4.rs:34-36). Both claims are now false:apply_asset_origin_authbuilds these from already-resolved config, andS3_CREDENTIALS_CACHEwas removed by this PR.s3_sigv4.rsis not in the diff, so this cannot be an inline comment. Proposed replacement:/// Values are already resolved from the app-config secret store when settings are /// built, so the caller passes them straight through without a runtime store read. /// Temporary credentials can include a session token, which becomes the signed /// `x-amz-security-token` header.
-
⛏ Struct doc at
settings.rs:355-356is now false. "the plaintext is never stored at runtime" holds forPartnerConfig(hash only) but not forEcPartner.api_token, which holds the resolved plaintext inSettingsfor the process lifetime. Worth narrowing to "the registry stores only the hash". Outside a hunk, so body-level. -
📌 Pre-existing real-world values in
fastly.toml, all outside every diff hunk.fastly.toml:4authors = ["jason@stackpop.com"],:10service_id = "dysUw6h73VzeomD61eal85", and:50data = "NVnTYrw5xoyTJDOwoUWoPJO3A6UCCXOJJUzgGTxxx7k="— a base64 32-byte Ed25519-shaped value insigning_keyswhose embeddedxxxsuggests deliberate mangling.viceroy-template.toml:55-59carries an explicit "generated for testing, never used in production" attestation for the same value;fastly.tomlhas none. Not asked for in this PR, but a secrets-hardening PR is the natural place to file it. -
👍 Verified clean and worth stating explicitly, so it is clear these were checked rather than skipped: verification-before-resolution ordering on all four adapters; a swallowed-failure scan of every added line (
ok(),unwrap_or_default(),unwrap_or(false),let _ =— the only hits are two intentionalOnceCell::setcalls); per-request versus startup secret reads (the whole per-request class is closed, and the only remainingsecret_store()callers read the separaterequest_signing.secret_store_id); theapi_token: Optionauth change traced from both directions; the deploy/runtime validation split, where runtime is a strict superset for value checks and a net tightening versusmain; the legacy-selector compatibility bridge (all three warn, none survives a round trip, andinit_cli_loggersetsLevelFilter::Infoso the warnings are actually visible); recursive resolution edge cases (empty arrays,Optioncontainers,nullversus absent, the internally-taggedAssetOriginAuth, non-string leaves, and no overlappingSecretFieldpaths);spin.toml's secret-variable encoding byte-for-byte againstspin_secret_variable_name; the Wrangler CI binding names and their fictional values; naming drift across code and all five manifests; WASM gating on every new item; secret exposure via every runtimeSettingsserialization path; andscripts/template-cache-local-test.shend to end including its CI greps. No real secrets or real-world values are introduced anywhere by this PR, and this PR structurally reduces the tracked-fastly.tomlsecret-leak risk.
Recommendation
Hold. The resolution mechanism is sound and the auth change is correct — the work needed is on the operator-facing surface: the three docs/local-dev blockers, the KeyInNamedStore guard, and either closing the trusted_client_ip.shared_secret gap or stating the deferral explicitly. The merge will also need a decision on the RuntimeStoreConfig-versus-&EnvConfig seam against main.
CI Status
browser integration tests: PASSintegration tests: PASSintegration tests (Fastly EC lifecycle): PASSprepare integration artifacts: PASSCodeQL: FAILAnalyze (rust): PASSAnalyze (actions): PASSAnalyze (javascript-typescript): PASScargo test: PASS (required)cargo test (axum native): PASScargo test (ts CLI, native): PASScargo test (cross-adapter parity): PASScargo check (cloudflare native + wasm32-unknown-unknown): PASScargo check/build/test (spin native + wasm32-wasip1): PASScargo fmt: PASS (required)format-typescript: PASS (required)format-docs: PASS (required)vitest: PASS
# Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Third pass, reviewing head 4258a6b. The branch now merges main through #1077 and adds two substantive changes, both reviewed in full: the pass-2 docs suggestion applied verbatim, and a tip commit that integrates the newly-merged [trusted_client_ip] feature into the secret-reference model and bumps EdgeZero to v0.0.8. The new work held up under scrutiny — no new findings. What remains blocking is the open CodeQL alert set, carried over a second time.
Verified on the tip commit specifically:
trusted_client_ip.shared_secretis wired end-to-end: required leaf under an optional section, push-time reference check, and the ≥32-graphic-ASCII validator split onto the field leaf so it prunes at push time (proven by the new test using a 31-char key name) while still enforcing on the resolved value at runtime. A present section with a missing or unresolvable key fails closed with the path-only error message.- The
json_bool_or_string_is_truefix forpull_sync_enabledmatchesfrom_value_or_str'sbool::from_strexactly, so there is no accepted spelling the strip logic misses;tinybird.enabledand the DataDome flags are strict bools that fail deserialization loudly, so they need no equivalent. - The DataDome bypass credential is no longer normalized (restoring legacy request-time semantics, with a preservation test); the server-side key keeps its legacy trim — per-field legacy behavior, deliberate.
- Merge resolutions for #1048/#1070/#1077 are correct; notably, client-IP resolution runs before header sanitization, and when app state fails to build the resolver receives no config and falls back to the untrusted peer address.
Blocking
🔧 wrench
- CodeQL: 14 high
rust/cleartext-loggingalerts still open — see Cross-cutting below
Non-blocking
⛏ nitpick
Cargo.lockprostitertoolsedges still rewritten — see Cross-cutting below
Cross-cutting / body-level findings
- 🔧 CodeQL: 14 high
rust/cleartext-loggingalerts still open. Carried over from the two previous review rounds. One of the original 15 is now marked fixed; the remaining 14 are open and none were dismissed, so the CodeQL check will fail again once analysis reruns on this head. As established in the first round, all of them are taint over-approximation (Settings-derived values flowing through the resolution/validation functions into logs of plainly non-secret fields), not real value leaks — but they still need triage: dismiss each in the code-scanning UI with a justification, or add a CodeQL suppression/model exclusion, otherwise the check stays red here and re-fires on every future PR touching these paths. - ⛏
Cargo.lockstill carries the rewritten prostitertoolsedges (0.13.0 → 0.10.5 while 0.13.0 stays in the graph for other consumers) — unaddressed nit; the inline thread from the first review round stays open, so no new inline comment here.
CI Status
GitHub checks have not yet started for head 4258a6b (the latest complete runs — Run Format, Run Tests, Integration Tests, CodeQL Advanced, all green at the workflow level — are on the preceding merge commit 71df1aa). Local verification was run in the reviewer worktree at this head instead: cargo fmt --all -- --check, cargo clippy-fastly, cargo check-axum / cargo check-cloudflare / cargo check-spin under the EdgeZero v0.0.8 bump, targeted cargo test-fastly for the modules the tip commit touches (22 trusted_client_ip + 10 secret_resolution + 19 config_payload + 68 datadome + the fastly manifest-metadata test), cargo test-spin for the variable-encoder tests, and prettier for the changed docs — all pass.
- Run Format: not started on this head (passes locally)
- Run Tests: not started on this head (touched modules pass locally)
- Integration Tests: not started on this head
- CodeQL: not started on this head (14 alerts from the prior analysis remain open)
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
This lands the #846 secret-store migration cleanly: app config carries stable secret-store key names, resolution happens once after envelope verification, and runtime code no longer reads static credentials per request. The metadata contract, the deploy/runtime validation split, and the fail-closed test coverage are all well built.
One blocking problem: the tip commit (4258a6bf) bumped the EdgeZero pin from rev 0d6ebf9b to tag v0.0.8, and v0.0.8 changed how runtime_env_config reads the edgezero_runtime_env store. Rev 0d6ebf9b read unscoped EDGEZERO__* keys; v0.0.8 reads only service-scoped EDGEZERO__SERVICES__<service_id>__* keys, and its own doc comment states "Legacy unscoped entries are not read." The manifests and one doc snippet were left on the unscoped form, so the logical-to-physical secret store mapping is inert and every Fastly request returns 500.
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 lines outside the diff.
How the blocking finding was confirmed
Built the debug wasm at this head and ran it under Viceroy against the harness's generated config:
| Config | Result |
|---|---|
this head as-is (ts_secrets store + unscoped mapping) |
HTTP 500 — failed to resolve secret reference at 'publisher.proxy_secret' from secret store 'trusted_server_secrets' |
local secret store renamed to trusted_server_secrets, mapping removed |
HTTP 200 |
EDGEZERO__SERVICES__dysUw6h73VzeomD61eal85__… (real service id) |
HTTP 500 |
EDGEZERO__SERVICES____… (empty id) |
HTTP 500 |
EDGEZERO__SERVICES__0000000000000000000000__… (Viceroy's service_id()) |
HTTP 200 |
Note the store name in the error is the logical trusted_server_secrets, not the mapped ts_secrets — the mapping never reached EnvConfig. No "edgezero_runtime_env not found" warning appeared, so the store opened fine; only the key form was wrong.
Effect on the CI gates: scripts/template-cache-local-test.sh esi fails 0 passed / 13 failed at this head (every request 500). With the one-line key change it is 21 passed / 0 failed, and inline mode is 8 passed / 0 failed.
Blocking
🔧 wrench
edgezero_runtime_envmapping is inert under the v0.0.8 pin; every Fastly request 500s — see inline atfastly.toml:81- Same inert key in the Fastly integration fixture — see inline at
crates/trusted-server-integration-tests/fixtures/configs/viceroy-template.toml:89 fastly.mddocuments the persisted mapping key in the form the runtime ignores — see inline atdocs/guide/fastly.md:281
❓ question
- No migration path for configs that currently hold real secret values — see the cross-cutting section below
Non-blocking
♻️ refactor / 🤔 thinking / ⛏ nitpick
- Dead docs anchor
#secret-store-migration— see inline atdocs/guide/ec-setup-guide.md:41 validate_config_for_deployis identical tovalidate_config_for_startup; theresolved_secretsflag is dead — see inline atcrates/trusted-server-core/src/integrations/datadome.rs:480- First request's
worker::Envis pinned for the isolate's lifetime — see inline atcrates/trusted-server-adapter-cloudflare/src/app.rs:62 access_token_secretis dropped silently while every sibling deprecated field warns — see inline atcrates/trusted-server-core/src/settings.rs:1901- Resolved DataDome server-side key is trimmed, but the resolved bypass credential deliberately is not — see inline at
crates/trusted-server-core/src/integrations/datadome.rs:395 - Harness now appends secret entries the tip commit made redundant, with different values — see inline at
scripts/template-cache-local-test.sh:226
👍 praise
- Disabled features never need their secret provisioned — see inline at
crates/trusted-server-core/src/config_payload.rs:63
Cross-cutting / body-level findings
-
❓ No migration path for configs that currently hold real secret values.
validate_settings_for_deployno longer callsreject_placeholder_secrets, and secret-reference validation is only "non-empty after trim". An operator who pushes their existingtrusted-server.toml— the one whosepublisher.proxy_secret,ec.passphrase, andhandlers[*].passwordhold real credential values — gets those values written into the config-store blob, which is exactly the exposure this change sets out to remove, and then a hard startup failure on the next deploy because no secret-store key by that name exists. The docs describe the greenfield ordering well but I could not find a "migrating an existing config" path. Is there an intended step this PR should carry: ats config pushwarning when a referenced key is absent from the target secret store, a docs section, or a release note? -
🤔 Per-request startup cost on Cloudflare and Spin.
TrustedServerApp::routes()callsbuild_state()→load_startup_settings()on every request in both adapters, so each request now performs one secret-store read per declared secret field on top of the envelope parse and SHA-256 verify. Spin additionally opens and reads the KV config store per request, where it previously parsed a compile-timeinclude_str!oftrusted-server.example.toml. Fastly is inherently per-instance so it is unaffected. Worth confirming this is acceptable, or caching the built state per isolate. -
📌
docs/guide/cli.md:69still says "EdgeZero v0.0.4".docs/guide/configuration.md:191was de-versioned in this PR ("The pinned EdgeZero loader…") for exactly this reason;cli.mdkeeps the literal version whileCargo.tomlnow pinsv0.0.8. Not a changed file, so it cannot carry an inline comment. -
📌
trusted-server.example.tomlstill ships deprecated selectors. Line 193 has# secret_store = "s3-auth"and line 511 has# credential_secret_store = "ts_secrets"; both fields are now deserialize-only, warn-and-ignore. The bypass block's comment also still says the credential "is loaded from the Secret Store at runtime (>= 32 bytes of high-entropy material)", which is now resolved at startup instead. Both lines fall outside this PR's diff hunks. -
👍 The validation split is carefully built. Moving every secret check to a field-level
#[validate(custom(...))]— including splittingvalidate_trusted_client_ip's struct-level schema validator so only the header checks remain there — is what letsvalidate_excluding_secretsstrip exactly the secret leaves without dropping unrelated structural errors. TheIntegrationSettingscustomDebugthat prints only integration ids closes a real leak, since resolved DataDome credentials now live in that raw JSON map. And the error paths never carry secret values, with tests asserting the negative.
CI Status
Only one check reported on head 4258a6bf:
- Analyze (javascript-typescript): PASS
Run Tests, Run Format, Integration Tests, and CodeQL's Rust analysis are not run on this head — their last runs were on the parent commit 71df1aa3, which is before the EdgeZero v0.0.8 bump that introduces the blocking finding. Branch protection reports no required checks on this branch. Results from running the full gate list locally against this head:
| Gate | Result |
|---|---|
cargo fmt --all -- --check |
PASS |
clippy-fastly / -axum / -cloudflare / -cloudflare-wasm / -spin-native / -spin-wasm |
PASS |
clippy -p trusted-server-cli |
PASS |
cargo test-fastly (167 + 2291 + 2 + 21 + 3) |
PASS |
cargo test-axum / test-cloudflare / test-spin |
PASS |
| parity suite (13) | PASS |
cargo test -p trusted-server-cli (152 + 5 + 29 + 1) |
PASS |
integration-tests --bins (8) |
PASS |
docs prettier --check |
PASS |
template-cache-local-test.sh esi |
FAIL — 0 passed, 13 failed |
template-cache-local-test.sh inline |
FAIL |
The two harness failures are the blocking finding, and both pass once it is fixed.
4258a6b to
b89eb7a
Compare
Unify Tinybird, DataDome, and S3 static credentials under the logical default secret store, resolve them during typed config loading, and remove request-time static secret reads. Honor Fastly logical-to-physical store mappings, preserve deserialize-only selector compatibility, redact runtime values, and document provisioning and migration behavior.
Resolve trusted client IP credentials through the configured secret store, align adapter templates and operator guidance, and update EdgeZero dependencies to v0.0.8.
b89eb7a to
b540002
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Moves every static app-config credential from a plaintext value in the pushed blob to a secret-store key name resolved after envelope verification, on all four adapters. The deploy/runtime validation split is the right shape (validate_settings_for_deploy checks key-name structure, validate_settings_for_runtime runs every value validator against resolved secrets), and the fail-closed test matrix in config_payload.rs covers the hard cases: missing key, malformed path, non-string leaf, invalid UTF-8, empty resolved value, inactive-feature stripping, string-boolean flags in both directions, placeholder-after-resolution, and no-mutation-on-failure. I read every changed file and found no correctness bug.
The one thing blocking a clean review is a question about rollout, not a defect in the code: this is an unconditional cutover with no blob shape that both the old and new binary accept.
2 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 this branch. Both were applied in a scratch worktree at this head and verified before posting. The remaining comments describe the fix in prose because the change spans multiple files or lines outside the diff hunks.
Blocking
❓ question
- No blob/binary version that both the old and new binary accept — see "Cross-cutting" below
Non-blocking
🔧 wrench
- Broken doc anchor
#secret-store-migration— see inline atdocs/guide/ec-setup-guide.md:41(suggestion) trusted-server.example.tomlstill promises a push-time placeholder gate that no longer exists — see inline attrusted-server.example.toml:44
♻️ refactor
- Resolver silently treats a named-store secret as a default-store secret — see inline at
crates/trusted-server-core/src/secret_resolution.rs:30-33(suggestion) validate_config_for_deployandvalidate_config_for_startupare byte-identical — see inline atcrates/trusted-server-core/src/integrations/datadome.rs:480
🤔 thinking
- "Is this secret required" now lives in three hand-maintained places — see inline at
crates/trusted-server-core/src/config_payload.rs:63 - Cloudflare keeps the first request's
Envforever — see inline atcrates/trusted-server-adapter-cloudflare/src/app.rs:51 - Fastly now does N secret-store round trips per request — see "Cross-cutting" below
📌 out of scope
- Spin request-signing config moved stores in a secrets PR — see inline at
crates/trusted-server-adapter-spin/src/platform.rs:126
📝 note
- PR body's EdgeZero pin is stale — see "Cross-cutting" below
Cross-cutting / body-level findings
-
❓ No blob/binary version that both the old and new binary accept. There is no
[secrets]mode flag and no plaintext fallback, so for an already-deployed service both deploy orders are a full outage until both halves land:- New WASM + currently-deployed plaintext blob →
resolve_secret_referencestreats the live 32-byteproxy_secretas a key name, misses in the secret store,settings_from_config_bloberrors →startup_error_routeranswers every route. - Key-name blob pushed + old WASM still live →
Ec::validate_passphraserejects"ec_passphrase"(13 chars <MIN_PASSPHRASE_LENGTH= 32) → the same startup-error router.
That second direction only fails closed because key names happen to be short. A key name of 32+ characters passes
validate_passphrase, is not inEc::PASSPHRASE_PLACEHOLDERS, and the old binary then HMACs every EC ID off the key name — every visitor identifier rotates silently.publisher.proxy_secrethas no length floor at all, so a rollback there always silently re-keys existing signed proxy URLs rather than failing.The configuration guide covers first-time setup ordering ("Start or deploy instances after the store and pushed config are both ready"), which has no analogue for a service that is already serving. Two questions: does
trusted-server-deployerland the blob and the WASM atomically, or is a maintenance window assumed? And should the docs (or a deploy-time key-name check) steer operators away from 32+ character key names, so that a binary rollback stays fail-closed instead of silently re-keying EC identity? - New WASM + currently-deployed plaintext blob →
-
🤔 Fastly now does N secret-store round trips per request. Fastly Compute spawns a Wasm instance per request, so
build_state→load_settings_from_config_store→resolve_secret_referencesruns on every request, andFastlyPlatformSecretStore::get_bytesperforms a freshSecretStore::openper key (crates/trusted-server-adapter-fastly/src/platform.rs:102). The starter config is 3 keys = 6 host calls; a config with partners, S3 asset auth, DataDome, and Tinybird is several times that. This replaces lazy per-feature reads (Tinybird per emit, DataDome per protected request, S3 behind a processLazyLock) with unconditional work on the TTFB path. Not a blocker, and the redacted-at-rest win is worth real latency — but the cheap mitigation is to open the store once and reuse the handle for every key in a single resolution pass, rather than per key. -
📝 PR body's EdgeZero pin is stale. The description says the branch pins commit
0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34from the unmergedstackpop/edgezero#344.Cargo.tomlandCargo.lockactually pin the released tagv0.0.8(5679641), which is strictly better than a commit on an open PR branch. Worth correcting in the description so a future reader does not go looking for an unmerged dependency. -
👍 Resolved secrets cannot leak through validation errors.
validation_error_summary(crates/trusted-server-core/src/settings.rs:3382) replacesformat!("{err}")onValidationErrors, whoseparamscarry the offending value — so a short resolved passphrase can no longer print itself into the startup error.IntegrationSettings's hand-writtenDebugprinting only sorted integration ids is the same instinct, and both have tests asserting the secret is absent from the output. That is exactly the failure mode this change would otherwise have introduced, caught before it shipped. -
👍 The fail-closed test matrix is the hard half of this feature, and it is actually covered.
resolves_all_static_credentials_from_the_mapped_default_store,active_partner_pull_sync_fails_when_its_token_is_missing,inactive_optional_features_do_not_resolve_stale_secret_references,string_true/string_false_pull_sync_flag_*,does_not_mutate_data_when_resolution_fails, andfailed_lookup_reports_safe_reference_context_without_secret_valuestogether pin both the resolution semantics and the no-leak guarantee.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- format-docs: PASS (required)
- format-typescript: PASS (required)
- vitest: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
| for field in C::secret_fields() { | ||
| if matches!(field.kind, SecretKind::StoreRef) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
♻️ refactor — This skips one SecretKind variant and funnels both others through default_store_name. EdgeZero v0.0.8 defines three:
pub enum SecretKind {
KeyInDefault,
KeyInNamedStore { store_ref_field: &'static str },
StoreRef,
}TrustedServerAppConfig::secret_fields() is all KeyInDefault today and secret_metadata_lists_all_secret_paths_and_optionality asserts it, so there is no live bug. But the moment someone annotates a field with #[secret(store_ref = "…")], that field resolves from the default store instead of the named one — silently reading the wrong secret rather than erroring. SecretKind is not #[non_exhaustive], so an exhaustive match also turns any future upstream variant into a compile error here instead of a silent mis-resolution.
| for field in C::secret_fields() { | |
| if matches!(field.kind, SecretKind::StoreRef) { | |
| continue; | |
| } | |
| for field in C::secret_fields() { | |
| match field.kind { | |
| SecretKind::KeyInDefault => {} | |
| // `store_ref` leaves hold a logical store id, not a secret key. | |
| SecretKind::StoreRef => continue, | |
| // Resolving these against `default_store_name` would read the | |
| // wrong store, so fail closed instead. | |
| SecretKind::KeyInNamedStore { .. } => { | |
| return Err(configuration_error(format!( | |
| "unsupported named secret store for `{}`", | |
| field.dotted_path() | |
| ))); | |
| } | |
| } |
Scratch-verified at this head: cargo fmt --all -- --check, cargo clippy-fastly, cargo check-fastly, cargo check-axum, cargo check-cloudflare all clean, and the 10 secret_resolution / config_payload tests pass natively. No post-verification drift.
There was a problem hiding this comment.
Retaining the prior disposition from #1036 (comment). TrustedServerAppConfig emits only KeyInDefault, and the metadata test pins that application contract. Named-store secret leaves are unsupported by Trusted Server; any future metadata change must update the resolver and its tests deliberately. No change in this PR.
| identify and batch-sync APIs, so its partner needs `api_token`. Partners that | ||
| do not call either API may omit it. Provision high-entropy values under | ||
| `ec_passphrase` and `partner_api_token`; see | ||
| [Configuration](/guide/configuration#secret-store-migration). |
There was a problem hiding this comment.
🔧 wrench — This anchor does not exist. docs/guide/configuration.md has no heading that slugifies to secret-store-migration; its headings in this area are ### Static secret references, ### Tinybird auction telemetry, and ### Generate Secure Secrets. The link currently lands on the page with no scroll target, which is the one place a reader following the secret-reference explanation most needs to arrive.
| [Configuration](/guide/configuration#secret-store-migration). | |
| [Configuration](/guide/configuration#static-secret-references). |
Scratch-verified with the repo-pinned prettier: All matched files use Prettier code style!
There was a problem hiding this comment.
This repeats #1036 (comment). Retaining the recorded no-change disposition. No documentation change in this PR.
| password = "handler_password" | ||
|
|
||
| # You can add more handlers to basic-auth-protect other path prefixes. The | ||
| # sample password below is a known placeholder that deploy validation rejects, |
There was a problem hiding this comment.
🔧 wrench — This claim is no longer true, and neither is the matching sentence in the section header above (lines 33-36: "Each handler needs a non-placeholder username/password (deploy validation rejects the sample password below)").
validate_settings_for_deploy no longer calls reject_placeholder_secrets or validate_admin_handler_passwords — both moved to validate_settings_for_runtime, where they check the resolved value. That split is correct now that the field holds a key name, but it means password = "replace-with-admin-password" is a perfectly valid key name that passes ts config validate and ts config push clean. An operator who uncomments this block trusting the comment gets no push-time error at all; they find out at instance startup, if the store happens to hold a placeholder under that key.
Proposed fix (apply manually — lines 33-36 sit outside any RIGHT-side hunk, and the sentence here runs onto line 45, past the end of this hunk, so neither block can be expressed as a suggestion):
Header comment, lines 33-36:
# HTTP Basic-auth handler(s). At least one handler whose `path` regex covers
# the /_ts/admin endpoints is mandatory; startup fails without it. `password`
# is a key in trusted_server_secrets, not the credential itself; the resolved
# value is rejected at startup if it is a known placeholder.This comment, lines 43-45:
# You can add more handlers to basic-auth-protect other path prefixes. Each
# needs its own secret-store key name:And the sample on line 49 reads better as a key name than as a password placeholder — e.g. password = "secure_handler_password".
There was a problem hiding this comment.
This repeats #1036 (comment). Retaining the recorded disposition: the executable starter handler uses the handler_password key reference, while the quoted wording is confined to a commented secondary example. No example or validation change in this PR.
| Self::try_new(config).map(|_| ()) | ||
| } | ||
|
|
||
| pub(crate) fn validate_config_for_deploy( |
There was a problem hiding this comment.
♻️ refactor — This is byte-identical to validate_config_for_startup directly above (both are Self::try_new(config).map(|_| ())), and config.rs threads a resolved_secrets: bool through validate_enabled_integrations purely to choose between the two:
if resolved_secrets {
DataDomeIntegration::validate_config_for_startup(config)?;
} else {
DataDomeIntegration::validate_config_for_deploy(config)?;
}try_new has no resolved-value check left to skip — the bypass credential's 32-byte floor moved to request time in take_protection_test_bypass_header, which protection_test_bypass_accepts_short_resolved_credential pins. So the parameter and the second function select the same behaviour, which reads as if a deploy/runtime distinction exists for DataDome when it does not.
Proposed fix (apply manually — spans datadome.rs and config.rs): drop validate_config_for_deploy and the resolved_secrets parameter, calling validate_config_for_startup unconditionally. If the split is deliberately a placeholder for a value check you expect to add, a one-line comment saying so would keep the next editor from assuming the two already differ.
There was a problem hiding this comment.
This is the same concern previously discussed at #1036 (comment). Retaining the separate deploy/runtime validation entry points as an explicit phase boundary, even though their current DataDome bodies are identical. No change in this PR.
| Ok(settings) | ||
| } | ||
|
|
||
| fn remove_inactive_secret_references(data: &mut serde_json::Value) { |
There was a problem hiding this comment.
🤔 thinking — After this PR, "when is this secret required" is encoded in three places, in three different representations:
TrustedServerAppConfig::secret_fields()—optionalflags andOptionalField/Fieldsegments (config.rs)validate_secret_key_references— typed, deploy-time (config.rs)- this function — raw JSON pointers, load-time
They agree today; I checked each condition against the matching serde attribute, including the asymmetry where pull_sync_enabled accepts a string boolean while tinybird.enabled and DataDome's flags do not. That asymmetry is correct — only pull_sync_enabled carries deserialize_with = "from_value_or_str" — but it is the kind of thing that reads as a bug to the next person and is invisible from here.
The drift risk is that #3 is string-keyed with no compiler help: rename a field and the strip silently stops happening, so a stale reference for a disabled feature starts failing the whole config load instead of being skipped. A short comment on this function naming its two counterparts, and noting that the as_bool vs json_bool_or_string_is_true split mirrors the per-field deserializers, would make the coupling discoverable.
There was a problem hiding this comment.
Acknowledged. The raw pruning must happen before resolution and deserialization, and the current boolean handling intentionally mirrors each field’s deserializer. The active, inactive, omitted, and string-boolean cases are covered by the config_payload regression tests. We are not redesigning or further documenting this internal coupling in this PR.
| static CLOUDFLARE_CONFIG_JSON: std::sync::OnceLock<String> = std::sync::OnceLock::new(); | ||
| thread_local! { | ||
| static CLOUDFLARE_CONFIG_JSON: std::cell::OnceCell<String> = const { std::cell::OnceCell::new() }; | ||
| static CLOUDFLARE_ENV: std::cell::OnceCell<worker::Env> = const { std::cell::OnceCell::new() }; |
There was a problem hiding this comment.
🤔 thinking — lib.rs calls set_cloudflare_env(env.clone()) on every request, but OnceCell::set succeeds only once, so every later call is silently discarded and settings_from_cloudflare_config_json reads secrets through the first request's Env for the isolate's whole lifetime. Since routes() rebuilds state per request, that stale handle is used on every subsequent secret resolution.
Moving from OnceLock to thread_local! is right — worker::Env is JsValue-backed and not Send/Sync — but the once-only semantics look accidental rather than intended here. A cell that takes the current request's env would avoid holding a JS handle across request contexts:
static CLOUDFLARE_ENV: std::cell::RefCell<Option<worker::Env>> =
const { std::cell::RefCell::new(None) };with set_cloudflare_env doing a replace(Some(env)) and the reader cloning out of the borrow.
To be clear about the strength of this: the Cloudflare integration tests run against wrangler dev across many requests and pass, so I could not show breakage — secret and text bindings are plain values, not I/O objects. Flagging the pattern, not a defect. Apply manually; it is a small change but not a single-hunk replacement.
There was a problem hiding this comment.
This repeats #1036 (comment) without a new failing case or runtime contract. Retaining the existing first-wins binding lifecycle. The binding lookup is synchronous, and the multi-request Cloudflare integration tests pass. No lifecycle change in this PR.
| pub(crate) struct ConfigStoreHandleAdapter(pub(crate) ConfigStoreHandle); | ||
|
|
||
| impl PlatformConfigStore for ConfigStoreHandleAdapter { | ||
| fn get(&self, _store_name: &StoreName, key: &str) -> Result<String, Report<PlatformError>> { |
There was a problem hiding this comment.
📌 out of scope — Dropping the Spin-variable encoding here in favour of verbatim KV keys is a breaking change for Spin operators, independent of secret references. Together with the spin.toml change removing v_current_x2dkid and v_active_x2dkids, anyone running request_signing on Spin must move current-kid, active-kids, and every public JWK entry out of component variables and into the default key-value store, or key rotation and discovery stop resolving after this lands.
The spin.toml comment documents the new layout well, and the spin_encoder_accepts_every_creatable_kid portability contract is correctly retargeted at the secret encoder. The gap is discoverability: nothing in the PR description or the docs mentions it, so a Spin operator reading the changelog for "secret-store config resolution" has no reason to expect their signing config moved. Worth either a line in the description or a follow-up issue for the Spin migration note.
There was a problem hiding this comment.
The verbatim Spin KV-key behavior is intentional, documented in spin.toml, and pinned by config_store_handle_adapter_reads_verbatim_kv_key. We are not adding a separate migration note or follow-up issue in this PR.
|
Review follow-up to the comments submitted against The rollout concern does not apply to the current deployment state. This is intentionally a hard cutover, and that deployment model is already documented. The secret-reference configuration and binary have already been deployed to production, so merging this branch to We are not adding a transitional mode, plaintext fallback, migration orchestration, or key-length convention. For the other body-level observations:
The seven inline replies record the remaining no-change dispositions. All checks passed on the reviewed head; CI is rerunning after the merge from Re-requesting review from @prk-Jr. |
3 participants
Summary
trusted_server_secretsas the logical store name while allowing adapters to map it to a physical store such as Fastly'sts_secrets. Missing or invalid secrets fail configuration loading without exposing their values.secret_storeselectors for one release, warn that they are ignored, and omit them when serializing configuration.api_tokenreferences optional. Partners without one remain available for source-domain lookup, bidstream EIDs, and outbound pull sync, but cannot authenticate to the inbound identify or batch-sync APIs.ts_pull_tokenremains required only when pull sync is enabled.This fixes the deployment failure where a valid secret existed in Fastly but Trusted Server opened the logical store name instead of the mapped physical store.
Changes
.env.dev.env.exampleCargo.tomlCargo.lockcrates/trusted-server-adapter-axum/src/app.rscrates/trusted-server-adapter-cloudflare/src/app.rscrates/trusted-server-adapter-cloudflare/src/lib.rscrates/trusted-server-adapter-cloudflare/src/platform.rscrates/trusted-server-adapter-cloudflare/wrangler.ci.tomlcrates/trusted-server-adapter-cloudflare/wrangler.tomlcrates/trusted-server-adapter-fastly/src/app.rscrates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-adapter-fastly/src/tinybird.rscrates/trusted-server-adapter-spin/spin.tomlcrates/trusted-server-adapter-spin/src/app.rscrates/trusted-server-adapter-spin/src/platform.rscrates/trusted-server-core/src/config.rscrates/trusted-server-core/src/config_payload.rscrates/trusted-server-core/src/ec/auth.rscrates/trusted-server-core/src/ec/registry.rscrates/trusted-server-core/src/integrations/datadome.rscrates/trusted-server-core/src/integrations/datadome/protection.rscrates/trusted-server-core/src/lib.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/secret_resolution.rscrates/trusted-server-core/src/settings.rscrates/trusted-server-core/src/settings_data.rscrates/trusted-server-integration-tests/Cargo.tomlcrates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.tomlcrates/trusted-server-integration-tests/fixtures/configs/viceroy-template.tomlcrates/trusted-server-integration-tests/src/bin/generate-viceroy-config.rscrates/trusted-server-integration-tests/tests/common/config.rscrates/trusted-server-integration-tests/tests/environments/axum.rsdocs/guide/asset-routes.mddocs/guide/configuration.mddocs/guide/ec-setup-guide.mddocs/guide/fastly.mdtrusted_server_secretsto physicalts_secretsmapping and provisioning requirements.docs/guide/getting-started.mddocs/guide/integrations/datadome.mdfastly.tomltrusted-server.example.tomlScope
This PR touches the core schema, each adapter startup path, integration fixtures, and operator documentation because secret references must behave the same on Fastly, Axum, Cloudflare, and Spin. The request-signing key collection, rotation stores, and Fastly management credentials remain outside this change because those stores are managed at runtime rather than loaded as static application configuration.
EdgeZero dependency
This PR depends on stackpop/edgezero#344, "Support optional typed secret paths and Fastly store mappings." That PR adds optional intermediate path handling and persists validated logical-to-physical store mappings during Fastly provisioning and staged deployment. Trusted Server pins its tested commit,
0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34. All checks on the EdgeZero PR pass.Closes
Closes #684
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run, no JS source changedcd crates/trusted-server-js/lib && npm run format, no JS source changedcd docs && npm run formatfastly compute servecargo test-cloudflare,cargo test-spin, adapter parity tests, CLI tests, Cloudflare and Spin WASM checks, all adapter-specific Clippy targets, andgit diff --check/health; a settings-load probe found no secret-resolution or application-state errorsChecklist
unwrap()in production code, useexpect("should ...")