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

NEP 0413: Add signMessage method to Wallet Standard #413

Merged
merged 44 commits into from
Feb 24, 2023

Conversation

gutsyphilip
Copy link
Contributor

@gutsyphilip gutsyphilip commented Oct 12, 2022

Summary

This is a proposal for a new NEP regarding Wallet Standards. It covers adding a new method 'verifyOwner` which signs a message and verifies wallet ownership. The message is not sent to blockchain.

References

This interface is now supported by Wallet Selector as shown below:

near/wallet-selector#391

More developers within the ecosystem have also begun to add support for this to their Wallets.

@gutsyphilip gutsyphilip changed the title NEP 0XXX: Add verifyOwner method to Wallet Standard NEP 0413: Add verifyOwner method to Wallet Standard Oct 12, 2022
@frol frol added WG-wallet-standards Wallet Standards Work Group should be accountable S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. labels Oct 13, 2022
@frol
Copy link
Collaborator

frol commented Oct 13, 2022

@gutsyphilip Hey, Thanks for the contribution! Please, help us understand whether this PR is marked as drafts because you haven’t polished it from your side or are you waiting for someone’s review.

@gutsyphilip
Copy link
Contributor Author

@gutsyphilip Hey, Thanks for the contribution! Please, help us understand whether this PR is marked as draft because you haven’t polished it from your side or if are you waiting for someone’s review.

I'm working on polishing it and making sure it has all the required information. Any pointers/feedback based on what I have so far?

@frol
Copy link
Collaborator

frol commented Oct 13, 2022

@gutsyphilip I am acting as a moderator for the Wallet Standards, so I have not reviewed the proposal itself. I just wanted to make sure those who will potentially review it, set their expectations right.

@ori-near ori-near added the A-NEP A NEAR Enhancement Proposal (NEP). label Oct 13, 2022
Copy link

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Awesome work @gutsyphilip!

Mostly comments around clarity but also some questions around the approach and requests to flesh out a few more details in place 🙂

neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
@ori-near ori-near added S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. and removed S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. labels Oct 28, 2022
@ramilexe
Copy link

ramilexe commented Nov 1, 2022

Hi, I am Ramil from CIDT. We are developing OmniWallet. Regarding this PR I have following questions/ideas:

  • do we want to support binary message for signing?
  • do we want to support something similar to EIP-712 to be able to sign typed structured data?

@lostpebble
Copy link

Hi,

I added some concerns about this API in this issue on Wallet Selector: near/wallet-selector#434 (comment)

But to summarize here:

To confirm that the signing was done correctly, the actual data that needs to be validated against is the returned object from verifyOwner(), but with the signature property removed and then ran through JSON.stringify()- pretty much like this:

const owner = await wallet.verifyOwner({
  message: "test message for verification",
});

const recreatedMessage: any = {
  ...owner,
};

delete recreatedMessage.signature;

const stringified = JSON.stringify(recreatedMessage);

Basically, that recreatedMessage object (before being stringified) will look like this:

const data = {
    accountId,
    message,
    blockId: blockInfo.header.hash,
    publicKey: Buffer.from(publicKey.data).toString('base64'),
    keyType: publicKey.keyType
};

But importantly - it has to look like this exactly, if the verification is going to pass.

I find this to not be a very "deterministic" data structure, relying on exact property order on objects doesn't feel very stable.

I understand that the message is not encoded directly because of security concerns. But surely there could have been a more "stable" data structure to use in this case? Perhaps just an array with the interface of [accountId, message, blockId, publicKey] (with the public key being a fully formed string key so we don't need the keyType)? And in this case, would blockId even be necessary? I'm not very clued up on how this could be abused, but I would think an output data structure like this probably doesn't have an input area that could be abused? Also, not having blockId would mean we don't have to make any external calls at all in this process (on the wallet side).


Secondly, why are we not just sending the public key as a ed25519:* string from the get go? This would prevent having to send keyType and having to convert it using bs58 on the dApp side again. This is also the string format which can be directly compared to the public keys which are returned from the Near API for a certain account.

@gagdiez
Copy link
Contributor

gagdiez commented Nov 8, 2022

Some comments:

  1. +1 on having the public key as a simple string, as @lostpebble proposes, it simplifies the logic of checking the signature a lot!

  2. @gutsyphilip The message to be signed should not be optional (i.e. it should be string and not string?), this simplifies the implementation for wallets and makes clear to the devs using it that they need to sign a message.

  3. @lostpebble The currently proposed structure is not so terrible, since the ordering when encoding to a string is clear, also, having a dictionary-like helps the structure to be more explicit.

  4. @ramilexe This is not to sign arbitrary messages, but to sign a very specific one, that will be used exclusively as an authentication token.

@gutsyphilip Giving that the proposal was made 27 days ago, could we aim to pass it from a draft to a proposal by the end of week? what do you think?

neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
@gagdiez
Copy link
Contributor

gagdiez commented Nov 9, 2022

@gutsyphilip based on some @toteto comments, lets please make sure to include a section in which we explain exactly what each parameter is.

For example, signature is a string, but particularly a base64 string that represents something.

@esaminu
Copy link
Contributor

esaminu commented Feb 14, 2023

@gagdiez @abacabadabacaba Ethereum's analogous NEP seems to be more generic https://eips.ethereum.org/EIPS/eip-712. IMO the NEP looks good after the latest changes though the state parameter should be required in the input interface and forwarded by wallets as is to the output interface. It's true that the callbackUrl and state parameters are specific to browser wallets, EIP-712 just has a generic callback function but it's fine to include them as optional parameters and mark them as required in browser implementations in the NEP.

If it is separated then this could be generic and the other NEP could be browser wallet specific. I don't understand how a message signing NEP will not inevitably be used for authorization and to prove ownership of an account so I don't see the need to separate in that sense.

To be clear IMO we should:

  1. Add the state parameter to the input interface and the output interface instructing wallets to forward that parameter as is and dApps to provide a unique session token and validate it when called back
  2. Mark state and callbackUrl as only required for browser implementations
  3. Merge this NEP

@jon-lewis
Copy link

IMO the NEP looks good after the latest changes though I think the state parameter should be required in the input interface and forwarded by wallets as is to the output interface

As an alternative, we could keep this NEP as-is and, for auth, instruct devs to put the callbackUrl and state into the message field, that way they both get signed and sent back to the callbackUrl. One drawback would be the displayed message in wallets may initially be ugly.

This would allow us to move forward and iterate in future NEPs.

@gagdiez
Copy link
Contributor

gagdiez commented Feb 16, 2023

@esaminu @jon-lewis @abacabadabacaba I added a sentence suggesting to keep a state variable, and to pass it as the message when requesting signatures for authentication.

P.S. What I don't fully understand is why the nonce cannot be used as a state in this case. But that is a topic for another thread.

neps/nep-0413.md Outdated Show resolved Hide resolved
gagdiez and others added 2 commits February 16, 2023 16:28
Co-authored-by: Jon Lewis <jonathandavidlewis92@gmail.com>
@frol frol mentioned this pull request Feb 20, 2023
2 tasks
@gagdiez
Copy link
Contributor

gagdiez commented Feb 21, 2023

@abacabadabacaba @esaminu please take a look at the comment above, sorry for the ping, just trying to keep the ball rolling on this NEP.

Copy link
Collaborator

@abacabadabacaba abacabadabacaba left a comment

Choose a reason for hiding this comment

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

@gagdiez I see that some of my concerns are addressed. However, I feel that the issue with state is not fully resolved.

Asking developers to include state in the message would not only lead to a confusing API for developers. Also, the users will be asked to approve signing some random string as a "message". This will lead to increased alert fatigue among the users, as they would be trained to approve confirmation prompts without understanding them. But not showing the message would undermine the system, as it would be possible that the user didn't see the message they signed.

For this reason, the state and the message should be separate. Also, the state doesn't need to be signed. And the error response should include the state as well, even though it doesn't include the signature, so that random websites could not activate an app's callback even with a fake error response.

neps/nep-0413.md Outdated Show resolved Hide resolved
neps/nep-0413.md Outdated Show resolved Hide resolved
gagdiez and others added 2 commits February 23, 2023 10:53
Co-authored-by: Vlad Frolov <frolvlad@gmail.com>
@gagdiez
Copy link
Contributor

gagdiez commented Feb 23, 2023

@abacabadabacaba thank you for the clarification. I now added state as an optional input/output for web wallets, and added a paragraph explaining in which context it is necessary. I think with this all the security concerns are covered.

Copy link
Collaborator

@abacabadabacaba abacabadabacaba left a comment

Choose a reason for hiding this comment

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

@gagdiez Indeed they are. While this NEP is far from perfect from the design perspective, at least using it shouldn't result in security issues in the apps as well as the wallets.

@frol frol merged commit b91f1b0 into near:master Feb 24, 2023
@frol
Copy link
Collaborator

frol commented Feb 24, 2023

@abacabadabacaba Thanks for standing on guard for the security of the NEAR ecosystem!

@gagdiez Thanks for addressing all the concerns!

Thanks to everyone involved for shaping the NEAR ecosystem!

@nearbuild
Copy link

would be great if next steps and call to actions post merge were put on this thread @frol

@frol
Copy link
Collaborator

frol commented Mar 2, 2023

@nearbuild The next step is getting this NEP implemented in Wallet Selector (see the call for contributors) and Wallets.

@Cameron-Banyan Have you had a chance to broadcast it to the Wallet Builders community?

Non-web Wallets, such as [Ledger](https://www.ledger.com) can directly return the `SignedMessage` (in preference as a JSON object) and raise an error on failure.

## References
A full example on how to implement the `signMessage` method can be [found here](https://github.com/gagdiez/near-login/blob/main/tests/authentication/auth.ava.ts#L27-#L65).

Choose a reason for hiding this comment

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

@gagdiez is there any way/example on how to verify the signature for browser wallets?

The verifySignature function in the example needs the message, nonce, recipient and callbackUrl to re-construct the payload which was actually signed but we lose all this data on redirect?

What we get back in the URL is only the SignedMessage.

Copy link
Contributor

@gagdiez gagdiez Mar 10, 2023

Choose a reason for hiding this comment

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

with the current approach, you will need the user to pass it back to you. If you are implementing an app in which the user is asked to sign a message, then you will need to persist somewhere the parameters, redirect the user to the wallet, get the key+signature from fragment, and the rest of the parameters from the persisted storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP A NEAR Enhancement Proposal (NEP). S-approved A NEP that was approved by a working group. WG-wallet-standards Wallet Standards Work Group should be accountable
Projects
Status: APPROVED NEPs
Development

Successfully merging this pull request may close these issues.