Opened 3 years ago

Closed 2 years ago

#20029 closed enhancement (fixed)

prop224: Implementation of INTRODUCE1 and INTRODUCE_ACK cells

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, TorCoreTeam201612, review-group-15
Cc: Actual Points:
Parent ID: #17241 Points: 6
Reviewer: asn Sponsor: SponsorR-must

Description

Main ticket for the implementation of section 3.2 of proposal 224. This section specifies the format and how to handle INTRODUCE1 and INTRODUCE_ACK cells.

Note: this depends on #20004 for which we hope to get merged upstream _before_ this is completed.

Child Tickets

Change History (24)

comment:1 Changed 2 years ago by dgoulet

Keywords: TorCoreTeam201611 added; TorCoreTeam201609 removed

comment:2 Changed 2 years ago by dgoulet

Status: newaccepted

I've started implementation of this. Nothing final or even working but you can find the latest here: ticket20029_030_01.

This branch is based on #19043 but might be rebased at some point so don't be surprised.

comment:3 Changed 2 years ago by dgoulet

@asn, if you could look at this: https://gitlab.com/dgoulet/tor/merge_requests/14

The merge request is a diff between #19043 and this ticket.

It needs comment over the functions but I would like your opinion on the structure and how it looks then I'll add the comments and submit it for review.

comment:4 Changed 2 years ago by asn

Hmm, nice! This part was not that hard after all.

I did an initial review on gitlab to move this forward.

comment:5 Changed 2 years ago by dgoulet

Status: acceptedneeds_revision

comment:6 Changed 2 years ago by dgoulet

Keywords: TorCoreTeam201612 added; TorCoreTeam201611 removed

comment:7 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Oki, I've addressed all the things plus unit tests.

This branch is based on #19043 that needs revision so only consider first _six_ commits that is the one related to INTRODUCE1 for review. We'll have to wait for #19043 to be merged upstream before we can merge this one.

See branch ticket20029_030_03 and gitlab merge request: https://gitlab.com/dgoulet/tor/merge_requests/15

comment:8 Changed 2 years ago by dgoulet

Status: needs_reviewneeds_revision

Quick update, we'll wait until #19043 is merged before making this one in review so avoid complicated rebasing and confusion.

comment:9 Changed 2 years ago by dgoulet

Status: needs_revisionneeds_review

Oki! Now we have a clean branch: ticket20029_030_04

https://gitlab.com/dgoulet/tor/merge_requests/17

Code coverage for the INTRODUCE1 cell is 100%.

comment:10 Changed 2 years ago 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".

comment:11 Changed 2 years ago by arma

10) Is there any reason to keep an intro1 circuit open once it's received its intro1 cell? I notice we're closing it on nak. Why not close it on ack too? Is there anything that it can do after it gets its intro1 cell that we want to allow?

comment:12 in reply to:  10 Changed 2 years ago by dgoulet

Replying to 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

comment:13 in reply to:  11 Changed 2 years ago by dgoulet

Replying to arma:

10) Is there any reason to keep an intro1 circuit open once it's received its intro1 cell? I notice we're closing it on nak. Why not close it on ack too? Is there anything that it can do after it gets its intro1 cell that we want to allow?

Very good question! Current code doesn't close the client circuit after a ACK... I think it might be because before we used to allow multiple INTRODUCE1 cell so we let the client close it but it's not the case anymore so I believe closing it here is good thing to do. Thoughts?

comment:14 Changed 2 years ago by dgoulet

I've also rebased squashed all fixup commits from above review to make a clean merge request in Gitlab based on the those previous fixes.

See branch ticket20029_030_05
https://gitlab.com/dgoulet/tor/merge_requests/18

comment:15 Changed 2 years ago by nickm

Keywords: review-group-14 added

comment:16 Changed 2 years ago by asn

Hey,

I did another short pass of this branch. I ended up making a small commit that improves some documentation. See my branch ticket20029_030_05: https://gitweb.torproject.org/user/asn/tor.git/commit/?h=ticket20029_030_05

comment:17 Changed 2 years ago by dgoulet

Great thanks! I pushed your commit in the gitlab merge request. Re-freshener, the code is here (now including asn's commit):

See branch ticket20029_030_05
https://gitlab.com/dgoulet/tor/merge_requests/18

comment:18 Changed 2 years ago by asn

Reviewer: asn

comment:19 Changed 2 years ago by nickm

Keywords: review-group-15 added; review-group-14 removed

comment:20 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

comment:21 Changed 2 years ago by dgoulet

Every comment from nickm have been addressed with fixup commit in still in ticket20029_030_05

comment:22 Changed 2 years ago by dgoulet

Rebased on latest master and fixup commit squashed: ticket20029_030_06

Only resquash: ticket20029_030_06-resquash

comment:23 Changed 2 years ago by nickm

ticket20029_030_06-resquash is just what I was hoping for: the same end-result, only squashed:

[816]$ git diff dgoulet/ticket20029_030_05 dgoulet/ticket20029_030_06-resquash 
[817]$

Resolving minor merge conflicts now...

comment:24 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Done, merged. Woo!

Note: See TracTickets for help on using tickets.