Skip to content
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.

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

Remove Boring SSL #69

Closed
rjzak opened this issue Jul 26, 2022 · 17 comments · Fixed by #71
Closed

Remove Boring SSL #69

rjzak opened this issue Jul 26, 2022 · 17 comments · Fixed by #71

Comments

@rjzak
Copy link
Contributor

rjzak commented Jul 26, 2022

Remove Boring SSL, only used in implementations/hostcalls/rust/src/signatures/rsa.rs.

Motivation:

  • We want to support wasi-crypto in Enarx, but our Wasm component is compiled as x86_64-unknown-linux-musl, which conflicts with Boring since it's in C++.

In-Progress:

CC: @jedisct1

@jedisct1
Copy link
Member

That would be a dealbreaker for us.

Ignoring security concerns, the pure Rust RSA implementation is very slow. Way too slow for our requirements, and it doesn't even justify using hostcalls vs compiling to WebAssembly.

The RSA crate was originally used in that code, and got eventually replaced with BoringSSL for that reason.

Is C++ incompatible with static builds? That looks like a toolchain issue, a configuration issue, or something to fix in the boring crate rather than a permanent obstacle.

@rjzak
Copy link
Contributor Author

rjzak commented Jul 26, 2022

Could we have a compromise where a feature flag could select RustCrypto over Boring? I'll take a closer look at Boring to see about the compilation issue. But Boring + Rust + musl results in trying to use musl-g++ (not resolved with sudo ln -s /bin/g++ /bin/musl-g++), and the compiler not finding several standard header files.

CC: @npmccallum

@jedisct1
Copy link
Member

jedisct1 commented Jul 26, 2022

The boring crate seems to always try to compile to the host architecture (compiling with cargo-zigbuild build --host=x86_64-unknown-linux-musl still makes it try to compile for ARM on my M1 Mac). But fixing this would make a lot of people happy :)

@rjzak
Copy link
Contributor Author

rjzak commented Jul 26, 2022

Out of curiosity, what would you say was the performance boost by switching to boring?

@tarcieri
Copy link

I’m curious to know as well. We should open a tracking issue on the rsa crate about the performance difference

@jedisct1
Copy link
Member

Benchmark of JWT verification/signature:

Screen Shot 2022-07-26 at 18 39 38

(C@E module is pure WebAssembly)

@npmccallum
Copy link

@jedisct1 Did you build rsa in release mode with all the optimizations on?

@jedisct1
Copy link
Member

Of course, yes.

@jedisct1
Copy link
Member

boring already has some support for cross-compilation, as it can cross-compile for Android and iOS (see get_boringssl_cmake_config in the build.rs file).

@npmccallum
Copy link

@jedisct1 boring is a non-starter for Enarx given our hardware environment. Can we have both implementations behind a feature flag?

@PiotrSikora
Copy link
Contributor

FYI, there is also bssl-sys crate (still under development?) by BoringSSL team. Perhaps that could work?

@npmccallum
Copy link

@PiotrSikora We will not use boring.

@PiotrSikora
Copy link
Contributor

@npmccallum the original comment complains about C++ (which last time I checked, was optional in BoringSSL, and perhaps it being required is an issue with boring crate). I didn't use bssl-sys, but it looks like it's using bindgen, so it should be pure C. Is your issue with using BoringSSL in general or with the C++ bits?

@jedisct1
Copy link
Member

It shouldn't be too hard to support both. So, yes, it looks like a good path forward.

@npmccallum
Copy link

@PiotrSikora We have historically tried boring, openssl, and mbed. Trying to use them in our context causes a lot of problems (not just C/C++). We gave up after months of pain and now use RustCrypto for everything.

@messense
Copy link

I'm working on rust-lang/cmake-rs#158 to improve cross compiling support in cmake-rs, hopefully it will make cross compiling boringssl easier.

@npmccallum
Copy link

The problem as I see it is that wasi-crypto is mixing crypto implementations. This requires everyone to tries to use it to adopt multiple crypto systems. Unfortunately, wasi-crypto has chosen RustCrypto for everything except RSA; but then adopts a completely different crypto system for RSA.

We need to decide on one crypto system for the reference implementation. If others want a boring-backed implementation of wasi-crypto then they should build one. But the reference implementation should not force people to adopt multiple crypto systems. Further, as a reference implementation, wasi-crypto should not chose its crypto system based on criteria like performance. Rather it should choose the crypto system which is the easiest for all parties to get working (that's the purpose of a reference implementation).

Given these criteria, there are only two choices for a Rust-language reference implementation: ring or RustCrypto. Both are pretty trivial to add as dependencies and will build in every situation where Rust builds native code. An argument could be made for both of them. But we should pick one and use that for the reference implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants