Do not set CURLOPT_SSLENGINEDEFAULT automatically

[dwmw2]

There were bugs in the PKCS#11 engine, and fixing them triggers bugs in
OpenSSL. Just don't get involved; there's no need to be making the engine
methods the default anyway.

OpenSC/libp11#108
openssl/openssl#1639

[jay]

I realize this has been a big source of pain for you but there is not enough here to justify removing this. You haven't explained why the code is incorrect for engines, though I can see in the bug why it is a problem for yours. My understanding of this is that the default engine is supposed to be used for all crypto protocols when we call ENGINE_set_default. Isn't that what most people would expect, and wouldn't we break things by removing it?

[dwmw2]

My understanding of this is that the default engine is supposed to be used for all crypto protocols when we call ENGINE_set_default. Isn't that what most people would expect, and wouldn't we break things by removing it?

Yes, but there are very few reasons to care about that. You're right — If you have an external engine that has some kind of crypto acceleration for software keys, like Intel AES-NI instruction support, you can load that engine and it will be used for all such operations, overriding the internal OpenSSL functionality. And at https://www.openssl.org/blog/blog/2015/11/23/engine-building-lesson-2-an-example-md5-engine/ there's a lovely example of how to write your own engine to override the MD5 methods in OpenSSL.

In general, though, this isn't stunningly useful. And if it was, then OpenSSL would be configured to load that engine by default.... like it did the AESNI engine, without applications needing to ask for it.

So no, the interesting use case for the engine is not that. It's for loading certificates and private keys. The documentation for CURLOPT_SSLENGINEsays that "It will be used as the identifier for the crypto engine you want to use for your private key."

Actually it can be used for loading certificates too, but only with engine_pkcs11 AFAIK, because the LOAD_CERT_CTRL command you use to load a certificate is specific to that engine and I'm not aware of any other engine which implements it.

And remember, I haven't removed the CURLOPT_SSLENGINE_DEFAULT option; only stopped the curl(1) command line tool from setting it by default. Because it is triggering other bugs in the ecosystem, for no good reason.

[dwmw2]

Citing from OpenSC/libp11#110 (comment) where one of the core OpenSSL developers says:

You know, if I understand things correctly, the real issue here is that OpenSSL's apps/apps.c:setup_engine() calls ENGINE_set_default().

He's absolutely right. The problem is that things are calling ENGINE_set_default() when they shouldn't, because they don't really mean it. We can avoid caring about this whole can of worms, by just NOT DOING THAT.

Note that I have not changed the libcurl API here. All I've done is change the behaviour of the command-line tool. There are four possibilities for the tool's behaviour. It can enable the CURLOPT_SSLENGINEDEFAULT option by default, or not. And it can have a command-line option to (dis,re-en)able it according to the user's whim, or not.

In this patch I've disabled it by default, and I haven't added a command-line option to let you turn it back on again. I briefly pondered doing so but decided it wasn't worth it, because I cannot think of a single valid use case where you'd actually want to load an engine and use ENGINE_set_default() on it.

Even if you live in Russia and you want to use GOST everywhere, you'll have configured OpenSSL system-wide to load that engine automatically (via openssl.cnf), so there's no need to be messing with curl to make it do so.

@jay: If I'm missing something, I'd be very interested to hear what you think we'd be breaking by removing it.

[jay]

The problem is that things are calling ENGINE_set_default() when they shouldn't, because they don't really mean it. We can avoid caring about this whole can of worms, by just NOT DOING THAT.

But what I'm saying to you is I think in our case it was intentional, it was really meant. And that option has been the same in curl for 15 years with nary a report about this.

If I'm missing something, I'd be very interested to hear what you think we'd be breaking by removing it.

Specifically I was referring to --engine documented as "[selecting] the OpenSSL crypto engine to use for cipher operations". My question --and it is a question not an assertion-- is if we would break anything (or really just not behave as one would expect) by not setting CURLOPT_SSLENGINE_DEFAULT (ENGINE_set_default). The answer does not seem to be a hard no. I'd guess at some point in the past crypto accelerators were more common and desirable as the default; @levitte alludes to as much in his explanation.

Let's look at this a different way. Instead of what could break, what engines are we fixing? Is it exclusive to just pkcs, and for any engine using that name (if such things are allowed); and in that case could we make a workaround for just that? I'm still trying to wrap my head around why you wouldn't want to default any methods at all, why if your token doesn't have xxx methods OpenSSL would be using them.

[dwmw2]

I cannot think of any case where you actually want to use ENGINE_set_default(). Which is why I removed it — without even giving a --engine-default command line option to put it back.

I don't think this is just the PKCS#11 engine; OpenSSL currently doesn't have a way for engines to support hardware keys, without curl inappropriately marking them as the default.... even for keys which they can't handle. It's a problem for any software which calls ENGINE_set_default() when it shouldn't be, for any engine.

However, my primary motivation for turning this unnecessary behaviour off is to make it work nicely with the PKCS#11 engine... and in fact I have another way to cope with that. Because quite frankly, if I pass you the string pkcs11:manufacturer=piv_II;id=%01 as my certificate, then I BLOODY WELL SHOULDN'T HAVE TO TELL YOU to use the PKCS#11 engine explicitly.

If curl is built with GnuTLS, I don't need to do that.
If curl is built with NSS (and these fixes), I don't need to do that.

So I need to know that curl is built with OpenSSL, and that I need to set this silly ENGINE option to point to the PKCS#11 engine anyway... which sucks.

I plan to fix that, to make it work consistently regardless of which crypto library curl is built with. Which means that I'll implicitly set the PKCS#11 engine if we're passed a PKCS#11 URI as our certificate/key. And I suippose in that case we can avoid using ENGINE_set_default().

So I don't need to care about the case where anyone explicitly specifies the ENGINE at all. I can let that continue to do the silly pointless default thing that achieves nothing but to break stuff, if you really insist.

[bagder]

Thanks, merged in commit a1a5cd0.