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.
On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote:
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.
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".
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.
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
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