feat: remove keys on mediator when deleting connections #1143

genaris · 2022-12-07T03:11:32Z

This PR addresses the case where a mobile agent (or any agent that uses mediator) deletes a connection or OOB record, meaning that the corresponding recipient keys should be removed from its mediator (as per Aries RFC 0211). Otherwise, mediator will still be forwarding messages to the agent that can't (and more importantly don't want to) be handled.

This will mean that, when using mediator, in order to delete a connection or out-of-band record, the agent must be online. This is aligned to the opposite case: connection and OOB invitation creation, which also need the agent to be online to be in sync with its mediator.

Signed-off-by: Ariel Gentile <[email protected]>

…-keys

Signed-off-by: Ariel Gentile <[email protected]>

codecov-commenter · 2022-12-07T03:48:28Z

Codecov Report

Merging #1143 (4d22ce3) into main (979c695) will increase coverage by 0.14%.
The diff coverage is 64.10%.

@@            Coverage Diff             @@
##             main    #1143      +/-   ##
==========================================
+ Coverage   88.34%   88.48%   +0.14%     
==========================================
  Files         706      706              
  Lines       16436    16468      +32     
  Branches     2662     2670       +8     
==========================================
+ Hits        14520    14572      +52     
+ Misses       1909     1889      -20     
  Partials        7        7

Impacted Files	Coverage Δ
packages/core/src/modules/oob/OutOfBandApi.ts	`83.33% <16.66%> (-1.39%)`	⬇️
packages/core/src/modules/routing/RecipientApi.ts	`70.86% <27.27%> (+9.33%)`	⬆️
...ules/routing/services/MediationRecipientService.ts	`85.86% <92.85%> (+1.33%)`	⬆️
...ges/core/src/modules/connections/ConnectionsApi.ts	`68.88% <100.00%> (+1.19%)`	⬆️
...c/modules/routing/messages/KeylistUpdateMessage.ts	`100.00% <100.00%> (ø)`
...ore/src/modules/routing/services/RoutingService.ts	`100.00% <100.00%> (ø)`
packages/core/src/modules/dids/DidsApi.ts	`78.57% <0.00%> (+7.14%)`	⬆️
.../core/src/modules/routing/RecipientModuleConfig.ts	`100.00% <0.00%> (+26.66%)`	⬆️

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

packages/core/src/modules/connections/ConnectionsApi.ts

Signed-off-by: Ariel Gentile <[email protected]>

TimoGlastra · 2022-12-08T01:46:11Z

packages/core/src/modules/connections/ConnectionsApi.ts

+    const connection = await this.connectionService.getById(this.agentContext, connectionId)
+
+    if (connection.mediatorId && connection.did) {
+      const did = await this.didResolverService.resolve(this.agentContext, connection.did)
+
+      if (did.didDocument) {
+        await this.routingService.removeRouting(this.agentContext, {
+          recipientKeys: did.didDocument.recipientKeys,
+          mediatorId: connection.mediatorId,
+        })
+      }
+    }


Just thinking forward a bit here. If we start supporting public dids I could have multiple connections using the same did, thus removing all the keys at the mediator could lead to issues for other connections. That's not the case at the moment, but just thinking here whether that could lead to issues in the future.

Also we have the didRecord that contains the recipientKeys of the did document, which means we wouldn't have to fetch it from the ledger. Not sure though if that's a better approach?

Good point here. So the goal is to simply remove the recipientKeys needed for this particular connection, provided they won't affect other connections also related to the same public DID.

If i'm not wrong, even if we are using public DIDs for invitations, we will be always doing some sort of key rotation, because otherwise we wouldn't know, when receiving a message, which connection is that message for. What would be the most direct approach to get the actual recipient keys I need to delete?

What would be the most direct approach to get the actual recipient keys I need to delete?

Note sure. That can be really difficult. We would need to check whether any connection of our of band invitation exist with a did that uses a specific key.... I think for now this is fine. We will probably keep the behaviour as you described until at least didcomm v2 and then we only work with dids rather than keys, so it will be easier to discover I guess.

We also don't delete keys for connectionless exchanges after they're done (can be hard to determine what done means in a generic way). We should probably also add that

TimoGlastra · 2022-12-08T01:47:21Z

packages/core/src/modules/oob/OutOfBandApi.ts

@@ -565,6 +564,20 @@ export class OutOfBandApi {
   * @param outOfBandId the out of band record id
   */
  public async deleteById(outOfBandId: string) {
+    const outOfBandRecord = await this.getById(outOfBandId)
+
+    const relatedConnections = await this.connectionsApi.findAllByOutOfBandId(outOfBandId)


This can become quite an expensive query if you have a lot of connections created. Another reason to support pagination...

Yes... Hopefully, delete operations will not be so common, so the performance impact would not be so big.

TimoGlastra · 2022-12-08T01:48:33Z

packages/core/src/modules/oob/OutOfBandApi.ts

+    const relatedConnections = await this.connectionsApi.findAllByOutOfBandId(outOfBandId)
+
+    // If it uses mediation and there are no related connections, proceed to delete keys from mediator
+    if (outOfBandRecord.mediatorId && (relatedConnections.length === 0 || outOfBandRecord.reusable)) {


Why do we delete the recipientKeys if the oob record is reusable?

If an Out of Band record is reusable, every time a new connection is created based on it, a new recipient key will be created and shared with the mediator. My reasoning is that if we delete the out of band record we will not be affecting any connection created from it, and therefore we are safe to delete that key on mediator. Maybe I'm missing something?

Yes that makes sense, hadn't thought about it. Maybe add it as a comment to the code :)

TimoGlastra · 2022-12-08T01:51:03Z

packages/core/src/modules/routing/RecipientApi.ts

@@ -370,8 +373,22 @@ export class RecipientApi {
    return mediationRecord
  }

-  public async notifyKeylistUpdate(connection: ConnectionRecord, verkey: string) {
-    const message = this.mediationRecipientService.createKeylistUpdateMessage(verkey)
+  public async notifyKeylistUpdate(connection: ConnectionRecord, verkey: string, action?: KeylistUpdateAction) {


Not really related to this PR, but maybe we can update this method to something like:

Suggested change

public async notifyKeylistUpdate(connection: ConnectionRecord, verkey: string, action?: KeylistUpdateAction) {

public async notifyKeylistUpdate({

connectionId: string

publicKeyBase58: string // or even publicKeyFingerprint

action: KeylistUpdateAction // make action required

}) {

}

Agree. I just didn't want to introduce breaking changes at the API level on this PR. But for sure this method must be updated. I would like to do something similar to Discover Features module, where we have the option of doing sync or async queries, in order to have a more harmonized Recipient API

TimoGlastra · 2022-12-08T01:52:50Z

packages/core/src/modules/routing/services/MediationRecipientService.ts

      endpoints: mediationRecord.endpoint ? [mediationRecord.endpoint] : routing.endpoints,
      routingKeys: mediationRecord.routingKeys.map((key) => Key.fromPublicKeyBase58(key, KeyType.Ed25519)),
    }
  }

+  public async removeMediationRouting(
+    agentContext: AgentContext,
+    { recipientKeys, mediatorId }: RemoveRoutingOptions


On the service level we usually use records over ids, not sure if using a record is better here though

True. It was just to have a similar API to addMediationRouting, which also takes mediatorId. Overall I think the API used in this module is quite different from other parts of the framework.

Signed-off-by: Ariel Gentile <[email protected]>

genaris added 7 commits December 1, 2022 15:13

feat(connections): remove key on mediator when deleting a connection

e86a1c0

Signed-off-by: Ariel Gentile <[email protected]>

Merge remote-tracking branch 'upstream/main' into feat/delete-routing…

689ee10

…-keys

style: fix lint error

423ca63

Signed-off-by: Ariel Gentile <[email protected]>

fix: add mediatorId to connections

67a3ff8

Signed-off-by: Ariel Gentile <[email protected]>

fix: don't use default mediator for routing removal

d6c9a07

Signed-off-by: Ariel Gentile <[email protected]>

add this feature to OOB module

be2f1b4

Signed-off-by: Ariel Gentile <[email protected]>

add tests for OOB and connections using mediation

3e0b484

Signed-off-by: Ariel Gentile <[email protected]>

genaris requested a review from a team as a code owner December 7, 2022 03:11

TimoGlastra reviewed Dec 7, 2022

View reviewed changes

packages/core/src/modules/connections/ConnectionsApi.ts Outdated Show resolved Hide resolved

feat: allow to send multiple keys in single message

10b8a69

Signed-off-by: Ariel Gentile <[email protected]>

genaris requested a review from TimoGlastra December 7, 2022 15:21

TimoGlastra reviewed Dec 8, 2022

View reviewed changes

TimoGlastra approved these changes Dec 12, 2022

View reviewed changes

genaris added 5 commits December 12, 2022 10:08

Merge branch 'main' into feat/delete-routing-keys

f87c128

Signed-off-by: Ariel Gentile <[email protected]>

chore: add note about reusable OOB

38838f2

Signed-off-by: Ariel Gentile <[email protected]>

test: accept both did:key and base58 encoding in keylist

4d22ce3

Signed-off-by: Ariel Gentile <[email protected]>

test: accept did:key encoding in oob mediation

b15b039

Signed-off-by: Ariel Gentile <[email protected]>

test: add missing change

cf6ab5d

Signed-off-by: Ariel Gentile <[email protected]>

genaris mentioned this pull request Dec 12, 2022

Mediation Recipient module API improvements #1154

Open

genaris enabled auto-merge (squash) December 12, 2022 14:54

genaris mentioned this pull request Dec 12, 2022

Unable to process requests from multi-use invitations when agent uses mediation #1138

Open

genaris merged commit 1af57fd into openwallet-foundation:main Dec 12, 2022

genaris deleted the feat/delete-routing-keys branch December 12, 2022 15:39

berendsliedrecht mentioned this pull request Jan 18, 2023

fix/transport connection id animo/aries-framework-javascript#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove keys on mediator when deleting connections #1143

feat: remove keys on mediator when deleting connections #1143

genaris commented Dec 7, 2022

codecov-commenter commented Dec 7, 2022 •

edited

Loading

TimoGlastra Dec 8, 2022

genaris Dec 8, 2022

TimoGlastra Dec 12, 2022

TimoGlastra Dec 8, 2022

genaris Dec 8, 2022

TimoGlastra Dec 8, 2022

genaris Dec 8, 2022

TimoGlastra Dec 9, 2022

TimoGlastra Dec 8, 2022

genaris Dec 8, 2022

TimoGlastra Dec 8, 2022

genaris Dec 8, 2022

-  public async notifyKeylistUpdate(connection: ConnectionRecord, verkey: string, action?: KeylistUpdateAction) {
+  public async notifyKeylistUpdate({
+    connectionId: string
+    publicKeyBase58: string // or even publicKeyFingerprint
+    action: KeylistUpdateAction // make action required
+ }) {
+ }

feat: remove keys on mediator when deleting connections #1143

feat: remove keys on mediator when deleting connections #1143

Conversation

genaris commented Dec 7, 2022

codecov-commenter commented Dec 7, 2022 • edited Loading

Codecov Report

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

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

codecov-commenter commented Dec 7, 2022 •

edited

Loading