Skip to content

Add ts cli#799

Open
ChristianPavilonis wants to merge 19 commits into
mainfrom
feature/ts-cli-base
Open

Add ts cli#799
ChristianPavilonis wants to merge 19 commits into
mainfrom
feature/ts-cli-base

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • Adds the host-target ts operator CLI crate and binary.
  • Wires EdgeZero-backed lifecycle delegation and Trusted Server config init/validate/push flows.
  • Moves runtime settings loading to flattened app-config entries and updates docs/CI for the host-target CLI.

Changes

File Change
crates/trusted-server-cli/ Adds the base CLI crate, clap args, dispatch, EdgeZero delegate, config commands, and tests.
crates/trusted-server-core/src/config_payload.rs Adds canonical flattened config payload generation and hash metadata.
crates/trusted-server-core/src/settings_data.rs Loads settings from the platform config store instead of embedded build output.
crates/trusted-server-adapter-fastly/src/app.rs, src/main.rs Reads settings through the Fastly config-store adapter.
.github/workflows/*, .cargo/config.toml Adds host-target CLI check/test coverage and local aliases.
edgezero.toml, trusted-server.example.toml, .gitignore Adds EdgeZero manifest/template config and ignores operator-owned config.
docs/guide/cli.md, docs/guide/getting-started.md, README.md Documents base CLI install/config workflow.

Closes

No issue provided; this PR is split out from the combined feature/ts-cli-next branch.

Test plan

  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo test --package trusted-server-cli --target aarch64-apple-darwin — 12 passed

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing/log macros (not println!)
  • New code has tests
  • No secrets or credentials committed

Comment thread crates/trusted-server-core/src/settings.rs Fixed
@ChristianPavilonis ChristianPavilonis changed the title Add EdgeZero-backed ts CLI Add ts cli Jun 23, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Introduces the host-target ts operator CLI and, more significantly, re-architects runtime settings loading: config moves from a build-time-embedded TOML (include_bytes!(target/trusted-server-out.toml), baked by build.rs) to a runtime read of an EdgeZero BlobEnvelope from a platform config store, with per-chunk + envelope SHA-256 integrity verification and a Fastly chunk-pointer reassembly path. build.rs becomes a no-op, the config crate drops to a dev-dependency, and EdgeZero git deps are pinned to a fixed rev.

Clean, well-tested work — strong unit coverage (round-trip, tamper-rejection, chunk reassembly, placeholder rejection) and good error messages. No blocking issues; all findings below are non-blocking.

3 of the inline comments carry a one-click GitHub suggestion (verified in a scratch worktree: cargo fmt --check, cargo clippy -p trusted-server-core --all-targets --all-features -D warnings, and cargo test -p trusted-server-core — 1426 passed). The canary comment is prose because it has no single clean fix.

Non-blocking

♻️ refactor

  • request_body_bytes is an incomplete abstraction (proxy) — see inline at crates/trusted-server-core/src/proxy.rs:44
  • request_body_bytes is an incomplete abstraction (request signing) — see inline at crates/trusted-server-core/src/request_signing/endpoints.rs:27

🤔 thinking

  • Unbounded pre-allocation from operator-controlled envelope_len — see inline at crates/trusted-server-core/src/settings_data.rs:113
  • EdgeZero entry-point canary downgraded to a no-op — see inline at crates/trusted-server-integration-tests/tests/common/ec.rs:283

Cross-cutting / body-level findings

  • 🤔 Startup INSECURE: proxy.certificate_check is disabled warning dropped — the old load_settings emitted a startup log::warn! when proxy.certificate_check was off; the new blob path (settings_from_config_blob / get_settings_from_config_store) does not. A per-backend warning still exists at request time (adapter-fastly/src/backend.rs:219), so coverage isn't zero, but the loud startup signal is gone. Cheap to re-add in get_settings_from_config_store.
  • 📝 #[serde(deny_unknown_fields)] added broadly (settings.rs, consent_config.rs, auction_config_types.rs) — good hardening (catches typo'd keys; the from_toml test flipped from "extra fields ignored" to "rejected"), but it's a behavior change: any deployed config carrying extra/forward-compat keys now fails to load. Validated at ts config push time, which is the right place — flagging the migration implication for existing operator configs.
  • 🌱 Minor: adapter-fastly/src/main.rs opens the same TRUSTED_SERVER_CONFIG_STORE twice per request (open_trusted_server_config_store for the flag + edgezero_config_store_handle for dispatch) — harmless, mildly wasteful. Separately, .github/workflows/test.yml's "Verify Fastly WASM release build" step still sets TRUSTED_SERVER__PUBLISHER__ORIGIN_URL, now vestigial since build.rs no longer bakes env-derived config.

👍 praise

  • EdgeZero git deps pinned from branch = "main" to a fixed rev — reproducible builds.
  • Integrity model in resolve_fastly_chunk_pointer + settings_from_config_blob: per-chunk len+sha, envelope len+sha, and BlobEnvelope::verify() — solid defense-in-depth, with tampered_blob_hash_is_rejected and strings_that_look_like_json_scalars_round_trip_as_strings guarding the TOML→JSON→TOML secret round-trip.
  • build.rs reduced from 72 lines of #[path]-included module hacks to a 3-line no-op.

CI Status — all 14 checks PASS

  • cargo test: PASS (required)
  • cargo fmt: PASS (required)
  • format-typescript: PASS (required)
  • format-docs: PASS (required)
  • Analyze (rust): PASS
  • Analyze (javascript-typescript): PASS
  • Analyze (actions): PASS
  • CodeQL: PASS
  • vitest: PASS
  • integration tests: PASS
  • integration tests (EdgeZero entry point): PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS

Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-core/src/request_signing/endpoints.rs
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-integration-tests/tests/common/ec.rs Outdated

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Review of #799. The substantive change is sound: runtime config moves from build-time-baked TOML to a config-store blob envelope with solid integrity checks (per-chunk + envelope len/SHA-256 + verify() + post-load placeholder rejection), and the host-only ts CLI is cleanly isolated from the wasm build. CI is green. Findings are mostly non-blocking — no 🔧 blockers — with one open ❓ question on secret handling worth resolving before merge.

Blocking

❓ question

  • Plaintext secrets in the app-config blobcrates/trusted-server-core/src/config.rs:92 (inline).

Non-blocking

🤔 thinking

  • Settings reloaded up to 3×/requestcrates/trusted-server-adapter-fastly/src/main.rs:355,383 (inline).
  • Dead build-time env exportsscripts/integration-tests.sh:54-58 (inline).
  • EdgeZero entry-point canary downgraded to a warningcrates/trusted-server-integration-tests/tests/common/ec.rs:296 (inline).
  • Auction test name overstates coveragecrates/trusted-server-core/src/auction/endpoints.rs:808 (inline).
  • Broad #[serde(deny_unknown_fields)] (settings.rs + ~20 config structs): unknown/typo'd keys now hard-fail load, which at runtime drops the whole app to the startup-error router. Good safety, but potentially breaking for existing operator configs that carried extra keys — worth a release note.

♻️ refactor

  • No-op Result body wrappers with unused _endpointcrates/trusted-server-core/src/proxy.rs:41, crates/trusted-server-core/src/request_signing/endpoints.rs:27 (inline).
  • Track the EdgeZero feature/extensible-cli branchCargo.toml:39-44, crates/trusted-server-integration-tests/Cargo.toml:13 (inline suggestions). Verified compiling against branch HEAD bf177b13.

🌱 seedling / 📝 note

  • prebid::validate_config_for_startup lacks a direct unit test — crates/trusted-server-core/src/integrations/prebid.rs:201 (inline).
  • Integration cargo test now depends on a generated config artifact — crates/trusted-server-integration-tests/tests/environments/fastly.rs:82 (inline).
  • Unused workspace dependency edgezero-adapter (Cargo.toml:39) — no first-party crate references it (only transitive via edgezero-cli). Drop it or document intent.
  • Reproducibility — a branch = ref (per the suggestions above) is not reproducible across machines/CI; pin back to a specific rev before merge.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests (vitest): PASS
  • integration tests (incl. EdgeZero entry point): PASS

Comment thread Cargo.toml Outdated
Comment thread crates/trusted-server-integration-tests/Cargo.toml Outdated
Comment thread crates/trusted-server-core/src/config.rs
Comment thread crates/trusted-server-adapter-fastly/src/main.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-integration-tests/tests/common/ec.rs Outdated
Comment thread scripts/integration-tests.sh
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/integrations/prebid.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Strong, well-tested PR. Unifies runtime config loading onto the EdgeZero app_config blob, adds the host-target ts operator CLI, pins all edgezero git deps to a fixed rev, and hardens request-body handling. Read every changed file; no bugs found. Findings below are non-blocking — one open question on secret-at-rest storage and several consistency/hardening suggestions.

Blocking

❓ question

  • Secrets stored in the config-store blob, not the secret storeTrustedServerAppConfig::SECRET_FIELDS = &[] keeps ec.passphrase, publisher.proxy_secret, handler.password, and ec.partners[].api_token as plaintext JSON inside the app_config config store. Intentional for this phase? (See inline comment on config.rs:92.)

Non-blocking

♻️ refactor

  • Auction stream-body handlingauction/endpoints.rs:81 silently treats a streamed body as empty; add an explicit is_stream() guard for parity with proxy.rs / request_signing.
  • Vacuous Result on body_as_readerproxy.rs:40 and publisher.rs:35 return Result but never error; add the stream guard or keep infallible.

🤔 thinking

  • x-ts-entry-point: edgezero on all responses — exposes internal routing/migration state to clients; used as a CI canary (main.rs:488). Consider gating/documenting.
  • Dependency-parity allowlist grew ~18 cratescheck-integration-dependency-versions.sh; each entry weakens the drift guard.

🌱 seedling

  • get_settings_from_services unusedsettings_data.rs:42, pub with no in-repo caller; confirm it's for future adapters or drop.
  • Unbounded blob reassemblysettings_data.rs:114, chunks concatenated before the total-length check.

📝 note

  • #[serde(deny_unknown_fields)] is a behavior change — existing trusted-server.toml files with extra/typo'd/legacy keys now hard-fail validation and runtime load (the round-trip test flipped from "extra fields ignored" to "rejected"). Good hardening — call it out in migration/release notes (settings.rs, plus the consent/auction config structs).
  • CLI error typeResult<(), String> deviates from the error-stack convention but is constrained by edgezero_cli's API (run.rs:54).

⛏ nitpick

  • Store-id resolutiondefault_config_store_name reads raw env while default_config_key uses EnvConfig (settings_data.rs:52).

👍 praise

  • Streaming-body rejection on sign/verify/rotate/deactivate request paths closes a real correctness gap (streamed bodies were silently treated as empty — could bypass signature verification), with thorough tests.
  • Pinning all edgezero deps from branch = "main" to a fixed rev — reproducible builds.
  • Defense-in-depth on Fastly chunk reconstruction: per-chunk len + sha, envelope len + sha, then envelope integrity verify.
  • Reusing the request-scope AppState settings snapshot instead of re-reading the config store on every finalize / EC step — removes redundant reads and guarantees finalize uses the same settings that built the router.
  • is_edgezero_enabled switched to try_get (no panic on backend error) with graceful legacy fallback.
  • Removing build-time secret env injection from CI now that runtime loads from the config store.

CI Status

  • fmt: PASS
  • clippy (workspace + host-target CLI): PASS
  • rust tests (workspace + host-target CLI): PASS
  • js tests (vitest): PASS
  • integration / edgezero / browser: PASS
  • docs + ts format, CodeQL: PASS

Comment thread crates/trusted-server-core/src/config.rs
Comment thread crates/trusted-server-adapter-fastly/src/main.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs Outdated
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-core/src/settings_data.rs
Comment thread crates/trusted-server-cli/src/run.rs
Comment thread scripts/check-integration-dependency-versions.sh

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review — follow-up fd0322c2

The follow-up commit resolves the substantive findings from the prior review, with tests:

  • Per-request settings reloadedgezero_main now reuses the request-scope AppState.settings snapshot for entry-point + EC finalization (one load per request), with a reload fallback only for startup-error routers.
  • Streaming request bodiesrequest_body_bytes rejects Body::Stream with BadRequest; sign / verify / rotate streaming-rejection tests added.
  • EdgeZero entry-point guard → new x-ts-entry-point: edgezero header is hard-asserted by the EdgeZero CI job, replacing the dropped 405 canary.
  • Dead build-time env exports removed from scripts/integration-tests.sh and test.yml; strict key validation documented; missing Viceroy config now errors with guidance.
  • Bonus hardening: chunk-pointer reconstruction no longer pre-allocates from the operator-controlled envelope_len (String::new()), removing an untrusted-capacity vector. 👍

The open ❓ on SECRET_FIELDS = &[] is answered (intentional inline-secret behavior for this PR; secrets-store routing deferred), and the auction-test-name / prebid-direct-test items are acknowledged as scoped follow-ups.

Approving. A few non-blocking nits inline: a leftover no-op Result in body_as_reader, the always-on entry-point header's client visibility, EC-finalize branch duplication, and an unused workspace dependency.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests (vitest): PASS
  • integration tests (incl. EdgeZero entry point): PASS

Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread Cargo.toml Outdated
@ChristianPavilonis ChristianPavilonis linked an issue Jun 29, 2026 that may be closed by this pull request
Replace the custom trusted-server CLI lifecycle and config payload plumbing with a thin EdgeZero delegation layer using typed config push/validate flows.

Add TrustedServerAppConfig wrapper in core with deploy-time validation and move blob reconstruction into runtime helpers.

Drop flattened config entry publishing and route app-config through EdgeZero blob envelope handling while keeping edgezero flag reads in trusted_server_config.

Update CLI and architecture docs for the new model and adjust fastly adapter store selection.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review (3rd pass) — head 37718b7

Everything actionable from the two prior review rounds is resolved, and the new work is well-tested. CI is green across all adapter jobs (fastly, cloudflare, spin, axum, cross-adapter parity).

Prior nits addressed: unused edgezero-adapter dep removed; the no-op Result in body_as_reader fixed in both proxy.rs and publisher.rs; SECRET_FIELDS = &[] documented with a Phase-1 rationale; the x-ts-entry-point header removed.

New work reviewed:

  • Chunk-pointer hardening (settings_data.rs): validates the ≤8000-byte Fastly entry limit, per-chunk lengths, and a checked_add sum against envelope_len before allocating, with a running overflow guard and a new mismatch test. Switched to EnvConfig consistently.
  • Cloudflare runtime config loading: clean cfg(wasm32) OnceLock injection via env.var("TRUSTED_SERVER_CONFIG")settings_from_config_blob, with proper Report error handling.
  • Route-dispatch test coverage, rebase API-drift fixes, and an EdgeZero dep rev bump (still pinned).

Approving. Three non-blocking notes inline: the removed EdgeZero entry-point CI guard (now relying on forced rollout_pct=100 rather than a positive assertion), the Cloudflare example-config fallback shipping placeholder secrets, and a brittle test-config string substitution.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests (incl. cross-adapter parity): PASS
  • js tests (vitest): PASS
  • integration tests (incl. EdgeZero entry point): PASS


fn generated_config_store_blocks(envelope_json: &str, edgezero_enabled: bool) -> String {
let edgezero_enabled_value = if edgezero_enabled { "true" } else { "false" };
let edgezero_rollout_pct = if edgezero_enabled { "100" } else { "0" };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 thinking — Forcing edgezero_rollout_pct = "100" here is a good way to make the EdgeZero CI job take the EdgeZero path. But note cfcd35885 also removed assert_edgezero_entry_point, the x-ts-entry-point header, and the EXPECT_EDGEZERO_ENTRY_POINT gate — so there's no longer a positive assertion that test_ec_lifecycle_fastly actually routed through edgezero_main. If main() silently falls back to legacy_main (e.g. the config store can't be opened), the EC scenarios still pass and the integration tests (EdgeZero entry point) job stays green. Forcing rollout makes that unlikely, but the explicit guard is gone. If keeping it matters, a debug-gated signal or a startup log-assertion would restore the guarantee without the always-on client-visible header. Non-blocking.

return build_state_with_settings(settings);
}

let settings = Settings::from_toml(include_str!("../../../trusted-server.example.toml"))?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

📝 note — When TRUSTED_SERVER_CONFIG is unset, build_state falls back to Settings::from_toml(example.toml). Unlike the blob path (settings_from_config_blob), from_toml does not run reject_placeholder_secrets, so a misconfigured Worker boots with the template's placeholder secrets instead of failing fast. Dev-only adapter and pre-existing pattern, but worth a guard (or at least a log::warn!) so a missing config surfaces loudly. Non-blocking.

template_path.display()
))?;
let config_json = cloudflare_config_json(origin_port())?;
let generated = template.replace(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpicktemplate.replace("TRUSTED_SERVER_CONFIG = \"{}\"", …) silently no-ops if the template placeholder ever drifts, in which case the generated config omits TRUSTED_SERVER_CONFIG and the Worker quietly falls back to example config. Consider asserting the replacement changed the string (e.g. assert_ne!(generated, template)). Test-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port EdgeZero CLI to TS

4 participants