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

Allow to sign transaction manually #5640

Open
caffeinum opened this issue Oct 30, 2018 · 31 comments
Open

Allow to sign transaction manually #5640

caffeinum opened this issue Oct 30, 2018 · 31 comments

Comments

@caffeinum
Copy link

caffeinum commented Oct 30, 2018

What problem are you trying to solve?

I don't want to store keys in browser, but I don't want to buy/use Ledger.

Describe the solution you'd like

I'd like metamask could import my addresses without private keys, and only ask for signing, which I would do myself.

There are apps that allow you to sign arbitrary transactions using QR codes on the device with no internet access.

It works like Ledger, only on your phone. An example: https://flightwallet.org

Additional context

So one of the solutions would be to:

  • when Metamask is asked to sign a tx,
  • show raw tx QR,
  • scan it on your phone,
  • sign using securely stored keys
  • use computer webcam to scan signed tx QR
  • deploy to network.
@charles-cooper
Copy link

Is this a dup of #4861?

@caffeinum
Copy link
Author

caffeinum commented Oct 31, 2018

Actually, yes, it's only now there are more apps such as Parity Signer. And they support BTC, too, so maybe we need some protocol or interface for these flows.

@r001
Copy link

r001 commented Nov 5, 2018

I want to implement this feature into Metamask as I think it is quite important to do. I base my stuff on develop branch. The way I plan it: External accounts can be imported, meaning you have to enter manually the account name. For those accounts when you want to sign something a window pops up with a QR code of the transaction json, and a place where you can insert the signed raw transaction to be sent to the system. The challenge is that we might need multiple qr codes for the raw transaction since the size of it can vary a lot. I am unfortunately not an expert in react, but will try hard.

The trick with these accounts that we can not store their private keys, since the whole point of external account is separation of signing. I'm still investigating the code.

@caffeinum
Copy link
Author

@r001 nice, I would be happy to help!

The challenge is that we might need multiple qr codes for the raw transaction since the size of it can vary a lot.

I have tested, and QR can fit a lot of data. You just use smaller squares. I works well on smartphone cameras or on recent Macbook webcams.

Moreover, we can respond not with signed tx, but pass only the signature itself.

The trick with these accounts that we can not store their private keys, since the whole point of external account is separation of signing. I'm still investigating the code.

Metamask supports Ledger, right? We can use this logic.

@r001
Copy link

r001 commented Nov 5, 2018

I have tested, and QR can fit a lot of data. You just use smaller squares. I works well on smartphone cameras or on recent Macbook webcams.
Moreover, we can respond not with signed tx, but pass only the signature itself.
If this can be accomplished, that would be great. Because I guess signature should be a fixed length thing.

@caffeinum
Copy link
Author

caffeinum commented Nov 5, 2018

@r001 https://ethereum.stackexchange.com/questions/1990/what-is-the-ethereum-transaction-data-structure

An Ethereum transaction - as in, what you pass to sendRawTransaction() - consists of the following fields, in order and RLP-encoded (note that the field names are not part of the encoded data):

nonce - transaction sequence number fr the sending account
gasprice - price you are offering to pay
startgas - maximum amount of gas allowed for the transaction
to - destination address (account or contract address)
value - eth to transfer to the destination, if any
data - all of the interesting stuff goes here
v - along with r and s makes up the ECDSA signature
r
s
Any payload, whether raw data or a contract function signature and parameters, is encoded into the data field.

Thus, we only need to pass v,r,s

@charles-cooper
Copy link

Apparently you can save some space by using alphanumeric mode for 5.5 bits per character (rather than normal binary at 8 bits per char).

I think error correction will also be an important consideration. QR codes can use built in Reed-Solomon correction but there is still a chance for the signing device to sign the wrong tx. I think the user interface needs a way for the user to be able to check the tx data (and not just by visually inspecting the individual bytes, this is too error prone because only a single byte needs to be off to have serious consequences). I think a checksum/hash on both sides (on the tx generator and the tx signer), and possibly an image representation of the hash (like some apps use to display addresses) is necessary.

@r001
Copy link

r001 commented Nov 6, 2018

I'm done with importing the external account. Now Metamask sees the newly imported address nicely, all seems to work fine, except of course transferring anything. I had to make some modification to ethereumjs-wallet implementation, where I added a feature to use fake private keys to enable to work with external accounts where evidently no private key is available. I also added tests to it. Now comes the hard part for me, since I'm not much of an expert in frontend development. I plan to alter the modal window popping up when someone clicks on an address. (It is a good start since it has QR code display, and some buttons.) My plan is to have three steps on the window: 1. the QR code. 2. Is a text saying "sign the tx on your own device". 3. Paste signature here (textbox + camera scanner) 4. an ok and cancel button.

@charles-cooper Thanks for the idea! It is really important to have the right data to come through QR codes. If I have time for that, I will add that feature too.

@caffeinum Thanks for the info. I will double check it in the yellow paper.

BTW There is and incredible work behind Metamask, congrats to the devs!

@charles-cooper
Copy link

@r001 nice work!

By the way, I'm starting to realize this is broader than just pertaining to metamask since it requires coordination between tx generators and tx signers (and tx broadcasters, if they are not the same as the tx generator). Perhaps there should be a discussion about standards around QR code signing flow. Would this be a good candidate for a standards track EIP?

@r001
Copy link

r001 commented Nov 6, 2018

@charles-cooper I actually wrote already a very simple offline signer for android (that took the transaction to sign from parity) where the challenge was that the length of an ethereum transaction is arbitrary. I solved it by simply sending multiple QR codes and checked the sequence number. So in any case we might need to transfer multiple qr codes to the signer in case the data is too long, and of course there is the way back with the signed transaction: you can send the entire raw signed tx, or as @caffeinum has suggested, only the signature, which actually makes more sense, since it is a fixed length data.

Yes, it makes sense to standardize it, since there is a clear protocol that must be made between tx creator and signer, so that multiple solutions can work together.

@caffeinum
Copy link
Author

caffeinum commented Nov 7, 2018

@charles-cooper

I think the user interface needs a way for the user to be able to check the tx data

We do that in our iOS demo app! It’s at https://flightwallet.org.

@r001

I actually wrote already a very simple offline signer for android

Do you want to cooperate on that and put our apps into the same place? We have a frontend for that, too. https://github.com/flightwallet

@ohld
Copy link
Contributor

ohld commented Nov 7, 2018

@caffeinum I've just tested your flightwallet.org and recorded some demonstration of the working prototype. Transaction was signed successfully. I've transferred some tesnet BTC to the address that was generated by the app.

https://www.youtube.com/watch?v=aZ3Zq0u_bUA

@r001
Copy link

r001 commented Nov 13, 2018

I have a question rgarding implementation, maybe the devs can help me. As far as I see, there is two strategies to implement this:

  1. The good: Implement ExternalKeyring and implement the signing methods there. This way every time a tx must be signet it is for sure end up here, and even future releases can rely on this feature. The problem with this solution that Keyrings are controlled by KeyringController that is in metamask-background that currently has no mechanism to send actions to the metamask-ui directly, so that should be implemented too, or by completely rewriting the KeyRing mechanism do something like with unconfirmed transactions.

  2. The bad: Implement this at the metamask-ui. At each "Confirm" key to pop up a modal that shows the transaction and thus enables external signing, and has an input for the raw transaction. The disadvantage of this solution is that if the metamask-ui gets rewritten, or new sign features added, the modal must be inclueded too.

  3. The ugly: Implement a "details" button on each confirmation window that enables to read the transaction data to the external signer. This way metamask would not keep record of the transaction, nor would it read the raw transaction (or signature) from the external signer. Of course this is the easiest to code.

I need a confirm from devs that in case of 1. and 2. I am on the right track.

@r001
Copy link

r001 commented Dec 6, 2018

I'm still working on the code, hopefully finish soon.

Please see transaction encoding document for efficient QR transmission.
https://github.com/r001/qr-encoding/blob/master/README.md
I hope it makes sense to you. Let's discuss it! Please feel free to propose updates on it. Please let me know, if I am off with the idea!

Note: there is no protocol for sending multiple QR codes. In the first version there will be only single QR code to send.
@charles-cooper @caffeinum

@caffeinum
Copy link
Author

caffeinum commented Dec 7, 2018

@r001 I am not sure if we better continue the discussion here or move to your repo's issues section.

  1. Did you think of building a small js example of the document?
  2. Probably it makes sense to use ethereum:// protocol in the QR, so apps could have been set up to open these signatures automatically.

Also, to the payment protocol EIP: It would be a good idea to check up with BIP70. It's a similar thing for bitcoin, but without QRs. Probably, we can take something out of these.

P.S. @r001 I put a star on your repo! 😬

@ohld
Copy link
Contributor

ohld commented Dec 7, 2018

@caffeinum: or just ‘ethereum:’

@r001
Copy link

r001 commented Dec 9, 2018

@caffeinum @charles-cooper The encoder and decoder has been created. Please find it at:
https://github.com/r001/qr-encoding
Installation and running instructions and json structure, please check:
https://github.com/r001/qr-encoding/blob/master/INSTALL.md

The external signing feature is implemented into Metamask. I will do some more testing, and css tweeeking, and also have to rebase the code.

@r001
Copy link

r001 commented Dec 10, 2018

Created the pull request here:
https://github.com/MetaMask/metamask-extension/pull/5903/files

r001 added a commit to r001/metamask-extension that referenced this issue Dec 15, 2018
@r001
Copy link

r001 commented Dec 15, 2018

@caffeinum @ohld @charles-cooper
The signer should create a signature of the form eths:/?s=0x<signature_hex>, as opposed to plain eths:0x<signature_hex> to make sure to distinguish the signature from transaction eths:/?t= that is created by Metamask. All this is to make sure each qr code has a standard url format.

I was thinking to rename eths (as of Ethereum Signature Protocol) to ethq (Ethereum QR Code Transmission Protocol). The latter one makes more sense.

@caffeinum
Copy link
Author

caffeinum commented Dec 15, 2018

@r001 I have some general comments on your protocol.

  1. There is already ethereum QR code protocol, and it uses ethereum: URI scheme. I think it won't be bad to use the same protocol. ERC: Standard URI scheme with metadata, value and byte code  ethereum/EIPs#67

As I understand, URI scheme should say, which app you should launch, and not what's inside the link (e.g. Microsoft Office links, https://docs.microsoft.com/en-us/office/client-developer/office-uri-schemes). If it's so, ethereum: is a good match.

  1. What about backwards compatibility with existing ethereum: QR code reader apps? It defines the link which can contain a destination address, amount and even contracts' ABI.

  2. As argued in the EIP67 linked thread, it makes sense to make the link human-readable. What if we use ethereum:0xDE571NA710NADD5E55?nonce=145&amount=0.5 and then build the transaction on the device? The only change from EIP67 then would be the additional nonce parameter.

P.S. I will duplicate this to issues

P.P.S. Found useful reference: ethereum/EIPs#831

r001 added a commit to r001/metamask-extension that referenced this issue Feb 14, 2019
r001 added a commit to r001/metamask-extension that referenced this issue Mar 8, 2019
@RAFI-006
Copy link

RAFI-006 commented Jan 6, 2020

Can anyone tell me how can i do transaction without asking user private key, as i am developing in my mobile app so no option for metmask

@RAFI-006
Copy link

RAFI-006 commented Jan 6, 2020

I am writng the code in C# using netherium of microsoft, Any kind of help is deeply appreciated

@caffeinum
Copy link
Author

@RAFI-006 you should look into Ethereum docs, more specifically, into netherium docs. This issue is a wrong place to look for an answers.

Basically, you'll need to know a sender address and you can build a raw transaction manually, but you still need to sign it with user private key afterwards. See example for web3.js and ethereum.js building a raw tx: https://ethereum.stackexchange.com/questions/36358/how-to-properly-create-a-raw-transaction-and-sign-it-using-web3-in-browser#36363

@domob1812
Copy link

@r001 As I understand it, you are working on this feature in #6267, right? Also it seems that the majority of the work is handling the data transfer / offline-signing part. Would it make sense to try and merge just the "watch-only account" part initially, while finalising the signing? At least for me, having watch-only addresses (even if I can't transact with them through MetaMask) would already be a huge feature improvement.

@r001
Copy link

r001 commented Sep 3, 2020

@domob1812 Hi, thank you for the interest in this feature. Actually I have been using this feature for more than a year, it works perfectly. Since for some reason (EDIT: actually the reason was partly that I was very new with node, and some concepts I just did not know about :) ) it still did not get merged I have to rebase the code for the newer versions all the time. I think I have done at least 3 updates. Maybe I will do some rebase in the coming months, so you can actually use it for signing, and of course you can do the watch-only too.

@fyookball
Copy link

I'm also interested in this feature! Can I beta test it? :)

@r001
Copy link

r001 commented Sep 6, 2020

I'm also interested in this feature! Can I beta test it? :)

If you can build it yourself, then you can already use this code at #6267. I have just rebased it to v8.0.9 of Metamask. As it is not even included into official code as of yet, you need to use it with extreme caution, and NOT in production environment.

r001 added a commit to r001/metamask-extension that referenced this issue Sep 10, 2020
@Gudahtt Gudahtt removed the P3-soon label Jan 6, 2021
@fyookball
Copy link

just checking in on the status of this. Higher security options are relevant in this bull market as investors' assets appreciate.

@AndreasGassmann
Copy link
Contributor

AndreasGassmann commented Mar 12, 2021

We would also be very interested to see this feature added to MetaMask so we can add support for it in AirGap Vault. I did not give @r001's version a try yet, but something like this is definitely needed to provide users with more secure options of storing their private keys.

@r001 Which wallet are you currently using to do the air-gap?

EDIT: After reading the other thread, I assume it's airsign :)

@fxmarty
Copy link

fxmarty commented Aug 13, 2021

Any news on this? I would love to be able to use my raspberry pi for cold private key storage.

@AndreasGassmann
Copy link
Contributor

A few weeks ago, MetaMask added QR signing capabilities. Keystone and AirGap Vault have added support for this. AirGap Vault can be installed on any device and it should also run in a Raspberry Pi (It just requires Chrome), but of course you can also build your own implementation instead if you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests