Skip to content

Add native secret-store config resolution - #1036

Open
ChristianPavilonis wants to merge 24 commits into
mainfrom
edgezero-secrets
Open

Add native secret-store config resolution#1036
ChristianPavilonis wants to merge 24 commits into
mainfrom
edgezero-secrets

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Store references to static credentials in Trusted Server configuration, then resolve those references from the platform's secret store after the signed configuration blob passes integrity checks.
  • Resolve publisher, Edge Cookie partner, handler, Tinybird, DataDome, and S3 credentials while loading typed application configuration. Runtime code receives redacted values and no longer reads these static credentials during requests.
  • Keep trusted_server_secrets as the logical store name while allowing adapters to map it to a physical store such as Fastly's ts_secrets. Missing or invalid secrets fail configuration loading without exposing their values.
  • Accept the old feature-specific secret_store selectors for one release, warn that they are ignored, and omit them when serializing configuration.
  • Make Edge Cookie partner api_token references 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_token remains 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

File Change
.env.dev Clarify that the file contains non-secret development overlays and point local users to the config blob and secret-store setup.
.env.example Document logical-to-physical secret-store mapping and replace plaintext secret examples with platform secret-store guidance.
Cargo.toml Pin the EdgeZero revision that supports optional secret paths and persisted Fastly store mappings.
Cargo.lock Record the updated EdgeZero dependency graph.
crates/trusted-server-adapter-axum/src/app.rs Pass the Axum secret-store adapter into typed settings loading.
crates/trusted-server-adapter-cloudflare/src/app.rs Resolve configuration references through the Cloudflare Worker environment during startup.
crates/trusted-server-adapter-cloudflare/src/lib.rs Make the Worker environment available to startup configuration loading.
crates/trusted-server-adapter-cloudflare/src/platform.rs Expose the Cloudflare secret-store adapter within the crate for configuration resolution.
crates/trusted-server-adapter-cloudflare/wrangler.ci.toml Add fictional local secret bindings used by Cloudflare integration tests.
crates/trusted-server-adapter-cloudflare/wrangler.toml Document how operators provision Worker secrets referenced by application configuration.
crates/trusted-server-adapter-fastly/src/app.rs Load Fastly runtime store mappings, resolve typed secrets at startup and reload, and cover mapped-store behavior with tests.
crates/trusted-server-adapter-fastly/src/main.rs Build the Fastly application with the runtime environment mapping used by EdgeZero.
crates/trusted-server-adapter-fastly/src/tinybird.rs Use the Tinybird token resolved during configuration loading instead of reading a secret store during each request.
crates/trusted-server-adapter-spin/spin.toml Declare Spin secret variables for application-config references.
crates/trusted-server-adapter-spin/src/app.rs Pass the Spin secret store into startup configuration loading.
crates/trusted-server-adapter-spin/src/platform.rs Add the Spin adapter used to resolve typed application secrets.
crates/trusted-server-core/src/config.rs Mark secret-bearing fields, make partner API-token references optional, add conditional requirements, support the deserialize-only selector bridge, and split deploy-time structure checks from post-resolution validation.
crates/trusted-server-core/src/config_payload.rs Verify blob integrity before resolving references and add fail-closed tests for missing, malformed, inactive, and optional secrets, including partners without API tokens.
crates/trusted-server-core/src/ec/auth.rs Keep inbound bearer authentication fail-closed when a configured partner has no API token.
crates/trusted-server-core/src/ec/registry.rs Register every partner by source domain while hashing and indexing only configured API tokens; validate partner structure before deployment and defer token-value checks until references have been resolved.
crates/trusted-server-core/src/integrations/datadome.rs Load DataDome credentials into redacted runtime settings and ignore the old store selector.
crates/trusted-server-core/src/integrations/datadome/protection.rs Use the resolved DataDome key without a request-time secret-store lookup while retaining the configuration-gated test bypass.
crates/trusted-server-core/src/lib.rs Export the secret-resolution module.
crates/trusted-server-core/src/proxy.rs Use resolved publisher and S3 credentials and remove feature-specific runtime secret-store reads.
crates/trusted-server-core/src/publisher.rs Update publisher tests for DataDome's typed secret reference.
crates/trusted-server-core/src/secret_resolution.rs Add recursive typed resolution for nested objects, arrays, optional containers, and redacted errors.
crates/trusted-server-core/src/settings.rs Separate reference-bearing application configuration from resolved runtime settings, make partner API tokens optional, and sanitize validation failures.
crates/trusted-server-core/src/settings_data.rs Define the logical default secret store and thread it through config-store loading.
crates/trusted-server-integration-tests/Cargo.toml Make TOML parsing available to the integration config generator.
crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml Replace integration fixture credentials with secret key names.
crates/trusted-server-integration-tests/fixtures/configs/viceroy-template.toml Add the local runtime mapping and fictional secret-store entries used by Viceroy.
crates/trusted-server-integration-tests/src/bin/generate-viceroy-config.rs Generate local secret-store data for references found in integration configuration.
crates/trusted-server-integration-tests/tests/common/config.rs Build test envelopes from the typed application-config representation.
crates/trusted-server-integration-tests/tests/environments/axum.rs Supply fictional referenced secrets to the Axum integration environment.
docs/guide/asset-routes.md Update asset-route examples to use the resolved publisher secret model.
docs/guide/configuration.md Explain reference syntax, conditional requirements, optional partner API access, compatibility behavior, redaction, and migration from feature-specific stores.
docs/guide/ec-setup-guide.md Clarify that the demo requires a partner API token because it exercises inbound identify and batch-sync APIs, while other partners may omit it.
docs/guide/fastly.md Document Fastly's logical trusted_server_secrets to physical ts_secrets mapping and provisioning requirements.
docs/guide/getting-started.md Add local setup instructions for config blobs and referenced secret values.
docs/guide/integrations/datadome.md Replace the old DataDome store selector with a typed key reference.
fastly.toml Configure the local Fastly runtime mapping and a placeholder physical secret store.
trusted-server.example.toml Replace plaintext credentials with key names, mark partner API tokens as optional for partners that do not use inbound APIs, and add Tinybird and DataDome reference examples.

Scope

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-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run, no JS source changed
  • JS format: cd crates/trusted-server-js/lib && npm run format, no JS source changed
  • Docs format: cd docs && npm run format
  • WASM build: the deployment workflow built and staged the Fastly artifact
  • Manual testing via fastly compute serve
  • Other: cargo test-cloudflare, cargo test-spin, adapter parity tests, CLI tests, Cloudflare and Spin WASM checks, all adapter-specific Clippy targets, and git diff --check
  • Staged Fastly deployment: run 32784895487 passed /health; a settings-load probe found no secret-resolution or application-state errors

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code, use expect("should ...")
  • Logging follows project conventions; no direct stdout or stderr logging was added
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis
ChristianPavilonis marked this pull request as draft August 18, 2026 18:29
@ChristianPavilonis ChristianPavilonis changed the title feat: add native secret-store config resolution Add native secret-store config resolution Aug 18, 2026
@aram356 aram356 added this to the 202608 milestone Aug 18, 2026
@aram356

aram356 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis to test it before merging into #1019

@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review August 24, 2026 22:48
@ChristianPavilonis
ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 August 24, 2026 22:48
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.

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at crates/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
  • EchoSecretStore makes resolution untestable — see inline at crates/trusted-server-core/src/config_payload.rs:145
  • expect() on the Tinybird token traps the Wasm guest — see inline at crates/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 at trusted-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 windowdocs/guide/configuration.md:60-72 gives 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 on main today (MIN_PASSPHRASE_LENGTH = 32, crates/trusted-server-core/src/settings.rs) — against passphrase = "ec_passphrase" (13 bytes). That yields short_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 PRCargo.toml:57-62 moves all six edgezero crates from stable tag v0.0.4 to git rev 0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34, a commit on the still-open stackpop/edgezero#344. Merging this puts main on a branch commit of an unmerged PR: if #344 is rebased or force-pushed before it merges, that commit can become unreachable and main stops 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 on main acceptable here?

  • 📝 CI coverage gap on the reviewed head — only Analyze (javascript-typescript) ran on 1315cdb1. 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 commit 598f7100, three commits earlier. That leaves 070397f1 Resolve static credentials through typed config, b1e967e3, and 1315cdb1 without Rust, adapter, or lint coverage. Worth re-triggering the suite on the current head before merge, independent of the findings above.

  • 👍 validation_error_summary is a real leak fixcrates/trusted-server-core/src/settings.rs:2387-2424 walks ValidationErrors emitting only path: code, never validator's params, which hold the offending value. The previous code formatted ValidationErrors wholesale into a config error message.

  • 👍 Deleting S3_CREDENTIALS_CACHE removes a genuinely bad structurecrates/trusted-server-core/src/proxy.rs previously kept a process-global HashMap keyed on the plaintext secret access key, with unbounded growth and a poisoning-prone Mutex. Startup-resolved values are strictly better.

  • 👍 IntegrationSettings's custom Debug closes the DataDome-key leak that the flattened JsonValue map would otherwise print.

  • 👍 The two payload resolution tests are the right pairresolves_all_static_credentials_from_the_mapped_default_store proves every path arm resolves through a mapped physical store, and inactive_optional_features_do_not_resolve_stale_secret_references proves disabled features do not demand stale references. Also good: dropping include_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.

Comment thread crates/trusted-server-core/src/secret_resolution.rs
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-core/src/settings.rs
Comment thread crates/trusted-server-core/src/config_payload.rs
Comment thread crates/trusted-server-core/src/config_payload.rs
Comment thread crates/trusted-server-core/src/ec/registry.rs
Comment thread docs/guide/configuration.md Outdated
Comment thread trusted-server.example.toml Outdated
Comment thread crates/trusted-server-core/src/secret_resolution.rs
Comment thread crates/trusted-server-adapter-spin/spin.toml Outdated
@ChristianPavilonis

Copy link
Copy Markdown
Collaborator Author

Review follow-up for b47ced81a:

  • The accepted diagnostics, DataDome pruning, Fastly mapping, EC validation, harness, example, and documentation changes are implemented. I replied to and resolved all 11 inline threads.
  • I am not adding migration orchestration for the plaintext-to-reference transition. Trusted Server has not entered production with the old plaintext configuration model, so secret references will be the production baseline rather than a live migration. Deployment machinery here would be speculative.
  • The EdgeZero revision remains an intentional dependency for RC validation. PR Support optional typed secret paths and Fastly store mappings stackpop/edgezero#344 is still the upstream dependency; Add native secret-store config resolution #1036 should use its accepted final revision before a mainline merge if project policy requires that.
  • Local validation passed: all four adapter test suites, all six Clippy targets, Rust and docs formatting, git diff --check, focused regression tests, and both template-cache harness modes. GitHub checks are rerunning on the pushed head.

Re-requesting review from @prk-Jr.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-logging alerts — see Cross-cutting below

Non-blocking

♻️ refactor / 🤔 thinking / ⛏ nitpick / 📝 note / 🌱 seedling

  • .env.example ships 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_name holds the resolved key value at runtime — see inline at crates/trusted-server-core/src/integrations/datadome.rs:184 (suggestion)
  • validate_config_for_deploy uses HashMap<_, ()> as a set — see inline at crates/trusted-server-core/src/ec/registry.rs:77
  • Hooks::stores() duplicates edgezero.toml — see inline at crates/trusted-server-adapter-fastly/src/app.rs:1332
  • Cargo.lock rewrote prost's itertools edges — see Cross-cutting below

Cross-cutting / body-level findings

  • 🔧 CodeQL check failed: 15 high rust/cleartext-logging alerts. 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 of resolve_secret_references / validate_tinybird_secret / validate_admin_handler_passwords as secret-tainted and flags logs of plainly non-secret fields (asset-route prefixes in settings.rs, DataDome registration flags in datadome.rs:979, consent clamping in consent_config.rs, header names in response_privacy.rs, etc.). No alert is a real value leak — the nearest real vector is the inline finding at secret_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.lock moved prost's itertools dependency edges from 0.13.0 to 0.10.5. The edgezero pin update also rewrote prost-build/prost-derive's itertools edges 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

Comment thread crates/trusted-server-core/src/secret_resolution.rs Outdated
Comment thread .env.example Outdated
Comment thread docs/guide/configuration.md
Comment thread crates/trusted-server-core/src/secret_resolution.rs
Comment thread crates/trusted-server-core/src/integrations/datadome.rs
Comment thread crates/trusted-server-core/src/ec/registry.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
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.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-logging alerts still open — see Cross-cutting below

Non-blocking

⛏ nitpick

  • Stale "must be at least 32 bytes" claim for proxy_secret — see inline at docs/guide/configuration.md:361 (suggestion)
  • Cargo.lock prost itertools edges still rewritten — see Cross-cutting below

Cross-cutting / body-level findings

  • 🔧 CodeQL: 15 high rust/cleartext-logging alerts still open. Carried over from the previous review round. The code fix in 76f6f13 does not clear them — all 15 are taint over-approximation (CodeQL treats everything flowing out of resolve_secret_references / validate_tinybird_secret / validate_admin_handler_passwords as 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.lock still carries the rewritten prost itertools edges (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

Comment thread docs/guide/configuration.md Outdated
ChristianPavilonis added a commit that referenced this pull request Aug 28, 2026
# 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
…erver into edgezero-secrets

# Conflicts:
#	crates/trusted-server-core/src/config.rs
#	scripts/template-cache-local-test.sh
#	trusted-server.example.toml
…zero-secrets

# Conflicts:
#	crates/trusted-server-adapter-axum/src/app.rs
#	crates/trusted-server-adapter-fastly/src/app.rs
#	crates/trusted-server-core/src/proxy.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -- --check clean, clippy-fastly / clippy-axum / clippy-cloudflare / clippy-spin-native all clean at -D warnings, 2,477 tests passing across test-fastly (including 2,284 core tests), test-axum, test-cloudflare and test-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

  • KeyInNamedStore fields would silently resolve from the default store — see inline at crates/trusted-server-core/src/secret_resolution.rs:29
  • trusted_client_ip.shared_secret is the one credential left plaintext in the blob — see inline at crates/trusted-server-core/src/config.rs:135
  • Local fastly compute serve cannot start — no app-config secrets seeded — see inline at fastly.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:5 and docs/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.lock churn: prost's itertools 0.13.0 → 0.10.5 — see inline at Cargo.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_deploy are byte-identical, so resolved_secrets is a no-op — see inline at crates/trusted-server-core/src/config.rs:284
  • pull_sync_enabled read with as_bool() but deserialized with from_value_or_str — see inline at crates/trusted-server-core/src/config_payload.rs:84
  • resolve_leaf discards the PlatformError cause, losing the primary triage signal — see inline at crates/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 at crates/trusted-server-core/src/ec/registry.rs:403
  • require_nonempty_token is a flag that earns nothing — see inline at crates/trusted-server-core/src/ec/registry.rs:348
  • Cloudflare re-resolves every secret per request, against request #1's Env — see inline at crates/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 at crates/trusted-server-adapter-spin/src/platform.rs:127
  • EDGEZERO__STORES__SECRETS__…__NAME silently redirects the Axum lookups — see inline at docs/guide/getting-started.md:104
  • Only worked example puts the passphrase in argv — see inline at docs/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_summary drops the validator message — see inline at crates/trusted-server-core/src/settings.rs:3403
  • Dead deprecation branches in try_new — see inline at crates/trusted-server-core/src/integrations/datadome.rs:390
  • S3Credentials rebuilt with three allocations per signing call — see inline at crates/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_resolution should be pub(crate) — see inline at crates/trusted-server-core/src/lib.rs:67
  • tinybird.remove("access_token_secret") is dead — see inline at crates/trusted-server-core/src/config_payload.rs:73
  • Duplicate use lines — see inline at crates/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_token doc still says "Plaintext API token" — see inline at crates/trusted-server-core/src/settings.rs:377
  • .env.example ordering 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().kv is declared but nothing on the Fastly path consumes it — see inline at crates/trusted-server-adapter-fastly/src/app.rs:1338
  • jq dependency not in Prerequisites — see inline at docs/guide/getting-started.md:80
  • configuration.md S3 table vs example key mismatch — see inline at docs/guide/configuration.md:1099

👍 praise

  • Hand-written IntegrationSettings Debug closes a real leak — see inline at crates/trusted-server-core/src/settings.rs:220
  • hooks_store_metadata_matches_edgezero_manifest pins exactly the right invariant — see inline at crates/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-logging high 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 log route.prefix, a cache rule id, a path, and DataDome's sdk_origin/rewrite_sdk/enable_protection; the named source try_new_with_secret_validation does 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: Settings now carries resolved plaintext secrets, so every log of any Settings-derived field lands in a taint path. The architectural signal is real and new even though each alert is not — before this PR, "log a Settings field" 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 whose Debug/Display/Serialize cannot emit the value. Note Redacted is 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/main conflicts in Cargo.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.rs and docs/guide/configuration.md. In config.rs, main already carries a secret_fields() stub returning Vec::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, main threads &EnvConfig with config_store_name(env) / config_key(env), while this PR replaces that seam with RuntimeStoreConfig / DEFAULT_CONFIG_STORE_ID / env_config_from_runtime_dictionary. Separately, this PR rewrites "EdgeZero's env overlay" → "The pinned EdgeZero loader" in three places in configuration.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 swaps Settings::from_toml + validate_settings_for_deploy for toml::from_str::<TrustedServerAppConfig> + TrustedServerAppConfig::new, and generated_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-26 already 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-62 pins all six edgezero crates to rev 0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34. Verified against stackpop/edgezero: it is not an ancestor of origin/main, git branch -a --contains and git tag --contains are both empty, and it is reachable only as a bare refs/commit/<sha>. It is 140 commits behind v0.0.7 (5c9886e5), which main now uses — so merging as-is downgrades EdgeZero across adapter-{axum,cloudflare,fastly,spin}, cli and core. Upstream stackpop/edgezero#344 is still open with mergeCommit: null, and its current head is 055f7e94, 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 point cargo fetch fails on every cold cache including CI. All seven edgezero packages in Cargo.lock do 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-62 gained a required setup step documented nowhere. That section is untouched by this PR and still reads fastly compute serve with no config or secret preparation, but local serve now requires hand-seeded secrets (see the fastly.toml comment). grep -rn "local_server.secret_stores" docs/ returns only key-rotation.md and request-signing.md, both about signing_keys. Option B got a full rewrite; Option A got nothing. The recipe already exists at scripts/template-cache-local-test.sh:227-243. Body-level because line 46 is outside every RIGHT-side hunk.

  • ♻️ resolve_secret_references deep-clones the whole config for an atomicity guarantee no caller uses (secret_resolution.rs:29 and :43). The only production caller is config_payload.rs:53, which passes a local data that is dropped on the error path anyway; the clone's sole beneficiary is does_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 the KeyInNamedStore suggestion.

  • Stale S3Credentials doc 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_auth builds these from already-resolved config, and S3_CREDENTIALS_CACHE was removed by this PR. s3_sigv4.rs is 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-356 is now false. "the plaintext is never stored at runtime" holds for PartnerConfig (hash only) but not for EcPartner.api_token, which holds the resolved plaintext in Settings for 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:4 authors = ["jason@stackpop.com"], :10 service_id = "dysUw6h73VzeomD61eal85", and :50 data = "NVnTYrw5xoyTJDOwoUWoPJO3A6UCCXOJJUzgGTxxx7k=" — a base64 32-byte Ed25519-shaped value in signing_keys whose embedded xxx suggests deliberate mangling. viceroy-template.toml:55-59 carries an explicit "generated for testing, never used in production" attestation for the same value; fastly.toml has 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 intentional OnceCell::set calls); per-request versus startup secret reads (the whole per-request class is closed, and the only remaining secret_store() callers read the separate request_signing.secret_store_id); the api_token: Option auth change traced from both directions; the deploy/runtime validation split, where runtime is a strict superset for value checks and a net tightening versus main; the legacy-selector compatibility bridge (all three warn, none survives a round trip, and init_cli_logger sets LevelFilter::Info so the warnings are actually visible); recursive resolution edge cases (empty arrays, Option containers, null versus absent, the internally-tagged AssetOriginAuth, non-string leaves, and no overlapping SecretField paths); spin.toml's secret-variable encoding byte-for-byte against spin_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 runtime Settings serialization path; and scripts/template-cache-local-test.sh end 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.toml secret-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: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • CodeQL: FAIL
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS
  • cargo test: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo fmt: PASS (required)
  • format-typescript: PASS (required)
  • format-docs: PASS (required)
  • vitest: PASS

Comment on lines +29 to +33
let mut resolved_data = data.clone();
for field in C::secret_fields() {
if matches!(field.kind, SecretKind::StoreRef) {
continue;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrenchKeyInNamedStore secret fields would silently resolve from the default store.

SecretKind has three variants (edgezero-core/src/app_config.rs:148-164): KeyInDefault, KeyInNamedStore { store_ref_field }, and StoreRef. The matches! skips only StoreRef, so KeyInNamedStore falls through to resolve_leaf, which reads default_store_name unconditionally. SecretKind is not #[non_exhaustive] and this is an if, not a match — so adding a store-selected secret field later compiles cleanly and reads the wrong store. That is the "something else steers which secret gets read" shape, arrived at by refactor rather than by attack.

Latent, not live: TrustedServerAppConfig currently emits only KeyInDefault, pinned by config.rs:745-751. An exhaustive match makes the next person add the handling deliberately instead of inheriting the wrong default.

Suggested change
let mut resolved_data = data.clone();
for field in C::secret_fields() {
if matches!(field.kind, SecretKind::StoreRef) {
continue;
}
let mut resolved_data = data.clone();
for field in C::secret_fields() {
match field.kind {
SecretKind::KeyInDefault => {}
// The field's value is a store id, not a key name.
SecretKind::StoreRef => continue,
SecretKind::KeyInNamedStore { store_ref_field } => {
return Err(configuration_error(format!(
"secret field `{}` selects the store named by `{store_ref_field}`, which this \
resolver does not support",
field.dotted_path()
)));
}
}

Comment thread fastly.toml
Comment on lines 62 to +64
[[local_server.secret_stores.ts_secrets]]
key = "tinybird_access_append_token"
data = "test-tinybird-access-append-token"
key = "placeholder"
data = "placeholder"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — Local fastly compute serve cannot start: no app-config secrets are seeded here.

trusted-server.example.toml references three key names — handler_password (:41), publisher_proxy_secret (:63) and ec_passphrase (:77) — and this store contains only tinybird_auction_append_token and placeholder. All three are optional: false, so there is no skip path.

ts config push --local will not create them either. The pinned CLI is explicit: local-server seeding is "config-stores only, and kv/secret local-server seeding is hand-edited until we add equivalent writers for those kinds" (edgezero-adapter-fastly/src/cli.rs:1589-1590). Following the documented flow therefore fails at resolution with failed to resolve secret reference at 'publisher.proxy_secret' from secret store ts_secrets, and since resolution is fail-closed no state is built, so every request gets the startup-error response.

Note this is not the logical-vs-physical mapping bug — the mapping at :70 is present and correct. The runtime opens the right store; the store is just missing the keys.

This PR's own harness proves the fix shape: scripts/template-cache-local-test.sh:226-241 appends exactly these three entries to a throwaway copy in $WORK. Nothing carries them into the tracked manifest or into getting-started.md, which is why CI stays green while a fresh clone breaks.

Suggested change
[[local_server.secret_stores.ts_secrets]]
key = "tinybird_access_append_token"
data = "test-tinybird-access-append-token"
key = "placeholder"
data = "placeholder"
# `ts config push --local` writes config stores only, so these local
# entries are hand-maintained. Values are fictional local-only stand-ins;
# add one entry per secret key name your trusted-server.toml references.
[[local_server.secret_stores.ts_secrets]]
key = "publisher_proxy_secret"
data = "fictional-local-publisher-proxy-secret-value"
[[local_server.secret_stores.ts_secrets]]
key = "ec_passphrase"
data = "fictional-local-ec-passphrase-secret-value"
[[local_server.secret_stores.ts_secrets]]
key = "handler_password"
data = "fictional-local-handler-password-secret-value"

Optional hardening worth considering instead of literal data =: Fastly's local secret stores accept env = "VAR", as fastly.toml:52-54 already does for api-keys. That keeps values out of the tracked file entirely and removes any temptation to paste a real one in.

Comment thread .env.dev
Comment on lines 5 to 6
# [publisher]
TRUSTED_SERVER__PUBLISHER__ORIGIN_URL=http://localhost:9090

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — The Axum quick-start cannot complete, because the starter config's publisher domains are reserved placeholders.

trusted-server.example.toml:57-59 ships domain = "example.com" and cookie_domain = ".example.com", and both are in the reject lists at settings.rs:131-132 (PLACEHOLDER_DOMAINS / PLACEHOLDER_COOKIE_DOMAINS). validate_non_secret_deploy_placeholders raises TrustedServerError::InsecureDefault (config.rs:320-325) through validate_settings_for_deployTrustedServerAppConfig::validate()validate_excluding_secrets, which run_config_push_typed calls. The error is keyed at the root field trusted_server (config.rs:110-115), and prune_secret_leaf only descends secret paths — so it survives pruning and both ts config validate and ts config push fail. reject_placeholder_secrets re-checks the same three fields at runtime (settings.rs:3071-3079), so even a hand-forced blob would not start. .env.dev currently overrides only ORIGIN_URL.

It fails loudly rather than silently, but a new user has no way to know example.com is reserved, and the block at getting-started.md:73-79 is presented as a complete copy-paste quick-start. This PR's own harness confirms the diagnosis at scripts/template-cache-local-test.sh:184-186, with the comment "The example publisher domains are reserved placeholders that validation rejects."

localhost/localhost is the pair fixtures/configs/trusted-server.integration.toml:6-7 already uses, and validate_cookie_domain (settings.rs:3345) rejects only ;, \n and \r, so the dotless form is fine. The env overlay replaces existing scalar leaves only, and both keys exist in the template, so the override lands.

Suggested change
# [publisher]
TRUSTED_SERVER__PUBLISHER__ORIGIN_URL=http://localhost:9090
# [publisher]
# The starter config ships `example.com`/`.example.com`, which are reserved
# placeholders that both `ts config push` and runtime validation reject.
TRUSTED_SERVER__PUBLISHER__DOMAIN=localhost
TRUSTED_SERVER__PUBLISHER__COOKIE_DOMAIN=localhost
TRUSTED_SERVER__PUBLISHER__ORIGIN_URL=http://localhost:9090

Comment on lines +70 to +77
1. Populate the physical store mapped from `trusted_server_secrets` with the
existing credential values without printing them in shell history, logs, or
CI output.
2. Replace each active credential value with a stable key name and remove the
legacy Tinybird, DataDome, and S3 `secret_store` selectors.
3. Run `ts config validate`, then `ts config push --adapter fastly --no-diff`.
4. Restart/redeploy instances as needed to load the new values. Rotation is
startup-scoped; changing a store value does not alter already-built state.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — This migration runbook never creates or links the physical store, so an operator following it verbatim takes the service down.

Step 1 says "the physical store mapped from trusted_server_secrets", but nothing in this section creates that mapping, and the only prose about it (:63-66) is descriptive rather than actionable — "An adapter can map the logical ID… Fastly commonly maps…". There is no ts provision, no EDGEZERO__STORES__SECRETS__TRUSTED_SERVER_SECRETS__NAME export, no resource-link step, and no cross-link to fastly.md#secret-stores, where all three are documented correctly.

It fails silently at the mapping layer and then hard at startup. env_config_from_runtime_dictionary treats a missing edgezero_runtime_env as optional and returns an empty EnvConfig with only a log::warn! (edgezero-adapter-fastly/src/lib.rs:189-203), so secret_store_name falls back to the logical trusted_server_secrets. This repo's actual store is ts_secrets, so every lookup misses, resolve_leaf errors (secret_resolution.rs:165-172), settings never build, and the service serves its startup-error response.

The suggestion below replaces the whole list rather than just the first item, because the renumbering and the blank line before item 2 are both required — without them the pinned prettier rejects the file and format-docs (a required check) fails. I verified this exact replacement passes prettier --check.

Suggested change
1. Populate the physical store mapped from `trusted_server_secrets` with the
existing credential values without printing them in shell history, logs, or
CI output.
2. Replace each active credential value with a stable key name and remove the
legacy Tinybird, DataDome, and S3 `secret_store` selectors.
3. Run `ts config validate`, then `ts config push --adapter fastly --no-diff`.
4. Restart/redeploy instances as needed to load the new values. Rotation is
startup-scoped; changing a store value does not alter already-built state.
1. Create the physical store and persist its mapping, so the runtime resolves
`trusted_server_secrets` to the store you populate in the next step. On
Fastly this is one command, and the linking requirements are in
[Fastly setup](/guide/fastly#secret-stores):
```bash
export EDGEZERO__STORES__SECRETS__TRUSTED_SERVER_SECRETS__NAME=ts_secrets
ts provision --adapter fastly
```
Skipping this leaves the runtime looking for a store literally named
`trusted_server_secrets`; every lookup then misses and startup fails closed.
2. Populate the physical store mapped from `trusted_server_secrets` with the
existing credential values without printing them in shell history, logs, or
CI output.
3. Replace each active credential value with a stable key name and remove the
legacy Tinybird, DataDome, and S3 `secret_store` selectors.
4. Run `ts config validate`, then `ts config push --adapter fastly --no-diff`.
5. Restart/redeploy instances as needed to load the new values. Rotation is
startup-scoped; changing a store value does not alter already-built state.

Comment on lines +160 to +162
Provision the physical store mapped from logical `trusted_server_secrets` with
the existing credential values before pushing a migrated config. On Fastly,
`ts_secrets` is the documented example physical name. Then validate and push:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — Same omission as the migration runbook, on the deploy path.

"Provision the physical store mapped from logical trusted_server_secrets" states the outcome, not an instruction: it names no command and links nowhere. The link line at :170-171 points at /guide/configuration and /guide/cli, and neither carries the provision step either — see the comment on configuration.md:70. The consequence is the same fail-closed startup outage.

Apply manually — the fix pairs with the configuration.md change, so it is not a self-contained single-file suggestion. Append to that sentence:

See [Fastly setup](/guide/fastly#secret-stores) for the `ts provision` command and
the service-link requirements.


[[ec.partners]]
name = "Mocktioneer SSP"
source_domain = "formally-vital-lion.edgecompute.app"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏕 camp site — This is a real deployed Fastly Compute hostname, and the same PR already uses a fictional form for the same partner elsewhere.

formally-vital-lion.edgecompute.app appears at :31, :57, :59, :78, :151, :154 and :187. *.edgecompute.app is a live Fastly Compute domain, which CLAUDE.md's "example or fictional information only" rule covers. Meanwhile docs/guide/configuration.md:596 — added in this PR — uses source_domain = "mocktioneer.example" for the same partner, so the PR is inconsistent with itself.

These are context lines rather than added lines, so it predates this PR. Flagging it because the PR edits their immediate neighbours and a secrets/config-hardening PR is the natural moment. Apply manually — seven occurrences; a file-wide s/formally-vital-lion.edgecompute.app/mocktioneer.example/g would align it with configuration.md.

Comment on lines +1099 to +1100
| `type` | String | Yes | none | Must be `s3_sigv4` |
| `region` | String | Yes | none | AWS region used in the SigV4 credential scope |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📌 out of scope — The "Default key" column and the example directly below it disagree, which reads like an error even though it is not.

The table keeps access_key_id / secret_access_key while the example uses s3_access_key_id / s3_secret_access_key. The table is correct — it matches default_s3_access_key_id() at settings.rs:752-754 — but a reader comparing the two will assume one of them is wrong. Worth a word distinguishing "the default key name if you omit it" from "the key name used in this example".

Comment on lines +220 to +229
impl std::fmt::Debug for IntegrationSettings {
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let mut integration_ids = self.entries.keys().collect::<Vec<_>>();
integration_ids.sort_unstable();
formatter
.debug_struct("IntegrationSettings")
.field("integration_ids", &integration_ids)
.finish()
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — This hand-written Debug closes a leak that Redacted could not have caught.

entries: HashMap<String, JsonValue> holds the raw DataDome integration JSON after resolution, so the values sit there as bare JsonValue with no Redacted wrapper to help — a derived Debug would have printed resolved secrets verbatim. Pinned by settings_debug_redacts_resolved_static_credentials (config.rs:832). Easy thing to miss, and it was not missed.

Comment on lines +1394 to +1435
#[test]
fn hooks_store_metadata_matches_edgezero_manifest() {
let manifest: toml::Value = toml::from_str(include_str!("../../../edgezero.toml"))
.expect("should parse edgezero manifest");
let manifest_stores = manifest
.get("stores")
.and_then(toml::Value::as_table)
.expect("manifest should declare stores");
let metadata = TrustedServerApp::stores();

for (kind, runtime_store) in [
(
"config",
metadata.config.expect("should declare config stores"),
),
("kv", metadata.kv.expect("should declare KV stores")),
(
"secrets",
metadata.secrets.expect("should declare secret stores"),
),
] {
let manifest_store = manifest_stores
.get(kind)
.and_then(toml::Value::as_table)
.unwrap_or_else(|| panic!("manifest should declare {kind} stores"));
let manifest_default = manifest_store
.get("default")
.and_then(toml::Value::as_str)
.unwrap_or_else(|| panic!("manifest {kind} stores should declare a default"));
let manifest_ids = manifest_store
.get("ids")
.and_then(toml::Value::as_array)
.unwrap_or_else(|| panic!("manifest {kind} stores should declare ids"))
.iter()
.map(toml::Value::as_str)
.collect::<Option<Vec<_>>>()
.unwrap_or_else(|| panic!("manifest {kind} store ids should be strings"));

assert_eq!(runtime_store.default, manifest_default);
assert_eq!(runtime_store.ids, manifest_ids);
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praisehooks_store_metadata_matches_edgezero_manifest pins exactly the right invariant.

The runtime dictionary key is derived from the store id in stores(), so an id that drifts away from edgezero.toml would silently reintroduce the very bug this PR fixes — with no compile error and no test failure. Asserting stores() against the manifest via include_str! closes that, needs no filesystem access under Viceroy, and follows the repo's include_str!-over-codegen precedent for checked-in assets.

Comment on lines 38 to 60
@@ -36,15 +48,88 @@ pub fn settings_from_config_blob(
.attach(error.to_string())
})?;

let settings = Settings::from_json_value(envelope.into_data())?;
settings.reject_placeholder_secrets()?;
let mut data = envelope.into_data();
remove_inactive_secret_references(&mut data);
resolve_secret_references::<TrustedServerAppConfig>(
&mut data,
secret_store,
default_secret_store_name,
)?;
let settings = Settings::from_json_value(data)?;
crate::config::validate_settings_for_runtime(&settings)?;
Ok(settings)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — Centralising the whole order of operations in one function is what makes the four-adapter story reviewable.

envelope.verify()into_data() → strip inactive references → resolve → deserialize → validate_settings_for_runtime, with every adapter funnelling through here. No adapter can resolve against unverified bytes, and tampered_blob_hash_is_rejected pins the ordering. Combined with the clone-then-swap in secret_resolution.rs, a partial resolution can never reach Settings. Auditing this took one read instead of four.

aram356 added a commit that referenced this pull request Sep 3, 2026
# Conflicts:
#	crates/trusted-server-adapter-fastly/src/app.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add secret-store backed config references for secret values

3 participants