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

From: Jeff King <peff@peff.net>
Date: Oct 11, 2021, 16:12 UTC
Message-ID: <YWRia5oAm4HddIe3@coredump.intra.peff.net>
In-Reply-To: <87k0ijlmon.fsf@evledraar.gmail.com>

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

Show 19 quoted lines

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