prop224: Implementation of INTRODUCE1 and INTRODUCE_ACK cells
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 (moved) for which we hope to get merged upstream before this is completed.
Activity
changed milestone to %Tor: 0.3.0.x-final
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 (moved) but might be rebased at some point so don't be surprised.
Trac:
Status: new to accepted-
@asn, if you could look at this: https://gitlab.com/dgoulet/tor/merge_requests/14
The merge request is a diff between #19043 (moved) 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.
-
Oki, I've addressed all the things plus unit tests.
This branch is based on #19043 (moved) 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 (moved) to be merged upstream before we can merge this one.
See branch
ticket20029_030_03and gitlab merge request: https://gitlab.com/dgoulet/tor/merge_requests/15Trac:
Status: needs_revision to needs_review -
Quick update, we'll wait until #19043 (moved) is merged before making this one in review so avoid complicated rebasing and confusion.
Trac:
Status: needs_review to needs_revision -
Oki! Now we have a clean branch:
ticket20029_030_04https://gitlab.com/dgoulet/tor/merge_requests/17
Code coverage for the INTRODUCE1 cell is 100%.
Trac:
Status: needs_revision to needs_review 
In hs_intro_received_introduce1:
s/of one circuit with one client/since one circuit with one client/
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:
What does
make the circuit that it has seen one INTRODUCE1 cellmean?log_warn(LD_PROTOCOL, "Rejecting %s INTRODUCE1 cell. "might want to be a LOG_PROTOCOL_WARN like in that recent other commit I saw?
The
validate the cell that is the expectedphrase also wants some fixing up. Same withRelay down the cell onto the service intro circuit. Anda client can only send once an INTRODUCE1 cell.log_warn(LD_PROTOCOL, "No intro circuit found for INTRODUCE1 cell " "with auth key %s from circuit %" PRIu32Is this really a LD_PROTOCOL situation? Also, why LCOV_EXCL that part -- isn't this normal code that will get triggered in normal operation?
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?
-
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? -
circuit_is_suitable_for_introduce1() calls circuit_is_suitable_intro_point() which has surprising warning messages about "Rejecting ESTABLISH_INTRO".
-
Replying to arma:
In hs_intro_received_introduce1:
s/of one circuit with one client/since one circuit with one client/
Fixup in
67216f6Is 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 invalidin 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` -
Replying to arma:
- 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?
-
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_05https://gitlab.com/dgoulet/tor/merge_requests/18 
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-
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_05https://gitlab.com/dgoulet/tor/merge_requests/18 mentioned in issue #21266 (moved)
moved to tpo/core/tor#20029 (closed)
mentioned in issue tpo/core/tor#21266 (closed)
