Authenticate Libp2p connections #3534
Conversation
Co-authored-by: Jarred Parr <[email protected]>
| let mut message = vec![0u8; len]; | ||
| stream | ||
| .read_exact(&mut message) | ||
| .await | ||
| .with_context(|| "Failed to read message")?; |
There was a problem hiding this comment.
Do we fail gracefully here (return Error) if the len encoded in the messages is incorrect. I.e. if messages says it's 8 bytes but is actually only 4 bytes will we just get `Error("failed to readd message")? same question if the len is 4 bytes but the message is 8, what happens?
There was a problem hiding this comment.
If the message is longer and we only ask for the first 4 bytes, deserialization of the actual underlying message (in this case the auth_message) will fail
If the message is shorter, we will time out after the 5 seconds and fail negotiation
There was a problem hiding this comment.
Great just wanted to be sure, that's what I expect to happen
| ) -> std::task::Poll<libp2p::core::transport::TransportEvent<Self::ListenerUpgrade, Self::Error>> | ||
| { | ||
| match self.as_mut().project().inner.poll(cx) { | ||
| Poll::Ready(event) => Poll::Ready(match event { |
There was a problem hiding this comment.
Why is it not sufficient here to to just wrap the inner poll (and maybe do the event translation) and handle all the auth stuff in the dial and dial_as_listener functions?
There was a problem hiding this comment.
The majority of the time poll is the one dealing with the actual listening logic and handling new incoming connections. dial_as_listener, while I never hit it in testing, is likely used for some sort of NATing
There was a problem hiding this comment.
Oh that's really weird/unexpected
| let auth_upgrade = Box::pin(async move { | ||
| // Perform the inner upgrade | ||
| let mut stream = upgrade.await?; | ||
|
|
||
| // Time out the authentication block | ||
| async_timeout(AUTH_HANDSHAKE_TIMEOUT, async { | ||
| // Open a substream for the handshake | ||
| let mut substream = | ||
| poll_fn(|cx| stream.as_connection().poll_inbound_unpin(cx)).await?; | ||
|
|
||
| // (inbound) Verify the remote peer's authentication | ||
| verify_peer_authentication( | ||
| &mut substream, | ||
| stake_table, | ||
| stream.as_peer_id(), | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| warn!("Failed to verify remote peer: {:?}", e); | ||
| std::io::Error::new(std::io::ErrorKind::Other, e) | ||
| })?; | ||
|
|
||
| // (outbound) Authenticate with the remote peer | ||
| authenticate_with_remote_peer(&mut substream, auth_message) | ||
| .await | ||
| .map_err(|e| { | ||
| warn!("Failed to authenticate with remote peer: {:?}", e); | ||
| std::io::Error::new(std::io::ErrorKind::Other, e) | ||
| })?; | ||
|
|
||
| Ok::<(), T::Error>(()) | ||
| }) | ||
| .await | ||
| .map_err(|e| { | ||
| warn!("Timed out performing authentication handshake: {:?}", e); | ||
| std::io::Error::new(std::io::ErrorKind::TimedOut, e) | ||
| })??; |
There was a problem hiding this comment.
This is the same as the code in dial_as_listener right? can we just make this a function or remove it here as I indicated
There was a problem hiding this comment.
Yeah it is similar, let me see if I can make it less duplicated
| authenticate_with_remote_peer(&mut substream, auth_message) | ||
| .await | ||
| .map_err(|e| { | ||
| warn!("Failed to authenticate with remote peer: {:?}", e); | ||
| std::io::Error::new(std::io::ErrorKind::Other, e) | ||
| })?; | ||
|
|
||
| // (inbound) Verify the remote peer's authentication | ||
| verify_peer_authentication(&mut substream, stake_table, stream.as_peer_id()) | ||
| .await | ||
| .map_err(|e| { | ||
| warn!("Failed to verify remote peer: {:?}", e); | ||
| std::io::Error::new(std::io::ErrorKind::Other, e) | ||
| })?; |
There was a problem hiding this comment.
I'm not sure how the substream here works but we effectively just always do things in this order (write our auth message, then wait for the peers and verify it) or does it some how mess up? Just thinking that this logic is the same everywhere and would be nice if it were just one function called like `auth_with_peer(stream
There was a problem hiding this comment.
This one is actually the only one in this order, the others perform the reverse. We want to synchronously wait and make the handshake order explicit because if the peer is not actively receiving our message it will fail (e.g. if they are both sending data)
There was a problem hiding this comment.
Ok that's what I was afraid of. I knew this one was the opposite order of the listening side so I wanted to be sure it had to be that order. My thought was that sending could always happen first since both nodes need to get the auth message. I was thinking they could both send at the same time, then both receive after sending but I guess one send might fail in that case
| pub stake_table: Arc<Option<HashSet<S>>>, | ||
|
|
||
| /// A pre-signed message that we send to the remote peer for authentication | ||
| pub auth_message: Arc<Option<Vec<u8>>>, | ||
|
|
There was a problem hiding this comment.
Is it Optional because of upgrading, or some other reason?
There was a problem hiding this comment.
Yeah it is for upgrading, but I am open to making it non-optional
There was a problem hiding this comment.
Long run non optional what we want, but for now it makes sense
|
I can approve after you do the de-duplication |
Done! Hopefully this made things look a little nicer |
Closes #3490
This PR:
Implements authentication against the stake table for Libp2p connections. It does so by introducing a mutual, bidirectional authentication handshake when a new connection is established.
Here's how it works:
PeerId.PeerIdis then checked against the remote peer'sPeerId, which Libp2p has already negotiated.The authentication message is pre-signed to minimize scope of the private key in networking segments of the code. The mitigation for replay attacks is the above
PeerIdcomparison.It also:
This PR does not:
Key places to review:
stake_table_transport.rs- almost all of the logic is hereHow to test this PR:
Since the examples use
from_config, you can test in the examples. Try signing the wrong thing and see what happens.