Re: [PATCH 1/2] http.c: prompt for SSL client certificate password

From: Mark Lodato <lodatom@gmail.com>
Date: Jun 13, 2009, 00:49 UTC
Message-ID: <ca433830906121749t2cb008b2wf72a95d275277cd9@mail.gmail.com>
In-Reply-To: <7vk53h3rey.fsf@alter.siamese.dyndns.org>

On Fri, Jun 12, 2009 at 8:31 PM, Junio C Hamano<gitster@pobox.com> wrote:

Show 16 quoted lines

> Mark Lodato <lodatom@gmail.com> writes:
>
>>> And for the libcurl not supporting this, I figure it _could_ be done by
>>> simply letting libcurl prope the remote and see if it can access it without
>>> a passphrase as that would then imply that isn't necessary.
>>>
>>> I'm not familiar enough with the code and architecture to deem how suitable
>>> such an action would be.
>>
>> I don't think it is possible to check to see if it is encrypted from
>> within git (without calling OpenSSL directly).
>
> I think what Daniel is suggesting is to attempt making a test connection
> (that does not have to have anything to do with the real object transfer)
> without passphrase to see if it fails.  If it doesn't, you know you do not
> need a passphrase to unlock the key/cert.

Hmm, I did not do this initially since I thought it was not possible without calling OpenSSL directly. If you do not set CURLOPT_KEYPASSWD, OpenSSL will prompt the user without telling the program. But now that you and Daniel mention it, I think I now believe it is possible to autodetect by setting CURLOPT_KEYPASSWD to "" during the trial connection. But is it OK to perform a trial connection that serves no other purpose? If so, I will work on creating a new patch that does this.

> While I still think that kind of automated detection would be necessary in
> the longer term (in other words, we do not necessarily have to have it in
> the initial implementation that appears in our official release), until that
> materializes, I think it is more prudent to follow the approach below.

Understood. If the above works, I see no need to go with my original patch series.

Show 8 quoted lines

>>> <snip...> If you can't do that, probably you can introduce a config var that says
>>> "this certificate is encrypted", and bypass your new code if that config var isn't set.
>
> I think I've said this already in another message, but "I break your
> working setup with my patch, but you can add this configuration to unbreak
> it" should not be done lightly, certainly without a good reason.  And the
> reason here as far as I can see is that the code chooses not to bother
> with the autodetection of encryptedness of the cert/key.  So...

Again, it wasn't that I didn't bother; it was that I thought this was not possible. If the autodetection doesn't pan out, I understand your reasoning and will change the default to be the old behavior.

Thanks again, Mark