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

Closed
wants to merge 9 commits into from
Closed

NEP-461: Prefix Tag for Signed Messages #461

wants to merge 9 commits into from

Conversation

gagdiez
Copy link
Contributor

@gagdiez gagdiez commented Feb 14, 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 added the S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. label Feb 14, 2023
@gagdiez gagdiez requested a review from a team as a code owner February 14, 2023 16:51
@gagdiez
Copy link
Contributor Author

gagdiez commented Feb 14, 2023

@abacabadabacaba would you like to be a coauthor of this NEP, since it was basically your proposal? Please let me know.

@gagdiez gagdiez changed the title Prefix Tag for Non-Transactional Messages - DRAFT WIP: Prefix Tag for Non-Transactional Messages Feb 14, 2023
@gagdiez gagdiez marked this pull request as draft February 14, 2023 16:57
@gagdiez
Copy link
Contributor Author

gagdiez commented Feb 14, 2023

Please notice that right now we are proposing only to use the prefix in non-transactional, in this way, we don't need to rush a protocol change, only change meta-transactions NEP a little bit

@morgsmccauley morgsmccauley marked this pull request as ready for review February 14, 2023 17:43
@morgsmccauley
Copy link

Sorry @gagdiez misclicked and can't move it back to draft..

@frol frol added A-NEP A NEAR Enhancement Proposal (NEP). WG-wallet-standards Wallet Standards Work Group should be accountable labels Feb 14, 2023
@frol frol marked this pull request as draft February 14, 2023 17:51
@ori-near
Copy link
Contributor

Hi @gagdiez – thank you for starting this proposal. As the moderator, we labeled this PR as "Needs author revision" because we assume you are still working on it since you submitted it in "Draft" mode.

Please ping the @near/nep-moderators once you are ready for us to review it. We will review it again in early April, unless we hear from you sooner. We typically close NEPs that are inactive for more than two months, so please let us know if you need more time.

@frol frol added WG-protocol Protocol Standards Work Group should be accountable and removed WG-wallet-standards Wallet Standards Work Group should be accountable labels Feb 14, 2023
@abacabadabacaba
Copy link
Collaborator

@gagdiez Sure, and we will also need to make sure that any existing proposals that involve using account keys adhere to this scheme.

@gagdiez gagdiez changed the title WIP: Prefix Tag for Non-Transactional Messages WIP: Prefix Tag for Signed Messages Feb 15, 2023
@gagdiez gagdiez changed the title WIP: Prefix Tag for Signed Messages Prefix Tag for Signed Messages Feb 16, 2023
@gagdiez gagdiez 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 Feb 16, 2023
@gagdiez gagdiez marked this pull request as ready for review February 16, 2023 15:34
neps/nep-0461.md Outdated Show resolved Hide resolved
neps/nep-0461.md Outdated Show resolved Hide resolved
@Ekleog-NEAR
Copy link
Contributor

A few questions (disclaimer: I came from reviewing near/nearcore#8578):

  • Why should the discriminant be explicit for classic transactions, but implicit for other messages?
  • What if a NEP needs to define two message types? IMO we should just "be IANA" and have one number, not directly related to the NEP number, per message kind. We could then decide to reserve one range for "experimentation purposes" so it could be used during experiments and not-yet-approved NEPs
  • Why do we need a different range for on-chain messages and off-chain messages? (It might be easier for a human to distinguish whether the message is on- or off-chain by peering at bytes, but we can just have our "be IANA" policy actually do that without enforcing that in the range)

I would also like to suggest an alternative design for the same problem, which should simplify implementation. We could for now simply say:

if a message starts with "N461" (which should be way out of the range of valid transactions already), then parse it as Nep461Message, which is a borsh-serializable enum that can get new variants with any NEP

This would cost us 4 more bytes for the binary format of NEP 366, but I don’t think it’d actually be a problem? It would also help us not have to worry that much about having multiple types of messages, though we could still decide to introduce new numbers (with us as the IANA registry as per my questions above) if we ever hit a use case that would benefit from reducing the size by 4 bytes. (Long-term we could maybe envision transitioning the current Transaction to also be encoded as an N461 variant, which could help us simplify our code again, but this would be outside of the scope of this alternative proposal)

Basically, it would be like saying "we are IANA, currently messages starting with 0-64 are Transactions and messages starting with "N461" are Nep461Message", so it would be as extensible as the solution suggested by my questions above.

WDYT?

@frol
Copy link
Collaborator

frol commented Feb 20, 2023

Thank you @gagdiez for submitting this NEP.

As a moderator, I reviewed this NEP and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-protocol working group members to assign 2 Technical Reviewers to complete a technical review (see expectations). If you want to assign yourself, please mention that you are acting as the Technical Reviewers.

Please find some guidelines below for completing the technical review.

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 recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation)

    • A summary of benefits

    • A summary of concerns and blockers that were identified on the way and their status (some will be resolved, others dismissed, etc)

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.

@frol frol added S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. and removed S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. labels Feb 20, 2023
@jakmeier
Copy link
Contributor

jakmeier commented Feb 21, 2023

Great thoughts and valid questions Léo! I am also interested in the answers.

Regarding your proposal

Basically, it would be like saying "we are IANA, currently messages starting with 0-64 are Transactions and messages starting with "N461" are Nep461Message", so it would be as extensible as the solution suggested by my questions above.

I see one technical disadvantage. When a new NEP is in its prototyping, approval, or testing phase, we already need a number. The IANA approach would only assign the number once approved, I assume. Libraries would want to include a way to construct unapproved messages, to make prototyping possible. The "NEP-number = message tag" approach makes this straight forward. In the IANA approach I am not quite sure how that would work, but surely we could work around it somehow.

@Ekleog-NEAR
Copy link
Contributor

I think in the "we are IANA" approach, we could deal with that by setting a range that would be for experimentation purposes, similar to what was done with eg. ipv4 class E. While developing the messages would then use magic numbers from this class using whatever scheme the devs want to use, and as soon as the NEP lands only the constant magic number would need to change to the allocated one :)

neps/nep-0461.md Outdated Show resolved Hide resolved
@abacabadabacaba
Copy link
Collaborator

@Ekleog-NEAR @gagdiez We can reserve the range starting with $2^{31}+2^{30}$ for experimentation/private use. Basically, say that no standard is going to use such prefixes.

Regarding NEP number vs we are IANA approach: I think that it is desirable to have less bureocracy. If a NEP needs multiple prefixes, we can allocate some, but this should be rare.

Regarding using enum for different message types: this will also serialize to different prefixes. Moreover, it is unlikely that any code will need to deserialize a message as an enum, because each piece of code is going to deal only with one message type. In fact, a prefixed message shouldn't be stored anywhere, the prefix should only be added to create/verify a signature. So, this is different from how enums are normally used.

@frol frol changed the title Prefix Tag for Signed Messages NEP-461: Prefix Tag for Signed Messages Feb 27, 2023
Co-authored-by: Evgeny Kapun <abacabadabacaba@gmail.com>
@gagdiez
Copy link
Contributor Author

gagdiez commented Mar 23, 2023

Were are we standing with this, I assume it was already used for the early implementation of meta transactions? @firatNEAR

Copy link
Member

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

IMO we should just "be IANA"

This introduces some complexity which could be problematic (security-wise) for apps that tries to encode different types of messages in the first four bytes. Using the NEP number seems like a straightforward way to use this proposal.

Recommendation

I recommend approving this NEP.

Benefits

It addresses a potential vector of attack that could be created in the feature unintentionally. It brings awareness to NEP developers and helps align them on a proper solution that fits current and future use cases.

Concerns

# Concern Resolution Status
1 We need to make sure classic tx encoding is not changed in the future.

neps/nep-0461.md Show resolved Hide resolved
neps/nep-0461.md Show resolved Hide resolved
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
Member

Choose a reason for hiding this comment

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

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.


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
Member

Choose a reason for hiding this comment

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

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
Member

Choose a reason for hiding this comment

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

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.

neps/nep-0461.md Show resolved Hide resolved
Comment on lines +19 to +23
| Implicit Prefix | Message |
| - | - |
| $< 2^{30}$ | [Classic Transactions](https://nomicon.io/RuntimeSpec/Transactions) |
| $2^{30}$ + NEP-number | On-chain Actionable Message, e.g. [`SignedDelegateAction`](https://github.com/near/NEPs/blob/master/neps/nep-0366.md) |
| $2^{31}$ + NEP-number | Off-chain Message, e.g. [`signMessage`](https://github.com/near/NEPs/pull/413) |
Copy link
Member

Choose a reason for hiding this comment

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

Should we explicitly reserve the range [3 * 2^30, 4 * 2^30) as unused / experimentation / undefined?

@Ekleog-NEAR
Copy link
Contributor

This introduces some complexity which could be problematic (security-wise) for apps that tries to encode different types of messages in the first four bytes. Using the NEP number seems like a straightforward way to use this proposal.

Uhhh I’m not sure I understand your concern? The reason why I said that we should "be IANA" is exactly because using the NEP number introduces quite a bit of code complexity in security-sensitive code. To be more precise, all the bit-twiddling in near/nearcore#8578 would have been replaced with a simple if discriminant == [the number we assign to the meta-tx NEP] { ... }.

It does add some process complexity when creating a new NEP (we need to give it one ID), but I think this is much less harmful security-wise than having more code complexity in areas of the code dealing with signatures.

If we really want to use the NEP number, then at least we should remove the part about on-chain and off-chain messages being in two different ranges, that just adds useless complexity to our numbering scheme… unless there is a reason for this that I cannot think of?

@bowenwang1996
Copy link
Collaborator

As a working group member, I nominate @DavidM-D and @robin-near as SMEs to review this NEP.

@ori-near ori-near added S-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. and removed S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. labels Mar 28, 2023
@robin-near
Copy link
Contributor

Re: IANA vs NEP, we could also take the approach often used by Google and Meta - reserve a global ID (maybe by committing to a file in the repo, maybe an enum definition) even for experimentation, and the production ID will be the same. Some IDs will end up being dropped if the NEP is not ultimately implemented, but it's fine to have gaps anyway. If we stick with using NEP, we could also add one more byte to the prefix to allow multiple messages.

Either way, overall it seems that the primary concern is with how to make sure everybody is aware of the same scheme and sticks to the same scheme. Could we tackle that with changing the code-level APIs so that it is very difficult to implement new features that involve signing messages not conforming to the scheme that we decide on? Not being able to compile one's code will force them to look for what they are supposed to do.

@gagdiez gagdiez closed this by deleting the head repository May 18, 2023
@frol
Copy link
Collaborator

frol commented May 19, 2023

@gagdiez Hey, did you intent to close this NEP?

@gagdiez
Copy link
Contributor Author

gagdiez commented May 20, 2023

@frol not at all :huge_face_palm: I'll check if there's a simple way to restore it, worst case scenario I can open a new PR and we reference the discussion

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: RETRACTED
Development

Successfully merging this pull request may close these issues.

10 participants