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

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

[Mark Lodato]

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 <lodatom@gmail.com>
---

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

[PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Mark Lodato]

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 <lodatom@gmail.com>
---

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

Re: [PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Karsten Weiss]

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)).

>> +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

Re: [PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Mark Lodato]

On Mon, Jun 15, 2009 at 1:43 PM, Karsten Weiss<knweiss@gmx.de> wrote:
> 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)).

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

Re: [PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> 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).

Re: [PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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).

I guess I somewhat misread what you were saying (and I seem to be doing
this more often recently---I should slow down).

For key/cert type, the current cURL interface you used expects the caller
to say "I am giving you the name of the cert file, and the file is of this
type".  I think the usability enhancement would be something like "Here is
the cert file; it should be one of the types supported by you (I do not
know nor care what exact type it is, but the end user tells me that you
should be able to use it).  Please do whatever necessary with it."

For key/cert passphrase, the current cURL interface we use expects the
caller to give a string value via setopt.  I wonder if there already is an
existing interface to give a callback function that is responsible for
doing user interaction and return a string?  The best case would be to use
such an interface if available; otherwise, it would be good to add such an
interface to libcURL for us and other people to use.

I imagine the user would look something like this:

	static char *ssl_cert_password;
	static const char *callback(const char *hint, int trial, void *cb)
        {
		char buf[256];
                if (!trial)
                	return ssl_cert_password ? ssl_cert_password : "";
		if (5 < trial) {
			error("Wrong passphrase. Giving up...");
			return NULL;
                }
		sprintf(buf, "Passphrase to unlock %s: ", hint);
                ssl_cert_password = getpass(buf);
                return ssl_cert_password;
        }

where

 (1) The calling program (i.e. us) sets the address of the callback
     function via curl_easy_setopt() when registering a key/cert file;

 (2) When libcURL needs to unlock the key/cert file and sees such a
     callback registered, it is called with "hint" (probably the filename
     of the key/cert file it is trying to unlock), trial count (initially
     zero) and an arbitrary callback data the program passed when it
     registered the callback in the step (1).  The callback is expected to
     return a passphrase string.

 (3) The callback can return an empty string to tell the library to try
     using an empty passphrase (aka unencrypted keyfile).

 (4) If the returned string does not unlock the key/cert file
     successfully, libcURL is expected to call the callback with the same
     hint/cb but trial count incremented.  The callback can return NULL to
     signal that the user/program gave up.

That way, we wouldn't even have to make a "trial connection" we earlier
discussed.  The first request to make a connection we make into the
library can also serve as the "trial connection".

Re: [PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Karsten Weiss]

On Mon, 15 Jun 2009, Mark Lodato wrote:

>> (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

Re: [PATCH 2/2] http.c: add http.sslCertType and http.sslKeyType

[Mark Lodato]

On Mon, Jun 15, 2009 at 1:43 PM, Karsten Weiss<knweiss@gmx.de> wrote:
>>> +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

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

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> 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 <lodatom@gmail.com>
> ---
>
> 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.

In other words, this needs to be queued on top of rc/http-push series, and
the review process should involve the original author (Cc'ed).

Tay, comments?

> 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.

I think we didn't quite follow an old thread through, then.  

Cf. http://thread.gmane.org/gmane.comp.version-control.git/113985/focus=114014

Both Mike's in the thread Cc'ed.

> 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.

Yeah, as long as get_curl_handle() is still protected not to call
curl_easy_setopt() with an option that is unknown to the version of
libcURL, I think the config reader and variable declarations, and
definitions can lose conditional compilation and it would make the overall
code easier to read.

Thanks.

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

[Tay Ray Chuan]

Hi,

On Mon, Jun 15, 2009 at 12:35 PM, Junio C Hamano<gitster@pobox.com> wrote:
> In other words, this needs to be queued on top of rc/http-push series, and
> the review process should involve the original author (Cc'ed).
>
> Tay, comments?

Thanks for the heads-up. I don't have anything to add, since Mark's
work doesn't really affect mine (http fetching logic).

-- 
Cheers,
Ray Chuan

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

[Mike Ralphson]

2009/6/15 Junio C Hamano <gitster@pobox.com>
>
> Mark Lodato <lodatom@gmail.com> writes:
> > 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.
>
> I think we didn't quite follow an old thread through, then.
>
> Cf. http://thread.gmane.org/gmane.comp.version-control.git/113985/focus=114014
>
> Both Mike's in the thread Cc'ed.

Yep, apologies for having dropped the ball on this. I had got back to
it but parked it again while Ray Chaun's series was in flight.

Will be offline for a couple of weeks around solstice / Glastonbury
but able to pick it up again after that if no-one beats me to it. I've
noted Daniel's point below also:

2009/6/12 Daniel Stenberg <daniel@haxx.se>:
> On Thu, 11 Jun 2009, Junio C Hamano wrote:
>
>>   #if !defined(CURLOPT_KEYPASSWD)
>>   # if defined(CURLOPT_SSLKEYPASSWD)
>>   #  define CURLOPT_KEYTPASSWD CURLOPT_SSLKEYPASSWD
>>   # elif defined(CURLOPT_SSLCERTPASSWD
>>   #  define CURLOPT_KEYTPASSWD CURLOPT_SSLCERTPASSWD
>>   # endif
>>   #endif
>
> Just note that these CURLOPT_* symbols provided by libcurl are enums, not
> defines, so unfortunately you can't do it this exact #ifdef way.

Mike