noise: make functions infallible where possible (#366)

cloudflare · Oct 23, 2023 · 62c8873 · 62c8873
1 parent 4de6415
commit 62c8873
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 52 deletions.
diff --git 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
@@ -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
@@ -375,27 +375,27 @@ impl NoiseParams {
         static_public: x25519::PublicKey,
         peer_static_public: x25519::PublicKey,
         preshared_key: Option<[u8; 32]>,
-    ) -> Result<NoiseParams, WireGuardError> {
+    ) -> 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
     fn set_static_private(
         &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, WireGuardError> {
+    ) -> 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
@@ -198,18 +198,17 @@ impl Tunn {
         persistent_keepalive: Option<u16>,
         index: u32,
         rate_limiter: Option<Arc<RateLimiter>>,
-    ) -> Result<Self, &'static str> {
+    ) -> 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<Arc<RateLimiter>>,
-    ) -> 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)
     }