sfackler / rust-openssl

OpenSSL bindings for Rust
1.4k stars 749 forks source link

Building with BoringSSL should set ossl110, etc. #1944

Open davidben opened 1 year ago

davidben commented 1 year ago

In C, BoringSSL defines OPENSSL_VERSION_NUMBER to match OpenSSL 1.1.1 because, by default, you're expected to use the code you use for OpenSSL 1.1.1. Occasionally things vary from the target version, so we have OPENSSL_IS_BORINGSSL, but the default is to match 1.1.1.

rust-openssl's initial BoringSSL port wasn't done right. Rather than following this standard pattern, it defaults to giving BoringSSL the code for the very oldest OpenSSL! This has caused a lot of mess over the short period of time this port has existed:

We're still not done. The code below was deprecated years before the port existed, and will break very soon. Also it means rust-openssl loses this fix. https://github.com/sfackler/rust-openssl/blob/master/openssl/src/rsa.rs#L584 https://github.com/sfackler/rust-openssl/blob/master/openssl/src/dsa.rs#L317

I'll upload a PR to do a point fix there, but that is merely fixing a symptom of a structural flaw in the port. The true fix is for rust-openssl to follow the supported pattern in C. The default for OpenSSL derivatives should be to target the OpenSSL version they advertise.

That'll simplify a lot of the cfg(ossl110, boringssl) lines. Though we'll need to clean up some tech debt in the process:

(We also have our own BORINGSSL_API_VERSION define, but that matters less if you only target one BoringSSL. Though @maurer may care about this due to Android.)

CC @maurer @alex @reaperhulk

alex commented 1 year ago

Yes, I agree we should default to treating BoringSSL as its "equivalent" OpenSSL and opt things out as required, instead of the reverse. I had this on my TODO list and then... forgot about it, because I'm an irresponsible person.

davidben commented 1 year ago

Oh we should also resolve the bssl-sys situation. Though I think that's blocked on the Android side, as they haven't yet updated to bindgen 0.65. (@maurer, see b/279198502 internally.)

davidben commented 1 year ago

I'm not sure what way you all want to express it in the build (figure I'll leave that to you), but hacking the cfgs in, I see a few compat functions we'll want on the BoringSSL side. I'll take a pass adding in some of those and we can iterate from there.

davidben commented 1 year ago

Playing with this a bit, the other issue is rust-openssl should still pick up osslconf = "OPENSSL_NO_FOO" in BoringSSL and LibreSSL. Both forks set those to disable features, but rust-openssl is ignoring it.