Opened 2 years ago

Closed 13 months ago

Last modified 13 months ago

#8368 closed enhancement (implemented)

Add tor.service (for systemd) to upstream package

Reported by: jamielinux Owned by:
Priority: normal Milestone: Tor: 0.2.5.x-final
Component: Tor Version:
Keywords: tor-relay, systemd, 025-triaged, nickm-review-0255 Cc: intrigeri@…
Actual Points: Parent ID:
Points:

Description

In Fedora we have a custom systemd service file for running Tor. We are encouraged to push changes upstream, and thus I am proposing that it be included as part of the upstream tarball. I have pasted the contents below, but please do advise on whether this can be improved:

$ cat tor.service
[Unit]
Description = Anonymizing overlay network for TCP
After = syslog.target network.target nss-lookup.target

[Service]
Type = simple
ExecStart = /usr/bin/tor --runasdaemon 0 --defaults-torrc /usr/share/tor/defaults-torrc -f /etc/tor/torrc --quiet
ExecReload = /bin/kill -HUP ${MAINPID}
ExecStop = /bin/kill -INT ${MAINPID}
TimeoutSec = 30
Restart = on-failure
LimitNOFILE = 4096

[Install]
WantedBy = multi-user.target

Child Tickets

Attachments (3)

Change History (22)

comment:1 Changed 2 years ago by jamielinux

Actually, the ExecStart line should probably just say this:

ExecStart = /usr/bin/tor --runasdaemon 0 -f /etc/tor/torrc --quiet

comment:2 Changed 2 years ago by nickm

  • Keywords tor-relay added
  • Milestone set to Tor: 0.2.5.x-final
  • Status changed from new to needs_review

If we're going to ship this with Tor, it should probably have a comment at the top explaining what it is and how to use it.

I'm not very knowledgeable about how Unixes launch things nowadays -- is this Fedora-specific, or will this file work with lots of other systems?

(If the latter, has anybody else written a systemd file that we could also look at to see if it has any ideas that this one might be missing?)

comment:3 follow-up: Changed 2 years ago by jamielinux

If we're going to ship this with Tor, it should probably
have a comment at the top explaining what it is and how
to use it.

Sure. Something like:

# This is a service file for use with the systemd init daemon. It should be
# placed with other systemd units, usually at /usr/lib/systemd/system/tor.service

I'm not very knowledgeable about how Unixes launch things nowadays
-- is this Fedora-specific, or will this file work with lots of
other systems?

It should work with any other distribution that has implemented systemd, which now includes Arch Linux (default init), OpenSUSE (default init in 12.1), soon Mageia (will be default init from version 3) and probably others.

(If the latter, has anybody else written a systemd file that we
could also look at to see if it has any ideas that this one might
be missing?)

Yes that's a good idea. The Arch Linux one is here:
https://projects.archlinux.org/svntogit/community.git/tree/trunk/tor.service?h=packages/tor

They do things a bit differently but dropping the service file proposed here into Arch Linux would work fine, and vice versa.

I'm also not sure what the best value for LimitNOFILE is yet. The sysv initscripts distributed at the moment have some logic to raise ulimit based on the file descriptors available.

comment:4 in reply to: ↑ 3 Changed 2 years ago by arma

Replying to jamielinux:

I'm also not sure what the best value for LimitNOFILE is yet. The sysv initscripts distributed at the moment have some logic to raise ulimit based on the file descriptors available.

Something like https://gitweb.torproject.org/debian/tor.git/blob/HEAD:/debian/tor.init#l38 ?

If you want to pick just one ulimit -n, 32768 is a good choice.

4096 is way too low for fast relays.

comment:5 Changed 2 years ago by nickm

It looks as if we're converging on a thing to ship here. The ideal format to get this in would be a patch generated by "git format-patch", containing a file for the "changes" directory, as described in doc/HACKING. Next-best would be just a patch.

comment:6 Changed 2 years ago by jamielinux

If you want to pick just one ulimit -n, 32768 is a good choice.

4096 is way too low for fast relays.

Thanks for the advice. I've fixed this in git for the Fedora package and have fixed in the patch attached.

The ideal format to get this in would be a patch generated by
"git format-patch", containing a file for the "changes" directory,
as described in doc/HACKING.

Sure, I've attached a patch.

comment:7 Changed 2 years ago by jamielinux

We've been considering hardening the tor systemd service and I paste our current draft below. We're working on further hardening and testing at the moment.

I do have one question. If limiting the tor service to specific devices (with DeviceAllow), does it actually need /dev/random?

[Service]
...
PrivateTmp = yes
LimitNPROC = 2
DeviceAllow = /dev/null rw
DeviceAllow = /dev/urandom r
DeviceAllow = /dev/random r
InaccessibleDirectories = /
ReadOnlyDirectories = /etc /usr
ReadWriteDirectories = /var/lib/tor /var/log/tor

comment:8 Changed 2 years ago by nickm

If you have a working /dev/urandom, Tor shouldn't need /dev/random.

comment:9 Changed 22 months ago by Joshua

  • Priority changed from minor to normal

Are the arguments "--runasdaemon 0" and "--quiet" really needed? If possible they should be omitted, because they overwrite the settings in the torrc file, making it impossible to change them.

I've got this problem at the moment with the SysVinit script, which uses the deprecated TorCtl. TorCtl forces the argument '--log \"notice file $LOGFILE\"' upon the user, which results in my logging settings in /etc/torrc being ignored. This being very annoying to me, I think the incorporation of a cleaner systemd script is not "minor"; I've changed the priority to "normal".

comment:10 Changed 21 months ago by intrigeri

  • Cc intrigeri@… added

comment:11 Changed 21 months ago by intrigeri

According to Michael Stapelberg (who is maintaining systemd in Debian), --quiet should not be used in this case: it loses some valuable input that would otherwise be collected by systemd and displayed with "systemctl status tor" (especially useful if it fails to start, actually).

For the record, I've been working on a systemd profile too. I will merge with the one proposed here. The main area I've been working on is the AppArmor integration (currently done with aa-exec in the Debian initscript), first with a rough shell wrapper, and working with systemd upstream to add native AppArmor profile loading support so that we can get rid of the wrapper eventually.

comment:12 Changed 15 months ago by nickm

  • Keywords systemd added

comment:13 Changed 15 months ago by misc

I would remove --quiet and set it in the configuration if needed. For --runasdaemon, this is needed for Type=simple, so changing that setting would break the service file ( ie, if it fork when we tell to not fork ), but it can be dropped if we switch to Type=notify as proposed in another bug report.

I am not sure ExecStop is needed, since by default, a SIGTERM is sent. If that's not sufficient, I would say using KillSignal would be better that explicitly using kill, 1 less fork :)

comment:14 Changed 14 months ago by andrea

  • Keywords 025-triaged added

comment:15 Changed 13 months ago by jamielinux

I've attached an improved patch with some saner defaults. This service file is tested for Fedora 20 and should work on any Linux distribution that runs on systemd.

I've removed --quiet, which was a remnant from the previous packager. I should have noticed and removed this sooner. I've also removed --runasdaemon 0, as 0 is the default. Specifying it is useless and overrides the user. As misc suggested, I've replaced ExecStop with KillSignal.

The hardening options are optional but provide some simple restrictions that add a layer of protection.

The V2 patch also restricted read-only/read-write access to certain directories, but this is dependent on where the "DataDirectory" and "log debug file" locations are, so I've removed them in the V3 patch.

Last edited 13 months ago by jamielinux (previous) (diff)

comment:16 Changed 13 months ago by nickm

  • Keywords nickm-review-0255 added

comment:17 Changed 13 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

Thanks! I've merged your v3 patch, put it in the right place (we reorganized contrib yesterday), made it so that the paths get fixed up by autoconf, and written a changes file. Your patch went in as 71d77fa6f1033117174b714f72c3be0cfa6d9ff8; the tweaks were 33ca09b5b98d363d506864bb83a500aa7d073860. Thanks!

comment:18 Changed 13 months ago by nickm

Whoops; those commit numbers turned out to be a42e81eea10363654cde5724337c4aaea2bb40d0 and cae638805385ae0ef717eb21e1592aeb1d85182c.

comment:19 Changed 13 months ago by jamielinux

Nice. Thanks Nick! :)

Note: See TracTickets for help on using tickets.