Repository navigation
fix(provenance): validate attestation by shape, not truthiness - #339
Conversation
lywinged
left a comment
There was a problem hiding this comment.
The change is right and I found no defect in it. Three things stand between it and a merge, and none of them is a bug.
-
An attestation with an extra member verifies today and will not after this, and the body should say so.
_check_structure is shared with verify_record, so the new strictness reaches records that already exist. RuntimeInfo carries extra="forbid", so an attestation carrying any member outside the five it allows, platform, measurement, rim_uri, nonce and firmware_version, is refused. Measured: a tee-attested record built on main at 372b9af with attestation naming platform, measurement and a vendor_note signs and verifies there, and the same signed record is refused here. Your reading is the right one, and I checked it rather than assuming: server-provenance-v1.md section 3 points at the TRACE v0.2 section 3.1 runtime shape, and schema/trace-claim.json gives that object additionalProperties: false over exactly those five, so such a record was never conformant and the non-breaking tick is defensible. The effect is still that a verifier which accepted a record yesterday refuses it after this lands, which is what a report of it will look like from outside. One sentence in the body naming that, and naming the schema line it rests on, costs nothing and answers the report before it arrives. -
It does not merge. provenance.py conflicts with main, because #325 landed on the same function. On the rebase, the isinstance(attestation, dict) guard #325 added becomes redundant: RuntimeInfo.model_validate refuses a non-object anyway. Keeping it is harmless and gives a shorter message for the commonest case, dropping it is also fine, and it is worth deciding rather than letting whichever side the merge favours decide it.
-
The examples the description leads with are already closed. attestation: "not-actually-attestation", the list case, and [] on a non-TEE record are all refused on main today by that same #325 guard. The description was written on 12 September, before it landed. Three holes are still open on main and this closes them: {} on a non-TEE record, which is falsy and so passes the old check as if it were null; a tee-attested attestation naming a platform that does not exist; and one naming platform: "software-only", which is shape-valid and is still the claim without the thing that backs it. Those are the ones worth leading with.
What I checked and found right. The four checks ci.yml runs pass on 1c5b83c. Reverting to the two truthiness tests fails 21 of the 165 tests in tests/test_provenance.py, so the fix is load-bearing rather than decorative. The platform rule is the part with no cheaper substitute: keeping the RuntimeInfo validation and dropping only the software-only rejection leaves 163 of those 165 passing, and the two that fail are test_tee_attested_rejects_software_only_as_the_named_platform and test_the_verifier_rejects_a_shape_valid_software_only_tee_attested_record. Those two are why that rule is not deletable, and they are the strongest part of this branch.
Bring main in, put a sentence in the body for the first point, and I will approve whatever head that produces. The gate reads the latest review per maintainer that is not a plain comment, against the current head, so this one stops counting the moment that approval lands.
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
1c5b83c to
e726cf9
Compare
lywinged
left a comment
There was a problem hiding this comment.
Approved on e726cf9.
The three things I asked for are here. The branch sits on 3aaf0dd, so provenance.py no longer conflicts, and the isinstance(dict) guard #325 added is gone with its reason in a comment, which is a decision rather than whichever side a merge would have favoured. The description now leads with the holes still open rather than the ones #325 had already closed. The note about already-signed records is there. It does not name the schema line, which I asked for and am not holding you to, so for the record that line is schema/trace-claim.json 143, additionalProperties: false over the five members runtime declares.
What I measured on this head, in a fresh worktree, on 3.11 and on 3.12: the steps ci.yml runs are green, 1639 passed, 1 skipped, and tests/test_provenance.py 170 passed. That is my own run rather than the repository's, which is green on its own account. Restoring provenance.py to 3aaf0dd and keeping your tests fails 15 of those 170, across 7 functions. Two narrower mutants show where the width carries weight. elif attestation:, which fixes the tee-attested side and leaves the other on truthiness, fails 14 across the suite: 10 here, and 4 in tests/test_public_functions_raise_what_they_document.py, which already held the False, 0 and empty-string half before this branch existed. Keeping the model validation and dropping only the software-only rejection fails 2, the same two as before.
Two rules this adds have nothing holding them, and one is what your note is about. Loosening RuntimeInfo's own extra="forbid" to extra="ignore" leaves the suite green at 1639, and typing rim_uri, nonce and firmware_version as Any leaves it green as well. Both are changes to what an already-signed record does, measured end to end: an attestation carrying vendor_note, and one carrying nonce: 5 with rim_uri: {"a": 1}, each build and verify on 3aaf0dd and are refused here by name. Your note covers the first and not the second. Two cases close both.
The CHANGELOG.md entry is the part I would fix before this merges, and it is the same thing I asked you to fix in the description a day ago. Three of its sentences describe c04d938 rather than the base you rebased onto. Measured on 3aaf0dd: of {}, [], 0 and the empty string on a non-TEE record, only {} still builds, and the other three are refused by #325's guard; attestation: "hello" and attestation: ["anything"] on a tee-attested record are refused there too; and {"kind": "tee-attested", "attestation": "not-actually-attestation"}, the example the paragraph closes on, is refused on 3aaf0dd as well. _check_structure()'s rules there are three, not two. And models.RuntimeInfo._origin_cannot_claim_hardware does not exist: the validator is models.TrustRecord._origin_cannot_claim_hardware at models.py 393, and it requires software-only where your rule forbids it, so it is the converse rather than the same rule. The entry says 33 regression tests; collected, the branch adds 38 cases in 13 functions, and 33 is the number at 2929cac, before e726cf9 derived _HARDWARE_PLATFORMS and added the guard. It was already 38 at 1c5b83c, which I reviewed without checking it, so that one is my miss as much as your entry's.
One claim in the new test I checked by running it. The docstring on test_hardware_platform_list_is_actually_populated says an empty parametrize list "silently collects zero test cases and the run still shows green". On the pinned pytest 9.1.1 it collects one case, [NOTSET], and prints SKIPPED ... got empty parameter set, which addopts = "-v" puts in every log. The arithmetic beside it holds: RuntimeInfo accepts ten platforms, nine of them hardware.
Three I would not hold the merge for. The comment at provenance.py 198 is indented eight spaces where 199 to 207 are at four; it was at four on 1c5b83c, so this push introduced it. The added blank line is 23, and pydantic sits after rfc8785; ruff check --select I reports 17 against main's 16, which is why I001 is in the ignore list. And provenance.py 222 interpolates the whole pydantic str(exc) where sign.py 608 uses exc.message with the failing path; on a function verify_record reaches with caller-supplied input, an attestation with fifty extra members produces a ProvenanceError of 10,945 characters over 151 lines, echoing its own input and a pydantic URL.
This approval clears the gate, so the entry can merge as written, and I would rather it did not. The gate pins an approval to the head commit, so push the entry's corrections and I will approve that commit.
…lease notes Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
|
@lywinged corrected the changelog and test descriptions in a65fc64. Added builder and verifier coverage for extra attestation members and each optional field’s type, with valid controls. The provenance suite passes all 178 tests. Relaxing the extra-member rule fails two new cases; relaxing optional-field types fails six. Lint, style, and mypy pass. The full Windows suite has the same five newline/path failures as the original head; temporary normalization clears the affected modules. CI and CodeQL are now running on the new head. Could you re-review by September 17? |
lywinged
left a comment
There was a problem hiding this comment.
Approved on a65fc64.
Both rules are held now. Loosening RuntimeInfo's extra="forbid" to extra="ignore" fails 2 where it failed nothing before, and typing rim_uri, nonce and firmware_version as Any fails 6 where it failed nothing. test_attestation_shape_constraints is wider than what I asked for: each case runs through the builder and through a directly signed record, and the valid control through the same entry point is what makes each one mean something.
The entry reads true against 3aaf0dd now, and its new sentence about the schema holds: schema/trace-claim.json declares "type": "string" for rim_uri, nonce and firmware_version, and additionalProperties: false on runtime. Nine hardware platforms is the number I measured last time. The docstring on the derivation guard is right as well.
The four CI steps with the hash-pinned install, on 3.11 and on 3.12: green, 1647 passed, 1 skipped, tests/test_provenance.py 178. The branch does not carry main's ruff bump from #350, and 0.16.7 passes on this head anyway, so it needs nothing from you. main has moved since you pushed and GitHub still reports this mergeable.
provenance.py 218 still interpolates the whole pydantic str(exc), and I am still not holding the merge for it. It is cheaper than it looks, though. Wrapping the first error's location and message the way sign.py 608 does takes the fifty-extra-member case from 10,945 characters to 105, and your eight match=field cases still pass, because the field name is in the location. All 178 stay green.
What this changes
provenance._check_structure() was checking attestation by truthiness instead of type and shape. Spec requires it to be null unless kind == "tee-attested", and then match the TRACE v0.2 §3.1 runtime shape.
Three holes remain open on main:
Because the same check runs in verify_record(), a correctly-signed forged record with any of these also passed.
Note: This is stricter for already-signed records. An attestation with any extra member (beyond the five fields allowed by RuntimeInfo / schema) that used to verify will now be refused.
Type of change
Spec section
Not a change to
spec/trace-v0.2.mditself this implements the existingserver-provenance-v1.md§3attestationcontract and TRACE v0.2 §3.1runtimeshape more strictly in code. No wire format or normative text changes.Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text