fix(provenance): establish cnf object boundary - #252
Conversation
Signed-off-by: altrudev <266135212+altrudev@users.noreply.github.com>
Signed-off-by: altrudev <266135212+altrudev@users.noreply.github.com>
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
lywinged
left a comment
There was a problem hiding this comment.
Reproduced on a clean checkout at b37519e. Four gates green locally: ruff check src tests scripts, tools/check_dashes.py, mypy src/agentrust_trace (Success: no issues found in 10 source files), pytest at 1027 passed, 1 skipped.
The mutation property holds as you state it. Restoring (record.get("cnf") or {}).get("jwk") and
changing nothing else: 8 failed, 1019 passed, 1 skipped. Running the eight parameters against
main gives the split your body describes:
'not-an-object' AttributeError: 'str' object has no attribute 'get'
['unexpected'] AttributeError: 'list' object has no attribute 'get'
1 AttributeError: 'int' object has no attribute 'get'
True AttributeError: 'bool' object has no attribute 'get'
'' accepted, verify_record returns
[] accepted, verify_record returns
0 accepted, verify_record returns
False accepted, verify_record returns
On the falsey half verify_record returns normally, so those records verified rather than merely
skipping a check.
One addition to the evidence rather than a correction to it. The mutation you state is the old
path. There is a second mutant these tests kill, and it is the more plausible wrong fix:
_c = record.get("cnf")
cnf = _as_object(_c, "cnf") if _c else {}That reads as faithful, and it closes the AttributeError in the issue title. It passes the four
truthy parameters and fails exactly the four falsey ones, 4 failed, 8 passed in the new file. So
the falsey half of the matrix is not only covering the second symptom, it is what stops a wrong
fix from looking right. That is the part of this PR I would least want trimmed in review.
Two checks for whoever else reads this.
sign.verify_record already refuses all eight, as ValueError, at the validate_json
schema step (sign.py:518):
record does not conform to the TRACE v0.2 schema at cnf: '' is not of type 'object'.
provenance.verify_record has no schema step, so an explicit guard is the only means it has. The
two verifiers now agree on this field, each by the mechanism available to it, and there is nothing
for this PR to do in sign.py.
spec/server-provenance-v1.md line 106 carries cnf.jwk as required: yes, so no record the
spec calls valid is newly refused. Sweeping what verify_record reads off the record once this
lands, signature is the only one still used as a type nothing has established. Separate from
this PR and not a reason to hold it.
Approving.
Tool-assisted: the matrix, the mutants and this write-up.
|
Batch response for this cluster is here: agentrust-io/agent-manifest#357 (comment) Short version: the finding class is real and welcome. Your CI had never run, held under first-time-contributor gating, until I released 36 runs across your PRs an hour ago, and five of your eight are now red. Please fix those, sequence trace-spec#258 against #252 which touch the same two files, and tell me the order you want them reviewed in. |
imran-siddique
left a comment
There was a problem hiding this comment.
Merging, first in the order you asked for.
Two lines, and the defect behind them is the kind that survives review indefinitely because the code reads as defensive:
embedded = (record.get("cnf") or {}).get("jwk")or {} looks like a guard and is doing two different wrong things. A truthy non-object leaks AttributeError straight past the verifier's documented ProvenanceError boundary, so a caller written to the contract does not catch it. And a falsey non-object, 0, "", [], is silently converted into "no cnf present at all". Absent and malformed are different states, and only one of them is something a verifier should tolerate quietly. The second is the half I would have missed reading this quickly, and it is the one with a real consequence: a record can carry a malformed cnf and be verified as though it carried none.
Routing through _as_object() rather than adding a validator is right. It is the boundary already applied to identity and tool_catalog in the same function, so this removes an inconsistency rather than introducing a rule.
The regression matrix is built correctly, which is worth saying because it is the part that usually is not: signing the malformed and edge records with the trusted key before verification means a rejection cannot be attributed to an unrelated signature failure. A test that rejects for the wrong reason passes and proves nothing.
Six checks, only the maintainer gate red, and independently approved by @lywinged. A peer review from someone who has been finding real defects in this same module all week is worth more here than a second maintainer pass.
Closes #251. This also moves main, so #258 can take the rebase you committed to and drop the stacked predecessor from its diff. #254 is next.
What
Closes #251.
provenance.verify_record()now establishescnfthrough the existing_as_object()boundary before readingjwk.This keeps malformed externally supplied
cnfvalues inside the verifier's documentedProvenanceErrorrejection boundary instead of:AttributeErrorfor truthy non-object values; or(... or {})as thoughcnfwere absent.Missing
cnf, explicitnull, an empty object, and a valid embedded JWK keep their existing behavior.Scope
No schema or normative specification change. No new validator is introduced; this reuses the object-boundary helper already used for
identityandtool_catalogin the same function.Regression evidence
The added matrix signs the actual malformed/edge records with the trusted key before verification, so a rejection cannot be attributed to an unrelated invalid signature.
Covered controls:
null/{}controls;cnf.jwkcontrol.Mutation property: restoring the old
(record.get("cnf") or {}).get("jwk")path makes the truthy vectors escape through host-language exceptions and the falsey vectors cease to be rejected.Non-claim
This is exception-safety and API-contract consistency in the MCP provenance verifier. It does not change provenance trust semantics or claim a cryptographic vulnerability.
AI-assistance disclosure: ChatGPT assisted with source triage, adversarial matrix design, and drafting.
altrudevreviewed the bounded claim and remains responsible for the contribution.