From 3edca4a84f04316f9f9d2160da47a238c87bd7ed Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 22 Apr 2026 02:51:17 +0000 Subject: [PATCH] Notify initiator when inbound ACL rejects a Noise XX handshake Under Noise XX the responder only sees the initiator's static key in msg3, so the inbound-handshake ACL check cannot fire until msg3 has already been processed. By then the initiator has received msg2, passed its own outbound ACL check, and promoted its side of the peering. Silently tearing down the responder's state left the initiator as a "connected" peer with no traffic until the link-dead timeout fired tens of seconds later; the acl-allowlist integration test on the next branch timed out at the 5s convergence check on blocked nodes. The responder now sends an encrypted Disconnect on the freshly-completed Noise session before tearing down. The initiator decodes it via the existing handle_disconnect path and cleans up the zombie peer within one RTT. DisconnectReason::Other is used instead of SecurityViolation so the wire payload does not name the ACL mechanism; the behavioural signature (explicit reject right after handshake) is the only observable leak. Supporting changes: - Add send_encrypted_link_message_raw in node/mod.rs that operates on a raw NoiseSession + indices without going through self.peers, since the peer has not yet been promoted at the reject site. - Declare the acl test module in node/tests/mod.rs. It had been orphaned since PR #50 so none of its tests ran; two of them no longer compiled against the current next branch. Fixed the two and replaced test_inbound_msg1_denied_by_acl (asserts IK-era behaviour that cannot happen under XX) with test_inbound_msg3_denied_triggers_disconnect, a full end-to-end UDP test covering both the responder cleanup and the initiator's Disconnect-driven cleanup. --- CHANGELOG.md | 11 ++ src/node/handlers/handshake.rs | 28 ++++- src/node/mod.rs | 57 ++++++++++ src/node/tests/acl.rs | 190 +++++++++++++++++++++++++++------ src/node/tests/mod.rs | 1 + 5 files changed, 255 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c716876..db86ff5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -284,6 +284,17 @@ with v0.2.x peers. `AncestryRootNotMinimum`. The test now regenerates the identity until its `node_addr` is strictly larger than both the fixed parent and root. +- Responder now sends an encrypted `Disconnect` frame on the + newly-established Noise session when rejecting a peer in + `handle_msg3`, before tearing down. Under Noise XX the responder + only learns the initiator's identity from msg3, so by the time an + inbound-handshake policy check can reject, the initiator has + already received msg2 and promoted its side of the peering. Without + an explicit notification the initiator would keep the "connected" + state until link-dead timeout fired, producing the + `acl-allowlist` CI failure on the `next` branch. The notification + allows the initiator's existing `handle_disconnect` path to clean + up within one RTT. ## [0.2.0] - 2026-03-22 diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 73d9ad3..0bd262b 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -10,7 +10,7 @@ use crate::node::acl::PeerAclContext; use crate::node::wire::{Msg1Header, Msg2Header, Msg3Header, build_msg2, build_msg3}; use crate::node::{Node, NodeError}; use crate::peer::{ActivePeer, PeerConnection, PromotionResult, cross_connection_winner}; -use crate::protocol::NegotiationPayload; +use crate::protocol::{Disconnect, DisconnectReason, NegotiationPayload}; use crate::transport::{Link, LinkDirection, LinkId, ReceivedPacket}; use std::time::Duration; use tracing::{debug, info, warn}; @@ -817,6 +817,32 @@ impl Node { ) .is_err() { + // Notify the initiator via encrypted Disconnect so they clean + // up without waiting for link-dead timeout. The Noise session + // is fully established at this point (msg3 just succeeded), + // and the initiator has a matching session from processing + // msg2. Reason `Other` is used instead of `SecurityViolation` + // to avoid naming the ACL mechanism on the wire. + let reject_info = match self.connections.get_mut(&link_id) { + Some(conn) => match (conn.their_index(), conn.take_session()) { + (Some(idx), Some(session)) => Some((idx, session)), + _ => None, + }, + None => None, + }; + if let Some((their_idx, mut session)) = reject_info { + let payload = Disconnect::new(DisconnectReason::Other).encode(); + let _ = self + .send_encrypted_link_message_raw( + peer_node_addr, + packet.transport_id, + &packet.remote_addr, + &mut session, + their_idx, + &payload, + ) + .await; + } self.connections.remove(&link_id); self.remove_link(&link_id); return; diff --git a/src/node/mod.rs b/src/node/mod.rs index 63be574..be6db0f 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1901,6 +1901,63 @@ impl Node { Ok(()) } + + /// Encrypt and send a link-layer message using a raw Noise session. + /// + /// Unlike `send_encrypted_link_message`, this does not look the peer up + /// in `self.peers`. It takes the session and wire parameters directly, + /// so it can be used during handshake teardown (before promotion) where + /// the peer state lives in `self.connections` rather than `self.peers`. + /// + /// The inner-header timestamp is set to 0 — the session has just been + /// established and no session-elapsed reference is available yet; this + /// is acceptable because the frame is a one-shot sent before teardown. + /// No MMP stats or K-bit flag are recorded. + pub(super) async fn send_encrypted_link_message_raw( + &self, + node_addr: NodeAddr, + transport_id: crate::transport::TransportId, + remote_addr: &crate::transport::TransportAddr, + session: &mut crate::noise::NoiseSession, + their_index: crate::utils::index::SessionIndex, + plaintext: &[u8], + ) -> Result<(), NodeError> { + let inner_plaintext = prepend_inner_header(0, plaintext); + + let counter = session.current_send_counter(); + let payload_len = inner_plaintext.len() as u16; + let header = build_established_header(their_index, counter, 0u8, payload_len); + + let ciphertext = session + .encrypt_with_aad(&inner_plaintext, &header) + .map_err(|e| NodeError::SendFailed { + node_addr, + reason: format!("encryption failed: {}", e), + })?; + + let wire_packet = build_encrypted(&header, &ciphertext); + + let transport = self + .transports + .get(&transport_id) + .ok_or(NodeError::TransportNotFound(transport_id))?; + + transport + .send(remote_addr, &wire_packet) + .await + .map(|_| ()) + .map_err(|e| match e { + TransportError::MtuExceeded { packet_size, mtu } => NodeError::MtuExceeded { + node_addr, + packet_size, + mtu, + }, + other => NodeError::SendFailed { + node_addr, + reason: format!("transport send: {}", other), + }, + }) + } } impl fmt::Debug for Node { diff --git a/src/node/tests/acl.rs b/src/node/tests/acl.rs index f19ca2c..99e89b1 100644 --- a/src/node/tests/acl.rs +++ b/src/node/tests/acl.rs @@ -1,7 +1,7 @@ use super::*; use crate::ReceivedPacket; use crate::node::acl::PeerAclReloader; -use crate::node::wire::{build_msg1, build_msg2}; +use crate::node::wire::build_msg2; use crate::utils::index::SessionIndex; use std::path::PathBuf; use std::time::Duration; @@ -35,7 +35,7 @@ async fn test_outbound_connect_denied_by_denylist() { .initiate_connection( TransportId::new(1), TransportAddr::from_string("127.0.0.1:9000"), - PeerIdentity::from_pubkey_full(denied.pubkey_full()), + Some(PeerIdentity::from_pubkey_full(denied.pubkey_full())), ) .await; @@ -45,34 +45,6 @@ async fn test_outbound_connect_denied_by_denylist() { assert_eq!(node.peer_count(), 0); } -#[tokio::test] -async fn test_inbound_msg1_denied_by_acl() { - let (dir, mut node_b) = make_acl_node(); - let node_a = make_node(); - - std::fs::write(deny_path(&dir), format!("{}\n", node_a.npub())).unwrap(); - node_b.reload_peer_acl(); - - let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity.pubkey_full()); - let mut conn_a = PeerConnection::outbound(LinkId::new(1), peer_b_identity, 1000); - let noise_msg1 = conn_a - .start_handshake(node_a.identity.keypair(), node_a.startup_epoch, 1000) - .unwrap(); - let wire_msg1 = build_msg1(SessionIndex::new(7), &noise_msg1); - let packet = ReceivedPacket::with_timestamp( - TransportId::new(1), - TransportAddr::from_string("127.0.0.1:5000"), - wire_msg1, - 1000, - ); - - node_b.handle_msg1(packet).await; - - assert_eq!(node_b.peer_count(), 0); - assert_eq!(node_b.connection_count(), 0); - assert_eq!(node_b.link_count(), 0); -} - #[tokio::test] async fn test_outbound_msg2_denied_after_acl_reload() { let (dir, mut node_a) = make_acl_node(); @@ -114,6 +86,7 @@ async fn test_outbound_msg2_denied_after_acl_reload() { node_b.identity.keypair(), responder_epoch, &noise_msg1, + None, 1000, ) .unwrap(); @@ -132,6 +105,161 @@ async fn test_outbound_msg2_denied_after_acl_reload() { assert!(node_a.pending_outbound.is_empty()); } +/// Inbound rejection at msg3 must also cut down the initiator. +/// +/// Under Noise XX the responder only sees the initiator's identity after +/// processing msg3, so by the time the inbound ACL fires, the initiator +/// has already completed its side of the handshake and promoted the peer +/// locally. Without an explicit rejection signal the initiator would sit +/// as a "zombie peer" until link-dead timeout — several seconds too slow +/// for the `acl-allowlist` integration test's 5s convergence check. +/// +/// Exercises the full round trip: responder sends an encrypted +/// `Disconnect(Other)` on the newly-established Noise session, and the +/// initiator's existing `handle_disconnect` path tears the peer down. +#[tokio::test] +async fn test_inbound_msg3_denied_triggers_disconnect() { + use crate::config::UdpConfig; + use crate::node::acl::PeerAclReloader; + use crate::node::wire::build_msg1; + use crate::transport::udp::UdpTransport; + use crate::transport::{TransportHandle, packet_channel}; + use tokio::time::{Duration, timeout}; + + // === Setup: node A (initiator) and node B (responder) over UDP === + + let mut node_a = make_node(); + + let dir_b = tempfile::tempdir().unwrap(); + let mut node_b = make_node(); + node_b.peer_acl = PeerAclReloader::with_paths( + dir_b.path().join("peers.allow"), + dir_b.path().join("peers.deny"), + ); + std::fs::write( + dir_b.path().join("peers.deny"), + format!("{}\n", node_a.npub()), + ) + .unwrap(); + assert!(node_b.reload_peer_acl()); + + let transport_id_a = TransportId::new(1); + let transport_id_b = TransportId::new(1); + + let udp_config = UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + mtu: Some(1280), + ..Default::default() + }; + + let (packet_tx_a, mut packet_rx_a) = packet_channel(64); + let (packet_tx_b, mut packet_rx_b) = packet_channel(64); + + let mut transport_a = UdpTransport::new(transport_id_a, None, udp_config.clone(), packet_tx_a); + let mut transport_b = UdpTransport::new(transport_id_b, None, udp_config, packet_tx_b); + transport_a.start_async().await.unwrap(); + transport_b.start_async().await.unwrap(); + + let addr_b = TransportAddr::from_string(&transport_b.local_addr().unwrap().to_string()); + + node_a + .transports + .insert(transport_id_a, TransportHandle::Udp(transport_a)); + node_b + .transports + .insert(transport_id_b, TransportHandle::Udp(transport_b)); + + // === A initiates the handshake === + + let peer_b_identity = PeerIdentity::from_pubkey_full(node_b.identity.pubkey_full()); + let link_id_a = node_a.allocate_link_id(); + let mut conn_a = PeerConnection::outbound(link_id_a, peer_b_identity, 1000); + let our_index_a = node_a.index_allocator.allocate().unwrap(); + let noise_msg1 = conn_a + .start_handshake(node_a.identity.keypair(), node_a.startup_epoch, 1000) + .unwrap(); + conn_a.set_our_index(our_index_a); + conn_a.set_transport_id(transport_id_a); + conn_a.set_source_addr(addr_b.clone()); + + let wire_msg1 = build_msg1(our_index_a, &noise_msg1); + let link_a = Link::connectionless( + link_id_a, + transport_id_a, + addr_b.clone(), + LinkDirection::Outbound, + Duration::from_millis(100), + ); + node_a.links.insert(link_id_a, link_a); + node_a + .addr_to_link + .insert((transport_id_a, addr_b.clone()), link_id_a); + node_a.connections.insert(link_id_a, conn_a); + node_a + .pending_outbound + .insert((transport_id_a, our_index_a.as_u32()), link_id_a); + + let transport = node_a.transports.get(&transport_id_a).unwrap(); + transport + .send(&addr_b, &wire_msg1) + .await + .expect("Failed to send msg1"); + + // === B: msg1 → msg2 (no ACL check yet, identity unknown) === + + let packet_b_msg1 = timeout(Duration::from_secs(1), packet_rx_b.recv()) + .await + .expect("Timeout waiting for msg1") + .expect("Channel closed"); + node_b.handle_msg1(packet_b_msg1).await; + assert_eq!(node_b.peer_count(), 0, "B should not promote at msg1 (XX)"); + assert_eq!(node_b.connection_count(), 1, "B should hold pending conn"); + + // === A: msg2 → promotes, sends msg3 === + + let packet_a_msg2 = timeout(Duration::from_secs(1), packet_rx_a.recv()) + .await + .expect("Timeout waiting for msg2") + .expect("Channel closed"); + node_a.handle_msg2(packet_a_msg2).await; + assert_eq!(node_a.peer_count(), 1, "A promoted after msg2 (XX zombie)"); + + // === B: msg3 → ACL reject, sends encrypted Disconnect, tears down === + + let packet_b_msg3 = timeout(Duration::from_secs(1), packet_rx_b.recv()) + .await + .expect("Timeout waiting for msg3") + .expect("Channel closed"); + node_b.handle_msg3(packet_b_msg3).await; + + assert_eq!(node_b.peer_count(), 0, "B must not promote a denied peer"); + assert_eq!(node_b.connection_count(), 0, "B pending conn cleaned up"); + assert_eq!(node_b.link_count(), 0, "B link cleaned up"); + + // === A: encrypted frame arrives → handle_encrypted_frame → + // handle_disconnect → remove_active_peer === + + let disconnect_packet = timeout(Duration::from_secs(1), packet_rx_a.recv()) + .await + .expect("Timeout waiting for Disconnect from B") + .expect("Channel closed"); + node_a.handle_encrypted_frame(disconnect_packet).await; + + assert_eq!( + node_a.peer_count(), + 0, + "A must drop the zombie peer after receiving Disconnect" + ); + + // Clean up transports + for (_, t) in node_a.transports.iter_mut() { + t.stop().await.ok(); + } + for (_, t) in node_b.transports.iter_mut() { + t.stop().await.ok(); + } +} + #[tokio::test] async fn test_outbound_connect_not_denied_by_allowlist_miss() { let (dir, mut node) = make_acl_node(); @@ -144,7 +272,7 @@ async fn test_outbound_connect_not_denied_by_allowlist_miss() { .initiate_connection( TransportId::new(1), TransportAddr::from_string("127.0.0.1:9000"), - PeerIdentity::from_pubkey_full(denied.pubkey_full()), + Some(PeerIdentity::from_pubkey_full(denied.pubkey_full())), ) .await; diff --git a/src/node/tests/mod.rs b/src/node/tests/mod.rs index c18eff8..972be56 100644 --- a/src/node/tests/mod.rs +++ b/src/node/tests/mod.rs @@ -4,6 +4,7 @@ use crate::transport::{LinkDirection, TransportAddr, packet_channel}; use crate::utils::index::SessionIndex; use std::time::Duration; +mod acl; #[cfg(target_os = "linux")] mod ble; mod bloom;