⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Update BoringSSL to a newer version with updated patches #419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
kornelski merged 16 commits into from
Jan 20, 2026
Merged

Update BoringSSL to a newer version with updated patches #419

kornelski merged 16 commits into from
Jan 20, 2026

Conversation

@nox
Copy link
Collaborator

@nox nox commented Dec 20, 2025 •
edited
Loading

The BoringSSL commit this is based on is now 91a66a59b6c1435120ff83e245d7719411294386.

RPK support has drastically changed and is now more flexible, allowing clients to accept both RPKs and X.509 certificates.

While working on it, I noticed that how we disallowed X.509-related functions on RPK contexts was not as optimal nor as safe as it could be, which led to the second commit of this PR.

I'm going away for 8 months of parental leave on Tuesday so I guess I'll not be the person pushing the big green button on this 😁

@nox
Copy link
Collaborator Author

nox commented Dec 20, 2025

Ugh

  /Users/runner/work/boring/boring/target/x86_64-unknown-linux-gnu/debug/build/boring-sys-a316eb4dd2685ac9/out/boringssl/crypto/asn1/a_strex.cc:53:41: note: 'PRIX32' is defined in header '<cinttypes>'; did you forget to '#include <cinttypes>'?
     53 |     snprintf(buf, sizeof(buf), "\\U%04" PRIX32, c);
        |                                         ^~~~~~

@nox
Copy link
Collaborator Author

nox commented Dec 20, 2025

inttypes.h on some platforms is explicitly designed to not define PRIX32 and friends for C++ without __STDC_FORMAT_MACROS. BoringSSL itself is aware of it but I guess it never gets built on such platforms, or at least not anymore since this commit:

commit 5813c2c10c73d800f1b0d890a7d74ff973abbffc
Author: Adam Langley <[email protected]>
Date:   Wed Oct 30 22:48:00 2024

    crypto: switch to C++
    
    This change switches nearly all of BoringSSL to use C++. The public
    functions still use the C ABI, and so code written in C can still use
    BoringSSL. Also the use of the C++ standard library is minimal and no
    run-time requirement for it is intended.
    
    Change-Id: I902d2f51a3c8d6bd0dc4aabe1b192a15d7b788e8
    Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72747
    Commit-Queue: Adam Langley <[email protected]>
    Reviewed-by: Bob Beck <[email protected]>

@nox nox force-pushed the rpk branch 3 times, most recently from 839d1b4 to 20acdd6 Compare December 20, 2025 12:26
@nox
Copy link
Collaborator Author

nox commented Dec 20, 2025

The Android, MinGW and MSVC builds are all failing at link-time.

Android and MinGW missing symbols from the C++ runtime (which is now mandatory).

MSVC is missing __imp___CrtDbgReport.

@nox nox force-pushed the rpk branch 2 times, most recently from 7407adf to 1707caf Compare December 20, 2025 15:37
@nox
Copy link
Collaborator Author

nox commented Dec 20, 2025

Using CMAKE_MSVC_RUNTIME_LIBRARY fixed the MSVC builds.

@nox nox force-pushed the rpk branch 5 times, most recently from 3d6b352 to 9becf5a Compare December 22, 2025 10:09
@nox nox mentioned this pull request Dec 22, 2025
Closed
@bwesterb
Copy link
Member

bwesterb commented Dec 22, 2025

@nox From a quick look the PQ patch looks good to me. Were there merge conflicts or was it a clear rebase?

@bwesterb bwesterb self-requested a review December 22, 2025 14:16
@nox
Copy link
Collaborator Author

nox commented Dec 22, 2025

@nox From a quick look the PQ patch looks good to me. Were there merge conflicts or was it a clear rebase?

Everything changed since last time, as there was no SSL_CREDENTIAL concept in upstream BoringSSL.

@nox
Copy link
Collaborator Author

nox commented Dec 22, 2025

Silly me, you are talking about PQ, not RPK. Your patches applied cleanly.

kornelski and others added 11 commits January 14, 2026 00:58
@kornelski
Cross-platform Cargo registry cache
85cf1b0
@kornelski
cargo publish is target-specific
d224f2a
@kornelski
Include err.h in FFI bindings
8b17dc5
@kornelski
Use fips-build-compatible ERR_add_error_data
a37c6f4
@nox @kornelski
Define __STDC_FORMAT_MACROS when cross-building from macOS to Linux
0802744
@nox @kornelski
Pass -msse2 to i686 platforms on CI
9ba161b 
In upstream commit 56d3ad9d23bc130aa9404bfdd1957fe81b3ba498,
BoringSSL stopped assuming SSE2 support for i688 platforms,
requiring users to explicitly pass -msse2.

```
  target/i686-unknown-linux-gnu/debug/build/boring-sys-3edff0f2746d7cbb/out/boringssl/crypto/fipsmodule/../internal.h:120:2: error: #error "x86 assembly requires SSE2. Build with -msse2 (recommended), or disable assembly optimizations with -DOPENSSL_NO_ASM."
```
@nox @kornelski
Never use the debug CRT on Windows
785f23a 
See rust-lang/cmake-rs#30 (comment).
@nox @kornelski
Fix Android builds
6b61da0 
For starters, they should link against libc++, as they have always
intended to use STL "c++_shared".

https://github.com/Kitware/CMake/blob/824f2a7a200f9d3e41a2b68be2d5e1cc678afffa/Modules/Platform/Android-Common.cmake#L70-L75

Also, fix the variable names we define, as far as I know cmake never
cared about ANDROID_NATIVE_API_LEVEL nor ANDROID_STL.
@nox @kornelski
Fix MinGW builds
7cb9bf1 
Those need to link against libstdc++.
@kornelski
Flip is_rpk to has_x509_support
7e256f6
@nox @kornelski
Update boring to a newer version
96ba67d 
RPK support has changed completely, it uses SSL_CREDENTIAL now.

Have fun reviewing this!
@nox @kornelski
Rework SslMethod
fc8c77d 
Instead of keeping around a flag on contexts to know
whether we are configured for RPK, we start from SslMethod
with a flag to know whether it is configured for X.509
certificates. We then propagate this flag to context builders
and contexts, defaulting to false, and introduce
`assume_x509` methods to inform the crate of X.509 support
for contexts created with other means than our own functions.

This improves the safety of the crate as any `SslContextBuilder`
configured with `SslMethod::tls_with_buffer` would crash if used
with functions involving X.509 certificates. This `SslMethod` is
made unsafe because we can't guarantee that we check for X.509
support from all FFI bindingsi (for example, BoringSSL crashes
if there is a mismatch in X.509 support in `SSL_set_SSL_CTX`).

Note that there is no point anyway in forbidding X.509 functions
on a context that supports RPK, as current BoringSSL is able
to negociate both raw public keys and X.509 certificates on the
same context.

Finally, I removed `SslMethod::tls_client` and other peer-specific
methods as they are just the same as there non-peer-specific
equivalent methods.
@kornelski
Copy link
Collaborator

kornelski commented Jan 20, 2026

I've rebased c5e80ae on other CI fixes. Extracted 7e256f6 to make later patches have smaller conflicts.

@kornelski kornelski mentioned this pull request Jan 20, 2026
Merged
@kornelski kornelski added the v5 label Jan 20, 2026
@kornelski kornelski merged commit 1ba3998 into cloudflare:master Jan 20, 2026
22 checks passed
@kornelski kornelski mentioned this pull request Jan 20, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwesterb bwesterb bwesterb approved these changes

@kornelski kornelski kornelski approved these changes

@cjpatton cjpatton Awaiting requested review from cjpatton

@rushilmehra rushilmehra Awaiting requested review from rushilmehra

Assignees

No one assigned

Labels

v5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nox @bwesterb @kornelski