[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 dgoulet):

 Replying to [comment:10 arma]:
 > In hs_intro_received_introduce1:
 >
 > 1)
 > s/of one circuit with one client/since one circuit with one client/

 Fixup in `67216f6`

 >
 > 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.

 Right so let me explain why I used `goto invalid` in the validate
 function. The main reason is because for a validation function, in my
 experience, multiple return sites are error prone over time but with a
 goto label you have strong semantic on what is invalid and what is not and
 can follow the progression. My two cents, some people like more return
 call-site. :)

 Back to this case, that function should have 1 and only 1 goal is to route
 the request to the right handler so this is why I put a single return
 statement for errors and a very obvious single return statement sending
 back the result of the handling as it's the _only_ place that the function
 should be "done", after a handler. Adding a "done" label makes it less
 "beautiful" I would say that there is one single good sequential path to
 follow which would look like this:

 {{{
   /* We are sure here to have at least DIGEST_LEN bytes. */
   if (introduce1_cell_is_legacy(request)) {
     /* Handle a legacy cell. */
     ret = rend_mid_introduce_legacy(circ, request, request_len);
     goto end;
   }
   /* Handle a non legacy cell. */
   ret = handle_introduce1(circ, request, request_len);
  end:
   return ret;
 }}}

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

 That's a really good question! I think it's an artifact of flagging
 `already_received_introduce1`.

 Fixup commit: `e7359cb`

 >
 > 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?

 Fixup commit: `ce2c5c7`

 >
 > 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}}}.
 >

 Fixup commit: `1147220`

 > 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?

 Indeed... those LCOV shouldn't be there at all! Fixup commit in `4b6e6a0`
 and `4fa514d`

 >
 > 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?

 Indeed! Fixup commit: `6895d12`

 >
 > 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?

 Yeah that was one of the question I had for you. I have a spec patch to
 add that value which would inform the client that the service is actually
 unreachable as the other error don't have that semantic at all. Worried?

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

 Good catch. Fixup commit: `339beab`

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