Add secret-store config references spec#715
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
Docs-only PR adding a v1 design spec for secret-store backed config references. The proposal is well-scoped (no auto-generation, no handlers.id, no request-signing migration) and the inline/ref dichotomy with strict production gating is a clean separation. Two correctness gaps and two open semantics questions block before merge; the rest are improvements to harden the spec against ambiguous implementation choices.
Blocking
🔧 wrench
- Hash determinism: shorthand and explicit-default refs hash differently for the same logical secret (see inline at §"Config hashing and canonicalization").
- Distinct-names validation:
providers.fastly.secrets.store_namemust be reconciled with the CLI's existingvalidate_distinct_names(see inline at §"Fastly provider configuration").
❓ question
- Production boundary: which exact code path enforces inline-secret rejection at the canonical-payload write boundary, not just the
applycommand boundary? - Fail-closed HTTP semantics: 401 vs 503 (and the same for
proxy_secret-backed endpoints)?
Non-blocking
📌 out of scope
handlers.iddeferral: open question #3 leaveshandlers.idfor a later version. Without it, the operator-chosen secret key is the only stable link between a[[handlers]]entry and its secret; reordering the TOML is fine, but renames/duplicates break the link. Worth a follow-up issue so v1.x can decide stability semantics before the user surface ossifies.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
| proxy_secret = { secret = "publisher/proxy_secret_v2" } | ||
| ``` | ||
|
|
||
| must change the config hash. |
There was a problem hiding this comment.
🔧 wrench — Hash determinism: shorthand and explicit-default refs resolve to the same secret but hash differently.
{ secret = "publisher/proxy_secret" } and { store = "ts_secrets", key = "publisher/proxy_secret" } both point at the same physical secret, but as written they produce different canonical TOML bytes and therefore different config_hash values. Attestation/diff tooling will report spurious config changes when an operator normalizes shorthand → explicit (or back).
Fix: either (a) require canonicalization to normalize one form to the other before hashing, or (b) state explicitly that the two forms are intentionally non-equivalent for hashing and document why. Either way, add the chosen rule to Acceptance Criteria.
| ```toml | ||
| [providers.fastly.secrets] | ||
| store_name = "customer_ts_secrets" | ||
| ``` |
There was a problem hiding this comment.
🔧 wrench — providers.fastly.secrets.store_name must be added to the CLI's validate_distinct_names.
crates/trusted-server-cli/src/config.rs:213-226 already enforces distinct Fastly Secret Store resource names across signing_secret_store_name and runtime_api_secret_store_name. Adding a third Secret Store without extending that check lets two different runtime aliases (ts_secrets, signing_keys, api-keys) silently link the same underlying resource, collapsing the isolation contract the spec assumes between application secrets and request-signing/runtime-API secrets.
Fix: spec should require provisioning to reject overlap across all three secret-store resource names, and Acceptance Criteria should add a checkbox for it.
|
|
||
| There is no v1 `--allow-inline-secrets` production escape hatch. | ||
|
|
||
| Rationale: local developers should not be uploading local inline config to production, and production provisioning should prevent accidental plaintext secret upload into the canonical runtime config store. |
There was a problem hiding this comment.
❓ question — Production boundary: where exactly is inline-secret rejection enforced?
The spec gates inline-secret rejection at ts provision fastly apply. But the canonical TOML payload is what actually lands in the Config Store. Is ts provision fastly apply the only code path that writes the canonical app config payload to a Fastly Config Store? If a future ts deploy, manual upload, or alternate CLI path can write that payload, inline plaintext can still reach production.
Clarify: is the gate at the command boundary sufficient, or should it also live at the payload-write boundary (the function that serializes + uploads canonical TOML)?
| | `publisher.proxy_secret` | First-party proxy signing, encryption/decryption, and URL-rewrite behavior requiring the proxy secret should fail or be disabled. Unrelated routes continue. | | ||
| | `edge_cookie.secret_key` | Edge Cookie generation/validation should fail or be disabled. Unrelated request handling continues. | | ||
|
|
||
| The exact HTTP status codes can be implementation-specific, but behavior must not silently skip security checks or generate weak fallback secrets. |
There was a problem hiding this comment.
❓ question — Fail-closed HTTP semantics: 401 vs 503?
"Deny access or return an auth/config error" is ambiguous. Today, crates/trusted-server-core/src/auth.rs returns 401 with WWW-Authenticate for failed basic auth. A probe receiving 401 can't distinguish "route exists, wrong creds" from "route is misconfigured"; 503/500 reveals "this route is broken". The choice has security (oracle attacks) and ops (paging signal) consequences.
Same question for proxy_secret-dependent endpoints (encrypt/decrypt URL): 4xx (treat as unsigned) or 5xx (refuse to serve)?
Lock specific status codes / failure modes into the spec; downstream callers will rely on them.
|
|
||
| ```text | ||
| ts_secrets | ||
| ``` |
There was a problem hiding this comment.
♻️ refactor — Alias naming inconsistency: ts_secrets vs ts_config_store.
Existing aliases: ts_config_store (crates/trusted-server-core/src/runtime_config.rs:22) for the application Config Store; signing_keys/api-keys for the existing Secret Stores. The new alias breaks both patterns: it's neither ts_<resource>_store (which would be ts_secret_store) nor an unprefixed purpose-name (which would be app_secrets/application_secrets).
Pick one convention and justify in the spec. ts_secret_store would parallel ts_config_store cleanly.
| 7. Continue serving with the resolved snapshot. | ||
|
|
||
| Missing secret refs at runtime must not make the entire service fail config loading. This protects production availability when a secret is deleted or changed out-of-band through the Fastly UI/CLI after a successful provisioning flow. | ||
|
|
There was a problem hiding this comment.
🤔 thinking — "Eager but non-fatal" + no retry = persistent fail-closed until next snapshot.
The runtime section reads as decisive ("eager per loaded config snapshot"); open question #6 punts retry. Operationally this is "affected feature stays fail-closed indefinitely until the next config refresh / cold start." That's defensible for v1, but the spec should state this explicitly here instead of leaving it to readers to infer from the open question list.
| - parsed/raw settings can contain inline values or refs | ||
| - resolved settings know whether each secret is available | ||
| - logging/debug output never prints resolved secret values | ||
|
|
There was a problem hiding this comment.
📝 note — Debug/Display semantics for SecretRef.
Redacted<T> redacts in Debug/Display. SecretRef carries store/key strings, which are operational metadata, not secret material — but readers will reasonably wonder whether they leak in logs. Spec should state explicitly that SecretRef Debug/Display is non-redacting (paths only), and ResolvedSecretString::Unavailable { reference } is safe to log.
| - Unknown keys inside ref tables are rejected. | ||
| - Empty strings are rejected for inline secret values. | ||
| - Empty `secret`, `store`, or `key` values are rejected. | ||
| - Mixed ref shapes are rejected. |
There was a problem hiding this comment.
📝 note — Tie SecretString validation into the existing validator::Validate pattern.
Existing fields use validator::Validate with custom validators like validate_redacted_not_empty (crates/trusted-server-core/src/settings.rs:332-335) and EdgeCookie::validate_secret_key (crates/trusted-server-core/src/settings.rs:282-286). The spec should note that SecretString validation slots into this pattern (custom validator for SecretString::Inline empty-check, parse-time rejection for ref shape errors) rather than introducing a parallel validation framework.
| - missing refs log warnings and mark the field unavailable | ||
| - affected features fail closed | ||
|
|
||
| `ts config validate` may warn when inline v1 secrets are present, but inline dev config remains valid. |
There was a problem hiding this comment.
⛏ nitpick — "may warn" should be "warns".
Production rejects inline v1 secrets; local may warn is too soft. Make the local-dev contract a definite warning (still permitting the inline value) so the two layers tell the same story.
| - [ ] Do not add automatic secret generation in v1. | ||
| - [ ] Do not add `handlers.id` in v1. | ||
| - [ ] Do not migrate request-signing secrets into the generic secret-ref model in v1. | ||
| - [ ] Document local/dev inline-secret support and production ref requirements. |
There was a problem hiding this comment.
⛏ nitpick — Acceptance Criteria gaps.
Add checkboxes for:
- the ref canonicalization rule (matches the hash-determinism wrench)
- extending
validate_distinct_namesto includeproviders.fastly.secrets.store_name - rejecting refs whose explicit
storealias has not been provisioned (mentioned in §"Provisioning validation" body, missing from criteria)
Summary
Changes
docs/superpowers/specs/2026-05-19-secret-store-config-references-design.mdCloses
Closes #714
Related #684
Test plan
cargo test --workspace --exclude trusted-server-clicargo test --package trusted-server-cli --target "$(rustc -vV | sed -n 's/^host: //p')"cargo clippy --workspace --exclude trusted-server-cli --all-targets --all-features -- -D warningscargo clippy --package trusted-server-cli --target "$(rustc -vV | sed -n 's/^host: //p')" --all-targets -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run format -- superpowers/specs/2026-05-19-secret-store-config-references-design.mdcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)