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;