[PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[Ævar Arnfjörð Bjarmason]

Change the error shown when a http.pinnedPubKey doesn't match to point
the http.pinnedPubKey variable added in aeff8a61216 (http: implement
public key pinning, 2016-02-15), e.g.:

    git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
    fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!

Before this we'd emit the exact same thing without the " with
http.pinnedPubkey configuration". The advantage of doing this is that
we're going to get a translated message (everything after the ":" is
hardcoded in English in libcurl), and we've got a reference to the
git-specific configuration variable that's causing the error.

Unfortunately we can't test this easily, as there are no tests that
require https:// in the test suite, and t/lib-httpd.sh doesn't know
how to set up such tests. See [1] for the start of a discussion about
what it would take to have divergent "t/lib-httpd/apache.conf" test
setups. #leftoverbits

1. https://lore.kernel.org/git/YUonS1uoZlZEt+Yd@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I had this waiting on the now-landed ab/http-drop-old-curl-plus due to
adding a new entry to git-curl-compat.h.

 git-curl-compat.h | 3 ++-
 http.c            | 4 ++++
 http.h            | 1 +
 remote-curl.c     | 4 ++++
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/git-curl-compat.h b/git-curl-compat.h
index a308bdb3b9b..56a83b6bbd8 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -64,16 +64,17 @@
 #if LIBCURL_VERSION_NUM >= 0x072200
 #define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0
 #endif
 
 /**
  * CURLOPT_PINNEDPUBLICKEY was added in 7.39.0, released in November
- * 2014.
+ * 2014. CURLE_SSL_PINNEDPUBKEYNOTMATCH was added in that same version.
  */
 #if LIBCURL_VERSION_NUM >= 0x072c00
 #define GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY 1
+#define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
 #endif
 
 /**
  * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
  *
  * The CURL_HTTP_VERSION_2 alias (but not CURL_HTTP_VERSION_2_0) has
diff --git a/http.c b/http.c
index d7c20493d7f..b6735b51c31 100644
--- a/http.c
+++ b/http.c
@@ -1486,12 +1486,16 @@ static int handle_curl_result(struct slot_results *results)
 		 * certificate, bad password, or something else wrong
 		 * with the certificate.  So we reject the credential to
 		 * avoid caching or saving a bad password.
 		 */
 		credential_reject(&cert_auth);
 		return HTTP_NOAUTH;
+#ifdef GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH
+	} else if (results->curl_result == CURLE_SSL_PINNEDPUBKEYNOTMATCH) {
+		return HTTP_NOMATCHPUBLICKEY;
+#endif
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
 	else if (results->http_code == 401) {
 		if (http_auth.username && http_auth.password) {
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
diff --git a/http.h b/http.h
index 3db5a0cf320..df1590e53a4 100644
--- a/http.h
+++ b/http.h
@@ -151,12 +151,13 @@ struct http_get_options {
 #define HTTP_OK			0
 #define HTTP_MISSING_TARGET	1
 #define HTTP_ERROR		2
 #define HTTP_START_FAILED	3
 #define HTTP_REAUTH	4
 #define HTTP_NOAUTH	5
+#define HTTP_NOMATCHPUBLICKEY	6
 
 /*
  * Requests a URL and stores the result in a strbuf.
  *
  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.
  */
diff --git a/remote-curl.c b/remote-curl.c
index 598cff7cde6..8700dbdc0ac 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -496,12 +496,16 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		die(_("repository '%s' not found"),
 		    transport_anonymize_url(url.buf));
 	case HTTP_NOAUTH:
 		show_http_message(&type, &charset, &buffer);
 		die(_("Authentication failed for '%s'"),
 		    transport_anonymize_url(url.buf));
+	case HTTP_NOMATCHPUBLICKEY:
+		show_http_message(&type, &charset, &buffer);
+		die(_("unable to access '%s' with http.pinnedPubkey configuration: %s"),
+		    transport_anonymize_url(url.buf), curl_errorstr);
 	default:
 		show_http_message(&type, &charset, &buffer);
 		die(_("unable to access '%s': %s"),
 		    transport_anonymize_url(url.buf), curl_errorstr);
 	}
 
-- 
2.33.0.1231.g24d802460a8

Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[Jeff King]

On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Change the error shown when a http.pinnedPubKey doesn't match to point
> the http.pinnedPubKey variable added in aeff8a61216 (http: implement
> public key pinning, 2016-02-15), e.g.:
> 
>     git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
>     fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!

TBH, I think the message as-is is sufficiently descriptive. That said,
it's not too much extra code to handle it specially, so I don't feel all
that strongly.

Maybe people care more about the translation aspect, but it feels like
that's the tip of the iceberg in terms of curl errors.

The patch itself looks correct to me.

-Peff

Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[SZEDER Gábor]

On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Change the error shown when a http.pinnedPubKey doesn't match to point
> the http.pinnedPubKey variable 

I'm not sure what this means.  Between the repeated
'http.pinnedPubKey' config variable name and the "doesn't match to
point the ..." part I can't decipher it.

> added in aeff8a61216 (http: implement
> public key pinning, 2016-02-15), e.g.:
> 
>     git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
>     fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!
> 
> Before this we'd emit the exact same thing without the " with
> http.pinnedPubkey configuration". The advantage of doing this is that
> we're going to get a translated message (everything after the ":" is
> hardcoded in English in libcurl), and we've got a reference to the
> git-specific configuration variable that's causing the error.
> 
> Unfortunately we can't test this easily, as there are no tests that
> require https:// in the test suite, and t/lib-httpd.sh doesn't know
> how to set up such tests. See [1] for the start of a discussion about
> what it would take to have divergent "t/lib-httpd/apache.conf" test
> setups. #leftoverbits
> 
> 1. https://lore.kernel.org/git/YUonS1uoZlZEt+Yd@coredump.intra.peff.net/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> I had this waiting on the now-landed ab/http-drop-old-curl-plus due to
> adding a new entry to git-curl-compat.h.
> 
>  git-curl-compat.h | 3 ++-
>  http.c            | 4 ++++
>  http.h            | 1 +
>  remote-curl.c     | 4 ++++
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/git-curl-compat.h b/git-curl-compat.h
> index a308bdb3b9b..56a83b6bbd8 100644
> --- a/git-curl-compat.h
> +++ b/git-curl-compat.h
> @@ -64,16 +64,17 @@
>  #if LIBCURL_VERSION_NUM >= 0x072200
>  #define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0
>  #endif
>  
>  /**
>   * CURLOPT_PINNEDPUBLICKEY was added in 7.39.0, released in November
> - * 2014.
> + * 2014. CURLE_SSL_PINNEDPUBKEYNOTMATCH was added in that same version.
>   */
>  #if LIBCURL_VERSION_NUM >= 0x072c00
>  #define GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY 1
> +#define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
>  #endif
>  
>  /**
>   * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
>   *
>   * The CURL_HTTP_VERSION_2 alias (but not CURL_HTTP_VERSION_2_0) has
> diff --git a/http.c b/http.c
> index d7c20493d7f..b6735b51c31 100644
> --- a/http.c
> +++ b/http.c
> @@ -1486,12 +1486,16 @@ static int handle_curl_result(struct slot_results *results)
>  		 * certificate, bad password, or something else wrong
>  		 * with the certificate.  So we reject the credential to
>  		 * avoid caching or saving a bad password.
>  		 */
>  		credential_reject(&cert_auth);
>  		return HTTP_NOAUTH;
> +#ifdef GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH
> +	} else if (results->curl_result == CURLE_SSL_PINNEDPUBKEYNOTMATCH) {
> +		return HTTP_NOMATCHPUBLICKEY;
> +#endif
>  	} else if (missing_target(results))
>  		return HTTP_MISSING_TARGET;
>  	else if (results->http_code == 401) {
>  		if (http_auth.username && http_auth.password) {
>  			credential_reject(&http_auth);
>  			return HTTP_NOAUTH;
> diff --git a/http.h b/http.h
> index 3db5a0cf320..df1590e53a4 100644
> --- a/http.h
> +++ b/http.h
> @@ -151,12 +151,13 @@ struct http_get_options {
>  #define HTTP_OK			0
>  #define HTTP_MISSING_TARGET	1
>  #define HTTP_ERROR		2
>  #define HTTP_START_FAILED	3
>  #define HTTP_REAUTH	4
>  #define HTTP_NOAUTH	5
> +#define HTTP_NOMATCHPUBLICKEY	6
>  
>  /*
>   * Requests a URL and stores the result in a strbuf.
>   *
>   * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.
>   */
> diff --git a/remote-curl.c b/remote-curl.c
> index 598cff7cde6..8700dbdc0ac 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -496,12 +496,16 @@ static struct discovery *discover_refs(const char *service, int for_push)
>  		die(_("repository '%s' not found"),
>  		    transport_anonymize_url(url.buf));
>  	case HTTP_NOAUTH:
>  		show_http_message(&type, &charset, &buffer);
>  		die(_("Authentication failed for '%s'"),
>  		    transport_anonymize_url(url.buf));
> +	case HTTP_NOMATCHPUBLICKEY:
> +		show_http_message(&type, &charset, &buffer);
> +		die(_("unable to access '%s' with http.pinnedPubkey configuration: %s"),
> +		    transport_anonymize_url(url.buf), curl_errorstr);
>  	default:
>  		show_http_message(&type, &charset, &buffer);
>  		die(_("unable to access '%s': %s"),
>  		    transport_anonymize_url(url.buf), curl_errorstr);
>  	}
>  
> -- 
> 2.33.0.1231.g24d802460a8
> 

Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[Ævar Arnfjörð Bjarmason]

On Sun, Oct 10 2021, SZEDER Gábor wrote:

> On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Change the error shown when a http.pinnedPubKey doesn't match to point
>> the http.pinnedPubKey variable 
>
> I'm not sure what this means.  Between the repeated
> 'http.pinnedPubKey' config variable name and the "doesn't match to
> point the ..." part I can't decipher it.

It should be "point to the" (but this grammar error is already in
"next").

Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[SZEDER Gábor]

On Mon, Oct 11, 2021 at 03:49:39AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Oct 10 2021, SZEDER Gábor wrote:
> 
> > On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> Change the error shown when a http.pinnedPubKey doesn't match to point
> >> the http.pinnedPubKey variable 
> >
> > I'm not sure what this means.  Between the repeated
> > 'http.pinnedPubKey' config variable name and the "doesn't match to
> > point the ..." part I can't decipher it.
> 
> It should be "point to the" (but this grammar error is already in
> "next").

So it's supposed to be

  ... a http.pinnedPubKey doesn't point to the http.pinnedPubKey
  variable ...

?  I still have no idea because of the repeated config variable name.

Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 11 2021, SZEDER Gábor wrote:

> On Mon, Oct 11, 2021 at 03:49:39AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Sun, Oct 10 2021, SZEDER Gábor wrote:
>> 
>> > On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >> Change the error shown when a http.pinnedPubKey doesn't match to point
>> >> the http.pinnedPubKey variable 
>> >
>> > I'm not sure what this means.  Between the repeated
>> > 'http.pinnedPubKey' config variable name and the "doesn't match to
>> > point the ..." part I can't decipher it.
>> 
>> It should be "point to the" (but this grammar error is already in
>> "next").
>
> So it's supposed to be
>
>   ... a http.pinnedPubKey doesn't point to the http.pinnedPubKey
>   variable ...
>
> ?  I still have no idea because of the repeated config variable name.

We emit this currently:

    $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
    fatal: unable to access 'https://github.com/git/git.git/': SSL: public key does not match pinned public key!

And with this change, this:

    $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
    fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!

I.e. this replaces a generic error message from curl with something that
points the user at the config variable in question.

Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors

[Jeff King]

On Mon, Oct 11, 2021 at 03:23:02PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > So it's supposed to be
> >
> >   ... a http.pinnedPubKey doesn't point to the http.pinnedPubKey
> >   variable ...
> >
> > ?  I still have no idea because of the repeated config variable name.
> 
> We emit this currently:
> 
>     $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
>     fatal: unable to access 'https://github.com/git/git.git/': SSL: public key does not match pinned public key!
> 
> And with this change, this:
> 
>     $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git
>     fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!
> 
> I.e. this replaces a generic error message from curl with something that
> points the user at the config variable in question.

FWIW, I too had to stare at the commit message when I first read it.
Perhaps:

  When curl gives us an error related to http.pinnedPubKey, we pass
  along the error from curl: "public key does not match pinned public
  key". But we do not mention the http.pinnedPubKey config, so the user
  may not realize where to start looking to address this.

As you say, this is already in next, so it's too late. So just thoughts
for next time (I find this "we do X, but the problem is Y" explanation
is often more clear than "change Z", because it makes the motivation
explicit).

-Peff