Skip to content

Conversation

Artemkaaas
Copy link
Contributor

@Artemkaaas Artemkaaas commented Nov 10, 2022

This PR introduces the initial implementation supporting DIDComm V2 messaging.
There are several fixme's in the code for resolving which I need recommendations.
The PR is huge( I could not find a way how to split it into parts.

What is included in this PR:

  • AgentMessage moved to DIDCommV1 message and also added DIDCommV2 message structure.
  • EnvelopService provides encryption for both message types V1/V2.
  • Integrated Sicpa DIDComm library to provide DIDComm V2 message encryption.
  • Updated MessageSender and MessageReceiver to support both types of messages.
  • Renamed signing-provider to key-provider and added X25519 key provider as it is needed for DIDComm V2 messaging.
  • Added separate temp function for creating DID's in the format and steps suitable for DIDComm V2 messaging.
  • Added DIDComm V2 version of Trust Ping protocol. Right now DIDComm V2 does not require the creation of Connection state object (there is not DID Exchange protocol adoption). Messages are sent just by passing DIDs of the sender and receiver.
  • Update Alice/Faber demo to create DIDComm V2 DIDs and send Ping.

Next steps:

  • Mediator provisioning and routing support
  • DID exchange adoption and making Connection state object??
  • Other protocols adoption?

Document reflecting the current state of the PR

@Artemkaaas Artemkaaas requested a review from a team November 10, 2022 11:07
@Artemkaaas Artemkaaas force-pushed the didcommv2-contribution branch from c3616f6 to 6197fe0 Compare November 10, 2022 11:21
@TimoGlastra
Copy link
Contributor

Wow that's a big PR! Thanks for the effort here. I've stated with the PR review and have reviewed 100 of the total 240 files for now. Will continue later.

I see you made the PR against the 0.3.0-pre branch, but we've recently merged that into main, can you point your PR towards the main branch instead?

@berendsliedrecht
Copy link
Contributor

Woah! That is a nice PR to receive, great work @Artemkaaas and the entire team behind it.

I do have one concern and I am not sure how the other maintainers feel about this. We want to make a very modular core. I believe the idea was to start with a DIDComm v1 or v2 implementation and the connected RFCs (discover features, OOB, etc.) and leave everything else outside of the core (like what we did with the bbs stuff, multitenancy, etc.). If we merge this PR it will likely be a lot more complex to strip it out of core again and into a separate package that can be added to the Agent as a module / protocol / whatever.

So my main question I guess of the the other maintainers, what would we like to be inside of the core for now? Would it be based on DIDComm v1 or v2? If DIDComm V2 will be in the core, how will we handle the underlying DIDComm dependency as right now I believe you used the WASM dependency right in core.

Can elaborate further on this if required during a WG call.

Copy link
Contributor

@2byrds 2byrds left a comment

Choose a reason for hiding this comment

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

Incredible work! I couldn't be happier to see this. SICPA DIDComm contributions have been much appreciated.
I think when this was originally developed, AFJ 0.3.0 and the move towards Indy-pendence and modularization hadn't gained momentum. In that context the formulation of this PR would be spot on.
In the post 0.3.0 AFJ context, this PR prompts important discussion points. There are opportunities to reduce overlap between messaging, crypto, etc. Can we discuss that together in an upcoming AFJ meeting? I admit I'll be out this coming Thursday for IIW and then again the next Thursday for Thanksgiving. But I think @genaris , @TimoGlastra , @blu3beri, @NB-MikeRichardson, @karimStekelenburg, etc. would agree that this would be a great opportunity to continue the momentum to modularize AFJ and contributions like these, in a way that makes both easier to fit, maintain, upgrade, and provide support for multiple platforms and use cases.

@Artemkaaas Artemkaaas changed the title Added support for DIDComm V2 messaging feat: Added support for DIDComm V2 messaging Nov 11, 2022
@Artemkaaas Artemkaaas force-pushed the didcommv2-contribution branch from 6197fe0 to f7839e9 Compare November 11, 2022 09:15
@Artemkaaas Artemkaaas changed the base branch from 0.3.0-pre to main November 11, 2022 09:21
@Artemkaaas Artemkaaas force-pushed the didcommv2-contribution branch from f7839e9 to b58bbc2 Compare November 11, 2022 09:29
@genaris
Copy link
Contributor

genaris commented Nov 11, 2022

Woah! That is a nice PR to receive, great work @Artemkaaas and the entire team behind it.

I do have one concern and I am not sure how the other maintainers feel about this. We want to make a very modular core. I believe the idea was to start with a DIDComm v1 or v2 implementation and the connected RFCs (discover features, OOB, etc.) and leave everything else outside of the core (like what we did with the bbs stuff, multitenancy, etc.). If we merge this PR it will likely be a lot more complex to strip it out of core again and into a separate package that can be added to the Agent as a module / protocol / whatever.

So my main question I guess of the the other maintainers, what would we like to be inside of the core for now? Would it be based on DIDComm v1 or v2? If DIDComm V2 will be in the core, how will we handle the underlying DIDComm dependency as right now I believe you used the WASM dependency right in core.

Can elaborate further on this if required during a WG call.

Expanding a bit what we've discussed in yesterday's WG call, this is definitely an important contribution and we want to make it part of the codebase, although we'll need to figure out the best way to do so because of the reasons you are saying.

The main problems we find at the moment are the dependency of a native library on the core (SICPA's didcomm in this case) and the need of extracting private keys from the wallet, which is something not desirable from security perspective. Apart from providing means to create/store keys and records securely, the Wallet should provide methods for using crypto material stored there. At the moment, it provides the pack and unpack method, which receives some input data referencing stored keys and returns some output, but private keys never leave the Wallet. This concept can become important in the future because the "Wallet" could be implemented as an HSM or Secure Element/Enclave or even remotely. In such cases it would be either impossible or insecure to retrieve secrets directly.

The issue about dependency could be solved if we define an interface for EnvelopeService (like we did for Proofs and Credentials modules) where different implementations could be dynamically loaded, including in this case the DIDCommV2 one provided by SICPA native library.

About the 'keys never leave the wallet' situation, it will be a bit more tricky because my understanding is that SICPA didcomm-rust uses the SecretResolver interface that is intended to actually retrieve key material, so in one way or another it should need to get and store the keys on the Wallet backend. But an elegant way to keep the keys retrieval and storage private in Wallet interface would be to register these EnvelopeServices into the Wallet so externally it would be transparent (we just ask the wallet to pack a message for "V1" or "V2" or whatever version, and it does the magic and returns the encrypted message... no matter if it can do it by its own means or it needs to ask to SICPA didcomm or @blu3beri-didcomm or any other library).

I'm not completely sure how this model would fit in current architecture but I'll think on some alternatives while reviewing and playing a bit on this such interesting PR!

@berendsliedrecht
Copy link
Contributor

berendsliedrecht commented Nov 11, 2022

@genaris Yeah you are making some good points.

Regarding the keys and keeping it inside the wallet, I think it should not be so complex as we make it out to be. I do not think that moving keys from the underlying wallet to a higher is so a big risk. When we have the keys in the lower level (without a Secure Enclave) we have a risk of another process reading the memory of the device. So whether the keys leave the wallet I would not see as a big increase in risk as they already are in memory, if that makes sense.

Now, HSM / Secure Enclave is very interesting and 100000% something we should be looking at. However, I think we can just create a wallet implementation when we want to integrate this and leaving it for later.

Also PS, since I have seen some interest in my, unfinished, didcomm-ts library, I have open sourced it so people can see my architecture in mind. It is heavily based on the rust implementation from sicpa, but with a crypto provider.

link: https://github.com/blu3beri/didcomm-ts

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2022

Codecov Report

Merging #1096 (a6ea054) into main (1c6b880) will decrease coverage by 1.46%.
The diff coverage is 69.02%.

@@            Coverage Diff             @@
##             main    #1096      +/-   ##
==========================================
- Coverage   88.82%   87.36%   -1.47%     
==========================================
  Files         707      751      +44     
  Lines       16593    17373     +780     
  Branches     2804     2977     +173     
==========================================
+ Hits        14739    15178     +439     
- Misses       1744     2056     +312     
- Partials      110      139      +29     
Impacted Files Coverage Δ
packages/core/src/agent/TransportService.ts 100.00% <ø> (ø)
packages/core/src/crypto/Key.ts 86.20% <0.00%> (-13.80%) ⬇️
...s/connections/handlers/ConnectionRequestHandler.ts 86.66% <ø> (ø)
.../connections/handlers/ConnectionResponseHandler.ts 83.33% <ø> (ø)
...connections/handlers/DidExchangeCompleteHandler.ts 84.61% <ø> (ø)
.../connections/handlers/DidExchangeRequestHandler.ts 80.00% <ø> (ø)
...connections/handlers/DidExchangeResponseHandler.ts 81.81% <ø> (ø)
...ges/core/src/modules/connections/handlers/index.ts 100.00% <ø> (ø)
...ges/core/src/modules/connections/messages/index.ts 100.00% <ø> (ø)
...ing/v1/handlers/TrustPingResponseMessageHandler.ts 71.42% <ø> (ø)
... and 191 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Artemkaaas Artemkaaas force-pushed the didcommv2-contribution branch 2 times, most recently from 9c565ad to 85109e2 Compare November 14, 2022 07:38
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.

Overall the PR looks really good and I don't think we're far away from merging. I've left a bunch of comments that nail down to the following topics:

  • Consistency with existing AFJ codebase (property names, casing, etc...)
  • Extending/modularizing existing APIs instead of adding new methods (with e.g. a V2 suffix)
  • Not adding the sicpa didcomm library to the core package
  • Splitting the didcomm v1 and v2 library as much as possible

I've also added a lot of questions for things that were unclear to me.

Anyway thanks a lot for this huge contribution!

@@ -0,0 +1 @@
@sicpa-dlab:registry=https://npm.pkg.github.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem resolved. Can we remove this file?

@@ -39,7 +39,7 @@ interface TransportSessionTable {
export interface TransportSession {
id: string
type: string
keys?: EnvelopeKeys
keys?: PackMessageParams
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still keys now? For didcomm v2 it will be the dids right?
Also I'm not sure if this type is the right type to use for the transport keys. This is also used before packing which means it doesn't include the exact key, which it does for v1. I think we should come up with a type that work for both v1 and v2 and don't try to reuse when not applicable

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it can be confusing. But the cause might be the whole design of the Didcomm v1 vs v2 solution 🤔

@TimoGlastra
Copy link
Contributor

When we have the keys in the lower level (without a Secure Enclave) we have a risk of another process reading the memory of the device. So whether the keys leave the wallet I would not see as a big increase in risk as they already are in memory, if that makes sense.

Now, HSM / Secure Enclave is very interesting and 100000% something we should be looking at. However, I think we can just create a wallet implementation when we want to integrate this and leaving it for later.

Problem is that if we add an API to get keys out of the wallet now, people will start using it and it will become MUCH harder to add HSM / secure enclave support later on.

@berendsliedrecht
Copy link
Contributor

Problem is that if we add an API to get keys out of the wallet now, people will start using it and it will become MUCH harder to add HSM / secure enclave support later on.

I don't think that we should provide an API that will get the keys out. The wallet implementation which exposes some sign(msg) method which could also take a publicKey and it will check if it has the associated privateKey with that publicKey.

This was just the first thing that popped up in my head, but I am sure we can provide all the functionality, without the user of the framework / developers of other modules, to sign/verify/encrypt/etc without exposing keys.

@genaris
Copy link
Contributor

genaris commented Nov 16, 2022

Problem is that if we add an API to get keys out of the wallet now, people will start using it and it will become MUCH harder to add HSM / secure enclave support later on.

I don't think that we should provide an API that will get the keys out. The wallet implementation which exposes some sign(msg) method which could also take a publicKey and it will check if it has the associated privateKey with that publicKey.

This was just the first thing that popped up in my head, but I am sure we can provide all the functionality, without the user of the framework / developers of other modules, to sign/verify/encrypt/etc without exposing keys.

Actually that's what I meant when saying that the Wallet should provide methods for using its crypto material. Now it's exposing wrap and unwrap (because it's tied to Indy's way of creating DIDComm messages) but it can be exposing sign methods that would be called by the DIDComm library. Or exposing a crypto provider object (or implementing its interface) used by didcomm-ts.

@spivachuk spivachuk force-pushed the didcommv2-contribution branch from b71f883 to b951b60 Compare November 22, 2022 17:33
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

Congrats guys, it is looking pretty nice! I know you are still working on it and more discussions are ongoing but I left a few comments regarding the latest changes in MessageSender and the generic usage of OutboundMessageContext (which might reduce the need of files changed in the PR).

Signed-off-by: Artemkaaas <artem.ivanov@evernym.com>
@Artemkaaas Artemkaaas force-pushed the didcommv2-contribution branch from 1372edb to 58ab2e7 Compare December 5, 2022 04:40
Signed-off-by: Artemkaaas <artem.ivanov@evernym.com>
@Artemkaaas Artemkaaas force-pushed the didcommv2-contribution branch from 10a91fb to 50236e2 Compare December 5, 2022 05:05
# Conflicts:
#	packages/core/src/agent/Dispatcher.ts
#	packages/core/src/agent/Handler.ts
#	packages/core/src/agent/MessageReceiver.ts
#	packages/core/src/agent/MessageSender.ts
#	packages/core/src/agent/__tests__/MessageSender.test.ts
#	packages/core/src/modules/connections/services/TrustPingService.ts
#	packages/core/src/modules/credentials/formats/__tests__/IndyCredentialFormatService.test.ts
#	packages/core/src/modules/credentials/formats/__tests__/JsonLdCredentialFormatService.test.ts
#	packages/core/src/modules/credentials/formats/jsonld/JsonLdCredentialFormatService.ts
#	packages/core/src/modules/dids/__tests__/dids-registrar.e2e.test.ts
#	packages/core/src/modules/dids/methods/sov/__tests__/IndySdkSovDidResolver.test.ts
#	packages/core/src/modules/ledger/__tests__/IndyLedgerService.test.ts
#	packages/core/src/modules/oob/OutOfBandApi.ts
#	packages/core/src/modules/oob/protocols/v1/handlers/HandshakeReuseHandler.ts
#	packages/core/src/types.ts
#	packages/core/src/utils/parseInvitation.ts
@TimoGlastra
Copy link
Contributor

I strongly agree on the need of removing complexity on current MessageSender class, but I'm not sure if it's better to define completely separated senders for each DIDComm version or to use a base MessageSender that receives the OutboundMessageContext object (where we can pass a V1/V2/Vx message) and according to it it would use one or another envelope.

I'm thinking mostly on sharing the logic for handling errors, adding messages to queues/sending to transports, etc. Of course this could also be achieved using an abstract MessageSender class and make V1MessageSender and V2MessageSender subclasses of it.

The first approach would have my preference as in general I try to follow the composition over inheritance model. It's easier to create consistent behaviour with a base message sender (or message sender coordinator or whatever we want to call it) that calls the specific message sender implementations than by having the logic logic inside of the message senders

@Artemkaaas
Copy link
Contributor Author

#1096 (comment)
@TimoGlastra I suggest postponing splitting Message Sender. As I mentioned in the document the main cause of having sendV1/sendV2 methods is that we have not fully integrated DidCommV2 with Connection state objects (current integration is super trivial and it is not unclear to us how to make it properly). I belive that once this Connections integration is done, all of v1/v2 branch methods go away.

@2byrds
Copy link
Contributor

2byrds commented Dec 5, 2022

Nice video from the latest Aries Javascript meeting, to catchup on the status of this PR:
https://wiki.hyperledger.org/display/ARIES/2022-12-01+Aries+Framework+JS+Meeting+notes?preview=%2F78022549%2F80773913%2Fvideo1253939668.mp4

Signed-off-by: Artemkaaas <artem.ivanov@dsr-corporation.com>
dkulic and others added 6 commits December 20, 2022 14:58
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Sacha Kozma <sacha.kozma@sicpa.com>
@yvgny yvgny force-pushed the didcommv2-contribution branch from 48dccc2 to 37310d2 Compare December 22, 2022 14:09
import { DidCommV1EnvelopeService } from '../index'

@injectable()
export class DidCommV1EnvelopeServiceImpl implements DidCommV1EnvelopeService {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming XxxImpl may suggest that there is no need for having a separate interface and implementation.

}

public async packMessage(
agentContext: AgentContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pass it via DI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the concept it's used across the framework, not sure what's the exact reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this on the last AFJ call, but still, it seems a bit harder to understand what's the difference between DI container and AgentContext and when to use one over another.

export { isDidCommV1Message } from './helpers'
export { DidCommV1Algorithms } from './types'
export { DidCommV1Types } from './types'
export { PlaintextDidCommV1Message } from './types'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather avoid adding definitions to the index. It's harder to see what objects are in a particular module (folder) when looking at folder tree. To me, indexes should be used only as export of internal items

@@ -39,7 +39,7 @@ interface TransportSessionTable {
export interface TransportSession {
id: string
type: string
keys?: EnvelopeKeys
keys?: PackMessageParams
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it can be confusing. But the cause might be the whole design of the Didcomm v1 vs v2 solution 🤔

dkulic and others added 16 commits January 3, 2023 10:29
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
…chment, V2Attachment)

Added tests for V2Attachment

Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Co-authored-by: Timo Glastra <timo@animo.id>
Signed-off-by: Darko Kulic <36031644+dkulic@users.noreply.github.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
Signed-off-by: Darko Kulic <darko.kulic@sicpa.com>
@TimoGlastra
Copy link
Contributor

There is now a feature branch in AFJ where we can continue the work: https://github.com/hyperledger/aries-framework-javascript/tree/feat/didcomm-v2

@TimoGlastra
Copy link
Contributor

Closing as it's superseded by other PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.