Skip to content
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

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

Closed
gmulhearn opened this issue Jun 13, 2024 · 5 comments
Closed

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

gmulhearn opened this issue Jun 13, 2024 · 5 comments
Assignees

Comments

@gmulhearn
Copy link
Contributor

gmulhearn commented Jun 13, 2024

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: hyperledger/aries-rfcs#717 (comment)

  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_docattach or did_rotateattach 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
Copy link
Contributor Author

Related to signed did_rotate attachments: #1228

@gmulhearn-anonyome
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

JamesKEbert pushed a commit that referenced this issue Jul 4, 2024
…1232)

* move existing into v1_0 section

Signed-off-by: George Mulhearn <[email protected]>

* duplicate types

Signed-off-by: George Mulhearn <[email protected]>

* static generic types for messages created and piped thru all layers

Signed-off-by: George Mulhearn <[email protected]>

* simplify generics

Signed-off-by: George Mulhearn <[email protected]>

* change approach to use runtime versioning rather than generics

Signed-off-by: George Mulhearn <[email protected]>

* v1_1 branch processing, and some clippy

Signed-off-by: George Mulhearn <[email protected]>

* remove old todos

Signed-off-by: George Mulhearn <[email protected]>

* fixes for aath with self for 4/7 performance on RFC0793 & 4/7 on 0023

Signed-off-by: George Mulhearn <[email protected]>

* smalls patches from acapy testing

Signed-off-by: George Mulhearn <[email protected]>

* fix up mimetype handling as a result of testing acapy (text/string)

Signed-off-by: George Mulhearn <[email protected]>

* handle multikey (acapy uses this)

Signed-off-by: George Mulhearn <[email protected]>

* make invite handshake 1.1

Signed-off-by: George Mulhearn <[email protected]>

* include invitation id

Signed-off-by: George Mulhearn <[email protected]>

* pthid in response (for acapy)

Signed-off-by: George Mulhearn <[email protected]>

* merge fix and add hack for local aath testing

Signed-off-by: George Mulhearn <[email protected]>

* fixes for didpeer2

Signed-off-by: George Mulhearn <[email protected]>

* improve VM handling to understand more DIDDoc styles (acapy AATH testing)

Signed-off-by: George Mulhearn <[email protected]>

* fmt

Signed-off-by: George Mulhearn <[email protected]>

* clean switcher

Signed-off-by: George Mulhearn <[email protected]>

* label pass, and some fixes

Signed-off-by: George Mulhearn <[email protected]>

* test fixes

Signed-off-by: George Mulhearn <[email protected]>

* response sign verification mostly impled

Signed-off-by: George Mulhearn <[email protected]>

* fix did rotate content

Signed-off-by: George Mulhearn <[email protected]>

* negative test

Signed-off-by: George Mulhearn <[email protected]>

* small patch re acapy testing

Signed-off-by: George Mulhearn <[email protected]>

* pass in handshake ver

Signed-off-by: George Mulhearn <[email protected]>

* clippy and rebase

Signed-off-by: George Mulhearn <[email protected]>

* unnecessary mockall dep

Signed-off-by: George Mulhearn <[email protected]>

* any-wrapper approach

Signed-off-by: George Mulhearn <[email protected]>

* lint

Signed-off-by: George Mulhearn <[email protected]>

* borrow registries instead

Signed-off-by: George Mulhearn <[email protected]>

* jws testing

Signed-off-by: George Mulhearn <[email protected]>

---------

Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn
Copy link
Contributor Author

Closed by #1232

lukewli-anonyome pushed a commit to lukewli-anonyome/aries-vcx that referenced this issue Jul 25, 2024
…ger#1226 (hyperledger#1232)

* move existing into v1_0 section

Signed-off-by: George Mulhearn <[email protected]>

* duplicate types

Signed-off-by: George Mulhearn <[email protected]>

* static generic types for messages created and piped thru all layers

Signed-off-by: George Mulhearn <[email protected]>

* simplify generics

Signed-off-by: George Mulhearn <[email protected]>

* change approach to use runtime versioning rather than generics

Signed-off-by: George Mulhearn <[email protected]>

* v1_1 branch processing, and some clippy

Signed-off-by: George Mulhearn <[email protected]>

* remove old todos

Signed-off-by: George Mulhearn <[email protected]>

* fixes for aath with self for 4/7 performance on RFC0793 & 4/7 on 0023

Signed-off-by: George Mulhearn <[email protected]>

* smalls patches from acapy testing

Signed-off-by: George Mulhearn <[email protected]>

* fix up mimetype handling as a result of testing acapy (text/string)

Signed-off-by: George Mulhearn <[email protected]>

* handle multikey (acapy uses this)

Signed-off-by: George Mulhearn <[email protected]>

* make invite handshake 1.1

Signed-off-by: George Mulhearn <[email protected]>

* include invitation id

Signed-off-by: George Mulhearn <[email protected]>

* pthid in response (for acapy)

Signed-off-by: George Mulhearn <[email protected]>

* merge fix and add hack for local aath testing

Signed-off-by: George Mulhearn <[email protected]>

* fixes for didpeer2

Signed-off-by: George Mulhearn <[email protected]>

* improve VM handling to understand more DIDDoc styles (acapy AATH testing)

Signed-off-by: George Mulhearn <[email protected]>

* fmt

Signed-off-by: George Mulhearn <[email protected]>

* clean switcher

Signed-off-by: George Mulhearn <[email protected]>

* label pass, and some fixes

Signed-off-by: George Mulhearn <[email protected]>

* test fixes

Signed-off-by: George Mulhearn <[email protected]>

* response sign verification mostly impled

Signed-off-by: George Mulhearn <[email protected]>

* fix did rotate content

Signed-off-by: George Mulhearn <[email protected]>

* negative test

Signed-off-by: George Mulhearn <[email protected]>

* small patch re acapy testing

Signed-off-by: George Mulhearn <[email protected]>

* pass in handshake ver

Signed-off-by: George Mulhearn <[email protected]>

* clippy and rebase

Signed-off-by: George Mulhearn <[email protected]>

* unnecessary mockall dep

Signed-off-by: George Mulhearn <[email protected]>

* any-wrapper approach

Signed-off-by: George Mulhearn <[email protected]>

* lint

Signed-off-by: George Mulhearn <[email protected]>

* borrow registries instead

Signed-off-by: George Mulhearn <[email protected]>

* jws testing

Signed-off-by: George Mulhearn <[email protected]>

---------

Signed-off-by: George Mulhearn <[email protected]>
Signed-off-by: lli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants