mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-11 17:17:54 +00:00
Admit rekey msg1 from established peers regardless of accept_connections
The accept_connections gate at the top of handle_msg1 was applied unconditionally, so rekey msg1 from a peer with whom an established link already existed was dropped on the same path as fresh handshakes from strangers. Combined with the dual-init tie-breaker, this deadlocked at ~25 minutes when both sides' rekey timers fired near-simultaneously: the smaller-NodeAddr side wins as initiator and expects the larger side to consume its rekey msg1, but if the larger side has accept_connections=false the gate dropped it. Both sides retried at 1 Hz indefinitely; the affected peer fell out of MMP-active rotation. Extract the gate decision into Node::should_admit_msg1, which admits unconditionally when addr_to_link already has an entry for the (transport_id, remote_addr) pair (rekey/restart on an established session) and otherwise consults the transport's accept_connections(). Fresh msg1 from strangers is still rejected before any Noise crypto. Three unit tests pin the truth table: no transport (admit), accept_off no-link (reject, behavior unchanged), accept_off with-link (admit, the carve-out). The fix generalizes for free to BLE, which has the same Node-level gate. TCP and Tor were never subject to this deadlock because their accept condition is runtime state (bind_addr.is_some() / onion_address.is_some()), not a config flag.
This commit is contained in:
@@ -10,6 +10,30 @@ use std::time::Duration;
|
||||
use tracing::{debug, info, warn};
|
||||
|
||||
impl Node {
|
||||
/// Returns true if an inbound msg1 should be admitted past the
|
||||
/// `accept_connections` gate.
|
||||
///
|
||||
/// Rekey/restart msg1 on an existing link is always admitted (the gate
|
||||
/// is meant to filter fresh handshakes from strangers, not maintenance
|
||||
/// traffic on established sessions). Otherwise the transport's
|
||||
/// `accept_connections` config decides; absence of a registered
|
||||
/// transport admits (no gate to apply).
|
||||
pub(in crate::node) fn should_admit_msg1(
|
||||
&self,
|
||||
transport_id: crate::transport::TransportId,
|
||||
remote_addr: &crate::transport::TransportAddr,
|
||||
) -> bool {
|
||||
if self
|
||||
.addr_to_link
|
||||
.contains_key(&(transport_id, remote_addr.clone()))
|
||||
{
|
||||
return true;
|
||||
}
|
||||
self.transports
|
||||
.get(&transport_id)
|
||||
.is_none_or(|t| t.accept_connections())
|
||||
}
|
||||
|
||||
/// Handle handshake message 1 (phase 0x1).
|
||||
///
|
||||
/// This creates a new inbound connection. Rate limiting is applied
|
||||
@@ -25,10 +49,11 @@ impl Node {
|
||||
return;
|
||||
}
|
||||
|
||||
// Check if this transport accepts inbound connections
|
||||
if let Some(transport) = self.transports.get(&packet.transport_id)
|
||||
&& !transport.accept_connections()
|
||||
{
|
||||
// accept_connections gate. Rekey/restart msg1 on an existing link
|
||||
// is always admitted; the gate only filters truly-fresh connections
|
||||
// from strangers. Without this carve-out, the dual-init tie-breaker
|
||||
// deadlocks when the larger-NodeAddr side has accept_connections=false.
|
||||
if !self.should_admit_msg1(packet.transport_id, &packet.remote_addr) {
|
||||
self.msg1_rate_limiter.complete_handshake();
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -951,3 +951,69 @@ async fn test_duplicate_msg2_dropped() {
|
||||
assert_eq!(node.connection_count(), 0);
|
||||
assert_eq!(node.peer_count(), 0);
|
||||
}
|
||||
|
||||
/// `should_admit_msg1` admits when no transport is registered for the id.
|
||||
/// (No gate to apply — the caller's other checks decide the outcome.)
|
||||
#[test]
|
||||
fn test_should_admit_msg1_no_transport() {
|
||||
let node = make_node();
|
||||
let addr = TransportAddr::from_string("10.0.0.2:2121");
|
||||
assert!(node.should_admit_msg1(TransportId::new(1), &addr));
|
||||
}
|
||||
|
||||
/// `should_admit_msg1` rejects a fresh msg1 (no addr_to_link entry) when
|
||||
/// the transport has accept_connections=false. Behavior unchanged from
|
||||
/// before the carve-out.
|
||||
#[tokio::test]
|
||||
async fn test_should_admit_msg1_rejects_fresh_when_accept_off() {
|
||||
use crate::config::TcpConfig;
|
||||
use crate::transport::tcp::TcpTransport;
|
||||
|
||||
let mut node = make_node();
|
||||
let transport_id = TransportId::new(1);
|
||||
|
||||
// bind_addr=None → accept_connections() == false
|
||||
let cfg = TcpConfig {
|
||||
bind_addr: None,
|
||||
..Default::default()
|
||||
};
|
||||
let (tx, _rx) = packet_channel(64);
|
||||
let tcp = TcpTransport::new(transport_id, None, cfg, tx);
|
||||
node.transports
|
||||
.insert(transport_id, TransportHandle::Tcp(tcp));
|
||||
|
||||
let addr = TransportAddr::from_string("10.0.0.2:2121");
|
||||
assert!(!node.should_admit_msg1(transport_id, &addr));
|
||||
}
|
||||
|
||||
/// ISSUE-2026-0004 regression test: `should_admit_msg1` admits rekey/restart
|
||||
/// msg1 from a peer with an existing link even when the transport has
|
||||
/// accept_connections=false. Without this, the dual-init tie-breaker
|
||||
/// deadlocks (the larger-NodeAddr side drops the winner's rekey msg1).
|
||||
#[tokio::test]
|
||||
async fn test_should_admit_msg1_admits_rekey_when_accept_off() {
|
||||
use crate::config::TcpConfig;
|
||||
use crate::transport::tcp::TcpTransport;
|
||||
|
||||
let mut node = make_node();
|
||||
let transport_id = TransportId::new(1);
|
||||
|
||||
let cfg = TcpConfig {
|
||||
bind_addr: None,
|
||||
..Default::default()
|
||||
};
|
||||
let (tx, _rx) = packet_channel(64);
|
||||
let tcp = TcpTransport::new(transport_id, None, cfg, tx);
|
||||
node.transports
|
||||
.insert(transport_id, TransportHandle::Tcp(tcp));
|
||||
|
||||
let addr = TransportAddr::from_string("10.0.0.2:2121");
|
||||
|
||||
// Pre-populate addr_to_link as if a session were established for this
|
||||
// peer on this transport (rekey msg1 will arrive against this entry).
|
||||
let link_id = node.allocate_link_id();
|
||||
node.addr_to_link
|
||||
.insert((transport_id, addr.clone()), link_id);
|
||||
|
||||
assert!(node.should_admit_msg1(transport_id, &addr));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user