Simplify heartbeat signature scheme #1537
Conversation
|
Currently waiting on feedback from @andreogle if he's happy with this scheme (I suspect he is, but will wait for confirmation). |
|
ChainAPI supports the new heartbeat signature authentication. I'm happy with this. Thanks for the help getting the address recovery to work @aquarat 💙 |
| const heartbeatPayload = { | ||
| const timestamp = Date.now(); | ||
|
|
||
| const heartbeatPayload = JSON.stringify({ |
There was a problem hiding this comment.
I think it's worth adding a comment explaining why this is stringified. i.e. to avoid any minor differences that might occur when encoding/decoding in other environments. The payload/message needs to be exactly the same everywhere
There was a problem hiding this comment.
I've added the following comment, LMK if you disagree:
/*
The heartbeat payload is serialised as JSON and then serialised again in JSON as the value of the payload field.
The reason this is done is to avoid any inconsistencies between different JSON serialisation implementations.
*/
| const timestamp = Date.now(); | ||
|
|
||
| const heartbeatPayload = JSON.stringify({ | ||
| timestamp, |
There was a problem hiding this comment.
Nit: I'd suggest using the UNIX timestamp (in seconds) and not the JS timestamp which is in ms... But this is a breaking change so we might just not do it.
There was a problem hiding this comment.
Good point. Unix time would be much better imo. And just to confirm, is this in UTC or is it the server timezone?
Why is it a breaking change?
There was a problem hiding this comment.
Don't you rely on the heartbeat timestamp being in ms? (That's how it worked until now). Changing that to seconds would mean the old values would need to be migrated.
EDIT: But tou will be implementing new handling for this anyway, so maybe it's not a big problem. But I am not sure if there aren't any other users of heartbeat that would be affected. (But they will be affected either way by the packing removal).
There was a problem hiding this comment.
ChainAPI hasn't been using the timestamp until now
There was a problem hiding this comment.
I've made the change by using Math.round(Date.now()) 👌
| const heartbeatPayload = { | ||
| const timestamp = Date.now(); | ||
|
|
||
| const heartbeatPayload = JSON.stringify({ |
There was a problem hiding this comment.
Hmm. Maybe it would be wise to make sure that the payload content fields are always sorted. JSON.stringify doesn't do that. Maybe something like [json-stable-stringify](https://github.com/ljharb/json-stable-stringify] would be better. It's not like JSON.stringify will stringify it randomly, the fields will be in the order they are presented in when creating the object. But just to be on the safe side?
There was a problem hiding this comment.
If we serialise it like this we will always get a consistent result.
Serialising a serialised payload gets rid of the unstable-ordering-related issue and any other potential issues as you're comparing the signature with a byte array effectively. Adding an additional library to ensure serialisation ordering consistency seems unnecessary to me and introduces a new dependency 🤷
Closes #1536
etherspacking is not well supported in the Elixir environment and even message signing has quirks associated with it that don't translate 1:1. This PR simplifies the heartbeat scheme by removing the packing component in favour of moving the timestamp into the JSON payload. This makes Airnode signatures easier to support in other languages.The JSON is signed and sent as serialised JSON to prevent differences in JSON marshaling/unmarshaling across languages causing signatures to fail.