Bug #10288: Fix newly identified issues to make our test suite more robust and faster
Firewall leak detector makes bad assumptions about PacketFu parsing
I've seen this:
calling as amnesia: host -T torproject.org 184.108.40.206 call returned: [0, "Using domain server:\nName: 220.127.116.11\nAddress: 18.104.22.168#53\nAliases: \n\ntorproject.org has address 22.214.171.124\ntorproject.org has address 126.96.36.199\ntorproject.org has address 188.8.131.52\ntorproject.org has address 184.108.40.206\ntorproject.org has IPv6 address 2001:41b8:202:deb:213:21ff:fe20:1426\ntorproject.org has IPv6 address 2001:858:2:2:aabb:0:563b:1e28\ntorproject.org has IPv6 address 2a01:4f8:172:1b46:0:abba:5:1\ntorproject.org has IPv6 address 2620:0:6b0:b:1a1a:0:26e5:4810\ntorproject.org mail is handled by 10 eugeni.torproject.org.\n", ""] When I do a TCP DNS lookup of "torproject.org" # features/step_definitions/firewall_leaks.rb:19 Then the firewall leak detector has detected leaks # features/step_definitions/firewall_leaks.rb:1 <Test::Unit::AssertionFailedError> exception expected but was <NoMethodError(<undefined method `ip_saddr' for nil:NilClass>)>. (Test::Unit::AssertionFailedError) ./features/step_definitions/firewall_leaks.rb:2:in `/^the firewall leak detector has detected leaks$/' features/tor_enforcement.feature:33:in `Then the firewall leak detector has detected leaks'
This implies that the various
.parsemethods can return
true. This feels like a bug in
Sadly the way this error happened prevented a
.pcap artifact to be saved. Also:
Then(/^the firewall leak detector has detected leaks$/) do assert_raise(Test::Unit::AssertionFailedError) do step 'all Internet traffic has only flowed through Tor' end end
That step calls
assert_all_connectionsand given how we have the opposite expectation about the assertion (
assert_raise) the artifact saving occurs in the opposite of what we want here.
Also: probably we'll end up throwing in some
assert@s in the @pcap_connections_helper (e.g. for the
nil issue above) so we should probably introduce another exception to throw when
assert_all_connections fails, and a subclass of
Test::Unit::AssertionFailedError would be good enough to make it possible to distinguish.
Merge remote-tracking branch 'origin/test/11508-possible-packetfu-bug' into testing (refs: #11508).
Test suite: fixup on debugging info logging for PacketFu.
We still get "undefined method `ip_saddr' for nil:NilClass" because
another instance of `ip_packet.ip_saddr` was forgotten and not dealt
Test suite: refine debug info collection of #11508.
I have seen yet another:
NoMethodError: undefined method `ip_saddr' for nil:NilClass
So we now know that `ip_packet` is not `nil`, it just doesn't have the
`ip_saddr` method (and probably not the `ip_daddr` one either) some
how (feels strange for an IP packet to not have a source or
Test suite: try possible fix for #11508.
Yup, it seems that all along I've just missed that we could have
IPv6Packet:s in `ip_packet`, and their source is accessed by
`.ipv6_saddr`, not `ip_saddr` (that's for IPv4Packet). So, let's just
try and see which one of the two each `ip_packet` has, because one of
them must be there!
Also, given that UDPPacket can be either IPv4 or IPv6 it seems safest
to try to parse each packet as IPv6Packet first -- that way we keep
looking at transport layer protocols for IPv4 only, and treat
everything IPv6 as the same, which makes sense, since we should block
all IPv6, so everything should be treated the same at all times.
#1 Updated by intrigeri about 2 years ago
- Parent task set to #10288
Shall we mark all scenarios that have "the firewall leak detector has detected leaks" as fragile?
#8 Updated by anonym over 1 year ago
- Assignee changed from anonym to intrigeri
- % Done changed from 10 to 30
- QA Check set to Ready for QA
- Feature Branch changed from wip/11508-possible-packetfu-bug to test/11508-possible-packetfu-bug
For now the feature branch contains no fix, only better data gathering. Please merge'n'review into
testing (and then into
devel), but do not close this ticket (just move the Target version to 2.11 and reassign to me as Dev needed)!
#11 Updated by anonym over 1 year ago
- % Done changed from 30 to 40
I just pushed fdd50f83f44aa9f46f08ab8bbedf646b2efb3a06 (to
feature/stretch). From the commit message:
I have seen yet another: NoMethodError: undefined method `ip_saddr' for nil:NilClass So we now know that `ip_packet` is not `nil` [...]
I based that on the fact that the only instance of
if ip_packet packet_info[:saddr] = ip_packet.ip_saddr [...]
nil. But if we look at the exception from above again we have: "undefined method `ip_saddr' for nil:NilClass". That instead seems to indicate that
nil. WTF? Also, I just realized
ip_saddr, but a
ipv6_saddrinstead! Whoops! :S So a
NoMethodErroris indeed to be expected here, but it shouldn't have said
[...]:IPv6Packet. However, after having carefully read the PacketFu sources, I now see that
nil:NilClassis to be expected given the implementation, so that exception is actually raised from within the
packetfumodule. Urgh, well this has confused me quite a bit!
I also noticed that
UDPPacket can be both IPv4 and IPv6, with either
.ipv6_saddr. That combined with the above lead me to 112b34da0648706850d3f780dc620c8de444c33e which I have some hope will fix this ticket once and for all. Wooh! I'm pushing it to
feature/stretch since we need wide exposure to get any confidence this bug indeed is fixed. I did successfully run the most relevant automated tests (
tor_enforcement.feature in particular) before pushing, so I doubt this will make the situation any worse.
#14 Updated by anonym over 1 year ago
- Status changed from In Progress to Fix committed
- Assignee deleted (
- % Done changed from 40 to 100
- QA Check changed from Ready for QA to Pass
I did this:
anonym@jenkins:~$ grep ip_saddr /var/lib/jenkins/jobs/test_Tails_ISO_*/builds/2017-03-*/archive/build-artifacts/debug.log | wc -l 43 anonym@jenkins:~$ grep ip_saddr /var/lib/jenkins/jobs/test_Tails_ISO_*/builds/2017-04-*/archive/build-artifacts/debug.log | wc -l 0
- the fix was pushed on March 20
- in March there was 43 occurrences of this issue
- in April none
=> I call this fixed!