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

fix: corrections and clarifications for did:peer:2 #62

Merged

Conversation

dbluhm
Copy link
Member

@dbluhm dbluhm commented Sep 29, 2023

As the author of the did:peer:4 generation algorithm, I obviously would like to see us all move to that method. However, it seems inevitable that we'll continue to use did:peer:2, at least for a while. To ease interop challenges across did:peer:2 implementations, I propose these clarifications to the did:peer:2 spec.

I believe this should address the concerns raised in #56.

cc @TelegramSam @FabioPinheiro @swcurran @genaris @TimoGlastra

@dbluhm
Copy link
Member Author

dbluhm commented Sep 29, 2023

A couple of decisions to call out:

The method for generating identifiers for verification methods that I proposed follows the same method used in did:key. This seemed like a fair precedent and least likely to cause confusion for users and implementers of both methods. However, I do note that of the did:peer:2 implementations I'm aware of, no one has used this method yet :sweat_smile: This would require changes to libraries.

I added some clarifications on how multiple services are expressed with did:peer:2. This is done by appending more .Sey... encoded services as opposed to encoding an array into the base64 of a single service element.

I added clarification that services should include ids. But, for backwards compatibility, it makes sense for us to still permit them to be omitted. I added clarification that when multiple services without ids are present in the DID, they should have incrementing integers appended to them to ensure uniqueness.

I added clarification that common string abbreviation should recursively descend through the service object.

I added clarification around what verification method types should be used for a given multibase encoded key, specifically selecting the "2020" variants so we get Ed25519VerificationKey2020 and X25519KeyAgreementKey2020. It likely makes sense to expand the table mapping multicodec prefix to verification method type with additional key types.

I added clarification that when using these verification method types, additional contexts are required in the resolved document.

@TimoGlastra
Copy link

The method for generating identifiers for verification methods that I proposed follows the same method used in did:key. This seemed like a fair precedent and least likely to cause confusion for users and implementers of both methods. However, I do note that of the did:peer:2 implementations I'm aware of, no one has used this method yet 😅 This would require changes to libraries.

IN AFJ we use sort of the same method as did:key (but without the z prefix). I think i got this from the spec.

https://github.com/hyperledger/aries-framework-javascript/blob/296955b3a648416ac6b502da05a10001920af222/packages/core/src/modules/dids/methods/peer/peerDidNumAlgo2.ts#L86C27-L86C27

I added clarification around what verification method types should be used for a given multibase encoded key, specifically selecting the "2020" variants so we get Ed25519VerificationKey2020 and X25519KeyAgreementKey2020. It likely makes sense to expand the table mapping multicodec prefix to verification method type with additional key types

This doesn't really matter right? As long as i can resolve the Multibase key to a verification method and can use that, i could use 2018, 2020, or even JsonWebKey2020. You also have MultibaseKey2022 now I think.

I've seen a lot of did:key resolvers that do things slightly different, but they all succeed in extracting the needed the key material.

@FabioPinheiro
Copy link
Contributor

@TimoGlastra the problem is not extracting the key material. Is the name/ref/id to give the key.
Since you refer to the key id used to encrypt when sending a message.

I do agree with @swcurran to use a index base name. Do over complicated stuff.
I know that no one implemented like that but if I'm going to help change fix libraries I would prefer something simple

Ex:
did:peer:2.Ez6LSghwSE437wnDE1pt3X6hVDUQzSjsHzinpX3XFvMjRAm7y.Vz6Mkhh1e5CEYYq6JBUcTZ6Cp2ranCWRrv7Yax3Le4N59R6dd.SeyJ0IjoiZG0iLCJzIjoiaHR0cHM6Ly9hbGljZS5kaWQuZm1ncC5hcHAvIiwiciI6W10sImEiOlsiZGlkY29tbS92MiJdfQ
Ez6LSghwSE437wnDE1pt3X6hVDUQzSjsHzinpX3XFvMjRAm7y will be #key-1 whatever that be
Vz6Mkhh1e5CEYYq6JBUcTZ6Cp2ranCWRrv7Yax3Le4N59R6dd will be #key-2 whatever that be

@dbluhm
Copy link
Member Author

dbluhm commented Oct 3, 2023

I don't have strong feelings about the IDs; however, I agree that just indexing them based on order they appear in the DID seems like the simplest and least "arbitrary" scheme. @TimoGlastra what do you think of going with #key-N style?

This doesn't really matter right? As long as i can resolve the Multibase key to a verification method and can use that, i could use 2018, 2020, or even JsonWebKey2020. You also have MultibaseKey2022 now I think.

I think this is true. How your did:peer:2 resolver decides to decode the document influences internal handling more than it does anything external. I'm okay with relaxing these constraints. I believe the real critical piece is consistent identifiers for the resources in the document and being able to reliably handle references to those resources.

@TelegramSam
Copy link
Collaborator

I like this.

@swcurran
Copy link
Collaborator

Looks good to me. Really want to get to did:peer:4, so am a little worried about the comment above that the libraries that do did:peer:2 as documented here, but if others think that is reasonable, I won’t argue.

That said, the sooner we deprecate numalgos 0, 1, 2 and 3, the better.

@FabioPinheiro
Copy link
Contributor

FabioPinheiro commented Oct 18, 2023

I like that key for numalgos 2 gets define in the specs.
My only concern is that most implementations that I have seen is using the string that encode the key as the key id.
Exdid:peer:2.Ez<abc>.Vz<xyz> will have the key ids #<abc> and #<xyz>.

I would prefer index-based. For being more easy.
But this change will be a breaking change for probably ALL libraries. Also will be hard on the applications that depend on those (especially during the change, since not everyone going to upgrade applications at the same time).
So I am concerned how long will we take to adopt and make everyone interoperable again.

@dbluhm
Copy link
Member Author

dbluhm commented Oct 18, 2023

@FabioPinheiro literally every implementation I'm aware of has done it differently, unfortunately. peerdid-python uses characters 1-9 of the multibase/multicodec encoded key. AFJ is using multibase minus the z. Agents tested in the didcomm v2 mediator test suite use the full multibase, as you've described. The didcomm-demo follows peerdid-python (mostly since interacting with a mediator using that library ended up being first integration target).

So I think no matter how we shake it, there's going to need to be changes in one library or another. I'm not sure how best to address that besides continuing to communicate about this interop challenge (which, in my view, is still less problematic after these changes than it was before) and implementing the changes in well known locations like the didcomm v2 mediator test suite, didcomm demo, ACA-Py, AFJ, etc. If we hit those well known projects, I think we should eventually get a majority of the ecosystem by osmosis.

Still, better yet, if implementations don't want to mess around with fixing did:peer:2, we can encourage everyone to push to did:peer:4. I think this is also an acceptable outcome and would consider these clarifications to be more for posterity's sake rather than the real solution to the interop challenges presented by an ambiguous did:peer:2 spec.

@dbluhm
Copy link
Member Author

dbluhm commented Oct 18, 2023

This doesn't really matter right? As long as i can resolve the Multibase key to a verification method and can use that, i could use 2018, 2020, or even JsonWebKey2020. You also have MultibaseKey2022 now I think.

I've seen a lot of did:key resolvers that do things slightly different, but they all succeed in extracting the needed the key material.

@TimoGlastra I've added a note conceding that exactly how you choose to "resolve" the DID doesn't matter too much as long as it's only used internally and the id is consistent. I think that's the only critical piece of the puzzle here.

Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
Copy link
Member Author

dbluhm commented Oct 19, 2023

I've implemented a minimal did:peer:2 library in python demonstrating these fixes: https://github.com/dbluhm/did-peer-2

@dbluhm
Copy link
Member Author

dbluhm commented Oct 20, 2023

I took a deeper look at what is intended by publicKeyMultibase in the DID Core spec. As written, publicKeyMultibase is simply a multibase encoding of the public key material WITHOUT the multicodec prefix that we're used to seeing in did:key and the serialized representation of the key in did:peer:2. This point has been terribly confused in more than one location (including draft specs at W3C). In order to align with the DID Core spec, if we were to use verification method type Ed25519VerificationKey2020 and X25519VerificationKey2020, we would actually need to strip the multicodec prefix from the serialized key representation and re-encode the key with base58 and prepend with z. In other words, we should no longer see z6Mk on ed25519 keys and just see z followed by random characters.

Given this, I think I will adjust the spec further to represent verification methods using Multikey which will simplify things in two ways. First, there's no need to vary the context depending on the presence of keys of different types. And second, there's no need to transform the key from it's serialized representation to remove the multicodec prefix.

Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
dbluhm added a commit to dbluhm/aries-cloudagent-python that referenced this pull request Oct 24, 2023
Speaking frankly, I felt that the peerdid library was a bit overly
complicated. On top of that, peerdid is not up to date with the
corrections proposed in this PR to the did:peer method specification:

decentralized-identity/peer-did-method-spec#62

did-peer-2 is a minimal implementation of did:peer:2 and did:peer:3.
It is up to date with the above PR. The corrections to the spec and the
library lend themselves to a simpler implementation of the did:peer:2
and did:peer:3 resolvers. This refactor supports work to add did:peer:2
support to OOB + DID Exchange.

New in this PR is a cleanup mechanism for the did:peer:3 to did:peer:2
mapping records. When a connection is deleted, we check whether there
are any potential mappings to remove.

Signed-off-by: Daniel Bluhm <[email protected]>
Copy link
Collaborator

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

Awesome work, @dbluhm — thanks for getting this done.

Copy link
Contributor

@FabioPinheiro FabioPinheiro left a comment

Choose a reason for hiding this comment

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

LGTM

Just added some small notes that I think can still be improved


Oh I forgot to submit my review

spec/core.md Show resolved Hide resolved
spec/core.md Show resolved Hide resolved
@dbluhm dbluhm requested a review from TelegramSam as a code owner October 25, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants