boring-sys: Ignore patches when boringSSL is precompiled #331
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjpatton
commented
Mar 13, 2025
Comment on lines
+721
to
723
| #[cfg(all(not(feature = "fips"), feature = "pq-experimental"))] | ||
| pub const X25519_KYBER768_DRAFT00_OLD: SslCurve = | ||
| SslCurve(ffi::SSL_CURVE_X25519_KYBER768_DRAFT00_OLD as _); |
Collaborator
Author
There was a problem hiding this comment.
Alternatively we could just remove these non-standard APIs from the bindings.
cjpatton
commented
Mar 13, 2025
cjpatton
force-pushed
the
cjpatton/prebuilt-ignore-patches
branch
from
0b94828 to
f587ae2
Compare
cjpatton
commented
Mar 14, 2025
| SslCurve(ffi::SSL_CURVE_X25519_KYBER768_DRAFT00 as _); | ||
|
|
||
| #[cfg(feature = "pq-experimental")] | ||
| #[cfg(all(not(feature = "fips"), feature = "pq-experimental"))] |
Collaborator
Author
There was a problem hiding this comment.
not(feature = "fips") prevents us from building this code whenever "fips" is enabled. This is necessary because this API is too new for older boringSSL builds.
We could weaken this constraint to not(feature = "fips-compat"), but this is somewhat of a broader change than what this PR is trying to do.
Note: this is not a breaking change because "fips" and "pq-experimental" were previously mutually exclusive.
kornelski
approved these changes
Mar 14, 2025
Internal users often have two builds for `boring`, one using a precompiled build of boringSSL and another built from source with patches applied. However the features that enable these builds are mutually exclusive. For example, the `"pq-experimental"` feature is required to build the source with all of the necessary codepoints for PQ key exchange, but if this feature is enabled and a precompiled boringSSL is provided, then the build will fail. This means users will have to also control their builds with mutually exclusive features. An alternative is to *ignore* features that enable patches whenever a precompiled boringSSL is provided. This is a little different from the "assume patched" environment variable, which applies whenever we're building from source.
The "fips" feature implies use of a prebuilt boringSSL. The boringSSL API consumed by `SslCurve` in incompatible with older versions of boringSSL. In the `ffi` bindings, the following symbols don't exist in older builds: * NID_X25519MLKEM768 * SSL_CURVE_X25519_MLKEM768 * NID_X25519Kyber768Draft00Old The following symbols have been renamed: * SSL_CURVE_P256KYBER768DRAFT00 => SSL_CURVE_P256_KYBER768_DRAFT00 * SSL_CURVE_X25519KYBER512DRAFT00 => SSL_CURVE_X25519_KYBER512_DRAFT00 * SSL_CURVE_X25519KYBER768DRAFT00OLD => SSL_CURVE_X25519_KYBER768_DRAFT00_OLD * SSL_CURVE_P256KYBER768DRAFT00 => SSL_CURVE_P256_KYBER768_DRAFT00 Meanwhile, the `ssl_set_curves_list()` API is stable across these versions of boringSSL. These codepoints are added to the `SslCurve` API whenever "pq-experimental" is enabled. Since this feature is no longer mutually exclusive with prebuilt boringSSL (`boring-sys` just ignores patches), we also need to disable this API whenever "fips" is enabled.
cjpatton
force-pushed
the
cjpatton/prebuilt-ignore-patches
branch
from
f587ae2 to
90121f0
Compare
Collaborator
Author
|
Rebased, plus I modified the warning to make it visible during the build. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Internal users often have two builds for
boring, one using a precompiled build of boringSSL and another built from source with patches applied. However the features that enable these builds are mutually exclusive. For example, the"pq-experimental"feature is required to build the source with all of the necessary codepoints for PQ key exchange, but if this feature is enabled and a precompiled boringSSL is provided, then the build will fail. This means users will have to also control their builds with mutually exclusive features.An alternative is to ignore features that enable patches whenever a precompiled boringSSL is provided. This is a little different from the "assume patched" environment variable, which applies whenever we're building from source.