fix: fixes for didexchange interop with aca-py #2655

Moopli · 2021-03-19T17:03:31Z

accept old and new didexchange message formats and mime types:

application/ssi-agent-wire is accepted as an inbound mime type
did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/didexchange/1.0 is accepted as
an inbound PIURI for didexchange messages
did doc can be parsed from "did_doc~attach" attachment in request
messages, as per didexchange spec, in addition to being accepted
inside "connection" field, as per connection-request spec

did doc format:

Parse legacy peer did docs with publicKey members even if their
context states they are using a newer format.
Translate raw ed25519 public key values to did:key when reading
recipient keys or routing keys

Signed-off-by: Filip Burlacu [email protected]

codecov · 2021-03-19T17:08:50Z

Codecov Report

Merging #2655 (9b5a622) into main (a0e436a) will increase coverage by 0.03%.
The diff coverage is 97.01%.

❗ Current head 9b5a622 differs from pull request most recent head 6c829dd. Consider uploading reports for the commit 6c829dd to get more accurate results

@@            Coverage Diff             @@
##             main    #2655      +/-   ##
==========================================
+ Coverage   88.38%   88.41%   +0.03%     
==========================================
  Files         265      265              
  Lines       20695    20757      +62     
==========================================
+ Hits        18291    18353      +62     
  Misses       1404     1404              
  Partials     1000     1000

Impacted Files	Coverage Δ
pkg/didcomm/common/service/did_comm_msg.go	`95.34% <0.00%> (-1.51%)`	⬇️
pkg/didcomm/common/service/destination.go	`100.00% <100.00%> (ø)`
pkg/didcomm/protocol/didexchange/service.go	`89.13% <100.00%> (+0.61%)`	⬆️
pkg/didcomm/protocol/didexchange/states.go	`89.41% <100.00%> (+0.07%)`	⬆️
pkg/didcomm/transport/http/inbound.go	`81.53% <100.00%> (ø)`
pkg/doc/did/doc.go	`88.31% <100.00%> (+0.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0e436a...6c829dd. Read the comment docs.

troyronda · 2021-03-20T13:06:30Z

pkg/didcomm/protocol/didexchange/service.go

@@ -44,6 +46,17 @@ const (
 	ResponseMsgType = PIURI + "/response"
 	// AckMsgType defines the did-exchange ack message type.
 	AckMsgType = PIURI + "/ack"
+	// OldPIURI is the old did-exchange protocol identifier URI.
+	OldPIURI = "did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/didexchange/1.0"


@swcurran @andrewwhitehead when will this old PIURI base (did:sov:BzCbsNYhMrjHiqZDTUASHg;spec) no longer be sent from AcaPy?

Noticed in msg transition:

In step 2 - community is updating implementations to send new formats. Target Completion Date: 2020.10.15"

Also, probably worth noting that DID Exchange was one of the "new" protocols. @swcurran @andrewwhitehead

From msg transition:

Note: Any RFCs that already use the new "https" message type should continue to use the use new format in all cases—accepting and sending. New protocols defined in new and updated RFCs should use the new "https" format.

At the moment ACA-Py has a startup option to use the new URI prefix on outgoing messages. If the .NET framework now accepts that then we may be able to make it the default in the next release. Changing the prefix is automatic, so it doesn't have custom behaviour depending on the message type (ie. did-exchange).

Looks like .NET has https handling code for the HTTPs prefixes. Can you please switch the default @swcurran @andrewwhitehead @tmarkovski - we should be using the https prefix in the test harness (and generally). openwallet-foundation/acapy#1042

@andrewwhitehead -- ACA-Py should be using the new prefix's on all new protocols anyway -- such as DID Exchange.

We need to do a quick pass through the various mobile agents to ensure that all still work, but I think we are good with switching.

We really need to figure out how to do these community updates better -- how we can communicate when we can go from step to step reliably. Basically -- that means we need participation from those involved -- software creators that build the frameworks and those that deploy them. It's the latter that is the tricky part.

pkg/didcomm/protocol/didexchange/states.go

troyronda · 2021-03-20T13:09:22Z

pkg/didcomm/transport/http/inbound.go

@@ -21,6 +21,11 @@ import (

 var logger = log.New("aries-framework/http")

+const (
+	// acceptInboundContentType additional content type to be accepted for inbound messages.
+	acceptInboundContentType = "application/ssi-agent-wire"


@swcurran @andrewwhitehead when does this old media type (application/ssi-agent-wire) go away?

Again need to test with the .NET framework, last I checked it only accepts this one.

openwallet-foundation/acapy#1044
hyperledger-archives/aries-framework-dotnet#175

troyronda · 2021-03-20T13:12:15Z

pkg/vdr/peer/vdr.go

@@ -50,5 +50,5 @@ func (v *VDR) Deactivate(did string, opts ...vdrapi.DIDMethodOption) error {

 // Accept did method.
 func (v *VDR) Accept(method string) bool {
-	return method == DIDMethod
+	return method == DIDMethod || method == "sov" // accept sov 'peer' did docs for interop


@swcurran @andrewwhitehead @Moopli @llorllale I don’t understand this one. Why would a did:sov be interpreted as did:peer?

If our peer DID docs are using did:sov then that's likely an error, although standardizing on did:peer also doesn't seem ideal until that spec is finalized.

@Moopli @andrewwhitehead we can't treat did:sov as did:peer - if we are being sent did:sov, we must resolve it using the did:sov driver.

If this is happening, AcaPy must fix it.

@Moopli please revert this change and describe the problem in an AcaPy issue.

@andrewwhitehead -- let us know the state of this. If this convention is in the wild, how do we eliminate it without breaking the interop we have today and add the interop we need tomorrow.

@swcurran I'd start by just generating our current peer DID Doc implementation with the did:peer prefix to see how that works with interop (assuming we don't currently use did:peer, but I can't see any references to it in the code base). Maybe @sklump could have a look at that this week?

@troyronda @andrewwhitehead I raised an issue over on aca-py for this openwallet-foundation/acapy#1048

Relevant:

AIP 2.0: support for the did:peer method hyperledger/aries-rfcs#611

RFC0023: clarify did vs did_doc~attach in requests and responses hyperledger/aries-rfcs#609

troyronda · 2021-03-20T13:14:53Z

pkg/doc/did/doc.go

+	return doc, nil
+}
+
+func legacyParseSov(raw *rawDoc) (*Doc, error) {


@swcurran @andrewwhitehead DID v1 documents now use the verificationMethod property not publicKey. I.e., We are receiving an invalid DID document and this is special handling due to that.

Again need to check with the .NET framework but I don't think it will accept verificationMethod.

@andrewwhitehead they will need to fix this. https://www.w3.org/TR/did-core/#verification-methods

It's about a community upgrade. When the first cut was implemented "publicKeys" was the attribute name in the then DID Core spec.

Yup. Created openwallet-foundation/acapy#1050 and hyperledger-archives/aries-framework-dotnet#178

troyronda · 2021-03-20T13:58:46Z

pkg/didcomm/protocol/didexchange/service.go

@@ -246,7 +259,11 @@ func (s *Service) Accept(msgType string) bool {
 	return msgType == InvitationMsgType ||
 		msgType == RequestMsgType ||
 		msgType == ResponseMsgType ||
-		msgType == AckMsgType
+		msgType == AckMsgType ||
+		msgType == OldInvitationMsgType ||


I'd prefer doing a string replace of did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/ => https://didcomm.org/ at a higher layer. I assume this isn't the only case of this old PIURI being sent from AcaPy?

@Moopli @llorllale

hyperledger/aries-rfcs#579 (comment)

troyronda · 2021-03-21T10:35:25Z

pkg/didcomm/common/service/destination.go

@@ -23,6 +26,8 @@ type Destination struct {

 const (
 	didCommServiceType = "did-communication"
+	// legacyDIDCommServiceType is the non-spec service type used by legacy didcomm agent systems.
+	legacyDIDCommServiceType = "IndyAgent"


@swcurran @andrewwhitehead When is the IndyAgent service type going away?

openwallet-foundation/acapy#1043
hyperledger-archives/aries-framework-dotnet#176

troyronda · 2021-03-22T20:11:40Z

pkg/doc/did/doc.go

@@ -603,7 +650,7 @@ func getVerification(doc *Doc, rawVerification interface{},
 		return nil, errors.New("rawVerification is not map[string]interface{}")
 	}

-	if context == contextV011 {
+	if legacy || context == contextV011 {


this is a bit unfortunate @fqutishat @Moopli
i.e., detecting legacy at two different layers.

troyronda · 2021-03-22T20:15:15Z

pkg/doc/did/doc.go

-	err = validate(data, raw.schemaLoader())
+
+	switch {
+	case strings.HasPrefix(raw.ID, "did:sov:"):


This might not be 100% correct. This is more a case of AcaPy currently sending some document in DID exchange as did:sov ...
@fqutishat @Moopli

@Moopli It would be safer to detect this case in DID Exchange code rather than here.

troyronda · 2021-03-23T11:32:50Z

pkg/didcomm/common/service/did_comm_msg.go

@@ -25,6 +25,9 @@ const (
 	jsonThreadID       = "thid"
 	jsonParentThreadID = "pthid"
 	jsonMetadata       = "_internal_metadata"
+
+	oldPIURI = "did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/"
+	newPIURI = "https://didcomm.org/"


Let's not call this one "new".

troyronda · 2021-03-23T11:35:57Z

pkg/didcomm/protocol/didexchange/models.go

@@ -74,6 +74,11 @@ type Request struct {
 	Label      string            `json:"label,omitempty"`
 	Connection *Connection       `json:"connection,omitempty"`
 	Thread     *decorator.Thread `json:"~thread,omitempty"`
+	// DID the did of the requester. Optional, may be present within `connection` instead.


@llorllale @Moopli - is the DID property supposed to be optional or is this for interop?

I see DocAttach is optional in the RFC.

hyperledger/aries-rfcs#608

troyronda · 2021-03-23T11:36:45Z

pkg/doc/did/doc.go

+	// Interop: handle legacy did docs that incorrectly indicate they use the new format
+	if requiresLegacyHandling(raw) {
+		ctx, _ := json.Marshal(raw.Context)
+		println("legacy:", string(ctx))


@Moopli please remove prints.

troyronda · 2021-03-23T11:38:00Z

pkg/framework/aries/framework.go


 	p, err := peer.New(ctx.StorageProvider())
 	if err != nil {
 		return fmt.Errorf("create new vdr peer failed: %w", err)
 	}

+	// Interop: use a did:sov VDR that will prefer to store sov DID docs locally, by wrapping the did:peer VDR.


@Moopli - no, you cannot override did:sov at this level. Needs to be within did exchange only.

Let's remove this change and topic from this PR. @Moopli

troyronda · 2021-03-23T11:39:09Z

pkg/vdr/sov/vdr.go

+SPDX-License-Identifier: Apache-2.0
+*/
+
+package sov


@Moopli - no - this is not did:sov. It is confusing to have this package named sov in vdr.

Additionally, it doesn't really help. You need to fix the document in did exchange.

Let's remove this change and topic from this PR @Moopli

troyronda · 2021-03-23T14:35:52Z

pkg/doc/did/doc.go

@@ -404,6 +410,22 @@ func ParseDocument(data []byte) (*Doc, error) {
 	return doc, nil
 }

+func requiresLegacyHandling(raw *rawDoc) bool {
+	if len(raw.PublicKey) == 0 { // docs in v1 format don't have a top-level publicKey array


|| len(raw.VerificationMethod) > 0

|| or &&? if we get a doc that has no keys, should we do legacy tweaks?

accept old and new didex. message formats and mime types: - application/ssi-agent-wire is accepted as an inbound mime type - did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/didexchange/1.0 is accepted as an inbound PIURI for didexchange messages - did doc can be parsed from "did_doc~attach" attachment in request messages, as per didexchange spec, in addition to being accepted inside "connection" field, as per connection-protocol spec did doc format: - Parse legacy peer did docs with publicKey members even if their context states they are using a newer format. - Translate raw ed25519 public key values to did:key when reading recipient keys or routing keys Signed-off-by: Filip Burlacu <[email protected]>

troyronda · 2021-03-23T14:46:46Z

pkg/didcomm/protocol/didexchange/models.go

+	Label  string            `json:"label,omitempty"`
+	Thread *decorator.Thread `json:"~thread,omitempty"`
+	// DID the did of the requester.
+	// Mandatory in did-exchange, but optional for backwards-compatibility with rfc 0160 connection protocol.


@Moopli @llorllale do we have more description of why backwards-compatibility with RFC 0160 is needed?

RFC0160 is not in the AIP 2.0 draft so far: https://github.com/hyperledger/aries-rfcs/pull/579/files?short_path=a9296f0#diff-a9296f04f44f2daa050b1b0724458240a85f029aa7b77ed83b17a5479ef97f85

We should probably confirm, but I believe there is a desire to deprecate RFC0160 going forward.

cc @swcurran @andrewwhitehead

@troyronda I have to look through aca-py to see if any elements of 0160 are still used there - this, in particular, is a place where afgo was behind.

…2655) accept old and new didex. message formats and mime types: - application/ssi-agent-wire is accepted as an inbound mime type - did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/didexchange/1.0 is accepted as an inbound PIURI for didexchange messages - did doc can be parsed from "did_doc~attach" attachment in request messages, as per didexchange spec, in addition to being accepted inside "connection" field, as per connection-protocol spec did doc format: - Parse legacy peer did docs with publicKey members even if their context states they are using a newer format. - Translate raw ed25519 public key values to did:key when reading recipient keys or routing keys Signed-off-by: Filip Burlacu <[email protected]>

Moopli force-pushed the didex-msg branch 2 times, most recently from d118a44 to 21c497a Compare March 20, 2021 02:59

Moopli marked this pull request as ready for review March 20, 2021 03:10

troyronda reviewed Mar 20, 2021

View reviewed changes

pkg/didcomm/protocol/didexchange/states.go Outdated Show resolved Hide resolved

troyronda reviewed Mar 20, 2021

View reviewed changes

troyronda requested review from llorllale, swcurran, andrewwhitehead, TelegramSam and fqutishat March 20, 2021 13:19

troyronda reviewed Mar 20, 2021

View reviewed changes

troyronda reviewed Mar 21, 2021

View reviewed changes

troyronda requested a review from tmarkovski March 21, 2021 10:39

This was referenced Mar 21, 2021

fix: fixes for outofband interop with acapy #2659

Merged

https prefix for PIURI as the default hyperledger-archives/aries-framework-dotnet#177

Open

Moopli force-pushed the didex-msg branch 2 times, most recently from bab6fc9 to c806dba Compare March 22, 2021 16:45

Moopli requested a review from troyronda March 22, 2021 17:03

Moopli force-pushed the didex-msg branch from c806dba to 92a29a8 Compare March 22, 2021 18:03

troyronda reviewed Mar 22, 2021

View reviewed changes

Moopli force-pushed the didex-msg branch 2 times, most recently from 7b3cad0 to d6d7736 Compare March 23, 2021 08:19

troyronda reviewed Mar 23, 2021

View reviewed changes

Moopli force-pushed the didex-msg branch 2 times, most recently from 5ffe344 to 82ab026 Compare March 23, 2021 14:28

Moopli force-pushed the didex-msg branch 2 times, most recently from c80f86b to 0565bea Compare March 23, 2021 14:34

troyronda reviewed Mar 23, 2021

View reviewed changes

Moopli force-pushed the didex-msg branch from 0565bea to 6c829dd Compare March 23, 2021 14:43

troyronda reviewed Mar 23, 2021

View reviewed changes

Moopli requested a review from troyronda March 23, 2021 15:22

troyronda approved these changes Mar 23, 2021

View reviewed changes

troyronda merged commit d769e21 into hyperledger-archives:main Mar 23, 2021

x0axz mentioned this pull request Jan 13, 2022

Enable HTTPS for Aries Agents hyperledger-archives/aries-framework-dotnet#211

Closed

fix: fixes for didexchange interop with aca-py #2655

fix: fixes for didexchange interop with aca-py #2655

Conversation

Moopli commented Mar 19, 2021 • edited Loading

codecov bot commented Mar 19, 2021 • edited Loading

Codecov Report

troyronda Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

troyronda Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

andrewwhitehead Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

troyronda Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

troyronda Mar 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 21, 2021 • edited Loading

Choose a reason for hiding this comment

troyronda Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troyronda Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moopli commented Mar 19, 2021 •

edited

Loading

codecov bot commented Mar 19, 2021 •

edited

Loading

troyronda Mar 20, 2021 •

edited

Loading

troyronda Mar 20, 2021 •

edited

Loading

andrewwhitehead Mar 21, 2021 •

edited

Loading

troyronda Mar 21, 2021 •

edited

Loading

troyronda Mar 20, 2021 •

edited

Loading

troyronda Mar 21, 2021 •

edited

Loading

troyronda Mar 21, 2021 •

edited

Loading

troyronda Mar 20, 2021 •

edited

Loading

troyronda Mar 22, 2021 •

edited

Loading

troyronda Mar 20, 2021 •

edited

Loading

troyronda Mar 21, 2021 •

edited

Loading

troyronda Mar 22, 2021 •

edited

Loading

troyronda Mar 22, 2021 •

edited

Loading

troyronda Mar 23, 2021 •

edited

Loading

troyronda Mar 23, 2021 •

edited

Loading

troyronda Mar 23, 2021 •

edited

Loading