Skip to content

Upgrade boring to 5.1#2446

Open
cjpatton wants to merge 8 commits into
cloudflare:masterfrom
cjpatton:cjpatton/boring-5
Open

Upgrade boring to 5.1#2446
cjpatton wants to merge 8 commits into
cloudflare:masterfrom
cjpatton:cjpatton/boring-5

Conversation

@cjpatton
Copy link
Copy Markdown
Contributor

@cjpatton cjpatton commented Apr 23, 2026

Bumps the boring workspace dependency from 4.3 to 5.1 and pins the quiche/deps/boringssl submodule to match boring-sys 5.1.0. Updates test expectations and CI accordingly (see commit messages for details).
Reviewers are encouraged to review these commit-by-commit. The changes were driven mostly by CI feedback.

@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch 11 times, most recently from 3a23c66 to 5f29272 Compare April 27, 2026 16:26
Comment thread quiche/src/tests.rs Outdated
Comment thread quiche/src/tests.rs Outdated
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch from 3b2caba to 239818b Compare April 27, 2026 18:41
Comment thread .github/workflows/stable.yml
Comment thread quiche/src/tests.rs Outdated
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch from 239818b to ac2778b Compare April 27, 2026 18:54
Comment thread quiche/src/build.rs Outdated
Comment thread quiche/src/build.rs Outdated
Comment thread Cross.toml
@cjpatton cjpatton marked this pull request as ready for review April 27, 2026 19:12
@cjpatton cjpatton requested a review from a team as a code owner April 27, 2026 19:12
Comment thread quiche/patches/boring-pq.patch Outdated
Comment thread quiche/src/lib.rs Outdated
Comment thread Cross.toml
Comment thread Cargo.toml Outdated
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch 4 times, most recently from bcf1b89 to 974059a Compare April 28, 2026 21:25
@cjpatton cjpatton requested a review from ghedo April 28, 2026 21:38
Comment thread .github/workflows/stable.yml
Comment thread .github/workflows/stable.yml
Comment thread quiche/src/tests.rs Outdated
Comment thread Cargo.toml Outdated
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch from 974059a to 26fcf2b Compare April 30, 2026 22:40
Comment thread quiche/src/tests.rs
let mut config = Config::new(PROTOCOL_VERSION).unwrap();
// Test assumes the server transitions to early-data state after a
// single `server_recv`, which requires a single-Initial ClientHello.
let mut config = test_utils::config_no_pq(PROTOCOL_VERSION).unwrap();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add 0-RTT tests that operate on post-quantum handshakes. How difficult would it be to parametrize this test to cover both cases?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is really hard coded around Client Hello fitting in the initial packet. We could write a different test that exercises 0-RTT, but it would work differently. Want me to add that?

@cjpatton cjpatton marked this pull request as draft May 19, 2026 21:48
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch 6 times, most recently from e707787 to 26261a7 Compare May 20, 2026 15:40
Comment thread .github/workflows/nightly.yml
Comment thread quiche/src/tests.rs
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch from 26261a7 to 05c6bd3 Compare May 20, 2026 18:03
cjpatton added 7 commits May 20, 2026 13:38
Bumps the `boring` workspace dependency from 4.3 to 5.1 and pins the
`quiche/deps/boringssl` submodule to match boring-sys` 5.1.0.

The main user-visible change is that post-quantum TLS key shares
(X25519MLKEM768 and P256Kyber768Draft00) are now offered by default.
`boringssl-boring-crate` inherits PQ defaults from `boring-sys` 5.1,
which applies `boring-pq.patch` to its vendored BoringSSL.

Also, update RPK to use the new credentials API in `boring-sys` 5.1.

Test expectations that encoded byte counts or packet counts from
handshakes with a single-Initial ClientHello have been updated for the
new multi-Initial with the larger ClientHello (containing
X25519MLKEM768).

A small set of tests that fundamentally require a single-Initial
ClientHello (e.g. they drive the handshake one packet at a time) opt out
of PQ via a new `test_utils::config_no_pq(version)` helper.

`Config::set_curves_list` is added as a `pub(crate)` method so
`test_utils` can wire classical curves through the BoringSSL and
openssl-quictls backends uniformly. The symbol is a real function on
BoringSSL and a header macro on OpenSSL; each backend provides an
`SSL_CTX_set1_groups_list` shim so the caller stays backend-agnostic.
BoringSSL 5.x emits C++ symbols (vtables, exception personality,
std::optional, etc.) into its static archives. Linking `libquiche.a`
into the C example binaries with `cc` therefore fails with undefined
references to the C++ runtime.

Switch the link step to `$(CXX)` so libc++/libstdc++ is pulled in
automatically. Wrap the source with `-x c ... -x none` to keep the C
language semantics for the .c file and let clang re-detect the
following `.a` archive normally.
boring-sys 5.x's build script unconditionally passes
`CMAKE_MSVC_RUNTIME_LIBRARY` for any `target_os == "windows"`. Combined
with cmake's default Windows behaviour, this makes cmake look for
`cl.exe` even on `*-windows-gnu` targets, where only MinGW gcc is
available, and the build fails during compiler detection.

Replace the `bwoodsend/setup-winlibs-action` step with a setup that
mirrors `boring`'s own CI:

  - For `x86_64-pc-windows-gnu`, point CC/CXX/include/library at the
    MSYS2 toolchain preinstalled at `C:\msys64`.
  - For `i686-pc-windows-gnu`, install a 32-bit MSYS2 environment via
    `msys2/setup-msys2@v2` (the runner's preinstalled MSYS2 is 64-bit
    only) and force `CMAKE_GENERATOR="MinGW Makefiles"` so cmake-rs
    doesn't fall back to "MSYS Makefiles" + cl.exe.

Also set `CXXFLAGS=-msse2` for `i686-pc-windows-msvc` to satisfy
BoringSSL's x86 SSE2 requirement on that target.
`tests::initial_cwnd` asserts that `tx_cap` falls in `[expected,
expected + 1]` after the handshake. For BBR2 in Startup the
congestion window grows by exactly `bytes_acked` per ACK, so the
post-handshake `tx_cap` equals `initial_cwnd` plus the bytes the
server sent during the handshake that the client has acknowledged.
That total is sensitive to the encoded length of the ACK frame's
`ack_delay` field (a VarInt of microseconds since receipt), which
can shift by a byte between architectures.

On aarch64 and armv7 (under `cross`'s Docker+QEMU) we observe
`tx_cap = expected + 2`, just outside the existing tolerance.
Allow a 4-byte upper-bound tolerance and replace the prior
`TODO understand` comment with an explanation of what the test is
actually measuring.
Two CI jobs broke after the boring 4 -> 5 upgrade because BoringSSL
is now built by `boring-sys` rather than by `quiche/src/build.rs`,
and a couple of flags we used to set in-tree need to be re-injected
from outside the build.

1. fuzz (nightly): BoringSSL 5.x consolidated the older
   `BORINGSSL_UNSAFE_{DETERMINISTIC,FUZZER}_MODE` defines into a
   single `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` switch.

2. quiche_multiarch (i686-unknown-linux-gnu): BoringSSL's x86
   assembly requires SSE2. `boring-sys` does set `-msse2` via
   `util/32-bit-toolchain.cmake`, but only when host != target; the
   `cross` `i686:edge` image is 32-bit-native, so that branch is
   skipped and the build fails with `x86 assembly requires SSE2`.
   Add a target-scoped `CFLAGS`/`CXXFLAGS` passthrough in
   `Cross.toml` so `-msse2 -mfpmath=sse` reaches BoringSSL's cmake
   build regardless.
Allow quiche to build with boring >= 4.19. The main differences between
4.x and 5.x are the RPK API and PQ enablement. Detect which version is
running at build time by parsing the Cargo.lock file: if 4.x is
detected, then emit `cfg(boring_v4)`. This is used to pick which RPK
APIs to build and adjust test expectations.

One alternative considered was to add boring 5.x support as a feature
flag. However, boring-sys doesn't allow two versions of itself to exist
in a crate's dependency tree. (I believe this is to prevent linking
against two copies of BoringSSL.)
@cjpatton cjpatton force-pushed the cjpatton/boring-5 branch from 05c6bd3 to 78e6a2c Compare May 20, 2026 20:38
@cjpatton cjpatton marked this pull request as ready for review May 20, 2026 20:41
@cjpatton
Copy link
Copy Markdown
Contributor Author

Rebased, ready for review once more.

Comment thread quiche/src/build.rs Outdated
Comment thread quiche/src/build.rs Outdated
/// exist at this point in the build, (b) parsing it is cheap and has
/// no extra dependencies, and (c) it avoids re-entering cargo from a
/// build script.
fn detect_boring_v4() -> bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea 😅 Could we check boring_sys::BORINGSSL_API_VERSION during build? It looks like v4 and v5 use different versions:
https://docs.rs/boring-sys/4.22.0/boring_sys/constant.BORINGSSL_API_VERSION.html
https://docs.rs/boring-sys/5.1.0/boring_sys/constant.BORINGSSL_API_VERSION.html

and since I don't think we'll ever update BoringSSL in v4 again BORINGSSL_API_VERSION <= 21.

Would require adding boring-sys as build dependency as well (optional in quiche and gated on boringssl-boring-crate), but not entirely sure it would work properly...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about differences in the boring API itself? E.g., in v4 the RPK API is different than in v5. While it's true that BORINGSSL_API_VERSION is sufficient for detecting differences between v4 and v5, this may not be true for v6, or even v5.2.x. It seems like detecting the crate version is strictly more robust, since it always has the information that we need.

Comment thread quiche/src/build.rs Outdated
Assumes boring 4 by default.
@cjpatton cjpatton requested a review from ghedo May 22, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants