| Message ID | 1465293252.46888.238.camel@infradead.org |
|---|---|
| State | Superseded |
| Headers | show |
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@infradead.org> 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@intel.com Intel Corporation
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@google.com> 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@infradead.org> 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@intel.com Intel Corporation
On Tue, 2016-06-14 at 11:01 +0200, Michael Schaller wrote: > 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? Sure, as long as you get the criteria right. It's deprecated on Linux systems where p11-kit is present. That's fairly much *all* traditional Linux distributions and many embedded ones, but that still leaves a number of platforms where OpenSSL could be used. That's why I went as far as 'these options should not need to be used explicitly' in the sample wpa_supplicant.conf file, but no further. I did almost submit a patch which rips out the support for the OpenSC engine — that one is lost *so* far in the mists of time that I couldn't even find a copy of its source, last time I looked. But it occurred to me that you could actually load *any* engine via opensc_engine_path, including the CAPI or OSX Keychain engines, and people might actually be doing so. > 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 FWIW if we're chasing stuff up into distributions there's a whole bunch of work going on to support PKCS#11 a a 'first class citizen'. It would basically Just Work™ for 802.1x in NetworkManager already if NM would just pass the string through, instead of validating a 'pkcs11:...' string as if it's a pathname and bailing out because no file exists with that name: https://bugzilla.gnome.org/show_bug.cgi?id=719982 It *does* work for OpenConnect VPN if you configure a PKCS#11 URI instead of a pathname, but you have to do that with nmcli because the GUI for selecting objects from PKCS#11 doesn't exist... although *that* is the subject of a GSoC project I'm mentoring this year, covered by https://bugzilla.gnome.org/show_bug.cgi?id=679860 It works for OpenVPN too, as long as your distro has incorporated the patches which enable URI support in pkcs11-helper: https://github.com/OpenSC/pkcs11-helper/pull/4
On Tue, Jun 14, 2016 at 11:26 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Tue, 2016-06-14 at 11:01 +0200, Michael Schaller wrote: >> 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? > > Sure, as long as you get the criteria right. > > It's deprecated on Linux systems where p11-kit is present. That's > fairly much *all* traditional Linux distributions and many embedded > ones, but that still leaves a number of platforms where OpenSSL could > be used. > > That's why I went as far as 'these options should not need to be used > explicitly' in the sample wpa_supplicant.conf file, but no further. > I forgot about the other platforms, again. Sorry. I guess an informational log message to suggest to use p11-kit instead is too much noise and so I guess this is all that can be done at the moment. Thanks David for thinking this thoroughly through. > I did almost submit a patch which rips out the support for the OpenSC > engine — that one is lost *so* far in the mists of time that I couldn't > even find a copy of its source, last time I looked. But it occurred to > me that you could actually load *any* engine via opensc_engine_path, > including the CAPI or OSX Keychain engines, and people might actually > be doing so. > I couldn't find anything about OpenSC's OpenSSL engine (engine_opensc.so) either and no supported Debian or Ubuntu release has a package that would provide that file. I guess they've moved on to pkcs11 + opensc module for good. And now that you mention it... The OpenSC configuration could indeed be used to use any OpenSSL engine. Deprecation is hard... :-/ >> 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 > > FWIW if we're chasing stuff up into distributions there's a whole bunch > of work going on to support PKCS#11 a a 'first class citizen'. It would > basically Just Work™ for 802.1x in NetworkManager already if NM would > just pass the string through, instead of validating a 'pkcs11:...' > string as if it's a pathname and bailing out because no file exists > with that name: https://bugzilla.gnome.org/show_bug.cgi?id=719982 > I hope that bug will be fixed for good one day. I'll forward the information to my colleague Mike Gerow and maybe he can provide that missing patch...
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) {