Skip to content

fix(tpm): preserve signature metadata in chained verification - #557

Merged
imran-siddique merged 1 commit into
agentrust-io:mainfrom
noah-ing:fix/tpm-chained-signature-metadata
Aug 25, 2026
Merged

fix(tpm): preserve signature metadata in chained verification#557
imran-siddique merged 1 commit into
agentrust-io:mainfrom
noah-ing:fix/tpm-chained-signature-metadata

Conversation

@noah-ing

@noah-ing noah-ing commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What

Forward the complete ParsedSignature from verify_tpm_quote_chained() to Agent Manifest instead of stripping it to raw signature bytes. Raise the dependency floor to agent-manifest>=0.11.1 and 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 only parsed.signature into 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 ParsedSignature verification 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=xml passes — 1,309 passed, 6 skipped; 85.70% coverage
  • ruff check src/ tests/ passes
  • mypy src/ passes — 63 source files
  • Manual test performed
  • bandit -r src/ -c pyproject.toml passes — no findings
  • pip-audit --skip-editable --ignore-vuln CVE-2026-69247 passes — no known vulnerabilities beyond the repository-documented ignore
  • AGT governance verification passes — runtime evidence 6/6, OWASP ASI 10/10
  • 10,000-call benchmark passes — p99 911 µs against the 5,000 µs gate
  • Wheel and source distribution build, twine check, and isolated smoke installs pass

Manual red/green used a clean environment with published agent-manifest==0.11.1 confirmed importing from site-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

@noah-ing
noah-ing requested review from a team as code owners August 24, 2026 15:56
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label Aug 24, 2026
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

imran-siddique
imran-siddique previously approved these changes Aug 24, 2026

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@imran-siddique

Copy link
Copy Markdown
Member

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 ### Fixed, so CHANGELOG.md now conflicts. Same append-point collision that hit #546 and #548 last week.

Rebase onto e1c9ee77c8d0 (current main) and it goes straight in.

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:

CHANGELOG.md                                      |   2 +
pyproject.toml                                    |   8 +-
src/cmcp_verify/tpm.py                            |  14 +-
tests/unit/test_tpm_chained_signature_metadata.py | 177 ++++++++++++++++++++++
tests/unit/test_tpm_chained_verify.py             |   1 +
5 files changed, 190 insertions(+), 12 deletions(-)

tests/unit/test_tpm_chained_signature_metadata.py and tests/unit/test_tpm_chained_verify.py are 9 passed on the resolved tree against published agent-manifest==0.11.1.

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 agent-manifest 0.11.0 where verify_tpm_quote is typed signature: bytes. Against the 0.11.1 floor this PR raises, the same files pass. Your floor bump is doing exactly what it says, and anyone reviewing this on a stale environment will see the same false red.

Signed-off-by: Noah Ingwers <98993329+noah-ing@users.noreply.github.com>
@noah-ing

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (a2893da, including the requested e1c9ee7) and kept both CHANGELOG entries in order. The two-dot and three-dot diffs are identical: five files, +190/-12. The two focused test files pass 9/9 against published agent-manifest==0.11.1.

Thanks for the careful review—and for calling out the stale-environment false red.

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@imran-siddique
imran-siddique merged commit 80d7439 into agentrust-io:main Aug 25, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:MEDIUM Contributor check flagged MEDIUM risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants