Repository navigation
fix: distinguish missing and malformed signatures - #390
Conversation
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
lywinged
left a comment
There was a problem hiding this comment.
CI is green on all eight check runs, which closes the "Fresh hosted CI is pending" line in your
update. The rest of your description holds where I can measure it: 2,188 passed and 6 skipped on
Linux, which is your 2,183 plus the five generator-fixture cases; tests/test_sign.py collects
175; 474 is test_sign.py, test_revocation_bundle.py, test_uri_formats_are_enforced.py and
test_schema_regex_semantics.py, at 175, 54, 207 and 38, the only subset of the six signing,
revocation, URI and regex files that sums to it. The baseline against c4fd779 gives 6 failed and
4 passed on the ten cases you add, with the fifth pass the control at tests/test_sign.py:376.
mypy clean on 13 files, ruff check src tests scripts as CI runs it, tools/check_dashes.py
and git diff --check clean. It merges clean into cfb0fc7 and into 450be9f.
The merge resolution is the part your description cannot check for itself, so I did. Replaying it
conflicts in tests/test_sign.py alone and sign.py auto-merges, which is what you say. Set
differencing the test names on 4e6cfd1 against both parents, 801 against 778 and 799, comes back
empty in both directions and the union of the parents is 801. Nothing was dropped.
Your changelog checklist is right and I checked it rather than agreeing: CONTRIBUTING.md asks
for an entry as step 5 of the steps for a normative contribution, so a non-normative fix owes none.
The guard is right. Approving.
Before you merge: #247 is listed to close
The API reports #247's closedByPullRequestsReferences containing this pull request, so merging
closes it. You wrote a sentence saying it does not, and the parser does not read the negation:
scanning your body for a keyword and a number finds exactly one pair, resolve #247, inside "This
does not resolve #247's separate number-spelling or canonical-encoding questions". Your branch
commits carry no #N at all, so the body is the whole of it. You know the mechanism, which is why
I read this as the parser rather than the author: on #388 on 09-21 you wrote Closes #379. and it
did exactly that. The field is not being read as an oracle either way, since #226 reports no such
reference and is still open although (#226) is in #389's title and in its merged subject line.
The one I would not want closed silently is the precedence section, which you ruled on 09-11 that
you would write and which is not in the repository yet. Also on that issue and unresolved: item 5
of your restatement on that issue, reaffirmed later and never given an issue, and both of
@chernistry's specification questions, which you ruled on 09-21 to proposals nobody has opened yet.
Unlinking in the Development panel should remove the link whatever made it, and a reword stops it
coming back. For completeness: gate is not what is holding this, since it skips a maintainer
author and concluded success. What is left is the code-owner rule: CODEOWNERS gives
* @agentrust-io/maintainers @lywinged, I am the only requested reviewer, and this approval is
what clears it. I cannot read the branch protection, so that is who rather than proof of
enforcement.
#388 split the model from the schema, and the parity test cannot see it
validate.py:27 adapts each shipped pattern at run time, $ to \Z and the dot to a class
excluding the line terminators. models.py:26 holds the identical string and hands it to pydantic
unadapted. So the two surfaces now carry the same text and behave differently.
Measured on a signed record at 4e6cfd1, model_validate against validate_json:
subject ends in |
models.py |
validate.py |
|---|---|---|
| nothing, the control | accepts | accepts |
| U+000A | refuses | refuses |
| U+000D | accepts | refuses |
| U+2028 | accepts | refuses |
tests/test_the_schema_and_the_models_agree.py is green, 19 passed. It cannot see this: the first
of the three outcomes it accepts per constraint is that the two artifacts hold the same pattern
string, and they do. The divergence is in the adaptation, which exists on one side only, so the
test is comparing the thing that still matches.
That makes it an instance of item 5 of your own restatement on #247, a parity test reporting
agreement between two surfaces that disagree, and an instance of #247's headline class, which is
the issue this pull request is listed to close. Neither is 390's doing and neither blocks it.
Two smaller things from the same sweep. Four more compiled patterns outside validate.py carry the
unadapted $ and are used with .match(): content_marking.py:40 and :41,
adapters/sandbox.py:128 and :130. The content_marking pair is live in the same way, a
subject ending in U+000A, U+000D or U+2028 binds through build_assertion and verify_assertion
while sign.verify_record refuses the same record. And provenance.py:59 and :60 are only half
the answer if they are read as one: \Z alone closes U+000A, and U+000D and U+2028 need the dot
class as well, which is why your #388 changed both.
The truthiness class, and the call you have already made twice
I enumerated src/agentrust_trace/ for guards whose refusal within four lines says missing,
absent, has no, carries no or declares no, then ran the candidates over eleven values: the nine you
parametrise, your empty-string case, and an absent member. Six read a member off a record or a JWK,
and four of the six report a present value as absent.
| guard | member | says it is absent for | otherwise |
|---|---|---|---|
sign.py:677, yours |
signature |
absent alone | nine malformed, "" schema-invalid |
provenance.py:450 |
signature |
absent None 0 False [] {} "" |
four malformed |
provenance.py:455 |
cnf.jwk |
absent None 0 False [] {} "" |
four unusable-key |
sign.py:436 |
trusted JWK x |
absent None 0 False [] {} "" |
four malformed |
content_marking.py:161 |
eat_profile |
absent None 0 False [] {} "" |
four accepted |
content_marking.py:214 |
data.subject |
absent alone | ten name the mismatch |
The first and last rows are in the table because they are the correct pattern, and the last one is
yours: content_marking.py:214 and :264 came in with 2a82bef, your #335. I ran :264 too, an
absent subject gives "has no subject" at 265 and all ten present values give the mismatch at 271
naming the value; it raises RecordMismatch where :214 raises ContentMarkingError, which your
commit message explains. So the call has been made twice here and both times by you, and the four
middle rows are the sites that predate both.
The two provenance.py rows are the split you ruled out, decided by truthiness, five lines apart,
in the other function of that name. sign.py:436 is in the file you are editing.
The content_marking.py:161 row I will file separately rather than argue here. It is @altrudev's
adjacent observation on #326, which he left to maintainers to consolidate and nothing now carries.
The part nobody measured is downstream of that guard: build_assertion at :161 copies a truthy
non-string profile into the assertion, and verify_assertion then accepts it and returns it to the
caller with its type intact.
Eight more guards came out of the sweep and are not in the table: sign.py:767 and :780 hedge
with "valid", sign.py:655 and :204 report all eleven values as absent so they carry no split,
content_marking.py:159, :199 and :222 are shape or regex checks, and sign.py:707 is
unreachable behind schema validation. Not a closed enumeration of the package.
Outside src/, the five literal messages in the table appear at exactly three more sites:
tests/test_sign.py:379, which pins a substring, and
examples/runtime-evidence/generate.py at 88 and 91, the second of those under the same
truthiness test.
Two values, 48 cases, and the second implementation
@chernistry's reproducer on #247 covers eight values, 0, false, [], {}, 1, true, [0]
and {"a": 1}. Your description adds two more, null and the empty string, and those two are
exactly where the reference and the TypeScript in #376 will disagree after this merges: 24 cases
each, one per verifier configuration. On null the pair is signature_malformed against
signature_missing, the reference following your ruling and verify.ts:249 not. On the empty
string it is schema_invalid against signature_missing.
I corrected that one line two ways and re-scored. Within each column the corpus and the TypeScript
are fixed and only the Python side moves; restoring verify.ts and rebuilding reproduces the
shipped verdicts byte for byte, so the corrected columns measure the patch and not the rebuild.
| Python side | TypeScript as shipped | corrected, "" left to the schema |
corrected, "" also malformed |
|---|---|---|---|
cfb0fc7 |
0 | 48 | 48 |
390 at 4e6cfd1 |
48 | 0 | 24 |
record.get("signature") is None |
24 | 24 | 48 |
Two columns because your ruling fixes that "missing" names an absent member, which already
excludes "", and does not fix whether an empty string is malformed or schema-invalid. That one is
yours to pick. Under either reading 390 is better than cfb0fc7 and better than the cheaper guard.
The middle column reaching zero is not independent evidence, since it is defined as the TypeScript
doing what 390 does; the right-hand column is the one that carries information.
The cheaper guard is the row worth a look, because it is the fix a reviewer would propose.
record.get("signature") is None closes the same four your reference got wrong and fails exactly
one of your 2,188 tests, the None case in your own parametrised set. Scored against the
TypeScript as it stands it looks ahead of you, 24 against 48. It is not: that verifier calls
null missing and so does the cheaper guard, so its lead is agreement with the thing you ruled
against. Correct the TypeScript and the ordering inverts.
Two limits on the table. Both verifiers still call a present empty eat_profile missing, at
sign.py:655 and verify.ts:234, which is the reading you just ruled against on signature. And
the 0 in the first column is a control for harness noise, not a claim that cfb0fc7 is correct.
The #376 ledger is already invalid, and the re-cut is not 390's doing
compare.py exits non-zero when a declared divergence goes unreached or its count no longer holds.
Run at #376's head, each tree scored against the same corpus:
| Python side | identical | known | unexpected | ledger | exit |
|---|---|---|---|---|---|
ccbd5b9, #376's own base |
1268 | 364 | 0 | exact | 0 |
3ddcbbf, after 385 |
1268 | 364 | 0 | exact | 0 |
74eafa0, after 386 |
1278 | 354 | 0 | 8 broken | 1 |
4891bb7, after 387 |
1302 | 330 | 0 | same 8 | 1 |
412f02c, after 388 |
1398 | 234 | 0 | same 8 | 1 |
c4fd779, after 383 |
1400 | 232 | 0 | 9 broken | 1 |
cfb0fc7, after 389 |
1400 | 232 | 0 | 9 broken | 1 |
4e6cfd1, this branch |
1448 | 136 | 48 | 10 broken | 1 |
The first two rows are the control. 386 broke eight entries on its own, 383 added the ninth, and
387, 388 and 389 added none, so the re-cut is owed already and 390 is not what owes it. What 390
does own is the unexpected column: every other tree in the series scores 0 and this branch scores
48, the two values above, and that is the only change in the series that needs a decision rather
than an arithmetic update. Entry 6 leaving the ledger is the automatic half, and at 96 cases it is
the largest entry in it. Nothing here blocks you: #376 is unmerged, so its job does not run on
main.
What I could not check
- The 12 published conformance vectors. Every figure above is over the 1632-case corpus
generated without--external, not the 1644 CI scores, so--expect-external 12never ran and
those 12 are outside all of it. - Windows, and the published
agentrust-tracewheel. Everything above is repository trees. - The branch protection, as above.
What this changes
A present signature such as
0,false,[],{}ornullcurrently reports that the signature field is missing. Check whether the key exists, then let the existing decoder report a wrong type. An empty string reaches schema validation and is rejected there. Absent signatures keep the existing missing-field error.Implements the decision on #247. All these invalid records still reject. The separate number-spelling and canonical-encoding questions tracked in issue #247 remain open.
Type of change
Python verifier error-reporting fix. No schema or normative change.
Validation
git diff --checkpassed.Nine wrong-type cases cover both empty and nonempty values, plus an empty-string case. The environment uses pinned development requirements plus
colorama==0.4.6on Windows.Checklist
Update after main merged
Updated to
4e6cfd1after #383 and #388 merged. The only manual conflict was intests/test_sign.py; both the signature and nonce regressions are retained. All 474 signing, revocation, URI and regex tests pass together. Ruff, mypy and style checks pass. Full Windows suite: 2,183 passed, 6 skipped and the same five previously confirmed generator-fixture failures. Hosted CI now passes. lywinged independently reviewed and approved commit 4e6cfd1; the broader follow-ups in issue #247 remain open.