| Message ID | CALt099LSCurvfzNXxt1Ro+rvHpaYfN_k=nmYxCQ+DFYrtwAA6A@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 30, 2016 at 07:19:40PM +0200, Michael Schaller wrote: > The first ENGINE_by_id call (line 730) in > tls_engine_load_dynamic_generic is used to check if a certain OpenSSL > engine is already loaded: > https://w1.fi/cgit/hostap/tree/src/crypto/tls_openssl.c#n730 > > This ENGINE_by_id call has a side effect though that it automatically > loads that engine with the default options if the shared object of > that engine can be found by openssl. This means that if the autoload > succeeds then this check will always be true and hence this engine > can't ever be loaded with the specific options for WPA supplicant as > specified in the configuration. Could you please provide some more details on a case where this happens and what those specific options in the configuration would be? I tried to test this myself, but for some reason, did not see OpenSSL being able to load any engine automatically.. Anyway, looking at the related configuration parameters, other than the specific directory of the engine file, the only thing that seems to impact the engine loading operations seems to be the module patch for pkcs11. And even that could potentially be set with ENGINE_ctrl_cmd_string() after the ENGINE_by_id() call.. > I'm not sure what's the best way to fix this issue but you'll find a > patch proposal in the end that iterates over the available engines > instead of using ENGINE_by_id to avoid the engine autoload. I'm not sure this would be generic enough if there is need to change parameters and wpa_supplicant allows the configuration parameters like pkcs11_module_path to be changed at runtime.. That might require an additional ENGINE_ctrl_cmd_string() call at some point.. And I'm not sure whether that would even work in practice, i.e., whether the pkcs11 engine allows such a change after having been used for some operations. Even for the case where nothing really changes in the configuration, I'm not sure I see why there would be need to query anything for OpenSSL either.. Wouldn't it be sufficient to track within wpa_supplicant whether the specific engine has been loaded or not? I'm not very familiar with the existing OpenSSL engine cases, so it is somewhat difficult to speculate on the best way of addressing this, though.
On Sun, 2016-06-05 at 17:06 +0300, Jouni Malinen wrote: > On Mon, May 30, 2016 at 07:19:40PM +0200, Michael Schaller wrote: > > The first ENGINE_by_id call (line 730) in > > tls_engine_load_dynamic_generic is used to check if a certain OpenSSL > > engine is already loaded: > > https://w1.fi/cgit/hostap/tree/src/crypto/tls_openssl.c#n730 > > > > This ENGINE_by_id call has a side effect though that it automatically > > loads that engine with the default options if the shared object of > > that engine can be found by openssl. This means that if the autoload > > succeeds then this check will always be true and hence this engine > > can't ever be loaded with the specific options for WPA supplicant as > > specified in the configuration. > > Could you please provide some more details on a case where this happens > and what those specific options in the configuration would be? > > I tried to test this myself, but for some reason, did not see OpenSSL > being able to load any engine automatically.. Historically, the PKCS#11 engine required you to jump through a lot of hoops to load and use it. We fixed this fairly recently. I suspect it's only Fedora where you can expect it to work out of the box with the distribution packages so far, but it will reach other distributions in time. You used to have to load the engine manually, and you used to have to specify the PKCS#11 module manually. Now you don't. It's named 'libpkcs11.so' in the OpenSSL engines directory so it's loaded automatically with ENGINE_by_id(), and it loads p11-kit-proxy.so as its module by default — so the system- configured PKCS#11 tokens for the process in question are automatically available. So it really should be as simple as loading the module, then using a PKCS#11 URI (as specified in RFC7512) to reference the key and/or certificate. In commit 96955192 I already fixed the code to automatically load the PKCS#11 engine when the string given for the cert or key starts with 'pkcs11:' and thus looks like a standard PKCS#11 URI. > Anyway, looking at the > related configuration parameters, other than the specific directory of > the engine file, the only thing that seems to impact the engine loading > operations seems to be the module patch for pkcs11. And even that could > potentially be set with ENGINE_ctrl_cmd_string() after the > ENGINE_by_id() call.. All of that is legacy crap. You shouldn't need it these days. > I'm not very familiar with the existing OpenSSL engine cases, so it is > somewhat difficult to speculate on the best way of addressing this, > though. The most interesting use case is when you just put a PKCS#11 URI into the 'private_key' or 'client_cert' fields instead of a file name, and it Just Works™. Import a cert into gnome-keyring or something like that (using seahorse). Run 'p11tool --list-all pkcs11:token=Gnome2%20Key%20Storage' and note the URL of the key and/or cert you want to use. Put them into the configuration file. That's it¹. You can also use certs/keys from your NSS softoken database, if you ask nicely. There are other engines, which are mostly uninteresting because people should be offering PKCS#11 modules for their hardware instead of OpenSSL-specific engines. And there are historical and/or badly- configured or badly-installed instances of ENGINE_pkcs11. Which aren't worth pandering to if they've been broken since 2002 either, (Although if they've only been broken since last year when I changed a bunch of stuff here, then fixing them *might* make sense).
David explains correctly what happens. This started happening for us on Debian testing with the release of the libengine-pkcs11-openssl package version 0.2.1-2 in March 2016 and it took us a while to analyze the issue. The package had changed the libpkcs11.so location from /usr/lib/ssl/engines/libpkcs11.so to /usr/lib/x86_64-linux-gnu/openssl-1.0.2/engines/libpkcs11.so. Since this move the autoload of the pkcs11 engine on the ENGINE_by_id call works and we get a pkcs11 engine initialized for the use of p11-kit instead of the expected initialization for the specified opencryptoki module. We initialize the pkcs11 engine and module path via D-Bus via the SetPKCS11EngineAndModulePath method. David, is the general recommendation to switch over to p11-kit? On Mon, Jun 6, 2016 at 10:26 AM David Woodhouse <dwmw2@infradead.org> wrote: > > On Sun, 2016-06-05 at 17:06 +0300, Jouni Malinen wrote: > > On Mon, May 30, 2016 at 07:19:40PM +0200, Michael Schaller wrote: > > > The first ENGINE_by_id call (line 730) in > > > tls_engine_load_dynamic_generic is used to check if a certain OpenSSL > > > engine is already loaded: > > > https://w1.fi/cgit/hostap/tree/src/crypto/tls_openssl.c#n730 > > > > > > This ENGINE_by_id call has a side effect though that it automatically > > > loads that engine with the default options if the shared object of > > > that engine can be found by openssl. This means that if the autoload > > > succeeds then this check will always be true and hence this engine > > > can't ever be loaded with the specific options for WPA supplicant as > > > specified in the configuration. > > > > Could you please provide some more details on a case where this happens > > and what those specific options in the configuration would be? > > > > I tried to test this myself, but for some reason, did not see OpenSSL > > being able to load any engine automatically.. > > Historically, the PKCS#11 engine required you to jump through a lot of > hoops to load and use it. We fixed this fairly recently. I suspect it's > only Fedora where you can expect it to work out of the box with the > distribution packages so far, but it will reach other distributions in > time. > > You used to have to load the engine manually, and you used to have to > specify the PKCS#11 module manually. > > Now you don't. It's named 'libpkcs11.so' in the OpenSSL engines > directory so it's loaded automatically with ENGINE_by_id(), and it > loads p11-kit-proxy.so as its module by default — so the system- > configured PKCS#11 tokens for the process in question are automatically > available. > > So it really should be as simple as loading the module, then using a > PKCS#11 URI (as specified in RFC7512) to reference the key and/or > certificate. > > In commit 96955192 I already fixed the code to automatically load the > PKCS#11 engine when the string given for the cert or key starts with > 'pkcs11:' and thus looks like a standard PKCS#11 URI. > > > Anyway, looking at the > > related configuration parameters, other than the specific directory of > > the engine file, the only thing that seems to impact the engine loading > > operations seems to be the module patch for pkcs11. And even that could > > potentially be set with ENGINE_ctrl_cmd_string() after the > > ENGINE_by_id() call.. > > All of that is legacy crap. You shouldn't need it these days. > > > I'm not very familiar with the existing OpenSSL engine cases, so it is > > somewhat difficult to speculate on the best way of addressing this, > > though. > > The most interesting use case is when you just put a PKCS#11 URI into > the 'private_key' or 'client_cert' fields instead of a file name, and > it Just Works™. > > Import a cert into gnome-keyring or something like that (using > seahorse). Run 'p11tool --list-all pkcs11:token=Gnome2%20Key%20Storage' > and note the URL of the key and/or cert you want to use. Put them into > the configuration file. That's it¹. > > You can also use certs/keys from your NSS softoken database, if you ask > nicely. > > There are other engines, which are mostly uninteresting because people > should be offering PKCS#11 modules for their hardware instead of > OpenSSL-specific engines. And there are historical and/or badly- > configured or badly-installed instances of ENGINE_pkcs11. Which aren't > worth pandering to if they've been broken since 2002 either, > > (Although if they've only been broken since last year when I changed a > bunch of stuff here, then fixing them *might* make sense). > > -- > David Woodhouse Open Source Technology Centre > David.Woodhouse@intel.com Intel Corporation > > > ¹ By using gnome-keyring as the example, I've neatly side-stepped the > question of providing the PIN, which is unfortunately still *different* > in the wpa_supplicant config file depending on whether you're using > GnuTLS or OpenSSL, IIRC. That ought to be uniform, but isn't.
On Mon, 2016-06-06 at 14:13 +0200, Michael Schaller wrote: > David explains correctly what happens. This started happening for us > on Debian testing with the release of the libengine-pkcs11-openssl > package version 0.2.1-2 in March 2016 and it took us a while to > analyze the issue. The package had changed the libpkcs11.so location > from /usr/lib/ssl/engines/libpkcs11.so to > /usr/lib/x86_64-linux-gnu/openssl-1.0.2/engines/libpkcs11.so. Since > this move the autoload of the pkcs11 engine on the ENGINE_by_id call > works and we get a pkcs11 engine initialized for the use of p11-kit > instead of the expected initialization for the specified opencryptoki > module. We initialize the pkcs11 engine and module path via D-Bus via > the SetPKCS11EngineAndModulePath method. > > David, is the general recommendation to switch over to p11-kit? Yes, definitely. With p11-kit you get a system-wide configuration for which modules to load into which applications, with a per-user override: https://p11-glue.freedesktop.org/doc/p11-kit/pkcs11-conf.html This is honoured directly by GnuTLS applications because GnuTLS uses libp11-kit directly. For other crypto libraries (OpenSSL, NSS, etc.) it can be achieved just by loading the p11-kit-proxy.so module — which loads all the modules specified by the p11-kit configuration, as "slots" of its own. In Fedora it is now considered a bug if any packaged software *doesn't* automatically load the tokens specified by the p11-kit configuration, and allow you to use a RFC7512 PKCS#11 URI in any place you would have been able to use a filename to specify a certificate or key. So to go back to what you said above... > we get a pkcs11 engine initialized for the use of p11-kit instead of > the expected initialization for the specified opencryptoki> module. It's not supposed to be "instead of". Your p11-kit configuration is supposed to say what modules you want loaded, and then they're visible. (Yes, it would be nicer if OpenSSL and the engine would use libp11-kit directly instead of the trick of loading p11-kit-proxy.so, but we'll work on that for OpenSSL 1.3 where we turn PKCS#11 into a proper first- class citizen and we'll be able to use it for trust and all kinds of other things properly.) > We initialize the pkcs11 engine and module path via D-Bus via> the > SetPKCS11EngineAndModulePath method. You shouldn't need to do that. If the 'filename' you pass starts with 'pkcs11:' then it gets done automatically. And if you are referencing objects in the PKCS#11 token by the horrid old crufty ID string that predated RFC7512, then don't :)
Thank you for the input, David. I'll discuss the issue with my colleague Mike Gerow and we will probably switch to using p11-kit. When it comes to WPA Supplicant itself do you think that manually specifying the pkcs11 engine and module path should be deprecated/removed? On Mon, Jun 6, 2016 at 2:34 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Mon, 2016-06-06 at 14:13 +0200, Michael Schaller wrote: >> David explains correctly what happens. This started happening for us >> on Debian testing with the release of the libengine-pkcs11-openssl >> package version 0.2.1-2 in March 2016 and it took us a while to >> analyze the issue. The package had changed the libpkcs11.so location >> from /usr/lib/ssl/engines/libpkcs11.so to >> /usr/lib/x86_64-linux-gnu/openssl-1.0.2/engines/libpkcs11.so. Since >> this move the autoload of the pkcs11 engine on the ENGINE_by_id call >> works and we get a pkcs11 engine initialized for the use of p11-kit >> instead of the expected initialization for the specified opencryptoki >> module. We initialize the pkcs11 engine and module path via D-Bus via >> the SetPKCS11EngineAndModulePath method. >> >> David, is the general recommendation to switch over to p11-kit? > > Yes, definitely. > > With p11-kit you get a system-wide configuration for which modules to > load into which applications, with a per-user override: > https://p11-glue.freedesktop.org/doc/p11-kit/pkcs11-conf.html > > This is honoured directly by GnuTLS applications because GnuTLS uses > libp11-kit directly. For other crypto libraries (OpenSSL, NSS, etc.) it > can be achieved just by loading the p11-kit-proxy.so module — which > loads all the modules specified by the p11-kit configuration, as > "slots" of its own. > > In Fedora it is now considered a bug if any packaged software *doesn't* > automatically load the tokens specified by the p11-kit configuration, > and allow you to use a RFC7512 PKCS#11 URI in any place you would have > been able to use a filename to specify a certificate or key. > > So to go back to what you said above... > >> we get a pkcs11 engine initialized for the use of p11-kit instead of >> the expected initialization for the specified opencryptoki> module. > > It's not supposed to be "instead of". Your p11-kit configuration is > supposed to say what modules you want loaded, and then they're visible. > > (Yes, it would be nicer if OpenSSL and the engine would use libp11-kit > directly instead of the trick of loading p11-kit-proxy.so, but we'll > work on that for OpenSSL 1.3 where we turn PKCS#11 into a proper first- > class citizen and we'll be able to use it for trust and all kinds of > other things properly.) > >> We initialize the pkcs11 engine and module path via D-Bus via> the >> SetPKCS11EngineAndModulePath method. > > You shouldn't need to do that. If the 'filename' you pass starts with > 'pkcs11:' then it gets done automatically. And if you are referencing > objects in the PKCS#11 token by the horrid old crufty ID string that > predated RFC7512, then don't :) > > -- > dwmw2
On Mon, 2016-06-06 at 14:56 +0200, Michael Schaller wrote: > Thank you for the input, David. I'll discuss the issue with my > colleague Mike Gerow and we will probably switch to using p11-kit. > When it comes to WPA Supplicant itself do you think that manually > specifying the pkcs11 engine and module path should be > deprecated/removed? Deprecated, yes. But there'll be a long tail of legacy installations (and distributions which aren't keeping up with the times) before we can actually *remove* the support. I'm not entirely sure about OS X either. It's perfectly reasonable to assume p11-kit on any Linux distribution, but perhaps OSX still wants to do its own thing? (And do we even have an engine or PKCS#11 module that accesses the OSX keychain...?)
I agree about the Linux part and I have no clue about the Mac OS X part. ;-) For me only remains one topic then. If specifying the pkcs11 engine and module path is on the way of deprecation (but IMHO not quite there yet) is it then worth fixing this issue? If yes, what about the proposed patch to not use ENGINE_by_id to check if an engine has been already loaded? On Mon, Jun 6, 2016 at 3:09 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Mon, 2016-06-06 at 14:56 +0200, Michael Schaller wrote: >> Thank you for the input, David. I'll discuss the issue with my >> colleague Mike Gerow and we will probably switch to using p11-kit. >> When it comes to WPA Supplicant itself do you think that manually >> specifying the pkcs11 engine and module path should be >> deprecated/removed? > > Deprecated, yes. But there'll be a long tail of legacy installations > (and distributions which aren't keeping up with the times) before we > can actually *remove* the support. > > I'm not entirely sure about OS X either. It's perfectly reasonable to > assume p11-kit on any Linux distribution, but perhaps OSX still wants > to do its own thing? (And do we even have an engine or PKCS#11 module > that accesses the OSX keychain...?) > > -- > dwmw2
On Mon, 2016-06-06 at 17:56 +0200, Michael Schaller wrote: > > For me only remains one topic then. If specifying the pkcs11 engine > and module path is on the way of deprecation (but IMHO not quite there > yet) is it then worth fixing this issue? If yes, what about the > proposed patch to not use ENGINE_by_id to check if an engine has been > already loaded? If I broke something last year (actually, I think it was December 2014) when I cleaned up the auto-load code paths, then sure — we should probably fix that. If it's something that basically never worked, then there's no point in fixing it now. I suppose that if we take the holistic view, I really did break it last year — not by anything I changed in hostap/wpa_supplicant, but by changing engine_pkcs11 to install into the standand engine directory so that ENGINE_by_id() *can* now find it. Perhaps just avoid the ENGINE_by_id() *if* there are explicit pre commands. In that case we'll do it through the dynamic engine anyway, and we don't need the fallback of iterating over the list? (That code path is broken if you need to use the dynamic engine to load the one you want, and if there are *no* 'pre' commands, right?...)
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. * The author(s) of the WPA Supplicant engine support (2) probably didn't know about the autoloading attempt / side effect of ENGINE_by_id and because of the lack of documentation and because the pkcs11 engine typically wasn't autoloadable they assumed that it could be used to determine if an engine is already loaded. The really tricky question is how to fix this issue. IMHO the ENGINE_by_id behavior can't be fixed because the autoloading is part of the API. This behavior should at least be documented though. That leaves us with fixing WPA Supplicant and here I don't know what's the best way to fix it. First of all the pkcs11 engine gets initialized as part of the tls_init call and I don't know how often tls_init is called and if it makes sense to skip the engine initialization if it has been already initialized previously. Secondly I also don't know if the configuration step should be skipped if the pkcs11 engine is already loaded as the engine could be differently configured to what is needed. Lastly some kind of check if the pkcs11 engine is already loaded is probably sensible as reinitialization doesn't work without prior removal/unload if the engine. David, what you propose with regards to skipping the first ENGINE_by_id call if pre commands are given would work as long as the pkcs11 engine isn't initialized yet for whatever reason. IMHO it would be sensible to have a check if the engine is already loaded, as proposed in my patch, and then the engine should be probably unloaded so that it can be reloaded with the proper configuration. What do you think? On Tue, Jun 7, 2016 at 8:51 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Mon, 2016-06-06 at 17:56 +0200, Michael Schaller wrote: >> >> For me only remains one topic then. If specifying the pkcs11 engine >> and module path is on the way of deprecation (but IMHO not quite there >> yet) is it then worth fixing this issue? If yes, what about the >> proposed patch to not use ENGINE_by_id to check if an engine has been >> already loaded? > > If I broke something last year (actually, I think it was December 2014) > when I cleaned up the auto-load code paths, then sure — we should > probably fix that. > > If it's something that basically never worked, then there's no point in > fixing it now. > > I suppose that if we take the holistic view, I really did break it last > year — not by anything I changed in hostap/wpa_supplicant, but by > changing engine_pkcs11 to install into the standand engine directory so > that ENGINE_by_id() *can* now find it. > > Perhaps just avoid the ENGINE_by_id() *if* there are explicit pre > commands. In that case we'll do it through the dynamic engine anyway, > and we don't need the fallback of iterating over the list? > > (That code path is broken if you need to use the dynamic engine to load > the one you want, and if there are *no* 'pre' commands, right?...) > > -- > dwmw2
--- ./src/crypto/tls_openssl.c.old 2016-05-30 13:35:15.341868226 +0000 +++ ./src/crypto/tls_openssl.c 2016-05-30 16:56:29.880912599 +0000 @@ -617,7 +617,14 @@ ENGINE *engine; const char *dynamic_id = "dynamic"; - engine = ENGINE_by_id(id); + /* + * Check if engine is already loaded. This intentionally doesn't use + * ENGINE_by_id as this would autoload an engine if it isn't loaded yet. + */ + for (engine = ENGINE_get_first(); engine; engine = ENGINE_get_next(engine)) { + if(!strcmp(id, ENGINE_get_id(engine))) + break; + } if (engine) { ENGINE_free(engine); wpa_printf(MSG_DEBUG, "ENGINE: engine '%s' is already "