authentication protocol for udp #2417

dshulyak · 2025-10-03T08:11:53Z

implementation of the #2091

dshulyak · 2025-10-03T08:12:45Z

this doesn't include integration code (e.g discovery, dataplane and raptorcast changes) and i still need to add more docs to the code itself

dnkolegov-ar · 2025-10-09T15:08:07Z

monad-wireauth-protocol/src/cookies.rs

+
+pub fn generate_cookie(cookie_secret: &[u8; 32], nonce: u64, remote_addr: &SocketAddr) -> [u8; 16] {
+    let mut address_bytes = [0u8; 16];
+    match remote_addr.ip() {


The remote port is not used. According to the spec: The initiator address is a concatenation of the message source IP address and source port.

thanks, i forgot about port, will add

dnkolegov-ar · 2025-10-09T15:19:43Z

monad-wireauth-protocol/src/common.rs

+    }
+}
+
+#[derive(Clone, Debug, Zeroize)]


There is no ZeroizeOnDrop. Is it intentional? It could be useful for tmp_x variables

yes, i am using Zeroizing for chaining_key and hash output. i think there was no need to zeroize every tmp variable, will recheck

dnkolegov-ar · 2025-10-09T15:30:51Z

monad-wireauth-protocol/src/handshake.rs

+            .expect("ephemeral private key must be set"),
+        &msg.ephemeral_public,
+    )?;
+    let temp = keyed_hash!(state.chaining_key.as_ref(), ecdh_ee.as_ref());


The ephemeral private key of the initiator's state can be erased here.

i guess i can .take() it from the state, but the state itself is dropped (and private key is erased at that point) soon after exiting this function

dnkolegov-ar · 2025-10-14T14:39:29Z

monad-wireauth-api/src/api.rs

+                        Ok(result) => {
+                            result.map(|(timer, r)| (timer, None, r.rekey, Some(r.terminated)))
+                        }
+                        Err(_) => None,


Why is it safe to ignore an error here and at similar places below?

tick() shouldn't be fallible, i will remove Result from every such function in a follow up

monad-wireauth-session/src/common.rs

dnkolegov-ar · 2025-10-15T18:47:31Z

monad-wireauth-protocol/src/cookies.rs

+    )
+    .map_err(CookieError::CookieDecryptionFailed)?;
+
+    Ok(reply.encrypted_cookie)


Maybe it is worth adding a comment that this function decrypts the cookie and returns its value. No comments, and a returned reply.encrypted_cookie is misleading.

dnkolegov-ar · 2025-10-15T18:52:02Z

monad-wireauth-session/src/common.rs

+        Ok((terminated, rekey))
+    }
+
+    pub fn handle_rekey_timer(&mut self) -> RekeyEvent {


It is called handle_rekey_timer, but nothing is handled here.

thanks, i will refactor this

monad-wireauth-session/src/initiator.rs

dnkolegov-ar · 2025-10-15T18:56:03Z

monad-wireauth-session/src/initiator.rs

+
+        let mut session = Initiator {
+            handshake_state,
+            common,


Maybe it should be called session state?

do you mean whole struct, e.g rename CommonSessionData to SessionState and all related variables?
i can rename, seems like a better choice

Correct:
common -> session_state
CommonSessionData -> SessionState

The Noise protocol framework uses CipherState, SymmetricState, HandshakeState, you can follow that naming for other relevant types: SessionState, ProtocolState, ...

dnkolegov-ar · 2025-10-15T19:12:13Z

monad-wireauth-api/src/cookie.rs

+
+use crate::error::{ProtocolErrorContext, Result};
+
+pub struct Cookies {


There is no zeroization for secrets

i don't think that it accomplishes anything. those secrets can be erased only when authentication protocol is terminated (e.g program closed for example).
and it doesn't matter if they are stolen when protocol is no longer working

dnkolegov-ar · 2025-10-15T19:21:45Z

monad-wireauth-api/src/cookie.rs

+        refresh_duration: Duration,
+    ) -> Self {
+        let mut cookie_secret = [0u8; 32];
+        rng.fill_bytes(&mut cookie_secret);


What happens if the responder is under load, sends a handshake response with a cookie, and then immediately crashes? The initiator is an honest node, and it will send the new handshake initiation with the cookie that can't be verified after reloading. Will the reloaded node accept that handshake after timeout?

P.S. It is easier to ask that explicitly than to go through the code

if responder is under load after crash then such message will have invalid mac2 and will be rejected.
if responder is not under load then invlalid mac2 will be ignored and initiation will pass.

do you think it is worth it to persist cookie secret to handle this case? to me it seems not worth it

So, if the responder is still under load after its crash, the honest initiator will not be able to establish the connection until some timer is triggered. In WireGuard, it seems to me, this would be Reject-After-Time × 3 seconds.
I am trying to understand which timeout defines that, and in how many seconds the honest peer will be able to establish a connection

initiator will retry if it doesn't get reply in session_timeout with some jitter

dnkolegov-ar · 2025-10-15T19:25:34Z

monad-wireauth-session/src/common.rs

+    pub ip_rate_limit_window: Duration,
+    pub max_requests_per_ip: usize,
+    pub ip_history_capacity: usize,
+    pub psk: [u8; 32],


Do we need to add zeroization for Config?

i think it makes sense to erase psk, as it meant to be long term secret

dnkolegov-ar · 2025-10-15T19:25:59Z

monad-wireauth-session/src/common.rs

+    pub rekey_deadline: Option<Duration>,
+    pub session_timeout_deadline: Option<Duration>,
+    pub max_session_duration_deadline: Option<Duration>,
+    pub stored_cookie: Option<[u8; 16]>,


Do we need to add zeroization for the cookie secret?

i think it only allows attacker to bypass dos-filter.
it doesn't cost much to wrap this in Zerozing, but practically it seemed to me unnecessary

dnkolegov-ar · 2025-10-16T08:39:39Z

monad-wireauth-api/src/api.rs

+use crate::{Initiator, Responder, Transport};
+use monad_wireauth_session::{Config, SessionIndex};
+
+pub struct API<C: Context> {


In WireGuard:

after an initiator receives a handshake response message (section 5.4.3), the responder cannot send transport data messages (section 5.4.6) until it has received the first transport data message from the initiator. And, further, transport data messages encrypted using the previous secure session might be in transit after a new secure session has been created. For these reasons, WireGuard keeps in memory the current secure session, the previous secure session, and the next secure session for the case of an unconfirmed session

Could you point me to the code where this logic is implemented? Or explain why this is not implemented and why the protocol does not need that functionality.

I think it should be in

monad-bft/monad-wireauth-api/src/api.rs

Line 400 in 98a6b9b

fn decrypt_data_packet(&mut self, packet: &mut [u8], remote_addr: SocketAddr) -> Result<bool> {

, but I do not see the entire workflow, only the mechanism that ensures that the initiator must send some data to the responder before the responder starts sending data to the initiator.

after an initiator receives a handshake response message (section 5.4.3), the responder cannot send transport data messages (section 5.4.6) until it has received the first transport data message from the initiator.

implemented by having a separate type for Responder, that type can't send messages, but can accept them (decrypt method), and on first successfully decrypted message it transitions to Transport type (establish method)

And, further, transport data messages encrypted using the previous secure session might be in transit after a new secure session has been created. For these reasons, WireGuard keeps in memory the current secure session, the previous secure session, and the next secure session for the case of an unconfirmed session

in api crate (state.rs) i track sessions with different state separately, sessions in Transport state can't be replaced by unconfirmed sessions, once Transport is created there is a call handle_established that evicts older sessions. that covers current secure session (Transport type) and separate unconfirmed session. so initiator or responder will never evict established session until new session transitions to Transport state

there is currently an expectation that both sides will initiate connect before sending messages, and i track 1 of each initiated and accepted instead of previous secure session. it addresses a problem that older session may get evicted during rekey while there are inflight messages

though this last part is not very intuitive, i will check how to transition to simply storing 2 last established sessions instead

dnkolegov-ar · 2025-10-16T08:48:43Z

monad-wireauth-api/src/state.rs

+            initiating_sessions: HashMap::new(),
+            responding_sessions: HashMap::new(),
+            transport_sessions: HashMap::new(),
+            last_open_session_by_public_key: HashMap::new(),


What does open mean? Who opened it? Can we use initiator/responder-based word?
Does Open mean not yet established?

should be established or transport last_transport_session_by_public_key

dnkolegov-ar · 2025-10-16T09:42:40Z

monad-wireauth-api/src/state.rs

+
+        self.transport_sessions.insert(session_id, transport);
+
+        terminated_sessions


It is not obvious without comments why the function called handle_established returns terminated_sessions also in

monad-bft/monad-wireauth-api/src/api.rs

Line 204 in 98a6b9b

let terminated_sessions =

those are replaced sessions, i will take a look if it can be simplified

dnkolegov-ar · 2025-10-24T19:18:52Z

monad-wireauth-api/src/api.rs

+
+        let mut initiator = self
+            .state
+            .remove_initiator(&response.receiver_index.get().into())


Why is the State removed before the response is authenticated/validated?

thanks, i forgot to update this method to use get -> validate -> remove , similarly to decrypt_data_packet

dnkolegov-ar · 2025-10-24T20:32:21Z

monad-wireauth-api/src/api.rs

+        response: &mut HandshakeResponse,
+        remote_addr: SocketAddr,
+    ) -> Result<()> {
+        if !self.check_under_load(remote_addr, response.sender_index.get(), response)? {


In WireGuard, verify MAC1 first — always. Check the MAC2 if the host is under load.
Here, first check is whether we are under load, and then check MAC1

i will update to run mac1 check before this, the same way it is done in accept_handshake_init

dnkolegov-ar · 2025-10-24T20:39:29Z

monad-wireauth-api/src/api.rs

+        }
+
+        let duration_since_start = self.context.duration_since_start();
+        let local_index = self.state.allocate_session_index();


We allocate and insert a new index here. What if we allocate it and then fail on validate_init? My understanding is that the error is returned, but handle_terminate is not called, leaving the index permanently marked as “allocated.”

thats a mistake

monad-wireauth-protocol/src/crypto.rs

dnkolegov-ar · 2025-10-26T18:23:47Z

@dshulyak When can I review the fixes for the findings? I think it makes sense to do one more round of the security review after you fix all current findings/comments.

dshulyak · 2025-10-27T08:18:10Z

@dnkolegov-ar i will push changes by wed 29th

dshulyak force-pushed the dmitry/wireauth branch 2 times, most recently from f088da4 to 070fb7c Compare October 7, 2025 07:05

authentication protocol for udp

98a6b9b

dshulyak force-pushed the dmitry/wireauth branch from 070fb7c to 98a6b9b Compare October 8, 2025 12:34

dnkolegov-ar reviewed Oct 9, 2025

View reviewed changes

dnkolegov-ar reviewed Oct 14, 2025

View reviewed changes

dnkolegov-ar reviewed Oct 15, 2025

View reviewed changes

dnkolegov-ar reviewed Oct 16, 2025

View reviewed changes

dnkolegov-ar reviewed Oct 24, 2025

View reviewed changes


		use crate::error::{ProtocolErrorContext, Result};

		pub struct Cookies {


		self.transport_sessions.insert(session_id, transport);

		terminated_sessions

Uh oh!

authentication protocol for udp #2417

Are you sure you want to change the base?

authentication protocol for udp #2417

Uh oh!

Conversation

dshulyak commented Oct 3, 2025

Uh oh!

dshulyak commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnkolegov-ar Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnkolegov-ar Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnkolegov-ar Oct 15, 2025 •

edited

Loading

dnkolegov-ar Oct 16, 2025 •

edited

Loading