spruceid / ssi

Core library for decentralized identity.
https://spruceid.dev
Apache License 2.0
193 stars 61 forks source link

"kid" field in JWT-formatted VP #478

Open vdods opened 2 years ago

vdods commented 2 years ago

The spec https://www.w3.org/TR/vc-data-model/#jwt-encoding describes the "kid" field of a JWT-formatted VP as optional:

kid MAY be used if there are multiple keys associated with the issuer of the JWT. The key discovery is out of the scope of this specification. For example, the kid can refer to a key in a DID document, or can be the identifier of a key inside a JWKS.

In 0.4 and in the current main branch, Presentation::decode_verify_jwt requires the "kid" field in order to determine the signing key. This makes a lot of sense to me personally, as the signer uses a specific key to sign the VP, and there's really no reason not to use that key's DID fragment URL as the "kid" field so that verification is straightforward.

The VC standard making it optional makes less sense, because it introduces unnecessary complexity to the key resolution phase, which I believe is the following:

While I agree with ssi's current implementation with respect to the "kid" field, its inability to resolve the signing key when the "kid" field isn't present means that the current implementation isn't compliant with the standard. Can anyone comment on this? Was it an intentional choice, a temporary step, etc?

vdods commented 2 years ago

This holds for VCs as well -- the "kid" field is required by Credential::decode_verify_jwt.

vdods commented 2 years ago

I asked for clarification on this in the standards github: https://github.com/w3c/vc-data-model/issues/951

vdods commented 2 years ago

Issue moved to https://github.com/w3c/vc-jwt/issues/31

vdods commented 1 year ago

I have an initial pass of this change working for VPs, and will do the same for VCs. I'm not 100% sure if my changes respect the semantics intended by the decode_verify_jwt methods of Credential and Presentation though. Partly because having to iterate over multiple keys makes it harder to return specific errors (compared to knowing the specific key to use for verification, as is the case in the existing impl which requires the "kid" field to specify the signing key).

Anyway, I'd still like some comment on the choice about requiring "kid", but I'd also like to know if you're generally interested in accepting a change so that the "kid" field is optional, as the standard calls for.

vdods commented 1 year ago

After more digging, I've found that the verifier does the expected proof checks based on the "iss" field. But it also requires that the "kid" field is set in the JWT, and errors out if not. Because the "kid" field is optional, this check should only happen if the "kid" field is present. Does this square with your understanding? This issue occurs for JWT-formatted VCs and VPs alike.

vdods commented 1 year ago

Nevermind -- this only applies when there are embedded LD-proofs. So the original question stands -- if the "kid" field is not present, then all the verification methods for the VP issuer ("iss" field in JWT) should be checked against the JWS. Thoughts on this?

sbihel commented 1 year ago

Yes it makes sense to me to only use kid when present. Until they make it always required