some bug fixes

- check interface is on subnet before inserting netmask/router into
  response
- dont always give back broadcast option (28)
- if config was changed but db contains an old lease, we delete
the expired lease before inserting a new one
- dont set broadcast flag on relayed ACK responses

Dockerfile

@@ -16,6 +16,8 @@ RUN apt-get -qq update; \
     apt-get -qq --no-install-recommends install \
     dumb-init \
     isc-dhcp-server \
+    iputils-ping \
+    iproute2 \
     ca-certificates \
     wget \
     sudo;

dora-core/src/server/context.rs

@@ -602,6 +602,7 @@ impl MsgContext<v4::Message> {
     /// these will be overwritten.
     pub fn populate_opts(&mut self, param_opts: &v4::DhcpOptions) -> Option<()> {
         use dhcproto::v4::{DhcpOption, OptionCode};
+        let subnet = self.subnet();
         // https://datatracker.ietf.org/doc/html/rfc3046#section-2.2
         // copy opt 82 (relay agent) into response
         let resp = self.resp_msg.as_mut()?;
@@ -614,21 +615,37 @@ impl MsgContext<v4::Message> {
         if let Some(id) = self.msg.opts().get(OptionCode::ClientIdentifier) {
             resp.opts_mut().insert(id.clone());
         }
-
-        // insert router & net mask & broadcast from interface
+        let mut interface_match = false;
+        // insert router/netmask
         // if the config provides these also, they will be overwritten
         if let Some(IpNetwork::V4(interface)) = self.interface {
-            resp.opts_mut()
-                .insert(DhcpOption::Router(vec![interface.ip()]));
-            resp.opts_mut()
-                .insert(DhcpOption::SubnetMask(interface.mask()));
-            resp.opts_mut()
-                .insert(DhcpOption::BroadcastAddr(interface.broadcast()));
+            // if we populate from interface, interface must be on same subnet as packet (local)
+            if matches!(subnet, Ok(subnet) if interface.contains(subnet)) {
+                resp.opts_mut()
+                    .insert(DhcpOption::Router(vec![interface.ip()]));
+                resp.opts_mut()
+                    .insert(DhcpOption::SubnetMask(interface.mask()));
+                interface_match = true;
+            }
+            // configured router/netmask will override interface
+            if let Some(v) = param_opts.get(OptionCode::Router) {
+                resp.opts_mut().insert(v.clone());
+            }
+            if let Some(v) = param_opts.get(OptionCode::SubnetMask) {
+                resp.opts_mut().insert(v.clone());
+            }
         }
 
         if let Some(DhcpOption::ParameterRequestList(requested)) =
             self.msg.opts().get(OptionCode::ParameterRequestList)
         {
+            // if broadcast addr is requested, try to fill from interface
+            if let Some(IpNetwork::V4(interface)) = self.interface {
+                if requested.contains(&v4::OptionCode::BroadcastAddr) && interface_match {
+                    resp.opts_mut()
+                        .insert(DhcpOption::BroadcastAddr(interface.broadcast()));
+                }
+            }
             // look in the requested list of params
             for code in requested {
                 // if we have that option, add it to the response

libs/ip-manager/src/lib.rs

@@ -260,12 +260,19 @@ where
         expires_at: SystemTime,
         state: Option<IpState>,
     ) -> Result<IpAddr, IpError<T::Error>> {
+        const MAX_ATTEMPTS: usize = 2;
         let subnet = network.subnet().into();
         // unfortunately the sqlite connection is sometimes unreliable under high contention, meaning
         // we need to make a few attempts to get an address.
         let mut attempts = 0;
         loop {
             let ip_range = range.start().into()..=range.end().into();
+            if attempts > MAX_ATTEMPTS {
+                return Err(IpError::MaxAttempts {
+                    range: ip_range,
+                    attempts,
+                });
+            }
             // find the min expired IP or where id matches
             let ip = match self
                 .store
@@ -286,25 +293,19 @@ where
                     )
                     .await
                 {
-                    Ok(ip) => ip.ok_or(IpError::RangeError { range: ip_range })?,
+                    Ok(ip) => ip.ok_or(IpError::RangeError {
+                        range: ip_range.clone(),
+                    })?,
                     Err(err) => {
                         attempts += 1;
-                        if attempts <= 1 {
-                            warn!("error grabbing new IP-- retrying");
-                            continue;
-                        } else {
-                            return Err(IpError::DbError(err));
-                        }
+                        warn!(?err, "error grabbing new IP-- retrying");
+                        continue;
                     }
                 },
                 Err(err) => {
                     attempts += 1;
-                    if attempts <= 1 {
-                        warn!("error grabbing next expired IP-- retrying");
-                        continue;
-                    } else {
-                        return Err(IpError::DbError(err));
-                    }
+                    warn!(?err, "error grabbing next expired IP-- retrying");
+                    continue;
                 }
             };
             match ip {
@@ -327,6 +328,7 @@ where
                                     .update_ip(ip, IpState::Probate, None, probation_time)
                                     .await
                                 {
+                                    attempts += 1;
                                     error!(?err, "failed to probate IP on ping success");
                                     // not returning error because we must give client an IP
                                 } else {
@@ -336,11 +338,16 @@ where
                             }
                         }
                     } else {
-                        error!(
+                        attempts += 1;
+                        warn!(
                             ?range,
                             ?ipv4,
-                            "IP returned from leases table is outside of network range"
+                            "IP for client id returned from leases table is outside of network range"
                         );
+                        // entry for ip/id but the range doesn't match, remove the old entry
+                        if let Err(err) = self.store.release_ip(ip, id).await {
+                            error!(?err, "failed to delete entry");
+                        }
                         continue;
                     }
                 }
@@ -530,6 +537,11 @@ pub enum IpError<E> {
     AddrInUse(IpAddr),
     #[error("error getting next IP in range {range:?}")]
     RangeError { range: RangeInclusive<IpAddr> },
+    #[error("error getting next IP in range {range:?} inside attempts {attempts:?}")]
+    MaxAttempts {
+        range: RangeInclusive<IpAddr>,
+        attempts: usize,
+    },
 }
 
 #[cfg(test)]

plugins/leases/src/lib.rs

@@ -24,6 +24,7 @@ use dora_core::{
     dhcproto::v4::{DhcpOption, Message, MessageType, OptionCode},
     metrics,
     prelude::*,
+    tracing::warn,
 };
 use message_type::MatchedClasses;
 use register_derive::Register;
@@ -262,7 +263,7 @@ where
                 }
             }
         }
-        debug!("leases plugin did not assign ip");
+        warn!("leases plugin did not assign ip, check configuration or try clearing leases table. submit bugs to: github.com/bluecatengineering/dora");
         Ok(Action::NoResponse)
     }
 

plugins/message-type/src/lib.rs

@@ -140,7 +140,7 @@ impl Plugin<Message> for MsgType {
                     .insert(DhcpOption::MessageType(MessageType::Offer));
             }
             Some(MessageType::Request) => {
-                if !req.giaddr().is_unspecified() {
+                if req.giaddr().is_unspecified() {
                     resp.set_flags(req.flags().set_broadcast());
                 }
                 resp.opts_mut()