search

ethereum / go-ethereum

Go implementation of the Ethereum protocol
https://geth.ethereum.org
GNU Lesser General Public License v3.0
47.49k stars 20.1k forks source link

Implement EIP-191 Signing API #17696

Closed tuxxy closed 5 years ago

tuxxy commented 6 years ago

Geth lacks an API to perform an EIP-191 conforming signature for a given unlocked account. An API should be implemented that allows for arbitrary data to be signed by a specified unlocked account. This API will encompass signing as well as verifying functionalities.

Current API: https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 1500.0 DAI (1500.0 USD @ $1.0/DAI) attached to it.

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 6 months, 1 week from now. Please review their action plans below:

1) paulrberg has been approved to start work.

I previously built a few private networks and I'd love to contribute to geth! I've also got a decent understanding of cryptography used in Ethereum.

I'll start by clearly breaking down what currently can be done and what would be the first subproblems in implementing EIP-191.

Learn more on the Gitcoin Issue Details page.

2) moodysalem has applied to start work _(Funders only: approve worker | reject worker)_.

  1. Prepare my development environment for go-ethereum. I am familiar with developing in golang from making significant additions to the open source Hyperledger Fabric project.
  2. Identify the RPC handling code
  3. Identify the signature algorithm
  4. Identify the code that provides unlocked accounts to geth
  5. Validate with geth maintainers that this feature should exist in the go-ethereum codebase (it is my understanding that the keystore related features are being moved into a separate application)
  6. Implement failing unit tests that make this RPC call over HTTP and WebSockets in the appropriate codebase
  7. Implement the RPC handler for this action

Learn more on the Gitcoin Issue Details page.

holiman commented 6 years ago

A few comments... The spec

I'm thinking that maybe the signer can, for now, only deal with certain subs of EIP-191, the first two being 0x45 and 0x01.

This also ties in with some other work I'm doing for clef, namely the ability to sign Clique headers. One thing I'm pondering is to have accounts_sign(content_type, ... ), where content-type can be e.g application/clique or application/eip191 etc.

holiman commented 6 years ago

Here is some WIP, where I've been trying out how to fit various types of signing into clef. See commit https://github.com/holiman/go-ethereum/commit/5b03e2029f4cd472f1167999fe6907614158ec81#diff-f8316f6ee7e75303007a55a1cde29245R420 in my branch geth_relay.

holiman commented 6 years ago

While I'm all for someone taking charge of this and helping out the implementation, I fear that the brunt of the work is not in actual coding, but a lot will have to go into coordination and discussions. But yes, I'd approve @PaulRBerg to take this on

PaulRBerg commented 6 years ago

Okay so I had a thorough read on both the EIP-191 and EIP-712 specs, their adjacent[1][2] links and your Clique WIP and I confirm I am able to take on both tasks.

Thanks for the brief intro to clef and for the accounts_sign suggestion. Just to confirm, you're referring to having the following list of content-type:

• application/clique • application/eip191 • application/eip712

Where eip191 maps to the 0x00 and 0x45 version bytes and eip712 to 0x01?

Re coordination - totally agree, the UX can be really easily a nightmare for the non-technical end user, thus I'd suggest, for now, asking wallets to only accept JSON compatible data which can be nicely presented just like here. Non-JSON could still be accepted, but with a warning saying "Your data is not JSON compatible. You can still sign it, but please be careful and double check". Furthermore, I see a potential cross over with ENS to make accounts look more human-readable.

Also, is there any Slack or Riot room focused specifically on geth and sigs? I think it'd be helpful to hang around there to keep discussions as lightweight as possible. I'll also try to DYOR as much as I can to avoid unnecessary questions.

holiman commented 6 years ago

We have a discord server: https://discord.gg/aBsXrzr . That server has a clef-channel, for all signing related stufd

holiman commented 6 years ago

Re content-types, nothing is set in stone, I have just tinkered a bit with how it could be done, to see if it makes sense

tuxxy commented 6 years ago

I want to point out the reason for opening this issue:

Currently, Ethereum signatures are sorta bad. EIP-191 and EIP-712 are the best we have right now and they have different usecases.

We need a way to sign data arbitrarily with an unlocked account for a long running process. We don't want to build a wallet because we don't think the ecosystem should be building one wallet per account type apps.

We also intend to integrate this with web3py for the Python/ETH ecosystem as well so we can use it in nucypher.

Thanks for jumping on this so quickly!

holiman commented 6 years ago

Currently, Ethereum signatures are sorta bad

Totally agree. Bad bordering useless.

It's great that you're jumping in, because I don't want to end up building an API endpoint that does not fit the usecase it's intended for, so it would be good to have some more info on e.g. how you would like to call it. While 712 has an example, the 191 spec (which I'm a co-author of) doesn't currently have any examples on the API-side for how the corresponding signing methods should look. I'm thinking that a starting point would be to implement 191/172 but return errors for other 191 subtypes (until those get specced and implemented)

And btw: unlock is being nuked from orbit, but a semi-advanced user will be able to set rules allowing certain types of operation automatically, such as e..g signing a certain type of data.

spm32 commented 6 years ago

Awesome to see so much conversation about this already! @holiman are you already in discussions with Paul via Discord? Alright on your end if I accept him for this bounty?

holiman commented 6 years ago

@ceresstation yup fine by me

PaulRBerg commented 6 years ago

Currently, Ethereum signatures are sorta bad. EIP-191 and EIP-712 are the best we have right now and they have different usecases.

Gotcha.

We also intend to integrate this with web3py for the Python/ETH ecosystem as well so we can use it in nucypher.

I totally acknowledge that and it's really exciting to contribute to so many projects at once. I'll aim to be the author of the PRs for we3b (js and py).

so it would be good to have some more info on e.g. how you would like to call it.

Before doing any coding, I'll devise a short but effective strategy. Doing a lot of research across many ethereum repos now.

I have a few other quick questions, but I'll defer them to Discord to keep this thread only for significant updates. Thank you @holiman @tuxxy @ceresstation!

PaulRBerg commented 6 years ago

Hey there, a fresh update, I created a small repo documenting my latest research: https://github.com/PaulRBerg/ethereum-signatures-progress

There's a list of resources and projects which have already tinkered with EIP-191 and EIP-712. Thanks to @cooganb for reaching out on Discord to mention their work on a basic login system adhering to 191.

I'll constantly update the registries in there as there is significant progress. As geth itself is concerned, I delved into its meat, playing around and analysing clef, rpc and signer. Strategy:

  1. Continue familiarising with the submodules above, specifically with signer as that's where the standards should be coded in
  2. As 191 is kinda encompassing 712, I'll first focus on a few commits for a minimum viable 191 which implements 0x45 and returns a hardcoded message for 0x01. Will iterate fast and continue from there.

Just to confirm, the signing methods should be available both in clef in the CLI and in RPC via clef_signTypedData? Or, for the JSON-RPC, is it that other prefix should be used (clef is not currently listed in the wiki)?

UPDATE: no answer needed anymore. I sorted it out, clef is using the account prefix as per the README.

cooganb commented 6 years ago

Great work @PaulRBerg ! Let me know if I can help with anything.

PaulRBerg commented 6 years ago

Significant update: https://github.com/ethereum/go-ethereum/pull/17789

gitcoinbot commented 6 years ago

@PaulRBerg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

PaulRBerg commented 6 years ago

I have just pushed an update on #17789 which implements a skeleton for testing the content types:

go test -v -cpu 4 ./signer/core -run TestSignData

Now, we have to sort out:

  1. If the hashing and recovering of what's done so far, at least 0x45 or data/plain is okay
  2. The nomenclature, i.e. text/plain instead of data/plain and the order on that
  3. What the user sees when signing data with a contract as a validator, I gave my recommendation in this comment
  4. Where to include the ecRecover function, although not usable for all signing types
  5. If the community wants to add any other type to the list
PaulRBerg commented 6 years ago

Latest work is on https://github.com/PaulRBerg/go-ethereum/tree/eip-191/712_wip

Getting close. Currently working on handling the params sent with a JSON-RPC call and validating them against the EIP-712 specs.

I'm also curious to see what apps will there be develope at EthSF and get in touch with the devs to see what would be the best solution for point 3 above.

gitcoinbot commented 6 years ago

@PaulRBerg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

PaulRBerg commented 6 years ago

Hi there, I participated at EthSF, but still worked on these EIPs on the side. Currently tweaking the rpc/http and rpc/server code to make it work with the arbitrary key values required by EIP-712. You can find my latest work at https://github.com/PaulRBerg/go-ethereum/tree/eip191/712_wip

The problem specifically is that the geth rpc server should accept a new struct called TypedData but it returns empty maps: map[] map[] map[] when querying eth_signStructuredData. Here's the struct definition, POST data and response:

type TypedData struct {
    Types       map[string] interface{}     `json:"types"`
    PrimaryType string              `json:"primaryType"`
    Domain      map[string] interface{}     `json:"domain"`
    Message     map[string] interface{} `json:"message"`
}
{
  "jsonrpc": "2.0",
  "method": "account_signStructuredData",
  "params": [
    "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826",
    {
      "types": {
        "EIP712Domain": [
          {
            "name": "name",
            "type": "string"
          },
          {
            "name": "version",
            "type": "string"
          },
          {
            "name": "chainId",
            "type": "uint256"
          },
          {
            "name": "verifyingContract",
            "type": "address"
          }
        ],
        "Person": [
          {
            "name": "name",
            "type": "string"
          },
          {
            "name": "wallet",
            "type": "address"
          }
        ],
        "Mail": [
          {
            "name": "from",
            "type": "Person"
          },
          {
            "name": "to",
            "type": "Person"
          },
          {
            "name": "contents",
            "type": "string"
          }
        ]
      },
      "primaryType": "Mail",
      "domain": {
        "name": "Ether Mail",
        "version": "1",
        "chainId": 1,
        "verifyingContract": "0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC"
      },
      "message": {
        "from": {
          "name": "Cow",
          "wallet": "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826"
        },
        "to": {
          "name": "Bob",
          "wallet": "0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB"
        },
        "contents": "Hello, Bob!"
      }
    }
  ],
  "id": 1
}
fmt.Println("data", data)
// output after RPC call is `data {map[]  map[] map[]}`

Specifically, I'm trying to make it work as explained in this StackOverflow thread. Someone pointed me that eth_getLogs is similar, will check that out.

UPDATE

I tried only with a basic struct and it still doesn't parse it correctly:

type TypedData struct {
    PrimaryType string `json:"primaryType"`
} 
fmt.Println("data.PrimaryType", data.PrimaryType)
// output after RPC call is `data.PrimaryType`

Currently investigating how crit FilterCriteria is parsed in func (api *PublicFilterAPI) GetLogs(ctx context.Context, crit FilterCriteria) ([]*types.Log, error) in eth/filters/api.go. There must be some fundamental differences in the core codecs used.

UPDATE 2

Figured it out by reading lines 274-276 from eth/filters/api.go:

// FilterCriteria represents a request to create a new filter.
// Same as ethereum.FilterQuery but with UnmarshalJSON() method.
type FilterCriteria ethereum.FilterQuery

Now writing the custom UnmarshalJSON() function for TypedData.

gitcoinbot commented 6 years ago

@PaulRBerg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

PaulRBerg commented 6 years ago

Major update, check it out on #17789.

  1. Finally solved the annoying TypedData issue I had with the JSON-RPC server (thank you @holiman)
  2. Changed contentType mimes back to the defaults of EIP-191. Just for the record, the table below mirrors what's now defined on the PR page.
  3. Implemented the encodeType function for EIP-712.

In this specific PR, there are a few debugging logs but please neglect them as they'll be scratched soon. For EIP-712, pretty much only encodeData is left and we're good to go to test it out until it breaks.

Version byte Name Description
0x00 text/validator Data with intended validator
0x01 data/typed EIP-712 typed data
0x02 application/clique Clique headers
0x45 text/plain Plain, personal messages
PaulRBerg commented 6 years ago

A new update on #17789 implements a recursive hashStruct in coalition with encodeData, but the latter still needs type handling. Most probably the current model of verifying the type in go directly should be changed with checking the types object (the strings themselves) sent along the data via JSON-RPC.

gitcoinbot commented 6 years ago

@PaulRBerg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

PaulRBerg commented 6 years ago

EIP-712 is almost implemented. Big updates in this repo: https://github.com/PaulRBerg/codebase-go/tree/master/eip712

I switched there for the last few days as testing is faster.

PaulRBerg commented 6 years ago

The first drafted version of EIP-712 is implemented in the last update on #17789! Accompanying tests were written. Important question:

Should I generate a slice of arguments (that is, []pack.Argument) and then run them through the Pack function in the abi package instead of manually doing the encoding? Currently, I'm adding empty bytes to match the 32-bytes requirement for bytes1:31 and uint160.

gitcoinbot commented 6 years ago

@PaulRBerg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

holiman commented 6 years ago

Please snooze this, he's doing good progress on this!

PaulRBerg commented 6 years ago

Thank you @holiman! Just for the record, I'm currently waiting for feedback on the latest drafts, but, at the same time, I pushed a commit yesterday which solves a few known bugs. Everything can be tracked on PR #17789.

gitcoinbot commented 6 years ago

@PaulRBerg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

PaulRBerg commented 6 years ago

It was great catching up with @holiman in Prague as we were able to discuss the last details for implementing EIP-191 and EIP-712. I absolutely enjoyed working on this - and now I can say that PR #17789 is done and pending for feedback from your end :)

Overview:

• Method eth_signData accepts a contentType parameter which fully adheres to the specs of EIP-191 • Method eth_signTypedDataaccepts a typedData json object conforming to EIP-712

I eventually separated the methods because (1) that's what the specs require and (2) there were a lot of assumptions in converting a map[string]interface{} to a TypedData object. It is much easier to let the rpc server deal with that via its built-in reflections.

Aiming to also get in touch with the EIP-712 authors to see if they could take a look.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 1500.0 DAI (1500.0 USD @ $1.0/DAI) has been submitted by:

  1. @paulrberg

@ceresstation please take a look at the submitted work:

PaulRBerg commented 5 years ago

This PR implements this: https://github.com/ethereum/go-ethereum/pull/17789

Should this issue be closed?

gitcoinbot commented 5 years ago
Good Communicator ⚡️ A *Good Communicator* Kudos has been sent to @PaulRBerg for this issue from @ceresstation. ⚡️ Nice work @PaulRBerg! Your Kudos has automatically been sent in the ETH address we have on file.
gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 1500.0 DAI (1500.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @PaulRBerg.

tuxxy commented 5 years ago

Does this need to be closed now?