OpenSSL: Include SIG and KEM algorithms in verbose #12030

ajbozarth · 2023-10-04T20:04:41Z

Currently the verbose output does not include which algorithms are used for the signature and key exchange when using OpenSSL. Including the algorithms used will enable better debugging when working on using new algorithm implementations. Know what algorithms are used has become more important with the fast growing research into new quantum-safe algorithms.

This implementation includes a build time check for the OpenSSL version to use a new function that will be included in OpenSSL 3.2 that was introduced in openssl/openssl@6866824

Based-on-patch-by: martinschmatz mrt@zurich.ibm.com

Currently the verbose output does not include which algorithms are used for the signature and key exchange when using OpenSSL. Including the algorithms used will enable better debugging when working on using new algorithm implementations. Know what algorithms are used has become more important with the fast growing research into new quantum-safe algorithms. This implementation includes a build time check for the OpenSSL version to use a new function that will be included in OpenSSL 3.2 that was introduced in openssl/openssl@6866824 Based-on-patch-by: martinschmatz <mrt@zurich.ibm.com>

bagder · 2023-10-04T21:00:07Z

vtls/openssl.c: In function ‘ossl_connect_step2’:
vtls/openssl.c:3991:5: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 3991 |     int psigtype_nid;
      |     ^~~
vtls/openssl.c:3994:5: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 3994 |     const char *negotiated_group_name;
      |     ^~~~~
vtls/openssl.c: In function ‘servercert’:
vtls/openssl.c:4272:3: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 4272 |   STACK_OF(X509) *certstack;
      |   ^~~~~~~~
vtls/openssl.c:4278:3: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 4278 |   int num_cert_levels = sk_X509_num(certstack);
      |   ^~~
vtls/openssl.c:4282:3: error: ‘for’ loop initial declarations are only allowed in C99 or C11 mode
 4282 |   for(int cert_level = 0; cert_level < num_cert_levels; cert_level++) {
      |   ^~~
vtls/openssl.c:4282:3: note: use option ‘-std=c99’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
vtls/openssl.c:4296:5: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 4296 |     EVP_PKEY *current_pkey;
      |     ^~~~~~~~
vtls/openssl.c:4298:5: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 4298 |     int key_bits = EVP_PKEY_bits(current_pkey);
      |     ^~~

ajbozarth · 2023-10-04T22:09:15Z

I'll continue working on the build errors, I'm having trouble recreating the build errors locally with the build steps I've been following, my previous commit leveraged my IDE's warnings to locate the issues.

ajbozarth · 2023-10-04T22:35:54Z

Pushed an update I believe will address the build issues I was still seeing. Calling it a day (CST) and will check in on the CI tomorrow. Thank you for the accommodation as a new contributor to curl

lib/vtls/openssl.c

@vszakats

- changed version check as suggested by @vszakats - initialized all vars that can be read prior to set - wrappped entire second code block in a version check for openssl3

ajbozarth · 2023-10-05T19:20:27Z

I just pushed an update that addressed both review suggestion as well as wrapping the certstack code in a version check for OpenSSL3. I added the version check because I realized that about have the functions used in that code block were added in v3 and are not available in 1.1 and forks of it like libressl. I'm not sure that the version check will solve all the implicit declaration of function errors though, I may need to add more version checks.

Again thank you for the reviews and feedback

ajbozarth · 2023-10-05T21:20:32Z

As far as I can tell the remaining CI failures are not caused by my changes. If that assessment is wrong feel free to point me at which failures I should address

bagder · 2023-10-05T21:35:17Z

As far as I can tell the remaining CI failures are not caused by my changes

I agree with this conclusion.

lib/vtls/openssl.c

bagder · 2023-10-09T06:20:59Z

I meant something like this:

From 35ece492255e800536902eedc2629f5fa45ca161 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Mon, 9 Oct 2023 08:19:52 +0200
Subject: [PATCH] proposal

---
 lib/vtls/openssl.c | 106 ++++++++++++++++++++++++---------------------
 1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index b03af921a..3b5994cdb 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -4084,10 +4084,65 @@ static CURLcode ossl_pkp_pin_peer_pubkey(struct Curl_easy *data, X509* cert,
     free(buff1);
 
   return result;
 }
 
+#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
+static void certtypes(struct Curl_easy *data,
+                      struct ossl_ssl_backend_data *backend)
+{
+  STACK_OF(X509) *certstack;
+  long verify_result;
+  int num_cert_levels;
+  int cert_level;
+
+  verify_result = SSL_get_verify_result(backend->handle);
+  if(verify_result != X509_V_OK)
+    certstack = SSL_get_peer_cert_chain(backend->handle);
+  else
+    certstack = SSL_get0_verified_chain(backend->handle);
+  num_cert_levels = sk_X509_num(certstack);
+  OpenSSL_add_all_algorithms();
+  OpenSSL_add_all_digests();
+
+  for(cert_level = 0; cert_level < num_cert_levels; cert_level++) {
+    char cert_algorithm[80] = "";
+    char group_name[80] = "";
+    char group_name_final[80] = "";
+    const X509_ALGOR *palg_cert = NULL;
+    const ASN1_OBJECT *paobj_cert = NULL;
+    X509 *current_cert;
+    EVP_PKEY *current_pkey;
+    int key_bits;
+    int key_sec_bits;
+    int get_group_name;
+
+    current_cert = sk_X509_value(certstack, cert_level);
+
+    X509_get0_signature(NULL, &palg_cert, current_cert);
+    X509_ALGOR_get0(&paobj_cert, NULL, NULL, palg_cert);
+    OBJ_obj2txt(cert_algorithm, sizeof(cert_algorithm), paobj_cert, 0);
+
+    current_pkey = X509_get0_pubkey(current_cert);
+    key_bits = EVP_PKEY_bits(current_pkey);
+    key_sec_bits = EVP_PKEY_get_security_bits(current_pkey);
+    get_group_name = EVP_PKEY_get_group_name(current_pkey, group_name,
+                                             sizeof(group_name), NULL);
+    msnprintf(group_name_final, sizeof(group_name_final), "/%s", group_name);
+
+    infof(data,
+          "  Cert %d: "
+          "Public key %s%s (%d/%d Bits/secBits), signed using %s",
+          cert_level, EVP_PKEY_get0_type_name(current_pkey),
+          get_group_name == 0 ? "" : group_name_final,
+          key_bits, key_sec_bits, cert_algorithm);
+  }
+}
+#else
+#define certtypes(x,y)
+#endif
+
 /*
  * Get the server cert, verify it and show it, etc., only call failf() if the
  * 'strict' argument is TRUE as otherwise all this is for informational
  * purposes only!
  *
@@ -4273,60 +4328,11 @@ static CURLcode servercert(struct Curl_cfilter *cf,
     }
     else
       infof(data, " SSL certificate verify ok.");
   }
 
-#if (OPENSSL_VERSION_NUMBER >= 0x30000000L)
-  {
-    STACK_OF(X509) *certstack;
-    long verify_result;
-    int num_cert_levels;
-    int cert_level;
-
-    verify_result = SSL_get_verify_result(backend->handle);
-    if(verify_result != X509_V_OK)
-      certstack = SSL_get_peer_cert_chain(backend->handle);
-    else
-      certstack = SSL_get0_verified_chain(backend->handle);
-    num_cert_levels = sk_X509_num(certstack);
-    OpenSSL_add_all_algorithms();
-    OpenSSL_add_all_digests();
-
-    for(cert_level = 0; cert_level < num_cert_levels; cert_level++) {
-      char cert_algorithm[80] = "";
-      char group_name[80] = "";
-      char group_name_final[80] = "";
-      const X509_ALGOR *palg_cert = NULL;
-      const ASN1_OBJECT *paobj_cert = NULL;
-      X509 *current_cert;
-      EVP_PKEY *current_pkey;
-      int key_bits;
-      int key_sec_bits;
-      int get_group_name;
-
-      current_cert = sk_X509_value(certstack, cert_level);
-
-      X509_get0_signature(NULL, &palg_cert, current_cert);
-      X509_ALGOR_get0(&paobj_cert, NULL, NULL, palg_cert);
-      OBJ_obj2txt(cert_algorithm, sizeof(cert_algorithm), paobj_cert, 0);
-
-      current_pkey = X509_get0_pubkey(current_cert);
-      key_bits = EVP_PKEY_bits(current_pkey);
-      key_sec_bits = EVP_PKEY_get_security_bits(current_pkey);
-      get_group_name = EVP_PKEY_get_group_name(current_pkey, group_name,
-                                               sizeof(group_name), NULL);
-      msnprintf(group_name_final, sizeof(group_name_final), "/%s", group_name);
-
-      infof(data,
-            "  Certificate level %d: "
-            "Public key type %s%s (%d/%d Bits/secBits), signed using %s",
-            cert_level, EVP_PKEY_get0_type_name(current_pkey),
-            get_group_name == 0 ? "" : group_name_final,
-            key_bits, key_sec_bits, cert_algorithm);
-    }
-  }
-#endif
+  certtypes(data, backend);
 
 #if (OPENSSL_VERSION_NUMBER >= 0x0090808fL) && !defined(OPENSSL_NO_TLSEXT) && \
   !defined(OPENSSL_NO_OCSP)
   if(conn_config->verifystatus) {
     result = verifystatus(cf, data);
-- 
2.42.0

Moved the certstack section of code into a function called `infof_certstack` as it constructs the certsatck then calls `infof` The new function is wrapped in a version check that created an empty macro when not built with OpenSSL 3

ajbozarth · 2023-10-12T23:08:03Z

Sorry for the delay, just pushed the update suggested by @bagder

I also receive some feedback and remarks from @martinschmatz who wrote the original patch I based this PR:

Should there be a discussion with the community about whether or not to include the remaining days of cert validity? So far, I was only interested in the algorithms, but for proactive cert renewal, knowing the remaining days of validity would/could be useful. Or have a general discussion about the info(s) to be included?

ajbozarth · 2023-10-23T14:07:39Z

Is there anything else I need to address or any other topics for us to discuss, since review has been silent for over a week?

bagder · 2023-10-23T22:12:07Z

Thanks!

Don't call `OpenSSL_add_all_algorithms(), `OpenSSL_add_all_digests()`. The caller code is meant for OpenSSL 3, while these two functions were only necessary before OpenSSL 1.1.0. They are missing from OpenSSL 3 if built with option `no-deprecated`, causing a build error. Regression from b6e6d4f curl#12030 Bug: curl#12380 (comment) Closes #xxxxx

- build quictls with `no-deprecated` in CI to have test coverage for this OpenSSL 3 configuration. - don't call `OpenSSL_add_all_algorithms()`, `OpenSSL_add_all_digests()`. The caller code is meant for OpenSSL 3, while these two functions were only necessary before OpenSSL 1.1.0. They are missing from OpenSSL 3 if built with option `no-deprecated`, causing build errors: ``` vtls/openssl.c:4097:3: error: call to undeclared function 'OpenSSL_add_all_algorithms'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] vtls/openssl.c:4098:3: error: call to undeclared function 'OpenSSL_add_all_digests'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] ``` Ref: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48587418?fullLog=true#L7667 Regression from b6e6d4f #12030 Bug: #12380 (comment) Reviewed-by: Alex Bozarth - vquic/curl_ngtcp2: fix using `SSL_get_peer_certificate` with `no-deprecated` quictls 3 builds. Do it by moving an existing solution for this from `vtls/openssl.c` to `vtls/openssl.h` and adjusting caller code. ``` vquic/curl_ngtcp2.c:1950:19: error: implicit declaration of function 'SSL_get_peer_certificate'; did you mean 'SSL_get1_peer_certificate'? [-Wimplicit-function-declaration] ``` Ref: https://github.com/curl/curl/actions/runs/6960723097/job/18940818625#step:24:1178 - curl_ntlm_core: fix `-Wunused-parameter`, `-Wunused-variable` and `-Wunused-function` when trying to build curl with NTLM enabled but without the necessary TLS backend (with DES) support. Closes #12384

Lower the barrier to enable `infof_certstack()` from OpenSSL 3 to OpenSSL 1.x and LibreSSL 3.6.x. Follow-up to b6e6d4f curl#12030 Closes #xxxxx

Lower the barrier to enable `infof_certstack()` from OpenSSL 3 to OpenSSL 1.1.x, and LibreSSL 3.6 or upper. With the caveat, that "group name" and "type name" are missing from the log output with these TLS backends. Follow-up to b6e6d4f #12030 Reviewed-by: Daniel Stenberg Closes #12385

github-actions bot added the TLS label Oct 4, 2023

Address build errors

936b44c

Add includes and elif version check

a4b0d65

vszakats reviewed Oct 5, 2023

View reviewed changes

lib/vtls/openssl.c Outdated Show resolved Hide resolved

planetf1 reviewed Oct 5, 2023

View reviewed changes

lib/vtls/openssl.c Outdated Show resolved Hide resolved

Address review

b6e1eb0

- changed version check as suggested by @vszakats - initialized all vars that can be read prior to set - wrappped entire second code block in a version check for openssl3

Added more version checks and cleaned up code

2dc867e

bagder reviewed Oct 5, 2023

View reviewed changes

lib/vtls/openssl.c Outdated Show resolved Hide resolved

martinschmatz mentioned this pull request Oct 6, 2023

Subtle bug in certificate selection (?) haproxy/haproxy#2300

Closed

move the certstack code into a function

685fa54

Moved the certstack section of code into a function called `infof_certstack` as it constructs the certsatck then calls `infof` The new function is wrapped in a version check that created an empty macro when not built with OpenSSL 3

bagder closed this in b6e6d4f Oct 23, 2023

github-actions bot added the hacktoberfest-accepted label Oct 23, 2023

ajbozarth deleted the verbose branch October 23, 2023 22:14

vszakats mentioned this pull request Nov 22, 2023

curl 8.3 build failing with OPENSSL 3 due to absence of DES functions #12380

Closed

vszakats mentioned this pull request Nov 22, 2023

openssl: fix building with v3 no-deprecated + add CI test #12384

Closed

vszakats mentioned this pull request Nov 23, 2023

openssl: enable infof_certstack for 1.1 and LibreSSL 3.6 #12385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenSSL: Include SIG and KEM algorithms in verbose #12030

OpenSSL: Include SIG and KEM algorithms in verbose #12030

ajbozarth commented Oct 4, 2023

bagder commented Oct 4, 2023

ajbozarth commented Oct 4, 2023

ajbozarth commented Oct 4, 2023

ajbozarth commented Oct 5, 2023

ajbozarth commented Oct 5, 2023

bagder commented Oct 5, 2023

bagder commented Oct 9, 2023

ajbozarth commented Oct 12, 2023

ajbozarth commented Oct 23, 2023

bagder commented Oct 23, 2023

OpenSSL: Include SIG and KEM algorithms in verbose #12030

OpenSSL: Include SIG and KEM algorithms in verbose #12030

Conversation

ajbozarth commented Oct 4, 2023

bagder commented Oct 4, 2023

ajbozarth commented Oct 4, 2023

ajbozarth commented Oct 4, 2023

ajbozarth commented Oct 5, 2023

ajbozarth commented Oct 5, 2023

bagder commented Oct 5, 2023

bagder commented Oct 9, 2023

ajbozarth commented Oct 12, 2023

ajbozarth commented Oct 23, 2023

bagder commented Oct 23, 2023