Always set alg and kid on expansion #502

nitro-neal · 2024-04-29T18:29:51Z

This pr resolves #3 from this issue:
#497

Always set alg and kid on expansion (to the values in the registry), support overriding of alg values

toDnsPacket:

check if the alg on the JWK == the alg in the registry for the given key type

fromDnsPacket:

KID is always either 0 for the identity key or the JWK thumbprint for all other keys (no overriding it)
the alg is always set to the default from the registry (https://did-dht.com/registry/#key-type-index) but can be overriden if you specify the alg property
alg can be omitted and if it is you assume it’s the default

changeset-bot · 2024-04-29T18:29:54Z

⚠️ No Changeset found

Latest commit: 143b293

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-04-29T18:32:40Z

TBDocs Report

✅ No errors or warnings

@web5/api

Project entry file: packages/api/src/index.ts

@web5/crypto

Project entry file: packages/crypto/src/index.ts

@web5/crypto-aws-kms

Project entry file: packages/crypto-aws-kms/src/index.ts

@web5/dids

Project entry file: packages/dids/src/index.ts

@web5/credentials

Project entry file: packages/credentials/src/index.ts

TBDocs Report Updated at 2024-05-07T21:17:36Z 143b293

codecov-commenter · 2024-04-29T18:36:04Z

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.81%. Comparing base (eabe5ca) to head (44b12b6).

❗ Current head 44b12b6 differs from pull request most recent head 143b293. Consider uploading reports for the commit 143b293 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
- Coverage   91.03%   90.81%   -0.23%     
==========================================
  Files         116      116              
  Lines       29562    29439     -123     
  Branches     2177     2161      -16     
==========================================
- Hits        26912    26734     -178     
- Misses       2615     2669      +54     
- Partials       35       36       +1

Components	Coverage Δ
agent	`79.90% <ø> (-0.73%)`	⬇️
api	`97.91% <ø> (+0.13%)`	⬆️
common	`98.68% <ø> (ø)`
credentials	`95.25% <ø> (-0.01%)`	⬇️
crypto	`93.81% <ø> (ø)`
dids	`97.63% <94.11%> (-0.06%)`	⬇️
identity-agent	`96.70% <ø> (ø)`
crypto-aws-kms	`100.00% <ø> (ø)`
proxy-agent	`96.70% <ø> (ø)`
user-agent	`96.70% <ø> (ø)`

packages/dids/src/methods/did-dht.ts

Co-authored-by: Gabe <[email protected]>

thehenrytsai

looks legit but no need for tests? 😆

packages/dids/src/methods/did-dht.ts

thehenrytsai · 2024-05-02T21:05:19Z

packages/dids/src/methods/did-dht.ts

@@ -1025,6 +1035,16 @@ export class DidDhtDocument {
          // Convert the public key from a byte array to JWK format.
          let publicKey = await DidDhtUtils.keyConverter(namedCurve).bytesToPublicKey({ publicKeyBytes });

+          // Always set the algorithm on did:dht expansion.
+          publicKey.alg = parsedAlg || getJoseSignatureAlgorithmFromPublicKey(publicKey);


Sanity: getJoseSignatureAlgorithmFromPublicKey() does not seem to return Ed25519 that will be expected of vector 3.

am I using this correctly?

export function getJoseSignatureAlgorithmFromPublicKey(publicKey: Jwk): string { const curveToJoseAlgorithm: Record<string, string> = { 'Ed25519' : 'EdDSA', 'P-256' : 'ES256', 'P-384' : 'ES384', 'P-521' : 'ES512', 'secp256k1' : 'ES256K', }; // If the key contains an `alg` property that matches a JOSE registered algorithm identifier, // return its value. if (publicKey.alg && Object.values(curveToJoseAlgorithm).includes(publicKey.alg)) { return publicKey.alg; } // If the key contains a `crv` property, return the corresponding algorithm. if (publicKey.crv && Object.keys(curveToJoseAlgorithm).includes(publicKey.crv)) { return curveToJoseAlgorithm[publicKey.crv]; } throw new Error( `Unable to determine algorithm based on provided input: alg=${publicKey.alg}, crv=${publicKey.crv}. ` + `Supported 'alg' values: ${Object.values(curveToJoseAlgorithm).join(', ')}. ` + `Supported 'crv' values: ${Object.keys(curveToJoseAlgorithm).join(', ')}.` ); }

I think the alg is always there for our jwks, but is that not necessarily the case for outside jwks inside the did dht??

if the alg is always there we can just do

if(parsedAlg) { publicKey.alg = parsedAlg }

I don't get if(parsedAlg) { publicKey.alg = parsedAlg }, if parsedAlg is always defined, then there is nothing do right? since:

const { id, t, k, c, a: parsedAlg } = DidDhtUtils.parseTxtDataToObject(answer.data);

parsed alg will not always be defined, but publicKey.alg is always defined

thehenrytsai · 2024-05-02T21:18:39Z

packages/dids/src/methods/did-dht.ts

+          publicKey.alg = parsedAlg || getJoseSignatureAlgorithmFromPublicKey(publicKey);
+
+          // Determine the Key ID (kid): '0' for the identity key or JWK thumbprint for others. Always set alg on expansion.
+          if (id !== '0' && publicKey.kid === undefined) {


For my understanding: why would kid be defined when id is not 0? May have opportunity to simplify the condition here?

simplifying to:

if (id === '0') { publicKey.kid = '0'; } else if (publicKey.kid === undefined) { publicKey.kid = await computeJwkThumbprint({ jwk: publicKey }); }

the publicKey.kid should be defined, but if it is not, we can recompute it deterministically with computeJwkThumbprint

packages/dids/src/methods/did-dht.ts

Co-authored-by: Henry Tsai <[email protected]>

thehenrytsai · 2024-05-03T00:23:57Z

packages/dids/src/methods/did-dht.ts

+          if (id === '0') {
+            publicKey.kid = '0';
+          } else if (publicKey.kid === undefined) {
+            publicKey.kid = await computeJwkThumbprint({ jwk: publicKey });
+          }


@decentralgabe just confirmed that id MUST be 0 for identity key, and undefined otherwise, (we should have tests if they don't exist to make sure these are true in implementation), so it seems to me the condition can simply be:

Suggested change

if (id === '0') {

publicKey.kid = '0';

} else if (publicKey.kid === undefined) {

publicKey.kid = await computeJwkThumbprint({ jwk: publicKey });

}

if (id === undefined) {

publicKey.kid = await computeJwkThumbprint({ jwk: publicKey });

}

Happy to discuss if I am not making sense!

i'm going to update the spec language to always remove the id property
@thehenrytsai correctly pointed out we can discern the 0 key based on the did identifier
all other keys will use the thumbprint

Always set alg and kid on expansion

c0eb781

update

3e71313

nitro-neal added 3 commits April 29, 2024 11:51

update

ab67df8

update

7f8fd7d

update

24f308e

nitro-neal mentioned this pull request Apr 29, 2024

Update did:dht to conform to the latest version of the specification #497

Open

nitro-neal added 2 commits April 30, 2024 11:15

update

ec1c910

update

0c54f87

decentralgabe reviewed Apr 30, 2024

View reviewed changes

packages/dids/src/methods/did-dht.ts Outdated Show resolved Hide resolved

nitro-neal and others added 2 commits May 1, 2024 11:05

Update packages/dids/src/methods/did-dht.ts

62cdc2f

Co-authored-by: Gabe <[email protected]>

update

44b12b6

nitro-neal marked this pull request as ready for review May 1, 2024 18:56

nitro-neal requested review from jiyoonie9, diehuxx, mistermoe, frankhinek and thehenrytsai as code owners May 1, 2024 18:56

thehenrytsai reviewed May 2, 2024

View reviewed changes

nitro-neal and others added 6 commits May 2, 2024 15:51

Update packages/dids/src/methods/did-dht.ts

e409348

Co-authored-by: Henry Tsai <[email protected]>

Update packages/dids/src/methods/did-dht.ts

d9608f1

Co-authored-by: Henry Tsai <[email protected]>

Update packages/dids/src/methods/did-dht.ts

a62c204

Co-authored-by: Henry Tsai <[email protected]>

Update packages/dids/src/methods/did-dht.ts

07dea7e

Co-authored-by: Henry Tsai <[email protected]>

Update packages/dids/src/methods/did-dht.ts

847ecb1

Co-authored-by: Henry Tsai <[email protected]>

updating for simpler logic

fe5d87f

decentralgabe previously approved these changes May 2, 2024

View reviewed changes

thehenrytsai reviewed May 3, 2024

View reviewed changes

fix lint

c9493ff

nitro-neal dismissed decentralgabe’s stale review via c9493ff May 6, 2024 22:43

nitro-neal added 2 commits May 6, 2024 15:43

Merge branch 'main' into set-alg-kid-expansion

964ca7b

wip

143b293

nitro-neal marked this pull request as draft May 7, 2024 21:14

nitro-neal closed this May 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always set alg and kid on expansion #502

Always set alg and kid on expansion #502

nitro-neal commented Apr 29, 2024 •

edited

Loading

changeset-bot bot commented Apr 29, 2024 •

edited

Loading

github-actions bot commented Apr 29, 2024 •

edited

Loading

codecov-commenter commented Apr 29, 2024 •

edited

Loading

thehenrytsai left a comment

thehenrytsai May 2, 2024

nitro-neal May 2, 2024

nitro-neal May 2, 2024 •

edited

Loading

thehenrytsai May 3, 2024

nitro-neal May 6, 2024

thehenrytsai May 2, 2024

nitro-neal May 2, 2024

nitro-neal May 2, 2024

thehenrytsai May 3, 2024

decentralgabe May 3, 2024

Always set alg and kid on expansion #502

Always set alg and kid on expansion #502

Conversation

nitro-neal commented Apr 29, 2024 • edited Loading

changeset-bot bot commented Apr 29, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Apr 29, 2024 • edited Loading

@web5/api

@web5/crypto

@web5/crypto-aws-kms

@web5/dids

@web5/credentials

codecov-commenter commented Apr 29, 2024 • edited Loading

Codecov Report

thehenrytsai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal May 2, 2024 • edited Loading

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

nitro-neal commented Apr 29, 2024 •

edited

Loading

changeset-bot bot commented Apr 29, 2024 •

edited

Loading

github-actions bot commented Apr 29, 2024 •

edited

Loading

codecov-commenter commented Apr 29, 2024 •

edited

Loading

nitro-neal May 2, 2024 •

edited

Loading