feat: support for did:jwk and p-256, p-384, p-512 #1446

TimoGlastra · 2023-05-01T17:04:37Z

No description provided.

Signed-off-by: Timo Glastra <timo@animo.id>

packages/core/src/crypto/keyUtils.ts

codecov-commenter · 2023-05-01T17:29:26Z

Codecov Report

Merging #1446 (6401107) into main (7dd4061) will decrease coverage by 9.69%.
The diff coverage is 34.61%.

@@            Coverage Diff             @@
##             main    #1446      +/-   ##
==========================================
- Coverage   85.55%   75.86%   -9.69%     
==========================================
  Files         886      848      -38     
  Lines       21156    20434     -722     
  Branches     3635     3524     -111     
==========================================
- Hits        18100    15503    -2597     
- Misses       2881     4593    +1712     
- Partials      175      338     +163

Impacted Files	Coverage Δ
packages/core/src/crypto/keyUtils.ts	`75.00% <ø> (ø)`
...s/core/src/modules/dids/domain/key-type/ed25519.ts	`68.42% <ø> (-27.04%)`	⬇️
...ds/domain/verificationMethod/VerificationMethod.ts	`100.00% <ø> (ø)`
packages/core/src/crypto/EcCompression.ts	`10.41% <10.41%> (ø)`
...re/src/modules/dids/methods/jwk/JwkDidRegistrar.ts	`16.00% <16.00%> (ø)`
...ckages/core/src/modules/dids/methods/jwk/DidJwk.ts	`17.85% <17.85%> (ø)`
.../src/modules/dids/methods/jwk/didJwkDidDocument.ts	`30.76% <30.76%> (ø)`
...ges/core/src/modules/dids/domain/keyDidDocument.ts	`50.94% <33.33%> (-49.06%)`	⬇️
...ore/src/modules/dids/methods/jwk/JwkDidResolver.ts	`33.33% <33.33%> (ø)`
packages/core/src/crypto/JwkTypes.ts	`35.29% <35.29%> (ø)`
... and 15 more

... and 217 files with indirect coverage changes

berendsliedrecht

Really nice addition! Left some remarks, nothing that should be difficult to resolve.

packages/askar/src/utils/askarKeyTypes.ts

packages/core/src/crypto/EcCompression.ts

berendsliedrecht · 2023-05-01T17:49:41Z

packages/core/src/crypto/Jwk.ts

+  // p-256, p-384, p-521
+  if (isP256Jwk(jwk) || isP384Jwk(jwk) || isP521Jwk(jwk)) {
+    // TODO: do we want to use the compressed key in the Key instance?
+    const publicKeyBuffer = Buffer.concat([TypedArrayEncoder.fromBase64(jwk.x), TypedArrayEncoder.fromBase64(jwk.y)])


Okay so I did some digging and we should a check for uncompressed points.

If the prefix is 0x3 or 0x2, the key is uncompressed and we do not support that. Or we would have to add support, which is relatively easy, but maybe unnecessary.

this also means that we would have to add the 0x4 prefix when using compressed.

Elliptic curve point compression w3c-ccg/did-method-key#32

Sorry for the lack of sources, I will do my best to find more.

I think the did:key spec only supports the compressed variant (that's what I understand from the thread), but you're saying the JWK itself could contain both compressed and uncompressed variants? I think we could just support uncompressed as well (as it's just skipping the compression/uncompression and thus simpler).

We'll always encode to compressed variant (because why not), but for decompression I should just look if the key is prefixed with 0x3 or 0x2 and if so, remove that previx and not do the decompression?

AFAIK the JWK spec does not allow for compressed values.

Hmm in that case, there's no reason to support the uncompressed variant I think? did:key only supports compressed, and the jwk will have separate values (x and y), not a single concatenated public key

Agreed. We can always add uncompressed later on, if needed.

AFAIK the JWK spec does not allow for compressed values.

The JWK spec does very much support compressed and uncompressed. We are talking specifically about key representation and not the compressed format of a jwt, etc.

I think the did:key spec only supports the compressed variant (that's what I understand from the thread), but you're saying the JWK itself could contain both compressed and uncompressed variants?

No, I mean that we should check whether it is compressed or uncompressed, based on the prefix, and handle accordingly. We will break keys if this mechanism is implemented incorrectly which will lead to a lot of errors.

I think we could just support uncompressed as well (as it's just skipping the compression/uncompression and thus simpler).

Yes! I don't think it's too much effort.

We'll always encode to compressed variant (because why not), but for decompression I should just look if the key is prefixed with 0x3 or 0x2 and if so, remove that previx and not do the decompression?

Yes, I would have to look at the exact algo for this, but I think that's it.

@blu3beri I've addressed all comments except the ones related to compression of P keys. Can you help me out a bit in pointing out what I need to exactly?

berendsliedrecht · 2023-05-01T17:54:21Z

packages/core/src/crypto/Jwk.ts

+    }
+  }
+
+  if (key.keyType === KeyType.P256 || key.keyType === KeyType.P384 || key.keyType === KeyType.P521) {


So this one would also need to account for the prefix.

Not really right? We could just always output the compressed variant, but support both variants the other way around

The compressed variant, using only the X coordinate for the representation needs a prefix in the jwk of 0x03 or 0x02 I think depending on wether the y coordinate is odd or even (but that's from memory, would need a source but I am fairly sure). If we encode it without a prefix it will be interpreted incorrectly by the receiver.

packages/core/src/crypto/JwkTypes.ts

packages/core/src/crypto/__tests__/Jwk.test.ts

packages/core/src/crypto/keyUtils.ts

Co-authored-by: Berend Sliedrecht <61358536+blu3beri@users.noreply.github.com> Signed-off-by: Timo Glastra <timo@animo.id>

packages/core/src/crypto/EcCompression.ts

packages/core/src/crypto/JwkTypes.ts

karimStekelenburg · 2023-05-01T20:21:59Z

packages/core/src/crypto/JwkTypes.ts

  x: string
  y?: string
+  use?: 'sig' | 'enc'


What do we do if the use param isn't provided? According to the spec we can enforce it:

4.2. "use" (Public Key Use) Parameter

The "use" (public key use) parameter identifies the intended use of
the public key. The "use" parameter is employed to indicate whether
a public key is used for encrypting data or verifying the signature
on data.

Values defined by this specification are:

o "sig" (signature)
o "enc" (encryption)

Other values MAY be used. The "use" value is a case-sensitive
string. Use of the "use" member is OPTIONAL, unless the application
requires its presence.

I think this depends on how purely we want to follow the spec. If we go 100%, we need to allow arbitrary string values as well. If not, I think we should maybe enforce the user to specify its value?

For now, we just use it in parsing. It's required to be understood (not required to be present) for did:jwk. So we use when handling jwks, but only when we parse a JWK from someone else. When we create a JWK, for now I just always use the full capabilities of a key, and we don't include the use key. That's to to keep it 'simpler' (support the spec, but don't expose all functionality on our side).

I can update it to be a 'enc' | 'sig' | string value

What do you mean by "required to be understood"?

I'm fine with keeping it as is as long as we actually use it. Adding the string type without adding logic to handle that doesn't make sense to me.

I was just wondering why we include it, as it's optional, only used for incoming JWKs, and only used by the forEncrypting and forSigning getters. Can we actually use those getters (as the use value is optional)?

I was just wondering why we include it

Because it's required by the did:jwk spec:

If the JWK contains a use property with the value "sig" then the keyAgreement property is not included in the DID Document. If the use value is "enc" then only the keyAgreement property is included in the DID Document.
The JWK should have the appropriate use value set to match the capabilities of the specified crv. For example, the
curve ed25519 is only valid for "sig" use and X25519 is only valid for "enc" (see RFC 8037 and the second example below).
The JWK may contain additional custom properties and values which will be accessible only in the verificationMethod. Any additional properties other than use (as documented above) are not referenced or used in the generation of the DID Document.

So we use it in the minimal way possible. When we parse a did:jwk we do the following:

If no use property is present, we check the default capabilities of a key type (enc, sig or both)

If a use property is present, we check if the value is valid for the key type (ed25519 can't have enc). If it's valid we only inlcude the verificadtionMethod types that link to sig vs enc. So if it's enc, only the keyAgreement will be added to the resolved did:jwk document

karimStekelenburg · 2023-05-01T20:28:50Z

packages/core/src/crypto/KeyType.ts

  Bls12381g1 = 'bls12381g1',
  Bls12381g2 = 'bls12381g2',
  X25519 = 'x25519',
+  P256 = 'p256',
+  P384 = 'p384',
+  P521 = 'p521',


Why are the values here different from the spec (e.g. p256 vs P-256)?

Because we started with defining key types as lowercase in AFJ, and that's what we're now doing for consistency I guess 🤷

This is probably related to the discussion here

Because we started with defining key types as lowercase in AFJ, and that's what we're now doing for consistency I guess 🤷

If we aim for consistency, I'd say we follow the specs 😉. That way, people who are new to AFJ but are familiar with the specs will recognize things faster.

I would say consistency is keeping aligned with what we're already doing, which is lowercase.

I would like to pick this up as part of the wallet refactoring, as there's different formats being used by Askar, JWK, etc... If we want to follow the JWK spec, it would be P-256, which would be inconsistent with what we have in AFJ (and we should then also change all other key types).

packages/core/src/modules/dids/methods/jwk/DidJwk.ts

karimStekelenburg · 2023-05-01T20:36:56Z

packages/core/src/modules/dids/domain/key-type/keyDidJsonWebKey.ts

+import { getJsonWebKey2020VerificationMethod } from '../verificationMethod'
+import { VERIFICATION_METHOD_TYPE_JSON_WEB_KEY_2020, isJsonWebKey2020 } from '../verificationMethod/JsonWebKey2020'
+
+export const keyDidJsonWebKey: KeyDidMapping = {


Not something for now, but I personally find these kinds of objects that contains various attributes and functions confusing. I know this is normal in the world of JS, but I don't think it's in line with the rest of the code base.

In this particular case, I'm also not a fan of how it's implemented. But I do think it's quite a normal pattern to use, and don't see anything wrong with it. If you're only interested in static methods, an object that implements an interface is fine I think.

Would you prefer to refactor this to a class?

I agree with your point, but I just think it's very different from the rest of the code base. I would prefer refactoring to a class, purely out of consistency and readability reasons.

This is a personal preference, though. I'm not a fan of the JS way of things ;)

Sure we can refactor to a class. Just need to think about better naming for this paradigm

Signed-off-by: Timo Glastra <timo@animo.id>

berendsliedrecht · 2023-05-10T11:13:51Z

packages/core/src/crypto/EcCompression.ts

+ */
+function compressECPoint(x: Uint8Array, y: Uint8Array): Uint8Array {
+  const out = new Uint8Array(x.length + 1)
+  out[0] = 2 + (y[y.length - 1] & 1)


Yeah it seems like here it does the prefix, so it is already added. But there is no validate check so I am not sure what happend if the prefix is not 0x02 or 0x03.

berendsliedrecht · 2023-05-10T11:14:17Z

packages/core/src/crypto/EcCompression.ts

+export function expand(publicKey: Uint8Array, curve: 'P-256' | 'P-384' | 'P-521'): Uint8Array {
+  const publicKeyComponent = Buffer.from(publicKey).toString('hex')
+  const { prime, b, pIdent } = getConstantsForCurve(curve)
+  const signY = new Number(publicKeyComponent[1]).valueOf() - 2


Also happens here.

berendsliedrecht

Great work @TimoGlastra! I see the prefixes of compressed and uncompressed are dealt with by code I did not see :). LGTM!

Signed-off-by: Timo Glastra <timo@animo.id>

feat: support for did:jwk and p-256, p-384, p-512

1bd5c5c

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra requested a review from a team as a code owner May 1, 2023 17:04

TimoGlastra added 2 commits May 1, 2023 19:06

fix: type error

ff0cea4

Signed-off-by: Timo Glastra <timo@animo.id>

Merge branch 'main' into feat/did-jwk

2104c0d

TimoGlastra commented May 1, 2023

View reviewed changes

packages/core/src/crypto/keyUtils.ts Outdated Show resolved Hide resolved

berendsliedrecht requested changes May 1, 2023

View reviewed changes

Update packages/core/src/crypto/EcCompression.ts

79398ba

Co-authored-by: Berend Sliedrecht <61358536+blu3beri@users.noreply.github.com> Signed-off-by: Timo Glastra <timo@animo.id>

karimStekelenburg reviewed May 1, 2023

View reviewed changes

TimoGlastra added 2 commits May 9, 2023 11:49

Merge remote-tracking branch 'upstream/main' into feat/did-jwk

c1327ce

chore: address feedback

3d4284e

Signed-off-by: Timo Glastra <timo@animo.id>

berendsliedrecht reviewed May 10, 2023

View reviewed changes

TimoGlastra mentioned this pull request May 10, 2023

feat: support more key types in jws service #1453

Merged

berendsliedrecht approved these changes May 10, 2023

View reviewed changes

Merge branch 'main' into feat/did-jwk

6401107

TimoGlastra enabled auto-merge (squash) May 10, 2023 19:35

test: incorrect seed

c5ba6db

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra merged commit 700d3f8 into openwallet-foundation:main May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for did:jwk and p-256, p-384, p-512 #1446

feat: support for did:jwk and p-256, p-384, p-512 #1446

TimoGlastra commented May 1, 2023

codecov-commenter commented May 1, 2023 •

edited

Loading

berendsliedrecht left a comment

berendsliedrecht May 1, 2023

TimoGlastra May 1, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 1, 2023

karimStekelenburg May 1, 2023

berendsliedrecht May 6, 2023

berendsliedrecht May 6, 2023

TimoGlastra May 9, 2023

berendsliedrecht May 1, 2023

TimoGlastra May 1, 2023

berendsliedrecht May 6, 2023

karimStekelenburg May 1, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 1, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 9, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 1, 2023

karimStekelenburg May 1, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 9, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 1, 2023

karimStekelenburg May 1, 2023

TimoGlastra May 9, 2023

berendsliedrecht May 10, 2023

berendsliedrecht May 10, 2023

berendsliedrecht left a comment

feat: support for did:jwk and p-256, p-384, p-512 #1446

feat: support for did:jwk and p-256, p-384, p-512 #1446

Conversation

TimoGlastra commented May 1, 2023

codecov-commenter commented May 1, 2023 • edited Loading

Codecov Report

berendsliedrecht 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

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

berendsliedrecht left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 1, 2023 •

edited

Loading