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

Vendor webauthn dependency #1311

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Vendor webauthn dependency #1311

merged 1 commit into from
Oct 19, 2023

Conversation

edmundnoble
Copy link
Contributor

@edmundnoble edmundnoble commented Oct 17, 2023

webauthn has been giving us some issues upgrading to GHC 9.8 and they also use cryptonite instead of crypton. The authors of the webauthn package plan to fix these issues but also plan to make major API changes in that same upcoming release. Unfortunately the signature verification function we use from webauthn is internal, so it's possible that it's removed or moved.

There are two things we need from webauthn:

  1. deserializing the webauthn signature, which has a schema that's part of the webauthn spec. It's unlikely that this code has any problems that merit patching, but we definitely don't want this decoder to change from under us because it's for on-chain data, so it should be in Pact.
  2. verifying the webauthn signature, which entails delegating to crypton, which does the actual cryptography. This code is sensitive because it's signature verification, so it belongs in Pact.

webauthn (and jose, which it depends on) have >8000 lines of source Haskell, not including comments etc. If we vendor the relevant parts instead, we only add ~600SLOC, and that's without trying to minimize it.

For the reasons above, in particular stability in the future w.r.t. on-chain serialization and signature verification, I recommend that we vendor the webauthn dependency, which this PR does.

  • replay successful

@imalsogreg
Copy link
Contributor

This approach seems reasonable to me after reading your analysis. My only concern would be that we should somehow periodically watch Tweag's implementation for security patches and bugfixes, which we will need to apply to our fork.

@edmundnoble
Copy link
Contributor Author

Yes we'll have to downstream any security patches or bugfixes. From what I can tell though, chances are, none come our way. The deserialization code isn't subject to buffer overflows or anything like that because it uses cborg; the verification code just branches on the type of signature and calls into crypton, which is probably the thing that gets security patches.

@emilypi
Copy link
Member

emilypi commented Oct 17, 2023

Thanks @edmundnoble I agree that this is a good thing to vendor. If replay is successful, I'm okay with giving the go-ahead.

@edmundnoble
Copy link
Contributor Author

We don't actually have any txs that use webauthn yet because the fork hasn't happened yet 😅 so a replay won't catch any issues. There are tests for it in Pact though. I suggest merging with CI (for some reason CI isn't passing, I don't see what's going on), and the testing process next release will catch any inconsistencies that aren't encapsulated by the code being a copy+paste or the tests we have.

Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

Approved

@emilypi
Copy link
Member

emilypi commented Oct 18, 2023

Needs to be followed up with a replay but we'll catch that later, same as we did with crypton.

@edmundnoble edmundnoble merged commit 9d8d097 into master Oct 19, 2023
9 of 13 checks passed
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.

4 participants