Skip to content

Commit

Permalink
Only restart PQ if there was actually a previous handshake
Browse files Browse the repository at this point in the history
There was a potential race condition in our PQ restart logic that could
cause restart to be called erroneously before the initial handshake was
performed. The decision on whether to restart PQ was done based on the
time since last handshake, as reported by the wireguard interface. The
problem was that wireguard will uses None to represent both the case
where no handshake has been performed and the case where a connection
has been rejected after 180 seconds, so our PQ restart logic would
incorrectly trigger if there was a wg consolidation happening between
PQ peer being added and a handshake happening. Now there's an
additional timestamp in the PQ entity to help guide if PQ should be
restarted
  • Loading branch information
Mathias Peters authored and LukasPukenis committed Feb 11, 2025
1 parent 50a5096 commit d7dcc6b
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 1 deletion.
Empty file added .unreleased/LLT-5935
Empty file.
22 changes: 21 additions & 1 deletion crates/telio-pq/src/entity.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
use std::net::SocketAddr;
use std::sync::Arc;
use std::time::{Duration, Instant};

use parking_lot::Mutex;

use telio_model::features::FeaturePostQuantumVPN;
use telio_task::io::chan;
use telio_utils::telio_log_debug;

// This constant is based on section 6.1 of the wireguard whitepaper
// It is the amount of time that has to happen since the last handshake before a connection is abandoned
const REJECT_AFTER_TIME: Duration = Duration::from_secs(180);

struct Peer {
pubkey: telio_crypto::PublicKey,
wg_secret: telio_crypto::SecretKey,
addr: SocketAddr,
/// This is a key rotation task guard, its `Drop` implementation aborts the task
_rotation_task: super::conn::ConnKeyRotation,
keys: Option<super::Keys>,
last_handshake_ts: Option<Instant>,
}

pub struct Entity {
Expand Down Expand Up @@ -53,6 +59,7 @@ impl Entity {
super::Event::Handshake(addr, keys) => {
if peer.addr == addr {
peer.keys = Some(keys);
peer.last_handshake_ts = Some(Instant::now());
}
}
super::Event::Rekey(super::Keys {
Expand Down Expand Up @@ -106,8 +113,20 @@ impl Entity {
}

pub async fn restart(&self) {
let peer = self.peer.lock().take();
let peer = {
let mut peer = self.peer.lock();
let should_restart = peer
.as_ref()
.and_then(|peer| peer.last_handshake_ts)
.is_some_and(|ts| ts.elapsed() > REJECT_AFTER_TIME);
if should_restart {
peer.take()
} else {
None
}
};
if let Some(peer) = peer {
telio_log_debug!("Restarting postquantum entity");
self.start_impl(peer.addr, peer.wg_secret, peer.pubkey)
.await;
}
Expand All @@ -132,6 +151,7 @@ impl Entity {
&self.features,
),
keys: None,
last_handshake_ts: None,
});

#[allow(mpsc_blocking_send)]
Expand Down

0 comments on commit d7dcc6b

Please sign in to comment.