diff mbox series

[Openvpn-devel,3/4] Refactor SSL_CTX_use_CryptoAPI_certificate()

Message ID 20230315013516.1256700-4-selva.nair@gmail.com
State Accepted
Headers show
Series Add some tests for cryptoapi.c functions | expand

Commit Message

Selva Nair March 15, 2023, 1:35 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

- Loading the certificate and key into the provider is split out of
  setting up the SSL context. This allows testing of signing by
  cryptoapi-provider interface without dependence on SSL context
  or link-time wrapping.

Change-Id: I269b94589636425e1ba9bf953047d238fa830376
Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/cryptoapi.c | 63 +++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 22 deletions(-)

Comments

Gert Doering March 16, 2023, 9:53 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Stared-at-code, looks all reasonable.

Tested on local MinGW->W10 build (of cryptoapi_testdriver.exe) and GHA
(to get all 32/64 bit and OpenSSL 1/3 combinations built), everything
passes.

Your patch has been applied to the master and release/2.6 branch.

commit 0ad5f4d6c44daedca00dc399a5f914ac5850caa0 (master)
commit 5c2154ca49a591afd8faa8e535a67b149ddbd354 (release/2.6)
Author: Selva Nair
Date:   Tue Mar 14 21:35:15 2023 -0400

     Refactor SSL_CTX_use_CryptoAPI_certificate()

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230315013516.1256700-4-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26414.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 022f53d4..20b7d985 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -401,11 +401,17 @@  get_cert_name(const CERT_CONTEXT *cc, struct gc_arena *gc)
     return name;
 }
 
-int
-SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+/**
+ * Load certificate matching 'cert_prop' from Windows cert store
+ * into xkey provider and return pointers to X509 cert and private key.
+ * Returns 1 on success, 0 on error.
+ * Caller must free 'cert' and 'privkey' after use.
+ */
+static int
+Load_CryptoAPI_certificate(const char *cert_prop, X509 **cert, EVP_PKEY **privkey)
 {
+
     HCERTSTORE cs;
-    X509 *cert = NULL;
     CAPI_DATA *cd = calloc(1, sizeof(*cd));
     struct gc_arena gc = gc_new();
 
@@ -450,9 +456,9 @@  SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
     }
 
     /* cert_context->pbCertEncoded is the cert X509 DER encoded. */
-    cert = d2i_X509(NULL, (const unsigned char **) &cd->cert_context->pbCertEncoded,
-                    cd->cert_context->cbCertEncoded);
-    if (cert == NULL)
+    *cert = d2i_X509(NULL, (const unsigned char **) &cd->cert_context->pbCertEncoded,
+                     cd->cert_context->cbCertEncoded);
+    if (*cert == NULL)
     {
         msg(M_NONFATAL, "Error in cryptoapicert: X509 certificate decode failed");
         goto err;
@@ -468,28 +474,16 @@  SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
         /* private key may be in a token not available, or incompatible with CNG */
         msg(M_NONFATAL|M_ERRNO, "Error in cryptoapicert: failed to acquire key. Key not present or "
             "is in a legacy token not supported by Windows CNG API");
-        goto err;
-    }
-
-    /* Public key in cert is NULL until we call SSL_CTX_use_certificate(),
-     * so we do it here then...  */
-    if (!SSL_CTX_use_certificate(ssl_ctx, cert))
-    {
+        X509_free(*cert);
         goto err;
     }
 
     /* the public key */
-    EVP_PKEY *pkey = X509_get_pubkey(cert);
+    EVP_PKEY *pkey = X509_get_pubkey(*cert);
     cd->pubkey = pkey; /* will be freed with cd */
 
-    /* SSL_CTX_use_certificate() increased the reference count in 'cert', so
-     * we decrease it here with X509_free(), or it will never be cleaned up. */
-    X509_free(cert);
-    cert = NULL;
-
-    EVP_PKEY *privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
-                                              xkey_cng_sign, (XKEY_PRIVKEY_FREE_fn *) CAPI_DATA_free);
-    SSL_CTX_use_PrivateKey(ssl_ctx, privkey);
+    *privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
+                                     xkey_cng_sign, (XKEY_PRIVKEY_FREE_fn *) CAPI_DATA_free);
     gc_free(&gc);
     return 1; /* do not free cd -- its kept by xkey provider */
 
@@ -498,5 +492,30 @@  err:
     gc_free(&gc);
     return 0;
 }
+
+int
+SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+{
+    X509 *cert = NULL;
+    EVP_PKEY *privkey = NULL;
+    int ret = 0;
+
+    if (!Load_CryptoAPI_certificate(cert_prop, &cert, &privkey))
+    {
+        return ret;
+    }
+    if (SSL_CTX_use_certificate(ssl_ctx, cert)
+        && SSL_CTX_use_PrivateKey(ssl_ctx, privkey))
+    {
+        ret = 1;
+    }
+
+    /* Always free cert and privkey even if retained by ssl_ctx as
+     * they are reference counted */
+    X509_free(cert);
+    EVP_PKEY_free(privkey);
+    return ret;
+}
+
 #endif  /* HAVE_XKEY_PROVIDER */
 #endif                          /* _WIN32 */