Adds support for linking against boringssl #1649

maurer · 2022-06-02T20:07:13Z

The boringssl-sys crate is specific to the revision of boringssl you are
using, so you will need to generate it from a boringssl build tree and
point the openssl crate at it via cargo source replacement, or place the
build tree next to rust-openssl while building.

Co-authored-by: Matthew Maurer [email protected]
Co-authored-by: Andrew Scull [email protected]
Fixes: #1519

maurer · 2022-06-02T20:10:59Z

We're currently using We're currently using these patches in Android with a bit of extra glue to connect with our build system.

We're open to restructuring this patch (even significantly), but this is a working starting point.

sfackler · 2022-06-03T01:04:05Z

openssl/src/ssl/mod.rs

+// use bitflags::bitflags;
+// use cfg_if::cfg_if;
+// use foreign_types::{ForeignType, ForeignTypeRef, Opaque};
+use libc::{c_char, c_int, c_long, c_uchar, c_uint, c_ulong, c_void};
+// use once_cell::sync::{Lazy, OnceCell};
+// use std::any::TypeId;
+// use std::cmp;
+// use std::collections::HashMap;
+// use std::ffi::{CStr, CString};
+// use std::fmt;
+// use std::io;
+// use std::io::prelude::*;
+// use std::marker::PhantomData;
+// use std::mem::{self, ManuallyDrop};
+// use std::ops::{Deref, DerefMut};
+// use std::panic::resume_unwind;
+// use std::path::Path;
+// use std::ptr;
+// use std::slice;
+// use std::str;
+// use std::sync::{Arc, Mutex};


This was removed in the version we've got in Android, but didn't make it into the merge to produce this patch, I'll remove it.

openssl/Cargo.toml

sfackler · 2022-06-03T01:08:20Z

The path-based bssl-sys isn't going to work since we can't publish with that setup.

openssl-sys now optionally uses bindgen to generate its bindings - could we avoid the separate sys crate by using that for boringssl? It seems fine to require bindgen for boringssl support.

sfackler · 2022-06-03T01:10:04Z

openssl/src/hkdf.rs

@@ -0,0 +1,89 @@
+use crate::cvt;


This is a drive-by feature addition? Not opposed, just want to make sure I understand.

OpenSSL exposes HKDF via the EVP_PKEY API. BoringSSL doesn't, so this was to reach parity there. It's easy to split this back into a separate patch (it was in our repo), I had it squashed because it was part of bringing BoringSSL up to parity.

Would you prefer this in a separate commit?

(Drive-by comment, I haven't looked at any of this PR yet.)

FWIW we'll probably add OpenSSL's HKDF API in BoringSSL soon as a compatibility shim for legacy code. That might make this simpler.

It's not a good API... jamming HKDF into the same API as ECDH is absurd and forced other API mistakes (too many copies and some completely ad hoc notion of "modes" that is unsupported by the HKDF specification), but apparently that's what other projects expect so okay whatever. :-)

I'm happy to expose any part of OpenSSL's public API as long as the bindings are sound :)

OpenSSL often designs bad APIs. You really should expose, say, a good HKDF API and just happen to implement it with OpenSSL.

Ideally those two are one and the same, but not here in the slightest. To that end...

These are reasonable APIs
https://docs.rs/openssl-hkdf/0.2.0/openssl_hkdf/hkdf/fn.hkdf_expand.html
https://docs.rs/openssl-hkdf/0.2.0/openssl_hkdf/hkdf/fn.hkdf_extract.html

These are not and should not be exported.
https://docs.rs/openssl-hkdf/0.2.0/openssl_hkdf/hkdf/struct.HkdfDeriver.html
This is simply not how HKDF works at all. Please consult RFC 5869. Indeed even OpenSSL are moving away from stuffing KDFs into EVP_PKEY_CTX (see EVP_KDF), though their new one still makes similar mistakes.

I am absolutely aware that OpenSSL's APIs contain significant bits of jank. However, generally speaking I see the role of this library to as directly as possible expose those APIs, jank and all.

I mean, that makes sense for something complex like TLS where you need to expose it as-is to get full fidelity. For something like this, OpenSSL's APIs are essentially a really messy builder pattern. You don't get any better fidelity by exposing it, just an incorrect interpretation of OpenSSL's design patterns and unsafe APIs. (OpenSSL's API here mixes together two completely different kinds of secrets in this notion of "mode". You actually break domain separation---meaning protocols built this way aren't secure---by doing this.)

maurer · 2022-06-04T15:46:13Z

The path-based bssl-sys isn't going to work since we can't publish with that setup.

openssl-sys now optionally uses bindgen to generate its bindings - could we avoid the separate sys crate by using that for boringssl? It seems fine to require bindgen for boringssl support.

One of the parts of the negotiation with the boringssl folks to add the Rust bindings there was to have the generation inside boringssl rather than as a standalone or external package so that the bindings would always be updated in sync. Effectively, upstream wants to deliver the bindings with their package, not have them be installed alongside.

That said, there's nothing stopping us from having a bssl-sys crate with a specific boringssl and its corresponding generated sources. I'd want to chat with them before doing this (boringssl does not have "releases", so there's no obvious cadence to update such a crate on). What do you think?

davidben · 2022-06-04T17:19:44Z

That said, there's nothing stopping us from having a bssl-sys crate with a specific boringssl and its corresponding generated sources. I'd want to chat with them before doing this (boringssl does not have "releases", so there's no obvious cadence to update such a crate on).

We can discuss this more during the week, but we tried this with mundane. I think it was a mistake and we should undo what we did there. It's possible there are other ways to do it that will work less badly, but my sense so far is cargo is unsuitable for shipping a non-Rust library like this.

sfackler · 2022-06-12T23:13:09Z

I don't know how boringssl is distributed, but one potential possibility could be to have BoringSSL actually install the generated Rust sources in /etc or wherever and have openssl-sys pull those in directly. That way we could be confident that they're matching with the version we're linking against but not be forced to bundle the BoringSSL C sources in bssl-sys.

maurer · 2022-07-23T17:01:54Z

Notable pieces that should probably be considered beyond simple review:

I changed the -1 mac_iter value to use self.mac_iter, as boringssl rejects it. I believe this to be correct and tests pass, but I wanted to call attention to it.
I am currently adding a boringssl-master test in addition to a specific revision of boringssl. The intention is that you only make the specific version a blocking test, and leave master as a warning/informative
I've removed the boringssl HKDF patch, we can land that in a follow-up CL
The intended way to use boringssl is displayed in ci.yml - basically you patch a in a replacement for the placeholder I've put on crates.io. If this approach is acceptable, I can add instructions to README.md.

maurer · 2022-08-10T20:21:53Z

@sfackler Gentle ping

openssl/src/bn.rs

openssl/src/ssl/test/mod.rs

openssl/src/x509/mod.rs

sfackler · 2022-08-20T23:03:20Z

Using patch to hook up to the installed bindings seems a bit janky. Couldn't we have openssl-sys pull them in automatically with the build script finding the right directory and an include!(...)?

maurer · 2022-09-01T18:31:00Z

Using patch to hook up to the installed bindings seems a bit janky. Couldn't we have openssl-sys pull them in automatically with the build script finding the right directory and an include!(...)?

I didn't do it that way for a few reasons:

The rust crate doesn't get installed anywhere, it just lives in the build output directory. This means that users will never successfully autodetect through build.rs, they will always need to tell it where to find it somehow, likely an environment variable.
The crate has two files right now, and might have more in the future (to deal with some functions that are header-only in boringssl but are not in openssl). This means that include! would not be sufficient by itself, we'd also need a local mimic of the directory / module structure, and this could easily get out of sync.
If we built with environment variable based detection, then when we (Android) consume this crate, we'd have to carry a patch patching that back out, because users can't set environment variable in their builds, and we'd have to do a bunch of hackery. I know this isn't a constraint on your development, but I included it in the interests of completeness.

All that said, if you'd strongly prefer I do this with include! and env! I can do it.

sfackler · 2022-09-14T01:40:25Z

Could it be installed somewhere?
I think include! can handle submodules properly, and if it can't #[path = "..."] mod foo; definitely can.
If it can be installed somewhere that's specified in e.g. a pkg-config variable we wouldn't need the env var.

I guess more broadly I'm wondering about the intended consumers of the BoringSSL support. If it's just Google internal stuff then I guess I don't care how janky the integration is :). If it's something that people in the broader ecosystem could pick up then I'm going to be more picky about the setup. I know BoringSSL has historically been a "if you aren't in Google, don't use this" kind of situation but it seems like that is empirically not really limiting external users too much.

davidben · 2022-09-19T21:56:40Z

BoringSSL is not something we support installing system-wide in that way. (Not unlike typical Rust libraries!) We don't promise a stable ABI or API and instead expect that consumers match header and link version, build against known revisions, and keep those revisions regularly updated. The so called "live at head" model.

This means the overall project (the root of the dependency tree) using it will have some pre-existing story for building BoringSSL and some pre-existing opinion as to which instance they're building. This is especially important when you don't want to link two copies of a dependency together. (Either because the language, like C/C++, does not support it, or because the dependency's types leak through another library, such as rust-openssl, or because you don't want to worry about security bugs in both copies.)

Alas, tools for cross-language dependency management, especially if one is C/C++, are so fragmented that there isn't much for a library build to do but to provide some way for the user to point at a dependency instance and let them pick. Auto-detection will just risk picking up the wrong version, possibly resulting in multiple linking and thus memory errors in C/C++.

I know BoringSSL has historically been a "if you aren't in Google, don't use this" kind of situation but it seems like that is empirically not really limiting external users too much.

Please see the README for what it actually says. There are some non-Google users of BoringSSL, but they're one that can work with the deployment model. That they can work with it is not a reason for rust-openssl to disregard it. :-)

davidben · 2022-09-19T21:57:25Z

I'm not familiar enough with Cargo to know how best to translate that to Cargo (I'll leave that to you all), but that's the deployment model to aim for.

sfackler · 2022-09-21T01:11:07Z

Makes sense that we'd never expect BoringSSL to be installed at the system level.

While this is probably the first time the build process will be searching for Rust source code in an arbitrary external folder (or at least it's very uncommon), it feels basically equivalent to the very common need to search for C header files. The openssl-sys build script does this to discover exactly which version of Open/LibreSSL it's building against for example, and if the bindgen feature is enabled will generate the bindings directly from those headers. Just like with a pure-C or C++ build system you always run the risk of finding headers and libraries that are not ABI compatible, but I think that's basically unavoidable.

In the Open/LibreSSL world, we need to find the location of the runtime libraries and the location of the header files. Ignoring the pkg-config workflow, the build environment would need to inject either the OPENSSL_DIR environment variable, in which case we look in $OPENSSL_DIR/lib and $OPENSSL_DIR/include, or for the OPENSSL_LIB_DIR and OPENSSL_INCLUDE_DIR environment variables which specify each directory directly. It doesn't matter if that directory is /usr or $HOME/my-local-openssl-copy or whatever. We could specify that when linking against BoringSSL, we'd look in $OPENSSL_DIR/etc/rust/src or $OPENSSL_RUST_DIR for the generated binding code, then we can adjust the openssl-sys build script to pull in that source directly without the need to override bits of the standard Cargo build system.

maurer · 2022-09-21T02:10:42Z

I'm not sure I follow why having OPENSSL_RUST_DIR as an environment variable is considered cleaner than a .cargo/config.toml setting, but I don't feel strongly about this. I'll adjust the patches to use an environment variable instead.

Note: It's going to be a bit ugly - I'm going to have to do the "have a build system that is in sync with what is in boringssl" thing, as:

#[path] does not support macro expansion, so I can't put env! in there.
include! does seem to search relative to the include!'d source (surprised there, but happily so)
The build.rs for the bssl-sys crate currently builds and links in a supplementary .c file.

This means that to avoid using .cargo/config.toml, openssl-sys's build.rs will need to include! the contents of the other build.rs (possibly need to copy the .c and .h files or modify the build script somehow so it will find them?), and then we will be able to include! the library.

sfackler · 2022-09-21T11:32:41Z

Oh, if there's a build script involved in bssl-sys, then we may not be able to take that approach then, unfortunately.

We should be able to go with the .cargo/config approach. I'll take another look over the PR tonight, but we can probably land it.

openssl-sys/Cargo.toml

sfackler · 2022-09-22T00:43:34Z

I think this looks good other than renaming the Cargo feature temporarily! I will merge and cut a release once that's done.

The boringssl-sys crate is specific to the revision of boringssl you are using, so you will need to generate it from a boringssl build tree and point the openssl crate at it via cargo source replacement, or place the build tree next to rust-openssl while building. Co-authored-by: Matthew Maurer <[email protected]>

* BoringSSL does not use the bindgen feature, since it generates the crate separately. * BoringSSL shouldn't run the systest tests (maintained out of tree) or the openssl-errors tests (BoringSSL doesn't support external errors) * Adds both master and a known-working revision to CI. The master build should be made non-blocking.

* Needless borrow * Explicit auto-deref

maurer · 2022-09-23T02:17:07Z

I renamed the feature, and added two extra cleanup commits to keep the build happy:

Fix up compiler lints (necessary)
Fix up clippy lints (bonus, just did it while I was at it)

sfackler · 2022-09-26T13:17:35Z

Thanks for sticking with it! I will cut a release tonight.

eloff · 2022-12-06T00:32:37Z

openssl-sys/src/lib.rs

+        #[cfg(not(ossl111b))]
+        let init_options = OPENSSL_INIT_LOAD_SSL_STRINGS;
+        #[cfg(ossl111b)]
+        let init_options = OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_NO_ATEXIT;


This fixes some crashes I was experiencing in diesel using libpq, which uses openssl. The atexit handler was racing the r2d2 connection pool worker threads when main() exits, and that caused a seg fault.

See: sfackler/r2d2#137

👍🏼 Reproduced using MySql also

sfackler reviewed Jun 3, 2022

View reviewed changes

openssl/Cargo.toml Outdated Show resolved Hide resolved

sfackler reviewed Jun 3, 2022

View reviewed changes

maurer force-pushed the boringssl branch from 2b41512 to 8f885c2 Compare July 23, 2022 16:57

maurer force-pushed the boringssl branch from 8f885c2 to c185478 Compare July 23, 2022 17:10

maurer requested a review from sfackler July 23, 2022 23:14

maurer force-pushed the boringssl branch from c185478 to 1c2df4b Compare August 10, 2022 19:57

sfackler reviewed Aug 20, 2022

View reviewed changes

openssl/src/bn.rs Outdated Show resolved Hide resolved

sfackler reviewed Aug 20, 2022

View reviewed changes

openssl/src/ssl/test/mod.rs Outdated Show resolved Hide resolved

sfackler reviewed Aug 20, 2022

View reviewed changes

openssl/src/x509/mod.rs Outdated Show resolved Hide resolved

maurer force-pushed the boringssl branch 2 times, most recently from 866f251 to b07aa3d Compare September 1, 2022 18:23

sfackler reviewed Sep 22, 2022

View reviewed changes

openssl-sys/Cargo.toml Outdated Show resolved Hide resolved

maurer force-pushed the boringssl branch from b07aa3d to 5f3024d Compare September 23, 2022 01:07

maurer force-pushed the boringssl branch 2 times, most recently from 4b81cb8 to efec36d Compare September 23, 2022 01:36

benbrittain and others added 4 commits September 22, 2022 18:51

Fix new compiler lints (explicit drop for Boxes)

c26bdb3

Fix new clippy lints

f5cb751

* Needless borrow * Explicit auto-deref

maurer force-pushed the boringssl branch from b434316 to f5cb751 Compare September 23, 2022 01:52

Merge branch 'master' into boringssl

c49476b

sfackler merged commit 0af10fb into sfackler:master Sep 26, 2022

eloff reviewed Dec 6, 2022

View reviewed changes

alex mentioned this pull request Dec 25, 2022

Detect boringssl in expando.c? #1768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for linking against boringssl #1649

Adds support for linking against boringssl #1649

maurer commented Jun 2, 2022

maurer commented Jun 2, 2022

sfackler Jun 3, 2022

maurer Jun 4, 2022

sfackler commented Jun 3, 2022

sfackler Jun 3, 2022

maurer Jun 4, 2022 •

edited

Loading

davidben Jun 4, 2022

sfackler Jun 12, 2022

davidben Jun 13, 2022

sfackler Jun 13, 2022

davidben Jun 13, 2022

maurer commented Jun 4, 2022

davidben commented Jun 4, 2022 •

edited

Loading

sfackler commented Jun 12, 2022 •

edited

Loading

maurer commented Jul 23, 2022

maurer commented Aug 10, 2022

sfackler commented Aug 20, 2022

maurer commented Sep 1, 2022

sfackler commented Sep 14, 2022

davidben commented Sep 19, 2022

davidben commented Sep 19, 2022

sfackler commented Sep 21, 2022

maurer commented Sep 21, 2022 •

edited

Loading

sfackler commented Sep 21, 2022

sfackler commented Sep 22, 2022

maurer commented Sep 23, 2022

sfackler commented Sep 26, 2022

eloff Dec 6, 2022

thomasmost Apr 28, 2023

Adds support for linking against boringssl #1649

Adds support for linking against boringssl #1649

Conversation

maurer commented Jun 2, 2022

maurer commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Jun 3, 2022

Choose a reason for hiding this comment

maurer Jun 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurer commented Jun 4, 2022

davidben commented Jun 4, 2022 • edited Loading

sfackler commented Jun 12, 2022 • edited Loading

maurer commented Jul 23, 2022

maurer commented Aug 10, 2022

sfackler commented Aug 20, 2022

maurer commented Sep 1, 2022

sfackler commented Sep 14, 2022

davidben commented Sep 19, 2022

davidben commented Sep 19, 2022

sfackler commented Sep 21, 2022

maurer commented Sep 21, 2022 • edited Loading

sfackler commented Sep 21, 2022

sfackler commented Sep 22, 2022

maurer commented Sep 23, 2022

sfackler commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurer Jun 4, 2022 •

edited

Loading

davidben commented Jun 4, 2022 •

edited

Loading

sfackler commented Jun 12, 2022 •

edited

Loading

maurer commented Sep 21, 2022 •

edited

Loading