Opened 19 months ago

Last modified 3 months ago

#25140 needs_review task

Parse only .torrc files in torrc.d directory

Reported by: iry Owned by: Jigsaw52
Priority: High Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.1-alpha
Severity: Major Keywords: 034-triage-20180328, 035-removed-20180711
Cc: iry, adrelanos, whonix-devel@…, danielpinto52@…, tseretni-rmd Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description (last modified by iry)

Currently, when using a torrc.d directory, for example:

%include /etc/torrc.d/

Every file in the directory will be treated and parsed as a valid Tor
configuration file. However, sometime, this may not be what users and
developers want.

For example, users may use /etc/torrc.d/50_user.torrc as the place to
put their own torrc configurations. But sometimes, when they use a
text editor to edit it, the text editor will leave a
/etc/torrc.d/50_user.torrc~ file which will also be treated as a valid
torrc file.

Another example that also happens very frequently is, when dpkg does
an update on /etc/torrc.d/30_distribution.torrc, users' previous
configuration can be saved as
/etc/torrc.d/30_distribution.torrc.dpkg-old which will also be parsed
by Tor.

In best case users will just be frustrated because Tor does not work
as expected and in worst case this could be dangerous. This could be a
severe problem especially because of the following reasons:

  1. filename.torrc~ filename.torrc.dpkg-old has higher priority than

filename.torrc when Tor does the parsing.

  1. In most cases, this will happen without being noticed by the normal

suer.

teor suggested on the tor-dev@:

To be more precise, most tools accept files ending in ".conf".
We might want tor to accept ".conf" for consistency.

I suggest we also accept files called "torrc", or ending in ".torrc".
This should probably also include files called literally ".torrc".

Downstream discussion to link everything together: http://forums.dds6qkxpwdeubwucdiaord2xgbbeyds25rbsgr73tbfpqpt4a6vjwsyd.onion/t/torrc-d-is-comming/4041/20

Child Tickets

Change History (46)

comment:1 Changed 19 months ago by iry

Cc: adrelanos whonix-devel@… added
Description: modified (diff)
Version: Tor: unspecifiedTor: 0.3.3.1-alpha

comment:2 Changed 19 months ago by nickm

This is a good idea, and I wish we had done it in the first version of the %include feature. One thing I'm concerned about: How do we handle the migration here? Anybody who currently has a working setup will have it fail if we start requiring a suffix that they didn't know to provide, and I really don't like breaking compatibility.

What if we created a new syntax, like:

%include /etc/torrc.d/*.conf

?

comment:3 Changed 19 months ago by iry

Current result from the discussion on @tor-dev: https://lists.torproject.org/pipermail/tor-dev/2018-February/012902.html

  • %include /etc/torrc.d means parsing all the files in the directory
  • implement %include /etc/torrc.d/*.extension syntax
  • let Tor log each config file it loads
  • warn on potentially unexpected files
  • document *.conf as the recommend usage
  • document how to manually migrate from * to *.conf
  • recommend /etc/torrc.d/*.conf as "a default extension for packagers"

comment:4 Changed 19 months ago by iry

implement %include /etc/torrc.d/*.extension syntax

I have been thinking about this for a while. I still find supporting a very flexible extension name may cause incompatibility between different distros and confusion to users and developers.

How do you like the idea to add another flag named something like StrictParsing which will let Tor only parse .config files when it is set to 1?

comment:5 Changed 19 months ago by iry

What if we created a new syntax, like: %include /etc/torrc.d/*.conf

My thoughts on this just in case people do not notice it: https://lists.torproject.org/pipermail/tor-dev/2018-February/012899.html

comment:6 Changed 19 months ago by iry

It seems still take a while before 0.3.3 becomes stable. But I notice that:

https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

For the most recent supported stable release only, we intend that:

Smaller bugs that significantly impact user experience will get fixed.

Is it possible to get the fix into 0.3.2, nickm?

comment:7 Changed 19 months ago by nickm

No, I really don't think so. This is much closer to a new feature than it is to a bugfix. Moreover, it promises to involve stuff (pattern matching, file systems, string handling) that have a way of getting unexpected bugs the first time around.

comment:8 Changed 19 months ago by nickm

Milestone: Tor: 0.3.4.x-final

comment:9 in reply to:  7 Changed 19 months ago by iry

Replying to nickm:

No, I really don't think so. This is much closer to a new feature than it is to a bugfix. Moreover, it promises to involve stuff (pattern matching, file systems, string handling) that have a way of getting unexpected bugs the first time around.

Thank you so much for your explanation, nickm! Your immediate reply is really helpful for the downstream to make decisions on how to deal with it.

comment:10 in reply to:  3 Changed 19 months ago by Jigsaw52

Cc: danielpinto52@… added

I agree that supporting wildcards in %includes would be a nice feature. I remember looking for code for processing wildcards in files in tor and not finding any so this might take some effort to develop.

If someone wants to take on this, the relevant changes are on ./src/or/config.c

In this file, there is a method config_process_include which is called for every %include found and receives the string after %include as first argument.
This method then calls config_get_file_list which is responsible for returning a list of files to be processed for that %include line. Note that config_get_file_list does not care about recursive includes, it just returns a list of files in a folder if the included path was a folder or a list with a single file if it was a file.

So, the only existing method that needs changes to support this is config_get_file_list.

comment:11 Changed 18 months ago by Jigsaw52

Owner: set to Jigsaw52
Status: newassigned

Assigning it to me as I've started to work on this.

comment:12 Changed 17 months ago by nickm

Keywords: 034-triage-20180328 added

comment:13 Changed 17 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:14 Changed 17 months ago by Jigsaw52

I've been working on this on this branch: https://github.com/Jigsaw52/tor/tree/torrc-include-wildcards-25140

However there is still some work to do until I can propose it to merge:

  • Write tests (which will likely find some bugs)
  • Test more throughly on Windows
  • Test more throughly with sandbox enabled
  • Documentation

I might be able to finish it on the next weekend. When is the deadline for the 0.3.4 merge?

comment:15 in reply to:  14 Changed 17 months ago by asn

Replying to Jigsaw52:

I've been working on this on this branch: https://github.com/Jigsaw52/tor/tree/torrc-include-wildcards-25140

However there is still some work to do until I can propose it to merge:

  • Write tests (which will likely find some bugs)
  • Test more throughly on Windows
  • Test more throughly with sandbox enabled
  • Documentation

I might be able to finish it on the next weekend. When is the deadline for the 0.3.4 merge?

Feature freeze is on May 15th according to: https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

comment:16 Changed 17 months ago by nickm

Keywords: 034-removed-20180328 removed

We can take this in 0.3.4 if the code is ready, and I hear Jigsaw52 is working on it. :)

comment:17 Changed 17 months ago by Jigsaw52

Status: assignedneeds_review

comment:18 Changed 16 months ago by asn

Reviewer: ahf

comment:19 Changed 16 months ago by ahf

Status: needs_reviewneeds_revision
  • 3ca5f8173298a4fa3bda6c5c3c3da528fc7cf85a: Looks fine to me.
  • edd70916b66fca41fc8aab10893ba731c1b75840:
    1. Would it make sense to ensure that the tor_glob() function works the same way on both Windows and POSIX when glob'ing on directory and file? A part of the glob test case is split between Windows and POSIX there.
  • 5aec94bc3bb6623bd8f62aedeca8aaf699a0e8b6:
    1. In config_get_file_list(): Why must the matches be sorted using smartlist_sort_strings()? Shouldn't that happen in the glob function then?
    2. I think config_get_glob_opened_files() might benefit from having some of this logic split into some more functions.
  • 15de04088b40efc85dc32f26fe7ec4a1abebb5d7: Looks fine. Good with some documentation.

Generally it looks good. It is very nice with a lot of tests for this code. Only concern for me is that there is some fairly big functions in this that I think could leverage from being split into smaller functions where possible.

comment:20 Changed 16 months ago by tseretni-rmd

Cc: tseretni-rmd added

comment:21 Changed 15 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Move most needs_revision tickets from 0.3.4 to 0.3.5: we're about to hit the feature freeze.

If you really need to get this into 0.3.4, please drop me a line. :)

comment:22 Changed 14 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

Removing needs_revision tickets from 0.3.5 that seem to be stalled. Please move back if they are under active revision or discussion.

comment:23 in reply to:  19 Changed 11 months ago by Jigsaw52

I was finally able to find time to get back to this.

In the meantime, there was a big refactoring of the tor code base so I've created a new branch based on the code after the refactoring.
This means that the commit hashes on previous comments are now useless but I've added the equivalents on the new branch to the comments.

The new branch is here: https://github.com/Jigsaw52/tor/commits/torrc-include-wildcards-25140_rebased

For my first 4 commits the code is mostly the same as before just in different files. The only change I recall is removing PATH_MAX from tor_glob and config tests due to #26873. There are also new commits that either address previous mentioned issues or other issues I've found while testing.

Addressing the previous comments:

  • edd70916b66fca41fc8aab10893ba731c1b75840 (now 7543b9253000e238788d0a209b3b501a5a16be66:
    1. Would it make sense to ensure that the tor_glob() function works the same way on both Windows and POSIX when glob'ing on directory and file? A part of the glob test case is split between Windows and POSIX there.

This has been improved: now wildcards are supported on all path components on Windows (commit 8bf917c4d8369bde9be53a6746c532d9932b05f3) and the behavior of the path separator at the end of the path is the same as on POSIX (commit ba8f29f24a353892a1e9dfab397ce543a6d192d3).

  • 5aec94bc3bb6623bd8f62aedeca8aaf699a0e8b6 (now fcc7275f5c80214e746bd6778f98af730993d8b2):
    1. In config_get_file_list(): Why must the matches be sorted using smartlist_sort_strings()? Shouldn't that happen in the glob function then?

I followed the approach of the existing tor_listdir function which also leaves sorting as a responsibility of the caller.
I do not know if there was a reason for this, but I followed it for consistency.

  1. I think config_get_glob_opened_files() might benefit from having some of this logic split into some more functions.

This has been addressed in commit ba8f29f24a353892a1e9dfab397ce543a6d192d3: as the Windows version of tor_gob ended up sharing much of the code with get_glob_opened_files (due to both needing to expand path components with globs manually), all the common code has been split into smaller functions.

Also, I noticed that #27186 is addressed by my first commit (61ee3726caad0aa8399ad07af1fe6eb6aa6aecb2).

comment:24 Changed 11 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.6.x-final
Status: needs_revisionneeds_review

comment:25 Changed 10 months ago by ahf

Created https://github.com/torproject/tor/pull/450

Please in the future do PR's against torproject/tor on Github to make our workflow easier for reviews :-)

Let's see what CI says.

comment:26 Changed 10 months ago by ahf

Status: needs_reviewneeds_revision

This patch is breaking our CI build.

Please see the log here: https://travis-ci.org/torproject/tor/jobs/447736208

In the future, please submit patches as pull requests via Github's interface by adding a PR to the torproject/tor repository. This allows us to see if your code passes all test via Travis.

Once the errors have been fixed, please open a new PR and I will close the one I opened to do a Travis test run.

comment:27 Changed 10 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:28 Changed 10 months ago by Jigsaw52

I've created the pull request with the problem hopefully fixed:

https://github.com/torproject/tor/pull/489

Also, I've rebased the branch to the current master and reverted commit ba3c785092fce23ce9ec52c89ed69b2c0f0f92f6 which fixed #27186 because I've already implemented the same feature on my branch with additional information: my version shows both the pattern on %include and the files that will be included when expanding it.

comment:29 Changed 10 months ago by teor

Status: needs_revisionneeds_review

comment:30 Changed 8 months ago by ahf

Status: needs_reviewneeds_revision

I've left some comments on the GH pull request. Thanks for working on this.

comment:31 Changed 8 months ago by Jigsaw52

Status: needs_revisionneeds_review

I've fixed the mentioned issues. Additionally, I've rebased to the current master and added support for bool type on the checkSpace.pl script because it started complaining when I had functions returning bool.

comment:32 Changed 7 months ago by ahf

Status: needs_reviewneeds_revision

Looks good. Only a small correction needed. Please change the changes file's tag (between ( and )) to be 'configuration'. Once that is done I think we'll move this to merge_ready and then Nick can have a look at it too before it goes in.

Thanks for doing this work!

comment:33 Changed 7 months ago by Jigsaw52

Status: needs_revisionneeds_review

I've fixed the changes file tag.

comment:34 Changed 7 months ago by ahf

Status: needs_reviewmerge_ready

I'm marking this merge ready now. I think it looks good.

Thanks for working on this and thanks for being so fast with your fixes for review comments.

Nick (or one of the other people who does merges) will have a look at this again once the merge window re-opens :-)

comment:35 Changed 6 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.4.1.x-final

comment:36 Changed 6 months ago by dgoulet

Status: merge_readyneeds_revision

So hmmm... this commit is the one that worries me the most:

https://github.com/torproject/tor/pull/489/commits/d8ef6c80c9774b2c4c9c3ee27aa5214083bc9e30

Not only that tor_glob() function is kind of insane but also it seems most of it was copied from tor_listdir. I would really like us to avoid code duplication from that madness.

That function should be simplified to have a tor_listdir() per platform instead of a mayhem of #ifdef. And then extract the common part that we can use within tor_glob() in smaller functions that lib/path/ can all use.

Sorry to put that back in needs_revision but we just can't pile up technical debt like that.

comment:37 Changed 6 months ago by Jigsaw52

Status: needs_revisionneeds_review

I agree but the tor_glob function was changed by a later commit.

After developing get_glob_opened_files in commit 111ec3ebd5c6cf4b46344f98a1b78b12dcabdf6a (https://github.com/torproject/tor/pull/489/commits/111ec3ebd5c6cf4b46344f98a1b78b12dcabdf6a), the Windows version of tor_glob ended up sharing much of the code with get_glob_opened_files (due to both needing to expand path components with globs manually). Because of that, all the common code has been split into smaller functions in commit c5883f356f92aca478450f0e09d6caac53ce2085 (https://github.com/torproject/tor/pull/489/commits/c5883f356f92aca478450f0e09d6caac53ce2085).

I think it might be easier to review this pull request code by looking at the diff for the whole pull request (the files changed tab in github). I've avoided squashing the first 4 commits because they were already reviewed in https://trac.torproject.org/projects/tor/ticket/25140#comment:19 but I now wish I hadn't as the pull request would be easier to read if they had been squashed.

comment:38 Changed 5 months ago by ahf

I've just been over this again, and I think it's starting to look good. I'd like to get one more pair of eyes on this because it's a slightly big change.

comment:39 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

Thanks for this code! I did a review.

It seems like the documentation and tests are incomplete.
I'd like to see documentation for:

  • escaping wildcards (for non-Windows)
  • globs are only allowed at the end of paths (for Windows)

I'd like to see more tests for these cases, and for the dot-file case, where the documentation does not match the code.

I don't know what will happen when this code interacts with the sandbox.
I also didn't review memory management in detail.

Please make sure it has been tested with "Sandbox 1" and ASan. It might be useful to have an integration test script in src/test that launches tor with some example configs. The existing sh and py files in src/test are good examples to follow.

comment:40 Changed 5 months ago by teor

I found an Apple-specific test variant, which I think is probably a BSD-specific variant.
See my comment on the pull request for details.

comment:41 Changed 4 months ago by Jigsaw52

Thanks for the review!

I've written a few commits to address your concerns, check the pull request comments for details.

I've tested with Sandbox enabled and it should work. The only restriction is that reloading the tor config after adding new files or folders to any of the folders where tor is looking for config files will not work. This restriction has been added to the documentation.

I also used valgrind during development and testing. Hopefully that has caught any memory issues.

comment:42 Changed 4 months ago by teor

It might be useful to have an integration test script in src/test that launches tor with some example configs. The existing sh and py files in src/test are good examples to follow.

Did you write an integration test script?

These kinds of scripts are useful for testing the external config interface of Tor, to make sure we haven't broken it. (Unit tests only test so much.)

comment:43 Changed 4 months ago by Jigsaw52

I've added an integration test in commit 5659899.

It tests using wildcards with %include and the controls commands to reload torrc, SAVECONF and getinfo config-can-saveconf.

I also wanted to use the same test to test reloading the configuration file with the seccomp sanbox enabled but I discovered that doing so crashes tor on Travis CI. The problem is unrelated to this code as I've tested and it happens when %including a folder, which uses the tor_listdir function that was refactored but functionally unchanged (it will use the same syscalls and the same arguments as before). This issue is likely related to #27315.

I will look into this problem further but it is going to be difficult to solve as I am still not able to reproduce the issue on my system.

Last edited 4 months ago by Jigsaw52 (previous) (diff)

comment:44 Changed 4 months ago by Jigsaw52

I was finally able to run Travis CI on my system and reproduce the problem.

The crash when reloading the configuration file with seccomp sandbox enabled was actually a bug that only happens with libc < 2.26. libc versions before 2.26 did not use openat to open files but used it on readdir. The previous seccomp rules for %included files and directories allowed them to be opened, which added either the rule for openat or for open, depending on your libc version. Because the initial %include sandbox rules were only tested on libc >= 2.26, where everything used openat, this was fine. When testing on libc < 2.26, only open (not openat) was allowed for the %included files and directories. Because readdir, which will be called for any %included directories, used openat, it caused tor to crash.

The fix was always adding the openat rule for %included directories regardless of the libc version in use.

Additionally, another problem was found and fixed: when tor is compiled with NSS, it will call getpeername and socket with some arguments that were not allowed by the sandbox rules. This caused tor to crash on startup. I've added the required rules and tor no longer crashes.

Also, I have mixed feelings on adding an automatic test for the seccomp sandbox. While it is great that this functionally can now be tested automatically (and it caught two bugs already), the seccomp sandbox is, by its nature, very fragile and this test will easily break when people start running on systems with different configurations. What do you think? Should this test configuration enable the seccomp sandbox?

comment:45 Changed 3 months ago by ahf

Status: needs_revisionneeds_review

Moved to needs_review again as per discussion on IRC.

comment:46 Changed 3 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final
Note: See TracTickets for help on using tickets.