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

[Feature] Support DIDExchange 1.1 #1228 #1230

Merged
merged 26 commits into from
Jun 26, 2024

Conversation

gmulhearn
Copy link
Contributor

@gmulhearn gmulhearn commented Jun 18, 2024

This PR closes #1228 by implementing DIDExchange v1.1 support. It also does alot of DIDExchange/OOB/AATH fixes of bugs i found along the way whilst testing this implementation in the AATH setup against itself and against acapy.

DIDExchange v1.1 Implementation

Messages

The previous message structure for DIDExchange were scoped to v1.0. This implementation creates v1.1 message structures to sit alongside v1.0. The approach taken largely follows the guide laid out in the README. This is also the approach i did previously whilst creating issue-credential-v2 & present-proof-v2 message structures.

Where i took a bit of creative liberty though, was with creating a common module called v1_x. I felt this was necessary, as all messages in DIDExchange v1.1 & v1.0 are the exact same, except for the RESPONSE message (which simply adds the did_rotate field). So the v1_x module contains the common definitions for the complete, problemreport & request message structures, and some utility code for the dealing with the varying response messages.

Again, taking some creative liberty, i added an invisible version marker field on these common messages:

    #[serde(skip, default = "DidExchangeTypeV1::new_v1_1")]
    pub(crate) version: DidExchangeTypeV1,

This field is invisible to the consumer and to the JSON serialized format, however it's a run-time marker that allows us to convert from and to the AriesMessage type without losing the context of which DIDExchange version the message is. The unit tests ensure this is as expected. I modified the AriesMessage and DidExchange de/serializers to deal with setting and using this field properly.

I also played with another way of doing this, which used a compile-time marker for V1.0 vs V1.1 messages, it looked like this:

// in v1_x module
pub struct RequestContent<MinorVer> {
    ...,
    _marker: PhantomData<MinorVer>
}

// in v1_1 module
pub type RequestV1_1Content = RequestContent<DidExchangeTypeV1_1>;

// in v1_0 module
pub type RequestV1_0Content = RequestContent<DidExchangeTypeV1_0>;

this way, at compile time, there is 2 distinct types for v1.1 vs v1.0 messages. However, this got a bit ugly when actually using the implementation, as we need to create AnyXYZ wrappers for the two variants of each message, and pass those around everywhere, and then all consumers would need to switch/match on the different variants. I preferred the run-time way, but open to the other way.

Implementation (State machine)

The implementation did not have to change dramatically. essentially:

  • take AnyResponse instead of Response
  • requester impl will always try to request with a 1.1 request (and deal with it if the responder sends back a 1.0 response)
  • responder will decide what to do based on whether the request is v1.0 or v1.1. If v1.1, then we attach a signed did_rotate attachment, else default is to attach a signed did_doc.
  • The requester does not currently verify the signature of the response did-rotate. this is a detail delegated to this ticket: [FIX] DIDExchange handlers should do more signature checking #1226
  • add passthru of label - bcus why not
  • some code cleanup

Fixes for AATH

  • some of the backchannel APIs were not including enough data for the AATH controller to be happy, i've added some of those fields in.
  • ACApy compatibility: made it so that responses and complete messages will keep re-using the pthid. I don't necessarily agree with this being the correct usage of pthid, however it's what other agents do and what DIDExchange expects, and it doesn't hurt our implementation. some discussion: Special case of threading in didexchange #0023 ? aries-rfcs#817
  • changed our DIDExchange did:peer:4 creation helper it put a Verification Method (VM) reference in the keyAgreement of the DIDDoc, rather than directly embedding the VM. this is more "normal" from my understanding
  • Fixed up a bunch of didcomm_utils which were a bit slack
    • resolve_base58_key_agreement not being clear on it's purpose of fetching ed25519 base58 keys
    • resolve_base58_key_agreement not checking that the keys it fetched were ed25519
    • exact same case for get_ed25519_base58_routing_keys ^
    • implemented get_ed25519_base58_recipient_keys, which resolves keys referenced in a given diddoc service
  • make it so that the EncryptionEnvelope first tries get_ed25519_base58_recipient_keys (yields more accurate result than just "get the first key agreement key" as was previously done and is now used as a backup)
  • TODO comments where i think there should be future work
  • add support for mimetype of text/string. i'm not sure if it's valid, but acapy uses it, and so does the DIDExchange spec.
  • also made it so that the attachment type has a MaybeKnown for the MimeType when deserializing. Without this, it would fail to deserialize the whole attachment/message if the mimetype is unrecognised (i ran into this with acapy's text/string.)
  • made it so that dereference_key DIDDoc utility will check ALL places that an embedded VerificationMethod could be. Rather than just the "normal" case where it is embedded in verificationMethod field.
  • added support for understanding the verificationMethodType of MultiKey, which seems to be the preferred type in the did:peer spec. Also expanded the .public_key method to know how to resolve this VM type into a Key.
  • a bunch of fixes to the did:peer2 implementation:
    • append_encoded_key_segments was adding some encoded VMs twice
    • append keys into the verificationMethod field, and add a reference in the appropriate type (authentication, assertion, etc). rather than embedded (less "normal")
    • add an acapy sample and spec defined sample into the test suite
    • allow extra JSON fields to be included in the encoded service (e.g. for recipientKeys (acapy uses this correctly))
    • the VM key IDs in resolved DIDDocs were being given an ID based on the "prefixless fingerprint" (to_did_url_reference), i'm not sure where this idea came from, but the spec defined way is to just index the keys #key-N (nth_key_did_url_reference).

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn
Copy link
Contributor Author

gmulhearn commented Jun 19, 2024

Some notes on AATH testing:

Aries VCX (ACME) <-> ACApy (BOB):

RFC0023:

  • T003-RFC0023: success
  • others: fail (alot of them due to acapy's unqualified DIDs being sent alongside a legacy diddoc (which we fail to tolerate [FIX] DIDExchange should handle legacy DIDDoc format #1227), OR because the OOB has an embedded service rather than a DID, OR because the VCX AATH handlers aren't implemented)

RFC0793:

  • T001-RFC0793.1 fails (we don't support unqualified)
  • T001-RFC0793.2 fails (expects did:peer2, we only send did:peer4)
  • T001-RFC0793.3 success (did:peer4)
  • T002-RFC0793 fails (we accept only qualified DIDs)
  • T003-RFC0793.1 success
  • T003-RFC0793.2 fails (expects did:peer2, we only send did:peer4)
    • Note that if we force the scenario to use pure did:peer:4 <-> did:peer:4, then it succeeds.

Aries VCX (BOB) <-> ACApy (ACME):

RFC0023:

  • T001-RFC0023: success
  • T003-RFC0023: success
  • others; fail (mostly due to unimplemented VCX AATH handlers)

RFC0793*:

  • T001-RFC0793 success
  • T002-RFC0793 success
  • T003-RFC0793 success (and success in pure did:peer:4 case)

this one felt good, full suite:

> behave -D Acme=http://0.0.0.0:9020 -D Bob=http://0.0.0.0:9030 -t RFC0793
...
1 feature passed, 0 failed, 14 skipped
7 scenarios passed, 0 failed, 153 skipped
63 steps passed, 0 failed, 1320 skipped, 0 undefined

Aries VCX <-> Aries VCX:

RFC0023:

  • T001-RFC0023: success
  • T003-RFC0023: success
  • others; fail (mostly due to unimplemented VCX AATH handlers)

RFC0793*:

  • T001-RFC0793 (2/3 success). 1 failure due to only support did:peer:4 requests
  • T002-RFC0793 (1/2 success). 1 failure due to only support did;peer:4 requests
  • T003-RFC0793 (1/2 success). 1 failure due to only support did:peer:4 requests

Exceptions

  • for a lot of the aries-vcx BOB <-> ACAPY ACME tests, VCX is cheating a bit and getting away with it. as alot of these tests prompt BOB to create an unqualified DID invitation or a did:peer2 invitation, but VCX will just disobey and only create did:peer:4s. however the AATH is not picking up on this
  • when testing aries-vcx BOB <-> ACApy (ACME), there is a race condition on my machine. essentially, VCX was sending a DIDExchange response to acapy before acapy could catch up (most of the time). The result is that ACApy would create a NEW connection record and mess with the AATH state machine. If we add a 0.1second delay, then it gives acapy enough time to catch up.... not sure what to do about this, it's an upstream problem.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
…ing)

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn gmulhearn marked this pull request as ready for review June 21, 2024 01:35
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@JamesKEbert
Copy link
Contributor

Awesome stuff! Been slowly working through the PR review 👍

requester impl will always try to request with a 1.1 request (and deal with it if the responder sends back a 1.0 response)

We might be able to make the default be based on what is specified in a given OOB invite handshake protocols array. If they list 1.0, then that's what we use. If they specify 1.1 then we use 1.1. If this is using did exchange not via oob (not sure if we are/care to support) then defaulting to 1.1 makes sense.

Although I suppose it should work fine to use just 1.1 by default and then downgrade to 1.0 if that's what gets returned in the response. And that might be better as that means we try and use 1.1 always first.

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn-anonyome
Copy link
Contributor

gmulhearn-anonyome commented Jun 24, 2024

@JamesKEbert good point, i had a go at this in my latest commit: 5f5133d

let me know what you think, i can always revert.

If this is using did exchange not via oob (not sure if we are/care to support)

This is the case when we send a request based off an implicit invitation (e.g. a public resolvable DID with a service)

Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

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

Awesome stuff!
I definitely agree with the choice to have a common module for all the messages, as it's really just an extra field and does not deserve an entire new full gambit of protocol folder/messages/etc.

I have some concerns about the VCX message structures though, in particular relating to versions and invisible markers on Aries messages--I'd much rather we store the full version with all of the messages. Cause I can also agree that having multiple versions and needing to create AnyXYZ wrappers is gross. Chatting about it on the community call might be a good idea

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
@gmulhearn-anonyome
Copy link
Contributor

As discussed on call, i'm changing the approach to NOT have the invisible version marker inside the message. instead we create AnyXYZ wrappers, which have the minor version metadata externally in the enum variants

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

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

Good to go from my POV 👍

@gmulhearn gmulhearn merged commit 1a52699 into hyperledger:main Jun 26, 2024
22 checks passed
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.

[Feature] Support DIDExchange 1.1
3 participants