On Tue, Apr 30, 2013 at 01:04:17PM -0700, Jerry Qassar wrote:
> First, thanks very much for taking a look at this. I wasn't 100% certain about
> the versioning to use for it (specifically the version-to-0x mapping), so any
> input on that would be a big help. I'll try to answer your questions below...
The explanation is hiding in a comment in curlver.h:
This is the numeric version of the libcurl version number, meant for
easier parsing and comparions by programs. The LIBCURL_VERSION_NUM
define will always follow this syntax:
0xXXYYZZ
Where XX, YY and ZZ are the main version, release and patch numbers in
hexadecimal (using 8 bits each). All three numbers are always
represented using two digits. 1.2 would appear as "0x010200" while
version 9.11.7 appears as "0x090b07".
So I think you'd just want to check:
#if LIBCURL_VERSION_NUM >= 0x070903
It looks like we already have such an #if block for ssl_key, so you
could just add the new options there.
> > Current curl seems to take ENG only for the key, and assumes you have
> > the certificate on disk [...]
> [...]
> Curl already does support engine-based certificates (in code and
> help).
My "seems to..." above was based on reading the curl_easy_setopt
manpage. It sounds like that documentation may be out of date.
> I can't think of a way to reliably provide a hardware-dependent engine
> implementation to the suite. So ENG is probably out, but I can think
> of something to verify PEM and DER once I figure out how the test
> suite works.
I think the trickiest part for testing PEM/DER support will actually be
setting up the apache config to authenticate a user based on a client
side certificate. I was hoping you might know off-hand since that is
obviously the goal of your patch (though of course you might not be
using apache). The relevant config would be in t/lib-httpd/apache.conf,
and the tests could probably go into t/t5551-http-fetch.sh.
> > 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
>
> ...that last one could probably be a little better, but it's curl-managed.
Hmm. So all of that happens when we actually try to make the request. I
think that should be OK. Curl is presumably smart enough to fail early
in the *_perform() functions, when it notices the handle is in a bogus
state.
> Thanks very much for the constructive input. Once I make the
> corrections and determine how to make some appropriate tests I'll
> resubmit. I guess my open question is, if you wish to wrap the prop
> setting in a curl version #if, what version is desired?
>From my reading of the curl docs, it's 7.9.3.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html