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

Add support for RFC 0434: Out-of-Band #344

Closed
Tracked by #345
TimoGlastra opened this issue Jul 3, 2021 · 8 comments
Closed
Tracked by #345

Add support for RFC 0434: Out-of-Band #344

TimoGlastra opened this issue Jul 3, 2021 · 8 comments
Assignees
Labels
AIP 2.0 Tasks related to AIP 2.0 support Protocol Tasks related to an Aries Protocol Type: Enhancement
Milestone

Comments

@TimoGlastra
Copy link
Contributor

Spec: https://github.com/hyperledger/aries-rfcs/blob/b3a3942ef052039e73cd23d847f42947f8287da2/features/0434-outofband/README.md

@jakubkoci
Copy link
Contributor

jakubkoci commented Nov 4, 2021

I wrote down a few notes I would like to share with you here.

Inbound Message processing rules

  1. Unpack if it’s packed message (JweLikeMessage)
    • Packed message can by
      • anoncrypt (unpack returns recipient key)
      • authcrypt (unpack returns recipient key and sender key)
  2. Transform JSON message to AgentMessage instance
  3. Validate message
  4. Validate if an agent is capable to respond to the message
    • agent has a connection identified by both recipient and sender key, we can’t identify connection just by recipient key because we cannot be sure that the sender is the same, therefore sender key must be the same as connection.theirKey
    • message has a ~service decorator -> deprecated when a new OOB is in use
    • it’s an old OOB message with did or service-related attributes
    • it’s a new OOB message and has a service attribute (maybe this can or should be validated in OOB module 🤷‍♂️ or receiveInvitation from OOB module moves to MessageReceiver and keeps just createInvitation)
  5. What if we don’t have information on how to respond at the moment of receiving a message?
    • Throw an error immediately and stop message processing.
    • Throw an error immediately and store the message for later. Yes but for how long?
    • Dispatch the message and leave the problem to MessageSender. MessageSender can store the messages which wasn't able to process for later… Hmm that doesn’t make much sense
  6. Dispatch message inside InboundMessageContext instance

Implementation Ideas

I started with an idea to create a separate OOB module which would create and process OOB messages. The problem is that I needed to copy-paste a lot of code from ConnectionModule and MessageReceiver. There is a similarity with ConnectionModule because we need to create connections and with MessageReceiver because we need to check if we have a connection and dispatch messages. Also, OOB module oob.receiveInvitation method is quite similar to messageSender.receiveMessage:

MessageReceiver

  • isPacked -> unpack
  • connectionExists -> reuse connection
  • transformMessage
  • validateMessage
  • saveSession
  • dispatch one message

OOB

  • connectionExists -> create or reuse connection
  • transformMessage
  • validateMessage
  • dispatch one or more messages

Currently, we assume there are three types of messages. The connection invitation which we pass to connections.receiveInvitation. Packed message and other OOB messages with ~service decorator, both passed to agent.receiveMessage. New OOB also has three types but is organized in a bit different way. There is an OOB message containing one or more JsonMessage.

agent.connections.receiveInvitation(invitation: InvitationJsonMessage)
agent.receiveMessage(message: PackedMessage | DecoratedJsonMessage)

To me, it makes sense to have just one method which would accept all

agent.receiveMessage(message: OobMessage | PackedMessage | JsonMessage)

The problem is that JsonMessage does not have ~service decorator and that processing of OobMessage could recursively call receiveMessage.

I suggest to implement OOB as part of ConnectionsModule for start.

connections.receiveOobInvitation(message: OobMessage)
connections.receiveInvitation(message: OobMessage | InvitationJsonMessage)
connections.createOobInvitation(...)

oob.receiveMessage(OobMessage)
messageReceive.receiveMessage(PackedMessage | JsonMessage+service)

This allows us gradual update according to Aries RFC 0496: Transition to the Out of Band and DID Exchange Protocols, create a new connection, call MessageReceiver.receiveMessage(PackedMessage | DecoratedJsonMessage) many times. We can also add ~service decorator to JsonMessage for now and remove this when we get rid of the current OOB implementation relying on ~service.

@jakubkoci
Copy link
Contributor

jakubkoci commented Nov 25, 2021

I found a problem with processing the connection request when the connection already exists.

The OOB spec says: “if a connection they have was created from an out-of-band invitation from the same services DID of a new invitation message, the connection MAY be reused. The receiver may choose to not reuse the existing connection for privacy purposes and repeat a handshake protocol to receive a redundant connection.”

Let’s say that a connection request is sent from Bob (invitee) to Alice (inviter), encrypted with Alice’s key (recipientKey), signed by Bob’s key (senderKey). Current implementation finds a connection based on Alice’s key (recipientKey) and because Bob is trying to make a new connection (by sending packed Connection Request message) with a different key (senderKey), it fails with the following error:

AriesFrameworkError: Inbound message senderKey '4Bxpr2BAeZUAAGw6bg4aKWc9XWDh7oFaBf4wVxCMteJe' is different from connection.theirKey '8aZJYVzp8TyfjY5Eh9rXwzpUNGjsoUwrVAYqTEL7JE83'

I guess we should not throw an error and pass the message forward. If the new connection is successfully made, the previous connection became stale. I’m not sure how to handle that.

I tried to change the logic in MessageReceiver.receiveMessage method to this:

if (senderKey && recipientKey) {
  connection = await this.connectionService.findByTheirKey(senderKey)
}

But there are failing mediation tests with that. I’m digging into it and trying to understand what’s the problem, but I just wanted to share it with you here if you have any ideas.

@TimoGlastra
Copy link
Contributor Author

Let’s say that a connection request is sent from Bob (invitee) to Alice (inviter), encrypted with Alice’s key (recipientKey), signed by Bob’s key (senderKey). Current implementation finds a connection based on Alice’s key (recipientKey) and because Bob is trying to make a new connection (by sending packed Connection Request message) with a different key (senderKey), it fails with the following error:

I'm not following you 100%. When alice receives the request the connection doesn't have theirKey yet, so how can this happen?

@jakubkoci
Copy link
Contributor

Yeah, sorry I didn't explain it well. I try it again :)

Let’s say that there is an existing connection between Alice (A.pk1, B.pk1) and Bob (B.pk1, A.pk1).

  • Alice (inviter) sends OOB invitation with A.pk1.
  • Bob (invitee) receives the invitation but decides to not reuse the connection. Bob creates a connection request with new B.pk2 and encrypts it with A.pk1
  • Alice receives the connection request. Unpack returns A.pk1 as recipientKey and B.pk2 as senderKey
  • Alice already has a connection where recipientKey is A.pk1, therefore theirKey is not null but is equal to B.pk1, therefor it's different than senderKey B.pk2.

@jakubkoci
Copy link
Contributor

Aha, if I remove the condition connection && connection.theirKey !== null && connection.theirKey !== senderKey it passes. But that's probably something we don't want to do :)

@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Nov 25, 2021

I understand now. Let's talk about it during the WG call. There are some things to consider here, but I think we could loosen the restriction a bit and allow it to pass through as you said. I think this is related to the work we will need to do for connections using public dids, as it would also create the scenario where the same key is used for multiple connections. Difference is that we would still like to create a new key for each interaction, we just associate it with the public did.

If we loosen the restrictions, it will work something like: Alice receives a connection request. We look at the recipientKey and senderKey to retrieve the connection.

We could still check then if the request is valid based on the following checks (in the connection response handler):

  • Is the key used to connect associated with a public did, and is that public did allowed to be used as an implicit invitation?
  • Did we create a connection invitation / OOB invitation that uses this key, that hasn't been completed yet (could be a multi-use invitation)

In general protocol implementation themselves check whether a connection is attached and whether that connection is complete. This can be easily done using context.assertReadyConnection(). The most important aspect here is that we don't want to associate a connection to the context that is not related.

@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Nov 25, 2021

As a practical note. We can remove the check, but need to modify the connection assignment a few lines above to only be assigned to the upper scope connection property.

So something like this:

    let connection: ConnectionRecord | null = null

    // Only fetch connection if recipientKey and senderKey are present (AuthCrypt)
    if (senderKey && recipientKey) {
     connection = await this.connectionService.findByQuery({ verkey: recipientKey, theirKey: senderKey })
    }

@TimoGlastra TimoGlastra added this to the 0.2.0 milestone Mar 1, 2022
@TimoGlastra
Copy link
Contributor Author

Fixed in #717, outstanding issues tracked in #764

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP 2.0 Tasks related to AIP 2.0 support Protocol Tasks related to an Aries Protocol Type: Enhancement
Projects
None yet
Development

No branches or pull requests

2 participants