Skip to content
This repository has been archived by the owner on Mar 2, 2022. It is now read-only.

Change tor config before tor is launched #85

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

juga0
Copy link
Collaborator

@juga0 juga0 commented Feb 6, 2018

to don't get error about options that can't be changed once it's launched.

An alternative to #80, as commented in #83

@coveralls
Copy link

coveralls commented Feb 6, 2018

Coverage Status

Coverage remained the same at ?% when pulling b9ad1a5 on juga0:feature/change_config_before_tor_run into 92abe44 on TheTorProject:develop.

@ln5
Copy link

ln5 commented Feb 6, 2018

FWIW, I've successfully run this branch with the following patch:

--- a/bwscanner/attacher.py
+++ b/bwscanner/attacher.py
@@ -151,6 +151,7 @@ def connect_to_tor(launch_tor, circuit_build_timeout, control_port=None,
         'FetchUselessDescriptors': 1,
         'FetchDirInfoEarly': 1,
         'FetchDirInfoExtraEarly': 1,
+        'DisableDebuggerAttachment': 0,
     }

     if tor_overrides:

@DonnchaC
Copy link
Member

Are we sure that the options are being set when connecting to an existing Tor daemon? Setting DisableDebuggerAttachment should fail and raise an exception when being setting on a running Tor but this does not seem to be happening?

Maybe the options are not being applied when calling tor.config.save() after connect to Tor?

@meejah
Copy link
Member

meejah commented Feb 11, 2018

If options aren't being set on save() that's a huge bug -- that's basically the whole point of TorConfig. txtorcon doesn't do any special handing here and Tor IME does one of two things: gives you a 500 level error and aborts the entire SETCONF or decides it can't continue at all and exits (which seems exceedingly rude, but .. I think there's good reasons? For example, this can happen if ownership/permissions are weird on new hidden-service-directories.).

I don't know if there's any cases where it ignores something in a SETCONF but continues anyway.

@meejah
Copy link
Member

meejah commented Feb 11, 2018

I just tried on a running Tor with carml: if you're setting DisableDebuggerAttachment to the value it already has, nothing happens (tor continues and you get OK from the SETCONF). If you're changing it, you get a 553 error.

@juga0 juga0 changed the title Change tor config before is launched Change tor config before tor is launched Feb 11, 2018
@juga0
Copy link
Collaborator Author

juga0 commented Feb 11, 2018

@meejah independently on whether the options are really set or an error is raised when connecting to tor via SOCKS port, talking today with @DonnchaC we thought that we should only support launching tor (and not connecting to), as that allows setting specific options and won't interfere with the running tor process.

@meejah
Copy link
Member

meejah commented Mar 19, 2018

Okay, launching your own Tor sounds safest. In that case, you can set any options you want in a TorConfig object and pass it into the txtorcon.launch() function as _tor_config=

BTW .. the intent of the leading underscore there is that maybe if you need to do that, txtorcon should add features to launch(), so if the only thing you need is the debugging-attachement please file an feature-request on txtorcon for that to be added as a kwarg. :)

@juga0
Copy link
Collaborator Author

juga0 commented Mar 20, 2018

I forgot to comment here that i realized we still need to support to connect to tor for the chutney tests. Unless we find a way launch tor in a chutney fashion, what it seems to me like rewriting chutney functionality from bwscanner, though @teor2345 will know better.

@juga0
Copy link
Collaborator Author

juga0 commented Mar 20, 2018

In any case, tor configuration is changed in this PR in the way @meejah suggests before launching tor: ccddac6#diff-5d97531ed54d2f8c4d91ebce0fb91774R170

Copy link
Member

@meejah meejah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but see inline comments.

tor = yield txtorcon.launch(reactor)
# Launch tor with config, in order to don't get CONF_CHANGED when
# updating options that can't be changed while tor is running.
tor = yield txtorcon.launch(reactor, _tor_config=tor_config)
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When launching tor like this (and passing in the config) you shouldn't have to assign the object nor do .save() (as below).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, .save() is not being call when launching tor, in this part of the if, it's call when connecting to tor in the else below, as you point out.

# TODO: check whether CONF_CHANGED will happen here or not because
# we get the state later
tor.config = tor_config
tor.config.save()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't really assign to this in the Tor object. Nor should you have to save() (all config will have been passed in to tor on the command-line). If you do call .get_config() anywhere else, it will return the same object (but, you should use the async method to retrieve the config in case it's not available yet).

BTW, do you know which config-options you're passing in that aren't provided by launch(), or is it "maybe anything at all that Tor supports"..?

Copy link
Collaborator Author

@juga0 juga0 Mar 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options we're passing are (https://github.com/juga0/bwscanner/blob/ccddac6f20ce7963e8fc2e8d9921f672da6be390/bwscanner/attacher.py#L146):

# Options for spawned or running Tor to load the correct descriptors.
tor_options = {
    'LearnCircuitBuildTimeout': 0,  # Disable adaptive circuit timeouts.
    'CircuitBuildTimeout': circuit_build_timeout,
    'UseEntryGuards': 0,  # Disable UseEntryGuards to avoid PathBias warnings.
    'UseMicroDescriptors': 0,
    'FetchUselessDescriptors': 1,
    'FetchDirInfoEarly': 1,
    'FetchDirInfoExtraEarly': 1,
}

So what would be the correct way to force change the configuration (if any) when tor is already running (by command line) and we use txtorcon.connect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From http://timaq4ygg2iegci7.onion/guide.html#connecting-to-a-running-tor:

Similarly, the current configuration is available via .config. You can change the configuration
by updating attributes on this class but it won’t take effect until you call txtorcon.TorConfig.save().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that does read confusingly now that I re-read it. What I mean there is that you can "update things" by e.g. tor.config.UseEntryGuards = 1 and tor.config.save() not by setting tor.config directly. I'll try to make that docstring more-clear... :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit 353ebac#diff-5d97531ed54d2f8c4d91ebce0fb91774R180 should fix it.

I also moved TorConfig setup inside of if launch_tor 353ebac#diff-5d97531ed54d2f8c4d91ebce0fb91774R163

to don't get error about options that can't be changed once it's launched.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants