Build against openssl-sys low-level bindings

[Jakuje]

Description

Linking applications against different bindings to the same library (openssl) is undefined behavior in rust ecosystem.

This is removing the low-level bindings and bindgen dependency, delefating this to the openssl-sys crate providing all the low-level functions.

Unfortunately, it does not expose all the constants and defines, that need to be defined manually.

The direct static linking to libfips is preserved.

Checklist

• Test suite updated
• Rustdoc string were added or updated
• CHANGELOG and/or other documentation added or updated
• This is not a code change

Reviewer's checklist:

• Any issues marked for closing are fully addressed
• There is a test suite reasonably covering new functionality or modifications
• This feature/change has adequate documentation added
• A changelog entry is added if the change is significant
• Code conform to coding style that today cannot yet be enforced via the check style test
• Commits have short titles and sensible text
• Doc string are properly updated

[simo5]

github is too stupid to realize on its own when a comment is just a comment and whwn it is part of a review ... liberating all my "pending" comments

[ngg]

While I was checking the bindings, I found a small issue with the current bindings generator, it's not including function names matching OpenSSL_* like OpenSSL_version() (just the full lowercase and full uppercase prefixes are allowlisted).

[Jakuje]

I tried to build sequoia-openpgp against this, but it pulls older version of openssl-sys (through native-tls, which is missing some of the features that we need) and causes the build failure (as it looks like we can use only one of the versions in the build process).

[ngg]

I tried to build sequoia-openpgp against this, but it pulls older version of openssl-sys (through native-tls, which is missing some of the features that we need) and causes the build failure (as it looks like we can use only one of the versions in the build process).

What commit of sequoia-openpgp did you try? You should use the main branch, it works for me, I just get a few missing ossl::bindings::* errors. I modified openpgp/Cargo.toml to use a local path for ossl, then simply ran cargo test -p sequoia-openpgp --no-default-features --features crypto-openssl

[Jakuje]

I had some older branch, but I was able to provide replacements defines. Now I should have all the remaining defines for sequoia to build (from my branch as well as from main). The only failures I see are the leak_checks

[ngg]

I had some older branch, but I was able to provide replacements defines. Now I should have all the remaining defines for sequoia to build (from my branch as well as from main). The only failures I see are the leak_checks

Great. The leak_checks failures are pre-existing, I have those too.

[Jakuje]

openssl-sys uses a different mechanism to find openssl than ossl did before (OPENSSL_DIR envvar vs pkg-config), pls make sure to match how you find openssl if you decide to use bindgen on top of openssl-sys.

My reading is that openssl-sys is also using pkg-config to find OpenSSL (with some handmade overrides in this file):

https://github.com/rust-openssl/rust-openssl/blob/master/openssl-sys/build/find_normal.rs#L210

I would like to avoid reimplementing this search inside of the ossl crate, which is why I did use the hardcoded list of constants we use rather than pulling bindgen again. I would love to get them into openssl-sys, but I think this is not a critical part.

We have a different semantics to build the OpenSSL for fips build, but I tried not to touch the fips build paths.

Also, my use-case would require using openssl-sys and its "vendored" feature, because my dependency graph contains both sequoia -> ossl path and other crates using openssl crate, and I'll need vendoring support for some platforms. Currently with this PR it seemed working that I define both ossl's "dynamic" feature and openssl-sys's "vendored" feature, pls keep it a supported use case. In that case openssl-sys pulls in the openssl-src crate and uses header files from that, so bindgen should use that too.

This sounds to me like the least problematic solution. If we would like to pass the vendored feature to openssl-sys from ossl, I guess you would need also changes in sequoia to pass it from there, which does not sound to me like sustainable.

I hope I followed up on all questions and concerns. Please, let me know if I missed something or if there are some concerns. Changing to ready for review.

[simo5]

I want to retain the ability to make builds w/o openssl-sys, as we want to be able to pull in a custom configured openssl static build, rather than have to deal with the openssl-src package (which is not on 4.0 yet either).

So I think we should have a openssl-sys feature, and potentially move the actual bindings generation to a ossl-sys subpackage in order to allow to unconflicts the links = "openssl" package key in the toml

[Jakuje]

Seems like it will be more complex to spin up the ossl-sys as it looks like cargo does not support mutually exclusive dependencies on feature level. So the ways out from here are likely:

• rename the links on ossl-sys to something like ossl to make cargo happy (this wont prevent anyone from linking to two different openssl's)
• Just not provide the links (with the very same outcome as its done now).

(unless I miss something)

That said, I would probably prefer to stick with this version instead of creating yet another crate just for the bindings (even though I have the work mostly done) as it really does not justify the complexity added. But dropping the branch here anyway:

https://github.com/Jakuje/kryoptic/tree/ossl-sys

[simo5]

Seems like it will be more complex to spin up the ossl-sys as it looks like cargo does not support mutually exclusive dependencies on feature level. So the ways out from here are likely:

* rename the `links` on ossl-sys to something like ossl to make cargo happy (this wont prevent anyone from linking to two different openssl's)

* Just not provide the `links` (with the very same outcome as its done now).

(unless I miss something)

That said, I would probably prefer to stick with this version instead of creating yet another crate just for the bindings (even though I have the work mostly done) as it really does not justify the complexity added. But dropping the branch here anyway:

https://github.com/Jakuje/kryoptic/tree/ossl-sys

we just make ossl-sys optional at the ossl crate level (just like openssl-sys is) and pull it in when dynamic, static or fips are selected, but not when openssl-sys is selected.

[simo5]

We need to add a check like we have in kryoptic's build.rs to ossl/build.rs:

#[cfg(all(feature = "openssl-sys", feature = "ossl-sys"))]
compile_error!("openssl-sys and ossl-sys are incompatible features as they both cause linking to OpenSSL");

Then change most places where we have [#cfg(not(feature = "openssl-sys"))] to [#cfg(feaute = "ossl-sys")]

[Jakuje]

We need to add a check like we have in kryoptic's build.rs to ossl/build.rs:

#[cfg(all(feature = "openssl-sys", feature = "ossl-sys"))]
compile_error!("openssl-sys and ossl-sys are incompatible features as they both cause linking to OpenSSL");

This is already in ossl/build.rs

Then change most places where we have [#cfg(not(feature = "openssl-sys"))] to [#cfg(feaute = "ossl-sys")]

Will do.

The CI now fails in the cases where we implicitly assume static. I will make it explicit feature and add some more checks to make sure there are not both static and dynamic.

[simo5]

Minor nits only, otherwise LGTM

Can you add a CHANGELOG entry too ?

[ngg]

I'm not sure whether you want to stay compatible with latest release, but if a crate depends on ossl with its dynamic feature like how sequoia does, it won't compile now because dynamic is not compatible with default.
I don't really think it's possible to stay compatible with the current design, the old "default" is closest to the new ossl-sys/static, but the old "default + dynamic" would correspond to ossl-sys without static.
The cleanest solution would probably be releasing ossl v2.0.0 and not even trying to stay compatible and removing the dynamic feature all together.

[simo5]

Kryoptic still builds with ossl-sys dynamically linked, so dynamic is still relevant at the ossl level, otherwise the only way to dynamically link would be against openssl-sys, and I want an escape hatch to continue developemnt w/o having to hardcode local patched copies of openssl-sys when they do not expose stuff I need.

If you think we should not change the default, that is fine, we can retain the default as it was, and have openssl-sys be only an opt-in option

[simo5]

In fact, thinking more about it that is what we should probably do, @Jakuje can we drop "openssl-sys" from default and go back to "dynamic" for kryptic-lib and to "ossl-sys" for ossl ?

[ngg]

EvpPkey::fromdata() is not compatible either with the default feature set, the new default uses openssl-sys which has these constants as i32, and u32: From<i32> is not satisfied, so if it's used with an argument like ossl::bindings::EVP_PKEY_KEYPAIR, then it no longer compiles.

[ngg]

I didn't mean to remove the possibility to use dynamic linking with ossl-sys, I just meant to remove the actual "feature" called dynamic, because it doesn't do anything now, same as the ossl-sys feature

[simo5]

we can easily change the features and keep ossl-sys as is (ossl/dynamic -> oss-sys, ossl -> ossl-sys/static)

but the incompatibility with openssl-sys EVP_PKEY_KEYPAIR is a problem

one way out is to not export all of openssl-sys bindings automatically, but instead manually export allow the compatible ones and override the incompatible ones with the old versions ...

this is getting ugly though :(

[simo5]

I think we should merge this PR with just the minor tweak I requested though, and next week we can decide if we should release a 2.0 version of ossl and make more serious adjustments to the API, there are a few things that may be nice to change after all.

[ngg]

If the default of ossl stays using ossl-sys, then the pkey and similar incompatibilities are not a problem, because it will only cause errors when explicitly switching to openssl-sys.

I think if you remove ossl-sys/static feature but add ossl-sys/dynamic instead, you can have ossl's default to use ossl-sys (meaning static), ossl's dynamic forward to ossl-sys/dynamic, and also having neither sys features set defaulting to use ossl-sys, then it should stay compatible with 1.x versions for all the already existing feature combinations (including no-default-feature).

Then later if you choose to release 2.x, you can clean up the features a bit if you want to.

[simo5]

yeah this is definitely an option, in order to make a quick release, and then we work on improving the API for a 2.0, the goal would be that applications should not need to use bindings at all but only ossl specific wrappers in most cases

[Jakuje]

In fact, thinking more about it that is what we should probably do, @Jakuje can we drop "openssl-sys" from default and go back to "dynamic" for kryptic-lib and to "ossl-sys" for ossl ?

Will do. Makes sense.

EvpPkey::fromdata() is not compatible either with the default feature set, the new default uses openssl-sys which has these constants as i32, and u32: From is not satisfied, so if it's used with an argument like ossl::bindings::EVP_PKEY_KEYPAIR, then it no longer compiles.

Right. This mismatch is pain. I have a simple patch for sequoia to use the new PKeyClass (rather than depending on the constants from bindings::). Reverting the default to dynamic from ossl-sys will keep the existing code working, I think.

[ngg]

I've created a test branch with 2 relevant commits:

• the first commit just updates ossl revision to the current f4a885d commit without any other changes, showing this new version is compatible with the released one for sequoia
• the second commit switches to use openssl-sys

Both looks ok.

[Jakuje]

Thank you for double-checking. I had similar changes locally. Lets wait for (hopefully) final review from Simo to get this merged!

[simo5]

just a nit and a question, but overall LGTM

[simo5]

LGTM

[nwalfield]

Thanks everyone for working on this!