refactor: simplify passive keepalive (#70)

WireGuard has a passive keepalive mechanism. To quote the spec:

> If a peer has received a validly-authenticated transport data message
(section 5.4.6), but does not have any packets
itself to send back for Keepalive-Timeout seconds, it sends a keepalive
message.

This is currently implemented correctly in `boringtun` but it is
somewhat convoluted.

1. On various parts in the code, the internal timers are ticked over.
For example, when we receive keepalive messages or data messages.
2. Whether or not we should send a passive keepalive is tracked in the
`want_keepalive` boolean.
3. This boolean is set whenever we receive _any_ packet (including
keepalives). This is a bug.
4. The above bug is mitigated because of an additional condition that
the last received data packet must be after the last sent packet.
5. Lastly, the `want_keepalive` boolean is checked and directly set to
false as part of our timer code. Combining these two things makes the
code hard to reason about.

We can simplify this greatly by directly tracking the timestamp, when a
keepalive is due. The new `want_keepalive_at` timer is set every time we
receive a data packet and cleared every time we send a packet. In
`update_timers_at`, we simply check if `now` has surpassed that timer
and send a keepalive if that is the case.

As a bonus, this functionality is now also unit-tested.

Author: thomaseizinger

boringtun/src/noise/mod.rs

@@ -717,7 +717,8 @@ mod tests {
     use super::*;
     use rand::{rngs::OsRng, RngCore};
     use timers::{KEEPALIVE_TIMEOUT, MAX_JITTER, REJECT_AFTER_TIME, SHOULD_NOT_USE_AFTER_TIME};
-    use tracing::Level;
+    use tracing::{level_filters::LevelFilter, Level};
+    use tracing_subscriber::util::SubscriberInitExt;
 
     fn create_two_tuns(now: Instant) -> (Tunn, Tunn) {
         let my_secret_key = x25519_dalek::StaticSecret::random_from_rng(OsRng);
@@ -1050,6 +1051,88 @@ mod tests {
         assert_eq!(sent_packet_buf, recv_packet_buf);
     }
 
+    #[test]
+    fn silent_without_application_traffic_and_persistent_keepalive() {
+        let _guard = tracing_subscriber::fmt()
+            .with_test_writer()
+            .with_max_level(LevelFilter::DEBUG)
+            .set_default();
+
+        let mut now = Instant::now();
+
+        let (mut my_tun, mut their_tun) = create_two_tuns_and_handshake(now);
+        assert_eq!(my_tun.persistent_keepalive(), None);
+        their_tun.set_persistent_keepalive(10);
+
+        let mut my_dst = [0u8; 1024];
+        let mut their_dst = [0u8; 1024];
+
+        now += Duration::from_secs(1);
+
+        let sent_packet_buf = create_ipv4_udp_packet();
+
+        // First, perform an application-level handshake.
+
+        {
+            // Send the request.
+
+            let data = my_tun
+                .encapsulate_at(&sent_packet_buf, &mut my_dst, now)
+                .unwrap_network();
+
+            now += Duration::from_secs(1);
+
+            let data = their_tun.decapsulate_at(None, data, &mut their_dst, now);
+            assert!(matches!(data, TunnResult::WriteToTunnelV4(..)));
+        }
+
+        now += Duration::from_secs(1);
+
+        {
+            // Send the response.
+
+            let data = their_tun
+                .encapsulate_at(&sent_packet_buf, &mut their_dst, now)
+                .unwrap_network();
+
+            now += Duration::from_secs(1);
+
+            let data = my_tun.decapsulate_at(None, data, &mut my_dst, now);
+            assert!(matches!(data, TunnResult::WriteToTunnelV4(..)));
+        }
+
+        // Wait for `KEEPALIVE_TIMEOUT`.
+
+        now += KEEPALIVE_TIMEOUT;
+
+        let keepalive = my_tun.update_timers_at(&mut my_dst, now).unwrap_network();
+        parse_keepalive(&mut their_tun, keepalive, now);
+
+        // Idle for 60 seconds.
+
+        for _ in 0..60 {
+            now += Duration::from_secs(1);
+
+            // `my_tun` stays silent (i.e. does not respond to keepalives with keepalives).
+            assert!(matches!(
+                my_tun.update_timers_at(&mut my_dst, now),
+                TunnResult::Done
+            ));
+
+            // `their_tun` will emit persistent keep-alives as we idle.
+            match their_tun.update_timers_at(&mut their_dst, now) {
+                TunnResult::Done => {}
+                TunnResult::Err(wire_guard_error) => panic!("{wire_guard_error}"),
+                TunnResult::WriteToNetwork(keepalive) => {
+                    parse_keepalive(&mut my_tun, keepalive, now)
+                }
+                TunnResult::WriteToTunnelV4(_, _) | TunnResult::WriteToTunnelV6(_, _) => {
+                    unreachable!()
+                }
+            }
+        }
+    }
+
     #[test]
     fn rekey_without_response() {
         let _guard = tracing_subscriber::fmt()

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;
@@ -57,7 +56,9 @@ pub struct Timers {
     is_initiator: bool,
     timers: [Instant; TimerName::Top as usize],
     /// Did we receive data without sending anything back?
-    want_keepalive: bool,
+    ///
+    /// If `Some`, tracks the timestamp when we want to send a keepalive.
+    want_passive_keepalive_at: Option<Instant>,
     /// Did we send data without hearing back?
     ///
     /// If `Some`, tracks the timestamp when we want to initiate the new handshake.
@@ -81,7 +82,7 @@ impl Timers {
         Timers {
             is_initiator: false,
             timers: [now; TimerName::Top as usize],
-            want_keepalive: Default::default(),
+            want_passive_keepalive_at: Default::default(),
             want_handshake_at: Default::default(),
             persistent_keepalive: usize::from(persistent_keepalive.unwrap_or(0)),
             should_reset_rr: reset_rr,
@@ -105,7 +106,7 @@ impl Timers {
             *t = now;
         }
         self.want_handshake_at = None;
-        self.want_keepalive = false;
+        self.want_passive_keepalive_at = None;
     }
 }
 
@@ -126,11 +127,13 @@ impl Tunn {
     pub(super) fn timer_tick(&mut self, timer_name: TimerName, now: Instant) {
         match timer_name {
             TimeLastPacketReceived => {
-                self.timers.want_keepalive = true;
                 self.timers.want_handshake_at = None;
             }
             TimeLastPacketSent => {
-                self.timers.want_keepalive = false;
+                self.timers.want_passive_keepalive_at = None;
+            }
+            TimeLastDataPacketReceived => {
+                self.timers.want_passive_keepalive_at = Some(now + KEEPALIVE_TIMEOUT);
             }
             TimeLastDataPacketSent => {
                 match self.timers.want_handshake_at {
@@ -231,7 +234,6 @@ impl Tunn {
         // 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;
@@ -325,9 +327,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
+                    .want_passive_keepalive_at
+                    .is_some_and(|keepalive_at| now >= keepalive_at)
                 {
                     tracing::debug!("KEEPALIVE(KEEPALIVE_TIMEOUT)");
                     keepalive_required = true;
@@ -401,4 +404,9 @@ impl Tunn {
             None
         }
     }
+
+    #[cfg(test)]
+    pub fn set_persistent_keepalive(&mut self, keepalive: u16) {
+        self.timers.persistent_keepalive = keepalive as usize;
+    }
 }