Repository navigation
fix(mcp): versioned injective canonical signing shared across Python and Rust - #4320
Kevin Sanjaya (incr3d1ble) wants to merge 4 commits into
Conversation
…and Rust Resolves microsoft#4199. The previous encodings joined variable-length fields with a bare separator (Rust: "{ts}:{nonce}:{payload}", Python: "{nonce}|{ts}|{sender}|{payload}"), so field values containing the separator merged distinct field tuples into one identical HMAC input: nonce "n" + payload "x:y" collided with nonce "n:x" + payload "y", letting a field-reframed envelope verify under an independent nonce store. Introduce one versioned, injective canonical representation (mcp-sig-v2) shared by both SDKs: fields are length-prefixed with their UTF-8 byte length, the millisecond timestamp is placed last, None and "" sender share one encoding, and signatures use standard padded base64 in both SDKs. - Add identical fixed HMAC test vectors to both SDKs, covering separators, JSON-looking text, Unicode, empty vs absent sender, and full timestamp precision. - Add cross-SDK vectors plus a regression proving the n/x:y -> n:x/y reframing is rejected with no legacy fallback. - Document the coordinated breaking migration in docs/security/canonical-signing-v2.md. Signed-off-by: augustkevinsanjaya <120900788+incr3d1ble@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
github-actions
Bot
added
documentation
|
@microsoft-github-policy-service agree |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Thank you for taking #4199. I built both encoders from this head and ran 26 vectors (your 5 plus 21 adversarial: separators in keys, nested prefixes, length-run splits, NUL bytes, a payload equal to a whole canonical string) through Python and Rust: all 26 signatures match and the decoding is unique, so the length-prefixed form is injective over (nonce, sender, payload, timestamp). Both verifiers fail closed. Before this can go further there is one design question for the maintainers and a set of concrete fixes.
- Design question (for Ricky Gummadi (@Ricky-G) and Prayag (@prayagupa), not something the author can settle alone): #4199 presupposes Rust adopts the Python v2 signer from the still-open #4132, which defines v2 as a JSON array with
nulldistinct from""and microsecond timestamps. This PR rewrites the same Python function to a third format (mcp-sig-v2, millisecond precision,Noneequal to""), so two competing v2 formats are now in flight for one file and docs/specs/MCP-SECURITY-GATEWAY-1.0.md section 7.4 matches neither. One format needs to be chosen before either PR lands. - Please fill the repository PR template (Type of Change, Packages Affected, Checklist, Attribution, AI Assistance including the autonomous-submission box, IP). #4199 also asks for a real cross-SDK sign-and-verify test in both directions; the body leaves that box unchecked.
| ``""`` share the single ``0:`` encoding. The millisecond timestamp is | ||
| placed last. | ||
| """ | ||
| timestamp_ms = int(timestamp.timestamp() * 1000) |
There was a problem hiding this comment.
agent-governance-python/agent-os/src/agent_os/mcp_message_signer.py:300 None and "" sender share the 0: encoding while the verifier echoes the caller-asserted sender_id, so a relay can flip a signed message between "no sender" and "empty sender" without breaking the MAC. Please either distinguish them (a type byte, as #4132 does) or state in the spec why the two are equivalent for every consumer.
| fn none_and_empty_sender_share_one_encoding() { | ||
| let signer = vector_signer("n"); | ||
| let without = signer.sign("payload", None).unwrap(); | ||
| let empty = signer.sign("payload", Some("")).unwrap(); |
There was a problem hiding this comment.
agent-governance-rust/agentmesh-mcp/src/mcp/signing.rs:353 none_and_empty_sender_share_one_encoding panics with NonceGeneration("no nonce values remaining"): the deterministic generator is seeded with one nonce and sign() runs twice. cargo test -p agentmesh-mcp on this head is 53 passed, 1 failed. Please seed two nonces or build two signers, and run the Rust suite locally (the body says the Rust side was not compiled).
| /// character (`|`, `:`) inside any field cannot merge two distinct field | ||
| /// tuples into the same HMAC input (#4199). `sender_id=None` and `""` share | ||
| /// the single `0:` encoding. The millisecond timestamp is placed last. | ||
| fn signature_input(timestamp_ms: u64, nonce: &str, sender_id: Option<&str>, payload: &str) -> String { |
There was a problem hiding this comment.
agent-governance-rust/agentmesh-mcp/src/mcp/signing.rs:190 cargo fmt wants this signature wrapped, and the trailing comma at :268 removed; the file was fmt-clean on main. The file also changed from CRLF to LF, which makes the diff whole-file; please keep the original line endings so the real change (+176/-36) is reviewable.
|
|
||
| /// Signed MCP message envelope. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct McpSignedMessage { |
There was a problem hiding this comment.
agent-governance-rust/agentmesh-mcp/src/mcp/signing.rs:27 Renaming timestamp_secs to timestamp_ms, adding sender_id, and switching the base64 alphabet are wire-breaking with no legacy path. Please add BREAKING_CHANGES.md and crate and agent-os CHANGELOG entries, and update the stale sign() snippets in agent-governance-rust/agentmesh-mcp/README.md:33 and agent-governance-rust/agentmesh/README.md:155.
| @@ -0,0 +1,86 @@ | |||
| """Cross-SDK shared HMAC vectors for MCP canonical signing v2 (#4199). | |||
There was a problem hiding this comment.
agent-governance-python/agent-os/tests/test_mcp_signature_cross_sdk.py:1 Missing the Microsoft license header (the License Headers job will fail). At :38 please use pytest's monkeypatch.setattr for _utcnow so a failing test restores the module global.
| @@ -0,0 +1,79 @@ | |||
| # MCP Canonical Signing v2 (`mcp-sig-v2`) — Cross-SDK Spec | |||
There was a problem hiding this comment.
docs/security/canonical-signing-v2.md:1 Needs the docs front-matter block (check_frontmatter --strict fails) and the spell checker flags blen and injective; byte length reads better than blen. Please also replace the em dashes.
Completes the Rust half of microsoft#4199 on top of the microsoft#4132 v2 format. The Rust McpMessageSigner now signs the same compact JSON array as the Python agent-os signer, so the two competing in-flight formats for one canonical encoder collapse into one: microsoft#4132 stays the Python-side definition, this PR adds the Rust implementation, the shared cross-SDK vector table, and the coordinated migration docs. Review items addressed: the single-use nonce test panic (each signer now signs once), fmt-clean formatting, original CRLF line endings for the Rust files (plumbing commit, no filter normalization), Microsoft license header and monkeypatch usage in the Python tests, docs front-matter with no flagged terms, BREAKING_CHANGES.md plus crate and agent-os changelog entries, and the stale sign() README snippets. Absent and empty senders are now distinct signatures, closing the relay flip raised in review. Signed-off-by: augustkevinsanjaya <120900788+incr3d1ble@users.noreply.github.com>
The first pass normalized line endings on the two Rust READMEs and the two changelog files; the agentmesh README carries mixed endings upstream, so the byte-level edit restores its exact per-line endings and the changelogs stay LF. The real diff is now confined to the intended changes. Signed-off-by: augustkevinsanjaya <120900788+incr3d1ble@users.noreply.github.com>
|
Thank you MohammadHaroonAbuomar for the review, and especially for independently running 26 vectors through both encoders; that gave me the confidence to rewrite rather than patch. Every point is addressed in
One honest caveat: full Verification this round: the 9-vector table was regenerated by replicating #4132's |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Thank you, this is a strong second round. Adopting the #4132 array encoding resolves the format conflict: the Python builder is now byte-identical to #4132's head,
nulland""senders are distinct, the Rust suite is 55 passed, fmt is clean, CRLF is restored, the breaking-change and changelog entries, README snippets, license header, monkeypatch fixture, doc front matter and template are all in. I re-ran 19 vectors (your 9 plus 10 adversarial) through both SDKs: identical bytes, and Python-signed envelopes verify in Rust and Rust-signed in Python, 19 of 19 each way, with every tamper and the v1 reframe rejected. Three small items remain before approval. - agent-governance-rust/agentmesh-mcp/README.md:1 This line flipped from LF to CRLF; please restore it so the file keeps its original mixed endings.
- For the maintainers (Ricky Gummadi (@Ricky-G), Prayag (@prayagupa)): this PR and #4132 now carry the same Python builder, so they need sequencing. Either land #4132 first and have this PR rebase and drop its duplicate Python commit, or land this first and have #4132 rebase; whichever lands second should also fix the sentence in #4132's spec text that says Rust still uses the legacy framing. A cross-process interop CI job (one SDK signs, the other verifies, in CI) can stay a tracked follow-up.
| TS_B = datetime(2023, 11, 14, 22, 13, 20, 999999, tzinfo=UTC) | ||
|
|
||
| # (nonce, sender, payload, timestamp, expected base64-standard signature) | ||
| VECTORS = [ |
There was a problem hiding this comment.
agent-governance-python/agent-os/tests/test_mcp_signature_cross_sdk.py:27 The Spell Check job scans added lines as plain text and will flag the base64 fragments in the vector table (Cran, Pxrynub, Ofjfna, Orww, Kshm, Thfmw, ZJILDE, Bmac, Uecnun, Oevfdp, Beqyq, Pknc) plus timespec, utcoffset, Hinnant and uote. Please add them to .cspell-repo-terms.txt, or add a base64 ignore pattern to .cspell.json if the maintainers prefer that.
| ("n", None, "x:y", TS_A, "ulKQJ88YmF6olg4ML3ekYf7vTA2jh7m434nn/bh59ns="), | ||
| ("n:x", None, "y", TS_A, "4LV3y6oukPpuK7KicYSjmRejFuDmA2q2NiDgocNtUZs="), | ||
| ("a|b", "s|t", "p|q", TS_A, "JYCranEAW75tG9xotPxrynub4VUm1JJ04Pn5jrwPknc="), | ||
| ("நி", None, "🚀 {\"json\":[1,2]}", TS_A, "OfjfnaSRytNHOrwwVGIjfVI9+KshmVKThfmw+ZJILDE="), |
There was a problem hiding this comment.
agent-governance-python/agent-os/tests/test_mcp_signature_cross_sdk.py:31 ruff format would rewrite the quote style and three tuples here (:31, :34, :38); please run it on the file.
There was a problem hiding this comment.
Ran ruff format (0.12.4, the version pinned in requirements/ci-lint.txt) on the file, it wrapped the three long tuples and fixed the quote style. Suite still 3/3 after the reformat, and ruff check is clean on both touched Python files.
…e endings Adds the base64 vector fragments and reviewer-named terms to .cspell-repo-terms.txt under a labelled PR section, runs ruff format over the cross-SDK test file (three long tuples wrapped, suite still green), and rebuilds the agentmesh-mcp README from the upstream blob at the byte level so its original mixed line endings survive the one-line sign() change. Signed-off-by: augustkevinsanjaya <120900788+incr3d1ble@users.noreply.github.com>
|
The agentmesh-mcp README is rebuilt from the upstream blob at the byte level now, so line 1 keeps its LF and the diff there is down to the one On sequencing: either order works for me. If #4132 lands first I'll rebase and drop the duplicate Python commit here; if this one lands first, I can send the one-line fix to #4132's section 7.4 (Rust is no longer on the legacy framing) as a small follow-up to that branch. Maintainers' call. |
This branch has not been deployed
2 participants
Related Issue
Resolves #4199 (Rust half). Built on the v2 format defined in #4132.
Problem & Solution
Problem: #4199 and #4132 both touch the same canonical encoder with two different v2 shapes, and the Rust signer still MACs the ambiguous
{timestamp_secs}:{nonce}:{payload}colon join.Solution: this PR now adopts the #4132 v2 format (
["agent-os:mcp-message:v2", nonce, timestamp, sender, payload], compact JSON,ensure_ascii=False, microsecond UTC timestamps,nulldistinct from"") as the single shared representation, and completes #4199 by implementing it in Rust, adding the shared cross-SDK vector table, and documenting the coordinated migration. The Python-side change is byte-identical to #4132's encoder; if #4132 lands first, this PR's Python commit can be dropped without touching the rest.Impact on Your Work
Rust consumers migrate to the v2 envelope (
timestamp_us,sender_id: Option<String>,sign(payload, sender_id)); Python behavior follows #4132. Both SDKs verify each other's signatures. Breaking migration documented inBREAKING_CHANGES.md, the crate CHANGELOG, the agent-os CHANGELOG, anddocs/security/canonical-signing-v2.md.Timeline
None
Alternatives Considered
Keeping the length-prefixed encoding from the first version of this PR. Dropped after review: it created a third competing format for one function while #4132 already has review investment and a spec section (7.4). The JSON array is likewise one-to-one over (nonce, timestamp, sender, payload) with no escaping scheme to maintain.
Type of Change
Package(s) Affected
(plus the Rust
agentmesh-mcpcrate, tracked in this monorepo underagent-governance-rust/)Testing
Unit Testing
tests/test_mcp_signature_cross_sdk.py(Python): 9-vector shared HMAC table, reframing regression, null vs empty-sender distinctness. 3/3 passed locally (Windows, Python 3.13.2, pytest 9.1.1) against the fix(agent-os): prevent MCP signature field reframing #4132 encoder.signing.rs: the same 9 vectors, reframing regression, null/empty distinctness, and a timestamp-formatting test mirroring Pythonisoformat.cargo fmt -p agentmesh-mcp --checkexits 0. Fullcargo testrequires the MSVC linker, which this Windows environment lacks; CI covers the build and test run.Manual Testing
Vector table generated by replicating #4132's
_build_canonical_stringbyte-for-byte (script in the branch history); the table includes the reframing pair, separators in every field, Unicode plus JSON-looking payloads, quotes/backslash/control-char/NUL escaping, a payload equal to a whole canonical string, microsecond precision, and the null/empty-sender pair.Checklist
cargo fmt --checkclean; Python files pass the vector tests; ruff to run in CI)Attribution & Prior Art
Prior art / related projects:
AI Assistance
AI tools were used to draft the encoder/tests/docs; every change was reviewed and executed locally before submission (vector generation run, pytest runs shown above, fmt check run).
IP, Patents, and Licensing
Cross-SDK verification (from #4199)
Both SDKs assert the identical 9-vector HMAC table (
test_mcp_signature_cross_sdk.py/matches_python_v2_shared_vectors), which pins sign-and-verify equivalence in both directions at the byte level; MohammadHaroonAbuomar additionally ran 26 vectors through both encoders with matching results. An automated cross-process interop CI job (Python signing, Rust verifying, and vice versa) is the natural follow-up once one of the two PRs lands and both toolchains exist in CI.Signed-off-by: augustkevinsanjaya 120900788+incr3d1ble@users.noreply.github.com