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

Jerry Qassar [off-list ref] 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.

quoted

quoted

quoted

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

quoted

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.