Bug 207129
| Summary: | security/tor: Request to add multi-instance support | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | nusenu <freebsd-vheg> | ||||||||||||||||||||
| Component: | Individual Port(s) | Assignee: | Jan Beich <jbeich> | ||||||||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||||||||
| Severity: | Affects Some People | CC: | yuri | ||||||||||||||||||||
| Priority: | --- | Keywords: | feature | ||||||||||||||||||||
| Version: | Latest | Flags: | vlad-fbsd:
maintainer-feedback?
(yuri) |
||||||||||||||||||||
| Hardware: | Any | ||||||||||||||||||||||
| OS: | Any | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
nusenu
2016-02-12 11:39:34 UTC
Maintainer reset. New maintainer, request feedback. Created attachment 176091 [details]
patch
Implemented multi-instance support.
Bumped PORTREVISION.
Passes poudriere.
Created attachment 176094 [details]
patch
Thanks for working on this! Can we remove the (currently) hard-coded tor user and DataDir parameters? I run every tor instance under a separate user. You can have a look at: https://github.com/nusenu/ansible-relayor Currently defaults are hardcoded for convenience, you can override them in rc.conf.
The patch can be extended to optionally supply security credentials and data dirs for instances. For example:
> tor_instances="inst1{:user1:group1:/data/dir1} {inst2{:user2:group2:/data/dir2} ...}"
Please confirm that this is what you mean.
Is the user specified in that setting actually used to start the tor daemon or is it used after tor started to drop root privileges to that user - equivalent to the User parameter in torrc - (allowing <1024 port bindings)? The extra instances are started exactly the same way as the main instance is started by the system, just with the different user. The comment in /etc/rc.subr says that they are started using su(1) with the user ${name}_user using su(1).
If you want to start them as root, you should say "inst1:root:wheel" and write the desired user into torrc@inst1. That is great thanks for the explanation. Are (all) tor instances restarted during a tor package upgrade? (I'm not sure if that is even the case for single instances, since I don't use the current rc.d script) Daemons are not restarted during the package upgrade as far as I know. So you need to first stop the service, upgrade, and start the service again. But all instances will be stopped/started together during the 'service' command. It is best, and easiest to manage to use rc scripts to start services. Such ease of use is the design goal here. One more comment/question: Can I also specify the pidfile in tor_instances=.. to override the hardcoded path? It is doable, I just didn't think this ability was needed. Ok, I will redefine instance as: inst_name{:inst_conf:inst_user:inst_group:inst_pidfile:inst_data_dir} for maximum flexibility.
Awesome! I'm happy to test the upcoming patch. Since I'm auto-generating rc.local entries: are multiple occurrences of tor_instances=... merged or does the last one override the previous entry? tor_instances should be space-separated.
As usual in shell, you can extend the variable by:
> tor_instances="${tor_instances} inst..."
Created attachment 176109 [details]
patch
nusenu,
Please test the attached patch.
Now there are two ways to specify an instance:
1. short: just a name
2. extended: inst_name:inst_conf:inst_user:inst_group:inst_pidfile:inst_data_dir
thanks! It basically works, a few comments: 1) I put "root" as the inst_user but another in the torrc itself (to bind to <1024 ports), in my case the chown commands do not cause any issues because the datadirs are autogenerated by my ansible role already but generally it could fail for others because you will do a chown root /<DataDir> and the user specified in the torrc will no longer be able to access it. The easiest fix: Leave that responsibility to the user (having proper folders). Since a proper automated solution is not easily possible here - I guess. 2) I define N instances in tor_instances, but there are N+1 tor instances running after "service tor start" (default instance). I don't care about the default instance. How does one disable the default instance properly? Quick 'n dirty fix would be to rename /usr/local/etc/tor/torrc but that seems a dirty solution? Created attachment 176120 [details]
patch
Added tor_disable_default_instance rc parameter allowing to prevent the default instance in case when tor_instances are used.
nusenu, 1) rc script will not auto-create the data directory with proper permissions in case of setuid option in torrc. You need to manually create the directory. 2) Implemented tor_disable_default_instance Created attachment 176123 [details]
patch
Minor corrections.
Created attachment 176124 [details]
patch
Minor improvement.
Created attachment 176150 [details]
patch
Comment change.
any chance we can get this patch included in the near future? This is ready to commit. You can e-mail any port committer and ask them -) A commit references this bug: Author: jbeich Date: Wed Nov 2 02:57:04 UTC 2016 New revision: 425102 URL: https://svnweb.freebsd.org/changeset/ports/425102 Log: security/tor: add multi-instance support PR: 207129 Submitted by: yuri@rawbw.com (maintainer) Changes: head/security/tor/Makefile head/security/tor/files/pkg-message.in head/security/tor/files/tor.in Do you want the same change to security/tor-devel? If so, please, submit another patch. Jan, Not at this time. This should be enough for users for now. Instead, I am going to generalize the same approach, and submit it as a patch into /etc/rc.subr, so that it will be easy to apply it to other ports. Hi, I'm sorry I stumbled on a problem: since upgrading from 0.2.8.9 to 0.2.8.9_1 the "onestatus" no longer works if more than one instance is configured. reproducer: install tor-0.2.8.9 service tor onestatus (works as expected) upgrade to tor 0.2.8.9_1 configure two instances, set tor_enable="YES" and tor_disable_default_instance="YES" while they are NOT running run: service tor onestatus output: tor is not running. /usr/local/etc/rc.d/tor: WARNING: onestatus failed for the tor instance inst1:.. tor is not running. /usr/local/etc/rc.d/tor: WARNING: onestatus failed for the tor instance inst2:.. background, other tools like ansible rely on the output of 'service <name> onestatus' for their service management module: https://github.com/ansible/ansible-modules-core/blob/devel/system/service.py#L954 Also invoking service tor start twice will give you a similar problem on the second run (when instances are already running). regarding multi-instance support in rc.subr, I asked Hiroki Sato a while ago but it didn't go anywhere apparently. https://lists.freebsd.org/pipermail/freebsd-rc/2014-October/003570.html Yes, I see what you are saying. This is only the matter of the messages, not commands themselves. I will make messages and error codes more intelligent, and will attach another patch. Created attachment 176669 [details]
Patch improving instance messages and error codes
nusenu,
This patch attempts to address your complaint:
1. It removes the extraneous warning message.
2. It prepends service messages with the instance identification when instances are used.
3. It aggregates error codes into the final code of the 'service' command.
4. It also has some comments change.
5. It bumps PORTREVISION.
Please let me know if this fixes the problem.
Yuri
As for /etc/rc.subr patch, I will generalize rc script as a procedure that will make it applicable to any ports. net/cjdns is one other port I have in mind. This shouldn't be particularly difficult, just some tedious shell language. Hi, thank you for your always timely reactions! I tested the patch (manually copied over the lines) and return code values are as expected now. Preliminary tests with ansible's service module showed that it works as intended now. Regarding the general approach: If rc.subr gets multi-instance support it would be great if once could restart/reload/start/stop specific instances of a service as well. Is there already a bugzilla entry for rc.subr multi-instance support? (In reply to nusenu from comment #34) You're welcome! (In reply to nusenu from comment #35) I just created the problem for the general instance implementation suggestion: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214284 It includes manipulation with individual instances. Comment on attachment 176669 [details]
Patch improving instance messages and error codes
This patch is ready to be committed as is. But I will also implement the individual instance manipulation functionality for Tor, and will update it soon.
Created attachment 176703 [details] Patch improving instance messages and error codes, implementing single instance operations With this patch, syntax of the 'service' call becomes: > service tor <command> {<instance>} <instance> can be either one of the extra-instances, or 'main' for the original instance. A commit references this bug: Author: jbeich Date: Mon Nov 7 06:24:37 UTC 2016 New revision: 425595 URL: https://svnweb.freebsd.org/changeset/ports/425595 Log: security/tor: improve multi-instance support - rc.d commands now accept optional instance argument - `status` command output is no longer ambigous before: $ service tor status tor is running as pid 22222. tor is running as pid 33333. tor is running as pid 11111. after: $ service tor status tor instance inst1: tor is running as pid 22222. tor instance inst2: tor is running as pid 33333. tor main instance: tor is running as pid 11111. $ service tor restart inst1 tor instance inst1: Stopping tor. Waiting for PIDS: 22222. Starting tor. [...] PR: 207129 Submitted by: Yuri Victorovich <yuri@rawbw.com> (maintainer) Changes: head/security/tor/Makefile head/security/tor/files/tor.in Thanks. Landed with minor change: /usr/local -> %%PREFIX%% as it was before. Thanks! Note, you may want to hide some errors: $ service tor status tor instance inst1: mkdir: /var/db/tor/instance@inst1: Permission denied tor is running as pid 22222. tor instance inst2: mkdir: /var/db/tor/instance@inst2: Permission denied tor is running as pid 33333. tor main instance: tor is running as pid 11111. (In reply to Jan Beich (mail not working) from comment #42) Thanks, I will fix this. Yuri (In reply to Yuri Victorovich from comment #38) To be honest I'd prefer if the instance identifier would be part of the service name (see first post in this bug entry) because that would not break existing tools like ansible (and probably puppet?). I like how in OpenBSD it was really easy to implement something like multi-instance support by simply linking tor's rc.d script as often as you want with a new name[1] (i.e. tor_inst1). Anyway I really appreciate your work on this bug - thanks! [1] https://github.com/nusenu/ansible-relayor/blob/master/tasks/openbsd_service.yml (In reply to nusenu from comment #44) I see what you are saying. I don't use OpenBSD. But there's almost nothing that prevents the same on FreeBSD. These two ways can perfectly coexist. Just the 'name' variable should be derived from $0 in rc script, and all variables should be updated accordingly to depend on 'name'. Personally, I view the need to create symbolic links as a hack rather than a feature, but this is entirely a matter of personal preference. Thank you for pointing this out. Will implement this once I have time. Yuri If to do this for other services, one needs to change lines in rc script in this fashion in each one: > : ${xxx_flags:="-a -b -c"} to > eval ": \${${name}_flags:=\"-a -b -c\"}" These would be more extensive changes in each rc script in ports, vs. just changing 2 lines the way how it is now in Tor. This is why I saw this way as a better choice. |
