test: trigger and detect an Optimistic Ack attack #1999
Conversation
| stream.send(vec![42; LEN].into()).await.unwrap(); | ||
| let send_result = stream.flush().await; | ||
| // connection should abort since we inject a skip packet number | ||
| assert!(send_result.is_err()); |
There was a problem hiding this comment.
Is there any way to inspect the actual connection error.. ProtocolViolation?
There was a problem hiding this comment.
You should be able to match on it:
| { | ||
| // Return the largest packet number given multiple composed Interceptor implementations. | ||
| #[inline(always)] | ||
| fn intercept_rx_inject_ack(&mut self, space: PacketNumberSpace) -> Option<VarInt> { |
There was a problem hiding this comment.
This might have already been discussed, but is there a way this could be more generic, as in just fn intercept_rx_frame(&mut self, frame: Frame) -> Frame and you could just implement one that intercepts an Ack frame and modifies it?
There was a problem hiding this comment.
Tried this suggestion but ran into a type issue with the Frame<AckRanges, Data> type. I suspect we might also run into similar issues with the Data generic type.
The trait s2n_quic_core::frame::ack::AckRanges has different impls and it mismatches in our return type.
Specifically, in space/mod the type is s2n_quic_core::frame::ack::AckRangesDecoder, while in the interceptor we construct a s2n_quic_transport::ack::ack_ranges::AckRanges
error[E0308]: mismatched types
--> quic/s2n-quic/src/tests/skip_packets.rs:206:50
|
182 | fn intercept_rx_frame<'a, A: s2n_quic_core::frame::ack::AckRanges, D>(
| - this type parameter
...
206 | s2n_quic_core::frame::Frame::Ack(ack)
| -------------------------------- ^^^ expected type parameter `A`, found struct `s2n_quic_transport::ack::ack_ranges::AckRanges`
| |
| arguments to this enum variant are incorrect
|
= note: expected struct `s2n_quic_core::frame::Ack<A>`
found struct `s2n_quic_core::frame::Ack<s2n_quic_transport::ack::ack_ranges::AckRanges>`
There was a problem hiding this comment.
I think I would prefer something like:
fn intercept_rx_ack<A: interceptor::Ack>(&mut self, ack: &mut A);where
pub trait Ack {
fn space(&self) -> PacketNumberSpace;
fn range(&mut self, range: RangeInclusive<VarInt>);
}There was a problem hiding this comment.
I had a version working with the Ack trait, but I didn't see the purpose of it in the end. Let me know if my approach has any downsides compared to the trait
There was a problem hiding this comment.
My thinking with the trait was to avoid allocating for the ranges entirely and just dispatch directly to the handle_ack_frame function. Making it a trait means that the concrete space types don't leak into this interface.
There was a problem hiding this comment.
I'm not completely opposed to your approach. It just locks us into a concrete type for the range buffer.
There was a problem hiding this comment.
I thought the concrete ack::Ranges type was being kept internal to the interceptor::Ack type and not leaking into the interface... though is the impl<'a> From<&'a Ack> for frame::Ack<&'a ack::Ranges> the issue?
There was a problem hiding this comment.
The concrete type is your new Ack itself. So if that Ack struct started owning a reference to a space, it would need to be generic over some trait that dispatches ACK frames.
There was a problem hiding this comment.
Thanks, updated using the Trait. I had to add Sized to the PacketSpace trait, not sure if there is a downside to that
toidiu
force-pushed
the
ak-insertAckRange
branch
from
8490527 to
60fe563
Compare
toidiu
force-pushed
the
ak-insertAckRange
branch
from
60fe563 to
5f81fda
Compare
| let mut intercept_ack_range = InterceptAckRange { space, ack_range }; | ||
| packet_interceptor.intercept_rx_ack(&mut intercept_ack_range); | ||
| let (start, end) = intercept_ack_range.ack_range.into_inner(); |
There was a problem hiding this comment.
This isn't quite what i had in mind... I should have communicated better.
My thinking was you just call the interceptor on every packet and every call to interceptor::Ack::range would treat it like an individual ACK frame. The way you've implemented it here relies on the peer to send an ACK frame, which isn't super flexible.
|
|
||
| pub trait Ack { | ||
| fn space(&self) -> PacketNumberSpace; | ||
| fn set_range(&mut self, range: RangeInclusive<VarInt>); |
There was a problem hiding this comment.
The name implies you can only set a single range. I'm not sure that's what we want for the general purpose.
| stream.send(vec![42; LEN].into()).await.unwrap(); | ||
| let send_result = stream.flush().await; | ||
| // connection should abort since we inject a skip packet number | ||
| assert!(send_result.is_err()); |
There was a problem hiding this comment.
You should be able to match on it:
|
Priorities shifted but PR is still active and I plan to work on it. |
3 participants
#1962
Description of changes:
We previously added mitigation for the Optimistic Ack attack where we verified that we were skipping packets to help detect and mitigate the attack. This PR exposes a new method on the packet_interceptor which I use to inject a skipped packet number and confirm that the mitigation actually works (the connection is closed).
Callout:
The
intercept_rx_inject_ackonly accepts a single packet number. I originally attempted to take a range but that seemed non-trivial (how to handle multiple composed interceptors). Instead I opted for a single PacketNumber, which allows me to test my usecase. Since the Interceptor is unstable we should be able to change this API in the future as well.Testing:
New integration test.
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed? -->
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.