Skip to content

Commit

Permalink
test(connlib): ensure ICMP replies make it back to the client (#5104)
Browse files Browse the repository at this point in the history 
In order to exercise all codepaths of connlib, we need to send traffic
in both directions. This patch sends ICMP replies from the gateway to
the client upon receipt of an ICMP request.
  • Loading branch information
@thomaseizinger
thomaseizinger committed May 24, 2024
1 parent 32a1ab1 commit 994b25b
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 28 deletions.
71 changes: 53 additions & 18 deletions rust/connlib/tunnel/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use connlib_shared::{
use firezone_relay::{AddressFamily, AllocationPort, ClientSocket, PeerSocket};
use ip_network::Ipv4Network;
use ip_network_table::IpNetworkTable;
use ip_packet::IpPacket;
use pretty_assertions::assert_eq;
use proptest::{
arbitrary::any,
Expand Down Expand Up @@ -59,8 +58,8 @@ struct TunnelTest {
relay: SimRelay<firezone_relay::Server<StdRng>>,
portal: SimPortal,

client_received_packets: VecDeque<IpPacket<'static>>,
gateway_received_icmp_packets: VecDeque<(Instant, IpAddr, IpAddr)>,
gateway_received_icmp_requests: VecDeque<(Instant, IpAddr, IpAddr)>,
client_received_icmp_replies: VecDeque<(Instant, IpAddr, IpAddr)>,

#[allow(dead_code)]
logger: DefaultGuard,
Expand All @@ -82,7 +81,8 @@ struct ReferenceState {
/// The IP ranges we are connected to.
connected_resources: IpNetworkTable<()>,

gateway_received_icmp_packets: VecDeque<(Instant, IpAddr, IpAddr)>,
gateway_received_icmp_requests: VecDeque<(Instant, IpAddr, IpAddr)>,
client_received_icmp_replies: VecDeque<(Instant, IpAddr, IpAddr)>,
}

/// The possible transitions of the state machine.
Expand Down Expand Up @@ -159,9 +159,9 @@ impl StateMachineTest for TunnelTest {
gateway,
portal,
logger,
client_received_packets: Default::default(),
gateway_received_icmp_packets: Default::default(),
relay,
gateway_received_icmp_requests: Default::default(),
client_received_icmp_replies: Default::default(),
};

let mut buffered_transmits = VecDeque::new();
Expand Down Expand Up @@ -218,8 +218,12 @@ impl StateMachineTest for TunnelTest {

// Assert: Check that our actual state is equivalent to our expectation (the reference state).
assert_eq!(
state.gateway_received_icmp_packets,
ref_state.gateway_received_icmp_packets
state.gateway_received_icmp_requests,
ref_state.gateway_received_icmp_requests
);
assert_eq!(
state.client_received_icmp_replies,
ref_state.client_received_icmp_replies
);
assert!(buffered_transmits.is_empty()); // Sanity check to ensure we handled all packets.

Expand Down Expand Up @@ -277,7 +281,8 @@ impl ReferenceStateMachine for ReferenceState {
relay,
client_cidr_resources: IpNetworkTable::new(),
connected_resources: Default::default(),
gateway_received_icmp_packets: Default::default(),
gateway_received_icmp_requests: Default::default(),
client_received_icmp_replies: Default::default(),
})
.boxed()
}
Expand Down Expand Up @@ -426,7 +431,9 @@ impl TunnelTest {
continue;
}

if let ControlFlow::Break(_) = self.try_handle_gateway(dst, src, &payload) {
if let ControlFlow::Break(_) =
self.try_handle_gateway(dst, src, &payload, buffered_transmits)
{
continue;
}

Expand Down Expand Up @@ -536,7 +543,10 @@ impl TunnelTest {
return;
}

if self.try_handle_gateway(dst, src, payload).is_break() {
if self
.try_handle_gateway(dst, src, payload, buffered_transmits)
.is_break()
{
return;
}

Expand Down Expand Up @@ -577,7 +587,11 @@ impl TunnelTest {
.state
.decapsulate(dst, src, payload, self.now, &mut buffer)
}) {
self.client_received_packets.push_back(packet.to_owned());
self.client_received_icmp_replies.push_back((
self.now,
packet.source(),
packet.destination(),
));
};

ControlFlow::Break(())
Expand All @@ -588,6 +602,7 @@ impl TunnelTest {
dst: SocketAddr,
src: SocketAddr,
payload: &[u8],
buffered_transmits: &mut VecDeque<(Transmit<'static>, Option<SocketAddr>)>,
) -> ControlFlow<()> {
let mut buffer = [0u8; 200]; // In these tests, we only send ICMP packets which are very small.

Expand All @@ -602,11 +617,29 @@ impl TunnelTest {
}) {
// TODO: Assert that it is an ICMP packet.

self.gateway_received_icmp_packets.push_back((
self.now,
packet.source(),
packet.destination(),
));
let packet_src = packet.source();
let packet_dst = packet.destination();

assert_eq!(
packet_src,
self.client.tunnel_ip(packet_src),
"ICMP packet to originate from client"
);

self.gateway_received_icmp_requests
.push_back((self.now, packet_src, packet_dst));

if let Some(transmit) = self.gateway.span.in_scope(|| {
self.gateway.state.encapsulate(
ip_packet::make::icmp_response_packet(packet_dst, packet_src),
self.now,
)
}) {
let transmit = transmit.into_owned();
let dst = transmit.dst;

buffered_transmits.push_back((transmit, self.gateway.sending_socket_for(dst)));
};
};

ControlFlow::Break(())
Expand Down Expand Up @@ -776,8 +809,10 @@ impl ReferenceState {
if self.connected_resources.longest_match(dst).is_some() {
tracing::debug!("Connected to resource, expecting packet to be routed to gateway");

self.gateway_received_icmp_packets
self.gateway_received_icmp_requests
.push_back((self.now, src, dst));
self.client_received_icmp_replies
.push_back((self.now, dst, src));
return;
}

Expand Down
57 changes: 47 additions & 10 deletions rust/ip-packet/src/make.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,27 @@ use pnet_packet::{
use crate::MutableIpPacket;
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};

pub fn icmp_request_packet(source: IpAddr, dst: IpAddr) -> MutableIpPacket<'static> {
match (source, dst) {
pub fn icmp_request_packet(src: IpAddr, dst: IpAddr) -> MutableIpPacket<'static> {
icmp_packet(src, dst, 1, 0, IcmpKind::Request)
}

pub fn icmp_response_packet(src: IpAddr, dst: IpAddr) -> MutableIpPacket<'static> {
icmp_packet(src, dst, 1, 0, IcmpKind::Response)
}

enum IcmpKind {
Request,
Response,
}

fn icmp_packet(
src: IpAddr,
dst: IpAddr,
seq: u16,
identifier: u16,
kind: IcmpKind,
) -> MutableIpPacket<'static> {
match (src, dst) {
(IpAddr::V4(src), IpAddr::V4(dst)) => {
use crate::{
icmp::{
Expand All @@ -28,14 +47,24 @@ pub fn icmp_request_packet(source: IpAddr, dst: IpAddr) -> MutableIpPacket<'stat
ipv4_header(src, dst, IpNextHeaderProtocols::Icmp, &mut buf[..]);

let mut icmp_packet = MutableIcmpPacket::new(&mut buf[20..]).unwrap();
icmp_packet.set_icmp_type(IcmpTypes::EchoRequest);
icmp_packet.set_icmp_code(IcmpCodes::NoCode);

match kind {
IcmpKind::Request => {
icmp_packet.set_icmp_type(IcmpTypes::EchoRequest);
icmp_packet.set_icmp_code(IcmpCodes::NoCode);
}
IcmpKind::Response => {
icmp_packet.set_icmp_type(IcmpTypes::EchoReply);
icmp_packet.set_icmp_code(IcmpCodes::NoCode);
}
}

icmp_packet.set_checksum(0);

let mut echo_request_packet =
MutableEchoRequestPacket::new(icmp_packet.payload_mut()).unwrap();
echo_request_packet.set_sequence_number(1);
echo_request_packet.set_identifier(0);
echo_request_packet.set_sequence_number(seq);
echo_request_packet.set_identifier(identifier);
echo_request_packet.set_checksum(crate::util::checksum(
echo_request_packet.to_immutable().packet(),
2,
Expand All @@ -59,13 +88,21 @@ pub fn icmp_request_packet(source: IpAddr, dst: IpAddr) -> MutableIpPacket<'stat

let mut icmp_packet = MutableIcmpv6Packet::new(&mut buf[40..]).unwrap();

icmp_packet.set_icmpv6_type(Icmpv6Types::EchoRequest);
icmp_packet.set_icmpv6_code(Icmpv6Code::new(0)); // No code for echo request
match kind {
IcmpKind::Request => {
icmp_packet.set_icmpv6_type(Icmpv6Types::EchoRequest);
icmp_packet.set_icmpv6_code(Icmpv6Code::new(0));
}
IcmpKind::Response => {
icmp_packet.set_icmpv6_type(Icmpv6Types::EchoReply);
icmp_packet.set_icmpv6_code(Icmpv6Code::new(0));
}
}

let mut echo_request_packet =
MutableEchoRequestPacket::new(icmp_packet.payload_mut()).unwrap();
echo_request_packet.set_identifier(0);
echo_request_packet.set_sequence_number(1);
echo_request_packet.set_identifier(identifier);
echo_request_packet.set_sequence_number(seq);
echo_request_packet.set_checksum(0);

let checksum = crate::icmpv6::checksum(&icmp_packet.to_immutable(), &src, &dst);
Expand Down

0 comments on commit 994b25b

Please sign in to comment.