squid
squid copied to clipboard
squid-cache
Bug 5154: reject literal IPv6 address support is disabled
Address.cc:663 getAddrInfo() assertion failed: false
Squid v5 added Ip::Address::fromHost(), which attempts to parse the host field of a URL to see if it is a literal IP address. When the function parses a literal IPv6 address, it returns true, which eventually leads to a failed assert in IP::Address::getAddrInfo() when IPv6 is disabled.
The fix is to have fromHost() return false when the URL contains a literal IPv6 address and IPv6 support is disabled, causing squid to return 500 to the user like other existing code paths that encounter IPv6 addresses.
adjusted PR title and line wrapped description (no changes) to make Anubis happy.
Thank you for working on this bug fix!
What does falsy Ip::EnableIpv6 really mean? The current EnableIpv6 description does not answer this question. I can think of several possible answers, but only one of these answers can be correct:
It means the OS is unable to handle IPv6 values and problems are expected to occur if any IPv6 related operations are attempted. Those problems range from not providing the right amount of memory to IP related data types (eg translating a IP string into binary format) [when the OS itself lacks support or is broken] through to occasional packet loss in traffic [when admin decides they have an "IPv4-only network" and breaks things accordingly].
Specific to this problem: When IPv6 support is disabled Squid is unable[1] to identify what an "IPv6 address" is.
[1] or explicitly not allowed by admin.
Amos: adjusted PR title
The current "reject literal IPv6 address support is disabled" title is missing some words. Please re-adjust. FWIW, if we have to pick between the two evils, it is much better to keep Anubis unhappy (until the PR changes are settled, and we know exactly what the title should be) than to risk merging with a semantically (and grammatically) invalid title.
Amos: adjusted PR title and line wrapped description (no changes) to make Anubis happy.
I added the typical assertion text (which was previously found in the PR title) to the PR description so that important information is not lost as we continue to adjust this PR and its metadata.
I also reformatted the PR description (to use all the allowed 72 characters) and restored the author-provided Squid version formatting -- "Squid v5" instead of "Squid-5". As you probably know, I think we should use "Squid vN" formatting when possible because it is more readable, more logical, and more common among software projects. Please avoid making "no changes" claims when you change more than line wrapping.
Said that, we should replace "Squid v5" with the (7-character) commit SHA that caused the assertion this PR fixes.
I do not know why the basic "make check" test fails for this PR, but we will have to address that as well.
TESTING: layer-00-default
BUILD: .././test-suite/buildtests/layer-00-default.opts
checking whether to enable IPv6... yes
FAIL: tests/testIpAddress
FAIL: tests/testHttpRequest
make[6]: *** [test-suite.log] Error 1
Unfortunately, our Semaphore CI scripts do not expose test-suite.log contents, so it is not clear to me what is going on. If you can reproduce locally, great. If you cannot, do you have permissions to start an ssh session on the corresponding Semaphore CI page?
These are the "make check" errors I see on Ubuntu 16.04.4 LTS:
===================================================
Squid Web Proxy 6.0.0-VCS: src/test-suite.log
===================================================
# TOTAL: 31
# PASS: 29
# SKIP: 0
# XFAIL: 0
# FAIL: 2
# XPASS: 0
# ERROR: 0
.. contents:: :depth: 2
FAIL: tests/testHttpRequest
===========================
SKIP: security/libsecurity.la PeerOptions (not implemented).
SKIP: store/libstore.la Controller (not implemented).
.SKIP: mem/libmem.la Init (not implemented).
.SKIP: mem/libmem.la Init (not implemented).
F.SKIP: mem/libmem.la Init (not implemented).
.........
testHttpRequest.cc:117:Assertion
Test name: testHttpRequest::testIPv6HostColonBug
equality assertion failed
- Expected: [2000:800::45]
- Actual : 2000:800::45
Failures !!!
Run: 12 Failure total: 1 Failures: 1 Errors: 0
SKIP: store/libstore.la ~Controller (not implemented).
FAIL tests/testHttpRequest (exit status: 1)
FAIL: tests/testIpAddress
=========================
.......F........stub time| Unable to convert '192.168.100.12' to the rDNS type requested.
.F.
testIpAddress.cc:247:Assertion
Test name: testIpAddress::testStringConstructor
assertion failed
- Expression: !bnIPA.isAnyAddr()
testIpAddress.cc:599:Assertion
Test name: testIpAddress::testMasking
assertion failed
- Expression: !maskIPA.isNoAddr()
Failures !!!
Run: 17 Failure total: 2 Failures: 2 Errors: 0
FAIL tests/testIpAddress (exit status: 1)
Glad you can reproduce these PR regressions locally! Please ask for help if you cannot fix them.
Unfortunately, based on the test logs you shared, I cannot tell whether this is a new bug introduced by the PR code changes or an old bug in the unit tests exposed by those changes. I also cannot tell whether addressing my change request first will help with these regressions.
FWIW, in most cases, you can ignore the SKIP lines/warnings.
Both of these tests fail when exercising IPv6 functionality. It seems to me the best approach is to disable testHttpRequest::testIPv6HostColonBug() and a portion of testIpAddress::testStringConstructor() when building with --disable-ipv6.
Both of these tests fail when exercising IPv6 functionality. It seems to me the best approach is to disable testHttpRequest::testIPv6HostColonBug() and a portion of testIpAddress::testStringConstructor() when building with --disable-ipv6.
-
testHttpRequest::testIPv6HostColonBug(): When IPv6 is disabled, this test should check that Squid interprets all three test URLs/cases correctly. What does "correctly" mean here? I have not checked all the details, but it could mean that HttpRequest::FromUrl() fails, returning
nullptr. Please note that simply disabling those tests (when IPv6 is disabled) would potentially expose such Squid to the bug those tests are trying to protect us from -- the bug may resurface when IPv6 is disabled! -
testIpAddress::testStringConstructor(): When IPv6 is disabled, this test should check that Squid interprets all three test URLs/cases correctly. What does "correctly" mean here? I have not checked all the details, but I suspect that some of the bnIPA and cnIPA assertions will be different and some may be gone. It may make sense to group assertions based on the Ip::EnableIpv6 value. Please note that simply disabling those tests (when IPv6 is disabled) would potentially expose such Squid to the bug this PR is fixing (or similar IPv6 address handling regressions); these tests should be protecting us from them.
Feel free to refactor the test cases or add new ones, of course. The end result should preserve or improve existing coverage for both IPv6-enabled and IPv6-disabled cases (where applicable), but the details are for you to implement; we are here to help and review as needed. Please focus on keeping/making the tests useful rather than on making make check successful.
It would be easier to adjust these tests if we fix the corresponding Ip::Address APIs as discussed at https://github.com/squid-cache/squid/pull/947#discussion_r765313875, but I doubt you have the appetite and the resources to implement those (significant) API changes. We can just fix the affected test cases instead for now.
The tests are failing because they do not call Ip::ProbeTransport() to properly initialize Ip::EnableIpv6.
The tests are failing because they do not call Ip::ProbeTransport() to properly initialize Ip::EnableIpv6.
I suspect that is not the only reason, but I am glad you are making progress with the fix! See testRock::setUp() and testACLMaxUserIP::setUp() for examples on how to add initial priming/configuration/initialization steps to a bunch of related CppUnit tests. It looks like you will need to add testIpAddress::setUp() and adjust the existing testHttpRequest::setUp() to call Ip::ProbeTransport().
The test programs didn't call Ip::ProbeTransport(), which means Ip::EnableIpv6 always had its default value of zero regardless of whether the --disable-ipv6 configuration setting was used. The two failing tests were executing the non-IPv6 code path I changed in Ip::Address::lookupHostIP() (confirmed by stepping through the tests with gdb). The tests worked before because the non-IPv6 code path fell through to the IPv6 code path with the local variables having the proper values for the tests to pass. Adding Ip::ProbeTransport() fixes the tests when the --disable-ipv6 configure flag is omitted.
I think that the two tests should be at least partially disabled when the --disable-ipv6 configuration flag is provided. I would disable them using the USE_IPV6 macro that is set by the configuration flag (default 1; zero when disabled).
The test programs didn't call Ip::ProbeTransport(), which means Ip::EnableIpv6 always had its default value of zero regardless of whether the --disable-ipv6 configuration setting was used.
Adding Ip::ProbeTransport() fixes the tests when the --disable-ipv6 configure flag is omitted.
I assume you have added an Ip::ProbeTransport() call to new/adjusted setUp() test class methods[^1], but I am puzzled why you are not pushing this part of the fix to the PR branch.
[^1]: or some better place if CppUnit supports one, of course
The two failing tests were executing the non-IPv6 code path I changed in Ip::Address::lookupHostIP() (confirmed by stepping through the tests with gdb). The tests worked before because the non-IPv6 code path fell through to the IPv6 code path with the local variables having the proper values for the tests to pass.
I cannot fill in all the details based on the above high-level description, but it sounds like you discovered a problem with the official lookupHostIP() code, and this PR either has fixed that problem already or we need to do more to fix it. Perhaps this will become clearer when you push the changes and adjust the PR description (which will become an official commit message body when this PR is merged) accordingly.
I think that the two tests should be at least partially disabled when the --disable-ipv6 configuration flag is provided. I would disable them using the USE_IPV6 macro that is set by the configuration flag (default 1; zero when disabled).
This last part does not make sense to me yet -- why use a macro (which does not cover some of the supported IPv4-only environments) when we have Ip::EnableIpv6 (which covers all supported environments)? If you think some parts of the tests should be disabled, please disable them (using Ip::EnableIpv6 if at all possible) and push the changes so that we can review/discuss them. When deciding what to disable or adjust in these tests, please keep the earlier specific caveats about those tests purpose/applicability to IPv4-only environments in mind.
If you are reluctant to use EnableIpv6 because it requires a ProbeTransport() call, then I think we should address that worry by making sure that call is made. There are ways to guarantee (at C++/compiler level) a ProbeTransport() call before EnableIpv6 value is used, but those ways are a tiny bit expensive, so perhaps we can convince ourselves that they are not needed here. Either way, we should use EnableIpv6 (and make sure it is usable).
I am using gdb to step through the testIpAddress test suite with and without --disable-ipv6 to look for any other related misbehavior. I will check in my changes on Monday or Tuesday.
It there a reason this wasn't sorted for 5.8?
There are probably many reasons. AFAICT: The PR is waiting for its author since January 2022. There have been no other volunteers to take over and finish this work since then.
I'll happily take over this PR if that's allowed and the owner is MIA
Can I just clarify what's needed?
I'll happily take over this PR if that's allowed and the owner is MIA
You are certainly welcome to fix this bug, especially when the previous attempt has not resulted in a merged PR! I do not know whether GitHub will allow you to push to this PR branch. (If it does not,) you can create your own branch and open a new PR.
Can I just clarify what's needed?
I did not check all the details, but, AFAICT:
- There is at least one outstanding change request that needs to be addressed. There is a specific code change suggestion in that review. It was a partial review, so more change requests are possible.
- The branch needs to be updated to resolve conflicts with the current master branch.
- If any automated tests are still failing, those failures need to be addressed.
- PR title needs to be fixed.
- If possible, PR description should be updated to point to the commit that broke Squid.
I've had no success recreating this outside production so I've not been able to bisect the issue, assuming it wasn't always broken, I've created a new PR - let me know if that's sufficient
https://github.com/squid-cache/squid/pull/1421