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
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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 blob —
crates/trusted-server-core/src/config.rs:92(inline).
Non-blocking
🤔 thinking
- Settings reloaded up to 3×/request —
crates/trusted-server-adapter-fastly/src/main.rs:355,383(inline). - Dead build-time env exports —
scripts/integration-tests.sh:54-58(inline). - EdgeZero entry-point canary downgraded to a warning —
crates/trusted-server-integration-tests/tests/common/ec.rs:296(inline). - Auction test name overstates coverage —
crates/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
Resultbody wrappers with unused_endpoint—crates/trusted-server-core/src/proxy.rs:41,crates/trusted-server-core/src/request_signing/endpoints.rs:27(inline). - Track the EdgeZero
feature/extensible-clibranch —Cargo.toml:39-44,crates/trusted-server-integration-tests/Cargo.toml:13(inline suggestions). Verified compiling against branch HEADbf177b13.
🌱 seedling / 📝 note
prebid::validate_config_for_startuplacks a direct unit test —crates/trusted-server-core/src/integrations/prebid.rs:201(inline).- Integration
cargo testnow 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 viaedgezero-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
| edgezero-adapter = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | ||
| edgezero-adapter-axum = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | ||
| edgezero-adapter-cloudflare = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | ||
| edgezero-adapter-fastly = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | ||
| edgezero-cli = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc" } | ||
| edgezero-core = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } |
There was a problem hiding this comment.
♻️ refactor — Track the in-flight EdgeZero CLI branch rather than the superseded rev. PR #799 depends on EdgeZero CLI APIs that have since moved to feature/extensible-cli (stackpop/edgezero#269). Point all six crates at it:
| edgezero-adapter = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | |
| edgezero-adapter-axum = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | |
| edgezero-adapter-cloudflare = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | |
| edgezero-adapter-fastly = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | |
| edgezero-cli = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc" } | |
| edgezero-core = { git = "https://github.com/stackpop/edgezero", rev = "89f592665a8b386111995da5cbf3fcc068113dfc", default-features = false } | |
| edgezero-adapter = { git = "https://github.com/stackpop/edgezero", branch = "feature/extensible-cli", default-features = false } | |
| edgezero-adapter-axum = { git = "https://github.com/stackpop/edgezero", branch = "feature/extensible-cli", default-features = false } | |
| edgezero-adapter-cloudflare = { git = "https://github.com/stackpop/edgezero", branch = "feature/extensible-cli", default-features = false } | |
| edgezero-adapter-fastly = { git = "https://github.com/stackpop/edgezero", branch = "feature/extensible-cli", default-features = false } | |
| edgezero-cli = { git = "https://github.com/stackpop/edgezero", branch = "feature/extensible-cli" } | |
| edgezero-core = { git = "https://github.com/stackpop/edgezero", branch = "feature/extensible-cli", default-features = false } |
Verified locally against branch HEAD bf177b13: cargo check passes for trusted-server-core (wasm32-wasip1), trusted-server-adapter-fastly (wasm32-wasip1), and trusted-server-cli (host); CLI tests 8/8. branch = ref is not reproducible across CI machines — pin back to a rev before merge.
There was a problem hiding this comment.
Understood, wont merge until edge zero pr is merged
| } | ||
|
|
||
| impl edgezero_core::app_config::AppConfigMeta for TrustedServerAppConfig { | ||
| const SECRET_FIELDS: &'static [edgezero_core::app_config::SecretField] = &[]; |
There was a problem hiding this comment.
❓ question — SECRET_FIELDS is empty, so ts config push serializes the entire Settings — including ec.passphrase, publisher.proxy_secret, ec.partners[].api_token, and handlers[].password — as plaintext JSON into the app_config config store. edgezero.toml declares a dedicated [stores.secrets] that nothing routes to. Is keeping these in the app-config blob intentional (relying on config-store ACLs / parity with the previous wasm-baked secrets), or should they be declared as SecretFields so EdgeZero routes them to the secrets store? Worth resolving before merge.
There was a problem hiding this comment.
Answered/explicitly deferred for this PR. The current ts config push behavior intentionally preserves the existing inline-settings model rather than introducing a partial secret-management feature here. Full secret refs/provisioning require EdgeZero support and are tracked in trusted-server#684 plus stackpop/edgezero#284; this PR should not half-implement nested/array secret extraction.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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 store —
TrustedServerAppConfig::SECRET_FIELDS = &[]keepsec.passphrase,publisher.proxy_secret,handler.password, andec.partners[].api_tokenas plaintext JSON inside theapp_configconfig store. Intentional for this phase? (See inline comment onconfig.rs:92.)
Non-blocking
♻️ refactor
- Auction stream-body handling —
auction/endpoints.rs:81silently treats a streamed body as empty; add an explicitis_stream()guard for parity withproxy.rs/request_signing. - Vacuous
Resultonbody_as_reader—proxy.rs:40andpublisher.rs:35returnResultbut never error; add the stream guard or keep infallible.
🤔 thinking
x-ts-entry-point: edgezeroon 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 crates —
check-integration-dependency-versions.sh; each entry weakens the drift guard.
🌱 seedling
get_settings_from_servicesunused —settings_data.rs:42,pubwith no in-repo caller; confirm it's for future adapters or drop.- Unbounded blob reassembly —
settings_data.rs:114, chunks concatenated before the total-length check.
📝 note
#[serde(deny_unknown_fields)]is a behavior change — existingtrusted-server.tomlfiles 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 type —
Result<(), String>deviates from theerror-stackconvention but is constrained byedgezero_cli's API (run.rs:54).
⛏ nitpick
- Store-id resolution —
default_config_store_namereads raw env whiledefault_config_keyusesEnvConfig(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
AppStatesettings 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_enabledswitched totry_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
| } | ||
|
|
||
| impl edgezero_core::app_config::AppConfigMeta for TrustedServerAppConfig { | ||
| const SECRET_FIELDS: &'static [edgezero_core::app_config::SecretField] = &[]; |
There was a problem hiding this comment.
❓ question — SECRET_FIELDS = &[] opts out of EdgeZero secret-store routing. EdgeZero treats a #[secret] field's value as a reference into [stores.secrets]. With none declared, ec.passphrase, publisher.proxy_secret, handler.password, and ec.partners[].api_token are embedded as plaintext JSON in the app_config config store — even though edgezero.toml already declares a secrets store and request_signing resolves keys from it.
Pre-PR these secrets were baked into the WASM binary (arguably worse), so this is likely not a regression. Is leaving SECRET_FIELDS empty an intentional phase-1 decision, or should the app-level secrets migrate to the secrets store?
| compat::to_fastly_response(response).send_to_client(); | ||
| } | ||
|
|
||
| fn mark_edgezero_entry_point(response: &mut HttpResponse) { |
There was a problem hiding this comment.
🤔 thinking — x-ts-entry-point: edgezero is added to every EdgeZero-path response, including those served to real clients. It usefully drives the CI canary in assert_edgezero_entry_point, but it also exposes internal routing / migration state externally. Consider gating it to non-prod, namespacing it, or documenting it as an intentional diagnostic header.
|
|
||
| fn body_as_reader(body: EdgeBody) -> Cursor<bytes::Bytes> { | ||
| Cursor::new(body.into_bytes().unwrap_or_default()) | ||
| fn body_as_reader(body: EdgeBody) -> Result<Cursor<bytes::Bytes>, Report<TrustedServerError>> { |
There was a problem hiding this comment.
♻️ refactor — body_as_reader now returns Result but always yields Ok; the ? at call sites adds no real error path. For response bodies a streamed body is still silently emptied by into_bytes().unwrap_or_default(). Either add the stream-rejection guard (symmetry with the new request-body helpers) or keep the function infallible to avoid misleading error plumbing. Same applies to publisher.rs:35.
| /// Returns [`TrustedServerError::Configuration`] when the config blob is | ||
| /// missing, cannot be read, fails envelope verification, or fails Trusted | ||
| /// Server settings validation. | ||
| pub fn get_settings_from_services( |
There was a problem hiding this comment.
🌱 seedling — get_settings_from_services is pub but has no in-repo caller (the Fastly adapter uses get_settings_from_config_store via load_settings_from_config_store). Presumably staged for future Cloudflare/Axum/Spin adapters that carry RuntimeServices — confirm that intent or drop it to avoid an unused public surface.
| } | ||
| /// Returns the default `EdgeZero` app-config store name. | ||
| #[must_use] | ||
| pub fn default_config_store_name() -> StoreName { |
There was a problem hiding this comment.
⛏ nitpick — default_config_store_name reads the raw env var directly while default_config_key (just below) resolves through EnvConfig::from_env().store_key(...). Routing both through EnvConfig keeps a single source of truth for the app_config store identifiers.
| message: "Failed to validate configuration".to_string(), | ||
| })?; | ||
| let mut envelope_json = String::new(); | ||
| for chunk in pointer.chunks { |
There was a problem hiding this comment.
🌱 seedling — Chunks are concatenated into envelope_json before the total envelope_len check, so there's no upper bound on reassembled size until after the fact. Only an operator with config-store write access could abuse this, so risk is low — but an early cumulative-size cap would harden the path.
| /// | ||
| /// Returns an error when command parsing, config validation, `EdgeZero` | ||
| /// delegation, or config initialization fails. | ||
| pub fn run_from_env() -> Result<(), String> { |
There was a problem hiding this comment.
📝 note — The CLI uses Result<(), String> rather than the repo's error-stack Report<E> convention. This is constrained by edgezero_cli's String-based public API, so it's a defensible deviation for a thin delegation binary — noting for awareness, no action required.
| # tooling while the integration crate's native test stack resolves different | ||
| # compatible major/minor lines, or no longer resolves the old line after | ||
| # trusted-server-core stopped depending on the config crate. | ||
| "convert_case" |
There was a problem hiding this comment.
🤔 thinking — This PR adds ~18 crates to the parity allowlist (reqwest, hashbrown, winnow, toml_datetime, convert_case, windows-*). Pragmatic, but each entry weakens the shared-dependency drift guard. Worth confirming none of these are actually runtime-shared between the workspace and the integration crate (most look dev/test- or transitive-only).
4 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!)