search

hyperledger / aries-vcx

aries-vcx is set of crates to work with DIDs, DID Documents, DIDComm, Verifiable Credentials and Hyperledger Aries.
https://didcomm.org
Apache License 2.0
122 stars 81 forks source link

[FIX] DIDExchange handlers should do more signature checking #1226

Closed gmulhearn closed 2 months ago

gmulhearn commented 3 months ago

I'm finding that currently, the DIDExchange requester and responder impls are not checking some cryptographic parts of the DIDExchange flow. Particularly:

  1. ~when the inviter/responder receives a request, (DidExchangeResponder::<ResponseSent>::receive_request), then it should check that the wire-level sender of the request is from a key that matches a key of the DIDDoc inside the request. The spec says to do this:~

    ~After receiving the exchange request, the responder evaluates the provided DID and DID Doc according to the DID Method Spec.~ ~The responder should check the information presented with the keys used in the wire-level message transmission to ensure they match.~

~I don't believe that ACAPy does this~

EDIT: descoping this, as the community does not seem to find this case necessary: https://github.com/hyperledger/aries-rfcs/issues/717#issuecomment-1069768955

  1. when the invitee/requester receives a response (DidExchangeRequester::receive_response), then it should check that DIDDoc or the DIDRotate attachment is signed JWS, and is signed with the original inviter key. The spec is clear on this:

    When the requester receives the response message, they will decrypt the authenticated envelope which confirms the source's authenticity. After decryption validation, the signature on the did_doc~attach or did_rotate~attach MUST be validated, if present. The key used in the signature MUST match the key used in the invitation. After attachment signature validation, they will update their wallet with the new information, and use that information in sending the complete message.

ACApy seems to do this

gmulhearn commented 3 months ago

Related to signed did_rotate attachments: https://github.com/hyperledger/aries-vcx/issues/1228

gmulhearn-anonyome commented 2 months ago

Part 2 is fixed in #1232 . I'm still questioning the importance of part 1, as other agents (acapy, credo) don't seem to do this AFAIK

JamesKEbert commented 2 months ago

I don't know that (1) is required as the signatures are not serving any real purpose as I understand it, also (as mentioned here), for instance.

gmulhearn-anonyome commented 2 months ago

yea agreed. ok i'm going to descope it from this issue

gmulhearn commented 2 months ago

Closed by #1232