David, thank you for commit f1931ec9d3d4ccdc277c170da5bf37e912e5cba4. I've tested the patch and it works like a charm. :-) On Tue, Jun 7, 2016 at 11:54 AM, David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote: > On Tue, 2016-06-07 at 09:50 +0200, Michael Schaller wrote: >> David, thanks for the details. I think I slowly get a timeline how >> this issue came into being: >> 1) 2002: OpenSSL enables automatic loading of engines by the >> ENGINE_by_id call: >> https://github.com/openssl/openssl/commit/aae329c447025eb87dab294d909f9fbc48f7174c >> 2) 2005: WPA Supplicant gets engine support: >> https://w1.fi/cgit/hostap-history/commit/?id=e7eb09e9652dd745e1df3649b79af70426ab6bc9 >> 3) 2014: engine_pkcs11 defaults to p11-kit: >> https://github.com/OpenSC/engine_pkcs11/commit/d04bccbdb8c0607038be1fa4aa802268ad5c1edd >> 4) 2016: OpenSSL engines directory autoselection got fixed in >> engine_pkcs11: https://github.com/OpenSC/engine_pkcs11/commit/d04bccbdb8c0607038be1fa4aa802268ad5c1edd >> 5) 2016: Debian fixes the packaging to install libpkcs11.so in the >> correct directory: >> https://anonscm.debian.org/cgit/pkg-opensc/engine-pkcs11.git/commit/?id=0f9adff289380caf2887276d6e979871dbe174ba >> >> IMHO no one is really to blame for this issue but rather changes piled >> up to lead to a breakage. >> From my point of view this went wrong: >> * The OpenSSL commit to enable engine autoloading (1) should IMHO have >> used a new function for autoloading rather than extending >> ENGINE_by_id. In any case they should have updated the documentation. > > Hm, wait a moment. Let's take another look at those 'pre' and 'post' > commands. That isn't an OpenSSL thing; that's purely for the benefit of > our own tls_engine_load_engine_dynamic() function. > > What we call 'pre' commands are actually the commands we give to the > "dynamic" engine to get it to load the engine we want — e.g.: > > char *engine_id = "pkcs11"; > const char *pre_cmd[] = { > "SO_PATH", NULL /* pkcs11_so_path */, > "ID", NULL /* engine_id */, > "LIST_ADD", "1", > /* "NO_VCHECK", "1", */ > "LOAD", NULL, > NULL, NULL > }; > > Then the "post" commands are the commands given to that engine before > we (later, from tls_engine_init()) call ENGINE_init() on it. > > It's the *post* command which tells engine_pkcs11 where to find the > actual PKCS#11 module we want it to use (which can now be unspecified > and default to p11-kit-proxy.so). > > So letting engine_pkcs11 get autoloaded by ENGINE_by_id() isn't a > problem at all, is it? > > The only thing is that if it *does* get autoloaded, and if we have a > 'post' command, we should still give it the 'post' command instead of > assuming it's already completely set up. > > We can't actually tell if it's been autoloaded or not. But it does look > like it's harmless to give it the MODULE_PATH command even if it has > already been initialised. (We could probably do with fixing the engine > to return an error to the MODULE_PATH command if it's already > initialised with a different module, but that isn't a case that worked > for us before so I can live with that.) > > So how about this... > > diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c > index c831fba..23ac64b 100644 > --- a/src/crypto/tls_openssl.c > +++ b/src/crypto/tls_openssl.c > @@ -729,10 +729,16 @@ static int tls_engine_load_dynamic_generic(const char *pre[], > > engine = ENGINE_by_id(id); > if (engine) { > - ENGINE_free(engine); > wpa_printf(MSG_DEBUG, "ENGINE: engine '%s' is already " > "available", id); > - return 0; > + /* > + * If it was auto-loaded by ENGINE_by_id() we might still > + * need to tell it which PKCS#11 module to use in legacy > + * (non-p11-kit) environments. Do so now; even if it was > + * properly initialised before, setting it again will be > + * harmless. > + */ > + goto found; > } > ERR_clear_error(); > > @@ -769,7 +775,7 @@ static int tls_engine_load_dynamic_generic(const char *pre[], > id, ERR_error_string(ERR_get_error(), NULL)); > return -1; > } > - > + found: > while (post && post[0]) { > wpa_printf(MSG_DEBUG, "ENGINE: '%s' '%s'", post[0], post[1]); > if (ENGINE_ctrl_cmd_string(engine, post[0], post[1], 0) == 0) { > > > > -- > David Woodhouse Open Source Technology Centre > David.Woodhouse@xxxxxxxxx Intel Corporation _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
