fix(credo-interop): various didexchange and did:peer related fixes

[dbluhm]

Fixes #2742 and various other issues discovered while testing ACA-Py against AFJ.

• Send and accept didexchange/1.1 as handshake protocol in OOB
• Credo did not like the fact that we were including both jwk and kid in the protected headers of signed attachments. The kid was redundant so I removed it.
• Credo does not accept Multikey verification method type yet (I opened a PR though: feat: add Multikey as supported vm type credo-ts#1720 -- tests failing 😄). So I adjusted our did:peer:4 doc creation to use Ed25519VerificationKey2020 for now. Multikey is now supported by Credo so I'll revert this change.

Updated Summary of Changes

Fixes

• Askar is_transaction check throwing NoneType exception when no session is open
• DID Peer 4 Document creation assigns its key to authentication and keyAgreement verification relationships.
• OOB clean_finished_oob_record was suppressing exceptions from some bad code. OOB Records were not getting correctly deleted because the delete_record call was outside of the context holding the session. Exceptions are now logged as a warning from that block of code.
• Removed kid from the headers of signed attachments; this was an overspecification -- either jwk or kid are expected but not both.

Removals

• DIDCommPrefix environment variable manipulation. Given that we no longer support sending the old prefix, it was no longer needed. It's presence made some unit tests intermittently fail depending on test execution order.
• ConnRecord.Protocol Enum was removed in favor of a simpler SUPPORTED_PROTOCOLS list. The logic surrounding the Enum was convoluted and barely used. I opted to simplify.
• (Commented out) code for sending problem report warnings on version mismatch. As noted in the comments below, these warnings are no longer recommended.
• All aries_cloudagent/core/util.py methods associated with version definition loading and parsing. This was an error prone piece of code (that recently broke loading protocols from plugins altogether) that was responsible for determining what the "correct" version of a protocol to respond with given the version definition for the protocol. It did this by dynamically loading the definition.py module for the message package through a very brittle process of guessing at the location of the module relative to the module containing the agent message class in question. However, all this information was already known to the ProtocolRegistry; there is no need to dynamically load it. Additionally, the support for emitting warnings that it enabled is no longer recommended by the RFCs. These methods were removed and replaced with calls to the ProtocolRegistry.

Refactors

Protocol Registry and Dispatcher make_message

The Protocol Registry had seen only minor updates since it was first written years ago. I found the code to be a little brittle, depending a little too heavily on loosely defined lists of dictionaries. I formalized the data structures used to resolve a message class from a message type and introduced some helper types for parsing information from message type strings (MessageType, ProtocolIdentifier, MessageVersion, etc.).

The refactors correct the issue discussed in the comments of this PR regarding past versions of ACA-Py being unable to handle minor versions greater than the current minor version within a major version of a protocol.

These refactors had some minor effects on the Dispatcher make_message

Specifics:

• registry.resolve_message_class now returns either a Type[AgentMessage] or DeferLoad of an AgentMessage. I think the DeferLoad just wasn't around when the ProtocolRegistry was first written because it was the perfect tool for the job. This results in Message Classes being cached after first load so they don't have to be dynamically imported every time. I didn't measure but I suspect this has a positive impact on performance.
• Dispatcher's make_message no longer returns a "warning" string (see removal note above).
• register_controllers no longer takes a version_definition as a parameter (it was unused)
• ProtocolDefinitions, a combination of the protocol identifier URI and the information from the version definition (definition.py in the protocol package) are now stored in the registry. All information about currently supported versions is stored in the registry (see note about removal of core/util.py version definition methods above).
• Streamlined message type registration:
  • No longer takes a list of typesets, just one flat typeset. The list of typeset functionality was unused and just made for unreadable code.
  • Other improvements that aren't worth noting the exact details of.
• resolve_message_class will return the message class for the current supported minor version for minor versions greater than what is supported.

AgentMessage

Some minor updates were made to AgentMessage to make it easier to access and update message version information:

• AgentMessage._type is now a MessageTypeStr instance. It behaves like a str but has additional properties for protocol, version, name, etc.
• New AgentMessage._version property for accessing AgentMessage._type.version as a string
• assign_version_from(msg: AgentMessage) to set the version of the current message based on the version of another message.
• assign_version(version: str) for manually setting the version to a value.
• Removed the @property.setter for the _type attribute which was unused and had dubious utility.

DID Exchange Manager

Behavior of the manager is now governed by the connection_protocol property of the ConnRecord associated with the exchange. If the protocol is didexchange/1.0 the original DID Exchange manager behavior is executed for backwards compatibility with previous versions of ACA-Py. If the protocol is didexchange/1.1, the manager will execute the new behavior, including qualified DIDs and did_rotate~attach. If ACA-Py's settings do not permit qualified DIDs (through omission of emit-did-peer-X flags on startup), the manager will default to the original unqualified DID behavior.

The manager will send messages with the same versions as those it receives. E.g. didexchange/1.0/request received, didexchange/1.0/response sent. didexchange/1.1/request received, didexchange/1.1/response sent.

Using a public DID for a DID Exchange now requires didexchange/1.1 support since the did_rotate~attach mechanism is used.

Various refactors were made to streamline logic and make it easier to reuse chunks.

Added a protocol parameter to implicit DID Exchange request Admin API endpoint. This endpoint triggers a request to a public DID without previously receiving an invitation from that DID. The protocol parameter selects which version of DID Exchange to use, 1.1 or 1.0. The default is 1.0 for now.

Deprecated: An Admin API endpoint that was apparently broken and unused was marked as deprecated: POST /didexchange/receive-request. The docs for the endpoint describe that it is intended to be used to receive a request against a public DID. However, requests to a public DID should be sent directly to the messaging endpoint of the public DID. Given this and the fact that errors in the endpoint rendered it useless (until fixed in this PR), I marked the endpoint as deprecated.

Out of Band Manager and Invitation message

Minor updates were made:

The manager passes the version of the handshake protocol used to the DID Exchange manager so that it can keep track of this in the connection_protocol attribute of the ConnRecord created.

The invitation message (and relevant Admin API endpoints) now accept didexchange/1.1 as a handshake protocol value.

The HSProto and HSProtoSpec were also streamlined to remove unused features. Additionally, rfc as a method for referencing an HSProto was removed given that DID Exchange 1.0 and 1.1 are both from the same RFC.

[sonarcloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[dbluhm]

@genaris just FYI, with these changes, ACA-Py and AFJ can now successfully complete OOB + DIDExchange with did:peer:4 (or AFJ emitting did:peer:1 and ACA-Py emitting did:peer:4, I believe). I commented on an issue in Credo on issues with did:peer:2 and tagged you. No promises but I may take a stab at updating the did:peer:2 resolution to match the updates to the spec. Timo made the very valid point on that issue that this would cause issues with backwards compatibility. So I think focusing on did:peer:4 is fair for now! Sorry for the noise!

[genaris]

Wow nice work @dbluhm ! Not only fixes in ACA-Py but also in Credo. That's what I'm talking about! 😄

Now that your Multikey support PR is merged in Credo main branch, I guess you can re-attempt using it.

[dbluhm]

I haven't gotten this quite right yet. Checking DID Exchange backwards compatibility with earlier versions of ACA-Py, I get this error:

alice_1             | Traceback (most recent call last):
alice_1             |   File "/root/.cache/pypoetry/virtualenvs/aries-cloudagent-VA82Wl8V-py3.9/lib/python3.9/site-packages/aiohttp/web_protocol.py", line 452, in _handle_request
alice_1             |     resp = await request_handler(request)
alice_1             |   File "/root/.cache/pypoetry/virtualenvs/aries-cloudagent-VA82Wl8V-py3.9/lib/python3.9/site-packages/aiohttp/web_app.py", line 543, in _handle
alice_1             |     resp = await handler(request)
alice_1             |   File "/root/.cache/pypoetry/virtualenvs/aries-cloudagent-VA82Wl8V-py3.9/lib/python3.9/site-packages/aiohttp/web_middlewares.py", line 114, in impl
alice_1             |     return await handler(request)
alice_1             |   File "/usr/src/app/aries_cloudagent/admin/server.py", line 181, in ready_middleware
alice_1             |     return await handler(request)
alice_1             |   File "/usr/src/app/aries_cloudagent/admin/server.py", line 218, in debug_middleware
alice_1             |     return await handler(request)
alice_1             |   File "/root/.cache/pypoetry/virtualenvs/aries-cloudagent-VA82Wl8V-py3.9/lib/python3.9/site-packages/aiohttp_apispec/middlewares.py", line 45, in validation_middleware
alice_1             |     return await handler(request)
alice_1             |   File "/usr/src/app/aries_cloudagent/admin/server.py", line 451, in setup_context
alice_1             |     return await task
alice_1             |   File "/usr/local/lib/python3.9/asyncio/futures.py", line 284, in __await__
alice_1             |     yield self  # This tells Task to wait for completion.
alice_1             |   File "/usr/local/lib/python3.9/asyncio/tasks.py", line 328, in __wakeup
alice_1             |     future.result()
alice_1             |   File "/usr/local/lib/python3.9/asyncio/futures.py", line 201, in result
alice_1             |     raise self._exception
alice_1             |   File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
alice_1             |     result = coro.send(None)
alice_1             |   File "/usr/src/app/aries_cloudagent/protocols/out_of_band/v1_0/routes.py", line 296, in invitation_receive
alice_1             |     result = await oob_mgr.receive_invitation(
alice_1             |   File "/usr/src/app/aries_cloudagent/protocols/out_of_band/v1_0/manager.py", line 509, in receive_invitation
alice_1             |     oob_record = await self._perform_handshake(
alice_1             |   File "/usr/src/app/aries_cloudagent/protocols/out_of_band/v1_0/manager.py", line 883, in _perform_handshake
alice_1             |     raise OutOfBandManagerError(
alice_1             | aries_cloudagent.protocols.out_of_band.v1_0.manager.OutOfBandManagerError: Unable to create connection. Could not perform handshake using any of the handshake_protocols (supported [None])

The updates for the DID Exchange version were inadvertently breaking. I'll have to look deeper. My first pass was not much more than a copy-paste from OOB.

[dbluhm]

I've spent a fair amount of time sifting through the behavior of ACA-Py as it relates to handling messages from protocol versions different than the explicitly supported versions.

What should happen, according to RFC 0003, is that if a message with a minor version greater than what is supported is received, we handle the message still and respond anyways with our lower protocol version. Through this, the response recipient knows that there is a version mismatch and that some fields may have been ignored.

For example, if we get didexchange/1.1/request but we only support didechange/1.0/request explicitly, we will do our best to handle the message and respond with didexchange/1.0/response.

As it turns out, recent past releases of ACA-Py emit this error on receipt of a didexchange/1.1/request:

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
    result = coro.send(None)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/dispatcher.py", line 142, in handle_message
    (message, warning) = await self.make_message(
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/dispatcher.py", line 293, in make_message
    message_cls = registry.resolve_message_class(message_type)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/protocol_registry.py", line 243, in resolve_message_class
    return ClassLoader.load_class(msg_cls)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/utils/classloader.py", line 98, in load_class
    if "." in class_name:
TypeError: argument of type 'NoneType' is not iterable

This essentially means that a new ACA-Py release would be incompatible with past versions of ACA-Py when using DID Exchange.

I could adjust our behavior as a newer version of ACA-Py to attempt to respond with 1.0 still but this, I think, would technically be out of spec, on top of being somewhat of a pain. Realistically, breaking backwards compatibility would probably cause more pain though.

Thoughts? @swcurran @ianco

[swcurran]

I’m confused. Would we not want to add support for 1.1 and handle both 1.0 and 1.1? Presumably we want to support did_rotate~attach? The 003 RFC says that is the ideal — respond with the same version as received.

[dbluhm]

Within a given major version of a protocol, an agent should:

• Respond to a minimum supported minor version, defaulting to "0".
  • An agent SHOULD keep minimum supported minor version at "0" unless it is unsecure or extremely complicated to do so.
• Respond with or initiate a protocol instance the current fully supported minor version.

The way I interpret that second point from the semver rules, it seems to suggest that we should always respond with our "most current" minor version. By doing so, we make it clear to the other party that they may experience feature degradation or minor version mismatch since the language describing sending a problem report with a warning has been removed from the protocol:

Prior wording of this protocol included the following suggestion that is now considered deprecated. See note below about deprecating the "warning" problem reports.

In addition to responding with the protocol message (if necessary), the agent MAY also want to send a warning problem-report message with code version-with-degraded-features.

And:

Prior wording of this protocol included the following that is now considered deprecated. See note below about deprecating the "warning" problem reports.

In addition to responding with the protocol message, an agent MAY send a warning problem-report message with code fields-ignored-due-to-version-mismatch

Note: The deprecation of the "warning" problem-reports in cases of minor version mismatches is because the recipient of the response can detect the mismatch by looking at the PIURI, making the "warning" unnecessary, and because the problem-report message may be received after (and definitely at a different time than) the response message, and so the warning is of very little value to the recipient. Recipients should still be aware that minor version mismatch warning problem-report messages may be received and handle them appropriately, likely by quietly ignoring them.

[dbluhm]

I might be splitting hairs 😅 I think the ideal would be to only "implement" a single minor version of any major version of a protocol. Honestly wondering if DID Exchange 1.1 should have been DID Exchange 2.0 because, though did_rotate is technically optional, it will break handling of the messages still for did_doc~attach to be absent.

[swcurran]

Sorry — I’m not understanding the issue. Probably we need to discuss. It would be good to get a clear outline of what is happening, what could be done easily and what you recommend we do.

• ACA-Py current minor version is…
  • Should we change that?
• Receive DID Exchange 1.0 request
  • ACA-Py currently …
  • ACA-Py should be change to ...
• Receive DID Exchange 1.1 request
  • ACA-Py currently…
  • ACA-Py should be changed to …

I think the ideal is that ACA-Py be changed to have a current minor version of 1.1 and handle both 1.0 and 1.1 requests. However, I gather that is problematic…

Thanks!

[dbluhm]

Yeah, okay, allow me to slow down and take a deep breath for a moment lol.

The ideal: ACA-Py always just sends didexchange/1.1 since that reduces complexity. However, this would break backwards compatibility. Theoretically, the way DIDComm protocol versions should be handled, past versions of ACA-Py should have been able to work with 1.1; but, due to a bug in past versions, it cannot handle versions of the protocol greater than its "current minor version" (1.0).

(As much as it chagrins me to have explicit different paths for the same major version of a protocol rather than being able to just ignore some attributes) the right answer here is to support both 1.0 and 1.1, responding in kind to requests. I believe it should be possible to include both didexchange/1.0 and didexchange/1.1 in the OOB handshake_protocol list.

My biggest concern is just managing complexity in the DID Exchange protocol implementation. It's already quite dense. I'll continue to work to find a balance between just getting the dang feature implemented and not doing things that will make me sad later lol.

[genaris]

After looking at @swcurran's clarification PR in Aries RFC, I realized that the DID Exchange version was not explicitly changed on its RFC. Some time ago it was agreed to do so (see hyperledger/aries-rfcs#795 (comment)) and we reflected that into Credo implementation, but now I'm not sure if it's clear for everyone.

I think it's the right thing to do although this makes things harder to implement in ACA-Py due to the aforementioned bug in previous versions. BTW, do you think if as a workaround (until unqualified dids support is completely removed) it could be possible to use discover-features to query for DID Exchange support and, based on that, send 1.1 or 1.0 requests? I guess it would be tricky, since a connection is not established yet, but maybe possible somehow?

[dbluhm]

After looking at @swcurran's clarification PR in Aries RFC, I realized that the DID Exchange version was not explicitly changed on its RFC.

https://github.com/hyperledger/aries-rfcs/blob/main/features/0023-did-exchange/README.md#11----signed-rotations-without-did-documents

It looks like the version bump was made but that just the title and metadata didn't get updated. Or at least that's how I read the intent and my memory of the discussions.

BTW, do you think if as a workaround (until unqualified dids support is completely removed) it could be possible to use discover-features to query for DID Exchange support and, based on that, send 1.1 or 1.0 requests? I guess it would be tricky, since a connection is not established yet, but maybe possible somehow?

We've got a few entry points to a DID Exchange protocol:

• We create an OOB invitation with the handshake protocol
• Received OOB invitation with Handshake protocol declared
• We receive a DID Exchange request through a public DID
• We send a DID Exchange request through a public DID

When we create an OOB invitation, we can include both didexchage/1.0 and didexchange/1.1 in the protocol list. We'll receive a request from them with one version or the other and then proceed from that point with the same version they sent.

When we receive an OOB invitation, if didexchange/1.1 is in the protocol list, we'll use it. Otherwise, we'll go to 1.0.

When we receive a DID Exchange request through a public DID, we will just match the version received in the request.

When we send a DID Exchange request, we could potentially do a discover features protocol with the public DID (I don't think ACA-Py technically supports this right now since there's no connection yet)... But to this point, we've left it up to the controller to orchestrate the flow of multiple protocols. So continuing that pattern, I think we'd add a switch to the request through public DID Admin API endpoint to give the controller the ability to pick which version of the DID Exchange protocol to send. We can default to 1.0.

[dbluhm]

A lot of progress on this PR. I think we're where we wanted to be implementation wise now. I just need to update the tests that are failing and do some regression checks against Credo to make sure that's still working, too.

[swcurran]

@jamshale @amanji @ianco — nice to get some eyes on this one. If we can get this merged, we can get an rc3 out, and an official 0.12.0. That would be good!

[dbluhm]

Unit tests fixed now. Might be room for some additions to the tests. I've tested this branch against Credo 0.5.0 and tests passed so looking good on that front. I'll run some tests against previous ACA-Py versions.

[dbluhm]

Diagnosing an issue:

acapy_1  | Traceback (most recent call last):
acapy_1  |   File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
acapy_1  |     result = coro.send(None)
acapy_1  |   File "/usr/src/app/aries_cloudagent/core/dispatcher.py", line 210, in handle_message
acapy_1  |     await handler(context, responder)
acapy_1  |   File "/usr/src/app/aries_cloudagent/protocols/didexchange/v1_0/handlers/complete_handler.py", line 29, in handle
acapy_1  |     await mgr.accept_complete(context.message, context.message_receipt)
acapy_1  |   File "/usr/src/app/aries_cloudagent/protocols/didexchange/v1_0/manager.py", line 1064, in accept_complete
acapy_1  |     conn_rec.my_did = await self.long_did_peer_4_to_short(conn_rec.my_did)
acapy_1  |   File "/usr/src/app/aries_cloudagent/connections/base_manager.py", line 115, in long_did_peer_4_to_short
acapy_1  |     await wallet.store_did(did_info)
acapy_1  |   File "/usr/src/app/aries_cloudagent/wallet/askar.py", line 279, in store_did
acapy_1  |     raise WalletDuplicateError("DID already present in wallet")
acapy_1  | aries_cloudagent.wallet.error.WalletDuplicateError: DID already present in wallet

[dbluhm]

@ianco I seem to be running amok of the reusing did:peer:4 changes. My tests work if I make sure to set the create_unique_did query param on OOB invite creation. Otherwise, it would reuse the same DID for subsequent connections. Is this the intended behavior? It seems like inverting the create_unique_did so it's always true would more closely mimic the original behavior of the OOB create invite endpoint.

[ianco]

@ianco I seem to be running amok of the reusing did:peer:4 changes. My tests work if I make sure to set the create_unique_did query param on OOB invite creation. Otherwise, it would reuse the same DID for subsequent connections. Is this the intended behavior? It seems like inverting the create_unique_did so it's always true would more closely mimic the original behavior of the OOB create invite endpoint.

That was based on this comment from @swcurran : #2703 (comment)

I don't have an opinion on what should be the default on the create_unique_did attribute ...

(Connection reuse won't work unless we use the same DID on each invitation ...)

[ianco]

Diagnosing an issue:

acapy_1  | Traceback (most recent call last):
acapy_1  |   File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
acapy_1  |     result = coro.send(None)
acapy_1  |   File "/usr/src/app/aries_cloudagent/core/dispatcher.py", line 210, in handle_message
acapy_1  |     await handler(context, responder)
acapy_1  |   File "/usr/src/app/aries_cloudagent/protocols/didexchange/v1_0/handlers/complete_handler.py", line 29, in handle
acapy_1  |     await mgr.accept_complete(context.message, context.message_receipt)
acapy_1  |   File "/usr/src/app/aries_cloudagent/protocols/didexchange/v1_0/manager.py", line 1064, in accept_complete
acapy_1  |     conn_rec.my_did = await self.long_did_peer_4_to_short(conn_rec.my_did)
acapy_1  |   File "/usr/src/app/aries_cloudagent/connections/base_manager.py", line 115, in long_did_peer_4_to_short
acapy_1  |     await wallet.store_did(did_info)
acapy_1  |   File "/usr/src/app/aries_cloudagent/wallet/askar.py", line 279, in store_did
acapy_1  |     raise WalletDuplicateError("DID already present in wallet")
acapy_1  | aries_cloudagent.wallet.error.WalletDuplicateError: DID already present in wallet

Is this happening in an integration test?

Possibly aca-py should just check before adding a DID, if the DID is already present ...

[swcurran]

To encourage connection reuse where possible, we want an Agent to use the same DID for invitations. The recommendation is that ACA-Py should default to enabling reuse, and a controller must go out of its way to force a new connection for every invitation. Does that make sense and answer the question?

[dbluhm]

Okay, I think I'm detecting where my assumptions have broken down. To restate, we want to encourage connection reuse by using the same DID for invitations; then, in the process of connecting, the invite creator should rotate out for a new did:peer:4 DID.

Sound right? I think some of my changes are erroneously assuming that a DID associated with the connection means we don't need to create a new one. I believe this was the case before the reuse PR. But with this new condition, I'll have to adjust my logic to make sure the new DID is rotated in for the connection.

[swcurran]

Yes — that’s what I think we want. Use the same did:2/4 for every invitation, enabling reuse, and rotate to a new DID when connecting.

And in the super edge case where an Agent wants to actively prevent connection reuse, they have that option to use a new DID on every invitation and (presumably) still rotate to a new DID when connecting.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[dbluhm]

(Please note my updated summary of changes in the opening comment of this PR)

As a result of these changes, in combination with previously merged PRs, backwards compatibility is influenced by the following factors:

Past == ACA-Py 0.11.0
New == ACA-Py 0.12.0 or ACA-Py with the changes from main + this PR

• New can NOT set --emit-did-peer-4 or --emit-did-peer-2 and initiate a connection via invitation since this would result in OOB invitations with a did:peer:4 did as a service which is unresolvable to Past.
  • Therefore, New cannot initiate via invitation DID Exchange 1.1 and did:peer:4 with agents supporting it AND interoperate with Past at the same time.
• New can accept invitations from Past and respond in kind
• New can accept requests from Past and respond in kind
• New can accept invitations from New (or equivalent featured agent) and respond in kind (e.g OOB invite with HS didexchange 1.1) but cannot create a did:peer:2/4 without the flags.
  • Therefore, New cannot respond to an invitation from New with did:peer:2/4 AND interoperate with Past at the same time.
• New can accept requests from New (or equivalent featured agent) and respond in kind (with did:peer:2/4 regardless of flag state)

Are these interop constraints we're happy with or should we revisit what triggers the OOB behavior?

[ianco]

A lot of the details are over my head but overall looks good to me.

[swcurran]

w00t!!! Thanks, @dbluhm — awesome work!