From fc5559069c3ffe150bf0df304b59c880a481b674 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 13:59:16 +0200 Subject: [PATCH 01/10] wire/dhcp: BROADCAST flag is MSB (0x8000), not LSB (0x0001). This fixes DHCP on Linksys WRT1900AC. With 0x0001, it would not reply to DISCOVERs at all, probably because seeing an unknown reserved flag being set. With 0x8000, it works fine. --- src/wire/dhcpv4.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wire/dhcpv4.rs b/src/wire/dhcpv4.rs index 8ac8e59..de281b3 100644 --- a/src/wire/dhcpv4.rs +++ b/src/wire/dhcpv4.rs @@ -455,7 +455,7 @@ impl> Packet { /// Returns true if the broadcast flag is set. pub fn broadcast_flag(&self) -> bool { let field = &self.buffer.as_ref()[field::FLAGS]; - NetworkEndian::read_u16(field) & 0b1 == 0b1 + NetworkEndian::read_u16(field) & 0x8000 == 0x8000 } } @@ -578,7 +578,7 @@ impl + AsMut<[u8]>> Packet { /// Sets the broadcast flag to the specified value. pub fn set_broadcast_flag(&mut self, value: bool) { let field = &mut self.buffer.as_mut()[field::FLAGS]; - NetworkEndian::write_u16(field, if value { 1 } else { 0 }); + NetworkEndian::write_u16(field, if value { 0x8000 } else { 0 }); } } From 6d37633353331a54cc27f77631eb0659d716dafd Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 14:02:27 +0200 Subject: [PATCH 02/10] wire/dhcp: use bitflags for the FLAGS field. --- src/wire/dhcpv4.rs | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/wire/dhcpv4.rs b/src/wire/dhcpv4.rs index de281b3..bddf144 100644 --- a/src/wire/dhcpv4.rs +++ b/src/wire/dhcpv4.rs @@ -1,5 +1,6 @@ // See https://tools.ietf.org/html/rfc2131 for the DHCP specification. +use bitflags::bitflags; use byteorder::{ByteOrder, NetworkEndian}; use crate::wire::arp::Hardware; @@ -34,6 +35,12 @@ enum_with_unknown! { } } +bitflags! { + pub struct Flags: u16 { + const BROADCAST = 0b1000_0000_0000_0000; + } +} + impl MessageType { fn opcode(&self) -> OpCode { match *self { @@ -452,10 +459,9 @@ impl> Packet { Ipv4Address::from_bytes(field) } - /// Returns true if the broadcast flag is set. - pub fn broadcast_flag(&self) -> bool { + pub fn flags(&self) -> Flags { let field = &self.buffer.as_ref()[field::FLAGS]; - NetworkEndian::read_u16(field) & 0x8000 == 0x8000 + Flags::from_bits_truncate(NetworkEndian::read_u16(field)) } } @@ -575,10 +581,10 @@ impl + AsMut<[u8]>> Packet { field.copy_from_slice(value.as_bytes()); } - /// Sets the broadcast flag to the specified value. - pub fn set_broadcast_flag(&mut self, value: bool) { + /// Sets the flags to the specified value. + pub fn set_flags(&mut self, val: Flags) { let field = &mut self.buffer.as_mut()[field::FLAGS]; - NetworkEndian::write_u16(field, if value { 0x8000 } else { 0 }); + NetworkEndian::write_u16(field, val.bits()); } } @@ -828,7 +834,7 @@ impl<'a> Repr<'a> { options = next_options; } - let broadcast = packet.broadcast_flag(); + let broadcast = packet.flags().contains(Flags::BROADCAST); Ok(Repr { transaction_id, @@ -870,7 +876,12 @@ impl<'a> Repr<'a> { packet.set_your_ip(self.your_ip); packet.set_server_ip(self.server_ip); packet.set_relay_agent_ip(self.relay_agent_ip); - packet.set_broadcast_flag(self.broadcast); + + let mut flags = Flags::empty(); + if self.broadcast { + flags |= Flags::BROADCAST; + } + packet.set_flags(flags); { let mut options = packet.options_mut()?; @@ -1072,7 +1083,7 @@ mod test { packet.set_hops(0); packet.set_transaction_id(0x3d1d); packet.set_secs(0); - packet.set_broadcast_flag(false); + packet.set_flags(Flags::empty()); packet.set_client_ip(IP_NULL); packet.set_your_ip(IP_NULL); packet.set_server_ip(IP_NULL); From d34f4f783b6464305ef2f7a3169ee493b1532f8d Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 14:03:50 +0200 Subject: [PATCH 03/10] socket/dhcp: do not set BROADCAST flag. Reasons: 1. We were already accidentally not setting the BROADCAST flag due to it being the wrong bit (see previous commit). 2. Major OSes don't set it. 3. rfc1542 section 3.1.1 states it's discouraged, and the issue it's supposed to workaround doesn't apply to smoltcp. Unfortunately, some client implementations are unable to receive such unicast IP datagrams until they know their own IP address (..) This addition to the protocol is a workaround for old host implementations. Such implementations SHOULD be modified so that they may receive unicast BOOTREPLY messages, thus making use of this workaround unnecessary. In general, the use of this mechanism is discouraged. --- src/socket/dhcpv4.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index d93ae4f..ee061a4 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -354,7 +354,7 @@ impl Dhcpv4Socket { router: None, subnet_mask: None, relay_agent_ip: Ipv4Address::UNSPECIFIED, - broadcast: true, + broadcast: false, requested_ip: None, client_identifier: Some(ethernet_addr), server_identifier: None, @@ -410,7 +410,6 @@ impl Dhcpv4Socket { } dhcp_repr.message_type = DhcpMessageType::Request; - dhcp_repr.broadcast = false; dhcp_repr.requested_ip = Some(state.requested_ip); dhcp_repr.server_identifier = Some(state.server.identifier); @@ -445,7 +444,6 @@ impl Dhcpv4Socket { ipv4_repr.dst_addr = state.server.address; dhcp_repr.message_type = DhcpMessageType::Request; dhcp_repr.client_ip = state.config.address.address(); - dhcp_repr.broadcast = false; net_debug!("DHCP send renew to {}: {:?}", ipv4_repr.dst_addr, dhcp_repr); ipv4_repr.payload_len = udp_repr.header_len() + dhcp_repr.buffer_len(); From af4db615f5a1d03df76880ed878626a674c3c77c Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 14:10:08 +0200 Subject: [PATCH 04/10] socket/dhcp: Use random transaction_id instead of sequential. This is a minor security improvement against blind packet spoofing, since it adds more entropy to the packets. --- src/socket/dhcpv4.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index ee061a4..19ee093 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -340,9 +340,9 @@ impl Dhcpv4Socket { // 0x0f * 4 = 60 bytes. const MAX_IPV4_HEADER_LEN: usize = 60; - // We don't directly increment transaction_id because sending the packet + // We don't directly modify self.transaction_id because sending the packet // may fail. We only want to update state after succesfully sending. - let next_transaction_id = self.transaction_id + 1; + let next_transaction_id = crate::rand::rand_u32(); let mut dhcp_repr = DhcpRepr { message_type: DhcpMessageType::Discover, From 3b7100c5010c827b38264a8d1f8055792256a470 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 19:41:10 +0200 Subject: [PATCH 05/10] socket/dhcp: log incoming reprs as well as outgoing. --- src/socket/dhcpv4.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index 19ee093..29f644f 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -203,10 +203,10 @@ impl Dhcpv4Socket { }; net_debug!( - "DHCP recv {:?} from {} ({})", + "DHCP recv {:?} from {}: {:?}", dhcp_repr.message_type, src_ip, - server_identifier + dhcp_repr ); match (&mut self.state, dhcp_repr.message_type) { From 94541ae827e9a1a500e076d4a6972814927ecafa Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 20:05:25 +0200 Subject: [PATCH 06/10] socket/dhcp: add basic test --- src/socket/dhcpv4.rs | 299 ++++++++++++++++++++++++++++++++++++++++++- src/socket/mod.rs | 4 +- 2 files changed, 301 insertions(+), 2 deletions(-) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index 29f644f..1af3ab5 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -100,6 +100,8 @@ enum ClientState { } /// Return value for the `Dhcpv4Socket::poll` function +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum Event<'a> { /// Configuration has been lost (for example, the lease has expired) Deconfigured, @@ -328,6 +330,16 @@ impl Dhcpv4Socket { Some((config, renew_at, expires_at)) } + #[cfg(not(test))] + fn random_transaction_id() -> u32 { + crate::rand::rand_u32() + } + + #[cfg(test)] + fn random_transaction_id() -> u32 { + 0x12345678 + } + pub(crate) fn dispatch(&mut self, cx: &Context, emit: F) -> Result<()> where F: FnOnce((Ipv4Repr, UdpRepr, DhcpRepr)) -> Result<()>, @@ -342,7 +354,7 @@ impl Dhcpv4Socket { // We don't directly modify self.transaction_id because sending the packet // may fail. We only want to update state after succesfully sending. - let next_transaction_id = crate::rand::rand_u32(); + let next_transaction_id = Self::random_transaction_id(); let mut dhcp_repr = DhcpRepr { message_type: DhcpMessageType::Discover, @@ -504,3 +516,288 @@ impl<'a> From for Socket<'a> { Socket::Dhcpv4(val) } } + +#[cfg(test)] +mod test { + + use super::*; + use crate::wire::EthernetAddress; + + // =========================================================================================// + // Helper functions + + fn send( + socket: &mut Dhcpv4Socket, + timestamp: Instant, + (ip_repr, udp_repr, dhcp_repr): (Ipv4Repr, UdpRepr, DhcpRepr), + ) -> Result<()> { + net_trace!("send: {:?}", ip_repr); + net_trace!(" {:?}", udp_repr); + net_trace!(" {:?}", dhcp_repr); + + let mut payload = vec![0; dhcp_repr.buffer_len()]; + dhcp_repr + .emit(&mut DhcpPacket::new_unchecked(&mut payload)) + .unwrap(); + + let mut cx = Context::DUMMY.clone(); + cx.now = timestamp; + socket.process(&cx, &ip_repr, &udp_repr, &payload) + } + + fn recv(socket: &mut Dhcpv4Socket, timestamp: Instant, mut f: F) + where + F: FnMut(Result<(Ipv4Repr, UdpRepr, DhcpRepr)>), + { + let mut cx = Context::DUMMY.clone(); + cx.now = timestamp; + let result = socket.dispatch(&cx, |(mut ip_repr, udp_repr, dhcp_repr)| { + assert_eq!(ip_repr.protocol, IpProtocol::Udp); + assert_eq!( + ip_repr.payload_len, + udp_repr.header_len() + dhcp_repr.buffer_len() + ); + + // We validated the payload len, change it to 0 to make equality testing easier + ip_repr.payload_len = 0; + + net_trace!("recv: {:?}", ip_repr); + net_trace!(" {:?}", udp_repr); + net_trace!(" {:?}", dhcp_repr); + Ok(f(Ok((ip_repr, udp_repr, dhcp_repr)))) + }); + match result { + Ok(()) => (), + Err(e) => f(Err(e)), + } + } + + macro_rules! send { + ($socket:ident, $repr:expr) => + (send!($socket, time 0, $repr)); + ($socket:ident, $repr:expr, $result:expr) => + (send!($socket, time 0, $repr, $result)); + ($socket:ident, time $time:expr, $repr:expr) => + (send!($socket, time $time, $repr, Ok(( )))); + ($socket:ident, time $time:expr, $repr:expr, $result:expr) => + (assert_eq!(send(&mut $socket, Instant::from_millis($time), $repr), $result)); + } + + macro_rules! recv { + ($socket:ident, [$( $repr:expr ),*]) => ({ + $( recv!($socket, Ok($repr)); )* + recv!($socket, Err(Error::Exhausted)) + }); + ($socket:ident, time $time:expr, [$( $repr:expr ),*]) => ({ + $( recv!($socket, time $time, Ok($repr)); )* + recv!($socket, time $time, Err(Error::Exhausted)) + }); + ($socket:ident, $result:expr) => + (recv!($socket, time 0, $result)); + ($socket:ident, time $time:expr, $result:expr) => + (recv(&mut $socket, Instant::from_millis($time), |result| { + assert_eq!(result, $result) + })); + } + + #[cfg(feature = "log")] + fn init_logger() { + struct Logger; + static LOGGER: Logger = Logger; + + impl log::Log for Logger { + fn enabled(&self, _metadata: &log::Metadata) -> bool { + true + } + + fn log(&self, record: &log::Record) { + println!("{}", record.args()); + } + + fn flush(&self) {} + } + + // If it fails, that just means we've already set it to the same value. + let _ = log::set_logger(&LOGGER); + log::set_max_level(log::LevelFilter::Trace); + + println!(); + } + + // =========================================================================================// + // Constants + + const TXID: u32 = 0x12345678; + + const MY_IP: Ipv4Address = Ipv4Address([192, 168, 1, 42]); + const SERVER_IP: Ipv4Address = Ipv4Address([192, 168, 1, 1]); + const DNS_IP_1: Ipv4Address = Ipv4Address([1, 1, 1, 1]); + const DNS_IP_2: Ipv4Address = Ipv4Address([1, 1, 1, 2]); + const DNS_IP_3: Ipv4Address = Ipv4Address([1, 1, 1, 3]); + const DNS_IPS: [Option; DHCP_MAX_DNS_SERVER_COUNT] = + [Some(DNS_IP_1), Some(DNS_IP_2), Some(DNS_IP_3)]; + const MASK_24: Ipv4Address = Ipv4Address([255, 255, 255, 0]); + + const MY_MAC: EthernetAddress = EthernetAddress([0x02, 0x02, 0x02, 0x02, 0x02, 0x02]); + + const IP_BROADCAST: Ipv4Repr = Ipv4Repr { + src_addr: Ipv4Address::UNSPECIFIED, + dst_addr: Ipv4Address::BROADCAST, + protocol: IpProtocol::Udp, + payload_len: 0, + hop_limit: 64, + }; + + const IP_RECV: Ipv4Repr = Ipv4Repr { + src_addr: SERVER_IP, + dst_addr: MY_IP, + protocol: IpProtocol::Udp, + payload_len: 0, + hop_limit: 64, + }; + + const IP_SEND: Ipv4Repr = Ipv4Repr { + src_addr: MY_IP, + dst_addr: SERVER_IP, + protocol: IpProtocol::Udp, + payload_len: 0, + hop_limit: 64, + }; + + const UDP_SEND: UdpRepr = UdpRepr { + src_port: 68, + dst_port: 67, + }; + const UDP_RECV: UdpRepr = UdpRepr { + src_port: 67, + dst_port: 68, + }; + + const DHCP_DEFAULT: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Unknown(99), + transaction_id: TXID, + client_hardware_address: MY_MAC, + client_ip: Ipv4Address::UNSPECIFIED, + your_ip: Ipv4Address::UNSPECIFIED, + server_ip: Ipv4Address::UNSPECIFIED, + router: None, + subnet_mask: None, + relay_agent_ip: Ipv4Address::UNSPECIFIED, + broadcast: false, + requested_ip: None, + client_identifier: None, + server_identifier: None, + parameter_request_list: None, + dns_servers: None, + max_size: None, + lease_duration: None, + }; + + const DHCP_DISCOVER: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Discover, + client_identifier: Some(MY_MAC), + parameter_request_list: Some(&[1, 3, 6]), + max_size: Some(1432), + ..DHCP_DEFAULT + }; + + const DHCP_OFFER: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Offer, + server_ip: SERVER_IP, + server_identifier: Some(SERVER_IP), + + your_ip: MY_IP, + router: Some(SERVER_IP), + subnet_mask: Some(MASK_24), + dns_servers: Some(DNS_IPS), + lease_duration: Some(60), + + ..DHCP_DEFAULT + }; + + const DHCP_REQUEST: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Request, + client_identifier: Some(MY_MAC), + server_identifier: Some(SERVER_IP), + max_size: Some(1432), + + requested_ip: Some(MY_IP), + parameter_request_list: Some(&[1, 3, 6]), + ..DHCP_DEFAULT + }; + + const DHCP_ACK: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Ack, + server_ip: SERVER_IP, + server_identifier: Some(SERVER_IP), + + your_ip: MY_IP, + router: Some(SERVER_IP), + subnet_mask: Some(MASK_24), + dns_servers: Some(DNS_IPS), + lease_duration: Some(60), + + ..DHCP_DEFAULT + }; + + // =========================================================================================// + // Tests + + fn socket() -> Dhcpv4Socket { + #[cfg(feature = "log")] + init_logger(); + + let mut s = Dhcpv4Socket::new(); + assert_eq!(s.poll(), Some(Event::Deconfigured)); + s + } + + fn socket_bound() -> Dhcpv4Socket { + let mut s = socket(); + s.state = ClientState::Renewing(RenewState { + config: Config { + address: Ipv4Cidr::new(MY_IP, 24), + dns_servers: DNS_IPS, + router: Some(SERVER_IP), + }, + server: ServerInfo { + address: SERVER_IP, + identifier: SERVER_IP, + }, + renew_at: Instant::from_secs(30), + expires_at: Instant::from_secs(60), + }); + + s + } + + #[test] + fn test_bind() { + let mut s = socket(); + + recv!(s, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + assert_eq!(s.poll(), None); + send!(s, (IP_RECV, UDP_RECV, DHCP_OFFER)); + assert_eq!(s.poll(), None); + recv!(s, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + assert_eq!(s.poll(), None); + send!(s, (IP_RECV, UDP_RECV, DHCP_ACK)); + + assert_eq!( + s.poll(), + Some(Event::Configured(&Config { + address: Ipv4Cidr::new(MY_IP, 24), + dns_servers: DNS_IPS, + router: Some(SERVER_IP), + })) + ); + + match s.state { + ClientState::Renewing(r) => { + assert_eq!(r.renew_at, Instant::from_secs(30)); + assert_eq!(r.expires_at, Instant::from_secs(60)); + } + _ => panic!("Invalid state"), + } + } +} diff --git a/src/socket/mod.rs b/src/socket/mod.rs index d1de47d..d28bb2d 100644 --- a/src/socket/mod.rs +++ b/src/socket/mod.rs @@ -216,7 +216,9 @@ impl Context { max_transmission_unit: 1500, }, #[cfg(all(feature = "medium-ethernet", feature = "socket-dhcpv4"))] - ethernet_address: None, + ethernet_address: Some(crate::wire::EthernetAddress([ + 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, + ])), now: Instant::from_millis_const(0), }; } From 6768d89165a3252e73c7f6b035dde1fd1f1b8cbb Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Thu, 14 Oct 2021 23:33:25 +0200 Subject: [PATCH 07/10] socket/dhcp: add renew test --- src/socket/dhcpv4.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index 1af3ab5..a1fe717 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -740,6 +740,18 @@ mod test { ..DHCP_DEFAULT }; + const DHCP_RENEW: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Request, + client_identifier: Some(MY_MAC), + // NO server_identifier in renew requests, only in first one! + client_ip: MY_IP, + max_size: Some(1432), + + requested_ip: None, + parameter_request_list: Some(&[1, 3, 6]), + ..DHCP_DEFAULT + }; + // =========================================================================================// // Tests @@ -800,4 +812,35 @@ mod test { _ => panic!("Invalid state"), } } + + #[test] + fn test_renew() { + let mut s = socket_bound(); + + recv!(s, []); + assert_eq!(s.poll(), None); + recv!(s, time 40_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + assert_eq!(s.poll(), None); + + match &s.state { + ClientState::Renewing(r) => { + // the expiration still hasn't been bumped, because + // we haven't received the ACK yet + assert_eq!(r.expires_at, Instant::from_secs(60)); + } + _ => panic!("Invalid state"), + } + + send!(s, time 40_000, (IP_RECV, UDP_RECV, DHCP_ACK)); + assert_eq!(s.poll(), None); + + match &s.state { + ClientState::Renewing(r) => { + // NOW the expiration gets bumped + assert_eq!(r.renew_at, Instant::from_secs(40 + 30)); + assert_eq!(r.expires_at, Instant::from_secs(40 + 60)); + } + _ => panic!("Invalid state"), + } + } } From a43a6772c96877868f88b2166c7ceae45b5c5b79 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Fri, 15 Oct 2021 00:38:42 +0200 Subject: [PATCH 08/10] socket/dhcp: add retransmission/timeout tests --- src/socket/dhcpv4.rs | 192 +++++++++++++++++++++++++++++++++---------- 1 file changed, 148 insertions(+), 44 deletions(-) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index a1fe717..b5bd824 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -545,30 +545,43 @@ mod test { socket.process(&cx, &ip_repr, &udp_repr, &payload) } - fn recv(socket: &mut Dhcpv4Socket, timestamp: Instant, mut f: F) - where - F: FnMut(Result<(Ipv4Repr, UdpRepr, DhcpRepr)>), - { + fn recv( + socket: &mut Dhcpv4Socket, + timestamp: Instant, + reprs: &[(Ipv4Repr, UdpRepr, DhcpRepr)], + ) { let mut cx = Context::DUMMY.clone(); cx.now = timestamp; - let result = socket.dispatch(&cx, |(mut ip_repr, udp_repr, dhcp_repr)| { - assert_eq!(ip_repr.protocol, IpProtocol::Udp); - assert_eq!( - ip_repr.payload_len, - udp_repr.header_len() + dhcp_repr.buffer_len() - ); - // We validated the payload len, change it to 0 to make equality testing easier - ip_repr.payload_len = 0; + let mut i = 0; - net_trace!("recv: {:?}", ip_repr); - net_trace!(" {:?}", udp_repr); - net_trace!(" {:?}", dhcp_repr); - Ok(f(Ok((ip_repr, udp_repr, dhcp_repr)))) - }); - match result { - Ok(()) => (), - Err(e) => f(Err(e)), + while socket.poll_at(&cx) <= PollAt::Time(timestamp) { + let _ = socket.dispatch(&cx, |(mut ip_repr, udp_repr, dhcp_repr)| { + assert_eq!(ip_repr.protocol, IpProtocol::Udp); + assert_eq!( + ip_repr.payload_len, + udp_repr.header_len() + dhcp_repr.buffer_len() + ); + + // We validated the payload len, change it to 0 to make equality testing easier + ip_repr.payload_len = 0; + + net_trace!("recv: {:?}", ip_repr); + net_trace!(" {:?}", udp_repr); + net_trace!(" {:?}", dhcp_repr); + + let got_repr = (ip_repr, udp_repr, dhcp_repr); + match reprs.get(i) { + Some(want_repr) => assert_eq!(want_repr, &got_repr), + None => panic!("Too many reprs emitted"), + } + i += 1; + Ok(()) + }); + } + + if i != reprs.len() { + panic!("Too few reprs emitted. Wanted {}, got {}", reprs.len(), i); } } @@ -584,20 +597,12 @@ mod test { } macro_rules! recv { - ($socket:ident, [$( $repr:expr ),*]) => ({ - $( recv!($socket, Ok($repr)); )* - recv!($socket, Err(Error::Exhausted)) + ($socket:ident, $reprs:expr) => ({ + recv!($socket, time 0, $reprs); }); - ($socket:ident, time $time:expr, [$( $repr:expr ),*]) => ({ - $( recv!($socket, time $time, Ok($repr)); )* - recv!($socket, time $time, Err(Error::Exhausted)) + ($socket:ident, time $time:expr, $reprs:expr) => ({ + recv(&mut $socket, Instant::from_millis($time), &$reprs); }); - ($socket:ident, $result:expr) => - (recv!($socket, time 0, $result)); - ($socket:ident, time $time:expr, $result:expr) => - (recv(&mut $socket, Instant::from_millis($time), |result| { - assert_eq!(result, $result) - })); } #[cfg(feature = "log")] @@ -710,7 +715,7 @@ mod test { router: Some(SERVER_IP), subnet_mask: Some(MASK_24), dns_servers: Some(DNS_IPS), - lease_duration: Some(60), + lease_duration: Some(1000), ..DHCP_DEFAULT }; @@ -735,7 +740,7 @@ mod test { router: Some(SERVER_IP), subnet_mask: Some(MASK_24), dns_servers: Some(DNS_IPS), - lease_duration: Some(60), + lease_duration: Some(1000), ..DHCP_DEFAULT }; @@ -776,8 +781,8 @@ mod test { address: SERVER_IP, identifier: SERVER_IP, }, - renew_at: Instant::from_secs(30), - expires_at: Instant::from_secs(60), + renew_at: Instant::from_secs(500), + expires_at: Instant::from_secs(1000), }); s @@ -804,43 +809,142 @@ mod test { })) ); - match s.state { + match &s.state { ClientState::Renewing(r) => { - assert_eq!(r.renew_at, Instant::from_secs(30)); - assert_eq!(r.expires_at, Instant::from_secs(60)); + assert_eq!(r.renew_at, Instant::from_secs(500)); + assert_eq!(r.expires_at, Instant::from_secs(1000)); } _ => panic!("Invalid state"), } } + #[test] + fn test_discover_retransmit() { + let mut s = socket(); + + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + recv!(s, time 1_000, []); + recv!(s, time 10_000, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + recv!(s, time 11_000, []); + recv!(s, time 20_000, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + + // check after retransmits it still works + send!(s, time 20_000, (IP_RECV, UDP_RECV, DHCP_OFFER)); + recv!(s, time 20_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + } + + #[test] + fn test_request_retransmit() { + let mut s = socket(); + + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + send!(s, time 0, (IP_RECV, UDP_RECV, DHCP_OFFER)); + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 1_000, []); + recv!(s, time 5_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 6_000, []); + recv!(s, time 10_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 15_000, []); + recv!(s, time 20_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + + // check after retransmits it still works + send!(s, time 20_000, (IP_RECV, UDP_RECV, DHCP_ACK)); + + match &s.state { + ClientState::Renewing(r) => { + assert_eq!(r.renew_at, Instant::from_secs(20 + 500)); + assert_eq!(r.expires_at, Instant::from_secs(20 + 1000)); + } + _ => panic!("Invalid state"), + } + } + + #[test] + fn test_request_timeout() { + let mut s = socket(); + + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + send!(s, time 0, (IP_RECV, UDP_RECV, DHCP_OFFER)); + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 5_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 10_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 20_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + recv!(s, time 30_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + + // After 5 tries and 70 seconds, it gives up. + // 5 + 5 + 10 + 10 + 20 = 70 + recv!(s, time 70_000, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + + // check it still works + send!(s, time 60_000, (IP_RECV, UDP_RECV, DHCP_OFFER)); + recv!(s, time 60_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + } + #[test] fn test_renew() { let mut s = socket_bound(); recv!(s, []); assert_eq!(s.poll(), None); - recv!(s, time 40_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + recv!(s, time 500_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); assert_eq!(s.poll(), None); match &s.state { ClientState::Renewing(r) => { // the expiration still hasn't been bumped, because // we haven't received the ACK yet - assert_eq!(r.expires_at, Instant::from_secs(60)); + assert_eq!(r.expires_at, Instant::from_secs(1000)); } _ => panic!("Invalid state"), } - send!(s, time 40_000, (IP_RECV, UDP_RECV, DHCP_ACK)); + send!(s, time 500_000, (IP_RECV, UDP_RECV, DHCP_ACK)); assert_eq!(s.poll(), None); match &s.state { ClientState::Renewing(r) => { // NOW the expiration gets bumped - assert_eq!(r.renew_at, Instant::from_secs(40 + 30)); - assert_eq!(r.expires_at, Instant::from_secs(40 + 60)); + assert_eq!(r.renew_at, Instant::from_secs(500 + 500)); + assert_eq!(r.expires_at, Instant::from_secs(500 + 1000)); } _ => panic!("Invalid state"), } } + + #[test] + fn test_renew_retransmit() { + let mut s = socket_bound(); + + recv!(s, []); + recv!(s, time 500_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + recv!(s, time 749_000, []); + recv!(s, time 750_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + recv!(s, time 874_000, []); + recv!(s, time 875_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + + // check it still works + send!(s, time 875_000, (IP_RECV, UDP_RECV, DHCP_ACK)); + match &s.state { + ClientState::Renewing(r) => { + // NOW the expiration gets bumped + assert_eq!(r.renew_at, Instant::from_secs(875 + 500)); + assert_eq!(r.expires_at, Instant::from_secs(875 + 1000)); + } + _ => panic!("Invalid state"), + } + } + + #[test] + fn test_renew_timeout() { + let mut s = socket_bound(); + + recv!(s, []); + recv!(s, time 500_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + recv!(s, time 999_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + recv!(s, time 1_000_000, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + match &s.state { + ClientState::Discovering(_) => {} + _ => panic!("Invalid state"), + } + } } From bcf6211fbef3aecf7fcdc2eb62425536ecbdcd94 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Fri, 15 Oct 2021 01:02:31 +0200 Subject: [PATCH 09/10] socket/dhcp: add nak tests --- src/socket/dhcpv4.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index b5bd824..fc3f925 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -653,6 +653,14 @@ mod test { hop_limit: 64, }; + const IP_SERVER_BROADCAST: Ipv4Repr = Ipv4Repr { + src_addr: SERVER_IP, + dst_addr: Ipv4Address::BROADCAST, + protocol: IpProtocol::Udp, + payload_len: 0, + hop_limit: 64, + }; + const IP_RECV: Ipv4Repr = Ipv4Repr { src_addr: SERVER_IP, dst_addr: MY_IP, @@ -745,6 +753,13 @@ mod test { ..DHCP_DEFAULT }; + const DHCP_NAK: DhcpRepr = DhcpRepr { + message_type: DhcpMessageType::Nak, + server_ip: SERVER_IP, + server_identifier: Some(SERVER_IP), + ..DHCP_DEFAULT + }; + const DHCP_RENEW: DhcpRepr = DhcpRepr { message_type: DhcpMessageType::Request, client_identifier: Some(MY_MAC), @@ -880,6 +895,17 @@ mod test { recv!(s, time 60_000, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); } + #[test] + fn test_request_nak() { + let mut s = socket(); + + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + send!(s, time 0, (IP_RECV, UDP_RECV, DHCP_OFFER)); + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_REQUEST)]); + send!(s, time 0, (IP_SERVER_BROADCAST, UDP_RECV, DHCP_NAK)); + recv!(s, time 0, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + } + #[test] fn test_renew() { let mut s = socket_bound(); @@ -947,4 +973,13 @@ mod test { _ => panic!("Invalid state"), } } + + #[test] + fn test_renew_nak() { + let mut s = socket_bound(); + + recv!(s, time 500_000, [(IP_SEND, UDP_SEND, DHCP_RENEW)]); + send!(s, time 500_000, (IP_SERVER_BROADCAST, UDP_RECV, DHCP_NAK)); + recv!(s, time 500_000, [(IP_BROADCAST, UDP_SEND, DHCP_DISCOVER)]); + } } From 48debf7db89876aa5ade4ee8dd48e584e991cced Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Tue, 19 Oct 2021 01:17:28 +0200 Subject: [PATCH 10/10] dhcp: add "ignore NAKs" option. This workarounds a known broken Vodafone Fiber router which replies with both NAK and ACK: ![screenshot-2021-10-19_01-18-41](https://user-images.githubusercontent.com/1247578/137819133-a8f9ab28-8bc5-4cca-9c91-2eac15d88070.png) --- src/socket/dhcpv4.rs | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/src/socket/dhcpv4.rs b/src/socket/dhcpv4.rs index fc3f925..2673720 100644 --- a/src/socket/dhcpv4.rs +++ b/src/socket/dhcpv4.rs @@ -122,6 +122,9 @@ pub struct Dhcpv4Socket { /// Max lease duration. If set, it sets a maximum cap to the server-provided lease duration. /// Useful to react faster to IP configuration changes and to test whether renews work correctly. max_lease_duration: Option, + + /// Ignore NAKs. + ignore_naks: bool, } /// DHCP client socket. @@ -141,17 +144,45 @@ impl Dhcpv4Socket { config_changed: true, transaction_id: 1, max_lease_duration: None, + ignore_naks: false, } } + /// Get the configured max lease duration. + /// + /// See also [`Self::set_max_lease_duration()`] pub fn max_lease_duration(&self) -> Option { self.max_lease_duration } + /// Set the max lease duration. + /// + /// When set, the lease duration will be capped at the configured duration if the + /// DHCP server gives us a longer lease. This is generally not recommended, but + /// can be useful for debugging or reacting faster to network configuration changes. + /// + /// If None, no max is applied (the lease duration from the DHCP server is used.) pub fn set_max_lease_duration(&mut self, max_lease_duration: Option) { self.max_lease_duration = max_lease_duration; } + /// Get whether to ignore NAKs. + /// + /// See also [`Self::set_ignore_naks()`] + pub fn ignore_naks(&self) -> bool { + self.ignore_naks + } + + /// Set whether to ignore NAKs. + /// + /// This is not compliant with the DHCP RFCs, since theoretically + /// we must stop using the assigned IP when receiving a NAK. This + /// can increase reliability on broken networks with buggy routers + /// or rogue DHCP servers, however. + pub fn set_ignore_naks(&mut self, ignore_naks: bool) { + self.ignore_naks = ignore_naks; + } + pub(crate) fn poll_at(&self, _cx: &Context) -> PollAt { let t = match &self.state { ClientState::Discovering(state) => state.retry_at, @@ -242,7 +273,9 @@ impl Dhcpv4Socket { } } (ClientState::Requesting(_), DhcpMessageType::Nak) => { - self.reset(); + if !self.ignore_naks { + self.reset(); + } } (ClientState::Renewing(state), DhcpMessageType::Ack) => { if let Some((config, renew_at, expires_at)) = @@ -257,7 +290,9 @@ impl Dhcpv4Socket { } } (ClientState::Renewing(_), DhcpMessageType::Nak) => { - self.reset(); + if !self.ignore_naks { + self.reset(); + } } _ => { net_debug!(