fix: gate SCONE Initial padding on connection parameter#3573
fix: gate SCONE Initial padding on connection parameter#3573mxinden merged 2 commits intomozilla:v0.24from
Conversation
The `scone` connection parameter (off by default since mozilla#3492) only gated advertisement of the SCONE transport parameter. The Initial- packet SCONE indication (`0xc8 0x13`) was still emitted unconditionally, so every default-config client was signaling SCONE on the wire without actually negotiating it. This is suspected — not yet confirmed — to be the cause of a Firefox 150 regression where Facebook's edge reacts to the indicator and Bitdefender's HTTP/3 inspection then trips on the altered server-side frames (bug 2034178). The QUIC handshake still completes, so the H3->H2 fallback never kicks in. Gate both the build-time reservation and the pad-time indicator on `scone_enabled()`; when disabled, pad Initials with zeros as before SCONE was introduced.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.24 #3573 +/- ##
=======================================
Coverage 94.30% 94.30%
=======================================
Files 127 127
Lines 38739 38744 +5
Branches 38739 38744 +5
=======================================
+ Hits 36532 36538 +6
+ Misses 1369 1367 -2
- Partials 838 839 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Merging this PR will improve performance by 4.42%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | decode 4096 bytes, mask ff |
17.9 µs | 17.4 µs | +3.24% |
| ⚡ | Simulation | decode 1048576 bytes, mask ff |
4.5 ms | 4.4 ms | +3.12% |
| ⚡ | Simulation | decode 4096 bytes, mask 7f |
24 µs | 23.1 µs | +3.69% |
| ⚡ | Simulation | decode 1048576 bytes, mask 7f |
6.1 ms | 5.9 ms | +3.72% |
| ⚡ | Simulation | decode 4096 bytes, mask 3f |
27.1 µs | 26 µs | +4.38% |
| ⚡ | Simulation | decode 1048576 bytes, mask 3f |
6.9 ms | 6.6 ms | +4.42% |
Comparing mxinden:disable-scone-padding (5acfebb) with main (faa1437)2
Footnotes
-
27 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
v0.24(5b4e850) during the generation of this report, somain(faa1437) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Benchmark resultsSignificant performance differences relative to 5b4e850. transfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: 💔 Performance has regressed by +1.7809%. time: [213.29 ms 213.69 ms 214.13 ms]
thrpt: [467.00 MiB/s 467.97 MiB/s 468.83 MiB/s]
change:
time: [+1.5083% +1.7809% +2.0702] (p = 0.00 < 0.05)
thrpt: [-2.0282% -1.7497% -1.4859]
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severeAll resultstransfer/1-conn/1-100mb-resp (aka. Download)/mtu-1504: Change within noise threshold. time: [210.29 ms 210.69 ms 211.20 ms]
thrpt: [473.49 MiB/s 474.64 MiB/s 475.54 MiB/s]
change:
time: [+0.9479% +1.2313% +1.5149] (p = 0.00 < 0.05)
thrpt: [-1.4923% -1.2163% -0.9390]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS)/mtu-1504: Change within noise threshold. time: [283.41 ms 285.34 ms 287.29 ms]
thrpt: [34.808 Kelem/s 35.046 Kelem/s 35.284 Kelem/s]
change:
time: [-2.0322% -1.1433% -0.2062] (p = 0.02 < 0.05)
thrpt: [+0.2067% +1.1565% +2.0744]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS)/mtu-1504: No change in performance detected. time: [38.518 ms 38.635 ms 38.769 ms]
thrpt: [25.794 B/s 25.883 B/s 25.962 B/s]
change:
time: [-0.8107% -0.2416% +0.2935] (p = 0.40 > 0.05)
thrpt: [-0.2927% +0.2422% +0.8173]
No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
4 (4.00%) high mild
2 (2.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload)/mtu-1504: 💔 Performance has regressed by +1.7809%. time: [213.29 ms 213.69 ms 214.13 ms]
thrpt: [467.00 MiB/s 467.97 MiB/s 468.83 MiB/s]
change:
time: [+1.5083% +1.7809% +2.0702] (p = 0.00 < 0.05)
thrpt: [-2.0282% -1.7497% -1.4859]
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severestreams/walltime/1-streams/each-1000-bytes: No change in performance detected. time: [585.20 µs 587.24 µs 589.63 µs]
change: [-0.7761% -0.3475% +0.1442] (p = 0.14 > 0.05)
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
1 (1.00%) high mild
8 (8.00%) high severestreams/walltime/1000-streams/each-1-bytes: Change within noise threshold. time: [12.445 ms 12.464 ms 12.483 ms]
change: [+0.5105% +0.8797% +1.1947] (p = 0.00 < 0.05)
Change within noise threshold.streams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [45.144 ms 45.185 ms 45.228 ms]
change: [+0.9311% +1.0683% +1.2060] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [78.361 ms 78.417 ms 78.475 ms]
change: [-1.4960% -1.3374% -1.2045] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mildtransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [79.772 ms 79.854 ms 79.957 ms]
change: [-2.0362% -1.8822% -1.7222] (p = 0.00 < 0.05)
Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
1 (1.00%) low mild
7 (7.00%) high mild
3 (3.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [80.215 ms 80.297 ms 80.397 ms]
change: [+0.3437% +0.5618% +0.7430] (p = 0.00 < 0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severetransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [79.933 ms 80.003 ms 80.084 ms]
change: [-2.0771% -1.8793% -1.7084] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severeDownload data for |
mxinden
changed the title
Client/server transfer resultsPerformance differences relative to 5b4e850. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
There was a problem hiding this comment.
Pull request overview
This PR fixes unintended on-wire signaling of SCONE by ensuring the SCONE Initial-padding indication is only emitted when the scone connection parameter is enabled, restoring zero-padding behavior when it is disabled. This aligns runtime packet formatting with the existing gating of the SCONE transport parameter and aims to prevent default clients from advertising SCONE implicitly.
Changes:
- Gate Initial-packet SCONE space reservation on
conn_params.scone_enabled(). - Gate emission of the SCONE Initial-padding indication on
conn_params.scone_enabled(); otherwise pad Initials with zero bytes. - Bump workspace version from
0.24.1to0.24.2(and updateCargo.lockaccordingly).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| neqo-transport/src/connection/mod.rs | Gates SCONE reservation/indication on scone_enabled() and uses zero padding when disabled. |
| neqo-transport/src/connection/tests/handshake.rs | Updates corruption test to treat padding as zero-filled when SCONE is disabled. |
| Cargo.toml | Workspace version bump to 0.24.2. |
| Cargo.lock | Updates workspace member package versions to 0.24.2. |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
|
CC @martinthomson since this was his change originally. |
@larseggert do you feel strongly about it? As far as I can tell, clippies complaints are unrelated. I would like to keep the change minimal: |
|
If unrelated, I'm ok. Do a different PR for them (or I can after lunch) |
|
@larseggert note that this is targeting the |
|
Ah. Sorry. Jet lagged with 2h sleep and a cancelled plane in LHR. Am not fully coherent. |
Update to Neqo v0.24.2 more specifically include the following fix. - fix: gate SCONE Initial padding on connection parameter See mozilla/neqo#3573 and https://github.com/mozilla/neqo/releases/tag/v0.24.2 for details. Differential Revision: https://phabricator.services.mozilla.com/D296306
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
If there were protocol police, I would be ringing them right now. We should be free to pad packets with whatever noise we like.
| .skip(1) // Skip the last byte, which might be a SCONE indicator. | ||
| .find(|&(_, &v)| v != Connection::SCONE_INDICATION[0]) // The SCONE padding value. |
There was a problem hiding this comment.
Note that this is simply a revert of #2814. Fine for now in tests?
| if self.conn_params.scone_enabled() { | ||
| // This ensures that the last bytes are a SCONE indication, if there is enough space. | ||
| // This is not tracked, other than for congestion control (above) | ||
| if pad_amount >= Self::SCONE_INDICATION.len() { |
There was a problem hiding this comment.
FWIW, this is not at all how I would have "fixed" this. I would have changed this to:
| if pad_amount >= Self::SCONE_INDICATION.len() { | |
| if pad_amount >= Self::SCONE_INDICATION.len() && self.conn_params.scone_enabled() { |
There was a problem hiding this comment.
At this point I don't know whether BitDefender trips over Self::SCONE_INDICATION[0] or Self::SCONE_INDICATION[1], thus I opted for disabling both on self.conn_params.scone_enabled(), using 0 padding instead as we did before.
@martinthomson is there much value in skipping Self::SCONE_INDICATION[0] but including Self::SCONE_INDICATION[1]?
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
If there were protocol police, I would be ringing them right now. We should be free to pad packets with whatever noise we like.
We are in touch with BitDefender. I assume they will fix their bug soon. Once fixed, we can re-enable SCONE. We expect a large amount of users to be affected by this BitDefender bug. I doubt the majority of users running into this bug associate it BitDefender instead of Firefox. Given that SCONE doesn't provide user value yet, I assumed temporarily disabling its padding is worth those users being able to access facebook.com.
Do you disagree with this strategy @martinthomson? What should we do instead?
| if self.conn_params.scone_enabled() { | ||
| // This ensures that the last bytes are a SCONE indication, if there is enough space. | ||
| // This is not tracked, other than for congestion control (above) | ||
| if pad_amount >= Self::SCONE_INDICATION.len() { |
There was a problem hiding this comment.
At this point I don't know whether BitDefender trips over Self::SCONE_INDICATION[0] or Self::SCONE_INDICATION[1], thus I opted for disabling both on self.conn_params.scone_enabled(), using 0 padding instead as we did before.
@martinthomson is there much value in skipping Self::SCONE_INDICATION[0] but including Self::SCONE_INDICATION[1]?
| .skip(1) // Skip the last byte, which might be a SCONE indicator. | ||
| .find(|&(_, &v)| v != Connection::SCONE_INDICATION[0]) // The SCONE padding value. |
There was a problem hiding this comment.
Note that this is simply a revert of #2814. Fine for now in tests?
The
sconeconnection parameter (off by default since #3492) only gated advertisement of the SCONE transport parameter. The Initial packet SCONE indication (0xc8 0x13) was still emitted unconditionally, so every default-config client was signaling SCONE on the wire without actually negotiating it.This is suspected — not yet confirmed — to be the cause of a Firefox 150 regression where Facebook's edge reacts to the indicator and Bitdefender's HTTP/3 inspection then trips on the altered server-side
frames (bug 2034178). The QUIC handshake still completes, so the H3->H2 fallback never kicks in.
Gate both the build-time reservation and the pad-time indicator on
scone_enabled(); when disabled, pad Initials with zeros as before SCONE was introduced.Includes the version bump to
v0.24.2.Pull request targets the
v0.24branch, branched off of thev0.24.1tag.