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

Investigate, debate and add support for Peer DID 3 to ACA-Py #2249

Closed
swcurran opened this issue May 30, 2023 · 33 comments
Closed

Investigate, debate and add support for Peer DID 3 to ACA-Py #2249

swcurran opened this issue May 30, 2023 · 33 comments
Assignees

Comments

@swcurran
Copy link
Contributor

swcurran commented May 30, 2023

A follow up to issue #2156 — for high level details see the presentation and recording from today’s ACA-Pug meeting. Also relevant is the Peer DID Method Specification, where DID Peer 3 is defined.

In this ticket, we want to add support for did:peer:3 to ACA-Py such that when a DID initially created as a did:peer:2 is used for messaging, did:peer:3 is used in its place. I think (not certain) that only the peer of the other party would ever use this — I don’t think the creator of the DID ever references itself in a presentation.

The issue of two DIDs (the 2 and 3 forms) needs to be “handled” in the connection record and possibly the DID record. Perhaps “their_did” should always be the did:peer:3 whenever a DID Peer 2 is created? Or perhaps there needs to be the idea of a synonym DID so that multiple identifiers can be used for a single DID. That might be needed for the transition of unqualified DIDs to qualified ones, where we would like to use did:peer:3.

@Jsyro
Copy link
Contributor

Jsyro commented May 31, 2023

Existing work for Peer:did:1 to be repurposed for this new requirement.

#2174

@Jsyro
Copy link
Contributor

Jsyro commented Jun 8, 2023

Update: I have taken the branch related to #2174, resolved a few outstanding issues, and am now developing some unit tests to explore his existing work. Will be working to create tests that represent the spec for peer:did:2.

@Jsyro
Copy link
Contributor

Jsyro commented Jun 9, 2023

ok, been reading lots. Couple of question @swcurran

This describes the did exchange itself (how to serialize and encode the document). https://identity.foundation/peer-did-method-spec/#multi-key-creation
For each of those 'purposecodes' there is a corresponding b64encoded json payload, we are focused on 'Method 2' here

I'm trying to figure out what the spec actually says about those payloads?
Do we need to support all of those purpose codes right away?
How many of these json objects might be in used by aca-py already? (e.g. Do we need to define all of these classes, or just the way they are packaged/exchanged?

@swcurran
Copy link
Contributor Author

swcurran commented Jun 9, 2023

Regards the purposecodes. I think we need code that given a DIDDoc will encode it into a did:peer:2, and given a did:peer:2 will decode it into a DIDDoc. As such, for purity, all purpose codes should be supported. In practice with Aries, there will only be a V verification key, as where we are will be using this, the DIDDoc will contain only a single Verification key. Depending on what key types we will be using, an E encryption/key agreement key may also be needed.

I’d say start with the DIDDoc we have today for peer DIDs, support that in a relatively generic way. That will uncover some other questions — like how far down the multibase encoded, multicodec-encoded rabbit hole we should go. Is there a library that we could depend on to provide that? It would be worth looking at how we are handling did:key in ACA-Py (and perhaps AFJ).

For other than V and E, we could add an error that the purposecode is not supported — and others could add such support when we know it is needed.

Why V for sure and only “perhaps E”? An Ed25519 Verification key can be converted determininstically into an x22519 key agreement key. As such, the unqualified Peer DIDs we create generally only have a verification key. However, not all key types support such a transformation, and many in the Aries community do not like to use the same key for multiple purposes. In those cases, there will be both a V and an E key in a peer:did:2 to be encoded/decoded.

@dbluhm @andrewwhitehead @TimoGlastra @ashcherbakov — any comments on this? Any existing code we should be using?

@swcurran
Copy link
Contributor Author

swcurran commented Jun 9, 2023

@Jsyro — I was pointed to this repository within SICPA — https://github.com/sicpa-dlab/peer-did-python. You should definitely be looking at this. It will likely have most/all of what you need for peer:did:2 and likely we’re better off putting peer:did:3 into that repo, and using it in ACA-Py.

@ashcherbakov
Copy link

Agree, it makes sense to use https://github.com/sicpa-dlab/peer-did-python. It doesn't have peer:did:3 support, but it's not so hard to add it there.

@Jsyro
Copy link
Contributor

Jsyro commented Jun 15, 2023

I have been able to resolve did:peer:2's into the full did doc and generate the did from the document, with examples to work from.
This issue appears to be rooted in the examples on the spec website having possible issues (b64 padding and a mismatched encryption key).

@swcurran
Copy link
Contributor Author

The library should be able to deal with padding — which is always a pain.

Does the library also have did:peer:1 support? I notice that currently AFJ is using did:peer:1, so perhaps we do have to support that as well. Or we push on AFJ to move quickly to did:peer:2/3 support ASAP and skip the did:peer:1 stage.

@Jsyro
Copy link
Contributor

Jsyro commented Jun 16, 2023

It does not appear to have did:peer:1 support. @andrewwhitehead ?

@Jsyro
Copy link
Contributor

Jsyro commented Jun 16, 2023

Interesting Curveball, the spec actually provides a regex for identifying dids.... which allows for = signs.

https://identity.foundation/peer-did-method-spec/index.html#matching-regex

Should I open an issue on the spec? or in the sicpa library? or both? @swcurran @andrewwhitehead

@andrewwhitehead
Copy link
Contributor

Method 1 support is a draft PR: sicpa-dlab/peer-did-python#46

@swcurran
Copy link
Contributor Author

Fascinating….my $0.02CDN (which is worth exactly that) would be to try the regex in the spec in the implementation and see if the tests pass (and add some more…). If it works, I’d say go with it….

But don’t listen to me.

@Jsyro
Copy link
Contributor

Jsyro commented Jun 22, 2023

DID Spec conflict has been resolved here.

Focused on constructing did:peer:2 from existing connection information.

@dbluhm
Copy link
Contributor

dbluhm commented Jun 26, 2023

I'd like to get more eyes on this issue before we get too far on did:peer:3

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

@Jsyro
Copy link
Contributor

Jsyro commented Jun 29, 2023

Update: Working to integrate the peer-did-python library. https://github.com/sicpa-dlab/peer-did-python/

Would like to make all DID Documents extensions of the library DIDDocument class, with any modifications needed for each DIDMethod. However exisiting unqualified did's are not accepted by the library, so they need prefixed before being accepted. Unqualified DID's either need the prefix did:sov: if they are public dids, or did:peer:0 if they are part of the connection. The existing DIDDoc class only expects did:sov: dids, so flow changes need to be made to accomodate documents related to other did methods.

Few hurdles avenues to go from here.

a) I could to try and migrate existing working code to leverage the library now, adding development time and tinkering with more existing working code. The library does not accept unqualified dids, so that is a pain point.
b) I could just separate code flows for existing working code, and just detect did:peer:2 and only use the library there, however the payload validations using marshmallow will become quite messy
c) Not use the library classes and keep custom behaviour/classes that are more inline with existing work, but then would required maintenance in this repo in the future.

I originally tried b) until the marshmallow validation blocked me, now I'm working with a) and trying to add the appropriate prefix JIT for the library classes to be used. Looking for input or support on where to go from here. @shaangill025?

@swcurran
Copy link
Contributor Author

swcurran commented Jul 7, 2023

Sorry for the delay in replying to this. We talked about this yesterday, but adding here for completeness and to make sure we are in sync.

The path the Aries community is planning to go for existing unqualified DIDs is to update them as follows:

  • take their DIDDoc
  • deterministically define a peer:did:2 version of the DIDDoc
    • “Deterministically": Both agents with the DIDDoc (the agent that create the DIDDoc, and the agent that received the DIDDoc) MUST create the same did:peer:2 from the given DIDDoc.
  • determine a peer:did:3 from the peer:did:3
  • From then on, recognize the DID using it’s peer:did:3 or unqualified identifier.

We will have a “community coordinated update” which will formalize the process and timing for the upgrade, but basically:

  • Release and deploy code that handles the receipt of unqualified and peer:did:2/3 qualified DIDs, and continues to create and transmit unqualified DIDs
  • Release and deploy code that handles the receipt of unqualified and peer:did:2/3 qualified DIDs, migrates existing unqualified DIDs to peer:did:2/3, and creates and transmits did:peer:2/3 DIDs.
  • Release and deploy code that eliminates the handling of unqualified DIDs.

@swcurran
Copy link
Contributor Author

swcurran commented Jul 7, 2023

As discussed as well, it appears it is the ACA-Py is making assumptions about the precise structure of DIDDocs that is not generalized to match the spec. Notably, what ACA-Py assumes is a verkey may in fact be a reference to a verkey that needs to be dereferenced. Confirmed with @dbluhm that the dereferencing step is not being done.

@Jsyro
Copy link
Contributor

Jsyro commented Jul 7, 2023

Update: my immediate goal is to convert the connections protocol to create and recieve did:peer:2 and utilize the peer-did-python library. After some more context from Stephen shared above, i've made progress, but have found myself in catch 22.

If I create a did/did_doc in the way defined by the example, the DIDDoc and DID are created, and sent as an aries message, however when the receiver resolves the did, it resolves the service entry to an UnknownService object instead of a DIDCommService object which causes errors down the line. I have set the type string in the service creation to DIDCommMessaging like the example, and did-communication (the literal used inside the class itself), neither has given the desired effect

I can construct the DIDDoc using the class constructors and add an instance of DIDCommService which is serialized and deserialized correctly, but the constructor requires a did, a did for a document that has not been created yet...

@swcurran
Copy link
Contributor Author

swcurran commented Jul 7, 2023

That’s a fun one! What constructor are you calling that requires a DID and DIDDoc and what is it you are constructing? Part of ACA-Py or one of the libraries you are using?

@dbluhm
Copy link
Contributor

dbluhm commented Jul 8, 2023

@Jsyro What branch are you working from? Some of the challenges you're experiencing may stem from the problem solved by this PR (that really ought to get merged...): Indicio-tech/pydid#57

@dbluhm
Copy link
Contributor

dbluhm commented Jul 8, 2023

I've published a pre-release of pydid containing changes for supporting DIDCommMessaging as a service type; @Jsyro can you try out https://pypi.org/project/pydid/0.3.9a0/ and see if that helps resolve the UnknownService issue?

@hiteshgh
Copy link

will carry fwd ( partially)

@Jsyro
Copy link
Contributor

Jsyro commented Jul 10, 2023

Update for ACA-pug, @dbluhm 's merge of this PR resolve the serialization deserialization of the DIDDocument services.

The active task is to change how acapy extracts ConnectionTargets from the DIDDocuments, to resolve the recipient_key reference instead of copying the string and assuming it is the verkey itself.

After successfully getting a connection established using did's with peer:did:2, I'll be working on adding code flow to support both existing unqualified dids, and did:peer:2s.

Other items on the path to closing this issue are:

  • Migrate my changes out of the connections protocol and into the DIDExchange protocol, I incorrectly made the changes in the connection manager.py file.
  • Allowing connections to be established with both unqualified did's and did:peer:2
  • Open Draft PR for community discussion.
  • Code to convert/promote existing connections with unqualified did's to use did:peer:2
  • Optionally use did:peer:3 when possible
  • Tests

@Jsyro
Copy link
Contributor

Jsyro commented Jul 19, 2023

Update:

Discovered the magic payload for the peer-did-python library to properly type the service classes. sicpa-dlab/peer-did-python#60

Migrated the required changes to the DIDExchange manager file and can create an OOB message, recieve an OOB message, and Establish a connection where both sides are using did:peer:2.

@Jsyro
Copy link
Contributor

Jsyro commented Jul 25, 2023

Update:

I have two agents that can send or receive the 'old' dids and DIDDocuments and consume them. Additionally there is a flag that allows for the sending of did:peer:2.

These changes are prepared to consume an DIDExchange that uses a 'did:peer:2'

@Jsyro
Copy link
Contributor

Jsyro commented Aug 8, 2023

Update (I'm away until the 15th):

The DIDDoc class is only used for connections, but is being tested like it was built for multiple use cases. This means most of the tests are invalid and not representative of real situations.

Connection did_docs (that use the RECORD_TYPE_DID_DOC='did_doc' storage type) will now be created using the peerdid library, which involves creating a new method that accepts the verkey and a service object, calling create_peer_did_numalgo_2 and resolve_peer_did.

When loading a did_document from storage, if an exception is thrown, attempt a custom method that will assume the json loaded was from an obsolete "DIDDoc" object, extract the key details (ED25519 Key and Service), create a did:peer:2 and resolve_peer_did to create a new DIDDocument object, and updating the record in storage to the recovered value.

In summary, there are three situations:

  • Existing unqualified connections with unqualified dids and DIDDoc objects, There is no requirement that acapy preserve the exact did document that was originally received when the connection was established, as long as it is equivalent (ED25519 keys and service) it can be updated to use the pydid.DIDDocument class.
  • Accepting and sending did:peer:2, processing the expected did:peer:3 and resolving them to DIDDocument objects for storage. did:peer:2 will only be seen on new connections after the code is merged and the appropriate flag is set.
  • Accepting and sending did:peer:3, acapy will need to preprocess these for existing connections when upgraded by loading the DIDDocument object from storage, and reading the id field and converting that did:peer:2 to a did:peer:3. If that object in storage cannot be loaded to a DIDDocument, the exception will be handled, but the 'fetch_did_document' will return a DIDDocument() instance regardless.

@swcurran
Copy link
Contributor Author

Some other notes/questions after more discussions.

An open question @Jsyro and I discussed is about when a DID (the identifier) is used after a connection is created. It turns out that in DIDComm v1.0, the DID seems not to ever be used. All messages are sent, accompanied by the base58 Ed25519 public key, and that key is indexed by ACA-Py to allow finding the connection to which it is associated. In DIDComm v2.0, this changes (AFAIK), as the message can be accompanied by the key or a reference to the key — such as a <DID>#<keyref>. Thus, the conversion of existing unqualified peer DIDs, and, in fact, the use of did:peer:3s are not really relevant until DIDComm v2.0 when other than the “raw” public key may be used in sending DIDComm messages. AFAIK, the DID does get stored in a list of DIDs, that is (presumably) searchable by DID.

I think we will need to resolve DIDs in the following ways for peer DIDs:

  • For unqualified DIDs, generate a peer:did:3 DID for the DIDDoc, and make the DID resolvable by both the unqualified and peer:did:3 identifiers.
  • For arriving peer:did:2, convert the DID to a peer: did:3 and make it searchable by just the peer:did:3.
    • If we ever need to resolve a peer:did:2, convert it to its peer:did:3 form before searching.
    • Or should we also store and make searchable the did:peer:2 form?

Other questions that came up. @dbluhm — let’s plan to talk with @Jsyro on Tuesday about these.

  • Does your recent resolver update (feat: add legacy peer did resolver #2404) impact what Jason is doing?
  • In the connection structure, should the form of the DID should be converted from unqualified to peer:did:3 form?
  • I think existing DIDDocs should be converted from the current form to the resolved peer:did:2 form. E.g. use the final version of the Timo Algorithm, generate a did:peer:2 and then generate the DIDDoc from the did:peer:2 form. That algorithm is to be moved into the community update.
    • Should this be done automatically or on use as described in Jason’s comment above?
    • Should we NOT store the resolved DIDDoc, but generate it dynamically from the did:peer:2 identifier so that if the generator process changes over time, the updates are picked up?
    • Should new keys found in the authentication item be added to the did:peer:2 string as part of Timo’s Algorithm?
  • Should the resolved version of a did:peer:2 contain the did:peer:3 identifier so that it is consistent in name between the DID and the ID in the DIDDoc? E.g., should Example 4 in this section of the DID Peer Specification change such that the ID is the did:peer:3 form?

@Jsyro — re: questions about non-top level id and controller entries in the existing DIDDoc test cases in ACA-Py. I think the rule we follow in converting DIDDoc is:

  • Follow Timo’s Algorithm to extract the keys/key types, and the service endpoints as they exist.
  • Generate a did:peer:2 string
  • Resolve the did:peer:2 string to a new DIDDoc following the did:peer:2 to DIDDoc algorithm.

In other words — anything non-standard in the ACA-Py test DIDDocs will be eliminated.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 15, 2023

Putting some thoughts down in advance of our discussion.

  • feat: add legacy peer did resolver #2404 itself does not directly impact this work but feat: resolve connection targets and permit connecting via public DID #2409 does.
  • feat: resolve connection targets and permit connecting via public DID #2409 is very close to all the changes required to support receiving a peer DID in a DID Exchange request or response. I think the only thing that would need to be added is a peer DID resolver and perhaps a small tweak to the language of logs to say that's it's just "resolving the DID" instead of "resolving a public DID."
  • What feat: resolve connection targets and permit connecting via public DID #2409 doesn't include is creation of Peer DIDs or storing Peer DIDs. Creation will of course cover sending out peer DIDs ourselves. Storing peer DIDs will facilitate resolving a did:peer:2 from a did:peer:3.
  • I think we should update all peer:did:2 resolvers to include an alsoKnownAs field in the resolved document. This should be populated with the did:peer:3, as calculated according to the rules defined in the spec.
  • I think the did:peer:3 resolver should work by looking up a mapping to a did:peer:2 and then resolving the did:peer:2 to a document but then inserting the did:peer:3 value for all IDs and controller values instead of the did:peer:2. The resolved did:peer:3 document should, in my opinion, also contain an alsoKnownAs field with the did:peer:2 that it is derived from.
  • DIDs stored in connection records should be qualified.

@swcurran
Copy link
Contributor Author

I was thinking the reverse — that on receipt, the did:peer:2 should be resolved, the DIDDoc stored and referenced/indexed/tagged via its did:peer:3 name. The did:peer:2 is thus only used as a vehicle for transporting the DIDDoc in a concise way. Receipt of a peer:did:2 in any situation other than in establishing a connection is handled by converting it to its did:peer:3 name and resolving it. In other words, treating the did:peer:3 form as the ID of the DID.

In the DIDDoc, the id would be the did:peer:3 form, and there would be no need for storing the did:peer:2 form at all. The alsoKnownAs item could be inserted into the DIDDoc, but it’s not really needed.

I think this avoids the messiness of AlsoKnownAs and the need for dual names for a DID. I think this might need a change to the spec to set the did:peer:3 string as the id in the resolved DIDDoc.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 15, 2023

Interesting. I think my view derives from the idea that resolving a document where the ID doesn't match the DID being resolved is a problem.

@swcurran
Copy link
Contributor Author

That’s where I started. But the more I thought about it, the more I thought that treating the DID as primarily a did:peer:3 is easier. The fact that it arrives as a did:peer:2 is a detail. It might have been better to have did:peer:2 defined as a did:peer:3 DID URL:

did:peer:3.<identifier for 3>?arg=<identifier for 2>, on initial use, and in subsequent uses, the arg is dropped. That way the did identifier is always the short form.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 15, 2023

Is it too late to pursue that direction? 😅 I think something like that would require relaxing some constraints in a few places. The DID Resolver expects to strictly resolve a DID for instance and dereferencing is defined to result in returning a portion of a document.

I see what you mean about the existence of the did:peer:2 as being incidental. I don't feel strongly about whether the did:peer:2 itself sticks around. I think I am more strongly opinionated about what resolving a did:peer:2 should look like, though. Having it resolve to a document containing did:peer:3 might be a shortcut to our goals but it feels like a kludge.

The DID Core spec says this about the alsoKnownAs property:

A DID subject can have multiple identifiers for different purposes, or at different times. The assertion that two or more DIDs (or other types of URI) refer to the same DID subject can be made using the alsoKnownAs property.

And then in the note below the section:

Applications might choose to consider two identifiers related by alsoKnownAs to be equivalent if the alsoKnownAs relationship is reciprocated in the reverse direction. It is best practice not to consider them equivalent in the absence of this inverse relationship.

This feels more or less exactly like the relationship that exists between did:peer:2 and did:peer:3. I'm much more comfortable using mechanisms defined in the DID spec rather than coming up with our own conventions. However, I acknowledge that even following the alsoKnownAs, some of this will just have to be convention/defined by the did:peer spec to describe expectations for resolving the did:peer:3.

@Jsyro
Copy link
Contributor

Jsyro commented Aug 24, 2023

update:

peer_did_python.dids.resolve_peer_did decodes the components of the did:peer:2 as described, however the service entry has a recipient_key that is a DIDUrl that refers to one of the verificationmethods of the resolved diddoc, however the did:peer:2 encoding does not encode an id field. meaning that the recipient key needs to re-referenced after did resolution based on the key type, looking for a Ed25519VerificationKey2020.

A workaround that could be handled by the peer-did-python library, and potentially an issue with the spec. as raw verkey's in the recipient_key field is not recommended in DIDCommV1Service's, and DIDCommV2Service's don't use it.

@Jsyro Jsyro assigned swcurran and unassigned Jsyro Dec 8, 2023
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

No branches or pull requests

6 participants