| Message ID | 1516998620.3034.16.camel@HansenPartnership.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] PATCH v3 1/2] openssl: add engine method for loading the key | expand |
Am 26.01.18 um 21:30 schrieb James Bottomley: > 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> > > --- > > v2: add better configuration guarding > --- > 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 20a519ec..d3f35030 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); > } > > +#if HAVE_OPENSSL_ENGINE > +static int > +ui_read(UI *ui, UI_STRING *uis) I would rather a bit more verbose method name here. ui_read seems a bit too generic. > +{ > + 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); This pointer is never used and the function also seem to have no side effects. So seem like this can be removed. > + 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) > +{ > +#if 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); This should use our standard openssl error reporting. Not directly writing to 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); Same as above. > + out: > + UI_destroy_method(ui); > + return pkey; > +#else > + return NULL; > +#endif > +} ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Mon, 2018-01-29 at 08:43 +0100, Arne Schwabe wrote: > Am 26.01.18 um 21:30 schrieb James Bottomley: > > > > 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.c > > om> > > > > --- > > > > v2: add better configuration guarding > > --- > > 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 20a519ec..d3f35030 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); > > } > > > > +#if HAVE_OPENSSL_ENGINE > > +static int > > +ui_read(UI *ui, UI_STRING *uis) > > I would rather a bit more verbose method name here. ui_read seems a > bit too generic. I can call it anything you like, but the naming scheme in the file seems to be <openssl structure>_<function>, like md_ctx_init() etc, EVP_MD_CTX is the structure and init the function. Here UI is the structure and read the function. Would you prefer UI_reader to match the method set name? > > +{ > > + 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); > > This pointer is never used and the function also seem to have no side > effects. So seem like this can be removed. It's used two lines below: > > > > + char password[64]; > > + > > + cb(password, sizeof(password), 0, d); Here -----------------------------^ If you mean it can be removed because the openssl password callback data pointer is never used in the openvpn implementation then, yes, that's true, but I think following the openssl standard practice is better otherwise engine keys would break if someone ever found a future openvpn use for the data pointer. > > + UI_set_result(ui, uis, password); > > + > > + return 1; > > + } > > + return 0; > > +} > > +#endif > > + > > +EVP_PKEY * > > +engine_load_key(const char *file, SSL_CTX *ctx) > > +{ > > +#if 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); > > This should use our standard openssl error reporting. Not directly > writing to stderr. Agreed, I'll make them crypto_msg() calls. James > > > > + 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); > > Same as above. > > > > > + out: > > + UI_destroy_method(ui); > > + return pkey; > > +#else > > + return NULL; > > +#endif > > +} > ------------------------------------------------------------------------------ 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_openssl.c b/src/openvpn/crypto_openssl.c index 20a519ec..d3f35030 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); } +#if 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) +{ +#if 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_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 cc1f8f15..200c0189 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -907,7 +907,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> --- v2: add better configuration guarding --- 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(-)