Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"kid" field in JWT-formatted VP #478

Open
vdods opened this issue Oct 17, 2022 · 7 comments
Open

"kid" field in JWT-formatted VP #478

vdods opened this issue Oct 17, 2022 · 7 comments

Comments

@vdods
Copy link
Contributor

vdods commented Oct 17, 2022

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:

  • Retrieve the DID document corresponding to the "iss" field (which should be a [primary?] DID URL).
  • Try to verify the VP signature using each of the authenticationMethods in the DID document.
  • If one of the verifications succeeds, then the VP's signature is deemed valid. If all of them fail, the VP's signature is deemed invalid.

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
Copy link
Contributor Author

vdods commented Oct 17, 2022

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

@vdods
Copy link
Contributor Author

vdods commented Oct 17, 2022

I asked for clarification on this in the standards github: w3c/vc-data-model#951

@vdods
Copy link
Contributor Author

vdods commented Oct 17, 2022

Issue moved to w3c/vc-jose-cose#31

@vdods
Copy link
Contributor Author

vdods commented Oct 20, 2022

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
Copy link
Contributor Author

vdods commented Nov 15, 2022

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
Copy link
Contributor Author

vdods commented Nov 15, 2022

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
Copy link
Member

sbihel commented Nov 15, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants