The change to BoringSSL breaks builds #102

ckruse · 2023-12-01T08:27:52Z

Hey there,

the change to BoringSSL breaks builds, I now get weird OpenSSL linker errors:

= note: /usr/bin/ld: /home/runner/work/foo/target/debug/deps/libopenssl-d864c125a418118f.rlib(openssl-d864c125a418118f.openssl.86fb40cd50d7adbe-cgu.11.rcgu.o): in function `openssl::error::Error::get':
          /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-0.10.59/src/error.rs:122: undefined reference to `ERR_get_error_all'
          /usr/bin/ld: /home/runner/work/foo/target/debug/deps/libopenssl_sys-2290329161c421c2.rlib(openssl_sys-2290329161c421c2.openssl_sys.30b56d82c2cac715-cgu.0.rcgu.o): in function `openssl_sys::openssl::ssl::SSL_CTX_set_mode':
          /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-sys-0.9.95/src/./ssl.rs:246: undefined reference to `SSL_CTX_ctrl'
          /usr/bin/ld: /home/runner/work/foo/target/debug/deps/libopenssl_sys-2290329161c421c2.rlib(openssl_sys-2290329161c421c2.openssl_sys.30b56d82c2cac715-cgu.0.rcgu.o): in function `openssl_sys::openssl::ssl::SSL_CTX_add_extra_chain_cert':
          /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-sys-0.9.95/src/./ssl.rs:380: undefined reference to `SSL_CTX_ctrl'
          /usr/bin/ld: /home/runner/work/foo/target/debug/deps/libopenssl_sys-2290329161c421c2.rlib(openssl_sys-2290329161c421c2.openssl_sys.30b56d82c2cac715-cgu.0.rcgu.o): in function `openssl_sys::openssl::tls1::SSL_set_tlsext_host_name':
          /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-sys-0.9.95/src/./tls1.rs:24: undefined reference to `SSL_ctrl'
          /usr/bin/ld: /home/runner/work/foo/target/debug/deps/libopenssl_sys-2290329161c421c2.rlib(openssl_sys-2290329161c421c2.openssl_sys.30b56d82c2cac715-cgu.0.rcgu.o): in function `openssl_sys::openssl::ssl::SSL_CTX_set_min_proto_version':
          /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-sys-0.9.95/src/./ssl.rs:455: undefined reference to `SSL_CTX_ctrl'
          /usr/bin/ld: /home/runner/work/foo/target/debug/deps/libopenssl_sys-2290329161c421c2.rlib(openssl_sys-2290329161c421c2.openssl_sys.30b56d82c2cac715-cgu.0.rcgu.o): in function `openssl_sys::openssl::ssl::SSL_CTX_set_max_proto_version':
          /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-sys-0.9.95/src/./ssl.rs:464: undefined reference to `SSL_CTX_ctrl'
          collect2: error: ld returned 1 exit status
          
  = note: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
  = note: use the `-l` flag to specify native libraries to link
  = note: use the `cargo:rustc-link-lib` directive to specify the native libraries to link with Cargo (see https://doc.rust-lang.org/cargo/reference/build-scripts.html#cargorustc-link-libkindname)

I guess this is because of a conflict betweet OpenSSL and BoringSSL. Am I right?

What can I do to fix this?

The text was updated successfully, but these errors were encountered:

photino · 2023-12-01T09:04:07Z

We also have this prolem.

jedisct1 · 2023-12-01T10:14:28Z

A possible workaround is to replace OpenSSL with BoringSSL in other dependencies.

In particular, there are tokio-boring and hyper-boring that can act as drop-in replacements for their openssl-based counterparts.

Alternatively, the legacy implementation is still there. It's currently only enabled when targeting WebAssembly.

Ideally, there should be a cargo compile-time flag to optionally force its usage on other targets, but I didn't find any ways to express "use this set of dependencies if the target is webassembly or if that compile flag was given" with cargo.

jedisct1 · 2023-12-01T10:21:47Z

If you can write a small test case that reproduces the issue, maybe this is something that can be reported to the maintainers of the boring crate. BoringSSL has a build option to add a prefix to symbols in order to avoid conflicts; that can help.

ckruse · 2023-12-01T10:27:08Z

I can't switch to BoringSSL.

How about adding a second feature flag (e.g. legacy-rsa) for the old implementation and this condition?

#[cfg(not(any(feature = "pure-rust", feature = "legacy-rsa", target_arch = "wasm32", target_arch = "wasm64")))]
pub use self::rsa::*;
#[cfg(any(feature = "pure-rust", feature = "legacy-rsa", target_arch = "wasm32", target_arch = "wasm64"))]
pub use self::rsa_legacy::*;

One could enable the legacy implementation by adding features = ["legacy-rsa"]

jedisct1 · 2023-12-01T10:28:46Z

A second feature flag will not solve the cargo configuration issue: how to automatically enable it when the target is webassembly?

photino · 2023-12-01T10:48:56Z

Could we consider adding features for the JWT algorithms? It will also help reduce the compile time. In our project, we do not use the RSA algorithms at all.

jedisct1 · 2023-12-01T10:57:35Z

Please let's stay on topic.

jedisct1 · 2023-12-01T11:08:23Z

Can you provide a minimal test case that imports both boring-sys and openssl-sys and triggers a linking error? I tried with an empty app importing both dependencies, but that wasn't enough to raise an error.

ckruse · 2023-12-01T12:27:25Z

I've been trying, but I can't figure it out for now. I have to make a deeper look this weekend.

rudi10-1 · 2023-12-03T09:32:22Z

Minimal Cargo.toml:

[package]
name = "openboring"
version = "0.1.0"
edition = "2021"

[dependencies]
reqwest = "0.11.22"
tokio = { version = "1.34", features = ["full"] }

and main.rs:

#[tokio::main]
async fn main() {
    reqwest::get("https://www.rust-lang.org").await.unwrap().text().await.unwrap();
}

That builds just fine. But after doing nothing else than cargo add jwt-simple, cargo buildoutput is like:

  = note: /usr/bin/ld: /home/mii/dev/openboring/target/debug/deps/libopenssl-c7f58d1a78e0653c.rlib(openssl-c7f58d1a78e0653c.openssl.e077e3f1aa129e6-cgu.01.rcgu.o): in function `openssl::ssl::SslRef::peer_certificate':
          /home/mii/.cargo/registry/src/index.crates.io-6f17d22bba15001f/openssl-0.10.60/src/ssl/mod.rs:2569: undefined reference to `SSL_get1_peer_certificate'

jedisct1 · 2023-12-03T14:46:01Z

Builds fine on macOS 🤷‍♂

jedisct1 · 2023-12-03T14:55:06Z

On Linux any of the following changes to cargo.toml makes it build:

reqwest = { version = "0.11.22", default-features = false, features = ["native-tls-vendored"] }

reqwest = { version = "0.11.22", default-features = false, features = ["rustls-tls"] }

sebadob · 2023-12-15T09:38:54Z

Unfortunately, it breaks quite a few of my projects as well.

I am using rust-only whenever I can, which was the reason why I changed from josekit to jwt-simple in the first place.
I am compiling everything with musl target's to get way smaller container images with no dependencies and therefore no attack surface at all.

I am left with one dependency in all my projects, that demands openssl, which is webauthn-rs.
Unfortunately, they don't offer a rust-only solution.

This forces me to use openssl-sys with the vendored feature to make everything work. This was fine for a long time, but this whole thing breaks apart now with the switch to boring in this project.

I would really appreciate it, if jwt-simple could just add a pure-rust or rust-only feature flag or however it would be called. I know that there is currently the Marvin Attack out there which was the reason for the whole switch (I guess), but the RustCrypto project is working on a fix for this and will have a solution in the near future, I guess.

I prepared a minimal "crashing" example repo, but at this moment, I do not know a solution to make it work without forking either jwt-simple or webauthn-rs and making them work together.

The repo can be found here:

https://github.com/sebadob/jwt-simple-openssl-conflict

jedisct1 · 2023-12-15T10:41:21Z

Read above.

The rust implementation is still there.

But I don't know how to automatically enable for WebAssembly or when a cargo flag is given.

Tell me how to do it 😀

jedisct1 · 2023-12-15T10:49:12Z

In order to help solve the symbol conflict between OpenSSL and BoringSSL, what would be useful is a minimal test case that only involves these two crates.

jedisct1 · 2023-12-15T10:51:48Z

I think the BoringSSL issue referenced above would solve it. But someone has to update all the symbols to add the prefix.

This is easy to do, but it still has to be done. I'm sure the maintainers of the boring crate would appreciate it.

sebadob · 2023-12-15T11:05:06Z

Read above.

The rust implementation is still there.

But I don't know how to automatically enable for WebAssembly or when a cargo flag is given.

Tell me how to do it 😀

Yes I know that the old one is still there. That's the good thing. :D
Instead of adding dependencies by target build arch, I would simply put them behind feature flags.
For instance your default could be with boring, and if you want anything else, others need to set enable-default-features = false and specify rust-only or wasm, or whatever is needed there.

I don't know if I have enough time later on, but maybe I could prepare a PR for this.

In order to help solve the symbol conflict between OpenSSL and BoringSSL, what would be useful is a minimal test case > that only involves these two crates.

My small test repo does exactly that. It has only jwt-simple with boring and webauthn-rs with openssl as dependencies.
I guess I can even remove the webauthn-proto dependency.

jedisct1 · 2023-12-15T11:16:15Z

It should compile with the default flags, including webassembly.

So, there's no way in cargo to include a dependency according to the target or a flag?

jedisct1 · 2023-12-15T11:17:44Z

My small test repo does exactly that. It has only jwt-simple with boring and webauthn-rs with openssl as dependencies.

Only with the openssl and boring crates. Not with JWT crates. That would be more useful to help the boring maintainers.

sebadob · 2023-12-15T11:23:16Z

I have never activated feature flags by target arch automatically, I don't know if this is possible.
But what would work most probably, if you keep the current setup and make it possible to opt-out in non-wasm environments only, which could be done by deactivating default features, which would be ignored for wasm.

I can test some things later.

Only with the openssl and boring crates. Not with JWT crates. That would be more useful to help the boring maintainers.

Got it, maybe this is easily changed though.

sebadob · 2023-12-15T11:48:05Z

Platform specific features are an open issue for cargo -> rust-lang/cargo#1197

But the solution from the PR works in my case.
It works with just specifying pure-rust as a feature. This would still pull in the then unsued boring crate. This does not produce any conflicts though, it is just cleaner to disable the default features in that case and not pull this crate at all.

The wasm version should work without any feature specification as well. I would like someone else to test this though, I have not done much with wasm and do not currently have any test setup for this.

jedisct1 · 2023-12-15T12:09:20Z

Having to add a cargo flags just so that the project would compile is not clean. It would just be an awful hack to work around a limitation of Cargo due to its ancient design.

But more importantly, that would be a breaking change for webassembly users. And this is something I'd really like to avoid.

Wouldn't a "build.rs" file work either?

If not, I can see two possible workarounds:

if there's a way to select a dependency feature according to two conditions, a new RSA crate that only imports either the rsa or the boring crate depending to a cargo flag can be written
or we can try to also use boring for webassembly, shipping a precompiled library. That approach works well for now but may break with a future rust version due to the forthcoming webassembly/wasi ABI breaking changes.

sebadob · 2023-12-15T12:11:47Z

You only need to add the flag if you want to opt-out of the current usage of boring ssl.

It is not even a breaking change and could be released as a patch version.
I, on my side, need to add the feature, because of my conflicts with openssl-sys and static compilation to musl targets.

edit:

Just to be clear: The PR does not change anything about the current behavior.
The defaults are exactly the same.

sebadob · 2023-12-15T12:36:48Z

I have set up a wasm project myself now. Tbh, it did not even compile to wasm with the current release version 0.12.1, because getrandom does not work by default for wasm*.unknown-unknown targets.

I made it work and it does indeed compile on wasm (now) without specifying any features at all.

Now it does:

work with wasm with just default features
work locally with default features
if you need to opt-out of boring locally becuase of static openssl-sys setup and musl targets, you can do this with the pure-rust feature

seanpianka · 2023-12-15T19:53:13Z

I'm piping in here without much to add besides that the addition of libc to the dependency chain breaks my build within an alpine-musl build environment (which otherwise relies on rustls for SSL in all other dependencies).

sebadob · 2023-12-15T19:59:24Z

I'm piping in here without much to add besides that the addition of libc to the dependency chain breaks my build within > an alpine-musl build environment (which otherwise relies on rustls for SSL in all other dependencies).

That is the exact same reason as in my case. Any musl target will break as soon as you add any vendored C libraries.
My PR fixes this though. It should work for you, if you change to the PR temporarily with:

jwt-simple = { git = "https://github.com/sebadob/rust-jwt-simple/tree/feature-pure-rust", default-features = false, features = ["pure-rust"] }

Feedback would be nice, if this fixes it for you.

seanpianka · 2023-12-16T06:23:09Z

Thanks for the fork! I applied it using a slightly modified line in my Cargo.toml files and it indeed removed libc from the dependency chain:

jwt-simple = { git = "https://github.com/sebadob/rust-jwt-simple", branch = "feature-pure-rust", default-features = false, features = ["pure-rust"] }

e.g.

-[[package]]
-name = "boring"
-version = "4.2.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8fed6dc629997e8c3c4d1dbd59cf748512b9b0e0fbc509cf9b0e2edca9d24d1a"
-dependencies = [
- "bitflags 2.4.1",
- "boring-sys",
- "foreign-types",
- "libc",
- "once_cell",
-]
-
-[[package]]
-name = "boring-sys"
-version = "4.2.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "91102749c3c99028f344c3bce1dc54b1aac49d52d03c60c75996f81ebdeaa41c"
-dependencies = [
- "bindgen",
- "cmake",
- "fs_extra",
- "fslock",
-]
-

It now builds correctly in my build environment, e.g.

FROM rust:1.71.0-alpine3.18 AS build
RUN apk update \
    && apk add musl-dev openssl-dev make bash htop tree py3-pip lld
ENV RUSTFLAGS="-Clink-arg=-fuse-ld=lld"
RUN rustup target add x86_64-unknown-linux-musl

jedisct1 mentioned this issue Dec 1, 2023

Prefixing BoringSSL symbols cloudflare/boring#197

Open

sebadob mentioned this issue Dec 15, 2023

Feature: pure rust #103

Closed

jedisct1 closed this as completed in a2db6d8 Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The change to BoringSSL breaks builds #102

The change to BoringSSL breaks builds #102

ckruse commented Dec 1, 2023

photino commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

ckruse commented Dec 1, 2023 •

edited

Loading

jedisct1 commented Dec 1, 2023

photino commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

ckruse commented Dec 1, 2023

rudi10-1 commented Dec 3, 2023

jedisct1 commented Dec 3, 2023

jedisct1 commented Dec 3, 2023

sebadob commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

sebadob commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

sebadob commented Dec 15, 2023

sebadob commented Dec 15, 2023 •

edited

Loading

jedisct1 commented Dec 15, 2023

sebadob commented Dec 15, 2023 •

edited

Loading

sebadob commented Dec 15, 2023

seanpianka commented Dec 15, 2023

sebadob commented Dec 15, 2023

seanpianka commented Dec 16, 2023

The change to BoringSSL breaks builds #102

The change to BoringSSL breaks builds #102

Comments

ckruse commented Dec 1, 2023

photino commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

ckruse commented Dec 1, 2023 • edited Loading

jedisct1 commented Dec 1, 2023

photino commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

jedisct1 commented Dec 1, 2023

ckruse commented Dec 1, 2023

rudi10-1 commented Dec 3, 2023

jedisct1 commented Dec 3, 2023

jedisct1 commented Dec 3, 2023

sebadob commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

sebadob commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

jedisct1 commented Dec 15, 2023

sebadob commented Dec 15, 2023

sebadob commented Dec 15, 2023 • edited Loading

jedisct1 commented Dec 15, 2023

sebadob commented Dec 15, 2023 • edited Loading

sebadob commented Dec 15, 2023

seanpianka commented Dec 15, 2023

sebadob commented Dec 15, 2023

seanpianka commented Dec 16, 2023

ckruse commented Dec 1, 2023 •

edited

Loading

sebadob commented Dec 15, 2023 •

edited

Loading

sebadob commented Dec 15, 2023 •

edited

Loading