patch, 2 partshttp.c: add http.sslCertType and http.sslKeyType

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

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

Hi Mark!

On Sun, 14 Jun 2009, Mark Lodato wrote:

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

                  ^^^^
                  KEY

Regards, Karsten

On Mon, Jun 15, 2009 at 1:43 PM, Karsten Weiss<knweiss@gmx.de> wrote:

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

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

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

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

On Mon, 15 Jun 2009, Mark Lodato wrote:

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

(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

On Mon, Jun 15, 2009 at 1:43 PM, Karsten Weiss<knweiss@gmx.de> wrote:

Whoops - thanks.  Sorry for that typo.

Mark

Mark Lodato <lodatom@gmail.com> writes:

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?

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.

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.

Hi,

On Mon, Jun 15, 2009 at 12:35 PM, Junio C Hamano<gitster@pobox.com> wrote:

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

2009/6/15 Junio C Hamano <gitster@pobox.com>

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

Mike