Repository navigation
Bind credentials to approved contexts and add secure provisioning - #1453
ChristianPavilonis wants to merge 18 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
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
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
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.
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
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.
Restore migration diagnostics and short partner pull-token support. Reject unsafe stdin, validate provisioning configs, and distinguish unsupported checks. Retain partner registries with loaded adapter state, reserve app credential names on signing reads, and strengthen native and mutation regressions. Document migration hazards and the Viceroy TLS evidence boundary.
Review follow-up c278e6aPushed c278e6a18. This implements the agreed subset of the feedback. Thread resolutions below record fixes, prior fixes, and explicit deferrals; they do not mean every suggestion was implemented or that this PR is ready for release. Responses to the review summariesThis accounts for the body-level points in aram356's review, prk-Jr's review, and dhruv8sh's review. Each inline point also receives a reply on its own thread.
Implemented inline changes
The remaining inline Debug-redaction, ABI-constant deduplication, unsigned_headers rename/redundant signing, CHANGELOG link/spacing, stale Axum sentence, broken Axum anchor, and Bash 3.2 example portability requests were not selected for implementation. They are explicitly deferred in their thread replies, not silently treated as fixes. Security evidence and limitsThe Fastly TLS concern was confirmed by execution. An isolated app-policy mutant still passed all 34 native checks under Viceroy 0.21.0 when both verified and nocert backends were declared and trusted-HTTPS positive controls were required. Viceroy rejected the bad certificate even with the app setting off. The independent core backend-spec test catches that same mutation. TESTING.md and scripts/README.md now distinguish these claims. Checks against the pushed source contents passed:
Existing ignored/manual/platform checks, macOS PTY behavior, and additional unrelated browser/deadline/EOF runtime harnesses were not rerun. Hosted CI must validate this pushed commit; these results are not a claim that every CI job ran locally. Still gatedWe agreed to wait for a released EdgeZero version containing IABTechLab/edgezero#444. The current implementation pin remains c199355e; this is not a released-prerequisite claim. #1479 continues to gate dependency acceptance, IAM separation, protected provisioning, provider behavior, and migration/deployment rehearsals. Closing review discussions does not clear those gates. No production credentials, deployment, PR merge, or release tag were used. |
…t without changing auction behavior.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Follow-up pass over the changes since 8bb27824e: the two main merges, c278e6a18, and the two CodeQL log commits. The earlier blocker is resolved. The branch is current with main. trusted_forwarder.shared_secret is now the thirteenth binding role, with removed-reference rejection and the same length, character and placeholder checks. The signing read path refuses ts1_ kids before any secret read. --stdin rejects terminal input and trailing CR/LF. Load diagnostics are restored without echoing removed values. The pull-token minimum is gone. Each adapter state keeps its own partner registry. One new blocker: at the pinned EdgeZero revision, ts secrets cannot resolve Fastly resources against the real API.
Blocking
🔧 wrench
- Pinned EdgeZero revision breaks
ts secretsresource resolution on Fastly: see inline atCargo.toml:60
Non-blocking
⛏ nitpick
- Loader doc comment no longer matches its diagnostics: see inline at
crates/trusted-server-core/src/config_payload.rs:60(suggestion)
CI Status
All 30 check runs pass on 96fdb107d, including cargo test, cargo fmt, format-typescript and format-docs (required), the adapter suites, cross-adapter parity, integration, browser and trace tests, worker deadlines, the ts CLI suites on Linux and macOS, and CodeQL.
| edgezero-cli = { git = "https://github.com/IABTechLab/edgezero", tag = "v0.0.11" } | ||
| edgezero-core = { git = "https://github.com/IABTechLab/edgezero", tag = "v0.0.11", default-features = false } | ||
| # The implicit-secret companion includes the ingress/pre-dispatch APIs from v0.0.11. | ||
| edgezero-adapter-axum = { git = "https://github.com/IABTechLab/edgezero", rev = "c199355e5893fe58e050ed7bae8c5f8462e2f511", default-features = false } |
There was a problem hiding this comment.
🔧 wrench: At c199355e, ts secrets plan, check and set --adapter fastly cannot resolve resources against the real Fastly API.
fastly_secrets::resolve_resources requests the inventory for every store in edgezero.toml. Our manifest declares [stores.config], so it always fetches GET /resources/stores/config?limit=100 and parses the reply as a paginated {"data":[…],"meta":{…}} page. Fastly returns the config-store list as a bare JSON array. EdgeZero's own deploy path at this same revision already parses it that way (edgezero-adapter-fastly/src/cli.rs:801, parse_store_inventory(ResourceKind::Config, …)). The companion PR fixed this after the pinned commit, in IABTechLab/edgezero#444 commit c24cd9940 ("Fix Fastly secret resource resolution and native name validation").
The migration guide documents Fastly as the only adapter with direct ts secrets set. At this head, every Fastly plan, check and set stops with unable to resolve complete Fastly Config Store inventory, so operators cannot provision before cutover. CI stays green because FakeTransport in crates/trusted-server-cli/src/commands/secrets/tests.rs (lines 881-882) replies to /resources/stores/config? with the same paginated shape the pinned parser expects.
Reproduction: in a scratch copy of 96fdb107d, I changed only that fake to reply with [{"id":"fictional-config-store","name":"trusted_server_config"}]. secrets_cli_plan_and_stdin_create_replace_use_actual_shared_transport then fails with "unable to resolve complete Fastly Config Store inventory". I did not call the real Fastly API.
Proposed fix (apply manually; it spans Cargo.toml, Cargo.lock and tests.rs): this folds into the release-pin gate you already have. The released EdgeZero version must contain c24cd9940. When you repin, change the fake to answer GET /resources/stores/config with a bare array, so the test matches Fastly's real shape. #1479 also still records the older pin 4f645b1e.
| /// settings are invalid. | ||
| /// Rejects invalid envelopes, removed reference properties, malformed feature | ||
| /// configuration, missing/invalid native bindings, or invalid resolved settings. | ||
| /// Diagnostics never attach provider errors, values, or untrusted input snippets. |
There was a problem hiding this comment.
⛏ nitpick: Since c278e6a18, line 72 passes serde's detailed error through. Some of those messages quote the offending input, for example invalid type: integer `123`, expected a string. Removed credential fields are rejected before parsing, so this can't leak credentials, but "never attach … untrusted input snippets" is no longer true.
| /// Diagnostics never attach provider errors, values, or untrusted input snippets. | |
| /// Diagnostics never attach provider errors or credential values. Structural | |
| /// errors can quote non-credential config input, such as a misspelled field. |
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
Local follow-up checks for c278e6a passed on the exact source contents committed here:
Targeted mutants now fail the loaded-token, rate-limit, partner-pairing, pull-method, DataDome TLS, signing-read, route-isolation, stdin, environment-selector, and process-group regressions. The native Fastly TLS-policy mutant survives because Viceroy enforces certificate verification independently; its matching core backend assertion fails as expected. Native runs used Viceroy 0.21.0, Spin 4.1.0, and Wrangler 4.83.0/workerd with worker-build 0.8.5. Real stdin/hidden-input checks ran through Linux pipes and a PTY with fictional transport.
Existing ignored/manual/platform checks, macOS PTY behavior, and additional unrelated browser/deadline/EOF runtime harnesses were not rerun. Hosted CI will validate the pushed commit. Local checks do not certify deployment gates or every CI job.
Dependency and deployment readiness
EdgeZero is pinned to published commit
c199355e5893fe58e050ed7bae8c5f8462e2f511, companion IABTechLab/edgezero#444. We are waiting for an accepted released version containing that prerequisite before final integration. 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.