Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
Does anybody familiar with the http codepath have comments on this?
The authoritative source
https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
tells me that the CURLOPT_* used in this patch are available since
cURL 7.9.3, but I see a #if LIBCURL_VERSION_NUM 0x070704 in http.h
so these may have to be protected in a similar way.
[the entire message unsnipped for reference]
Jerry Qassar jqas...@gmail.com writes:
curl provides many options for configuring the way it negotiates an SSL
connection (with its default OpenSSL support), including ways to define
the SSL engine used, and parameters to set the format of the key and
certificate used. Unfortunately, git does not parse some of the
critical ones needed by curl to support PKCS#11.
Add the following git config variables (and direct env-set variables):
* http.sslengine
A string variable that sets CURLOPT_SSLENGINE on the back end.
Can be set from environment using GIT_SSL_ENGINE.
* http.sslkeytype
A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE.
Can be set from environment using GIT_SSL_KEYTYPE.
* http.sslcerttype
A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE.
Can be set from environment using GIT_SSL_CERTTYPE.
Parsing these new variables combined with related patches to curl
will allow git to support native authentication with smart cards.
Note: It's difficult to test this without the related curl patches,
which I will be submitting soon. At the very least, leaving these new
options unset doesn't break anything, and setting them has little
effect without the back-end curl changes needed to 'turn on' PKCS#11.
Any suggestions would be greatly appreciated.
Signed-off-by: Jerry Qassar jqas...@gmail.com
---
Documentation/config.txt | 13 +
http.c | 36
2 files changed, 49 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c67038b..d155620 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1440,16 +1440,29 @@ http.sslVerify::
over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
variable.
+http.sslEngine::
+ String specifying the SSL engine to be used by curl. This can be used
to
+ specify non-default or dynamically loaded engines. Can be overridden by
+ the 'GIT_SSL_ENGINE' environment variable.
+
http.sslCert::
File containing the SSL certificate when fetching or pushing
over HTTPS. Can be overridden by the 'GIT_SSL_CERT' environment
variable.
+http.sslCertType::
+ Specifies the format of the certificate to curl as one of (PEM|DER|ENG).
+ Can be overridden by the 'GIT_SSL_CERTTYPE' environment variable.
+
http.sslKey::
File containing the SSL private key when fetching or pushing
over HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment
variable.
+http.sslKeyType::
+ Specifies the format of the private key to curl as one of (PEM|DER|ENG).
+ Can be overridden by the 'GIT_SSL_KEYTYPE' environment variable.
+
http.sslCertPasswordProtected::
Enable Git's password prompt for the SSL certificate. Otherwise
OpenSSL will prompt the user, possibly many times, if the
diff --git a/http.c b/http.c
index 92aba59..06cb22e 100644
--- a/http.c
+++ b/http.c
@@ -49,6 +49,10 @@ static struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
static const char *user_agent;
+static const char *ssl_keytype;
+static const char *ssl_certtype;
+static const char *ssl_engine;
+
#if LIBCURL_VERSION_NUM = 0x071700
/* Use CURLOPT_KEYPASSWD as is */
#elif LIBCURL_VERSION_NUM = 0x070903
@@ -211,6 +215,17 @@ static int http_options(const char *var, const char
*value, void *cb)
if (!strcmp(http.useragent, var))
return git_config_string(user_agent, var, value);
+ /* Adding parsing for curl options relating to engines and */
+ /* key/cert types. This is necessary if attempting to */
+ /* specify an external engine (e.g. for smartcards.) */
+
+ if (!strcmp(http.sslkeytype, var))
+ return git_config_string(ssl_keytype, var, value);
+ if (!strcmp(http.sslcerttype, var))
+ return git_config_string(ssl_certtype, var, value);
+ if (!strcmp(http.sslengine, var))
+ return git_config_string(ssl_engine, var, value);
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
}
@@ -321,6 +336,22 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
}
+ /* Adding setting of engine-related curl SSL options. */
+ if (ssl_engine != NULL) {
+ curl_easy_setopt(result, CURLOPT_SSLENGINE, ssl_engine);
+
+ /* Within the lifetime of
Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
On Tue, Apr 30, 2013 at 09:45:44AM -0700, Junio C Hamano wrote: The authoritative source https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions tells me that the CURLOPT_* used in this patch are available since cURL 7.9.3, but I see a #if LIBCURL_VERSION_NUM 0x070704 in http.h so these may have to be protected in a similar way. Yeah, I think we'd want to protect it with an #if. More on the exact version below. [the entire message unsnipped for reference] Jerry Qassar jqas...@gmail.com writes: curl provides many options for configuring the way it negotiates an SSL connection (with its default OpenSSL support), including ways to define the SSL engine used, and parameters to set the format of the key and certificate used. Unfortunately, git does not parse some of the critical ones needed by curl to support PKCS#11. I don't know anything about curl's PKCS#11 support, so I can't comment too much on how these options work. But from my reading of the curl_easy_setopt(3), these look right, and I can comment on the http.c bits. * http.sslkeytype A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE. Can be set from environment using GIT_SSL_KEYTYPE. * http.sslcerttype A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE. Can be set from environment using GIT_SSL_CERTTYPE. Parsing these new variables combined with related patches to curl will allow git to support native authentication with smart cards. Current curl seems to take ENG only for the key, and assumes you have the certificate on disk (which should be fine, as it is not a secret). I can imagine that the user experience with a smartcard is better, though, if it can use ENG to just pull the cert straight from the card. Is that what your curl patches are for (my comments below are all predicated on that assumption)? Since these options are selected from a finite set, we need to consider who is responsible for complaining to the user when they set http.sslkeytype to foobar. It probably makes sense to pass that responsibility along to curl, so that as it grows in capabilities, we do not have to keep adjusting our list (and it would already catch passing sslcerttype=ENG to versions of curl without your patches). Would we need the #if for the libcurl version to reference the new release of curl that has your patches? I think we don't, as existing versions would just complain when getting sslcerttype=ENG. IOW, it is not a build-time problem, but a run-time problem. Note: It's difficult to test this without the related curl patches, which I will be submitting soon. At the very least, leaving these new options unset doesn't break anything, and setting them has little effect without the back-end curl changes needed to 'turn on' PKCS#11. Any suggestions would be greatly appreciated. I think you'd be able to set the types to DER and test that, though the curl_easy_setopt manpage notes that a bug in OpenSSL prevents this from working. The ENG method, even with the right curl support, is probably going to be hard to test (unless there is some mock engine we can plug into OpenSSL for testing). I don't think we currently test even the basics of client certs in the test suite. A good first step might be to check that the existing sslkey and sslcert options work, followed by this patch and checking whether sslkeytype=DER works. http.sslKey:: File containing the SSL private key when fetching or pushing over HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment variable. +http.sslKeyType:: + Specifies the format of the private key to curl as one of (PEM|DER|ENG). + Can be overridden by the 'GIT_SSL_KEYTYPE' environment variable. + Setting this to ENG impacts how http.sslKey is interpreted (and of course whether http.sslEngine is used at all). We might want to mention that here. @@ -211,6 +215,17 @@ static int http_options(const char *var, const char *value, void *cb) if (!strcmp(http.useragent, var)) return git_config_string(user_agent, var, value); + /* Adding parsing for curl options relating to engines and */ + /* key/cert types. This is necessary if attempting to */ + /* specify an external engine (e.g. for smartcards.) */ /* * Style nit: our multi-line comments usually look * like this. No right-hand border, and no text on * the top or bottom lines. */ + if (!strcmp(http.sslkeytype, var)) + return git_config_string(ssl_keytype, var, value); + if (!strcmp(http.sslcerttype, var)) + return git_config_string(ssl_certtype, var, value); + if (!strcmp(http.sslengine, var)) + return git_config_string(ssl_engine, var, value); We aren't checking the validity of the type fields here, which I think is OK. But then later, we do: + if (ssl_keytype != NULL) + curl_easy_setopt(result,
Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
On Tue, Apr 30, 2013 at 11:27 AM, Jeff King p...@peff.net wrote: On Tue, Apr 30, 2013 at 09:45:44AM -0700, Junio C Hamano wrote: The authoritative source https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions tells me that the CURLOPT_* used in this patch are available since cURL 7.9.3, but I see a #if LIBCURL_VERSION_NUM 0x070704 in http.h so these may have to be protected in a similar way. Yeah, I think we'd want to protect it with an #if. More on the exact version below. 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 entire message unsnipped for reference] Jerry Qassar jqas...@gmail.com writes: curl provides many options for configuring the way it negotiates an SSL connection (with its default OpenSSL support), including ways to define the SSL engine used, and parameters to set the format of the key and certificate used. Unfortunately, git does not parse some of the critical ones needed by curl to support PKCS#11. I don't know anything about curl's PKCS#11 support, so I can't comment too much on how these options work. But from my reading of the curl_easy_setopt(3), these look right, and I can comment on the http.c bits. * http.sslkeytype A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLKEYTYPE. Can be set from environment using GIT_SSL_KEYTYPE. * http.sslcerttype A string variable, either PEM/DER/ENG, that sets CURLOPT_SSLCERTTYPE. Can be set from environment using GIT_SSL_CERTTYPE. Parsing these new variables combined with related patches to curl will allow git to support native authentication with smart cards. Current curl seems to take ENG only for the key, and assumes you have the certificate on disk (which should be fine, as it is not a secret). I can imagine that the user experience with a smartcard is better, though, if it can use ENG to just pull the cert straight from the card. Is that what your curl patches are for (my comments below are all predicated on that assumption)? The concomitant curl patch(es; I need to separate them) are mentioned here: http://curl.haxx.se/mail/lib-2013-04/0336.html 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. On git's side we just need to expose the proper options for setopt configuration. No special work is needed to support them otherwise. Since these options are selected from a finite set, we need to consider who is responsible for complaining to the user when they set http.sslkeytype to foobar. It probably makes sense to pass that responsibility along to curl, so that as it grows in capabilities, we do not have to keep adjusting our list (and it would already catch passing sslcerttype=ENG to versions of curl without your patches). Would we need the #if for the libcurl version to reference the new release of curl that has your patches? I think we don't, as existing versions would just complain when getting sslcerttype=ENG. IOW, it is not a build-time problem, but a run-time problem. I can answer this for you as I runtime-tested this with much swearing after making the initial git changes. :) It's curl's responsibility to parse what it gets from the setopt calls and shoot errors back--for example, when I fixed the conf-reading and the git option-setting but not the handle cloning, I would get: error: crypto engine not set, can't load certificate while accessing ... git doesn't do any validation of any curl options that I see, other than type matching. For example, git doesn't check the value of ssl_key itself, it just passes it on. Note: It's difficult to test this without the related curl patches, which I will be submitting soon. At the very least, leaving these new options unset doesn't break anything, and setting them has little effect without the back-end curl changes needed to 'turn on' PKCS#11. Any suggestions would be greatly appreciated. I think you'd be able to set the types to DER and test that, though the curl_easy_setopt manpage notes that a bug in OpenSSL prevents this from working. The ENG method, even with the right curl support, is probably going to be hard to test (unless there is some mock engine we can plug into OpenSSL for testing). I don't think we currently test even the basics of client certs in the test suite. A good first step might be to check that the existing sslkey and sslcert options work, followed by this patch and checking whether sslkeytype=DER works. 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
Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
Jerry Qassar jqas...@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... 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. + 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? 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote: Jerry Qassar jqas...@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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http.c: Add config options/parsing for SSL engine vars
On Tue, Apr 30, 2013 at 1:29 PM, Jeff King p...@peff.net wrote: On Tue, Apr 30, 2013 at 01:17:03PM -0700, Junio C Hamano wrote: Jerry Qassar jqas...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
