fix(relay): actually expire channels which allows re-binding them (#4094

) Previously, the relay neither scheduled a `Wake` command nor did it register a `TimedAction` to expire a channel binding. Such an action was only scheduled after the first refresh. This PR fixes this and adds a test that asserts we can re-bind the same channel to a different peer after 15 minutes. Resolves: #3979.
firezone · Mar 12, 2024 · 36848e3 · 36848e3
1 parent 32e16ec
commit 36848e3
Show file tree

Hide file tree

Showing 3 changed files with 148 additions and 14 deletions.
diff --git a/rust/relay/src/server.rs b/rust/relay/src/server.rs
@@ -417,10 +417,13 @@ where
 
                         channel.bound = false;
 
-                        self.time_events.add(
+                        let wake_deadline = self.time_events.add(
                             now + Duration::from_secs(5 * 60),
                             TimedAction::DeleteChannel((client, chan)),
                         );
+                        self.pending_commands.push_back(Command::Wake {
+                            deadline: wake_deadline,
+                        });
                     }
                 }
                 TimedAction::DeleteChannel((client, chan)) => {
@@ -693,10 +696,13 @@ where
 
             tracing::info!(target: "relay", "Refreshed channel binding");
 
-            self.time_events.add(
+            let wake_deadline = self.time_events.add(
                 channel.expiry,
                 TimedAction::UnbindChannel((sender, requested_channel)),
             );
+            self.pending_commands.push_back(Command::Wake {
+                deadline: wake_deadline,
+            });
             self.send_message(
                 channel_bind_success_response(request.transaction_id()),
                 sender,
@@ -870,10 +876,12 @@ where
         id: AllocationId,
         now: SystemTime,
     ) {
+        let expiry = now + CHANNEL_BINDING_DURATION;
+
         let existing = self.channels_by_client_and_number.insert(
             (client, requested_channel),
             Channel {
-                expiry: now + CHANNEL_BINDING_DURATION,
+                expiry,
                 peer_address: peer,
                 allocation: id,
                 bound: true,
@@ -886,6 +894,14 @@ where
             .insert((client, peer), requested_channel);
 
         debug_assert!(existing.is_none());
+
+        let wake_deadline = self.time_events.add(
+            expiry,
+            TimedAction::UnbindChannel((client, requested_channel)),
+        );
+        self.pending_commands.push_back(Command::Wake {
+            deadline: wake_deadline,
+        });
     }
 
     fn send_message(&mut self, message: Message<Attribute>, recipient: ClientSocket) {
@@ -1076,7 +1092,7 @@ impl Allocation {
     }
 }
 
-#[derive(PartialEq)]
+#[derive(PartialEq, Debug)]
 enum TimedAction {
     ExpireAllocation(AllocationId),
     UnbindChannel((ClientSocket, u16)),

diff --git a/rust/relay/src/time_events.rs b/rust/relay/src/time_events.rs
@@ -7,6 +7,7 @@ use std::time::SystemTime;
 ///
 /// It is the caller's responsibility to keep track of actual time passing.
 /// They should call [`TimeEvents::next_trigger`] to find out when to next call [`TimeEvents::pending_actions`].
+#[derive(Debug)]
 pub struct TimeEvents<A> {
     events: Vec<TimeEvent<A>>,
 }
@@ -18,6 +19,7 @@ where
     /// Add an action to be executed at the specified time.
     ///
     /// Returns the new wake deadline for convenience.
+    #[must_use]
     pub fn add(&mut self, trigger: SystemTime, action: A) -> SystemTime {
         self.events.retain(|event| event.action != action);
         self.events.push(TimeEvent {
@@ -85,6 +87,7 @@ impl<A> PartialOrd for TimeEvent<A> {
 }
 
 #[cfg(test)]
+#[allow(unused_must_use)]
 mod tests {
     use super::*;
     use std::time::Duration;

diff --git a/rust/relay/tests/regression.rs b/rust/relay/tests/regression.rs
@@ -291,7 +291,6 @@ fn ping_pong_relay(
     #[strategy(firezone_relay::proptest::transaction_id())] allocate_transaction_id: TransactionId,
     #[strategy(firezone_relay::proptest::transaction_id())]
     channel_bind_transaction_id: TransactionId,
-    #[strategy(firezone_relay::proptest::allocation_lifetime())] lifetime: Lifetime,
     #[strategy(firezone_relay::proptest::username_salt())] username_salt: String,
     #[strategy(firezone_relay::proptest::channel_number())] channel: ChannelNumber,
     source: SocketAddrV4,
@@ -306,6 +305,7 @@ fn ping_pong_relay(
 
     let mut server = TestServer::new(public_relay_addr).with_nonce(nonce);
     let secret = server.auth_secret().to_owned();
+    let lifetime = Lifetime::new(Duration::from_secs(60 * 60)).unwrap(); // Lifetime longer than channel expiry
 
     server.assert_commands(
         from_client(
@@ -350,10 +350,10 @@ fn ping_pong_relay(
             ),
             now,
         ),
-        [send_message(
-            source,
-            channel_bind_response(channel_bind_transaction_id),
-        )],
+        [
+            Wake(now + Duration::from_secs(60 * 10)),
+            send_message(source, channel_bind_response(channel_bind_transaction_id)),
+        ],
     );
 
     let now = now + Duration::from_secs(1);
@@ -376,12 +376,117 @@ fn ping_pong_relay(
     );
 }
 
+#[proptest]
+fn allows_rebind_channel_after_expiry(
+    #[strategy(firezone_relay::proptest::transaction_id())] allocate_transaction_id: TransactionId,
+    #[strategy(firezone_relay::proptest::transaction_id())]
+    channel_bind_transaction_id: TransactionId,
+    #[strategy(firezone_relay::proptest::transaction_id())]
+    channel_bind_2_transaction_id: TransactionId,
+    #[strategy(firezone_relay::proptest::username_salt())] username_salt: String,
+    #[strategy(firezone_relay::proptest::channel_number())] channel: ChannelNumber,
+    source: SocketAddrV4,
+    peer: SocketAddrV4,
+    peer2: SocketAddrV4,
+    public_relay_addr: Ipv4Addr,
+    #[strategy(firezone_relay::proptest::now())] now: SystemTime,
+    #[strategy(firezone_relay::proptest::nonce())] nonce: Uuid,
+) {
+    let _ = env_logger::try_init();
+
+    let mut server = TestServer::new(public_relay_addr).with_nonce(nonce);
+    let secret = server.auth_secret().to_owned();
+    let lifetime = Lifetime::new(Duration::from_secs(60 * 60)).unwrap(); // Lifetime longer than channel expiry
+
+    server.assert_commands(
+        from_client(
+            source,
+            Allocate::new_authenticated_udp_implicit_ip4(
+                allocate_transaction_id,
+                Some(lifetime.clone()),
+                valid_username(now, &username_salt),
+                &secret,
+                nonce,
+            ),
+            now,
+        ),
+        [
+            Wake(now + lifetime.lifetime()),
+            CreateAllocation(49152, AddressFamily::V4),
+            send_message(
+                source,
+                allocate_response(
+                    allocate_transaction_id,
+                    public_relay_addr,
+                    49152,
+                    source,
+                    &lifetime,
+                ),
+            ),
+        ],
+    );
+
+    let now = now + Duration::from_secs(1);
+
+    server.assert_commands(
+        from_client(
+            source,
+            ChannelBind::new(
+                channel_bind_transaction_id,
+                channel,
+                XorPeerAddress::new(peer.into()),
+                valid_username(now, &username_salt),
+                &secret,
+                nonce,
+            ),
+            now,
+        ),
+        [
+            Wake(now + Duration::from_secs(60 * 10)), // For channel expiry
+            send_message(source, channel_bind_response(channel_bind_transaction_id)),
+        ],
+    );
+
+    let now = now + Duration::from_secs(60 * 10 + 1);
+
+    server.assert_commands(
+        forward_time_to(now),
+        [
+            Wake(now + Duration::from_secs(60 * 5)), // For channel deletion
+        ],
+    );
+
+    let now = now + Duration::from_secs(60 * 5 + 1);
+
+    server.assert_commands(forward_time_to(now), []);
+
+    let now = now + Duration::from_secs(1);
+
+    server.assert_commands(
+        from_client(
+            source,
+            ChannelBind::new(
+                channel_bind_2_transaction_id,
+                channel,
+                XorPeerAddress::new(peer2.into()),
+                valid_username(now, &username_salt),
+                &secret,
+                nonce,
+            ),
+            now,
+        ),
+        [
+            Wake(now + Duration::from_secs(60 * 10)), // For channel expiry
+            send_message(source, channel_bind_response(channel_bind_2_transaction_id)),
+        ],
+    );
+}
+
 #[proptest]
 fn ping_pong_ip6_relay(
     #[strategy(firezone_relay::proptest::transaction_id())] allocate_transaction_id: TransactionId,
     #[strategy(firezone_relay::proptest::transaction_id())]
     channel_bind_transaction_id: TransactionId,
-    #[strategy(firezone_relay::proptest::allocation_lifetime())] lifetime: Lifetime,
     #[strategy(firezone_relay::proptest::username_salt())] username_salt: String,
     #[strategy(firezone_relay::proptest::channel_number())] channel: ChannelNumber,
     source: SocketAddrV6,
@@ -398,6 +503,7 @@ fn ping_pong_ip6_relay(
     let mut server =
         TestServer::new((public_relay_ip4_addr, public_relay_ip6_addr)).with_nonce(nonce);
     let secret = server.auth_secret().to_owned();
+    let lifetime = Lifetime::new(Duration::from_secs(60 * 60)).unwrap(); // Lifetime longer than channel expiry
 
     server.assert_commands(
         from_client(
@@ -442,10 +548,10 @@ fn ping_pong_ip6_relay(
             ),
             now,
         ),
-        [send_message(
-            source,
-            channel_bind_response(channel_bind_transaction_id),
-        )],
+        [
+            Wake(now + Duration::from_secs(60 * 10)),
+            send_message(source, channel_bind_response(channel_bind_transaction_id)),
+        ],
     );
 
     let now = now + Duration::from_secs(1);
@@ -580,6 +686,15 @@ impl TestServer {
                         parse_message(&payload)
                     )
                 }
+                (Output::SendMessage((_, message)), Command::Wake { deadline, .. }) => {
+                    panic!(
+                        "Expected `SendMessage({message:?})`, got `Wake({})`",
+                        deadline
+                            .duration_since(SystemTime::UNIX_EPOCH)
+                            .unwrap()
+                            .as_secs(),
+                    )
+                }
                 (
                     Output::SendChannelData((peer, channeldata)),
                     Command::SendMessage { recipient, payload },