-
Notifications
You must be signed in to change notification settings - Fork 1k
Adds message signing to wallet #4286
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
base: master
Are you sure you want to change the base?
Adds message signing to wallet #4286
Conversation
Adds the ability to sign arbitrary messages using wallet accounts. The user can specify the elliptic curve to use for signing (secp256r1 or secp256k1). It generates a payload using salt and the provided message, then signs the payload with each available wallet account, displaying the address, public key, salt and signature.
| var signature = Crypto.Sign(payload, key.PrivateKey, selectedCurve); | ||
|
|
||
| ConsoleHelper.Info("Address: ", account.Address); | ||
| ConsoleHelper.Info(" PublicKey: ", key.PublicKey.EncodePoint(true).ToHexString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont really support secp256k1, the key you get is still p-256 pubkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think I saw in the code for Crypto.Sign that both were supported so that is why I added it.
Do you want me tor remove the option to select curve and only use secp256r1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a structured format that specifies the address, public key, and algorithm for signing. This also facilitates verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions:
-
Should we require the user specifiy either the public key or address for signing?
-
If yes to question 1: If there is only one address in the open wallet, could we default to using this one? I think this will make signing more user friendly.
-
For specifing the algorithim, do you mean the curve or padding, salt, etc.? Or both?
-
Can we provide defaults for each option so that the command become user friendly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we need to define an output format including address, public key, and algorithm of the signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest commit includes the algorithm and curve used + some helpful text and link to documentation on how to verify the generated signatures.
@erikzhang, is this what you wanted?
Or did you want a stuctured format in terms of JSON or YAML?
Or did you want us to define a standard for the padding and salting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want a standard with json format please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikzhang I made a draft: neo-project/proposals#210
| w.Write((byte)0x01); | ||
| w.Write((byte)0x00); | ||
| w.Write((byte)0x01); | ||
| w.Write((byte)0xF0); | ||
| w.WriteVarBytes(paramBytes); | ||
| w.Write((ushort)0); | ||
| w.Flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the meaning of this script, some standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, could you clarify it @adrian-fjellberg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is to ensure that the message is not also a valid NEO transaction. I do not think it is a defined standard but it is the way both NEON wallet and NeoLine signs messages.
Do you want me to add comments to the code to clearify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, its just some prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add comments to the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments added in the latest commit.
|
I also started on documenting the command line feature: |
Refactors the `sign message` command to: - allow only signing only using the secp256r1 curve. - output the algorithm used to create the payload - output guide on how to verify the signed message Renames the `sign` command to `sign transaction` to avoid ambiguous calls with the new `sign message` command. Refactors the `OnCommand` method on the ConsoleServiceBase class so that parsing of arguments are differed to later in the execution. This is to solve an issue that occurs when ambiguous calls occur, one method might output errors in when parsing of arguments.
…message-sign' into feature/cli-arbitrary-message-sign
| /// <param name="jsonObjectToSign">Json object to sign</param> | ||
| [ConsoleCommand("sign", Category = "Wallet Commands")] | ||
| /// <param name="jsonObjectToSign">The json string that records the transaction information</param> | ||
| [ConsoleCommand("sign transaction", Category = "Wallet Commands")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sign tx is shorter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Fixed 👍
|
Is this armed for Neo4 or next release? @adrian-fjellberg |
|
|
|
|
As fast as possilbe beacuse it will be important for nodes to be able to do offline signing in a secure way to claim candidate roles in the new Goevernance platform set to be released soon. So I should change to |
| using (var w = new BinaryWriter(ms, Encoding.UTF8, true)) | ||
| { | ||
| // We add these 4 bytes to prevent the signature from being a valid transaction | ||
| w.Write((byte)0x01); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to follow something like the https://www.cyfrin.io/blog/understanding-ethereum-signature-standards-eip-191-eip-712, domain separator it's important to avoid signature replays between different chains, like testnet and mainnet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neo/src/Neo/Network/P2P/Helper.cs
Lines 71 to 88 in ac7ebab
| public static byte[] GetSignData(this IVerifiable verifiable, uint network) | |
| { | |
| /* Same as: | |
| using MemoryStream ms = new(); | |
| using BinaryWriter writer = new(ms); | |
| writer.Write(network); | |
| writer.Write(verifiable.Hash); | |
| writer.Flush(); | |
| return ms.ToArray(); | |
| */ | |
| var buffer = new byte[SignDataLength]; | |
| BinaryPrimitives.WriteUInt32LittleEndian(buffer, network); | |
| verifiable.Hash.Serialize(buffer.AsSpan(sizeof(uint))); | |
| return buffer; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrian-fjellberg we should call this method, let me add some changes. Also this PR should be moved now to https://github.com/neo-project/neo-node
|
I only need the posibilty for nodes to sign an arbitrary message... We get 100% value from the current implementation. It satisfies all the needs of the reason behind the proposed change. The value is:
|
Adds the ability to sign arbitrary messages using wallet accounts. The user can specify the elliptic curve to use for signing (secp256r1 or secp256k1). It generates a payload using salt and the provided message, then signs the payload with each available wallet account, displaying the address, public key, salt and signature.
Description
This PR introduces a new
sign_messagecommand to the Neo CLI, enabling message signing outside of transactions.Previously, the CLI only supported signing serialized transaction data, which made it impossible to produce verifiable message signatures for use in external applications, authentication workflows, or dApp integrations.
With this change, users can now easily sign and verify arbitrary messages directly from their local wallet.
Change Log
Fixes #4285
Type of change
How Has This Been Tested?
sign_messageon local CLI with both curvesChecklist: