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-483: Prefix Tag for Signed Messages #483

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

gagdiez
Copy link
Contributor

@gagdiez gagdiez commented May 24, 2023

A proposal to include a standardized prefix in all signed messages that are not transactions.

Particularly, we propose to prepend the prefix $2^{30}$ + NEP-number actionable messages (i.e. those that are processed on-chain), and the prefix $2^{31}$ + NEP-number to all non-actionable messages (i.e. those destined for off-chain use).


NEP Status (Updated by NEP moderators)

SME reviews:

Voting indications:

Protocol Work Group voting has not started yet.

@gagdiez gagdiez requested a review from a team as a code owner May 24, 2023 09:16
@gagdiez gagdiez changed the title NEP-461: Prefix Tag for Signed Messages NEP-483: Prefix Tag for Signed Messages May 24, 2023
@gagdiez
Copy link
Contributor Author

gagdiez commented May 24, 2023

A revival of #461 which I mistakenly closed.

Maybe this is great moment to get back in the move, since the previous PR was stagnated for two months. I know that making NEPs is a slow process, but it is always highly rewarding!

cc 1. @robin-near @Ekleog-NEAR @mfornet @jakmeier @firatNEAR @abacabadabacaba you were all part of the conversation.

cc 2. @frol @bowenwang1996 @DavidM-D @robin-near you were part of the committee appointed to this PR.

Including a large `u32` prefix allows to easily distinguish transactions from messages, ensuring that they never overlap.

```ts
sha256.hash(Borsh.serialize<u32>(large-number) + Borsh.serialize(MessageStructure))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

original comment by @mfornet

Before reading the suggestion, I thought about the user signing a message of this type:

Suggested change
sha256.hash(Borsh.serialize<u32>(large-number) + Borsh.serialize(MessageStructure))
Borsh.serialize<u32>(large-number) + sha256.hash(Borsh.serialize(MessageStructure))

It is 4 bytes longer, but the advantage is that the wallet (signing device) can tell apart the target (i.e transaction/on-chain/off-chain) by just looking at this payload, rather than requiring the whole message. It is probably not safe anyway to sign a payload without seeing the message, but it might make sense for low-end devices (like some hardware wallets) where having the whole message is expensive or not possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfornet wouldn't this have the problem that sha256(transaction) could collide with borsh(prefix) + sha256(MessageStruture)? If all transactions are of the form sha256(prefix + transaction) then by definition there cannot be a collision.


We will need to update [NEP366](https://github.com/near/NEPs/blob/master/neps/nep-0366.md) to follow this standard, and make sure all future NEPs follow it, which might be hard to track.

If at any point there is an update on how transactions are encoded, this NEP could stop being valid.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

original comment by @mfornet

If any protocol change updates the tx encoding, we should enforce that the first 4 bytes are used following this NEP. (i.e., they must be decoded as a number in the range [0, 2**30).

What do you think the best way is to be alert of this? (Putting some comments in the client source code?)

Copy link
Contributor Author

@gagdiez gagdiez May 24, 2023

Choose a reason for hiding this comment

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

original comment by @mfornet

I propose introducing a "protocol update" that enforces the first four bytes of every "classic tx" to be in the proper range. This is the case today (implicitly), but we should add it to the spec, and an extra assertion to the client (no real protocol change required) so we don't break this in the future.

@gagdiez
Copy link
Contributor Author

gagdiez commented May 24, 2023

The last comments were involved on the important discussion of IANA vs NEP-prefix on tagging the transactions. I will try to summarise the discussion here:

The current proposal is to implement a message prefix in the form:
sha256( borsh(NEP-number) + borsh(MessageStructure) )

@Ekleog-NEAR proposed to use the IANA approach, since that would allow to simply make if discriminant == standard, and one NEP could handle multiple prefixes.

discriminant + sha256( borsh(MessageStructure) )

@abacabadabacaba and @jakmeier raised security concerns, since the discriminant could potentially be encoded in a way that collides with other transactions, and plead for keeping the proposal as it is.

@mfornet was proposing to stick with the NEP-prefix, but in the form of a non-serialized prefix:

borsh(NEP-number) + sha256( borsh(MessageStructure) )

proposal  pros cons
IANA  simpler to read we would need to have a way to centralise and communicate the IANA prefixes
encoded(NEP+message)  straightforward prefix need to decode first 4 bytes to check the message type
NEP+encode(message)  straightforward prefix, no need to decode first 4 bytes adds 4 bytes of information

I personally think that we can keep the current proposal unmodified since:

  • NEPs should present only one type of message, if multiple are present, then that signals the need for more NEPs (e.g. it has not been a problem so far in building NFT / FTs as multiple NEPs)
  • Adding the NEP + encode(message) would make the transaction more human readable, but on the actual implementation has little impact since anyway you need to "check first four bytes and act on them", one pass of deserializing 4 bytes does not save significative computational resources (i.e. time, processing power).

@jakmeier
Copy link
Contributor

Noteworthy that we already had to pick something when implementing delegate actions to enable meta transactions.
There we went with "the current proposal". But to be more precise, here is exactly what we implemented.

To sign a message, we use discriminant(nep) as a prefix, where the discriminant in constructed as described in the main comment of this PR (add 2^30 or 2^31 to the NEP number).

From that, we define a new signature method, let's call it sign_nep483.

sign_nep483(msg, nep) := sign(sha256(discriminant(nep) ++ borsh(msg)))

sign refers to an ed25519 or secp256k1 signature.

I think so far this is exactly what the "current proposal" is. But we had to make one more decision, which is aobut what to send across the wire. We went with this:

SignedMessage {
    signature := sign_nep483(msg, nep),
    msg := msg,
}

Note that msg is without the prefix. This means the actual prefix on the wire is only the signature. The receiver has to know (or guess) the NEP to verify the signature, as it is not included in the transmitted message.

I believe this makes sense because the purpose of the prefix is to avoid encoding collision attacks. It it not meant as a way to give types to messages, that IMO should happen independently of this NEP.

You may find the full implementation as it is currently running in mainnet here: https://github.com/near/nearcore/blob/489b9767c38eef28d6f467846f8f938e602ecc94/core/primitives/src/signable_message.rs

@frol frol added WG-protocol Protocol Standards Work Group should be accountable A-NEP A NEAR Enhancement Proposal (NEP). S-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. labels May 24, 2023
@frol frol requested review from a team and bowenwang1996 May 24, 2023 20:15
@frol
Copy link
Collaborator

frol commented May 24, 2023

you were part of the committee appointed to this PR.

This NEP needs to be reviewed by @near/wg-protocol work group. I am only playing a moderator role here.

@gagdiez
Copy link
Contributor Author

gagdiez commented May 25, 2023

sign_nep483(msg, nep) := sign(sha256(discriminant(nep) ++ borsh(msg)))

@jakmeier indeed, this is exactly the "current proposal", glad to see the NEP already being implemented <3 , we now wait for the @near/wg-protocol work group to give their verdict.

Of course, meanwhile the community can keep commenting and working on the NEP if any major concerns arise.

@bowenwang1996
Copy link
Collaborator

bowenwang1996 commented May 29, 2023

As a working group member, I nominate @Ekleog and @abacabadabacaba as subject matter experts to review this NEP.

Technical Review Guidelines
  • First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.
  • Second, once all the suggestions are addressed, produce a Technical Summary, which helps the working group members make a weighted decision faster. Without the summary, the working group will have to read the whole discussion and potentially miss some details.
    Technical Summary guidelines:
    • A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation). Please note that this is the reviewer's personal recommendation.
    • A summary of benefits that surfaced in previous discussions. This should include a concise list of all the benefits that others raised, not just the ones that the reviewer personally agrees with.
    • A summary of concerns or blockers, along with their current status and resolution. Again, this should reflect the collective view of all commenters, not just the reviewer's perspective.
      Here is a nice example and a template for your convenience:
### Recommendation
Add recommendation
### Benefits
* Benefit
* Benefit
### Concerns
| # | Concern | Resolution | Status |
| - | - | - | - |
| 1 | Concern | Resolution | Status |
| 2 | Concern | Resolution | Status |

Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.

@DavidM-D
Copy link
Contributor

DavidM-D commented Sep 7, 2023

I approve this NEP

@walnut-the-cat
Copy link
Contributor

Hello, NEP moderator here.

@Ekleog and @abacabadabacaba , are you still the right SMEs to review this NEP?

@gagdiez , could you confirm if this project is still ongoing?

@gagdiez
Copy link
Contributor Author

gagdiez commented Nov 20, 2023

@walnut-the-cat as far as I know it has even been implemented for some NEPs (particularly, meta-transactions), so yes, this is still valid and worth reviewing

@Ekleog-NEAR
Copy link
Contributor

Oh right I totally forgot about this, thank you. I won’t have the time to review again today, and will be away the rest of the week, but I can try to review early next week.

That said, TBH considering all the work on this was already landed and is already part of our protocol, I’m not sure it makes sense to finish the NEP review process with this one. I still have the same concerns I had given on NEP-461, about the currently-written layer mix between NEP number (which is an internal detail of our specification process) and message tag. Incidentally, I cannot find the message where @jakmeier or @abacabadabacaba have raised the security concerns listed in the summary above, and I do not understand what they would be.

But what would be the point of raising this again now, considering all the work on this had already landed three months before I was tagged as a NEP reviewer?

So I’d argue in favor of bypassing the NEP process here, acknowledging that we messed up by landing the code (long) before reaching an agreement on what the NEP contents should be, and landing this PR to document the implementation choices we made.

@walnut-the-cat WDYT?

@jakmeier
Copy link
Contributor

So I’d argue in favor of bypassing the NEP process here, acknowledging that we messed up by landing the code (long) before reaching an agreement on what the NEP contents should be, and landing this PR to document the implementation choices we made.

I don't think landing meta transaction is the same as deciding on a standard for NEP-483.

I admit, the timing of events was bad here.
But it's not necessarily wrong to solve a problem for a specific feature (NEP-461) first and then later decide on a generally acceptable standard.

The purpose of NEP-483 is, as I understand it, to agree on how contextually bound signatures should be implemented for future cases, inside and outside the core protocol. Meta-transaction needed to solve one possible conflict of a key being shared for signature in different contexts and hence we came up with a solution before the standard was ready.

As more things are signed with user account keys, there are more and more potential conflicts. Thus, I can see value in formally reviewing and working out the details for a standardized solution. Having meta transaction already shipped can even be a useful precedent to understand practical implications of this particular choice.

All that said, I think we also need to think about priority of different proposals. I don't know how high it is for this specific proposal, given that nobody really seems to be waiting for a resolution on it right now.

@walnut-the-cat
Copy link
Contributor

@Ekleog-NEAR and @abacabadabacaba , if you agree with what @jakmeier says, let's have another look at the NEP and see what we can align on. Please focus on general applicability than specific usecase when reviewing the NEP.

Let's target December 6th as the new review completion date.

@Ekleog-NEAR
Copy link
Contributor

Before handing in my technical report, I have one comment: I do not think this numbering scheme is good, because:

  1. It uses the whole space of remaining message IDs, thus making it impossible to extend later on
  2. It does not allow one NEP to define two new message types
  3. The separation in IDs between on-chain and off-chain is not, as far as I can tell, motivated

Currently, there is a single message type that we can no longer change: on-chain NEP 366, ie. 2**30 + 366.

As such, I suggest changing the numbering scheme to be (B = 2**30):

  • [B, B + 2**20): Space reserved for the NEP number. Each NEP can use without any process the message types [B + NEP << 4, B + (NEP + 1) << 4), thus allowing each NEP up to 16 message types without any process, for NEP numbers up to 2**16
  • [B + 2**20, B + 2**21): Space reserved for experimentation. Numbers here MUST NOT be used by any production system
  • [B + 2**21, 2**32): Reserved for future use, eg. if one NEP ever were to require more than 16 message types.

The only issue will be backward compatibility with NEP 366. If we allowed a single message type for each NEP, it’d be easy, so this is one solution. Another would be to use the numbering scheme I suggest, and just have NEP 366 use a number from the region of NEP 22 (= 366 // 16), which we know doesn’t use any anyway.

WDYT?

@walnut-the-cat
Copy link
Contributor

cc. @gagdiez for @Ekleog-NEAR 's comment

@walnut-the-cat
Copy link
Contributor

@gagdiez , happy new year. Would like to resurface this per @Ekleog-NEAR 's comment and want to hear your thoughts.

@gagdiez
Copy link
Contributor Author

gagdiez commented Jan 8, 2024

I am not necessarily opposed to what @Ekleog-NEAR is proposing, but I would like to point that it would already conflict with two NEPs (and thus 3 types of signed message) at least:

Touching on other points:

@Ekleog-NEAR I assume the difference between 2**30 and 2**31 was to make very simple to discard messages that were NOT supposed to be relayed to the blockchain based on the prefix.

@Ekleog-NEAR I remember there was a discussion on how many messages an NEP can have, and that maybe an NEP with multiple messages was indeed multiple NEPs.


Now I would like to remember everyone that I created this document after the protocol team requested help writing the NEP. With this, I mean that I do not have strong opinions on the exact ranges the messages should live in, or even if the "message prefixes" should be standardized.

It seams, given the amount of time this PR has been opened - and the diverse opinions that different team members present - that the protocol team cannot reach consensus on the idea... at least not through this async channel. Thus, I encourage the moderators to reach all important parties and take the discussion to a live and synchronous channel (e.g. a videocall) and reach out to me to keep helping with the NEP writing.

@Ekleog-NEAR
Copy link
Contributor

Thank you for your answer, I hadn’t known of NEP-413! And this one is actually problematic, as it uses 2^31 + 413 as a prefix tag, which is not the one of any other low-number NEP. Once more, I’m very sad that we built upon this NEP without first seeing it to completion, because now we have a fractured NEP range that makes it hard to have a good numbering scheme :’(

As for easily discarding messages that are not one of the known types, we’ll have to match on the message type anyway to know what to do with the message, so I don’t really see a point in doing so.

This being said, here is a new numbering proposal, that should keep working with NEP-413 while still avoiding the forward-compatibility issue that with the current definition we’re burning the whole available NEP range:

  • [0, 2**30): Regular transactions
  • [_, 2**30 + 2**22): 2**18 blocks of 2**4 numbers, allowing for up to 262144 NEPs of up to 4 message types
  • [_, 2**31): Space reserved for future use
  • [_, 2**31 + 2**20): Space reserved for manual allocation of message types for NEPs that’d need more than 4, NEP-413 would fall here
  • [_, 2**32): Space reserved for future use

WDYT?

Also, to answer @gagdiez’s comment, does anyone know who had defined the previous scheme, as it seems like they might object to what I’m suggesting? All I know of is the discussion in #461, and I don’t think any objection to a scheme like this was raised there, there were just objections to the "we are IANA" approach for developer velocity

@walnut-the-cat
Copy link
Contributor

@gagdiez, any thoughs on @Ekleog-NEAR 's proposal?

@victorchimakanu
Copy link

Hello @gagdiez @Ekleog-NEAR , I'd like to confirm the current status of this NEP?

@flmel
Copy link
Member

flmel commented Sep 27, 2024

Hello, I want to resurface this NEP @gagdiez @Ekleog-NEAR can you confirm you want to proceed with this NEP ?

@Ekleog-NEAR
Copy link
Contributor

I have no power on this beyond the comments I already posted, so there's not much value I can add here now.

@gagdiez
Copy link
Contributor Author

gagdiez commented Oct 1, 2024

@flmel added @Ekleog-NEAR comments on the NEP, this should be ready to be reviewed

@Ekleog-NEAR
Copy link
Contributor

Almost there! There is still just one last thing before I can give my r+: there is still a mention of the distinction between on-chain messages and off-chain messages. In particular, there's still mention of $2^{30}$ + NEP-number, which is incompatible with the other numbering scheme I was suggesting.

So at least one of the two numbering schemes needs to be removed from the NEP before landing, because it currently is self-contradictory.

@walnut-the-cat
Copy link
Contributor

Hey @gagdiez , we plan to pull in more engineers to review this NEP and move it forward. Having said that, the NEP is quite old and I think it will be great if you can summarize what have been discussed so far so, the NEP Reviewer can only pay attention to the latest summary + NEP itself.

@bowenwang1996
Copy link
Collaborator

As a working group member, I nominate @Longarithm as a SME to review this NEP

@shreyan-gupta
Copy link

I was walking through this NEP and all the previous discussions and context associated and I had a couple of comments.

Firstly, I understand this NEP was originally proposed but not approved however NEP-366 DelegateAction and NEP-413 signMessage have already been approved and implemented and built on top of the ideas of this proposed NEP. Confirming that this NEP needs to be backward compatible and respect the implementations of NEP-366 and NEP-413? I don't suppose we can change the specifications of serialization of DelegateAction and signMessage? This btw changes what can possibly be proposed as part of this NEP.

Secondly, I didn't quite understand what is the exact purpose of this NEP? Is it to just define a definitive way to distinguish transactions from non-transactions? Or is it to add very clear specification on what future messages should look like and adhere to it? I believe it's the former?

In which the case, I don't understand why are we adding specific details like + NEP-number to this proposal? Instead this proposal needs to be as generic as possible, where we maybe just specify the range of usable numbers and nothing more. Future NEPs should take the burden of figuring out which exact prefix to use and being backward compatible with existing messages.

This also avoids the problem mentioned of multiple new messages being added with a future new NEP. Infact we don't even need to ideally specify what non-transaction messages are structured like as long as we can clearly specify what transaction messages should look like and no one else should overlap with that space.

And lastly, I noticed we have NEP-541 which adds transaction priority fees which may be related to this NEP. While txn priority is still at the proposal stage, I do believe it would change Transaction to VersionedTrasaction (or TransactionV1 vs TransactionV0) as we are adding a new field priority_fee. I believe that should perhaps be a good time for us to actually define a good structure for the serialization of Transaction instead of talking about what non-transactions should look like.

Thoughts?

@Longarithm
Copy link
Member

TLDR, I think the proposal should be improved in the following ways:

  1. Looks like that "Classic Transactions" must be extended with "Any messages signed on chain". Because the same problem arises with messages circulating within validator network, e.g. Approval, ChunkEndorsement, AnnounceAccount. They even have the highest priority, because compromising these may compromise the whole chain.

  2. Present why it is better than alternatives, in the space of prepending different prefixes:

  • IANA, NEP+encode(message) as described in @gagdiez' comment
  • other ways to ensure uniqueness of prefixes, as @shreyan-gupta says
  1. Specification must also include high-level details. In particular, an answer how are we going to ensure that implementors of new messages follow the protocol.

More detailed:

Similarly to @shreyan-gupta' comment, I'm not convinced that we need to use NEP number in on-chain messages. Because all these messages are defined in nearcore and are likely to belong to one crate, this crate can as well maintain array of used prefixes, in the range [0, 2**30).

As for off-chain messages, should we consider defining a registry for commonly used ones? This is what "Future possibilities" section is for. Without it, separating messages just with NEP number makes sense, but again, I'd like to see more clarification how this will be controlled. A reference to SignableMessage in https://github.com/near/nearcore/blob/489b9767c38eef28d6f467846f8f938e602ecc94/core/primitives/src/signable_message.rs could be fine.

Moreover, we need to define migration from all current signed messages, or explain why it can be skipped for some messages. This migration must involve protocol upgrade.

With that said, the security issue with signed messages is very clear. One of the goals of my comment is about making the path to solving this issue clearer.

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-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. WG-protocol Protocol Standards Work Group should be accountable
Projects
Status: NEW❗
Status: REVIEW
Development

Successfully merging this pull request may close these issues.