[tor-bugs] #25061 [Core Tor/Tor]: Relays consider it a bootstrapping failure if they can't extend for somebody else's circuit

[Tor Bug Tracker & Wiki]

#25061: Relays consider it a bootstrapping failure if they can't extend for
somebody else's circuit
-------------------------------------------------+-------------------------
 Reporter:  arma                                 |          Owner:
                                                 |  catalyst
     Type:  defect                               |         Status:
                                                 |  assigned
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.3.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  backport-032, 033-must, stability,   |  Actual Points:
  033-triage-20180320, 033-included-20180320     |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by arma):

 The hint is that this warn happens through
 control_event_bootstrap_prob_or() which is in four places in
 connection_or.c. That call happens when an OR connection fails, and none
 of those four callsites check whether it's an origin circuit, that is,
 whether it's "our" circuit or somebody else's.

 You'll notice that somewhere along the line we wrapped those calls with
 {{{
    if (!authdir_mode_tests_reachability(options)
 }}}

 Commit d37fae2f is the commit from ancient history to skim, and commit
 818332e7 is the more recent commit in this area that should give you some
 more context.

 The fix might start with:

 * In connection_or.c, when we're considering whether to call
 control_event_bootstrap_prob_or, break that "if
 authdir_mode_tests_reachability" check out its into own function, called
 something like "could this connection be a bootstrap attempt", which
 checks if it's a reachability test, but also does more checks.

 The trouble here is that in these "more checks", we want to check if there
 are any origin circuits pending on that connection attempt. And after some
 digging it looks like we don't actually know that here.

 So the first option I thought of is that in that "could it be" function,
 we want to call circuit_get_all_pending_on_channel(), and then iterate
 over the resulting list to see if any of them are CIRCUIT_IS_ORIGIN().
 That's certainly the most straightforward solution, in that it doesn't go
 mess with other code much. But it's kind of crummy, because we're walking
 another list (twice, in fact) every time there's a connection failure.
 It's not *so* bad though, because somewhere along the line somebody went
 to the trouble of making a separate list just for the circuits that are
 pending a channel attempt -- lucky us.

 My second thought is: wait, we already *do* walk that list, later on, when
 we're trying to handle all of the circuits that were waiting for this
 channel to succeed or fail. That happens in circuit_n_chan_done(). So in
 the
 {{{
       if (!status) { /* chan failed; close circ */
 }}}
 clause in that loop, we could check if it's an origin circuit, and if so
 set a flag that makes us do a bootstrap complaint if the flag was ever set
 to 1.

 But! circuit_n_chan_done() gets called from
 connection_or_about_to_close(), which is way after we know *why* the conn
 closed. So that's still not best.

 Could we add a field to or_connection_t which was why the connection
 failed, and memorize it when it failed, and then use it during
 circuit_n_chan_done() to report bootstrap issues accurately? Maybe --
 let's call that '''approach one'''.

 A third option is: how about we set a flag in or_connection_t, which is
 "has an origin circuit ever been interested in my outcome", and that flag
 starts off as 0, and it gets set to 1 when we're about to add a circuit to
 {{{circuits_pending_chans}}} on a circuit where {{{CIRCUIT_IS_ORIGIN()}}}
 is true.

 But! We only add to {{{circuits_pending_chans}}} inside
 {{{circuit_set_state()}}}, and that function doesn't know which chan or
 conn we're planning to attach this circ to.

 The better callsite would be
 circuit_handle_first_hop(), which is entirely for origin circuits. There
 are two cases we need to handle here. One is when there isn't a good conn
 available, and we decide to launch one in this circuit. That's the easy
 case: if channel_connect_for_circuit() returns an n_chan, we set the "we
 wanted this channel for an origin circuit" flag on it right then. The
 other is if channel_get_for_extend returned NULL but didn't set
 should_launch, meaning there is a conn somewhere out there, but it's not
 ready yet -- and we can't easily get ahold of it from this function.

 For that second case, check out channel_get_for_extend(). There are two
 places that call it: circuit_handle_first_hop(), which is for origin
 circuits, and circuit_extend(), which is for handling EXTEND cells so it
 is explicitly not for origin circuits. If we added a flag to that
 function, to say whether it's an origin circuit we're asking on behalf of,
 then *it* can mark conns that had origin circuits asking about them. The
 only case I can see in that function where we would need to mark the chan
 is when we're incrementing n_inprogress_goodaddr.

 So, let's call that "add a flag to the channel and turn it on when an
 origin circuit inquired" idea '''approach two'''.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25061#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online