add did:jwk method support #1128

tadejpodrekar · 2023-02-20T09:12:23Z

What is being changed

Added an implementation of 'AbstractIdentifierProvider' for the did:jwk method according to the specification.
The provider currently supports Secp256k1, Secp256r1, Ed25519, X25519 key types.

Add create did:jwk identifier tests
Passing
Add resolve did:jwk tests
Passing
Add tests creating a valid VC using the did:jwk method
Passing

Quality

Check all that apply:

I want these changes to be integrated
I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
I allow my PR to be updated by the reviewers (to speed up the review process).
I added unit tests.
I added integration tests.

mirceanis

An excellent PR, thank you for contributing this!

I added some notes about error messages and a question about the spec.
Please take a look.

mirceanis · 2023-02-21T15:02:54Z

packages/did-provider-jwk/src/jwkDidUtils.ts

+  }
+  if(keyType === 'Secp256r1') {
+    const EC = new elliptic.ec('p256')
+    // add '03' prefix to public key


Where is this prefix coming from?
Can you please share a link to some reference?

We do it in our implementation as well (compressed format):
https://github.com/Sphereon-Opensource/ssi-sdk/blob/develop/packages/jwk-did-provider/src/functions.ts#L113

It is added to indicate a compressed public key, otherwise we have an invalid hex string. The prefix (02 if the y coordinate of the public key point is even, 03 if the y is odd) is removed in the Veramo kms: https://github.com/uport-project/veramo/blob/abfba31ff8497f00cddec5ca7a561fe32b1ab0bb/packages/kms-local/src/key-management-system.ts#L333

hmm.. this seems odd (no pun intended :) )
Veramo kms should not remove this prefix. At most, it should remove the 0x prefix from hex encoded public keys, but not the parity.
If it's removing the parity it is a bug.

Oh, it really is removing the parity (https://github.com/uport-project/veramo/blob/9c73d98fd217ed9a612767f49a235cdbf43619cb/packages/kms-local/src/key-management-system.ts#L333).

@nklomp I missed this in my original review. Can you share some insight for why this would be needed?

It shouldn't. I think we might need to look at how to fix it for any keys already stored though

I can add a new script to the @veramo/data-store#migrations, but it will only cover the TypeORM based storage.

oh.. actually, it will have to be done manually because the TypeORM migrations don't have access to the decryption key to get the private keys and recompute the correct public key :(

Yeah, I think we can do it in code as well. A simple check for the prefix on a Secp256r1 key should suffice

I created a PR with the fix and proposed a method to recompute the keys: #1137

I'd appreciate your review @nklomp

mirceanis · 2023-02-21T15:29:45Z

packages/did-provider-jwk/src/resolver.ts

+        assertionMethod: [`${did}#0`],
+        authentication: [`${did}#0`],
+        capabilityInvocation: [`${did}#0`],
+        capabilityDelegation: [`${did}#0`],
+        keyAgreement: [`${did}#0`],


According to the did:jwk spec, the key relationships should consider the use property of a JWK.

When use is enc, only the keyAgreement relationship should be added.

When use is sig, all except the keyAgreement relationship should be added.

I'm not aware of other implementations of this resolver so I can't tell if imposing this alignment with the spec would cause incompatibilities. What do you think?

You could have a look at our Veramo compatible did:jwk implementation:
https://github.com/Sphereon-Opensource/ssi-sdk/blob/develop/packages/jwk-did-provider/src/functions.ts#L70

I have added the use property to the resolved JWK object and the corresponding key relationships according to the spec.

packages/did-provider-jwk/src/resolver.ts

packages/did-provider-jwk/src/jwk-did-provider.ts

nklomp · 2023-02-21T18:22:28Z

Cool. We also created a JWK DID provider for Veramo, which looks very similar but has some small changes compared to this PR.

Also allows to import keys, instead of relying on key generation
Takes the use property of a JWK into account
resolution is more compliant to the general DID spec with errors and resolution metadata

We created it in our repo, as we needed it as part of the JFF Plugfest. I think it makes sense that we merge some features.

https://github.com/Sphereon-Opensource/ssi-sdk/blob/develop/packages/jwk-did-provider

Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

…essage Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

…m/veramo into did-jwk-support

…id-jwk-support

tadejpodrekar · 2023-02-24T12:34:29Z

@mirceanis thank you for your comments. I have updated the implementation according to your suggestions. The changes also contain support for better error handling for the did resolution and the ability to import supported keys. The implementation is a bit different than the one from Sphereon, but the functionalities are covered.

@nklomp thank you very much for pointing that out. This PR should provide the basic functionalities for did:jwk method, improvements and changes to the implementation could be made in subsequent PR. If you have any other suggestions we are open to it.

mirceanis

This looks good!

I still have doubts about the prefix added to Secp256r1 public keys and I'll have to look into the key manager implementation to figure out where it's removing the parity.

~~Did you already spot this to speed up my search?~~ nevermind, I found it.

mirceanis · 2023-02-24T13:12:29Z

packages/did-provider-jwk/src/jwkDidUtils.ts

+  }
+  if(keyType === 'Secp256r1') {
+    const EC = new elliptic.ec('p256')
+    // add '03' prefix to public key


hmm.. this seems odd (no pun intended :) )
Veramo kms should not remove this prefix. At most, it should remove the 0x prefix from hex encoded public keys, but not the parity.
If it's removing the parity it is a bug.

codecov · 2023-02-24T13:30:20Z

Codecov Report

Base: 80.25% // Head: 85.17% // Increases project coverage by +4.92% 🎉

Coverage data is based on head (eb861a9) compared to base (125bf42).
Patch coverage: 83.82% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #1128      +/-   ##
==========================================
+ Coverage   80.25%   85.17%   +4.92%     
==========================================
  Files         118      149      +31     
  Lines        4056    15261   +11205     
  Branches      875     1612     +737     
==========================================
+ Hits         3255    12999    +9744     
- Misses        798     2262    +1464     
+ Partials        3        0       -3

Impacted Files	Coverage Δ
packages/core-types/src/coreEvents.ts	`100.00% <ø> (ø)`
...s/credential-ld/src/module-types/jsonld/index.d.ts	`0.00% <0.00%> (ø)`
packages/data-store-json/src/types.ts	`0.00% <0.00%> (ø)`
packages/did-comm/src/didcomm.ts	`83.26% <ø> (+10.12%)`	⬆️
packages/did-comm/src/index.ts	`100.00% <ø> (ø)`
packages/did-comm/src/message-handler.ts	`72.10% <ø> (+12.64%)`	⬆️
.../protocols/coordinate-mediation-message-handler.ts	`89.63% <ø> (ø)`
packages/did-comm/src/protocols/index.ts	`100.00% <ø> (ø)`
...omm/src/protocols/messagepickup-message-handler.ts	`91.25% <ø> (ø)`
.../did-comm/src/protocols/routing-message-handler.ts	`89.65% <ø> (ø)`
... and 210 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tadejpodrekar added 17 commits February 15, 2023 17:29

feat: added support for did:jwk

04af253

fix: updated keyType check

128db09

feat: added did:jwk support to test agents

97497e8

feat: test did:jwk identifier creation

ca7eabc

feat: test did:jwk resolution

d771675

feat: test creating valid a VC using did:jwk

3fc5063

fix: clarify did:jwk test comment

0cd5b79

fix: update package.json did-provider-jwk module

6b174ac

fix: changed optional parameter type

6e12c00

fix: resolve pr comment

0a71d2a

fix: formatting

07cf5c9

fix: updated did:jwk provider README

02c6c45

feat: did:jwk added keytype support for Secp256r1 and X25519

59f90ba

fix: update did:jwk test comment

11d6d81

fix: updated did:jwk provider README

4bb733b

fix: updated browser test for did:jwk provider

d469ab0

fix: change jwk crv value for Ed25519

7f551e9

mirceanis requested changes Feb 21, 2023

View reviewed changes

tadejpodrekar and others added 11 commits February 22, 2023 08:18

fix: Update packages/did-provider-jwk/src/resolver.ts error messages

161ff07

Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/resolver.ts error message

b2660e2

Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/resolver.ts error message

7f4db44

Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/resolver.ts error message

daea8ae

Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/jwkDidUtils.ts error message

6e134c8

Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/jwk-did-provider.ts error m…

6993688

…essage Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/jwk-did-proveder.ts error m…

de9ebdf

…essage Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/jwk-did-provider.ts error m…

1821a88

…essage Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/jwk-did-provider.ts error m…

207bfc6

…essage Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: Update packages/did-provider-jwk/src/jwk-did-provider.ts error m…

4f8ee63

…essage Co-authored-by: Mircea Nistor <mirceanis@gmail.com>

fix: remove Buffer use in jwk-did-provider

b79559e

tadejpodrekar added 6 commits February 22, 2023 12:51

fix: move jwk provider types to separate file

ab5ca72

Merge branch 'did-jwk-support' of https://github.com/blockchain-lab-u…

9317a62

…m/veramo into did-jwk-support

feat: add key import support and jwk use

5a69cda

fix: add tests and use parameter handling

13e4271

fix: missing bracket

58ffcaf

Merge branch 'next' of https://github.com/tadejpodrekar/veramo into d…

eb861a9

…id-jwk-support

mirceanis approved these changes Feb 24, 2023

View reviewed changes

mirceanis merged commit 0a22d9c into decentralized-identity:next Feb 24, 2023

tadejpodrekar deleted the did-jwk-support branch April 11, 2023 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add did:jwk method support #1128

add did:jwk method support #1128

tadejpodrekar commented Feb 20, 2023

mirceanis left a comment

mirceanis Feb 21, 2023

nklomp Feb 21, 2023

tadejpodrekar Feb 22, 2023

mirceanis Feb 24, 2023

mirceanis Feb 24, 2023

nklomp Feb 24, 2023

mirceanis Feb 24, 2023

mirceanis Feb 24, 2023

nklomp Feb 24, 2023

mirceanis Feb 24, 2023

mirceanis Feb 21, 2023

nklomp Feb 21, 2023

tadejpodrekar Feb 24, 2023

nklomp commented Feb 21, 2023 •

edited

Loading

tadejpodrekar commented Feb 24, 2023

mirceanis left a comment •

edited

Loading

mirceanis Feb 24, 2023

codecov bot commented Feb 24, 2023

add did:jwk method support #1128

add did:jwk method support #1128

Conversation

tadejpodrekar commented Feb 20, 2023

What is being changed

Quality

mirceanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklomp commented Feb 21, 2023 • edited Loading

tadejpodrekar commented Feb 24, 2023

mirceanis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2023

Codecov Report

nklomp commented Feb 21, 2023 •

edited

Loading

mirceanis left a comment •

edited

Loading