[PATCH] add support for specifying an SSL cipher list

[PATCH] add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

Teach git about a new option, "http.sslCipherList", which permits one to
specify a list of ciphers to use when negotiating SSL connections.  The
setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
variable.

Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
---

I was recently helping someone diagnose the following error when
trying to clone a remote repository:

  fatal: unable to access 'https://example.org/': Cannot communicate
  securely with peer: no common encryption algorithm(s).

This happens when the remote server and the default libcurl
configuration do not share any ciphers in common.  In this particular
case the solution was to add 'ecdhe_ecdsa_aes_128_gcm_sha_256' to the
list of ciphers via CURLOPT_SSL_CIPHER_LIST.  This patch permits one
to make such a configuration change in git.

 Documentation/config.txt |  7 +++++++
 http.c                   | 11 +++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..b17985c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1560,6 +1560,13 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.sslCipherList::
+  A list of SSL ciphers to use when negotiating an SSL connection.
+  The available ciphers depend on whether libcurl was built against
+  NSS or OpenSSL and the particular configuration of the crypto
+  library in use.  Can be overwridden by the 'GIT_SSL_CIPHER_LIST'
+  environment variable.
+
 http.sslVerify::
 	Whether to verify the SSL certificate when fetching or pushing
 	over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
diff --git a/http.c b/http.c
index 4b179f6..8077f8d 100644
--- a/http.c
+++ b/http.c
@@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
+static const char *ssl_cipherlist;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslcipherlist", var)) {
+		return git_config_string(&ssl_cipherlist, var, value);
+	}
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
@@ -361,6 +365,13 @@ static CURL *get_curl_handle(void)
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
+	if (getenv("GIT_SSL_CIPHER_LIST"))
+		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
+
+	if (ssl_cipherlist != NULL)
+		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
+				ssl_cipherlist);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-- 
2.4.0

Re: [PATCH] add support for specifying an SSL cipher list

[Junio C Hamano]

Lars Kellogg-Stedman <lars@redhat.com> writes:

> Teach git about a new option, "http.sslCipherList", which permits one to
> specify a list of ciphers to use when negotiating SSL connections.  The
> setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
> variable.
>
> Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
> ---
>
> I was recently helping someone diagnose the following error when
> trying to clone a remote repository:
>
>   fatal: unable to access 'https://example.org/': Cannot communicate
>   securely with peer: no common encryption algorithm(s).
>
> This happens when the remote server and the default libcurl
> configuration do not share any ciphers in common.  In this particular
> case the solution was to add 'ecdhe_ecdsa_aes_128_gcm_sha_256' to the
> list of ciphers via CURLOPT_SSL_CIPHER_LIST.  This patch permits one
> to make such a configuration change in git.
>
>  Documentation/config.txt |  7 +++++++
>  http.c                   | 11 +++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..b17985c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1560,6 +1560,13 @@ http.saveCookies::
>  	If set, store cookies received during requests to the file specified by
>  	http.cookieFile. Has no effect if http.cookieFile is unset.
>  
> +http.sslCipherList::
> +  A list of SSL ciphers to use when negotiating an SSL connection.
> +  The available ciphers depend on whether libcurl was built against
> +  NSS or OpenSSL and the particular configuration of the crypto
> +  library in use.  Can be overwridden by the 'GIT_SSL_CIPHER_LIST'
> +  environment variable.

It is not clear to me what definition of "override" this sentence
uses.  If you set something to this configuration variable, and if
you want to revert the list back to whatever cURL uses by default,
what exact value should I set GIT_SSL_CIPHER_LIST to?  Do I have to
find out the list of cipher suites cURL uses by default from the doc
and list them all in the correct order, or can I merely set it to an
empty string, i.e.

	$ GIT_SSL_CIPHER_LIST= git fetch ...

or what?

I also wonder if this feature is something we would want a test or
two to protect against future changes accidentally breaking it, but
I do not offhand know how hard it would be to come up with a
reasonable test.

Thanks.

> diff --git a/http.c b/http.c
> index 4b179f6..8077f8d 100644
> --- a/http.c
> +++ b/http.c
> @@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
>  static int curl_ssl_verify = -1;
>  static int curl_ssl_try;
>  static const char *ssl_cert;
> +static const char *ssl_cipherlist;
>  #if LIBCURL_VERSION_NUM >= 0x070903
>  static const char *ssl_key;
>  #endif
> @@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
>  		curl_ssl_verify = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp("http.sslcipherlist", var)) {
> +		return git_config_string(&ssl_cipherlist, var, value);
> +	}
>  	if (!strcmp("http.sslcert", var))
>  		return git_config_string(&ssl_cert, var, value);
>  #if LIBCURL_VERSION_NUM >= 0x070903
> @@ -361,6 +365,13 @@ static CURL *get_curl_handle(void)
>  	if (http_proactive_auth)
>  		init_curl_http_auth(result);
>  
> +	if (getenv("GIT_SSL_CIPHER_LIST"))
> +		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> +
> +	if (ssl_cipherlist != NULL)
> +		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> +				ssl_cipherlist);
> +
>  	if (ssl_cert != NULL)
>  		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>  	if (has_cert_password())

Re: [PATCH] add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

[Apologies for the dupe; this should have been cc'd to the list]

> It is not clear to me what definition of "override" this sentence
> uses.

I was using it in what I thought was the common sense of "git will use
the value in the environment variable if it exists rather than any
value in the git configuration".  I apologize if this wasn't clear;
can you suggest how I might rephrase that?

> If you set something to this configuration variable, and if
> you want to revert the list back to whatever cURL uses by default,
> what exact value should I set GIT_SSL_CIPHER_LIST to?

So, with the current version of the patch there isn't an easy way to
say, "use the defaults instead of what is in my git configuration".
Setting GIT_SSL_CIPHER_LIST to an empty string would simply disable
SSL.

I'll submit a new version of the patch that treats an emtpy cipher
list as meaning, "do not explicitly set CURLOPT_SSL_CIPHER_LIST".

> I also wonder if this feature is something we would want a test or
> two to protect against future changes accidentally breaking it, but
> I do not offhand know how hard it would be to come up with a
> reasonable test.

Yeah, I looked briefly through the tests but I didn't see any existing
SSL tests and wasn't sure where to start.  I'm open to suggestions on
this front.

-- 
Lars Kellogg-Stedman <lars@redhat.com> | larsks @ {freenode,twitter,github}
Cloud Engineering / OpenStack          | http://blog.oddbit.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2] http: add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

Teach git about a new option, "http.sslCipherList", which permits one to
specify a list of ciphers to use when negotiating SSL connections.  The
setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
variable.

Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
---
 Documentation/config.txt |  8 ++++++++
 http.c                   | 11 +++++++++++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..b0af723 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1560,6 +1560,14 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.sslCipherList::
+  A list of SSL ciphers to use when negotiating an SSL connection.
+  The available ciphers depend on whether libcurl was built against
+  NSS or OpenSSL and the particular configuration of the crypto
+  library in use.  Can be overwridden by the 'GIT_SSL_CIPHER_LIST'
+  environment variable.  To force git to use libcurl's default cipher
+  list, set GIT_SSL_CIPHER_LIST to the empty string.
+
 http.sslVerify::
 	Whether to verify the SSL certificate when fetching or pushing
 	over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
diff --git a/http.c b/http.c
index 4b179f6..55adff1 100644
--- a/http.c
+++ b/http.c
@@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
+static const char *ssl_cipherlist;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslcipherlist", var)) {
+		return git_config_string(&ssl_cipherlist, var, value);
+	}
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
@@ -361,6 +365,13 @@ static CURL *get_curl_handle(void)
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
+	if (getenv("GIT_SSL_CIPHER_LIST"))
+		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
+
+	if (ssl_cipherlist != NULL && ssl_cipherlist[0] != '\0')
+		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
+				ssl_cipherlist);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-- 
2.4.0

Re: [PATCH] add support for specifying an SSL cipher list

[Junio C Hamano]

Lars Kellogg-Stedman <lars@redhat.com> writes:

> [Apologies for the dupe; this should have been cc'd to the list]
>
>> It is not clear to me what definition of "override" this sentence
>> uses.
>
> I was using it in what I thought was the common sense of "git will use
> the value in the environment variable if it exists rather than any
> value in the git configuration".  I apologize if this wasn't clear;
> can you suggest how I might rephrase that?

I was hinting that the usual "override" that needs to specify
the list to be used exactly would not be very useful, in that
people often want to say one of the three things:

 - allow this to be used in addition to what you usually use; or

 - what you usually use is fine, but never use this one as it was
   recently discovered to be insecure; or

 - I have something nonstandard configured but ignore that
   configuration for this invocation only and reset to the default
   behaviour.

If you are changing the behaviour in your reroll, I suspect you
wouldn't be doing the common "override".  If you are going to do the
'reset on empty', then 'You can set the environment variable to an
empty string to reset to the default cipher list used by libcURL.'
may be a natural way to describe it.

I briefly wondered if lack of the other two ("allow this too",
"forbid this") might become an issue not just for the environment,
but also for the configuration variable.  It is probably not a huge
issue because you can say "http.<url>.sslCipherList" to limit the
scope of the affected site [*1*].

CURLOPT_SSL_CIPHER_LIST appeared in cURL 7.9 which is relatively
ancient, so it should be safe to use (please write that down in your
commit log message).

Thanks.


[Footnote]

*1* And it is a bad idea to address "allow this too" and "forbid
    this" at our level---the semantics of CURLOPT_SSL_CIPHER_LIST
    given by libcURL itself depends on the crypto backend (when
    using OpenSSL and GnuTLS, you can say !, +, - to tweak; when
    using NSS, you can only say "use these and nothing else").

Re: [PATCH] add support for specifying an SSL cipher list

[Tay Ray Chuan]

On Thu, May 7, 2015 at 10:16 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..b17985c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1560,6 +1560,13 @@ http.saveCookies::
>         If set, store cookies received during requests to the file specified by
>         http.cookieFile. Has no effect if http.cookieFile is unset.
>
> +http.sslCipherList::
> +  A list of SSL ciphers to use when negotiating an SSL connection.
> +  The available ciphers depend on whether libcurl was built against
> +  NSS or OpenSSL and the particular configuration of the crypto
> +  library in use.  Can be overwridden by the 'GIT_SSL_CIPHER_LIST'
> +  environment variable.
> +
>  http.sslVerify::
>         Whether to verify the SSL certificate when fetching or pushing
>         over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment

You might want to mention the libcurl option that this conf
corresponds to, so that a reader could go look it up in the libcurl
documentation to get an idea of the ciphers available, and list syntax
to be used that would be accepted by us (but really by libcurl). But
we also don't have to go as far as reproducing it here (eg. ciphers
separated by colons) since this it tied to the libcurl version the
user is linking against.

-- 
Cheers,
Ray Chuan

Re: [PATCH] add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Fri, May 08, 2015 at 12:42:02AM +0800, Tay Ray Chuan wrote:
> You might want to mention the libcurl option that this conf
> corresponds to, so that a reader could go look it up in the libcurl
> documentation to get an idea of the ciphers available...

I actually removed references to the specific option before submitting
the patch, because none of the other settings that affect curl options
explicitly document the corresponding curl option name.  But I am
happy to add it back.

> to be used that would be accepted by us (but really by libcurl). But
> we also don't have to go as far as reproducing it here (eg. ciphers
> separated by colons) since this it tied to the libcurl version the
> user is linking against.

Right, I think that documenting the curl option is sufficient, and
then people can consult the libcurl documentation if they need
details.

-- 
Lars Kellogg-Stedman <lars@redhat.com> | larsks @ {freenode,twitter,github}
Cloud Engineering / OpenStack          | http://blog.oddbit.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Thu, May 07, 2015 at 09:33:01AM -0700, Junio C Hamano wrote:
> If you are changing the behaviour in your reroll, I suspect you
> wouldn't be doing the common "override".  If you are going to do the
> 'reset on empty', then 'You can set the environment variable to an
> empty string to reset to the default cipher list used by libcURL.'
> may be a natural way to describe it.

Thanks for your comments.  I will work on rephrasing things a bit.

-- 
Lars Kellogg-Stedman <lars@redhat.com> | larsks @ {freenode,twitter,github}
Cloud Engineering / OpenStack          | http://blog.oddbit.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3] http: add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

Teach git about a new option, "http.sslCipherList", which permits one to
specify a list of ciphers to use when negotiating SSL connections.  The
setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
variable.

Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
---

This addresses (I hope!) comments from Junio and Ray, and also resolves some
whitespace issues present in the earlier version of the patch.

 Documentation/config.txt | 13 +++++++++++++
 http.c                   | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..b982d66 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1560,6 +1560,19 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.sslCipherList::
+	A list of SSL ciphers to use when negotiating an SSL connection.
+	The available ciphers depend on whether libcurl was built against
+	NSS or OpenSSL and the particular configuration of the crypto
+	library in use.  Internally this sets the CURLOPT_SSL_CIPHER_LIST
+	option; see the libcurl documentation for that option for more
+	details on the format of this list.
+
+	Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
+	To force git to use libcurl's default cipher list and ignore any
+	explicit http.sslCipherList option, set GIT_SSL_CIPHER_LIST to the
+	empty string.
+
 http.sslVerify::
 	Whether to verify the SSL certificate when fetching or pushing
 	over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
diff --git a/http.c b/http.c
index 4b179f6..b617546 100644
--- a/http.c
+++ b/http.c
@@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
+static const char *ssl_cipherlist;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslcipherlist", var)) {
+		return git_config_string(&ssl_cipherlist, var, value);
+	}
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
@@ -361,6 +365,16 @@ static CURL *get_curl_handle(void)
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
+	if (getenv("GIT_SSL_CIPHER_LIST"))
+		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
+
+	/* See http://curl.haxx.se/libcurl/c/CURLOPT_SSL_CIPHER_LIST.html
+	 * for details on the format of and available values for
+	 * CURLOPT_SSL_CIPHER_LIST. */
+	if (ssl_cipherlist != NULL && ssl_cipherlist[0] != '\0')
+		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
+				ssl_cipherlist);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-- 
2.4.0

Re: [PATCH v3] http: add support for specifying an SSL cipher list

[Eric Sunshine]

On Thu, May 7, 2015 at 2:17 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
> Teach git about a new option, "http.sslCipherList", which permits one to
> specify a list of ciphers to use when negotiating SSL connections.  The
> setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
> variable.
>
> Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..b982d66 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1560,6 +1560,19 @@ http.saveCookies::
>         If set, store cookies received during requests to the file specified by
>         http.cookieFile. Has no effect if http.cookieFile is unset.
>
> +http.sslCipherList::
> +       A list of SSL ciphers to use when negotiating an SSL connection.
> +       The available ciphers depend on whether libcurl was built against
> +       NSS or OpenSSL and the particular configuration of the crypto
> +       library in use.  Internally this sets the CURLOPT_SSL_CIPHER_LIST
> +       option; see the libcurl documentation for that option for more
> +       details on the format of this list.
> +
> +       Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
> +       To force git to use libcurl's default cipher list and ignore any
> +       explicit http.sslCipherList option, set GIT_SSL_CIPHER_LIST to the
> +       empty string.

Much nicer description than previous rounds.

A couple style nits below.

>  http.sslVerify::
>         Whether to verify the SSL certificate when fetching or pushing
>         over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
> diff --git a/http.c b/http.c
> index 4b179f6..b617546 100644
> --- a/http.c
> +++ b/http.c
> @@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
>  static int curl_ssl_verify = -1;
>  static int curl_ssl_try;
>  static const char *ssl_cert;
> +static const char *ssl_cipherlist;
>  #if LIBCURL_VERSION_NUM >= 0x070903
>  static const char *ssl_key;
>  #endif
> @@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
>                 curl_ssl_verify = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp("http.sslcipherlist", var)) {
> +               return git_config_string(&ssl_cipherlist, var, value);
> +       }
>         if (!strcmp("http.sslcert", var))
>                 return git_config_string(&ssl_cert, var, value);
>  #if LIBCURL_VERSION_NUM >= 0x070903
> @@ -361,6 +365,16 @@ static CURL *get_curl_handle(void)
>         if (http_proactive_auth)
>                 init_curl_http_auth(result);
>
> +       if (getenv("GIT_SSL_CIPHER_LIST"))
> +               ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> +
> +       /* See http://curl.haxx.se/libcurl/c/CURLOPT_SSL_CIPHER_LIST.html
> +        * for details on the format of and available values for
> +        * CURLOPT_SSL_CIPHER_LIST. */

Format multi-line comments like this:

    /*
     * This is a multi-line
     * comment.
     */

> +       if (ssl_cipherlist != NULL && ssl_cipherlist[0] != '\0')

In git code, this is usually spelled:

    if (ssl_cipherlist && *ssl_cipherlist)

> +               curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> +                               ssl_cipherlist);
>+
>         if (ssl_cert != NULL)
>                 curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>         if (has_cert_password())
> --
> 2.4.0

Re: [PATCH v3] http: add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

On Thu, May 07, 2015 at 02:41:07PM -0400, Eric Sunshine wrote:
> Format multi-line comments like this:
> 
>     /*
>      * This is a multi-line
>      * comment.
>      */

Noted, thanks.

> > +       if (ssl_cipherlist != NULL && ssl_cipherlist[0] != '\0')
> 
> In git code, this is usually spelled:
> 
>     if (ssl_cipherlist && *ssl_cipherlist)

Huh.  At least in http.c, explicit checks against NULL seem more
common:

    if (ssl_cert != NULL)
    if (ssl_key != NULL)
    if (ssl_capath != NULL)

Etc.  I was just trying to make the new code look like the existing
code.  If nobody else has an opinion on this, I'm inclined to leave
the first clause as-is so that it matches and change the check for an
empty string.

-- 
Lars Kellogg-Stedman <lars@redhat.com> | larsks @ {freenode,twitter,github}
Cloud Engineering / OpenStack          | http://blog.oddbit.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3] http: add support for specifying an SSL cipher list

[Eric Sunshine]

On Thu, May 7, 2015 at 2:48 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
> On Thu, May 07, 2015 at 02:41:07PM -0400, Eric Sunshine wrote:
>> > +       if (ssl_cipherlist != NULL && ssl_cipherlist[0] != '\0')
>>
>> In git code, this is usually spelled:
>>
>>     if (ssl_cipherlist && *ssl_cipherlist)
>
> Huh.  At least in http.c, explicit checks against NULL seem more
> common:
>
>     if (ssl_cert != NULL)
>     if (ssl_key != NULL)
>     if (ssl_capath != NULL)
>
> Etc.  I was just trying to make the new code look like the existing
> code.  If nobody else has an opinion on this, I'm inclined to leave
> the first clause as-is so that it matches and change the check for an
> empty string.

Sounds good. Matching existing style makes sense.

Re: [PATCH v3] http: add support for specifying an SSL cipher list

[Junio C Hamano]

Lars Kellogg-Stedman <lars@redhat.com> writes:

> Teach git about a new option, "http.sslCipherList", which permits one to
> specify a list of ciphers to use when negotiating SSL connections.  The
> setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
> variable.
>
> Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
> ---
>
> This addresses (I hope!) comments from Junio and Ray, and also resolves some
> whitespace issues present in the earlier version of the patch.

Sounds good.

>  Documentation/config.txt | 13 +++++++++++++
>  http.c                   | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..b982d66 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1560,6 +1560,19 @@ http.saveCookies::
>  	If set, store cookies received during requests to the file specified by
>  	http.cookieFile. Has no effect if http.cookieFile is unset.
>  
> +http.sslCipherList::
> +	A list of SSL ciphers to use when negotiating an SSL connection.
> +	The available ciphers depend on whether libcurl was built against
> +	NSS or OpenSSL and the particular configuration of the crypto
> +	library in use.  Internally this sets the CURLOPT_SSL_CIPHER_LIST
> +	option; see the libcurl documentation for that option for more
> +	details on the format of this list.
> +
> +	Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
> +	To force git to use libcurl's default cipher list and ignore any
> +	explicit http.sslCipherList option, set GIT_SSL_CIPHER_LIST to the
> +	empty string.
> +

This will not format well, I am afraid.  The second and subsequent
paragraphs in a description of an enumerated item need to lose the
initial indentation and the empty line that breaks paragraph need
to be replaced with a single '+' (plus).  See "color::" in the same
document for an example.

We chose to use AsciiDoc primarily because its marked-up source is
easily read as a plain text files, but it is unfortunately somewhat
finicky around here.

>  http.sslVerify::
>  	Whether to verify the SSL certificate when fetching or pushing
>  	over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
> diff --git a/http.c b/http.c
> index 4b179f6..b617546 100644
> --- a/http.c
> +++ b/http.c
> @@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
>  static int curl_ssl_verify = -1;
>  static int curl_ssl_try;
>  static const char *ssl_cert;
> +static const char *ssl_cipherlist;
>  #if LIBCURL_VERSION_NUM >= 0x070903
>  static const char *ssl_key;
>  #endif
> @@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
>  		curl_ssl_verify = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp("http.sslcipherlist", var)) {
> +		return git_config_string(&ssl_cipherlist, var, value);
> +	}
>  	if (!strcmp("http.sslcert", var))
>  		return git_config_string(&ssl_cert, var, value);
>  #if LIBCURL_VERSION_NUM >= 0x070903
> @@ -361,6 +365,16 @@ static CURL *get_curl_handle(void)
>  	if (http_proactive_auth)
>  		init_curl_http_auth(result);
>  
> +	if (getenv("GIT_SSL_CIPHER_LIST"))
> +		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> +
> +	/* See http://curl.haxx.se/libcurl/c/CURLOPT_SSL_CIPHER_LIST.html
> +	 * for details on the format of and available values for
> +	 * CURLOPT_SSL_CIPHER_LIST. */

I see Eric already commented on multi-line comment and what he said
is correct, but as an in-code comment, I do not see much value in
this---anybody who is _reading_ code would know to look up
CURLOPT_SSL_CIPHER_LIST in cURL documentation, I would expect (and
of course this will not be shown to the end user).

> +	if (ssl_cipherlist != NULL && ssl_cipherlist[0] != '\0')
> +		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> +				ssl_cipherlist);
> +
>  	if (ssl_cert != NULL)
>  		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>  	if (has_cert_password())

[PATCH v4] http: add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

Teach git about a new option, "http.sslCipherList", which permits one to
specify a list of ciphers to use when negotiating SSL connections.  The
setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
variable.

Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
---
 Documentation/config.txt | 13 +++++++++++++
 http.c                   | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..e3f95a2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1560,6 +1560,19 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.sslCipherList::
+  A list of SSL ciphers to use when negotiating an SSL connection.
+  The available ciphers depend on whether libcurl was built against
+  NSS or OpenSSL and the particular configuration of the crypto
+  library in use.  Internally this sets the 'CURLOPT_SSL_CIPHER_LIST'
+  option; see the libcurl documentation for more details on the format
+  of this list.
++
+Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
+To force git to use libcurl's default cipher list and ignore any
+explicit http.sslCipherList option, set 'GIT_SSL_CIPHER_LIST' to the
+empty string.
+
 http.sslVerify::
 	Whether to verify the SSL certificate when fetching or pushing
 	over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
diff --git a/http.c b/http.c
index 4b179f6..3a39d07 100644
--- a/http.c
+++ b/http.c
@@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
+static const char *ssl_cipherlist;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslcipherlist", var)) {
+		return git_config_string(&ssl_cipherlist, var, value);
+	}
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
@@ -361,6 +365,13 @@ static CURL *get_curl_handle(void)
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
+	if (getenv("GIT_SSL_CIPHER_LIST"))
+		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
+
+	if (ssl_cipherlist != NULL && *ssl_cipherlist)
+		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
+				ssl_cipherlist);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-- 
2.4.0

Re: [PATCH v4] http: add support for specifying an SSL cipher list

[Eric Sunshine]

On Thu, May 7, 2015 at 11:44 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
> Teach git about a new option, "http.sslCipherList", which permits one to
> specify a list of ciphers to use when negotiating SSL connections.  The
> setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
> variable.
>
> Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
> ---
> diff --git a/http.c b/http.c
> index 4b179f6..3a39d07 100644
> --- a/http.c
> +++ b/http.c
> @@ -187,6 +188,9 @@ static int http_options(const char *var, const char *value, void *cb)
>                 curl_ssl_verify = git_config_bool(var, value);
>                 return 0;
>         }
> +       if (!strcmp("http.sslcipherlist", var)) {
> +               return git_config_string(&ssl_cipherlist, var, value);
> +       }

Style nit: None of the other conditionals in http_options() use curly
braces when the 'if' body is a one-liner.

>         if (!strcmp("http.sslcert", var))
>                 return git_config_string(&ssl_cert, var, value);
>  #if LIBCURL_VERSION_NUM >= 0x070903

Re: [PATCH v4] http: add support for specifying an SSL cipher list

[SZEDER Gábor]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1102 bytes --]

> +http.sslCipherList::
> +  A list of SSL ciphers to use when negotiating an SSL connection.
> +  The available ciphers depend on whether libcurl was built against
> +  NSS or OpenSSL and the particular configuration of the crypto
> +  library in use.  Internally this sets the 'CURLOPT_SSL_CIPHER_LIST'
> +  option; see the libcurl documentation for more details on the format
> +  of this list.
> ++
> +Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
> +To force git to use libcurl's default cipher list and ignore any
> +explicit http.sslCipherList option, set 'GIT_SSL_CIPHER_LIST' to the
> +empty string.
> +

... or with 'git -c http.sslCipherList <cmd>' on the command line (but I
don't think it should be mentioned here that a config variable from a
config file can be overridden via 'git -c', because that's true for all
config variables anyway).

However, speaking of command line, could you please add this new config
variable to the completion script (contrib/completion/git-completion.bash,
somewhere around line 2120)?  Thanks.

Best,
Gábor

[PATCH v5] http: add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

Teach git about a new option, "http.sslCipherList", which permits one to
specify a list of ciphers to use when negotiating SSL connections.  The
setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
variable.

Signed-off-by: Lars Kellogg-Stedman <lars@redhat.com>
---

Addressing comments from Gábor and Eric.

 Documentation/config.txt               | 13 +++++++++++++
 contrib/completion/git-completion.bash |  1 +
 http.c                                 | 10 ++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..e3f95a2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1560,6 +1560,19 @@ http.saveCookies::
 	If set, store cookies received during requests to the file specified by
 	http.cookieFile. Has no effect if http.cookieFile is unset.
 
+http.sslCipherList::
+  A list of SSL ciphers to use when negotiating an SSL connection.
+  The available ciphers depend on whether libcurl was built against
+  NSS or OpenSSL and the particular configuration of the crypto
+  library in use.  Internally this sets the 'CURLOPT_SSL_CIPHER_LIST'
+  option; see the libcurl documentation for more details on the format
+  of this list.
++
+Can be overridden by the 'GIT_SSL_CIPHER_LIST' environment variable.
+To force git to use libcurl's default cipher list and ignore any
+explicit http.sslCipherList option, set 'GIT_SSL_CIPHER_LIST' to the
+empty string.
+
 http.sslVerify::
 	Whether to verify the SSL certificate when fetching or pushing
 	over HTTPS. Can be overridden by the 'GIT_SSL_NO_VERIFY' environment
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5944c82..43bfc0c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2123,6 +2123,7 @@ _git_config ()
 		http.noEPSV
 		http.postBuffer
 		http.proxy
+		http.sslCipherList
 		http.sslCAInfo
 		http.sslCAPath
 		http.sslCert
diff --git a/http.c b/http.c
index 4b179f6..f0c5bbc 100644
--- a/http.c
+++ b/http.c
@@ -36,6 +36,7 @@ char curl_errorstr[CURL_ERROR_SIZE];
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
 static const char *ssl_cert;
+static const char *ssl_cipherlist;
 #if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
 #endif
@@ -187,6 +188,8 @@ static int http_options(const char *var, const char *value, void *cb)
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp("http.sslcipherlist", var))
+		return git_config_string(&ssl_cipherlist, var, value);
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
 #if LIBCURL_VERSION_NUM >= 0x070903
@@ -361,6 +364,13 @@ static CURL *get_curl_handle(void)
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
+	if (getenv("GIT_SSL_CIPHER_LIST"))
+		ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
+
+	if (ssl_cipherlist != NULL && *ssl_cipherlist)
+		curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
+				ssl_cipherlist);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
-- 
2.4.0

Re: [PATCH v4] http: add support for specifying an SSL cipher list

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> ... or with 'git -c http.sslCipherList <cmd>' on the command line (but I
> don't think it should be mentioned here that a config variable from a
> config file can be overridden via 'git -c', because that's true for all
> config variables anyway).

I do not think it would be very useful in practice anyway, so I
agree we should not add it there.  You would not use the blanket
http.sslCipherList but target-specific http.<url>.sslCipherList more
often to work around specific quirks of one site in your
configurtion, and specifying the blanket form with "-c" on the
command line would not defeat that.

Speaking of urlmatch-assisted configuration variables, do they get
TAB completed on the command line?

Re: [PATCH v5] http: add support for specifying an SSL cipher list

[Lars Kellogg-Stedman]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Fri, May 08, 2015 at 09:22:15AM -0400, Lars Kellogg-Stedman wrote:
> Teach git about a new option, "http.sslCipherList", which permits one to
> specify a list of ciphers to use when negotiating SSL connections.  The
> setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
> variable.

Junio, et al,

I just wanted to follow up and see if folks were happy with the latest
version of the patch.

Cheers,

-- 
Lars Kellogg-Stedman <lars@redhat.com> | larsks @ {freenode,twitter,github}
Cloud Engineering / OpenStack          | http://blog.oddbit.com/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5] http: add support for specifying an SSL cipher list

[Eric Sunshine]

On Thu, May 14, 2015 at 3:25 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
> On Fri, May 08, 2015 at 09:22:15AM -0400, Lars Kellogg-Stedman wrote:
>> Teach git about a new option, "http.sslCipherList", which permits one to
>> specify a list of ciphers to use when negotiating SSL connections.  The
>> setting can be overwridden by the GIT_SSL_CIPHER_LIST environment
>> variable.
>
> I just wanted to follow up and see if folks were happy with the latest
> version of the patch.

Here's what Junio's latest "What's Cooking"[1] says:

    * ls/http-ssl-cipher-list (2015-05-08) 1 commit
     - http: add support for specifying an SSL cipher list

     Introduce http.<url>.SSLCipherList configuration variable to tweak
     the list of cipher suite to be used with libcURL when talking with
     https:// sites.

     Will merge to 'next'.

The "Will merge to 'next'" is explained in [2].

[1]: http://article.gmane.org/gmane.comp.version-control.git/268680/
[2]: http://article.gmane.org/gmane.comp.version-control.git/268100/