[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #8786 [Tor]: Add extra-info line that tracks the number of consensus downloads of each pluggable transports



#8786: Add extra-info line that tracks the number of consensus downloads of each
pluggable transports
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  karsten
     Type:  enhancement                          |         Status:
 Priority:  Low                                  |  needs_revision
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  pt, tor-bridge, flashproxy,          |        Version:
  026-triaged-1, 026-deferrable,                 |     Resolution:
  027-triaged-1-out                              |  Actual Points:
Parent ID:                                       |         Points:
  Sponsor:                                       |
-------------------------------------------------+-------------------------

Comment (by karsten):

 nickm, thanks a lot for the review!  I worked on those suggestion and
 pushed them to the same branch.  But let me start with another fix that I
 started working on before reading your review:

 '''tl;wr:'''  My branch (commit cda77b4) is broken, though only in a way
 that it logs something inaccurate, not in a harmful way.   I believe that
 neither the bridges' configurations nor the extended OR port
 implementation are to blame.  I wrote a fix for this.

 After adding extensive logging to all code having to do with dirreq_id and
 letting it run for hours, I found the issue with my branch (commit
 cda77b4).  Here's what happened:

  - The client sends a BEGIN_DIR cell on its circuit to the bridge.
  - The bridge assigns dirreq_id 9779 to the circuit and the circuit's
 p_chan, opens an edge connection and a dir connection, and assigns the
 same dirreq_id 9779 to both new connections.  It then sends back a cell
 (CONNECTED?) to the client and waits for the client to send its GET
 request via that circuit.
  - The client sends another BEGIN_DIR cell on the same circuit.
  - The bridge assigns dirreq_id 9780 to the circuit and its p_chan,
 '''overwriting previous value 9779'''.  It also creates a new edge
 connection and a new dir connection and sets their dirreq_id to 9780, but
 that's irrelevant here.
  - The client sends the GET request to the dir connection with dirreq_id
 9779, which is a request for a consensus.
  - The bridge attempts to find circuit with dirreq_id 9779 to find out the
 transport name of its p_chan, but there is no circuit with that dirreq_id
 anymore, because it was overwritten with 9780 above.  The bridge falls
 back to counting this request as coming in via the <OR> transport, which
 is wrong.  It should probably count it as <??>, but really we should fix
 this bug and count it as obfs3 in this case.
  - The client sends a second GET request for the bridge's server
 descriptor to the dir connection with dirreq_id 9780.

 I'm also pasting the logs below:

 {{{
 Feb 22 14:34:48.463 [notice] [...]
 Feb 22 14:34:48.673 [notice] XXX8786 Assigning new dirreq_id 9779 to
 circuit with p_circ_id 3181807694 and non-zero dirreq_id 9776 and to
 p_chan with global_identifier 5 and previous dirreq_id 9776.
 Feb 22 14:34:48.673 [notice] XXX8786 Assigning dirreq_id 9779 to new edge
 connection.
 Feb 22 14:34:48.674 [notice] XXX8786 Assigning dirreq_id 9779 to new dir
 connection.
 Feb 22 14:34:48.674 [notice] XXX8786 Assigning new dirreq_id 9780 to
 circuit with p_circ_id 3181807694 and non-zero dirreq_id 9779 and to
 p_chan with global_identifier 5 and previous dirreq_id 9779.
 Feb 22 14:34:48.674 [notice] XXX8786 Assigning dirreq_id 9780 to new edge
 connection.
 Feb 22 14:34:48.674 [notice] XXX8786 Assigning dirreq_id 9780 to new dir
 connection.
 Feb 22 14:34:48.675 [notice] XXX8786 Request for /tor/status-vote/current
 /consensus-microdesc/1AFF35+2C2591+E34739.
 Feb 22 14:34:48.675 [notice] XXX8786 Counting directory request for /tor
 /status-vote/current/consensus-microdesc/1AFF35+2C2591+E34739 with
 dirreq_id 9779 as <OR>, because we didn't find a circuit with matching
 dirreq_id.
 Feb 22 14:34:48.676 [notice] XXX8786 Request for /tor/server/authority.
 Feb 22 14:34:53.461 [notice] [...]
 }}}

 The bug here is that the dirreq_id-assigning code does not take concurrent
 directory requests over the same circuit into account.  We can expect that
 the stats based on dirreq_id, in particular "dirreq-v3-tunneled-dl" lines,
 are partially broken, too.  Fortunately, we're not using those lines for
 anything critical.  That's the bad news.

 The good news is that I wrote a fix for counting requests by transport.
 Please see commit a743f3a in the updated branch task-8786 in my public
 repository.  The commit message says what was fixed.  Pasting here, though
 it overlaps with the description above:

 {{{
     The previous approach to look up the transport name of a directory
     request by going through the list of circuits and finding the circuit
     with same dirreq_id was broken.  That dirreq_id may already have been
     overwritten by a newly arriving BEGIN_DIR cell before the HTTP GET
     request that we're about to include in the stats arrives and gets
     parsed.  This issue also affects "dirreq-v3-tunneled-dl" lines, but
     fixing those is independent of implementing this new feature.

     The new approach is to store the transport name in the locally created
     dir connection when processing the BEGIN_DIR cell.  That way it cannot
     be changed by a subsequent BEGIN_DIR cell on the same circuit.  The
     downside is a new char* in dir_connection_t, the upside is that we can
     avoid iterating over all circuits.
 }}}

 nickm, I also tried to address your suggestions in subsequent commit
 1508787.

 I would like to get this branch tested on asn's and mrphs' bridges and on
 an IPv6 bridge before getting it merged.  It also contains a temp commit
 for testing that '''must''' be reverted before merging!  Leaving this
 ticket at needs_revision until that is all done.  Deferring until 0.2.9 is
 fine by me.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/8786#comment:22>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs