Include signed ip address in TIER2 handshake. #8902 #9100

ghost · 2023-05-23T17:55:26Z

#8902

The problem being solved

Prevent man in middle attacks on Tier2 network

The approach taken

Pass the signed ip address of the sender

Remaining steps to fully deploy this solution after this PR is merged

Fail the handshake only after all nodes on both mainnet and testnet have updated to the latest version of Tier2 handshake that includes signing the ip address. It currently only fails if the signed ip address is present and faulty but doesn't fail if the signed ip address is absent (for backward compatibility temporary migration purposes)

Also needed to run auto-linter to pass buildkite tests

cargo fmt -- --emit files 
cargo clippy --fix -- -A clippy::all -W clippy::useless_format -W clippy::clone_on_copy
cargo clippy --fix -- -A clippy::all -W clippy::useless_format -W clippy::redundant_clone
./scripts/formatting --fix

ghost · 2023-05-23T18:24:57Z

current sanity test error is due to peer signing on its local address of 0.0.0.0 but being advertised as 127.0.0.1, which is only the case for localnet. Will think of a fix for this and update code

saketh-are · 2023-05-23T18:39:55Z

chain/network/src/network_protocol/mod.rs

@@ -90,6 +90,45 @@ impl std::str::FromStr for PeerAddr {
    }
 }

+// Wrapper around std::net::IpAddr, which constructs a SignedOwnedIpAddress
+#[derive(Clone, PartialEq, Eq, Debug, Hash)]
+pub struct OwnedIpAddress {


I don't see the need for this struct wrapping a single field. I would rather redesign the functions you have placed below in the impl to be static functions accepting an instance of std::net::IpAddr as an argument.

As discussed offline, it's needed to implement the interface in order to hide the serialization of ip_addr from the user and in order to form the message (SignedOwnedIpAddress) to be sent over the network to contain both required ip address and signature

If i understand correctly, there always needs to be a wrapper class in order to introduce new member methods to existing class such as the examples below

nearcore/chain/network/src/tcp.rs

Lines 150 to 152 in 7d0b0b4

pub struct ListenerAddr(std::net::SocketAddr);

impl fmt::Debug for ListenerAddr {

nearcore/chain/network/src/tcp.rs

Line 60 in 7d0b0b4

pub(crate) struct Socket(tokio::net::TcpSocket);

nearcore/chain/network/src/tcp.rs

Line 212 in 7d0b0b4

pub(crate) struct Listener(tokio::net::TcpListener);

Sorry if we already covered this, but what's the issue with defining a constructor for SignedIpAddress which takes two args (private_key and ip address) and returns an object with the ip address and signature?

One other concern I have with the OwnedIPAddress wrapper is the "owned" language which has been borrowed from OwnedAccount. In the context of accounts it makes sense to say "owned" but not really for IP Addresses. If we really cannot get rid of this intermediate layer (please see question 1 first) consider naming it something like WrappedIpAddress for clarity.

Regarding 1. good point, I thought about it before but somehow forgot it. I guess I preferred the other approach at the time as I could refer to protobuf serialization of OwnedAccount. Updated to SignedIpAddress with constructors as suggested.

Ignoring 2. since it's redundant thanks to 1.

chain/network/src/network_protocol/mod.rs

chain/network/src/network_protocol/network.proto

chain/network/src/network_protocol/proto_conv/net.rs

chain/network/src/peer/peer_actor.rs

chain/network/src/peer/tests/communication.rs

saketh-are · 2023-05-23T19:52:52Z

Hey @soonnear, thanks for sharing this. I left some stylistic comments to start, but this looks pretty good overall. Also still need to review tests.

chain/network/src/network_protocol/tests.rs

saketh-are · 2023-05-23T20:15:30Z

chain/network/src/network_protocol/tests.rs

+        net::IpAddr::V6(ip) => ip.octets().to_vec(),
+    };
+    let signature = node_key.sign(&ip_bytes);
+    assert!(signature.verify(&ip_bytes, &node_key.public_key()));


This seems like a test of the sign function (which accepts an arbitrary sequence of bytes), which should have its own unit tests.

The sign function is independent of ip data, this test is specific to checking the sign function works with our serialized approach for ip data that we discussed.

What does this test catch? The fact that .octets().to_vec() produces a Vec<u8> is something checked by the compiler. The fact that sign will work on an arbitrary sequence of bytes is a test of the sign function. I don't think this test justifies its existence here.

this test also shows how the interface of the new SignedIpAddress object looks like from the client code perspective.
It also tests that the interface works as expected.
Removed the redundant code but maintained the test of the introduced interface SignedIpAddress which tests both that the sign and verify() works deterministically with its hidden serialization function of octets().to_vec()

chain/network/src/network_protocol/tests.rs

chain/network/src/peer_manager/tests/connection_pool.rs

saketh-are · 2023-05-23T20:25:01Z

chain/network/src/peer_manager/tests/connection_pool.rs

+    let mut rng = make_rng(921853233);
+    let rng = &mut rng;
+    let mut clock = time::FakeClock::default();
+    let chain = Arc::new(data::Chain::make(&mut clock, rng, 10));


This test is quite long and it would benefit from some visual segmentation for readability. For example, the other tests in this file put an empty line after these first four lines to visually separate the setup of these basic variables from the rest of the test-specific code.

Added some white spaces, hopefully that helps.

Not sure which test you changed but test_backward_compatible_handshake_without_signed_ip_address still looks to have this issue.

Ah lol, the test test_backward_compatible_handshake_without_signed_ip_address is in a different file, this comment is pointing towards connection_pool.rs whereas that test is in the file communication.rs

I previously updated the test_signed_ip_address which is being referred to by this code review comment.

Updated the test as suggested, however, do note that this backward compatibility test is just temporary and will be removed once all nodes have fully migrated to signing ip address.

chain/network/src/config.rs

chain/network/src/peer/peer_actor.rs

ghost · 2023-05-26T13:08:49Z

updated to address first draft review comments, thanks! @saketh-are

chain/network/src/network_protocol/tests.rs

chain/network/src/config.rs

saketh-are · 2023-05-28T04:26:45Z

chain/network/src/peer/peer_actor.rs

@@ -286,7 +290,7 @@ impl PeerActor {
        };
        let my_node_info = PeerInfo {
            id: network_state.config.node_id(),
-            addr: network_state.config.node_addr.as_ref().map(|a| **a),
+            addr: Some(stream.local_addr), // update own local address based on required stream


What was the address stored previously here and why do we need to change it?

it fails the handshake test for localnet.
Previously stores the listening address, which can be set as 0.0.0.0 and optiona, now will store the actual connection address which will be set as 127.0.0.1 for localnet and is mandatory

Ok, I think I understand what's going on from some of your other comments. network_state.config.node_addr has the ip address on which the node listens for new connections. When that listener does accept a connection and create a new stream, it is more correct/direct to just take the address for the stream from the stream object rather than assume it will be identical to the one for the listener and indirectly get the address through the network config.

Can you remove or modify this comment? It is not clear to me what is being said (for example we are not "updating" anything here, it is the spawn logic and everything is being set for the first time).

Yes, you understood it perfectly. Took me some time diving into the code to figure out the differences between both socket addresses as well.
I removed the comment instead of updating the comment.

core/chain-configs/src/genesis_validate.rs

nearcore/src/config_validate.rs

nearcore/src/state_sync.rs

core/chain-configs/src/genesis_config.rs

saketh-are · 2023-05-28T15:18:54Z

A better title for this PR would be something like "Include signed ip address in TIER2 handshake."

Also, please add some description which explains what the PR is doing and why. You've linked #8902, the issue being addressed, but have taken a different approach than suggested there ("by implementing a 3-way handshake"). The description should include a few sentences very briefly summarizing:

The problem being solved
The approach taken
Remaining steps to fully deploy this solution after this PR is merged

ghost · 2023-05-29T06:46:23Z

note that the unrelated code were added as part of clippy linter auto updates.
I believe a new rule was added, but it did not update the code in that commit (and that merged commit did fail the buildkite) so i've to update here or it wont pass the nayduck clippy tests.

ghost · 2023-05-29T06:58:49Z

Thanks! Updated both the PR title and description as suggested.

Also updated code based on 2nd revision comments, thanks! @saketh-are

chain/network/src/testonly/stream.rs

saketh-are · 2023-05-29T14:08:50Z

Thanks @soonnear, this is looking good. I've left just a few final comments; this should be ready to merge once you've taken a look.

saketh-are

Thanks, LGTM

saketh-are · 2023-05-29T17:44:36Z

chain/network/src/network_protocol/mod.rs

@@ -90,6 +90,36 @@ impl std::str::FromStr for PeerAddr {
    }
 }

+/// Proof that a given peer_id owns an ip address, included in Handshake message


Can we revise this comment to say what's really going on here? As mentioned previously ip addresses aren't really "owned." I would suggest something like:

// Signed ip address included as part of the Handshake message. // Used to authenticate identity of connected peers. // Necessary because peer discovery happens via unauthenticated PeerInfos.

saketh-are · 2023-05-29T17:46:17Z

chain/network/src/network_protocol/mod.rs

+        return ip_bytes;
+    }
+
+    fn ip_bytes(&self) -> Vec<u8> {


Why do we need the separate helper function? Can we just inline it here?

saketh-are

Thanks, LGTM

…t::IpAddr

…ning std::net::IpAddr for Protocol Buffer

…pAddress

…erialization and Deserialization

…ocol buffer message serialization

…nd properly handles errors otherwise, tested backwards compatibility with lack of signature

… address is 127.0.0.1

…it tests

…ignedIpAddress instead

* Algorithm Design: Sign, Verify Ip Address With Interface SignedIpAddress * Implemented Protocol Buffer Serialization Deserialization For std::net::IpAddr and SignedIpAddress * Implemented serialization deserialization of Handshake message containing SignedIpAddress for Protocol Buffer * Implemented and unit tested verifying ip address is properly signed and properly handles errors otherwise * Tested backwards compatibility with lack of signature for ip address

…#9100)" This reverts commit bc9ba97.

…9191) * Revert "Include signed ip address in TIER2 handshake. #8902 (#9100)" This reverts commit bc9ba97. * reserved field number for backward compatibility of protocol buffer * Added comment on purpose for reservation

…#9100)" (near#9191) * Revert "Include signed ip address in TIER2 handshake. near#8902 (near#9100)" This reverts commit bc9ba97. * reserved field number for backward compatibility of protocol buffer * Added comment on purpose for reservation

ghost self-requested a review as a code owner May 23, 2023 17:55

ghost requested a review from mzhangmzz May 23, 2023 17:55

ghost self-assigned this May 23, 2023

ghost requested a review from saketh-are May 23, 2023 17:55

saketh-are reviewed May 23, 2023

View reviewed changes

ghost removed the request for review from mzhangmzz May 26, 2023 10:33