From 61c1d7d3658fdcd4b32f8b071cef7941120f8abc Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 16 Sep 2021 19:33:24 +0200 Subject: Add AllowRange option to disable HTTP Range usage apt makes heavy usage of HTTP1.1 features including Range and If-Range. Sadly it is not obvious if the involved server(s) (and proxies) actually support them all. The Acquire::http::AllowRange option defaults to true as before, but now a user can disable Range usage if it is known that the involved server is not dealing with such requests correctly. --- test/integration/test-http-if-range | 77 +++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100755 test/integration/test-http-if-range (limited to 'test/integration') diff --git a/test/integration/test-http-if-range b/test/integration/test-http-if-range new file mode 100755 index 000000000..7eb667ba7 --- /dev/null +++ b/test/integration/test-http-if-range @@ -0,0 +1,77 @@ +#!/bin/sh +set -e + +TESTDIR="$(readlink -f "$(dirname "$0")")" +. "$TESTDIR/framework" +setupenvironment +configarchitecture 'amd64' + +changetowebserver + +TESTFILE='aptarchive/testfile' +HTTPFILE="http://localhost:${APTHTTPPORT}/testfile" +DOWNFILE='./downloaded/testfile' +DOWNLOADLOG='rootdir/tmp/testdownloadfile.log' + +testdownloadfile() { + rm -f "$DOWNLOADLOG" + msgtest "Testing download of file with" "$1" + if ! downloadfile "$HTTPFILE" "$DOWNFILE" > "$DOWNLOADLOG"; then + cat >&2 "$DOWNLOADLOG" + msgfail + else + msgpass + fi +} + +nopartialfile() { + rm -f "$DOWNFILE" +} +validpartialfile() { + head -n 5 "$TESTFILE" > "$DOWNFILE" + touch -d "$(stat --format '%y' "${TESTFILE}")" "$DOWNFILE" +} +badpartialfile() { + head -n 5 "$TESTFILE" > "$DOWNFILE" + touch -d 'now + 1hour' "$DOWNFILE" +} +fullfile() { + cp -a "$TESTFILE" "$DOWNFILE" +} + +cp -a "${TESTDIR}/framework" "$TESTFILE" + +testrun() { + nopartialfile + testdownloadfile "no file $1" + testwebserverlaststatuscode "$2" "$DOWNLOADLOG" + testsuccess cmp "$TESTFILE" "$DOWNFILE" + + validpartialfile + testdownloadfile "good partial file $1" + testwebserverlaststatuscode "$3" "$DOWNLOADLOG" + testsuccess cmp "$TESTFILE" "$DOWNFILE" + + badpartialfile + testdownloadfile "bad partial file $1" + testwebserverlaststatuscode "$4" "$DOWNLOADLOG" + testsuccess cmp "$TESTFILE" "$DOWNFILE" + + fullfile + testdownloadfile "complete file $1" + testwebserverlaststatuscode "$5" "$DOWNLOADLOG" + testsuccess cmp "$TESTFILE" "$DOWNFILE" +} + +testrun 'defaults' '200' '206' '200' '416' + +webserverconfig 'aptwebserver::support::range' 'false' +testrun 'no ranges' '200' '200' '200' '200' +webserverconfig 'aptwebserver::support::range' 'true' + +webserverconfig 'aptwebserver::support::if-range' 'false' +# the second 206 is bad, but we are unable to detect this +testrun 'buggy server' '200' '206' '206' '416' +echo 'Acquire::http::localhost::AllowRanges "false";' > rootdir/etc/apt/apt.conf.d/noallowranges +testrun 'range disabled by conf' '200' '200' '200' '200' +rm rootdir/etc/apt/apt.conf.d/noallowranges -- cgit v1.2.3-70-g09d2 From d013f8957c0d464e0059cc107ca79d887cf9f8aa Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 16 Sep 2021 19:48:33 +0200 Subject: Disable HTTP Range usage if varnish < 6.4 is involved Debian buster (oldstable) ships 6.1 while bullseye (stable) ships 6.5 and so the later is 'fixed'. Upstream declares 6.0 still as supported. It might be still a while we encounter "bad" versions in the wild, so if we can detect and work around the issue at runtime automatically we can save some users from running into "persistent" partial files. References: https://varnish-cache.org/docs/6.4/whats-new/changes-6.4.html#changes-in-behavior --- methods/basehttp.cc | 27 +++++++++++++++++++++++++++ test/integration/test-http-if-range | 5 +++++ 2 files changed, 32 insertions(+) (limited to 'test/integration') diff --git a/methods/basehttp.cc b/methods/basehttp.cc index 396b33999..df34698cd 100644 --- a/methods/basehttp.cc +++ b/methods/basehttp.cc @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -19,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -255,6 +257,24 @@ bool RequestState::HeaderLine(string const &Line) /*{{{*/ return true; } + if (Server->RangesAllowed && stringcasecmp(Tag, "Via:") == 0) + { + auto const parts = VectorizeString(Val, ' '); + std::string_view const varnish{"(Varnish/"}; + if (parts.size() != 3 || parts[1] != "varnish" || parts[2].empty() || + not APT::String::Startswith(parts[2], std::string{varnish}) || + parts[2].back() != ')') + return true; + auto const version = parts[2].substr(varnish.length(), parts[2].length() - (varnish.length() + 1)); + if (version.empty()) + return true; + std::string_view const varnishsupport{"6.4~"}; + if (debVersioningSystem::CmpFragment(version.data(), version.data() + version.length(), + varnishsupport.begin(), varnishsupport.end()) < 0) + Server->RangesAllowed = false; + return true; + } + return true; } /*}}}*/ @@ -410,6 +430,13 @@ BaseHttpMethod::DealWithHeaders(FetchResult &Res, RequestState &Req) } /* else pass through for error message */ } + // the server is not supporting ranges as much as we would like. Retry without ranges + else if (not Server->RangesAllowed && (Req.Result == 416 || Req.Result == 206)) + { + RemoveFile("server", Queue->DestFile); + NextURI = Queue->Uri; + return TRY_AGAIN_OR_REDIRECT; + } // retry after an invalid range response without partial data else if (Req.Result == 416) { diff --git a/test/integration/test-http-if-range b/test/integration/test-http-if-range index 7eb667ba7..ca9f3b2e6 100755 --- a/test/integration/test-http-if-range +++ b/test/integration/test-http-if-range @@ -75,3 +75,8 @@ testrun 'buggy server' '200' '206' '206' '416' echo 'Acquire::http::localhost::AllowRanges "false";' > rootdir/etc/apt/apt.conf.d/noallowranges testrun 'range disabled by conf' '200' '200' '200' '200' rm rootdir/etc/apt/apt.conf.d/noallowranges +# detect varnish < 6.4 automatically +webserverconfig 'aptwebserver::response-header::Via' '1.1 varnish (Varnish/6.1)' +testrun 'bad varnish' '200' '200' '200' '200' +webserverconfig 'aptwebserver::response-header::Via' '1.1 varnish (Varnish/6.4)' +testrun 'good varnish' '200' '206' '206' '416' -- cgit v1.2.3-70-g09d2 From 1edf8551cef0a7db7fdcdd5d6b06aec2ea7bb70d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 16 Sep 2021 22:02:26 +0200 Subject: Use exact If-Range match in our test webserver RFC7233 3.2 If-Range specifies the comparison to be an exact match, not a less or equal, which makes no sense in this context anyhow. Our server exists only to write our tests against it so this isn't much of a practical issue. I did confirm with a crashing server that no test (silently) depends on this or exhibits a different behaviour not explicitly checked for. --- test/integration/test-http-if-range | 15 ++++++++++++--- test/interactive-helper/aptwebserver.cc | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) (limited to 'test/integration') diff --git a/test/integration/test-http-if-range b/test/integration/test-http-if-range index ca9f3b2e6..462d731cf 100755 --- a/test/integration/test-http-if-range +++ b/test/integration/test-http-if-range @@ -31,7 +31,11 @@ validpartialfile() { head -n 5 "$TESTFILE" > "$DOWNFILE" touch -d "$(stat --format '%y' "${TESTFILE}")" "$DOWNFILE" } -badpartialfile() { +badolderpartialfile() { + head -n 5 "$TESTFILE" > "$DOWNFILE" + touch -d "$(stat --format '%y' "${TESTFILE}") - 1sec" "$DOWNFILE" +} +badnewerpartialfile() { head -n 5 "$TESTFILE" > "$DOWNFILE" touch -d 'now + 1hour' "$DOWNFILE" } @@ -52,8 +56,13 @@ testrun() { testwebserverlaststatuscode "$3" "$DOWNLOADLOG" testsuccess cmp "$TESTFILE" "$DOWNFILE" - badpartialfile - testdownloadfile "bad partial file $1" + badolderpartialfile + testdownloadfile "bad old partial file $1" + testwebserverlaststatuscode "$4" "$DOWNLOADLOG" + testsuccess cmp "$TESTFILE" "$DOWNFILE" + + badnewerpartialfile + testdownloadfile "bad new partial file $1" testwebserverlaststatuscode "$4" "$DOWNLOADLOG" testsuccess cmp "$TESTFILE" "$DOWNFILE" diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index 58ba54f84..d4bac24d1 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -828,7 +828,7 @@ static void * handleClient(int const client, size_t const id) /*{{{*/ ifrange = LookupTag(*m, "If-Range", ""); bool validrange = (ifrange.empty() == true || (RFC1123StrToTime(ifrange, cache) == true && - cache <= data.ModificationTime())); + cache == data.ModificationTime())); // FIXME: support multiple byte-ranges (APT clients do not do this) if (condition.find(',') == std::string::npos) -- cgit v1.2.3-70-g09d2