fix(tpm): preserve signature metadata in chained verification - #557
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
imran-siddique
left a comment
There was a problem hiding this comment.
Checked the published artifact rather than the tree, because a dependency floor raise onto a brand-new API is exactly where this project has been caught before: a fix can sit on main, never get a version bump, and PyPI keeps serving the old thing while local editable installs mask it.
Downloaded agent_manifest-0.11.1-py3-none-any.whl from PyPI. agent_manifest/_tpm_verify.py carries class ParsedSignature with sig_alg, hash_alg and signature, and verify_tpm_quote is typed signature: bytes | ParsedSignature with a docstring naming RSASSA, RSAPSS and ECDSA against SHA-256, SHA-384 and SHA-512. So the API this floor depends on is genuinely in the published wheel, and 0.11.1 is current latest. Green CI on this branch means the resolver got that wheel, not a local tree.
The security framing is the part I most want to endorse. This was not an arbitrary-signature bypass, the bytes still had to verify under the legacy RSASSA/SHA-256 defaults, and you said so plainly instead of reaching for the larger claim. The real defect is the mismatch in both directions: a valid RSAPSS/SHA-384 quote failing, and an RSASSA/SHA-256 signature declared as RSAPSS or SHA-384 passing while cMCP reported algorithms it had not applied. Reported-but-discarded metadata is worse than absent metadata, because a verifier downstream reads it as checked.
Scoping it as narrow downstream adoption of #320, and noting verify_tpm_quote_chained() has no current cMCP runtime caller so the claim path is unchanged, is the right level of claim for a one-token diff.
Non-blocking, not worth sending this back for: the pyproject edit drops the comment explaining why 0.6.1 was the floor, the uncaught RuntimeError on a peer-supplied manifest declaring ML-DSA-65 or hybrid without the [pq] extra. >=0.11.1 subsumes the constraint so nothing breaks, but the reason is now gone from the file, and that reason is the kind someone deletes later precisely because it is no longer written down. Worth carrying both rationales next time the floor moves.
Approving and merging.
|
Approval above stands. The merge did not go through, and it is nothing to do with your change. #563 merged about twenty minutes ago and added its own first bullet under Rebase onto I resolved it locally to check it is not hiding anything, and it is not. Purely additive: main's #552 bullet stays first, yours follows it, no other hunk moves. Proof that nothing of #563's got reverted, two-dot and three-dot against main are byte-identical:
Worth recording, because it nearly became a bad review comment: my first run of those two files on the resolved tree gave 4 failures, purely because the environment had |
Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
c0c56e3 to
e39dc2f
Compare
|
Rebased onto current Thanks for the careful review—and for calling out the stale-environment false red. |
imran-siddique
left a comment
There was a problem hiding this comment.
Re-approving. The earlier approval shows as dismissed because the force-push dropped it, not because anything was withdrawn.
The rebase is exactly what I asked for. Verified rather than taken:
Both CHANGELOG entries are present and in the right order under ### Fixed: main's #552 bullet first, then your agent-manifest#320 one. That matches the resolution I computed locally before asking, so nothing of #563's got dropped on the way through.
A real trial merge in a detached worktree applies cleanly, no conflicted paths. Worth saying because I owe you a correction here: when I predicted this PR was conflict-free yesterday I used the three-argument git merge-tree, which printed nothing at all on a genuine CHANGELOG.md conflict. That is why my review said "approving and merging" and then the merge was refused. The false negative was mine, not a race on your side.
One note on your two-dot and three-dot equality: it was true when you wrote it and is not now. I measure two-dot at 17 files, +225/-395 against three-dot at 5 files, +190/-12, because the branch is five commits behind main after #558, #559, #560, #564 and #570 landed. Nothing to do about it, the merge handles it, but it is worth knowing that the check has a shelf life measured in however long the queue takes to move.
The substance is unchanged from the last review: ParsedSignature forwarded whole so the envelope's declared scheme and digest stay authoritative, verified against the published agent-manifest==0.11.1 wheel rather than a local tree.
Merging now.
What
Forward the complete
ParsedSignaturefromverify_tpm_quote_chained()to Agent Manifest instead of stripping it to raw signature bytes. Raise the dependency floor toagent-manifest>=0.11.1and add regressions for RSAPSS/SHA-384 plus mismatched scheme and digest declarations.Why
parse_tpmt_signature()already recovered the declared scheme and digest, but the wrapper passed onlyparsed.signatureinto verification. Bare signatures intentionally use Agent Manifest's legacy RSASSA/SHA-256 compatibility defaults, so the metadata cMCP reported could differ from the algorithms it actually applied: a valid RSAPSS/SHA-384 quote failed, while RSASSA/SHA-256 signatures declared as RSAPSS or SHA-384 passed.#320 shipped algorithm-aware
ParsedSignatureverification in 0.11.1. This is the narrow downstream adoption of that API.verify_tpm_quote_chained()is public but has no current cMCP runtime caller, so the current TRACE claim-verification path is unchanged.Security impact
Narrow positive impact for direct callers of
verify_tpm_quote_chained(): the declared TPM signature scheme and digest now constrain verification instead of being reported but discarded.This was not an arbitrary-signature bypass—the signature bytes still had to verify under the legacy RSASSA/SHA-256 defaults. The change adds no cryptographic implementation and does not alter trust roots, certificate-chain validation, quote bindings, or the current runtime claim path; algorithm dispatch remains owned by Agent Manifest.
Test plan
pytest tests/unit/ tests/conformance/ tests/integration/ -v --tb=short --cov=src --cov-report=xmlpasses — 1,309 passed, 6 skipped; 85.70% coverageruff check src/ tests/passesmypy src/passes — 63 source filesbandit -r src/ -c pyproject.tomlpasses — no findingspip-audit --skip-editable --ignore-vuln CVE-2026-69247passes — no known vulnerabilities beyond the repository-documented ignoretwine check, and isolated smoke installs passManual red/green used a clean environment with published
agent-manifest==0.11.1confirmed importing fromsite-packages. On unchanged cMCP, all three new cases failed: valid RSAPSS/SHA-384 returned false, while false hash and false scheme declarations returned true. After the change, those three regressions and the six existing chained-verifier cases all passed. Forcing 0.11.0 made the existing Azure control and RSAPSS case fail, confirming that 0.11.1 is the required floor.DCO sign-off
Developer Certificate of Origin (https://developercertificate.org).