Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always set alg and kid on expansion #502

Closed
wants to merge 18 commits into from
Closed

Always set alg and kid on expansion #502

Changes from 15 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c0eb781
Always set alg and kid on expansion
nitro-neal Apr 29, 2024
3e71313
update
nitro-neal Apr 29, 2024
ab67df8
update
nitro-neal Apr 29, 2024
7f8fd7d
update
nitro-neal Apr 29, 2024
24f308e
update
nitro-neal Apr 29, 2024
ec1c910
update
nitro-neal Apr 30, 2024
0c54f87
update
nitro-neal Apr 30, 2024
62cdc2f
Update packages/dids/src/methods/did-dht.ts
nitro-neal May 1, 2024
44b12b6
update
nitro-neal May 1, 2024
e409348
Update packages/dids/src/methods/did-dht.ts
nitro-neal May 2, 2024
d9608f1
Update packages/dids/src/methods/did-dht.ts
nitro-neal May 2, 2024
a62c204
Update packages/dids/src/methods/did-dht.ts
nitro-neal May 2, 2024
07dea7e
Update packages/dids/src/methods/did-dht.ts
nitro-neal May 2, 2024
847ecb1
Update packages/dids/src/methods/did-dht.ts
nitro-neal May 2, 2024
fe5d87f
updating for simpler logic
nitro-neal May 2, 2024
c9493ff
fix lint
nitro-neal May 6, 2024
964ca7b
Merge branch 'main' into set-alg-kid-expansion
nitro-neal May 6, 2024
143b293
wip
nitro-neal May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions packages/dids/src/methods/did-dht.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { extractDidFragment } from '../utils.js';
import { DidError, DidErrorCode } from '../did-error.js';
import { DidVerificationRelationship } from '../types/did-core.js';
import { EMPTY_DID_RESOLUTION_RESULT } from '../types/did-resolution.js';
import { getJoseSignatureAlgorithmFromPublicKey } from '@web5/crypto/utils';

/**
* Represents a BEP44 message, which is used for storing and retrieving data in the Mainline DHT
Expand Down Expand Up @@ -347,6 +348,15 @@ export enum DidDhtRegisteredKeyType {
secp256r1 = 2
}

/**
* Private helper that maps did dht registered key types to their corresponding default algorithm identifiers.
*/
const KeyTypeToDefaultAlgorithmMap = {
[DidDhtRegisteredKeyType.Ed25519] : 'Ed25519',
[DidDhtRegisteredKeyType.secp256k1] : 'ES256K',
[DidDhtRegisteredKeyType.secp256r1] : 'ES256',
}

/**
* Maps {@link https://www.w3.org/TR/did-core/#verification-relationships | DID Core Verification Relationship}
* values to the corresponding record name in the DNS packet representation of a DHT DID document.
Expand Down Expand Up @@ -1013,8 +1023,8 @@ export class DidDhtDocument {
// Process verification methods.
case dnsRecordId.startsWith('k'): {
// Get the method ID fragment (id), key type (t), Base64URL-encoded public key (k), and
// optionally, controller (c) from the decoded TXT record data.
const { id, t, k, c } = DidDhtUtils.parseTxtDataToObject(answer.data);
// optionally, controller (c) and alg (a) from the decoded TXT record data.
const { id, t, k, c, a: parsedAlg } = DidDhtUtils.parseTxtDataToObject(answer.data);

// Convert the public key from Base64URL format to a byte array.
const publicKeyBytes = Convert.base64Url(k).toUint8Array();
Expand All @@ -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 });

// DID DHT spec requires `alg` in keys in the DID document
publicKey.alg = parsedAlg || getJoseSignatureAlgorithmFromPublicKey(publicKey);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(', ')}.`
  );
}

Copy link
Contributor Author

@nitro-neal nitro-neal May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Determine the Key ID (kid): '0' for the identity key or JWK thumbprint for others.
if (id === '0') {
publicKey.kid = '0';
} else if (publicKey.kid === undefined) {
publicKey.kid = await computeJwkThumbprint({ jwk: publicKey });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Initialize the `verificationMethod` array if it does not already exist.
didDocument.verificationMethod ??= [];

Expand Down Expand Up @@ -1159,7 +1179,12 @@ export class DidDhtDocument {
let methodId = vm.id.split('#').pop()!; // Remove fragment prefix, if any.
idLookup.set(methodId, dnsRecordId);

const publicKey = vm.publicKeyJwk;
const publicKey = vm.publicKeyJwk!;

// Always set `kid` to `0` if `methodId` is `0`, even if `kid` is not given, as a caller/user convenience.
if(methodId === '0') {
publicKey.kid = '0';
}
nitro-neal marked this conversation as resolved.
Show resolved Hide resolved

if (!(publicKey?.crv && publicKey.crv in AlgorithmToKeyTypeMap)) {
throw new DidError(DidErrorCode.InvalidPublicKeyType, `Verification method '${vm.id}' contains an unsupported key type: ${publicKey?.crv ?? 'undefined'}`);
Expand All @@ -1177,6 +1202,12 @@ export class DidDhtDocument {
// Define the data for the DNS TXT record.
const txtData = [`id=${methodId}`, `t=${keyType}`, `k=${publicKeyBase64Url}`];

// Only set the algorithm property (`a`) if it differs from the default algorithm for the key type.
const algorithmUsedByKey = getJoseSignatureAlgorithmFromPublicKey(publicKey);
if(algorithmUsedByKey !== KeyTypeToDefaultAlgorithmMap[keyType]) {
txtData.push(`a=${algorithmUsedByKey}`);
}

// Add the controller property, if set to a value other than the Identity Key (DID Subject).
if (vm.controller !== didDocument.id) txtData.push(`c=${vm.controller}`);

Expand Down
Loading