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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2013, 20:17 UTC
Message-ID: <7vzjwfbwow.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAJC3a=pU2m8LpUh0u0XXXgOiDiPo-QAdA471orGs9jcyDJTU5g@mail.gmail.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.

Show 24 quoted lines

>>> > +   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?

Show 9 quoted lines

>> 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.