Opened 2 years ago

Last modified 3 months ago

#8908 needs_revision enhancement

Tor systemd socket activation support

Reported by: cypherpunks Owned by: intgr
Priority: trivial Milestone: Tor: 0.2.7.x-final
Component: Tor Version: Tor: 0.2.7
Keywords: tor-relay, systemd, lorax, 027-triaged-1-in Cc: steven@…
Actual Points: Parent ID:
Points: small

Description

Allow Tor to be started on-demand on Linux under systemd.

WIP patch posted here: https://lists.torproject.org/pipermail/tor-dev/2013-May/004885.html

More info about socket activation: http://0pointer.de/blog/projects/socket-activation.html

Child Tickets

Change History (18)

comment:1 Changed 2 years ago by arma

  • Priority changed from minor to normal

comment:2 Changed 2 years ago by nickm

  • Milestone set to Tor: 0.2.5.x-final

comment:3 Changed 2 years ago by nickm

  • Status changed from new to needs_review

comment:4 Changed 2 years ago by nickm

This actually looks reasonably solid. Here are a few points we should think about:

  • Does it actually work to only start Tor when Tor receives a SocksPort or ControlPort request? When Tor first starts after significant downtime, it needs to download a pretty big amount of directory data, and build enough circuits for user traffic. Does that happen fast enough to answer the request that made systemd launch Tor?
  • A Tor is _supposed_ to actually turn itself nearly off when it sees no user traffic. Does your need for this feature mean that feature is not working?
  • If this is incompatible with hibernation, shouldn't we detect its use along with hibernation, and warn the user?
  • It appears that we do systemd socket discovery by default, unconditionally. Can that be right?
  • I would like if the implementation in Tor (that is, the parts outside of the sd-daemon.c code) were not systemd-specific. For example, it would also be nice to have the ability to write a little launcher program that bound to some low ports, then did a setuid(tor-daemon) and exec()d Tor. That program might want to pass a list of fds via the command line. For such a program, it would make sense to reuse 90% of this code, but have the fds come from some source other than sd-daemon.c.
  • There are no unit tests for this code at all. Is there anything we can do to make this tested? I would love for 0.2.5 to be the release where we stop adding untested code.

Minor nitpicks:

  • The new C code in Tor doesn't follow K&R indentation style like the rest of Tor.
  • I bet when we go to compile this on windows, we'll find at least one or two more methods that should have been wrapped in #ifndef _WIN32.

I can look at the code in more detail later, but I figured a rapid review here would be of benefit.

comment:5 Changed 2 years ago by intgr

  • Owner changed from cypherpunks to intgr
  • Priority changed from normal to minor
  • Status changed from needs_review to assigned

This actually looks reasonably solid

Thanks. It wasn't meant to be ready for merging yet, but it works well enough for me and I wanted to get some feedback before I invest more time into it.

When Tor first starts after significant downtime, it needs to download a pretty big amount of directory data, and build enough circuits for user traffic. Does that happen fast enough to answer the request that made systemd launch Tor?

I tried it by deleting /var/lib/tor and running "time torify wget http://httpbin.org/ip -o/dev/null -O-"

Over five tries I got 27 seconds, 11s, 9s, 67s, 12s. I guess some applications might hit timeouts at 67 seconds, but wget and Firefox don't seem to have a problem.

If this is incompatible with hibernation, shouldn't we detect its use along with hibernation, and warn the user?

Well it should be fixed one way or another before merging. As mentioned in the mailing list thread, we could keep the listening sockets open, but simply reject new incoming connections.

It appears that we do systemd socket discovery by default, unconditionally. Can that be right?

I think that's fine. If it isn't started by systemd then the environment vars will be unset and it will behave just like before.

For example, it would also be nice to have the ability to write a little launcher program that bound to some low ports, then did a setuid(tor-daemon) and exec()d Tor

The interface used by systemd to pass in sockets is pretty generic (sets 2 environment vars, LISTEN_PID and LISTEN_FDS). I think it could be re-used for that. TODO: documentation.

Is there anything we can do to make this tested?

Sure, I'll try to figure something out.

comment:6 Changed 2 years ago by intgr

  • Priority changed from minor to normal
  • Status changed from assigned to needs_revision

Oops, I messed up the fields (dunno how).

comment:7 Changed 2 years ago by nickm

  • Keywords tor-relay added

comment:8 Changed 2 years ago by stebalien

  • Cc steven@… added

comment:9 Changed 17 months ago by nickm

  • Keywords systemd added

comment:10 Changed 17 months ago by misc

So looking at the patch (quickly), I have a few remark :

  • the use and inclusion of sd-daemon.c is deprecated. I would also recommend against bundling if possible.
  • in systemd_adopt_socket, why not use the sd_is_socket function ?
  • in connection_listener_new, why do you close the tor_close_socket if listen fail ( ie, this is not present in the original code, and that's already done in the cleanup label code, after the label 'err')

Otherwise, that's quite a interesting approach ( comparing the exiting config with the systemd socket ), so I will take a bit more time to look in more details.

comment:11 Changed 16 months ago by nickm

  • Milestone changed from Tor: 0.2.5.x-final to Tor: 0.2.6.x-final

Moving nearly all needs_revision tickets into 0.2.6, as now untimely for 0.2.5.

comment:12 Changed 10 months ago by nickm

  • Milestone changed from Tor: 0.2.6.x-final to Tor: 0.2.???

I think we could take this if the revisions get made, but we're shouldn't block a release on the revisions.

comment:13 Changed 8 months ago by candrews

*bump*

Any updates on this issue? It certainly seems like it would be a valuable addition to tor.

comment:14 Changed 7 months ago by nickm

If somebody cleans up the above issues in the patch, I believe it could get merged.

comment:15 Changed 7 months ago by nickm

  • Keywords lorax added

comment:16 Changed 4 months ago by nickm

  • Milestone changed from Tor: 0.2.??? to Tor: 0.2.7.x-final

These might also be worth looking at in 0.2.7

comment:17 Changed 3 months ago by nickm

  • Keywords 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:18 Changed 3 months ago by isabela

  • Points set to small
  • Priority changed from normal to trivial
  • Version changed from Tor: unspecified to Tor: 0.2.7
Note: See TracTickets for help on using tickets.