Opened 6 months ago

Closed 7 days ago

#28693 closed defect (fixed)

Add an option to disable circuit padding

Reported by: teor Owned by:
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: wtf-pad, tor-relay, tor-cell, padding, 041-must
Cc: nickm Actual Points: 0.33
Parent ID: #28634 Points: 0.5
Reviewer: asn Sponsor: Sponsor2

Description

We need an option like ConnectionPadding to disable circuit padding.
Do we also need ReducedCircuitPadding for mobile?

Child Tickets

TicketStatusOwnerSummaryComponent
#28610closedwill WTF-PAD impair bandwidth scanning?Core Tor/Tor

Change History (28)

comment:1 Changed 4 months ago by asn

Milestone: Tor: 0.4.0.x-finalTor: unspecified

comment:2 Changed 4 months ago by mikeperry

If we do a ReducedCircuitPadding, we should have a way to specify which padding machines are acceptable for use when it is set.

comment:3 Changed 4 months ago by mikeperry

Keywords: 041-proposed added

comment:4 Changed 4 months ago by mikeperry

Points: 0.5

comment:5 Changed 4 months ago by mikeperry

Priority: MediumHigh

comment:6 Changed 4 months ago by nickm

Sponsor: Sponsor2

comment:7 Changed 4 months ago by nickm

Resolution: fixed
Status: newclosed

The answer here is "no -- not once #28693 is implemented".

comment:8 in reply to:  7 Changed 4 months ago by teor

Replying to nickm:

The answer here is "no -- not once #28693 is implemented".

I don't understand.
This ticket is #28693, and it's not implemented.

comment:9 Changed 4 months ago by teor

Resolution: fixed
Status: closedreopened

I still think we need a CircuitPadding option.

comment:10 Changed 4 months ago by teor

For example:
sbws doesn't want padding on its circuits, because padding interferes with bandwidth measurements.

comment:11 Changed 7 weeks ago by teor

Keywords: 041-must added; 041-proposed removed

comment:12 Changed 5 weeks ago by mikeperry

Parent ID: #28632#28634

See also #30173. Both the consensus param and the torrc option should play nice with eachother.

comment:13 Changed 5 weeks ago by teor

Status: reopenedneeds_revision

We have two similar options, ConnectionPadding and CircuitPaddingDisabled.

In the current branch, 0 means "connection padding off" but "circuit padding on". Which makes it easier for a developer or user to make a mistake.

But it's hard to make the options entirely consistent, because ConnectionPadding has 3 options (force on, negotiate, force off), but CircuitPadding has 2 (negotiate, force off).

Here's one possible design (let's call it A):

0 auto 1
ConnectionPadding off negotiate on
CircuitPadding off negotiate can't force on: reject config as invalid

Here is another (let's call it B):

0 auto 1
ConnectionPadding off negotiate on
CircuitPadding off no third state: reject config as invalid negotiate

In A, ConnectionPadding and CircuitPadding are consistent with each other: 0 means off and auto means negotiate.

In B, CircuitPadding is consistent with other 2-state torrc options, because it uses 0 and 1. But negotiate is auto in ConnectionPadding and 1 in CircuitPadding.

Do we have any existing options that are (0, auto) or (1, auto)?

  • If we don't, we shouldn't add one.
  • If we do, I guess it's ok, as long as we document it.

comment:14 Changed 5 weeks ago by mikeperry

Actual Points: 0.33
Status: needs_revisionneeds_review

Ok, I took approach B, since we don't have any autobools that don't support 1. https://github.com/torproject/tor/pull/965

That PR contains fixes for #28693, #30173, and #29203. I recommend that the same reviewer review all three tickets -- they are extremely similar and related code (hence all in the same branch with the same test; doing these separetely would involve needless extra mental context switching and extra communication).

comment:15 Changed 5 weeks ago by teor

Status: needs_reviewneeds_revision

Both Travis and Appveyor CIs failed on https://github.com/torproject/tor/pull/965

comment:16 Changed 5 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.4.1.x-final

These tickets all have code in the same pull request, so I'm assuming they're all 0.4.1.

comment:17 Changed 5 weeks ago by mikeperry

Status: needs_revisionneeds_review

CI issues fixed in the PR.

comment:18 Changed 4 weeks ago by asn

Reviewer: asn

comment:19 Changed 4 weeks ago by asn

Status: needs_reviewneeds_revision

Comment left in #30173.

comment:20 Changed 3 weeks ago by mikeperry

Status: needs_revisionneeds_review

comment:21 Changed 3 weeks ago by asn

Status: needs_reviewneeds_revision

comment:22 Changed 3 weeks ago by cypherpunks

please add bool options

CircuitPadding

and

ReducedCircuitPadding


comment:23 in reply to:  22 Changed 2 weeks ago by mikeperry

Replying to cypherpunks:

please add bool options

CircuitPadding

and

ReducedCircuitPadding

Is there some reason you are saying this? This is already implemented in the branch for this ticket.

comment:24 Changed 2 weeks ago by mikeperry

Status: needs_revisionneeds_review

comment:25 Changed 11 days ago by asn

Status: needs_reviewmerge_ready

comment:26 Changed 7 days ago by nickm

Can I please have a torspec branch for the consensus parameters?

comment:27 Changed 7 days ago by nickm

Ah, there is one. NM.

comment:28 Changed 7 days ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged1

Note: See TracTickets for help on using tickets.