diff --git a/ntp-proto/src/algorithm/kalman/combiner.rs b/ntp-proto/src/algorithm/kalman/combiner.rs index 5b41f7568..871a45d6d 100644 --- a/ntp-proto/src/algorithm/kalman/combiner.rs +++ b/ntp-proto/src/algorithm/kalman/combiner.rs @@ -41,7 +41,7 @@ pub(super) fn combine( selection.first().map(|first| { let mut estimate = first.state; if !algo_config.ignore_server_dispersion { - estimate = estimate.add_server_dispersion(first.source_uncertainty.to_seconds()) + estimate = estimate.add_server_dispersion(first.source_uncertainty.to_seconds()); } let mut used_sources = vec![(first.index, estimate.uncertainty.determinant())]; diff --git a/ntp-proto/src/algorithm/kalman/config.rs b/ntp-proto/src/algorithm/kalman/config.rs index a336b817b..7322feec5 100644 --- a/ntp-proto/src/algorithm/kalman/config.rs +++ b/ntp-proto/src/algorithm/kalman/config.rs @@ -27,7 +27,7 @@ pub struct AlgorithmConfig { #[serde(default = "default_poll_interval_low_weight")] pub poll_interval_low_weight: f64, /// Amount which a measurement contributes to the state, above - /// which we start decreasing the poll_interval interval. (weight, 0-1) + /// which we start decreasing the `poll_interval` interval. (weight, 0-1) #[serde(default = "default_poll_interval_high_weight")] pub poll_interval_high_weight: f64, /// Amount of hysteresis in changing the poll interval (count, 1+) diff --git a/ntp-proto/src/algorithm/kalman/mod.rs b/ntp-proto/src/algorithm/kalman/mod.rs index 690fa1ef4..37d5e5198 100644 --- a/ntp-proto/src/algorithm/kalman/mod.rs +++ b/ntp-proto/src/algorithm/kalman/mod.rs @@ -111,16 +111,17 @@ impl KalmanClockController KalmanClockController>>(), ); if let Some(combined) = combine(&selection, &self.algo_config) { @@ -237,8 +238,7 @@ impl KalmanClockController v) - .unwrap_or(false) + .is_some_and(|v| self.timedata.accumulated_steps > v) { error!("Unusually large clock step suggested, please manually verify system clock and reference clock state and restart if appropriate. If the clock is significantly wrong, you can use `ntp-ctl force-sync` to correct it."); #[cfg(not(test))] @@ -315,10 +315,11 @@ impl KalmanClockController( synchronization_config: &SynchronizationConfig, algo_config: &AlgorithmConfig, - candidates: Vec>, + candidates: &[SourceSnapshot], ) -> Vec> { let mut bounds: Vec<(f64, BoundType)> = Vec::with_capacity(2 * candidates.len()); - for snapshot in candidates.iter() { + for snapshot in candidates { let radius = snapshot.offset_uncertainty() * algo_config.range_statistical_weight + snapshot.delay * algo_config.range_delay_weight; if radius > algo_config.maximum_source_uncertainty @@ -42,7 +42,7 @@ pub(super) fn select( let mut maxt: f64 = 0.0; let mut cur: usize = 0; - for (time, boundtype) in bounds.iter() { + for (time, boundtype) in &bounds { match boundtype { BoundType::Start => cur += 1, BoundType::End => cur -= 1, @@ -64,7 +64,7 @@ pub(super) fn select( && snapshot.offset() + radius >= maxt && snapshot.leap_indicator.is_synchronized() }) - .cloned() + .copied() .collect() } else { vec![] @@ -125,7 +125,7 @@ mod tests { ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates.clone()); + let result = select(&sysconfig, &algconfig, &candidates.clone()); assert_eq!(result.len(), 0); let algconfig = AlgorithmConfig { @@ -134,7 +134,7 @@ mod tests { range_delay_weight: 1.0, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates.clone()); + let result = select(&sysconfig, &algconfig, &candidates.clone()); assert_eq!(result.len(), 0); let algconfig = AlgorithmConfig { @@ -143,7 +143,7 @@ mod tests { range_delay_weight: 1.0, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates); + let result = select(&sysconfig, &algconfig, &candidates); assert_eq!(result.len(), 4); } @@ -166,7 +166,7 @@ mod tests { range_delay_weight: 1.0, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates.clone()); + let result = select(&sysconfig, &algconfig, &candidates.clone()); assert_eq!(result.len(), 3); let algconfig = AlgorithmConfig { @@ -175,7 +175,7 @@ mod tests { range_delay_weight: 1.0, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates.clone()); + let result = select(&sysconfig, &algconfig, &candidates.clone()); assert_eq!(result.len(), 2); let algconfig = AlgorithmConfig { @@ -184,7 +184,7 @@ mod tests { range_delay_weight: 1.0, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates.clone()); + let result = select(&sysconfig, &algconfig, &candidates.clone()); assert_eq!(result.len(), 1); let algconfig = AlgorithmConfig { @@ -193,7 +193,7 @@ mod tests { range_delay_weight: 1.0, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates); + let result = select(&sysconfig, &algconfig, &candidates); assert_eq!(result.len(), 0); } @@ -218,14 +218,14 @@ mod tests { minimum_agreeing_sources: 3, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates.clone()); + let result = select(&sysconfig, &algconfig, &candidates.clone()); assert_eq!(result.len(), 3); let sysconfig = SynchronizationConfig { minimum_agreeing_sources: 4, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates); + let result = select(&sysconfig, &algconfig, &candidates); assert_eq!(result.len(), 0); } @@ -248,7 +248,7 @@ mod tests { minimum_agreeing_sources: 1, ..Default::default() }; - let result = select(&sysconfig, &algconfig, candidates); + let result = select(&sysconfig, &algconfig, &candidates); assert_eq!(result.len(), 0); } } diff --git a/ntp-proto/src/algorithm/kalman/source.rs b/ntp-proto/src/algorithm/kalman/source.rs index f93c28358..d5f137e62 100644 --- a/ntp-proto/src/algorithm/kalman/source.rs +++ b/ntp-proto/src/algorithm/kalman/source.rs @@ -31,7 +31,7 @@ /// unit of time. /// /// This modules input consists of measurements containing: -/// - the time of the measurement t_m +/// - the time of the measurement `t_m` /// - the measured offset d /// - the measured transmission delay r /// @@ -43,7 +43,7 @@ /// For this, a further piece of information is needed: a measurement /// related to the frequency difference. Although mathematically not /// entirely sound, we construct the frequency measurement also using -/// the previous measurement (which we will denote with t_p and D_p). +/// the previous measurement (which we will denote with `t_p` and `D_p`). /// It turns out this works well in practice /// /// The observation is then the vector (D, D-D_p), and the observation @@ -242,14 +242,14 @@ pub struct AveragingBuffer { const INITIALIZATION_FREQ_UNCERTAINTY: f64 = 100.0; /// Approximation of 1 - the chi-squared cdf with 1 degree of freedom -/// source: https://en.wikipedia.org/wiki/Error_function +/// source: fn chi_1(chi: f64) -> f64 { - const P: f64 = 0.3275911; - const A1: f64 = 0.254829592; - const A2: f64 = -0.284496736; - const A3: f64 = 1.421413741; - const A4: f64 = -1.453152027; - const A5: f64 = 1.061405429; + const P: f64 = 0.327_591_1; + const A1: f64 = 0.254_829_592; + const A2: f64 = -0.284_496_736; + const A3: f64 = 1.421_413_741; + const A4: f64 = -1.453_152_027; + const A5: f64 = 1.061_405_429; let x = (chi / 2.).sqrt(); let t = 1. / (1. + P * x); @@ -258,10 +258,12 @@ fn chi_1(chi: f64) -> f64 { } impl AveragingBuffer { + #[allow(clippy::cast_precision_loss)] fn mean(&self) -> f64 { self.data.iter().sum::() / (self.data.len() as f64) } + #[allow(clippy::cast_precision_loss)] fn variance(&self) -> f64 { let mean = self.mean(); self.data.iter().map(|v| sqr(v - mean)).sum::() / ((self.data.len() - 1) as f64) @@ -283,7 +285,7 @@ pub trait MeasurementNoiseEstimator { fn reset(&mut self) -> Self; // for SourceSnapshot - fn get_max_roundtrip(&self, samples: &i32) -> Option; + fn get_max_roundtrip(&self, samples: i32) -> Option; fn get_delay_mean(&self) -> f64; } @@ -291,7 +293,7 @@ impl MeasurementNoiseEstimator for AveragingBuffer { type MeasurementDelay = NtpDuration; fn update(&mut self, delay: Self::MeasurementDelay) { - self.update(delay.to_seconds()) + self.update(delay.to_seconds()); } fn get_noise_estimate(&self) -> f64 { @@ -310,8 +312,9 @@ impl MeasurementNoiseEstimator for AveragingBuffer { AveragingBuffer::default() } - fn get_max_roundtrip(&self, samples: &i32) -> Option { - self.data[..*samples as usize] + #[allow(clippy::cast_sign_loss)] + fn get_max_roundtrip(&self, samples: i32) -> Option { + self.data[..samples as usize] .iter() .copied() .fold(None, |v1, v2| { @@ -349,7 +352,7 @@ impl MeasurementNoiseEstimator for f64 { *self } - fn get_max_roundtrip(&self, _samples: &i32) -> Option { + fn get_max_roundtrip(&self, _samples: i32) -> Option { Some(1.) } @@ -382,7 +385,7 @@ impl } fn process_offset_steering(&mut self, steer: f64) { - for sample in self.init_offset.data.iter_mut() { + for sample in &mut self.init_offset.data { *sample -= steer; } } @@ -447,7 +450,7 @@ impl /// not so much that each individual poll message gives us very little new information. fn update_desired_poll( &mut self, - source_defaults_config: &SourceDefaultsConfig, + source_defaults_config: SourceDefaultsConfig, algo_config: &AlgorithmConfig, p: f64, weight: f64, @@ -525,7 +528,7 @@ impl /// Update our estimates based on a new measurement. fn update( &mut self, - source_defaults_config: &SourceDefaultsConfig, + source_defaults_config: SourceDefaultsConfig, algo_config: &AlgorithmConfig, measurement: Measurement, ) -> bool { @@ -632,7 +635,7 @@ impl // Returns whether the clock may need adjusting. pub fn update_self_using_measurement( &mut self, - source_defaults_config: &SourceDefaultsConfig, + source_defaults_config: SourceDefaultsConfig, algo_config: &AlgorithmConfig, mut measurement: Measurement, ) -> bool { @@ -648,7 +651,7 @@ impl fn update_self_using_raw_measurement( &mut self, - source_defaults_config: &SourceDefaultsConfig, + source_defaults_config: SourceDefaultsConfig, algo_config: &AlgorithmConfig, measurement: Measurement, ) -> bool { @@ -715,6 +718,7 @@ impl index: Index, config: &AlgorithmConfig, ) -> Option> { + #[allow(clippy::cast_sign_loss)] match &self.0 { SourceStateInner::Initial(InitialSourceFilter { noise_estimator, @@ -722,7 +726,7 @@ impl last_measurement: Some(last_measurement), samples, }) if *samples > 0 => { - let max_roundtrip = noise_estimator.get_max_roundtrip(samples)?; + let max_roundtrip = noise_estimator.get_max_roundtrip(*samples)?; Some(SourceSnapshot { index, source_uncertainty: last_measurement.root_dispersion, @@ -736,7 +740,7 @@ impl .iter() .copied() .sum::() - / (*samples as f64), + / f64::from(*samples), 0.0, ]), uncertainty: Matrix::new([ @@ -758,11 +762,11 @@ impl leap_indicator: filter.last_measurement.leap, last_update: filter.last_iter, }), - _ => None, + SourceStateInner::Initial(_) => None, } } - pub fn get_desired_poll(&self, limits: &PollIntervalLimits) -> PollInterval { + pub fn get_desired_poll(&self, limits: PollIntervalLimits) -> PollInterval { match &self.0 { SourceStateInner::Initial(_) => limits.min, SourceStateInner::Stable(filter) => filter.desired_poll_interval, @@ -838,7 +842,7 @@ impl< self.state.process_offset_steering(steer); } super::KalmanControllerMessageInner::FreqChange { steer, time } => { - self.state.process_frequency_steering(time, steer) + self.state.process_frequency_steering(time, steer); } } } @@ -848,7 +852,7 @@ impl< measurement: Measurement, ) -> Option { if self.state.update_self_using_measurement( - &self.source_defaults_config, + self.source_defaults_config, &self.algo_config, measurement, ) { @@ -862,24 +866,25 @@ impl< fn desired_poll_interval(&self) -> PollInterval { self.state - .get_desired_poll(&self.source_defaults_config.poll_interval_limits) + .get_desired_poll(self.source_defaults_config.poll_interval_limits) } fn observe(&self) -> super::super::ObservableSourceTimedata { - self.state - .snapshot(&self.index, &self.algo_config) - .map(|snapshot| snapshot.observe()) - .unwrap_or(ObservableSourceTimedata { + self.state.snapshot(&self.index, &self.algo_config).map_or( + ObservableSourceTimedata { offset: NtpDuration::ZERO, uncertainty: NtpDuration::MAX, delay: NtpDuration::MAX, remote_delay: NtpDuration::MAX, remote_uncertainty: NtpDuration::MAX, last_update: NtpTimestamp::default(), - }) + }, + |snapshot| snapshot.observe(), + ) } } +#[allow(clippy::too_many_lines)] #[cfg(test)] mod tests { use crate::{packet::NtpLeapIndicator, time_types::NtpInstant}; @@ -921,7 +926,7 @@ mod tests { last_iter: base, })); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -969,7 +974,7 @@ mod tests { })); source.process_offset_steering(-1800.0); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1017,7 +1022,7 @@ mod tests { })); source.process_offset_steering(1800.0); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1039,7 +1044,7 @@ mod tests { D: Debug + Clone + Copy, N: MeasurementNoiseEstimator + Clone, >( - noise_estimator: N, + noise_estimator: &N, delay: D, ) { let base = NtpTimestamp::from_fixed_int(0); @@ -1121,7 +1126,7 @@ mod tests { ); source.update_self_using_raw_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1197,7 +1202,7 @@ mod tests { ); source.update_self_using_raw_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1238,7 +1243,7 @@ mod tests { #[test] fn test_offset_steering_and_measurements_normal() { test_offset_steering_and_measurements( - AveragingBuffer { + &AveragingBuffer { data: [0.0, 0.0, 0.0, 0.0, 0.875e-6, 0.875e-6, 0.875e-6, 0.875e-6], next_idx: 0, }, @@ -1248,7 +1253,7 @@ mod tests { #[test] fn test_offset_steering_and_measurements_constant_noise_estimate() { - test_offset_steering_and_measurements(1e-9, ()); + test_offset_steering_and_measurements(&1e-9, ()); } #[test] @@ -1380,7 +1385,7 @@ mod tests { .snapshot(0_usize, &AlgorithmConfig::default()) .is_none()); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1404,7 +1409,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1428,7 +1433,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1452,7 +1457,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1476,7 +1481,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1500,7 +1505,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1524,7 +1529,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1548,7 +1553,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay, @@ -1609,7 +1614,7 @@ mod tests { .snapshot(0_usize, &AlgorithmConfig::default()) .is_none()); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1633,7 +1638,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1657,7 +1662,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1681,7 +1686,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1706,7 +1711,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1730,7 +1735,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1754,7 +1759,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1778,7 +1783,7 @@ mod tests { > 1.0 ); source.update_self_using_measurement( - &SourceDefaultsConfig::default(), + SourceDefaultsConfig::default(), &AlgorithmConfig::default(), Measurement { delay: NtpDuration::from_seconds(0.0), @@ -1819,7 +1824,7 @@ mod tests { let config = SourceDefaultsConfig::default(); let algo_config = AlgorithmConfig { poll_interval_hysteresis: 2, - ..Default::default() + ..AlgorithmConfig::default() }; let base = NtpTimestamp::from_fixed_int(0); @@ -1858,59 +1863,59 @@ mod tests { let pollup = source .desired_poll_interval .inc(PollIntervalLimits::default()); - source.update_desired_poll(&config, &algo_config, 1.0, 1.0, baseinterval * 2.); + source.update_desired_poll(config, &algo_config, 1.0, 1.0, baseinterval * 2.); assert_eq!(source.poll_score, 0); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); - source.update_desired_poll(&config, &algo_config, 1.0, 0.0, baseinterval * 2.); + source.update_desired_poll(config, &algo_config, 1.0, 0.0, baseinterval * 2.); assert_eq!(source.poll_score, -1); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); - source.update_desired_poll(&config, &algo_config, 1.0, 0.0, baseinterval * 2.); + source.update_desired_poll(config, &algo_config, 1.0, 0.0, baseinterval * 2.); assert_eq!(source.poll_score, 0); assert_eq!(source.desired_poll_interval, pollup); - source.update_desired_poll(&config, &algo_config, 1.0, 1.0, baseinterval * 3.); + source.update_desired_poll(config, &algo_config, 1.0, 1.0, baseinterval * 3.); assert_eq!(source.poll_score, 0); assert_eq!(source.desired_poll_interval, pollup); - source.update_desired_poll(&config, &algo_config, 1.0, 0.0, baseinterval); + source.update_desired_poll(config, &algo_config, 1.0, 0.0, baseinterval); assert_eq!(source.poll_score, 0); assert_eq!(source.desired_poll_interval, pollup); - source.update_desired_poll(&config, &algo_config, 0.0, 0.0, baseinterval * 3.); + source.update_desired_poll(config, &algo_config, 0.0, 0.0, baseinterval * 3.); assert_eq!(source.poll_score, 0); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); - source.update_desired_poll(&config, &algo_config, 1.0, 0.0, baseinterval * 2.); + source.update_desired_poll(config, &algo_config, 1.0, 0.0, baseinterval * 2.); assert_eq!(source.poll_score, -1); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); - source.update_desired_poll(&config, &algo_config, 1.0, 0.0, baseinterval * 2.); + source.update_desired_poll(config, &algo_config, 1.0, 0.0, baseinterval * 2.); assert_eq!(source.poll_score, 0); assert_eq!(source.desired_poll_interval, pollup); - source.update_desired_poll(&config, &algo_config, 1.0, 1.0, baseinterval); + source.update_desired_poll(config, &algo_config, 1.0, 1.0, baseinterval); assert_eq!(source.poll_score, 1); assert_eq!(source.desired_poll_interval, pollup); - source.update_desired_poll(&config, &algo_config, 1.0, 1.0, baseinterval); + source.update_desired_poll(config, &algo_config, 1.0, 1.0, baseinterval); assert_eq!(source.poll_score, 0); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); - source.update_desired_poll(&config, &algo_config, 1.0, 0.0, baseinterval); + source.update_desired_poll(config, &algo_config, 1.0, 0.0, baseinterval); assert_eq!(source.poll_score, -1); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); source.update_desired_poll( - &config, + config, &algo_config, 1.0, (algo_config.poll_interval_high_weight + algo_config.poll_interval_low_weight) / 2., @@ -1921,14 +1926,14 @@ mod tests { source.desired_poll_interval, PollIntervalLimits::default().min ); - source.update_desired_poll(&config, &algo_config, 1.0, 1.0, baseinterval); + source.update_desired_poll(config, &algo_config, 1.0, 1.0, baseinterval); assert_eq!(source.poll_score, 1); assert_eq!( source.desired_poll_interval, PollIntervalLimits::default().min ); source.update_desired_poll( - &config, + config, &algo_config, 1.0, (algo_config.poll_interval_high_weight + algo_config.poll_interval_low_weight) / 2., @@ -1945,7 +1950,7 @@ mod tests { fn test_wander_estimation() { let algo_config = AlgorithmConfig { precision_hysteresis: 2, - ..Default::default() + ..AlgorithmConfig::default() }; let base = NtpTimestamp::from_fixed_int(0); diff --git a/ntp-proto/src/algorithm/mod.rs b/ntp-proto/src/algorithm/mod.rs index 0af89e098..8e56ddb8e 100644 --- a/ntp-proto/src/algorithm/mod.rs +++ b/ntp-proto/src/algorithm/mod.rs @@ -67,6 +67,9 @@ pub trait TimeSyncController: Sized + Send + 'static { >; /// Create a new clock controller controlling the given clock + /// # Errors + /// + /// Returns `Error` if Controller can't be made. fn new( clock: Self::Clock, synchronization_config: SynchronizationConfig, @@ -75,6 +78,9 @@ pub trait TimeSyncController: Sized + Send + 'static { ) -> Result::Error>; /// Take control of the clock (should not be done in new!) + /// # Errors + /// + /// Returns `Error` if clock can't be controlled. fn take_control(&mut self) -> Result<(), ::Error>; /// Create a new source with given identity @@ -91,14 +97,14 @@ pub trait TimeSyncController: Sized + Send + 'static { /// or not it is usable for synchronization) has changed. fn source_update(&mut self, id: Self::SourceId, usable: bool); /// Notify the controller of a new measurement from a source. - /// The list of SourceIds is used for loop detection, with the - /// first SourceId given considered the primary source used. + /// The list of `SourceIds` is used for loop detection, with the + /// first `SourceId` given considered the primary source used. fn source_message( &mut self, id: Self::SourceId, message: Self::SourceMessage, ) -> StateUpdate; - /// Non-message driven update (queued via next_update) + /// Non-message driven update (queued via `next_update`) fn time_update(&mut self) -> StateUpdate; } diff --git a/ntp-proto/src/clock.rs b/ntp-proto/src/clock.rs index 1e9c76b15..f0e246333 100644 --- a/ntp-proto/src/clock.rs +++ b/ntp-proto/src/clock.rs @@ -9,29 +9,53 @@ use crate::{ pub trait NtpClock: Clone + Send + 'static { type Error: std::error::Error + Send + Sync; - // Get current time + /// Get current time + /// + /// # Errors + /// + /// Returns `Self::Error` if the current time can't be received. fn now(&self) -> Result; - // Change the frequency of the clock, returning the time - // at which the change was applied. + /// Change the frequency of the clock, returning the time + /// at which the change was applied. + /// + /// # Errors + /// + /// Returns `Self::Error` if the frequency can't be changed. fn set_frequency(&self, freq: f64) -> Result; - // Get the frequency of the clock + /// Get the frequency of the clock + /// + /// # Errors + /// + /// Returns `Self::Error` if the frequency can't be received. fn get_frequency(&self) -> Result; - // Change the current time of the clock by offset. Returns - // the time at which the change was applied. + /// Change the current time of the clock by offset. Returns + /// the time at which the change was applied. + /// + /// # Errors + /// + /// Returns `Self::Error` if the current time can't be changed. fn step_clock(&self, offset: NtpDuration) -> Result; - // A clock can have a built in NTP clock discipline algorithm - // that does more processing on the offsets it receives. This - // functions disables that discipline. + /// A clock can have a built in NTP clock discipline algorithm + /// that does more processing on the offsets it receives. This + /// functions disables that discipline. + /// + /// # Errors + /// + /// Returns `Self::Error` if the disciple can't be disabled. fn disable_ntp_algorithm(&self) -> Result<(), Self::Error>; - // Provide the system with our current best estimates for - // the statistical error of the clock (est_error), and - // the maximum deviation due to frequency error and - // distance to the root clock. + /// Provide the system with our current best estimates for + /// the statistical error of the clock `est_error`, and + /// the maximum deviation due to frequency error and + /// distance to the root clock. + /// + /// # Errors + /// + /// Returns `Self::Error` if the estimate can't be updated. fn error_estimate_update( &self, est_error: NtpDuration, @@ -39,5 +63,9 @@ pub trait NtpClock: Clone + Send + 'static { ) -> Result<(), Self::Error>; // Change the indicators for upcoming leap seconds and // the clocks synchronization status. + /// + /// # Errors + /// + /// Returns `Self::Error` if the status can't be updated. fn status_update(&self, leap_status: NtpLeapIndicator) -> Result<(), Self::Error>; } diff --git a/ntp-proto/src/config.rs b/ntp-proto/src/config.rs index 0004f2b6e..f46e9c148 100644 --- a/ntp-proto/src/config.rs +++ b/ntp-proto/src/config.rs @@ -28,9 +28,9 @@ pub struct StepThreshold { } impl StepThreshold { + #[must_use] pub fn is_within(&self, duration: NtpDuration) -> bool { - self.forward.map(|v| duration < v).unwrap_or(true) - && self.backward.map(|v| duration > -v).unwrap_or(true) + self.forward.map_or(true, |v| duration < v) && self.backward.map_or(true, |v| duration > -v) } } @@ -62,6 +62,7 @@ impl<'de> Deserialize<'de> for ThresholdPart { where E: de::Error, { + #[allow(clippy::cast_precision_loss)] self.visit_f64(v as f64) } @@ -69,6 +70,7 @@ impl<'de> Deserialize<'de> for ThresholdPart { where E: de::Error, { + #[allow(clippy::cast_precision_loss)] self.visit_f64(v as f64) } @@ -129,6 +131,7 @@ impl<'de> Deserialize<'de> for StepThreshold { where E: de::Error, { + #[allow(clippy::cast_precision_loss)] self.visit_f64(v as f64) } @@ -136,6 +139,7 @@ impl<'de> Deserialize<'de> for StepThreshold { where E: de::Error, { + #[allow(clippy::cast_precision_loss)] self.visit_f64(v as f64) } @@ -210,7 +214,7 @@ pub struct SourceDefaultsConfig { impl Default for SourceDefaultsConfig { fn default() -> Self { Self { - poll_interval_limits: Default::default(), + poll_interval_limits: PollIntervalLimits::default(), initial_poll_interval: default_initial_poll_interval(), } } @@ -226,7 +230,7 @@ pub struct SynchronizationConfig { /// Minimum number of survivors needed to be able to discipline the system clock. /// More survivors (so more servers from which to get the time) means a more accurate time. /// - /// The spec notes (CMIN was renamed to MIN_INTERSECTION_SURVIVORS in our implementation): + /// The spec notes (CMIN was renamed to `MIN_INTERSECTION_SURVIVORS` in our implementation): /// /// > CMIN defines the minimum number of servers consistent with the correctness requirements. /// > Suspicious operators would set CMIN to ensure multiple redundant servers are available for the @@ -240,7 +244,7 @@ pub struct SynchronizationConfig { /// remote servers from causing us to drift too far. /// /// Note that this is not used during startup. To limit system clock changes - /// during startup, use startup_panic_threshold + /// during startup, use `startup_panic_threshold` #[serde(default = "default_single_step_panic_threshold")] pub single_step_panic_threshold: StepThreshold, diff --git a/ntp-proto/src/cookiestash.rs b/ntp-proto/src/cookiestash.rs index 6f5c59545..c7b5f77ea 100644 --- a/ntp-proto/src/cookiestash.rs +++ b/ntp-proto/src/cookiestash.rs @@ -53,6 +53,7 @@ impl CookieStash { } /// Number of cookies missing from the stash + #[allow(clippy::cast_possible_truncation)] pub fn gap(&self) -> u8 { // This never overflows or underflows since cookies.len will // fit in a u8 and 0 <= self.valid <= self.cookies.len() diff --git a/ntp-proto/src/identifiers.rs b/ntp-proto/src/identifiers.rs index 6960bb367..6896f1983 100644 --- a/ntp-proto/src/identifiers.rs +++ b/ntp-proto/src/identifiers.rs @@ -17,6 +17,10 @@ impl ReferenceId { // Network Time Security (NTS) negative-acknowledgment (NAK), from rfc8915 pub const KISS_NTSN: ReferenceId = ReferenceId(u32::from_be_bytes(*b"NTSN")); + /// # Panics + /// + /// Will panic if `addr` is not a valid ip address. + #[must_use] pub fn from_ip(addr: IpAddr) -> ReferenceId { match addr { IpAddr::V4(addr) => ReferenceId(u32::from_be_bytes(addr.octets())), @@ -30,20 +34,20 @@ impl ReferenceId { ReferenceId(value) } - pub(crate) fn is_deny(&self) -> bool { - *self == Self::KISS_DENY + pub(crate) fn is_deny(self) -> bool { + self == Self::KISS_DENY } - pub(crate) fn is_rate(&self) -> bool { - *self == Self::KISS_RATE + pub(crate) fn is_rate(self) -> bool { + self == Self::KISS_RATE } - pub(crate) fn is_rstr(&self) -> bool { - *self == Self::KISS_RSTR + pub(crate) fn is_rstr(self) -> bool { + self == Self::KISS_RSTR } - pub(crate) fn is_ntsn(&self) -> bool { - *self == Self::KISS_NTSN + pub(crate) fn is_ntsn(self) -> bool { + self == Self::KISS_NTSN } pub(crate) fn to_bytes(self) -> [u8; 4] { diff --git a/ntp-proto/src/io.rs b/ntp-proto/src/io.rs index f67decf51..87241633f 100644 --- a/ntp-proto/src/io.rs +++ b/ntp-proto/src/io.rs @@ -1,4 +1,4 @@ -/// Write trait for structs that implement std::io::Write without doing blocking io +/// Write trait for structs that implement `std::io::Write` without doing blocking io pub trait NonBlockingWrite: std::io::Write {} impl NonBlockingWrite for std::io::Cursor where std::io::Cursor: std::io::Write {} diff --git a/ntp-proto/src/ipfilter.rs b/ntp-proto/src/ipfilter.rs index ea3385b7d..673703cdd 100644 --- a/ntp-proto/src/ipfilter.rs +++ b/ntp-proto/src/ipfilter.rs @@ -75,7 +75,7 @@ impl BitTree { *val = apply_mask(*val, *len); } // Ensure values are sorted by value and then by length - data.sort(); + data.sort_unstable(); let mut result = BitTree { nodes: vec![TreeNode::default()], @@ -87,6 +87,8 @@ impl BitTree { /// Create the substructure for a node, recursively. /// Max recursion depth is maximum value of data[i].1/4 /// for any i + #[allow(clippy::cast_possible_truncation)] + #[allow(clippy::cast_sign_loss)] fn fill_node(&mut self, mut data: &mut [(u128, u8)], node_index: usize) { // distribute the data into 16 4-bit buckets let mut counts = [0; 16]; @@ -183,7 +185,7 @@ impl IpFilter { for subnet in subnets { match subnet.addr { IpAddr::V4(addr) => ipv4list.push(( - (u32::from_be_bytes(addr.octets()) as u128) << 96, + u128::from(u32::from_be_bytes(addr.octets())) << 96, subnet.mask, )), IpAddr::V6(addr) => { @@ -202,40 +204,43 @@ impl IpFilter { /// Complexity: O(1) pub fn is_in(&self, addr: &IpAddr) -> bool { match addr { - IpAddr::V4(addr) => self.is_in4(addr), - IpAddr::V6(addr) => self.is_in6(addr), + IpAddr::V4(addr) => self.is_in4(*addr), + IpAddr::V6(addr) => self.is_in6(*addr), } } - fn is_in4(&self, addr: &Ipv4Addr) -> bool { + /// # Panics + /// + /// Panics if `addr` has invalid octets. + fn is_in4(&self, addr: Ipv4Addr) -> bool { self.ipv4_filter - .lookup((u32::from_be_bytes(addr.octets()) as u128) << 96) + .lookup(u128::from(u32::from_be_bytes(addr.octets())) << 96) } - fn is_in6(&self, addr: &Ipv6Addr) -> bool { + fn is_in6(&self, addr: Ipv6Addr) -> bool { self.ipv6_filter.lookup(u128::from_be_bytes(addr.octets())) } } #[cfg(feature = "__internal-fuzz")] pub mod fuzz { - use super::*; + use super::{IpAddr, IpFilter, IpSubnet}; fn contains(subnet: &IpSubnet, addr: &IpAddr) -> bool { match (subnet.addr, addr) { (IpAddr::V4(net), IpAddr::V4(addr)) => { let net = u32::from_be_bytes(net.octets()); let addr = u32::from_be_bytes(addr.octets()); - let mask = 0xFFFFFFFF_u32 - .checked_shl((32 - subnet.mask) as u32) + let mask = 0xFFFF_FFFF_u32 + .checked_shl(u32::from(32 - subnet.mask)) .unwrap_or(0); (net & mask) == (addr & mask) } (IpAddr::V6(net), IpAddr::V6(addr)) => { let net = u128::from_be_bytes(net.octets()); let addr = u128::from_be_bytes(addr.octets()); - let mask = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF_u128 - .checked_shl((128 - subnet.mask) as u32) + let mask = 0xFFFF_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF_FFFF_u128 + .checked_shl(u32::from(128 - subnet.mask)) .unwrap_or(0); (net & mask) == (addr & mask) } @@ -252,6 +257,7 @@ pub mod fuzz { false } + #[allow(clippy::missing_panics_doc)] pub fn fuzz_ipfilter(nets: &[IpSubnet], addr: &[IpAddr]) { let filter = IpFilter::new(nets); diff --git a/ntp-proto/src/keyset.rs b/ntp-proto/src/keyset.rs index 333995d4a..a608e0511 100644 --- a/ntp-proto/src/keyset.rs +++ b/ntp-proto/src/keyset.rs @@ -36,7 +36,7 @@ impl std::fmt::Debug for DecodedServerCookie { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("DecodedServerCookie") .field("algorithm", &self.algorithm) - .finish() + .finish_non_exhaustive() } } @@ -50,6 +50,7 @@ impl KeySetProvider { /// Create a new keysetprovider that keeps history old /// keys around (so in total, history+1 keys are valid /// at any time) + #[must_use] pub fn new(history: usize) -> Self { KeySetProvider { current: Arc::new(KeySet { @@ -64,6 +65,8 @@ impl KeySetProvider { } #[cfg(feature = "__internal-fuzz")] + #[allow(clippy::cast_possible_truncation)] + #[must_use] pub fn dangerous_new_deterministic(history: usize) -> Self { KeySetProvider { current: Arc::new(KeySet { @@ -78,12 +81,12 @@ impl KeySetProvider { } /// Rotate a new key in as primary, forgetting an old one if needed + #[allow(clippy::cast_possible_truncation)] pub fn rotate(&mut self) { let next_key = AesSivCmac512::new(aes_siv::Aes256SivAead::generate_key(rand::thread_rng())); let mut keys = Vec::with_capacity((self.history + 1).min(self.current.keys.len() + 1)); - for key in self.current.keys + for key in &self.current.keys [self.current.keys.len().saturating_sub(self.history)..self.current.keys.len()] - .iter() { // This is the rare case where we do really want to make a copy. keys.push(AesSivCmac512::new(GenericArray::clone_from_slice( @@ -101,20 +104,33 @@ impl KeySetProvider { }); } + /// # Panics + /// + /// Panics if `buf` can't be converted to system time. + /// + /// # Errors + /// + /// Errors if `len` is bigger or equal to `primary`. pub fn load( reader: &mut impl Read, history: usize, ) -> std::io::Result<(Self, std::time::SystemTime)> { let mut buf = [0; 64]; reader.read_exact(&mut buf[0..20])?; + let time = std::time::SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(u64::from_be_bytes(buf[0..8].try_into().unwrap())); let id_offset = u32::from_be_bytes(buf[8..12].try_into().unwrap()); let primary = u32::from_be_bytes(buf[12..16].try_into().unwrap()); let len = u32::from_be_bytes(buf[16..20].try_into().unwrap()); - if primary > len { - return Err(std::io::ErrorKind::Other.into()); + + if primary >= len { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Primary key must be less than length", + )); } + let mut keys = vec![]; for _ in 0..len { reader.read_exact(&mut buf[0..64])?; @@ -133,6 +149,14 @@ impl KeySetProvider { )) } + /// # Panics + /// + /// Panics if we can't get the current time. + /// + /// # Errors + /// + /// Errors if we can't write to the sink. + #[allow(clippy::cast_possible_truncation)] pub fn store(&self, writer: &mut impl Write) -> std::io::Result<()> { let time = std::time::SystemTime::now() .duration_since(std::time::SystemTime::UNIX_EPOCH) @@ -141,13 +165,14 @@ impl KeySetProvider { writer.write_all(&self.current.id_offset.to_be_bytes())?; writer.write_all(&self.current.primary.to_be_bytes())?; writer.write_all(&(self.current.keys.len() as u32).to_be_bytes())?; - for key in self.current.keys.iter() { + for key in &self.current.keys { writer.write_all(key.key_bytes())?; } Ok(()) } - /// Get the current KeySet + /// Get the current `KeySet` + #[must_use] pub fn get(&self) -> Arc { self.current.clone() } @@ -161,10 +186,12 @@ pub struct KeySet { impl KeySet { #[cfg(feature = "__internal-fuzz")] + #[must_use] pub fn encode_cookie_pub(&self, cookie: &DecodedServerCookie) -> Vec { self.encode_cookie(cookie) } + #[allow(clippy::cast_possible_truncation)] pub(crate) fn encode_cookie(&self, cookie: &DecodedServerCookie) -> Vec { let mut output = cookie.plaintext(); let plaintext_length = output.as_slice().len(); @@ -192,6 +219,7 @@ impl KeySet { } #[cfg(feature = "__internal-fuzz")] + #[allow(clippy::missing_errors_doc)] pub fn decode_cookie_pub(&self, cookie: &[u8]) -> Result { self.decode_cookie(cookie) } @@ -292,6 +320,7 @@ impl std::fmt::Debug for KeySet { } #[cfg(any(test, feature = "__internal-fuzz"))] +#[must_use] pub fn test_cookie() -> DecodedServerCookie { DecodedServerCookie { algorithm: AeadAlgorithm::AeadAesSivCmac256, diff --git a/ntp-proto/src/lib.rs b/ntp-proto/src/lib.rs index fab02f281..f53c75f22 100644 --- a/ntp-proto/src/lib.rs +++ b/ntp-proto/src/lib.rs @@ -5,6 +5,9 @@ //! //! Please visit the [ntpd-rs](https://github.com/pendulum-project/ntpd-rs) project //! for more information. +#![deny(clippy::pedantic)] +#![allow(clippy::module_name_repetitions)] +#![allow(clippy::explicit_iter_loop)] // breaking on msrv #![forbid(unsafe_code)] #![cfg_attr(not(feature = "__internal-api"), allow(unused))] diff --git a/ntp-proto/src/nts_pool_ke.rs b/ntp-proto/src/nts_pool_ke.rs index 908048506..89b1fbe33 100644 --- a/ntp-proto/src/nts_pool_ke.rs +++ b/ntp-proto/src/nts_pool_ke.rs @@ -13,6 +13,7 @@ pub struct SupportedAlgorithmsDecoder { } impl SupportedAlgorithmsDecoder { + #[must_use] pub fn step_with_slice( mut self, bytes: &[u8], @@ -34,7 +35,7 @@ impl SupportedAlgorithmsDecoder { record: NtsRecord, ) -> ControlFlow, KeyExchangeError>, Self> { use ControlFlow::{Break, Continue}; - use NtsRecord::*; + use NtsRecord::{EndOfMessage, Error, SupportedAlgorithmList, Warning}; let mut state = self; @@ -87,6 +88,9 @@ pub struct ClientToPoolData { } impl ClientToPoolData { + /// # Errors + /// + /// Returns `KeyExchangeError` if extracting the `NtsKeys` fails. pub fn extract_nts_keys( &self, stream: &rustls23::ConnectionCommon, @@ -98,6 +102,7 @@ impl ClientToPoolData { } impl ClientToPoolDecoder { + #[must_use] pub fn step_with_slice( mut self, bytes: &[u8], @@ -120,8 +125,10 @@ impl ClientToPoolDecoder { ) -> ControlFlow, Self> { use self::AeadAlgorithm as Algorithm; use ControlFlow::{Break, Continue}; - use KeyExchangeError::*; - use NtsRecord::*; + use KeyExchangeError::{NoValidAlgorithm, NoValidProtocol}; + #[cfg(feature = "ntpv5")] + use NtsRecord::DraftId; + use NtsRecord::{AeadAlgorithm, EndOfMessage, Error, NextProtocol, NtpServerDeny, Warning}; let mut state = self; @@ -238,6 +245,7 @@ pub struct PoolToServerData { } impl PoolToServerDecoder { + #[must_use] pub fn step_with_slice( mut self, bytes: &[u8], @@ -260,8 +268,10 @@ impl PoolToServerDecoder { ) -> ControlFlow, Self> { use self::AeadAlgorithm as Algorithm; use ControlFlow::{Break, Continue}; - use KeyExchangeError::*; - use NtsRecord::*; + use KeyExchangeError::{NoValidAlgorithm, NoValidProtocol}; + #[cfg(feature = "ntpv5")] + use NtsRecord::DraftId; + use NtsRecord::{AeadAlgorithm, EndOfMessage, Error, NextProtocol, Warning}; let mut state = self; diff --git a/ntp-proto/src/nts_record.rs b/ntp-proto/src/nts_record.rs index b31b90cdc..ce0f4be36 100644 --- a/ntp-proto/src/nts_record.rs +++ b/ntp-proto/src/nts_record.rs @@ -1,3 +1,4 @@ +#![allow(clippy::cast_possible_truncation)] use std::{ fmt::Display, io::{Read, Write}, @@ -117,7 +118,7 @@ impl NtsRecord { } } NtsRecord::Server { name, .. } => { - if name.as_bytes().len() >= (u16::MAX as usize) { + if name.len() >= (u16::MAX as usize) { return Err(WriteError::TooLong); } } @@ -267,6 +268,7 @@ impl NtsRecord { } #[cfg(feature = "nts-pool")] + #[must_use] pub fn client_key_exchange_records_fixed( c2s: Vec, s2c: Vec, @@ -328,7 +330,7 @@ impl NtsRecord { .iter() .map(|&algo| (algo as u16, algo.key_size())) .collect(), - }) + }); } if let Some(ntp_port) = ntp_port { @@ -367,6 +369,9 @@ impl NtsRecord { response.into_boxed_slice() } + /// # Errors + /// + /// Errors if reading bytes fails. pub fn read(reader: &mut impl NonBlockingRead) -> std::io::Result { let raw_record_type = read_u16_be(reader)?; let critical = raw_record_type & 0x8000 != 0; @@ -460,6 +465,9 @@ impl NtsRecord { }) } + /// # Errors + /// + /// Errors if writing to sink fails. pub fn write(&self, mut writer: impl NonBlockingWrite) -> std::io::Result<()> { // error out early when the record is invalid if let Err(e) = self.validate() { @@ -467,7 +475,7 @@ impl NtsRecord { } // all messages start with the record type - let record_type = self.record_type() | ((self.is_critical() as u16) << 15); + let record_type = self.record_type() | (u16::from(self.is_critical()) << 15); writer.write_all(&record_type.to_be_bytes())?; let size_of_u16 = std::mem::size_of::() as u16; @@ -570,6 +578,7 @@ impl NtsRecord { Ok(()) } + #[must_use] pub fn decoder() -> NtsRecordDecoder { NtsRecordDecoder { bytes: vec![] } } @@ -578,12 +587,14 @@ impl NtsRecord { #[cfg(feature = "__internal-fuzz")] impl<'a> arbitrary::Arbitrary<'a> for NtsRecord { fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + use NtsRecord::{ + AeadAlgorithm, EndOfMessage, Error, NewCookie, NextProtocol, Port, Server, Warning, + }; let record = u16::arbitrary(u)?; let critical = record & 0x8000 != 0; let record_type = record & !0x8000; - use NtsRecord::*; Ok(match record_type { 0 => EndOfMessage, 1 => NextProtocol { @@ -638,6 +649,9 @@ impl NtsRecordDecoder { const HEADER_BYTES: usize = 4; /// Try to decode the next record. Returns None when there are not enough bytes + /// # Errors + /// + /// Errors if `NtsRecord` is invalid. pub fn step(&mut self) -> std::io::Result> { if self.bytes.len() < Self::HEADER_BYTES { return Ok(None); @@ -659,6 +673,7 @@ impl NtsRecordDecoder { } } + #[must_use] pub fn new() -> Self { Self::default() } @@ -705,8 +720,7 @@ impl Display for KeyExchangeError { ), Self::NoCookies => write!(f, "Missing cookies"), Self::Io(e) => write!(f, "{e}"), - Self::Tls(e) => write!(f, "{e}"), - Self::Certificate(e) => write!(f, "{e}"), + Self::Tls(e) | Self::Certificate(e) => write!(f, "{e}"), Self::DnsName(e) => write!(f, "{e}"), Self::IncompleteResponse => write!(f, "Incomplete response"), } @@ -743,14 +757,19 @@ impl KeyExchangeError { } } + #[must_use] pub fn to_error_code(&self) -> u16 { - use KeyExchangeError::*; + use KeyExchangeError::{ + BadRequest, BadResponse, Certificate, DnsName, IncompleteResponse, InternalServerError, + InvalidFixedKeyLength, Io, NoCookies, NoValidAlgorithm, NoValidProtocol, Tls, + UnknownErrorCode, UnrecognizedCriticalRecord, + }; match self { UnrecognizedCriticalRecord => NtsRecord::UNRECOGNIZED_CRITICAL_RECORD, - BadRequest => NtsRecord::BAD_REQUEST, InternalServerError | Io(_) => NtsRecord::INTERNAL_SERVER_ERROR, UnknownErrorCode(_) + | BadRequest | BadResponse | NoValidProtocol | NoValidAlgorithm @@ -764,7 +783,7 @@ impl KeyExchangeError { } } -/// From https://www.rfc-editor.org/rfc/rfc8915.html#name-network-time-security-next- +/// From - #[derive(Debug, PartialEq, Eq, Clone, Copy, Default)] #[repr(u16)] pub enum ProtocolId { @@ -799,7 +818,7 @@ impl ProtocolId { } } -/// From https://www.iana.org/assignments/aead-parameters/aead-parameters.xhtml +/// From #[derive(Debug, PartialEq, Eq, Clone, Copy, Default)] #[repr(u16)] pub enum AeadAlgorithm { @@ -810,6 +829,7 @@ pub enum AeadAlgorithm { impl AeadAlgorithm { // per https://www.rfc-editor.org/rfc/rfc8915.html#section-5.1 + #[must_use] pub const fn c2s_context(self, protocol: ProtocolId) -> [u8; 5] { // The final octet SHALL be 0x00 for the C2S key [ @@ -822,6 +842,7 @@ impl AeadAlgorithm { } // per https://www.rfc-editor.org/rfc/rfc8915.html#section-5.1 + #[must_use] pub const fn s2c_context(self, protocol: ProtocolId) -> [u8; 5] { // The final octet SHALL be 0x01 for the S2C key [ @@ -833,6 +854,7 @@ impl AeadAlgorithm { ] } + #[must_use] pub const fn try_deserialize(number: u16) -> Option { match number { 15 => Some(AeadAlgorithm::AeadAesSivCmac256), @@ -845,7 +867,7 @@ impl AeadAlgorithm { &[Self::AeadAesSivCmac512, Self::AeadAesSivCmac256]; pub(crate) fn extract_nts_keys( - &self, + self, protocol: ProtocolId, tls_connection: &tls_utils::ConnectionCommon, ) -> Result { @@ -872,7 +894,7 @@ impl AeadAlgorithm { } #[cfg(feature = "nts-pool")] - fn try_into_nts_keys(&self, RequestedKeys { c2s, s2c }: &RequestedKeys) -> Option { + fn try_into_nts_keys(self, RequestedKeys { c2s, s2c }: &RequestedKeys) -> Option { match self { AeadAlgorithm::AeadAesSivCmac256 => { let c2s = Box::new(AesSivCmac256::from_key_bytes(c2s).ok()?); @@ -890,7 +912,7 @@ impl AeadAlgorithm { } #[cfg(feature = "nts-pool")] - fn key_size(&self) -> u16 { + fn key_size(self) -> u16 { match self { AeadAlgorithm::AeadAesSivCmac256 => AesSivCmac256::key_size() as u16, AeadAlgorithm::AeadAesSivCmac512 => AesSivCmac512::key_size() as u16, @@ -964,6 +986,10 @@ pub struct KeyExchangeResultDecoder { } impl KeyExchangeResultDecoder { + fn new() -> Self { + Self::default() + } + pub fn step_with_slice( mut self, bytes: &[u8], @@ -979,168 +1005,195 @@ impl KeyExchangeResultDecoder { } } + #[allow(clippy::inline_always)] #[inline(always)] fn step_with_record( self, record: NtsRecord, ) -> ControlFlow, Self> { - use self::AeadAlgorithm as Algorithm; - use ControlFlow::{Break, Continue}; - use KeyExchangeError::*; - use NtsRecord::*; - - let mut state = self; - match record { - EndOfMessage => { - let Some(protocol) = state.protocol else { - return ControlFlow::Break(Err(KeyExchangeError::NoValidProtocol)); - }; - - // the spec notes - // - // > If the NTS Next Protocol Negotiation record offers Protocol ID 0 (for NTPv4), - // > then this record MUST be included exactly once. Other protocols MAY require it as well. - // - // but we only support Protocol ID 0 (and assume ntpv5 behaves like ntpv4 in this regard) - let Some(algorithm) = state.algorithm else { - return ControlFlow::Break(Err(KeyExchangeError::NoValidAlgorithm)); - }; - - if state.cookies.is_empty() { - Break(Err(KeyExchangeError::NoCookies)) - } else { - Break(Ok(PartialKeyExchangeData { - remote: state.remote, - port: state.port, - protocol, - algorithm, - cookies: state.cookies, - #[cfg(feature = "nts-pool")] - supported_algorithms: state.supported_algorithms, - })) - } - } + NtsRecord::EndOfMessage => self.handle_end_of_message(), #[cfg(feature = "ntpv5")] - DraftId { .. } => { - tracing::debug!("Unexpected draft id"); - Continue(state) - } - NewCookie { cookie_data } => { - state.cookies.store(cookie_data); - Continue(state) - } - Server { name, .. } => { - state.remote = Some(name); - Continue(state) - } - Port { port, .. } => { - state.port = Some(port); - Continue(state) - } - Error { errorcode } => { - // - Break(Err(KeyExchangeError::from_error_code(errorcode))) - } - Warning { warningcode } => { - tracing::warn!(warningcode, "Received key exchange warning code"); - - Continue(state) - } - NextProtocol { protocol_ids } => { - let selected = ProtocolId::IN_ORDER_OF_PREFERENCE - .iter() - .find_map(|proto| protocol_ids.contains(&(*proto as u16)).then_some(*proto)); - - match selected { - None => Break(Err(NoValidProtocol)), - Some(protocol) => { - // The NTS Next Protocol Negotiation record [..] MUST occur exactly once in every NTS-KE request and response. - match state.protocol { - None => { - state.protocol = Some(protocol); - Continue(state) - } - Some(_) => Break(Err(KeyExchangeError::BadResponse)), - } - } - } - } - AeadAlgorithm { algorithm_ids, .. } => { - // it MUST include at most one - let algorithm_id = match algorithm_ids[..] { - [] => return Break(Err(NoValidAlgorithm)), - [algorithm_id] => algorithm_id, - _ => return Break(Err(BadResponse)), - }; - - let selected = Algorithm::IN_ORDER_OF_PREFERENCE - .iter() - .find(|algo| (algorithm_id == (**algo as u16))); - - match selected { - None => Break(Err(NoValidAlgorithm)), - Some(algorithm) => { - // for the protocol ids we support, the AeadAlgorithm record must be present - match state.algorithm { - None => { - state.algorithm = Some(*algorithm); - Continue(state) - } - Some(_) => Break(Err(KeyExchangeError::BadResponse)), - } - } - } + NtsRecord::DraftId { .. } => self.handle_unexpected_draft_id(), + NtsRecord::NewCookie { cookie_data } => self.handle_new_cookie(cookie_data), + NtsRecord::Server { name, .. } => self.handle_server(name), + NtsRecord::Port { port, .. } => self.handle_port(port), + NtsRecord::Warning { warningcode } => self.handle_warning(warningcode), + NtsRecord::NextProtocol { protocol_ids } => self.handle_next_protocol(&protocol_ids), + NtsRecord::AeadAlgorithm { algorithm_ids, .. } => { + self.handle_aead_algorithm(&algorithm_ids) } - - Unknown { critical, .. } => { - if critical { - Break(Err(KeyExchangeError::UnrecognizedCriticalRecord)) - } else { - Continue(state) - } + NtsRecord::Unknown { critical, .. } => self.handle_unknown(critical), + NtsRecord::Error { errorcode } => { + ControlFlow::Break(Err(KeyExchangeError::from_error_code(errorcode))) } #[cfg(feature = "nts-pool")] - KeepAlive => { - state.keep_alive = true; - Continue(state) - } + NtsRecord::KeepAlive => self.handle_keep_alive(), #[cfg(feature = "nts-pool")] - SupportedAlgorithmList { + NtsRecord::SupportedAlgorithmList { supported_algorithms, - } => { - use self::AeadAlgorithm; - - state.supported_algorithms = Some( - supported_algorithms - .into_iter() - .filter_map(|(aead_protocol_id, key_length)| { - let aead_algorithm = AeadAlgorithm::try_deserialize(aead_protocol_id)?; - Some((aead_algorithm, key_length)) - }) - .collect::>() - .into_boxed_slice(), - ); - - Continue(state) - } + } => self.handle_supported_algorithm_list(supported_algorithms), #[cfg(feature = "nts-pool")] - FixedKeyRequest { .. } => { - // a client should never receive a FixedKeyRequest + NtsRecord::FixedKeyRequest { .. } => { tracing::warn!("Unexpected fixed key request"); - Continue(state) + ControlFlow::Continue(self) } #[cfg(feature = "nts-pool")] - NtpServerDeny { .. } => { - // a client should never receive a NtpServerDeny - tracing::warn!("Unexpected ntp server deny"); - Continue(state) + NtsRecord::NtpServerDeny { .. } => { + tracing::warn!("Unexpected NTP server deny"); + ControlFlow::Continue(self) } } } - fn new() -> Self { - Self::default() + fn handle_end_of_message( + self, + ) -> ControlFlow, Self> { + let Some(protocol) = self.protocol else { + return ControlFlow::Break(Err(KeyExchangeError::NoValidProtocol)); + }; + + let Some(algorithm) = self.algorithm else { + return ControlFlow::Break(Err(KeyExchangeError::NoValidAlgorithm)); + }; + + if self.cookies.is_empty() { + ControlFlow::Break(Err(KeyExchangeError::NoCookies)) + } else { + ControlFlow::Break(Ok(PartialKeyExchangeData { + remote: self.remote.clone(), + port: self.port, + protocol, + algorithm, + cookies: self.cookies, + #[cfg(feature = "nts-pool")] + supported_algorithms: self.supported_algorithms.clone(), + })) + } + } + + #[cfg(feature = "ntpv5")] + fn handle_unexpected_draft_id( + self, + ) -> ControlFlow, Self> { + tracing::debug!("Unexpected draft id"); + ControlFlow::Continue(self) + } + + fn handle_new_cookie( + mut self, + cookie_data: Vec, + ) -> ControlFlow, Self> { + self.cookies.store(cookie_data); + ControlFlow::Continue(self) + } + + fn handle_server( + mut self, + name: String, + ) -> ControlFlow, Self> { + self.remote = Some(name); + ControlFlow::Continue(self) + } + + fn handle_port( + mut self, + port: u16, + ) -> ControlFlow, Self> { + self.port = Some(port); + ControlFlow::Continue(self) + } + + fn handle_warning( + self, + warningcode: u16, + ) -> ControlFlow, Self> { + tracing::warn!(warningcode, "Received key exchange warning code"); + ControlFlow::Continue(self) + } + + fn handle_next_protocol( + mut self, + protocol_ids: &[u16], + ) -> ControlFlow, Self> { + let selected = ProtocolId::IN_ORDER_OF_PREFERENCE + .iter() + .find_map(|proto| protocol_ids.contains(&(*proto as u16)).then_some(*proto)); + + match selected { + None => ControlFlow::Break(Err(KeyExchangeError::NoValidProtocol)), + Some(protocol) => match self.protocol { + None => { + self.protocol = Some(protocol); + ControlFlow::Continue(self) + } + Some(_) => ControlFlow::Break(Err(KeyExchangeError::BadResponse)), + }, + } + } + + fn handle_aead_algorithm( + mut self, + algorithm_ids: &[u16], + ) -> ControlFlow, Self> { + let algorithm_id = match algorithm_ids { + [] => return ControlFlow::Break(Err(KeyExchangeError::NoValidAlgorithm)), + [algorithm_id] => *algorithm_id, + _ => return ControlFlow::Break(Err(KeyExchangeError::BadResponse)), + }; + + let selected = AeadAlgorithm::IN_ORDER_OF_PREFERENCE + .iter() + .find(|algo| (algorithm_id == (**algo as u16))); + + match selected { + None => ControlFlow::Break(Err(KeyExchangeError::NoValidAlgorithm)), + Some(algorithm) => match self.algorithm { + None => { + self.algorithm = Some(*algorithm); + ControlFlow::Continue(self) + } + Some(_) => ControlFlow::Break(Err(KeyExchangeError::BadResponse)), + }, + } + } + + fn handle_unknown( + self, + critical: bool, + ) -> ControlFlow, Self> { + if critical { + ControlFlow::Break(Err(KeyExchangeError::UnrecognizedCriticalRecord)) + } else { + ControlFlow::Continue(self) + } + } + + #[cfg(feature = "nts-pool")] + fn handle_keep_alive( + mut self, + ) -> ControlFlow, Self> { + self.keep_alive = true; + ControlFlow::Continue(self) + } + + #[cfg(feature = "nts-pool")] + fn handle_supported_algorithm_list( + mut self, + supported_algorithms: Vec<(u16, u16)>, + ) -> ControlFlow, Self> { + self.supported_algorithms = Some( + supported_algorithms + .into_iter() + .filter_map(|(aead_protocol_id, key_length)| { + let aead_algorithm = AeadAlgorithm::try_deserialize(aead_protocol_id)?; + Some((aead_algorithm, key_length)) + }) + .collect::>() + .into_boxed_slice(), + ); + + ControlFlow::Continue(self) } } @@ -1164,18 +1217,26 @@ pub struct KeyExchangeClient { impl KeyExchangeClient { const NTP_DEFAULT_PORT: u16 = 123; + #[must_use] pub fn wants_read(&self) -> bool { self.tls_connection.wants_read() } + /// # Errors + /// + /// Returns error if reading the TLS socket fails. pub fn read_socket(&mut self, rd: &mut dyn Read) -> std::io::Result { self.tls_connection.read_tls(rd) } + #[must_use] pub fn wants_write(&self) -> bool { self.tls_connection.wants_write() } + /// # Errors + /// + /// Returns error if writing to the TLS socket fails. pub fn write_socket(&mut self, wr: &mut dyn Write) -> std::io::Result { self.tls_connection.write_tls(wr) } @@ -1258,6 +1319,10 @@ impl KeyExchangeClient { }) } + /// # Errors + /// + /// Returns error if writing to an `NtsRecord` fails. + /// Returns error if writing to the TLS connection fails. pub fn new( server_name: String, tls_config: tls_utils::ClientConfig, @@ -1388,11 +1453,17 @@ impl KeyExchangeServerDecoder { self, record: NtsRecord, ) -> ControlFlow, Self> { - use self::AeadAlgorithm as Algorithm; use ControlFlow::{Break, Continue}; - use KeyExchangeError::*; - use NtsRecord::*; + #[cfg(feature = "ntpv5")] + use NtsRecord::DraftId; + use NtsRecord::{ + AeadAlgorithm, EndOfMessage, Error, NewCookie, NextProtocol, Port, Server, Unknown, + Warning, + }; + #[cfg(feature = "nts-pool")] + use NtsRecord::{FixedKeyRequest, KeepAlive, NtpServerDeny, SupportedAlgorithmList}; + #[allow(unused_mut)] let mut state = self; match record { @@ -1432,7 +1503,7 @@ impl KeyExchangeServerDecoder { Continue(state) } Error { errorcode } => { - // + // Return error from `errorcode` Break(Err(KeyExchangeError::from_error_code(errorcode))) } Warning { warningcode } => { @@ -1440,66 +1511,14 @@ impl KeyExchangeServerDecoder { Continue(state) } - NextProtocol { protocol_ids } => { - #[cfg(feature = "ntpv5")] - let selected = if state.allow_v5 { - protocol_ids - .iter() - .copied() - .find_map(ProtocolId::try_deserialize_v5) - } else { - protocol_ids - .iter() - .copied() - .find_map(ProtocolId::try_deserialize) - }; - - #[cfg(not(feature = "ntpv5"))] - let selected = protocol_ids - .iter() - .copied() - .find_map(ProtocolId::try_deserialize); - - match selected { - None => Break(Err(NoValidProtocol)), - Some(protocol) => { - // The NTS Next Protocol Negotiation record [..] MUST occur exactly once in every NTS-KE request and response. - match state.protocol { - None => { - state.protocol = Some(protocol); - Continue(state) - } - Some(_) => Break(Err(KeyExchangeError::BadRequest)), - } - } - } - } - AeadAlgorithm { algorithm_ids, .. } => { - let selected = algorithm_ids - .iter() - .copied() - .find_map(Algorithm::try_deserialize); - - match selected { - None => Break(Err(NoValidAlgorithm)), - Some(algorithm) => { - // for the protocol ids we support, the AeadAlgorithm record must be present - match state.algorithm { - None => { - state.algorithm = Some(algorithm); - Continue(state) - } - Some(_) => Break(Err(KeyExchangeError::BadRequest)), - } - } - } - } - + NextProtocol { protocol_ids } => state.handle_next_protocol(&protocol_ids), + AeadAlgorithm { algorithm_ids, .. } => state.handle_aead_algorithm(&algorithm_ids), #[cfg(feature = "nts-pool")] KeepAlive => { state.keep_alive = Some(true); Continue(state) } + #[allow(clippy::used_underscore_binding)] #[cfg(feature = "nts-pool")] SupportedAlgorithmList { supported_algorithms: _supported_algorithms, @@ -1535,6 +1554,76 @@ impl KeyExchangeServerDecoder { fn new() -> Self { Self::default() } + + fn handle_next_protocol( + mut self, + protocol_ids: &[u16], + ) -> ControlFlow, KeyExchangeServerDecoder> + { + use crate::nts_record::ControlFlow::{Break, Continue}; + use crate::KeyExchangeError::NoValidProtocol; + + #[cfg(feature = "ntpv5")] + let selected = if self.allow_v5 { + protocol_ids + .iter() + .copied() + .find_map(ProtocolId::try_deserialize_v5) + } else { + protocol_ids + .iter() + .copied() + .find_map(ProtocolId::try_deserialize) + }; + + #[cfg(not(feature = "ntpv5"))] + let selected = protocol_ids + .iter() + .copied() + .find_map(ProtocolId::try_deserialize); + + match selected { + None => Break(Err(NoValidProtocol)), + Some(protocol) => { + // The NTS Next Protocol Negotiation record [..] MUST occur exactly once in every NTS-KE request and response. + match self.protocol { + None => { + self.protocol = Some(protocol); + Continue(self) + } + Some(_) => Break(Err(KeyExchangeError::BadRequest)), + } + } + } + } + + fn handle_aead_algorithm( + mut self, + algorithm_ids: &[u16], + ) -> ControlFlow, KeyExchangeServerDecoder> + { + use self::AeadAlgorithm as Algorithm; + use ControlFlow::{Break, Continue}; + use KeyExchangeError::NoValidAlgorithm; + let selected = algorithm_ids + .iter() + .copied() + .find_map(Algorithm::try_deserialize); + + match selected { + None => Break(Err(NoValidAlgorithm)), + Some(algorithm) => { + // for the protocol ids we support, the AeadAlgorithm record must be present + match self.algorithm { + None => { + self.algorithm = Some(algorithm); + Continue(self) + } + Some(_) => Break(Err(KeyExchangeError::BadRequest)), + } + } + } + } } #[derive(Debug)] @@ -1556,28 +1645,40 @@ enum State { } impl KeyExchangeServer { + #[must_use] pub fn wants_read(&self) -> bool { self.tls_connection.wants_read() } + /// # Errors + /// + /// Returns error if reading the TLS socket fails. pub fn read_socket(&mut self, rd: &mut dyn Read) -> std::io::Result { self.tls_connection.read_tls(rd) } + #[must_use] pub fn wants_write(&self) -> bool { self.tls_connection.wants_write() } + /// # Errors + /// + /// Returns error if writing to the TLS socket fails. pub fn write_socket(&mut self, wr: &mut dyn Write) -> std::io::Result { self.tls_connection.write_tls(wr) } + /// # Errors + /// + /// Returns error if writing to an `NtsRecord` fails. + /// Returns error if writing to the TLS connection fails. fn send_records( tls_connection: &mut tls_utils::ServerConnection, records: &[NtsRecord], ) -> std::io::Result<()> { let mut buffer = Vec::with_capacity(1024); - for record in records.iter() { + for record in records { record.write(&mut buffer)?; } @@ -1606,6 +1707,7 @@ impl KeyExchangeServer { } } + #[must_use] pub fn progress( mut self, ) -> ControlFlow, Self> { @@ -1622,9 +1724,8 @@ impl KeyExchangeServer { // see https://docs.rs/rustls/latest/rustls/struct.Reader.html#method.read if self.wants_write() { return ControlFlow::Continue(self); - } else { - return ControlFlow::Break(self.end_of_file()); } + return ControlFlow::Break(self.end_of_file()); } Ok(n) => { match self.state { @@ -1637,7 +1738,7 @@ impl KeyExchangeServer { // all records have been decoded; send a response // continues for a clean shutdown of the connection by the client self.state = State::Done; - return self.decoder_done(data); + return self.decoder_done(&data); } ControlFlow::Break(Err(error)) => { Self::send_error_record(&mut self.tls_connection, &error); @@ -1662,9 +1763,8 @@ impl KeyExchangeServer { // see https://docs.rs/rustls/latest/rustls/struct.Reader.html#method.read if self.wants_write() { return ControlFlow::Continue(self); - } else { - return ControlFlow::Break(self.end_of_file()); } + return ControlFlow::Break(self.end_of_file()); } _ => { let error = KeyExchangeError::Io(e); @@ -1695,12 +1795,12 @@ impl KeyExchangeServer { } #[cfg(feature = "nts-pool")] + #[must_use] pub fn privileged_connection(&self) -> bool { self.tls_connection .peer_certificates() .and_then(|cert_chain| cert_chain.first()) - .map(|cert| self.pool_certificates.contains(cert)) - .unwrap_or(false) + .is_some_and(|cert| self.pool_certificates.contains(cert)) } #[cfg(feature = "nts-pool")] @@ -1738,7 +1838,7 @@ impl KeyExchangeServer { fn decoder_done( mut self, - data: ServerKeyExchangeData, + data: &ServerKeyExchangeData, ) -> ControlFlow, Self> { let algorithm = data.algorithm; let protocol = data.protocol; @@ -1748,7 +1848,7 @@ impl KeyExchangeServer { tracing::debug!(?protocol, ?algorithm, "selected AEAD algorithm"); - match self.extract_nts_keys(&data) { + match self.extract_nts_keys(data) { Ok(keys) => { let records = NtsRecord::server_key_exchange_records( protocol, @@ -1776,6 +1876,9 @@ impl KeyExchangeServer { } } + /// # Errors + /// + /// Returns `KeyExchangeError` if the `alpn_protocols` is invalid. pub fn new( tls_config: Arc, keyset: Arc, @@ -2149,7 +2252,7 @@ mod test { let error = client_decode_records(&records).unwrap_err(); - assert!(matches!(error, KeyExchangeError::NoValidProtocol)) + assert!(matches!(error, KeyExchangeError::NoValidProtocol)); } #[test] @@ -2921,7 +3024,7 @@ mod test { Certified, } - fn client_server_pair(client_type: ClientType) -> (KeyExchangeClient, KeyExchangeServer) { + fn client_server_pair(client_type: &ClientType) -> (KeyExchangeClient, KeyExchangeServer) { #[allow(unused)] use tls_utils::CloneKeyShim; @@ -3065,7 +3168,7 @@ mod test { #[test] fn test_keyexchange_roundtrip() { - let (mut client, server) = client_server_pair(ClientType::Uncertified); + let (mut client, server) = client_server_pair(&ClientType::Uncertified); let mut buffer = Vec::with_capacity(1024); for record in NtsRecord::client_key_exchange_records(None, []).iter() { @@ -3091,7 +3194,7 @@ mod test { #[test] #[cfg(feature = "nts-pool")] fn test_keyexchange_roundtrip_fixed_not_authorized() { - let (mut client, server) = client_server_pair(ClientType::Uncertified); + let (mut client, server) = client_server_pair(&ClientType::Uncertified); let c2s: Vec<_> = (0..).take(64).collect(); let s2c: Vec<_> = (0..).skip(64).take(64).collect(); @@ -3113,7 +3216,7 @@ mod test { #[test] #[cfg(feature = "nts-pool")] fn test_keyexchange_roundtrip_fixed_authorized() { - let (mut client, server) = client_server_pair(ClientType::Certified); + let (mut client, server) = client_server_pair(&ClientType::Certified); let c2s: Vec<_> = (0..).take(64).collect(); let s2c: Vec<_> = (0..).skip(64).take(64).collect(); @@ -3143,7 +3246,7 @@ mod test { #[cfg(feature = "nts-pool")] #[test] fn test_supported_algos_roundtrip() { - let (mut client, server) = client_server_pair(ClientType::Uncertified); + let (mut client, server) = client_server_pair(&ClientType::Uncertified); let mut buffer = Vec::with_capacity(1024); for record in [ @@ -3171,7 +3274,7 @@ mod test { } for n in 0..buffer.len() { - let (mut client, server) = client_server_pair(ClientType::Uncertified); + let (mut client, server) = client_server_pair(&ClientType::Uncertified); client .tls_connection .writer() diff --git a/ntp-proto/src/packet/crypto.rs b/ntp-proto/src/packet/crypto.rs index dbfa78854..c15bb5ed3 100644 --- a/ntp-proto/src/packet/crypto.rs +++ b/ntp-proto/src/packet/crypto.rs @@ -84,6 +84,9 @@ pub trait Cipher: Sync + Send + ZeroizeOnDrop + 'static { /// - encrypts `plaintext_length` bytes from the buffer /// - puts the nonce followed by the ciphertext into the buffer /// - returns the size of the nonce and ciphertext + /// # Errors + /// + /// Returns error if encryption fails. fn encrypt( &self, buffer: &mut [u8], @@ -91,6 +94,9 @@ pub trait Cipher: Sync + Send + ZeroizeOnDrop + 'static { associated_data: &[u8], ) -> std::io::Result; + /// # Errors + /// + /// Returns error if decryption fails. // MUST support arbitrary length nonces fn decrypt( &self, @@ -168,7 +174,8 @@ impl AesSivCmac256 { #[cfg(feature = "nts-pool")] pub fn key_size() -> usize { // prefer trust in compiler optimisation over trust in mental arithmetic - Self::new(Default::default()).key.len() + use aead::generic_array::GenericArray; + Self::new(GenericArray::default()).key.len() } #[cfg(feature = "nts-pool")] @@ -253,7 +260,8 @@ impl AesSivCmac512 { #[cfg(feature = "nts-pool")] pub fn key_size() -> usize { // prefer trust in compiler optimisation over trust in mental arithmetic - Self::new(Default::default()).key.len() + use aead::generic_array::GenericArray; + Self::new(GenericArray::default()).key.len() } #[cfg(feature = "nts-pool")] @@ -341,6 +349,7 @@ impl IdentityCipher { #[cfg(test)] impl ZeroizeOnDrop for IdentityCipher {} +#[allow(clippy::cast_possible_truncation)] #[cfg(test)] impl Cipher for IdentityCipher { fn encrypt( diff --git a/ntp-proto/src/packet/error.rs b/ntp-proto/src/packet/error.rs index b50b34b99..56e108f93 100644 --- a/ntp-proto/src/packet/error.rs +++ b/ntp-proto/src/packet/error.rs @@ -16,7 +16,12 @@ pub enum ParsingError { impl ParsingError { pub(super) fn get_decrypt_error(self) -> Result> { - use ParsingError::*; + #[cfg(feature = "ntpv5")] + use ParsingError::V5; + use ParsingError::{ + DecryptError, IncorrectLength, InvalidVersion, MalformedCookiePlaceholder, + MalformedNonce, MalformedNtsExtensionFields, + }; match self { InvalidVersion(v) => Err(InvalidVersion(v)), @@ -33,7 +38,12 @@ impl ParsingError { impl ParsingError { pub(super) fn generalize(self) -> ParsingError { - use ParsingError::*; + #[cfg(feature = "ntpv5")] + use ParsingError::V5; + use ParsingError::{ + DecryptError, IncorrectLength, InvalidVersion, MalformedCookiePlaceholder, + MalformedNonce, MalformedNtsExtensionFields, + }; match self { InvalidVersion(v) => InvalidVersion(v), diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index ceb3c6bf8..9e0c738c8 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -1,3 +1,4 @@ +#![allow(clippy::cast_possible_truncation)] use std::{ borrow::Cow, io::{Cursor, Write}, @@ -127,8 +128,16 @@ impl std::fmt::Debug for ExtensionField<'_> { impl<'a> ExtensionField<'a> { const HEADER_LENGTH: usize = 4; + #[must_use] pub fn into_owned(self) -> ExtensionField<'static> { - use ExtensionField::*; + use ExtensionField::{ + InvalidNtsEncryptedField, NtsCookie, NtsCookiePlaceholder, UniqueIdentifier, Unknown, + }; + + #[cfg(feature = "ntpv5")] + use ExtensionField::{ + DraftIdentification, Padding, ReferenceIdRequest, ReferenceIdResponse, + }; match self { Unknown { @@ -163,7 +172,14 @@ impl<'a> ExtensionField<'a> { minimum_size: u16, version: ExtensionHeaderVersion, ) -> std::io::Result<()> { - use ExtensionField::*; + use ExtensionField::{ + InvalidNtsEncryptedField, NtsCookie, NtsCookiePlaceholder, UniqueIdentifier, Unknown, + }; + + #[cfg(feature = "ntpv5")] + use ExtensionField::{ + DraftIdentification, Padding, ReferenceIdRequest, ReferenceIdResponse, + }; match self { Unknown { type_id, data } => { @@ -190,6 +206,9 @@ impl<'a> ExtensionField<'a> { } } + /// # Errors + /// + /// Returns `io::Error` if serialization fails. #[cfg(feature = "__internal-fuzz")] pub fn serialize_pub( &self, @@ -219,7 +238,7 @@ impl<'a> ExtensionField<'a> { (data_length as u16 + ExtensionField::HEADER_LENGTH as u16).max(minimum_size); if version == ExtensionHeaderVersion::V4 { - actual_length = next_multiple_of_u16(actual_length, 4) + actual_length = next_multiple_of_u16(actual_length, 4); } w.write_all(&ef_id.to_type_id().to_be_bytes())?; @@ -452,6 +471,9 @@ impl<'a> ExtensionField<'a> { Ok(()) } + /// # Errors + /// + /// Returns `io::Error` if encoding fails. #[cfg(feature = "ntpv5")] pub fn encode_padding_field( mut w: impl NonBlockingWrite, @@ -485,10 +507,8 @@ impl<'a> ExtensionField<'a> { Ok(ExtensionField::UniqueIdentifier(message[..].into())) } - fn decode_nts_cookie( - message: &'a [u8], - ) -> Result> { - Ok(ExtensionField::NtsCookie(message[..].into())) + fn decode_nts_cookie(message: &'a [u8]) -> Self { + ExtensionField::NtsCookie(message[..].into()) } fn decode_nts_cookie_placeholder( @@ -503,14 +523,11 @@ impl<'a> ExtensionField<'a> { } } - fn decode_unknown( - type_id: u16, - message: &'a [u8], - ) -> Result> { - Ok(ExtensionField::Unknown { + fn decode_unknown(type_id: u16, message: &'a [u8]) -> Self { + ExtensionField::Unknown { type_id, data: Cow::Borrowed(message), - }) + } } #[cfg(feature = "ntpv5")] @@ -532,7 +549,7 @@ impl<'a> ExtensionField<'a> { } fn decode( - raw: RawExtensionField<'a>, + raw: &RawExtensionField<'a>, #[cfg_attr(not(feature = "ntpv5"), allow(unused_variables))] extension_header_version: ExtensionHeaderVersion, ) -> Result> { @@ -543,7 +560,7 @@ impl<'a> ExtensionField<'a> { match raw.type_id { TypeId::UniqueIdentifier => EF::decode_unique_identifier(message), - TypeId::NtsCookie => EF::decode_nts_cookie(message), + TypeId::NtsCookie => Ok(EF::decode_nts_cookie(message)), TypeId::NtsCookiePlaceholder => EF::decode_nts_cookie_placeholder(message), #[cfg(feature = "ntpv5")] TypeId::DraftIdentification => { @@ -553,7 +570,7 @@ impl<'a> ExtensionField<'a> { TypeId::ReferenceIdRequest => Ok(ReferenceIdRequest::decode(message)?.into()), #[cfg(feature = "ntpv5")] TypeId::ReferenceIdResponse => Ok(ReferenceIdResponse::decode(message).into()), - type_id => EF::decode_unknown(type_id.to_type_id(), message), + type_id => Ok(EF::decode_unknown(type_id.to_type_id(), message)), } } } @@ -597,9 +614,8 @@ impl<'a> ExtensionFieldData<'a> { version: ExtensionHeaderVersion, ) -> std::io::Result<()> { if !self.authenticated.is_empty() || !self.encrypted.is_empty() { - let cipher = match cipher.get(&self.authenticated) { - Some(cipher) => cipher, - None => return Err(std::io::Error::new(std::io::ErrorKind::Other, "no cipher")), + let Some(cipher) = cipher.get(&self.authenticated) else { + return Err(std::io::Error::new(std::io::ErrorKind::Other, "no cipher")); }; // the authenticated extension fields are always followed by the encrypted extension @@ -657,56 +673,50 @@ impl<'a> ExtensionFieldData<'a> { RawExtensionField::V4_UNENCRYPTED_MINIMUM_SIZE, version, ) { - let (offset, field) = field.map_err(|e| e.generalize())?; + let (offset, field) = field.map_err(super::ParsingError::generalize)?; size = offset + field.wire_length(version); - match field.type_id { - ExtensionFieldTypeId::NtsEncryptedField => { - let encrypted = RawEncryptedField::from_message_bytes(field.message_bytes) - .map_err(|e| e.generalize())?; - - let cipher = match cipher.get(&efdata.untrusted) { - Some(cipher) => cipher, - None => { - efdata.untrusted.push(InvalidNtsEncryptedField); - is_valid_nts = false; - continue; - } - }; - - let encrypted_fields = match encrypted.decrypt( - cipher.as_ref(), - &data[..header_size + offset], - version, - ) { - Ok(encrypted_fields) => encrypted_fields, - Err(e) => { - // early return if it's anything but a decrypt error - e.get_decrypt_error()?; - - efdata.untrusted.push(InvalidNtsEncryptedField); - is_valid_nts = false; - continue; - } - }; - - // for the current ciphers we allow in non-test code, - // the nonce should always be 16 bytes - debug_assert_eq!(encrypted.nonce.len(), 16); - - efdata.encrypted.extend(encrypted_fields); - cookie = match cipher { - super::crypto::CipherHolder::DecodedServerCookie(cookie) => Some(cookie), - super::crypto::CipherHolder::Other(_) => None, - }; - - // All previous untrusted fields are now validated - efdata.authenticated.append(&mut efdata.untrusted); - } - _ => { - let field = - ExtensionField::decode(field, version).map_err(|e| e.generalize())?; - efdata.untrusted.push(field); - } + if field.type_id == ExtensionFieldTypeId::NtsEncryptedField { + let encrypted = RawEncryptedField::from_message_bytes(field.message_bytes) + .map_err(super::ParsingError::generalize)?; + + let Some(cipher) = cipher.get(&efdata.untrusted) else { + efdata.untrusted.push(InvalidNtsEncryptedField); + is_valid_nts = false; + continue; + }; + + let encrypted_fields = match encrypted.decrypt( + cipher.as_ref(), + &data[..header_size + offset], + version, + ) { + Ok(encrypted_fields) => encrypted_fields, + Err(e) => { + // early return if it's anything but a decrypt error + e.get_decrypt_error()?; + + efdata.untrusted.push(InvalidNtsEncryptedField); + is_valid_nts = false; + continue; + } + }; + + // for the current ciphers we allow in non-test code, + // the nonce should always be 16 bytes + debug_assert_eq!(encrypted.nonce.len(), 16); + + efdata.encrypted.extend(encrypted_fields); + cookie = match cipher { + super::crypto::CipherHolder::DecodedServerCookie(cookie) => Some(cookie), + super::crypto::CipherHolder::Other(_) => None, + }; + + // All previous untrusted fields are now validated + efdata.authenticated.append(&mut efdata.untrusted); + } else { + let field = ExtensionField::decode(&field, version) + .map_err(super::ParsingError::generalize)?; + efdata.untrusted.push(field); } } @@ -740,23 +750,23 @@ impl<'a> RawEncryptedField<'a> { fn from_message_bytes( message_bytes: &'a [u8], ) -> Result> { - use ParsingError::*; - let [b0, b1, b2, b3, ref rest @ ..] = message_bytes[..] else { - return Err(IncorrectLength); + return Err(ParsingError::IncorrectLength); }; let nonce_length = u16::from_be_bytes([b0, b1]) as usize; let ciphertext_length = u16::from_be_bytes([b2, b3]) as usize; - let nonce = rest.get(..nonce_length).ok_or(IncorrectLength)?; + let nonce = rest + .get(..nonce_length) + .ok_or(ParsingError::IncorrectLength)?; // skip the lengths and the nonce. pad to a multiple of 4 let ciphertext_start = 4 + next_multiple_of_u16(nonce_length as u16, 4) as usize; let ciphertext = message_bytes .get(ciphertext_start..ciphertext_start + ciphertext_length) - .ok_or(IncorrectLength)?; + .ok_or(ParsingError::IncorrectLength)?; Ok(Self { nonce, ciphertext }) } @@ -767,13 +777,10 @@ impl<'a> RawEncryptedField<'a> { aad: &[u8], version: ExtensionHeaderVersion, ) -> Result>, ParsingError>> { - let plaintext = match cipher.decrypt(self.nonce, self.ciphertext, aad) { - Ok(plain) => plain, - Err(_) => { - return Err(ParsingError::DecryptError( - ExtensionField::InvalidNtsEncryptedField, - )); - } + let Ok(plaintext) = cipher.decrypt(self.nonce, self.ciphertext, aad) else { + return Err(ParsingError::DecryptError( + ExtensionField::InvalidNtsEncryptedField, + )); }; RawExtensionField::deserialize_sequence( @@ -783,13 +790,15 @@ impl<'a> RawEncryptedField<'a> { version, ) .map(|encrypted_field| { - let encrypted_field = encrypted_field.map_err(|e| e.generalize())?.1; + let encrypted_field = encrypted_field + .map_err(super::error::ParsingError::generalize)? + .1; if encrypted_field.type_id == ExtensionFieldTypeId::NtsEncryptedField { // TODO: Discuss whether we want this check Err(ParsingError::MalformedNtsExtensionFields) } else { - Ok(ExtensionField::decode(encrypted_field, version) - .map_err(|e| e.generalize())? + Ok(ExtensionField::decode(&encrypted_field, version) + .map_err(super::error::ParsingError::generalize)? .into_owned()) } }) @@ -1015,7 +1024,7 @@ mod tests { type_id: ExtensionFieldTypeId::NtsCookiePlaceholder, message_bytes: &[1; COOKIE_LENGTH], }; - let output = ExtensionField::decode(raw, ExtensionHeaderVersion::V4).unwrap_err(); + let output = ExtensionField::decode(&raw, ExtensionHeaderVersion::V4).unwrap_err(); assert!(matches!(output, ParsingError::MalformedCookiePlaceholder)); @@ -1023,7 +1032,7 @@ mod tests { type_id: ExtensionFieldTypeId::NtsCookiePlaceholder, message_bytes: &[0; COOKIE_LENGTH], }; - let output = ExtensionField::decode(raw, ExtensionHeaderVersion::V4).unwrap(); + let output = ExtensionField::decode(&raw, ExtensionHeaderVersion::V4).unwrap(); let ExtensionField::NtsCookiePlaceholder { cookie_length } = output else { panic!("incorrect variant"); @@ -1056,7 +1065,7 @@ mod tests { data.extend(&[0]); // Padding let raw = RawExtensionField::deserialize(&data, 4, ExtensionHeaderVersion::V5).unwrap(); - let ef = ExtensionField::decode(raw, ExtensionHeaderVersion::V4).unwrap(); + let ef = ExtensionField::decode(&raw, ExtensionHeaderVersion::V4).unwrap(); let ExtensionField::DraftIdentification(ref parsed) = ef else { panic!("Unexpected extension field {ef:?}... expected DraftIdentification"); diff --git a/ntp-proto/src/packet/mac.rs b/ntp-proto/src/packet/mac.rs index 468a88ac6..4b01f5167 100644 --- a/ntp-proto/src/packet/mac.rs +++ b/ntp-proto/src/packet/mac.rs @@ -54,7 +54,7 @@ mod tests { mac: Cow::Borrowed(&[1, 2, 3, 4, 5, 6, 7, 8]), }; - let input = input.to_owned(); + let input = input.clone(); let mut w = Vec::new(); input.serialize(&mut w).unwrap(); diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index e8f6e8163..92b8ce350 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -61,6 +61,7 @@ impl NtpLeapIndicator { } } + #[must_use] pub fn is_synchronized(&self) -> bool { !matches!(self, Self::Unknown) } @@ -154,7 +155,7 @@ pub struct RequestIdentifier { impl NtpHeaderV3V4 { const WIRE_LENGTH: usize = 48; - /// A new, empty NtpHeader + /// A new, empty `NtpHeader` fn new() -> Self { Self { leap: NtpLeapIndicator::NoWarning, @@ -172,6 +173,7 @@ impl NtpHeaderV3V4 { } } + #[allow(clippy::cast_possible_wrap)] fn deserialize(data: &[u8]) -> Result<(Self, usize), ParsingError> { if data.len() < Self::WIRE_LENGTH { return Err(ParsingError::IncorrectLength); @@ -196,6 +198,7 @@ impl NtpHeaderV3V4 { )) } + #[allow(clippy::cast_sign_loss)] fn serialize(&self, mut w: impl NonBlockingWrite, version: u8) -> std::io::Result<()> { w.write_all(&[(self.leap.to_bits() << 6) | (version << 3) | self.mode.to_bits()])?; w.write_all(&[self.stratum, self.poll.as_byte(), self.precision as u8])?; @@ -249,7 +252,7 @@ impl NtpHeaderV3V4 { // Timestamp must be last to make it as accurate as possible. transmit_timestamp: clock.now().expect("Failed to read time"), leap: system.time_snapshot.leap_indicator, - reference_timestamp: Default::default(), + reference_timestamp: NtpTimestamp::default(), } } @@ -285,14 +288,19 @@ impl NtpHeaderV3V4 { } impl<'a> NtpPacket<'a> { + #[must_use] pub fn into_owned(self) -> NtpPacket<'static> { NtpPacket::<'static> { header: self.header, efdata: self.efdata.into_owned(), - mac: self.mac.map(|v| v.into_owned()), + mac: self.mac.map(mac::Mac::into_owned), } } + /// # Errors + /// + /// Returns error if `data` has incorrect length. + /// Returns error if the parsing fails. #[allow(clippy::result_large_err)] pub fn deserialize( data: &'a [u8], @@ -305,129 +313,136 @@ impl<'a> NtpPacket<'a> { let version = (data[0] & 0b0011_1000) >> 3; match version { - 3 => { - let (header, header_size) = - NtpHeaderV3V4::deserialize(data).map_err(|e| e.generalize())?; - let mac = if header_size != data.len() { - Some(Mac::deserialize(&data[header_size..]).map_err(|e| e.generalize())?) - } else { - None - }; - Ok(( - NtpPacket { - header: NtpHeader::V3(header), - efdata: ExtensionFieldData::default(), - mac, - }, - None, + 3 => Self::deserialize_v3(data), + 4 => Self::deserialize_v4(data, cipher), + #[cfg(feature = "ntpv5")] + 5 => Self::deserialize_v5(data, cipher), + _ => Err(PacketParsingError::InvalidVersion(version)), + } + } + + #[allow(clippy::result_large_err)] + fn deserialize_v3( + data: &'a [u8], + ) -> Result<(Self, Option), PacketParsingError<'a>> { + let (header, header_size) = + NtpHeaderV3V4::deserialize(data).map_err(error::ParsingError::generalize)?; + let mac = if header_size == data.len() { + None + } else { + Some(Mac::deserialize(&data[header_size..]).map_err(error::ParsingError::generalize)?) + }; + Ok(( + NtpPacket { + header: NtpHeader::V3(header), + efdata: ExtensionFieldData::default(), + mac, + }, + None, + )) + } + + #[allow(clippy::result_large_err)] + fn deserialize_v4( + data: &'a [u8], + cipher: &(impl CipherProvider + ?Sized), + ) -> Result<(Self, Option), PacketParsingError<'a>> { + let (header, header_size) = + NtpHeaderV3V4::deserialize(data).map_err(error::ParsingError::generalize)?; + + Self::deserialize_with_extension_fields( + data, + header_size, + cipher, + ExtensionHeaderVersion::V4, + NtpHeader::V4, + header, + ) + } + + #[allow(clippy::result_large_err)] + #[cfg(feature = "ntpv5")] + fn deserialize_v5( + data: &'a [u8], + cipher: &(impl CipherProvider + ?Sized), + ) -> Result<(Self, Option), PacketParsingError<'a>> { + let (header, header_size) = + v5::NtpHeaderV5::deserialize(data).map_err(error::ParsingError::generalize)?; + + // TODO: Check extension field handling in V5 + let (packet, cookie) = Self::deserialize_with_extension_fields( + data, + header_size, + cipher, + ExtensionHeaderVersion::V5, + NtpHeader::V5, + header, + )?; + + match packet.draft_id() { + Some(id) if id == v5::DRAFT_VERSION => Ok((packet, cookie)), + received @ (Some(_) | None) => { + tracing::error!( + expected = v5::DRAFT_VERSION, + received, + "Mismatched draft ID ignoring packet!" + ); + Err(PacketParsingError::V5( + v5::V5Error::InvalidDraftIdentification, )) } - 4 => { - let (header, header_size) = - NtpHeaderV3V4::deserialize(data).map_err(|e| e.generalize())?; - - let construct_packet = |remaining_bytes: &'a [u8], efdata| { - let mac = if !remaining_bytes.is_empty() { - Some(Mac::deserialize(remaining_bytes)?) - } else { - None - }; + } + } - let packet = NtpPacket { - header: NtpHeader::V4(header), - efdata, - mac, - }; + #[allow(clippy::result_large_err)] + fn deserialize_with_extension_fields( + data: &'a [u8], + header_size: usize, + cipher: &(impl CipherProvider + ?Sized), + extension_header_version: ExtensionHeaderVersion, + header_constructor: F, + header: H, + ) -> Result<(Self, Option), PacketParsingError<'a>> + where + F: Fn(H) -> NtpHeader, + { + let construct_packet = |remaining_bytes: &'a [u8], efdata| { + let mac = if remaining_bytes.is_empty() { + None + } else { + Some(Mac::deserialize(remaining_bytes)?) + }; + + let packet = NtpPacket { + header: header_constructor(header), + efdata, + mac, + }; + + Ok::<_, ParsingError>(packet) + }; - Ok::<_, ParsingError>(packet) - }; - - match ExtensionFieldData::deserialize( - data, - header_size, - cipher, - ExtensionHeaderVersion::V4, - ) { - Ok(decoded) => { - let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) - .map_err(|e| e.generalize())?; - - Ok((packet, decoded.cookie)) - } - Err(e) => { - // return early if it is anything but a decrypt error - let invalid = e.get_decrypt_error()?; - - let packet = construct_packet(invalid.remaining_bytes, invalid.efdata) - .map_err(|e| e.generalize())?; - - Err(ParsingError::DecryptError(packet)) - } - } - } - #[cfg(feature = "ntpv5")] - 5 => { - let (header, header_size) = - v5::NtpHeaderV5::deserialize(data).map_err(|e| e.generalize())?; - - let construct_packet = |remaining_bytes: &'a [u8], efdata| { - let mac = if !remaining_bytes.is_empty() { - Some(Mac::deserialize(remaining_bytes)?) - } else { - None - }; + match ExtensionFieldData::deserialize(data, header_size, cipher, extension_header_version) { + Ok(decoded) => { + let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) + .map_err(error::ParsingError::generalize)?; - let packet = NtpPacket { - header: NtpHeader::V5(header), - efdata, - mac, - }; + Ok((packet, decoded.cookie)) + } + Err(e) => { + // return early if it is anything but a decrypt error + let invalid = e.get_decrypt_error()?; - Ok::<_, ParsingError>(packet) - }; + let packet = construct_packet(invalid.remaining_bytes, invalid.efdata) + .map_err(error::ParsingError::generalize)?; - // TODO: Check extension field handling in V5 - let res_packet = match ExtensionFieldData::deserialize( - data, - header_size, - cipher, - ExtensionHeaderVersion::V5, - ) { - Ok(decoded) => { - let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) - .map_err(|e| e.generalize())?; - - Ok((packet, decoded.cookie)) - } - Err(e) => { - // return early if it is anything but a decrypt error - let invalid = e.get_decrypt_error()?; - - let packet = construct_packet(invalid.remaining_bytes, invalid.efdata) - .map_err(|e| e.generalize())?; - - Err(ParsingError::DecryptError(packet)) - } - }; - - let (packet, cookie) = res_packet?; - - match packet.draft_id() { - Some(id) if id == v5::DRAFT_VERSION => Ok((packet, cookie)), - received @ (Some(_) | None) => { - tracing::error!( - expected = v5::DRAFT_VERSION, - received, - "Mismatched draft ID ignoring packet!" - ); - Err(ParsingError::V5(v5::V5Error::InvalidDraftIdentification)) - } - } + Err(ParsingError::DecryptError(packet)) } - _ => Err(PacketParsingError::InvalidVersion(version)), } } + #[allow(clippy::missing_errors_doc)] + #[allow(clippy::cast_possible_truncation)] #[cfg(test)] pub fn serialize_without_encryption_vec( &self, @@ -444,6 +459,10 @@ impl<'a> NtpPacket<'a> { Ok(buffer) } + /// # Errors + /// + /// Returns error if the serialization fails. + #[allow(clippy::cast_possible_truncation)] pub fn serialize( &self, w: &mut Cursor<&mut [u8]>, @@ -464,12 +483,12 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(_) => { /* No extension fields in V3 */ } NtpHeader::V4(_) => { self.efdata - .serialize(&mut *w, cipher, ExtensionHeaderVersion::V4)? + .serialize(&mut *w, cipher, ExtensionHeaderVersion::V4)?; } #[cfg(feature = "ntpv5")] NtpHeader::V5(_) => { self.efdata - .serialize(&mut *w, cipher, ExtensionHeaderVersion::V5)? + .serialize(&mut *w, cipher, ExtensionHeaderVersion::V5)?; } } @@ -492,6 +511,8 @@ impl<'a> NtpPacket<'a> { Ok(()) } + #[allow(clippy::cast_possible_truncation)] + #[must_use] pub fn nts_poll_message( cookie: &'a [u8], new_cookies: u8, @@ -529,7 +550,9 @@ impl<'a> NtpPacket<'a> { ) } + #[allow(clippy::cast_possible_truncation)] #[cfg(feature = "ntpv5")] + #[must_use] pub fn nts_poll_message_v5( cookie: &'a [u8], new_cookies: u8, @@ -570,12 +593,13 @@ impl<'a> NtpPacket<'a> { ) } + #[must_use] pub fn poll_message(poll_interval: PollInterval) -> (Self, RequestIdentifier) { let (header, id) = NtpHeaderV3V4::poll_message(poll_interval); ( NtpPacket { header: NtpHeader::V4(header), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }, id, @@ -583,6 +607,7 @@ impl<'a> NtpPacket<'a> { } #[cfg(feature = "ntpv5")] + #[must_use] pub fn poll_message_upgrade_request(poll_interval: PollInterval) -> (Self, RequestIdentifier) { let (mut header, id) = NtpHeaderV3V4::poll_message(poll_interval); @@ -603,6 +628,7 @@ impl<'a> NtpPacket<'a> { } #[cfg(feature = "ntpv5")] + #[must_use] pub fn poll_message_v5(poll_interval: PollInterval) -> (Self, RequestIdentifier) { let (header, id) = v5::NtpHeaderV5::poll_message(poll_interval); @@ -637,7 +663,7 @@ impl<'a> NtpPacket<'a> { recv_timestamp, clock, )), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }, NtpHeader::V4(header) => { @@ -829,11 +855,12 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn rate_limit_response(packet_from_client: Self) -> Self { match packet_from_client.header { NtpHeader::V3(header) => NtpPacket { header: NtpHeader::V3(NtpHeaderV3V4::rate_limit_response(header)), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }, NtpHeader::V4(header) => NtpPacket { @@ -875,6 +902,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn nts_rate_limit_response(packet_from_client: Self) -> Self { match packet_from_client.header { NtpHeader::V3(_) => unreachable!("NTS shouldn't work with NTPv3"), @@ -913,11 +941,12 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn deny_response(packet_from_client: Self) -> Self { match packet_from_client.header { NtpHeader::V3(header) => NtpPacket { header: NtpHeader::V3(NtpHeaderV3V4::deny_response(header)), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }, NtpHeader::V4(header) => NtpPacket { @@ -959,6 +988,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn nts_deny_response(packet_from_client: Self) -> Self { match packet_from_client.header { NtpHeader::V3(_) => unreachable!("NTS shouldn't work with NTPv3"), @@ -997,6 +1027,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn nts_nak_response(packet_from_client: Self) -> Self { match packet_from_client.header { NtpHeader::V3(_) => unreachable!("NTS shouldn't work with NTPv3"), @@ -1046,6 +1077,7 @@ impl<'a> NtpPacket<'a> { }) } + #[must_use] pub fn version(&self) -> u8 { match self.header { NtpHeader::V3(_) => 3, @@ -1055,10 +1087,12 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn header(&self) -> NtpHeader { self.header } + #[must_use] pub fn leap(&self) -> NtpLeapIndicator { match self.header { NtpHeader::V3(header) => header.leap, @@ -1068,6 +1102,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn mode(&self) -> NtpAssociationMode { match self.header { NtpHeader::V3(header) => header.mode, @@ -1082,6 +1117,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn poll(&self) -> PollInterval { match self.header { NtpHeader::V3(h) | NtpHeader::V4(h) => h.poll, @@ -1090,6 +1126,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn stratum(&self) -> u8 { match self.header { NtpHeader::V3(header) => header.stratum, @@ -1099,6 +1136,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn precision(&self) -> i8 { match self.header { NtpHeader::V3(header) => header.precision, @@ -1108,6 +1146,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn root_delay(&self) -> NtpDuration { match self.header { NtpHeader::V3(header) => header.root_delay, @@ -1117,6 +1156,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn root_dispersion(&self) -> NtpDuration { match self.header { NtpHeader::V3(header) => header.root_dispersion, @@ -1126,6 +1166,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn receive_timestamp(&self) -> NtpTimestamp { match self.header { NtpHeader::V3(header) => header.receive_timestamp, @@ -1135,6 +1176,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn transmit_timestamp(&self) -> NtpTimestamp { match self.header { NtpHeader::V3(header) => header.transmit_timestamp, @@ -1144,6 +1186,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn reference_id(&self) -> ReferenceId { match self.header { NtpHeader::V3(header) => header.reference_id, @@ -1166,6 +1209,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn is_kiss(&self) -> bool { match self.header { NtpHeader::V3(header) => header.stratum == 0, @@ -1175,6 +1219,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn is_kiss_deny(&self) -> bool { self.is_kiss() && match self.header { @@ -1184,6 +1229,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn is_kiss_rate( &self, #[cfg_attr(not(feature = "ntpv5"), allow(unused))] own_interval: PollInterval, @@ -1198,6 +1244,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn is_kiss_rstr(&self) -> bool { self.is_kiss() && match self.header { @@ -1207,6 +1254,7 @@ impl<'a> NtpPacket<'a> { } } + #[must_use] pub fn is_kiss_ntsn(&self) -> bool { self.is_kiss() && match self.header { @@ -1217,6 +1265,7 @@ impl<'a> NtpPacket<'a> { } #[cfg(feature = "ntpv5")] + #[must_use] pub fn is_upgrade(&self) -> bool { matches!( self.header, @@ -1227,6 +1276,7 @@ impl<'a> NtpPacket<'a> { ) } + #[must_use] pub fn valid_server_response(&self, identifier: RequestIdentifier, nts_enabled: bool) -> bool { if let Some(uid) = identifier.uid { let auth = check_uid_extensionfield(self.efdata.authenticated.iter(), &uid); @@ -1302,6 +1352,7 @@ fn check_uid_extensionfield<'a, I: IntoIterator>>( #[cfg(any(test, feature = "__internal-fuzz", feature = "__internal-test"))] impl NtpPacket<'_> { + #[must_use] pub fn test() -> Self { Self::default() } @@ -1328,7 +1379,7 @@ impl NtpPacket<'_> { #[cfg(feature = "ntpv5")] // TODO can we just reuse the cookie as the origin timestamp? NtpHeader::V5(ref mut header) => { - header.client_cookie = v5::NtpClientCookie::from_ntp_timestamp(timestamp) + header.client_cookie = v5::NtpClientCookie::from_ntp_timestamp(timestamp); } } } @@ -1410,12 +1461,14 @@ impl Default for NtpPacket<'_> { fn default() -> Self { Self { header: NtpHeader::V4(NtpHeaderV3V4::new()), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, } } } +#[allow(clippy::cast_possible_truncation)] +#[allow(clippy::too_many_lines)] #[cfg(test)] mod tests { use crate::{ @@ -1500,13 +1553,13 @@ mod tests { precision: -24, root_delay: NtpDuration::from_fixed_int(1023 << 16), root_dispersion: NtpDuration::from_fixed_int(893 << 16), - reference_id: ReferenceId::from_int(0x5ec69f0f), - reference_timestamp: NtpTimestamp::from_fixed_int(0xe5f662987b61b9af), - origin_timestamp: NtpTimestamp::from_fixed_int(0xe5f663667b64995d), - receive_timestamp: NtpTimestamp::from_fixed_int(0xe5f6636681405590), - transmit_timestamp: NtpTimestamp::from_fixed_int(0xe5f663a8761dde48), + reference_id: ReferenceId::from_int(0x5ec6_9f0f), + reference_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_6298_7b61_b9af), + origin_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_6366_7b64_995d), + receive_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_6366_8140_5590), + transmit_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_63a8_761d_de48), }), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }; @@ -1529,13 +1582,13 @@ mod tests { precision: -24, root_delay: NtpDuration::from_fixed_int(1023 << 16), root_dispersion: NtpDuration::from_fixed_int(893 << 16), - reference_id: ReferenceId::from_int(0x5ec69f0f), - reference_timestamp: NtpTimestamp::from_fixed_int(0xe5f662987b61b9af), - origin_timestamp: NtpTimestamp::from_fixed_int(0xe5f663667b64995d), - receive_timestamp: NtpTimestamp::from_fixed_int(0xe5f6636681405590), - transmit_timestamp: NtpTimestamp::from_fixed_int(0xe5f663a8761dde48), + reference_id: ReferenceId::from_int(0x5ec6_9f0f), + reference_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_6298_7b61_b9af), + origin_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_6366_7b64_995d), + receive_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_6366_8140_5590), + transmit_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_63a8_761d_de48), }), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }; @@ -1561,13 +1614,13 @@ mod tests { precision: -23, root_delay: NtpDuration::from_fixed_int(566 << 16), root_dispersion: NtpDuration::from_fixed_int(951 << 16), - reference_id: ReferenceId::from_int(0xc035676c), - reference_timestamp: NtpTimestamp::from_fixed_int(0xe5f661fd6f165f03), - origin_timestamp: NtpTimestamp::from_fixed_int(0xe5f663a87619ef40), - receive_timestamp: NtpTimestamp::from_fixed_int(0xe5f663a8798c6581), - transmit_timestamp: NtpTimestamp::from_fixed_int(0xe5f663a8798eae2b), + reference_id: ReferenceId::from_int(0xc035_676c), + reference_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_61fd_6f16_5f03), + origin_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_63a8_7619_ef40), + receive_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_63a8_798c_6581), + transmit_timestamp: NtpTimestamp::from_fixed_int(0xe5f6_63a8_798e_ae2b), }), - efdata: Default::default(), + efdata: ExtensionFieldData::default(), mac: None, }; @@ -1824,7 +1877,7 @@ mod tests { assert_eq!( header.reference_timestamp, - NtpTimestamp::from_fixed_int(0x4E54503544524654) + NtpTimestamp::from_fixed_int(0x4E54_5035_4452_4654) ); } diff --git a/ntp-proto/src/packet/v5/extension_fields.rs b/ntp-proto/src/packet/v5/extension_fields.rs index 55fa43d5f..fb653b395 100644 --- a/ntp-proto/src/packet/v5/extension_fields.rs +++ b/ntp-proto/src/packet/v5/extension_fields.rs @@ -102,7 +102,7 @@ impl ReferenceIdRequest { Some(ReferenceIdResponse { bytes }) } - pub fn serialize(&self, mut writer: impl NonBlockingWrite) -> std::io::Result<()> { + pub fn serialize(self, mut writer: impl NonBlockingWrite) -> std::io::Result<()> { let payload_len = self.payload_len; let ef_len: u16 = payload_len + 4; @@ -136,11 +136,11 @@ impl ReferenceIdRequest { }) } - pub const fn offset(&self) -> u16 { + pub const fn offset(self) -> u16 { self.offset } - pub const fn payload_len(&self) -> u16 { + pub const fn payload_len(self) -> u16 { self.payload_len } } diff --git a/ntp-proto/src/packet/v5/mod.rs b/ntp-proto/src/packet/v5/mod.rs index 8d9aaa05a..3a49ba536 100644 --- a/ntp-proto/src/packet/v5/mod.rs +++ b/ntp-proto/src/packet/v5/mod.rs @@ -264,6 +264,7 @@ impl NtpHeaderV5 { const WIRE_LENGTH: usize = 48; const VERSION: u8 = 5; + #[allow(clippy::cast_possible_wrap)] pub(crate) fn deserialize( data: &[u8], ) -> Result<(Self, usize), ParsingError> { @@ -297,6 +298,8 @@ impl NtpHeaderV5 { )) } + #[allow(clippy::cast_sign_loss)] + #[allow(clippy::cast_possible_wrap)] #[allow(dead_code)] pub(crate) fn serialize(&self, mut w: impl NonBlockingWrite) -> std::io::Result<()> { w.write_all(&[(self.leap.to_bits() << 6) | (Self::VERSION << 3) | self.mode.to_bits()])?; @@ -493,11 +496,11 @@ mod tests { ); assert_eq!( parsed.receive_timestamp, - NtpTimestamp::from_fixed_int(0x1111111111111111) + NtpTimestamp::from_fixed_int(0x1111_1111_1111_1111) ); assert_eq!( parsed.transmit_timestamp, - NtpTimestamp::from_fixed_int(0x2222222222222222) + NtpTimestamp::from_fixed_int(0x2222_2222_2222_2222) ); let mut buffer: [u8; 48] = [0u8; 48]; @@ -507,6 +510,7 @@ mod tests { assert_eq!(data, buffer); } + #[allow(clippy::cast_possible_wrap)] #[test] fn test_encode_decode_roundtrip() { for i in 0..=u8::MAX { diff --git a/ntp-proto/src/packet/v5/server_reference_id.rs b/ntp-proto/src/packet/v5/server_reference_id.rs index 0511aeb7a..7539b8481 100644 --- a/ntp-proto/src/packet/v5/server_reference_id.rs +++ b/ntp-proto/src/packet/v5/server_reference_id.rs @@ -66,10 +66,12 @@ pub struct BloomFilter([u8; Self::BYTES]); impl BloomFilter { pub const BYTES: usize = 512; + #[must_use] pub const fn new() -> Self { Self([0; Self::BYTES]) } + #[must_use] pub fn contains_id(&self, other: &ServerId) -> bool { other.0.iter().all(|idx| self.is_set(*idx)) } @@ -96,10 +98,13 @@ impl BloomFilter { union } + #[allow(clippy::cast_possible_truncation)] + #[must_use] pub fn count_ones(&self) -> u16 { self.0.iter().map(|b| b.count_ones() as u16).sum() } + #[must_use] pub const fn as_bytes(&self) -> &[u8; Self::BYTES] { &self.0 } @@ -133,7 +138,7 @@ impl Debug for BloomFilter { .0 .chunks_exact(32) .map(|chunk| chunk.iter().fold(0, |acc, b| acc | b)) - .map(|b| char::from_u32(0x2800 + b as u32).unwrap()) + .map(|b| char::from_u32(0x2800 + u32::from(b)).unwrap()) .collect(); f.debug_tuple("BloomFilter").field(&str).finish() @@ -219,6 +224,7 @@ impl RemoteBloomFilter { Ok(()) } + #[allow(clippy::cast_possible_truncation)] fn advance_next_to_request(&mut self) { self.next_to_request = (self.next_to_request + self.chunk_size) % BloomFilter::BYTES as u16; @@ -236,7 +242,7 @@ impl Debug for RemoteBloomFilter { .field("last_requested", &self.last_requested) .field("next_to_request", &self.next_to_request) .field("is_filled", &self.is_filled) - .finish() + .finish_non_exhaustive() } } @@ -316,6 +322,7 @@ mod tests { } } + #[allow(clippy::cast_possible_truncation)] #[test] fn requesting() { use ResponseHandlingError::{MismatchedCookie, MismatchedLength, NotAwaitingResponse}; @@ -383,7 +390,7 @@ mod tests { continue; }; - for _chunk in 0..((512 / chunk_size) + 1) { + for _chunk in 0..=(512 / chunk_size) { let cookie = NtpClientCookie::new_random(); let request = bf.next_request(cookie); let response = request.to_response(&target_filter).unwrap(); diff --git a/ntp-proto/src/server.rs b/ntp-proto/src/server.rs index b2bd98172..84e5bc4a6 100644 --- a/ntp-proto/src/server.rs +++ b/ntp-proto/src/server.rs @@ -93,7 +93,7 @@ pub struct Server { // Quick estimation of ntp packet message version without doing full parsing fn fallback_message_version(message: &[u8]) -> u8 { - message.first().map(|v| (v & 0b0011_1000) >> 3).unwrap_or(0) + message.first().map_or(0, |v| (v & 0b0011_1000) >> 3) } impl Server { @@ -169,6 +169,7 @@ impl Server { /// If the buffer isn't large enough to encode the reply, this /// will log an error and ignore the incoming packet. A buffer /// as large as the message will always suffice. + #[allow(clippy::cast_possible_truncation)] pub fn handle<'a>( &mut self, client_ip: IpAddr, @@ -216,10 +217,10 @@ impl Server { if non_nts_action == FilterAction::Ignore { stats_handler.register(version, nts, ServerReason::Policy, ServerResponse::Ignore); return ServerAction::Ignore; - } else { - action = ServerResponse::Deny; - reason = ServerReason::Policy; } + + action = ServerResponse::Deny; + reason = ServerReason::Policy; } let mut cursor = Cursor::new(buffer); @@ -261,7 +262,7 @@ impl Server { ServerResponse::Ignore => unreachable!(), }; match result { - Ok(_) => { + Ok(()) => { stats_handler.register(version, nts, reason, action); let length = cursor.position(); ServerAction::Respond { @@ -312,6 +313,7 @@ impl TimestampedCache { } } + #[allow(clippy::cast_possible_truncation)] fn index(&self, item: &T) -> usize { use std::hash::{BuildHasher, Hasher}; @@ -407,6 +409,7 @@ impl<'de> Deserialize<'de> for IpSubnet { } } +#[allow(clippy::too_many_lines)] #[cfg(test)] mod tests { use std::net::{Ipv4Addr, Ipv6Addr}; @@ -479,6 +482,7 @@ mod tests { } } + #[allow(clippy::cast_possible_truncation)] fn serialize_packet_unencrypted(send_packet: &NtpPacket) -> Vec { let mut buf = vec![0; 1024]; let mut cursor = Cursor::new(buf.as_mut_slice()); @@ -489,6 +493,7 @@ mod tests { buf } + #[allow(clippy::cast_possible_truncation)] fn serialize_packet_encrypted(send_packet: &NtpPacket, key: &dyn Cipher) -> Vec { let mut buf = vec![0; 1024]; let mut cursor = Cursor::new(buf.as_mut_slice()); diff --git a/ntp-proto/src/source.rs b/ntp-proto/src/source.rs index c27a8cade..b38c1e0c8 100644 --- a/ntp-proto/src/source.rs +++ b/ntp-proto/src/source.rs @@ -43,6 +43,7 @@ impl SourceNtsData { self.cookies.get() } + #[must_use] pub fn get_keys(self) -> (Box, Box) { (self.c2s, self.s2c) } @@ -52,7 +53,7 @@ impl std::fmt::Debug for SourceNtsData { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("SourceNtsData") .field("cookies", &self.cookies) - .finish() + .finish_non_exhaustive() } } @@ -109,7 +110,7 @@ impl> OneWaySource bool { self.0 != 0 } @@ -194,6 +196,7 @@ impl Reach { } /// Number of polls since the last message we received + #[must_use] pub fn unanswered_polls(&self) -> u32 { self.0.trailing_zeros() } @@ -237,13 +240,16 @@ pub struct NtpSourceSnapshot { } impl NtpSourceSnapshot { + /// # Errors + /// + /// Returns `AcceptSynchronizationError` if source is rejected. pub fn accept_synchronization( &self, local_stratum: u8, local_ips: &[IpAddr], #[cfg_attr(not(feature = "ntpv5"), allow(unused_variables))] system: &SystemSnapshot, ) -> Result<(), AcceptSynchronizationError> { - use AcceptSynchronizationError::*; + use AcceptSynchronizationError::{Loop, ServerUnreachable, Stratum}; if self.stratum >= local_stratum { debug!( @@ -304,6 +310,7 @@ impl NtpSourceSnapshot { } #[cfg(feature = "__internal-test")] +#[must_use] pub fn source_snapshot() -> NtpSourceSnapshot { use std::net::Ipv4Addr; @@ -318,7 +325,7 @@ pub fn source_snapshot() -> NtpSourceSnapshot { reach, poll_interval: crate::time_types::PollIntervalLimits::default().min, - protocol_version: Default::default(), + protocol_version: ProtocolVersion::default(), #[cfg(feature = "ntpv5")] bloom_filter: None, } @@ -347,6 +354,7 @@ pub enum ProtocolVersion { } impl ProtocolVersion { + #[must_use] pub fn is_expected_incoming_version(&self, incoming_version: u8) -> bool { match self { ProtocolVersion::V4 => incoming_version == 4 || incoming_version == 3, @@ -395,6 +403,7 @@ impl Clone for NtpSourceUpdate { #[cfg(feature = "__internal-test")] impl NtpSourceUpdate { + #[must_use] pub fn snapshot(snapshot: NtpSourceSnapshot) -> Self { NtpSourceUpdate { snapshot, @@ -485,7 +494,7 @@ impl> NtpSource> NtpSource NtpSourceActionIterator { if !self.reach.is_reachable() && self.tries >= STARTUP_TRIES_THRESHOLD { @@ -726,13 +739,13 @@ impl> NtpSource> NtpSource Some(response), _ => None, }) - .next() } else { message .untrusted_extension_fields() - .filter_map(|ef| match ef { + .find_map(|ef| match ef { ExtensionField::ReferenceIdResponse(response) => Some(response), _ => None, }) - .next() }; if let Some(ref_id) = bloom_responses { @@ -785,14 +796,13 @@ impl> NtpSource> NtpSource> NtpSource { impl> System { + /// # Errors + /// + /// Returns `NtpClock::Error` if the `Controller` can't be created. pub fn new( clock: Controller::Clock, synchronization_config: SynchronizationConfig, @@ -221,7 +224,7 @@ impl Result<(), ::Error> { self.ensure_controller_control() } + /// # Errors + /// + /// Returns `NtpClock::Error` if controlling the `Controller` fails. fn ensure_controller_control(&mut self) -> Result<(), ::Error> { if !self.controller_took_control { self.controller.take_control()?; @@ -248,6 +257,9 @@ impl Self { Self { instant: Instant::now(), } } + #[must_use] pub fn abs_diff(self, rhs: Self) -> NtpDuration { // our code should always give the bigger argument first. debug_assert!( @@ -40,6 +46,7 @@ impl NtpInstant { NtpDuration::from_system_duration(duration) } + #[must_use] pub fn elapsed(&self) -> std::time::Duration { self.instant.elapsed() } @@ -55,7 +62,7 @@ impl Add for NtpInstant { } } -/// NtpTimestamp represents an ntp timestamp without the era number. +/// `NtpTimestamp` represents an ntp timestamp without the era number. #[derive(Copy, Clone, Eq, PartialEq, PartialOrd, Ord, Default, Serialize, Deserialize)] pub struct NtpTimestamp { timestamp: u64, @@ -82,6 +89,7 @@ impl NtpTimestamp { /// Create an NTP timestamp from the number of seconds and nanoseconds that have /// passed since the last ntp era boundary. + #[must_use] pub const fn from_seconds_nanos_since_ntp_era(seconds: u32, nanos: u32) -> Self { // Although having a valid interpretation, providing more // than 1 second worth of nanoseconds as input probably @@ -98,6 +106,7 @@ impl NtpTimestamp { NtpTimestamp::from_bits(timestamp.to_be_bytes()) } + #[must_use] pub fn is_before(self, other: NtpTimestamp) -> bool { // Around an era change, self can be near the maximum value // for NtpTimestamp and other near the minimum, and that must @@ -186,7 +195,7 @@ impl SubAssign for NtpTimestamp { } } -/// NtpDuration is used to represent signed intervals between NtpTimestamps. +/// `NtpDuration` is used to represent signed intervals between `NtpTimestamps`. /// A negative duration interval is interpreted to mean that the first /// timestamp used to define the interval represents a point in time after /// the second timestamp. @@ -227,11 +236,12 @@ impl NtpDuration { // Although saturating is safe to do, it probably still // should never happen in practice, so ensure we will // see it when running in debug mode. - debug_assert!(self.duration <= 0x0000FFFFFFFFFFFF); + debug_assert!(self.duration <= 0x0000_FFFF_FFFF_FFFF); - match self.duration > 0x0000FFFFFFFFFFFF { - true => 0xFFFFFFFF_u32, - false => ((self.duration & 0x0000FFFFFFFF0000) >> 16) as u32, + if self.duration > 0x0000_FFFF_FFFF_FFFF { + 0xFFFF_FFFF_u32 + } else { + ((self.duration & 0x0000_FFFF_FFFF_0000) >> 16) as u32 } .to_be_bytes() } @@ -259,11 +269,14 @@ impl NtpDuration { /// Convert to an f64; required for statistical calculations /// (e.g. in clock filtering) + #[allow(clippy::cast_precision_loss)] + #[must_use] pub fn to_seconds(self) -> f64 { // dividing by u32::MAX moves the decimal point to the right position - self.duration as f64 / u32::MAX as f64 + self.duration as f64 / f64::from(u32::MAX) } + #[must_use] pub fn from_seconds(seconds: f64) -> Self { debug_assert!(!(seconds.is_nan() || seconds.is_infinite())); @@ -272,11 +285,9 @@ impl NtpDuration { // Ensure proper saturating behaviour let duration = match i as i64 { - i if i >= i32::MIN as i64 && i <= i32::MAX as i64 => { - (i << 32) | (f * u32::MAX as f64) as i64 - } - i if i < i32::MIN as i64 => i64::MIN, - i if i > i32::MAX as i64 => i64::MAX, + i if i32::try_from(i).is_ok() => (i << 32) | (f * f64::from(u32::MAX)) as i64, + i if i < i64::from(i32::MIN) => i64::MIN, + i if i > i64::from(i32::MAX) => i64::MAX, _ => unreachable!(), }; @@ -284,6 +295,7 @@ impl NtpDuration { } /// Interval of same length, but positive direction + #[must_use] pub const fn abs(self) -> Self { Self { duration: self.duration.abs(), @@ -291,6 +303,7 @@ impl NtpDuration { } /// Interval of same length, but positive direction + #[must_use] pub fn abs_diff(self, other: Self) -> Self { (self - other).abs() } @@ -299,14 +312,16 @@ impl NtpDuration { /// (second return value) representing the length of this duration. /// The number of nanoseconds is guaranteed to be positive and less /// than 10^9 + #[must_use] pub const fn as_seconds_nanos(self) -> (i32, u32) { ( (self.duration >> 32) as i32, - (((self.duration & 0xFFFFFFFF) * 1_000_000_000) >> 32) as u32, + (((self.duration & 0xFFFF_FFFF) * 1_000_000_000) >> 32) as u32, ) } - /// Interpret an exponent `k` as `2^k` seconds, expressed as an NtpDuration + /// Interpret an exponent `k` as `2^k` seconds, expressed as an `NtpDuration` + #[must_use] pub const fn from_exponent(input: i8) -> Self { Self { duration: match input { @@ -318,7 +333,8 @@ impl NtpDuration { } } - /// calculate the log2 (floored) of the duration in seconds (i8::MIN if 0) + /// calculate the log2 (floored) of the duration in seconds (`i8::MIN` if 0) + #[must_use] pub fn log2(self) -> i8 { if self == NtpDuration::ZERO { return i8::MIN; @@ -327,6 +343,7 @@ impl NtpDuration { 31 - (self.duration.leading_zeros() as i8) } + #[must_use] pub fn from_system_duration(duration: Duration) -> Self { let seconds = duration.as_secs(); let nanos = duration.subsec_nanos(); @@ -336,7 +353,7 @@ impl NtpDuration { debug_assert!(nanos < 1_000_000_000); // NTP uses 1/2^32 sec as its unit of fractional time. // our time is in nanoseconds, so 1/1e9 seconds - let fraction = ((nanos as u64) << 32) / 1_000_000_000; + let fraction = (u64::from(nanos) << 32) / 1_000_000_000; // alternatively, abuse FP arithmetic to save an instruction // let fraction = (nanos as f64 * 4.294967296) as u64; @@ -551,7 +568,7 @@ impl Default for PollIntervalLimits { impl std::fmt::Debug for PollInterval { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "PollInterval({} s)", 2.0_f64.powf(self.0 as _)) + write!(f, "PollInterval({} s)", 2.0_f64.powf(f64::from(self.0))) } } @@ -559,14 +576,17 @@ impl PollInterval { pub const NEVER: PollInterval = PollInterval(i8::MAX); #[cfg(test)] + #[must_use] pub fn test_new(value: i8) -> Self { Self(value) } + #[must_use] pub fn from_byte(value: u8) -> Self { Self(value as i8) } + #[must_use] pub fn as_byte(self) -> u8 { self.0 as u8 } @@ -586,16 +606,19 @@ impl PollInterval { Self(self.0 - 1).max(limits.min) } + #[must_use] pub const fn as_log(self) -> i8 { self.0 } + #[must_use] pub const fn as_duration(self) -> NtpDuration { NtpDuration { duration: 1 << (self.0 + 32), } } + #[must_use] pub const fn as_system_duration(self) -> Duration { Duration::from_secs(1 << self.0) } @@ -624,6 +647,7 @@ impl<'de> Deserialize<'de> for FrequencyTolerance { } impl FrequencyTolerance { + #[must_use] pub const fn ppm(ppm: u32) -> Self { Self { ppm } } @@ -637,6 +661,7 @@ impl Mul for NtpDuration { } } +#[allow(clippy::missing_panics_doc)] #[cfg(feature = "__internal-fuzz")] pub fn fuzz_duration_from_seconds(v: f64) { if v.is_finite() { @@ -660,7 +685,7 @@ mod tests { #[test] fn test_timestamp_era_change() { let mut a = NtpTimestamp::from_fixed_int(1); - let b = NtpTimestamp::from_fixed_int(0xFFFFFFFFFFFFFFFF); + let b = NtpTimestamp::from_fixed_int(0xFFFF_FFFF_FFFF_FFFF); assert_eq!(a - b, NtpDuration::from_fixed_int(2)); assert_eq!(b - a, NtpDuration::from_fixed_int(-2)); @@ -681,7 +706,7 @@ mod tests { fn test_timestamp_from_seconds_nanos() { assert_eq!( NtpTimestamp::from_seconds_nanos_since_ntp_era(0, 500_000_000), - NtpTimestamp::from_fixed_int(0x80000000) + NtpTimestamp::from_fixed_int(0x8000_0000) ); assert_eq!( NtpTimestamp::from_seconds_nanos_since_ntp_era(1, 0), @@ -704,7 +729,7 @@ mod tests { #[test] fn test_duration_as_seconds_nanos() { assert_eq!( - NtpDuration::from_fixed_int(0x80000000).as_seconds_nanos(), + NtpDuration::from_fixed_int(0x8000_0000).as_seconds_nanos(), (0, 500_000_000) ); assert_eq!( @@ -797,7 +822,7 @@ mod tests { assert!(NtpDuration::from_exponent(i) > NtpDuration::from_fixed_int(0)); } for i in -128..-32 { - NtpDuration::from_exponent(i); // should not crash + let _ = NtpDuration::from_exponent(i); // should not crash } } diff --git a/ntp-proto/src/tls_utils.rs b/ntp-proto/src/tls_utils.rs index fe576fe34..a3062e5ee 100644 --- a/ntp-proto/src/tls_utils.rs +++ b/ntp-proto/src/tls_utils.rs @@ -1,13 +1,13 @@ #[cfg(feature = "rustls23")] mod rustls23_shim { - /// The intent of this ClientCertVerifier is that it accepts any connections that are either + /// The intent of this `ClientCertVerifier` is that it accepts any connections that are either /// a.) not presenting a client certificate /// b.) are presenting a well-formed, but otherwise not checked (against a trust root) client certificate /// - /// This is because RusTLS apparently doesn't accept every kind of self-signed certificate. + /// This is because `RusTLS` apparently doesn't accept every kind of self-signed certificate. /// - /// The only goal of this ClientCertVerifier is to achieve that, if a client presents a TLS certificate, - /// this certificate shows up in the .peer_certificates() for that connection. + /// The only goal of this `ClientCertVerifier` is to achieve that, if a client presents a TLS certificate, + /// this certificate shows up in the `.peer_certificates()` for that connection. #[cfg(feature = "nts-pool")] #[derive(Debug)] pub struct AllowAnyAnonymousOrCertificateBearingClient { @@ -23,6 +23,7 @@ mod rustls23_shim { #[cfg(feature = "nts-pool")] impl AllowAnyAnonymousOrCertificateBearingClient { + #[must_use] pub fn new(provider: &CryptoProvider) -> Self { AllowAnyAnonymousOrCertificateBearingClient { supported_algs: provider.signature_verification_algorithms, @@ -93,6 +94,7 @@ mod rustls23_shim { pub use rustls_pemfile2::pkcs8_private_keys; pub use rustls_pemfile2::private_key; + #[must_use] pub fn rootstore_ref_shim(cert: &super::Certificate) -> super::Certificate { cert.clone() } @@ -100,22 +102,26 @@ mod rustls23_shim { pub trait CloneKeyShim {} + #[must_use] pub fn client_config_builder( ) -> rustls23::ConfigBuilder { ClientConfig::builder() } + #[must_use] pub fn client_config_builder_with_protocol_versions( versions: &[&'static rustls23::SupportedProtocolVersion], ) -> rustls23::ConfigBuilder { ClientConfig::builder_with_protocol_versions(versions) } + #[must_use] pub fn server_config_builder( ) -> rustls23::ConfigBuilder { ServerConfig::builder() } + #[must_use] pub fn server_config_builder_with_protocol_versions( versions: &[&'static rustls23::SupportedProtocolVersion], ) -> rustls23::ConfigBuilder { @@ -276,11 +282,11 @@ mod rustls21_shim { ) -> Result, std::io::Error> { for item in std::iter::from_fn(|| rustls_pemfile1::read_one(rd).transpose()) { match item { - Ok(rustls_pemfile1::Item::RSAKey(key)) - | Ok(rustls_pemfile1::Item::PKCS8Key(key)) - | Ok(rustls_pemfile1::Item::ECKey(key)) => { - return Ok(Some(super::PrivateKey(key))) - } + Ok( + rustls_pemfile1::Item::RSAKey(key) + | rustls_pemfile1::Item::PKCS8Key(key) + | rustls_pemfile1::Item::ECKey(key), + ) => return Ok(Some(super::PrivateKey(key))), Err(e) => return Err(e), _ => {} } diff --git a/ntpd/src/ctl.rs b/ntpd/src/ctl.rs index 871f893a2..8d2f361f1 100644 --- a/ntpd/src/ctl.rs +++ b/ntpd/src/ctl.rs @@ -46,11 +46,6 @@ pub enum NtpCtlAction { pub(crate) struct NtpCtlOptions { config: Option, format: Format, - help: bool, - version: bool, - validate: bool, - status: bool, - force_sync: bool, action: NtpCtlAction, } @@ -77,10 +72,10 @@ impl NtpCtlOptions { match arg { CliArg::Flag(flag) => match flag.as_str() { "-h" | "--help" => { - options.help = true; + options.action = NtpCtlAction::Help; } "-v" | "--version" => { - options.version = true; + options.action = NtpCtlAction::Version; } option => { Err(format!("invalid option provided: {option}"))?; @@ -101,18 +96,18 @@ impl NtpCtlOptions { }, CliArg::Rest(rest) => { if rest.len() > 1 { - eprintln!("Warning: Too many commands provided.") + eprintln!("Warning: Too many commands provided."); } for command in rest { match command.as_str() { "validate" => { - options.validate = true; + options.action = NtpCtlAction::Validate; } "status" => { - options.status = true; + options.action = NtpCtlAction::Status; } "force-sync" => { - options.force_sync = true; + options.action = NtpCtlAction::ForceSync; } unknown => { eprintln!("Warning: Unknown command {unknown}"); @@ -123,51 +118,36 @@ impl NtpCtlOptions { } } - options.resolve_action(); // nothing to validate at the moment Ok(options) } - - /// from the arguments resolve which action should be performed - fn resolve_action(&mut self) { - if self.help { - self.action = NtpCtlAction::Help; - } else if self.version { - self.action = NtpCtlAction::Version; - } else if self.validate { - self.action = NtpCtlAction::Validate; - } else if self.status { - self.action = NtpCtlAction::Status; - } else if self.force_sync { - self.action = NtpCtlAction::ForceSync; - } else { - self.action = NtpCtlAction::Help; - } - } } -fn validate(config: Option) -> std::io::Result { +fn validate(config: Option<&PathBuf>) -> ExitCode { // Late completion not needed, so ignore result. crate::daemon::tracing::tracing_init(LogLevel::Info, true).init(); - match Config::from_args(config, vec![], vec![]) { + match Config::from_args(config.as_ref(), vec![], vec![]) { Ok(config) => { if config.check() { eprintln!("Config looks good"); - Ok(ExitCode::SUCCESS) + ExitCode::SUCCESS } else { - Ok(ExitCode::FAILURE) + ExitCode::FAILURE } } Err(e) => { eprintln!("Error: Could not load configuration: {e}"); - Ok(ExitCode::FAILURE) + ExitCode::FAILURE } } } const VERSION: &str = env!("CARGO_PKG_VERSION"); +/// # Errors +/// +/// Returns 'Error' if arguments to program are invalid. pub fn main() -> std::io::Result { let options = match NtpCtlOptions::try_parse_from(std::env::args()) { Ok(options) => options, @@ -183,10 +163,10 @@ pub fn main() -> std::io::Result { eprintln!("ntp-ctl {VERSION}"); Ok(ExitCode::SUCCESS) } - NtpCtlAction::Validate => validate(options.config), - NtpCtlAction::ForceSync => force_sync::force_sync(options.config), + NtpCtlAction::Validate => Ok(validate(options.config.as_ref())), + NtpCtlAction::ForceSync => force_sync::force_sync(options.config.as_ref()), NtpCtlAction::Status => { - let config = Config::from_args(options.config, vec![], vec![]); + let config = Config::from_args(options.config.as_ref(), vec![], vec![]); if let Err(ref e) = config { println!("Warning: Unable to load configuration file: {e}"); @@ -273,7 +253,7 @@ async fn print_state(print: Format, observe_socket: PathBuf) -> Result std::io::Result> { - let config: ObservabilityConfig = Default::default(); + let config = ObservabilityConfig::default(); // be careful with copying: tests run concurrently and should use a unique socket name! let path = std::env::temp_dir().join(format!("ntp-test-stream-{}", alloc_port())); @@ -356,8 +339,8 @@ mod tests { #[tokio::test] async fn test_control_socket_source() -> std::io::Result<()> { let value = ObservableState { - program: Default::default(), - system: Default::default(), + program: ProgramData::default(), + system: SystemSnapshot::default(), sources: vec![], servers: vec![], }; @@ -374,8 +357,8 @@ mod tests { #[tokio::test] async fn test_control_socket_prometheus() -> std::io::Result<()> { let value = ObservableState { - program: Default::default(), - system: Default::default(), + program: ProgramData::default(), + system: SystemSnapshot::default(), sources: vec![], servers: vec![], }; diff --git a/ntpd/src/daemon/clock.rs b/ntpd/src/daemon/clock.rs index 3814b95e9..01aad568c 100644 --- a/ntpd/src/daemon/clock.rs +++ b/ntpd/src/daemon/clock.rs @@ -40,6 +40,8 @@ impl NtpClock for NtpClockWrapper { offset: ntp_proto::NtpDuration, ) -> Result { let (seconds, nanos) = offset.as_seconds_nanos(); + + #[allow(clippy::cast_lossless)] self.0 .step_clock(TimeOffset { seconds: seconds as _, diff --git a/ntpd/src/daemon/config/mod.rs b/ntpd/src/daemon/config/mod.rs index 75640b874..9ad21e264 100644 --- a/ntpd/src/daemon/config/mod.rs +++ b/ntpd/src/daemon/config/mod.rs @@ -83,13 +83,13 @@ impl CliArg { if let Some((key, value)) = long_arg.split_once('=') { if takes_argument.contains(&key) { - processed.push(CliArg::Argument(key.to_string(), value.to_string())) + processed.push(CliArg::Argument(key.to_string(), value.to_string())); } else { - invalid? + invalid?; } } else if takes_argument.contains(&long_arg) { if let Some(next) = arg_iter.next() { - processed.push(CliArg::Argument(long_arg.to_string(), next)) + processed.push(CliArg::Argument(long_arg.to_string(), next)); } else { Err(format!("'{}' expects an argument", &long_arg))?; } @@ -116,12 +116,12 @@ impl CliArg { // short version of --help has no arguments processed.push(CliArg::Flag(flag)); } else { - Err(format!("'-{}' expects an argument", char))?; + Err(format!("'-{char}' expects an argument"))?; } break; - } else { - processed.push(CliArg::Flag(flag)); } + + processed.push(CliArg::Flag(flag)); } } _argument => rest.push(arg), @@ -225,10 +225,10 @@ where #[cfg(not(target_os = "linux"))] panic!("Custom clock paths not supported on this platform"); - } else { - tracing::debug!("using REALTIME clock"); - Ok(NtpClockWrapper::new(UnixClock::CLOCK_REALTIME)) } + + tracing::debug!("using REALTIME clock"); + Ok(NtpClockWrapper::new(UnixClock::CLOCK_REALTIME)) } fn deserialize_interface<'de, D>(deserializer: D) -> Result, D::Error> @@ -265,7 +265,9 @@ pub enum TimestampMode { impl TimestampMode { #[cfg(target_os = "linux")] pub(crate) fn as_interface_mode(self) -> timestamped_socket::socket::InterfaceTimestampMode { - use timestamped_socket::socket::InterfaceTimestampMode::*; + use timestamped_socket::socket::InterfaceTimestampMode::{ + HardwareAll, None, SoftwareAll, SoftwareRecv, + }; match self { TimestampMode::Software => None, TimestampMode::KernelRecv => SoftwareRecv, @@ -276,7 +278,7 @@ impl TimestampMode { #[cfg(any(target_os = "linux", target_os = "freebsd"))] pub(crate) fn as_general_mode(self) -> timestamped_socket::socket::GeneralTimestampMode { - use timestamped_socket::socket::GeneralTimestampMode::*; + use timestamped_socket::socket::GeneralTimestampMode::{None, SoftwareAll, SoftwareRecv}; match self { TimestampMode::Software => None, TimestampMode::KernelRecv => SoftwareRecv, @@ -285,6 +287,8 @@ impl TimestampMode { } #[cfg(not(any(target_os = "linux", target_os = "freebsd")))] + #[allow(clippy::unused_self)] + #[allow(clippy::enum_glob_use)] pub(crate) fn as_general_mode(self) -> timestamped_socket::socket::GeneralTimestampMode { use timestamped_socket::socket::GeneralTimestampMode::*; None @@ -319,9 +323,9 @@ pub struct ObservabilityConfig { impl Default for ObservabilityConfig { fn default() -> Self { Self { - log_level: Default::default(), + log_level: Option::default(), ansi_colors: default_ansi_colors(), - observation_path: Default::default(), + observation_path: Option::default(), observation_permissions: default_observation_permissions(), metrics_exporter_listen: default_metrics_exporter_listen(), } @@ -377,6 +381,7 @@ impl Config { let meta = std::fs::metadata(&file)?; let perm = meta.permissions(); + #[allow(clippy::cast_possible_truncation)] if perm.mode() as libc::mode_t & libc::S_IWOTH != 0 { warn!("Unrestricted config file permissions: Others can write."); } @@ -411,7 +416,7 @@ impl Config { } pub fn from_args( - file: Option>, + file: Option<&impl AsRef>, sources: Vec, servers: Vec, ) -> Result { @@ -527,6 +532,7 @@ mod tests { use super::*; #[test] + #[allow(clippy::too_many_lines)] fn test_config() { let config: Config = toml::from_str("[[source]]\nmode = \"server\"\naddress = \"example.com\"").unwrap(); @@ -718,18 +724,18 @@ mod tests { #[test] fn system_config_accumulated_threshold() { let config: Result = toml::from_str( - r#" + r" accumulated-step-panic-threshold = 0 - "#, + ", ); let config = config.unwrap(); assert!(config.accumulated_step_panic_threshold.is_none()); let config: Result = toml::from_str( - r#" + r" accumulated-step-panic-threshold = 1000 - "#, + ", ); let config = config.unwrap(); @@ -742,9 +748,9 @@ mod tests { #[test] fn system_config_startup_panic_threshold() { let config: Result = toml::from_str( - r#" + r" startup-step-panic-threshold = { forward = 10, backward = 20 } - "#, + ", ); let config = config.unwrap(); @@ -767,9 +773,9 @@ mod tests { } let result: Result = toml::from_str( - r#" + r" duration = nan - "#, + ", ); let error = result.unwrap_err(); @@ -785,9 +791,9 @@ mod tests { } let result: Result = toml::from_str( - r#" + r" threshold = nan - "#, + ", ); let error = result.unwrap_err(); @@ -797,9 +803,9 @@ mod tests { #[test] fn deny_unknown_fields() { let config: Result = toml::from_str( - r#" + r" unknown-field = 42 - "#, + ", ); let error = config.unwrap_err(); @@ -826,24 +832,26 @@ mod tests { #[test] fn daemon_synchronization_config() { let config: Result = toml::from_str( - r#" + r" does_not_exist = 5 - "#, + ", ); assert!(config.is_err()); let config: Result = toml::from_str( - r#" + r" minimum-agreeing-sources = 2 [algorithm] initial-wander = 1e-7 - "#, + ", ); let config = config.unwrap(); assert_eq!(config.synchronization_base.minimum_agreeing_sources, 2); - assert_eq!(config.algorithm.initial_wander, 1e-7); + + // see https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp + assert!((config.algorithm.initial_wander - 1e-7).abs() < 1e-8); } } diff --git a/ntpd/src/daemon/config/ntp_source.rs b/ntpd/src/daemon/config/ntp_source.rs index d7f7eb271..135c08a69 100644 --- a/ntpd/src/daemon/config/ntp_source.rs +++ b/ntpd/src/daemon/config/ntp_source.rs @@ -322,6 +322,7 @@ impl NormalizedAddress { } #[cfg(test)] + #[allow(clippy::unused_async)] pub async fn lookup_host(&self) -> std::io::Result + '_> { // We don't want to spam a real DNS server during testing. This is an attempt to randomize // the returned addresses somewhat. @@ -374,7 +375,7 @@ mod tests { NtpSourceConfig::Pool(c) => c.addr.to_string(), #[cfg(feature = "unstable_nts-pool")] NtpSourceConfig::NtsPool(c) => c.addr.to_string(), - NtpSourceConfig::Sock(_c) => "".to_string(), + NtpSourceConfig::Sock(_c) => String::new(), } } @@ -475,15 +476,15 @@ mod tests { #[test] fn test_deserialize_source_pem_certificate() { - let contents = include_bytes!("../../../testdata/certificates/nos-nl.pem"); - let path = std::env::temp_dir().join("nos-nl.pem"); - std::fs::write(&path, contents).unwrap(); - #[derive(Deserialize, Debug)] struct TestConfig { source: NtpSourceConfig, } + let contents = include_bytes!("../../../testdata/certificates/nos-nl.pem"); + let path = std::env::temp_dir().join("nos-nl.pem"); + std::fs::write(&path, contents).unwrap(); + let test: TestConfig = toml::from_str(&format!( r#" [source] diff --git a/ntpd/src/daemon/config/server.rs b/ntpd/src/daemon/config/server.rs index bde265609..3c4d43afa 100644 --- a/ntpd/src/daemon/config/server.rs +++ b/ntpd/src/daemon/config/server.rs @@ -130,7 +130,7 @@ impl TryFrom<&str> for ServerConfig { denylist: default_denylist(), allowlist: default_allowlist(), rate_limiting_cache_size: Default::default(), - rate_limiting_cutoff: Default::default(), + rate_limiting_cutoff: Duration::default(), require_nts: None, }) } @@ -144,7 +144,7 @@ impl From for ServerConfig { denylist: default_denylist(), allowlist: default_allowlist(), rate_limiting_cache_size: Default::default(), - rate_limiting_cutoff: Default::default(), + rate_limiting_cutoff: Duration::default(), require_nts: None, } } @@ -284,7 +284,7 @@ mod tests { ) .unwrap(); - assert_ne!(test.keyset, KeysetConfig::default()) + assert_ne!(test.keyset, KeysetConfig::default()); } #[test] diff --git a/ntpd/src/daemon/keyexchange.rs b/ntpd/src/daemon/keyexchange.rs index 9397342d6..62cf8bc41 100644 --- a/ntpd/src/daemon/keyexchange.rs +++ b/ntpd/src/daemon/keyexchange.rs @@ -131,8 +131,7 @@ async fn run_nts_ke( for client_cert in &nts_ke_config.authorized_pool_server_certificates { let pool_certificate_file = std::fs::File::open(client_cert).map_err(|e| { io_error(&format!( - "error reading authorized-pool-server-certificate at `{:?}`: {:?}", - client_cert, e + "error reading authorized-pool-server-certificate at `{client_cert:?}`: {e:?}" )) })?; let mut certs: Vec<_> = ntp_proto::tls_utils::pemfile::certs(&mut std::io::BufReader::new( @@ -141,11 +140,10 @@ async fn run_nts_ke( .collect::>>()?; // forbid certificate chains at this point if certs.len() == 1 { - pool_certs.push(certs.pop().unwrap()) + pool_certs.push(certs.pop().unwrap()); } else { return Err(io_error(&format!( - "pool certificate file at `{:?}` should contain exactly one certificate", - client_cert + "pool certificate file at `{client_cert:?}` should contain exactly one certificate" ))); } } @@ -224,12 +222,7 @@ async fn key_exchange_server( debug!("Potential client-triggered accept error in NTS-KE: {}", e); continue; } - Err(e) - if matches!( - e.raw_os_error(), - Some(ENFILE) | Some(EMFILE) | Some(ENOMEM) | Some(ENOBUFS) - ) => - { + Err(e) if matches!(e.raw_os_error(), Some(ENFILE | EMFILE | ENOMEM | ENOBUFS)) => { error!("Out of resources in NTS-KE, consider raising limits or lowering max parallel connections: {}", e); tokio::time::sleep(timeout).await; continue; @@ -944,11 +937,10 @@ mod tests { let error = result.unwrap_err(); - match error { - KeyExchangeError::Io(error) => { - assert_eq!(error.kind(), std::io::ErrorKind::ConnectionRefused); - } - _ => panic!(), + if let KeyExchangeError::Io(error) = error { + assert_eq!(error.kind(), std::io::ErrorKind::ConnectionRefused); + } else { + panic!() } } diff --git a/ntpd/src/daemon/mod.rs b/ntpd/src/daemon/mod.rs index 510783378..666e6e639 100644 --- a/ntpd/src/daemon/mod.rs +++ b/ntpd/src/daemon/mod.rs @@ -29,6 +29,9 @@ use self::tracing::LogLevel; const VERSION: &str = env!("CARGO_PKG_VERSION"); +/// # Errors +/// +/// Returns 'Error' if arguments to program are invalid. pub fn main() -> Result<(), Box> { let options = NtpDaemonOptions::try_parse_from(std::env::args())?; @@ -39,7 +42,7 @@ pub fn main() -> Result<(), Box> { config::NtpDaemonAction::Version => { eprintln!("ntp-daemon {VERSION}"); } - config::NtpDaemonAction::Run => run(options)?, + config::NtpDaemonAction::Run => run(&options)?, } Ok(()) @@ -49,13 +52,13 @@ pub fn main() -> Result<(), Box> { // log level based on the config if required. pub(crate) fn initialize_logging_parse_config( initial_log_level: Option, - config_path: Option, + config_path: Option<&PathBuf>, ) -> Config { let mut log_level = initial_log_level.unwrap_or_default(); let config_tracing = crate::daemon::tracing::tracing_init(log_level, true); let config = ::tracing::subscriber::with_default(config_tracing, || { - match Config::from_args(config_path, vec![], vec![]) { + match Config::from_args(config_path.as_ref(), vec![], vec![]) { Ok(c) => c, Err(e) => { // print to stderr because tracing is not yet setup @@ -78,8 +81,8 @@ pub(crate) fn initialize_logging_parse_config( config } -fn run(options: NtpDaemonOptions) -> Result<(), Box> { - let config = initialize_logging_parse_config(options.log_level, options.config); +fn run(options: &NtpDaemonOptions) -> Result<(), Box> { + let config = initialize_logging_parse_config(options.log_level, options.config.as_ref()); let runtime = if config.servers.is_empty() && config.nts_ke.is_empty() { Builder::new_current_thread().enable_all().build()? diff --git a/ntpd/src/daemon/ntp_source.rs b/ntpd/src/daemon/ntp_source.rs index 34b4de1fc..6c2a7aa07 100644 --- a/ntpd/src/daemon/ntp_source.rs +++ b/ntpd/src/daemon/ntp_source.rs @@ -84,13 +84,25 @@ enum SocketResult { Abort, } +#[allow(clippy::large_enum_variant)] +enum SelectResult { + Timer, + Recv(Result, std::io::Error>), + SystemUpdate( + Result< + SystemSourceUpdate, + tokio::sync::broadcast::error::RecvError, + >, + ), +} + impl, T> SourceTask where C: 'static + NtpClock + Send + Sync, T: Wait, { - async fn setup_socket(&mut self) -> SocketResult { + fn setup_socket(&mut self) -> SocketResult { let socket_res = match self.interface { #[cfg(target_os = "linux")] Some(interface) => { @@ -116,22 +128,11 @@ where SocketResult::Ok } + #[allow(clippy::too_many_lines)] async fn run(&mut self, mut poll_wait: Pin<&mut T>) { loop { let mut buf = [0_u8; 1024]; - #[allow(clippy::large_enum_variant)] - enum SelectResult { - Timer, - Recv(Result, std::io::Error>), - SystemUpdate( - Result< - SystemSourceUpdate, - tokio::sync::broadcast::error::RecvError, - >, - ), - } - let selected: SelectResult = tokio::select! { () = &mut poll_wait => { SelectResult::Timer @@ -149,14 +150,9 @@ where tracing::debug!("accept packet"); match accept_packet(result, &buf, &self.clock) { AcceptResult::Accept(packet, recv_timestamp) => { - let send_timestamp = match self.last_send_timestamp { - Some(ts) => ts, - None => { - debug!( - "we received a message without having sent one; discarding" - ); - continue; - } + let Some(send_timestamp) = self.last_send_timestamp else { + debug!("we received a message without having sent one; discarding"); + continue; }; let actions = self.source.handle_incoming( packet, @@ -223,7 +219,7 @@ where for action in actions { match action { ntp_proto::NtpSourceAction::Send(packet) => { - if matches!(self.setup_socket().await, SocketResult::Abort) { + if matches!(self.setup_socket(), SocketResult::Abort) { self.channels .msg_for_system_sender .send(MsgForSystem::NetworkIssue(self.index)) @@ -254,24 +250,24 @@ where Err(error) => { warn!(?error, "poll message could not be sent"); - match error.raw_os_error() { - Some(libc::EHOSTDOWN) - | Some(libc::EHOSTUNREACH) - | Some(libc::ENETDOWN) - | Some(libc::ENETUNREACH) => { - self.channels - .msg_for_system_sender - .send(MsgForSystem::NetworkIssue(self.index)) - .await - .ok(); - self.channels - .source_snapshots - .write() - .expect("Unexpected poisoned mutex") - .remove(&self.index); - return; - } - _ => {} + if let Some( + libc::EHOSTDOWN + | libc::EHOSTUNREACH + | libc::ENETDOWN + | libc::ENETUNREACH, + ) = error.raw_os_error() + { + self.channels + .msg_for_system_sender + .send(MsgForSystem::NetworkIssue(self.index)) + .await + .ok(); + self.channels + .source_snapshots + .write() + .expect("Unexpected poisoned mutex") + .remove(&self.index); + return; } } Ok(opt_send_timestamp) => { @@ -290,7 +286,7 @@ where .ok(); } ntp_proto::NtpSourceAction::SetTimer(timeout) => { - poll_wait.as_mut().reset(Instant::now() + timeout) + poll_wait.as_mut().reset(Instant::now() + timeout); } ntp_proto::NtpSourceAction::Reset => { self.channels @@ -356,7 +352,7 @@ where unreachable!("Should not be updating system from startup") } ntp_proto::NtpSourceAction::SetTimer(timeout) => { - poll_wait.as_mut().reset(Instant::now() + timeout) + poll_wait.as_mut().reset(Instant::now() + timeout); } ntp_proto::NtpSourceAction::Reset => { unreachable!("Should not be resetting from startup") @@ -406,14 +402,17 @@ fn accept_packet<'a, C: NtpClock>( timestamp, .. }) => { - let recv_timestamp = timestamp.map(convert_net_timestamp).unwrap_or_else(|| { - if let Ok(now) = clock.now() { - debug!(?size, "received a packet without a timestamp, substituting"); - now - } else { - panic!("Received packet without timestamp and couldn't substitute"); - } - }); + let recv_timestamp = timestamp.map_or_else( + || { + if let Ok(now) = clock.now() { + debug!(?size, "received a packet without a timestamp, substituting"); + now + } else { + panic!("Received packet without timestamp and couldn't substitute"); + } + }, + convert_net_timestamp, + ); // Note: packets are allowed to be bigger when including extensions. // we don't expect them, but the server may still send them. The @@ -431,10 +430,9 @@ fn accept_packet<'a, C: NtpClock>( warn!(?receive_error, "could not receive packet"); match receive_error.raw_os_error() { - Some(libc::EHOSTDOWN) - | Some(libc::EHOSTUNREACH) - | Some(libc::ENETDOWN) - | Some(libc::ENETUNREACH) => AcceptResult::NetworkGone, + Some(libc::EHOSTDOWN | libc::EHOSTUNREACH | libc::ENETDOWN | libc::ENETUNREACH) => { + AcceptResult::NetworkGone + } _ => AcceptResult::Ignore, } } @@ -541,6 +539,7 @@ mod tests { let cur = std::time::SystemTime::now().duration_since(std::time::SystemTime::UNIX_EPOCH)?; + #[allow(clippy::cast_possible_truncation)] Ok(NtpTimestamp::from_seconds_nanos_since_ntp_era( EPOCH_OFFSET.wrapping_add(cur.as_secs() as u32), cur.subsec_nanos(), @@ -579,7 +578,8 @@ mod tests { } } - async fn test_startup() -> ( + #[allow(clippy::type_complexity)] + fn test_startup() -> ( SourceTask, T>, Socket, mpsc::Receiver>>, @@ -643,7 +643,7 @@ mod tests { #[tokio::test] async fn test_poll_sends_state_update_and_packet() { // Note: Ports must be unique among tests to deal with parallelism - let (mut process, socket, _, _system_update_sender) = test_startup().await; + let (mut process, socket, _, _system_update_sender) = test_startup(); let (poll_wait, poll_send) = TestWait::new(); @@ -674,7 +674,7 @@ mod tests { #[tokio::test] async fn test_timeroundtrip() { // Note: Ports must be unique among tests to deal with parallelism - let (mut process, mut socket, mut msg_recv, _system_update_sender) = test_startup().await; + let (mut process, mut socket, mut msg_recv, _system_update_sender) = test_startup(); let system = SystemSnapshot { time_snapshot: TimeSnapshot { @@ -723,7 +723,7 @@ mod tests { #[tokio::test] async fn test_deny_stops_poll() { // Note: Ports must be unique among tests to deal with parallelism - let (mut process, mut socket, mut msg_recv, _system_update_sender) = test_startup().await; + let (mut process, mut socket, mut msg_recv, _system_update_sender) = test_startup(); let (poll_wait, poll_send) = TestWait::new(); @@ -758,7 +758,7 @@ mod tests { poll_send.notify(); tokio::select! { - _ = tokio::time::sleep(Duration::from_millis(10)) => {/*expected */}, + () = tokio::time::sleep(Duration::from_millis(10)) => {/*expected */}, _ = socket.recv(&mut buf) => { unreachable!("should not receive anything") } } diff --git a/ntpd/src/daemon/nts_key_provider.rs b/ntpd/src/daemon/nts_key_provider.rs index a1752deba..225584086 100644 --- a/ntpd/src/daemon/nts_key_provider.rs +++ b/ntpd/src/daemon/nts_key_provider.rs @@ -19,6 +19,7 @@ pub async fn spawn(config: KeysetConfig) -> watch::Receiver> { if let Ok(meta) = std::fs::metadata(&path) { let perm = meta.permissions(); + #[allow(clippy::cast_possible_truncation)] if perm.mode() as libc::mode_t & (libc::S_IWOTH | libc::S_IROTH | libc::S_IXOTH) != 0 { diff --git a/ntpd/src/daemon/observer.rs b/ntpd/src/daemon/observer.rs index 193184bc1..5d03019b8 100644 --- a/ntpd/src/daemon/observer.rs +++ b/ntpd/src/daemon/observer.rs @@ -94,9 +94,8 @@ async fn observer( let start_time = Instant::now(); let timeout = std::time::Duration::from_millis(500); - let path = match config.observation_path { - Some(path) => path, - None => return Ok(()), + let Some(path) = config.observation_path else { + return Ok(()); }; // this binary needs to run as root to be able to adjust the system clock. @@ -120,12 +119,7 @@ async fn observer( debug!("Unexpectedly closed unix socket: {e}"); continue; } - Err(e) - if matches!( - e.raw_os_error(), - Some(ENFILE) | Some(EMFILE) | Some(ENOMEM) | Some(ENOBUFS) - ) => - { + Err(e) if matches!(e.raw_os_error(), Some(ENFILE | EMFILE | ENOMEM | ENOBUFS)) => { error!( "Not enough resources available to accept incoming observability socket: {e}" ); @@ -179,7 +173,11 @@ async fn handle_connection( .cloned() .collect(), system: *system_reader.borrow(), - servers: server_reader.borrow().iter().map(|s| s.into()).collect(), + servers: server_reader + .borrow() + .iter() + .map(std::convert::Into::into) + .collect(), }; super::sockets::write_json(stream, &observe).await?; @@ -196,7 +194,8 @@ mod tests { #[cfg(feature = "unstable_ntpv5")] use ntp_proto::v5::{BloomFilter, ServerId}; use ntp_proto::{ - NtpDuration, NtpLeapIndicator, PollIntervalLimits, Reach, ReferenceId, TimeSnapshot, + NtpDuration, NtpLeapIndicator, ObservableSourceTimedata, PollIntervalLimits, Reach, + ReferenceId, TimeSnapshot, }; use tokio::{io::AsyncReadExt, net::UnixStream}; @@ -220,7 +219,7 @@ mod tests { source_snapshots.insert( id, ObservableSourceState { - timedata: Default::default(), + timedata: ObservableSourceTimedata::default(), unanswered_polls: Reach::default().unanswered_polls(), poll_interval: PollIntervalLimits::default().min, nts_cookies: None, @@ -287,7 +286,7 @@ mod tests { source_snapshots.insert( id, ObservableSourceState { - timedata: Default::default(), + timedata: ObservableSourceTimedata::default(), unanswered_polls: Reach::default().unanswered_polls(), poll_interval: PollIntervalLimits::default().min, nts_cookies: None, diff --git a/ntpd/src/daemon/server.rs b/ntpd/src/daemon/server.rs index f13bf69b9..bf42b383b 100644 --- a/ntpd/src/daemon/server.rs +++ b/ntpd/src/daemon/server.rs @@ -58,7 +58,7 @@ impl ServerStatHandler for ServerStats { (ServerResponse::ProvideTime, _) => self.nts_accepted_packets.inc(), (ServerResponse::Deny, _) => self.nts_denied_packets.inc(), (ServerResponse::Ignore, ServerReason::RateLimit) => { - self.nts_rate_limited_packets.inc() + self.nts_rate_limited_packets.inc(); } _ => { /* counted above */ } } @@ -147,32 +147,31 @@ impl ServerTask { let mut cur_socket = None; loop { // open socket if it is not already open - let socket = match &mut cur_socket { - Some(socket) => socket, - None => { - let new_socket = loop { - let socket_res = open_ip( - self.config.listen, - timestamped_socket::socket::GeneralTimestampMode::SoftwareRecv, - ); - - match socket_res { - Ok(socket) => break socket, - Err(error) => { - warn!(?error, ?self.config.listen, "Could not open server socket"); - tokio::time::sleep(self.network_wait_period).await; - } + let socket = if let Some(socket) = &mut cur_socket { + socket + } else { + let new_socket = loop { + let socket_res = open_ip( + self.config.listen, + timestamped_socket::socket::GeneralTimestampMode::SoftwareRecv, + ); + + match socket_res { + Ok(socket) => break socket, + Err(error) => { + warn!(?error, ?self.config.listen, "Could not open server socket"); + tokio::time::sleep(self.network_wait_period).await; } - }; + } + }; - // system and keyset may now be wildly out of date, ensure they are always updated. - self.server - .update_system(*self.system_receiver.borrow_and_update()); - self.server - .update_keyset(self.keyset.borrow_and_update().clone()); + // system and keyset may now be wildly out of date, ensure they are always updated. + self.server + .update_system(*self.system_receiver.borrow_and_update()); + self.server + .update_keyset(self.keyset.borrow_and_update().clone()); - cur_socket.insert(new_socket) - } + cur_socket.insert(new_socket) }; let mut buf = [0_u8; MAX_PACKET_SIZE]; @@ -287,6 +286,7 @@ mod tests { let mut cursor = Cursor::new(buf.as_mut_slice()); send_packet.serialize(&mut cursor, &NoCipher, None).unwrap(); + #[allow(clippy::cast_possible_truncation)] let end = cursor.position() as usize; buf.truncate(end); buf @@ -305,7 +305,7 @@ mod tests { let join = ServerTask::spawn( config, - Default::default(), + ServerStats::default(), system_snapshots, keyset, clock, diff --git a/ntpd/src/daemon/sock_source.rs b/ntpd/src/daemon/sock_source.rs index 1eeb225e3..074096aa5 100644 --- a/ntpd/src/daemon/sock_source.rs +++ b/ntpd/src/daemon/sock_source.rs @@ -25,7 +25,7 @@ struct SockSample { magic: i32, } -const SOCK_MAGIC: i32 = 0x534f434b; +const SOCK_MAGIC: i32 = 0x534f_434b; const SOCK_SAMPLE_SIZE: usize = 40; #[derive(Debug)] @@ -101,6 +101,16 @@ fn create_socket>(path: T) -> std::io::Result { Ok(socket) } +enum SelectResult { + SockRecv(Result), + SystemUpdate( + Result< + SystemSourceUpdate, + tokio::sync::broadcast::error::RecvError, + >, + ), +} + impl> SockSourceTask where C: 'static + NtpClock + Send + Sync, @@ -109,16 +119,6 @@ where loop { let mut buf = [0; SOCK_SAMPLE_SIZE]; - enum SelectResult { - SockRecv(Result), - SystemUpdate( - Result< - SystemSourceUpdate, - tokio::sync::broadcast::error::RecvError, - >, - ), - } - let selected: SelectResult = tokio::select! { result = self.socket.recv(&mut buf) => { SelectResult::SockRecv(result) @@ -181,12 +181,8 @@ where } }, SelectResult::SystemUpdate(result) => match result { - Ok(update) => { - self.source.handle_message(update.message); - } - Err(e) => { - error!("Error receiving system update: {:?}", e) - } + Ok(update) => self.source.handle_message(update.message), + Err(e) => error!("Error receiving system update: {:?}", e), }, }; } @@ -256,7 +252,9 @@ mod tests { std::time::SystemTime::now().duration_since(std::time::SystemTime::UNIX_EPOCH)?; Ok(NtpTimestamp::from_seconds_nanos_since_ntp_era( - EPOCH_OFFSET.wrapping_add(cur.as_secs() as u32), + EPOCH_OFFSET.wrapping_add( + u32::try_from(cur.as_secs()).expect("Couldn't fit unix epoch inside u32"), + ), cur.subsec_nanos(), )) } @@ -350,6 +348,7 @@ mod tests { } #[test] + #[allow(clippy::float_cmp)] fn test_deserialize_sample() { // Example sock sample let buf = [ @@ -357,7 +356,8 @@ mod tests { 119, 19, 65, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 75, 67, 79, 83, ]; let sample = deserialize_sample(Ok(buf.len()), buf).unwrap(); - assert_eq!(sample.offset, 318975.704798661); + + assert_eq!(sample.offset, 318_975.704_798_661); assert_eq!(sample.pulse, 0); assert_eq!(sample.leap, 0); assert_eq!(sample.magic, SOCK_MAGIC); diff --git a/ntpd/src/daemon/sockets.rs b/ntpd/src/daemon/sockets.rs index ab3284da0..953c20356 100644 --- a/ntpd/src/daemon/sockets.rs +++ b/ntpd/src/daemon/sockets.rs @@ -20,9 +20,13 @@ where T: serde::Deserialize<'a>, { buffer.clear(); + + #[allow(clippy::cast_possible_truncation)] let msg_size = stream.read_u64().await? as usize; buffer.resize(msg_size, 0); + stream.read_exact(buffer).await?; + serde_json::from_slice(buffer) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidInput, e)) } diff --git a/ntpd/src/daemon/spawn/mod.rs b/ntpd/src/daemon/spawn/mod.rs index 4820c05fc..bbe075599 100644 --- a/ntpd/src/daemon/spawn/mod.rs +++ b/ntpd/src/daemon/spawn/mod.rs @@ -216,7 +216,7 @@ pub trait Spawner { fn get_addr_description(&self) -> String; /// Get a description of the type of spawner - fn get_description(&self) -> &str; + fn get_description(&self) -> &'static str; } pub async fn spawner_task( diff --git a/ntpd/src/daemon/spawn/nts.rs b/ntpd/src/daemon/spawn/nts.rs index 07b59cb25..8d0bad187 100644 --- a/ntpd/src/daemon/spawn/nts.rs +++ b/ntpd/src/daemon/spawn/nts.rs @@ -38,13 +38,14 @@ impl From> for NtsSpawnError { pub(super) async fn resolve_addr(address: (&str, u16)) -> Option { match tokio::net::lookup_host(address).await { - Ok(mut addresses) => match addresses.next() { - Some(address) => Some(address), - None => { + Ok(mut addresses) => { + if let Some(address) = addresses.next() { + Some(address) + } else { warn!("received unknown domain name from NTS-ke"); None } - }, + } Err(e) => { warn!(error = ?e, "error while resolving source address, retrying"); None @@ -56,7 +57,7 @@ impl NtsSpawner { pub fn new(config: NtsSourceConfig) -> NtsSpawner { NtsSpawner { config, - id: Default::default(), + id: SpawnerId::default(), has_spawned: false, } } @@ -126,7 +127,7 @@ impl Spawner for NtsSpawner { self.config.address.to_string() } - fn get_description(&self) -> &str { + fn get_description(&self) -> &'static str { "nts" } } diff --git a/ntpd/src/daemon/spawn/nts_pool.rs b/ntpd/src/daemon/spawn/nts_pool.rs index 7d2615197..e62aad7e5 100644 --- a/ntpd/src/daemon/spawn/nts_pool.rs +++ b/ntpd/src/daemon/spawn/nts_pool.rs @@ -48,8 +48,8 @@ impl NtsPoolSpawner { pub fn new(config: NtsPoolSourceConfig) -> NtsPoolSpawner { NtsPoolSpawner { config, - id: Default::default(), - current_sources: Default::default(), + id: SpawnerId::default(), + current_sources: Vec::default(), //known_ips: Default::default(), } } @@ -136,10 +136,10 @@ impl Spawner for NtsPoolSpawner { } fn get_addr_description(&self) -> String { - format!("{} ({})", self.config.addr.deref(), self.config.count) + format!("{} ({})", &*self.config.addr, self.config.count) } - fn get_description(&self) -> &str { + fn get_description(&self) -> &'static str { "nts-pool" } } diff --git a/ntpd/src/daemon/spawn/pool.rs b/ntpd/src/daemon/spawn/pool.rs index 50077da09..d3b3c9878 100644 --- a/ntpd/src/daemon/spawn/pool.rs +++ b/ntpd/src/daemon/spawn/pool.rs @@ -38,9 +38,9 @@ impl PoolSpawner { pub fn new(config: PoolSourceConfig) -> PoolSpawner { PoolSpawner { config, - id: Default::default(), - current_sources: Default::default(), - known_ips: Default::default(), + id: SpawnerId::default(), + current_sources: Vec::default(), + known_ips: Vec::default(), } } } @@ -126,10 +126,10 @@ impl Spawner for PoolSpawner { } fn get_addr_description(&self) -> String { - format!("{} ({})", self.config.addr.deref(), self.config.count) + format!("{} ({})", &*self.config.addr, self.config.count) } - fn get_description(&self) -> &str { + fn get_description(&self) -> &'static str { "pool" } } diff --git a/ntpd/src/daemon/spawn/sock.rs b/ntpd/src/daemon/spawn/sock.rs index fa68a2691..300e1e250 100644 --- a/ntpd/src/daemon/spawn/sock.rs +++ b/ntpd/src/daemon/spawn/sock.rs @@ -17,7 +17,7 @@ impl SockSpawner { pub fn new(config: SockSourceConfig) -> SockSpawner { SockSpawner { config, - id: Default::default(), + id: SpawnerId::default(), has_spawned: false, } } @@ -67,7 +67,7 @@ impl Spawner for SockSpawner { self.config.path.display().to_string() } - fn get_description(&self) -> &str { + fn get_description(&self) -> &'static str { "sock" } } diff --git a/ntpd/src/daemon/spawn/standard.rs b/ntpd/src/daemon/spawn/standard.rs index 464cf355a..dbcc74757 100644 --- a/ntpd/src/daemon/spawn/standard.rs +++ b/ntpd/src/daemon/spawn/standard.rs @@ -44,7 +44,7 @@ impl std::error::Error for StandardSpawnError {} impl StandardSpawner { pub fn new(config: StandardSource) -> StandardSpawner { StandardSpawner { - id: Default::default(), + id: SpawnerId::default(), config, resolved: None, has_spawned: false, @@ -135,7 +135,7 @@ impl Spawner for StandardSpawner { self.config.address.to_string() } - fn get_description(&self) -> &str { + fn get_description(&self) -> &'static str { "standard" } } @@ -324,20 +324,16 @@ mod tests { } let seen_addresses = seen_addresses; - for addr in seen_addresses.iter() { + for addr in &seen_addresses { assert!( addresses.contains(addr), - "{:?} should have been drawn from {:?}", - addr, - addresses + "{addr:?} should have been drawn from {addresses:?}" ); } assert!( seen_addresses.iter().any(|seen| seen != &initial_addr), - "Re-resolved\n\n\t{:?}\n\n should contain at least one address that isn't the original\n\n\t{:?}", - seen_addresses, - initial_addr, + "Re-resolved\n\n\t{seen_addresses:?}\n\n should contain at least one address that isn't the original\n\n\t{initial_addr:?}", ); } diff --git a/ntpd/src/daemon/system.rs b/ntpd/src/daemon/system.rs index 56e0fdf8a..1154150b2 100644 --- a/ntpd/src/daemon/system.rs +++ b/ntpd/src/daemon/system.rs @@ -67,7 +67,7 @@ impl Future for SingleshotSleep { this.enabled = false; std::task::Poll::Ready(v) } - u => u, + u @ std::task::Poll::Pending => u, } } } @@ -106,7 +106,7 @@ pub async fn spawn { - system - .add_spawner(StandardSpawner::new(cfg.clone())) - .map_err(|e| { - tracing::error!("Could not spawn source: {}", e); - std::io::Error::new(std::io::ErrorKind::Other, e) - })?; + system.add_spawner(StandardSpawner::new(cfg.clone())); } NtpSourceConfig::Nts(cfg) => { - system - .add_spawner(NtsSpawner::new(cfg.clone())) - .map_err(|e| { - tracing::error!("Could not spawn source: {}", e); - std::io::Error::new(std::io::ErrorKind::Other, e) - })?; + system.add_spawner(NtsSpawner::new(cfg.clone())); } NtpSourceConfig::Pool(cfg) => { - system - .add_spawner(PoolSpawner::new(cfg.clone())) - .map_err(|e| { - tracing::error!("Could not spawn source: {}", e); - std::io::Error::new(std::io::ErrorKind::Other, e) - })?; + system.add_spawner(PoolSpawner::new(cfg.clone())); } #[cfg(feature = "unstable_nts-pool")] NtpSourceConfig::NtsPool(cfg) => { - system - .add_spawner(NtsPoolSpawner::new(cfg.clone())) - .map_err(|e| { - tracing::error!("Could not spawn source: {}", e); - std::io::Error::new(std::io::ErrorKind::Other, e) - })?; + system.add_spawner(NtsPoolSpawner::new(cfg.clone())); } NtpSourceConfig::Sock(cfg) => { - system - .add_spawner(SockSpawner::new(cfg.clone())) - .map_err(|e| { - tracing::error!("Could not spawn source: {}", e); - std::io::Error::new(std::io::ErrorKind::Other, e) - })?; + system.add_spawner(SockSpawner::new(cfg.clone())); } } } for server_config in server_configs.iter() { - system.add_server(server_config.to_owned()).await; + system.add_server(server_config.to_owned()); } let handle = tokio::spawn(async move { @@ -225,7 +200,7 @@ impl< synchronization_config: SynchronizationConfig, algorithm_config: Controller::AlgorithmConfig, source_defaults_config: SourceDefaultsConfig, - keyset: tokio::sync::watch::Receiver>, + keyset: &tokio::sync::watch::Receiver>, ip_list: tokio::sync::watch::Receiver>, have_sources: bool, ) -> (Self, DaemonChannels) { @@ -275,9 +250,9 @@ impl< spawn_rx, spawn_tx, - sources: Default::default(), - servers: Default::default(), - spawners: Default::default(), + sources: HashMap::default(), + servers: Vec::default(), + spawners: Vec::default(), clock, timestamp_mode, interface, @@ -290,10 +265,7 @@ impl< ) } - fn add_spawner( - &mut self, - spawner: impl Spawner + Send + Sync + 'static, - ) -> Result { + fn add_spawner(&mut self, spawner: impl Spawner + Send + Sync + 'static) -> SpawnerId { let (notify_tx, notify_rx) = mpsc::channel(MESSAGE_BUFFER_SIZE); let id = spawner.get_id(); let spawner_data = SystemSpawnerData { id, notify_tx }; @@ -302,7 +274,7 @@ impl< let spawn_tx = self.spawn_tx.clone(); // tokio::spawn(async move { spawner.run(spawn_tx, notify_rx).await }); tokio::spawn(spawner_task(spawner, spawn_tx, notify_rx)); - Ok(id) + id } async fn run(&mut self, mut wait: Pin<&mut SingleshotSleep>) -> std::io::Result<()> { @@ -363,7 +335,7 @@ impl< let _ = self.system_update_sender.send(update); } ntp_proto::SystemAction::SetTimer(duration) => { - wait.as_mut().reset(tokio::time::Instant::now() + duration) + wait.as_mut().reset(tokio::time::Instant::now() + duration); } } } @@ -484,8 +456,8 @@ impl< self.sources.insert( source_id, SourceState { - source_id, spawner_id, + source_id, }, ); @@ -554,7 +526,7 @@ impl< Ok(()) } - async fn add_server(&mut self, config: ServerConfig) { + fn add_server(&mut self, config: ServerConfig) { let stats = ServerStats::default(); self.servers.push(ServerData { stats: stats.clone(), diff --git a/ntpd/src/daemon/util.rs b/ntpd/src/daemon/util.rs index 7c59eef5f..7da7be7cf 100644 --- a/ntpd/src/daemon/util.rs +++ b/ntpd/src/daemon/util.rs @@ -3,6 +3,8 @@ use ntp_proto::NtpTimestamp; // Epoch offset between NTP and UNIX timescales pub(crate) const EPOCH_OFFSET: u32 = (70 * 365 + 17) * 86400; +#[allow(clippy::cast_possible_truncation)] +#[allow(clippy::cast_sign_loss)] pub(crate) fn convert_net_timestamp(ts: timestamped_socket::socket::Timestamp) -> NtpTimestamp { NtpTimestamp::from_seconds_nanos_since_ntp_era( EPOCH_OFFSET.wrapping_add(ts.seconds as _), @@ -10,6 +12,8 @@ pub(crate) fn convert_net_timestamp(ts: timestamped_socket::socket::Timestamp) - ) } +#[allow(clippy::cast_possible_truncation)] +#[allow(clippy::cast_sign_loss)] pub(crate) fn convert_clock_timestamp(ts: clock_steering::Timestamp) -> NtpTimestamp { NtpTimestamp::from_seconds_nanos_since_ntp_era( EPOCH_OFFSET.wrapping_add(ts.seconds as _), diff --git a/ntpd/src/force_sync/algorithm.rs b/ntpd/src/force_sync/algorithm.rs index 587d46e91..a1ee5bc4a 100644 --- a/ntpd/src/force_sync/algorithm.rs +++ b/ntpd/src/force_sync/algorithm.rs @@ -64,14 +64,15 @@ impl SingleShotController { const ASSUMED_UNCERTAINTY: NtpDuration = NtpDuration::from_exponent(-1); fn try_steer(&self) { - if self.sources.len() < self.min_agreeing { - return; - } - struct Event { offset: NtpDuration, count: isize, } + + if self.sources.len() < self.min_agreeing { + return; + } + let mut events: Vec<_> = self .sources .values() @@ -102,17 +103,18 @@ impl SingleShotController { } } + #[allow(clippy::cast_sign_loss)] if peak as usize >= self.min_agreeing { let mut sum = 0.0; let mut count = 0; for source in self.sources.values() { if source.get_offset().abs_diff(peak_offset) < Self::ASSUMED_UNCERTAINTY { count += 1; - sum += source.get_offset().to_seconds() + sum += source.get_offset().to_seconds(); } } - let avg_offset = NtpDuration::from_seconds(sum / (count as f64)); + let avg_offset = NtpDuration::from_seconds(sum / f64::from(count)); self.offer_clock_change(avg_offset); std::process::exit(0); @@ -188,12 +190,12 @@ impl TimeSyncController for SingleShotController { self.sources.insert(id, message); // TODO, check and update time once we have sufficient sources self.try_steer(); - Default::default() + ntp_proto::StateUpdate::default() } fn time_update(&mut self) -> ntp_proto::StateUpdate { // no need for action - Default::default() + ntp_proto::StateUpdate::default() } } diff --git a/ntpd/src/force_sync/mod.rs b/ntpd/src/force_sync/mod.rs index 20b044ad0..c780d6828 100644 --- a/ntpd/src/force_sync/mod.rs +++ b/ntpd/src/force_sync/mod.rs @@ -23,22 +23,22 @@ fn human_readable_duration(abs_offset: f64) -> String { let mut offset = abs_offset; let mut res = String::new(); if offset >= 86400.0 { - let days = (offset / 86400.0).floor() as u64; - offset -= days as f64 * 86400.0; - res.push_str(&format!("{} day(s) ", days)); + let days = (offset / 86400.0).floor(); + offset -= days * 86400.0; + res.push_str(&format!("{days:.0} day(s) ")); } if offset >= 3600.0 { - let hours = (offset / 3600.0).floor() as u64; - offset -= hours as f64 * 3600.0; - res.push_str(&format!("{} hour(s) ", hours)); + let hours = (offset / 3600.0).floor(); + offset -= hours * 3600.0; + res.push_str(&format!("{hours:.0} hour(s) ")); } if offset >= 60.0 { - let minutes = (offset / 60.0).floor() as u64; - offset -= minutes as f64 * 60.0; - res.push_str(&format!("{} minute(s) ", minutes)); + let minutes = (offset / 60.0).floor(); + offset -= minutes * 60.0; + res.push_str(&format!("{minutes:.0} minute(s) ")); } if offset >= 1.0 { - res.push_str(&format!("{:.0} second(s)", offset)); + res.push_str(&format!("{offset:.0} second(s)")); } res } @@ -49,11 +49,14 @@ fn try_date_display(offset: NtpDuration) -> Option { .duration_since(UNIX_EPOCH) .map(|d| d.as_secs()) .unwrap_or(0); + + #[allow(clippy::cast_possible_truncation)] + #[allow(clippy::cast_sign_loss)] let ts = since_epoch + (offset.to_seconds() as u64); std::process::Command::new("date") .arg("-d") - .arg(format!("@{}", ts)) + .arg(format!("@{ts}")) .arg("+%c") .output() .ok() @@ -109,7 +112,7 @@ impl SingleShotController { } } -pub(crate) fn force_sync(config: Option) -> std::io::Result { +pub(crate) fn force_sync(config: Option<&PathBuf>) -> std::io::Result { let config = initialize_logging_parse_config(Some(LogLevel::Warn), config); // Warn/error if the config is unreasonable. We do this after finishing @@ -135,11 +138,11 @@ pub(crate) fn force_sync(config: Option) -> std::io::Result { | config::NtpSourceConfig::Nts(_) | config::NtpSourceConfig::Sock(_) => total_sources += 1, config::NtpSourceConfig::Pool(PoolSourceConfig { count, .. }) => { - total_sources += count + total_sources += count; } #[cfg(feature = "unstable_nts-pool")] config::NtpSourceConfig::NtsPool(NtsPoolSourceConfig { count, .. }) => { - total_sources += count + total_sources += count; } } } diff --git a/ntpd/src/lib.rs b/ntpd/src/lib.rs index fd9658536..c67e59df6 100644 --- a/ntpd/src/lib.rs +++ b/ntpd/src/lib.rs @@ -1,3 +1,6 @@ +#![deny(clippy::pedantic)] +#![allow(clippy::module_name_repetitions)] +#![allow(clippy::explicit_iter_loop)] // breaking on msrv #![forbid(unsafe_code)] mod ctl; diff --git a/ntpd/src/metrics/exporter.rs b/ntpd/src/metrics/exporter.rs index 1277c3e3d..09deeec96 100644 --- a/ntpd/src/metrics/exporter.rs +++ b/ntpd/src/metrics/exporter.rs @@ -110,6 +110,9 @@ impl NtpMetricsExporterOptions { } } +/// # Errors +/// +/// Returns 'Error' if arguments to program are invalid. pub fn main() -> Result<(), Box> { let options = NtpMetricsExporterOptions::try_parse_from(std::env::args())?; match options.action { @@ -121,22 +124,21 @@ pub fn main() -> Result<(), Box> { eprintln!("ntp-metrics-exporter {VERSION}"); Ok(()) } - MetricsAction::Run => run(options), + MetricsAction::Run => run(&options), } } -fn run(options: NtpMetricsExporterOptions) -> Result<(), Box> { - let config = initialize_logging_parse_config(None, options.config); +fn run(options: &NtpMetricsExporterOptions) -> Result<(), Box> { + let config = initialize_logging_parse_config(None, options.config.as_ref()); Builder::new_current_thread().enable_all().build()?.block_on(async { let timeout = std::time::Duration::from_millis(1000); - let observation_socket_path = match config.observability.observation_path { - Some(path) => Arc::new(path), - None => { - eprintln!("An observation socket path must be configured using the observation-path option in the [observability] section of the configuration"); - std::process::exit(1); - } + let observation_socket_path = if let Some(path) = config.observability.observation_path { + Arc::new(path) + } else { + eprintln!("An observation socket path must be configured using the observation-path option in the [observability] section of the configuration"); + std::process::exit(1); }; println!( @@ -185,7 +187,7 @@ fn run(options: NtpMetricsExporterOptions) -> Result<(), Box { error!("Not enough resources available to accept incoming connection: {e}"); @@ -218,6 +220,12 @@ async fn handle_connection( stream: &mut (impl tokio::io::AsyncWrite + tokio::io::AsyncRead + Unpin), observation_socket_path: &Path, ) -> std::io::Result<()> { + const ERROR_RESPONSE: &str = concat!( + "HTTP/1.1 500 Internal Server Error\r\n", + "content-type: text/plain\r\n", + "content-length: 0\r\n\r\n", + ); + // Wait until a request was sent, dropping the bytes read when this scope ends // to ensure we don't accidentally use them afterwards { @@ -260,12 +268,6 @@ async fn handle_connection( Err(e) => { tracing::warn!("hit an error: {e}"); - const ERROR_RESPONSE: &str = concat!( - "HTTP/1.1 500 Internal Server Error\r\n", - "content-type: text/plain\r\n", - "content-length: 0\r\n\r\n", - ); - stream.write_all(ERROR_RESPONSE.as_bytes()).await?; } } diff --git a/ntpd/src/metrics/mod.rs b/ntpd/src/metrics/mod.rs index 91843e966..c84515a28 100644 --- a/ntpd/src/metrics/mod.rs +++ b/ntpd/src/metrics/mod.rs @@ -12,7 +12,7 @@ struct Measurement { impl Measurement { fn simple(value: T) -> Vec> { vec![Measurement { - labels: Default::default(), + labels: Vec::default(), value, }] } @@ -24,7 +24,9 @@ enum Unit { } impl Unit { - fn as_str(&self) -> &str { + #[allow(clippy::unused_self)] + #[allow(clippy::trivially_copy_pass_by_ref)] + fn as_str(&self) -> &'static str { "seconds" } } @@ -47,7 +49,7 @@ fn format_metric( w: &mut impl std::fmt::Write, name: &str, help: &str, - metric_type: MetricType, + metric_type: &MetricType, unit: Option, measurements: Vec>, ) -> std::fmt::Result { @@ -139,12 +141,13 @@ macro_rules! collect_servers { }}; } +#[allow(clippy::too_many_lines)] pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> std::fmt::Result { format_metric( w, "ntp_uptime", "Time that the ntp daemon is running", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), vec![Measurement { labels: vec![ @@ -160,7 +163,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_system_poll_interval", "[DEPRECATED] Time between polls of the system", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), Measurement::simple( state @@ -178,7 +181,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_system_accumulated_steps", "Accumulated amount of seconds that the system needed to jump the time", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), Measurement::simple(state.system.time_snapshot.accumulated_steps.to_seconds()), )?; @@ -187,19 +190,18 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_system_accumulated_steps_threshold", "Threshold for the accumulated step amount at which the NTP daemon will exit (or -1 if no threshold was set)", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), Measurement::simple(state.system .accumulated_steps_threshold - .map(|v| v.to_seconds()) - .unwrap_or(-1.0)), + .map_or(-1.0, ntp_proto::NtpDuration::to_seconds)), )?; format_metric( w, "ntp_system_leap_indicator", "Indicates that a leap second will take place", - MetricType::Gauge, + &MetricType::Gauge, None, Measurement::simple(state.system.time_snapshot.leap_indicator as i64), )?; @@ -208,7 +210,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_system_root_delay", "Distance to the closest root time source", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), Measurement::simple(state.system.time_snapshot.root_delay.to_seconds()), )?; @@ -217,7 +219,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_system_root_dispersion", "Estimate of how precise our time is", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), Measurement::simple(state.system.time_snapshot.root_dispersion.to_seconds()), )?; @@ -226,7 +228,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_system_stratum", "Stratum of our clock", - MetricType::Gauge, + &MetricType::Gauge, None, Measurement::simple(state.system.stratum), )?; @@ -235,7 +237,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_poll_interval", "Time between polls of the source", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), collect_sources!(state, |p| p.poll_interval.as_duration().to_seconds()), )?; @@ -244,7 +246,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_unanswered_polls", "Number of polls since the last successful poll with a maximum of eight", - MetricType::Gauge, + &MetricType::Gauge, None, collect_sources!(state, |p| p.unanswered_polls), )?; @@ -253,7 +255,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_nts_cookies_available", "Number of unused cookies available for nts-enabled ntp exchanges", - MetricType::Gauge, + &MetricType::Gauge, None, collect_some_sources!(state, |p| p.nts_cookies), )?; @@ -262,7 +264,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_offset", "Offset between the upstream source and system time", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), collect_sources!(state, |p| p.timedata.offset.to_seconds()), )?; @@ -271,7 +273,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_delay", "Current round-trip delay to the upstream source", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), collect_sources!(state, |p| p.timedata.delay.to_seconds()), )?; @@ -280,7 +282,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_uncertainty", "Estimated error of the source clock", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), collect_sources!(state, |p| p.timedata.uncertainty.to_seconds()), )?; @@ -289,7 +291,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_root_delay", "Root delay reported by the time source", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), collect_sources!(state, |p| p.timedata.remote_delay.to_seconds()), )?; @@ -298,7 +300,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_source_root_dispersion", "Uncertainty reported by the time source", - MetricType::Gauge, + &MetricType::Gauge, Some(Unit::Seconds), collect_sources!(state, |p| p.timedata.remote_uncertainty.to_seconds()), )?; @@ -307,7 +309,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_received_packets_total", "Number of incoming packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.received_packets.get()), )?; @@ -316,7 +318,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_accepted_packets_total", "Number of packets accepted", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.accepted_packets.get()), )?; @@ -325,7 +327,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_denied_packets_total", "Number of denied packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.denied_packets.get()), )?; @@ -334,7 +336,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_ignored_packets_total", "Number of packets ignored", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.ignored_packets.get()), )?; @@ -343,7 +345,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_rate_limited_packets_total", "Number of rate limited packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.rate_limited_packets.get()), )?; @@ -352,7 +354,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_response_send_errors_total", "Number of packets where there was an error responding", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.response_send_errors.get()), )?; @@ -361,7 +363,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_nts_received_packets_total", "Number of incoming NTS packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.nts_received_packets.get()), )?; @@ -370,7 +372,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_nts_accepted_packets_total", "Number of NTS packets accepted", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.nts_accepted_packets.get()), )?; @@ -379,7 +381,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_nts_denied_packets_total", "Number of denied NTS packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.nts_denied_packets.get()), )?; @@ -388,7 +390,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_nts_rate_limited_packets_total", "Number of rate limited NTS packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.nts_rate_limited_packets.get()), )?; @@ -397,7 +399,7 @@ pub fn format_state(w: &mut impl std::fmt::Write, state: &ObservableState) -> st w, "ntp_server_nts_nak_packets_total", "Number of NTS nak responses to packets", - MetricType::Counter, + &MetricType::Counter, None, collect_servers!(state, |s| s.stats.nts_nak_packets.get()), )?; diff --git a/nts-pool-ke/src/condcompile/cli.rs b/nts-pool-ke/src/condcompile/cli.rs index 7ab142643..e600fef32 100644 --- a/nts-pool-ke/src/condcompile/cli.rs +++ b/nts-pool-ke/src/condcompile/cli.rs @@ -62,13 +62,13 @@ impl CliArg { if let Some((key, value)) = long_arg.split_once('=') { if takes_argument.contains(&key) { - processed.push(CliArg::Argument(key.to_string(), value.to_string())) + processed.push(CliArg::Argument(key.to_string(), value.to_string())); } else { - invalid? + invalid?; } } else if takes_argument.contains(&long_arg) { if let Some(next) = arg_iter.next() { - processed.push(CliArg::Argument(long_arg.to_string(), next)) + processed.push(CliArg::Argument(long_arg.to_string(), next)); } else { Err(format!("'{}' expects an argument", &long_arg))?; } @@ -95,12 +95,12 @@ impl CliArg { // short version of --help has no arguments processed.push(CliArg::Flag(flag)); } else { - Err(format!("'-{}' expects an argument", char))?; + Err(format!("'-{char}' expects an argument"))?; } break; - } else { - processed.push(CliArg::Flag(flag)); } + + processed.push(CliArg::Flag(flag)); } } _argument => rest.push(arg), diff --git a/nts-pool-ke/src/condcompile/config.rs b/nts-pool-ke/src/condcompile/config.rs index dea2168c5..6a4e6ff52 100644 --- a/nts-pool-ke/src/condcompile/config.rs +++ b/nts-pool-ke/src/condcompile/config.rs @@ -46,15 +46,18 @@ impl Display for ConfigError { impl std::error::Error for ConfigError {} impl Config { + #[allow(clippy::unused_self)] + // TODO impl config check pub fn check(&self) -> bool { true } async fn from_file(file: impl AsRef) -> Result { + const S_IWOTH: u32 = 2; + let meta = std::fs::metadata(&file)?; let perm = meta.permissions(); - const S_IWOTH: u32 = 2; if perm.mode() & S_IWOTH != 0 { warn!("Unrestricted config file permissions: Others can write."); }