Repository navigation
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: security-scanner — Security Review
Security Review
No other security issues found. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for tackling this critical security issue! Great job on the detailed explanation and comprehensive test coverage. Before merging:
Refer to CONTRIBUTING.md for guidance. |
🤖 AI Agent: test-generator — `agent-os/src/agent_os/mcp_message_signer.py`
|
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. [Security-critical bug fix with breaking changes; clean implementation but requires migration planning.]
Action items:
Warnings:
|
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
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. |
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
LHMQ878
changed the title
There was a problem hiding this comment.
Pull request overview
Hardens agent_os’s Python-only MCPMessageSigner against signature forgery by making the signed canonical string injective (field-boundary-safe) via length-prefixed framing and a distinct None marker, and adds targeted regression tests covering the known collision/forgery cases.
TL;DR: 0 blockers, 2 warnings. Fine as follow-ups.
| # | Sev | Issue | Where |
|---|---|---|---|
| 1 | Warn | Docstring slightly overstates “each field is length-prefixed” even though None uses a marker |
mcp_message_signer.py:_canonical_field |
| 2 | Warn | New tests add more is True / is False boolean assertions (repo guidance prefers truthiness checks) |
test_mcp_message_signer.py |
Changes:
- Replace ambiguous
|-joined canonicalization with framed fields to prevent cross-field reframing attacks. - Introduce
_canonical_field()and update_build_canonical_string()to use framed components (including aNonemarker). - Add regression + injectivity tests that fail on the vulnerable implementation and pass with the fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
agent-governance-python/agent-os/src/agent_os/mcp_message_signer.py |
Implements length-prefixed canonicalization to prevent signature collisions/forgery. |
agent-governance-python/agent-os/tests/test_mcp_message_signer.py |
Adds focused tests for boundary-shift forgeries, None vs "", and injectivity. |
Comments suppressed due to low confidence (2)
agent-governance-python/agent-os/tests/test_mcp_message_signer.py:99
- Avoid
is Falsefor boolean assertions; usenot <expr>so the assertion is about truthiness rather than object identity (aligns with the repo's Python CodeQL guidance).
assert result.is_valid is False
assert result.payload is None
agent-governance-python/agent-os/tests/test_mcp_message_signer.py:107
- Avoid
is Falsefor boolean assertions; usenot <expr>so the assertion is about truthiness rather than object identity (aligns with the repo's Python CodeQL guidance).
assert self._signer().verify_message(forged).is_valid is False
| Each field is prefixed with its own length, so a separator appearing | ||
| inside a field is just one of that field's characters and cannot be read | ||
| as the end of it. ``None`` gets a distinct marker rather than being | ||
| folded into the empty string, so an absent sender and an empty sender | ||
| do not sign identically. |
| assert result.is_valid is False | ||
| assert "Invalid signature" in result.failure_reason |
| signer = self._signer() | ||
| result = signer.verify_message(signer.sign_message(payload, sender_id=sender_id)) | ||
|
|
||
| assert result.is_valid is True |
07335d6 to
605c1d8
Compare
github-actions
Bot
added
the
documentation
|
Three of the findings converge on the same point ( The old format cannot be a verification fallback. This is the part worth being explicit about, because "provide a migration path" usually means accepting both formats for a release. Here that would keep the vulnerability: the ambiguity is the exploit, and an attacker chooses which format their envelope claims to be, so a verifier that accepts old signatures is still forgeable. There is no migration window available — the change is to what a signature means, not to how it is encoded. The API surface is unchanged. What a consumer does: upgrade signer and verifier together, and re-sign anything stored signed and verified later (there is no way to convert an old signature without the key). On the specific note that there is no in-repo consumer, so this affects external callers only: On the
|
github-actions
Bot
added
size/L
`_build_canonical_string` joined the signed fields with a plain separator:
f"{nonce}|{timestamp_ms}|{sender_id or ''}|{payload}"
`|` is legal inside an MCP payload and inside a sender id, so the encoding
is not injective: distinct (nonce, timestamp, sender_id, payload) tuples
collapse to the same canonical string and therefore to the same HMAC.
An attacker holding one valid envelope can forge others without the
signing key, by moving text across a field boundary and reusing the
signature verbatim:
signed: sender_id="alice", payload="alpha|beta"
forged: sender_id="alice|alpha", payload="beta" -> verifies
The dangerous direction is the reverse, which prepends attacker-chosen
text to the payload a consumer will act on:
signed: sender_id="alice|INJECTED", payload="x"
forged: sender_id="alice", payload="INJECTED|x" -> verifies
`sender_id or ''` also erased the distinction between an absent sender and
an empty one, so an envelope signed with `sender_id=None` verified as one
sent by `""`.
Every field is now length-prefixed, and `None` gets a marker distinct from
the empty string. The reader of each field is told how many characters it
has before reading them, so no boundary can move and the encoding is
injective. Verified over 270 tuples built from separator-heavy, marker-
shaped and prefix-shaped field values: 0 collisions.
The signature format changes, so a signer and a verifier must be upgraded
together; envelopes signed by an older version will not verify against
this one. There is no in-repo consumer -- `MCPMessageSigner` is exported
from `agent_os` for external callers only -- and the previous format
cannot be kept as a fallback without keeping the forgery.
10 regression tests: 4 of the 19 tests in the file fail without the
change.
Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
`injective` is the precise term for the property the fix rests on -- the length-prefixed encoding maps distinct field tuples to distinct strings -- so it goes in the dictionary rather than being paraphrased away. `neighbours` becomes `neighbors`, matching the spelling already used elsewhere in the tree. The job's other reported words come from files this branch does not touch; that is a defect in how the changed-line set is computed, fixed separately in microsoft#3530. Signed-off-by: LHMQ878 <230791102+LHMQ878@users.noreply.github.com>
The signature format change is consumer-visible and this file is where the repo
records those, so the entry says what an external caller has to do rather than
only that something broke.
Two things it states explicitly. That the old format cannot be accepted as a
verification fallback: the ambiguity is what the forgery exploits, and an
attacker chooses which format their envelope claims to be, so a compatibility
window would keep the vulnerability rather than ease the migration. And that the
Python API is unchanged -- `sign_message` and `verify_message` keep their
signatures and return types, and a stale signature comes back as
`MCPVerificationResult.failed("Invalid signature.")` rather than raising, so the
break surfaces as a verification failure and not an exception a caller has to
start handling.
Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
agent-governance-python/agent-os/src/agent_os/mcp_message_signer.py:283
- The docstring says each field is length-prefixed, but
Noneis encoded as a"-"marker (not length-prefixed). Tweaking the wording avoids a misleading statement about the framing scheme.
Each field is prefixed with its own length, so a separator appearing
inside a field is just one of that field's characters and cannot be read
as the end of it. ``None`` gets a distinct marker rather than being
folded into the empty string, so an absent sender and an empty sender
do not sign identically.
BREAKING_CHANGES.md:30
- This entry states “Every field is now length-prefixed” but
Noneis represented by a"-"marker instead. Updating the wording makes the breaking-change note precisely match the implementation.
`|` is legal inside a payload and inside a sender id, so the encoding was not
injective — distinct field tuples produced the same canonical string and
therefore the same HMAC. Every field is now length-prefixed and `None` gets a
marker distinct from the empty string.
feac2b2 to
d98e13c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
agent-governance-python/agent-os/src/agent_os/mcp_message_signer.py:283
- The docstring says “Each field is prefixed with its own length”, but
Noneis encoded as the sentinel"-"(not length-prefixed). Tightening this wording helps avoid confusion for anyone re-implementing the canonicalization in another language.
"""Return *value* framed so it cannot be confused with its neighbors.
Each field is prefixed with its own length, so a separator appearing
inside a field is just one of that field's characters and cannot be read
as the end of it. ``None`` gets a distinct marker rather than being
folded into the empty string, so an absent sender and an empty sender
do not sign identically.
BREAKING_CHANGES.md:30
- This paragraph says “Every field is now length-prefixed”, but the implementation encodes
Noneas a sentinel marker ("-") rather than a length-prefixed value. Clarifying the wording here will make the breaking-change note precisely match the new format.
`|` is legal inside a payload and inside a sender id, so the encoding was not
injective — distinct field tuples produced the same canonical string and
therefore the same HMAC. Every field is now length-prefixed and `None` gets a
marker distinct from the empty string.
|
The encoding change itself is fine. I checked it for collisions, including Separately, the PR body says no |
…coding Section 7.4 still specified `canonical = payload + nonce + timestamp + (sender_id or "")`, the plain concatenation this branch replaced. Other SDKs implement against the spec, so leaving it would keep publishing the ambiguity as the interoperable format even after the Python implementation stopped using it -- and the text was already self-contradictory, promising a separator it then omitted. 7.4 now gives the length-framed encoding, the field order the implementation actually uses, the null-sender marker, the milliseconds-since-epoch rendering of the timestamp, a worked example, and an explicit prohibition on accepting the old form (accepting both preserves the forgery, since the attacker picks which form to claim). Two adjacent errors in 7.3 fixed while here: the signature is base64-encoded, not hex, and `timestamp` is a timezone-aware instant rather than a string. The spec could drift this far because no test read it. Three conformance assertions now pin 7.4 against the literals in the spec text rather than against whatever the code emits. Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
|
Both right, and the spec point is the more important of the two. Fixed in Section 7.4You are right that leaving it would keep publishing the ambiguity as the interoperable format — other SDKs sign against the spec, so the Python implementation quietly moving off it is worse than either one alone. The text was also self-contradictory as written: "concatenating the following fields with a separator", followed by a formula with no separator in it. 7.4 now specifies the length-framed encoding, and states the injectivity requirement as the normative property rather than just showing a formula that happens to satisfy it — a future editor changing the framing needs to know what the framing is for: with the worked example Two more divergences in the same areaChecking 7.4 against the implementation turned up two things beyond the encoding, both in §7.3 and both pre-dating this branch:
Verified rather than read off the source: the signature round-trips through Why it driftedNothing in
Discrimination: substituting the old One caveat on verification: PR bodyCorrected — thanks for catching it. That paragraph was written before |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
agent-governance-python/agent-os/src/agent_os/mcp_message_signer.py:318
timestamp_ms = int(timestamp.timestamp() * 1000)goes through a floating-point POSIX timestamp, which can introduce off-by-1ms rounding on some platforms/values (and can break cross-implementation/spec conformance). Derive epoch milliseconds using integertimedeltaarithmetic instead to make the canonical string deterministic.
timestamp_ms = int(timestamp.timestamp() * 1000)
return "|".join(
cls._canonical_field(field)
for field in (nonce, str(timestamp_ms), sender_id, payload)
)
docs/specs/MCP-SECURITY-GATEWAY-1.0.md:600
- The spec says fields are prefixed with their length “in characters”, but that’s ambiguous for Unicode across languages (e.g., UTF-16 code units in JS vs Unicode code points in Python). Please define the length unit precisely so independent implementations compute identical canonical strings.
Each field MUST therefore be framed with its own length in characters,
and the fields MUST be joined in the order below:
docs/specs/MCP-SECURITY-GATEWAY-1.0.md:622
- “base64-encoded” is underspecified and can lead to interop failures (standard base64 vs base64url; padded vs unpadded). Since the Python implementation produces RFC 4648 standard base64 with '=' padding, the spec should nail this down explicitly.
The signature MUST be base64-encoded. **[Pure Specification]**
Conflict in BREAKING_CHANGES.md is a slot collision, not a semantic one: main added the HostSession snapshot-shape entry where this branch added the signer canonical-string entry, both at the top of a newest-first file. Kept as two separate entries, this branch's first.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
docs/specs/MCP-SECURITY-GATEWAY-1.0.md:604
- The pseudocode should match the clarified definition of length by using the UTF-8 byte length of the field value (not a language-native string length).
= len(v) + ":" + v otherwise
docs/specs/MCP-SECURITY-GATEWAY-1.0.md:599
- “length in characters” is ambiguous for a multi-language spec (UTF-16 code units vs Unicode code points vs UTF-8 bytes) and will lead to signature mismatches across implementations. Since the HMAC input is explicitly UTF-8(canonical), define the length as the number of bytes in the field’s UTF-8 encoding.
This issue also appears on line 604 of the same file.
Each field MUST therefore be framed with its own length in characters,
docs/specs/MCP-SECURITY-GATEWAY-1.0.md:622
- “base64-encoded” is underspecified (standard vs urlsafe alphabet, with/without padding). For interoperability, specify RFC 4648 standard base64 with '=' padding (matching Python base64.b64encode()).
The signature MUST be base64-encoded. **[Pure Specification]**
agent-governance-python/agent-os/tests/test_spec_mcp_gateway_conformance.py:333
- This test constructs the timestamp from a decimal float;
1785448158.059is not exactly representable as a binary float, sodatetime.fromtimestamp(...)can round to an adjacent millisecond and make this assertion flaky across platforms/Python builds. Build the timestamp using integer seconds + timedelta(milliseconds=59) instead.
timestamp=datetime.fromtimestamp(1785448158.059, tz=timezone.utc),
|
Closing per maintainer decision; this repository is not accepting submissions from this account. |
Fixes #3507
Problem
_build_canonical_stringjoined the signed fields with a plain separator:|is legal inside an MCP payload and inside a sender id, so the encoding is not injective: distinct(nonce, timestamp, sender_id, payload)tuples collapse to the same canonical string and therefore to the same HMAC. An attacker holding one valid envelope can forge others without the signing key, by moving text across a field boundary and reusing the signature verbatim.sender_idpayloadalicealpha|betaalice|alphabetaalice|INJECTEDxaliceINJECTED|xNonep""pThe second pair is the dangerous direction: it prepends attacker-chosen text to the payload a consumer will act on. The third is a separate collision --
sender_id or ''erased the difference between an absent sender and an empty one.The module's other defences are all sound and all miss this. Replay protection does not apply, because a receiver has its own nonce store and has never seen the reused nonce (and the forgery can be delivered instead of the original rather than after it). The HMAC-before-nonce-commit ordering is correct and deliberate.
hmac.compare_digestis used correctly -- the comparison is not the problem, the string being signed is. Andtest_verify_detects_tampered_payloadpasses because it changes the payload without compensating elsewhere; the forgery preserves the concatenation invariant.Change
Every field is length-prefixed, and
Nonegets a marker distinct from the empty string:The reader of each field is told how many characters it has before reading them, so a separator inside a field is just one of that field's characters and cannot be read as the end of it. The timestamp is rendered before framing so it is covered the same way as the rest.
Compatibility
This changes the signature format. A signer and a verifier must be upgraded together; envelopes signed by an older version will not verify against this one. That is unavoidable -- the old format cannot be accepted as a fallback without keeping the forgery available.
The blast radius inside AGT is nil:
MCPMessageSigneris exported fromagent_os/__init__.pyfor external callers only, and nothing else in the tree callssign_message/verify_message. It is also Python-only -- no Rust, Go, .NET or TypeScript twin exists -- so there is no cross-SDK parity change to make.BREAKING_CHANGES.mdentry added ind98e13c7. It records that the old plain-concatenation format cannot be accepted as a verification fallback: the ambiguity is the exploit, and the attacker chooses which format their envelope claims to be, so a verifier that still honours old signatures remains forgeable. The API surface is unchanged -- a stale signature comes back asMCPVerificationResult.failed("Invalid signature.")rather than raising.Verification
"|","a|","|a","a|b"), values shaped like the length prefix itself ("1:a","2:ab"), and values equal to the absent-value marker ("-") -- 270 distinct canonical strings, 0 collisions. Pinned as a test.test_mcp_message_signer.pyfail against unmodifiedmcp_message_signer.py. All 19 pass with the change."3:abc","5:hello"), content equal to the marker ("-"), and non-ASCII content.-k mcp), 0 failed.ruff checkoutput on the two files is identical to base apart from shifted line numbers (8 pre-existing findings, unchanged).ruff format --checkflags the same one file on base and on the branch; the test file stays clean.Checklist