feat: signable message prefix for meta tx

[jakmeier]

This is a necessary change to the meta transaction signature scheme to
address security related concerns with the original proposal.

Meta transactions introduce DelegateAction and
SignedDelegateAction where the signature key is an account key.
Transactions are also signed by account keys.

We don't want to have the possibility that the borsh serialization of a
Transaction and a DelegateAction end up with the same binary
representation. In that case it would be indistinguishable whether the
account owner signed the Transaction or the DelegateAction, which
opens the door to various security vulnerabilities.

There is the draft NEP-461 to solve this in a general way. To push
forward with meta transaction, we will use that proposed schema.
Regardless of whether the standard gets accepted or not, at least this
solves the problem for meta transactions and makes it possible to add
more types of messages later that are also signed with account keys.

This is not a protocol change. It only affects meta transaction which
have not been stabilized yet.

[jakmeier]

This should be ready now. I believe this addresses all concerns that were open regarding the signature schema.

I also tried to make the changes more than just a local fix to the meta transaction problem. Essentially already (roughly) implementing what is proposed in NEP461. But there are some differences in my implementation here and the NEP, see my comment here: near/NEPs#461 (comment)

@abacabadabacaba it would be great to have your insights on this, what you would suggest to use exactly as the hash input.

@Ekleog-NEAR I also included you as a reviewer, I think your system security + Rust experience would be handy here, if you have time for a review. (You should be able to understand the issue regarding the ambiguous signature without understanding meta transaction.) I'm also interested regarding a clean crate interface, as I introduce quite bit of extra pub types and methods here.

And just for context: We kind of want this merged ASAP because several teams working on meta transactions are blocked on these changes to near-primitives not being published on crates.io. Also, to get meta transactions ready on mainnet this quarter, we need to stabilize meta transaction by Monday... But otherwise, no time pressure ;)

[bowenwang1996]

Overall looks good to me. One thing that I don't quite understand is the NEP argument to MessageDiscriminant initializers. What is the purpose of taking a nep number as an argument?

[bowenwang1996]

The naming of this function feels a bit confusing

[jakmeier]

Good point. I renamed it to get_nep461_hash to make clear this is specially constructed hash according to the rules dictated by nep461. And I added a comment. I hope this makes it clearer.

[jakmeier]

Overall looks good to me. One thing that I don't quite understand is the NEP argument to MessageDiscriminant initializers. What is the purpose of taking a nep number as an argument?

NEP-461 defines that the NEP number should be part of it. It's just the direct implementation of that.

SignableMessageType is a typed wrapper around to reduce user errors.

Why not only have strongly typed constructors? The problem I try to solve here is for the next NEPs that add a signable messages. They will need to update the definition of SignableMessageType. That can be tricky to prototype and test.
In nearcore, we can just add a nightly feature for it and depend on the code that way. But client applications such as near-cli depend on the crates.io version of the code. They couldn't even create a new kind of MessageDiscriminant before the feature is stabilized and we release a new version of near-primitives.
This way, they can at least create such messages and send it to e.g. betanet that already understand the new type of message.

[Ekleog-NEAR]

Disclaimer: I don’t have all the context around meta transactions, besides the birds-eye-view of what they’re trying to do.

This seems to me like a very clever solution to a problem that we shouldn’t have. Would it be possible to, instead of knowing that account ids are limited to 64 bytes and taking advantage of that, instead just pay the cost of a protocol change now and make what’s signed always be an #[non_exhaustive] enum SignedMessageContents { Transaction(Transaction), DelegateAction(DelegateAction) } ; without having to use tricks to not sign the discriminant? (Though we’d still need to make sure this is prefixed by some magic number to not conflict with current signatures) It seems to me like this protocol change would be minimal in volume and allow us to avoid all the bit-shifting; and if an account key is literally only ever used to sign such a SignedMessage then we’d know without a doubt that we’re safe.

Also we may want to add an expiration time to the signed messages, so that we would avoid issues with transactions created years ago that would be like current transactions and limit our agility — something like:

struct SignedMessage {
    magic: u32, // = some clever magic number that is not possible to have with current transactions
    expiry_timestamp: u64,
    contents: SignedMessageContents,
}

but that’d probably be quite a lot to implement for protocol changes; so maybe just leaving some space for that field and mandating it to be 0 for now would make sense — we’d need to make validators not include transactions that have an expiry timestamp in the next 5 minutes and then other validators would reject blocks that have blocks with expired timestamps or something like that.

Also, beyond code quality, signing more than the actual message makes me a bit nervous (because of implicit parts), but I don’t have enough context to speak confidently about it. I’d need to know literally all the places where we use an account key for signing in order to speak more about it.

Now, what I’m suggesting also would mean that all old tooling would need to be updated… but AFAICT this is only for RPC, so it means that as soon as we’ll have left a reasonable amount of time elapse we can remove the old code paths and only accept new SignedMessage blurbs any longer.

[jakmeier]

Thanks for taking a look!

This seems to me like a very clever solution to a problem that we shouldn’t have. Would it be possible to, instead of knowing that account ids are limited to 64 bytes and taking advantage of that, instead just pay the cost of a protocol change now and make what’s signed always be an #[non_exhaustive] enum SignedMessageContents { Transaction(Transaction), DelegateAction(DelegateAction) } ; without having to use tricks to not sign the discriminant?

Not sure I understand this... We are already signing the discriminant in this proposal. The magic is that for transactions the discriminant is also part of the transaction struct.

Yes, we could avoid the magic by making a protocol update and including the discriminant in the message body. But that would have huge consequences. I am guessing here but it seems all wallet implementation would need to change how they sign things. Even the ledger hardware wallet integration. And we would add 4 dead bytes to every single transaction and future type of message that will be invented, which I am not sure is worth it.

All in all, for meta transactions we won't be able to make such a huge change. And if we really want to avoid the magic later, the involved protocol change would be so big that a small change to how meta transactions work would probably be the least of our problems.

Also we may want to add an expiration time to the signed messages

I can see the concern but I don't think this is the right layer to address it. Today, normal transactions define that with the block hash on which they build, meta transactions set a maximum block height. Like that, I imagine each type of message will want to define what is an acceptable lifetime is for them.

[Ekleog-NEAR]

Not sure I understand this... We are already signing the discriminant in this proposal. The magic is that for transactions the discriminant is also part of the transaction struct.

Right, sorry, this is a leftover from my initial misunderstanding of the changes, I missed removing this one after understanding.

I am guessing here but it seems all wallet implementation would need to change how they sign things. Even the ledger hardware wallet integration.

Good point, I hadn’t thought of that, thank you!

And we would add 4 dead bytes to every single transaction and future type of message that will be invented, which I am not sure is worth it.

Is this a problem? (serious question)

I think even just replacing the bit-shifting discriminant magic with using a magic 4-byte value for the signature would be enough to address the problems I mentioned (although not the ones about possible future changes to Transaction, so that’d need good documentation and/or implementing BorshDeserialize in a way that works also for raw Transactions)

To clarify, what I’m now suggesting is in addition to Transaction, we can deserialize a SignedMessageContents that always starts with a magic 4-byte value, then is the borsh serialization of an enum with one variant, DelegateAction. We can still deserialize raw Transactions. This would already avoid I think all the bit-magic that you’re doing, at the cost of a magic 4 bytes value at the beginning (which we could probably lower to a 1-byte value if that’s actually important, given accounts are currently <64 in size so we have 256-64 values currently free for the first byte)

Then we could do the following changes to clean up the code, though that’s not necessary for avoiding the bit-magic already:

• We add a Transaction variant to the SignedMessageContents enum, that does basically the same as deserializing from a raw Transaction
• We make our SDK actually generate this variant rather than the enum
• Enough time after, we deprecate generating the raw Transaction
• After careful analysis of transactions that actually get included, maybe in a few years we can actually remove the code for deserializing raw Transactions

Today, normal transactions define that with the block hash on which they build, meta transactions set a maximum block height.

Makes sense; my thought was "we need a way to invalidate messages that we no longer know how to deserialize so we could reuse these bitpatterns for other things", but given your message about wallets I think it makes sense that we’ll not actually want to do that, so it’s better to just have one expiry mode per type.

[jakmeier]

And we would add 4 dead bytes to every single transaction and future type of message that will be invented, which I am not sure is worth it.

Is this a problem? (serious question)

I don't know it it is a problem. For transactions and meta transactions, the overhead of 4B for every transaction seems small enough. (10M daily transactions would only make state grow by an extra 40MB per day.)
But we can't predict all use cases. This should be flexible enough that it can be used for all kinds of messages you want to attest for using an account on the NEAR blockchain.

I think even just replacing the bit-shifting discriminant magic with using a magic 4-byte value for the signature would be enough to address the problems I mentioned (although not the ones about possible future changes to Transaction, so that’d need good documentation and/or implementing BorshDeserialize in a way that works also for raw Transactions)

To clarify, what I’m now suggesting is in addition to Transaction, we can deserialize a SignedMessageContents that always starts with a magic 4-byte value, then is the borsh serialization of an enum with one variant, DelegateAction. We can still deserialize raw Transactions. This would already avoid I think all the bit-magic that you’re doing, at the cost of a magic 4 bytes value at the beginning (which we could probably lower to a 1-byte value if that’s actually important, given accounts are currently <64 in size so we have 256-64 values currently free for the first byte)

I don't really understand what concern you solve with this. The problem we have with meta transactions and NEP-461 wants to fix more generally is that Borsh serialized structs don't inlcude type information. This tag, implicit or not, will solve the case where somebody maliciously mixes different types, or even message bodies that aren't borsh encoded.

What I like about the implicit prefix is that it only affects the signature schema, instead of requiring specific details inside the body. A compatible implementation just has to use the NEP-461 signature schema and is 100% free to chose the body. The content could be a JSON, it could be an executable, an E-Mail, or anything else. This can also be used easily in programming languages that don't have borsh support.

All that said, I don't think it's a big issue to change. But this decision should be left to the authors of NEP-461 (@gagdiez , @firatNEAR , @abacabadabacaba). I just want to make meta transactions compatible with what they suggest to make the standard.

[jakmeier]

heads up: I will merge this & release near-primitives on Monday morning.

We just had a meta tx sync across teams and this is needed for other teams. Deadlines are all super tight and we can't afford to wait longer. If we have to make changes afterwardsto the structs I introduce here, we will have to make another near-primitives release. But for now we assume the current implementation is okay to move forward as it is.

[jakmeier]

This is now merged to unblock a few things.

But feedback is still welcome, I am happy to create another PR to make changes where necessary.

[Ekleog-NEAR]

Sorry about that! I had somehow missed the notifications.

What I like about the implicit prefix is that it only affects the signature schema, instead of requiring specific details inside the body. A compatible implementation just has to use the NEP-461 signature schema and is 100% free to chose the body. The content could be a JSON, it could be an executable, an E-Mail, or anything else. This can also be used easily in programming languages that don't have borsh support.

Even if we do switch to a magic number + enum variant, we could still have an enum variant that’s just a Vec<u8> for borsh, so I don’t think this would preclude anything. Basically, my thoughts are that we could reduce complexity in security-sensitive parts of our code, but as soon as this will be stable we no longer will be able to (because we’ll have to keep backwards-compatible with the signatures that have this very high discriminant forever). Now I don’t think there’s any particular issue with the current implementation, it’d just be a way to reduce the risk that we all missed something :)

[jakmeier]

@Ekleog-NEAR no worries at all, I didn't expect a response before Monday!

I'm not quite sure what your suggestion is here. Do you just want to use [u8;4] instead of u32 for the tag / discriminant? Isn't that equivalent? I am already thinking of the tag as 4 bytes and the enum as a type safe wrapper around that to avoid accidents. The "bit magic" just comes in because I tried to keep the implement close to how it's described in the specification https://github.com/near/NEPs/pull/461/files

[Ekleog-NEAR]

I guess even keeping the exact NEP 461, I’d just rather like if we had a system that was not actually fully generic like NEP 461 suggests, but only actually pattern-matched on "is it a Transaction or a DelegateAction" with a (like now) magic number for DelegateAction; as this would I think significantly simplify the code. (And well I just submitted my thoughts on NEP 461 itself as well :))

[jakmeier]

Thanks Léo, I see what you mean now.

Continuing the discussion on the NEP makes more sense. But with respect to the PR, sorry for impolitely merging this while the discussion was still open. I needed to publish something in a new near-primitives release. Once we figured out how exactly NEP-461 should look, we can also do another near-primitives release.

[Ekleog-NEAR]

No problem at all! I understand the urgency, and your code is certainly the best way to generically implement NEP-461 :)