fix(schema): hold an RSA confirmation key to carrying its key material - #311
fix(schema): hold an RSA confirmation key to carrying its key material#311lywinged wants to merge 2 commits into
Conversation
`schema/trace-claim.json` says of the confirmation key that "Keys must carry
actual key material", and enforced it for `OKP` and `EC` and for nothing else. A
`cnf.jwk` of `{"kty": "RSA"}` with no `n` and no `e` validated, and the record
then failed inside the verifier, where `jwk_thumbprint` reports a missing
required thumbprint member. Nothing was accepted that should have been refused,
since every path downstream fails closed. What was wrong is which instrument
spoke: the schema is the artifact an implementation in any language validates
against, and it was not the thing that told the producer the key was unusable.
What it tracks, since a schema change has to track normative text rather than a
description: section 3.2.2 requires the record to be bound by a signature made by
the key in `cnf`, and section 3.3 rule 1 has a verifier check that binding before
any other field is trusted. A confirmation key carrying no key material cannot
satisfy either, so refusing it enforces what those sections already require.
`sign.jwk_thumbprint` already knows `OKP`, `EC` and `RSA`, so naming the RSA
members adds no key type the package does not handle.
The reasoning for both of those sits in the `$comment` beside the constraint
rather than in `description`, which is the convention the private-member fix set
in the same block, and the `n` and `e` declarations are load-bearing rather than
decorative: without them a non-string modulus reaches `additionalProperties`,
which admits any canonicalizable value. A test covers that.
A `kty` enum would be a different act and is deliberately not done here. Section
3.2.1 states signing algorithms per envelope context and fixes no set for the
embedded-signature form of section 3.2.2, so narrowing `kty` in the schema would
add a constraint the specification does not make. That is a normative question,
and RFC 9964's registration of `AKP` for ML-DSA is the reason to answer it
deliberately rather than by hard-coding a list now.
Both copies of the schema move together, the published one and the packaged one,
and the new test asserts they are the same bytes, as `test_provenance_cnf_boundary`
already does for its own reason. The packaged v0.1 schema carries the same two
branches and is left alone, on the grounds agentrust-io#296 left it alone: nothing loads it, and
`test_the_superseded_schema_is_still_loaded_by_nothing` fails if anything starts to.
Signed-off-by: Louie Lu <48041247+lywinged@users.noreply.github.com>
|
❔ Contributor Check: UNKNOWN
Automated check by AgenTrust Contributor Check. |
github-actions
Bot
added
the
needs-review:UNKNOWN
|
This PR corrects the JSON schema, now |
Review on agentrust-io#311 by @rajnisht7: the schema was corrected and `models.JWK` was not, so `JWK(kty="RSA")` still constructed, and the new tests measured only the schema. Both points hold. `models.JWK` carried `required_by_kty = {"OKP": (...), "EC": (...)}`, the same two-entry table the schema's `allOf` carried, and `.get(kty, ())` gave `RSA` no requirement at all. That half-made state is the one `test_all_three_layers_draw_the_line_in_the_same_place` was written for on `iat`: "while it was half made, the model and the schema disagreed". The schema is what an implementation in another language validates against; `models.JWK` is exported and is what a Python caller reaches. A producer meets them in an order nobody controls. `RSA` is added to the model's table, and `n` and `e` are declared members rather than left to `extra="allow"`, for two reasons a control demonstrates: without the declarations the validator raises `AttributeError` instead of refusing the key, and a non-string modulus is stored as an untyped extra rather than refused. The same two declarations are load-bearing in the schema for the same second reason. Three tests in `test_models.py` mirror the `OKP` and `EC` pairs already there. The instrument that would have caught this is the parametrized case table in `test_cnf_jwk_carries_key_material.py`: eleven confirmation keys checked against the schema and the model together, asserting the two agree, including on the `kty` neither artifact names, where they agree to accept. Declaring two optional members on a model whose output is signed is the risk this carries, and it is already instrumented: `_omit_absent_optionals` keeps them out of a dump, `test_the_round_trip_returns_the_same_record` compares the whole record including `cnf`, and disabling that serializer makes both new members appear as `null` and fails it. A record round-trips to identical canonical bytes. Signed-off-by: Louie Lu <48041247+lywinged@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGTKY8Fut5trnDKm8TReTa
|
@rajnisht7 Both points are right, and the second one is the sharper of the two. Fixed in 33dbe14.
required_by_kty = {"OKP": ("crv", "x"), "EC": ("crv", "x", "y")}
required = required_by_kty.get(self.kty, ())which is the same two-entry table the schema's The repository already names that state. What changed:
On your second point, the answer I think is the real one is not more cases but a different instrument. Where I would push back slightly is on "the complete producer path", not because the concern is wrong but because this package cannot host that test honestly. One risk this carries that is worth stating: declaring two optional members on a model whose output gets signed. That is already instrumented rather than argued. 1342 passed and 1 skipped on 3.11 and on 3.12, up 14 from the previous commit. |
3 participants
What this changes
The confirmation key says of itself that "Keys must carry actual key material: OKP keys require crv and x; EC keys require crv, x, and y", and the
allOfbehind that sentence carried branches forOKPandECand for nothing else. So this validated:and the record then failed inside the verifier, where
sign.jwk_thumbprintreportsjwk with kty='RSA' is missing required thumbprint member 'e', and in the conformance suite, where TR-SIG acceptsOKPandEC.Nothing was accepted that should have been refused: every path downstream fails closed. What was wrong is which instrument spoke. The schema is the artifact an implementation in any language validates against, and it was not the thing that told the producer the key was unusable. A producer working in another language saw a valid record and learned otherwise from a Python traceback, from a conformance run, or not at all.
RSAnow requiresnande, declared as strings. Those declarations are load-bearing rather than decorative: without them a non-string modulus reachesadditionalProperties, which admits any canonicalizable value, and a test covers that. The reasoning for the constraint sits in the$commentbeside it rather than indescription, which is the convention the private-member fix set in the same block. Both copies of the schema move together, the publishedschema/trace-claim.jsonand the packagedsrc/agentrust_trace/schema/trace-v0.2.json, and a test asserts they are the same bytes, astest_provenance_cnf_boundaryalready does for its own reason. The v0.1 schema in the package carries the same two branches and is left alone, on the grounds #296 left it alone: nothing loads it, andtest_the_superseded_schema_is_still_loaded_by_nothingfails if anything starts to.What it tracks
A schema change has to track normative text rather than a description, so: section 3.2.2 requires the record to be bound by a signature made by the key in
cnf, and section 3.3 rule 1 has a verifier check that binding before any other field is trusted. A confirmation key carrying no key material cannot satisfy either.sign.jwk_thumbprintalready knowsOKP,ECandRSA, so naming the RSA members adds no key type the package does not handle, and nothing that verifies today is refused.What this deliberately does not do
A
ktyenum would also close the unrecognised-key-type case, and it is not proposed here. Section 3.2.1 states signing algorithms per envelope context,ES256,ES384orEdDSAfor JWT andCOSE_Sign1orCOSE_Signfor CBOR-COSE, and the embedded-signature form in section 3.2.2 names no algorithm set at all. A schema enum would add a constraint the specification does not make, which is a normative question and belongs in a spec change proposal rather than in a schema fix.It is worth answering deliberately rather than by hard-coding a list now: RFC 9964 registered
AKPas a JWK key type for ML-DSA in JOSE and COSE, so the set of key types someone might reasonably put here is not the set it was a year ago.Found while answering #307, which is unrelated in substance: the reporter there is asking about a different member of
cnf.Checked on the branch:
pytest1328 passed and 1 skipped on 3.11 and on 3.12, withruff check src tests scripts,python tools/check_dashes.pyandmypy src/agentrust_traceclean, on a fresh clone with onlypip install -e ".[dev]". Every committed example record still validates, and the private-member refusal from #296 still holds. Removing the RSA branch fails three of the new tests, and removing thenandedeclarations fails a fourth.Type of change
Spec section
None changed. The schema is brought into line with sections 3.2.2 and 3.3, both already merged.
Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change): entry added under Unreleased, Fixed<!-- CHANGED: #NNN: description -->in spec text: not a breaking change