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

feat: did:peer:2 and did:peer:4 support in DID Exchange #1550

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Aug 18, 2023

Add support to did:peer:2 and did:peer:4 in DID Exchange protocol, in order to start aligning with RFC 0793.

This includes the recent changes in signatures for DID Exchange 1.1, as specified in RFC 0023.

Currently, AFJ uses qualified DIDs for DID Exchange protocol, in did:peer:1 format. The community is going towards did:peer:2 and the new did:peer:4 (more meaningful in DIDComm v2 world). So in this PR we are trying to do the transition from did:peer:1 to did:peer:4 in order to be compatible with ACA-Py when it adds support for the reception of qualified DIDs in DID Exchange protocol.

The logic is the following:

  • There is a new setting in ConnectionsModuleConfig to choose the default DID Peer num algo to use in DID Exchange Requests. If not defined, did:peer:1 will be used (this is current behaviour)
  • There is a new optional setting in both OOB Reception and Invitation acceptance methods to override default setting. The use case would be: we know our mediator supports only did:peer:4 (ACA-Py) so we connect with it using that algo, while still using did:peer:1 for other agents (AFJ, AFGo)
  • In DID Exchange Responses, we use the num algo the requester has used. So if they used did:peer:2, we'll respond with a did:peer:2 as well, regardless of our default setting

In addition to this, we add here a new setting called ourDid to specify a did when receiving an invitation. This can be used with peer dids but also public dids (as long as their private keys are held by the agent).

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (a641a96) 62.69% compared to head (6313c0a) 54.68%.

Files Patch % Lines
...ore/src/modules/connections/DidExchangeProtocol.ts 70.27% 11 Missing ⚠️
...src/modules/connections/ConnectionsModuleConfig.ts 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1550      +/-   ##
==========================================
- Coverage   62.69%   54.68%   -8.01%     
==========================================
  Files         779      711      -68     
  Lines       18128    15902    -2226     
  Branches     3128     2757     -371     
==========================================
- Hits        11365     8696    -2669     
- Misses       6219     6596     +377     
- Partials      544      610      +66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@swcurran
Copy link
Contributor

@Jsyro @dbluhm — FYI.

@Patrik-Stas — is Aries VCX moving to DID peer 2 support?

It would be really good to get some AATH tests that enable starting the Aries components to initiate connections with did:peer:2 and ensure interop. We’ll see about what that would take.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

I'm not too big a fan of adding the peer did numalgo as an option everywhere, as it will clash when we add support for public dids in didexchange.

Do you think we could set a default numalgo, and if you don't want to use the default numalgo, you can also just provide a did to the method. This means it has the same interface whether you're using peer dids or not

@TimoGlastra
Copy link
Contributor

Left some thoughts on the signed attachments in did exchange: hyperledger/aries-rfcs#717 (comment)

@genaris
Copy link
Contributor Author

genaris commented Aug 18, 2023

I'm not too big a fan of adding the peer did numalgo as an option everywhere, as it will clash when we add support for public dids in didexchange.

Do you think we could set a default numalgo, and if you don't want to use the default numalgo, you can also just provide a did to the method. This means it has the same interface whether you're using peer dids or not

So you mean to add an optional ourDid parameter to both receiveInvitation and acceptInvitation in a way that we'll attempt to resolve it if defined, or use default numAlgo to generate a peer did otherwise? i.e. in DidExchangeProtocol.createRequest():

const didDocument = ourDid
      ? await didsApi.resolveDidDocument(ourDid)
      : await this.createPeerDidDoc(
          agentContext,
          this.routingToServices(routing),
          config.peerNumAlgoForDidExchangeRequests
        )

In the use-case where we want to use did:peer:2 to connect to our mediator while we use did:peer:1 by default, we would need to do something like:

    const result = await agent.dids.create({
      method: 'peer',
      didDocument,
      options: {
        numAlgo: PeerDidNumAlgo.MultipleInceptionKeyWithoutDoc,
      },
    })
    await agent.oob.receiveInvitationFromUrl('invitationUrl', {
       ...
      ourDid: result.didState.did,
    })

That seems convenient and it will be definitely a new step towards public DID support in DID Exchange Requests/Responses.

@genaris genaris force-pushed the feat/did-peer-3 branch 2 times, most recently from a8c01df to eea78f2 Compare September 22, 2023 20:18
@genaris genaris force-pushed the feat/did-peer-3 branch 2 times, most recently from 4d088b0 to 92011e1 Compare December 5, 2023 00:01
@genaris genaris marked this pull request as ready for review December 7, 2023 14:07
Signed-off-by: Ariel Gentile <[email protected]>
@genaris genaris changed the title feat: did:peer:2 support in DID Exchange feat: did:peer:2 and did:peer:4 support in DID Exchange Dec 11, 2023
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

One security concern about alsoKnownAs

role: DidDocumentRole.Received,
didDocument,
tags: {
// We need to save the recipientKeys, so we can find the associated did
// of a key when we receive a message from another connection.
recipientKeyFingerprints: didDocument.recipientKeys.map((key) => key.fingerprint),
alsoKnownAs: didDocument.alsoKnownAs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have any security implications? I could include a did here i don't control and if the also known as was used for querying i could use another did to make it look like i also control the other did.

There should be some sort of verification between these I think if we want to use it for this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to think a bit more about that, because it should not be a problem when dealing with peer dids (as it is our agent who will be generating the didDoc based on the spec, and the alsoKnownAs is only populated where applicable) but it might be an issue in case we get a DID Exchange Request using a public/custom did.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the context of did:peer helps answer this question as @genaris calls out but just to add color in the general case, if the DID identified in the alsoKnownAs list is resolved and the original DID is found in that DIDs alsoKnownAs list, this gives us certainty that the owner of this DID controls the other DID.

For better worded color lol, consider this note from the DID Core spec on AKA:

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. In other words, the presence of an alsoKnownAs assertion does not prove that this assertion is true. Therefore, it is strongly advised that a requesting party obtain independent verification of an alsoKnownAs assertion.

Given that the DID subject might use different identifiers for different purposes, an expectation of strong equivalence between the two identifiers, or merging the information of the two corresponding DID documents, is not necessarily appropriate, even with a reciprocal relationship.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for popping in here @dbluhm!

So we need to make sure we've verified the relation of alsoKnownAs both ways before we allow it to be queried.

Maybe we can rename it to verifiedAlsoKnownAs or something different like relatedDids you mentioned ariel.

packages/core/src/modules/dids/repository/DidRecord.ts Outdated Show resolved Hide resolved
@genaris genaris requested a review from a team as a code owner December 12, 2023 01:16
Signed-off-by: Ariel Gentile <[email protected]>
@TimoGlastra
Copy link
Contributor

Is this ready to be merged @genaris, or are there some fixes related to alsoKnownAs that need to be addressed first?

@genaris
Copy link
Contributor Author

genaris commented Dec 14, 2023

Is this ready to be merged @genaris, or are there some fixes related to alsoKnownAs that need to be addressed first?

From my side I think it is fine to be merged. For this PR I took a more conservative approach for the alternativeDidsso it will only consider peer dids. Later on, if we need to do use alsoKnownAs for public DIDs we can do some updates to do the reciprocal relationship check that @dbluhm mentioned.

@TimoGlastra TimoGlastra merged commit edf493d into openwallet-foundation:main Dec 14, 2023
7 checks passed
@genaris genaris mentioned this pull request Jan 28, 2024
genaris added a commit to genaris/credo-ts that referenced this pull request Jan 29, 2024
genaris added a commit to genaris/credo-ts that referenced this pull request Jan 29, 2024
Signed-off-by: Ariel Gentile <[email protected]>

feat: deliver messages individually, not fetching from the queue every time

Signed-off-by: Ariel Gentile <[email protected]>

chore: revert to free runners (openwallet-foundation#1662)

Signed-off-by: Ry Jones <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

chore: create settings.yml (openwallet-foundation#1663)

Signed-off-by: Ry Jones <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

chore: fix ci and add note to readme (openwallet-foundation#1669)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

docs: update active maintainers (openwallet-foundation#1664)

Signed-off-by: Karim Stekelenburg <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

feat: did:peer:2 and did:peer:4 support in DID Exchange (openwallet-foundation#1550)

Signed-off-by: Ariel Gentile <[email protected]>

feat(presentation-exchange): added PresentationExchangeService (openwallet-foundation#1672)

Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

chore: removed jan as maintainer (openwallet-foundation#1678)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

ci: change secret (openwallet-foundation#1679)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

chore: add meta to rxjs timeout errors (openwallet-foundation#1683)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

build(deps): bump ws and @types/ws (openwallet-foundation#1686)

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

build(deps): bump follow-redirects from 1.15.2 to 1.15.4 (openwallet-foundation#1694)

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

chore: update shared components libraries (openwallet-foundation#1691)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

fix: properly print key class (openwallet-foundation#1684)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

feat(present-proof): add support for aries RFC 510 (openwallet-foundation#1676)

Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

fix(present-proof): isolated tests (openwallet-foundation#1696)

Signed-off-by: Ariel Gentile <[email protected]>

feat(indy-vdr): register revocation registry definitions and status list (openwallet-foundation#1693)

Signed-off-by: Ariel Gentile <[email protected]>

chore: rename to credo-ts (openwallet-foundation#1703)

Signed-off-by: Ry Jones <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

ci: fix git checkout path and update setup-node (openwallet-foundation#1709)

Signed-off-by: Ariel Gentile <[email protected]>

fix: remove check for DifPresentationExchangeService dependency (openwallet-foundation#1702)

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

docs: update zoom meeting link (openwallet-foundation#1706)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

refactor(oob)!: make label optional (openwallet-foundation#1680)

Signed-off-by: Timo Glastra <[email protected]>
Co-authored-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

feat: support short legacy connectionless invitations (openwallet-foundation#1705)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

feat(dids)!: did caching (openwallet-foundation#1710)

feat: did caching

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

fix: jsonld document loader node 18 (openwallet-foundation#1454)

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

build(deps): bump amannn/action-semantic-pull-request from 5.3.0 to 5.4.0 (openwallet-foundation#1656)

build(deps): bump amannn/action-semantic-pull-request

Bumps [amannn/action-semantic-pull-request](https://github.com/amannn/action-semantic-pull-request) from 5.3.0 to 5.4.0.
- [Release notes](https://github.com/amannn/action-semantic-pull-request/releases)
- [Changelog](https://github.com/amannn/action-semantic-pull-request/blob/main/CHANGELOG.md)
- [Commits](amannn/action-semantic-pull-request@v5.3.0...v5.4.0)

---
updated-dependencies:
- dependency-name: amannn/action-semantic-pull-request
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timo Glastra <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>

feat: did rotate (openwallet-foundation#1699)

Signed-off-by: Ariel Gentile <[email protected]>

refactor: pickup protocol method names

Signed-off-by: Ariel Gentile <[email protected]>
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