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

On Tue, Apr 30, 2013 at 1:29 PM, Jeff King [off-list ref] wrote:

On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote:

quoted

Jerry Qassar [off-list ref] writes:

quoted

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?

quoted

quoted

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.

quoted

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