fix(mc-host): authenticate daemon version transcripts - #58
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summary (commit 0be6776)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0be6776)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Reviewed by gemini-3.7-flash · Input: 96.1K · Output: 7.8K · Cached: 244.6K |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0be6776def
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| SERVER_PROOF_DOMAIN, | ||
| clientNonce, | ||
| server.serverNonce, | ||
| server.daemonVer, |
There was a problem hiding this comment.
When an updated client encounters an already-running daemon from the preceding release (or an older client encounters the updated daemon), both peers still advertise wire_version: 2 and use the same subc-*-v1 domains, but they now calculate different HMACs because only one side includes daemon_ver; the handshake therefore fails with proof_mismatch before compatibility policy can inspect the daemon version. Introduce a separately identifiable proof/wire version or an explicit migration path so mixed-generation upgrades remain operable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Investigated this one carefully, and I don't think it holds as stated — a separately identifiable proof version and an explicit migration path both already exist. Declining the change, with the evidence below.
The proof version is already distinct from the wire version. release/mc-host-release.json (mirrored into generated-contract.ts) carries a proof block independent of versions.wire_protocol:
"proof": {
"current_offers": [2],
"current_version": 2,
"legacy_stop_only": {
"adjacent_release_only": true,
"missing_offer_inference": false,
"scope": "stop_only",
"version": 1
},
"transcript_fields": ["offers", "selected_version", "daemon_ver",
"client_nonce", "server_nonce", "daemon_id"]
}So proof.current_version is not conflated with wire_protocol. Two peers that both advertise wire_version: 2 are still separable on proof version.
daemon_ver was already a normative transcript field before this PR. It is listed in proof.transcript_fields above, and that contract was frozen in an ancestor commit (5038833, "harden lifecycle fences and freeze release contract (U1/U8)") — this branch does not modify release/mc-host-release.json at all. This change brings the Rust and TypeScript implementations into conformance with the already-frozen contract rather than introducing a new, unversioned transcript.
The migration path is proof.legacy_stop_only. Proof version 1 remains supported at scope: "stop_only", adjacent_release_only: true. A preceding-generation daemon stays reachable for exactly one operation — stop — so the lifecycle layer can shut it down and start the matching generation. It is deliberately not usable for traffic, which is the intended outcome rather than a gap.
Failing closed here is specified, not incidental. docs/mc-host-wire-protocol.md:834:
Host restart MUST close old listener/generations, mint fresh key and daemon ID, bind and publish under lock, and reject mixed-generation authentication.
It also doesn't degrade into a retry storm or an opaque failure. isShadowDialTransient in client.ts:407 returns false for AuthError, with the documented rule that "authentication and protocol failures stop the episode permanently" — so a mismatch stops the episode instead of hammering the daemon. The CLI taxonomy already surfaces it as a version-alignment problem: both unsupported_proof_version and incompatible_daemon map to remediation align_versions.
One genuinely open item your comment prompted, which I'd rather flag than paper over: offers and selected_version appear in proof.transcript_fields, but neither compute_proof (crates/mc-host/src/auth.rs:135) nor computeProof (auth.ts:85) binds them today — both bind domain, nonces, daemon_ver, and daemon_id only. If proof-version negotiation lands later in this stack, that's expected; if not, it's a contract-versus-implementation drift worth its own change. Either way it's pre-existing and outside this PR, which only adds daemon_ver.
For what it's worth I did verify the cross-language construction is byte-identical (same field order, same big-endian length prefix) and that cargo test -p mc-host --lib auth passes 14/14, including client_auth_is_bound_to_the_server_reported_daemon_version.
| * `HMAC-SHA256(key, ASCII(domain) || client_nonce || server_nonce || daemon_id)` | ||
| * `HMAC-SHA256(key, ASCII(domain) || client_nonce || server_nonce || | ||
| * u32be(len(daemon_ver)) || UTF8(daemon_ver) || daemon_id)` | ||
| * per wire doc Section 5.2. Exported so tests can reproduce the committed |
There was a problem hiding this comment.
This comment says the new construction is defined by wire-doc Section 5.2, but docs/mc-host-wire-protocol.md:196-206 still publishes the old proof arrays and HMAC(... || server_nonce || daemon_id) formula. Because that document declares itself the direct-only wire authority, an independent implementation or conformance vector built from it will reject the updated Rust and TypeScript peers; update the canonical formula and both literal proof vectors in the same change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in a28a915, which is the current head of this branch.
docs/mc-host-wire-protocol.md Section 5.2 now publishes the version-bound formula:
HMAC-SHA256(key, ASCII(D) || client_nonce || server_nonce ||
u32be(len(daemon_ver)) || UTF8(daemon_ver) || daemon_id)
Both literal proof vectors in the document were regenerated and match the committed test vectors byte-for-byte. I diffed them against crates/mc-host/tests/protocol_vectors.rs::committed_auth_proof_vectors_pin_the_construction:
- server proof:
64, 154, 84, 68, 23, 100, 116, 189, ... 91, 108, 85, 131 - client auth:
184, 138, 243, 55, 0, 189, 88, 52, ... 90, 147, 247, 187
The document also now states the inputs those vectors use (key 00..1f, client nonce 20..3f, server nonce 40..5f, daemon_ver mc-host/0.1.0, daemon ID 60..6f) and explains why the u32be length prefix is there: it keeps the transcript injective, because daemon_ver is the only variable-length field between the fixed-length nonces and the trailing daemon_id.
Verified green: cargo test -p mc-host --test protocol_vectors passes 18/18, so an independent implementation built from the document now agrees with both the Rust and TypeScript peers.
The auth transcript now length-prefixes and folds daemon_ver into both proofs; update the normative formula, regenerate the canonical ServerProof/ClientAuth literals, and document the big-endian length prefix so the committed Rust and TypeScript test vectors trace back to the spec.
1 participant
Summary
Daemon version metadata is now authenticated in both handshake directions. The proof transcript length-prefixes the UTF-8 version, and Rust, TypeScript, raw-client, and fake-peer vectors share the same construction.
Verification
cargo test -p mc-host auth --lib --testsbun test packages/plugin/src/shared/mc-host-client/auth.test.ts packages/plugin/src/shared/mc-host-client/client.test.tsStack
Stacked on #46.