Opened 13 months ago

Last modified 12 days ago

#25140 merge_ready task

Parse only .torrc files in torrc.d directory

Reported by: iry Owned by: Jigsaw52
Priority: High Milestone: Tor: 0.4.1.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 (35)

comment:1 Changed 13 months ago by iry

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

comment:2 Changed 13 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 13 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 13 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 13 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 13 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 13 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 13 months ago by nickm

Milestone: Tor: 0.3.4.x-final

comment:9 in reply to:  7 Changed 13 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 13 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 12 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 11 months ago by nickm

Keywords: 034-triage-20180328 added

comment:13 Changed 11 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 11 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 11 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 11 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 10 months ago by Jigsaw52

Status: assignedneeds_review

comment:18 Changed 10 months ago by asn

Reviewer: ahf

comment:19 Changed 10 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 10 months ago by tseretni-rmd

Cc: tseretni-rmd added

comment:21 Changed 9 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 7 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 5 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 5 months ago by nickm

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

comment:25 Changed 4 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 4 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 3 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 3 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 3 months ago by teor

Status: needs_revisionneeds_review

comment:30 Changed 2 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 2 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 6 weeks 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 6 weeks ago by Jigsaw52

Status: needs_revisionneeds_review

I've fixed the changes file tag.

comment:34 Changed 4 weeks 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 12 days ago by nickm

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