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.

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

add support for all DID doc updates from latest DID core spec #110

Open
mirceanis opened this issue Mar 11, 2021 · 19 comments
Open

add support for all DID doc updates from latest DID core spec #110

mirceanis opened this issue Mar 11, 2021 · 19 comments
Labels
enhancement New feature or request pinned an issue that may take a while to fix and should not be closed automatically.

Comments

@mirceanis
Copy link
Member

Reposting the proposal from #99 (comment) as a separate issue:

The current DIDAttributeChanged scheme used to populate the DID document is insufficient for covering all the cases described by the more modern versions of the DID spec.

@AlexYenkalov has suggested a naming scheme that uses a set of characters to describe where a key should be placed.

const DOCUMENT_SECTION_FLAGS = {
  v: 'verificationMethod',
  a: 'assertionMethod',
  u: 'authentication',
  k: 'keyAgreement',
  d: 'capabilityDelegation',
  i: 'capabilityInvocation',
  s: 'service'
}

Besides this, the did/ prefix can also be shortened (or dropped?) to make room for more expressive future additions

So, instead of trying to match the attribute name against:

/^did\/(pub|auth|svc)\/(\w+)(\/(\w+))?(\/(\w+))?$/

the resolver would look for something like this

/^d\/([vaukdis]*)\/(\w+)(\/(\w+))?$/

Examples:

  • if a key needs to be expressed as a verificationMethod and also be listed in the authentication, assertionMethod, capabilityDelegation and capabilityInvocation sections, the attribute name would start with d/vaudi/
  • if a keyAgreement key is added, the attribute name would start with d/vk/
  • if a service is added, then d/s/

This can be made even more efficient by recognizing that keys would always be listed in the verificationMethod array and referenced from the other sections, so the /v/ flag can be dropped and considered implicit as long as the /s/ flag is not used.

There is also the problem of the types of verificationMethods that can be listed.
The names of the methods listed in the DID spec registries are quite long, so they would not fit into the 32-byte limitation of the attribute name.

To get around this problem, I propose implementing a mapping to shorter strings:

const VERIFICATION_METHOD_TYPES: Record<string, string> = {
  jwk: 'JsonWebKey2020',
  secp256k1: 'EcdsaSecp256k1VerificationKey2019',
  secpk1Rec: 'EcdsaSecp256k1RecoveryMethod2020',
  secpk1Sch: 'SchnorrSecp256k1VerificationKey2019',
  ed25519: 'Ed25519VerificationKey2018',
  x25519: 'X25519KeyAgreementKey2019',
  blsG1: 'Bls12381G1Key2020',
  blsG2: 'Bls12381G2Key2020',
  gpg: 'GpgVerificationKey2020',
  rsa: 'RsaVerificationKey2018'
}

If the list of these methods is expected to remain relatively stable, an even more efficient mapping can be created:

const VERIFICATION_METHOD_TYPES: Record<string, string> = {
  j: 'JsonWebKey2020',
  s: 'EcdsaSecp256k1VerificationKey2019',
  R: 'EcdsaSecp256k1RecoveryMethod2020',
  S: 'SchnorrSecp256k1VerificationKey2019',
  e: 'Ed25519VerificationKey2018',
  x: 'X25519KeyAgreementKey2019',
  b: 'Bls12381G1Key2020',
  B: 'Bls12381G2Key2020',
  g: 'GpgVerificationKey2020',
  r: 'RsaVerificationKey2018'
}

Continuing along this line of reasoning, the key encoding can be shortened to one of:

const KEY_ENCODINGS: Record<string, string> = {
  `j`: `publicKeyJwk`,
  `58`: `publicKeyBase58`,
  `e`: `ethereumAddress`,
  `b`: `blockchainAccountId`,

  //other legacy types can also be expressed, since they are trivial to implement without dependencies:
  `16`: `publicKeyHex`,
  `64`: `publicKeyBase64`,

  //these are very inefficient in terms of storage
  `g`: `publicKeyGpg`,
  `p`: 'publicKeyPem'
}

There can be an implicit encoding of base58btc(publicKeyBase58) if none is specified.

Example:

  • to list an X25519KeyAgreementKey2019 with base58btc encoding, one would publish an attribute with the name d/k/x/58
  • to list an RsaVerificationKey2018 assertionMethod with publicKeyPem encoding, one would publish an attribute with the name d/a/r/p
  • to list an EcdsaSecp256k1RecoveryMethod2020 assertionMethod and authentication with ethereumAddress encoding, one would publish an attribute with the name d/au/R/e

Any thoughts?

cc @awoie @rado0x54

@awoie
Copy link
Member

awoie commented Mar 11, 2021

This seems to be reasonable. The PR should then also clean up the DID ETHR spec. Although ideally, we would just use raw bytes instead of strings to be more flexible but I guess that would be a more breaking change. Then we could reserve 1-3 bytes for the different options and maintain a list in the DID ETHR spec for the mapping.

@mirceanis
Copy link
Member Author

I agree that raw bytes would fit the bill in terms of efficiency, but would be a tiny bit harder to implement. I guess the string solution is "close enough"

The thing that is hardest for me to accept is the publicKeyJwk encoding, since it would require implementors of the spec to adopt a JOSE stack as dependency for this reason alone, or try to reason about all the possible combinations and redundancies that the JWK introduces.

@awoie
Copy link
Member

awoie commented Mar 12, 2021

@mirceanis Is anyone specifically asking for JWK? The spec does not mandate us to support it. IMO, PEM is even worse. We could say we only support base58 or multibase (depending on where the DID Spec lands).

@mirceanis
Copy link
Member Author

Good question. I suppose there are no direct requests for JWK, nor for PEM (which, I agree, is even worse).
I'm OK with not supporting JWK directly, but this may create issues for folks that want to use JOSE stacks without conversions.

Alternatively, we could provide workarounds that are explicitly called "expensive" to maybe deter folks from using them.
Examples:

  • for a secp256k1 assertionMethod as serialized JWK one would use an attribute with the name d/a/s/jwk_expensive and the value hexToBytes(JSON.stringify(jsonWebKey))
  • PEM encoding for an RSA one would be d/a/r/pem_expensive and the value stringToBytes(< pem string >)
    This way, the resolver doesn't actually deal with any encoding

@awoie
Copy link
Member

awoie commented Mar 15, 2021

Couldn't we provide a utility function that converts b58 -> JWK? I don't think a lot of ppl are using PEM anyways.

@mirceanis
Copy link
Member Author

Of course we could, but the question is if this kind of conversion should be part of the spec or not

@AlexYenkalov
Copy link

@mirceanis @awoie We currently facing the case where we would need to use the initial key of a newly created did:ethr to sign credentials/presentations using JsonWebSignature2020

JSON-LD signing/verification logic plays well here only if we have publicKeyJwk representation inside of the document.

It's kind of a show-stopper for us now.

The main problem is that the key of an initial key-pair of did:ethr is represented through blockchainAccountId and can't be translated to a respective publicKeyJwk representation.

@mirceanis
Copy link
Member Author

@AlexYenkalov if you are starting from a keypair, then you can use did:ethr:<publicKey> instead of did:ethr:<ethereumAddress>. Starting with the ethereumAddress representation, it's impossible to add the full public key to the DID document for fresh DIDs

The default document for a did:ethr<publicKey> will contain a verificationMethod with publicKeyHex that can be converted to JWK.
For secp256k1 I suppose the dependencies to do this conversion to JWK are already imported by the resolver so perhaps that would be an option.

@stale
Copy link

stale bot commented May 12, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the incomplete This is not well defined. Ask for clarification before starting work label May 12, 2021
@mirceanis mirceanis added the pinned an issue that may take a while to fix and should not be closed automatically. label May 12, 2021
@stale stale bot removed the incomplete This is not well defined. Ask for clarification before starting work label May 12, 2021
@tankcdr
Copy link

tankcdr commented Apr 6, 2022

@mirceanis I'm going to fund this work. Is there general agreement on how to move forward updating ethr-did and ethr-did-resolver? Are you the best point of contact for my team to contact?

@mirceanis
Copy link
Member Author

mirceanis commented Apr 6, 2022

@tankcdr I'm your contact, yes.

There were some unanswered questions about key encoding.

For efficiency and cost reasons, keys should be stored in their raw byte form in the EVM events, and it would be up to the resolver to interpret them back into jwk, pem, base58, etc encodings after they are interpreted.
The problem with this is that the resolver would need to understand how to do these conversions, introducing a lot of overhead both to the resolver and the spec.

This means that we would need to reduce the possible encodings to a minimum.
It is trivial to support different bases, but I'm not very familiar with the ramifications of JWK.

@shresthaal
Copy link

@mirceanis i am working with @tankcdr to work on this enhancement. Can we connect this week?

@mirceanis
Copy link
Member Author

@mirceanis i am working with @tankcdr to work on this enhancement. Can we connect this week?

I'm unable to commit to a schedule in the next few weeks because I recently went on paternity leave, but I will try to read proposals async, if you make them. Don't count on timely replies from me yet :)

Regarding the unanswered questions I posted before,
@awoie proposed that we only support a couple of encodings for keys and provide some tools in a different library to convert document keys to new encodings for situations where that would be needed. This seems to me like the best of both worlds since it keeps the spec and reference resolver simpler.

@leowcy
Copy link

leowcy commented May 2, 2022

@mirceanis Hey Mirceanis. I noticed that in the latest W3C core spec, there are two more keys (controller & alsoKnownAs) which are not shown in the suggested DOCUMENT_SECTION_FLAGS scheme above. Can I ask why and will these two also be added? Looks like someone has asked the similar question for "alsoKnownAs"on another ticket (decentralized-identity/veramo#260) which has been close.

@mirceanis
Copy link
Member Author

Hi @leowcy ,
Those 2 keys are optional in the DID spec and were not added to this DID method because there is no equivalent mechanism that could populate them.

There is a potential use for alsoKnownAs to link together a did:ethr:<publicKey> to its subset referred to by did:ethr:<ethereumAddress>, or perhaps even a little further with using did:ethr:<ens.domain>, but at this point these are not necessary, and would only serve to complicate the spec.

did:ethr is based on ERC1056 which has the concept of owner, but that is only useful for the ERC1056 smart contract implementation, and is not meant to be reflected in the DID document as a controller using a DID URI format, but rather as one of the verification methods using at most a blockchainAccountId, since it is represented internally as an ethereum address.

@veromassera
Copy link

veromassera commented Dec 11, 2023

Hello @mirceanis, Is there any progress on this topic?

I need to store bls public keys in did:ethr and I also need the resolver to be able to return this information.
I suppose that changing the resolver so that it can interpret the signature type data is enough, right? But if we don't adopt a standard it doesn't make sense. Thanks for the information you can give me.

@mirceanis
Copy link
Member Author

mirceanis commented Dec 18, 2023

@veromassera There was no progress on this, but you can still add other key types using the existing resolver code.
For example, adding a BLS G2 key for assertion can be expressed by setting an attribute name of did/pub/Bls12381G2Key2020.

See this test case as an example:

it('add Bls12381G2Key2020 assertion key', async () => {

@veromassera
Copy link

Thank you very much for the example.
It was very helpful.
I had not noticed the OR in this line "const type = legacyAttrTypes[match[4]] || match[4]"

@mirceanis
Copy link
Member Author

You're welcome!

It's still not a complete solution, though, as it won't work with all key type / verification relationships.

The example I posted can't be used to add a key to the authentication relationship ( using /sigAuth) but can be used for keyAgreement ( /enc) because of the 32 byte limit for the attrName param.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pinned an issue that may take a while to fix and should not be closed automatically.
Projects
None yet
Development

No branches or pull requests

7 participants