DHCP improvements.

Cargo.toml

@@ -63,7 +63,7 @@ members = [
 [workspace.dependencies]
 embassy-futures = { version = "0.1", default-features = false }
 embassy-sync = { version = "0.3", default-features = false }
-embassy-time = { version = "0.1", default-features = false }
+embassy-time = { version = "0.2", default-features = false }
 embedded-io-async = { version = "0.6", default-features = false }
 embedded-nal-async = { version = "0.6", default-features = false }
 embedded-svc = { version = "0.26", default-features = false, features = ["embedded-io-async"] }

edge-dhcp/src/lib.rs

@@ -1,7 +1,10 @@
+#![no_std]
+
+use core::fmt;
 /// This code is a `no_std` and no-alloc modification of https://github.com/krolaw/dhcp4r
 use core::str::Utf8Error;
 
-use no_std_net::Ipv4Addr;
+pub use no_std_net::Ipv4Addr;
 
 use num_enum::TryFromPrimitive;
 
@@ -78,6 +81,22 @@ pub enum MessageType {
     Inform = 8,
 }
 
+impl fmt::Display for MessageType {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::Discover => "DHCPDISCOVER",
+            Self::Offer => "DHCPOFFER",
+            Self::Request => "DHCPREQUEST",
+            Self::Decline => "DHCPDECLINE",
+            Self::Ack => "DHCPACK",
+            Self::Nak => "DHCPNAK",
+            Self::Release => "DHCPRELEASE",
+            Self::Inform => "DHCPINFORM",
+        }
+        .fmt(f)
+    }
+}
+
 /// DHCP Packet Structure
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub struct Packet<'a> {
@@ -131,16 +150,26 @@ impl<'a> Packet<'a> {
     }
 
     pub fn new_reply<'b>(&self, ip: Option<Ipv4Addr>, options: Options<'b>) -> Packet<'b> {
+        let mut ciaddr = Ipv4Addr::UNSPECIFIED;
+        if ip.is_some() {
+            for opt in self.options.iter() {
+                if matches!(opt, DhcpOption::MessageType(MessageType::Request)) {
+                    ciaddr = self.ciaddr;
+                    break;
+                }
+            }
+        }
+
         Packet {
             reply: true,
             hops: 0,
             xid: self.xid,
             secs: 0,
             broadcast: self.broadcast,
-            ciaddr: ip.unwrap_or(Ipv4Addr::UNSPECIFIED),
+            ciaddr,
             yiaddr: ip.unwrap_or(Ipv4Addr::UNSPECIFIED),
             siaddr: Ipv4Addr::UNSPECIFIED,
-            giaddr: Ipv4Addr::UNSPECIFIED,
+            giaddr: self.giaddr,
             chaddr: self.chaddr,
             options,
         }
@@ -297,7 +326,7 @@ impl From<&Packet<'_>> for Settings {
     }
 }
 
-#[derive(Clone, PartialEq, Eq, Debug)]
+#[derive(Clone, PartialEq, Eq)]
 pub struct Options<'a>(OptionsInner<'a>);
 
 impl<'a> Options<'a> {
@@ -429,6 +458,22 @@ impl<'a> Options<'a> {
     pub fn iter(&self) -> impl Iterator<Item = DhcpOption<'a>> + 'a {
         self.0.iter()
     }
+
+    pub(crate) fn requested_ip(&self) -> Option<Ipv4Addr> {
+        self.iter().find_map(|option| {
+            if let DhcpOption::RequestedIpAddress(ip) = option {
+                Some(ip)
+            } else {
+                None
+            }
+        })
+    }
+}
+
+impl fmt::Debug for Options<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_set().entries(self.iter()).finish()
+    }
 }
 
 #[derive(Clone, PartialEq, Eq, Debug)]
@@ -480,16 +525,30 @@ impl<'a> OptionsInner<'a> {
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum DhcpOption<'a> {
+    /// 53: DHCP Message Type
     MessageType(MessageType),
+    /// 54: Server Identifier
     ServerIdentifier(Ipv4Addr),
+    /// 55: Parameter Request List
     ParameterRequestList(&'a [u8]),
+    /// 50: Requested IP Address
     RequestedIpAddress(Ipv4Addr),
+    /// 12: Host Name Option
     HostName(&'a str),
+    /// 3: Router Option
     Router(Ipv4Addrs<'a>),
+    /// 6: Domain Name Server Option
     DomainNameServer(Ipv4Addrs<'a>),
+    /// 51: IP Address Lease Time
     IpAddressLeaseTime(u32),
+    /// 1: Subnet Mask
     SubnetMask(Ipv4Addr),
+    /// 56: Message
     Message(&'a str),
+    /// 57: Maximum DHCP Message Size
+    MaximumMessageSize(u16),
+    /// 61: Client-identifier
+    ClientIdentifier(&'a [u8]),
     Unrecognized(u8, &'a [u8]),
 }
 
@@ -521,6 +580,9 @@ impl<'a> DhcpOption<'a> {
                 HOST_NAME => DhcpOption::HostName(
                     core::str::from_utf8(bytes.remaining()).map_err(Error::InvalidUtf8Str)?,
                 ),
+                MAXIMUM_DHCP_MESSAGE_SIZE => {
+                    DhcpOption::MaximumMessageSize(u16::from_be_bytes(bytes.remaining_arr()?))
+                }
                 ROUTER => {
                     DhcpOption::Router(Ipv4Addrs(Ipv4AddrsInner::ByteSlice(bytes.remaining())))
                 }
@@ -534,6 +596,13 @@ impl<'a> DhcpOption<'a> {
                 MESSAGE => DhcpOption::Message(
                     core::str::from_utf8(bytes.remaining()).map_err(Error::InvalidUtf8Str)?,
                 ),
+                CLIENT_IDENTIFIER => {
+                    if len < 2 {
+                        return Err(Error::DataUnderflow);
+                    }
+
+                    DhcpOption::ClientIdentifier(bytes.remaining())
+                }
                 _ => DhcpOption::Unrecognized(code, bytes.remaining()),
             };
 
@@ -563,7 +632,9 @@ impl<'a> DhcpOption<'a> {
             Self::DomainNameServer(_) => DOMAIN_NAME_SERVER,
             Self::IpAddressLeaseTime(_) => IP_ADDRESS_LEASE_TIME,
             Self::SubnetMask(_) => SUBNET_MASK,
+            Self::MaximumMessageSize(_) => MAXIMUM_DHCP_MESSAGE_SIZE,
             Self::Message(_) => MESSAGE,
+            Self::ClientIdentifier(_) => CLIENT_IDENTIFIER,
             Self::Unrecognized(code, _) => *code,
         }
     }
@@ -585,6 +656,8 @@ impl<'a> DhcpOption<'a> {
             Self::IpAddressLeaseTime(secs) => f(&secs.to_be_bytes()),
             Self::SubnetMask(mask) => f(&mask.octets()),
             Self::Message(msg) => f(msg.as_bytes()),
+            Self::MaximumMessageSize(size) => f(&size.to_be_bytes()),
+            Self::ClientIdentifier(id) => f(id),
             Self::Unrecognized(_, data) => f(data),
         }
     }
@@ -657,3 +730,5 @@ const DHCP_MESSAGE_TYPE: u8 = 53;
 const SERVER_IDENTIFIER: u8 = 54;
 const PARAMETER_REQUEST_LIST: u8 = 55;
 const MESSAGE: u8 = 56;
+const MAXIMUM_DHCP_MESSAGE_SIZE: u8 = 57;
+const CLIENT_IDENTIFIER: u8 = 61;

edge-dhcp/src/server.rs

@@ -2,7 +2,7 @@ use core::fmt::Debug;
 
 use embassy_time::{Duration, Instant};
 
-use log::info;
+use log::{info, warn};
 
 use super::*;
 
@@ -34,77 +34,71 @@ impl<'a> ServerOptions<'a> {
             return None;
         }
 
-        let mt = request.options.iter().find_map(|option| {
-            if let DhcpOption::MessageType(mt) = option {
-                Some(mt)
+        let message_type = request.options.iter().find_map(|option| {
+            if let DhcpOption::MessageType(message_type) = option {
+                Some(message_type)
             } else {
                 None
             }
         });
 
-        if let Some(mt) = mt {
-            let server_identifier = request.options.iter().find_map(|option| {
-                if let DhcpOption::ServerIdentifier(ip) = option {
-                    Some(ip)
-                } else {
-                    None
-                }
-            });
-
-            if server_identifier == Some(self.ip)
-                || server_identifier.is_none() && matches!(mt, MessageType::Discover)
-            {
-                info!("Request: ({mt:?}) {request:?}");
-
-                let request = match mt {
-                    MessageType::Discover => {
-                        let requested_ip = request.options.iter().find_map(|option| {
-                            if let DhcpOption::RequestedIpAddress(ip) = option {
-                                Some(ip)
-                            } else {
-                                None
-                            }
-                        });
-
-                        Some(Action::Discover(requested_ip, &request.chaddr))
-                    }
-                    MessageType::Request => {
-                        let ip = request
-                            .options
-                            .iter()
-                            .find_map(|option| {
-                                if let DhcpOption::RequestedIpAddress(ip) = option {
-                                    Some(ip)
-                                } else {
-                                    None
-                                }
-                            })
-                            .unwrap_or(request.ciaddr);
-
-                        Some(Action::Request(ip, &request.chaddr))
-                    }
-                    MessageType::Release => Some(Action::Release(request.yiaddr, &request.chaddr)),
-                    MessageType::Decline => Some(Action::Decline(request.yiaddr, &request.chaddr)),
-                    _ => None,
-                };
+        let message_type = if let Some(message_type) = message_type {
+            message_type
+        } else {
+            warn!("Ignoring DHCP request, no message type found: {request:?}");
+            return None;
+        };
 
-                return request;
+        let server_identifier = request.options.iter().find_map(|option| {
+            if let DhcpOption::ServerIdentifier(ip) = option {
+                Some(ip)
+            } else {
+                None
             }
+        });
+
+        if server_identifier.is_some() && server_identifier != Some(self.ip) {
+            warn!("Ignoring {message_type} request, not addressed to this server: {request:?}");
+            return None;
         }
 
-        None
+        info!("Received {message_type} request: {request:?}");
+        match message_type {
+            MessageType::Discover => Some(Action::Discover(
+                request.options.requested_ip(),
+                &request.chaddr,
+            )),
+            MessageType::Request => {
+                let requested_ip = request.options.requested_ip().or_else(|| {
+                    if request.ciaddr.is_unspecified() {
+                        None
+                    } else {
+                        Some(request.ciaddr)
+                    }
+                })?;
+
+                Some(Action::Request(requested_ip, &request.chaddr))
+            }
+            MessageType::Release if server_identifier == Some(self.ip) => {
+                Some(Action::Release(request.yiaddr, &request.chaddr))
+            }
+            MessageType::Decline if server_identifier == Some(self.ip) => {
+                Some(Action::Decline(request.yiaddr, &request.chaddr))
+            }
+            _ => None,
+        }
     }
 
     pub fn offer(
         &self,
         request: &Packet,
-        ip: Ipv4Addr,
+        yiaddr: Ipv4Addr,
         opt_buf: &'a mut [DhcpOption<'a>],
     ) -> Packet<'a> {
-        self.reply(request, MessageType::Offer, Some(ip), opt_buf)
+        self.reply(request, MessageType::Offer, Some(yiaddr), opt_buf)
     }
 
-    pub fn ack_nack(
+    pub fn ack_nak(
         &self,
         request: &Packet,
         ip: Option<Ipv4Addr>,
@@ -125,14 +119,14 @@ impl<'a> ServerOptions<'a> {
     fn reply(
         &self,
         request: &Packet,
-        mt: MessageType,
+        message_type: MessageType,
         ip: Option<Ipv4Addr>,
         buf: &'a mut [DhcpOption<'a>],
     ) -> Packet<'a> {
         let reply = request.new_reply(
             ip,
             request.options.reply(
-                mt,
+                message_type,
                 self.ip,
                 self.lease_duration.as_secs() as _,
                 self.gateways,
@@ -142,7 +136,7 @@ impl<'a> ServerOptions<'a> {
             ),
         );
 
-        info!("Reply: {reply:?}");
+        info!("Sending {message_type} reply: {reply:?}");
 
         reply
     }
@@ -185,7 +179,7 @@ impl<const N: usize> Server<N> {
                         ))
                     .then_some(ip);
 
-                    Some(server_options.ack_nack(request, ip, opt_buf))
+                    Some(server_options.ack_nak(request, ip, opt_buf))
                 }
                 Action::Release(_ip, mac) | Action::Decline(_ip, mac) => {
                     self.remove_lease(mac);