Opened 10 months ago

Closed 8 months ago

#28738 closed defect (fixed)

Stop merging multiple torrc options with the same name

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: nickm Sponsor:

Description

If sbws is configured with a default torrc option and an extra torrc option with the same name, then it merges the arguments in both options:
https://gitweb.torproject.org/sbws.git/tree/sbws/util/stem.py#n175

This is wrong in different ways, depending on the option

  1. "SocksPort auto 9050" is not a valid torrc option, the correct syntax is "SocksPort auto\nSocksPort 9050"
  2. "ExitPolicy accept *:80 accept *:443" is not a valid torrc option, the correct syntax is "ExitPolicy accept *:80,accept *:443" or "ExitPolicy accept *:80\nExitPolicy accept *:443"

Instead of trying to understand torrc options, sbws should just set them all at once, and let Tor sort out the details.
The stem.control.set_options() function takes a list of options, and applies them all at the same time.

There are a few issues with this approach:

  1. Operators can't override some of sbws' default torrc options (we can fix this in #28737)
  2. Some torrc options need to be set after tor has bootstrapped:
    • DisablePredictedCircuits, but we might remove it in #28701
    • LeaveStreamsUnattached, but I think it can be set on the command-line
  3. Some torrc options aren't supported by all tor versions (#28646, #28692), so they need to be set at runtime, and allowed to fail

Child Tickets

Change History (10)

comment:1 in reply to:  description Changed 10 months ago by juga

Reply was intended to reply other ticket

Last edited 10 months ago by juga (previous) (diff)

comment:2 Changed 10 months ago by juga

Status: newneeds_review

comment:3 Changed 9 months ago by dgoulet

Reviewer: nickm

comment:4 Changed 9 months ago by nickm

Status: needs_reviewneeds_revision

I left a comment on the PR. There are some options that _are_ allowed to appear more than once. For example, this is legal:

  Log debug file /foo/debug.log
  Log notice file /foo/notice.log

  SocksPort 9999
  SocksPort 9998

comment:5 Changed 9 months ago by teor

I left a comment on the PR:
https://github.com/torproject/sbws/pull/308#discussion_r243477846

I don't think sbws should duplicate Tor's option checks. Let's just put the options into the format that stem requires, and let Tor tell us if they worked.

comment:6 Changed 9 months ago by juga

Status: needs_revisionneeds_review

I made the modifications that teor suggested.
I should change the commits' messages when i squash, since they are not true with the fixups.

comment:7 Changed 8 months ago by nickm

Status: needs_reviewneeds_information

I think this is okay now, but in the long term we will want to fix the problem with option order not being preserved. We could have a new ticket for that, but it shouldn't block this patch IMO.

comment:8 Changed 8 months ago by nickm

Status: needs_informationmerge_ready

Oops, I meant to put this in merge_ready

comment:9 in reply to:  7 Changed 8 months ago by juga

Replying to nickm:

I think this is okay now, but in the long term we will want to fix the problem with option order not being preserved. We could have a new ticket for that, but it shouldn't block this patch IMO.

I created https://trac.torproject.org/projects/tor/ticket/29033

comment:10 Changed 8 months ago by juga

Resolution: fixed
Status: merge_readyclosed

Thanks, squashed and merged.

Note: See TracTickets for help on using tickets.