TBD54566975 / web5-spec

Web5 Spec
https://tbd54566975.github.io/web5-spec/
Apache License 2.0
7 stars 5 forks source link

Update `did:jwk` resolve test vectors #69

Closed mistermoe closed 10 months ago

mistermoe commented 10 months ago
mistermoe commented 10 months ago

great point @decentralgabe we should definitely do that. will update the PR to reflect

frankhinek commented 10 months ago

just a comment (non blocking) - I would recommend adding vectors for all the key type/algs we support. I've run into issues by assuming these work and realizing much later they in fact don't. Forcing compatibility with each support key type here will prevent that risk.

Great point. One thing to note is that we're not yet at a point of consistent support for crypto algs across the web5-* libs:

We could always expand the did:jwk test vector to include what should be supported and not run the vector for all SDKs until their crypto packages are fully compliant.

frankhinek commented 10 months ago

Do we want to consider canonicalizing the public key before Base64URL encoding?

per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

andresuribe87 commented 10 months ago

Do we want to consider canonicalizing the public key before Base64URL encoding?

per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

I thought that would be part of creation, but I believe this PR is only for resolution

frankhinek commented 10 months ago

Do we want to consider canonicalizing the public key before Base64URL encoding? per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

I thought that would be part of creation, but I believe this PR is only for resolution

It is but this PR's resolution results are based on not-canonicalizing the public key. As a result, if a web5-* implementation does canonicalization it will fail this vector.

andresuribe87 commented 10 months ago

Do we want to consider canonicalizing the public key before Base64URL encoding? per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

I thought that would be part of creation, but I believe this PR is only for resolution

It is but this PR's resolution results are based on not-canonicalizing the public key. As a result, if a web5-* implementation does canonicalization it will fail this vector.

I'm a bit confused still, hoping you can help me understand.

Are you suggesting that resolution should canonicalize the encoded JWK before putting it in the did document? I.e. something like the following

resolve("did:jwk:<foo>").didDocument.verificationMethod[0].publicKeyJwk == canonicalize(decode64url("<foo>"))
decentralgabe commented 10 months ago

Canonicalization was removed from the latest draft, so I would advise against it https://github.com/quartzjer/did-jwk/pull/21/files

frankhinek commented 10 months ago

Canonicalization was removed from the latest draft, so I would advise against it https://github.com/quartzjer/did-jwk/pull/21/files

Excellent insight! Thanks @decentralgabe. Makes the decision easy.

mistermoe commented 10 months ago

updated vectors to include supported cryptographic algorithms - secp256k1 & Ed25519