[tor-bugs] #28780 [Core Tor/Tor]: circpadding: Add machine flag for not closing circuit if machine is active

[Tor Bug Tracker & Wiki]

#28780: circpadding: Add machine flag for not closing circuit if machine is active
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Very High                            |      Milestone:  Tor:
                                                 |  0.4.1.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  wtf-pad, tor-relay, tor-cell,        |  Actual Points:  6
  padding, 041-proposed, network-team-           |
  roadmap-2019-Q1Q2                              |
Parent ID:  #28634                               |         Points:  5
 Reviewer:  asn                                  |        Sponsor:
                                                 |  Sponsor2
-------------------------------------------------+-------------------------
Changes (by nickm):

 * status:  merge_ready => needs_revision


Comment:

 I want to talk about this approach more before we merge it.

 As I understand it, the idea here is that some circuits that would
 otherwise close should instead stay open longer, so that padding can be
 sent on them.  This patch achieves that by adding a call inside
 `circuit_mark_for_close` to `circpad_circuit_should_be_marked_for_close`,
 which potentially overrides the decision to mark the circuit, and changes
 its purpose instead.  Later, the circuit is supposed to get expired
 naturally when it times out.

 Here are some things that worry me about the logic:

 1. Intercepting `circuit_mark_for_close()` is pretty risky IMO.  It no
 longer 'does what it says on the label' and instead might keep the circuit
 open.  There are over 100 callers to circuit_mark_for_close(); I think it
 will be hard to audit them all.

 2. The function `circpad_circuit_should_be_marked_for_close()` is named as
 if it were a predicate function, but in fact it changes the circuit's
 purpose in some cases.  (Also, it doesn't answer the question "should this
 circuit be marked for close"; it answers "can this circuit be allowed to
 close, given that somebody else wants to mark it.")

 3. The function `circuit_expire_old_circuits_clientside()`, which we are
 relying on to kill of circuits eventually, uses `circuit_mark_for_close()`
 to close the circuits it wants to expire, and circuit_mark_for_close is
 now potentially overridden.

 Here's one possibility of we could do instead:

 1. Rename the new purpose from C_CIRCUIT_PADDING to
 CIRCUIT_PADDING_SHUTDOWN to make it clear that the circuit's purpose is
 "sending padding until it dies."

 2. Leave circuit_mark_for_close() untouched from the current codebase.
 Instead add a new function, `circuit_transition_to_shutdown()`.  This
 function should either mark the circuit for close immediately by calling
 circuit_mark_for_close(), or should change the circuit's purpose to
 CIRCUIT_PADDING_SHUTDOWN.

 3. Audit the codebase so that we change some calls from
 circuit_mark_for_close to circuit_transition_to_shutdown.  We should only
 do this for calls that use REASON_FINISHED or REASON_NONE or
 REASON_IP_NOW_REDUNDANT today.

 How does that sound?

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