openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
421 stars 515 forks source link

Qualified Peer DIDs responder not respecting requesters DID Peer Method #2735

Closed nodlesh closed 8 months ago

nodlesh commented 10 months ago

I am creating a DID Exchange connection between a requester and responder ACA-Py agents. The wallet type for both agents is askar-annoncreds. the requester is set to emit did:peer:2 DIDs, and the responder is set to emit did:peer:4. The scenario expects the responder to respond in kind to did:peer:2 even though emit did:peer:4 is set. Actually the expectation is, no matter what the responder is set to emit, they should always respond in kind.

Result: In the response to the request, the requester's DID is a did:peer:4 instead of a did:peer:2. However the connection can still be completed.

If this scenario is reversed, the requester is set to did:peer:4 and the responder is set to did:peer:2, then the responder will respond properly with a did:peer:4 did.

As a side note, and this may relate to the issue, initially I expected that the inviter might set the stance for which DID peer method to use. So if the responder set to did:peer:2 creates the invitation and sends to the requester, the method used for the protocol would be did:peer:2. However it doesn’t seem to work like that. It seems that whomever sends the request after receiving the invitation is the one who sets the stance for the DID peer method. Is this expected?

AATH test that manifests this is T003-RFC0793, https://github.com/hyperledger/aries-agent-test-harness/blob/b53e8e495ec9553fd1b473d5418f3cef93c9be72/aries-test-harness/features/0793-peer-did.feature#L73

swcurran commented 10 months ago

I’m not sure that the responder has to respond in kind. Responding in kind would be more likely to succeed (since we know the other side “understands” that DID type…), but is not really required.

@dbluhm — what do you think? Should we add a requirement to the did:peer spec and ACA-Py to respond in kind? Or perhaps that requirement already exists and I don’t know about it?

nodlesh commented 10 months ago

If the response in kind isn't a requirement then I'll have to calibrate the test sceanrios not to check that. I used that as one way to tell if things were working correctly and to pass/fail the test based on that. The tests would then just make sure there is a successful connection and move on.

dbluhm commented 10 months ago

@swcurran It is not currently required in any specification I'm aware of to respond with a DID of the same method or, in the case of did:peer, variety. I'm reluctant to add something like that to the spec based on my observations so far. As long as both sides of the message exchange are able to resolve the DID, which DID method shouldn't matter. This is more or less the goal of having various DID methods to begin with, I'd say.

In practice, it makes sense to respond in kind, just to narrow the variety of options but I'd leave this up to the implementation to decide how they wanted to treat it. What ACA-Py is doing is essentially a respond-in-kind already; the reason the inviter can't set the expectation is because at the invitation phase, no DIDs have been created yet (just a plain old key) so there's no suit for the invitee to follow.

This inconsistency in connection establishment should be resolved by DIDComm v2's approach to things.

nodlesh commented 10 months ago

It is reasonable then, for the tests to just check if the DID is a qualified DID? As in `assert DID starts with did:peer", with no number.

swcurran commented 10 months ago

If it is specifically a “did:peer” test then that makes sense. If not, it should be required that it is a qualified DID (“did:” prefix) and no more. We’re running into problems in places where we have been too tight in verifying for types of DIDs that were in play at the time, and then expanding the permitted DID types.

dbluhm commented 8 months ago

This should now be resolved.