Opened 3 months ago

Last modified 4 weeks ago

#27841 merge_ready defect

Close intro circuit after introduction has been completed

Reported by: asn Owned by: neel
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs dos 033-backport, 034-backport
Cc: neel, asn Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

We are currently leaving inroduction circuits open after intro has been done, even tho we ignore any further introductions on that circuit.

In the future, we can consider closing that circuit to decrease the load on the network.

Child Tickets

Change History (20)

comment:1 Changed 2 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 2 months ago by neel

Should I close the intro circuit once INTRODUCE2 cells have been parsed and processed? If so, is hs_circ_handle_introduce2() a good place to close the intro circuits?

comment:3 Changed 2 months ago by neel

Never mind Comment 2. After reading rend-spec-v2.txt in torspec, I think it should be done in hs_service_receive_intro_established() instead. Is this correct?

UPDATE: To be more descriptive: should I call circuit_mark_for_close() in hs_service_receive_intro_established() on a successful call and not just on a failure?

Last edited 2 months ago by neel (previous) (diff)

comment:4 Changed 2 months ago by asn

Hey neel, currently stuck in post-meeting backlog. Will try to get to this late next week. Thanks!

comment:5 Changed 8 weeks ago by asn

Hey neel. I think that's not right. We want the service to keep its intro circuits open so that it receives multiple introductions through them.

On the other hand, on the client-side, after the client completes the introduction with the service, we want the client to close the client-side part of the introduction circuit.

So in C <-> IP <-> HS we want the link from C to IP to close after the introduction has been completed. Good luck! :)

comment:6 Changed 8 weeks ago by neel

Thanks for the feedback.

I looked at handle_introduce_ack_success() and found this code:

 end:
  /* We don't need the intro circuit anymore. It did what it had to do! */
  circuit_change_purpose(TO_CIRCUIT(intro_circ),
                         CIRCUIT_PURPOSE_C_INTRODUCE_ACKED);
  circuit_mark_for_close(TO_CIRCUIT(intro_circ), END_CIRC_REASON_FINISHED);

  /* XXX: Close pending intro circuits we might have in parallel. */
  return;

Isn't this already being done after the first comment above (or in handle_introduce_ack_success())?

comment:7 Changed 8 weeks ago by dgoulet

Cc: asn added
Status: assignedneeds_information

Yes client side we are fine. I think this ticket was more on the introduction point side?

Now I just realized something that is maybe bad in v3 (not in v2).

See handle_introduce1() (hs_intropoint.c). Notice at the end that we only close the circuit if we send a NACK but not a ACK. Actually, it should be the opposite! The reason is that if you ACK, then the client will close that circuits so instead of waiting for another round trip for the DESTROY cell, the IP can just send it after the ACK and the client will likely close it much faster.

Now, why we shouldn't close with a NACK? Because, in case of a NACK, the client will use the same circuit to re-extend to a new IP. If the current IP is closing the circuits, that re-extend is most likely failing... So the whole "reextend on NACK" optimization is rendered useless by closing the circuit on NACK on the intro side.

To summarize (all of this intro point side):

  • Close circuit on ACK
  • Keep circuit on NACK.

Thoughts?

Last edited 8 weeks ago by dgoulet (previous) (diff)

comment:8 in reply to:  7 Changed 8 weeks ago by asn

Status: needs_informationnew

Replying to dgoulet:

Yes client side we are fine. I think this ticket was more on the introduction point side?

Now I just realized something that is maybe bad in v3 (not in v2).

See handle_introduce1() (hs_intropoint.c). Notice at the end that we only close the circuit if we send a NACK but not a ACK. Actually, it should be the opposite! The reason is that if you ACK, then the client will close that circuits so instead of waiting for another round trip for the DESTROY cell, the IP can just send it after the ACK and the client will likely close it much faster.

Now, why we shouldn't close with a NACK? Because, in case of a NACK, the client will use the same circuit to re-extend to a new IP. If the current IP is closing the circuits, that re-extend is most likely failing... So the whole "reextend on NACK" optimization is rendered useless by closing the circuit on NACK on the intro side.

To summarize (all of this intro point side):

  • Close circuit on ACK
  • Keep circuit on NACK.

Thoughts?

Hm, that's interesting.

I think your suggestion makes sense!

However, I think it would be great after we write this patch, we also test that this "reextend on NACK" optimization works as intended.

comment:9 Changed 8 weeks ago by neel

Status: newassigned

comment:11 Changed 7 weeks ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final

comment:12 Changed 6 weeks ago by neel

Status: assignedneeds_review

comment:13 Changed 5 weeks ago by dgoulet

Reviewer: dgoulet

comment:14 Changed 5 weeks ago by dgoulet

Keywords: 033-backport 034-backport added
Status: needs_reviewneeds_revision

So this is a fix that probably applies back to 0.3.2.1-alpha I believe. Meaning the changes file is wrong and the branch should be based on maint-0.3.3 because I realized we are most likely to backport this fix since the re-extend optimization is busted.

Now, on another note, I've discuss it with arma further on IRC and maybe actively closing the circuit is not ideal as in "we want to let the client decide what to do".

Thus, it would simplify the patch, we just need to NOT close the circuit on NACK and that is it. We'll backport that fix.

Sorry neel for the confusion. Thanks!

comment:15 Changed 5 weeks ago by neel

A PR based on maint-3.3 which includes your suggestions is here: https://github.com/torproject/tor/pull/487

If you want it based on master, tell me and I can base the patch off that instead.

comment:16 Changed 5 weeks ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:17 Changed 5 weeks ago by neel

Status: needs_revisionneeds_review

comment:18 Changed 4 weeks ago by dgoulet

Status: needs_reviewmerge_ready

Big thanks neel!

ACK.

comment:19 Changed 4 weeks ago by asn

LGTM!

comment:20 Changed 4 weeks ago by dgoulet

Milestone: Tor: 0.4.0.x-finalTor: 0.3.4.x-final

Merged into 035 and master.

Note: See TracTickets for help on using tickets.