fix(content-marking): require record.alg during verification - #254
fix(content-marking): require record.alg during verification#254altrudev wants to merge 4 commits into
Conversation
…o#253) 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.
Agreed on the diagnosis and the direction. ref.get("alg", "sha256") leaves the local variable
saying sha256 for a producer that said nothing, so any later check on alg reads a value the
assertion never carried, and the spec row is exact: record.alg | yes.
CI is red on this branch, on the mypy step and not on pytest, which is easy to miss:
src/agentrust_trace/content_marking.py:163: error: Argument 2 to "_digest" has incompatible type "Any | None"; expected "str" [arg-type]
mypy src/agentrust_trace is its own step in .github/workflows/ci.yml, so the build fails even
though pytest is 1018 passed, 1 skipped and ruff and check_dashes are clean. main, #252,
#257 and #258 all report Success: no issues found in 10 source files, so it is new here: the
default made the argument Any, and without it the type checker sees Any | None and has
something to say.
The fix is already written in #253. Its Smallest fix section is two lines:
alg = ref.get("alg")
if alg not in _ALGS:
raise ContentMarkingError(...)and the branch has the first. Adding the second clears the type error, Success: no issues found in 10 source files, with the suite still at 1018 passed, 1 skipped.
One thing to weigh before taking that line as written, and it is about an older defect rather than
about your change. _ALGS is a dict, so alg not in _ALGS raises on an unhashable value before
the membership test can refuse it:
alg=[1] TypeError: unhashable type: 'list'
alg=[] TypeError: unhashable type: 'list'
alg={'a': 1} TypeError: unhashable type: 'dict'
alg={} TypeError: unhashable type: 'dict'
All four reproduce identically on main, because the default only ever substituted for an absent
key. Your change did not create it. But moving the membership test up to the call site moves that
TypeError up with it, and past the record.hash check, so it becomes reachable where today it
is not. Same assertion, alg: [1] with a malformed hash:
today ContentMarkingError: record.hash 'nope' is not a sha256:/sha384: digest
with `alg not in _ALGS` TypeError: unhashable type: 'list'
Narrowing on the type instead keeps the shape of #253's fix and does not do that:
alg = ref.get("alg")
if not isinstance(alg, str):
raise ContentMarkingError(f"unsupported digest algorithm {alg!r}; use sha256 or sha384")I ran it: mypy Success: no issues found in 10 source files, ruff clean, 1018 passed, 1 skipped,
_digest's signature untouched, and ContentMarkingError for all four unhashable values instead
of TypeError. Every hashable wrong value is refused as it is today: 1, True, 1.5, None,
"", 0, False, "sha512" and "SHA256" all reach unsupported digest algorithm ...; use sha256 or sha384. Putting the same check inside _digest instead does not clear mypy, because the
complaint is about the argument at line 163 and not about the body.
One consequence worth a test either way: the guard sits above the record.hash format check, so an
assertion whose alg is not a string and whose hash is also malformed now reports the algorithm
where today it reports the hash. A wrong alg string like "sha512" is unaffected, since the
narrowing does not fire on it and the hash check still runs first.
Second thing, smaller. Reverting only the source change and running your new file:
test_missing_record_alg_is_refused_instead_of_defaulting_to_sha256 FAILED
test_null_record_alg_is_refused PASSED
test_explicit_sha256_and_sha384_still_verify PASSED
ref.get("alg", "sha256") applies its default only when the key is absent, so an explicit null
already reached _digest as None and was already refused on main. After this change both cases
are the same None, so no mutation whose result depends only on ref.get("alg") is killed by the
null test alone.
It is not dead, though, and I would rather be accurate than tidy about it. An edit that
reintroduces the "alg" in ref distinction does separate them, and one of those is a convention
rather than a contrivance:
alg = "sha256" if ("alg" in ref and ref["alg"] is None) else ref.get("alg")an explicit null read as "unset, take the default" rather than as a value. That is the one mutant
in my set killed by test_null_record_alg_is_refused, and it is the only test that kills it. So
the test earns its place. What it does not do is what its name suggests, which is cover a second
case alongside the one above it, and a line in it naming the implementation it excludes would save
the next reader the mutant run.
Tool-assisted: the matrix, the sweep 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. |
|
Addressed the released CI/type-check failure and @lywinged's review at
Fresh CI/CodeQL for the new head is currently back behind the repository's workflow-approval gate; I am not manually re-running it or claiming green until it executes. |
lywinged
left a comment
There was a problem hiding this comment.
Verified at 9511872. mypy is green, Success: no issues found in 10 source files, and the suite
is 1022 passed, 1 skipped with ruff and check_dashes clean.
The narrowing closes the older defect on the way: the four unhashable values now read
unsupported digest algorithm ... where they escaped as TypeError before. Your new test is
better than the one I sketched, because it sets hash malformed alongside alg, so it pins the
precedence and the refusal in one place; reverting the narrowing fails it four of four. The
comment on the null test records exactly which implementation it excludes. Approving.
Tool-assisted: the re-run and this note.
What
Closes #253.
content_marking.verify_assertion()no longer invents SHA-256 when the signed assertion omits requiredrecord.alg.The companion specification marks
record.algas required and permitssha256orsha384. The verifier previously used:so a peer-produced assertion with no algorithm member could still verify when its digest happened to be SHA-256. The producer already emits the field; the gap was consumer-side validation of externally supplied assertions.
The verifier now reads the actual member and establishes that it is a string before passing it to
_digest():This both clears the static type boundary and prevents unhashable list/dict values from escaping as Python
TypeErrorthrough_ALGSmembership.Regression coverage
Focused tests cover:
record.alg-> refusal rather than a SHA-256 default;record.alg: null-> refusal, including the plausible "null means unset/default" near-miss;ContentMarkingError, neverTypeError, even when the hash is malformed too;The existing unsupported-algorithm producer test remains unchanged.
Scope
No wire-format, schema, or normative text change. No algorithm-confusion or cryptographic vulnerability claim: this is required-field enforcement, primitive-type establishment, and signed-claim completeness in the reference consumer.
The released CI failure on the original head was the mypy
Any | Noneargument passed to_digest. The current head contains the reviewed type narrowing and focused mutation-oriented coverage; fresh repository CI is still awaiting workflow approval and is not claimed green until it executes.AI-assistance disclosure: ChatGPT assisted with source triage, adversarial-case design, implementation drafting, CI reconciliation, and diff review.
altrudevreviewed the bounded claim and remains responsible for the contribution.