feat: Shuffle the client Initial crypto data #2228
Conversation
Look for ranges of `N` or more bytes of graphical ASCII data in `data`. Create at least one split point for each range, multiple ones each `N` bytes if the range is long enough. Create data chunks based on those split points. Shuffle the chunks and return them. CC @martinthomson @dennisjackson
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
|
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2228 +/- ##
==========================================
+ Coverage 95.37% 95.40% +0.02%
==========================================
Files 112 113 +1
Lines 36569 36750 +181
==========================================
+ Hits 34879 35061 +182
+ Misses 1690 1689 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Benchmark resultsPerformance differences relative to f3d0191. coalesce_acked_from_zero 1+1 entries: 💔 Performance has regressed. time: [104.93 ns 105.27 ns 105.62 ns]
change: [+17.767% +18.446% +19.100%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 3+1 entries: 💔 Performance has regressed. time: [120.56 ns 120.88 ns 121.23 ns]
change: [+20.871% +21.601% +22.224%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 10+1 entries: 💔 Performance has regressed. time: [120.50 ns 121.13 ns 121.85 ns]
change: [+22.039% +22.577% +23.126%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1000+1 entries: 💔 Performance has regressed. time: [100.08 ns 100.22 ns 100.39 ns]
change: [+21.662% +23.086% +24.354%] (p = 0.00 < 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [111.40 ms 111.46 ms 111.52 ms]
change: [+0.0818% +0.1568% +0.2254%] (p = 0.00 < 0.05)
transfer/pacing-false/varying-seeds: No change in performance detected. time: [27.305 ms 28.462 ms 29.636 ms]
change: [-2.2698% +3.2180% +8.9672%] (p = 0.26 > 0.05)
transfer/pacing-true/varying-seeds: No change in performance detected. time: [35.271 ms 37.145 ms 39.046 ms]
change: [-2.6502% +3.9287% +11.367%] (p = 0.28 > 0.05)
transfer/pacing-false/same-seed: No change in performance detected. time: [25.636 ms 26.461 ms 27.287 ms]
change: [-3.7859% +0.7352% +5.6287%] (p = 0.76 > 0.05)
transfer/pacing-true/same-seed: No change in performance detected. time: [40.711 ms 42.642 ms 44.591 ms]
change: [-5.7983% +0.2364% +7.0996%] (p = 0.95 > 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected. time: [920.00 ms 929.07 ms 938.27 ms]
thrpt: [106.58 MiB/s 107.63 MiB/s 108.70 MiB/s]
change:
time: [-1.6896% -0.2539% +1.1341%] (p = 0.73 > 0.05)
thrpt: [-1.1214% +0.2545% +1.7187%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [321.10 ms 324.14 ms 327.33 ms]
thrpt: [30.550 Kelem/s 30.851 Kelem/s 31.143 Kelem/s]
change:
time: [-0.3991% +0.9753% +2.3411%] (p = 0.16 > 0.05)
thrpt: [-2.2876% -0.9659% +0.4007%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: Change within noise threshold. time: [33.846 ms 34.084 ms 34.345 ms]
thrpt: [29.116 elem/s 29.340 elem/s 29.546 elem/s]
change:
time: [+0.2959% +1.1950% +2.1887%] (p = 0.01 < 0.05)
thrpt: [-2.1418% -1.1809% -0.2950%]
1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: Change within noise threshold. time: [1.6421 s 1.6602 s 1.6785 s]
thrpt: [59.577 MiB/s 60.234 MiB/s 60.899 MiB/s]
change:
time: [+0.4478% +2.2664% +4.0194%] (p = 0.01 < 0.05)
thrpt: [-3.8641% -2.2162% -0.4458%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
larseggert
changed the title
There was a problem hiding this comment.
I spent a few minutes writing some code that might be a good replacement for this if you only care about splitting at a useful point. This would be used by drawing a random value in the range (r.start + 1..end - 1) to split on. It's a lot less code...
/// Finds the range where the SNI extension lives.
/// If this isn't a ClientHello, then return the whole message.
fn find_sni_split(mut buf: &[u8]) -> Range<usize> {
fn read_len(buf: &[u8]) -> usize {
let mut len = 0;
for v in buf {
len = len << 8 & usize::from(v);
}
len
}
fn skip_vec<const N: usize>(i: usize, buf: &[u8]) -> usize {
i + N + read_len(&buf[i..i + N])
}
// ClientHello == 1
if buf[0] == 1 {
let mut i = 1 + 3 + 2 + 32; // msg_type, length, version, random
i = skip_vec::<1>(i, buf); // session_id
i = skip_vec::<2>(i, buf); // cipher_suites
i = skip_vec::<1>(i, buf); // compression_methods
i += 2; // extensions length
while i < buf.len() {
if buf[i] == 0 || buf[i + 1] == 0 {
// SNI!
return i..i + 4 + read_len(buf[i + 2..i + 4]);
}
}
}
0..buf.len()
}
neqo-transport/src/shuffle.rs
Outdated
|
|
||
| /// Find the ranges of all sequences of two or more ASCII "LDHD" (letters, digits, hyphens, dots) | ||
| /// bytes in `data`, and return the `n` longest ones in ascending order by start index. | ||
| fn ascii_sequences(data: &[u8], n: usize) -> impl Iterator<Item = Range<usize>> { |
There was a problem hiding this comment.
I'm still not that enthusiastic about this approach, but here's a way to improve it some.
Change the signature to fn ascii_sequences<const N: usize>(data: &[u8]) -> impl Interator....
Then collect the value into an array ([_; N]) for sorting rather than a Vec. You might have to unwrap, but that's not so bad.
There was a problem hiding this comment.
I did this in e9f3b58.
There was a problem hiding this comment.
But I don't think I can collect into an array, because there might be fewer than N ranges. I could collect into an array with Option<Range<usize>> and have some of them be None in that case - is that worth it?
Thanks for this, but I think it needs more work? I don't think this handles invalid crypto data well, |
|
Fair. It was only a few minutes of work. Adding bounds checks will complicate it a little, but not overmuch. Of course, you could take the attitude I did, which is that the TLS stack should not be producing invalid messages, so panicking in that case is justified. |
|
I am nervous about making assumptions about the input. But let me play with it and see how much complexity bounds checking adds. |
|
@martinthomson is your code taking |
Reorder
datainto chunks roughly delimited by the midpoints of ASCII "LDHD" (letters, digits, hyphens, dots) sequences.Look for the
nlongest ranges of ASCII "LDHD" characters indata. Create split points halfway into each range. Chunk the data based on those split points, shuffle the chunks and return them.CC @martinthomson @dennisjackson