Skip to content

Commit

Permalink
generate packet numbers from a single sequence
Browse files Browse the repository at this point in the history
Instead of keeping separate sequences for each packet number space, use
the same sequence.

This is needed to make it easier to support FIPS. This is because FIPS
mandates that the crypto module (e.g. BoringCrypto) has to validate that
the AEAD counter (which in QUIC corresponds to the packet number) is
stricly monotonically increasing (so that counters are not reused).

Because BoringCrypto saves the counter inside its own AEAD context, and
because new paths currently require starting from packet number 0 again,
the FIPS requirement would require us to maintain separate AEAD contexts
for each path, which we currently don't do (and would probably be messy
to implement).

This reverts commit 40e2433.
  • Loading branch information
ghedo committed Jan 10, 2025
1 parent 3541aa1 commit 79c366d
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 14 deletions.
25 changes: 15 additions & 10 deletions quiche/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,9 @@ pub struct Connection {
/// Packet number spaces.
pkt_num_spaces: [packet::PktNumSpace; packet::Epoch::count()],

/// Next packet number.
next_pkt_num: u64,

/// Peer's transport parameters.
peer_transport_params: TransportParams,

Expand Down Expand Up @@ -1882,6 +1885,8 @@ impl Connection {
packet::PktNumSpace::new(),
],

next_pkt_num: 0,

peer_transport_params: TransportParams::default(),

local_transport_params: config.local_transport_params.clone(),
Expand Down Expand Up @@ -3612,7 +3617,7 @@ impl Connection {
b.cap()
};

let pn = pkt_space.next_pkt_num;
let pn = self.next_pkt_num;
let largest_acked_pkt =
path.recovery.get_largest_acked_on_epoch(epoch).unwrap_or(0);
let pn_len = packet::pkt_num_len(pn, largest_acked_pkt);
Expand Down Expand Up @@ -4604,7 +4609,7 @@ impl Connection {
path.recovery.delivery_rate_update_app_limited(true);
}

pkt_space.next_pkt_num += 1;
self.next_pkt_num += 1;

let handshake_status = recovery::HandshakeStatus {
has_handshake_keys: self.pkt_num_spaces[packet::Epoch::Handshake]
Expand Down Expand Up @@ -8707,7 +8712,7 @@ pub mod testing {

space.key_update = Some(packet::KeyUpdate {
crypto_open: open_prev.unwrap(),
pn_on_update: space.next_pkt_num,
pn_on_update: self.client.next_pkt_num,
update_acked: true,
timer: time::Instant::now(),
});
Expand Down Expand Up @@ -8809,7 +8814,7 @@ pub mod testing {

let space = &mut conn.pkt_num_spaces[epoch];

let pn = space.next_pkt_num;
let pn = conn.next_pkt_num;
let pn_len = 4;

let send_path = conn.paths.get_active()?;
Expand Down Expand Up @@ -8872,7 +8877,7 @@ pub mod testing {
aead,
)?;

space.next_pkt_num += 1;
conn.next_pkt_num += 1;

Ok(written)
}
Expand Down Expand Up @@ -11157,7 +11162,7 @@ mod tests {

// Client acks RESET_STREAM frame.
let mut ranges = ranges::RangeSet::default();
ranges.insert(0..6);
ranges.insert(pipe.server.next_pkt_num - 5..pipe.server.next_pkt_num);

let frames = [frame::Frame::ACK {
ack_delay: 15,
Expand Down Expand Up @@ -13499,15 +13504,15 @@ mod tests {
for _ in 0..512 {
let recv_count = pipe.server.recv_count;

last_packet_sent = pipe.client.pkt_num_spaces[epoch].next_pkt_num;
last_packet_sent = pipe.client.next_pkt_num;

pipe.send_pkt_to_server(pkt_type, &frames, &mut buf)
.unwrap();

assert_eq!(pipe.server.recv_count, recv_count + 1);

// Skip packet number.
pipe.client.pkt_num_spaces[epoch].next_pkt_num += 1;
pipe.client.next_pkt_num += 1;
}

assert_eq!(
Expand Down Expand Up @@ -17220,7 +17225,7 @@ mod tests {
let mut b = octets::OctetsMut::with_slice(&mut pkt_buf);
let epoch = packet::Type::Short.to_epoch().unwrap();
let space = &mut pipe.client.pkt_num_spaces[epoch];
let pn = space.next_pkt_num;
let pn = pipe.client.next_pkt_num;
let pn_len = 4;

let hdr = Header {
Expand Down Expand Up @@ -17256,7 +17261,7 @@ mod tests {
aead,
)
.expect("packet encrypt");
space.next_pkt_num += 1;
pipe.client.next_pkt_num += 1;

pipe.server
.recv(&mut pkt_buf[..written], RecvInfo {
Expand Down
4 changes: 0 additions & 4 deletions quiche/src/packet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -852,8 +852,6 @@ pub struct PktNumSpace {

pub largest_rx_non_probing_pkt_num: u64,

pub next_pkt_num: u64,

pub recv_pkt_need_ack: ranges::RangeSet,

pub recv_pkt_num: PktNumWindow,
Expand All @@ -879,8 +877,6 @@ impl PktNumSpace {

largest_rx_non_probing_pkt_num: 0,

next_pkt_num: 0,

recv_pkt_need_ack: ranges::RangeSet::new(crate::MAX_ACK_RANGES),

recv_pkt_num: PktNumWindow::default(),
Expand Down

0 comments on commit 79c366d

Please sign in to comment.