Use hash of node's public key as unique node identifier #2241
Conversation
|
node_id_hash_pubkey@19936 aka 20210305.3 vs main ewma over 20 builds from 19660 to 19929 |
src/crypto/public_key.h
Outdated
| @@ -14,6 +14,12 @@ | |||
|
|
|||
| namespace crypto | |||
| { | |||
| static inline std::string get_public_key_pem_hash_hex( | |||
There was a problem hiding this comment.
Can you have a look at #1378 (comment)? It seems like this is taking the hash over the PEM and not DER.
There was a problem hiding this comment.
This is now done: we use the SHA-256 hash of the DER-encoded public key (hex-encoded) as the node ID (this seems to be standard, e.g. https://stackoverflow.com/questions/40404963/how-do-i-get-public-key-hash-for-ssl-pinning). We also use the same value in the SGX quote's report data.
| { | ||
| if (j.is_string()) | ||
| { | ||
| node_id = j.get<std::string>(); |
There was a problem hiding this comment.
Should we check that the JSON string looks like a digest here - that its the correct length at least, and contains only expected characters?
There was a problem hiding this comment.
We now check for both length and hex encoding.
Edit: The BFT scheme still uses monotonic IDs so this cannot be enforced just yet. I've added a comment there to link to #1852.
Co-authored-by: Amaury Chamayou <[email protected]>
Part of #1378
Very much draft for now as there are still some issues with BFT and two of the unit tests that will need further refactoring, but I want to measure the performance impact of this change first.Now read for reviewNode IDs used to be a monotonic counter, which could lead to replay attacks, and weren't convenient for operators who wouldn't know the ID of their node until this one had actually joined the network. This PR aims to solve these issues by using a fixed identifier (hex-encoded string of hash of node's identity public key) as the node's ID.
Notable changes:
std::optional<NodeId>(instead ofNoNode) in the consensus to differentiate whether we know the current leader node. This has some minor effects on the endpoints that return the current primary ID.10characters of the node ID in the node's log, e.g.:NodeIdis no longer included in the message headers (e.g. Raft's append entries). This means we don't pay the cost of integrity-protect it, which is fine because theNodeIdidentifies the channel to use for decryption./workspace/test_0as we have to create those in advance). The infra keeps track of the "real" new node ID too though.