(1) break out Error for readability, (2) remove NOP packet type since it is no longer useful.

zssp/src/constants.rs

@@ -79,9 +79,8 @@ pub(crate) const COUNTER_MAX_ALLOWED_OOO: usize = 16;
 
 // Packet types can range from 0 to 15 (4 bits) -- 0-3 are defined and 4-15 are reserved for future use
 pub(crate) const PACKET_TYPE_DATA: u8 = 0;
-pub(crate) const PACKET_TYPE_NOP: u8 = 1;
+pub(crate) const PACKET_TYPE_INITIAL_KEY_OFFER: u8 = 1; // "alice"
-pub(crate) const PACKET_TYPE_INITIAL_KEY_OFFER: u8 = 2; // "alice"
+pub(crate) const PACKET_TYPE_KEY_COUNTER_OFFER: u8 = 2; // "bob"
-pub(crate) const PACKET_TYPE_KEY_COUNTER_OFFER: u8 = 3; // "bob"
 
 // Key usage labels for sub-key derivation using NIST-style KBKDF (basically just HMAC KDF).
 pub(crate) const KBKDF_KEY_USAGE_LABEL_HMAC: u8 = b'M'; // HMAC-SHA384 authentication for key exchanges

zssp/src/error.rs

@@ -0,0 +1,75 @@
+use crate::sessionid::SessionId;
+
+pub enum Error {
+    /// The packet was addressed to an unrecognized local session (should usually be ignored).
+    UnknownLocalSessionId(SessionId),
+
+    /// Packet was not well formed.
+    InvalidPacket,
+
+    /// An invalid parameter was supplied to the function.
+    InvalidParameter,
+
+    /// Packet failed one or more authentication (MAC) checks.
+    ///
+    /// **IMPORTANT**: Do not reply to a peer who has sent a packet that has failed authentication.
+    /// Any response at all will leak to an attacker what authentication step their packet failed at
+    /// (timing attack), which lowers the total authentication entropy they have to brute force.
+    /// There is a safe way to reply if absolutely necessary; by sending the reply back after a constant
+    /// amount of time, but this is very difficult to get correct.
+    FailedAuthentication,
+
+    /// New session was rejected by the application layer.
+    NewSessionRejected,
+
+    /// Rekeying failed and session secret has reached its hard usage count limit.
+    MaxKeyLifetimeExceeded,
+
+    /// Attempt to send using session without established key.
+    SessionNotEstablished,
+
+    /// Packet ignored by rate limiter.
+    RateLimited,
+
+    /// The other peer specified an unrecognized protocol version.
+    UnknownProtocolVersion,
+
+    /// Caller supplied data buffer is too small to receive data.
+    DataBufferTooSmall,
+
+    /// Data object is too large to send, even with fragmentation.
+    DataTooLarge,
+
+    /// An unexpected buffer overrun occured while attempting to encode or decode a packet.
+    ///
+    /// This can only ever happen if exceptionally large key blobs or metadata are being used,
+    /// or as the result of an internal encoding bug.
+    UnexpectedBufferOverrun,
+}
+
+impl std::fmt::Display for Error {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::UnknownLocalSessionId(id) => f.write_str(format!("UnknownLocalSessionId({})", id).as_str()),
+            Self::InvalidPacket => f.write_str("InvalidPacket"),
+            Self::InvalidParameter => f.write_str("InvalidParameter"),
+            Self::FailedAuthentication => f.write_str("FailedAuthentication"),
+            Self::NewSessionRejected => f.write_str("NewSessionRejected"),
+            Self::MaxKeyLifetimeExceeded => f.write_str("MaxKeyLifetimeExceeded"),
+            Self::SessionNotEstablished => f.write_str("SessionNotEstablished"),
+            Self::RateLimited => f.write_str("RateLimited"),
+            Self::UnknownProtocolVersion => f.write_str("UnknownProtocolVersion"),
+            Self::DataBufferTooSmall => f.write_str("DataBufferTooSmall"),
+            Self::DataTooLarge => f.write_str("DataTooLarge"),
+            Self::UnexpectedBufferOverrun => f.write_str("UnexpectedBufferOverrun"),
+        }
+    }
+}
+
+impl std::error::Error for Error {}
+
+impl std::fmt::Debug for Error {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        std::fmt::Display::fmt(self, f)
+    }
+}

zssp/src/lib.rs

@@ -1,5 +1,6 @@
 mod applicationlayer;
 mod counter;
+mod error;
 mod sessionid;
 mod tests;
 mod zssp;
@@ -7,5 +8,6 @@ mod zssp;
 pub mod constants;
 
 pub use crate::applicationlayer::ApplicationLayer;
+pub use crate::error::Error;
 pub use crate::sessionid::SessionId;
-pub use crate::zssp::{Error, ReceiveContext, ReceiveResult, Session};
+pub use crate::zssp::{ReceiveContext, ReceiveResult, Session};

zssp/src/zssp.rs

@@ -22,55 +22,9 @@ use zerotier_utils::varint;
 use crate::applicationlayer::ApplicationLayer;
 use crate::constants::*;
 use crate::counter::{Counter, CounterValue, CounterWindow};
+use crate::error::Error;
 use crate::sessionid::SessionId;
 
-pub enum Error {
-    /// The packet was addressed to an unrecognized local session (should usually be ignored).
-    UnknownLocalSessionId(SessionId),
-
-    /// Packet was not well formed.
-    InvalidPacket,
-
-    /// An invalid parameter was supplied to the function.
-    InvalidParameter,
-
-    /// Packet failed one or more authentication (MAC) checks.
-    ///
-    /// **IMPORTANT**: Do not reply to a peer who has sent a packet that has failed authentication.
-    /// Any response at all will leak to an attacker what authentication step their packet failed at
-    /// (timing attack), which lowers the total authentication entropy they have to brute force.
-    /// There is a safe way to reply if absolutely necessary; by sending the reply back after a constant
-    /// amount of time, but this is very difficult to get correct.
-    FailedAuthentication,
-
-    /// New session was rejected by the application layer.
-    NewSessionRejected,
-
-    /// Rekeying failed and session secret has reached its hard usage count limit.
-    MaxKeyLifetimeExceeded,
-
-    /// Attempt to send using session without established key.
-    SessionNotEstablished,
-
-    /// Packet ignored by rate limiter.
-    RateLimited,
-
-    /// The other peer specified an unrecognized protocol version.
-    UnknownProtocolVersion,
-
-    /// Caller supplied data buffer is too small to receive data.
-    DataBufferTooSmall,
-
-    /// Data object is too large to send, even with fragmentation.
-    DataTooLarge,
-
-    /// An unexpected buffer overrun occured while attempting to encode or decode a packet.
-    ///
-    /// This can only ever happen if exceptionally large key blobs or metadata are being used,
-    /// or as the result of an internal encoding bug.
-    UnexpectedBufferOverrun,
-}
-
 /// Result generated by the packet receive function, with possible payloads.
 pub enum ReceiveResult<'a, H: ApplicationLayer> {
     /// Packet is valid, no action needs to be taken.
@@ -99,7 +53,7 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> {
 /// initiated a session or a rekey event. Initiator is Alice, responder is Bob.
 ///
 /// We require that after every rekeying event Alice and Bob switch roles.
-#[derive(PartialEq)]
+#[derive(PartialEq, Eq)]
 pub enum Role {
     Alice,
     Bob,
@@ -138,7 +92,7 @@ pub struct Session<Application: ApplicationLayer> {
 struct SessionMutableState {
     remote_session_id: Option<SessionId>,  // The other side's 48-bit session ID
     send_counters: [Counter; 2],           // Outgoing packet counter and nonce state, starts at 1
-    session_keys: [Option<SessionKey>; 2],                // Buffers to store current, next, and last active key
+    session_keys: [Option<SessionKey>; 2], // Buffers to store current and previous session key
     cur_session_key_id: bool,              // Pointer used for keys[] circular buffer
     offer: Option<EphemeralOffer>,         // Most recent ephemeral offer sent to remote
     last_remote_offer: i64,                // Time of most recent ephemeral offer (ms)
@@ -184,33 +138,6 @@ struct EphemeralOffer {
 #[repr(C, packed)]
 struct CanonicalHeader(pub u64, pub u32);
 
-impl std::fmt::Display for Error {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Self::UnknownLocalSessionId(id) => f.write_str(format!("UnknownLocalSessionId({})", id).as_str()),
-            Self::InvalidPacket => f.write_str("InvalidPacket"),
-            Self::InvalidParameter => f.write_str("InvalidParameter"),
-            Self::FailedAuthentication => f.write_str("FailedAuthentication"),
-            Self::NewSessionRejected => f.write_str("NewSessionRejected"),
-            Self::MaxKeyLifetimeExceeded => f.write_str("MaxKeyLifetimeExceeded"),
-            Self::SessionNotEstablished => f.write_str("SessionNotEstablished"),
-            Self::RateLimited => f.write_str("RateLimited"),
-            Self::UnknownProtocolVersion => f.write_str("UnknownProtocolVersion"),
-            Self::DataBufferTooSmall => f.write_str("DataBufferTooSmall"),
-            Self::DataTooLarge => f.write_str("DataTooLarge"),
-            Self::UnexpectedBufferOverrun => f.write_str("UnexpectedBufferOverrun"),
-        }
-    }
-}
-
-impl std::error::Error for Error {}
-
-impl std::fmt::Debug for Error {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        std::fmt::Display::fmt(self, f)
-    }
-}
-
 impl<Application: ApplicationLayer> Session<Application> {
     /// Create a new session and send an initial key offer message to the other end.
     ///
@@ -324,7 +251,7 @@ impl<Application: ApplicationLayer> Session<Application> {
                     PACKET_TYPE_DATA,
                     remote_session_id.into(),
                     counter,
-                    key_id
+                    key_id,
                 )?;
 
                 // Get an initialized AES-GCM cipher and re-initialize with a 96-bit IV built from remote session ID,
@@ -394,7 +321,12 @@ impl<Application: ApplicationLayer> Session<Application> {
         let state = self.state.read().unwrap();
         let key_id = state.cur_session_key_id;
         if let Some(key) = state.session_keys[key_id as usize].as_ref() {
-            Some((key.secret_fingerprint, key.creation_time, self.ratchet_counts[key_id as usize].load(Ordering::Relaxed), key.jedi))
+            Some((
+                key.secret_fingerprint,
+                key.creation_time,
+                self.ratchet_counts[key_id as usize].load(Ordering::Relaxed),
+                key.jedi,
+            ))
         } else {
             None
         }
@@ -419,10 +351,13 @@ impl<Application: ApplicationLayer> Session<Application> {
     ) {
         let state = self.state.read().unwrap();
         let current_key_id = state.cur_session_key_id;
-        if (state.session_keys[state.cur_session_key_id as usize]
+        if (state.session_keys[state.cur_session_key_id as usize].as_ref().map_or(true, |key| {
-                .as_ref()
+            key.role == Role::Bob
-                .map_or(true, |key| key.role == Role::Bob && (assume_key_is_too_old || key.lifetime.should_rekey(state.send_counters[current_key_id as usize].current(), current_time))))
+                && (assume_key_is_too_old
-            && state
+                    || key
+                        .lifetime
+                        .should_rekey(state.send_counters[current_key_id as usize].current(), current_time))
+        })) && state
             .offer
             .as_ref()
             .map_or(true, |o| (current_time - o.creation_time) > Application::REKEY_RATE_LIMIT_MS)
@@ -450,7 +385,11 @@ impl<Application: ApplicationLayer> Session<Application> {
                     &self.noise_ss,
                     state.session_keys[current_key_id as usize].as_ref(),
                     self.ratchet_counts[current_key_id as usize].load(Ordering::Relaxed),
-                    if has_existing_session { Some(&self.header_check_cipher) } else { None },
+                    if has_existing_session {
+                        Some(&self.header_check_cipher)
+                    } else {
+                        None
+                    },
                     mtu,
                     current_time,
                     &mut offer,
@@ -643,12 +582,7 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
         current_time: i64,
     ) -> Result<ReceiveResult<'a, Application>, Error> {
         debug_assert!(fragments.len() >= 1);
-
+        if packet_type == PACKET_TYPE_DATA {
-        // The first 'if' below should capture both DATA and NOP but not other types. Sanity check this.
-        debug_assert_eq!(PACKET_TYPE_DATA, 0);
-        debug_assert_eq!(PACKET_TYPE_NOP, 1);
-
-        if packet_type <= PACKET_TYPE_NOP {
             if let Some(session) = session {
                 let state = session.state.read().unwrap();
                 if let Some(session_key) = state.session_keys[key_id as usize].as_ref() {
@@ -699,12 +633,7 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
 
                     if aead_authentication_ok {
                         if session.receive_windows[key_id as usize].message_authenticated(counter) {
-                            if packet_type == PACKET_TYPE_DATA {
                             return Ok(ReceiveResult::OkData(&mut data_buf[..data_len]));
-                            } else {
-                                unlikely_branch();
-                                return Ok(ReceiveResult::Ok);
-                            }
                         }
                     }
                 }
@@ -907,7 +836,7 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
                                     state: RwLock::new(SessionMutableState {
                                         send_counters: [send_counter, Counter::new()],
                                         remote_session_id: Some(alice_session_id),
-                                        session_keys: [None, None],//this is the only value which will be writen later
+                                        session_keys: [None, None], //this is the only value which will be writen later
                                         cur_session_key_id: false,
                                         offer: None,
                                         last_remote_offer: current_time,
@@ -920,7 +849,7 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
                                 }),
                                 reply_counter,
                                 false,
-                                None
+                                None,
                             )
                         } else {
                             return Err(Error::NewSessionRejected);
@@ -1052,18 +981,14 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
                     let packet_end = idx;
                     if session.receive_windows[key_id as usize].message_authenticated(counter) {
                         // Initial key offers should only check this if this is a rekey
-                        let session_key = SessionKey::new(
+                        let session_key = SessionKey::new(session_key, Role::Bob, current_time, hybrid_kk.is_some());
-                            session_key,
-                            Role::Bob,
-                            current_time,
-                            hybrid_kk.is_some(),
-                        );
 
                         let mut state = session.state.write().unwrap();
                         let _ = state.session_keys[new_key_id as usize].replace(session_key);
                         let _ = state.remote_session_id.replace(alice_session_id);
                         let ratchet_count = session.ratchet_counts[key_id as usize].load(Ordering::SeqCst);
-                        if state.cur_session_key_id != new_key_id {//this prevents anything from being reset twice if the key offer was made twice
+                        if state.cur_session_key_id != new_key_id {
+                            //this prevents anything from being reset twice if the key offer was made twice
                             debug_assert!(new_key_id != key_id);
                             // receive_windows only has race conditions with the counter of the remote party. It is theoretically possible that the local host receives counters under new_key_id while the receive_window is still in the process of resetting, but this is very unlikely. If it does happen, two things could happen:
                             // 1) The received counter is less than what is currently stored in the window, so a valid packet would be rejected.
@@ -1091,7 +1016,6 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
                     } else {
                         return Ok(ReceiveResult::Ignored);
                     }
-
                 }
 
                 PACKET_TYPE_KEY_COUNTER_OFFER => {
@@ -1147,7 +1071,6 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
                             // Check that this is a counter offer to the original offer we sent.
                             if !(offer.id.eq(offer_id)) {
                                 return Ok(ReceiveResult::Ignored);
-
                             }
 
                             // Kyber1024 key agreement if enabled.
@@ -1196,12 +1119,7 @@ impl<Application: ApplicationLayer> ReceiveContext<Application> {
                             if session.receive_windows[key_id as usize].message_authenticated(counter) {
                                 // Alice has now completed and validated the full hybrid exchange.
 
-                                let session_key = SessionKey::new(
+                                let session_key = SessionKey::new(session_key, Role::Alice, current_time, hybrid_kk.is_some());
-                                    session_key,
-                                    Role::Alice,
-                                    current_time,
-                                    hybrid_kk.is_some(),
-                                );
 
                                 let new_key_id = offer.key_id;
                                 drop(state);
@@ -1304,7 +1222,11 @@ fn send_ephemeral_offer<SendFunction: FnMut(&mut [u8])>(
     }
     if let Some(current_key) = current_key {
         idx = safe_write_all(&mut packet_buf, idx, &[0x01])?;
-        idx = safe_write_all(&mut packet_buf, idx, &public_fingerprint_of_secret(current_key.ratchet_key.as_bytes())[..16])?;
+        idx = safe_write_all(
+            &mut packet_buf,
+            idx,
+            &public_fingerprint_of_secret(current_key.ratchet_key.as_bytes())[..16],
+        )?;
     } else {
         idx = safe_write_all(&mut packet_buf, idx, &[0x00])?;
     }
@@ -1397,7 +1319,7 @@ fn create_packet_header(
     packet_type: u8,
     recipient_session_id: SessionId,
     counter: CounterValue,
-    key_id: bool
+    key_id: bool,
 ) -> Result<(), Error> {
     let fragment_count = ((packet_len as f32) / (mtu - HEADER_SIZE) as f32).ceil() as usize;
 
@@ -1417,7 +1339,10 @@ fn create_packet_header(
         //   [112-115]/[14-] packet type (0-15)
         //   [116-121]/[-]   number of fragments (0..63 for 1..64 fragments total)
         //   [122-127]/[-15] fragment number (0, 1, 2, ...)
-        memory::store_raw((counter.to_u32().wrapping_shl(1) | (key_id as u32)).to_le(), &mut header_destination_buffer[4..]);
+        memory::store_raw(
+            (counter.to_u32().wrapping_shl(1) | (key_id as u32)).to_le(),
+            &mut header_destination_buffer[4..],
+        );
         memory::store_raw(
             (u64::from(recipient_session_id) | (packet_type as u64).wrapping_shl(48) | ((fragment_count - 1) as u64).wrapping_shl(52))
                 .to_le(),
@@ -1536,12 +1461,7 @@ fn parse_dec_key_offer_after_header(
 
 impl SessionKey {
     /// Create a new symmetric shared session key and set its key expiration times, etc.
-    fn new(
+    fn new(key: Secret<64>, role: Role, current_time: i64, jedi: bool) -> Self {
-        key: Secret<64>,
-        role: Role,
-        current_time: i64,
-        jedi: bool,
-    ) -> Self {
         let a2b: Secret<AES_KEY_SIZE> = kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_AES_GCM_ALICE_TO_BOB).first_n_clone();
         let b2a: Secret<AES_KEY_SIZE> = kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_AES_GCM_BOB_TO_ALICE).first_n_clone();
         let (receive_key, send_key) = match role {