[PATCH 1/2] http.c: fix compiling with libcurl 7.9.2

Change the minimimum required libcurl version for the http.sslKey option
to 7.9.3.  Previously, preprocessor macros checked for >= 7.9.2, which
is incorrect because CURLOPT_SSLKEY was introduced in 7.9.3.  This now
allows git to compile with libcurl 7.9.2.

Signed-off-by: Mark Lodato <redacted>
---

This patch series is independent of my other password prompting patch
series, and is based off 'next', which includes Tay Ray Chuan's recent
http changes.

Note that git still does not compile on libcurl before 7.9.1 or below,
since CURLOPT_FTP_USE_EPSV (http.c:236) is defined in libcurl 7.9.2.

One question: In http.c, there are unnecessary #if LIBCURL_VERSION_NUM's
surrounding the global variable declarations, in http_options(), and in
http_init().  Is there a reason why these exist?  If not, I think
removing them would make the code easier to read.

Any feedback or suggestions are appreciated!
Mark


 http.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 95b2137..b049948 100644
--- a/http.c
+++ b/http.c

@@ -20,7 +20,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
 
 static int curl_ssl_verify = -1;
 static const char *ssl_cert;
-#if LIBCURL_VERSION_NUM >= 0x070902
+#if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908

@@ -126,7 +126,7 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
-#if LIBCURL_VERSION_NUM >= 0x070902
+#if LIBCURL_VERSION_NUM >= 0x070903
 	if (!strcmp("http.sslkey", var))
 		return git_config_string(&ssl_key, var, value);
 #endif

@@ -196,7 +196,7 @@ static CURL *get_curl_handle(void)
 
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
-#if LIBCURL_VERSION_NUM >= 0x070902
+#if LIBCURL_VERSION_NUM >= 0x070903
 	if (ssl_key != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
 #endif

@@ -313,7 +313,7 @@ void http_init(struct remote *remote)
 		curl_ssl_verify = 0;
 
 	set_from_env(&ssl_cert, "GIT_SSL_CERT");
-#if LIBCURL_VERSION_NUM >= 0x070902
+#if LIBCURL_VERSION_NUM >= 0x070903
 	set_from_env(&ssl_key, "GIT_SSL_KEY");
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908

-- 
1.6.3.2

Add two new configuration variables, http.sslCertType and
http.sslKeyType, which tell libcurl the filetype for the SSL client
certificate and private key, respectively.  The main benefit is to allow
PKCS12 certificates for users with libcurl >= 7.13.0.

Signed-off-by: Mark Lodato <redacted>
---

Unfortunately, P12 support in libcurl is not great, so encrypted P12
certificates do not work at all.  At least now unencrypted certificates
are possible.  Hopefully, my password prompting patch series (once I
finish it) will resolve this issue.

As always, any feedback on this patch is appreciated.  In particular, I
welcome suggestions for improving the documentation phrasing.

 Documentation/config.txt |   10 ++++++++++
 http.c                   |   12 ++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2fecbe3..b19a923 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1038,11 +1038,21 @@ http.sslCert::
 	over HTTPS. Can be overridden by the 'GIT_SSL_CERT' environment
 	variable.
 
+http.sslCertType::
+	Filetype for SSL certificate.  Must be "PEM" (default), "DER", or
+	(if libcurl >= 7.13.0) "P12".  Can be overridden by the
+	'GIT_SSL_CERT_TYPE' 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::
+	Filetype for SSL private key.  Must be "PEM" (default), "DER", or
+	(if libcurl >= 7.13.0) "P12".  Can be overridden by the
+	'GIT_SSL_CERT_TYPE' environment variable.
+
 http.sslCAInfo::
 	File containing the certificates to verify the peer with when
 	fetching or pushing over HTTPS. Can be overridden by the

diff --git a/http.c b/http.c
index b049948..5716e4e 100644
--- a/http.c
+++ b/http.c

@@ -22,6 +22,8 @@ static int curl_ssl_verify = -1;
 static const char *ssl_cert;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
+static const char *ssl_cert_type;
+static const char *ssl_key_type;
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
 static const char *ssl_capath;

@@ -129,6 +131,10 @@ static int http_options(const char *var, const char *value, void *cb)
 #if LIBCURL_VERSION_NUM >= 0x070903
 	if (!strcmp("http.sslkey", var))
 		return git_config_string(&ssl_key, var, value);
+	if (!strcmp("http.sslcerttype", var))
+		return git_config_string(&ssl_cert_type, var, value);
+	if (!strcmp("http.sslkeytype", var))
+		return git_config_string(&ssl_key_type, var, value);
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
 	if (!strcmp("http.sslcapath", var))

@@ -199,6 +205,10 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x070903
 	if (ssl_key != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
+	if (ssl_cert_type != NULL)
+		curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);
+	if (ssl_key_type != NULL)
+		curl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_key_type);
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
 	if (ssl_capath != NULL)

@@ -315,6 +325,8 @@ void http_init(struct remote *remote)
 	set_from_env(&ssl_cert, "GIT_SSL_CERT");
 #if LIBCURL_VERSION_NUM >= 0x070903
 	set_from_env(&ssl_key, "GIT_SSL_KEY");
+	set_from_env(&ssl_cert, "GIT_SSL_CERT_TYPE");
+	set_from_env(&ssl_key, "GIT_SSL_KEY_TYPE");
 #endif
 #if LIBCURL_VERSION_NUM >= 0x070908
 	set_from_env(&ssl_capath, "GIT_SSL_CAPATH");

-- 
1.6.3.2

Hi Mark!

On Sun, 14 Jun 2009, Mark Lodato wrote:

Add two new configuration variables, http.sslCertType and
http.sslKeyType, which tell libcurl the filetype for the SSL client
certificate and private key, respectively.  The main benefit is to allow
PKCS12 certificates for users with libcurl >= 7.13.0.

This is interesting. Thanks for working on that!

(However, it's a similar issue like the question whether the private key 
is encrypted or not: Usability would be better if the certificate type 
could be determined automatically (without having to violate the 
layering)).

quoted

+http.sslKeyType::

+	Filetype for SSL private key.  Must be "PEM" (default), "DER", or
+	(if libcurl >= 7.13.0) "P12".  Can be overridden by the
+	'GIT_SSL_CERT_TYPE' environment variable.

                  ^^^^
                  KEY

Regards,
Karsten

On Mon, Jun 15, 2009 at 1:43 PM, Karsten Weiss[off-list ref] wrote:

Hi Mark!

On Sun, 14 Jun 2009, Mark Lodato wrote:

quoted

Add two new configuration variables, http.sslCertType and
http.sslKeyType, which tell libcurl the filetype for the SSL client
certificate and private key, respectively.  The main benefit is to allow
PKCS12 certificates for users with libcurl >= 7.13.0.

This is interesting. Thanks for working on that!

(However, it's a similar issue like the question whether the private key is
encrypted or not: Usability would be better if the certificate type could be
determined automatically (without having to violate the layering)).

Just as with determining if the certificate is password protected, it
is equally difficult to tell what type of file it is without calling
OpenSSL directly.

This brings up a good point: Should we (I) try to implement (client
certificate) usability features in git to work around deficiencies in
libcurl, or should we (I) write patches to fix/enhance libcurl
directly?  The latter would be much easier (though I could be wrong)
and would benefit other programs using libcurl, but would require
users to upgrade libcurl to get these new features, and of course
would rely on the libcurl developers accepting the patches.  I am
willing to do either, but I think the libcurl route would be better.
Any thoughts?


Anyway, to implement this in git, the algorithm would be something like:

for password in [None, "", prompt()]:
 for type in ["PEM", "DER", (if libcurl >= 7.13.0) "P12"]:
  try to make a connection with password and type
  if not certificate error:
   return success
else:
 return failure

This is much more difficult than it may at first appear.  I'm sure it
can be done, but it will take a while to get it right.


Mark

On Mon, Jun 15, 2009 at 1:43 PM, Karsten Weiss[off-list ref] wrote:

quoted

quoted

+http.sslKeyType::

+       Filetype for SSL private key.  Must be "PEM" (default), "DER", or
+       (if libcurl >= 7.13.0) "P12".  Can be overridden by the
+       'GIT_SSL_CERT_TYPE' environment variable.

                ^^^^
                KEY

Whoops - thanks.  Sorry for that typo.

Mark

Mark Lodato [off-list ref] writes:

This brings up a good point: Should we (I) try to implement (client
certificate) usability features in git to work around deficiencies in
libcurl, or should we (I) write patches to fix/enhance libcurl
directly?  The latter would be much easier (though I could be wrong)
and would benefit other programs using libcurl, but would require
users to upgrade libcurl to get these new features, and of course
would rely on the libcurl developers accepting the patches.  I am
willing to do either, but I think the libcurl route would be better.
Any thoughts?

I agree that would be a better approach in the longer term.  There is no
point in many projects that use libcURL reinventing the wheel that could
be in the shared library.

Perhaps we could do both ;-).

That is, (1) give libcURL a way to allow callers ask if the key/cert is
encrypted, and then (2) on git side we only add code to ask libcURL using
that interface _only if and when available_; otherwise we do not even try
to bypass layers but just ask the user to tell us via configuration (or
command line).

On Mon, 15 Jun 2009, Mark Lodato wrote:

quoted

(However, it's a similar issue like the question whether the private key is
encrypted or not: Usability would be better if the certificate type could be
determined automatically (without having to violate the layering)).

Just as with determining if the certificate is password protected, it
is equally difficult to tell what type of file it is without calling
OpenSSL directly.

Hm, thinking about the encryption case: Maybe I'm missing something but 
wouldn't it be enough to simply peek at the key file and look for the 
string "ENCRYPTED" in a header like this?

-----BEGIN RSA PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED

I.e. a simple, temporary solution that does not depend on OpenSSL to 
prevent the introduction of the new http.sslCertNoPass flag?

(But now that you've also created patches for PKCS12 support this might 
not be feasible anymore?)

This brings up a good point: Should we (I) try to implement (client
certificate) usability features in git to work around deficiencies in
libcurl, or should we (I) write patches to fix/enhance libcurl
directly?  The latter would be much easier (though I could be wrong)
and would benefit other programs using libcurl, but would require
users to upgrade libcurl to get these new features, and of course
would rely on the libcurl developers accepting the patches.  I am
willing to do either, but I think the libcurl route would be better.
Any thoughts?

(As a git user without libcurl insights) I think that such query functions 
about private keys (Is it encrypted?) or certificates (What type is it?) 
would make sense and belong into libcurl. (And it would be great if these 
queries could be answered *without* performing actual trial network 
connections just by looking into the respective key/certificate files.)

Karsten