Stop merging multiple torrc options with the same name

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 legacy/trac#28737 (moved))
2. Some torrc options need to be set after tor has bootstrapped:

• DisablePredictedCircuits, but we might remove it in legacy/trac#28701 (moved)
• LeaveStreamsUnattached, but I think it can be set on the command-line

3. Some torrc options aren't supported by all tor versions (legacy/trac#28646 (moved), legacy/trac#28692 (moved)), so they need to be set at runtime, and allowed to fail

changed milestone to %sbws: 1.0.x-final in legacy/trac

added component::core tor/sbws in Legacy / Trac milestone::sbws: 1.0.x-final in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac reviewer::nickm in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac labels

Reply was intended to reply other ticket

https://github.com/torproject/sbws/pull/308

Trac:
Status: new to needs_review

Trac:
Reviewer: N/A to nickm

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

Trac:
Status: needs_review to needs_revision

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.

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_information

Oops, I meant to put this in merge_ready

Trac:
Status: needs_information to merge_ready

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

Thanks, squashed and merged.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

moved from legacy/trac#28738 (moved)

added Bug label and removed 1 deleted label

removed 1 deleted label