Tails

#2 Updated by intrigeri 2015-07-03 00:41:14

Tentatively assigned to kytv for 1.5 like the parent ticket, feel free to triage as you wish.

#6 Updated by kytv 2015-07-03 06:43:49

This ticket currently depends on 2f25a19 from Feature #9518, the custom exception for execute_successfully.

#9 Updated by kytv 2015-07-05 11:40:44

intrigeri wrote:
> Thanks! It seems to me that the proposed branch retries for all kinds of whois failures, and not only the ones about IP rate limiting, which means that we’re going to mask failures I’d rather catch — I have to say I don’t like this much, but perhaps I misunderstood the idea?

Perhaps it was me who misunderstood. I thought retrying whenever it failed would be good because any failure would be due to rate limiting errors or errors communicating with whichever whois server we hit, that is, not errors that ‘we’ can actually fix.

> [….]

> What do you think?

I’ll update this based on the suggestions.

#10 Updated by intrigeri 2015-07-06 01:16:00

> Perhaps it was me who misunderstood. I thought retrying whenever it failed would be good because any failure would be due to rate limiting errors or errors communicating with whichever whois server we hit, that is, not errors that ‘we’ can actually fix.

Well, a test suite should not assume that our side of things is perfect, and any error is caused by the Internet / Tor / the remote server. I’m all for retrying when a specific transient error that we can’t fix on our side is detected, but not with “oh it failed, let’s retry” in the general case.

#12 Updated by kytv 2015-07-06 10:56:28

This is an example of what I was referring to:

call returned: [0, "", ""]
spawning as root: restart-vidalia
spawn returned: [0, "", ""]
calling as amnesia: whois 'torproject.org'
call returned: [2, "", "Timeout.\n"]
Command failed: whois 'torproject.org'
error code: 2
stderr: Timeout.

<false> is not true.
  Scenario: whois(1) should work and go through Tor.                           # features/torified_misc.feature:83
    When I successfully query the whois directory service for "torproject.org" # features/step_definitions/torified_misc.rb:1
      ExecutionFailedInVM (ExecutionFailedInVM)
      ./features/support/helpers/vm_helper.rb:382:in `rescue in execute_successfully'
      ./features/support/helpers/vm_helper.rb:378:in `execute_successfully'
      ./features/step_definitions/torified_misc.rb:6:in `/^I successfully query the whois directory service for "([^"]+)"$/'
      features/torified_misc.feature:84:in `When I successfully query the whois directory service for "torproject.org"'
    Then the whois standard output contains "The Tor Project"                  # features/step_definitions/torified_misc.rb:28

#14 Updated by kytv 2015-07-08 13:10:12

intrigeri wrote:
> > This is an example of what I was referring to:
>
> Good, this gives an example for “I’m all for retrying when a specific transient error that we can’t fix on our side is detected” => please catch this specific error (looking for Timeout. in stderr) and retry. Either in this ticket/branch, or file a dedicated one and reassign this one to me for QA.

On it.

and another exception I’ll allow for:

spawn returned: [0, "", ""]
calling as amnesia: whois 'torproject.org'
call returned: [2, "", "[Jul 08 19:48:00] ERROR torsocks[6046]: Unable to resolve. Status reply: 4 (in socks5_recv_resolve_reply() at socks5.c:657)\ngetaddrinfo(whois.pir.org): Non-recoverable failure in name resolution\n"]
Command failed: whois 'torproject.org'
error code: 2
stderr: [Jul 08 19:48:00] ERROR torsocks[6046]: Unable to resolve. Status reply: 4 (in socks5_recv_resolve_reply() at socks5.c:657)
getaddrinfo(whois.pir.org): Non-recoverable failure in name resolution
. 

#15 Updated by intrigeri 2015-07-08 14:34:33

> On it.

:)

> and another exception I’ll allow for:

Totally makes sense to me!

#16 Updated by kytv 2015-07-08 16:16:24

intrigeri wrote:

> It also makes the the whois command is successful step useless, since it now uses vm.execute_successfully (for good reasons AFAICT), so the resulting test suite seems inconsistent to me.

As I’m going into this a bit further, it seems I’ll have to revert back to using vm.execute since we want to capture stderr.

As I see it:

execute_successfully will return true on success and false on failure. When true, stdout can be captured. When LIMIT EXCEEDED is in the whois results, whois exits 0 since whois did its job by returning what the server sent.

On failure, such as with the Timeout or Unable to resolve errors, as expected whois does not exit 0. Since whois exits non-zero, vm_execute_res will be false, causing vm_execute_res.stderr to be undefined.

spawning as root: restart-vidalia
spawn returned: [0, "", ""]
calling as amnesia: whois 'torproject.org'
call returned: [2, "", "Timeout.\n"]
Command failed: whois 'torproject.org'
error code: 2
stderr: Timeout.

<false> is not true.
  Scenario: whois(1) should work and go through Tor.                           # features/torified_misc.feature:47
    When I successfully query the whois directory service for "torproject.org" # features/step_definitions/torified_misc.rb:1
      undefined method `stderr' for nil:NilClass (NoMethodError)
      ./features/step_definitions/torified_misc.rb:12:in `rescue in block in <top (required)>'
      ./features/step_definitions/torified_misc.rb:5:in `/^I successfully query the whois directory service for "([^"]+)"$/'
      features/torified_misc.feature:48:in `When I successfully query the whois directory service for "torproject.org"'
    Then the whois standard output contains "The Tor Project"                  # features/step_definitions/torified_misc.rb:32

So—unless I’m wrong about this—I’ll revert to using execute for whois at least.

#17 Updated by intrigeri 2015-07-09 09:22:24

> execute_successfully will return true on success and false on failure.

Really? I thought it raised an exception on failure.
In any case, yes, using execute is the way to go whenever you want to analyze stderr, iirc.

#18 Updated by kytv 2015-07-09 11:13:11

intrigeri wrote:
> > execute_successfully will return true on success and false on failure.
>
> Really? I thought it raised an exception on failure.

Yes, that’s exactly right. I commented/posted too late at night.

> In any case, yes, using execute is the way to go whenever you want to analyze stderr, iirc.

Thanks for confirming, that coincides my experience(s).

#21 Updated by kytv 2015-07-11 10:44:41

intrigeri wrote:
> Great job, one can see that you put a lot of attention to details in this branch \o/
>

yay

> My only remaining comments are design ones, so let me first state that I’m not fully up-to-date wrt. the design patterns you and anonym settled on, and also I lack some of the big picture nowadays, so some of my comments may totally be disputable: I’m not necessarily more expert than you in this area anymore :)
>
> […]
>
> Why two hash chars? (sorry if my Ruby is poor; hint: it is :)

The second one is there so I can print a literal #, such as

Forcing new Tor circuit... (attempt #4)

>
> […]
>
> I’m not convinced by the soundness of having a function (itself called by steps) calling a step… especially when that step is currently run only from that function. I suggest to move all the logic from the step to the function, and then turn the step into a thin wrapper around the function: this way, we avoid the “calling steps from steps” ugliness in current code (steps can instead call the function directly), but still we keep the possibility open to add the step to a scenario in the future.

I’m glad you agree. :) I considered that after setting to RfQA and I changed it locally but didn’t commit and push yet.

>
> […]
>
> Why not a dedicated exception class for ‘LIMIT EXCEEDED’? It feels inconsistent with the fact we’re introcing one (WhoisLookupFailure) for the fallback kind of failure.

Touché. To be addressed.

>
> To end with I’m not convinced with the changes to the wget scenarios: one now has only a When step, which feels weird when reading the gherkin text. Also, the others now have only When and And, and in the current state of things the latter should be a Then instead. What benefit do we get from these changes? (except consistency with the whois scenarios, but that’s consistency with suboptimal design — half of the actual test is done in a When step, which is contrary to cucumber best practices — so IMO that wouldn’t be a good reason per-se).

That was done for Bug #9715 so that wget can be retried for transient errors.

(As for the suboptimal design..hmm..I’m not yet seeing how else to handle the retrying; my ruby-fu is still fairly weak).

#22 Updated by kytv 2015-07-11 11:48:09

kytv wrote:
> intrigeri wrote:

> > To end with I’m not convinced with the changes to the wget scenarios: one now has only a When step, which feels weird when reading the gherkin text. Also, the others now have only When and And, and in the current state of things the latter should be a Then instead. What benefit do we get from these changes? (except consistency with the whois scenarios, but that’s consistency with suboptimal design — half of the actual test is done in a When step, which is contrary to cucumber best practices — so IMO that wouldn’t be a good reason per-se).
>
> That was done for Bug #9715 so that wget can be retried for transient errors.
>
> (As for the suboptimal design..hmm..I’m not yet seeing how else to handle the retrying; my ruby-foo is still fairly weak).

To be clear: I didn’t know how to do the retrying without combining the steps due to vm_execute_res.success? being False when either whois timeout:s or the domain name fails to resolve.

#23 Updated by intrigeri 2015-07-11 13:50:09

>> (As for the suboptimal design..hmm..I’m not yet seeing how else to handle the retrying; my ruby-foo is still fairly weak).

> To be clear: I didn’t know how to do the retrying without combining the steps due to vm_execute_res.success? being False when either whois timeout:s or the domain name fails to resolve.

Here’s the way I would (try to) do it: the responsibility of the step that runs whois would only be to run whois, as many times needed to hide the specific errors that we have identified as being transient; it would pass as long as whois completes without raising one of those errors, after the specified maximum amount of retries, and regardless of its exit code. And the next steps (Then) would merely handle checking 1. the exit code; 2. the stdout for the expected content… just like they do in current devel. Of course I’m probably missing something that makes this hard, ugly, or even impossible.

#24 Updated by kytv 2015-07-12 00:22:14

intrigeri wrote:
> Here’s the way I would (try to) do it:

This should have been obvious to me it wasn’t right away: It’s not (always) wrong to ignore an exception that you yourself are raising; the next step will check success anyway.

Sigh. Sorry for the obtuseness.

-When /^I successfully query the whois directory service for "([^"]+)"$/ do |domain|
+When /^I query the whois directory service for "([^"]+)"$/ do |domain|
   next if @skip_steps_while_restoring_background
   @new_circuit_tries = 0
   until @new_circuit_tries == $config["MAX_NEW_TOR_CIRCUIT_RETRIES"] do
@@ -11,13 +11,11 @@ When /^I successfully query the whois directory service for "([^"]+)"$/ do |doma
         raise WhoisLookupFailure
       end
       break
-    rescue WhoisLookupFailure => e
+    rescue WhoisLookupFailure
       if @vm_execute_res.stderr['Timeout'] || \
          @vm_execute_res.stderr['Unable to resolve'] || \
          @vm_execute_res.stdout['LIMIT EXCEEDED']
         force_new_tor_circuit
-      else
-        raise e
       end
     end
   end

I think this and reverting my “combining commit” will address this design flaw. Testing.

#27 Updated by kytv 2015-07-13 01:27:35

Applied in changeset commit:5a9700169f7f0700b72acd95bdd574f2188451a6.

#28 Updated by kytv 2015-07-13 01:27:36

Applied in changeset commit:384daf878a62b2d1510dd7b29cd5bf217d9df847.

#31 Updated by cypherpunks 2015-08-05 09:38:56

Couldn’t an adversary deliberately cause WHOIS lookups from exit nodes to fail in an attempt to compel the creation of new Tor cicuit?

#32 Updated by intrigeri 2015-08-05 12:08:49

> Couldn’t an adversary deliberately cause WHOIS lookups from exit nodes to fail in an attempt to compel the creation of new Tor cicuit?

Yes, likely some adversaries can do that.
Did you notice that this landed in our test suite?