search

decentralized-identity / did-jwt

Create and verify DID verifiable JWT's in Javascript
Apache License 2.0
331 stars 70 forks source link

`verifyJWT` behavior when `didDocument.verificationMethod` property has multiple public key sets #267

Closed siacomuzzi closed 1 year ago

siacomuzzi commented 1 year ago

Please correct me if I'm wrong but when verificationMethod property in the DID Document contains multiple public key sets, all of them are tested during signature verification until find the correct one. Some examples:

ES256

https://github.com/decentralized-identity/did-jwt/blob/4efd9a755738a8a6347611cbded042a133d0b91a/src/VerifierAlgorithm.ts#L79-L86

ES256K

https://github.com/decentralized-identity/did-jwt/blob/4efd9a755738a8a6347611cbded042a133d0b91a/src/VerifierAlgorithm.ts#L106-L113

Ed25519

https://github.com/decentralized-identity/did-jwt/blob/4efd9a755738a8a6347611cbded042a133d0b91a/src/VerifierAlgorithm.ts#L176-L178

Could you please explain why library is doing this instead of using, for example, JWT's header.kid to take just one public key from the authenticators array?

Thank you!

mirceanis commented 1 year ago

I think it was mostly for convenience, in case there wasn't a kid specified in the header, but then the actual check for a kid was never added. Thanks for raising this issue!

If a kid is mentioned in the header, it should be used, indeed.

Would you like to contribute a PR with a fix?

bumblefudge commented 1 year ago

Also note, there have been some discussions of kid security in the VC-JWT repos lately, with implementer feedback coming from the OIDF community that relative kid references (i.e. #key1) leak less information that absolute ones (in flows where the JWK/token arrives separately): https://github.com/w3c/vc-jwt/issues/31#issuecomment-1384394003

I would tend to think that, as Kristina points out, both absolute and relative kids will probably persist in different architectures and veramo should prolly support both some day. but a PR implementing either is still infinitely more useful and welcome than no PR haha

mirceanis commented 1 year ago

I would imagine that it wouldn't be that hard to support both absolute and relative kid for this. There are 3 situations:

I don't have enough of a grasp of how things should work for non DID issuers. I wouldn't be opposed to adding functionality for that situation too.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.