Adding robustness checks to the QUIC handshake #2613
Conversation
alizohaib
requested review from
KershawChang,
martinthomson,
larseggert and
mxinden
as code owners
alizohaib
changed the title
There was a problem hiding this comment.
@larseggert, would you mind taking a look at this as well?
I am not sure about this one: it's sending a number of datagrams, not packets. That probably isn't right. And I need to understand what the goals are for this feature.
Added: Also, I'm not seeing tests for this, which is probably why I am unable to make sense of it.
neqo-transport/src/connection/mod.rs
Outdated
| @@ -1142,6 +1153,25 @@ impl Connection { | |||
|
|
|||
| match (&self.state, self.role) { | |||
| (State::Init, Role::Client) => { | |||
| // Before starting the client, send some noise | |||
| if self.conn_params.pre_init_noise_enabled() && self.pre_initial_pkts > 0 { | |||
There was a problem hiding this comment.
It should be enough to just check the counter. The counter should be zero otherwise.
neqo-transport/src/connection/mod.rs
Outdated
| /// Maximum number of dummy datagrams to send before Client Initial packets. | ||
| const MAX_PRE_INIT_PKTS: usize = 1; | ||
| /// Payload size range (in bytes) for pre-Initial packets. | ||
| const PRE_INIT_PAYLOAD_LEN_RANGE: (usize, usize) = (1200, 1400); |
There was a problem hiding this comment.
I like the noise terminology you used; suggest to apply it more consistently, hence the rename here. Check if similar renamings make sense elsewhere?
| const PRE_INIT_PAYLOAD_LEN_RANGE: (usize, usize) = (1200, 1400); | |
| const PRE_INIT_NOISE_LEN_RANGE: Range<usize> = (1200, 1400); |
Is the 1200-1400 range chosen for any particular reason? If so, might want to add a comment.
There was a problem hiding this comment.
Would a smaller payload, e.g. 10 bytes, do as well?
There was a problem hiding this comment.
We chose that range to mimic actual Initial packet sizes. A smaller payload could work, but if it is less than the 1200-byte minimum defined in the RFC, it would be trivial to determine whether the first packet is a Client Initial.
neqo-transport/src/connection/mod.rs
Outdated
| @@ -1142,6 +1153,25 @@ impl Connection { | |||
|
|
|||
| match (&self.state, self.role) { | |||
| (State::Init, Role::Client) => { | |||
| // Before starting the client, send some noise | |||
| if self.conn_params.pre_init_noise_enabled() && self.pre_initial_pkts > 0 { | |||
| self.pre_initial_pkts -= 1; | |||
There was a problem hiding this comment.
Decrement after the packet was generated? Otherwise you decrement but may not send.
neqo-transport/src/connection/mod.rs
Outdated
| self.pre_initial_pkts -= 1; | ||
| if let Some(path) = self.paths.primary() { | ||
| let v = random::<1>()[0] as usize; | ||
| let len = PRE_INIT_PAYLOAD_LEN_RANGE.0 + (v % (PRE_INIT_PAYLOAD_LEN_RANGE.1 - PRE_INIT_PAYLOAD_LEN_RANGE.0 + 1)); |
There was a problem hiding this comment.
Can this be simplified or at least explained better? Range may have some helper functions you can use.
neqo-transport/src/connection/mod.rs
Outdated
| if let Some(path) = self.paths.primary() { | ||
| let v = random::<1>()[0] as usize; | ||
| let len = PRE_INIT_PAYLOAD_LEN_RANGE.0 + (v % (PRE_INIT_PAYLOAD_LEN_RANGE.1 - PRE_INIT_PAYLOAD_LEN_RANGE.0 + 1)); | ||
| let mut dummy_buf: SmallVec<[u8; PRE_INIT_PAYLOAD_LEN_RANGE.1]> = smallvec![0u8; len]; |
There was a problem hiding this comment.
Is a SmallVec really gaining us anything here?
There was a problem hiding this comment.
Prefer a Vec unless a benchmark proves that a stack allocation is faster.
The first comment describing the commit could use some more content, agreed. (I also invited you to a related Signal chat just now.)
+1 on needing tests. |
Failed Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
Co-authored-by: Lars Eggert <lars@eggert.org> Signed-off-by: Ali Zohaib <alizohaib1995@gmail.com>
| if self.pre_initial_noise_pkts > 0 { | ||
| if let Some(path) = self.paths.primary() { | ||
| // Choose a random payload size between the noise payload length range. | ||
| let span = PRE_INIT_NOISE_LEN_RANGE.end() - PRE_INIT_NOISE_LEN_RANGE.start() + 1; |
There was a problem hiding this comment.
| let span = PRE_INIT_NOISE_LEN_RANGE.end() - PRE_INIT_NOISE_LEN_RANGE.start() + 1; | |
| let span = PRE_INIT_NOISE_LEN_RANGE.len() + 1; |
| let v = usize::from(random::<1>()[0]); | ||
| let len = PRE_INIT_NOISE_LEN_RANGE.start() + (v % span); | ||
|
|
||
| let mut dummy_buf= vec![0u8; len]; |
There was a problem hiding this comment.
| let mut dummy_buf= vec![0u8; len]; | |
| let mut noise = vec![0u8; len]; |
| // Before starting the client, send some noise | ||
| if self.pre_initial_noise_pkts > 0 { | ||
| if let Some(path) = self.paths.primary() { | ||
| // Choose a random payload size between the noise payload length range. |
There was a problem hiding this comment.
| // Choose a random payload size between the noise payload length range. | |
| // Choose a random payload size in the noise payload length range. |
Yes. Sorry about not doing that earlier. |
This change allows clients to transmit a variable number of pre-initial packets (default 1).