Repository navigation
docs: acs-verdict-evidence-v1 predicate (draft) - #297
zhaoxinghua09-cell wants to merge 1 commit into
Conversation
|
@narko4u, thank you for catching both holes in #33: the reused peer statement and expiry_disposition outside the quarantine digest. Vectors commit 617cf55 now puts the peer's verdict inside its signed statement, and #298 refuses each case. @zhaoxinghua09-cell, two more things turned up in those tests. The kit at e2f85f6 writes no signature, so its samples cannot pass your verification steps yet. The predicateType URI names the main branch, while this PR targets integration, so it stays 404 after merge. |
|
Hey! Quick note on the PR format. You've got "discussed in #33" in your description, but CONTRIBUTING.md needs one of these formal keywords before the issue number:
Just swap "discussed in" for "Part of" and you're good. Everything else looks solid. Thanks for the work on the verdict evidence predicate! |
|
Hi Steven, OWASP requires a sign-off on every commit, and ef2745c on this PR doesn't have one. The Please run these commands: They add a |
|
Hi Steven, this comment replaces my earlier one. The Nothing is wrong with your work. The check looks for a sign-off, and OWASP requires one on every commit. A sign-off is a The one commit on this pull request, ef2745c, doesn't have the line. Please run these steps on your computer.
The last command prints your name, so you can check it.
If Git says your branch has no upstream branch, run These steps ran on a copy of your branch with no conflicts. If the rebase does stop on a conflict, open each file Git lists and fix those lines. Then run
The output should end with The full guide is here: How to sign your commits. Thanks! |
drk-ram
left a comment
There was a problem hiding this comment.
Reviewed at ef2745c288f64f96e727e67643687e204463ffcf. I merged origin/integration (bba1b846fbe7511b0b6f876c61b1fe91b27b07da) into a scratch worktree (clean merge, nothing committed) and ran the suite on Windows, Python 3.11.9, because the checks shown on this PR are from 18 October and predate the nav and link guards.
Blocking
1. The merged tree fails two tests that this file causes. python -m pytest tests gives 1182 passed, 13 failed, 20 errors. Eleven of the failures and all 20 errors are in areas this PR does not touch (symlink handling, patch-context re-derivation, subprocess environment, intake-workflow tests) and I did not chase them. Two are about this page:
tests/test_doc_links.py::test_no_doc_links_to_a_github_repository_outside_the_allowlistreportsdocs\predicates\acs-verdict-evidence-v1.md:7: links to probityai/agent-evidence-vocabulary, not on the allowlist.tests/test_site_nav.py::test_every_pane_docs_url_is_built_and_every_built_page_is_in_the_pane_or_declaredreports thatpredicates/acs-verdict-evidence-v1/index.htmlis built but not in the pane, and asks for a titlednaventry inmkdocs.ymlor anextra.acs_nav.out_of_nav_prefixesentry for the directory.
The fix for each is small (a nav entry or declared prefix; an allowlist entry or a link to a document in this repository), but the PR cannot merge without it.
2. Process items already raised in this thread are still open: the dco sign-off on ef2745c and the discussed in #33 wording (Part of #33). The branch has to be rebased onto integration for any of that to take effect.
3. The text does not yet contain the two fixes the #33 thread says it contains. In the thread @astrogilda credited @narko4u with two holes: a reusable peer statement, and expiry_disposition outside the quarantine digest. Reading the file at this SHA, both are still there:
- Quarantine digest. The digest covers
vector_id,status,reason,quarantine_ref,quarantine_gap,owner,review_by,excluded_from.expiry_dispositionis not in the list, so it can be edited without breakingquarantine_digest_mismatch, which is the property the sentence below the list claims.statusandreasonare listed but defined nowhere in the quarantine table. - Peer statement. The statement is described as naming the vector, the requirement and the evidence artifact. It does not carry the verdict being witnessed. A statement that a peer checked requirement R on vector V against deployment D can therefore be attached to any verdict record for V, including one whose
observedorverdictthe peer never saw. The thread says the vectors commit now puts the peer's verdict inside its signed statement. The predicate has to say so, and step 9 has to compare it.
Spec defects to fix in the same pass
witness_scopecontradicts itself. The field table says every field is required in the record andwitness_scopeis one of them. The "Derivingwitness_scope" section says it is "computed by the consumer, never asserted by the runner", and the Fields section says a record "must not carry any field by which the runner asserts its own scope". A runner-writtenwitness_scopeis exactly that field, even though step 9 later checks it. Either remove it from the record and make it a verifier output, or rename it to a claimed value that step 9 compares and say that plainly.- The derivation table is incomplete. Rows cover equal/null, equal/present (peer, external), unequal/null and any/invalid. There is no row for
hosting_operator_id != deployment_operator_idwith a verifyingpeer_artifact, and the unequal/null row says the record is "not a valid SELF record" without saying what it is. Both operator IDs are runner-written strings in the same signed payload, so "comparable identifier" needs a definition (issuer-qualified, from a pinned registry) or equality is self-asserted. The row also says "rejected at verification step 9", butrejectedis averdictvalue and step 9 emitscontradicted. That is an unfortunate collision. vector_idis never tied to the subject.vector_idis the first 16 hex characters (64 bits) of a SHA-256 of "the vector input's canonical form". Step 7 compares the subject digest to the canonical vector input, and no step checksvector_id == subject.digest.sha256[:16]. Add that check, or drop the truncated field and use the full digest.- "Canonical" is undefined.
payload_digest,vector_idandprovenance.peer_artifactall say "canonical bytes"; the one definition given is "keys sorted, no whitespace, UTF-8". That is not RFC 8785 and leaves number serialization, escape forms and key ordering (code point vs UTF-16 code units) open, so two conforming verifiers can disagree on a digest. Name RFC 8785 JCS.draft-durga-agenttrust-trace-verification-00specifies JCS (RFC 8785) canonicalization together with fail-closed verification, which is the combination this predicate needs. Separately,peer_artifactshould be the SHA-256 of the DSSE payload bytes exactly as signed (or the PAE), not a re-canonicalization of a signed statement. A signature covers specific bytes. - Step 3 mislabels an unpinned key.
untrusted_keyis mapped tocontradicted. An unknown signer isnot_established;contradictedshould mean the evidence disproves the claim. The intro's three-valued outcome only works if those are kept apart. anchorhas no version binding. "Repo-relative, resolvable anchor" does not say relative to which spec release or commit. A heading rename silently changes what a verdict was about. Carry the spec version (or tree digest) in the record and inpayload_digest_fields.
Smaller
- The predicate URI is
blob/main/.... @astrogilda already noted it is 404 after a merge tointegration; it also stays 404 until the next promotion reachesmain(#464 makes that explicit), and a mutable branch URL is a weak identifier for something signed. Use the published site URL or another persistent URI and keep v2 on a new path as the Versioning section already does. v1.0.1-draftis described as breaking relative tov1.0.0-draftand becomesv1.0.0on acceptance. A reader will take that as a lower version number than the draft whose breaking changes it contains. Pick one scheme.- The PR body says the sample digests reproduce with
run_demo.py --selftest; the thread says the kit ate2f85f6writes no signature, so its samples cannot yet pass the verification steps. That should be stated in the document's status line.
The design direction (scope derived by the verifier, never asserted; a rejection weighted like a pass; quarantine with an expiry disposition) is sound and worth landing. It needs a pass for the points above first.
Raghuvar Karthik Durga (@drk-ram)
What
Draft predicate
docs/predicates/acs-verdict-evidence-v1.md(v1.0.1-draft), the evidence format for the acs-conformance verdict record discussed in #33.This addresses the two points raised in #33 (comment 6243997020):
lift_state/counter_signature; scope is derived by the verifier from a peer-authored signed statement (provenance.peer_artifact). Absent that artifact the record readsSELF(co-signature is the default, non-repudiation only).integrity.payload_digestover its own declared fields and a definedexpiry_dispositionforreview_by.Sample digests are reproducible on a clean checkout via
python run_demo.py --selftest(kit: zhaoxinghua09-cell/agent-evidence-kit). The quarantine row points at the MODIFY-composition gap opened as #296.The
predicateTypeURI in the sample resolves on merge (doc is this PR's payload).