Respond to requests from peers with invalid ENRs (#265)

* Respond to requests from peers with invalid ENRs

* Remove old test function

* Remove uncontactable peers from routing table

Author: AgeManning

README.md

@@ -80,11 +80,6 @@ A simple example of creating this service is as follows:
 
 # Addresses in ENRs 
 
-This protocol will drop messages (i.e not respond to requests) from peers that
-advertise non-contactable address in their ENR (e.g `127.0.0.1` when connecting
-to non-local nodes). This section
-explains the rationale behind this design decision.
-
 An ENR is a signed record which is primarily used in this protocol for
 identifying and connecting to peers. ENRs have **OPTIONAL** `ip` and `port`
 fields.
@@ -127,8 +122,5 @@ This is done in the following way:
 3. If a peer connects to us with an ENR that specifies an IP address that does
    not match the src socket it connects to us on (e.g `127.0.0.1`, or
    potentially some internal subnet IP that is unreachable from our current
-   network) we consider this peer malicious/faulty
-   and drop all packets. This way we can efficiently drop peers that may try to
-   get us to send messages to arbitrary remote IPs, and we can be sure that all
-   ENRs in our routing table are contactable (at least by our local node at
-   some point in time).
+   network) we consider this ENR invalid and will not add it to our routing
+   table. However we will still respond to discovery requests.

src/handler/mod.rs

@@ -74,12 +74,6 @@ use session::Session;
 // seconds).
 const BANNED_NODES_CHECK: u64 = 300; // Check every 5 minutes.
 
-// The one-time session timeout.
-const ONE_TIME_SESSION_TIMEOUT: u64 = 30;
-
-// The maximum number of established one-time sessions to maintain.
-const ONE_TIME_SESSION_CACHE_CAPACITY: usize = 100;
-
 /// Messages sent from the application layer to `Handler`.
 #[derive(Debug, Clone, PartialEq)]
 #[allow(clippy::large_enum_variant)]
@@ -216,8 +210,6 @@ pub struct Handler {
     active_challenges: HashMapDelay<NodeAddress, Challenge>,
     /// Established sessions with peers.
     sessions: LruTimeCache<NodeAddress, Session>,
-    /// Established sessions with peers for a specific request, stored just one per node.
-    one_time_sessions: LruTimeCache<NodeAddress, (RequestId, Session)>,
     /// The channel to receive messages from the application layer.
     service_recv: mpsc::UnboundedReceiver<HandlerIn>,
     /// The channel to send messages to the application layer.
@@ -308,10 +300,6 @@ impl Handler {
                         config.session_timeout,
                         Some(config.session_cache_capacity),
                     ),
-                    one_time_sessions: LruTimeCache::new(
-                        Duration::from_secs(ONE_TIME_SESSION_TIMEOUT),
-                        Some(ONE_TIME_SESSION_CACHE_CAPACITY),
-                    ),
                     active_challenges: HashMapDelay::new(config.request_timeout),
                     service_recv,
                     service_send,
@@ -549,9 +537,6 @@ impl Handler {
         // Check for an established session
         let packet = if let Some(session) = self.sessions.get_mut(&node_address) {
             session.encrypt_message::<P>(self.node_id, &response.encode())
-        } else if let Some(mut session) = self.remove_one_time_session(&node_address, &response.id)
-        {
-            session.encrypt_message::<P>(self.node_id, &response.encode())
         } else {
             // Either the session is being established or has expired. We simply drop the
             // response in this case.
@@ -847,7 +832,7 @@ impl Handler {
                 ephem_pubkey,
                 enr_record,
             ) {
-                Ok((mut session, enr)) => {
+                Ok((session, enr)) => {
                     // Remove the expected response for the challenge.
                     self.remove_expected_response(node_address.socket_addr);
                     // Receiving an AuthResponse must give us an up-to-date view of the node ENR.
@@ -868,28 +853,17 @@ impl Handler {
                         {
                             warn!(error = %e, "Failed to inform of established session")
                         }
-                        // When (re-)establishing a session from an outgoing challenge, we do not need
-                        // to filter out this request from active requests, so we do not pass
-                        // the message nonce on to `new_session`.
-                        self.new_session::<P>(node_address.clone(), session, None)
-                            .await;
-                        self.handle_message(
-                            node_address.clone(),
-                            message_nonce,
-                            message,
-                            authenticated_data,
-                        )
-                        .await;
                     } else {
-                        // IP's or NodeAddress don't match. Drop the session.
+                        // IP's or NodeAddress don't match.
+                        //
+                        // We still handle the request, but we do not add the ENR to our routing
+                        // table or consider the ENR valid.
                         warn!(
                             udp4_socket = ?enr.udp4_socket(),
                             udp6_socket = ?enr.udp6_socket(),
                             expected = %node_address,
                             "Session has invalid ENR",
                         );
-                        self.fail_session(&node_address, RequestError::InvalidRemoteEnr, true)
-                            .await;
 
                         // The ENR doesn't verify. Notify application.
                         self.notify_unverifiable_enr(
@@ -898,39 +872,20 @@ impl Handler {
                             node_address.node_id,
                         )
                         .await;
-
-                        // Respond to PING request even if the ENR or NodeAddress don't match
-                        // so that the source node can notice its external IP address has been changed.
-                        let maybe_ping_request = match session.decrypt_message(
-                            message_nonce,
-                            message,
-                            authenticated_data,
-                        ) {
-                            Ok(m) => match Message::decode(&m) {
-                                Ok(Message::Request(request)) if request.msg_type() == 1 => {
-                                    Some(request)
-                                }
-                                _ => None,
-                            },
-                            _ => None,
-                        };
-                        if let Some(request) = maybe_ping_request {
-                            debug!(
-                                %node_address,
-                                "Responding to a PING request using a one-time session.",
-                            );
-                            self.one_time_sessions
-                                .insert(node_address.clone(), (request.id.clone(), session));
-                            if let Err(e) = self
-                                .service_send
-                                .send(HandlerOut::Request(node_address.clone(), Box::new(request)))
-                                .await
-                            {
-                                warn!(error = %e, "Failed to report request to application");
-                                self.one_time_sessions.remove(&node_address);
-                            }
-                        }
                     }
+
+                    // When (re-)establishing a session from an outgoing challenge, we do not need
+                    // to filter out this request from active requests, so we do not pass
+                    // the message nonce on to `new_session`.
+                    self.new_session::<P>(node_address.clone(), session, None)
+                        .await;
+                    self.handle_message(
+                        node_address.clone(),
+                        message_nonce,
+                        message,
+                        authenticated_data,
+                    )
+                    .await;
                 }
                 Err(Error::InvalidChallengeSignature(challenge)) => {
                     warn!(
@@ -1294,24 +1249,6 @@ impl Handler {
         }
     }
 
-    /// Remove one-time session by the given NodeAddress and RequestId if exists.
-    fn remove_one_time_session(
-        &mut self,
-        node_address: &NodeAddress,
-        request_id: &RequestId,
-    ) -> Option<Session> {
-        match self.one_time_sessions.peek(node_address) {
-            Some((id, _)) if id == request_id => {
-                let (_, session) = self
-                    .one_time_sessions
-                    .remove(node_address)
-                    .expect("one-time session must exist");
-                Some(session)
-            }
-            _ => None,
-        }
-    }
-
     /// A request has failed.
     async fn fail_request(
         &mut self,

src/handler/session.rs

@@ -265,11 +265,3 @@ impl Session {
         Ok((packet, session))
     }
 }
-
-#[cfg(test)]
-pub(crate) fn build_dummy_session() -> Session {
-    Session::new(Keys {
-        encryption_key: [0; 16],
-        decryption_key: [0; 16],
-    })
-}

src/handler/tests.rs

@@ -15,10 +15,7 @@ use std::{
     ops::Add,
 };
 
-use crate::{
-    handler::{session::build_dummy_session, HandlerOut::RequestFailed},
-    RequestError::SelfRequest,
-};
+use crate::{handler::HandlerOut::RequestFailed, RequestError::SelfRequest};
 use active_requests::ActiveRequests;
 use std::time::Duration;
 use tokio::time::sleep;
@@ -78,10 +75,6 @@ async fn build_handler<P: ProtocolIdentity>(
         pending_requests: HashMap::new(),
         filter_expected_responses,
         sessions: LruTimeCache::new(config.session_timeout, Some(config.session_cache_capacity)),
-        one_time_sessions: LruTimeCache::new(
-            Duration::from_secs(ONE_TIME_SESSION_TIMEOUT),
-            Some(ONE_TIME_SESSION_CACHE_CAPACITY),
-        ),
         active_challenges: HashMapDelay::new(config.request_timeout),
         service_recv,
         service_send,
@@ -580,50 +573,6 @@ async fn test_self_request_ipv6() {
     );
 }
 
-#[tokio::test]
-async fn remove_one_time_session() {
-    let config = ConfigBuilder::new(ListenConfig::default()).build();
-    let key = CombinedKey::generate_secp256k1();
-    let enr = Enr::builder()
-        .ip4(Ipv4Addr::LOCALHOST)
-        .udp4(9000)
-        .build(&key)
-        .unwrap();
-    let (_, _, _, mut handler) = build_handler::<DefaultProtocolId>(enr, key, config).await;
-
-    let enr = {
-        let key = CombinedKey::generate_secp256k1();
-        Enr::builder()
-            .ip4(Ipv4Addr::LOCALHOST)
-            .udp4(9000)
-            .build(&key)
-            .unwrap()
-    };
-    let node_address = NodeAddress::new("127.0.0.1:9000".parse().unwrap(), enr.node_id());
-    let request_id = RequestId::random();
-    let session = build_dummy_session();
-    handler
-        .one_time_sessions
-        .insert(node_address.clone(), (request_id.clone(), session));
-
-    let other_request_id = RequestId::random();
-    assert!(handler
-        .remove_one_time_session(&node_address, &other_request_id)
-        .is_none());
-    assert_eq!(1, handler.one_time_sessions.len());
-
-    let other_node_address = NodeAddress::new("127.0.0.1:9001".parse().unwrap(), enr.node_id());
-    assert!(handler
-        .remove_one_time_session(&other_node_address, &request_id)
-        .is_none());
-    assert_eq!(1, handler.one_time_sessions.len());
-
-    assert!(handler
-        .remove_one_time_session(&node_address, &request_id)
-        .is_some());
-    assert_eq!(0, handler.one_time_sessions.len());
-}
-
 // Tests replaying active requests.
 //
 // In this test, Receiver's session expires and Receiver returns WHOAREYOU.

src/service.rs

@@ -410,6 +410,14 @@ impl Service {
                             self.rpc_failure(request_id, error);
                         }
                         HandlerOut::UnverifiableEnr{enr, socket, node_id} => {
+                            // We have received an ENR that has incorrect socket address fields
+                            // (given the source of the pakcet we received.
+                            // If this node exists in our routing table, remove it, as it may have shifted
+                            // ip addresses and is now no longer contactable.
+                            let key = kbucket::Key::from(node_id);
+                            if self.kbuckets.write().remove(&key) {
+                                debug!(?node_id, "Uncontactable node removed from routing table");
+                            }
                             self.send_event(Event::UnverifiableEnr{enr, socket, node_id});
                         }
                     }