feat: Protocols can use DIDs to identify the agents they communicate with #938

Moopli · 2019-12-06T17:31:22Z

Outbound: create a Destination struct from a DID using helper functions, so an agent can provide the DID of the target.

One function resolves a DID using your VDRI
The other uses a DID doc if it's already available

Inbound: receive the DID of the sender as a parameter to the message handler, if the sender has a DID doc in your VDRI.

Fixes #725

Moopli · 2019-12-06T17:32:21Z

BDD tests are currently broken, but review from @soluchok is needed, for the interface this provides to protocols.

Moopli · 2019-12-06T22:41:07Z

Current API this PR creates:

This saves the correlation/connection between another agent's peer DID/keys, and the peer DID your agent uses to communicate with them. DIDExchange uses this to save the connection information when the protocol succeeds.
pkg/didcomm/common/didconnection.DIDConnectionStore.SaveDIDConnection():
context.DIDConnectionStore().SaveDIDConnection(myDID, theirDID string, theirKeys []string) error

To get the Destination struct for sending outbound messages, provide the target's DID to pkg/didcomm/common/service.GetDestination():
service.GetDestination(did, serviceType, keyType string, vdr vdri.Registry) (*Destination, error)

But why would you, when you can call SendToDID on the outbound dispatcher, and also skip determining your sender key:

// SendToDID sends a message from myDID to the agent who owns theirDID
func (o *OutboundDispatcher) SendToDID(msg interface{}, myDID, theirDID string) error

And for inbound messages, HandleInbound() has been extended to provide DID parameters for the sender's DID and the recipient DID (since you need to know which of your DIDs the message was sent to).
HandleInbound(msg *DIDCommMsg, myDID string, theirDID string) (string, error)

I still have to write a lot of tests and then bugfix, but any feedback is welcome.

soluchok · 2019-12-07T06:50:52Z

pkg/didcomm/common/service/service.go

@@ -16,7 +16,7 @@ import (
 // Handler provides protocol service handle api.
 type Handler interface {
 	// HandleInbound handles inbound messages.
-	HandleInbound(msg *DIDCommMsg) (string, error)
+	HandleInbound(msg *DIDCommMsg, myDID string, theirDID string) (string, error)


It looks like my myDID is redundant. We can get my myDID using a GetDID function. e.g
myDID := GetDID(theirDID). According to that line https://github.com/hyperledger/aries-framework-go/pull/938/files#diff-b429c177e8c9b423d01fcc0d7d2383e9R81

This shouldn’t be guaranteed. An agent could have multiple connections to the same “theirDIDs” with different “myDIDs”.

Yeah, but it is a map. I mean only two DIDs can be related to each other. According to code we have. See link ^

Otherwise GetDID(theirDID) should return a list of myDids. Which means you can use any of them. Right?

I am a bit confused. Why do we need multiple myDIDs by theirDid ? If the connection is established it doesn’t make sense to create one more. I am ok to create one more connection but with different DIDs pair (with the same agent)

It's useful for communicating with a public DID - you might want multiple DIDs you can use to communicate with them, for privacy.

A Lookup function is needed in the didconnection package to make this more useful. See prior comment.

Since myDID and theirDID are parameters to HandleInbound, we don't need the theirDID -> myDID map. As parameters to HandleInbound, the DIDs can now be any values for any connection, even if theirDID is used in multiple connections.

soluchok · 2019-12-07T06:57:50Z

pkg/didcomm/dispatcher/outbound.go

+}
+
+// SendToDID sends a message from myDID to the agent who owns theirDID
+func (o *OutboundDispatcher) SendToDID(msg interface{}, myDID, theirDID string) error {


This function also can be optimized. We can get myDID from DIDConnectionStore by theirDID. It would be easy to use.
e.g SendToDID(msg interface{}, theirDID string)
and better to add a comment like function must be used after the connection was successfully established between two agents

Public DIDs, which we can use multiple peer DIDs to communicate with, means we can't actually do this mapping in all cases. I've added support for providing "myDID" to the inbound handler in a service.

Moopli · 2019-12-11T00:09:22Z

Tests are passing, meaning it's time to write more tests.
I will take this out of draft when code is better tested.

troyronda · 2019-12-11T00:36:54Z

pkg/didcomm/common/didconnection/didconnection.go

+	"github.com/hyperledger/aries-framework-go/pkg/storage"
+)
+
+// BaseDIDConnectionStore stores DIDs indexed by key


why the prefix Base?

Another name would be welcome - Store is the interface name, and I didn't want a stuttering name. I can pull the interface out into an api package in the framework (which would allow this struct type to be named Store), though the interface is only necessary to allow mocking, and isn't intended to be implemented and injected by the client.

If a package's only purpose is to implement a struct, it is okay to have that stutter.
I don't think Base adds any value.

(I'm also not understanding the conflict with the Store interface defined in another package?)

The conflict would be with the interface in the same package - didconnection.Store in didconnection/api.go. But I've renamed to ConnectionStore so there's no conflict.

pkg/didcomm/dispatcher/outbound.go

troyronda · 2019-12-11T00:43:20Z

pkg/didcomm/dispatcher/outbound.go

 }

+const (
+	ed25519KeyType     = "Ed25519VerificationKey2018"


I assume hardcoding the key type is temporary?

I can make a TODO and issue - this question extends beyond the scope of this PR, and I aimed to just carry over the current design with these constants. The key type needs to be flexible, and our resolution of keys from DID docs seems to already support multiple key types, but these constants need to be replaced by another system (the communication layer recognizing the key type being used by the connection, and fetching the appropriate keys from DID docs).

pkg/didcomm/packager/packager.go

troyronda · 2019-12-11T01:02:40Z

pkg/didcomm/packer/jwe/authcrypt/authcrypt_test.go

 		require.NoError(t, err)
 		require.Equal(t, msgIn, msgOut)
 		require.Equal(t, base58.Encode(sendKey), encSign)
+		// this won't work, since input keys and output keys to this Packer


what’s the resolution to this comment?

The JWE packer would need to be changed to use the same key format as Pack() parameters and Unpack() return values, so the keys returned by Unpack() are keys passed into Pack() rather than the Curve25519 conversions of given Ed25519 keys. With an agent that supports multiple formats for encryption keys, we could do this - enforce that the JWE packer uses Curve25519 keys as parameters/return values, and that it only be chosen for DIDComm channels that have Curve25519 pubkeys available on both ends.

Is this a TODO?

pkg/didcomm/packer/jwe/authcrypt/unpack.go

pkg/didcomm/packer/legacy/authcrypt/authcrypt_test.go

pkg/didcomm/protocol/didexchange/persistence.go

pkg/didcomm/protocol/didexchange/service.go

troyronda · 2019-12-11T01:18:41Z

pkg/didcomm/protocol/didexchange/states.go

@@ -404,6 +405,11 @@ func (ctx *context) getDIDDocAndConnection(pubDID string) (*did.Doc, *Connection
 			return nil, nil, fmt.Errorf("resolve public did[%s]: %w", pubDID, err)
 		}

+		err = ctx.connectionStore.didMap.SaveDIDFromDoc(didDoc, didCommServiceType, ed25519KeyType)


3 references above the func call might call for a refactor.

I will make an issue for this one - (permanent) connectionStore and the DIDConnection Store should be merged in another PR.

pkg/didcomm/common/didconnection/didconnection.go

pkg/didcomm/common/didconnection/didconnection_test.go

pkg/didcomm/common/service/destination.go

pkg/didcomm/packager/packager.go

pkg/didcomm/packer/jwe/authcrypt/unpack.go

Moopli · 2019-12-19T23:06:14Z

Coverage almost there. Next, will merge in updates (generic connection store), and test that DIDs are saved correctly for resolution.

pkg/didcomm/common/didconnection/didconnection.go

troyronda · 2019-12-20T19:19:49Z

pkg/didcomm/common/service/destination.go

@@ -24,7 +24,8 @@ type Destination struct {

 const (
 	didCommServiceType = "did-communication"
-	ed25519KeyType     = "Ed25519VerificationKey2018"
+	// TODO: hardcoded key type


Is there more context on this TODO? Why hardcoded?

When we're using another key type for the communication, we need to remove the hardcoding of the key type and instead set it based on some connection parameters.

created issue #1008

pkg/didcomm/common/transport/envelope.go

pkg/didcomm/packager/packager.go

pkg/didcomm/protocol/didexchange/states.go

troyronda · 2019-12-20T20:54:58Z

pkg/didcomm/protocol/didexchange/states.go

@@ -415,6 +422,16 @@ func (ctx *context) getDIDDocAndConnection(pubDID string) (*did.Doc, *Connection
 		return nil, nil, fmt.Errorf("create %s did: %w", didMethod, err)
 	}

+	// TODO: initialize did map (or mock) in all eleventy billion tests


purged this check now.
framework initializes this properly, and we don't put checks in to catch broken tests.

didconnection.Store provides a lookup that lets you find a DID using one of its public keys. This currently only works for peer DIDs. Signed-off-by: Filip Burlacu <[email protected]>

Outbound: SendToDID resolves the target DID, and sends the message to the destination specified in the DID doc. Inbound: HandleInbound receives the DID of the sender and the DID you used to decrypt the message. Signed-off-by: Filip Burlacu <[email protected]>

Moopli force-pushed the 725 branch 5 times, most recently from 9385017 to 03369ac Compare December 6, 2019 22:35

soluchok reviewed Dec 7, 2019

View reviewed changes

Moopli force-pushed the 725 branch 2 times, most recently from 2374d91 to e3409a0 Compare December 11, 2019 00:06