[tor-bugs] #20029 [Core Tor/Tor]: prop224: Implementation of INTRODUCE1 and INTRODUCE_ACK cells

[Tor Bug Tracker & Wiki]

#20029: prop224: Implementation of INTRODUCE1 and INTRODUCE_ACK cells
------------------------------------------------+--------------------------
 Reporter:  dgoulet                             |          Owner:  dgoulet
     Type:  enhancement                         |         Status:
                                                |  needs_review
 Priority:  High                                |      Milestone:  Tor:
                                                |  0.3.0.x-final
Component:  Core Tor/Tor                        |        Version:
 Severity:  Normal                              |     Resolution:
 Keywords:  tor-hs, prop224, TorCoreTeam201612  |  Actual Points:
Parent ID:  #17241                              |         Points:  6
 Reviewer:                                      |        Sponsor:
                                                |  SponsorR-must
------------------------------------------------+--------------------------

Comment (by arma):

 In hs_intro_received_introduce1:

 1)
 s/of one circuit with one client/since one circuit with one client/

 2)
 Is the
 {{{
   int ret;
   if (introduce1_cell_is_legacy(request)) {
     /* Handle a legacy cell. */
     ret = rend_mid_introduce_legacy(circ, request, request_len);
   } else {
     /* Handle a non legacy cell. */
     ret = handle_introduce1(circ, request, request_len);
   }
   return ret;
 }}}
 pattern better than getting rid of the variable and just returning in
 those two cases?
 Compare to how validate_introduce1_parsed_cell has a 'goto invalid' which
 just does a return -1 -- that function could also just return -1 each time
 it's decided it's uphappy. But even if you don't like returning early, you
 might want to pick which of these patterns you like more.

 In handle_introduce1:

 3)
 What does {{{make the circuit that it has seen one INTRODUCE1 cell}}}
 mean?

 4)
 {{{
     log_warn(LD_PROTOCOL, "Rejecting %s INTRODUCE1 cell. "
 }}}
 might want to be a LOG_PROTOCOL_WARN like in that recent other commit I
 saw?

 5)
 The {{{validate the cell that is the expected}}} phrase also wants some
 fixing up. Same with {{{Relay down the cell onto the service intro
 circuit}}}. And {{{a client can only send once an INTRODUCE1 cell}}}.

 6)
 {{{
       log_warn(LD_PROTOCOL, "No intro circuit found for INTRODUCE1 cell "
                             "with auth key %s from circuit %" PRIu32
 }}}
 Is this really a LD_PROTOCOL situation? Also, why LCOV_EXCL that part --
 isn't this normal code that will get triggered in normal operation?

 7)
 {{{
   if (send_introduce_ack_cell(client_circ, status)) {
 }}}
 Will you be happier if you check "< 0" explicitly here, to make it clearer
 that it's the failure case?

 8) Proposal 224 doesn't include {{{HS_INTRO_ACK_STATUS_CANT_RELAY =
 0x0003}}}. Are we thinking of this as a sort of generic "protocol error,
 sorry, something went wrong"? Does it leak anything interesting if we send
 back a 3 rather than a 2, based on the current implementation?

 9) circuit_is_suitable_for_introduce1() calls
 circuit_is_suitable_intro_point() which has surprising warning messages
 about "Rejecting ESTABLISH_INTRO".

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