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 _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel