Comment 1 Rene Ladan 2016-06-27 21:44:42 UTC

Maintainer reset.

New maintainer, request feedback.

Comment 3 Yuri Victorovich 2016-10-24 03:35:36 UTC

Created attachment 176091 [details]
patch

Implemented multi-instance support.
Bumped PORTREVISION.

Passes poudriere.

Comment 4 Yuri Victorovich 2016-10-24 07:44:50 UTC

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

Comment 6 Yuri Victorovich 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.

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 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 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.

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 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.

One more comment/question:
Can I also specify the pidfile in tor_instances=..
to override the hardcoded path?

Comment 13 Yuri Victorovich 2016-10-24 17:50:57 UTC

It is doable, I just didn't think this ability was needed.

Comment 14 Yuri Victorovich 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.

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?

Comment 17 Yuri Victorovich 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 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

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 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 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 2016-10-24 22:19:42 UTC

Created attachment 176123 [details]
patch

Minor corrections.

Comment 23 Yuri Victorovich 2016-10-24 22:36:32 UTC

Created attachment 176124 [details]
patch

Minor improvement.

Comment 24 Yuri Victorovich 2016-10-25 16:55:09 UTC

Created attachment 176150 [details]
patch

Comment change.

any chance we can get this patch included in the near future?

Comment 26 Yuri Victorovich 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 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 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 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.

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 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 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 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.

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?

Comment 36 Yuri Victorovich 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 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 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 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 2016-11-07 06:29:17 UTC

Thanks. Landed with minor change: /usr/local -> %%PREFIX%% as it was before.

Comment 41 Yuri Victorovich 2016-11-07 06:32:20 UTC

Thanks!

Comment 42 Jan Beich 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 2016-11-07 06:57:45 UTC

(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

Comment 45 Yuri Victorovich 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 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.