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

Function to request wallet sign a message (a string, not a transaction). #318

Closed
lukenguyen-me opened this issue Jun 8, 2022 · 20 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@lukenguyen-me
Copy link

lukenguyen-me commented Jun 8, 2022

Is your feature request related to a problem? Please describe. My application has a feature that require user to sign a message (a string of some data we defined ourself) using their wallet, and our backend will verify later.

Describe the solution you'd like Add function named "signMessage" (for example) took a string as parameter and return signed string.

Additional context Refer to similar function in EVM

/**

  • @params sign - ethers.providers.JsonRpcSigner
  • @params message - string data need to be signed */ await signer.signMessage(ethers.utils.arrayify(message));
@lukenguyen-me lukenguyen-me added the enhancement New feature or request label Jun 8, 2022
@lewis-sqa
Copy link
Contributor

lewis-sqa commented Jun 8, 2022

Hi @phucloc8697,

Thanks for raising this enhancement. I'd like to dig a little deeper into the use case of this. What's the intention for signing exactly? Will the message ever be sent to the blockchain or is maybe the idea to use this as a way to verify ownership of the message later on (as you mentioned by the backend)?

I should set the expectation that we're at the mercy of what the wallets we support can do. Off the top of my head, this should be possible with Ledger and Math Wallet but I'm almost certain Sender for example doesn't provide low-level signing like this. It only exposes signing (and sending) Transactions

@lukenguyen-me
Copy link
Author

Hi @lewis-sqa ,

  1. Yes, I would need it to verify ownership of the message. Nothing will be sent to the blockchain.

  2. If so, could you throw an exception in case the wallet doesn't provide signing method? Beside that, I would contact Sender and ask if they can provide this feature.

@lewis-sqa
Copy link
Contributor

lewis-sqa commented Jun 9, 2022

@phucloc8697 thanks for providing some clarity on this 👍

I did some investigation with Sender as I realise it's since implemented the Account class/interface from near-api-js which happens to ultimately expose the Signer functionality so in theory it should be possible!

We've raised a ticket internally to address this enhancement. We'll keep you posted here on the progress 🙂

@lukenguyen-me
Copy link
Author

@lewis-sqa Nice one! Thanks for your support

@lewis-sqa
Copy link
Contributor

@phucloc8697 We've ran into a couple of problems regarding this feature:

  1. There's been some concerns raised by core members of the NEAR team that signing arbitrary messages can be quite dangerous and leaves users exposed to potential 'sign phishing scams' that other chains such as Ethereum (EIP-191) have ran into. They later moved to structured messages as part of EIP-712 that helped solve the issue. NEAR are looking to mimic something similar and define a structure that's specific to the use case of verifying ownership.
  2. During the testing of SQC-153: Support for verifyOwner #320, I found (My) NEAR Wallet and Ledger to be the only reliable wallets that could sign anything other than transactions. Math Wallet hangs and Sender works only when the wallet is unlocked (if previously locked the user has to refresh the page to take effect). This doesn't mean we can't go ahead in the future when NEAR has published a spec for verifying ownership, but it does mean the abstraction we place around wallets will be broken as dApp developers can't assume any wallet will support this feature.

@lukenguyen-me
Copy link
Author

@lewis-sqa Thanks for working around. I understand the issue.

Btw, after some quick research, I found that the near-api-js does support method sign in class nearAPI.utils.key_pair.Signature . I can create key pair from BrowserLocalStorageKeyStore, and use this method generate a signed message, however this will not have the wallet to request user interaction (run in background). Is this way useful for this feature?

@lewis-sqa
Copy link
Contributor

@phucloc8697 So I believe a Signer.signMessage implementation tends to be backed ultimately by a KeyPair's sign method. The issue is really the security concerns of signMessage as arbitrary messages lack the protection detailed in EIP-712.

The block on this feature for us with Wallet Selector is the lack of spec from NEAR around how a dedicated verify ownership method will work. We're hoping to get some clarity on this in the next week

@mohamedalichelbi
Copy link

I can create key pair from BrowserLocalStorageKeyStore, and use this method generate a signed message.

Few considerations:
The key in your Dapp's localStorage will be a FunctionCall access key. It's the key generated upon user login, not the "main" key used to approve arbitrary TXs. But you should be able to use it for signatures. Just keep in mind that a NEAR account can always add or delete keys, so you'll also need to save the time at which the message was signed. When verifying the signature, you'll need to fetch the account's keys' list at the signature time.

@agileurbanite
Copy link

Looks like this might be a blocker for AstroDAO so need to consider for prioritization.

@janewang
Copy link
Contributor

@agileurbanite This should go out in today's release. cc @MaximusHaximus

@kujtimprenkuSQA
Copy link
Contributor

@phucloc8697 , @agileurbanite NEAR Wallet Selector v6.0.0 has been published on NPM.
https://www.npmjs.com/org/near-wallet-selector

This issue has been addressed in this PR: #391 . To find out which wallets support this feature at the moment read the description of the mentioned PR.

@austinabell
Copy link

I disagree that this interface should be a string. Will this not limit the functionality if you want to sign binary data in the future? Am I misunderstanding JS types that string can be used in some way from a Buffer/UInt8Array?

Related #405 (I am blind and didn't notice this issue when I created)

@austinabell
Copy link

austinabell commented Aug 17, 2022

Also, it really does not make sense that the format of what is signed is inferred by the implementation of wallet selector, some examples:

async verifyOwner({ message }) {
logger.log("MathWallet:verifyOwner", { message });
const account = getActiveAccount(store.getState());
if (!account) {
throw new Error("No active account");
}
const accountId = account.accountId;
const pubKey = await _state.wallet.signer.getPublicKey(accountId);
const block = await provider.block({ finality: "final" });
const data = {
accountId,
message,
blockId: block.header.hash,
publicKey: Buffer.from(pubKey.data).toString("base64"),
keyType: pubKey.keyType,
};
const encoded = JSON.stringify(data);
// Note: Math Wallet currently hangs when calling signMessage.
throw new Error(`Method not supported by ${metadata.name}`);
const signed = await _state.wallet.signer.signMessage(
new Uint8Array(Buffer.from(encoded)),
accountId,
options.network.networkId
);
return {
...data,
signature: Buffer.from(signed.signature).toString("base64"),
keyType: signed.publicKey.keyType,
};
},

const data = {
accountId,
message,
blockId: block.header.hash,
publicKey: Buffer.from(pubKey.data).toString("base64"),
keyType: pubKey.keyType,
};
const encoded = JSON.stringify(data);

This can be restrictive if someone wanted to sign custom data. This will be restricting for certain use cases like signing data to be verified within a contract call. Right now, with this, it signs some custom data that is generated with the string provided.

@janewang
Copy link
Contributor

cc @MaximusHaximus please see above.

@AmmarHumackicSQA let's track this in our board so we could discuss. The feature is requested by a few large project and we could improve the implementation if needed, thank you.

@AmmarHumackicSQA
Copy link
Contributor

Hey everyone, the NEP for verifyOwner is in the works and here you can check the draft of it: near/NEPs#413

@gagdiez
Copy link

gagdiez commented Nov 8, 2022

Is there any progress on this? The community keeps asking for it on a monthly basis.

@toteto
Copy link

toteto commented Nov 8, 2022

Is there any progress on this? The community keeps asking for it on a monthly basis.

☝️ Agree, this issue should take higher priority!

@AmmarHumackicSQA
Copy link
Contributor

Hey everyone, the NEP for verifyOwner is in the works and here you can check the draft of it: near/NEPs#413

As stated here ☝️, this is a work in progress. I see you guys @gagdiez @toteto already posted some questions there. Thanks!

@gagdiez
Copy link

gagdiez commented Nov 15, 2022

@MaximusHaximus @lewis-sqa @AmmarHumackicSQA @austinabell, we have greatly advanced the NEP413, your feedback is appreciated. We are reaching consensus in the community, so maybe we could start readily implementing it in the wallet selector?

@exalate-issue-sync
Copy link

Trentin Bergeron commented:

Please see NEP → near/NEPs#413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.