threads / patch / 39268

patchadd support for specifying an SSL cipher list

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

## tl;dr

20 messages between May 7, 2015 and May 14, 2015. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Lars Kellogg-Stedman· May 7, 2015, 14:16 UTC · lore

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(+)
Show changes to 2 files +18 −0

Documentation/config.txt, http.c

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
Junio C Hamano· May 7, 2015, 15:53 UTC · re: Lars Kellogg-Stedman · lore

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

Lars Kellogg-Stedman <lars@redhat.com> writes:
Show 38 quoted lines
> 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.
Show 36 quoted lines
> 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())
Lars Kellogg-Stedman· May 7, 2015, 16:04 UTC · re: Junio C Hamano · lore

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

[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/
Junio C Hamano· May 7, 2015, 16:33 UTC · re: Lars Kellogg-Stedman · lore

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

Lars Kellogg-Stedman <lars@redhat.com> writes:
Show 9 quoted lines
> [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").
Lars Kellogg-Stedman· May 7, 2015, 16:58 UTC · re: Junio C Hamano · lore

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

On Thu, May 07, 2015 at 09:33:01AM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 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/
Lars Kellogg-Stedman· May 7, 2015, 16:08 UTC · re: Junio C Hamano · lore

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

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(+)
Show changes to 2 files +19 −0

Documentation/config.txt, http.c

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
Tay Ray Chuan· May 7, 2015, 16:42 UTC · re: Lars Kellogg-Stedman · lore

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

On Thu, May 7, 2015 at 10:16 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
Show 18 quoted lines
> 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
Lars Kellogg-Stedman· May 7, 2015, 16:57 UTC · re: Tay Ray Chuan · lore

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

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/
Lars Kellogg-Stedman· May 7, 2015, 18:17 UTC · re: Lars Kellogg-Stedman · lore

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

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(+)
Show changes to 2 files +27 −0

Documentation/config.txt, http.c

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
Eric Sunshine· May 7, 2015, 18:41 UTC · re: Lars Kellogg-Stedman · lore

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

On Thu, May 7, 2015 at 2:17 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
Show 27 quoted lines
> 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.
Show 35 quoted lines
>  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)
Show 8 quoted lines
> +               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
Lars Kellogg-Stedman· May 7, 2015, 18:48 UTC · re: Eric Sunshine · lore

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

On Thu, May 07, 2015 at 02:41:07PM -0400, Eric Sunshine wrote:
Show 6 quoted lines
> Format multi-line comments like this:
> 
>     /*
>      * This is a multi-line
>      * comment.
>      */
Noted, thanks.
Show 5 quoted lines
> > +       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/
Eric Sunshine· May 7, 2015, 18:54 UTC · re: Lars Kellogg-Stedman · lore

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

On Thu, May 7, 2015 at 2:48 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
Show 18 quoted lines
> 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.
Junio C Hamano· May 7, 2015, 20:51 UTC · re: Lars Kellogg-Stedman · lore

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

Lars Kellogg-Stedman <lars@redhat.com> writes:
Show 10 quoted lines
> 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.
Show 25 quoted lines
>  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.

Show 35 quoted lines
>  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).

Show 7 quoted lines
> +	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())
Lars Kellogg-Stedman· May 8, 2015, 03:44 UTC · re: Lars Kellogg-Stedman · lore

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

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(+)
Show changes to 2 files +24 −0

Documentation/config.txt, http.c

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
Eric Sunshine· May 8, 2015, 03:53 UTC · re: Lars Kellogg-Stedman · lore

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

On Thu, May 7, 2015 at 11:44 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
Show 18 quoted lines
> 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
SZEDER Gábor· May 8, 2015, 12:15 UTC · re: Lars Kellogg-Stedman · lore

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

Show 13 quoted lines
> +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

Junio C Hamano· May 8, 2015, 15:59 UTC · re: SZEDER Gábor · lore

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

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?

Lars Kellogg-Stedman· May 8, 2015, 13:22 UTC · re: Lars Kellogg-Stedman · lore

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

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(+)
Show changes to 3 files +24 −0

Documentation/config.txt, contrib/completion/git-completion.bash, http.c

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
Lars Kellogg-Stedman· May 14, 2015, 19:25 UTC · re: Lars Kellogg-Stedman · lore

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

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/
Eric Sunshine· May 14, 2015, 19:39 UTC · re: Lars Kellogg-Stedman · lore

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

On Thu, May 14, 2015 at 3:25 PM, Lars Kellogg-Stedman <lars@redhat.com> wrote:
Show 8 quoted lines
> 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/

← back to recent threads