leather-io / extension

Leather browser extension
https://leather.io
MIT License
304 stars 142 forks source link

Expose method to sign arbitrary messages #1051

Closed aulneau closed 2 years ago

aulneau commented 3 years ago

There will be times when an app needs to sign an arbitrary message. This functionality exists in wallets in Eth, but we don't currently expose a function to do this in stacks apps.

It's suggested that we should make an equivalent to https://eips.ethereum.org/EIPS/eip-712

from discord:

ok so in the Eth world there's tons of arbitrary message signing right, for DeFi and DeX and whatnot

It always used to be the case that you'd sign an order hash, but that's not very readable for the user. (MetaMask popup shows you just a hex string, you have no idea what you are really signing.)

EIP712 defined a structure (in json) that makes it possible to show what that order hash consisted of

so you see the actual fields that went into generating the hash that you are signing

image

MarvinJanssen commented 3 years ago

Thanks for posting this! I think such a feature is crucial.

aulneau commented 3 years ago

Thanks for posting this! I think such a feature is crucial.

I agree, and I think many on the team would agree, too 👍 I'll work to update this issue as we talk through it.

markmhendrickson commented 3 years ago

@MarvinJanssen Mind indicating the use case(s) you'd like to build in the short-term with this to help weigh priorities?

MarvinJanssen commented 3 years ago

@markmhx talking short-term, there is really only one reason why I need it.

I am creating a server-side component that tracks user app bucket URLs (for zonefiles). In order to secure this API, the user should sign his/her bucket URL with Connect. The server then resolves the user's reported BNS name via the BNS contract and uses the corresponding principal to verify the signature. I basically need to verify that a specific message actually came from a principal who owns a name on BNS. If you have alternative solutions, let me know. My backup plan was introducing simple Clarity contract and having the user submit a proof via contract call, but that makes it a workaround for a workaround for a workaround...

psq commented 3 years ago

there are 2 main other use cases I've seen signatures being used on Eth:

MarvinJanssen commented 3 years ago

@psq yep long term we need it for DeFi and DeX, and platforms that do server-side matching that require user signatures. (Like 0x Protocol and Wyvern on Ethereum.)

markmhendrickson commented 3 years ago

Thanks for the additional context 🙏 I'm assigning to Jasper so he can design.

GinaAbrams commented 3 years ago

I see this is in the icebox @markmhx does that mean it will be considered in a later sprint? @MarvinJanssen is awaiting updates 🙂

markmhendrickson commented 3 years ago

I've moved back this issue back into the Kanban backlog.

Note that we're no longer working against 2-week sprints but rather incremental delivery of priorities as reflected in the backlog from top to bottom.

hstove commented 3 years ago

I am totally on board with evaluating a system for signed typed data structures, but that's a much bigger engineering task that I don't think we should take on first. The main benefit of the structured data is to fee-less on-chain transactions with logic tied to the data in that signature. However, two things:

Strings are the most obvious use case for Stacks at the moment. It would allow for the very common use case of providing off-chain access to a user that can prove they own an STX address. For example, a web app that gives specific functionality if you own an NFT.

To be more concrete about use cases:

markmhendrickson commented 3 years ago

It was determined during a conversation with @MarvinJanssen and @GinaAbrams yesterday that the .BTC app won't have an urgent need for arbitrary message signing unless two things happen sequentially:

  1. We're unable to deploy Atlas fixes to mainnet within the next couple of weeks.
  2. He's unable to implement the zonefile reporting mechanism he needs as a workaround using a custom Clarity smart contract and transaction signing and broadcasting with the Stacks Wallet extension.

The arbitrary message signing route is basically a fallback to the above routes, which are preferable in the order listed. It would involve sending signed messages with zonefile data to a centralized server, which would save the end user the cost of having to sign a transaction but also incur the undesirable involvement of a centralized party.

@agraebe @diwakergupta I'll check in with splat next week to see how the Atlas deployment plans are going so we can provide @MarvinJanssen with an update here in regards to timing and how it impacts his options.

He's happy to wait a week or two to see if we can get the fixes live on mainnet and save him the work of either workaround.

hozzjss commented 3 years ago

This is an invaluable thing to have, using signatures from the identity would prove the ownership of a name and a whole lot of things, especially when it comes to off-chain operations, 100% for this

MarvinJanssen commented 3 years ago

See this feature request: https://github.com/blockstack/stacks-blockchain/issues/2693. Clarity types are great for this sort of stuff as they are very readable. We could have a mechanism to hash a serialised CV and then sign it.

Here is a quick concept:

sign structured data concept
2snEM6 commented 3 years ago

This would also be great for bitcoin transaction signing, it would allow pools to implement bitcoin wallet functionality on the web, for instance to withdraw payouts, without needing to instruct the user to user third party wallets or ask the user for the private key (this is a no go). Is there any way I can help speed up this?

Thanks.

2snEM6 commented 3 years ago

I understand an EIP-712-like SIP is not really a blocker for this, rather an improvement. Correct?

MarvinJanssen commented 3 years ago

There would be virtually no way to use the resulting signatures in smart contracts without https://github.com/blockstack/stacks-blockchain/issues/2693.

(Some types can be hashed in Clarity but it's pretty limited.)

markmhendrickson commented 3 years ago

The UserX team will discuss the prioritization of this enhancement early next week as we head into Q3.

jasperjansz commented 3 years ago

Two quick drafts, how do these look to everyone?

image

MarvinJanssen commented 3 years ago

I like it! Is it only a UI draft or is there an upcoming technical spec?

aulneau commented 3 years ago

Two quick drafts, how do these look to everyone?

image

This is good, but there isn't always the guarantee that it will be structured data. it could be a hash or random message, too.

I think the left side one is better in my opinion, and perhaps we can have one that shows the same code block component we use for contract deploy?

markmhendrickson commented 3 years ago

I agree that the left-side one looks better, because the warning message above the submission button is much more noticeable and useful than placing it below the button.

Copy tweak suggestion: "Learn more about messaging signing".

This link could take the user to an FAQ on Hiro.so, but perhaps it should go to the docs instead. We should start thinking about docs for devs and users alike in regards to how messaging signing works. cc @pgray-hiro

Some use-case examples would be helpful here. E.g. "App X wants to help user Y do Z so it asks the user to sign a message that it will then use to do A, B and C."

beguene commented 3 years ago

The left-side looks better but I think the right is less confusing: the text to sign and the warning message are clearly separated, we know for sure that what we sign does not include the warning message itself.

gzelda commented 3 years ago

Share a specific message for off-chain that proves you own a STX address

  • sign a string, the "easy" version of this issue

Any progress of stacks-wallet about signing string message?

markmhendrickson commented 3 years ago

@jasperjansz will provide updated designs here soon

jasperjansz commented 3 years ago

Moving to backlog: https://www.figma.com/file/VupGh90FJtT0dcBj2Sh7bb/%F0%9F%93%8FSpec?node-id=429%3A7084

markmhendrickson commented 3 years ago

I've added some inline comments to Figma above ☝️

friedger commented 3 years ago

Use case: Created DID token for magic link: https://magic.link/docs/decentralized-id

Zk2u commented 3 years ago

hey squadaroos

we need this for Halo's authentication in the next 2-5 months as part of our pkAuthTime framework, which was specifically designed for our <50ms request time goal. any updates would be awesome, any prioritisation would be ⚡️⚡️

we're happy to help design the spec and eventual SIP for this at the Guild

pseudozach commented 2 years ago

any update on this? This is a basic functionality that should exist. Proving that user controls privatekey for account x in short +1 here's a use case: Satoshibles team have a discord-bot that tracks who owns what NFT. App keys are not usable to prove cryptographically in the backend that user x with address y owns nft id z. cc: @markmhx

fbwoolf commented 2 years ago

any update on this? This is a basic functionality that should exist. Proving that user controls privatekey for account x in short +1 here's a use case: Satoshibles team have a discord-bot that tracks who owns what NFT. App keys are not usable to prove cryptographically in the backend that user x with address y owns nft id z. cc: @markmhx

Yep, this is on our roadmap for Q1. Thanks for the feedback.

aviculturist commented 2 years ago

+1 super important feature.

davidfant commented 2 years ago

We need this to sign users in to a dapp with a backend. Basically, users sign in using their Stacks address, and our backend needs to be able to verify that a user can sign for the address they say they have. This is what we're currently doing in our backend for MetaMask signin:

const message = 'Some arbitrary signin message';
const address = '<the user\'s address>';
const signature = '<some signature created using metamask>';
const addressThatSignedMessageWithSignature = ethers.utils.verifyMessage(message, signature);
assert(address.toLowerCase() !== addressThatSignedMessageWithSignature.toLowerCase());
MarvinJanssen commented 2 years ago

@davidfant this is my take on it: https://github.com/stacksgov/sips/pull/57

I'd like to skip over a personal_sign equivalent for Stacks. You can find some reasoning in the draft.

Zk2u commented 2 years ago

Worth noting, we have a smallish implementation of this in Storm. Looks something like this.

Screenshot 2022-02-01 at 14 30 08
markmhendrickson commented 2 years ago

This functionality is now in development.

MarvinJanssen commented 2 years ago

Amazing!

@markmhx can you give more insight into what specifically is in development? Are we getting structured data, something like SIP018, or something else?

andresgalante commented 2 years ago

Hi @MarvinJanssen, yes, the idea is to follow SIP018. I'll let @beguene follow up with more details.

markmhendrickson commented 2 years ago

@landitus Can you update the designs here to include some of the new data included in the mockup from @beguene in our project brief?

image

Note that we also need an error state in the case in which the app provides a message and hash that don't match:

image
MarvinJanssen commented 2 years ago

@markmhx why is the app supposed to provide both the hash and the message? Just the message is enough.

landitus commented 2 years ago

FYI, we now have a milestone to group related issues: https://github.com/hirosystems/stacks-wallet-web/milestone/60

landitus commented 2 years ago

Here's an update on the designs for the message signing UI with the use-cases we've identified so far.

Use-cases

Let us know your feedback or any questions!

AMS - 1 AMS - 2 AMS - 3

Figma

fiftyeightandeight commented 2 years ago

This is great and in line with our expectation. Can I please check users will also have the ability export their pub key (also through API), so that dapps can easily verify the signature?

beguene commented 2 years ago

@fiftyeightandeight Yes this is planned, the signature along with the pubKey will be returned to the dapp. The dapp will then be able to verify the signature using stacks.js (independently of the Hiro wallet)

beguene commented 2 years ago

@landitus We should update the design and wording a bit, as the user will under the hood sign the hash in all cases: ECDSA always hashes the message before signing to prevent signature forgery. I think we can always show the hash box component below and have the button just be 'Sign'.

kyranjamie commented 2 years ago

+1 to comment in signing language Sign message would be okay.

Is "Your signed messages can be used to authorize transactions" correct? At least via the Ledger, messages are prefixed, preventing this. If we are actually signing entirely arbitrary messages, we'll need changes to the Ledger app as well, yes?

beguene commented 2 years ago

@kyranjamie Yes after discussing with @markmhx we are allowing the user to sign of any kind of messages including transactions, therefore we don't prefix the message in this wallet flow. Indeed this path won't work with the current Ledger app which prefixes all messages. The Ledger app needs remove the prefix too. We can always later add the prefix before sending to Ledger if we want a safer flow.

landitus commented 2 years ago

@landitus We should update the design and wording a bit, as the user will under the hood sign the hash in all cases: ECDSA always hashes the message before signing to prevent signature forgery. I think we can always show the hash box component below and have the button just be 'Sign'.

Here are the updated designs:

AMS - 2 1 AMS - 2 2 AMS - 2 3

Figma

MarvinJanssen commented 2 years ago

@kyranjamie Yes after discussing with @markmhx we are allowing the user to sign of any kind of messages including transactions, therefore we don't prefix the message in this wallet flow. Indeed this path won't work with the current Ledger app which prefixes all messages. The Ledger app needs remove the prefix too. We can always later add the prefix before sending to Ledger if we want a safer flow.

I vote against allowing direct access to the sign function. Just look at the lead up to Ethereum's personalSign to see why it's not great. We'll get an influx of people getting tricked into signing tx hashes by malicious dapps. Not to mention if legitimate apps get attacked causing them to serve malicious JS code.

SIP018 is prefixed for that reason. If "message only: string format" is a string-ascii/string-utf8 under the hood then it is covered too. Unless consider adding a prefix bite.

I like having the hash hidden by default.

aulneau commented 2 years ago

I also think it would be a mistake to expose the ability to sign any payload

markmhendrickson commented 2 years ago

@MarvinJanssen @aulneau We've discussed internally the idea of omitting the prefix but detecting when the user is prompted to sign a valid transaction and showing an additional warning / confirmation screen (e.g. "⚠️ You appear to be signing a transaction. Are you sure you want to do that?").

The idea being that it might leave open more possible use cases for apps (e.g. an app for queuing transactions for later broadcast based on network conditions?) while still preventing users from getting duped (since the UX would be as obvious signing a transaction for broadcast normally).

Is your sense that this still wouldn't alleviate the security concern for some reason? Or that there aren't such valid use cases for signing transactions that are returned to the app?

If so, we can certainly go the prefix route here instead. I'm just inclined to support the widest range of possible use cases as possible, assuming we don't compromise on security of course.