diff --git a/CHANGELOG.md b/CHANGELOG.md index 79351c3..e34c0a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -148,6 +148,11 @@ with v0.4.x or earlier peers. folded into the new tables with a one-time deprecation warning; migrate your `fips.yaml` to the new keys. +- `SessionDatagram::decrement_ttl` and `SessionDatagram::can_forward` now match + the forwarder's IP hop-limit semantics: `decrement_ttl` decrements first and + reports false when the result is zero, and `can_forward` is true only at a + TTL of 2 or more. + ### Deprecated - The `discovery` metric-family key (control-socket JSON). It is dual-emitted diff --git a/src/config/mod.rs b/src/config/mod.rs index 9d46f87..90ef742 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -24,6 +24,7 @@ mod node; mod peer; mod transport; +use crate::node::REKEY_JITTER_SECS; use crate::upper::config::{DnsConfig, TunConfig}; use crate::{Identity, IdentityError}; use serde::{Deserialize, Serialize}; @@ -750,6 +751,32 @@ impl Config { } } + // Reject rekey triggers that fire immediately and forever. Both + // arms are checked regardless of `node.rekey.enabled` so that + // turning rekey on later cannot surface a config error at a + // surprising moment. There is deliberately no upper bound: + // u64::MAX is the idiom for disabling one arm of the trigger. + let rekey = &self.node.rekey; + + if rekey.after_messages == 0 { + return Err(ConfigError::Validation( + "`node.rekey.after_messages` must be at least 1; 0 fires the message-count trigger on every poll instead of disabling it. \ + Use a very large value to effectively disable the message-count trigger." + .to_string(), + )); + } + + let jitter_secs = REKEY_JITTER_SECS.unsigned_abs(); + if rekey.after_secs <= jitter_secs { + return Err(ConfigError::Validation(format!( + "`node.rekey.after_secs` is {}, but must be greater than the per-session rekey jitter of {jitter_secs}s; \ + each session offsets the interval by a random value in [-{jitter_secs}, +{jitter_secs}] seconds, so a smaller interval saturates to zero \ + and rekeys on sight for roughly half of sessions. \ + Use a very large value to effectively disable the timer trigger.", + rekey.after_secs + ))); + } + Ok(()) } @@ -1655,6 +1682,86 @@ node: .expect("outbound_only should be exempt from the loopback check"); } + #[test] + fn test_validate_default_rekey_settings_ok() { + Config::default() + .validate() + .expect("shipped default rekey settings must validate"); + } + + #[test] + fn test_validate_rekey_after_messages_zero_rejected() { + let mut config = Config::default(); + config.node.rekey.after_messages = 0; + + let err = config.validate().expect_err("validation should fail"); + let msg = err.to_string(); + assert!(msg.contains("after_messages"), "got: {msg}"); + } + + #[test] + fn test_validate_rekey_after_messages_one_accepted() { + let mut config = Config::default(); + config.node.rekey.after_messages = 1; + + config + .validate() + .expect("after_messages = 1 rekeys every message, which is wasteful but well defined"); + } + + #[test] + fn test_validate_rekey_after_secs_at_or_below_jitter_rejected() { + let jitter = REKEY_JITTER_SECS.unsigned_abs(); + + for after_secs in [0, 1, jitter - 1, jitter] { + let mut config = Config::default(); + config.node.rekey.after_secs = after_secs; + + match config.validate() { + Err(e) => assert!(e.to_string().contains("after_secs"), "got: {e}"), + Ok(()) => panic!("after_secs = {after_secs} should be rejected"), + } + } + } + + #[test] + fn test_validate_rekey_after_secs_just_above_jitter_accepted() { + let mut config = Config::default(); + config.node.rekey.after_secs = REKEY_JITTER_SECS.unsigned_abs() + 1; + + config + .validate() + .expect("one second above the jitter bound leaves a non-zero effective interval"); + } + + #[test] + fn test_validate_rekey_unbounded_values_accepted() { + let mut config = Config::default(); + config.node.rekey.after_secs = u64::MAX; + config.node.rekey.after_messages = u64::MAX; + + config + .validate() + .expect("u64::MAX disables an arm of the trigger and must stay legal"); + } + + #[test] + fn test_validate_rekey_checked_even_when_disabled() { + let mut config = Config::default(); + config.node.rekey.enabled = false; + config.node.rekey.after_messages = 0; + + let err = config.validate().expect_err("validation should fail"); + assert!(err.to_string().contains("after_messages")); + + let mut config = Config::default(); + config.node.rekey.enabled = false; + config.node.rekey.after_secs = REKEY_JITTER_SECS.unsigned_abs(); + + let err = config.validate().expect_err("validation should fail"); + assert!(err.to_string().contains("after_secs")); + } + #[test] fn test_outbound_only_forces_ephemeral_bind() { let cfg = UdpConfig { diff --git a/src/node/reject.rs b/src/node/reject.rs index 46a6f74..a0931a6 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -232,7 +232,11 @@ pub enum ForwardingReject { /// `SessionDatagramRef::decode` returned an error. Tracked via /// [`ForwardingStats::decode_error_packets`](crate::node::stats::ForwardingStats). DecodeError, - /// Datagram arrived with TTL=0 — already exhausted, no forward. + /// Transit datagram whose TTL would reach zero on this hop, so it is + /// dropped rather than forwarded. Charged for an arrival at TTL 1 as + /// well as an already-exhausted arrival at TTL 0. Never charged for a + /// datagram addressed to this node, whose delivery is not TTL-gated and + /// is decided ahead of this test. /// Tracked via /// [`ForwardingStats::ttl_exhausted_packets`](crate::node::stats::ForwardingStats). TtlExhausted, diff --git a/src/proto/link.rs b/src/proto/link.rs index c644c32..5144882 100644 --- a/src/proto/link.rs +++ b/src/proto/link.rs @@ -171,19 +171,27 @@ impl SessionDatagram { self } - /// Decrement TTL, returning false if exhausted. + /// Decrement the TTL for a transit hop, returning whether the result may + /// still be transmitted. + /// + /// Follows IP semantics: the decrement happens first, and a datagram that + /// would leave with a TTL of zero is not transmitted. `saturating_sub` + /// folds an already-exhausted arrival (TTL 0) into the same outcome as a + /// last-hop arrival (TTL 1); both leave `ttl` at 0 and return false. + /// + /// This governs forwarding only. Delivery to the addressed node is not + /// TTL-gated and must not consult this method. pub fn decrement_ttl(&mut self) -> bool { - if self.ttl > 0 { - self.ttl -= 1; - true - } else { - false - } + self.ttl = self.ttl.saturating_sub(1); + self.ttl > 0 } - /// Check if the datagram can be forwarded. + /// Check whether this datagram would survive a transit hop. + /// + /// True only at TTL 2 or more: at TTL 1 the decrement leaves zero, so the + /// datagram is dropped rather than forwarded. pub fn can_forward(&self) -> bool { - self.ttl > 0 + self.ttl > 1 } /// Encode as link-layer message (msg_type + ttl + path_mtu + src_addr + dest_addr + payload). @@ -399,4 +407,44 @@ mod tests { assert_eq!(decoded.ttl, hop); } } + + #[test] + fn test_session_datagram_can_forward() { + let dg = SessionDatagram::new(make_node_addr(1), make_node_addr(2), vec![0x42]); + + assert!(!dg.clone().with_ttl(0).can_forward()); + assert!( + !dg.clone().with_ttl(1).can_forward(), + "ttl=1 would leave at zero, so it is not forwardable" + ); + assert!( + dg.clone().with_ttl(2).can_forward(), + "ttl=2 leaves at one, so it is forwardable" + ); + assert!(dg.with_ttl(255).can_forward()); + } + + #[test] + fn test_session_datagram_decrement_ttl() { + let base = SessionDatagram::new(make_node_addr(1), make_node_addr(2), vec![0x42]); + + let mut dg = base.clone().with_ttl(0); + assert!(!dg.decrement_ttl(), "ttl=0 is already exhausted"); + assert_eq!(dg.ttl, 0, "decrement must saturate rather than wrap"); + + let mut dg = base.clone().with_ttl(1); + assert!( + !dg.decrement_ttl(), + "ttl=1 leaves at zero, so it is dropped" + ); + assert_eq!(dg.ttl, 0); + + let mut dg = base.clone().with_ttl(2); + assert!(dg.decrement_ttl()); + assert_eq!(dg.ttl, 1); + + let mut dg = base.with_ttl(64); + assert!(dg.decrement_ttl()); + assert_eq!(dg.ttl, 63); + } }