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

Implement verifySignature #671

Merged
merged 7 commits into from
Apr 4, 2019
Merged

Conversation

vmchale
Copy link
Contributor

@vmchale vmchale commented Mar 1, 2019

This PR fixes the private/public key situation in wallet-api. The main changes are

  • Wallets are identified by private keys instead of Ints.
  • Signatures are attached to transactions (as per the spec), not the inputs of transactions.
  • Transactions have to be signed before submitting them to the blockchain.
  • verifySignature now has the correct number of arguments and works as expected.
  • plutus-tx has a SizedByteString n type to distinguish bytestrings of different lengths in PLC.

TO DOs:

  • Stop failing tests in wallet-api
  • Fix plutus-use-cases
  • Profile plutus-use-cases
  • Fix marlowe (The changes in marlowe are non-trivial - @nau could you please take a look?)
  • Fix plutus-playground
  • Fix plutus-tutorials
  • Rebase
  • Fix meadow

@j-mueller j-mueller self-assigned this Mar 26, 2019
@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch from 1a32d9d to bc4f38d Compare March 26, 2019 14:51
@vmchale vmchale self-assigned this Mar 26, 2019
@j-mueller
Copy link
Contributor

note that the crowdfunding tests currently fail because of a bytestring size mismatch - verifySignature expectes the sig. to be 64 bytes but the default size for ByteStrings assigned by the plugin is 32 bytes.

To fix that I'm adding some newtypes for ByteStrings of length 32 and 64.

@j-mueller
Copy link
Contributor

@michaelpj can you take alook at the last commit please -

the verify test fails with

        Exception: Type mismatch at () in term
          'arg_95'.
        Expected type
          '[(con bytestring) (con 32)]',
        found type
          '[(con bytestring) (con 64)]'

(See https://hydra.iohk.io/build/742161/nixlog/1) but I can't find the place where it would get the wrong size.

@michaelpj
Copy link
Contributor

You haven't changed what the builtins machinery in language-plutus-core thinks the type of verifySignature is, I think.

@michaelpj
Copy link
Contributor

We might need to ask @effectfully for help on what to change there.

@effectfully
Copy link
Contributor

effectfully commented Mar 27, 2019

It doesn't look to me that the problem is in the builtins machinery, because verifySignature is fully polymorphic there:

typedVerifySignature :: TypedBuiltinName (BSL.ByteString -> BSL.ByteString -> BSL.ByteString -> EvaluationResult Bool) (EvaluationResult Bool)
typedVerifySignature =
    TypedBuiltinName VerifySignature $
        TypeSchemeAllSize $ \s0 -> TypeSchemeAllSize $ \s1 -> TypeSchemeAllSize $ \s2 ->
            TypeSchemeBuiltin (TypedBuiltinSized (SizeBound s0) TypedBuiltinSizedBS) `TypeSchemeArrow`
            TypeSchemeBuiltin (TypedBuiltinSized (SizeBound s1) TypedBuiltinSizedBS) `TypeSchemeArrow`
            TypeSchemeBuiltin (TypedBuiltinSized (SizeBound s2) TypedBuiltinSizedBS) `TypeSchemeArrow`
            TypeSchemeBuiltin TypedBuiltinDyn

Here is my random guess: from the definition of instSize I kinda expect the following

term <- wrapSizedBsrel [BS32, BS32, BS64] $ instSize haskellBS32Size $ instSize haskellBS32Size $ instSize haskellBS64Size $ mkBuiltin PLC.VerifySignature

to elaborate to

verifySignature {64} {32} {32}

rather than

verifySignature {32} {32} {64}

Is my guess correct?

@effectfully
Copy link
Contributor

By the way,

-- | A 'BSL.ByteString' of 32 bytes.
newtype ByteString32 = ByteString32 { unByteString32 :: BSL.ByteString }
        deriving (Eq, Ord, Show, IsString)

-- | A 'BSL.ByteString' of 64 bytes.
newtype ByteString64 = ByteString64 { unByteString64 :: BSL.ByteString }
        deriving (Eq, Ord, Show, IsString)

this is terrible. Can we have

newtype Sized a (s :: k) = Sized a

and use Sized ByteString 32 and Sized ByteString 64 instead, so that we don't need to redefine all the functions (take, drop, etc) for distinctly sized bytestrings/integers?

If you decide to add this single newtype to handle everything, then please add it somewhere to language-plutus-core (PlutusPrelude.hs might be a fine place), so that I can add relevant KnownDynamicBuiltinType instances later.

@michaelpj
Copy link
Contributor

I think you hit the nail on the head there.

Yes, this is terrible, but we're not brave enough to do the Sized thing yet. I'm going to look into it, but I think it's more important to get something working asap.

@effectfully
Copy link
Contributor

I think you hit the nail on the head there.

Then I suggest to write it like that:

mkBuiltin PLC.VerifySignature `instSize` haskellBS32Size `instSize` haskellBS32Size `instSize` haskellBS64Size

Yes, this is terrible, but we're not brave enough to do the Sized thing yet. I'm going to look into it, but I think it's more important to get something working asap.

No problem, but I do not expect Sized to cause troubles here, it should provide a direct replacement.

@michaelpj
Copy link
Contributor

Well, it won't work directly since the way the compiler plugin directly identifies type constructors with types. So it's not able to have special cases for Sized ByteString 64 and Sized ByteString 32 without some work.

data SizedByteString (s::k) = SizedBytestring ByteString

would probably work, but I would still need to work out how to get the sizes in the type system etc. So - some work.

@effectfully
Copy link
Contributor

Ah, I see, thanks.

@j-mueller
Copy link
Contributor

@effectfully

Is my guess correct?

Yes it is, thanks.

I thought about a type parameter for the sizes too but I decided to go with the solution that had a smaller risk of failure :)

@j-mueller
Copy link
Contributor

@vmchale I updated plutus-use-cases so you can use that for profiling (the crowdfunding contract)

looks like everything is working, so I think now is a good time to rebase

@michaelpj
Copy link
Contributor

I pushed a commit that adds type-level sizes on the Haskell side, and converts them over properly. So we don't need anything special, and a bunch of size-polymorphic stuff Just Works.

Once this is merged I'll do the same for integers, but I don't want to hold this up.

@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch 4 times, most recently from 3125161 to 7ab2473 Compare March 31, 2019 18:17
@michaelpj michaelpj marked this pull request as ready for review April 1, 2019 08:39
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Broadly good. We may want to merge and address my comments later.

makeLift ''PubKey

-- | A cryptographic private key.
newtype PrivateKey = PrivateKey { getPrivateKey :: KeyBytes }
Copy link
Contributor

Choose a reason for hiding this comment

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

Both this and the public key should be 32-byte segments of the key bytes, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed it to use the PublicKey and PrivateKey types everywhere so KeyBytes type isn't actually necessary anymore. I think that's better than having a 64-byte ByteString with two dedicated segments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, but the PublicKey should definitely not contain the private key in case we serialize it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I just realised something. We do actually serialise private keys now. (To identify wallets in the Playground). But I think I'll change it so that wallets are still Ints and to get a wallet's private key we look up the index in the list of known private keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's benign, since it's just in the playground which isn't meant to be secure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but keeping wallets as Ints has the advantage that I don't need to update the playground code ;)

signedBy (Signature k) (PubKey s) = k == s
signedBy :: Signature -> PubKey -> TxId -> Bool
signedBy (Signature s) (PubKey k) txId =
let k' = ED25519.publicKey $ BSL.toStrict $ Builtins.unSizedByteString $ KB.getKeyBytes k
Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. I'd probably do this stuff in the construction of PublicKey. Note that the current version is potentially insecure, since we might put the PublicKey somewhere public even though it actually contains the private key too!

signedBy (Signature s) (PubKey k) txId =
let k' = ED25519.publicKey $ BSL.toStrict $ Builtins.unSizedByteString $ KB.getKeyBytes k
s' = ED25519.signature $ BSL.toStrict $ Builtins.unSizedByteString s
in throwCryptoError $ ED25519.verify <$> k' <*> pure (getTxId txId) <*> s' -- TODO: is this what we want
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I think ideally we'd handle these errors explicitly. This isn't a very exceptional error, and if we throw a bunch of exceptions then we have to write a robust harness in the executable, since we still want to send a response in case of an error, so it can't kill the whole executable.

let k = ED25519.secretKey $ BSL.toStrict $ Builtins.unSizedByteString $ KB.getKeyBytes privKey
pk = ED25519.toPublic <$> k
salt :: BS.ByteString
salt = "" -- TODO: do we need better salt?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this doesn't actually matter too much. AIUI, there are two reasons we might want a salt:

  • To get a different signature for each message, to make it harder to reuse signatures. This isn't so much of a problem for us since the transaction itself is only valid once.
  • To prevent replay attacks. Again, this isn't so much of a problem for us since transactions are only valid once, and in fact having replayability is actually helpful for us.

We could get a salt that's replayable by us by having it be pseudo-randomly generated with a seed that's passed in by the caller. Then the contract backend can still replay everything deterministically.

wallet-api/src/Ledger/Crypto.hs Show resolved Hide resolved
wallet-api/src/Ledger/Index.hs Outdated Show resolved Hide resolved
wallet-api/src/Wallet/Generators.hs Outdated Show resolved Hide resolved
-- ^ The 'SlotRange' during which this transaction may be validated.
txSignatures :: Map PubKey Signature
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we hash the transaction by using the derived Serialise instance. But we should be signing something that doesn't include the signatures, which that does. In particular, I'm not sure how we ever validate any signatures, since I'd expect us to recalculate the hash later and get a different thing...

Also, we don't want to just rely on the map being empty, since that way we can add more signatures later without having to contort ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

The hash is of a TxStripped which excludes the signatures.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/input-output-hk/plutus/blob/7ab2473b9b1856d0d8307916a85774810562a48a/wallet-api/src/Ledger/Tx.hs#L194

Although it might be better to just add a type parameter for the signatures to Tx...

Copy link
Contributor

Choose a reason for hiding this comment

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

Or have TxBody and Tx, which is the terminology that the specifications use, I think?

@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch 4 times, most recently from 901658f to c34069c Compare April 2, 2019 07:50
@michaelpj
Copy link
Contributor

I think marlowe just needs to have its compile warnings fixed.

@j-mueller
Copy link
Contributor

Right, I'll do that

Vanessa McHale and others added 2 commits April 4, 2019 11:43
* Add verifySignature

* Add swagger bits

* Use KeyBytes pervasively

* Fix up wallet-api library

* Test suite builds + fails

* Add fromHex

* Fix four test cases

* More sensible API

* Start signing mechanisms

* Remove ownSignature

* Remove unneeded functions

* rearrange by secret key

* Patch up types

* Remove bit we don't need

* Use PubKey

* Use signature in pubKeyTxIn

* Use bottom

* cardano-crypto and http-api-data

* transactions have signatures

* Show KeyBytes

* PublicKeySize invalid

* wallet-api tests pass

* fix plutus-use-cases
fix plutus-use-cases

fix warnings

keybytes exports

game contract types
@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch 2 times, most recently from 050357c to b2f3963 Compare April 4, 2019 10:05
@j-mueller j-mueller changed the title Partial signature implementation Implement verifySignature Apr 4, 2019
* PS bridge / JSON
* Marlowe
@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch from b2f3963 to f56e681 Compare April 4, 2019 10:20
@j-mueller
Copy link
Contributor

marlow test failures are odd, I was able to build it without problems:

grafik

@michaelpj
Copy link
Contributor

Ah - the marlowe tests are split out into another derivation, so that won't run the tests: you need localPackges.marlowe.testrun. I think the failure is genuine.

@michaelpj
Copy link
Contributor

It's something weird where we're not producing a transaction when we think we should. Also I guess this means the timing out is also real.

@michaelpj
Copy link
Contributor

@nau this looks suspicious: https://github.com/input-output-hk/plutus/pull/671/files#diff-f1f2d09538c116215822ad173fa89638R40

It looks like the ids are coming apart from the person's public key, is that right?

@michaelpj
Copy link
Contributor

I wonder whether something in the test might be causing all the contracts to fail by timeouts? Which might be very slow in the property test.

@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch 2 times, most recently from f83740a to 235da5f Compare April 4, 2019 14:16
@j-mueller j-mueller force-pushed the plutus-core/signature-implementation branch from 235da5f to cec125e Compare April 4, 2019 14:20
@michaelpj
Copy link
Contributor

Compile warnings for the stuff you commented out 🙈

@j-mueller
Copy link
Contributor

it should be OK with the latest commit (cec125e)

@michaelpj
Copy link
Contributor

👍

@michaelpj
Copy link
Contributor

Stylish haskell - last thing!

@michaelpj
Copy link
Contributor

\o/

@michaelpj
Copy link
Contributor

What a slog, great work all. Will merge shortly.

@michaelpj michaelpj merged commit 5c4a4dd into master Apr 4, 2019
@j-mueller j-mueller deleted the plutus-core/signature-implementation branch April 4, 2019 15:14
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.

5 participants