Skip to content

Use hash of node's public key as unique node identifier#2241

Merged
jumaffre merged 41 commits into
microsoft:mainfrom
jumaffre:node_id_hash_pubkey
Mar 5, 2021
Merged

Use hash of node's public key as unique node identifier#2241
jumaffre merged 41 commits into
microsoft:mainfrom
jumaffre:node_id_hash_pubkey

Conversation

@jumaffre
Copy link
Copy Markdown
Contributor

@jumaffre jumaffre commented Feb 25, 2021

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 review

Node 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:

  • We now use a std::optional<NodeId> (instead of NoNode) 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.
  • We only display the first 10 characters of the node ID in the node's log, e.g.:
2021-03-03T14:07:34.809510Z -0.001 0   [debug] ../src/consensus/aft/raft.h:994      | Send append entries from dc84b80910 to faeac68820: 11 to 10 (2)
  • Node-to-node channel refactoring so that the NodeId is 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 the NodeId identifies the channel to use for decryption.
  • The Python infra is mostly unchanged: nodes are still labelled incrementally (e.g. the node folder is still /workspace/test_0 as we have to create those in advance). The infra keeps track of the "real" new node ID too though.
  • BFT currently relies on a stable order of node IDs so that the primary at a given view can be computed deterministically by all nodes. So in BFT, we still use monotonic node IDs for now (see AFT: Support reconfiguration #1852 for more detail).

@ghost
Copy link
Copy Markdown

ghost commented Mar 2, 2021

node_id_hash_pubkey@19936 aka 20210305.3 vs main ewma over 20 builds from 19660 to 19929
images

@jumaffre jumaffre mentioned this pull request Mar 3, 2021
2 tasks
Comment thread doc/build_apps/demo.rst Outdated
Comment thread CMakeLists.txt Outdated
Comment thread src/consensus/aft/async_execution.h Outdated
Comment thread src/consensus/aft/raft_types.h
Comment thread src/consensus/aft/raft_types.h Outdated
Comment thread src/crypto/public_key.h Outdated

namespace crypto
{
static inline std::string get_public_key_pem_hash_hex(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you have a look at #1378 (comment)? It seems like this is taking the hash over the PEM and not DER.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread src/ds/json.h
Comment thread src/host/node_connections.h Outdated
Comment thread src/kv/test/stub_consensus.h
Comment thread src/host/node_connections.h
Comment thread src/node/node_id.h
{
if (j.is_string())
{
node_id = j.get<std::string>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we check that the JSON string looks like a digest here - that its the correct length at least, and contains only expected characters?

Copy link
Copy Markdown
Contributor Author

@jumaffre jumaffre Mar 4, 2021

Choose a reason for hiding this comment

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

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.

Comment thread src/node/node_id.h Outdated
Comment thread src/node/node_to_node.h Outdated
Comment thread src/crypto/key_pair.h Outdated
Comment thread python/utils/verify_quote.sh Outdated
Julien Maffre and others added 2 commits March 4, 2021 17:40
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
@jumaffre jumaffre merged commit a62eef9 into microsoft:main Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants