Version 0.10.31 and newer requires deprecated unavailable function DH_generate_parameters()

[nospam3089]

First, some essentials about my system:

% uname -srvo; openssl version
SunOS 5.11 omnios-r151036-4a32ffb911 illumos
OpenSSL 1.1.1h  22 Sep 2020

Please consider the following minimal program:

fn main() { openssl::init(); }

When compiled using the following Cargo.toml:

[package]
name = "use_30"
version = "1.0.0"

[dependencies]
openssl = "=0.10.30"

It builds without any problems:

% cargo build
Finished dev [unoptimized + debuginfo] target(s) in 0.02s

Yet, if attempting to use a newer crate version. Such as:

[package]
name = "use_31"
version = "1.0.0"

[dependencies]
openssl = "=0.10.31" # <- Please note change here

Attempts to build fail:

% cargo build
   Compiling use_31 v1.0.0 (/use_31)
error: linking with `gcc` failed: exit code: 1
  |
  = note: "gcc" "-m64" "-std=c99" "-L" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.11cfabo35ubj3n90.rcgu.o" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.2h84ng3nrkhrgu2f.rcgu.o" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.43mqo1s47j9zuiqf.rcgu.o" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.4h25pav9quln26yw.rcgu.o" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.cz3yqdixc20jps7.rcgu.o" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.ko9n82bk8pe0fyy.rcgu.o" "-o" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e" "/use_31/target/debug/deps/use_31-8abfdc4ee477799e.3wmayixdfbl31np0.rcgu.o" "-Wl,-zignore" "-nodefaultlibs" "-L" "/use_31/target/debug/deps" "-L" "/usr/lib/64" "-L" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib" "-Wl,-Bstatic" "/use_31/target/debug/deps/libopenssl-021bc23a5d411abd.rlib" "/use_31/target/debug/deps/libopenssl_sys-9e6168a15d82d352.rlib" "/use_31/target/debug/deps/liblibc-a31a2f2426a4216c.rlib" "/use_31/target/debug/deps/liblazy_static-bfd08e9b30be68c5.rlib" "/use_31/target/debug/deps/libforeign_types-b403bdcd167d48b9.rlib" "/use_31/target/debug/deps/libforeign_types_shared-fb619787edced1b1.rlib" "/use_31/target/debug/deps/libcfg_if-a831f1032deadde6.rlib" "/use_31/target/debug/deps/libbitflags-114fbb9ab937db58.rlib" "-Wl,--start-group" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libstd-7e61f9d70e8c0f40.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libpanic_unwind-7a1712c7fa57eeff.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libminiz_oxide-980809ebc65540a9.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libadler-8fc6e8b97b8c1d5c.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libobject-436b6abcf571c942.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libaddr2line-f35080c580336022.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libgimli-91387bf38b95550f.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/librustc_demangle-85a52887a68f16a4.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libhashbrown-3061d9995be624c0.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/librustc_std_workspace_alloc-c45dc78d84ccaa34.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libunwind-d8dfc0d0c03affb3.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libcfg_if-7508e56b7df26c77.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/liblibc-b62d905525b9ae71.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/liballoc-a3891a8dfe96ae57.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/librustc_std_workspace_core-4df365b4e890064a.rlib" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libcore-0cda113d16499694.rlib" "-Wl,--end-group" "/opt/ooce/rust/lib/rustlib/x86_64-unknown-illumos/lib/libcompiler_builtins-9b2fb916ba38d63e.rlib" "-Wl,-Bdynamic" "-lssl" "-lcrypto" "-lsocket" "-lposix4" "-lpthread" "-lresolv" "-lnsl" "-lumem" "-lgcc_s" "-lc" "-lm" "-lrt" "-lpthread" "-lssp"
  = note: Undefined            first referenced
           symbol                  in file
          DH_generate_parameters              /use_31/target/debug/deps/libopenssl-021bc23a5d411abd.rlib(openssl-021bc23a5d411abd.openssl.1djstp1s-cgu.10.rcgu.o)
          ld: fatal: symbol referencing errors. No output written to /use_31/target/debug/deps/use_31-8abfdc4ee477799e
          collect2: error: ld returned 1 exit status


error: aborting due to previous error

error: could not compile `use_31`

To learn more, run the command again with --verbose.

Attempting to build against 0.10.32 fails equally bad. The commit tagged openssl-sys-v0.9.59 (2020DEC09) mentions the addition of DH_generate_parameters, which we can see is the symbol missing in the compilation error above.

Looking at include/openssl/dh.h we learn that this function is conditionally compiled behind a deprecation guard, as this function was deprecated (2002DEC08) in OpenSSL 0.9.8. A version which was initially released more than a decade and a half ago (2005OCT10).

Quoting Configure on the use of its api option:

One of 0.9.8, 1.0.0, 1.0.1, 1.0.2, 1.1.0, 1.1.1, or 3.0 Define the public APIs as they were for that version including patch releases. If 'no-deprecated' is also given, do not compile support for interfaces deprecated up to and including the specified OpenSSL version.

I believe that OmniOS has had OpenSSL built with --api=1.0.0 since 2017AUG23. Between OmniOS r151026 and OmniOS r151036 that should add up to six stable releases, from which we might assume being enough time to prove shipping systems with such a library configuration seems to be reasonably valid.

Lacking a deeper understanding here, I wonder if anyone might have an idea on what might be the right way to solve this? Could putting access to deprecated stuff behind a feature be the way to go?

[sfackler]

It looks like the Dh::generate_params method should just be updated to use DH_generate_parameters_ex instead of DH_generate_parameters.

[nospam3089]

Not having actually looked at the source code in question, I didn't know if this was a case of willingly exposing the deprecated functions through the Rust API or a bug. Now assuming we can conclude that it is a bug.

It seems @wiomoc introduced Dh::generate_params as recent as at the end of last year, with PR-1353, and might perhaps have it still fresh in mind?

Without fully understanding neither the C or Rust API:s, I presume the preferance is to keep the signature of Dh::generate_params unchanged as is and update openssl-sys to simply export interface the contemporary C api?

Apart from my lack of proper knowledge, I have some practical troubles working on this. Because building rust-openssl freshly cloned fails due to ctest not supporting illumos.

The following diff seems to at least compile under Linux. I have not made any further attempts at determining whether it is correct or not (and thus am a bit reluctant suggest it as a pull request):

diff --git a/openssl-sys/src/dh.rs b/openssl-sys/src/dh.rs
index 75200fee..44b81bba 100644
--- a/openssl-sys/src/dh.rs
+++ b/openssl-sys/src/dh.rs
@@ -4,12 +4,12 @@ extern "C" {
     pub fn DH_new() -> *mut DH;
     pub fn DH_free(dh: *mut DH);
 
-    pub fn DH_generate_parameters(
+    pub fn DH_generate_parameters_ex(
+        dh: *mut DH,
         prime_len: c_int,
         generator: c_int,
-        callback: Option<extern "C" fn(c_int, c_int, *mut c_void)>,
-        cb_arg: *mut c_void,
-    ) -> *mut DH;
+        cb: *mut BN_GENCB,
+    ) -> c_int;
 
     pub fn DH_generate_key(dh: *mut DH) -> c_int;
     pub fn DH_compute_key(key: *mut c_uchar, pub_key: *const BIGNUM, dh: *mut DH) -> c_int;
diff --git a/openssl/src/dh.rs b/openssl/src/dh.rs
index ecb703b5..74a66455 100644
--- a/openssl/src/dh.rs
+++ b/openssl/src/dh.rs
@@ -77,15 +77,18 @@ impl Dh<Params> {
     ///
     /// This corresponds to [`DH_generate_parameters`].
     ///
-    /// [`DH_generate_parameters`]: https://www.openssl.org/docs/man1.1.0/crypto/DH_generate_parameters.html
+    /// [`DH_new`]: https://www.openssl.org/docs/man1.1.0/crypto/DH_new.html
+    /// [`DH_generate_parameters_ex`]: https://www.openssl.org/docs/man1.1.0/crypto/DH_generate_parameters.html
     pub fn generate_params(prime_len: u32, generator: u32) -> Result<Dh<Params>, ErrorStack> {
         unsafe {
-            Ok(Dh::from_ptr(cvt_p(ffi::DH_generate_parameters(
+            let dh = Dh::from_ptr(cvt_p(ffi::DH_new())?);
+            cvt(ffi::DH_generate_parameters_ex(
+                dh.0,
                 prime_len as i32,
                 generator as i32,
-                None,
                 ptr::null_mut(),
-            ))?))
+            ))?;
+            Ok(dh)
         }
     }

Does this at all look like what is needed?

[sfackler]

We can't remove DH_generate_parameters from openssl-sys, but you can add the _ex version next to it and use that instead.

[nospam3089]

Pull request 1431 created.

I've managed to successfully compile on both Linux and illumos with it, but not done any further attempts to verify anything.

Was about to ask about whether there were any test suites which we expected to be run, but got the notification that there are before doing so.

[nospam3089]

Branch updated with whitespace damage fixed and all tests passed successfully.

Please let me know where we go from here.

[nospam3089]

I can see that PR-#1431 was merged. That's great @sfackler, thank you!

One thing which worries me, just slightly, is that unintended usage of deprecated API:s might accidentally happen again. If not in this crate, possibly for other consumers of openssl-sys. Maybe this is a negligible risk, but it might be worth mentioning prior to closing the ticket.

Not until after suggesting to use features for API versioning, I noticed that there used to be something like that. Which was removed after discussion in issue #852. Given that at least one bug related to API versioning has been seen now, maybe it's time to revisit the idea? We know that obsoleted functionality started to become harder to use as recently as in 1.1.0, thus not making it unreasonable to think that tracking deprecation might become more important in 3.0.0 and beyond.

The way I understand the functionality addressed by issue #852, it seems to have been a user accessible way to opt-in to newer functionality (which I believe was later conceptually replaced by build time, automatically determined, osslαβγ features). How about reusing a rethought design and requiring the use of vαβγ features on openssl-sys to enable deprecated functionality? (Leaving all other existing features as is, of course). Maybe even naming these features apiαβγ to avoid name clash confusion.

A very far from complete patch, but hopefully enough to illustrate my thinking, could start as:

diff --git a/openssl-sys/Cargo.toml b/openssl-sys/Cargo.toml
index da8d4b40..6ee0629a 100644
--- a/openssl-sys/Cargo.toml
+++ b/openssl-sys/Cargo.toml
@@ -12,6 +12,7 @@ links = "openssl"
 build = "build/main.rs"
 
 [features]
+v098 = []
 vendored = ['openssl-src']
 
 [dependencies]
diff --git a/openssl-sys/src/dh.rs b/openssl-sys/src/dh.rs
index 3a4553c4..3337dea4 100644
--- a/openssl-sys/src/dh.rs
+++ b/openssl-sys/src/dh.rs
@@ -4,6 +4,8 @@ extern "C" {
     pub fn DH_new() -> *mut DH;
     pub fn DH_free(dh: *mut DH);
 
+    /// Deprecated in OpenSSL 0.9.8 and newer.
+    #[cfg(v098)]
     pub fn DH_generate_parameters(
         prime_len: c_int,
         generator: c_int,

Needless to say, I do not believe myself having thought about this a lot. So the perspective of greater insights would be welcome.

[sfackler]

There are a multitude of flags in OpenSSL to turn off various bits of the APIs, and it is unfortunately not feasible to test against every one of them. However, breakage tends to be pretty rare so the status quo of fixing things as they come up is probably the way to go.

There are very few direct consumers of the bindings in openssl-sys outside of the openssl crate. I don't think anyone will be calling DH_generate_consumers.