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

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

[Mark Lodato]

If an SSL client certificate is enabled (via http.sslcert or
GIT_SSL_CERT), prompt for the certificate password rather than
defaulting to OpenSSL's password prompt.  This causes the prompt to only
appear once each run.  Previously, OpenSSL prompted the user *many*
times, causing git to be unusable over HTTPS with client-side
certificates.

Note that the password is stored in memory in the clear while the
program is running.  This may be a security problem if git crashes and
core dumps.

The user is always prompted, even if the certificate is not encrypted.
This should be fine; unencrypted certificates are rare and a security
risk anyway.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---

See http://osdir.com/ml/git/2009-02/msg03402.html for a discussion of
this topic and an example showing how horrible the current password
prompts are.

The next patch adds an option to disable this feature.  I split it into
two commits in case the configuration option is not wanted.

I did not create any tests because the existing http.sslcert option has
no tests to begin with.

I would really like to use git over HTTPS with client certs, but the
current situation is just unusable.  So, I'm hoping this gets included
in git.git at some point.  I would be happy to hear any comments people
have about this patch series.  Thanks!


 http.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 2e3d649..1fc3444 100644
--- a/http.c
+++ b/http.c
@@ -26,6 +26,8 @@ static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static char *user_name, *user_pass;
+static char *ssl_cert_password;
+static int ssl_cert_password_required;
 
 static struct curl_slist *pragma_header;
 
@@ -167,6 +169,22 @@ static void init_curl_http_auth(CURL *result)
 	}
 }
 
+static int has_cert_password(void)
+{
+	if (ssl_cert_password != NULL)
+		return 1;
+	if (ssl_cert == NULL || ssl_cert_password_required != 1)
+		return 0;
+	/* Only prompt the user once. */
+	ssl_cert_password_required = -1;
+	ssl_cert_password = getpass("Certificate Password: ");
+	if (ssl_cert_password != NULL) {
+		ssl_cert_password = xstrdup(ssl_cert_password);
+		return 1;
+	} else
+		return 0;
+}
+
 static CURL *get_curl_handle(void)
 {
 	CURL *result = curl_easy_init();
@@ -189,6 +207,16 @@ static CURL *get_curl_handle(void)
 
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
+	if (has_cert_password())
+		curl_easy_setopt(result,
+#if LIBCURL_VERSION_NUM >= 0x071700
+				 CURLOPT_KEYPASSWD,
+#elif LIBCURL_VERSION_NUM >= 0x070903
+				 CURLOPT_SSLKEYPASSWD,
+#else
+				 CURLOPT_SSLCERTPASSWD,
+#endif
+				 ssl_cert_password);
 #if LIBCURL_VERSION_NUM >= 0x070902
 	if (ssl_key != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
@@ -329,8 +357,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
 
-	if (remote && remote->url && remote->url[0])
+	if (remote && remote->url && remote->url[0]) {
 		http_auth_init(remote->url[0]);
+		if (!prefixcmp(remote->url[0], "https://"))
+			ssl_cert_password_required = 1;
+	}
 
 #ifndef NO_CURL_EASY_DUPHANDLE
 	curl_default = get_curl_handle();
@@ -370,6 +401,13 @@ void http_cleanup(void)
 		free((void *)curl_http_proxy);
 		curl_http_proxy = NULL;
 	}
+
+	if (ssl_cert_password != NULL) {
+		memset(ssl_cert_password, 0, strlen(ssl_cert_password));
+		free(ssl_cert_password);
+		ssl_cert_password = NULL;
+	}
+	ssl_cert_password_required = 0;
 }
 
 struct active_request_slot *get_active_slot(void)
-- 
1.6.3.1

[PATCH 2/2] http.c: add http.sslCertNoPass option

[Mark Lodato]

Add a configuration option, http.sslCertNoPass, and associated
environment variable, GIT_SSL_CERT_NO_PASS, to allow disabling of the
SSL client certificate password prompt from within git.  If this option
is set to true, or if the environment variable exists, git falls back to
OpenSSL's prompts (as in earlier versions of git).

This option is useful in (at least) two cases:
1. The certificate is not encrypted and the user does not want to be
   prompted needlessly.
2. The user does not wish to leave the password in the clear in git's
   (and libcurl's) memory, in case the program crashes and core dumps.

The environment variable may only be used to disable, not to re-enable,
git's password prompt.  This behavior mimics GIT_NO_VERIFY; the mere
existence of the variable is all that is checked.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---
 Documentation/config.txt |    9 +++++++++
 http.c                   |    9 ++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2c03162..65c3ac5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1038,6 +1038,15 @@ http.sslKey::
 	over HTTPS. Can be overridden by the 'GIT_SSL_KEY' environment
 	variable.
 
+http.sslCertNoPass::
+	Disable git's password prompt for the SSL certificate.  OpenSSL
+	will still prompt the user, possibly many times, if the
+	certificate or private key is encrypted.  Useful if the
+	certificate is not encrypted (to disable the password prompt) or
+	if you do not wish to store the certificate password in git's
+	memory.  Can be overridden by the 'GIT_SSL_CERT_NO_PASS'
+	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 1fc3444..6ae59b6 100644
--- a/http.c
+++ b/http.c
@@ -131,6 +131,11 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
 	if (!strcmp("http.sslcainfo", var))
 		return git_config_string(&ssl_cainfo, var, value);
+	if (!strcmp("http.sslcertnopass", var)) {
+		if (git_config_bool(var, value))
+			ssl_cert_password_required = -1;
+		return 0;
+	}
 #ifdef USE_CURL_MULTI
 	if (!strcmp("http.maxrequests", var)) {
 		max_requests = git_config_int(var, value);
@@ -359,7 +364,9 @@ void http_init(struct remote *remote)
 
 	if (remote && remote->url && remote->url[0]) {
 		http_auth_init(remote->url[0]);
-		if (!prefixcmp(remote->url[0], "https://"))
+		if (ssl_cert_password_required == 0 &&
+		    !getenv("GIT_SSL_CERT_NO_PASS") &&
+		    !prefixcmp(remote->url[0], "https://"))
 			ssl_cert_password_required = 1;
 	}
 
-- 
1.6.3.1

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

[Mark Lodato]

Any thoughts on this?  I would love to see this in git 1.6.4, and I
don't think it affects people who do not use certificates.

~ Mark

On Wed, May 27, 2009 at 11:16 PM, Mark Lodato<lodatom@gmail.com> wrote:
> If an SSL client certificate is enabled (via http.sslcert or
> GIT_SSL_CERT), prompt for the certificate password rather than
> defaulting to OpenSSL's password prompt.  This causes the prompt to only
> appear once each run.  Previously, OpenSSL prompted the user *many*
> times, causing git to be unusable over HTTPS with client-side
> certificates.
>
> Note that the password is stored in memory in the clear while the
> program is running.  This may be a security problem if git crashes and
> core dumps.
>
> The user is always prompted, even if the certificate is not encrypted.
> This should be fine; unencrypted certificates are rare and a security
> risk anyway.
>
> Signed-off-by: Mark Lodato <lodatom@gmail.com>
> ---
>
> See http://osdir.com/ml/git/2009-02/msg03402.html for a discussion of
> this topic and an example showing how horrible the current password
> prompts are.
>
> The next patch adds an option to disable this feature.  I split it into
> two commits in case the configuration option is not wanted.
>
> I did not create any tests because the existing http.sslcert option has
> no tests to begin with.
>
> I would really like to use git over HTTPS with client certs, but the
> current situation is just unusable.  So, I'm hoping this gets included
> in git.git at some point.  I would be happy to hear any comments people
> have about this patch series.  Thanks!
>
>
>  http.c |   40 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 39 insertions(+), 1 deletions(-)
>
> diff --git a/http.c b/http.c
> index 2e3d649..1fc3444 100644
> --- a/http.c
> +++ b/http.c
> @@ -26,6 +26,8 @@ static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
>  static const char *curl_http_proxy;
>  static char *user_name, *user_pass;
> +static char *ssl_cert_password;
> +static int ssl_cert_password_required;
>
>  static struct curl_slist *pragma_header;
>
> @@ -167,6 +169,22 @@ static void init_curl_http_auth(CURL *result)
>        }
>  }
>
> +static int has_cert_password(void)
> +{
> +       if (ssl_cert_password != NULL)
> +               return 1;
> +       if (ssl_cert == NULL || ssl_cert_password_required != 1)
> +               return 0;
> +       /* Only prompt the user once. */
> +       ssl_cert_password_required = -1;
> +       ssl_cert_password = getpass("Certificate Password: ");
> +       if (ssl_cert_password != NULL) {
> +               ssl_cert_password = xstrdup(ssl_cert_password);
> +               return 1;
> +       } else
> +               return 0;
> +}
> +
>  static CURL *get_curl_handle(void)
>  {
>        CURL *result = curl_easy_init();
> @@ -189,6 +207,16 @@ static CURL *get_curl_handle(void)
>
>        if (ssl_cert != NULL)
>                curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> +       if (has_cert_password())
> +               curl_easy_setopt(result,
> +#if LIBCURL_VERSION_NUM >= 0x071700
> +                                CURLOPT_KEYPASSWD,
> +#elif LIBCURL_VERSION_NUM >= 0x070903
> +                                CURLOPT_SSLKEYPASSWD,
> +#else
> +                                CURLOPT_SSLCERTPASSWD,
> +#endif
> +                                ssl_cert_password);
>  #if LIBCURL_VERSION_NUM >= 0x070902
>        if (ssl_key != NULL)
>                curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
> @@ -329,8 +357,11 @@ void http_init(struct remote *remote)
>        if (getenv("GIT_CURL_FTP_NO_EPSV"))
>                curl_ftp_no_epsv = 1;
>
> -       if (remote && remote->url && remote->url[0])
> +       if (remote && remote->url && remote->url[0]) {
>                http_auth_init(remote->url[0]);
> +               if (!prefixcmp(remote->url[0], "https://"))
> +                       ssl_cert_password_required = 1;
> +       }
>
>  #ifndef NO_CURL_EASY_DUPHANDLE
>        curl_default = get_curl_handle();
> @@ -370,6 +401,13 @@ void http_cleanup(void)
>                free((void *)curl_http_proxy);
>                curl_http_proxy = NULL;
>        }
> +
> +       if (ssl_cert_password != NULL) {
> +               memset(ssl_cert_password, 0, strlen(ssl_cert_password));
> +               free(ssl_cert_password);
> +               ssl_cert_password = NULL;
> +       }
> +       ssl_cert_password_required = 0;
>  }
>
>  struct active_request_slot *get_active_slot(void)
> --
> 1.6.3.1
>
>

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

[Constantine Plotnikov]

How it works if git is run from IDEs (no tty will be available)?
Is there a way to redefine the way the password is got?
What about scripting scenarios where passwordless certificates are
likely to be used?

Regards,
Constantine

On Fri, Jun 5, 2009 at 6:44 AM, Mark Lodato <lodatom@gmail.com> wrote:
> Any thoughts on this?  I would love to see this in git 1.6.4, and I
> don't think it affects people who do not use certificates.
>
> ~ Mark
>
> On Wed, May 27, 2009 at 11:16 PM, Mark Lodato<lodatom@gmail.com> wrote:
>> If an SSL client certificate is enabled (via http.sslcert or
>> GIT_SSL_CERT), prompt for the certificate password rather than
>> defaulting to OpenSSL's password prompt.  This causes the prompt to only
>> appear once each run.  Previously, OpenSSL prompted the user *many*
>> times, causing git to be unusable over HTTPS with client-side
>> certificates.
>>
>> Note that the password is stored in memory in the clear while the
>> program is running.  This may be a security problem if git crashes and
>> core dumps.
>>
>> The user is always prompted, even if the certificate is not encrypted.
>> This should be fine; unencrypted certificates are rare and a security
>> risk anyway.
>>
>> Signed-off-by: Mark Lodato <lodatom@gmail.com>
>> ---
>>
>> See http://osdir.com/ml/git/2009-02/msg03402.html for a discussion of
>> this topic and an example showing how horrible the current password
>> prompts are.
>>
>> The next patch adds an option to disable this feature.  I split it into
>> two commits in case the configuration option is not wanted.
>>
>> I did not create any tests because the existing http.sslcert option has
>> no tests to begin with.
>>
>> I would really like to use git over HTTPS with client certs, but the
>> current situation is just unusable.  So, I'm hoping this gets included
>> in git.git at some point.  I would be happy to hear any comments people
>> have about this patch series.  Thanks!
>>
>>
>>  http.c |   40 +++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 39 insertions(+), 1 deletions(-)
>>
>> diff --git a/http.c b/http.c
>> index 2e3d649..1fc3444 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -26,6 +26,8 @@ static long curl_low_speed_time = -1;
>>  static int curl_ftp_no_epsv;
>>  static const char *curl_http_proxy;
>>  static char *user_name, *user_pass;
>> +static char *ssl_cert_password;
>> +static int ssl_cert_password_required;
>>
>>  static struct curl_slist *pragma_header;
>>
>> @@ -167,6 +169,22 @@ static void init_curl_http_auth(CURL *result)
>>        }
>>  }
>>
>> +static int has_cert_password(void)
>> +{
>> +       if (ssl_cert_password != NULL)
>> +               return 1;
>> +       if (ssl_cert == NULL || ssl_cert_password_required != 1)
>> +               return 0;
>> +       /* Only prompt the user once. */
>> +       ssl_cert_password_required = -1;
>> +       ssl_cert_password = getpass("Certificate Password: ");
>> +       if (ssl_cert_password != NULL) {
>> +               ssl_cert_password = xstrdup(ssl_cert_password);
>> +               return 1;
>> +       } else
>> +               return 0;
>> +}
>> +
>>  static CURL *get_curl_handle(void)
>>  {
>>        CURL *result = curl_easy_init();
>> @@ -189,6 +207,16 @@ static CURL *get_curl_handle(void)
>>
>>        if (ssl_cert != NULL)
>>                curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>> +       if (has_cert_password())
>> +               curl_easy_setopt(result,
>> +#if LIBCURL_VERSION_NUM >= 0x071700
>> +                                CURLOPT_KEYPASSWD,
>> +#elif LIBCURL_VERSION_NUM >= 0x070903
>> +                                CURLOPT_SSLKEYPASSWD,
>> +#else
>> +                                CURLOPT_SSLCERTPASSWD,
>> +#endif
>> +                                ssl_cert_password);
>>  #if LIBCURL_VERSION_NUM >= 0x070902
>>        if (ssl_key != NULL)
>>                curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
>> @@ -329,8 +357,11 @@ void http_init(struct remote *remote)
>>        if (getenv("GIT_CURL_FTP_NO_EPSV"))
>>                curl_ftp_no_epsv = 1;
>>
>> -       if (remote && remote->url && remote->url[0])
>> +       if (remote && remote->url && remote->url[0]) {
>>                http_auth_init(remote->url[0]);
>> +               if (!prefixcmp(remote->url[0], "https://"))
>> +                       ssl_cert_password_required = 1;
>> +       }
>>
>>  #ifndef NO_CURL_EASY_DUPHANDLE
>>        curl_default = get_curl_handle();
>> @@ -370,6 +401,13 @@ void http_cleanup(void)
>>                free((void *)curl_http_proxy);
>>                curl_http_proxy = NULL;
>>        }
>> +
>> +       if (ssl_cert_password != NULL) {
>> +               memset(ssl_cert_password, 0, strlen(ssl_cert_password));
>> +               free(ssl_cert_password);
>> +               ssl_cert_password = NULL;
>> +       }
>> +       ssl_cert_password_required = 0;
>>  }
>>
>>  struct active_request_slot *get_active_slot(void)
>> --
>> 1.6.3.1
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Mark Lodato]

On Fri, Jun 5, 2009 at 4:20 AM, Constantine
Plotnikov<constantine.plotnikov@gmail.com> wrote:
> How it works if git is run from IDEs (no tty will be available)?

Then this will be no worse than the current situation, which also uses
standard input to prompt for the password.  Note that a TTY is also
required if an HTTP password is requested.

> Is there a way to redefine the way the password is got?

No.  This may be nice, but it would be much more complicated to implement.

> What about scripting scenarios where passwordless certificates are
> likely to be used?

If you wish to use a client certificate without a password, then you
need the second patch in this series, which adds an option to disable
the password prompt.


Thanks for your input,
Mark

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

[Mark Lodato]

Any other thoughts, one way or the other?  Adding proper SSL/PKI
support would really help git adoption in the corporate world.  I am
willing to make any changes necessary to get this into git.git.

~ Mark

On Wed, May 27, 2009 at 11:16 PM, Mark Lodato<lodatom@gmail.com> wrote:
> If an SSL client certificate is enabled (via http.sslcert or
> GIT_SSL_CERT), prompt for the certificate password rather than
> defaulting to OpenSSL's password prompt.  This causes the prompt to only
> appear once each run.  Previously, OpenSSL prompted the user *many*
> times, causing git to be unusable over HTTPS with client-side
> certificates.
>
> Note that the password is stored in memory in the clear while the
> program is running.  This may be a security problem if git crashes and
> core dumps.
>
> The user is always prompted, even if the certificate is not encrypted.
> This should be fine; unencrypted certificates are rare and a security
> risk anyway.
>
> Signed-off-by: Mark Lodato <lodatom@gmail.com>
> ---
>
> See http://osdir.com/ml/git/2009-02/msg03402.html for a discussion of
> this topic and an example showing how horrible the current password
> prompts are.
>
> The next patch adds an option to disable this feature.  I split it into
> two commits in case the configuration option is not wanted.
>
> I did not create any tests because the existing http.sslcert option has
> no tests to begin with.
>
> I would really like to use git over HTTPS with client certs, but the
> current situation is just unusable.  So, I'm hoping this gets included
> in git.git at some point.  I would be happy to hear any comments people
> have about this patch series.  Thanks!
>
>
>  http.c |   40 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 39 insertions(+), 1 deletions(-)
>
> diff --git a/http.c b/http.c
> index 2e3d649..1fc3444 100644
> --- a/http.c
> +++ b/http.c
> @@ -26,6 +26,8 @@ static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
>  static const char *curl_http_proxy;
>  static char *user_name, *user_pass;
> +static char *ssl_cert_password;
> +static int ssl_cert_password_required;
>
>  static struct curl_slist *pragma_header;
>
> @@ -167,6 +169,22 @@ static void init_curl_http_auth(CURL *result)
>        }
>  }
>
> +static int has_cert_password(void)
> +{
> +       if (ssl_cert_password != NULL)
> +               return 1;
> +       if (ssl_cert == NULL || ssl_cert_password_required != 1)
> +               return 0;
> +       /* Only prompt the user once. */
> +       ssl_cert_password_required = -1;
> +       ssl_cert_password = getpass("Certificate Password: ");
> +       if (ssl_cert_password != NULL) {
> +               ssl_cert_password = xstrdup(ssl_cert_password);
> +               return 1;
> +       } else
> +               return 0;
> +}
> +
>  static CURL *get_curl_handle(void)
>  {
>        CURL *result = curl_easy_init();
> @@ -189,6 +207,16 @@ static CURL *get_curl_handle(void)
>
>        if (ssl_cert != NULL)
>                curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> +       if (has_cert_password())
> +               curl_easy_setopt(result,
> +#if LIBCURL_VERSION_NUM >= 0x071700
> +                                CURLOPT_KEYPASSWD,
> +#elif LIBCURL_VERSION_NUM >= 0x070903
> +                                CURLOPT_SSLKEYPASSWD,
> +#else
> +                                CURLOPT_SSLCERTPASSWD,
> +#endif
> +                                ssl_cert_password);
>  #if LIBCURL_VERSION_NUM >= 0x070902
>        if (ssl_key != NULL)
>                curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
> @@ -329,8 +357,11 @@ void http_init(struct remote *remote)
>        if (getenv("GIT_CURL_FTP_NO_EPSV"))
>                curl_ftp_no_epsv = 1;
>
> -       if (remote && remote->url && remote->url[0])
> +       if (remote && remote->url && remote->url[0]) {
>                http_auth_init(remote->url[0]);
> +               if (!prefixcmp(remote->url[0], "https://"))
> +                       ssl_cert_password_required = 1;
> +       }
>
>  #ifndef NO_CURL_EASY_DUPHANDLE
>        curl_default = get_curl_handle();
> @@ -370,6 +401,13 @@ void http_cleanup(void)
>                free((void *)curl_http_proxy);
>                curl_http_proxy = NULL;
>        }
> +
> +       if (ssl_cert_password != NULL) {
> +               memset(ssl_cert_password, 0, strlen(ssl_cert_password));
> +               free(ssl_cert_password);
> +               ssl_cert_password = NULL;
> +       }
> +       ssl_cert_password_required = 0;
>  }
>
>  struct active_request_slot *get_active_slot(void)
> --
> 1.6.3.1
>
>

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

[Nanako Shiraishi]

Quoting Mark Lodato <lodatom@gmail.com>:

> Any other thoughts, one way or the other?  Adding proper SSL/PKI
> support would really help git adoption in the corporate world.  I am
> willing to make any changes necessary to get this into git.git.

Somebody mentioned that your patch forces people to type password even when the certificate isn't encrypted. How was this issue addressed?

It would be ideal if you can inspect the certificate and decide if you need to ask for decrypting password before using it (and otherwise you don't ask). 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.

That way, people who are used to the old behavior don't have to change anything in their set-up.

If people didn't have to type password at all, and after your patch if they are forced to do something else to keep the old set-up working, that isn't nice.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

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

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

>> The user is always prompted, even if the certificate is not encrypted.
>> This should be fine; unencrypted certificates are rare and a security
>> risk anyway.

Hmm, "rare" is in the eyes of beholder.  For automated settings, I would
imagine that it is a necessary feature that we need to keep working.  Of
course the local box that keeps an unencrypted certificate used this way
must be well protected to make it _not_ a security risk, but that is not
an issue you are addressing with your patch anyway, so it is not nice to
dismiss possible usability issues like this.

>> I did not create any tests because the existing http.sslcert option has
>> no tests to begin with.

Again, not nice.  Not having tests in this particular patch may be Ok, as
long as you or other people fix that deficiency with follow-up patches,
but please don't be proud that you are following a bad example.

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

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> It would be ideal if you can inspect the certificate and decide if you
> need to ask for decrypting password before using it (and otherwise you
> don't ask). 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.

True, and true.

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

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> @@ -189,6 +207,16 @@ static CURL *get_curl_handle(void)
>  
>  	if (ssl_cert != NULL)
>  		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> +	if (has_cert_password())
> +		curl_easy_setopt(result,
> +#if LIBCURL_VERSION_NUM >= 0x071700
> +				 CURLOPT_KEYPASSWD,
> +#elif LIBCURL_VERSION_NUM >= 0x070903
> +				 CURLOPT_SSLKEYPASSWD,
> +#else
> +				 CURLOPT_SSLCERTPASSWD,
> +#endif
> +				 ssl_cert_password);

This is purely style and readability, but if you do something like this
much earlier in the file:

    #if !defined(CURLOPT_KEYPASSWD)
    # if defined(CURLOPT_SSLKEYPASSWD)
    #  define CURLOPT_KEYTPASSWD CURLOPT_SSLKEYPASSWD
    # elif defined(CURLOPT_SSLCERTPASSWD
    #  define CURLOPT_KEYTPASSWD CURLOPT_SSLCERTPASSWD
    # endif
    #endif

you can write your main codepath using the latest cURL API without ifdef.
The callsite can simply say:

	if (must_set_cert_password())
        	curl_easy_setopt(result, CURLOPT_KEYPASSWD, ssl_cert_password);

which I think would be much easier to follow.

This assumes that KEYPASSWD is the latest API, and in older versions only
names are different, which your code implies.  I have a vague recollection
that SSLCERTPASSWD actually deprecated KEYPASSWD (i.e. your #if...#endif
chain is wrong), but I didn't actually check the cURL documentation [*1*]
to see if that is the case.

[Reference]

*1* http://cool.haxx.se/cvs.cgi/curl/docs/libcurl/symbols-in-versions?rev=HEAD

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

[Daniel Stenberg]

On Fri, 12 Jun 2009, Nanako Shiraishi wrote:

> It would be ideal if you can inspect the certificate and decide if you need 
> to ask for decrypting password before using it (and otherwise you don't 
> ask). 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.

Is this really a common setup? Using an unencrypted private key sounds like a 
really bad security situation to me. The certificate is never encrupted, the 
passphrase is for the key.

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.

-- 

  / daniel.haxx.se

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

[Daniel Stenberg]

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.

-- 

  / daniel.haxx.se

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

[Constantine Plotnikov]

On Fri, Jun 12, 2009 at 11:56 AM, Daniel Stenberg<daniel@haxx.se> wrote:
> On Fri, 12 Jun 2009, Nanako Shiraishi wrote:
>
>> It would be ideal if you can inspect the certificate and decide if you
>> need to ask for decrypting password before using it (and otherwise you don't
>> ask). 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.
>
> Is this really a common setup? Using an unencrypted private key sounds like
> a really bad security situation to me. The certificate is never encrupted,
> the passphrase is for the key.
>
For SSH using unencrypted private key is very common for scripting and
cron jobs. For HTTPS situation looks like being worse since there is
no analog of ssh-agent that covers at least some of scripting
scenarios. Do we want to disable scripting for HTTPS?

Constantine

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

[Jakub Narebski]

Constantine Plotnikov <constantine.plotnikov@gmail.com> writes:
> On Fri, Jun 12, 2009 at 11:56 AM, Daniel Stenberg<daniel@haxx.se> wrote:
>> On Fri, 12 Jun 2009, Nanako Shiraishi wrote:
>>
>>> It would be ideal if you can inspect the certificate and decide if you
>>> need to ask for decrypting password before using it (and otherwise you don't
>>> ask). 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.
>>
>> Is this really a common setup? Using an unencrypted private key sounds like
>> a really bad security situation to me. The certificate is never encrupted,
>> the passphrase is for the key.
>>
> For SSH using unencrypted private key is very common for scripting and
> cron jobs. For HTTPS situation looks like being worse since there is
> no analog of ssh-agent that covers at least some of scripting
> scenarios. Do we want to disable scripting for HTTPS?

Actually you can use _encrypted_ private keys together with ssh-agent
and for example keychain helper for scripting.  You have to provide
password to all listed private keys only once at login.  I wonder if
something like this would be possible for HTTP certificates...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

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

[Rogan Dawes]

Jakub Narebski wrote:
>> For SSH using unencrypted private key is very common for scripting and
>> cron jobs. For HTTPS situation looks like being worse since there is
>> no analog of ssh-agent that covers at least some of scripting
>> scenarios. Do we want to disable scripting for HTTPS?
> 
> Actually you can use _encrypted_ private keys together with ssh-agent
> and for example keychain helper for scripting.  You have to provide
> password to all listed private keys only once at login.  I wonder if
> something like this would be possible for HTTP certificates...

I wonder if it might be possible using a PKCS#11 interface?

e.g. there are various "software" PKCS#11 implementations
(<http://trac.opendnssec.org/wiki/SoftHSM> springs to mind).

If you store your keys in the PKCS#11 store, and unlock them prior to
calling git, then the OpenSSL library might be able to access them
without a passphrase. Locking the PKCS#11 store would then secure the keys.

A little cumbersome, but possibly workable.

Rogan

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

[Mark Lodato]

Thanks for reviewing the patch.

On Thu, Jun 11, 2009 at 7:56 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>
>>> The user is always prompted, even if the certificate is not encrypted.
>>> This should be fine; unencrypted certificates are rare and a security
>>> risk anyway.
>
> Hmm, "rare" is in the eyes of beholder.  For automated settings, I would
> imagine that it is a necessary feature that we need to keep working.  Of
> course the local box that keeps an unencrypted certificate used this way
> must be well protected to make it _not_ a security risk, but that is not
> an issue you are addressing with your patch anyway, so it is not nice to
> dismiss possible usability issues like this.

Sorry about that wording - it probably is a more common case than I
imagine.  But patch 2/2 addresses this issue with an option to disable
the password prompt.  This does require one-time work for existing
users who use an unencrypted certificate, but overall I think the
patch series is a big win since encrypted certificates are not usable
at all currently.

>>> I did not create any tests because the existing http.sslcert option has
>>> no tests to begin with.
>
> Again, not nice.  Not having tests in this particular patch may be Ok, as
> long as you or other people fix that deficiency with follow-up patches,
> but please don't be proud that you are following a bad example.


Again, sorry about the wording.  I meant the above as an explanation
of why I did not include a test - I was not sure how to write one.  I
would be happy to write such a test if someone could give me some
guidance.


Thanks again!
Mark

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

[Mark Lodato]

On Fri, Jun 12, 2009 at 12:50 PM, Jakub Narebski<jnareb@gmail.com> wrote:
> Constantine Plotnikov <constantine.plotnikov@gmail.com> writes:
>> On Fri, Jun 12, 2009 at 11:56 AM, Daniel Stenberg<daniel@haxx.se> wrote:
>>> On Fri, 12 Jun 2009, Nanako Shiraishi wrote:
>>>
>>>> It would be ideal if you can inspect the certificate and decide if you
>>>> need to ask for decrypting password before using it (and otherwise you don't
>>>> ask). 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.
>>>
>>> Is this really a common setup? Using an unencrypted private key sounds like
>>> a really bad security situation to me. The certificate is never encrupted,
>>> the passphrase is for the key.
>>>
>> For SSH using unencrypted private key is very common for scripting and
>> cron jobs. For HTTPS situation looks like being worse since there is
>> no analog of ssh-agent that covers at least some of scripting
>> scenarios. Do we want to disable scripting for HTTPS?
>
> Actually you can use _encrypted_ private keys together with ssh-agent
> and for example keychain helper for scripting.  You have to provide
> password to all listed private keys only once at login.  I wonder if
> something like this would be possible for HTTP certificates...

I would love something like this - it would be useful for SVN as well.

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

[Mark Lodato]

On Fri, Jun 12, 2009 at 2:34 AM, Junio C Hamano<gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>
>> @@ -189,6 +207,16 @@ static CURL *get_curl_handle(void)
>>
>>       if (ssl_cert != NULL)
>>               curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>> +     if (has_cert_password())
>> +             curl_easy_setopt(result,
>> +#if LIBCURL_VERSION_NUM >= 0x071700
>> +                              CURLOPT_KEYPASSWD,
>> +#elif LIBCURL_VERSION_NUM >= 0x070903
>> +                              CURLOPT_SSLKEYPASSWD,
>> +#else
>> +                              CURLOPT_SSLCERTPASSWD,
>> +#endif
>> +                              ssl_cert_password);
>
> This is purely style and readability, but if you do something like this
> much earlier in the file:
>
>    #if !defined(CURLOPT_KEYPASSWD)
>    # if defined(CURLOPT_SSLKEYPASSWD)
>    #  define CURLOPT_KEYTPASSWD CURLOPT_SSLKEYPASSWD
>    # elif defined(CURLOPT_SSLCERTPASSWD
>    #  define CURLOPT_KEYTPASSWD CURLOPT_SSLCERTPASSWD
>    # endif
>    #endif
>
> you can write your main codepath using the latest cURL API without ifdef.
> The callsite can simply say:
>
>        if (must_set_cert_password())
>                curl_easy_setopt(result, CURLOPT_KEYPASSWD, ssl_cert_password);
>
> which I think would be much easier to follow.

I realized this after I submitted the patch.  Locally I have modified
my version to do something similar to the above, but checking libcurl
versions rather than checking the existence of the macros (which don't
exist, as Daniel pointed out.)  If this patch series is accepted, I
will make a cleaner version that includes this change.

Mark

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

[Mark Lodato]

On Fri, Jun 12, 2009 at 3:56 AM, Daniel Stenberg<daniel@haxx.se> wrote:
> On Fri, 12 Jun 2009, Nanako Shiraishi wrote:
>
>> It would be ideal if you can inspect the certificate and decide if you
>> need to ask for decrypting password before using it (and otherwise you don't
>> ask). 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.
>
> Is this really a common setup? Using an unencrypted private key sounds like
> a really bad security situation to me. The certificate is never encrupted,
> the passphrase is for the key.
>
> 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.)  To implement this in
libcurl, a possible solution is to always set
SSL_CTX_set_default_passwd_cb(), and have the callback function prompt
the user on the first call if CURLOPT_KEYPASSWD is not set.  If there
is interest, I could try this out and, if it works, submit a patch for
libcurl.

The upside of doing the prompting in git is that it works with old
libcurl versions... but I'm not sure this is a big deal.  Having it in
libcurl is probably better.


On Thu, Jun 11, 2009 at 7:42 PM, Nanako Shiraishi<nanako3@lavabit.com> wrote:
> Somebody mentioned that your patch forces people to type password
> even when the certificate isn't encrypted. How was this issue addressed?
>
> <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.

Patch 2/2 gives the user a way to disable this new password prompt.  I
imagine it is a more common for the certificate to be encrypted than
not, so I believe the default should be to prompt.


Mark

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

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> If this patch series is accepted, I
> will make a cleaner version that includes this change.

Sorry, but I do not understand this part of your message.

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

[Junio C Hamano]

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.

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.

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

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

[Mark Lodato]

On Fri, Jun 12, 2009 at 8:14 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>
>> If this patch series is accepted, I
>> will make a cleaner version that includes this change.
>
> Sorry, but I do not understand this part of your message.
>

Sorry about that.  I meant that I have cleaned up the code as you
suggested (see diff below), and that if you decide to include the
patch series into git.git (I see now you included it in pu), I can
either submit an additional patch to perform the cleanup, or submit a
new "v2" patch series incorporating these changes.  Is one preferred
over the other?

Also, I wasn't sure where to put the #defines; I chose to put them in
http.h, but should they go in http.c?

Thanks for the feedback!
Mark


diff --git c/http.c i/http.c
index 6ae59b6..7659ef4 100644
--- c/http.c
+++ i/http.c
@@ -213,16 +213,8 @@ static CURL *get_curl_handle(void)
        if (ssl_cert != NULL)
                curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
        if (has_cert_password())
-               curl_easy_setopt(result,
-#if LIBCURL_VERSION_NUM >= 0x071700
-                                CURLOPT_KEYPASSWD,
-#elif LIBCURL_VERSION_NUM >= 0x070903
-                                CURLOPT_SSLKEYPASSWD,
-#else
-                                CURLOPT_SSLCERTPASSWD,
-#endif
-                                ssl_cert_password);
-#if LIBCURL_VERSION_NUM >= 0x070902
+               curl_easy_setopt(result, CURLOPT_KEYPASSWD, ssl_cert_password);
+#ifndef NO_CURLOPT_SSLKEY
        if (ssl_key != NULL)
                curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
 #endif
diff --git c/http.h i/http.h
index 26abebe..b49c280 100644
--- c/http.h
+++ i/http.h
@@ -29,6 +29,12 @@
 #define curl_global_init(a) do { /* nothing */ } while(0)
 #endif

+#if LIBCURL_VERSION_NUM < 0x070903
+#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
+#elif LIBCURL_VERSION_NUM < 0x071700
+#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
+#endif
+
 #if (LIBCURL_VERSION_NUM < 0x070c04) || (LIBCURL_VERSION_NUM == 0x071000)
 #define NO_CURL_EASY_DUPHANDLE
 #endif

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

[Mark Lodato]

On Fri, Jun 12, 2009 at 8:31 PM, Junio C Hamano<gitster@pobox.com> wrote:
> 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.


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

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

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> On Fri, Jun 12, 2009 at 8:14 PM, Junio C Hamano<gitster@pobox.com> wrote:
>> Mark Lodato <lodatom@gmail.com> writes:
>>
>>> If this patch series is accepted, I
>>> will make a cleaner version that includes this change.
>>
>> Sorry, but I do not understand this part of your message.
>
> Sorry about that.  I meant that I have cleaned up the code as you
> suggested (see diff below), and that if you decide to include the
> patch series into git.git (I see now you included it in pu), I can
> either submit an additional patch to perform the cleanup, or submit a
> new "v2" patch series incorporating these changes.  Is one preferred
> over the other?

Ah, I see.

Here is how we do things around here.

Reviewers are usually faster to comment and offer improvement suggestions
than I pick up patches and apply them to my tree (in any branches).  While
a patch is under active discussion with suggestions that make the code
obviously better with simple changes, the submitter is expected to send
new "v$n" (n>=1) patches incorporating suggested improvements.  It often
is simpler and cleaner if such "replacement" patches are sent for anything
that hasn't landed on 'next' (or 'master/maint' for that matter), and I
make sure not to merge something that still has iffiness to 'next' (iow,
keeping it on 'pu') to help this process.

After the initial dust settles and reviewers agree that the patch is in a
good testable state, it lands in 'next', and if there are further
improvements and bugfixes, they are expected to be sent as incremental
patches.  That way, we do not have to record obvious shortcomings that
tend to appear in the initial submission in our history, while keeping the
record of incremental updates on top of what has been judged as "basically
sound" (aka "advances to 'next'").

So in this case, v2 is very much preferred.  There is no point recording
"Mark originally sent a code with #ifdef sprinkled heavily and then later
realized that the code becomes easier to read if #ifdef part is separated
out to only define the constants used in the code" as part of our official
history.

By the way, I forgot to say this even though I noticed you are new:
welcome to git development community.

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

[Daniel Stenberg]

On Fri, 12 Jun 2009, Junio C Hamano wrote:

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

Exactly.

Also note that (in response to Mark's other comments) you really should not go 
"beneath" libcurl, and use the SSL library directly without careful 
considerations since libcurl can be built to use one out of many SSL libs so 
it's far from sure that you're actually using OpenSSL. Or GnutTLS. Or NSS. 
Or...

If libcurl itself is not enough to solve the issue, I would probably claim 
that it would be wise to consider making libcurl support it so that the API 
remains SSL-lib agnostic to the app (git).

-- 

  / daniel.haxx.se