Opened 7 years ago

Closed 4 months ago

Last modified 4 months ago

#1922 closed enhancement (implemented)

torrc.d-style configuration directories

Reported by: aa138346 Owned by: Jigsaw52
Priority: Low Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, intro, tor-03-unspecified-201612
Cc: intrigeri, anonym, adrelanos@…, NickHopper, sajolida@…, mcs, mrphs, segfault@…, torproject.org@…, sirmatt@…, iry Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor:

Description

torrc.d-style configuration directories

Child Tickets

Attachments (5)

config.pch (2.3 KB) - added by nguybao 4 years ago.
Patch file for feature #1922
config.2.pch (2.1 KB) - added by proper 4 years ago.
0001-Refactor-torrc-reading-code-so-it-s-easier-to-adapt-.patch (8.5 KB) - added by infinity0 4 years ago.
tor.d.patch (30.0 KB) - added by NickHopper 3 years ago.
Read config files from a directory
tor.d.2.patch (52.3 KB) - added by NickHopper 3 years ago.

Download all attachments as: .zip

Change History (102)

comment:1 Changed 7 years ago by nickm

Milestone: Tor: unspecified

What for? If there's a good application for this and I'll bump up the priority.

comment:2 Changed 7 years ago by arma

One answer here might be to let other applications modify Tor's config without needing to learn how to modify the torrc file.

For example, we could imagine a 'tor-bridge' debian package that just adds a torrc.d/bridge-torrc file. Then you could become a bridge without needing a controller and without needing a text editor. We could also imagine a tor-relay deb.

Weasel, what do you think of this idea? Are debs like this a good idea, or is this approach bad precedent because it would bloat the number of total debs in the world too far?

It *might* also be useful for controllers like Vidalia on OSes where the Tor deb runs as a daemon and doesn't run as the user. In those cases we've talked about having Vidalia launch a script that prompts the user for her root password and then moves the new torrc file into place. Having it move its torrc into a Vidalia-specific name might keep things saner -- and might be doable without root if the torrc.d directory is owned by a group that the Vidalia deb puts the user into, as we might need to do to get ControlSocket going. On the other hand, ultimately it might instead just complicate the issue further once there are two controllers that each write their own config file.

If we do it, we'd want to sort out what to do if lines in different config files contradict each other.

comment:3 Changed 7 years ago by arma

To clarify here,

I was suggesting an /etc/torrc.d/ directory with 1 or more files, all of which are parsed as containing torrc lines.

(rransom thought I was proposing that there would be multiple torrc files in /etc/torrc.d , and a Tor instance would only read one.)

comment:4 Changed 7 years ago by arma

|<weasel> debs with just a single config file won't fly
|<weasel> also, files/dirs in /etc/ being user writable is obviously bad too. it's a privileged escalation attack waiting to happen
|<weasel> also, yes.  conflicts are an issue
|<weasel> I still think it's a good idea tho.  for instance the debian-defaults patch could just be a config file snippet then

|> in what way will they not fly?
|<weasel> ftp-master will never accept them
|<weasel> packages cause an overhead.  for a single file that's probably too much overhead.

comment:5 Changed 7 years ago by arma

weasel> it probably shouldn't read a hardcoded torrc.d but instead there should be an _include_ directive that can include other files, including handling wildcards

comment:6 in reply to:  5 Changed 6 years ago by aa138346

The specific reason I requested this functionality is because I want to be able to easily maintain a centralized list of rogue ExitNodes which can easily pushed out to other servers running tor. (See ticket #3143.)

But there are obviously other reasons why one might want to dynamically add/change configuration/performance options, while maintaining a base configuration. Some of these changes might even be made frequently.

Right now, I am just using the "-f" option with a simple script which assembles the new configuration to achieve what I want, but being able to dump everything into a directory would be great (either with a hard-coded torrc.d directory, or an include option).

comment:7 Changed 6 years ago by arma

From IRC:

> on tor-relays there is a person talking about the /var/run/tor/control file.
am i correct in thinking this is something some package script makes, not tor
itself?
<weasel> arma: on 0.2.2.x debian packages, it's tor's control socket.
> oh ho
> does it go wherever $piddir is?
<weasel> it goes to /var/run/tor.  which is also where pid is.
> perhaps this person who wants a tor config option for it is asking for a
reasonable thing
<weasel> it's not a config option.
<weasel> that's because ControlSocket is something that tor's config stuff
appends to rather than replaces.
<weasel> and because tor doesn't do tor.d/* style configuration, so I have to
patch the settings the package should have into the binary

http://archives.seul.org/tor/relays/Jul-2011/msg00024.html
and http://archives.seul.org/tor/relays/Jul-2011/msg00025.html

Nick, perhaps this counts as the 'good application' you were looking for?

comment:8 Changed 6 years ago by T(A)ILS developers

Cc: amnesia@… added

Quoting the Report from the "Tor ecosystem" BoF at DebConf 11 (http://archives.seul.org/or/dev/Jul-2011/msg00044.html):

We outlined some benefits that would have a /etc/tor/torrc.d directory
full of configuration snippets:

  • The 'tor' package will not (or less) need patches to change the default settings.
  • It would make it much easier to implement example setups (relays, exit relays, bridges) either through a debconf question or through a system script like 'tor-setup-relay'.
  • Tails developers will only need to drop a very small extra configuration file instead of having to rewrite the whole, which should ease development and reviewing.
  • Other packages which need a hidden service to work (e.g. 'onioncat') would be able to create it automatically by installing the proper configuration snippet. That would enable them to work right after being installed.

The include directive was asked on #2863, btw - might be considered a duplicate.

comment:9 Changed 5 years ago by nickm

Keywords: tor-client added

comment:10 Changed 5 years ago by nickm

Component: Tor ClientTor

comment:11 Changed 4 years ago by proper

I am copying in here, what Nick said at the duplicate of this one (#8291) and what I answered.

Replying to comment:2 nickm:

Some quick thoughts:

  • Changing the *default* search path for torrc is probably not a great idea; it would break anybody who is using the current default location.

Changing from /etc/tor/torrc to /etc/torrc was indeed no good idea. Sorry for suggesting it.

Replying to comment:2 nickm:

  • Having a tor.d directory is an okay idea. You'll need a fairly rigorous definition of what order the files in it are read, and how they interact with torrc and the defaults_torrc (if any). (Make sure you understand how the defaults-torrc logic works here before designing anything; make sure it can be reused.)

I think this way would be good:

defaults_torrc gets overruled by /etc/tor/torrc gets overruled by /etc/tor.d/10_something gets overruled by /etc/tor.d/20_somethingelse.

The same in other words:

defaults_torrc: lowest priority
/etc/tor/torrc: normal priority
/etc/tor/tor.d: highest priority

Sourcing /etc/tor.d/ in lexical order, i.e. first source /etc/tor.d/10_something, then source /etc/tor.d/20_somethingelse. If /etc/tor.d/10_something contains "CookieAuthentication 0" and /etc/tor.d/20_somethingelse contains "CookieAuthentication 1", then "CookieAuthentication 1" will win.

This is the way it usually works on Linux for folders such as /etc/profile.d, and the run-parts(8) tool works the same way.

Replying to comment:2 nickm:

  • SAVECONF will need to treat definitions in /etc/tor.d as defaults, so that when it replaces torrc, it doesn't copy all of tor.d into torrc.

Changed 4 years ago by nguybao

Attachment: config.pch added

Patch file for feature #1922

comment:12 Changed 4 years ago by nguybao

Status: newneeds_review

Attaching a patch file for this feature..requesting code review. (config.pch)

comment:13 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

Quick notes:

  • We don't use malloc; we use tor_malloc.
  • We don't use free; see tor_free.
  • Please use the same indentation style as the rest of Tor.
  • strcpy? Please, no. We don't want heap overflows.
    • In fact, please don't use strcpy in any other program that's supposed to be secure.
  • Please no bubble sorts, insertion sorts, or other inefficient reimplementations of algorithms that are supposed to be O(n lg n). Just use smartlist_sort() or smartlist_sort_strings().
  • Rather than hardwiring "/etc/", try using CONFDIR ?
  • If we can't get FN_FILE from file_status(), why skip the file? Shouldn't we warn?
  • Nothing in this code frees dirlist or its contents.
  • Rather than making a fake command line and passing it to load_torrc_from_disk(), why not refactor the code into two functions: one to find the right configuration file, and the other to read and parse it. That way, this code could only send the second one.

Some more fundamental issues

  • I thought that the semantics of options_init_from_string were that it replaced the current configuration with cf_defaults+cf. But that appears to means that, in this code, the original torrc file is completely replaced with the first file in /etc/tor.d/, then by the second, then by the third, and so on. (Is this tested?)
  • It seems that for an ordinary Tor user, there's no way to override this stuff. If the system has an /etc/tor.d, I can't override those options even with "-f my_torrc", since the torrc is considered second, and the /etc/tor.d contents are considered last. There is no way to override that directory with another one, either. I don't think that can be the right way to do it, can it?

comment:14 in reply to:  13 Changed 4 years ago by proper

Replying to nickm:

Some more fundamental issues

  • I thought that the semantics of options_init_from_string were that it replaced the current configuration with cf_defaults+cf. But that appears to means that, in this code, the original torrc file is completely replaced with the first file in /etc/tor.d/, then by the second, then by the third, and so on. (Is this tested?)

I did not mean, if /etc/tor.d/10_something contains "CookieAuthentication 1" and /etc/tor.d/20_somethingelse contains no line containing "CookieAuthentication", that /etc/tor.d/10_something is completely disregarded. The idea was that configuration fragments are joined together in a predictable order. (Bao, please see above.)

  • It seems that for an ordinary Tor user, there's no way to override this stuff. If the system has an /etc/tor.d, I can't override those options even with "-f my_torrc", since the torrc is considered second, and the /etc/tor.d contents are considered last. There is no way to override that directory with another one, either. I don't think that can be the right way to do it, can it?

This is a valid concern and no one considered that earlier. There is a solution.

At the moment in the Tor manual (man tor), there is...

-f FILE
Specify a new configuration file to contain further Tor configuration options. (Default: $HOME/.torrc, or /etc/tor/torrc if that file is not found)

and...

--defaults-torrc FILE
Specify a file in which to find default values for Tor options. The contents of this file are overridden by those in the regular configuration file, and by those on the command line. (Default: /etc/tor/torrc-defaults.)

I suggest adding..

--fragments FOLDER
Specify a folder to contain further Tor configuration file fragments. (Default: $HOME/.tor.d, or /etc/tor.d/ if that file is not found)

This should allow an ordinary Tor user to override it.

comment:15 Changed 4 years ago by nguybao

  1. We could have another function just like options_init_from_strings but without replacing the configuration.
  2. We could parse /etc/tor.d/ first then others after. That means it has the least precedence though.

comment:16 Changed 4 years ago by nickm

One crazy idea I had was to require that these files start with a two-digit number, since that's how everybody will do it anyway. Then we could define the priority of torrc to be (say) 50, and of the defaults-torrc to be 00, and of the command line to be 100.

comment:17 in reply to:  16 Changed 4 years ago by proper

Replying to nickm:

One crazy idea I had was to require that these files start with a two-digit number, since that's how everybody will do it anyway. Then we could define the priority of torrc to be (say) 50, and of the defaults-torrc to be 00, and of the command line to be 100.

Sounds good.

Changed 4 years ago by proper

Attachment: config.2.pch added

comment:18 in reply to:  15 Changed 4 years ago by proper

nguybao:

How about this code snippet? I gotta to run this through you guys before I add it to the ticket. I read all the configuration files and parse them at once. The default and "-f" files got first priority. /etc/tor.d got last priority.

comment:19 Changed 4 years ago by proper

Bao posted this on the tor-dev mailing list where it got lost. I think it's better to use only this ticket.

comment:20 Changed 4 years ago by proper

Actually, I was wrong, it hasn't been posted to the list at all. Sorry.

comment:21 Changed 4 years ago by nguybao

Status: needs_revisionneeds_review

comment:22 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

This needs more work than I can give it, but I should say some stuff:

  • It would be a good idea to learn how to do safe string handling in C. strcat is just as bad as strcpy. I do not want heap overflows in tor.
  • "/etc/tor.d" is still hardwired.
  • It still uses a fake command line.
  • It fails badly if smartlist_len(dirlist) is greater than CHAR_MAX. Why would you ever use a char for that?
  • concatenating files is probably not the right approach.

These are just the basic issues; there are lots of comments and design discussions I made above that are still unfixed. I think this whole thing needs a total rewrite.

comment:23 Changed 4 years ago by proper

Cc: adrelanos@… added
Status: needs_revisionnew

After bao no longer working on this... Anyone feel free to take this.

comment:24 Changed 4 years ago by proper

Regarding the usefulness of this I would like to add: for creating a Tor centric Debian blend, the tor.d directory would be required. It would allow Whonix (or Tails in case they're interested, can't speak for them) to become a Debian blend.

Paul Wise (of Debian) said:

[...] The tor.d directory would need to be implemented before Whonix could become a blend. [...]

(Keep your time. There are probably other non-Tor specific deal breakers for creating a Tor centric Debian blend.)

comment:25 Changed 4 years ago by infinity0

I am creating a tor-bridge-relay Debian package in #10970 and this would be greatly appreciated.

comment:26 Changed 4 years ago by infinity0

An alternative method (Debian-only) is to modify the tor initscript to read /etc/tor/*.torrc and add the appropriate -f args. This would likely be easier to write and pass review than C code.

comment:27 Changed 4 years ago by infinity0

I was mistaken about how -f worked; I thought you could pass multiple, since the man page says "Specify a new configuration file to contain further Tor configuration options.". Actually, it might be better to implement this, than to have tor decide the sort-order for parsing torrc in a directory.

comment:28 in reply to:  26 Changed 4 years ago by proper

Replying to infinity0:

An alternative method (Debian-only) is to modify the tor initscript to read /etc/tor/*.torrc and add the appropriate -f args. This would likely be easier to write and pass review than C code.

You make it sound really difficult. Is it? Would someone capable of C and remotely familiar with the Tor codebase need more than two hours for this small feature?

than to have tor decide the sort-order

This sounds complicated. Just do it the standard way, in lexical order.

comment:29 Changed 4 years ago by infinity0

The above patch is a refactoring that retains the same current behaviour. However, it makes future completion of this bug much easier. (I am already working on top of it.)

In order to implement multiple -f options, would be a 2-line diff on top of this patch. In order to implement a -d option would take some more effort. In both cases, some additional work is needed to make get_torrc_fname work properly, since that assumes we only ever read from one single torrc file.

edit: it looks like git-format-patch messed up the commit message formatting. :( hopefully it's still readable.

Last edited 4 years ago by infinity0 (previous) (diff)

Changed 3 years ago by NickHopper

Attachment: tor.d.patch added

Read config files from a directory

comment:30 Changed 3 years ago by NickHopper

Cc: NickHopper added
Status: newneeds_review

Nick suggested this task as something useful to do at the Reykjavik hack day.  Testing took me longer than it should have, but this patch changes options_init_from_torrc so that it:

  1. Reads the torrc-defaults file, if any exists
  1. Reads in lexicographic order all config files in the tor.d directory, if it exists.   List options are cumulative across the files in this category.
  1. Reads the torrc file, if any exists.

Comments and review welcome.

comment:31 Changed 3 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.6.x-final

comment:32 Changed 3 years ago by nickm

Looks initially plausible; we should take a closer look when we fork off 0.2.5 and start merging things for 0.2.6.

Do your tests cover all the changed code?

comment:33 in reply to:  32 Changed 3 years ago by NickHopper

Replying to

nickm:

Do your tests cover all the changed code?

No. In particular, the test cases added don't test the logic that checks for CONFDIR/tor.d. I couldn't think of an automated way to cover this with an unprivileged test process. Any suggestions? (I did test this in a VM with a setuid script)

comment:34 Changed 3 years ago by nickm

Maybe have CONFDIR be an argument to the functions that use it, and write a unit test (a la the stuff in test_config.c) that tries it with a cooked value to replace CONFDIR when testing those functions?

Other quick notes:

  • "make check-spaces" enforces some of our coding style rules
  • Was there going to be a ~/.tor.d/ as well?

Changed 3 years ago by NickHopper

Attachment: tor.d.2.patch added

comment:35 in reply to:  34 Changed 3 years ago by NickHopper

Replying to nickm:

Maybe have CONFDIR be an argument to the functions that use it, and write a unit test (a la the stuff in test_config.c) that tries it with a cooked value to replace CONFDIR when testing those functions?

OK, just added a new patch that has full branch coverage of the changed code.

  • "make check-spaces" enforces some of our coding style rules

And makes this happy, too.

  • Was there going to be a ~/.tor.d/ as well?

Is there a compelling reason to have this? I was thinking:

  • A local install will change CONFDIR,
  • if a user knows to make a ~/.tor.d/ s/he could also just pass -d ~/.tor.d

But if there's a good reason, it's not hard to add.

comment:36 Changed 3 years ago by intrigeri

Cc: intrigeri anonym added; amnesia@… removed

comment:37 Changed 3 years ago by Sebastian

What's the status here? Is this likely to land in 0.2.6?

comment:38 in reply to:  37 Changed 3 years ago by nickm

Replying to Sebastian:

What's the status here? Is this likely to land in 0.2.6?

It well might. Somebody's got to review it and we've got to figure out whether it's the right behavior.

comment:39 Changed 3 years ago by arma

(I pointed Sebastian here because he's been working on a patch to make approved-routers more reasonable, and maybe turning it into more torrc options, which one can put in a separate file using the feature described here, would be a nice way forward.)

comment:40 Changed 3 years ago by T(A)ILS developers

Cc: tails@… added

comment:41 Changed 3 years ago by T(A)ILS developers

Cc: tails@… removed

comment:42 Changed 3 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Moving this to 0.2.???. I can review and merge stuff, but I can't decide whether it's what TBB and debian and TAILS and everybody else want. Please discuss among yourselves and reach consensus?

(It would be silly for humans to spend their time writing more patches here if there isn't an agreement on how the feature actually works wrt stuff like defaults, torrc searching paths, SAVECONF, etc)

comment:43 Changed 3 years ago by federico3

Changing configuration structure will break the multiple init script in #6996

comment:44 in reply to:  43 Changed 3 years ago by proper

Replying to federico3:

Changing configuration structure will break the multiple init script in #6996

Not necessarily. If we leave /etc/tor folder as is, as well as introduce and an optional config option --confdir /etc/tor.d there should be no issues.

comment:45 in reply to:  42 ; Changed 3 years ago by proper

Replying to nickm:

Moving this to 0.2.???. I can review and merge stuff, but I can't decide whether it's what TBB and debian and TAILS and everybody else want. Please discuss among yourselves and reach consensus?

(It would be silly for humans to spend their time writing more patches here if there isn't an agreement on how the feature actually works wrt stuff like defaults, torrc searching paths, SAVECONF, etc)

Wondering if ccc 31C3 congress would be a good place to discuss this?

comment:46 in reply to:  45 ; Changed 3 years ago by intrigeri

Replying to proper:

Wondering if ccc 31C3 congress would be a good place to discuss this?

Happy to discuss it there, _if_ someone does the less-exciting homework of preparing this discussion (starting with listing corner cases, problematic and non-obvious ones, I guess) in advance.

comment:47 in reply to:  42 Changed 2 years ago by ageisp0lis

Replying to nickm:

Moving this to 0.2.???. I can review and merge stuff, but I can't decide whether it's what TBB and debian and TAILS and everybody else want. Please discuss among yourselves and reach consensus?

I was wondering about the status of this enhancement. We at Freedom of the Press Foundation are using Tails and authenticated hidden services in SecureDrop and would like the HidServAuth values to be persistent. We originally made the feature request with Tails, but per a more recent casual IRC conversation I had with some Tails devs, they are not planning on implementing it unless they get the torrc.d-style configuration directories discussed in this ticket. So we've had to create workarounds for now, and it seems the feature we'd like to see in Tails is dependent on these upstream changes to Tor. In conclusion, I hope that you might consider us as a strong voice and use case in favor of this enhancement, and I look forward to hearing when you think it might be merged if at all.

comment:48 in reply to:  46 Changed 2 years ago by proper

I think the latest status is this. Let me rehash.

nickm:

Moving this to 0.2.???. I can review and merge stuff, but I can't decide whether it's what TBB and debian and TAILS and everybody else want. Please discuss among yourselves and reach consensus?

(It would be silly for humans to spend their time writing more patches here if there isn't an agreement on how the feature actually works wrt stuff like defaults, torrc searching paths, SAVECONF, etc)

intrigeri:

Replying to proper:

Wondering if ccc 31C3 congress would be a good place to discuss this?

Happy to discuss it there, _if_ someone does the less-exciting homework of preparing this discussion (starting with listing corner cases, problematic and non-obvious ones, I guess) in advance.

As I understand, a total rehash, summary of everything that has been discussed here.

ageisp0lis, are you up to do this?

comment:49 Changed 2 years ago by sajolida

Cc: sajolida@… added

comment:50 Changed 2 years ago by nickm

I just heard a proposal from Hans that sees okay with me, if it's okay with TBB and Tails and Debian and Nyx.

  1. Add an %include directive to the torrc file. It can take a file or a directory. If you give it a directory, it includes all (non-dot) files in that directory in lexically sorted order (non-recursive).

2a. If an %include is used in the torrc file, SAVECONF will not be allowed. (Or will work funny? Or will require a force flag?)

2b. If %include is used only in the defaults-torrc file but not in the torrc, SAVECONF will still work fine.

  1. %include can be used recursively. (maybe save this part for later)
Last edited 2 years ago by nickm (previous) (diff)

comment:51 Changed 2 years ago by nickm

Damian says it sounds okay, and prefers the force option, and requests a getconf that will tell him whether saveconf would say no.

comment:52 Changed 2 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Points: medium
Priority: normalmajor

Weasel (of Debian) and Patrick (of Whonix) and Mike (of TBB) are all cool with this idea.

comment:53 Changed 2 years ago by eighthave

intrigeri said do it

comment:54 Changed 2 years ago by nickm

Here's how to implement it:

Main:

  • In config_get_lines(), add a recursive call to config_get_lines whenever it sees a line where k is %include. Add the returned items to the end of the list that's getting built.
  • Add a recursionlevel argument to config_get_lines(). When you call config_get_lines from inside config_get_lines, call it with a recursionlevel one level higher. If the recursionlevel is >= 32, return an error.

Controller-side:

  • Set a flag in the or_options_t somehow to say whether %include was used.
  • If the flag is true, then in handle_control_saveconf(), refuse to save unless there is an extra item in the arguments to saveconf, and that argument is true.
  • Make a new entry in getinfo_helper_misc() to handle a "config-can-saveconf". Make it yield "1" if saveconf would work and 0 otherwise.
  • Add config-can-saveconf to getinfo_items.

Documentation:

  • Describe this all in tor.1.txt.in
  • Describe the controller parts in control-spec.txt

comment:55 Changed 2 years ago by eighthave

Weasel and I (aka Hans) sketched out how we would use it in the Debian package, closely following the Apache pattern but with naming that is more appropriate in Tor:

/etc/tor/torrc::
%include /etc/tor/services-enabled/*.torrc
%include /etc/tor/instances-enabled/*.torrc

These dirs are present for the actual snippet files:
/etc/tor/services-available
/etc/tor/instances-available

These dirs include relative symlinks to *-available:
/etc/tor/services-enabled
/etc/tor/instances-enabled

For example:
/etc/tor/services-enabled/sparkleshare.torrc --> ../services-available/sparkleshare.torrc

The sparkleshare package would include:

/etc/tor/services-available/sparkleshare.torrc

The davical package would include:

/etc/tor/services-available/davical.torrc

comment:56 Changed 2 years ago by mcs

Cc: mcs added

comment:57 Changed 2 years ago by nickm

Status: needs_reviewnew

comment:58 Changed 20 months ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

It is impossible that we will fix all 226 currently open 028 tickets before 028 releases. Time to move some out. This is my second pass through the "new" and tickets, looking for things to move to 0.2.9.

comment:59 Changed 20 months ago by nickm

Keywords: intro added

comment:60 Changed 18 months ago by isabela

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

tickets market to be removed from milestone 029

comment:61 Changed 15 months ago by mrphs

Cc: mrphs added
Severity: Blocker

comment:62 Changed 15 months ago by mrphs

Severity: BlockerNormal

Accidentally set to blocker. Sorry! Reverting back to normal.

comment:63 Changed 13 months ago by segfault

Cc: segfault@… added

comment:64 Changed 13 months ago by Ralph

Cc: torproject.org@… added

comment:65 Changed 13 months ago by pastly

Cc: sirmatt@… added

comment:66 Changed 10 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:67 Changed 9 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:68 Changed 7 months ago by Jigsaw52

Owner: set to Jigsaw52
Status: newassigned

I am going to try to add this feature.

I have done some initial work to add support to include files on torrc.
I will be using the following branch: https://github.com/Jigsaw52/tor/tree/torrc-dir-fix-1922

comment:69 Changed 6 months ago by teor

Thanks!
Just a reminder that before we merge this feature, we'll want some unit tests for it.

comment:71 Changed 6 months ago by Jigsaw52

Status: assignedneeds_review

comment:72 Changed 6 months ago by dgoulet

Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Priority: HighLow

comment:73 Changed 5 months ago by Jigsaw52

Some changes on the master branch introduced merge conflicts on my branch.
Here is a branch with a currently merge-able version: https://github.com/Jigsaw52/tor/tree/torrc-dir-fix-1922_20170414

comment:74 Changed 5 months ago by nickm

Looks like good solid work! A few comments:

  1. +          if (!list) {
    +            list = included_list;
    +            list_last = included_list_last;
    +          } else if (included_list) {
    +            list_last->next = included_list;
    +            list_last = included_list_last;
    +          }
    

This conditional isn't necessary -- look what we were doing with the "next" pointer in the old code.

  1. Can the code in the if (!strcmp(k, "%include")) { block be extracted into a new function? It makes the config_get_lines_aux function huge, and it's logically fairly separate.
  1. There should probably be a flag to enable %include on specific files: we use config_get_lines() all over the place, and we don't necessarily want to allow includes in all of them.
  1. Maybe there should be two wrappers for config_get_lines_aux: one presenting the old API, and one presenting one the new API that allows includes and reports whether they have happened? That way we could avoid changing all 22 places that call config_get_lines().
  1. This could be much shorter and safer with tor_asprintf.
    +      size_t fullname_len = (size_t) (strlen(path) + strlen(f) + 2);
    +      char *fullname = tor_malloc_zero(fullname_len);
    +      strlcat(fullname, path, fullname_len);
    +      strlcat(fullname, PATH_SEPARATOR, fullname_len);
    +      strlcat(fullname, f, fullname_len);
    +      tor_free(f);
    
  1. This could use a changes file; see doc/HACKING/CodingStandards.md for more info on those.

comment:75 Changed 5 months ago by nickm

  1. For consistency, let's sort the output of tor_listdir().
  1. Maybe we shouldn't automatically detect directory vs file: instead, we should only load the configuration files in a directory if the string ends with /*
  1. Do we support quoted paths here? Maybe we should.

comment:76 Changed 5 months ago by nickm

Status: needs_reviewneeds_revision

comment:77 Changed 4 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

comment:78 Changed 4 months ago by Jigsaw52

Status: needs_revisionneeds_review

I've fixed most of the issues. Comments below:

  1. I find my version easier to read but I changed it back for consistency sake since this pattern is used on other places too.
  1. Done.
  1. and 4. I agree with the suggestion to expose the two APIs for config_get_lines and I implemented it. config_get_lines_include processes includes while config_get_lines is the same as before.
  1. Done.
  1. Done.
  1. I don't understand this one. I sort the list right after the calling tor_listdir().
  1. I do not like the paths ending in /* because it might mislead users into thinking the * wildcard will work for any pattern. However, I added tests to make sure ending the path with PATH_SEPARATOR is supported as it is any easy way to tell a directory and a regular file apart. I left automatic detection of file/directory.
  1. Added support for quoted paths. Added tests for it too.

I've merged with the current master and squashed all my changes here: https://github.com/Jigsaw52/tor/tree/torrc-dir-fix-1922_squashed

comment:79 Changed 4 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final

Pulling this back for consideration in 0.3.1, since the feature window isn't _quite_ closed yet. :)

comment:80 Changed 4 months ago by nickm

This is looking pretty good! As for the quoting thing though -- do you think it would be okay to use "unescape_string()" instead, and avoid adding a new function? Please let me know what you think.

comment:81 Changed 4 months ago by nickm

Also, I'm seeing a memory leak in the unit tests. I think the problem is that in config_get_file_list(), you need to free file_list using smartlist_free(), not tor_free()

comment:82 Changed 4 months ago by Jigsaw52

I don't think unescape_string() is a good function to unquote the paths because:

  1. C escape sequences will be unescaped. This is a problem on Windows, where a path like C:\tor would become C:<TAB>or
  2. Backslashes need to be escaped. On Windows this would make paths a mess.
  3. A path ending with backslash (e.g.: "C:\Programs and Files\tor\torrc.d\") would cause problems too.

That is why I created a new function. None of the unescaping functions I found would handle these cases well.

You are correct about the leak. I fixed it. Thanks for finding it.

I also added config-can-saveconf to the changes file and updated torrc.sample.in to include an explanation and examples of %include usage.

The new branch is here: https://github.com/Jigsaw52/tor/tree/torrc-dir-fix-1922_squashed2

Also, the torspec branch is here: https://github.com/Jigsaw52/torspec/tree/torrc-dir-fix-1922_squashed

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

comment:83 Changed 4 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Makes sense; merged it! Thanks again!

comment:84 Changed 4 months ago by nickm

Oh, wait. Actually, there turns out to be a problem with the unit tests on Windows. The issue is that parse_config_line_from_str_verbose() already _does_ look for strings that are surrounded by quotes, and processes them with unescape_string(). So I've tried to fix up the test behavior well enough for it to pass. I suspect more fiddling will be needed, but right now I'm just waiting for the windows jenkins builders to complete

comment:85 Changed 4 months ago by nickm

Resolution: implemented
Status: closedreopened

Okay, there's some other problem here too. https://jenkins.torproject.org/job/tor-ci-windows-commit/1298/console says that we're still failing on windows:

18:38:50 config/include_path_syntax: 
18:38:50   FAIL ../tor/src/test/test_config.c:5190: assert(config_get_lines_include(torrc_contents, &result, 0,&include_used) OP_EQ 0): -1 vs 0
18:38:50   [include_path_syntax FAILED]

Any insights?

comment:86 Changed 4 months ago by Jigsaw52

I do not have time do debug this right now. I will look at it in a few hours. But just a guess: could it be that, on Windows, dir already ends with a backslash (get_fname returns it already with the backslash)?

comment:87 Changed 4 months ago by nickm

I've investigated with wine, and it's failing even on the first line, because this is the torrc_contents:

%include "C:\users\nickm\Temp\tor_test_8_cyicnwzs/test_include_path_syntax"
%include C:\users\nickm\Temp\tor_test_8_cyicnwzs/test_include_path_syntax\
%include "C:\users\nickm\Temp\tor_test_8_cyicnwzs/test_include_path_syntax\\"

I think one fix here is to escape the string. I've got it working on windows, but now I need to test again on unix.

comment:88 Changed 4 months ago by nickm

(remember, since parse_config_line_from_str_verbose() already calls unescape_string(), we have to use the fancier escaping for this to work.)

comment:89 Changed 4 months ago by nickm

13034e1574bf5e in the Tor master repository makes this work for me on Wine, and doesn't break it on Linux. I'll see whether the Jenkins builders are happy.

comment:90 Changed 4 months ago by arma

I just closed #8108 as a duplicate of this ticket.

comment:91 Changed 4 months ago by Jigsaw52

I'm setting up a developing environment on Windows to test this better. Even if messing with escaping makes the test pass, I want to be sure this actually works as it should on Windows too.

comment:92 Changed 4 months ago by Jigsaw52

I've identified the source of the problem: parse_config_line_from_str_verbose() calls unescape_string() when a configuration value starts with a quote. unescape_string() returns NULL for anything that is not a valid C escaped string, causing parse_config_line_from_str_verbose() to stop reading the configuration and return the error "Invalid escape sequence in quoted string".

This problem is not specific to this feature. As it is currently implemented, the config parsing code assumes that any value enclosed by quotes is a valid C escaped string. If it is not, the configuration will be invalid. Any configuration option that takes paths will have problems with paths containing backslashes that are enclosed in quotes.

I'm unsure how to fix this. It is not feasible to make special cases for options that we know that take paths because the options parsing code is completely separate from the options validation code.

Is there any option that actually benefits from allowing C escaped strings as values? I'm thinking that maybe this feature could be removed but that will cause backwards compatibility problems if someone decided to use it in their config files.

comment:93 Changed 4 months ago by Jigsaw52

Maybe the only problem is that my test is wrong. The tor manual states: "C-style escaped characters are allowed inside quoted values". This is annoying for Windows users but it already works this way with all the other options taking paths. I guess the simple solution is just removing the two lines that test paths with quotes from torrc_contents.

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

comment:94 Changed 4 months ago by nickm

I think you're right there; it's not great, but let's reopen if needed, or open a new ticket if we come up with a better solution.

comment:95 Changed 4 months ago by nickm

Resolution: implemented
Status: reopenedclosed

comment:96 in reply to:  95 Changed 4 months ago by iry

Cc: iry added

Thank you for the great work, everyone!

I have been working on anon-connection-wizard which is a Python-clone of the Tor Launcher. The anon-connection-wizard needs to output the user's configuration into a torrc file. I would like to ask "when the Tor version supporting that feature will enter deb.torproject.org stretch repository"?

This information will help me to decide whether I need a transitional approach before the feature is available. Thank you very much!

comment:97 Changed 4 months ago by cypherpunks

This is not the best place to ask such questions.

The first released tor version with this feature is 0.3.1.1-alpha.
As usual there will be alpha packages on deb.torproject.org

If you want this feature _now_ you can use the nightly builds:
https://deb.torproject.org/torproject.org/dists/tor-nightly-master-stretch

Note: See TracTickets for help on using tickets.