Tails

#1 Updated by intrigeri 2014-08-26 23:14:13

• related to Feature #6305: Write tests for Tor bridges added

#2 Updated by intrigeri 2014-08-26 23:14:39

• Parent task set to Feature #6298

#3 Updated by intrigeri 2014-10-20 14:05:33

• blocks #8117 added

#9 Updated by anonym 2014-12-12 16:41:53

• Target version changed from Tails_1.2.2 to Tails_1.2.3

#10 Updated by anonym 2015-01-13 14:14:36

• Target version changed from Tails_1.2.3 to Tails_1.3

#11 Updated by anonym 2015-02-02 21:48:36

• Status changed from Confirmed to In Progress
• % Done changed from 0 to 20
• QA Check set to Dev Needed
• Feature Branch set to test/7821-tor

#13 Updated by anonym 2015-02-03 17:18:15

• % Done changed from 20 to 40

#16 Updated by anonym 2015-02-05 13:44:03

• Assignee changed from anonym to kytv
• % Done changed from 40 to 50
• QA Check changed from Dev Needed to Ready for QA

Note that some commits may seem unrelated. For instance, for

a8c2537 Hook transparent firewall leak checking via @check_tor_leaks tag.

we need:

366e7a2 Split VM networking into its own class, VMNet.

which in turn needed the fixes:

f8b116b Decouple VMStorage class from VM class.
fd61a7d Fix forgotten instance of VM class' s/update_domain/update/.
f9ce357 Improve encapsulation.

This will import most of the useful stuff from the good ol’ test/reorg and test/firewall-check-tag branches.

Also, because of

1a55905 Create the environment for remote shell calls more realisitcally.

the image tested muse be built from this branch!

#17 Updated by anonym 2015-02-05 13:45:03

• related to Feature #5644: Test suite: always check for firewall leaks added

#21 Updated by intrigeri 2015-02-09 10:27:29

• Assignee changed from kytv to anonym
• QA Check changed from Ready for QA to Dev Needed

• The iptables output parser scares me a bit. I’d rather see a grammar-based parser generator, but oh well, now the work has been done, let’s say it’s too late.
• I’m not sure what’s the value of “the firewall’s NAT rules only redirect traffic for Tor’s TransPort and DNSPort” without checking that the redirection to TransPort is limited to .onion mapped addresses, since we don’t want transparent proxying in general. It’s a bit confusing to read that step definition IMO. Not sure how to improve this without too much work, though.
• in “the firewall is configured to block all IPv6 traffic”, there’s an error message about “The NAT table” while we’re actually looking at the filter table.
• it feels weird to have “the firewall is configured to block all IPv6 traffic” checked in a feature that’s called “Tor is configured properly”. Maybe rename this feature torification.feature and adjust its title to match what we’re actually testing?
• I think that the title of the “untorified network connections to (\S+) fails” step should make it clear that it’s about HTTP only.
• My Ruby is no good, but in assert(!status.success? && status.stderr[expected_stderr], we’re checking that status.stderr includes expected_stderr, right? If that’s the case, then I suggest renaming the expected_stderr variable to expected_in_stderr so that it’s clearer what we’re actually checking.
• Instead of testing “untorified network connections to 1.2.3.4”, how about testing connections to an IP that has a web server listening on?
• Any reason not to call the destroy method in VMNet.clean_up instead of duplicating its code entirely?
• some code uses custom_sniffer that isn’t defined anywhere, and then it fails
• typo in “ESTABLISHED statate”
• in I re-run htpdate, maybe use && instead of ; between commands, so that it makes more sense to use execute_successfully?
• regarding I do a whois-lookup of domain, I’ve actually implemented a similar check in the torified_misc feature (introduced by my branch for Feature #5525) => we should merge these together at some point.
• now that the remote shell sets $PATH correctly, maybe we should stop running stuff in /usr/local/*bin with absolute paths? This way, we would also test $PATH implicitly.
• commit:f8b116b3aba199d0b4a00ffa8808bfaad24995ff adds a :mac attribute reader, which doesn’t seem to be used; buggy cherry-pick?

#24 Updated by anonym 2015-02-18 11:04:21

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Info Needed

intrigeri wrote:
> * The iptables output parser scares me a bit. I’d rather see a grammar-based parser generator, but oh well, now the work has been done, let’s say it’s too late.

I looked at a few of Ruby’s parser libraries, e.g. TreeTop (which is in Debian) and citrus (not in Debian), but it was my assessment that it would be a pretty large initial investment (+ more code) to make a parser that’s potentially more robust than than a simple state machine + heavy use of regex over full lines.

> * I’m not sure what’s the value of “the firewall’s NAT rules only redirect traffic for Tor’s TransPort and DNSPort” without checking that the redirection to TransPort is limited to .onion mapped addresses, since we don’t want transparent proxying in general. It’s a bit confusing to read that step definition IMO. Not sure how to improve this without too much work, though.

Ah, checking the destination was indeed forgotten. What about commit 10327cc? The step name is perhaps not the best still. What I wanted to do is the same as the (vague) manual test of looking at the output of iptables -t nat -L -n -v, and IMHO this step now does that adequately. What do you think?

> * in “the firewall is configured to block all IPv6 traffic”, there’s an error message about “The NAT table” while we’re actually looking at the filter table.

Whoops, that’s a copy-paste error. Fixed in commit 67aab00.

> * it feels weird to have “the firewall is configured to block all IPv6 traffic” checked in a feature that’s called “Tor is configured properly”. Maybe rename this feature torification.feature and adjust its title to match what we’re actually testing?

Well, it was in the “Tor” section of the manual test suite. :) Maybe tor.feature should be split into tor_stream_isolation.feature (all the “… SocksPort” scenarios") and tor_enforcement.feature (remaining scenarios)? Quite possibly it would make sense to merge the firewall_leaks.feature into tor_enforcement.feature since they’re just anti-tests.

> * My Ruby is no good, but in assert(!status.success? && status.stderr[expected_stderr], we’re checking that status.stderr includes expected_stderr, right?

Correct. I actually think I learned that trick when reviewing your work some time ago. :)

> If that’s the case, then I suggest renaming the expected_stderr variable to expected_in_stderr so that it’s clearer what we’re actually checking.

Fixed in commit 34a5a22.

> * I think that the title of the “untorified network connections to (\S+) fails” step should make it clear that it’s about HTTP only.

Sure, but I guess what I want the step to actually do is “untorified TCP connections … fail” since it’s not about HTTP particularly, so I might as well use netcat or whatever. So…

> * Instead of testing “untorified network connections to 1.2.3.4”, how about testing connections to an IP that has a web server listening on?

If so I guess we’d have to verify that a web server actually is listening on it, right? While that’s doable with a separate torified curl call it seems clunky.

What if we check that the firewall logged the dropped connection instead? OTOH that would be harder when a domain name is given instead. On second thought, though, the usage of a domain name vs IP address seems completely orthogonal to what I want to test. I think approach is better, so I’ve completely reworked this test in commit 5e6acef, and as a bonus we now also test that UDP (commit 30ae3bf) and ICMP (commit a32617a) packets are dropped.

> * some code uses custom_sniffer that isn’t defined anywhere, and then it fails

Fixed in commit 33a52a4.

> * typo in “ESTABLISHED statate”

Fixed in commit e02e06d.

> * in I re-run htpdate, maybe use && instead of ; between commands, so that it makes more sense to use execute_successfully?

Fixed in commit a1812ed.

> * regarding I do a whois-lookup of domain, I’ve actually implemented a similar check in the torified_misc feature (introduced by my branch for Feature #5525) => we should merge these together at some point.

Done in commit 3cc44d9.

> * now that the remote shell sets $PATH correctly, maybe we should stop running stuff in /usr/local/*bin with absolute paths? This way, we would also test $PATH implicitly.

Fixed in commit e998859.

> * commit:f8b116b3aba199d0b4a00ffa8808bfaad24995ff adds a :mac attribute reader, which doesn’t seem to be used; buggy cherry-pick?

Possibly, yeah. I had an idea of future use, but if so we’ll re-introduce them. Removed in commit 687b1b0. Their current names were not fitting in that class any way.

> * Back when we were on test/firewall-check-tag, to my request you introduced commit:894f78216861bb79a1ea36f1b5e5ebf2bbb2ac1d, and now it’s been left aside.

I’ve reworked this in another branch (Tor bridges, take 2) that I haven’t published yet. Will you take my word on that this will be fixed there?

> * Same for commit:942a030?

It’s already in commit 366e7a2.

> * My suggestion sent on 2013-08-19 in the “Please review’n’merge test/firewall-check-tag [was: Please review’n’merge test/reorg]” was apparently ignored.

As far as I could tell, none of them were relevant still.

> * Any reason not to call the destroy method in VMNet.clean_up instead of duplicating its code entirely?

destroy uses @net, which hasn’t been set before the first call to clean_up (that’s how it can be used to clean up instances remaining from previous test suite runs). I suppose destroy could call clean_up instead, which is funny since I just marked it private. OTOH, destroy is perhaps a poor choice of name since it easily can be confused with the destroy action in libvirt nomenclature. Perhaps destroy_and_undefine will make it self-documented (see below for your other concern)?

Hmm. Perhaps the most sensible thing is to remove clean_up and only have destroy/@destroy_and_undefine@ (as a public method) which does what clean_up did (i.e. lookup the domain by name)? What do you think?

> * In the “Please review’n’merge test/reorg” thread I’ve asked for documentation for the clean_up and destroy methods.
> * In commit:bdd57a05da7e30c33d8f0d47efde3d809b89b18b you had done some changes to follow-up on my review, and this seems to have been lost along the way.

Fixed for clean_up in commit e7dca50. As for destroy, would renaming it to destroy_and_undefine as suggested above be good enough? Otherwise, what is it that you want to be documented?

> * On 2013-08-19 in the “Please review’n’merge test/reorg” thread, I wrote “So please name it differently, else I might not be the only one to get confused :)” about the net variable. This hasn’t been addressed apparently.

Sorry, now fixed in commit e9b7b1a.

#27 Updated by intrigeri 2015-02-18 13:55:34

• Assignee changed from intrigeri to anonym
• % Done changed from 50 to 70
• QA Check changed from Info Needed to Dev Needed

#28 Updated by anonym 2015-02-18 15:23:55

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Impressive work!

Thanks, and thanks for the very thorough reviews!

> >> * it feels weird to have “the firewall is configured to block all IPv6 traffic” checked in a feature that’s called “Tor is configured properly”. Maybe rename this feature torification.feature and adjust its title to match what we’re actually testing?
>
> > Well, it was in the “Tor” section of the manual test suite. :) Maybe tor.feature should be split into tor_stream_isolation.feature (all the “… SocksPort” scenarios") and tor_enforcement.feature (remaining scenarios)? Quite possibly it would make sense to merge the firewall_leaks.feature into tor_enforcement.feature since they’re just anti-tests.
>
> I agree with this proposal, please go ahead.

Done in commits 83b5b29 and e19bce8.

> And then, perhaps torified_misc.feature that I’ve introduced can be merged into tor_enforcement.feature?

IMHO no. The tor_enforcement.feature should check that we cannot escape Tor, and tests that check that certain applications work when using Tor should be in some torified_*.feature", like torified_{browsing,gnupg}.feature. Makes sense? Otherwise, why not merging the latter two into tor_enforcement.feature@ as well?

> >> * Same for commit:942a030?
>
> > It’s already in commit 366e7a2.
>
> OK. I missed it since some bits are still not there, but perhaps there’s a good reason for it, so I’ll point them out and you can explain:
>
> * the VMNet.{ip,mac,bridge_name} accessors have been left aside, and bridge_name is still an attribute of the VMNet class, where we copy data on update, instead of being delegated to the libvirt network object with an accessor; IMO that’s worse separation of concerns than what commit:942a030 implemented

The VMNet.{ip,mac} accessors were already removed in commit 687b1b0. As for VMNet.bridge_name, now fixed in commit 37533f6, I hope.

> * the VM class still has :ip and :mac listed in attr_reader. Are they useful there?

Oops, those were forgotten. Removed in commit a510880.

> > OTOH, destroy is perhaps a poor choice of name since it easily can be confused with the destroy action in libvirt nomenclature. Perhaps destroy_and_undefine will make it self-documented (see below for your other concern)?
>
> Good idea.
>
> > Hmm. Perhaps the most sensible thing is to remove clean_up and only have destroy/@destroy_and_undefine@ (as a public method) which does what clean_up did (i.e. lookup the domain by name)? What do you think?
>
> Good idea.
>
> > As for destroy, would renaming it to destroy_and_undefine as suggested above be good enough?
>
> Yes.

All of these done in commit db1993b.

> Then I took a look at your recent commits, and I see:
>
> […]
>
> => the error message won’t work for ICMP, since port is nil in that case, right?

You’re right, good catch! Fixed in commit 1498d14.

#29 Updated by intrigeri 2015-02-18 23:53:33

• Assignee changed from intrigeri to anonym
• % Done changed from 70 to 90
• QA Check changed from Ready for QA to Info Needed

Code review passes. Full test suite passes except expected failures and:

  @keep_volumes
  Scenario: Upgrading an old Tails USB installation from an ISO image, running on the old version # features/usb_install.feature:223
    Given a computer                                                                              # features/step_definitions/common_steps.rb:64
    And I clone USB drive "old" to a new USB drive "to_upgrade"                                   # features/step_definitions/usb.rb:51
    And I setup a filesystem share containing the Tails ISO                                       # features/step_definitions/usb.rb:139
    When I start Tails from USB drive "old" with network unplugged and I login                    # features/step_definitions/common_steps.rb:165
      Remote shell seems to be down (RuntimeError)
      ./features/support/helpers/exec_helper.rb:19:in `rescue in wait_until_remote_shell_is_up'
      ./features/support/helpers/exec_helper.rb:14:in `wait_until_remote_shell_is_up'
      ./features/support/helpers/vm_helper.rb:357:in `wait_until_remote_shell_is_up'
      ./features/step_definitions/common_steps.rb:242:in `/^the computer (re)?boots Tails$/'
      ./features/step_definitions/common_steps.rb:175:in `/^I start Tails from (.+?) drive "(.+?)"(| with network unplugged) and I login(| with(| read-only) persistence password "([^"]+)")$/'
      features/usb_install.feature:227:in `When I start Tails from USB drive "old" with network unplugged and I login'

And on the second try (only running usb_install.feature this time), Scenario: Installing an old version of Tails to a pristine USB drive freezes after completing the installation.

I’ve not run the test suite on that box for a while, so I don’t remember if such problems usually happen there. I didn’t check if that was a regression either. anonym, any chance this is related to some of the VM class refactoring you’ve done on that branch? If not, I’ll just go ahead and merge.

#31 Updated by anonym 2015-02-19 11:09:38

• Assignee changed from anonym to intrigeri

intrigeri wrote:
> Code review passes. Full test suite passes except expected failures and:
>
> […]
> @ Remote shell seems to be down (RuntimeError)@
> […]
>
> I’ve not run the test suite on that box for a while, so I don’t remember if such problems usually happen there. I didn’t check if that was a regression either. anonym, any chance this is related to some of the VM class refactoring you’ve done on that branch? If not, I’ll just go ahead and merge.

I doubt the VM class refactoring affects this, since it only should change things in between scenarios and features, but this happen deep in a scenario at the ‘the computer boots Tails’ step.

However, perhaps my changes to the remote shell server in has something to do with it? In particular, for e

So the exact situation where we got this failure is (in the ‘the computer boots Tails’ step) when we’ve seen TailsGreeter.png and we run VMCommand.wait_until_remote_shell_is_up, which sends a true (but it could really be anything, even an invalid command) to the remote shell and, and simply waits for an answer for up to 30 seconds. I used to see errors often years ago, and that’s Feature #5491, but I haven’t seen it for like a year.

However, it seems suspicious that you and Alan (Feature #5491#note-6) suddenly see this issue again after I’ve modified the remote shell server, per commit 1a55905. The only change that should be relevant is that each remote shell call will result in an extra shell command run server-side before the requested command, namely 'su -c env ' + user, which is used to create an appropriate environment. That extra call will also happen to VMCommand.wait_until_remote_shell_is_up, so if there’s a problem there, it’s affected too. However, what could it be? Could 'su -c env ' + user some how fail because we’re too early in the boot process, i.e. something isn’t guaranteed to be set up by our boot process at this time, i.e. when Tails Greeter has just started? Any ideas?

I’m at a loss here. If this branch makes Feature #5491 worse, merging it is a bad idea. I suggest you re-run the full test suite multiple times and see if you get this error again.

#34 Updated by anonym 2015-02-19 20:53:01

• related to deleted (Feature #6305: Write tests for Tor bridges)

#35 Updated by anonym 2015-02-19 20:53:05

• blocks Feature #6305: Write tests for Tor bridges added

#36 Updated by anonym 2015-02-20 13:24:43

• QA Check changed from Info Needed to Ready for QA

We’ve discussed this off list, and hopefully commit dc77fae fixed intrigeri’s issue.

#37 Updated by intrigeri 2015-02-20 19:21:51

• QA Check changed from Ready for QA to Info Needed

anonym wrote:
> We’ve discussed this off list, and hopefully commit dc77fae fixed intrigeri’s issue.

Unfortunately not: I’ve seen Remote shell seems to be down (RuntimeError) again today while running the test suite from testing + test/7821-tor (at commit:dc77fae) merged into it. Re-running it with --debug flag to see if that commit sometimes helps or not. I had never seen this problem before, so it seems clear to me that this branch still makes the remote shell thing worse :(

#38 Updated by kytv 2015-02-20 20:17:03

• blocks Feature #8702: Write regression test for Unsafe Browser update checks added

#41 Updated by intrigeri 2015-02-21 08:36:12

• Assignee changed from intrigeri to anonym

anonym wrote:
> Argh! Ok. Assuming it’s the change I made to the remote shell server (commit 1a55905, which is my best bet here) that introduced this regression some how, it’s actually not at all a critical part of this branch. What if you revert all these commits
> […]
> and build a new image from the branch (actually, 1.3~rc1 should do) and then run it in the test suite on that branch?

I can do that, but my 2nd --debug run of usb_install.feature passed again, so I can’t efficiently evaluate how reverting these commits help (I’d have to run usb_install.feature a lot of times, which would take days. Instead, I’d rather use the specific test feature you gave me a few days ago in order to first evaluate how often the bug arises with these commits, and then do it again with these commits reverted. Unfortunately the pastebin thing has expired => could you please send me this feature again?

#42 Updated by intrigeri 2015-02-21 10:42:45

• Assignee changed from anonym to intrigeri

OK, I wrote a test feature that installs Tails on a USB virtual drive and boots from it 20 times (since I’ve mostly seen the bug when booting off emulated USB), and then I could reproduce it again. I see calling as root: echo 'hello?' in the debug log, and this call doesn’t return anything I can see. I’ve taken control of the TailsToaster VM, and verified that the autotest_remote_shell.py process is running. Sadly, the way we’re starting it doesn’t log anything, and we’re running SysV init, so I can’t check if it was started before GDM, and anything that autotest_remote_shell.py may print is probably entirely lost.

What’s clear, though, is that /etc/rc2.d/S05rc.local is meant to start after /etc/rc2.d/S03gdm3, so I’m not surprised that we have a race condition here, and sometimes the remote shell isn’t up before we see our Greeter welcome screen.

I’m now going to try and revert the commits that slightly change the remote shell behaviour (and might be enough to introduce a regression). Reverting these commits might be good enough for 1.3, but that’s not a good enough mid-term solution IMO (in some settings, it seems obvious that we will see this bug again): what we really need is to make sure we start the remote shell soon enough. I think that should be addressed in 1.3.1, and probably needs a dedicated ticket. That should be relatively easy. We should at the very least:

• migrate the remote shell startup to a dedicated initscript, that acts as a no-op unless autotest_never_use_this_option is passed on the kernel command-line
• set X-Start-Before: $x-display-manager gdm gdm3 in the remote shell initscript

And also, ideally:

• in autotest_remote_shell.py, touch some file after port.open() has succeeded
• patch the gdm3 initscript to wait for that file to appear, when autotest_never_use_this_option is passed on the kernel command-line, before it actually starts GDM.

(That last part is ugly but can beneficially be replaced by systemd’s daemon activation feedback protocol, in Tails/Jessie.)

#48 Updated by anonym 2015-02-22 20:07:05

• related to Bug #8941: The remote shell server is racing against Tails Greeter added

#49 Updated by intrigeri 2015-02-22 20:49:34

• QA Check changed from Info Needed to Ready for QA

#50 Updated by Tails 2015-02-22 22:42:51

• Status changed from In Progress to Fix committed
• % Done changed from 90 to 100

Applied in changeset commit:40d4c1bf9fc5cc2f251e0f546afd82f79c91169f.

#51 Updated by intrigeri 2015-02-22 22:44:27

• Assignee deleted (intrigeri)
• % Done changed from 100 to 90
• QA Check changed from Ready for QA to Pass

#52 Updated by BitingBird 2015-02-24 22:50:05

• Status changed from Fix committed to Resolved