Opened 6 months ago

Closed 3 months ago

#28692 closed defect (fixed)

sbws should set ConnectionPadding 0

Reported by: teor Owned by:
Priority: Medium Milestone: sbws: 1.0.x-final
Component: Core Tor/sbws Version:
Severity: Normal Keywords:
Cc: juga, teor Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description

There's no reason to pad connections in sbws: sbws is not trying to avoid netflow logs.
Padding can waste bandwidth, and reduce scanner accuracy,

Child Tickets

Change History (21)

comment:1 Changed 6 months ago by teor

Now here's a challenge: sbws should set ConnectionPadding 0 if the option is supported by Tor.
If the option isn't supported by Tor, then sbws should just continue.

You can test with Tor 0.2.9 (no padding support) and Tor 0.3.3 or later (padding support).

Here's a few things to try:

  • set the config option, and ignore the exception that stem returns for unsupported config options
  • use GETINFO config/names to see if the option is supported

comment:2 Changed 6 months ago by teor

See also #28694, where we'll also want to set CircuitPadding 0 (or whatever the option ends up being called).

comment:3 Changed 6 months ago by juga

Status: newneeds_review

comment:4 Changed 6 months ago by teor

Reviewer: teor
Status: needs_reviewneeds_revision

This patch makes the torrc options in sbws even more complicated.

Here's what sbws has right now:

  1. a list of hard-coded torrc options in globals.py: https://github.com/torproject/sbws/blob/master/sbws/globals.py#L10
  2. a list of configured torrc options in stem.py, and the hard-coded option LearnCircuitBuildTimeout 0: https://github.com/torproject/sbws/blob/master/sbws/util/stem.py#L127
  3. a custom list of torrc options that the user can configure for launch: https://github.com/torproject/sbws/blob/master/sbws/util/stem.py#L142
  4. a list of hard-coded torrc options in stem.py, that can't be set until after sbws has bootstrapped: https://github.com/torproject/sbws/blob/master/sbws/util/stem.py#L197

Then the pull request adds custom code to ignore the ConnectionPadding option if tor doesn't support it. This code is more complicated than it needs to be, and it has bugs.

Here's a simpler design:

  • a list of torrc options in stem.py, containing 1., 2. and 4., with a flag saying when the option should be applied to tor (1. and 2. on launch, 4. after launch)
  • a custom list of torrc options that the user can configure for launch (same as 3.)

(There's an even simpler design in #28712 that makes 3. into a torrc file, but it's a breaking change.)

Once we refactor the torrc option code, we can add:

  • a flag to ignore failures when applying the option after launch
  • a torrc option table entry: ConnectionPadding, 0, apply after launch, ignore failures

comment:5 Changed 6 months ago by juga

Which reminds me other refactoring i've been thinking for long ago: #28718.
Again, changing all the options system would be backwards-incompatible and would take same time.

comment:6 in reply to:  4 ; Changed 6 months ago by juga

Replying to teor:
[...]

Here's a simpler design:

  • a list of torrc options in stem.py,

Even if only stem.py uses these options, i think they should be in globals.py, since we might need to change them and globals.py should be the place where to change sbws defaults.

containing 1., 2. and 4., with a flag saying when the option should be applied to tor (1. and 2. on launch, 4. after launch)

  • a custom list of torrc options that the user can configure for launch (same as 3.)

(There's an even simpler design in #28712 that makes 3. into a torrc file, but it's a breaking change.)

There isn't a stem torrc syntax parser. This code (https://github.com/torproject/sbws/blob/master/sbws/util/stem.py#L157) was implementing part of it, it's not complete but was working with well-formatted torrc lines.
I found that the parser for stem configuration (https://stem.torproject.org/api/util/conf.html) is very close to what a torrc parser would need, yet might not support all the cases (https://github.com/torproject/tor/blob/master/doc/torrc_format.txt).

Options:

  1. we use stem.util.conf, let tor launch fail when options can't be parsed and inform the operator
  2. we extend stem.util.conf to become a torrc parser
  3. we implement a full torrc parser
  4. we accept torrc options only in the form of an INI file, which would be parsed by ConfigParser and converted to the stem's torrc dictionary (i'm not sure which cases might fail here either)

comment:7 Changed 6 months ago by juga

I think at this stage, to be able to progress, 1. is the fastest solution with hopefully less failures

comment:8 in reply to:  6 ; Changed 6 months ago by teor

Replying to juga:

Replying to teor:
[...]

Here's a simpler design:

  • a list of torrc options in stem.py,

Even if only stem.py uses these options, i think they should be in globals.py, since we might need to change them and globals.py should be the place where to change sbws defaults.

Let's leave any refactoring until #28737.

containing 1., 2. and 4., with a flag saying when the option should be applied to tor (1. and 2. on launch, 4. after launch)

  • a custom list of torrc options that the user can configure for launch (same as 3.)

(There's an even simpler design in #28712 that makes 3. into a torrc file, but it's a breaking change.)

There isn't a stem torrc syntax parser. This code (https://github.com/torproject/sbws/blob/master/sbws/util/stem.py#L157) was implementing part of it, it's not complete but was working with well-formatted torrc lines.
I found that the parser for stem configuration (https://stem.torproject.org/api/util/conf.html) is very close to what a torrc parser would need, yet might not support all the cases (https://github.com/torproject/tor/blob/master/doc/torrc_format.txt).

Why write a parser, when tor will parse lines for us?

Options:

  1. we use stem.util.conf, let tor launch fail when options can't be parsed and inform the operator
  2. we extend stem.util.conf to become a torrc parser
  3. we implement a full torrc parser
  4. we accept torrc options only in the form of an INI file, which would be parsed by ConfigParser and converted to the stem's torrc dictionary (i'm not sure which cases might fail here either)

Option 4 is #28737, and it should work reasonably well. But it's a new feature, so it belongs in sbws 1.1.

Here's another option:

  1. Fix sbws' current tor option parsing code:
    • allow options with no argument (#28715)
    • stop trying to merge sbws options with the same name (#28738), until we refactor in sbws 1.1 (#28737)

Then tell the operator if tor doesn't like the options.

Now the option bugs are dealt with in other tickets, let's talk about this ticket.

Here's a simple algorithm for ignoring failing options:

  1. sbws has OPTIONS, a hard-coded list of options and values, and CAN_FAIL, a hard-coded, ordered list of options that are allowed to fail
  2. Try launching tor with OPTIONS
  3. If tor fails to launch:
    • if CAN_FAIL is not empty, remove the first option in CAN_FAIL from OPTIONS, and go to step 2
    • otherwise, exit sbws with an error
  4. Log a message with the Tor version, and the CAN_FAIL options that were removed from OPTIONS

Let's put this new code in a new function. launch_tor is already almost 100 lines long.

Last edited 6 months ago by teor (previous) (diff)

comment:9 Changed 6 months ago by juga

Fix sbws' current tor option parsing code

Do we know which possible options an operator might need the "extra_lines" for?.
We only need them to use the testing network.

comment:10 in reply to:  9 Changed 6 months ago by teor

Replying to juga:

Fix sbws' current tor option parsing code

Do we know which possible options an operator might need the "extra_lines" for?.
We only need them to use the testing network.

Some operators might want a fixed SOCKSPort.
Some sbws tors might need custom options to bootstrap in their environment.

We could ask the current operators if they use extra_lines.

comment:11 in reply to:  8 ; Changed 6 months ago by juga

Replying to teor:

Here's a simple algorithm for ignoring failing options:

Wouldn't be easier to try to add them at runtime?. I think that was your suggestion in some other ticket.

comment:12 in reply to:  11 ; Changed 5 months ago by teor

Replying to juga:

Replying to teor:

Here's a simple algorithm for ignoring failing options:

Wouldn't be easier to try to add them at runtime?. I think that was your suggestion in some other ticket.

Setting ConnectionPadding would work at runtime. Are there any options that need to be set at launch, and could fail?

comment:13 in reply to:  12 ; Changed 5 months ago by juga

Replying to teor:

Are there any options that need to be set at launch, and could fail?

None of the ones that we currently set had fail. Is there an easy way to check that?.

comment:14 Changed 5 months ago by juga

Status: needs_revisionneeds_review

I created a new PR https://github.com/torproject/sbws/pull/316, since after https://trac.torproject.org/projects/tor/ticket/28692#comment:6 it's solved in a different way.
For now i've added only ConnectionPadding, but we can easily add more options to the dictionary.

comment:15 in reply to:  13 Changed 5 months ago by teor

Replying to juga:

Replying to teor:

Are there any options that need to be set at launch, and could fail?

None of the ones that we currently set had fail. Is there an easy way to check that?.

ConnectionPadding will fail on Tor 0.2.9.

comment:16 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

I made some suggestions on the pull request.

comment:17 Changed 5 months ago by juga

Status: needs_revisionneeds_review

I made the modifications that you suggested.

comment:18 Changed 3 months ago by juga

Merged in a branch with current master and current needs_review tickets and tested a whole loop with the public Tor network

comment:19 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

The code looks good.

I left some comments on the pull request.
Please feel free to squash and merge after you've dealt with them.

comment:20 Changed 3 months ago by juga

Status: needs_revisionmerge_ready

comment:21 Changed 3 months ago by juga

Resolution: fixed
Status: merge_readyclosed

Merged, thanks.

Note: See TracTickets for help on using tickets.