Add native secret-store config resolution - #1036
Conversation
|
@ChristianPavilonis to test it before merging into #1019 |
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
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
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
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
| The referenced value is resolved from `trusted_server_secrets` at startup. It | ||
| must be at least 32 bytes, so generate it with a cryptographically secure random | ||
| source. Keep that value confidential, rotate it only intentionally, and never | ||
| put it in the TOML file or pushed app-config blob. |
There was a problem hiding this comment.
⛏ nitpick — This still promises a hard requirement that 76f6f13 removed: MIN_PROXY_SECRET_LENGTH and validate_proxy_secret_strength are gone, and config_payload.rs now has a test asserting a short resolved proxy secret loads successfully. proxy-signing.md was already softened to "recommended" — this section should match, or operators will expect a startup failure that no longer happens.
| The referenced value is resolved from `trusted_server_secrets` at startup. It | |
| must be at least 32 bytes, so generate it with a cryptographically secure random | |
| source. Keep that value confidential, rotate it only intentionally, and never | |
| put it in the TOML file or pushed app-config blob. | |
| The referenced value is resolved from `trusted_server_secrets` at startup. | |
| Generate it with a cryptographically secure random source; at least 32 random | |
| bytes are recommended. Keep that value confidential, rotate it only | |
| intentionally, and never put it in the TOML file or pushed app-config blob. |
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 ...")