Repository navigation
Conversation
Wrap the key-rotation admin routes in the CLI so the admin password comes from TRUSTED_SERVER_ADMIN_PASSWORD instead of the command line, and guard permanent deletion behind a confirmation. Extract the admin HTTP client from ts cache purge so both commands share URL checks, auth and redirect handling. Lead the key-rotation docs with the new commands. Closes #1430
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Reviewed 51730bd to 2937f0f across correctness, architecture, code smells, and style. No verified findings.
Checks passed: 21 keys/cache fixture tests, 12 focused unit tests, cargo clippy-cli, changed-file Rust formatting, and git diff --check. Fixture tests exercised unconfirmed deletion refusal, remote plaintext rejection, and redirect refusal.
Not verified: docs Prettier, which was unavailable; real terminal interaction; live HTTPS deployment; and full workspace suites.
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds ts keys rotate and ts keys deactivate on an admin client extracted from ts cache purge. The request and response shapes match the server's RotateKeyResponse and DeactivateKeyResponse, and a 2xx with success: false is reported as a failure. One blocking issue: the --delete prompt counts as interactive when stderr is redirected, so a permanent delete can go through without the operator seeing the prompt.
All 8 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. Each was applied alone and all together on2937f0f72and passedcargo fmt --check,cargo clippy-cliand the host-target CLI tests (Prettier for the docs one). The behavior changes were also checked against the builttsbinary with a local fixture or a mutation run. The two findings under "Cross-cutting" have no inline anchor.
Blocking
🔧 wrench
--deleteprompt counts as interactive when only stdin is a terminal — see inline atcrates/trusted-server-cli/src/commands/keys/mod.rs:91
Non-blocking
♻️ refactor
- Pin the exact
Authorizationheader — see inline atcrates/trusted-server-cli/tests/keys_rotation.rs:126 - Test that
--yesrequires--delete— see inline atcrates/trusted-server-cli/src/commands/keys/mod.rs:341 - Check the URL and password before the delete prompt — see inline at
crates/trusted-server-cli/src/commands/keys/mod.rs:189 - Dead userinfo checks; converge with #1502's
service_endpoint— see inline atcrates/trusted-server-cli/src/admin_client.rs:50
🤔 thinking
- An empty
remaining_active_kidsafter a success means unknown, not none — see inline atcrates/trusted-server-cli/src/commands/keys/mod.rs:222
⛏ nitpick
- Show the netrc file format — see inline at
docs/guide/api-reference.md:796 - Drop the unused re-export — see inline at
crates/trusted-server-cli/src/commands/cache/mod.rs:48
Cross-cutting / body-level findings
- 🏕 Docs outside the diff still lead with curl and an inline password
docs/guide/error-reference.md:419-421and:442-444(the "Signing key not found" and "JWKS endpoint empty" fixes) still showcurl … -u admin:password, the pattern #1430 removes.ts keys rotate --service https://edge.example.comfits both.- The command index in
docs/guide/cli.md(which "summarizes native--helpoutput") has nots keys,ts keys rotateorts keys deactivaterows.ts cacheis missing too. CHANGELOG.md[Unreleased]### Addedhas no entry for the new commands.
- 📝 On the open questions in the description
- y/N or typing the kid: y/N matches
ts audit ad-templates generate, the prompt already names the kid, and the server refuses the two unrecoverable cases (the current key, the last active key) on its own. With the stderr fix above, y/N is enough. --json: not needed for this PR. 🌱 If rotation gets automated, scripts will wantnew_kidwithout parsingnew kid: …. #1502 already hasts keys list --json, so a--jsonthat prints the service's body would be consistent later.
- y/N or typing the kid: y/N matches
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): 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
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
ts keys deactivate --delete now refuses without --yes unless both stdin and stderr are terminals, matching ts audit ad-templates generate, and checks the service URL and admin password before it prompts. An empty remaining_active_kids after a success prints as unknown rather than none. Move the service base-URL join into url_guard::service_endpoint, dropping the unreachable userinfo checks, so the keys list/verify branch shares it. Drop the unused ADMIN_PASSWORD_ENVIRONMENT_VARIABLE re-export from commands::cache. Pin the exact Authorization header and test that --yes requires --delete. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Add ts cache and ts keys rows to the CLI command index, a changelog entry for ts keys rotate and deactivate, and the netrc line format for the curl fallback. Point the error-reference key fixes at ts keys rotate instead of curl with an inline password, and say that --delete needs --yes when stdin or stderr is not a terminal. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
@aram356 Review-body points, addressed in d923873:
|
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 2 at 2b17b185c. Every round-1 finding is fixed, and the fixes are pinned: reverting each one (the requires = "delete" rule, the password in basic_auth, either pre-prompt check, the "unknown" wording) fails a test, and the stderr check behaves correctly in a pty probe. One new blocking issue: text the service returns, in failures and in kid lists, reaches the terminal unescaped, so control sequences can rewrite what the operator reads.
2 of the inline comments below carry a one-click GitHub
suggestion; the kid-escaping fix spans three ranges and is described in prose. Each change, and both proposed regression tests, was applied alone and together on2b17b185cand passedcargo fmt --check,cargo clippy-cliand the host-target CLI tests; each test fails without its fix. The builttsbinary printed no raw ESC bytes with the fixes applied.
Blocking
🔧 wrench
- Failure text from the service reaches the terminal unescaped — see inline at
crates/trusted-server-cli/src/commands/keys/mod.rs:274 - Kids in successful output reach the terminal unescaped — see inline at
crates/trusted-server-cli/src/commands/keys/mod.rs:181
Non-blocking
⛏ nitpick
- Say why the run counts as non-interactive — see inline at
crates/trusted-server-cli/src/commands/keys/mod.rs:199
Cross-cutting / body-level findings
- 📌
ts cache purgeprints its response bodies unescaped too —crates/trusted-server-cli/src/commands/cache/purge.rs:70echoes the success body and:83-86the failure body as received. This predates the PR, so a follow-up issue rather than a change here; the same escaping would close it.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): 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
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
CI ran before #917 landed on main. A local trial merge with current main passes cargo fmt --check, cargo clippy-cli and the host-target CLI suite.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
ts keys rotate and ts keys deactivate match the server's request and response shapes in request_signing/endpoints.rs and rotation.rs, and all of the round-1 fixes are in place. I'm approving on the condition that aram356's two open round-2 🔧 findings land before merge. Text the service returns, including error messages and key IDs, still reaches the terminal unescaped. I read the source and agree with both. One non-blocking item from me.
1 inline comment carries a one-click GitHub
suggestion.
Non-blocking
♻️ refactor
- A dropped reply is reported as "could not reach": see inline at
crates/trusted-server-cli/src/admin_client.rs:90
CI Status
All 28 checks reported for 2b17b185c pass, including cargo fmt, cargo test, format-docs and format-typescript (required), plus cargo test (ts CLI, native) on ubuntu and macos and ts dev lint domains. main has moved since CI ran. I didn't repeat a trial merge.
Escape kids and failure details returned by the service so control sequences cannot rewrite the operator's terminal, say why a --delete run counts as non-interactive, and report a dropped reply separately from a refused connection. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
|
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 3 at 8bd25b30b. The three round-2 findings are fixed and pinned: removing the escape on the failure detail, on new_kid, or on the remaining kids each fails one of the new regression tests. The new dropped-reply message (from prk-Jr's suggestion) is right once the request is on the wire, but the client still misreports both edges: a reply that breaks after its status line gets no retry warning (blocking, because a retried rotation runs twice), and a stalled handshake is reported as a sent request.
All 3 inline comments below carry a one-click GitHub
suggestion; the truncated-reply regression test is in prose because it lives in another file. Each suggestion, and the prose test, was applied alone and together on8bd25b30band passedcargo fmt --check,cargo clippy-cliand the host-target CLI tests; the prose test fails without its fix. The behavior changes were also checked against the builttsbinary with local fixtures for each transport failure.
Blocking
🔧 wrench
- A reply cut off after its status line gets no retry warning — see inline at
crates/trusted-server-cli/src/admin_client.rs:106
Non-blocking
♻️ refactor
- A stalled TCP or TLS handshake is reported as a sent request — see inline at
crates/trusted-server-cli/src/admin_client.rs:82 - Pin both sides of the
is_connect()split — see inline atcrates/trusted-server-cli/tests/keys_rotation.rs:528
Cross-cutting / body-level findings
- 🌱 Show the cause, not reqwest's generic text — every transport probe printed the same
error sending request for url (…), with the URL a second time, whether the connection was refused, timed out or reset (prk-Jr noted this too). Walkingsource()and dropping the URL (without_url()) would make the two messages say what actually happened. - 📝 File the
ts cache purgeescaping follow-up — the issue you proposed in the PR conversation (escapepurge.rs:70and:83-86) isn't filed yet; linking it here keeps it from getting lost once this merges.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): 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
- prepare integration artifacts: PASS
- release scripts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
A local trial merge with current main (598a85170, through #1504) also passes cargo fmt --check, cargo clippy-cli and the host-target CLI suite.
Treat a body that fails after the status line as possibly applied, add a connect timeout so a stalled handshake reports as unreachable, show the error's cause chain without the URL, and pin both sides of the is_connect split with tests. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
Round 3 is addressed in 9045571:
|
Summary
ts keys rotateandts keys deactivateso operators can drive the signing-key admin endpoints from the CLI instead of hand-crafting authenticated requests.ts cache purgeinto a sharedadmin_clientmodule, and the service base-URL validation intourl_guard::service_endpoint(the same helper Add ts keys list and ts keys verify commands #1502 adds), so cache and keys commands use one code path.ts keys deactivate --deletechecks the service URL and admin password before prompting, and refuses without--yesunless both stdin and stderr are terminals.success: false, rather than reporting success.Changes
crates/trusted-server-cli/src/admin_client.rscrates/trusted-server-cli/src/url_guard.rsservice_endpointbase-URL join and validationcrates/trusted-server-cli/src/commands/cache/mod.rs,cache/purge.rscrates/trusted-server-cli/src/commands/keys/mod.rsrotateanddeactivatesubcommands (deactivate supports delete with y/N confirmation)crates/trusted-server-cli/src/commands/mod.rs,lib.rs,run.rskeyscommand and modulecrates/trusted-server-cli/tests/keys_rotation.rsdocs/guide/api-reference.md,docs/guide/key-rotation.md,docs/guide/cli.md,docs/guide/error-reference.md,CHANGELOG.mdCloses
Closes #1430
Spec
Spec: Scoped by issue #1430; no separate spec document.
Overlap with sibling PR
This overlaps with the sibling PR for #1431 (
ts keys list/verify, branchadd-ts-keys-list-verify). Both addcrates/trusted-server-cli/src/commands/keys/mod.rsand editrun.rs. This PR merges first; #1502 rebases onto it and converges on oneKeysCommandand one transport. Both PRs add an identicalurl_guard::service_endpoint.Open questions for reviewers
Delete confirmation is a y/N prompt. Should it instead require typing the kid?Resolved in review: y/N, with stdin and stderr both required to be terminals.Output is plain text, not JSON. Is aResolved in review: not in this PR.--jsonmode wanted?Test plan
cargo test-fastly && cargo test-axum(plustest-fastly-reuse,test-cloudflare,test-spin)cargo clippy-fastly && cargo clippy-axum(all 8 clippy aliases)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(1838 passed, 1 skipped, Node 24.20.0)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servekeys_rotationtests),template_build_digest, parityChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)