Adds support for linking against boringssl #1649
Conversation
|
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. |
openssl/src/ssl/mod.rs
Outdated
| // 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}; |
There was a problem hiding this comment.
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.
|
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. |
openssl/src/hkdf.rs
Outdated
| @@ -0,0 +1,89 @@ | |||
| use crate::cvt; | |||
There was a problem hiding this comment.
This is a drive-by feature addition? Not opposed, just want to make sure I understand.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
(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. :-)
There was a problem hiding this comment.
I'm happy to expose any part of OpenSSL's public API as long as the bindings are sound :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
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 |
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. |
|
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. |
|
Notable pieces that should probably be considered beyond simple review:
|
|
@sfackler Gentle ping |
|
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 |
maurer
force-pushed
the
boringssl
branch
2 times, most recently
from
866f251
to
b07aa3d
Compare
I didn't do it that way for a few reasons:
All that said, if you'd strongly prefer I do this with |
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. |
|
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++.
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. :-) |
|
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. |
|
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 |
|
I'm not sure I follow why having 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:
This means that to avoid using |
|
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 |
|
I think this looks good other than renaming the Cargo feature temporarily! I will merge and cut a release once that's done. |
maurer
force-pushed
the
boringssl
branch
2 times, most recently
from
4b81cb8
to
efec36d
Compare
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 <mmaurer@google.com>
* 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
|
I renamed the feature, and added two extra cleanup commits to keep the build happy:
|
|
Thanks for sticking with it! I will cut a release tonight. |
| #[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; |
There was a problem hiding this comment.
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
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 mmaurer@google.com
Co-authored-by: Andrew Scull ascull@google.com
Fixes: #1519