Jouni, thank you for committing the patches. David, Jouni, how about adding a log message that states that the pkcs11 engine and module path usage is deprecated and that they should switch to p11-kit URIs? FYI: I've opened a bug with Debian to include the patch in their packaging: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=827253 On Tue, Jun 7, 2016 at 4:49 PM, Michael Schaller <misch@xxxxxxxxxx> wrote: > 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
