Re: [PATCH 2/5] http: correct curl version check for CURLOPT_PINNEDPUBLICKEY

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	Bagas Sanjaya <bagasdotme@gmail.com>
Subject: Re: [PATCH 2/5] http: correct curl version check for CURLOPT_PINNEDPUBLICKEY
Date: Fri, 10 Sep 2021 16:37:48 +0200	[thread overview]
Message-ID: <87tuiscwso.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YTtpWADzTJEAIvk+@coredump.intra.peff.net>

On Fri, Sep 10 2021, Jeff King wrote:

> On Thu, Sep 09, 2021 at 04:12:26PM -0700, Junio C Hamano wrote:
>
>> > But in terms of compiling, all we care about is that the constant is
>> > there. So I think the cutoff point you found is what we want. Presumably
>> > when the file format isn't supported we'd get some error, though it's
>> > not clear if that would come during the actual curl_*_perform(), or if
>> > we should be checking the curl_easy_setopt() result.
>> 
>> If we were evaluating a patch to add support for pinnedpublickey
>> afresh back in, say, 2017, perhaps we cared enough about the
>> distinction between 7.39 and 7.44 (Nov 2014 and Aug 2015,
>> respectively), but I'd say cut-off at 7.44 for this, once it is
>> written and committed in our codebase, is good enough for us.
>> 
>> If the code originally had cut-off at 7.39 and we were raising the
>> floor to 7.44 with "sha256 weren't usable before that version" as
>> the justification, it would be a totally different situation and it
>> may be worth the code change, but I am not sure if going backwards
>> is worth it.
>> 
>> So, I dunno.
>
> I don't have a sense of whether the functionality difference between
> 7.39 and 7.44 actually matters.

For what it's worth I tested this as part of re-rolling, i.e. I grabbed
the tarball for 7.39[1].

By using the correct version number of 7.39 we'll support pinned public
keys there, but if you supply e.g. the "sha256/[...]" format we'll
instead of printing a warning about this version not supporting pinned
keys, we'll die with an error from curl itself.

I think whatever happens with 7.39..7.44 doesn't matter much, but this
does seem like more useful behavior, and we avoid the oddity of
hardcoding the "wrong" version (until you start looking more into it,
that is...).

Aside from 7.39..7.44 though it does seem like a really bad thing to do
to just warn that we don't support pinned public keys, but proceed with
the request anyway (which could also be a push).

I don't think it's worth changing that s/warning/die/g now, since the
target audience for a new git release with such an ancient curl version
is probably zero, or near enough to be zero.

I mean, we do have new git + old OS, but it tends to be *specific* old
versions, namely for releases of this age the one released with RHEL. I
think pretty nobody else does that (the rest are probably all
RHEL-derived). Per 013c7e2b070 (http: drop support for curl < 7.16.0,
2021-07-30) none of the RHEL out there have a curl in the 7.39..7.44
range.

The commit doesn't note the RHEL 8 version (which b.t.w., is something I
added to it, not you), but it seems to be 7.61.1[2]. So at least as far
as RHEL goes we'll never be stuck in the 7.39..7.44 range..

1. protip: curl.git git tags are rather useless, since (at least for old
   versions) the embedded version number is bumped sometime *after* the
   release).

   I also ran "diff -ru" on at least one old tag/tarball (I forget
   which) and there were a lot of changes (and not just some "make dist"
   stuff like autoconf files, version numbers or whatever).

   So in testing this I stopped using curl.git for anything but "git log
   -G<str>" searching and the like, and just tested with archived
   release tarballs.

2. https://access.redhat.com/solutions/4174711