Add ts cli#799
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChristianPavilonis
force-pushed
the
feature/ts-cli-base
branch
from
7fdbff6 to
2ea46f1
Compare
ChristianPavilonis
force-pushed
the
feature/ts-cli-base
branch
from
2ea46f1 to
edb7f0c
Compare
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.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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, andcargo test -p trusted-server-core— 1426 passed). The canary comment is prose because it has no single clean fix.
Non-blocking
♻️ refactor
request_body_bytesis an incomplete abstraction (proxy) — see inline atcrates/trusted-server-core/src/proxy.rs:44request_body_bytesis an incomplete abstraction (request signing) — see inline atcrates/trusted-server-core/src/request_signing/endpoints.rs:27
🤔 thinking
- Unbounded pre-allocation from operator-controlled
envelope_len— see inline atcrates/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 disabledwarning dropped — the oldload_settingsemitted a startuplog::warn!whenproxy.certificate_checkwas 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 inget_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; thefrom_tomltest 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 atts config pushtime, which is the right place — flagging the migration implication for existing operator configs. - 🌱 Minor:
adapter-fastly/src/main.rsopens the sameTRUSTED_SERVER_CONFIG_STOREtwice per request (open_trusted_server_config_storefor the flag +edgezero_config_store_handlefor dispatch) — harmless, mildly wasteful. Separately,.github/workflows/test.yml's "Verify Fastly WASM release build" step still setsTRUSTED_SERVER__PUBLISHER__ORIGIN_URL, now vestigial sincebuild.rsno longer bakes env-derived config.
👍 praise
- EdgeZero git deps pinned from
branch = "main"to a fixedrev— reproducible builds. - Integrity model in
resolve_fastly_chunk_pointer+settings_from_config_blob: per-chunk len+sha, envelope len+sha, andBlobEnvelope::verify()— solid defense-in-depth, withtampered_blob_hash_is_rejectedandstrings_that_look_like_json_scalars_round_trip_as_stringsguarding the TOML→JSON→TOML secret round-trip. build.rsreduced 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
| fn request_body_bytes( | ||
| body: EdgeBody, | ||
| _endpoint: &str, | ||
| ) -> Result<bytes::Bytes, Report<TrustedServerError>> { | ||
| Ok(body.into_bytes().unwrap_or_default()) | ||
| } |
There was a problem hiding this comment.
♻️ refactor — request_body_bytes takes an _endpoint: &str it never uses and returns a Result it never fails — it just maps a streaming body to empty bytes via unwrap_or_default() (the pinned edgezero Body::into_bytes() returns None for streams). The signature implies the intent — matching the new auction_rejects_streaming_body_instead_of_treating_as_empty test — was to reject streaming bodies. Make it actually reject, naming the endpoint:
| fn request_body_bytes( | |
| body: EdgeBody, | |
| _endpoint: &str, | |
| ) -> Result<bytes::Bytes, Report<TrustedServerError>> { | |
| Ok(body.into_bytes().unwrap_or_default()) | |
| } | |
| fn request_body_bytes( | |
| body: EdgeBody, | |
| endpoint: &str, | |
| ) -> Result<bytes::Bytes, Report<TrustedServerError>> { | |
| if body.is_stream() { | |
| return Err(Report::new(TrustedServerError::BadRequest { | |
| message: format!("{endpoint} request body must be buffered, not streamed"), | |
| })); | |
| } | |
| Ok(body.into_bytes().unwrap_or_default()) | |
| } |
(Scratch-verified: cargo fmt --check, cargo clippy -p trusted-server-core --all-targets --all-features -D warnings, cargo test -p trusted-server-core — 1426 passed, 0 failed.)
| fn request_body_bytes( | ||
| body: EdgeBody, | ||
| _endpoint: &str, | ||
| ) -> Result<bytes::Bytes, Report<TrustedServerError>> { | ||
| Ok(body.into_bytes().unwrap_or_default()) | ||
| } |
There was a problem hiding this comment.
♻️ refactor — Same incomplete abstraction as in proxy.rs: _endpoint is unused and the Result never fails, so a streaming body becomes empty bytes. This matters more here — handle_rotate_key treats an empty body as a valid no-kid rotation, so a streamed rotate-key body would be silently accepted rather than rejected. Make the helper actually reject streaming bodies:
| fn request_body_bytes( | |
| body: EdgeBody, | |
| _endpoint: &str, | |
| ) -> Result<bytes::Bytes, Report<TrustedServerError>> { | |
| Ok(body.into_bytes().unwrap_or_default()) | |
| } | |
| fn request_body_bytes( | |
| body: EdgeBody, | |
| endpoint: &str, | |
| ) -> Result<bytes::Bytes, Report<TrustedServerError>> { | |
| if body.is_stream() { | |
| return Err(Report::new(TrustedServerError::BadRequest { | |
| message: format!("{endpoint} request body must be buffered, not streamed"), | |
| })); | |
| } | |
| Ok(body.into_bytes().unwrap_or_default()) | |
| } |
(Scratch-verified: cargo fmt --check, cargo clippy -p trusted-server-core --all-targets --all-features -D warnings, cargo test -p trusted-server-core — 1426 passed, 0 failed.)
| .change_context(TrustedServerError::Configuration { | ||
| message: "Failed to validate configuration".to_string(), | ||
| })?; | ||
| let mut envelope_json = String::with_capacity(pointer.envelope_len); |
There was a problem hiding this comment.
🤔 thinking — envelope_len comes straight from the operator-written config-store pointer and is used to pre-size the buffer before any chunk is read or validated. A corrupt/huge value triggers a large allocation before the post-concat length check can reject it. Operator-controlled surface, so low severity — but the capacity hint buys nothing here; push_str grows as needed and the existing length/sha checks still validate the result:
| let mut envelope_json = String::with_capacity(pointer.envelope_len); | |
| let mut envelope_json = String::new(); |
(Scratch-verified: cargo fmt --check, cargo clippy -p trusted-server-core --all-targets --all-features -D warnings, cargo test -p trusted-server-core — 1426 passed, 0 failed.)
| /// Keep the request as a non-fatal diagnostic so the EdgeZero CI job still runs | ||
| /// the EC lifecycle scenarios instead of failing on a routing canary that is not | ||
| /// stable across runtime versions. | ||
| pub fn assert_edgezero_entry_point(base_url: &str) -> TestResult<()> { |
There was a problem hiding this comment.
🤔 thinking / 📌 out of scope — This canary was downgraded from a hard 405 assertion to a non-fatal log::warn! + Ok(()). Its original purpose was to prove the integration-tests-edgezero CI job actually exercises edgezero_main rather than silently falling back to legacy_main (the EC lifecycle scenarios pass on both paths). As a warning it can no longer fail CI, so a fixture/config-store regression that drops the EdgeZero job onto the legacy path would green both jobs — which is the exact failure this guard existed to catch, and directly relevant given this PR rewires runtime config loading.
The flaky-405 concern is legitimate, but consider restoring a deterministic EdgeZero-only signal (e.g. a header or route that only edgezero_main emits) rather than removing the guard, or track it as a follow-up issue. Was the downgrade intentional, or just to get CI green?
Apply manually — no single-line fix; needs a stable EdgeZero-only assertion.
3 participants
Summary
tsoperator CLI crate and binary.Changes
crates/trusted-server-cli/crates/trusted-server-core/src/config_payload.rscrates/trusted-server-core/src/settings_data.rscrates/trusted-server-adapter-fastly/src/app.rs,src/main.rs.github/workflows/*,.cargo/config.tomledgezero.toml,trusted-server.example.toml,.gitignoredocs/guide/cli.md,docs/guide/getting-started.md,README.mdCloses
No issue provided; this PR is split out from the combined
feature/ts-cli-nextbranch.Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test --package trusted-server-cli --target aarch64-apple-darwin— 12 passedChecklist
unwrap()in production code — useexpect("should ...")println!)