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
Open
nox wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Update BoringSSL to a newer version with updated patches #419

nox wants to merge 8 commits into from

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 2 times, most recently from ea2358a to 7dc02da Compare December 20, 2025 12:00
nox added 5 commits December 20, 2025 13:23
@nox
Update boring to a newer version
d46960f 
RPK support has changed completely, it uses SSL_CREDENTIAL now.

Have fun reviewing this!
@nox
Rework SslMethod
d662e39 
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.
@nox
Define __STDC_FORMAT_MACROS when cross-building from macOS to Linux
bda2944
@nox
Pass -msse2 to i686 platforms on CI
d51ce28 
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
Disable fail-fast on GitHub CI
20acdd6
@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 2 times, most recently from 94a5188 to d21b3b6 Compare December 20, 2025 21:42
@nox nox force-pushed the rpk branch 2 times, most recently from 3218133 to 3d6b352 Compare December 22, 2025 10:03
@nox
Fix Android builds
9becf5a 
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
Fix MinGW builds
c5e80ae 
Those need to link against libstdc++.
@nox nox mentioned this pull request Dec 22, 2025
Open
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwesterb bwesterb Awaiting requested review from bwesterb

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nox @bwesterb