[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
From:       Jerry Qassar <jqassar () gmail ! com>
Date:       2013-04-30 21:05:22
Message-ID: CAJC3a=qu8jxJ2pD=4kbPVqQDBgv6vxXVXkt6ezqRoPdtMEfjRA () mail ! gmail ! com
[Download RAW message or body]

On Tue, Apr 30, 2013 at 1:29 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote:
>
>> 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...
>
> My understanding is that we first have to tell curl "yes, use the
> engine", and then the engine-specific OpenSSL config can be loaded by
> curl. But I am just guessing from the conversation up until now; I know
> nothing about ssl crypto engines.

That's correct.  Putting the engine definitions into OpenSSL
configuration makes them available to OpenSSL only.  I made changes to
have curl/libcurl read the config files (it didn't before).  git needs
changes to let it set the appropriate libcurl parameters to set the
engine and key/cert type.  Otherwise libcurl has the capability but
git can't utilize it.

>
> As an aside, curl can be linked against gnutls, too. Does any of this
> work with gnutls? I think we don't have to care; curl abstracts all of
> that away from us, and it is up to the user to choose an engine that
> matches their library versions. But it might be a good point of
> reference when somebody later comes to the list and says "I followed the
> documentation, but it doesn't work".

gnutls doesn't let you specify an 'engine' per se and the underlying
engine calls return CURLE_NOT_BUILT_IN, so these changes would have no
effect.  That doesn't mean that gnutls can't work (I know for a fact
that it's possible for it to speak CAC), but I'm not sure that
anything git does internally matters to it.

Maybe the documentation also needs to say specifically that it is for OpenSSL?

>> > 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.
>
> Those are almost certainly from curl_errorstr() when we make the
> info/refs 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?
>
> I assume that curl is smart enough not to send any data over the wire,
> and that it is noticing early in the process that something is wrong and
> is barfing there.
>
> It would be nicer to notice earlier (when we are setting up the handle),
> but in practice I don't think it matters. We start off all http
> conversations by making a short GET, and we don't do any significant
> work beforehand. So as long as curl does not do significant work before
> hitting those errors internally, it probably does not matter much either
> way.
>
> -Peff

 Your surmise is correct here; you don't make it past the handshake.

--Jerry
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]