Repository navigation
Bind credentials to approved contexts and add secure provisioning - #1453
ChristianPavilonis wants to merge 15 commits into
Conversation
Document the selected design for issue #1037 using raw native secret values and purpose/identity/destination-derived lookup keys. Include the independent comparison's required normalization, CLI/native lookup, private-construction, retirement, and send-policy amendments. This is a design-only change; implementation and runtime proof remain.
Require explicit Spin manifest paths before secret planning and checks so native declaration validation and plan fingerprints use the selected manifest. Let Cargo check default fixture-generator freshness while preserving explicit executable overrides. Add CLI and shell regressions for both fixes.
c131cc7 to
67c5bee
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Merge base changes while preserving credential bindings, safe diagnostics and rewrite compatibility; provision generated secrets in EOF runtime fixtures.
…ve CodeQL false positives without changing runtime behavior.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This replaces operator-chosen secret references with context-derived bindings (#1037), adds ts secrets provisioning, and moves all four adapters onto EdgeZero's implicit-secret hooks. I found no path that exports a credential outside its loaded binding, and the derived keys match an independent implementation of the spec's golden vector. Changes are needed before merge: the EdgeZero pin rolls main's dependencies back, main's new forwarder credential has no binding role, two diagnostics were lost, pull tokens gained an undocumented 32-byte minimum, and the migration docs and --stdin path have operator hazards.
12 of the inline comments below carry a one-click GitHub
suggestion; use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The other comments describe the fix in prose because it touches several files or lines outside the diff. Each suggestion was applied alone to8bb27824eand checked with the gates named in its comment, then all twelve were applied together and the same gates rerun.
Blocking
🔧 wrench
- EdgeZero pin rolls main's dependencies back: see inline at
Cargo.toml:59 - Main's
trusted_forwarder.shared_secretneeds a binding role: see cross-cutting below - Config errors collapsed into one generic message: see inline at
crates/trusted-server-core/src/config.rs:133(suggestion) ts secrets planhides the removed-reference path: see inline atcrates/trusted-server-cli/src/commands/secrets/mod.rs:216(suggestion)- Pull tokens newly need 32 bytes: see inline at
crates/trusted-server-core/src/secret_bindings.rs:1170 - Migration guide misses read-back and trimming hazards: see inline at
docs/guide/secret-binding-migration.md:89(suggestion) --stdinkeeps trailing newlines and echoes on a terminal: see inline atcrates/trusted-server-cli/src/commands/secrets/mod.rs:163(suggestion)
Non-blocking
♻️ refactor
- Binding plan recompiled per request: see inline at
crates/trusted-server-core/src/ec/registry.rs:148 - Five security mutants survive the core suite: see inline at
crates/trusted-server-core/src/ec/registry.rs:544 - Pull request method unpinned: see inline at
crates/trusted-server-core/src/ec/pull_sync.rs:898(suggestion) - DataDome forced TLS and bound host unpinned: see inline at
crates/trusted-server-core/src/integrations/datadome/protection.rs:961(suggestion) - CLI tests depend on the ambient environment: see inline at
crates/trusted-server-cli/src/commands/secrets/tests.rs:632 plan/check/setskip push-time validation: see inline atcrates/trusted-server-cli/src/commands/secrets/mod.rs:212checkexit codes and per-entry store listing: see inline atcrates/trusted-server-cli/src/commands/secrets/mod.rs:401- Fastly bad-certificate case may not detect an app regression: see inline at
scripts/secret-bindings-runtime.mjs:320 - S3 fixture values shared across routes: see inline at
crates/trusted-server-integration-tests/src/lib.rs:63 - SIGTERM test has no grandchild: see inline at
scripts/secret-bindings-runtime.test.mjs:73 - Spin encoder tests exercise a test-only copy: see inline at
crates/trusted-server-adapter-spin/src/platform.rs:136 - Newline guidance for non-bearer roles: see inline at
docs/guide/secrets.md:244(suggestion) - CI runner must control
curlonPATH: see inline atdocs/guide/secrets-ci.md:57(suggestion)
🤔 thinking
LoadedConfigis discarded at load: see inline atcrates/trusted-server-core/src/config_payload.rs:28
⛏ nitpick
- CHANGELOG link and loose list: see inline at
CHANGELOG.md:12(suggestion) - Stale Axum release-gate sentence: see inline at
docs/guide/axum-dev.md:41(suggestion) - Broken Axum setup anchor: see inline at
docs/guide/secrets.md:177(suggestion) ${name^^}fails in Bash 3.2: see inline atdocs/guide/secrets.md:222(suggestion)
Cross-cutting / body-level findings
- 🔧 Merge main, and give
trusted_forwarder.shared_secreta binding role. The branch conflicts with main in 13 files. Main's #1107 added[trusted_forwarder] shared_secret, which uses the operator-chosen key-name model this PR removes (main'strusted-server.example.toml: "shared_secret is a key in trusted_server_secrets, not the credential itself"). If the merge keeps that field as written, a config-only writer can point it at another purpose's key, which is the substitution #1037 closes. The merge needs a 13th implicit role: a descriptor and scope (for exampleforwarder.shared, subject App, export None) that is active when[trusted_forwarder]is present; the old field added toreject_removed_references; validation matching main'svalidate_trusted_forwarder_shared_secret; ats secretsselector; docs and migration-table rows; and the "twelve" counts in tests, docs and the CHANGELOG updated. - 📌 Spec and plan still describe unfinished work. The design spec calls the schema "the proposed breaking schema, not configuration accepted by today's binary" (line 68) and the CLI "proposed, not available today" (line 263), and its status paragraph (line 7) still lists native HTTPS/no-follow behavior as an open release gate. The plan says the two highest-risk facts "remain unproven until implementation executes" (line 272), and its verification step runs
cargo test -p trusted-server-core secret_resolution(line 214), which matches no tests now thatsecret_resolution.rsis gone. - 📌 Guide gaps.
docs/guide/error-reference.mdhas no entries for the new load errors (removed credential reference,required resolved binding unavailable,resolved binding context mismatch). Thedocs/guide/cli.mdcommand index has nots secretsrows.docs/guide/proxy-signing.md:33still says "Compute expected tstoken using proxy_secret". - 📌 Undocumented behavior changes. A DataDome test-bypass credential shorter than 32 bytes used to be ignored with a warning (base test
short_test_bypass_credential_is_ignored_without_failing_startup); it now fails the whole config load, so it belongs under Breaking. Separately,--manifestnow defaults to$EDGEZERO_MANIFESTbeforeedgezero.toml(crates/trusted-server-cli/src/app_config.rs:31), which also changes which configts auditandts config ad-templatesresolve; that needs a CHANGELOG line. - 📌 CI supply chain and diagnostics (
.github/workflows/test.yml). The Spin tarball is downloaded without a checksum check (lines 302-308).wrangleris installed withnpm install --no-save, outside any lockfile (line 242).cargo install worker-buildrebuilds from source on every run with no cache (line 241). No step uploads the secret-bindings harness logs (runtime.log) on failure, on any adapter. - 📌 Harness hygiene.
scripts/secret-bindings-runtime.sh:29rebuildstrusted_server_adapter_spin.wasmwithtest-utilsinto the samewasm32-wasip1/releasepath where CI built the shipping artifact (test.yml:295); a separate target dir avoids that.scripts/test-fixture-generator.shchecks generator freshness, but no workflow runs it, so the generatedts1_declarations infastly.toml,crates/trusted-server-adapter-spin/spin.tomlandcrates/trusted-server-adapter-cloudflare/wrangler.ci.tomlcan drift from the compiler without CI noticing. - 📌 Dead legacy overlays.
.github/actions/setup-integration-test-env/action.yml(lines 90-91, 102-103 and 133-134) and.github/workflows/test.yml:292-293still exportTRUSTED_SERVER__PUBLISHER__PROXY_SECRETandTRUSTED_SERVER__EC__PASSPHRASE. Both name properties this PR removes, so they have no effect on these builds and point readers at a configuration path that no longer exists. - 🌱 Reserve
ts1_on the signing read path too.request_signing/endpoints.rs:262-267refusests1_kids on create and rotate because Cloudflare's secret namespace is flat, butrequest_signing/signing.rs:146loads whatever kidcurrent-kidinjwks_storenames. A writer who can change that config entry can point signing at an app credential's key. Applying the same check beforeget_bytescloses it. - ⛏ Small items. Pull-sync warnings no longer name the partner (for example "Pull sync: dispatch failed" in
ec/pull_sync.rs), which makes one broken partner hard to find, andsource_domainisn't secret.read_config_entry(settings_data.rs:94-97) drops the store and key from its error, though neither is secret.compile_bindingsis restructured around a CodeQL 2.27.3 false positive (secret_bindings.rs:802-806,#[allow(clippy::question_mark)]); dismissing the alert would keep?. The seven newexpectmessages incrates/trusted-server-adapter-axum/src/platform_https_tests.rsdon't follow the"should ..."form, and thevalidate_credentialasserts insecret_bindings_tests.rscarry no messages. Inec/pull_sync.rs,validated_pull_sync_url_rejects_http_schemeandvalidated_pull_sync_url_rejects_non_allowlisted_host(lines 667 and 679) now pass because the partner has no loaded binding, not because of the scheme or host; the checks they name live innormalize_pull_endpoint.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test: PASS (required)
- CLAUDE.md symlink guard: PASS
- Analyze (javascript-typescript): PASS
- Analyze (rust): PASS
- cargo test (cross-adapter parity): PASS
- ts dev lint domains (changed lines): PASS
- HTML EOF runtime (Fastly): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- cargo test (axum native): PASS
- Analyze (actions): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- docs-proposal scripts: PASS
- Analyze (python): PASS
- format-docs: PASS (required)
- cargo fmt: PASS (required)
- CI Node pin guard: PASS
- Analyze (javascript-typescript): PASS
- PBS Terraform and Compose checks: PASS
| # The implicit-secret APIs require this unreleased EdgeZero revision. Keep | ||
| # fastly, log-fastly, spin-sdk and validator aligned with it until that branch | ||
| # incorporates EdgeZero's dependency upgrade. |
There was a problem hiding this comment.
🔧 wrench: Merging this pin rolls main's dependency stack back.
4f645b1e7 branches from EdgeZero v0.0.9. It doesn't contain v0.0.10's dependency refresh, and it is 14 commits behind its own companion PR (IABTechLab/edgezero#444, head c199355e, which is already rebased onto v0.0.11 and is still a draft). Main is on tag = "v0.0.11", so merging this as-is moves main backwards:
| crate | main | this PR |
|---|---|---|
fastly |
0.13 |
0.12 |
log-fastly |
0.13 |
0.12 |
spin-sdk |
~7.0 |
~6.0 |
validator |
0.21 |
0.20 |
It also drops main's [patch.crates-io] esi override.
Fix (apply manually; this touches Cargo.toml and Cargo.lock together): rebase the EdgeZero work onto #444's v0.0.11-based head, restore main's versions and the esi patch, and pin a released EdgeZero tag once #444 lands instead of merging on a git rev.
| let mut settings: Settings = serde_json::from_value(value).map_err(|_| { | ||
| serde::de::Error::custom("invalid Trusted Server configuration structure") | ||
| })?; |
There was a problem hiding this comment.
🔧 wrench: Every structural config error now reads invalid Trusted Server configuration structure.
This deserializer runs for ts config validate, push and diff, and for runtime loading, before any credential is resolved. With a domian typo under [publisher], the PR head reports only that generic line; base reported unknown field `domian`, expected one of …. Operators lose the field name, the expected set, and type-mismatch details. The unresolved document holds feature settings, enable flags and derived lookup names but no credential values, and removed or shadow credential properties are rejected by path afterwards, so serde's message is safe to show here. The collision test that pins the generic message goes through the normalize path at line 138 and still passes.
| let mut settings: Settings = serde_json::from_value(value).map_err(|_| { | |
| serde::de::Error::custom("invalid Trusted Server configuration structure") | |
| })?; | |
| // Unresolved config carries no credentials, so the serde message is safe to show. | |
| let mut settings: Settings = | |
| serde_json::from_value(value).map_err(serde::de::Error::custom)?; |
Verified alone on the PR head: cargo fmt --all -- --check, all six adapter clippy aliases, the native core lib suite, test-fastly, test-fastly-reuse, test-axum, test-cloudflare, test-spin, the parity suite, template_build_digest, clippy-cli, and the CLI suite pass.
| let loaded = load_settings(&target.config) | ||
| .map_err(|_| "could not load effective unresolved app configuration".to_owned())?; | ||
| let bindings = compile_bindings(&loaded.settings) | ||
| .map_err(|_| "could not compile active credential bindings".to_owned())?; |
There was a problem hiding this comment.
🔧 wrench: ts secrets plan hides the error that tells an operator what to migrate.
With proxy_secret still in trusted-server.toml, ts secrets plan --adapter axum prints only could not load effective unresolved app configuration. load_settings already returns a user-facing string, and compile_bindings errors carry schema paths only, so forwarding both leaks nothing. With this change the same probe prints … publisher.proxy_secret: removed credential reference; migrate configuration, which is exactly what a migrating operator needs to see.
| let loaded = load_settings(&target.config) | |
| .map_err(|_| "could not load effective unresolved app configuration".to_owned())?; | |
| let bindings = compile_bindings(&loaded.settings) | |
| .map_err(|_| "could not compile active credential bindings".to_owned())?; | |
| let loaded = load_settings(&target.config).map_err(|error| { | |
| format!("could not load effective unresolved app configuration: {error}") | |
| })?; | |
| let bindings = compile_bindings(&loaded.settings) | |
| .map_err(|error| format!("could not compile active credential bindings: {error}"))?; |
Verified alone on the PR head: cargo fmt --all -- --check, cargo clippy-cli, and the full trusted-server-cli test suite pass.
| if matches!( | ||
| scope, | ||
| "ecpass" | "ecin" | "ecpull" | "clientip" | "ddbypass" | ||
| ) && bytes.len() < 32 | ||
| { | ||
| return Err(invalid(path, "credential below minimum length")); | ||
| } |
There was a problem hiding this comment.
🔧 wrench: Partner pull tokens now need at least 32 bytes, which base never required.
ecpull is a token the partner issues and Trusted Server sends outbound, so the operator can't choose its length. Base only required it to be non-empty ("ts_pull_token must not be empty when pull sync is enabled"), and base tests used the 14-byte outbound-token. The design spec says pull tokens need "nonempty RFC 6750 b64token values plus existing length/placeholder checks" (line 187). Because enabled credentials load as one snapshot, one partner with a shorter token now blocks provisioning and stops the whole config from loading. Neither the CHANGELOG nor the migration guide mentions it.
Proposed fix (apply manually; it touches this file and secret_bindings_tests.rs):
if matches!(scope, "ecpass" | "ecin" | "clientip" | "ddbypass") && bytes.len() < 32 {and in secret_bindings_tests.rs, replace the short-ecpull assertion at line 562 with:
assert!(validate_credential("ecpull", b"short").is_ok());
assert!(validate_credential("ecin", b"short").is_err());Verified on the PR head: cargo fmt --all -- --check, the core lib suite (3186 passed) and the CLI secrets tests pass. If the 32-byte floor is intended, keep it, but list it under Breaking in the CHANGELOG and the migration guide.
|
|
||
| Use `--adapter axum`, `cloudflare`, or `spin` for native rendering. Review all generated names, declarations, service/store IDs, contexts, and versions. Retain the secret-free plan with the release record. Plan checksums detect drift, not approval. | ||
|
|
||
| 5. Supply existing raw credential bytes securely to the generated bindings using [interactive Fastly provisioning](./secrets#first-fastly-setup-with-tinybird) or [native provisioning](./secrets#axum-native-environment-input). No JSON wrapping is required. Do not copy legacy names into new TOML. If validation rejects a former credential, arrange a recipient-side replacement rather than trimming bytes or weakening validation. Provision each required role, including separate inbound/pull tokens and all active S3 roles. |
There was a problem hiding this comment.
🔧 wrench: This step leaves out two hazards that can turn the migration into an outage.
- Fastly secret-store entries and Cloudflare Worker secrets can't be read back. An operator who no longer has the source value for
ec.passphraseorpublisher.proxyis doing a rotation, not a migration: passphrase-derived EC IDs change, and outstanding encrypted proxy and click URLs stop decoding. - Base trimmed resolved S3, Tinybird and DataDome server credentials. This PR preserves bytes, and two tests pin that on purpose. A stored value with a trailing newline that works today fails after the upgrade.
| 5. Supply existing raw credential bytes securely to the generated bindings using [interactive Fastly provisioning](./secrets#first-fastly-setup-with-tinybird) or [native provisioning](./secrets#axum-native-environment-input). No JSON wrapping is required. Do not copy legacy names into new TOML. If validation rejects a former credential, arrange a recipient-side replacement rather than trimming bytes or weakening validation. Provision each required role, including separate inbound/pull tokens and all active S3 roles. | |
| 5. Supply existing raw credential bytes securely to the generated bindings using [interactive Fastly provisioning](./secrets#first-fastly-setup-with-tinybird) or [native provisioning](./secrets#axum-native-environment-input). No JSON wrapping is required. Do not copy legacy names into new TOML. Fastly secret-store entries and Cloudflare Worker secrets cannot be read back, so take each value from its source of record. If an `ec.passphrase` or `publisher.proxy` value is unrecoverable, migration becomes a rotation: passphrase-derived EC IDs change and outstanding encrypted first-party proxy and click URLs stop decoding. Values are no longer trimmed. The previous binary trimmed resolved S3, Tinybird, and DataDome server credentials, so a stored value with surrounding whitespace or a trailing newline that worked before now fails validation or its outbound request. If validation rejects a former credential, arrange a recipient-side replacement rather than trimming bytes or weakening validation. Provision each required role, including separate inbound/pull tokens and all active S3 roles. |
Verified alone on the PR head: npm run format in docs/ passes.
If you'd rather fail fast in code, rejecting edge whitespace for ddserver and s3secret at validation time also works, but it means changing the two tests that pin byte preservation.
|
|
||
| ## Protected GitHub Actions example | ||
|
|
||
| This example belongs in the separately controlled provisioning repository. It assumes an isolated self-hosted runner labelled `secret-writer` already has a reviewed immutable `ts` executable at `/opt/trusted-server/bin/ts`, `jq`, and Bash. Do not share that runner with untrusted builds. Replace the approved revision and CLI digest through administrator-controlled Environment variables before use; the checks reject empty or malformed values. This is a workflow template, not an executed CI rehearsal. |
There was a problem hiding this comment.
♻️ refactor: The runner prerequisites list jq, but the pinned EdgeZero Fastly transport runs curl from PATH (Command::new("curl") in edgezero-adapter-fastly/src/cli.rs at 4f645b1e7) and hands it FASTLY_API_TOKEN plus the credential on stdin. On a secret-writing runner, whoever controls PATH controls that binary.
| This example belongs in the separately controlled provisioning repository. It assumes an isolated self-hosted runner labelled `secret-writer` already has a reviewed immutable `ts` executable at `/opt/trusted-server/bin/ts`, `jq`, and Bash. Do not share that runner with untrusted builds. Replace the approved revision and CLI digest through administrator-controlled Environment variables before use; the checks reject empty or malformed values. This is a workflow template, not an executed CI rehearsal. | |
| This example belongs in the separately controlled provisioning repository. It assumes an isolated self-hosted runner labelled `secret-writer` already has a reviewed immutable `ts` executable at `/opt/trusted-server/bin/ts`, a reviewed `curl`, and Bash. The Fastly transport runs `curl` from `PATH` and passes it `FASTLY_API_TOKEN` and the credential on stdin, so set an administrator-controlled `PATH` for the job. Do not share that runner with untrusted builds. Replace the approved revision and CLI digest through administrator-controlled Environment variables before use; the checks reject empty or malformed values. This is a workflow template, not an executed CI rehearsal. |
Verified alone on the PR head: npm run format in docs/ passes.
| - **Breaking:** App credentials use context-derived native bindings, not operator-selected secret references. Remove all twelve credential properties, including on disabled features, and provision raw values through reviewed `ts secrets plan` output. Partner inbound auth uses boolean `api_token_enabled`; optional S3 session credentials use boolean `session_token_enabled`, both default false. Enabled credentials and compiled destinations load as one snapshot. First-release direct `ts secrets set` supports Fastly only; all four adapters have native rendering. Cloudflare/Spin presence checks remain unsupported. See the provisioning, protected CI, and migration guides. New keys do not revoke retired contexts, warm states need recreation, and an old-binary rollback restores the original attack. Native/provider/IAM and migration rehearsals remain release gates; this entry is not a completed security-release claim. | ||
|
|
There was a problem hiding this comment.
⛏ nitpick: Link the issue, and drop the blank line 13, which turns the list loose (Markdown then renders each item in it as a paragraph).
| - **Breaking:** App credentials use context-derived native bindings, not operator-selected secret references. Remove all twelve credential properties, including on disabled features, and provision raw values through reviewed `ts secrets plan` output. Partner inbound auth uses boolean `api_token_enabled`; optional S3 session credentials use boolean `session_token_enabled`, both default false. Enabled credentials and compiled destinations load as one snapshot. First-release direct `ts secrets set` supports Fastly only; all four adapters have native rendering. Cloudflare/Spin presence checks remain unsupported. See the provisioning, protected CI, and migration guides. New keys do not revoke retired contexts, warm states need recreation, and an old-binary rollback restores the original attack. Native/provider/IAM and migration rehearsals remain release gates; this entry is not a completed security-release claim. | |
| - **Breaking:** App credentials use context-derived native bindings, not operator-selected secret references. Remove all twelve credential properties, including on disabled features, and provision raw values through reviewed `ts secrets plan` output. Partner inbound auth uses boolean `api_token_enabled`; optional S3 session credentials use boolean `session_token_enabled`, both default false. Enabled credentials and compiled destinations load as one snapshot. First-release direct `ts secrets set` supports Fastly only; all four adapters have native rendering. Cloudflare/Spin presence checks remain unsupported. See the provisioning, protected CI, and migration guides. New keys do not revoke retired contexts, warm states need recreation, and an old-binary rollback restores the original attack. Native/provider/IAM and migration rehearsals remain release gates; this entry is not a completed security-release claim. ([#1037](https://github.com/IABTechLab/trusted-server/issues/1037)) |
Verified alone on the PR head: Prettier with docs/.prettierrc passes, as CI's root-Markdown gate runs it.
|
|
||
| `ts secrets check --adapter axum` checks only its own environment for presence, | ||
| not remote process injection, credential validity, or delivery. Direct `set` | ||
| is unsupported. Native reads and TLS/no-follow rehearsal remain release gates. |
There was a problem hiding this comment.
⛏ nitpick: "Native reads and TLS/no-follow rehearsal remain release gates" is stale. cargo test-axum now runs the real HTTPS client through the untrusted-issuer, wrong-hostname and redirect cases, and the remaining deployment rehearsals are tracked in #1479.
| is unsupported. Native reads and TLS/no-follow rehearsal remain release gates. | |
| is unsupported. `cargo test-axum` exercises the real HTTPS client, including untrusted-issuer, | |
| wrong-hostname, and redirect cases; deployment rehearsals are tracked in #1479. |
Verified alone on the PR head: npm run format in docs/ passes.
| exec trusted-server-axum | ||
| ``` | ||
|
|
||
| First install the matching reviewed `trusted-server-axum` binary on `PATH` and supply the config-envelope environment variable using the [Axum setup](./getting-started#option-b-axum-dev-server). `export` here is a Bash builtin, not a command carrying a credential on subprocess argv. Do not run this loop as a child script and expect it to export into its parent. The store selector must also reach the runtime: planning honors manifest defaults, but the directly launched Axum binary reads this selector only from its process environment. A production supervisor must inject both the reviewed store selector and the generated credential variables into the actual process environment through its protected secret mechanism. Avoid dumping process environments. `check` examines only its own process environment; it cannot inspect a remote process or verify credential bytes. Restart Axum to load replacements. |
There was a problem hiding this comment.
⛏ nitpick: ./getting-started#option-b-axum-dev-server doesn't resolve: getting-started.md has no such heading. The Axum setup lives in axum-dev.md.
| First install the matching reviewed `trusted-server-axum` binary on `PATH` and supply the config-envelope environment variable using the [Axum setup](./getting-started#option-b-axum-dev-server). `export` here is a Bash builtin, not a command carrying a credential on subprocess argv. Do not run this loop as a child script and expect it to export into its parent. The store selector must also reach the runtime: planning honors manifest defaults, but the directly launched Axum binary reads this selector only from its process environment. A production supervisor must inject both the reviewed store selector and the generated credential variables into the actual process environment through its protected secret mechanism. Avoid dumping process environments. `check` examines only its own process environment; it cannot inspect a remote process or verify credential bytes. Restart Axum to load replacements. | |
| First install the matching reviewed `trusted-server-axum` binary on `PATH` and supply the config-envelope environment variable using the [Axum setup](./axum-dev). `export` here is a Bash builtin, not a command carrying a credential on subprocess argv. Do not run this loop as a child script and expect it to export into its parent. The store selector must also reach the runtime: planning honors manifest defaults, but the directly launched Axum binary reads this selector only from its process environment. A production supervisor must inject both the reviewed store selector and the generated credential variables into the actual process environment through its protected secret mechanism. Avoid dumping process environments. `check` examines only its own process environment; it cannot inspect a remote process or verify credential bytes. Restart Axum to load replacements. |
Verified alone on the PR head: npm run format in docs/ passes, and the link resolves.
| while IFS= read -r name; do | ||
| IFS= read -r -s -p "Credential for $name: " value </dev/tty | ||
| printf '\n' >/dev/tty | ||
| export "SPIN_VARIABLE_${name^^}=$value" |
There was a problem hiding this comment.
⛏ nitpick: ${name^^} needs Bash 4, so this loop fails with "bad substitution" under macOS's /bin/bash 3.2. tr works in both:
| export "SPIN_VARIABLE_${name^^}=$value" | |
| export "SPIN_VARIABLE_$(printf '%s' "$name" | tr '[:lower:]' '[:upper:]')=$value" |
Verified alone on the PR head: npm run format in docs/ passes.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Independent pass over the credential-binding engine, its runtime consumers (S3, pull sync, DataDome, Tinybird), the four adapters, and ts secrets. I found no path where a config-only change sends a credential to a destination or purpose other than the one its binding was derived for. Every field that affects sending is part of the derived key; the length-prefixed encoding has no collisions (the four golden vectors re-derive independently from the spec); and missing bindings fail closed on every adapter. The blocker below matches the existing review; the four inline findings are new and non-blocking.
Blocking
🔧 wrench
- Branch conflicts with current
main: see cross-cutting below
Non-blocking
🤔 thinking
CompiledBindingDebug prints the handler username: see inline atcrates/trusted-server-core/src/secret_bindings.rs:201
📝 note
- Pull allowlists with a trailing dot or several hosts no longer load: see inline at
crates/trusted-server-core/src/secret_bindings.rs:747
⛏ nitpick
- ABI domain separator duplicated as a literal: see inline at
crates/trusted-server-core/src/secret_bindings.rs:619 unsigned_headersnow receives signed headers: see inline atcrates/trusted-server-core/src/proxy.rs:1084
Cross-cutting / body-level findings
- 🔧 Branch conflicts with current
main.git merge-treeagainstmainatd1bb3fa19reports conflicts in 14 files, includingCargo.toml,Cargo.lock,config.rs,config_payload.rs,datadome/protection.rs,generate-viceroy-config.rsandtrusted-server.example.toml. The merge also needs to resolve main's[trusted_forwarder] shared_secret(#1107), which still uses the operator-chosen key-name model this PR removes. It needs a binding role and a removed-reference rejection, as the existing review describes. Hosted CI has not exercised the merged result.
CI Status
All 27 reported checks pass on 8bb27824e, including cargo test (required), cargo fmt (required), format-typescript (required), format-docs (required), the adapter suites, cross-adapter parity, integration and browser integration tests, and CodeQL.
|
|
||
| /// Public, secret-free explanation of an executable-owned binding. | ||
| #[derive(Clone, Debug, Eq, PartialEq, Serialize)] | ||
| pub struct CompiledBinding { |
There was a problem hiding this comment.
🤔 thinking: CompiledBinding derives Debug, and for handler.password its subject holds the handler username in plain text (built at line 903 from handler.username.expose()).
Handler.username is Redacted elsewhere, and settings.rs has a test that keeps handlers[].username out of Settings debug output. The design spec also says to keep usernames out of runtime diagnostics. Nothing debug-prints a plan or a compiled binding today, so this isn't a leak yet. But a later log::debug!("{plan:?}") or a Report attachment would print it. ResolvedBinding and ResolvedBindings already hand-write Debug; doing the same here keeps the type safe to log:
impl core::fmt::Debug for CompiledBinding {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
f.debug_struct("CompiledBinding")
.field("descriptor_id", &self.descriptor_id)
.field("scope", &self.scope)
.field("field_path", &self.field_path)
.field("key", &self.key)
.finish_non_exhaustive()
}
}Apply manually: it means removing Debug from the derive and adding the impl, which a single suggestion can't express. The Serialize output used by ts secrets plan can keep the subject, because the spec allows reviewed subject metadata in provisioning output.
| Ok(()) | ||
| } | ||
| fn derive_key(scope: &str, subject: &[String], export: &[String]) -> BindingResult<String> { | ||
| let mut payload = b"trusted-server-secret-binding-v1\0".to_vec(); |
There was a problem hiding this comment.
⛏ nitpick: The domain separator repeats BINDING_ABI_VERSION (line 24) as a literal. Bumping the constant would change the plan's binding_abi label but not the derived keys. Building it from the constant keeps them together, and the golden vectors still pin the bytes:
let mut payload = BINDING_ABI_VERSION.as_bytes().to_vec();
payload.push(0);| let host = url::Url::parse(&endpoint) | ||
| .ok() | ||
| .and_then(|url| url.host_str().map(str::to_owned)) | ||
| .ok_or_else(|| invalid(field_path, "invalid credential endpoint"))?; |
There was a problem hiding this comment.
📝 note: This check requires every pull_sync_allowed_domains entry to equal the endpoint host exactly. Base (registry.rs validate_pull_sync_fields) stripped trailing dots and accepted a match on any entry. Configs like these used to load and now fail:
pull_sync_allowed_domains = ["sync.example.com."]pull_sync_allowed_domains = ["sync.example.com", "backup.example.com"]
The spec intends this, and it fails safe. But the migration guide and the CHANGELOG's breaking-change entry don't mention it, so an operator with a multi-host allowlist only finds out when the config won't load. A line in docs/guide/secret-binding-migration.md would cover it.
| route: &ProxyAssetRoute, | ||
| target_url: &url::Url, | ||
| request_method: &Method, | ||
| unsigned_headers: &http::HeaderMap, |
There was a problem hiding this comment.
⛏ nitpick: handle_asset_proxy_request now runs apply_asset_origin_auth before calling this function, so unsigned_headers arrives already signed for GET (Authorization, x-amz-date, x-amz-content-sha256, and the session token if enabled). The HEAD and GET passes here sign again. The output is still correct, because sign_headers uses insert and Authorization isn't a signed header. But the name and doc are now misleading, and each preflight signs once more than it needs to. Passing a copy taken before signing, or renaming the parameter, would fix it.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
This replaces operator-chosen secret references with context-derived native bindings (#1037), adds ts secrets plan/check/set, and moves all four adapters onto EdgeZero's implicit-secret hooks. I traced each exporter (S3 signing, pull sync, DataDome Protection, Tinybird) back to its loaded binding. Every outbound URL, host, region and bearer header comes from the compiled target, not from mutable settings. Forced TLS applies on every export path. Missing or changed bindings fail before a backend is created. I agree with the two existing reviews and won't repeat their findings. The one blocker still open is the merge with current main. The two new findings below are documentation gaps and don't block merge.
Both inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion, or Add suggestion to batch to apply both together.
Blocking
🔧 wrench
- Branch still conflicts with current
main: see cross-cutting below
Non-blocking
📝 note
- Stricter endpoint grammar is missing from the migration guide: see inline at
docs/guide/secret-binding-migration.md:32(suggestion)
⛏ nitpick
- "Reordering entries does not" overstates what a reorder skips: see inline at
docs/guide/secret-binding-migration.md:106(suggestion)
Cross-cutting / body-level findings
- 🔧 Branch still conflicts with current
main. GitHub reports the PR asCONFLICTING.git merge-tree --write-tree origin/main 8bb27824e(main atd1bb3fa19) still conflicts, including ingenerate-viceroy-config.rs,tests/environments/axum.rs,tests/environments/cloudflare.rs,docs/guide/configuration.md,scripts/generate-integration-viceroy-configs.shandtrusted-server.example.toml. This is the same blocker both earlier reviews raised. It also has to settle[trusted_forwarder] shared_secretand the EdgeZero pin rollback described there. All 27 green checks ran on the pre-merge tree.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- PBS Terraform and Compose checks: PASS
- prepare integration artifacts: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
|
|
||
| Both new flags default to `false` and accept booleans only, not strings. Do not forget inbound authentication when removing `api_token`. An enabled role missing its binding fails loading, rather than silently disabling authentication. Disabled roles make no native reads. Existing pull boolean/string activation follows its existing normalization; the string `"false"` does not enable it. | ||
|
|
||
| Deprecated Tinybird/DataDome/S3 store selectors remain ignored, deserialize-only inputs; remove them. Tinybird's deprecated `access_token_secret` is still discarded, and `access_enabled = true` is still rejected. It is not a thirteenth role. Trusted-client-IP credentials already came from the native store; the old guide's inline-value description was wrong. |
There was a problem hiding this comment.
📝 note: The removed-properties table covers the credential fields, but upgrading also tightens endpoint syntax. Some configs the old binary loaded and used will now fail to load, and this guide doesn't mention it.
normalize_endpoint(secret_bindings.rs:666) rejects any?,#or%. Base accepted apull_sync_urlwith a query and kept it on the outbound request; base'sbuild_pull_request_url_appends_ec_idasserted that?x=1survived, and this PR changes that test to a query-free URL. A partner whose endpoint needs a query parameter now blocks the whole config load.validate_original_endpointsruns these checks on disabled features too: every partner'spull_sync_urlwhateverpull_sync_enabledis set to, DataDome'sprotection_api_originwhateverenabledis set to, and a non-empty Tinybirdapi_host. So a parked partner with a legacy URL also blocks loading.- Pull hosts with a trailing dot (base test
validated_pull_sync_url_accepts_normalized_allowlist_matchusedSYNC.PARTNER.TEST.) and IP literals are now rejected. Surrounding whitespace that base trimmed (S3origin_url, DataDomeprotection_api_origin) is also rejected. - Pull sync now builds its backend with
certificate_check: trueand ignoresproxy.certificate_check. Before, it followed that setting.
docs/guide/secrets.md:67 states the grammar, but an operator working through this checklist won't see it. This sits next to the earlier note on exact-match pull_sync_allowed_domains, so the suggestion covers both in one paragraph:
| Deprecated Tinybird/DataDome/S3 store selectors remain ignored, deserialize-only inputs; remove them. Tinybird's deprecated `access_token_secret` is still discarded, and `access_enabled = true` is still rejected. It is not a thirteenth role. Trusted-client-IP credentials already came from the native store; the old guide's inline-value description was wrong. | |
| Deprecated Tinybird/DataDome/S3 store selectors remain ignored, deserialize-only inputs; remove them. Tinybird's deprecated `access_token_secret` is still discarded, and `access_enabled = true` is still rejected. It is not a thirteenth role. Trusted-client-IP credentials already came from the native store; the old guide's inline-value description was wrong. | |
| Endpoint syntax is also stricter, including on disabled features. `pull_sync_url`, DataDome `protection_api_origin`, Tinybird `api_host`, and authenticated S3 `origin_url` must be HTTPS DNS names with no query, fragment, percent escape, userinfo, IP literal, trailing dot, or surrounding whitespace. A pull URL with a query parameter, which the old binary preserved and sent, now fails loading; get a query-free endpoint from the partner first. A nonempty `pull_sync_allowed_domains` may list only the endpoint hostname. Pull sync now always verifies certificates, whatever `proxy.certificate_check` says. |
A matching clause in the CHANGELOG Breaking entry would help too.
Applied alone to 8bb27824e; cd docs && npm run format passes with no drift.
|
|
||
| ## Retire a context | ||
|
|
||
| Changing a subject, endpoint, route policy, or binding ABI requires a new reviewed plan and newly provisioned bindings. Reordering entries does not. For each change: |
There was a problem hiding this comment.
⛏ nitpick: "Reordering entries does not" is right about keys, since collection_order_does_not_change_identity_keys pins that. But it reads as if a reorder needs no new plan either, and the CLI says otherwise. Each plan entry serializes its field_path (for example ec.partners[0].ts_pull_token), and entry_id hashes that metadata. effective_config_sha256 also covers the reordered array. So after a reorder, verify_plan rejects the old plan with "reviewed plan does not match current config…". Nothing breaks at runtime, but an operator who trusts this sentence will hit that error in check or set.
| Changing a subject, endpoint, route policy, or binding ABI requires a new reviewed plan and newly provisioned bindings. Reordering entries does not. For each change: | |
| Changing a subject, endpoint, route policy, or binding ABI requires a new reviewed plan and newly provisioned bindings. Reordering entries keeps every key, so it needs no new bindings. It does change each entry's `field_path` and entry ID, so `ts secrets check` and `ts secrets set` still require a regenerated, reviewed plan. For each change: |
Applied alone to 8bb27824e; cd docs && npm run format passes with no drift.
Resolve merge conflicts, preserve credential binding and base forwarding/deadline behavior, update the EdgeZero companion revision, and repair native fixtures.
Resolve merge conflicts while preserving credential-binding changes, base-branch Lockr routing, and connection-readiness regression coverage.
Summary
Closes #1037.
ts secrets plan/check/setworkflows, generated native declarations for all four adapters, secure direct Fastly writes, and operator/migration documentation.The threat model is a config-only writer, without native secret-writing, binding/deployment administration, or unreviewed execution with provisioning credentials. Complete already-provisioned contexts remain authorized. Retiring a binding blocks fresh loads, not credentials already captured by retained/in-flight snapshots.
Breaking change: reprovision before upgrading
Existing deployments must reprovision every required app credential under the generated bindings before deploying this version. Legacy secret names no longer work. Existing raw values can be reused; reprovisioning does not itself require rotation.
ec.partners[*].api_token_enabledand S3auth.session_token_enabledwhere those optional credentials are needed; both default tofalse. Enabled missing credentials fail loading.See the migration guide. Restoring the old binary restores the original attack.
Targeted security evidence
proxy.certificate_check=false, and valid-checksum config attacks against unchanged native bindings. Redirect sinks have positive controls. Unsigned Viceroy startup probes are recorded separately from application sends.test-utilsintegrations invoke the same core exporter through real native stores/transports. This proves the native core/adapter contract, not shipping S3 routing on those adapters.Validation
Native runs used Viceroy 0.21.0, Spin 4.1.0, and Wrangler 4.83.0/workerd with worker-build 0.8.5 matching the lockfile. Normal Rustdoc succeeds with existing private/broken-link warnings. Existing ignored platform/NSS/browser/doctest checks are not claimed as executed. Hosted CI will rerun on the pushed commit.
The latest merge from
mainalso brought two config round-trip tests that referenced removed helpers. They now use the existing generated-binding fixtures. The Spin installation link uses the approved GitHub release host, fixing the new changed-line domain gate without broadening its allowlist.Dependency and deployment readiness
EdgeZero is pinned to published commit
4f645b1e705d295117c93f4c74db12e5692ffb34, companion draft IABTechLab/edgezero#444. No local path patch or released-prerequisite claim.#1479 tracks the remaining deployment gates: dependency acceptance, actual config-publisher IAM separation, isolated provider create/replace/read-through and propagation/limits, protected provisioning CI, coordinated migration/rotation/retirement/downgrade rehearsals, and deployment-specific security evidence. Local tests do not clear those gates or authorize production rollout.
No real application credentials were read or provisioned. No production deployment, merge, or release tag was performed.