Opened 2 months ago

Last modified 3 weeks ago

#28780 needs_revision defect

circpadding: Add machine flag for not closing circuit if machine is active

Reported by: asn Owned by:
Priority: Very High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: wtf-pad, tor-relay, tor-cell, padding, 041-proposed
Cc: nickm Actual Points:
Parent ID: #28632 Points: 5
Reviewer: Sponsor: Sponsor2

Description

Child Tickets

Change History (16)

comment:1 Changed 6 weeks ago by asn

I attempted to implement this feature in my branch bug28780: https://github.com/torproject/tor/pull/630

The branch is based on adaptive_padding-final from #28142 and contains tests.

Let me know if you like the approach :)

comment:2 Changed 6 weeks ago by asn

Status: newneeds_review

comment:3 Changed 6 weeks ago by asn

Potential issue with branch above: What happens if one side closes the circuit anyhow, while the other side (with the machine enabled) keeps it alive even if the other side is closed?

comment:4 Changed 6 weeks ago by dgoulet

Reviewer: mikeperry

comment:5 Changed 6 weeks ago by mikeperry

Status: needs_reviewneeds_revision

Needs revision wrt to the one-side close issue and my comments in the PR.

comment:6 Changed 5 weeks ago by asn

Status: needs_revisionmerge_ready

I pushed Mike's latest branch here so that CI runs: https://github.com/torproject/tor/pull/644

I'm OK with the purpose-based approach, but now I see that we are using both my "special flag" and also the "special purpose" so we moved from one special thing to two. In this case, can you please motivate the new purpose further in the docs of CIRCUIT_PURPOSE_C_CIRCUIT_PADDING. IIUC, the new purpose is so that Tor does not attach
new streams to the circuit, or are there more reasons?

If that's the only reason, perhaps it would be wise to ask feedback from Nick to see if a new purpose is the best way to achieve that goal.

comment:7 Changed 5 weeks ago by asn

Status: merge_readyneeds_revision

Oops, marked as merge_ready before, switching to needs_rev.

Last edited 5 weeks ago by asn (previous) (diff)

comment:8 Changed 5 weeks ago by mikeperry

Status: needs_revisionneeds_review

Ok I switched to checking the purpose instead of the flag, and removed the flag.

While thinking about #28634, I started to realize we may need more here. We may want to flip this back to C_GENERAL isntead of closing it. Or at least think about how we would make this circuit behave more like it was real. Not sure about hidden service circuits, but general circuits are marked dirty and allowed to sit around for 10 minutes after use before being closed. I am not sure the best way to do this, though. Maybe we do need some kind of timer that does not send a packet but just causes a state transition.. Bleh.

I'm also getting more and more pessimistic about doing this or #28634 by Tuesday.. I think it is more important that what we deploy is sane and good rather than just does something. I think if we act early enough in 0.4.1 to make more reasonable machines, we'll have plenty of testing.

comment:9 in reply to:  8 Changed 5 weeks ago by asn

Replying to mikeperry:

I'm also getting more and more pessimistic about doing this or #28634 by Tuesday.. I think it is more important that what we deploy is sane and good rather than just does something. I think if we act early enough in 0.4.1 to make more reasonable machines, we'll have plenty of testing.

Agreed. Let's relax and focus on #28142 till then.

comment:10 Changed 5 weeks ago by asn

Milestone: Tor: 0.4.0.x-finalTor: unspecified

comment:11 Changed 5 weeks ago by mikeperry

Reviewer: mikeperry
Status: needs_reviewneeds_revision

Setting this to needs_revision while we think about what to do about the ditry and other time-based teardowns.

comment:12 Changed 5 weeks ago by mikeperry

Keywords: 041-proposed added

comment:13 Changed 5 weeks ago by mikeperry

Points: 5

(We've done about 2 points on this so far.. maybe 3 more to go, depending on the time thing?)

comment:14 Changed 5 weeks ago by mikeperry

Priority: MediumImmediate

comment:15 Changed 5 weeks ago by mikeperry

Priority: ImmediateVery High

comment:16 Changed 3 weeks ago by nickm

Sponsor: Sponsor2
Note: See TracTickets for help on using tickets.