Skip to content

Commit

Permalink
Capture packets in tests and ensure ack delay is correctly set
Browse files Browse the repository at this point in the history 
Sponsored by Stormshield
  • Loading branch information
@aochagavia
aochagavia committed May 3, 2023
1 parent 2be9e62 commit 286b8b2
Show file tree
Hide file tree
Showing 6 changed files with 271 additions and 126 deletions.
194 changes: 71 additions & 123 deletions quinn-proto/src/connection/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::{
cid_queue::CidQueue,
coding::BufMutExt,
config::{ServerConfig, TransportConfig},
crypto::{self, HeaderKey, KeyPair, Keys, PacketKey},
crypto::{self, KeyPair, Keys, PacketKey},
frame,
frame::{Close, Datagram, FrameStruct},
packet::{Header, LongType, Packet, PartialDecode, SpaceId},
Expand All @@ -31,7 +31,7 @@ use crate::{
token::ResetToken,
transport_parameters::TransportParameters,
Dir, EndpointConfig, Frame, Side, StreamId, Transmit, TransportError, TransportErrorCode,
VarInt, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, RESET_TOKEN_SIZE, TIMER_GRANULARITY,
VarInt, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, TIMER_GRANULARITY,
};

mod ack_frequency;
Expand All @@ -53,6 +53,9 @@ mod pacing;
mod packet_builder;
use packet_builder::PacketBuilder;

mod packet_crypto;
use packet_crypto::{PrevCrypto, ZeroRttCrypto};

mod paths;
use paths::PathData;
pub use paths::RttEstimator;
Expand Down Expand Up @@ -2015,40 +2018,13 @@ impl Connection {
ecn: Option<EcnCodepoint>,
partial_decode: PartialDecode,
) {
let header_crypto = if partial_decode.is_0rtt() {
if let Some(ref crypto) = self.zero_rtt_crypto {
Some(&*crypto.header)
} else {
debug!("dropping unexpected 0-RTT packet");
return;
}
} else if let Some(space) = partial_decode.space() {
if let Some(ref crypto) = self.spaces[space].crypto {
Some(&*crypto.header.remote)
} else {
debug!(
"discarding unexpected {:?} packet ({} bytes)",
space,
partial_decode.len(),
);
return;
}
} else {
// Unprotected packet
None
};

let packet = partial_decode.data();
let stateless_reset = packet.len() >= RESET_TOKEN_SIZE + 5
&& self.peer_params.stateless_reset_token.as_deref()
== Some(&packet[packet.len() - RESET_TOKEN_SIZE..]);

match partial_decode.finish(header_crypto) {
Ok(packet) => self.handle_packet(now, remote, ecn, Some(packet), stateless_reset),
Err(_) if stateless_reset => self.handle_packet(now, remote, ecn, None, true),
Err(e) => {
trace!("unable to complete packet decoding: {}", e);
}
if let Some(decoded) = packet_crypto::unprotect_header(
partial_decode,
&self.spaces,
self.zero_rtt_crypto.as_ref(),
self.peer_params.stateless_reset_token,
) {
self.handle_packet(now, remote, ecn, decoded.packet, decoded.stateless_reset);
}
}

Expand Down Expand Up @@ -3252,78 +3228,33 @@ impl Connection {
now: Instant,
packet: &mut Packet,
) -> Result<Option<u64>, Option<TransportError>> {
if !packet.header.is_protected() {
// Unprotected packets also don't have packet numbers
return Ok(None);
}
let space = packet.header.space();
let rx_packet = self.spaces[space].rx_packet;
let number = packet.header.number().ok_or(None)?.expand(rx_packet + 1);
let key_phase = packet.header.key_phase();

let mut crypto_update = false;
let crypto = if packet.header.is_0rtt() {
&self.zero_rtt_crypto.as_ref().unwrap().packet
} else if key_phase == self.key_phase || space != SpaceId::Data {
&self.spaces[space].crypto.as_mut().unwrap().packet.remote
} else if let Some(prev) = self.prev_crypto.as_ref().and_then(|crypto| {
// If this packet comes prior to acknowledgment of the key update by the peer,
if crypto.end_packet.map_or(true, |(pn, _)| number < pn) {
// use the previous keys.
Some(crypto)
} else {
// Otherwise, this must be a remotely-initiated key update, so fall through to the
// final case.
None
}
}) {
&prev.crypto.remote
} else {
// We're in the Data space with a key phase mismatch and either there is no locally
// initiated key update or the locally initiated key update was acknowledged by a
// lower-numbered packet. The key phase mismatch must therefore represent a new
// remotely-initiated key update.
crypto_update = true;
&self.next_crypto.as_ref().unwrap().remote
};

crypto
.decrypt(number, &packet.header_data, &mut packet.payload)
.map_err(|_| {
trace!("decryption failed with packet number {}", number);
None
})?;
let result = packet_crypto::decrypt_packet_body(
packet,
&self.spaces,
self.zero_rtt_crypto.as_ref(),
self.key_phase,
self.prev_crypto.as_ref(),
self.next_crypto.as_ref(),
)?;

if let Some(ref mut prev) = self.prev_crypto {
if prev.end_packet.is_none() && key_phase == self.key_phase {
// Outgoing key update newly acknowledged
prev.end_packet = Some((number, now));
self.set_key_discard_timer(now, space);
let pn = result.map(|d| {
if d.outgoing_key_update_acked {
if let Some(prev) = self.prev_crypto.as_mut() {
prev.end_packet = Some((d.number, now));
self.set_key_discard_timer(now, packet.header.space());
}
}
}

if !packet.reserved_bits_valid() {
return Err(Some(TransportError::PROTOCOL_VIOLATION(
"reserved bits set",
)));
}

if crypto_update {
// Validate and commit incoming key update
if number <= rx_packet
|| self
.prev_crypto
.as_ref()
.map_or(false, |x| x.update_unacked)
{
return Err(Some(TransportError::KEY_UPDATE_ERROR("")));
if d.incoming_key_update {
trace!("key update authenticated");
self.update_keys(Some((d.number, now)), true);
self.set_key_discard_timer(now, packet.header.space());
}
trace!("key update authenticated");
self.update_keys(Some((number, now)), true);
self.set_key_discard_timer(now, space);
}

Ok(Some(number))
d.number
});

Ok(pn)
}

fn update_keys(&mut self, end_packet: Option<(u64, Instant)>, remote: bool) {
Expand Down Expand Up @@ -3355,6 +3286,43 @@ impl Connection {
self.peer_params.min_ack_delay.is_some()
}

/// Decodes a packet, returning its decrypted payload, so it can be inspected in tests
#[cfg(test)]
pub(crate) fn decode_packet(&self, event: &ConnectionEvent) -> Option<Vec<u8>> {
if let ConnectionEventInner::Datagram {
first_decode,
remaining,
..
} = &event.0
{
if remaining.is_some() {
panic!("Packets should never be coalesced in tests");
}

let decrypted_header = packet_crypto::unprotect_header(
first_decode.clone(),
&self.spaces,
self.zero_rtt_crypto.as_ref(),
self.peer_params.stateless_reset_token,
)?;

let mut packet = decrypted_header.packet?;
packet_crypto::decrypt_packet_body(
&mut packet,
&self.spaces,
self.zero_rtt_crypto.as_ref(),
self.key_phase,
self.prev_crypto.as_ref(),
self.next_crypto.as_ref(),
)
.ok()?;

return Some(packet.payload.to_vec());
}

None
}

/// The number of bytes of packets containing retransmittable frames that have not been
/// acknowledged or declared lost.
#[cfg(test)]
Expand Down Expand Up @@ -3587,21 +3555,6 @@ mod state {
}
}

struct PrevCrypto {
/// The keys used for the previous key phase, temporarily retained to decrypt packets sent by
/// the peer prior to its own key update.
crypto: KeyPair<Box<dyn PacketKey>>,
/// The incoming packet that ends the interval for which these keys are applicable, and the time
/// of its receipt.
///
/// Incoming packets should be decrypted using these keys iff this is `None` or their packet
/// number is lower. `None` indicates that we have not yet received a packet using newer keys,
/// which implies that the update was locally initiated.
end_packet: Option<(u64, Instant)>,
/// Whether the following key phase is from a remotely initiated update that we haven't acked
update_unacked: bool,
}

struct InFlight {
/// Sum of the sizes of all sent packets considered "in flight" by congestion control
///
Expand Down Expand Up @@ -3679,11 +3632,6 @@ const MIN_PACKET_SPACE: usize = 40;
/// that numbers around 10 are a good compromise.
const MAX_TRANSMIT_SEGMENTS: usize = 10;

struct ZeroRttCrypto {
header: Box<dyn HeaderKey>,
packet: Box<dyn PacketKey>,
}

#[derive(Default)]
struct SentFrames {
retransmits: ThinRetransmits,
Expand Down
Loading

0 comments on commit 286b8b2

Please sign in to comment.