feat(openid4vci): give key resolution the proof context, bind attested keys - #151
Merged
Merged
Conversation
…d keys Closes #150. ResolveProofKey saw only the kid, and that is blind exactly where the EU ecosystem lives. In the attested-key form a wallet sends {typ, alg, kid, key_attestation} and the key that signed the proof is INSIDE the header — one of the attestation's attested_keys. A callback holding only the kid cannot reach it, so the consumer base64url-decoded the proof header itself to find the attestation before calling in: openvc decoding a header once and the caller decoding it again, two implementations of "what is a header", one of them free to drift. Two spec facts decide the shape of the fix. App. D: "If used with the jwt proof type, the Credential Issuer MUST validate that the JWT used as a proof is signed by a key contained in the attestation in the JOSE Header." A MUST that applies to us and that we could not express. And the jwt proof type fixes NO rule for how a kid names a key inside attested_keys. The spec's own example uses "kid": "0" — an index; wallets also use each JWK's kid member, or an RFC 7638 thumbprint. So that mapping is the caller's: guessing between three conventions is the same "silently prefers one" defect the exactly-one-key- parameter rule exists to prevent, and a wrong guess picks the wrong key out of a list the attacker supplied. Hence: parse in, bind in, trust out. verify_credential_request_proofs(..., resolve_proof_key_in_context=) takes a frozen ProofKeyContext (kid, alg, a read-only header, the parsed key_attestation, credential_issuer, index), so the resolver is one line of the caller's own rule. Pass it or resolve_proof_key, never both — a precedence between two key resolvers is that same silent preference. ResolveProofKey is unchanged and NOT deprecated: existing resolvers keep working untouched, and a context object grows without ever breaking them again. Widening the alias to (kid, header) would have been a MAJOR, and openbadgeslib pins openvc-core>=1.21,<2, so 2.0.0 would lock the reporting consumer out of its own fix; inspect-based arity dispatch would silently downgrade a resolver wrapped in cache.cached_resolve, which returns a one-arg closure, losing the header with no error at all. Also public: peek_key_attestation -> UnverifiedKeyAttestation, and peek_proof_header, so no caller writes that second decoder. Structure is validated, trust is not: attested_keys must be a non-empty array of JWK objects and every App. D member has its type pinned, while typ, exp and the signature are left on header/claims because those are a verifier's decisions and that verifier needs a wallet-provider anchor openvc has no model for (ADR-0007 D9, amended in a 1.24.0 addendum). BEHAVIOUR CHANGE: a proof carrying key_attestation must now be signed by one of its attested_keys, compared by RFC 7638 thumbprint, on every key source; and a malformed attestation rejects the proof before any crypto, with the other structure rules, rather than being read at the end. Previously the header was captured verbatim and never looked at — the pinned test asserting that is rewritten. That check STOPS NO ATTACKER and is documented that way in the docstring, threat-model I19 and Security-Model, because selling it as a defence would make the security story worse: whoever forges a proof also chooses its key_attestation, whose signature nothing here verifies, so they simply attest their own key. What it catches is an honest wallet — or the caller's own resolver — handing over a key the wallet never claimed, which would otherwise mint a credential bound to the wrong key and verify cleanly. It can only reject, never accept, which is the only reason an unverified blob may drive it at all. Who breaks, all previously accepted: an attestation that is not a parseable JWS or whose attested_keys is missing/empty/not an array of objects, and x5c or kid deployments whose attestation does not list the key the chain or the registry produced. Including the sharp one: jwk_thumbprint digests the coordinate strings as given, so a JWK with non-fixed-width x/y — non-conformant per RFC 7518 6.2.1.2, ~1 in 256 per coordinate, rare enough to pass every test and surface in production — thumbprints differently from the same key encoded correctly. The rejection names that cause, and a test pins it. No key proof that used to be rejected is now accepted. Two error paths are typed rather than left to escape: InvalidKey is a key-backend error, not a ProofError, and it is reachable from both sides of the comparison — a hostile attested_keys entry, and the caller's own resolver returning something like {"kty":"EC","crv":"P-256","x":123}, which passes the alg binding because that reads only kty/crv. Both are wrapped into ClaimsInvalid. Vectors: App. D's key attestation and the proof that indexes it. Both spec examples are printed DECODED and the proof's attestation is a placeholder, so what is third-party here is the shape, the test re-encodes them, and the fixture's provenance says exactly that. The fixtures README records the gap that remains — no attestation from a real wallet provider exists here, and the binding is on by default, so the ecosystem form it governs is pinned only by material this repo shaped. examples/13_oid4vci_key_attestation.py runs the whole flow and is executed by CI. Gate: flake8 clean over src/, tests/ and examples/; mypy clean; 1626 tests passing with 26 skipped.
luisgf
added a commit
that referenced
this pull request
Jul 29, 2026
One additive feature on 1.23.1, and the behaviour tightening that ships with it (#150, PR #151). OpenID4VCI key resolution now sees the proof it is resolving for. ResolveProofKey saw only the kid, which is blind exactly where the EU ecosystem lives: in the attested-key form a wallet sends {typ, alg, kid, key_attestation} and the key that signed the proof is inside the header, one of the attestation's attested_keys. A consumer had to base64url-decode the proof header itself to find it, so openvc decoded a header once and the caller decoded it again — two implementations of what a header is, one free to drift. resolve_proof_key_in_context takes a frozen ProofKeyContext (kid, alg, a read-only header, the parsed key_attestation, credential_issuer, index), so the resolver is one line of the caller's own mapping rule. ResolveProofKey is unchanged and NOT deprecated: the new resolver is a separate keyword, existing callables keep working, and a context object grows without breaking them again. peek_key_attestation and peek_proof_header are public so no caller writes that second decoder. Which key in attested_keys a kid names stays the caller's: OpenID4VCI fixes no rule for it — its own example uses an index, wallets also use each JWK's kid member or an RFC 7638 thumbprint — and guessing between three conventions is the "silently prefers one" defect the exactly-one-key-parameter rule exists to prevent. Behaviour change, in Changed rather than Added: a proof carrying key_attestation must be signed by one of its attested_keys (App. D's MUST, RFC 7638 thumbprint, every key source), and a malformed attestation now rejects the proof before any crypto. That check stops no attacker — whoever forges a proof also attests their own key, since its signature is never verified here — and the docstring, threat-model I19 and Security-Model are worded to that limit. It catches an honest wallet, or the caller's own resolver, producing a key the wallet never claimed. Nothing previously rejected is now accepted. Gate: flake8 clean over src/, tests/ and examples/; mypy over 61 source files; 1626 tests passing with 26 skipped; build plus twine check on both artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #150.
ResolveProofKeysaw only thekid, and that is blind exactly where the EU ecosystemlives. In the attested-key form a wallet sends
{typ, alg, kid, key_attestation}and thekey that signed the proof is inside the header — one of the attestation's
attested_keys. A callback holding only thekidcannot reach it, so the consumerbase64url-decoded the proof header itself before calling in: openvc decoding a header
once and the caller decoding it again, two implementations of "what is a header", one of
them free to drift.
The two spec facts that decide the shape
jwtproof type, the Credential Issuer MUST validatethat the JWT used as a proof is signed by a key contained in the attestation in the
JOSE Header." A MUST that applies to us and that we could not express.
jwtproof type fixes no rule for how akidnames a key insideattested_keys. The spec's own example uses"kid": "0"— an index; wallets also useeach JWK's
kidmember, or an RFC 7638 thumbprint.So: parse in, bind in, trust out. The mapping stays the caller's — guessing between
three conventions is the same "silently prefers one" defect the exactly-one-key-parameter
rule exists to prevent, and a wrong guess picks the wrong key out of a list the attacker
supplied.
What changed
verify_credential_request_proofs(..., resolve_proof_key_in_context=)takes a frozenProofKeyContext(kid,alg, a read-onlyheader, the parsedkey_attestation,credential_issuer,index), so the resolver is one line of the caller's own rule:Pass it or
resolve_proof_key, never both.ResolveProofKeyis unchanged and notdeprecated — existing resolvers keep working untouched, and a context object grows
without ever breaking them again. Widening the alias to
(kid, header)would have been aMAJOR, and
openbadgeslibpinsopenvc-core>=1.21,<2, so 2.0.0 would lock the reportingconsumer out of its own fix;
inspect-based arity dispatch would silently downgrade aresolver wrapped in
cache.cached_resolve, which returns a one-arg closure, losing theheader with no error.
Also public:
peek_key_attestation→UnverifiedKeyAttestation, andpeek_proof_header,so no caller writes that second decoder. Structure is validated, trust is not —
typ,expand the signature stay onheader/claimsbecause they are a verifier's decisionsand that verifier needs a wallet-provider anchor openvc has no model for (ADR-0007 D9,
amended here by a 1.24.0 addendum).
Behaviour change
A proof carrying
key_attestationmust now be signed by one of itsattested_keys(RFC 7638 thumbprint, every key source), and a malformed attestation rejects the proof
before any crypto rather than being read at the end. Previously the header was captured
verbatim and never looked at; the pinned test asserting that is rewritten.
That check stops no attacker, and the docstring, threat-model I19 and Security-Model
say so — selling it as a defence would make the security story worse. Whoever forges a
proof also chooses its
key_attestation, whose signature nothing here verifies, so theyattest their own key. It catches an honest wallet — or the caller's own resolver —
handing over a key the wallet never claimed, which would otherwise mint a credential
bound to the wrong key and verify cleanly. It can only reject, never accept, which is the
only reason an unverified blob may drive it.
Who breaks, all previously accepted:
attested_keysismissing/empty/not an array of objects;
x5corkiddeployments whose attestation does not list the key the chain or theregistry produced — including the sharp one:
jwk_thumbprintdigests the coordinatestrings as given, so a JWK with non-fixed-width
x/y(non-conformant per RFC 7518§6.2.1.2, ~1 in 256 per coordinate — rare enough to pass every test and surface in
production) thumbprints differently from the same key encoded correctly. The rejection
names that cause and a test pins it.
No key proof that used to be rejected is now accepted.
Two error paths are typed rather than left to escape:
InvalidKeyis a key-backend error,not a
ProofError, and it is reachable from both sides of the comparison — a hostileattested_keysentry, and the caller's own resolver returning something like{"kty":"EC","crv":"P-256","x":123}, which passes the alg binding because that reads onlykty/crv. Both are wrapped intoClaimsInvalid.Vectors, and the gap that remains
spec/key-attestation-app-d.jsonholds App. D's attestation and the proof that indexesit. Both spec examples are printed decoded and the proof's attestation is a
placeholder, so what is third-party is the shape; the test re-encodes them and the
fixture's provenance says exactly that. The fixtures README records what is still
missing: no attestation from a real wallet provider exists here, and the binding is on
by default, so the ecosystem form it governs is pinned only by material this repo shaped.
The recorded EUDI metadata advertises
key_attestations_requiredon all 27configurations, which is how likely it is that a real one differs in some way that
matters.
examples/13_oid4vci_key_attestation.pyruns the whole flow and is executed by CI.Still open
require_key_attestation— a presence check, meaningful only now that the binding exists(alone it is satisfied by
"key_attestation": "x"). Left out: it is the issuer's ownmetadata policy, and
if proof.key_attestation is None: raiseafter the call expressesit. Say the word and it lands.
Gate
flake8clean oversrc/,tests/,examples/;mypyclean; 1626 tests passing,26 skipped.