{"thread":{"id":"56578","subject":"[PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","startedAt":"2021-09-24T10:08:27Z","lastAt":"2021-10-11T16:12:30Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436936","messageId":"patch-1.1-6e65734cbce-20210924T100532Z-avarab@gmail.com","threadId":"56578","inReplyTo":null,"subject":"[PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-24T10:08:20Z","receivedAt":"2021-09-24T10:08:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the error shown when a http.pinnedPubKey doesn't match to point\nthe http.pinnedPubKey variable added in aeff8a61216 (http: implement\npublic key pinning, 2016-02-15), e.g.:\n\n    git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n    fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!\n\nBefore this we'd emit the exact same thing without the \" with\nhttp.pinnedPubkey configuration\". The advantage of doing this is that\nwe're going to get a translated message (everything after the \":\" is\nhardcoded in English in libcurl), and we've got a reference to the\ngit-specific configuration variable that's causing the error.\n\nUnfortunately we can't test this easily, as there are no tests that\nrequire https:// in the test suite, and t/lib-httpd.sh doesn't know\nhow to set up such tests. See [1] for the start of a discussion about\nwhat it would take to have divergent \"t/lib-httpd/apache.conf\" test\nsetups. #leftoverbits\n\n1. https://lore.kernel.org/git/YUonS1uoZlZEt+Yd@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nI had this waiting on the now-landed ab/http-drop-old-curl-plus due to\nadding a new entry to git-curl-compat.h.\n\n git-curl-compat.h | 3 ++-\n http.c            | 4 ++++\n http.h            | 1 +\n remote-curl.c     | 4 ++++\n 4 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex a308bdb3b9b..56a83b6bbd8 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -64,16 +64,17 @@\n #if LIBCURL_VERSION_NUM >= 0x072200\n #define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0\n #endif\n \n /**\n  * CURLOPT_PINNEDPUBLICKEY was added in 7.39.0, released in November\n- * 2014.\n+ * 2014. CURLE_SSL_PINNEDPUBKEYNOTMATCH was added in that same version.\n  */\n #if LIBCURL_VERSION_NUM >= 0x072c00\n #define GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY 1\n+#define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1\n #endif\n \n /**\n  * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.\n  *\n  * The CURL_HTTP_VERSION_2 alias (but not CURL_HTTP_VERSION_2_0) has\ndiff --git a/http.c b/http.c\nindex d7c20493d7f..b6735b51c31 100644\n--- a/http.c\n+++ b/http.c\n@@ -1486,12 +1486,16 @@ static int handle_curl_result(struct slot_results *results)\n \t\t * certificate, bad password, or something else wrong\n \t\t * with the certificate.  So we reject the credential to\n \t\t * avoid caching or saving a bad password.\n \t\t */\n \t\tcredential_reject(&cert_auth);\n \t\treturn HTTP_NOAUTH;\n+#ifdef GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH\n+\t} else if (results->curl_result == CURLE_SSL_PINNEDPUBKEYNOTMATCH) {\n+\t\treturn HTTP_NOMATCHPUBLICKEY;\n+#endif\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n \telse if (results->http_code == 401) {\n \t\tif (http_auth.username && http_auth.password) {\n \t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\ndiff --git a/http.h b/http.h\nindex 3db5a0cf320..df1590e53a4 100644\n--- a/http.h\n+++ b/http.h\n@@ -151,12 +151,13 @@ struct http_get_options {\n #define HTTP_OK\t\t\t0\n #define HTTP_MISSING_TARGET\t1\n #define HTTP_ERROR\t\t2\n #define HTTP_START_FAILED\t3\n #define HTTP_REAUTH\t4\n #define HTTP_NOAUTH\t5\n+#define HTTP_NOMATCHPUBLICKEY\t6\n \n /*\n  * Requests a URL and stores the result in a strbuf.\n  *\n  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.\n  */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 598cff7cde6..8700dbdc0ac 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -496,12 +496,16 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tdie(_(\"repository '%s' not found\"),\n \t\t    transport_anonymize_url(url.buf));\n \tcase HTTP_NOAUTH:\n \t\tshow_http_message(&type, &charset, &buffer);\n \t\tdie(_(\"Authentication failed for '%s'\"),\n \t\t    transport_anonymize_url(url.buf));\n+\tcase HTTP_NOMATCHPUBLICKEY:\n+\t\tshow_http_message(&type, &charset, &buffer);\n+\t\tdie(_(\"unable to access '%s' with http.pinnedPubkey configuration: %s\"),\n+\t\t    transport_anonymize_url(url.buf), curl_errorstr);\n \tdefault:\n \t\tshow_http_message(&type, &charset, &buffer);\n \t\tdie(_(\"unable to access '%s': %s\"),\n \t\t    transport_anonymize_url(url.buf), curl_errorstr);\n \t}\n \n-- \n2.33.0.1231.g24d802460a8\n\n"},{"id":"437047","messageId":"YU5CJC9XJvQITfr8@coredump.intra.peff.net","threadId":"56578","inReplyTo":"patch-1.1-6e65734cbce-20210924T100532Z-avarab@gmail.com","subject":"Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-24T21:24:52Z","receivedAt":"2021-09-24T21:24:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the error shown when a http.pinnedPubKey doesn't match to point\n> the http.pinnedPubKey variable added in aeff8a61216 (http: implement\n> public key pinning, 2016-02-15), e.g.:\n> \n>     git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n>     fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!\n\nTBH, I think the message as-is is sufficiently descriptive. That said,\nit's not too much extra code to handle it specially, so I don't feel all\nthat strongly.\n\nMaybe people care more about the translation aspect, but it feels like\nthat's the tip of the iceberg in terms of curl errors.\n\nThe patch itself looks correct to me.\n\n-Peff\n"},{"id":"438432","messageId":"20211010214209.GE571180@szeder.dev","threadId":"56578","inReplyTo":"patch-1.1-6e65734cbce-20210924T100532Z-avarab@gmail.com","subject":"Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-10-10T21:42:09Z","receivedAt":"2021-10-10T21:42:15Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Change the error shown when a http.pinnedPubKey doesn't match to point\n> the http.pinnedPubKey variable \n\nI'm not sure what this means.  Between the repeated\n'http.pinnedPubKey' config variable name and the \"doesn't match to\npoint the ...\" part I can't decipher it.\n\n> added in aeff8a61216 (http: implement\n> public key pinning, 2016-02-15), e.g.:\n> \n>     git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n>     fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!\n> \n> Before this we'd emit the exact same thing without the \" with\n> http.pinnedPubkey configuration\". The advantage of doing this is that\n> we're going to get a translated message (everything after the \":\" is\n> hardcoded in English in libcurl), and we've got a reference to the\n> git-specific configuration variable that's causing the error.\n> \n> Unfortunately we can't test this easily, as there are no tests that\n> require https:// in the test suite, and t/lib-httpd.sh doesn't know\n> how to set up such tests. See [1] for the start of a discussion about\n> what it would take to have divergent \"t/lib-httpd/apache.conf\" test\n> setups. #leftoverbits\n> \n> 1. https://lore.kernel.org/git/YUonS1uoZlZEt+Yd@coredump.intra.peff.net/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> I had this waiting on the now-landed ab/http-drop-old-curl-plus due to\n> adding a new entry to git-curl-compat.h.\n> \n>  git-curl-compat.h | 3 ++-\n>  http.c            | 4 ++++\n>  http.h            | 1 +\n>  remote-curl.c     | 4 ++++\n>  4 files changed, 11 insertions(+), 1 deletion(-)\n> \n> diff --git a/git-curl-compat.h b/git-curl-compat.h\n> index a308bdb3b9b..56a83b6bbd8 100644\n> --- a/git-curl-compat.h\n> +++ b/git-curl-compat.h\n> @@ -64,16 +64,17 @@\n>  #if LIBCURL_VERSION_NUM >= 0x072200\n>  #define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0\n>  #endif\n>  \n>  /**\n>   * CURLOPT_PINNEDPUBLICKEY was added in 7.39.0, released in November\n> - * 2014.\n> + * 2014. CURLE_SSL_PINNEDPUBKEYNOTMATCH was added in that same version.\n>   */\n>  #if LIBCURL_VERSION_NUM >= 0x072c00\n>  #define GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY 1\n> +#define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1\n>  #endif\n>  \n>  /**\n>   * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.\n>   *\n>   * The CURL_HTTP_VERSION_2 alias (but not CURL_HTTP_VERSION_2_0) has\n> diff --git a/http.c b/http.c\n> index d7c20493d7f..b6735b51c31 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1486,12 +1486,16 @@ static int handle_curl_result(struct slot_results *results)\n>  \t\t * certificate, bad password, or something else wrong\n>  \t\t * with the certificate.  So we reject the credential to\n>  \t\t * avoid caching or saving a bad password.\n>  \t\t */\n>  \t\tcredential_reject(&cert_auth);\n>  \t\treturn HTTP_NOAUTH;\n> +#ifdef GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH\n> +\t} else if (results->curl_result == CURLE_SSL_PINNEDPUBKEYNOTMATCH) {\n> +\t\treturn HTTP_NOMATCHPUBLICKEY;\n> +#endif\n>  \t} else if (missing_target(results))\n>  \t\treturn HTTP_MISSING_TARGET;\n>  \telse if (results->http_code == 401) {\n>  \t\tif (http_auth.username && http_auth.password) {\n>  \t\t\tcredential_reject(&http_auth);\n>  \t\t\treturn HTTP_NOAUTH;\n> diff --git a/http.h b/http.h\n> index 3db5a0cf320..df1590e53a4 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -151,12 +151,13 @@ struct http_get_options {\n>  #define HTTP_OK\t\t\t0\n>  #define HTTP_MISSING_TARGET\t1\n>  #define HTTP_ERROR\t\t2\n>  #define HTTP_START_FAILED\t3\n>  #define HTTP_REAUTH\t4\n>  #define HTTP_NOAUTH\t5\n> +#define HTTP_NOMATCHPUBLICKEY\t6\n>  \n>  /*\n>   * Requests a URL and stores the result in a strbuf.\n>   *\n>   * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.\n>   */\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 598cff7cde6..8700dbdc0ac 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -496,12 +496,16 @@ static struct discovery *discover_refs(const char *service, int for_push)\n>  \t\tdie(_(\"repository '%s' not found\"),\n>  \t\t    transport_anonymize_url(url.buf));\n>  \tcase HTTP_NOAUTH:\n>  \t\tshow_http_message(&type, &charset, &buffer);\n>  \t\tdie(_(\"Authentication failed for '%s'\"),\n>  \t\t    transport_anonymize_url(url.buf));\n> +\tcase HTTP_NOMATCHPUBLICKEY:\n> +\t\tshow_http_message(&type, &charset, &buffer);\n> +\t\tdie(_(\"unable to access '%s' with http.pinnedPubkey configuration: %s\"),\n> +\t\t    transport_anonymize_url(url.buf), curl_errorstr);\n>  \tdefault:\n>  \t\tshow_http_message(&type, &charset, &buffer);\n>  \t\tdie(_(\"unable to access '%s': %s\"),\n>  \t\t    transport_anonymize_url(url.buf), curl_errorstr);\n>  \t}\n>  \n> -- \n> 2.33.0.1231.g24d802460a8\n> \n"},{"id":"438436","messageId":"87o87wl485.fsf@evledraar.gmail.com","threadId":"56578","inReplyTo":"20211010214209.GE571180@szeder.dev","subject":"Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-11T01:49:39Z","receivedAt":"2021-10-11T01:50:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Oct 10 2021, SZEDER Gábor wrote:\n\n> On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Change the error shown when a http.pinnedPubKey doesn't match to point\n>> the http.pinnedPubKey variable \n>\n> I'm not sure what this means.  Between the repeated\n> 'http.pinnedPubKey' config variable name and the \"doesn't match to\n> point the ...\" part I can't decipher it.\n\nIt should be \"point to the\" (but this grammar error is already in\n\"next\").\n"},{"id":"438443","messageId":"20211011044730.GF571180@szeder.dev","threadId":"56578","inReplyTo":"87o87wl485.fsf@evledraar.gmail.com","subject":"Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-10-11T04:47:30Z","receivedAt":"2021-10-11T04:47:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 11, 2021 at 03:49:39AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Oct 10 2021, SZEDER Gábor wrote:\n> \n> > On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> Change the error shown when a http.pinnedPubKey doesn't match to point\n> >> the http.pinnedPubKey variable \n> >\n> > I'm not sure what this means.  Between the repeated\n> > 'http.pinnedPubKey' config variable name and the \"doesn't match to\n> > point the ...\" part I can't decipher it.\n> \n> It should be \"point to the\" (but this grammar error is already in\n> \"next\").\n\nSo it's supposed to be\n\n  ... a http.pinnedPubKey doesn't point to the http.pinnedPubKey\n  variable ...\n\n?  I still have no idea because of the repeated config variable name.\n\n\n"},{"id":"438461","messageId":"87k0ijlmon.fsf@evledraar.gmail.com","threadId":"56578","inReplyTo":"20211011044730.GF571180@szeder.dev","subject":"Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-11T13:23:02Z","receivedAt":"2021-10-11T13:23:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 11 2021, SZEDER Gábor wrote:\n\n> On Mon, Oct 11, 2021 at 03:49:39AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Sun, Oct 10 2021, SZEDER Gábor wrote:\n>> \n>> > On Fri, Sep 24, 2021 at 12:08:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> >> Change the error shown when a http.pinnedPubKey doesn't match to point\n>> >> the http.pinnedPubKey variable \n>> >\n>> > I'm not sure what this means.  Between the repeated\n>> > 'http.pinnedPubKey' config variable name and the \"doesn't match to\n>> > point the ...\" part I can't decipher it.\n>> \n>> It should be \"point to the\" (but this grammar error is already in\n>> \"next\").\n>\n> So it's supposed to be\n>\n>   ... a http.pinnedPubKey doesn't point to the http.pinnedPubKey\n>   variable ...\n>\n> ?  I still have no idea because of the repeated config variable name.\n\nWe emit this currently:\n\n    $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n    fatal: unable to access 'https://github.com/git/git.git/': SSL: public key does not match pinned public key!\n\nAnd with this change, this:\n\n    $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n    fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!\n\nI.e. this replaces a generic error message from curl with something that\npoints the user at the config variable in question.\n"},{"id":"438474","messageId":"YWRia5oAm4HddIe3@coredump.intra.peff.net","threadId":"56578","inReplyTo":"87k0ijlmon.fsf@evledraar.gmail.com","subject":"Re: [PATCH] http: check CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-11T16:12:27Z","receivedAt":"2021-10-11T16:12:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 11, 2021 at 03:23:02PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > So it's supposed to be\n> >\n> >   ... a http.pinnedPubKey doesn't point to the http.pinnedPubKey\n> >   variable ...\n> >\n> > ?  I still have no idea because of the repeated config variable name.\n> \n> We emit this currently:\n> \n>     $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n>     fatal: unable to access 'https://github.com/git/git.git/': SSL: public key does not match pinned public key!\n> \n> And with this change, this:\n> \n>     $ git -c http.pinnedPubKey=sha256/someNonMatchingKey ls-remote https://github.com/git/git.git\n>     fatal: unable to access 'https://github.com/git/git.git/' with http.pinnedPubkey configuration: SSL: public key does not match pinned public key!\n> \n> I.e. this replaces a generic error message from curl with something that\n> points the user at the config variable in question.\n\nFWIW, I too had to stare at the commit message when I first read it.\nPerhaps:\n\n  When curl gives us an error related to http.pinnedPubKey, we pass\n  along the error from curl: \"public key does not match pinned public\n  key\". But we do not mention the http.pinnedPubKey config, so the user\n  may not realize where to start looking to address this.\n\nAs you say, this is already in next, so it's too late. So just thoughts\nfor next time (I find this \"we do X, but the problem is Y\" explanation\nis often more clear than \"change Z\", because it makes the motivation\nexplicit).\n\n-Peff\n"}]}