fix(provenance): require cnf.jwk during verification - #258
Conversation
Signed-off-by: altrudev <266135212+altrudev@users.noreply.github.com>
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.
Checks out on my side. 1028 passed, 1 skipped, ruff, check_dashes and mypy all clean, and the
guard is real: deleting the two lines makes all four parameters of
test_missing_or_empty_cnf_jwk_is_refused fail while the nine others pass.
Enumerating the cnf shapes on both branches, signing each body with the trusted key so the
refusal cannot be blamed on the signature:
#252 (b37519e) this branch
cnf absent verifies refused
cnf: null verifies refused
cnf: {} verifies refused
cnf: {"jwk": null} verifies refused
cnf: {"other": "x"} verifies refused
cnf: {"jwk": {}} verifies refused
cnf: {"jwk": ""} verifies refused
cnf: {"jwk": false} verifies refused
Your coverage list names the first four. The other four are the same class and fall to the same
line, so the change is wider than the list claims rather than narrower.
One coverage note, from running the near-miss fix rather than the revert. Replacing the guard with
if embedded is None:
raise ProvenanceError("record carries no cnf.jwk")leaves the whole suite green at 1028 passed, 1 skipped, because all four parameters reduce to
embedded is None. The not is doing work nothing pins: it is what sends {"jwk": {}},
{"jwk": ""} and {"jwk": false} to no cnf.jwk rather than down into the thumbprint path. I
swept fourteen cnf.jwk values under both versions and every one is a ProvenanceError either
way, so this is which refusal rather than whether, and it is not a hole. Adding {"jwk": {}} to the
parametrize list pins the line as written, at the cost of one entry: I ran it, ruff stays clean,
the suite goes to 1029 passed, 1 skipped, and the is None version then fails on it.
I am not approving this yet, and the reason is a repository detail rather than anything about the
change. .github/workflows/require-maintainer-approval.yml hardcodes
const MAINTAINERS = ['imran-siddique', 'lywinged'] and gates merge on an APPROVED review from
one of them against the current head SHA, ignoring COMMENTED reviews. So an approval from me here
is not a second opinion, it is that gate going green. The gate is per pull request, so #252's own
check stays red either way, but merging this branch is what carries b051370 and b37519e into
main, and those two commits would arrive without an approval of their own. Your body is already
explicit that this is Draft until #252 merges, so the sequence you describe is the right one and it
is the approval order that needs to follow it.
My reading is that #252 should be reviewed and approved on its own first, then this one against its
own head. If a maintainer would rather I just approve both, I will.
Tool-assisted: the matrix, the sweep and this write-up.
|
Added Keeping this PR in Draft as planned until #252 merges; #252 now has maintainer approval on its current head. |
|
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. |
Dependency
Draft until #252 merges. This is intentionally stacked on PR #252 because #252 first establishes the
cnfobject boundary. Once #252 lands, its predecessor commits should drop from this diff before this PR is moved out of draft.Closes #255.
What
The server-provenance format marks
cnf.jwkas required. After #252,verify_record()safely establishescnfas an object, but missing,null, or emptycnfstill preserves the old behavior: the embedded confirmation-key check is skipped and the record is verified only against the externally supplied trusted key.This follow-on makes the required signed binding load-bearing:
The existing RFC 7638 thumbprint comparison then runs unconditionally for a present key.
Regression coverage
The #252 matrix is retained for non-object
cnfvalues. Its missing/null/empty-object control is deliberately changed to the required-field rule, and extended with falsey embedded-JWK cases that pin theif not embeddedguard:cnf->ProvenanceErrorfrom the object boundary;cnf->ProvenanceError;cnf: null->ProvenanceError;cnf: {}->ProvenanceError;cnf: {"jwk": null}->ProvenanceError;cnf: {"jwk": {}}->ProvenanceError;The records are signed over the exact malformed/edge bodies with the externally trusted key, so the refusal is not attributable to an unrelated bad signature.
Scope
No new trust-anchor mechanism and no unauthenticated-forgery claim. The external trusted key still gates authentication. This makes the reference verifier enforce the confirmation-key member the provenance format already marks required.
AI-assistance disclosure: ChatGPT assisted with source triage, adversarial-case design, implementation drafting, stacked-branch preparation, and diff review.
altrudevreviewed the bounded claim and remains responsible for the contribution.