From aebd871bfe783e4b918110e765d663f374fe50bd Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:27:47 +0100 Subject: [PATCH 01/13] Extract handshake-rekey timer --- boringtun/src/noise/handshake.rs | 13 +++++++------ boringtun/src/noise/timers.rs | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/boringtun/src/noise/handshake.rs b/boringtun/src/noise/handshake.rs index 0e555302..01098fbd 100644 --- a/boringtun/src/noise/handshake.rs +++ b/boringtun/src/noise/handshake.rs @@ -1,7 +1,7 @@ // Copyright (c) 2019 Cloudflare, Inc. All rights reserved. // SPDX-License-Identifier: BSD-3-Clause -use super::timers::COOKIE_EXPIRATION_TIME; +use super::timers::{COOKIE_EXPIRATION_TIME, REKEY_TIMEOUT}; use super::{HandshakeInit, HandshakeResponse, PacketCookieReply}; use crate::noise::errors::WireGuardError; use crate::noise::session::Session; @@ -441,11 +441,12 @@ impl Handshake { !matches!(self.state, HandshakeState::None | HandshakeState::Expired) } - pub(crate) fn timer(&self) -> Option { - match self.state { - HandshakeState::InitSent(HandshakeInitSentState { time_sent, .. }) => Some(time_sent), - _ => None, - } + pub(crate) fn rekey_timeout(&self) -> Option { + let HandshakeState::InitSent(HandshakeInitSentState { time_sent, .. }) = self.state else { + return None; + }; + + Some(time_sent + REKEY_TIMEOUT) } pub(crate) fn set_expired(&mut self) { diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 3ba0bbcd..1bd2d71c 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -245,7 +245,7 @@ impl Tunn { return TunnResult::Err(WireGuardError::ConnectionExpired); } - if let Some(time_init_sent) = self.handshake.timer() { + if let Some(rekey_timeout) = self.handshake.rekey_timeout() { // Handshake Initiation Retransmission if now - handshake_started >= REKEY_ATTEMPT_TIME { // After REKEY_ATTEMPT_TIME ms of trying to initiate a new handshake, @@ -258,7 +258,7 @@ impl Tunn { return TunnResult::Err(WireGuardError::ConnectionExpired); } - if now.duration_since(time_init_sent) >= REKEY_TIMEOUT { + if now >= rekey_timeout { // We avoid using `time` here, because it can be earlier than `time_init_sent`. // Once `checked_duration_since` is stable we can use that. // A handshake initiation is retried after REKEY_TIMEOUT + jitter ms, From 80e34b2cdf75e32b2ce52a36bda43922f6383229 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:30:09 +0100 Subject: [PATCH 02/13] Extract reject-after-time timer --- boringtun/src/noise/timers.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 1bd2d71c..8c606816 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -80,6 +80,10 @@ impl Timers { } } + pub(crate) fn reject_after_time(&self) -> Instant { + self[TimeSessionEstablished] + REJECT_AFTER_TIME * 3 + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -238,7 +242,7 @@ impl Tunn { // All ephemeral private keys and symmetric session keys are zeroed out after // (REJECT_AFTER_TIME * 3) ms if no new keys have been exchanged. - if now - session_established >= REJECT_AFTER_TIME * 3 { + if now >= self.timers.reject_after_time() { tracing::debug!("CONNECTION_EXPIRED(REJECT_AFTER_TIME * 3)"); self.handshake.set_expired(); self.clear_all(); From 9539f4b705a173d0395232093fd9dbdf13b4297a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 13 Jan 2025 16:43:06 +0100 Subject: [PATCH 03/13] Extract rekey-attempt timer --- boringtun/src/noise/timers.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 8c606816..ab583202 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -84,6 +84,10 @@ impl Timers { self[TimeSessionEstablished] + REJECT_AFTER_TIME * 3 } + pub(crate) fn rekey_attempt_time(&self) -> Instant { + self[TimeLastHandshakeStarted] + REKEY_ATTEMPT_TIME + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -251,7 +255,8 @@ impl Tunn { if let Some(rekey_timeout) = self.handshake.rekey_timeout() { // Handshake Initiation Retransmission - if now - handshake_started >= REKEY_ATTEMPT_TIME { + // Only applies if we initiated a handshake (and thus `rekey_timeout` is `Some`) + if now >= self.timers.rekey_attempt_time() { // After REKEY_ATTEMPT_TIME ms of trying to initiate a new handshake, // the retries give up and cease, and clear all existing packets queued // up to be sent. If a packet is explicitly queued up to be sent, then From 2e6a55e6049ef90cdc7e80393037793478df2574 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:34:24 +0100 Subject: [PATCH 04/13] Extract rekey-after-time-on-send timer --- boringtun/src/noise/timers.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index ab583202..7f2237c3 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -88,6 +88,17 @@ impl Timers { self[TimeLastHandshakeStarted] + REKEY_ATTEMPT_TIME } + pub(crate) fn rekey_after_time_on_send(&self) -> Option { + let session_established = self[TimeSessionEstablished]; + + if session_established >= self[TimeLastDataPacketSent] { + // If we haven't sent any data yet, this timer doesn't matter. + return None; + } + + Some(session_established + REKEY_AFTER_TIME) + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -283,8 +294,10 @@ impl Tunn { // ms old, we initiate a new handshake. If the sender was the original // responder of the handshake, it does not re-initiate a new handshake // after REKEY_AFTER_TIME ms like the original initiator does. - if session_established < data_packet_sent - && now - session_established >= REKEY_AFTER_TIME + if self + .timers + .rekey_after_time_on_send() + .is_some_and(|deadline| now >= deadline) { tracing::debug!("HANDSHAKE(REKEY_AFTER_TIME (on send))"); handshake_initiation_required = true; From 069576f3f2a37a14c69cf58f2f844dcc2e72be98 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:36:10 +0100 Subject: [PATCH 05/13] Extract reject-after-time-on-receive timer --- boringtun/src/noise/timers.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 7f2237c3..33de72d9 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -99,6 +99,17 @@ impl Timers { Some(session_established + REKEY_AFTER_TIME) } + pub(crate) fn reject_after_time_on_receive(&self) -> Option { + let session_established = self[TimeSessionEstablished]; + + if session_established >= self[TimeLastDataPacketReceived] { + // If we haven't received any data yet, this timer doesn't matter. + return None; + } + + Some(session_established + REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT) + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -307,9 +318,10 @@ impl Tunn { // of the handshake and if the current session key is REJECT_AFTER_TIME // - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT ms old, we initiate a new // handshake. - if session_established < data_packet_received - && now - session_established - >= REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT + if self + .timers + .reject_after_time_on_receive() + .is_some_and(|deadline| now >= deadline) { tracing::debug!( "HANDSHAKE(REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT (on receive))" From 24038d6b6efe7f7c30b9330e0734eb9983219337 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:36:50 +0100 Subject: [PATCH 06/13] Extract rekey-after-time-without-response timer --- boringtun/src/noise/timers.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 33de72d9..cc554f6a 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -110,6 +110,12 @@ impl Timers { Some(session_established + REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT) } + pub(crate) fn rekey_after_time_without_response(&self) -> Option { + let first_packet_without_reply = self.want_handshake_since?; + + Some(first_packet_without_reply + KEEPALIVE_TIMEOUT + REKEY_TIMEOUT) + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -335,8 +341,8 @@ impl Tunn { // we initiate a new handshake. if self .timers - .want_handshake_since - .is_some_and(|sent_at| now >= sent_at + KEEPALIVE_TIMEOUT + REKEY_TIMEOUT) + .rekey_after_time_without_response() + .is_some_and(|deadline| now >= deadline) { tracing::debug!("HANDSHAKE(KEEPALIVE + REKEY_TIMEOUT)"); handshake_initiation_required = true; From f5d18d340890b384e20c1673fab7720247050e9a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:37:34 +0100 Subject: [PATCH 07/13] Extract keepalive-after-time-without-send timer --- boringtun/src/noise/timers.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index cc554f6a..8d30c077 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -116,6 +116,22 @@ impl Timers { Some(first_packet_without_reply + KEEPALIVE_TIMEOUT + REKEY_TIMEOUT) } + pub(crate) fn keepalive_after_time_without_send(&self) -> Option { + if !self.want_keepalive { + return None; + } + + let last_packet_sent = self[TimeLastPacketSent]; + let last_data_packet_received = self[TimeLastDataPacketReceived]; + + if last_packet_sent >= last_data_packet_received { + // If we have sent a packet since we have last received one, this timer doesn't matter. + return None; + } + + Some(last_packet_sent + KEEPALIVE_TIMEOUT) + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -351,9 +367,10 @@ impl Tunn { if !handshake_initiation_required { // If a packet has been received from a given peer, but we have not sent one back // to the given peer in KEEPALIVE ms, we send an empty packet. - if data_packet_received > aut_packet_sent - && now - aut_packet_sent >= KEEPALIVE_TIMEOUT - && mem::replace(&mut self.timers.want_keepalive, false) + if self + .timers + .keepalive_after_time_without_send() + .is_some_and(|deadline| now >= deadline) { tracing::debug!("KEEPALIVE(KEEPALIVE_TIMEOUT)"); keepalive_required = true; From 5ec4311bca0e5cb6294bd7ee6606db1cf90cccdb Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:38:12 +0100 Subject: [PATCH 08/13] Extract persistent keep-alive timer --- boringtun/src/noise/timers.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 8d30c077..1a0096ff 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -132,6 +132,16 @@ impl Timers { Some(last_packet_sent + KEEPALIVE_TIMEOUT) } + pub(crate) fn next_persistent_keepalive(&self) -> Option { + let keepalive = Duration::from_secs(self.persistent_keepalive as u64); + + if keepalive.is_zero() { + return None; + } + + Some(self[TimerName::TimePersistentKeepalive] + keepalive) + } + fn is_initiator(&self) -> bool { self.is_initiator } @@ -377,9 +387,10 @@ impl Tunn { } // Persistent KEEPALIVE - if persistent_keepalive > 0 - && (now - self.timers[TimePersistentKeepalive] - >= Duration::from_secs(persistent_keepalive as _)) + if self + .timers + .next_persistent_keepalive() + .is_some_and(|deadline| now >= deadline) { tracing::debug!("KEEPALIVE(PERSISTENT_KEEPALIVE)"); self.timer_tick(TimePersistentKeepalive, now); @@ -399,7 +410,6 @@ impl Tunn { existing.is_none(), "Should never override existing handshake" ); - tracing::debug!(?jitter, "Scheduling new handshake"); return TunnResult::Done; From 151983a43f3f4210e51a085df8543da448d24874 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:34:53 +0100 Subject: [PATCH 09/13] Delete old timers --- boringtun/src/noise/timers.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 1a0096ff..7e124681 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -276,14 +276,6 @@ impl Tunn { handshake_initiation_required = true; } - // Load timers only once: - let session_established = self.timers[TimeSessionEstablished]; - let handshake_started = self.timers[TimeLastHandshakeStarted]; - let aut_packet_sent = self.timers[TimeLastPacketSent]; - let data_packet_received = self.timers[TimeLastDataPacketReceived]; - let data_packet_sent = self.timers[TimeLastDataPacketSent]; - let persistent_keepalive = self.timers.persistent_keepalive; - if self.handshake.is_expired() { return TunnResult::Err(WireGuardError::ConnectionExpired); } From 2bc194bc80e1419088da7047c528e991cd9a6068 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 14 Jan 2025 23:35:09 +0100 Subject: [PATCH 10/13] Remove unused import --- boringtun/src/noise/timers.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 7e124681..377ccfc9 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -3,7 +3,6 @@ use super::errors::WireGuardError; use crate::noise::{Tunn, TunnResult, N_SESSIONS}; -use std::mem; use std::ops::{Index, IndexMut}; use rand::Rng; From a3789f5fe77d32b1c7c5c1cdbb7c4ab827be811a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 13 Jan 2025 16:56:20 +0100 Subject: [PATCH 11/13] Move initiator condition into timer fn --- boringtun/src/noise/timers.rs | 64 ++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 377ccfc9..c8e8b2b5 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -88,6 +88,11 @@ impl Timers { } pub(crate) fn rekey_after_time_on_send(&self) -> Option { + if !self.is_initiator { + // If we aren't the initiator of the current session, this timer does not matter. + return None; + } + let session_established = self[TimeSessionEstablished]; if session_established >= self[TimeLastDataPacketSent] { @@ -99,6 +104,11 @@ impl Timers { } pub(crate) fn reject_after_time_on_receive(&self) -> Option { + if !self.is_initiator { + // If we aren't the initiator of the current session, this timer does not matter. + return None; + } + let session_established = self[TimeSessionEstablished]; if session_established >= self[TimeLastDataPacketReceived] { @@ -322,35 +332,33 @@ impl Tunn { handshake_initiation_required = true; } } else { - if self.timers.is_initiator() { - // After sending a packet, if the sender was the original initiator - // of the handshake and if the current session key is REKEY_AFTER_TIME - // ms old, we initiate a new handshake. If the sender was the original - // responder of the handshake, it does not re-initiate a new handshake - // after REKEY_AFTER_TIME ms like the original initiator does. - if self - .timers - .rekey_after_time_on_send() - .is_some_and(|deadline| now >= deadline) - { - tracing::debug!("HANDSHAKE(REKEY_AFTER_TIME (on send))"); - handshake_initiation_required = true; - } + // After sending a packet, if the sender was the original initiator + // of the handshake and if the current session key is REKEY_AFTER_TIME + // ms old, we initiate a new handshake. If the sender was the original + // responder of the handshake, it does not re-initiate a new handshake + // after REKEY_AFTER_TIME ms like the original initiator does. + if self + .timers + .rekey_after_time_on_send() + .is_some_and(|deadline| now >= deadline) + { + tracing::debug!("HANDSHAKE(REKEY_AFTER_TIME (on send))"); + handshake_initiation_required = true; + } - // After receiving a packet, if the receiver was the original initiator - // of the handshake and if the current session key is REJECT_AFTER_TIME - // - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT ms old, we initiate a new - // handshake. - if self - .timers - .reject_after_time_on_receive() - .is_some_and(|deadline| now >= deadline) - { - tracing::debug!( - "HANDSHAKE(REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT (on receive))" - ); - handshake_initiation_required = true; - } + // After receiving a packet, if the receiver was the original initiator + // of the handshake and if the current session key is REJECT_AFTER_TIME + // - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT ms old, we initiate a new + // handshake. + if self + .timers + .reject_after_time_on_receive() + .is_some_and(|deadline| now >= deadline) + { + tracing::debug!( + "HANDSHAKE(REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT (on receive))" + ); + handshake_initiation_required = true; } // If we have sent a packet to a given peer but have not received a From 8bde5a3ad30369a190d0578dc1f86c92933f89a2 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 13 Jan 2025 19:56:22 +0100 Subject: [PATCH 12/13] Return next scheduled update from `next_timer_update` --- boringtun/src/noise/timers.rs | 42 ++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index c8e8b2b5..262ec37d 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -242,6 +242,13 @@ impl Tunn { } } + fn next_expired_session(&self) -> Option { + self.sessions + .iter() + .flat_map(|s| Some(s.as_ref()?.established_at() + REJECT_AFTER_TIME)) + .min() + } + #[deprecated(note = "Prefer `Timers::update_timers_at` to avoid time-impurity")] pub fn update_timers<'a>(&mut self, dst: &'a mut [u8]) -> TunnResult<'a> { self.update_timers_at(dst, Instant::now()) @@ -425,7 +432,36 @@ impl Tunn { /// /// If this returns `None`, you may call it at your usual desired precision (usually once a second is enough). pub fn next_timer_update(&self) -> Option { - self.timers.send_handshake_at + // Mimic the `update_timers_at` function: If we have a handshake scheduled, other timers don't matter. + if let Some(scheduled_handshake) = self.timers.send_handshake_at { + return Some(scheduled_handshake); + } + + let common_timers = [ + self.next_expired_session(), + self.handshake.cookie_expiration(), + Some(self.timers.reject_after_time()), + ] + .into_iter(); + + if let Some(rekey_timeout) = self.handshake.rekey_timeout() { + earliest( + common_timers.chain([Some(rekey_timeout), Some(self.timers.rekey_attempt_time())]), + ) + } else { + // Persistent keep-alive only makes sense if the current session is active. + let persistent_keepalive = self.sessions[self.current % N_SESSIONS] + .as_ref() + .and_then(|_| self.timers.next_persistent_keepalive()); + + earliest(common_timers.chain([ + self.timers.rekey_after_time_on_send(), + self.timers.reject_after_time_on_receive(), + self.timers.rekey_after_time_without_response(), + self.timers.keepalive_after_time_without_send(), + persistent_keepalive, + ])) + } } #[deprecated(note = "Prefer `Tunn::time_since_last_handshake_at` to avoid time-impurity")] @@ -454,3 +490,7 @@ impl Tunn { } } } + +fn earliest(instants: impl IntoIterator>) -> Option { + instants.into_iter().flatten().min() +} From 7674b90efd65cd136448483914821fecc5ec6abb Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 15 Jan 2025 15:23:38 +0100 Subject: [PATCH 13/13] dbg timers --- boringtun/src/noise/timers.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/boringtun/src/noise/timers.rs b/boringtun/src/noise/timers.rs index 262ec37d..9072cc67 100644 --- a/boringtun/src/noise/timers.rs +++ b/boringtun/src/noise/timers.rs @@ -255,6 +255,21 @@ impl Tunn { } pub fn update_timers_at<'a>(&mut self, dst: &'a mut [u8], now: Instant) -> TunnResult<'a> { + tracing::debug!(send_handshake_at = ?self.timers.send_handshake_at); + tracing::debug!(next_expired_session = ?self.next_expired_session()); + tracing::debug!(cookie_expiration = ?self.handshake.cookie_expiration()); + tracing::debug!(reject_after_time = ?self.timers.reject_after_time()); + + tracing::debug!(rekey_timeout = ?self.handshake.rekey_timeout()); + tracing::debug!(rekey_attempt_time = ?self.timers.rekey_attempt_time()); + + tracing::debug!(rekey_after_time_on_send = ?self.timers.rekey_after_time_on_send()); + tracing::debug!(want_handshake_since = ?self.timers.want_handshake_since); + tracing::debug!(reject_after_time_on_receive = ?self.timers.reject_after_time_on_receive()); + tracing::debug!(rekey_after_time_without_response = ?self.timers.rekey_after_time_without_response()); + tracing::debug!(keepalive_after_time_without_send = ?self.timers.keepalive_after_time_without_send()); + tracing::debug!(next_persistent_keepalive = ?self.timers.next_persistent_keepalive()); + if let Some(scheduled_handshake_at) = self.timers.send_handshake_at { // If we have scheduled a handshake and the deadline expired, send it immediately. if now >= scheduled_handshake_at {