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

add prerequisites for expansion of connect flow #964

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

shamilovtim
Copy link
Contributor

@shamilovtim shamilovtim commented Oct 23, 2024

Context

The web5 connect flow previously relied on the delegateDid in order to encrypt, sign + verify and move the workflow from step to step.

While this worked for an MVP implementation to get the release out the door, it had a number of issues. The same DID should not have been used for sign/verify as is used for encryption and that same DID should also not be used for persistent delegation.

The code was implemented with TODOs knowing that this would need to be enhanced but the enhancements were slated as security enhancements only.

When the "export" flow was proposed it turned out that this code would need to be fixed immediately. In the export flow the delegation is only transferred at the end of the connect flow. This means that you therefore cannot rely on the delegation for the creation of shared encryption keys.

Therefore fixing this security issue became a priority and a prerequisite for the "export" flow.

Implementation

  1. Here I have implemented the suggested fixes by creating separate DIDs for signing and verification, encryption, and delegation.
  2. I have also mapped out the export flow, which will come in a separate PR.
  3. I cleaned up some comments
  4. I deduped the connect types, which I was able to simplify significantly.
  5. I moved the permissions helper into the initClient method to reduce the amount of boilerplate going on in the Web5 class

Breaking changes for wallet authors.

web5 connect's getAuthRequest() now returns an object which include both the authRequest and a DID:

{
  authRequest: Web5ConnectAuthRequest;
  clientEcdhDid: DidResolutionResult;
}

web5 connect's submitAuthResponse() now requires that the did received from getAuthRequest() is passed in to the method at position 4:

async function submitAuthResponse(
  selectedDid: string,
  authRequest: Web5ConnectAuthRequest,
  randomPin: string,
  clientEcdhDid: DidResolutionResult,
  agent: Web5Agent
) { ... }

Copy link

changeset-bot bot commented Oct 23, 2024

🦋 Changeset detected

Latest commit: 1b396bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@web5/agent Patch
@web5/api Patch
@web5/user-agent Patch
@web5/proxy-agent Patch
@web5/identity-agent Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Oct 23, 2024

TBDocs Report

✅ No errors or warnings

@web5/api

  • Project entry file: packages/api/src/index.ts

@web5/crypto

  • Project entry file: packages/crypto/src/index.ts

@web5/crypto-aws-kms

  • Project entry file: packages/crypto-aws-kms/src/index.ts

@web5/dids

  • Project entry file: packages/dids/src/index.ts

@web5/credentials

  • Project entry file: packages/credentials/src/index.ts

TBDocs Report Updated at 2024-10-24T17:04:00Z 1b396bd

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 95.12761% with 21 lines in your changes missing coverage. Please review.

Project coverage is 93.33%. Comparing base (05e5b64) to head (1b396bd).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #964      +/-   ##
==========================================
- Coverage   93.37%   93.33%   -0.04%     
==========================================
  Files         118      118              
  Lines       33806    33913     +107     
  Branches     2755     2765      +10     
==========================================
+ Hits        31565    31653      +88     
- Misses       2200     2217      +17     
- Partials       41       43       +2     
Components Coverage Δ
agent 87.86% <95.16%> (-0.02%) ⬇️
api 99.50% <95.00%> (-0.12%) ⬇️
common 95.02% <ø> (ø)
credentials 94.95% <ø> (ø)
crypto 93.79% <ø> (ø)
dids 97.77% <ø> (ø)
identity-agent 96.42% <ø> (ø)
crypto-aws-kms 100.00% <ø> (ø)
proxy-agent 96.42% <ø> (ø)
user-agent 96.57% <ø> (ø)

@shamilovtim shamilovtim linked an issue Oct 24, 2024 that may be closed by this pull request
* 5. `export`: (if `exported` is true) client will POST a {@link PortableDid}
* 6. `retrieve`: (if `exported` is true) wallet will GET the {@link PortableDid}
* 7. `export-token`: (if `exported` is true) wallet will POST the grants in order to finalize the flow.
* 8. `retrieve-token`: (if `exported` is true) client will GET the grants in order to finalize the flow.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are all renamed in the next PR to be more clear, so just ignore this part

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

Successfully merging this pull request may close these issues.

wallet connect: use a separate key for ecdh
1 participant