kadena-io / pact

The Pact Smart Contract Language
https://docs.kadena.io/build/pact
BSD 3-Clause "New" or "Revised" License
579 stars 100 forks source link

Vendor webauthn dependency #1311

Closed edmundnoble closed 10 months ago

edmundnoble commented 10 months ago

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.


imalsogreg commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

We don't actually have any txs that use webauthn yet because the fork hasn't happened yet :sweat_smile: 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.

emilypi commented 10 months ago

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