| Message ID | patch-2.5-511534ce17a-20210908T152807Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | post-v2.33 "drop support for ancient curl" follow-up | expand |
On Wed, Sep 08, 2021 at 05:31:53PM +0200, Ævar Arnfjörð Bjarmason wrote: > In aeff8a61216 (http: implement public key pinning, 2016-02-15) a > dependency and warning() was added if curl older than 7.44.0 was used, > but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced > in 7.39.0. According to the manpage for CURLOPT_PINNEDPUBLICKEY, it looks like support for various formats and implementations was phased in. In particular, 7.44.0 picked up sha256 support (I guess for a fingerprint? I've never used this feature) for most major implementations. 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. > Let's also remove the macro check before we declare the ssl_pinnedkey > variable, the pattern for other such variables is to declare the > static variable unconditionally, we just may not use it on older > versions. This reduces macro verbosity. > > The reduction in verbosity comes at the small cost of issuing a > warning about the unused variable if this code is compiled with curl > versions older than 7.39.0. I think that's an acceptable trade-off, > anyone compiling a new git with a 2014-era toolchain likely has at > least other warning that'll have prompted them not to use -Werror, and > if not maybe this'll prompt them to compile their new git with a more > modern libcurl. OK. That's a bit of a departure from how we've handled variables before, but it does make the code a bit cleaner. And I am fine with the attitude of "if you are using ancient tools, you may see some extra warnings". We already know this is true for older compilers, and it's not worth caring too much about. -Peff
Jeff King <peff@peff.net> writes: > On Wed, Sep 08, 2021 at 05:31:53PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> In aeff8a61216 (http: implement public key pinning, 2016-02-15) a >> dependency and warning() was added if curl older than 7.44.0 was used, >> but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced >> in 7.39.0. > > According to the manpage for CURLOPT_PINNEDPUBLICKEY, it looks like > support for various formats and implementations was phased in. In > particular, 7.44.0 picked up sha256 support (I guess for a fingerprint? > I've never used this feature) for most major implementations. > > 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. Thanks.
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. I just saw it as: if you have 7.39 then before it would not work because we didn't bother to compile it, and after it _might_ work depending on how you use it. So it's a strict increase in functionality. Likewise, if you compile Git against 7.39 and then later upgrade the shared library, you'd get the increased functionality. But I admit I don't really care that much, either. This is an obscure-ish feature in a 7 year old version of libcurl we are talking about. But there is one thing that does get weird if we don't do this patch. If we later take the approach of checking: #ifdef CURLOPT_PINNEDPUBLICKEY then that will subtly shift the cutoff point from 7.44 to 7.39 anyway. _If_ we are going to do that conversion in a later patch (as this series does), I think it makes sense to shift the version number explicitly in a commit with an explanation, as this commit does. -Peff
On Fri, Sep 10, 2021 at 10:19:04AM -0400, Jeff King wrote: > But there is one thing that does get weird if we don't do this patch. If > we later take the approach of checking: > > #ifdef CURLOPT_PINNEDPUBLICKEY > > then that will subtly shift the cutoff point from 7.44 to 7.39 anyway. > _If_ we are going to do that conversion in a later patch (as this series > does), I think it makes sense to shift the version number explicitly in > a commit with an explanation, as this commit does. Ah, nevermind. I just saw Ævar's re-roll where that strategy turns out to be a bad idea anyway. So that is no longer a compelling argument. :) -Peff
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
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
On Fri, 10 Sep 2021, Jeff King wrote: >> 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). (double-level quote since I miseed the original email saying this) This is simply not true and it makes me really curious why you would think this. We (in the curl project) tag the git repository exactly at the point we generate the release from. The release is however the generated tarball, and the tag is the moment in the git history where the release was done. That's why the release number at the time of the tag will always say "blabla-DEV" something. I know this, becasue I've done every single curl release personally, since the dawn of time. Of course we've only used git since about 2010 but I can't remember that we ever did it differently. The exact step-by-step to do a release is also documented since years back: https://curl.se/dev/release-procedure.html
On Fri, Sep 10 2021, Daniel Stenberg wrote: > On Fri, 10 Sep 2021, Jeff King wrote: > >>> 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). > > (double-level quote since I miseed the original email saying this) > > This is simply not true and it makes me really curious why you would > think this. > > We (in the curl project) tag the git repository exactly at the point > we generate the release from. The release is however the generated > tarball, and the tag is the moment in the git history where the > release was done. That's why the release number at the time of the tag > will always say "blabla-DEV" something. > > I know this, becasue I've done every single curl release personally, > since the dawn of time. Of course we've only used git since about 2010 > but I can't remember that we ever did it differently. > > The exact step-by-step to do a release is also documented since years back: > https://curl.se/dev/release-procedure.html I take that back, sorry. It turns out to just have been a stupid mistake of mine. I don't know where exactly but somewhere in the checking out of verions, building & installing them I had a LIBCURL_VERSION_NUM that didn't match. But looking back at the tarballs I unpacked (which I didn't remove in the interim) it does match. So it was just a mistake of mine, sorry to soil curl's good reputation on a public ML, my bad. For what it's worth I'm rather prejudiced to the knee-jerk reaction of just giving up on source control pretty early on for this sort of thing. I.e. seeing what state some some ancient software version from 10-15-20 years back was in. It is really common for the two to mismatch (e.g. because there was never a version for that release, the maintainer grabbed some version, did "make dist", adjusted files manually etc.). But apparently curl was rather early on bandwagon of good SCM practices.
On Fri, 10 Sep 2021, Ævar Arnfjörð Bjarmason wrote: > So it was just a mistake of mine, sorry to soil curl's good reputation on a > public ML, my bad. Ah, no worries. I'm just glad things are good! > It is really common for the two to mismatch (e.g. because there was never a > version for that release, the maintainer grabbed some version, did "make > dist", adjusted files manually etc.). But apparently curl was rather early > on bandwagon of good SCM practices. I'd like to think that we've had that part pretty well nailed since a very long time back, partly thanks to us sticking to simple and straight-forward even for version control. And of course thanks to git for making it easy to do it right!
diff --git a/http.c b/http.c index 56856178bfe..f7d5b6a0776 100644 --- a/http.c +++ b/http.c @@ -59,9 +59,7 @@ static struct { static const char *ssl_key; static const char *ssl_capath; static const char *curl_no_proxy; -#if LIBCURL_VERSION_NUM >= 0x072c00 static const char *ssl_pinnedkey; -#endif static const char *ssl_cainfo; static long curl_low_speed_limit = -1; static long curl_low_speed_time = -1; @@ -373,10 +371,10 @@ static int http_options(const char *var, const char *value, void *cb) } if (!strcmp("http.pinnedpubkey", var)) { -#if LIBCURL_VERSION_NUM >= 0x072c00 +#if LIBCURL_VERSION_NUM >= 0x072700 return git_config_pathname(&ssl_pinnedkey, var, value); #else - warning(_("Public key pinning not supported with cURL < 7.44.0")); + warning(_("Public key pinning not supported with cURL < 7.39.0")); return 0; #endif } @@ -845,7 +843,7 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key); if (ssl_capath != NULL) curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath); -#if LIBCURL_VERSION_NUM >= 0x072c00 +#if LIBCURL_VERSION_NUM >= 0x072700 if (ssl_pinnedkey != NULL) curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey); #endif
In aeff8a61216 (http: implement public key pinning, 2016-02-15) a dependency and warning() was added if curl older than 7.44.0 was used, but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced in 7.39.0. Let's also remove the macro check before we declare the ssl_pinnedkey variable, the pattern for other such variables is to declare the static variable unconditionally, we just may not use it on older versions. This reduces macro verbosity. The reduction in verbosity comes at the small cost of issuing a warning about the unused variable if this code is compiled with curl versions older than 7.39.0. I think that's an acceptable trade-off, anyone compiling a new git with a 2014-era toolchain likely has at least other warning that'll have prompted them not to use -Werror, and if not maybe this'll prompt them to compile their new git with a more modern libcurl. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- http.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)