-
Notifications
You must be signed in to change notification settings - Fork 200
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
feat: did rotate #1699
feat: did rotate #1699
Conversation
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Just to comment on what I think we'll end up doing in ACA-Py on the question of when to delete keys, ACA-Py has a cache for mapping from inbound keys to a did then to a connection. The cache has a TTL of one hour by default. So I think we'll commit the changes immediately and then let the cache handle any potentially straggling out-of-order messages. We'll see how that works out in implementation. |
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers, just some nits
const message = await this.didRotateService.createRotate(this.agentContext, { | ||
connection, | ||
did, | ||
routing: routing || (await this.routingService.getRouting(this.agentContext, {})), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to create routing when we have a toDid specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If toDid
is specified, routing is disallowed (the same thing as we did in #1550).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes if routing is not provided we will go to || (await this.routingService.getRouting(this.agentContext, {})),
, so it willl still be generated right? I think we shouldn't call getRouting
in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm under a heavy jetlag right now but I think that case will not happen, as a few lines before that we have:
if (toDid && routing) {
throw new AriesFrameworkError(`'routing' is disallowed when defining 'did'`)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but if routing
is not defined, this will not throw an error. Then later on it will do routing || (await this.routingService.getRouting(this.agentContext, {}))
. Routing will never be defined when toDid
is defined, so that will mean it will call this.routingService
.
I think || (await this.routingService.getRouting(this.agentContext, {}))
should ONLY be called if toDid
is not defined.
if (toDid && routing) {
throw new AriesFrameworkError(`'routing' is disallowed when defining 'did'`)
}
let routingToUse = routing
if (!toDid && !routing) {
routingToUse = await this.routingService.getRouting(this.agentContext, {})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something here, but I think your influenced by the jetlag 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh I see. It is creating an unnecessary routing 🤦. Thanks for the explanation, I think I should have some rest 😄.
|
||
for (const previousDid of connection.previousDids) { | ||
const did = await this.didResolverService.resolve(this.agentContext, previousDid) | ||
if (!did.didDocument) break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the did doc also be removed? Once we support dids othee than did:peer we will need to update this probably as you may not want to remove your public did (we could pass in a method array or something?)
removePreviousDids({ methods: ['key', 'peer', 'jwk'] })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moment the method is only intended to remove the relationship between the connection and its previous dids, meaning that records will not be deleted. This is actually the same thing that happens when we delete a connection.
IMHO we should revisit this process in order to clean up the underlying keys as well, but as you say, now we are supporting more DID methods and flows we should carefully analyze when this data is safe to be removed.
packages/core/src/modules/connections/__tests__/did-rotate.e2e.test.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/connections/handlers/DidRotateAckHandler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/connections/handlers/DidRotateHandler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/connections/services/DidRotateService.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: deliver messages individually, not fetching from the queue every time Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: revert to free runners (openwallet-foundation#1662) Signed-off-by: Ry Jones <ry@linux.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: create settings.yml (openwallet-foundation#1663) Signed-off-by: Ry Jones <ry@linux.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: fix ci and add note to readme (openwallet-foundation#1669) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> docs: update active maintainers (openwallet-foundation#1664) Signed-off-by: Karim Stekelenburg <karim@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: did:peer:2 and did:peer:4 support in DID Exchange (openwallet-foundation#1550) Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(presentation-exchange): added PresentationExchangeService (openwallet-foundation#1672) Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: removed jan as maintainer (openwallet-foundation#1678) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> ci: change secret (openwallet-foundation#1679) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: add meta to rxjs timeout errors (openwallet-foundation#1683) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> build(deps): bump ws and @types/ws (openwallet-foundation#1686) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> build(deps): bump follow-redirects from 1.15.2 to 1.15.4 (openwallet-foundation#1694) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: update shared components libraries (openwallet-foundation#1691) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix: properly print key class (openwallet-foundation#1684) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(present-proof): add support for aries RFC 510 (openwallet-foundation#1676) Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io> Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix(present-proof): isolated tests (openwallet-foundation#1696) Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(indy-vdr): register revocation registry definitions and status list (openwallet-foundation#1693) Signed-off-by: Ariel Gentile <gentilester@gmail.com> chore: rename to credo-ts (openwallet-foundation#1703) Signed-off-by: Ry Jones <ry@linux.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> ci: fix git checkout path and update setup-node (openwallet-foundation#1709) Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix: remove check for DifPresentationExchangeService dependency (openwallet-foundation#1702) Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> docs: update zoom meeting link (openwallet-foundation#1706) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> refactor(oob)!: make label optional (openwallet-foundation#1680) Signed-off-by: Timo Glastra <timo@animo.id> Co-authored-by: Ariel Gentile <gentilester@gmail.com> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: support short legacy connectionless invitations (openwallet-foundation#1705) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat(dids)!: did caching (openwallet-foundation#1710) feat: did caching Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> fix: jsonld document loader node 18 (openwallet-foundation#1454) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> build(deps): bump amannn/action-semantic-pull-request from 5.3.0 to 5.4.0 (openwallet-foundation#1656) build(deps): bump amannn/action-semantic-pull-request Bumps [amannn/action-semantic-pull-request](https://github.com/amannn/action-semantic-pull-request) from 5.3.0 to 5.4.0. - [Release notes](https://github.com/amannn/action-semantic-pull-request/releases) - [Changelog](https://github.com/amannn/action-semantic-pull-request/blob/main/CHANGELOG.md) - [Commits](amannn/action-semantic-pull-request@v5.3.0...v5.4.0) --- updated-dependencies: - dependency-name: amannn/action-semantic-pull-request dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com> feat: did rotate (openwallet-foundation#1699) Signed-off-by: Ariel Gentile <gentilester@gmail.com> refactor: pickup protocol method names Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com> Signed-off-by: Karim Stekelenburg <karim@animo.id>
Adds support for Aries RFC 0794.
API changes in Connections module are quite simple:
Former and new dids' keys will be marked as tags for the connection, in order to accept messages that may be received out of order (e.g. a message using previous keys after receiving the acknowledge)
There are also some fixes related to the changes introduced in #1550, related to cases where external dids are specified.
For the moment, when a rotation is being done, previous dids related to the connection are not removed, meaning that it is still possible to receive messages using former keys. There is a new API method called removePreviousDids that allows to explicitly remove any previous did associated to a given connection.
We should also consider that the keys related to dids are not actually being deleted from the wallet (this is something pending from general connection deletion flows), which is something that certainly would be nice to add in a further PR.