[Openvpn-devel,1/1] openssl: add engine method for loading the key

Message ID 1509192205.3021.7.camel@HansenPartnership.com
State Superseded
Headers show
Series
  • add engine keys keys
Related show

Commit Message

James Bottomley Oct. 28, 2017, 12:03 p.m.
As well as doing crypto acceleration, engines can also be used to load
key files.  If the engine is set, and the private key loading fails
for bio methods, this patch makes openvpn try to get the engine to
load the key.  If that succeeds, we end up using an engine based key.
This can be used with the openssl tpm engines to make openvpn use a
TPM wrapped key file.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 src/openvpn/crypto_backend.h | 13 ++++++++++++
 src/openvpn/crypto_openssl.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  6 +++++-
 3 files changed, 67 insertions(+), 1 deletion(-)

Comments

Илья Шипицин Oct. 29, 2017, 12:34 p.m. | #1
2017-10-28 17:03 GMT+05:00 James Bottomley <
James.Bottomley@hansenpartnership.com>:

> As well as doing crypto acceleration, engines can also be used to load
> key files.  If the engine is set, and the private key loading fails
> for bio methods, this patch makes openvpn try to get the engine to
> load the key.  If that succeeds, we end up using an engine based key.
> This can be used with the openssl tpm engines to make openvpn use a
> TPM wrapped key file.
>


it fails on mbedtls and openssl-1.1.0

https://travis-ci.org/chipitsine/openvpn/builds/294429659


>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  src/openvpn/crypto_backend.h | 13 ++++++++++++
>  src/openvpn/crypto_openssl.c | 49 ++++++++++++++++++++++++++++++
> ++++++++++++++
>  src/openvpn/ssl_openssl.c    |  6 +++++-
>  3 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 567fd9b2..0b4a9ce9 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -669,4 +669,17 @@ const char *translate_cipher_name_from_openvpn(const
> char *cipher_name);
>   */
>  const char *translate_cipher_name_to_openvpn(const char *cipher_name);
>
> +/**
> + * Load a key file from an engine
> + *
> + * @param file The engine file to load
> + * @param ui   The UI method for the password prompt
> + * @param data The data to pass to the UI method
> + *
> + * @return     The private key if successful or NULL if not
> + */
> +EVP_PKEY *
> +engine_load_key(const char *file, SSL_CTX *ctx);
> +
> +
>  #endif /* CRYPTO_BACKEND_H_ */
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 0134e55d..ee16a496 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -969,4 +969,53 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
>      HMAC_Final(ctx, dst, &in_hmac_len);
>  }
>
> +static int
> +ui_read(UI *ui, UI_STRING *uis)
> +{
> +    SSL_CTX *ctx = UI_get0_user_data(ui);
> +
> +    if (UI_get_string_type(uis) == UIT_PROMPT) {
> +        pem_password_cb *cb = SSL_CTX_get_default_passwd_cb(ctx);
> +        void *d = SSL_CTX_get_default_passwd_cb_userdata(ctx);
> +        char password[64];
> +
> +        cb(password, sizeof(password), 0, d);
> +        UI_set_result(ui, uis, password);
> +
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +EVP_PKEY *
> +engine_load_key(const char *file, SSL_CTX *ctx)
> +{
> +    UI_METHOD *ui;
> +    EVP_PKEY *pkey;
> +
> +    if (!engine_persist)
> +        return NULL;
> +
> +    ui = UI_create_method("openvpn");
> +
> +    if (!ui)
> +        return NULL;
> +
> +    UI_method_set_reader(ui, ui_read);
> +
> +    ERR_clear_error();         /* BIO read failure */
> +    if (!ENGINE_init(engine_persist)) {
> +       ERR_print_errors_fp(stderr);
> +       pkey = NULL;
> +       goto out;
> +    }
> +    pkey = ENGINE_load_private_key(engine_persist, file, ui, ctx);
> +    ENGINE_finish(engine_persist);
> +    if (!pkey)
> +       ERR_print_errors_fp(stderr);
> + out:
> +    UI_destroy_method(ui);
> +    return pkey;
> +}
> +
>  #endif /* ENABLE_CRYPTO && ENABLE_CRYPTO_OPENSSL */
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 92a662b5..52e9a869 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -839,7 +839,11 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx,
> const char *priv_key_file,
>                                     SSL_CTX_get_default_passwd_cb_
> userdata(ctx->ctx));
>      if (!pkey)
>      {
> -        goto end;
> +        pkey = engine_load_key(priv_key_file, ctx->ctx);
> +        if (!pkey)
> +        {
> +            goto end;
> +        }
>      }
>
>      if (!SSL_CTX_use_PrivateKey(ssl_ctx, pkey))
> --
> 2.12.3
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2017-10-28 17:03 GMT+05:00 James Bottomley <span dir="ltr">&lt;<a href="mailto:James.Bottomley@hansenpartnership.com" target="_blank">James.Bottomley@hansenpartnership.com</a>&gt;</span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">As well as doing crypto acceleration, engines can also be used to load<br>
key files.  If the engine is set, and the private key loading fails<br>
for bio methods, this patch makes openvpn try to get the engine to<br>
load the key.  If that succeeds, we end up using an engine based key.<br>
This can be used with the openssl tpm engines to make openvpn use a<br>
TPM wrapped key file.<br></blockquote><div><br><br></div><div>it fails on mbedtls and openssl-1.1.0<br><br><a href="https://travis-ci.org/chipitsine/openvpn/builds/294429659">https://travis-ci.org/chipitsine/openvpn/builds/294429659</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Signed-off-by: James Bottomley &lt;James.Bottomley@<wbr>HansenPartnership.com&gt;<br>
---<br>
 src/openvpn/crypto_backend.h | 13 ++++++++++++<br>
 src/openvpn/crypto_openssl.c | 49 ++++++++++++++++++++++++++++++<wbr>++++++++++++++<br>
 src/openvpn/ssl_openssl.c    |  6 +++++-<br>
 3 files changed, 67 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h<br>
index 567fd9b2..0b4a9ce9 100644<br>
--- a/src/openvpn/crypto_backend.h<br>
+++ b/src/openvpn/crypto_backend.h<br>
@@ -669,4 +669,17 @@ const char *translate_cipher_name_from_<wbr>openvpn(const char *cipher_name);<br>
  */<br>
 const char *translate_cipher_name_to_<wbr>openvpn(const char *cipher_name);<br>
<br>
+/**<br>
+ * Load a key file from an engine<br>
+ *<br>
+ * @param file The engine file to load<br>
+ * @param ui   The UI method for the password prompt<br>
+ * @param data The data to pass to the UI method<br>
+ *<br>
+ * @return     The private key if successful or NULL if not<br>
+ */<br>
+EVP_PKEY *<br>
+engine_load_key(const char *file, SSL_CTX *ctx);<br>
+<br>
+<br>
 #endif /* CRYPTO_BACKEND_H_ */<br>
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br>
index 0134e55d..ee16a496 100644<br>
--- a/src/openvpn/crypto_openssl.c<br>
+++ b/src/openvpn/crypto_openssl.c<br>
@@ -969,4 +969,53 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)<br>
     HMAC_Final(ctx, dst, &amp;in_hmac_len);<br>
 }<br>
<br>
+static int<br>
+ui_read(UI *ui, UI_STRING *uis)<br>
+{<br>
+    SSL_CTX *ctx = UI_get0_user_data(ui);<br>
+<br>
+    if (UI_get_string_type(uis) == UIT_PROMPT) {<br>
+        pem_password_cb *cb = SSL_CTX_get_default_passwd_cb(<wbr>ctx);<br>
+        void *d = SSL_CTX_get_default_passwd_cb_<wbr>userdata(ctx);<br>
+        char password[64];<br>
+<br>
+        cb(password, sizeof(password), 0, d);<br>
+        UI_set_result(ui, uis, password);<br>
+<br>
+        return 1;<br>
+    }<br>
+    return 0;<br>
+}<br>
+<br>
+EVP_PKEY *<br>
+engine_load_key(const char *file, SSL_CTX *ctx)<br>
+{<br>
+    UI_METHOD *ui;<br>
+    EVP_PKEY *pkey;<br>
+<br>
+    if (!engine_persist)<br>
+        return NULL;<br>
+<br>
+    ui = UI_create_method(&quot;openvpn&quot;);<br>
+<br>
+    if (!ui)<br>
+        return NULL;<br>
+<br>
+    UI_method_set_reader(ui, ui_read);<br>
+<br>
+    ERR_clear_error();         /* BIO read failure */<br>
+    if (!ENGINE_init(engine_persist)) {<br>
+       ERR_print_errors_fp(stderr);<br>
+       pkey = NULL;<br>
+       goto out;<br>
+    }<br>
+    pkey = ENGINE_load_private_key(<wbr>engine_persist, file, ui, ctx);<br>
+    ENGINE_finish(engine_persist);<br>
+    if (!pkey)<br>
+       ERR_print_errors_fp(stderr);<br>
+ out:<br>
+    UI_destroy_method(ui);<br>
+    return pkey;<br>
+}<br>
+<br>
 #endif /* ENABLE_CRYPTO &amp;&amp; ENABLE_CRYPTO_OPENSSL */<br>
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c<br>
index 92a662b5..52e9a869 100644<br>
--- a/src/openvpn/ssl_openssl.c<br>
+++ b/src/openvpn/ssl_openssl.c<br>
@@ -839,7 +839,11 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,<br>
                                    SSL_CTX_get_default_passwd_cb_<wbr>userdata(ctx-&gt;ctx));<br>
     if (!pkey)<br>
     {<br>
-        goto end;<br>
+        pkey = engine_load_key(priv_key_file, ctx-&gt;ctx);<br>
+        if (!pkey)<br>
+        {<br>
+            goto end;<br>
+        }<br>
     }<br>
<br>
     if (!SSL_CTX_use_PrivateKey(ssl_<wbr>ctx, pkey))<br>
<span class="gmail-HOEnZb"><font color="#888888">--<br>
2.12.3<br>
<br>
------------------------------<wbr>------------------------------<wbr>------------------<br>
Check out the vibrant tech community on one of the world&#39;s most<br>
engaging tech sites, Slashdot.org! <a href="http://sdm.link/slashdot" rel="noreferrer" target="_blank">http://sdm.link/slashdot</a><br>
______________________________<wbr>_________________<br>
Openvpn-devel mailing list<br>
<a href="mailto:Openvpn-devel@lists.sourceforge.net">Openvpn-devel@lists.<wbr>sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/<wbr>lists/listinfo/openvpn-devel</a><br>
</font></span></blockquote></div><br></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
James Bottomley Oct. 29, 2017, 3:07 p.m. | #2
On Sun, 2017-10-29 at 17:34 +0500, Илья Шипицин wrote:
> 2017-10-28 17:03 GMT+05:00 James Bottomley <
> James.Bottomley@hansenpartnership.com>:
> 
> > 
> > As well as doing crypto acceleration, engines can also be used to
> > load
> > key files.  If the engine is set, and the private key loading fails
> > for bio methods, this patch makes openvpn try to get the engine to
> > load the key.  If that succeeds, we end up using an engine based
> > key.
> > This can be used with the openssl tpm engines to make openvpn use a
> > TPM wrapped key file.
> > 
> 
> 
> it fails on mbedtls and openssl-1.1.0
> 
> https://travis-ci.org/chipitsine/openvpn/builds/294429659

It looks like it needs better config guarding; incremental attached
below.

However, it exposes an openvpn problem: engines aren't built with
openssl-1.1 because the configure.ac check for ENGINE_cleanup doesn't
find the function (it became a #define).  I'll see if I can fix that.

The mbedtls one looks like the function def needs to be in
crypto_openssl.h; I've moved it but can't compile check

James

---

diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 0b4a9ce9..cc8f138f 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -669,17 +669,5 @@ const char *translate_cipher_name_from_openvpn(const char *cipher_name);
  */
 const char *translate_cipher_name_to_openvpn(const char *cipher_name);
 
-/**
- * Load a key file from an engine
- *
- * @param file	The engine file to load
- * @param ui	The UI method for the password prompt
- * @param data	The data to pass to the UI method
- *
- * @return	The private key if successful or NULL if not
- */
-EVP_PKEY *
-engine_load_key(const char *file, SSL_CTX *ctx);
-
 
 #endif /* CRYPTO_BACKEND_H_ */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index ee16a496..1fcb80a6 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -969,6 +969,7 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
     HMAC_Final(ctx, dst, &in_hmac_len);
 }
 
+#ifdef HAVE_OPENSSL_ENGINE
 static int
 ui_read(UI *ui, UI_STRING *uis)
 {
@@ -986,10 +987,12 @@ ui_read(UI *ui, UI_STRING *uis)
     }
     return 0;
 }
+#endif
 
 EVP_PKEY *
 engine_load_key(const char *file, SSL_CTX *ctx)
 {
+#ifdef HAVE_OPENSSL_ENGINE
     UI_METHOD *ui;
     EVP_PKEY *pkey;
 
@@ -1016,6 +1019,9 @@ engine_load_key(const char *file, SSL_CTX *ctx)
  out:
     UI_destroy_method(ui);
     return pkey;
+#else
+    return NULL;
+#endif
 }
 
 #endif /* ENABLE_CRYPTO && ENABLE_CRYPTO_OPENSSL */
diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h
index 60a28123..759dc927 100644
--- a/src/openvpn/crypto_openssl.h
+++ b/src/openvpn/crypto_openssl.h
@@ -101,5 +101,17 @@ void crypto_print_openssl_errors(const unsigned int flags);
         msg((flags), __VA_ARGS__); \
     } while (false)
 
+/**
+ * Load a key file from an engine
+ *
+ * @param file	The engine file to load
+ * @param ui	The UI method for the password prompt
+ * @param data	The data to pass to the UI method
+ *
+ * @return	The private key if successful or NULL if not
+ */
+EVP_PKEY *
+engine_load_key(const char *file, SSL_CTX *ctx);
+
 
 #endif /* CRYPTO_OPENSSL_H_ */

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli Oct. 29, 2017, 3:15 p.m. | #3
James,

could you please resend a full patch, so to have a better overview of
the whole change?

Thanks!

On 29/10/17 23:07, James Bottomley wrote:
> On Sun, 2017-10-29 at 17:34 +0500, Илья Шипицин wrote:
>> 2017-10-28 17:03 GMT+05:00 James Bottomley <
>> James.Bottomley@hansenpartnership.com>:
>>
>>>
>>> As well as doing crypto acceleration, engines can also be used to
>>> load
>>> key files.  If the engine is set, and the private key loading fails
>>> for bio methods, this patch makes openvpn try to get the engine to
>>> load the key.  If that succeeds, we end up using an engine based
>>> key.
>>> This can be used with the openssl tpm engines to make openvpn use a
>>> TPM wrapped key file.
>>>
>>
>>
>> it fails on mbedtls and openssl-1.1.0
>>
>> https://travis-ci.org/chipitsine/openvpn/builds/294429659
> 
> It looks like it needs better config guarding; incremental attached
> below.
> 
> However, it exposes an openvpn problem: engines aren't built with
> openssl-1.1 because the configure.ac check for ENGINE_cleanup doesn't
> find the function (it became a #define).  I'll see if I can fix that.
> 
> The mbedtls one looks like the function def needs to be in
> crypto_openssl.h; I've moved it but can't compile check
> 
> James
> 
> ---
> 
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 0b4a9ce9..cc8f138f 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -669,17 +669,5 @@ const char *translate_cipher_name_from_openvpn(const char *cipher_name);
>   */
>  const char *translate_cipher_name_to_openvpn(const char *cipher_name);
>  
> -/**
> - * Load a key file from an engine
> - *
> - * @param file	The engine file to load
> - * @param ui	The UI method for the password prompt
> - * @param data	The data to pass to the UI method
> - *
> - * @return	The private key if successful or NULL if not
> - */
> -EVP_PKEY *
> -engine_load_key(const char *file, SSL_CTX *ctx);
> -
>  
>  #endif /* CRYPTO_BACKEND_H_ */
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index ee16a496..1fcb80a6 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -969,6 +969,7 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
>      HMAC_Final(ctx, dst, &in_hmac_len);
>  }
>  
> +#ifdef HAVE_OPENSSL_ENGINE
>  static int
>  ui_read(UI *ui, UI_STRING *uis)
>  {
> @@ -986,10 +987,12 @@ ui_read(UI *ui, UI_STRING *uis)
>      }
>      return 0;
>  }
> +#endif
>  
>  EVP_PKEY *
>  engine_load_key(const char *file, SSL_CTX *ctx)
>  {
> +#ifdef HAVE_OPENSSL_ENGINE
>      UI_METHOD *ui;
>      EVP_PKEY *pkey;
>  
> @@ -1016,6 +1019,9 @@ engine_load_key(const char *file, SSL_CTX *ctx)
>   out:
>      UI_destroy_method(ui);
>      return pkey;
> +#else
> +    return NULL;
> +#endif
>  }
>  
>  #endif /* ENABLE_CRYPTO && ENABLE_CRYPTO_OPENSSL */
> diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h
> index 60a28123..759dc927 100644
> --- a/src/openvpn/crypto_openssl.h
> +++ b/src/openvpn/crypto_openssl.h
> @@ -101,5 +101,17 @@ void crypto_print_openssl_errors(const unsigned int flags);
>          msg((flags), __VA_ARGS__); \
>      } while (false)
>  
> +/**
> + * Load a key file from an engine
> + *
> + * @param file	The engine file to load
> + * @param ui	The UI method for the password prompt
> + * @param data	The data to pass to the UI method
> + *
> + * @return	The private key if successful or NULL if not
> + */
> +EVP_PKEY *
> +engine_load_key(const char *file, SSL_CTX *ctx);
> +
>  
>  #endif /* CRYPTO_OPENSSL_H_ */
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
James Bottomley Oct. 29, 2017, 3:57 p.m. | #4
On Sun, 2017-10-29 at 23:15 +0800, Antonio Quartulli wrote:
> James,
> 
> could you please resend a full patch, so to have a better overview of
> the whole change?

Sure thing.  It's below.

James

---

From d55d6f50cd156ac8e5cdead1b5c03569885158f6 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Fri, 27 Oct 2017 10:21:14 +0200
Subject: [PATCH] openssl: add engine method for loading the key

As well as doing crypto acceleration, engines can also be used to load
key files.  If the engine is set, and the private key loading fails
for bio methods, this patch makes openvpn try to get the engine to
load the key.  If that succeeds, we end up using an engine based key.
This can be used with the openssl tpm engines to make openvpn use a
TPM wrapped key file.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 src/openvpn/crypto_openssl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/crypto_openssl.h | 12 ++++++++++
 src/openvpn/ssl_openssl.c    |  6 ++++-
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 0134e55d..1fcb80a6 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -969,4 +969,59 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
     HMAC_Final(ctx, dst, &in_hmac_len);
 }
 
+#ifdef HAVE_OPENSSL_ENGINE
+static int
+ui_read(UI *ui, UI_STRING *uis)
+{
+    SSL_CTX *ctx = UI_get0_user_data(ui);
+
+    if (UI_get_string_type(uis) == UIT_PROMPT) {
+        pem_password_cb *cb = SSL_CTX_get_default_passwd_cb(ctx);
+        void *d = SSL_CTX_get_default_passwd_cb_userdata(ctx);
+        char password[64];
+
+        cb(password, sizeof(password), 0, d);
+        UI_set_result(ui, uis, password);
+
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+EVP_PKEY *
+engine_load_key(const char *file, SSL_CTX *ctx)
+{
+#ifdef HAVE_OPENSSL_ENGINE
+    UI_METHOD *ui;
+    EVP_PKEY *pkey;
+
+    if (!engine_persist)
+        return NULL;
+
+    ui = UI_create_method("openvpn");
+
+    if (!ui)
+        return NULL;
+
+    UI_method_set_reader(ui, ui_read);
+
+    ERR_clear_error();		/* BIO read failure */
+    if (!ENGINE_init(engine_persist)) {
+	ERR_print_errors_fp(stderr);
+	pkey = NULL;
+	goto out;
+    }
+    pkey = ENGINE_load_private_key(engine_persist, file, ui, ctx);
+    ENGINE_finish(engine_persist);
+    if (!pkey)
+	ERR_print_errors_fp(stderr);
+ out:
+    UI_destroy_method(ui);
+    return pkey;
+#else
+    return NULL;
+#endif
+}
+
 #endif /* ENABLE_CRYPTO && ENABLE_CRYPTO_OPENSSL */
diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h
index 60a28123..759dc927 100644
--- a/src/openvpn/crypto_openssl.h
+++ b/src/openvpn/crypto_openssl.h
@@ -101,5 +101,17 @@ void crypto_print_openssl_errors(const unsigned int flags);
         msg((flags), __VA_ARGS__); \
     } while (false)
 
+/**
+ * Load a key file from an engine
+ *
+ * @param file	The engine file to load
+ * @param ui	The UI method for the password prompt
+ * @param data	The data to pass to the UI method
+ *
+ * @return	The private key if successful or NULL if not
+ */
+EVP_PKEY *
+engine_load_key(const char *file, SSL_CTX *ctx);
+
 
 #endif /* CRYPTO_OPENSSL_H_ */
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 92a662b5..52e9a869 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -839,7 +839,11 @@ tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
                                    SSL_CTX_get_default_passwd_cb_userdata(ctx->ctx));
     if (!pkey)
     {
-        goto end;
+        pkey = engine_load_key(priv_key_file, ctx->ctx);
+        if (!pkey)
+        {
+            goto end;
+        }
     }
 
     if (!SSL_CTX_use_PrivateKey(ssl_ctx, pkey))
Steffan Karger Nov. 1, 2017, 6:24 p.m. | #5
Hi,

On 29-10-17 16:57, James Bottomley wrote:
> On Sun, 2017-10-29 at 23:15 +0800, Antonio Quartulli wrote:
>> James,
>>
>> could you please resend a full patch, so to have a better overview of
>> the whole change?
> 
> Sure thing.  It's below.

Feature makes sense, so feature-ACK.  An early question though:

> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -969,4 +969,59 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
>      HMAC_Final(ctx, dst, &in_hmac_len);
>  }
>  
> +#ifdef HAVE_OPENSSL_ENGINE
> +static int
> +ui_read(UI *ui, UI_STRING *uis)
> +{
> +    SSL_CTX *ctx = UI_get0_user_data(ui);
> +
> +    if (UI_get_string_type(uis) == UIT_PROMPT) {
> +        pem_password_cb *cb = SSL_CTX_get_default_passwd_cb(ctx);
> +        void *d = SSL_CTX_get_default_passwd_cb_userdata(ctx);
> +        char password[64];
> +
> +        cb(password, sizeof(password), 0, d);
> +        UI_set_result(ui, uis, password);
> +
> +        return 1;
> +    }
> +    return 0;
> +}
> +#endif

This looks like it should use our user query wrappers from (e.g.)
console.h.  David, you're the expert here, what should James use to
query for passwords?

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Nov. 4, 2017, 5:58 p.m. | #6
Hi,

On Wed, Nov 01, 2017 at 07:24:02PM +0100, Steffan Karger wrote:
> This looks like it should use our user query wrappers from (e.g.)
> console.h.  David, you're the expert here, what should James use to
> query for passwords?

The mechanics are "query_user_...()", most conveniently

/**
 * A plain "make Gert happy" wrapper.  Same arguments as @query_user_add
 */
static inline bool
query_user_SINGLE(char *prompt, size_t prompt_len,
                  char *resp, size_t resp_len,
                  bool echo)

(console.h)

... most notably this is important because it can use "other mechanisms"
if console input is not available, for example, systemd querying.

gert
Selva Nair Nov. 4, 2017, 7:34 p.m. | #7
On Sat, Nov 4, 2017 at 1:58 PM, Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Wed, Nov 01, 2017 at 07:24:02PM +0100, Steffan Karger wrote:
> > This looks like it should use our user query wrappers from (e.g.)
> > console.h.  David, you're the expert here, what should James use to
> > query for passwords?
>
> The mechanics are "query_user_...()", most conveniently
>
> /**
>  * A plain "make Gert happy" wrapper.  Same arguments as @query_user_add
>  */
> static inline bool
> query_user_SINGLE(char *prompt, size_t prompt_len,
>                   char *resp, size_t resp_len,
>                   bool echo)
>
> (console.h)
>
> ... most notably this is important because it can use "other mechanisms"
> if console input is not available, for example, systemd querying.
>

Shouldn't this  happen automatically in this particular case as the patch
uses
SSL_CTX_get_default_passwd_cb_userdata() which would result in openssl using
to the password callback previously set in ssl_openssl.c ? And that
callback is
get_userpass() which should know whether to query the management, console or
something else.

Selva
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Nov 4, 2017 at 1:58 PM, Gert Doering <span dir="ltr">&lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<span class="gmail-"><br>
On Wed, Nov 01, 2017 at 07:24:02PM +0100, Steffan Karger wrote:<br>
&gt; This looks like it should use our user query wrappers from (e.g.)<br>
&gt; console.h.  David, you&#39;re the expert here, what should James use to<br>
&gt; query for passwords?<br>
<br>
</span>The mechanics are &quot;query_user_...()&quot;, most conveniently<br>
<br>
/**<br>
 * A plain &quot;make Gert happy&quot; wrapper.  Same arguments as @query_user_add<br>
 */<br>
static inline bool<br>
query_user_SINGLE(char *prompt, size_t prompt_len,<br>
                  char *resp, size_t resp_len,<br>
                  bool echo)<br>
<br>
(console.h)<br>
<br>
... most notably this is important because it can use &quot;other mechanisms&quot;<br>
if console input is not available, for example, systemd querying.<br></blockquote><div><br></div><div>Shouldn&#39;t this  happen automatically in this particular case as the patch uses</div><div>SSL_CTX_get_default_passwd_cb_userdata() which would result in openssl using</div><div>to the password callback previously set in ssl_openssl.c ? And that callback is</div><div>get_userpass() which should know whether to query the management, console or</div><div>something else.</div><div><br></div><div>Selva</div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
David Sommerseth Nov. 8, 2017, 12:27 p.m. | #8
On 04/11/17 20:34, Selva wrote:
> 
> 
> On Sat, Nov 4, 2017 at 1:58 PM, Gert Doering <gert@greenie.muc.de
> <mailto:gert@greenie.muc.de>> wrote:
> 
>     Hi,
> 
>     On Wed, Nov 01, 2017 at 07:24:02PM +0100, Steffan Karger wrote:
>     > This looks like it should use our user query wrappers from (e.g.)
>     > console.h.  David, you're the expert here, what should James use to
>     > query for passwords?
> 
>     The mechanics are "query_user_...()", most conveniently
> 
>     /**
>      * A plain "make Gert happy" wrapper.  Same arguments as @query_user_add
>      */
>     static inline bool
>     query_user_SINGLE(char *prompt, size_t prompt_len,
>                       char *resp, size_t resp_len,
>                       bool echo)
> 
>     (console.h)
> 
>     ... most notably this is important because it can use "other mechanisms"
>     if console input is not available, for example, systemd querying.
> 
> 
> Shouldn't this  happen automatically in this particular case as the
> patch uses
> SSL_CTX_get_default_passwd_cb_userdata() which would result in openssl using
> to the password callback previously set in ssl_openssl.c ? And that
> callback is
> get_userpass() which should know whether to query the management, console or
> something else.

All the hoops, loops and twists we take to retrieve a simple password
from a user ....

TL;DR: Yes, Selva is correct.

From the OpenSSL docs:

   SSL_CTX_get_default_passwd_cb() returns a function pointer to the
   password callback currently set in ctx.  If no callback was
   explicitly set, the NULL pointer is returned.

   SSL_CTX_get_default_passwd_cb_userdata() returns a pointer to the
   userdata currently set in ctx.   If no userdata was explicitly set,
   the NULL pointer is returned"

   (This is set using SSL_CTX_set_default_password_cb_userdata())

In tls_ctx_set_options() [ssl_openssl.c:267], we call:

   SSL_CTX_set_default_passwd_cb(ctx->ctx, pem_password_callback);

The pem_password_callback() again calls pem_password_setup()
[ssl.c:374-396] and the latter one calls get_user_pass() [misc.h:244]
which calls get_user_pass_cr() [misc.c:869].  This function will call
query_user_SINGLE() or using the management interface if that's what the
OpenVPN process have been told to use.

So when James' patch calls SSL_CTX_get_default_passwd_cb(ctx), a
function pointer to pem_password_callback() is returned, which then is
called with the cb() call a few lines further down.

I don't see us using the userdata part of this call chain (I don't see
traces of SSL_set_default_passwd_cb_userdata() in our code).  But having
the support for it makes no harm.  Until we start using it, it will be NULL.

And when seeing this part of the patch.  It makes me wonder if similar
approaches should be adopted a few other places too.  But that's a
completely different discussion.

I need to spend a bit more time to fully grasp the UI get/set calls and
the related implementation.  But what is done in regards to password
retrieving in ui_read() makes sense to me.
James Bottomley Nov. 8, 2017, 4:39 p.m. | #9
On Wed, 2017-11-08 at 13:27 +0100, David Sommerseth wrote:
> I need to spend a bit more time to fully grasp the UI get/set calls
> and the related implementation.  But what is done in regards to
> password retrieving in ui_read() makes sense to me.

How to use UI methods is (unsurprisingly) badly documented in openssl.
I picked up my knowledge of it by giving up on the docs and reading the
source code. Basically it's a huge overkill interface for reading and
verifying a passphrase.  There are about six overrideable methods:
opener, closer, reader, writer, flusher and prompt constructor.  The
interface seems to be designed to give control of all aspects of user
interaction, from passwords, inputs and error outputs.  However, it
only ever seems to be used for passwords.

The only thing I need from it is the reader method (that's what takes
input from the user) and of the five types
UIT_PROMPT/VERIFY/BOOLEAN/INPUT/ERROR I only need UIT_PROMPT because
that's asking the user for input, which duplicates what the PEM
password prompt does.

James
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 567fd9b2..0b4a9ce9 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -669,4 +669,17 @@  const char *translate_cipher_name_from_openvpn(const char *cipher_name);
  */
 const char *translate_cipher_name_to_openvpn(const char *cipher_name);
 
+/**
+ * Load a key file from an engine
+ *
+ * @param file	The engine file to load
+ * @param ui	The UI method for the password prompt
+ * @param data	The data to pass to the UI method
+ *
+ * @return	The private key if successful or NULL if not
+ */
+EVP_PKEY *
+engine_load_key(const char *file, SSL_CTX *ctx);
+
+
 #endif /* CRYPTO_BACKEND_H_ */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 0134e55d..ee16a496 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -969,4 +969,53 @@  hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
     HMAC_Final(ctx, dst, &in_hmac_len);
 }
 
+static int
+ui_read(UI *ui, UI_STRING *uis)
+{
+    SSL_CTX *ctx = UI_get0_user_data(ui);
+
+    if (UI_get_string_type(uis) == UIT_PROMPT) {
+        pem_password_cb *cb = SSL_CTX_get_default_passwd_cb(ctx);
+        void *d = SSL_CTX_get_default_passwd_cb_userdata(ctx);
+        char password[64];
+
+        cb(password, sizeof(password), 0, d);
+        UI_set_result(ui, uis, password);
+
+        return 1;
+    }
+    return 0;
+}
+
+EVP_PKEY *
+engine_load_key(const char *file, SSL_CTX *ctx)
+{
+    UI_METHOD *ui;
+    EVP_PKEY *pkey;
+
+    if (!engine_persist)
+        return NULL;
+
+    ui = UI_create_method("openvpn");
+
+    if (!ui)
+        return NULL;
+
+    UI_method_set_reader(ui, ui_read);
+
+    ERR_clear_error();		/* BIO read failure */
+    if (!ENGINE_init(engine_persist)) {
+	ERR_print_errors_fp(stderr);
+	pkey = NULL;
+	goto out;
+    }
+    pkey = ENGINE_load_private_key(engine_persist, file, ui, ctx);
+    ENGINE_finish(engine_persist);
+    if (!pkey)
+	ERR_print_errors_fp(stderr);
+ out:
+    UI_destroy_method(ui);
+    return pkey;
+}
+
 #endif /* ENABLE_CRYPTO && ENABLE_CRYPTO_OPENSSL */
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 92a662b5..52e9a869 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -839,7 +839,11 @@  tls_ctx_load_priv_file(struct tls_root_ctx *ctx, const char *priv_key_file,
                                    SSL_CTX_get_default_passwd_cb_userdata(ctx->ctx));
     if (!pkey)
     {
-        goto end;
+        pkey = engine_load_key(priv_key_file, ctx->ctx);
+        if (!pkey)
+        {
+            goto end;
+        }
     }
 
     if (!SSL_CTX_use_PrivateKey(ssl_ctx, pkey))