| Message ID | 1509192205.3021.7.camel@HansenPartnership.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add engine keys keys | expand |
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"><<a href="mailto:James.Bottomley@hansenpartnership.com" target="_blank">James.Bottomley@hansenpartnership.com</a>></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 <James.Bottomley@<wbr>HansenPartnership.com><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, &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("openvpn");<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 && 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->ctx));<br> if (!pkey)<br> {<br> - goto end;<br> + pkey = engine_load_key(priv_key_file, ctx->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'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
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
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 >
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))
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
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
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"><<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>></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> > This looks like it should use our user query wrappers from (e.g.)<br> > console.h. David, you're the expert here, what should James use to<br> > query for passwords?<br> <br> </span>The mechanics are "query_user_...()", most conveniently<br> <br> /**<br> * A plain "make Gert happy" 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 "other mechanisms"<br> if console input is not available, for example, systemd querying.<br></blockquote><div><br></div><div>Shouldn'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
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.
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
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))
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(-)