From a07f0593ab2ffd35b6e577aa393162e8b2202e2d Mon Sep 17 00:00:00 2001 From: Evan Cameron Date: Wed, 11 Oct 2023 16:50:33 -0400 Subject: [PATCH] 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 | 2 ++ dora-core/src/server/context.rs | 33 +++++++++++++++++++------- libs/ip-manager/src/lib.rs | 42 +++++++++++++++++++++------------ plugins/leases/src/lib.rs | 3 ++- plugins/message-type/src/lib.rs | 2 +- 5 files changed, 57 insertions(+), 25 deletions(-) diff --git a/Dockerfile b/Dockerfile index d6b4ac3..d593b81 100644 --- a/Dockerfile +++ b/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; diff --git a/dora-core/src/server/context.rs b/dora-core/src/server/context.rs index a4cc726..cefa246 100644 --- a/dora-core/src/server/context.rs +++ b/dora-core/src/server/context.rs @@ -602,6 +602,7 @@ impl MsgContext { /// 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 { 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 diff --git a/libs/ip-manager/src/lib.rs b/libs/ip-manager/src/lib.rs index 0689462..8dac477 100644 --- a/libs/ip-manager/src/lib.rs +++ b/libs/ip-manager/src/lib.rs @@ -260,12 +260,19 @@ where expires_at: SystemTime, state: Option, ) -> Result> { + 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 { AddrInUse(IpAddr), #[error("error getting next IP in range {range:?}")] RangeError { range: RangeInclusive }, + #[error("error getting next IP in range {range:?} inside attempts {attempts:?}")] + MaxAttempts { + range: RangeInclusive, + attempts: usize, + }, } #[cfg(test)] diff --git a/plugins/leases/src/lib.rs b/plugins/leases/src/lib.rs index e2078e7..529cd87 100644 --- a/plugins/leases/src/lib.rs +++ b/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) } diff --git a/plugins/message-type/src/lib.rs b/plugins/message-type/src/lib.rs index 7ea6f75..81f03e6 100644 --- a/plugins/message-type/src/lib.rs +++ b/plugins/message-type/src/lib.rs @@ -140,7 +140,7 @@ impl Plugin 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()