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

[Jeff King <peff@peff.net>]

On Fri, Sep 10, 2021 at 04:37:48PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

Thanks for testing that. That was really my one concern: that by not
issuing our own error for the semi-functional version, we might fail to
notice it entirely, putting people on that older version in a worse
spot. But it sounds like we (and curl) do the right thing.

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

Yeah, agreed. When we introduce optional features we should try to make
sure that they fail in the safest direction (assuming the user has
explicitly asked to use them, as in this case). But given the age and
history here, I'm not sure it matters that much at this point. On the
other hand, if it really is s/warning/die/, that's easy enough to do. I
could go either way.

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

I don't think the specific version range matters here. If I had curl
7.19.4 (for example) and tried to use the pinning feature I'd get a
warning but we'd continue without using the pinned key, which is
dangerous. That's true both before and after your patches.

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

Interesting to note. I'd always looked at curl.git in the past.

-Peff