Bug28738 #308
Bug28738 #308
Conversation
launch_tor is also parsing three sources of torrc options. Move the `extra_lines` torrc option set by a user configuration file to a function so that it can be tested. Add docstring and remove comments. Add sucess test.
User won't be able set options that have been already set. Fixes bug #28738. Bugfix v0.1.1
I think that the new check in parse_user_torrc_config is not exactly right; see review note.
sbws/util/stem.py
Outdated
| assert isinstance(existing_val, list) | ||
| existing_val.append(value) | ||
| torrc.update({key: existing_val}) | ||
| log.warn("The torrc option %s in `extra_lines` can not be set " |
So, it is not always right to reject a repeated option. There are some options (like Log) that you are allowed to set more than once, and others that you are not allowed to set more than once.
Also, the relative ordering of some options matters -- but I don't think any of those will come up for sbws, since they are all onion-service related.
(Actually, it is also okay to set SocksPort more than once -- tor just opens an extra socksport.)
@teor2345 commented that in the description of the ticket.
Operators can't override some of sbws' default torrc options (we can fix this in #28737)
If i understood them correctly, their idea was to solve this in #28737.
Since Log and SocksPort are common, maybe i can add an exception here for them.
Actually, why not just allow all options to appear multiple times, and then specifically block the ones that should not be replaced later on? There are a lot of items that can appear more than once. (grep for LINELIST in tor's src/app/config/config.c for a current list.)
I misunderstood stem.process.launch_tor_with_config(). When it gets an option with a list of arguments, it expands each argument with the option name.
So { "ORPort" : [ "auto", "9050" ] } becomes ORPort auto\nORPort 9050, in an arbitrary order. (See https://trac.torproject.org/projects/tor/ticket/28916 , which does not matter for sbws.)
Actually, why not just allow all options to appear multiple times, and then specifically block the ones that should not be replaced later on?
Tor already tells sbws when it doesn't like the options it was passed. Why should sbws duplicate the checks that tor already does?
Here's what I wrote on the ticket:
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.
But stem.process.launch_tor_with_config() doesn't accept a list, so we can't just combine all of sbws' option lines into a list.
So the current code in sbws master is the best we can do.
Let's remove commit 1aa93cf, and call this ticket done?
i haven't revert the commit to don't lose all the test.
I did an additional change in the fixup, that is to copy the dictionary and return a new one, so that the function doesn't have any side effect (always less confusing and less prone to bugs).
sbws/util/stem.py
Outdated
| @@ -140,8 +140,6 @@ def parse_user_torrc_config(torrc, torrc_text): | |||
| fail_hard('All torrc lines must have 2 or more words. "%s" has ' | |||
| 'fewer', line) | |||
| key, value = kv | |||
| log.info('Adding "%s %s" to torrc with which we are launching Tor', | |||
Maybe this could be useful as a log.debug()?
yes
|
squashed and merged |
No description provided.
The text was updated successfully, but these errors were encountered: