OpenSSL: Include SIG and KEM algorithms in verbose

[ajbozarth]

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]

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]

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]

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

[ajbozarth]

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]

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]

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

I agree with this conclusion.

[bagder]

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

[ajbozarth]

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]

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]

Thanks!