webauthn signatures by imalsogreg · Pull Request #1193 · kadena-io/pact

imalsogreg · 2023-04-14T22:51:33Z

This PR adds WebAuthn as a PPKScheme, allowing WebAuthn signatures to be used for user authentication in pact commands.

It also removes some things that were hard to maintain alongside WebAuthn:

Associated types for pub/private key and signature in the Scheme class are removed.
SomeKeyPair is removed.
All methods in the Scheme class except _valid are removed.
The ETH scheme and all its support code were removed.

Removing those things has several consequences:

Callsites of the removed Scheme class switch to monomorphic versions, assuming ED25519
The format-address repl builtin is removed (it relied on a deleted Scheme method)

The removed methods in Scheme are primarily used in client code, for instance when Pact generates a .yaml command file, or when we generate keys in tests. This helps justify the simplification of Scheme, since we're primarily impacting client code. The server-oriented code paths are relatively unchanged.

Implementation

A signature is valid as a WebAuthn signature if

it is formatted as a JSON object with a few required fields: clientDataJSON, authenticatorData, and signature
These pieces can combine according the the WebAuthn spec to produce a signature valid for the public key, and
The PactHash embedded in the clientDataJSON (where it is named "challenge") is the same as the PactHash of the Command.

Clients may issue transactions with webauthn signatures by stringifying a JSON blob according to the spec. An example of such a blob is included in the unit tests.

base64-bytestring upgrade

The webauthn library requires base64-bytestring > 1.0, and pact requires base64-bytestring < 1.0, so this PR also bumps the base64-bytestring library in pact (which will require a bump in chainweb-node, too).

Bumping base64-bytestring is difficult because the new version produces different error messages than the old version (and in some cases, the new version rejects base64-encoded messages that were accepted by old versions). These error messages end up on chain, so to get hash compatibility for historical blocks, we must emulate the old base64-bytestring's behavior. We encountered this issue only in the decodeBase64 native and the str-to-int native, so the behavior-shimming code is colocated with the native definitions (in Native.hs).

pact.cabal

src-ghc/Pact/Types/Crypto.hs

src/Pact/Native/Keysets.hs

src/Pact/Types/Command.hs

cabal.project

src-ghc/Pact/Types/Crypto.hs

src/Pact/Native.hs

emilypi · 2023-08-31T20:17:21Z

src/Pact/Native.hs

+
+    -- With Simplified error messages, map every error to a single string.
+    Left _ | behavior == Simplified ->
+             return $ Left "Could not base64-decode string"


weird indentations here and below on some things

Better now?

src/Pact/Types/Command.hs

pact.cabal

src/Pact/Native.hs

rsoeldner · 2023-09-01T08:18:27Z

src/Pact/Native.hs

+          Right bs -> case T.decodeUtf8' bs of
+            Right t -> return $ tStr t
+            Left _unicodeError -> evalError' i $ "invalid unicode"
+          Left base64Error -> evalError' i (pretty (T.pack base64Error))


This is not aligned with line 1349:

... Left e -> evalError' si (pretty e)

Oh, you mean 1349 calls pretty on a String while 1422 calls pretty on a Text? That does seem like a discrepancy. I'll change both to call pretty on a Text, and recheck replay. Good catch!

src/Pact/Types/Crypto.hs

rsoeldner · 2023-09-01T08:29:04Z

src/Pact/Types/Crypto.hs

+        WebAuthnSignature
+          { authenticatorData
+          , signature
+          , clientDataJSON } <- A.eitherDecode (BSL.fromStrict sigBS)


Maybe

Suggested change

WebAuthnSignature

{ authenticatorData

, signature

, clientDataJSON } <- A.eitherDecode (BSL.fromStrict sigBS)

WebAuthnSignature{..} <- A.eitherDecode (BSL.fromStrict sigBS)

using {-# LANGUAGE RecordWildCards #-}

I like NamedFieldPuns because it makes the identifiers being brought into scope explicit. Meaning: if someone reading the code didn't know what authenticationData is, or where it comes from, NamedFieldPuns makes it greppable, while RecordWildCards doesn't.

Alternatively, construct/destruct the thing directly since we're not even really punning as much as we are constructing directly.

I like NamedFieldPuns even for vanilla record desructuring too, since it removes the possibility of accidentally naming fields out of order. Meaning, I could write:

WebAuthnSignature sig authData clientData <- decode sigBS -- (Is this what you mean by destruct the thing directly?) ...

and it would typecheck but be subtly wrong, because I accidentally swapped the field names around for fields that have the same type.

src/Pact/Types/Crypto.hs

jwiegley · 2023-09-01T20:37:08Z

docs/en/pact-functions.md

 ```


-### format-address {#format-address}


What does dropping this have to do with WebAuthn signatures?

Dropping it was required after removing a method from the Scheme typeclass that format-address required.

We have to remove some methods and associated types from Scheme to accommodate webauthn. It's believed be to be unused or only used very rarely.

jwiegley · 2023-09-01T21:01:29Z

pact.cabal

    Pact.Types.Command
    Pact.Types.Continuation
    Pact.Types.Crypto
-    Pact.Types.ECDSA


Are we dropping this module because the WebAuthn support supercedes the functionality?

While we were modifying the Scheme typeclass to support webauthn, another instance of that typeclass, the ETH scheme (and the Pact.Types.ECDSA module that supported it) came up as a place to eliminate some effectively-dead code.

jwiegley · 2023-09-01T21:02:19Z

src/Pact/ApiReq.hs

    Right sd -> Right sd

-addSigToSigData :: SomeKeyPair -> SigData a -> IO (SigData a)
+addSigToSigData :: Ed25519KeyPair -> SigData a -> IO (SigData a)


If we start integrating more with other changes, would we still want to monomorphize the signature scheme?

There are some integrations we could make that would introduce new PPK schemes. Depending on the details of how those integrations work, they might not see the changes here in Pact.ApiReq, since Pact.ApiReq is only used client-side, e.g. by users signing requests and making API calls to a pact server. A rule of thumb I used in this PR was to be more cautious changing server-side code, but more liberal with client-side code as needed).

A new integration that merely needs signature verification in pact will not be bothered by the monomorphization happening in client code here. But an integration that also wanted client-side support would indeed be less convenient to write, after we remove SomeKeyPair.

jwiegley · 2023-09-01T21:04:59Z

src/Pact/Native.hs

 import Control.Monad
 import Control.Monad.IO.Class
 import qualified Data.Attoparsec.Text as AP
+import Data.Bifunctor (first)


This doesn't conflict with the import of first from Control.Arrow just above?

import Control.Arrow hiding (app, first)
Added it to the hiding so I could have my bifunctor first.

jwiegley · 2023-09-01T21:10:32Z

src/Pact/Types/Scheme.hs

 --------- PPKSCHEME DATA TYPE ---------

-data PPKScheme = ED25519 | ETH
+data PPKScheme = ED25519 | WebAuthn


I feel like there are really 4 PRs hiding in this one:

Monomorphizing the signature scheme

Dropping the ETH PPKScheme

Dropping format-address

Adding the WebAuthn PPKScheme

I want to hear from @larskuhtz whether we'd like to keep the ETH scheme, if we're going to be doing bridging with Ethereum...

Just 4? Don't forget bumping base64-bytestring! :) That's got to count as 3 all on its own.

src/Pact/Types/Crypto.hs

jwiegley

This is looking good, just a few comments made!

sirlensalot

Great work! ~~Main note is we should reconsider removing ETH, see comment in Pact.Types.Crypto.~~

EDIT scratch that, Eth ECDSA support for Pact payloads is a dead letter. ECDSA support in the context of ethereum compatibility (Solidity ABI, or EVM support) uses a different payload.

src/Pact/Native.hs

src/Pact/Types/Command.hs

src/Pact/Types/Crypto.hs

src/Pact/Types/SPV.hs

tests/pact/base64.repl

tests/pact/lib.repl

src/Pact/Types/ECDSA.hs

src/Pact/Repl/Lib.hs

tests/SchemeSpec.hs

Co-authored-by: John Wiegley <johnw@newartisans.com>

src/Pact/Native.hs

edmundnoble · 2023-09-15T12:54:39Z

src/Pact/Types/Crypto.hs

+        -- Extract the original challenge from client data.
+        ClientDataJSON { challenge } <- A.eitherDecode (BSL.fromStrict clientData)


I'm a bit worried by the amount of aeson instances here being used to decode things that are on-chain, but I don't suggest changing this at this point.

tests/SchemeSpec.hs

edmundnoble · 2023-09-15T13:07:19Z

src/Pact/Types/Crypto.hs

+        publicKey <- first show $
+          Serialise.deserialiseOrFail @WA.CosePublicKey
+            (BSL.fromStrict pubBS)


Should we restrict the algorithm here to just a few to avoid algorithm confusion attacks? I think @EnoF wanted the algorithm "ES256", which has COSE identifier -7. https://portswigger.net/web-security/jwt/algorithm-confusion

I've added some code that restricts to -7 or -8. (ECDSA SHA-256, and EdDSA respectively).

I informally tested the restriction by setting the requirement to -1 or -8 temporarily and observing that the happy path signature failed.

I'll hopefully be able to generate a test case that we can check in to the repo soon. But it requires spinning up a server that. implements webauthn to create new sample keys with a different signing algorithm.

edmundnoble · 2023-09-15T13:07:41Z

tests/SchemeSpec.hs

+-- This test uses example public keys, clientData and authenticatorData from a
+-- real WebAuthn browser session from a test user.
+verifyWebAuthnSignature :: Spec


If we do restrict the signing algorithm we should have a test for it

Co-authored-by: Edmund Noble <edmundnoble@users.noreply.github.com>

tests/PactContinuationSpec.hs

Co-authored-by: Lars Kuhtz <lars@kadena.io>

src/Pact/Types/Crypto.hs

rsoeldner reviewed Apr 26, 2023

View reviewed changes

emilypi reviewed Apr 26, 2023

View reviewed changes

cabal.project Show resolved Hide resolved

emilypi reviewed Apr 26, 2023

View reviewed changes

src-ghc/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

emilypi reviewed Apr 26, 2023

View reviewed changes

src-ghc/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

src-ghc/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

imalsogreg force-pushed the greg/webauthn-signatures branch from 585efde to 1d2315b Compare April 28, 2023 16:18

imalsogreg force-pushed the greg/webauthn-signatures branch from 1d2315b to c59307c Compare July 26, 2023 22:10

imalsogreg changed the base branch from lars/wip to master July 26, 2023 22:11

imalsogreg force-pushed the greg/webauthn-signatures branch from 9dc8ae0 to 70e8a64 Compare August 17, 2023 21:15

imalsogreg marked this pull request as ready for review August 23, 2023 19:56

imalsogreg requested review from jmcardon, jwiegley and sirlensalot as code owners August 23, 2023 19:56

imalsogreg force-pushed the greg/webauthn-signatures branch 2 times, most recently from 76a069d to ecfeba5 Compare August 28, 2023 23:17

emilypi reviewed Aug 31, 2023

View reviewed changes

src/Pact/Native.hs Outdated Show resolved Hide resolved

emilypi reviewed Aug 31, 2023

View reviewed changes

rsoeldner reviewed Sep 1, 2023

View reviewed changes

jwiegley reviewed Sep 1, 2023

View reviewed changes

src/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

jwiegley reviewed Sep 1, 2023

View reviewed changes

src/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

jwiegley reviewed Sep 1, 2023

View reviewed changes

sirlensalot suggested changes Sep 2, 2023

View reviewed changes

imalsogreg added 4 commits September 6, 2023 13:50

Implement webauthn signature checking

0a44308

bump version to 4.9

f057f11

Succeed on parsing of non-canonical base64 strings

e0dc76f

Accept non-canonical base64 strings for strToInt

8530fd1

imalsogreg and others added 8 commits September 6, 2023 13:50

lower bound on serialise dep

339ca47

PR feedback

47926f0

Call pretty on Text, not String

9ead3d5

formatting

5f06df8

syntax improvements

6307d3f

cleanup

620f522

Update src/Pact/Types/Crypto.hs

db21f87

Co-authored-by: John Wiegley <johnw@newartisans.com>

formatting

7c8fb49

imalsogreg force-pushed the greg/webauthn-signatures branch from a1bcd02 to 7c8fb49 Compare September 6, 2023 20:53

imalsogreg added 7 commits September 11, 2023 12:08

Remove unused imports

56c856d

Fix error message behavior for base64 native

9224829

Fix base64 error message spelling and make tests more explicit

d1c3eb8

Add test cases for webauthn signatures

8bd507b

Don't set address when making command with ed25519 keys

543e109

Move behavior check into error shimmer function

ac342b1

cleanup comments

0d088cb

edmundnoble reviewed Sep 15, 2023

View reviewed changes

imalsogreg and others added 3 commits September 15, 2023 16:24

Limit allowable webauthn signature algorithms

e3c060d

use a better runtime flag combinator

9470d1b

Update src/Pact/Native.hs

dada984

Co-authored-by: Edmund Noble <edmundnoble@users.noreply.github.com>

larskuhtz reviewed Sep 18, 2023

View reviewed changes

tests/PactContinuationSpec.hs Outdated Show resolved Hide resolved

Update tests/PactContinuationSpec.hs

76f35b0

Co-authored-by: Lars Kuhtz <lars@kadena.io>

jmcardon reviewed Sep 19, 2023

View reviewed changes

src/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

src/Pact/Types/Crypto.hs Outdated Show resolved Hide resolved

imalsogreg requested a review from sirlensalot September 20, 2023 18:33

cleanup

9ef8d2c

jmcardon approved these changes Sep 21, 2023

View reviewed changes

emilypi approved these changes Sep 21, 2023

View reviewed changes

sirlensalot approved these changes Sep 21, 2023

View reviewed changes

imalsogreg merged commit 445af04 into master Sep 21, 2023

		-- Extract the original challenge from client data.
		ClientDataJSON { challenge } <- A.eitherDecode (BSL.fromStrict clientData)

Conversation

imalsogreg commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

base64-bytestring upgrade

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilypi Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imalsogreg Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jwiegley left a comment

Choose a reason for hiding this comment

Uh oh!

sirlensalot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imalsogreg commented Apr 14, 2023 •

edited

Loading

emilypi Sep 1, 2023 •

edited

Loading

imalsogreg Sep 1, 2023 •

edited

Loading

sirlensalot left a comment •

edited

Loading