Re: [PATCH] http.c: Add config options/parsing for SSL engine vars

[Junio C Hamano <gitster@pobox.com>]

Jerry Qassar <jqassar@gmail.com> writes:

> Curl already does support engine-based certificates (in code and
> help).  Its problem is that a) it doesn't yet read your engine
> defs out of OpenSSL config, and b) a bug in copying the engine
> data, once that's patched, to the handle that calling apps use.

So once the problem (a) is fixed, if the user has OpenSSL config
then the user doesn't need configuration from setopt() side?  That
makes it sound like you do not need to patch us at all, but there
must be something else going on...

> On git's side we just need to expose the proper options for setopt
> configuration.  No special work is needed to support them
> otherwise.

... I am somewhat puzzled.

>>> > +   if (ssl_keytype != NULL)
>>> > +           curl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_keytype);
>>> > +   if (ssl_certtype != NULL)
>>> > +           curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_certtype);
>>
>> Shouldn't we be checking the result of curl_easy_setopt for errors here
>> (and when the engine cannot be loaded)?  I think we should probably die
>> if the engine can't be loaded, but at the very least we'd want to warn
>> the user that their settings are being ignored.
>
> Errors are handled by curl (up to this point):
>
> 1) Setting the cert type to FOO:
> error: not supported file type 'FOO' for certificate...
> fatal: HTTP request failed
>
> 2) Setting the key type to FOO:
> error: not supported file type for private key...
> fatal: HTTP request failed
>
> 3) Setting engine type to something invalid:
>  * SSL Engine 'pkcsfoo' not found (only with GIT_CURL_VERBOSE set)
> error: crypto engine not set, can't load certificate...
> fatal: HTTP request failed

Where do "error:" and "fatal:" happen in the codeflow?

I am guessing that "error:" may come from these easy_setopt() calls, but
the "fatal: HTTP request failed" come from us, much later in the
callpath when we actually make http request.

Between these two times, aren't we throwing user data at the cURL
library and possibly over the wire to the remote side (with a SSL
configuration that is different from what the user intended to use),
no?

That does not sound like a "managed by cURL" solution to me.
Shouldn't we notice the first error and abort without doing any
further damage?

>> Overall, I think it is looking good. Aside from a few style/cleanup
>> issues, my only real complaint is the lack of error-checking from
>> curl_easy_setopt.
>>
>> And of course adding some tests while you are working in the area would
>> be very nice. :)
> ...
> I guess my open question is, if you wish to wrap the
> prop setting in a curl version #if, what version is desired?

https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions

says that SSLKEYTYPE, SSLCERTTYPE, etc. come from 7.9.3, so it would
be 

        #if LIBCURL_VERSION_NUM >= 0x070903
            ...
        #endif

Thanks.