Bug 207129 - security/tor: Request to add multi-instance support
Summary: security/tor: Request to add multi-instance support
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Jan Beich
URL:
Keywords: feature
Depends on:
Blocks:
 
Reported: 2016-02-12 11:39 UTC by nusenu
Modified: 2016-11-07 23:51 UTC (History)
1 user (show)

See Also:
vlad-fbsd: maintainer-feedback? (yuri)


Attachments
patch (2.75 KB, patch)
2016-10-24 03:35 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch (2.77 KB, patch)
2016-10-24 07:44 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch (3.86 KB, patch)
2016-10-24 19:00 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch (4.05 KB, patch)
2016-10-24 20:35 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch (4.04 KB, patch)
2016-10-24 22:19 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch (4.01 KB, patch)
2016-10-24 22:36 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
patch (4.05 KB, patch)
2016-10-25 16:55 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
Patch improving instance messages and error codes (2.07 KB, patch)
2016-11-05 22:53 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff
Patch improving instance messages and error codes, implementing single instance operations (2.66 KB, patch)
2016-11-06 21:55 UTC, Yuri Victorovich
yuri: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nusenu 2016-02-12 11:39:34 UTC
There are multiple examples of ports that support multiple instances:



openvpn: 

# This script supports running multiple instances of openvpn.
# To run additional instances link this script to something like
# % ln -s openvpn openvpn_foo
# and define additional openvpn_foo_* variables in one of
# /etc/rc.conf, /etc/rc.conf.local or /etc/rc.conf.d/openvpn_foo

https://lists.freebsd.org/pipermail/freebsd-rc/2016-February/003678.html

tomcat:
https://lists.freebsd.org/pipermail/freebsd-rc/2016-February/003680.html


Since does not scale well across multiple CPUs, relay operators have to run multiple instances, therefore it would make sense to add multi-instance support to tor's rc script.
Comment 1 Rene Ladan freebsd_committer 2016-06-27 21:44:42 UTC
Maintainer reset.
Comment 2 VK freebsd_triage 2016-10-02 20:02:28 UTC
New maintainer, request feedback.
Comment 3 Yuri Victorovich freebsd_committer 2016-10-24 03:35:36 UTC
Created attachment 176091 [details]
patch

Implemented multi-instance support.
Bumped PORTREVISION.

Passes poudriere.
Comment 4 Yuri Victorovich freebsd_committer 2016-10-24 07:44:50 UTC
Created attachment 176094 [details]
patch
Comment 5 nusenu 2016-10-24 15:39:02 UTC
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
Comment 6 Yuri Victorovich freebsd_committer 2016-10-24 16:16:57 UTC
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.
Comment 7 nusenu 2016-10-24 17:06:16 UTC
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)?
Comment 8 Yuri Victorovich freebsd_committer 2016-10-24 17:13:45 UTC
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).
Comment 9 Yuri Victorovich freebsd_committer 2016-10-24 17:16:10 UTC
If you want to start them as root, you should say "inst1:root:wheel" and write the desired user into torrc@inst1.
Comment 10 nusenu 2016-10-24 17:19:39 UTC
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)
Comment 11 Yuri Victorovich freebsd_committer 2016-10-24 17:25:07 UTC
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.
Comment 12 nusenu 2016-10-24 17:43:30 UTC
One more comment/question:
Can I also specify the pidfile in tor_instances=..
to override the hardcoded path?
Comment 13 Yuri Victorovich freebsd_committer 2016-10-24 17:50:57 UTC
It is doable, I just didn't think this ability was needed.
Comment 14 Yuri Victorovich freebsd_committer 2016-10-24 18:10:18 UTC
Ok, I will redefine instance as: inst_name{:inst_conf:inst_user:inst_group:inst_pidfile:inst_data_dir} for maximum flexibility.
Comment 15 nusenu 2016-10-24 18:14:23 UTC
Awesome!
I'm happy to test the upcoming patch.
Comment 16 nusenu 2016-10-24 18:48:56 UTC
Since I'm auto-generating rc.local entries:
are multiple occurrences of
tor_instances=...
merged or does the last one override the previous entry?
Comment 17 Yuri Victorovich freebsd_committer 2016-10-24 18:58:53 UTC
tor_instances should be space-separated.
As usual in shell, you can extend the variable by:
> tor_instances="${tor_instances} inst..."
Comment 18 Yuri Victorovich freebsd_committer 2016-10-24 19:00:42 UTC
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
Comment 19 nusenu 2016-10-24 20:13:39 UTC
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?
Comment 20 Yuri Victorovich freebsd_committer 2016-10-24 20:35:58 UTC
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.
Comment 21 Yuri Victorovich freebsd_committer 2016-10-24 20:41:34 UTC
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
Comment 22 Yuri Victorovich freebsd_committer 2016-10-24 22:19:42 UTC
Created attachment 176123 [details]
patch

Minor corrections.
Comment 23 Yuri Victorovich freebsd_committer 2016-10-24 22:36:32 UTC
Created attachment 176124 [details]
patch

Minor improvement.
Comment 24 Yuri Victorovich freebsd_committer 2016-10-25 16:55:09 UTC
Created attachment 176150 [details]
patch

Comment change.
Comment 25 nusenu 2016-11-01 22:57:46 UTC
any chance we can get this patch included in the near future?
Comment 26 Yuri Victorovich freebsd_committer 2016-11-01 23:00:18 UTC
This is ready to commit. You can e-mail any port committer and ask them -)
Comment 27 commit-hook freebsd_committer 2016-11-02 02:57:23 UTC
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
Comment 28 Jan Beich freebsd_committer 2016-11-02 02:59:10 UTC
Do you want the same change to security/tor-devel? If so, please, submit another patch.
Comment 29 Yuri Victorovich freebsd_committer 2016-11-02 03:46:20 UTC
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.
Comment 30 nusenu 2016-11-05 12:45:30 UTC
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
Comment 31 Yuri Victorovich freebsd_committer 2016-11-05 20:18:50 UTC
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.
Comment 32 Yuri Victorovich freebsd_committer 2016-11-05 22:53:58 UTC
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
Comment 33 Yuri Victorovich freebsd_committer 2016-11-05 23:52:10 UTC
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.
Comment 34 nusenu 2016-11-06 10:57:38 UTC
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.
Comment 35 nusenu 2016-11-06 11:04:40 UTC
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?
Comment 36 Yuri Victorovich freebsd_committer 2016-11-06 21:06:26 UTC
(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 37 Yuri Victorovich freebsd_committer 2016-11-06 21:08:16 UTC
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.
Comment 38 Yuri Victorovich freebsd_committer 2016-11-06 21:55:42 UTC
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.
Comment 39 commit-hook freebsd_committer 2016-11-07 06:25:14 UTC
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
Comment 40 Jan Beich freebsd_committer 2016-11-07 06:29:17 UTC
Thanks. Landed with minor change: /usr/local -> %%PREFIX%% as it was before.
Comment 41 Yuri Victorovich freebsd_committer 2016-11-07 06:32:20 UTC
Thanks!
Comment 42 Jan Beich freebsd_committer 2016-11-07 06:49:27 UTC
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.
Comment 43 Yuri Victorovich freebsd_committer 2016-11-07 06:57:45 UTC
(In reply to Jan Beich (mail not working) from comment #42)

Thanks, I will fix this.

Yuri
Comment 44 nusenu 2016-11-07 23:15:00 UTC
(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
Comment 45 Yuri Victorovich freebsd_committer 2016-11-07 23:36:19 UTC
(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
Comment 46 Yuri Victorovich freebsd_committer 2016-11-07 23:51:18 UTC
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.