From 62c88737dbe0d4913f40c866fdacdfa272055f63 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 24 Oct 2023 04:39:35 +1100 Subject: [PATCH] noise: make functions infallible where possible (#366) --- boringtun/src/device/mod.rs | 30 ++++++------------------------ boringtun/src/ffi/mod.rs | 7 ++----- boringtun/src/noise/handshake.rs | 19 +++++++++---------- boringtun/src/noise/mod.rs | 21 ++++++++------------- 4 files changed, 25 insertions(+), 52 deletions(-) diff --git a/boringtun/src/device/mod.rs b/boringtun/src/device/mod.rs index bc14c896..04cd7285 100644 --- a/boringtun/src/device/mod.rs +++ b/boringtun/src/device/mod.rs @@ -334,8 +334,7 @@ impl Device { keepalive, next_index, None, - ) - .unwrap(); + ); let peer = Peer::new(tunn, next_index, endpoint, allowed_ips, preshared_key); @@ -453,8 +452,6 @@ impl Device { } fn set_key(&mut self, private_key: x25519::StaticSecret) { - let mut bad_peers = vec![]; - let public_key = x25519::PublicKey::from(&private_key); let key_pair = Some((private_key.clone(), public_key)); @@ -467,30 +464,15 @@ impl Device { let rate_limiter = Arc::new(RateLimiter::new(&public_key, HANDSHAKE_RATE_LIMIT)); for peer in self.peers.values_mut() { - let mut peer_mut = peer.lock(); - - if peer_mut - .tunnel - .set_static_private( - private_key.clone(), - public_key, - Some(Arc::clone(&rate_limiter)), - ) - .is_err() - { - // In case we encounter an error, we will remove that peer - // An error will be a result of bad public key/secret key combination - bad_peers.push(Arc::clone(peer)); - } + peer.lock().tunnel.set_static_private( + private_key.clone(), + public_key, + Some(Arc::clone(&rate_limiter)), + ) } self.key_pair = key_pair; self.rate_limiter = Some(rate_limiter); - - // Remove all the bad peers - for _ in bad_peers { - unimplemented!(); - } } #[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))] diff --git a/boringtun/src/ffi/mod.rs b/boringtun/src/ffi/mod.rs index 3b4e3bb3..1e5a2a9f 100644 --- a/boringtun/src/ffi/mod.rs +++ b/boringtun/src/ffi/mod.rs @@ -292,17 +292,14 @@ pub unsafe extern "C" fn new_tunnel( Some(keep_alive) }; - let tunnel = match Tunn::new( + let tunnel = Box::new(Mutex::new(Tunn::new( private_key, public_key, preshared_key, keep_alive, index, None, - ) { - Ok(t) => Box::new(Mutex::new(t)), - Err(_) => return ptr::null_mut(), - }; + ))); PANIC_HOOK.call_once(|| { // FFI won't properly unwind on panic, but it will if we cause a segmentation fault diff --git a/boringtun/src/noise/handshake.rs b/boringtun/src/noise/handshake.rs index b7c93731..40ed8037 100644 --- a/boringtun/src/noise/handshake.rs +++ b/boringtun/src/noise/handshake.rs @@ -375,19 +375,19 @@ impl NoiseParams { static_public: x25519::PublicKey, peer_static_public: x25519::PublicKey, preshared_key: Option<[u8; 32]>, - ) -> Result { + ) -> NoiseParams { let static_shared = static_private.diffie_hellman(&peer_static_public); let initial_sending_mac_key = b2s_hash(LABEL_MAC1, peer_static_public.as_bytes()); - Ok(NoiseParams { + NoiseParams { static_public, static_private, peer_static_public, static_shared, sending_mac1_key: initial_sending_mac_key, preshared_key, - }) + } } /// Set a new private key @@ -395,7 +395,7 @@ impl NoiseParams { &mut self, static_private: x25519::StaticSecret, static_public: x25519::PublicKey, - ) -> Result<(), WireGuardError> { + ) { // Check that the public key indeed matches the private key let check_key = x25519::PublicKey::from(&static_private); assert_eq!(check_key.as_bytes(), static_public.as_bytes()); @@ -404,7 +404,6 @@ impl NoiseParams { self.static_public = static_public; self.static_shared = self.static_private.diffie_hellman(&self.peer_static_public); - Ok(()) } } @@ -415,15 +414,15 @@ impl Handshake { peer_static_public: x25519::PublicKey, global_idx: u32, preshared_key: Option<[u8; 32]>, - ) -> Result { + ) -> Handshake { let params = NoiseParams::new( static_private, static_public, peer_static_public, preshared_key, - )?; + ); - Ok(Handshake { + Handshake { params, next_index: global_idx, previous: HandshakeState::None, @@ -432,7 +431,7 @@ impl Handshake { stamper: TimeStamper::new(), cookies: Default::default(), last_rtt: None, - }) + } } pub(crate) fn is_in_progress(&self) -> bool { @@ -475,7 +474,7 @@ impl Handshake { &mut self, private_key: x25519::StaticSecret, public_key: x25519::PublicKey, - ) -> Result<(), WireGuardError> { + ) { self.params.set_static_private(private_key, public_key) } diff --git a/boringtun/src/noise/mod.rs b/boringtun/src/noise/mod.rs index 79a6b923..76e377b6 100644 --- a/boringtun/src/noise/mod.rs +++ b/boringtun/src/noise/mod.rs @@ -198,18 +198,17 @@ impl Tunn { persistent_keepalive: Option, index: u32, rate_limiter: Option>, - ) -> Result { + ) -> Self { let static_public = x25519::PublicKey::from(&static_private); - let tunn = Tunn { + Tunn { handshake: Handshake::new( static_private, static_public, peer_static_public, index << 8, preshared_key, - ) - .map_err(|_| "Invalid parameters")?, + ), sessions: Default::default(), current: Default::default(), tx_bytes: Default::default(), @@ -221,9 +220,7 @@ impl Tunn { rate_limiter: rate_limiter.unwrap_or_else(|| { Arc::new(RateLimiter::new(&static_public, PEER_HANDSHAKE_RATE_LIMIT)) }), - }; - - Ok(tunn) + } } /// Update the private key and clear existing sessions @@ -232,17 +229,16 @@ impl Tunn { static_private: x25519::StaticSecret, static_public: x25519::PublicKey, rate_limiter: Option>, - ) -> Result<(), WireGuardError> { + ) { self.timers.should_reset_rr = rate_limiter.is_none(); self.rate_limiter = rate_limiter.unwrap_or_else(|| { Arc::new(RateLimiter::new(&static_public, PEER_HANDSHAKE_RATE_LIMIT)) }); self.handshake - .set_static_private(static_private, static_public)?; + .set_static_private(static_private, static_public); for s in &mut self.sessions { *s = None; } - Ok(()) } /// Encapsulate a single packet from the tunnel interface. @@ -606,10 +602,9 @@ mod tests { let their_public_key = x25519_dalek::PublicKey::from(&their_secret_key); let their_idx = OsRng.next_u32(); - let my_tun = Tunn::new(my_secret_key, their_public_key, None, None, my_idx, None).unwrap(); + let my_tun = Tunn::new(my_secret_key, their_public_key, None, None, my_idx, None); - let their_tun = - Tunn::new(their_secret_key, my_public_key, None, None, their_idx, None).unwrap(); + let their_tun = Tunn::new(their_secret_key, my_public_key, None, None, their_idx, None); (my_tun, their_tun) }