Opened 9 years ago

Closed 9 years ago

#2091 closed enhancement (implemented)

Support ControlSocket as well as ControlPort

Reported by: arma Owned by:
Priority: High Milestone:
Component: Archived/Vidalia Version:
Severity: Keywords:
Cc: weasel Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Quoting from https://trac.vidalia-project.net/ticket/239 :

One suggestion recently was to let Tor have an open ControlSocket on Debian by default, such that only users in a particular group can connect to it and talk to Tor. This means Vidalia will be able to talk to Tor by default, but only if the Vidalia user has the appropriate privileges. This also means that Vidalia won't try to run Tor as the current user, which is a good change.

Tor has supported the ControlSocket option since 0.2.0.3-alpha, but no controllers have ever used it (so it may not still work as advertised). But controlsocket support seems to be one of the main blocking issues in letting Vidalia talk to the Debian tor daemon safely.

Child Tickets

Attachments (2)

controlsocket.patch (49.1 KB) - added by chiiph 9 years ago.
Patch for ControlSocket implementation
controlsocket-v2.patch (41.3 KB) - added by chiiph 9 years ago.
New patch. Changes regarding Matt's comments

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by chiiph

From a quick look it seems this can be easy to implement in Vidalia.

What I still don't get is what's the difference between setting Tor's ControlPort or setting ControlSocket path, because if Tor's already running, you have to either set one control or the other to communicate, and if Tor isn't running, Vidalia won't be able to call the init script, since it's a service and as such it must be initiated as root.
For what I understand ControlSocket only provides a different way of communicating to Tor, it doesn't give any extra control over it. But may be I'm getting this wrong.

comment:2 Changed 9 years ago by arma

Cc: weasel added

I believe (and we should get weasel and erinn to weigh in on this) that one plan was to make a tor group, and make the default Tor deb open a controlsocket that accepts connections from applications in the tor group, and then when we install Vidalia we have its install scripts fix up the group membership (possibly asking the human which user will be running Vidalia) so things will work smoothly.

I can imagine some problems with this approach though, so I think we should get weasel to remind us what he was thinking long ago.

comment:3 Changed 9 years ago by arma

See also item n on https://www.torproject.org/getinvolved/volunteer#Projects

Vidalia currently doesn't play nicely with Tor on Linux and Unix platforms. Currently, on Debian and Ubuntu, there is a configuration mechanism which allows Vidalia to override Tor's ability to start on boot (by sourcing /etc/default/tor.vidalia which sets RUN_DAEMON=no at the user's request), but full implementation of ControlPort communication is still required.
A better solution on Linux and Unix platforms would be to use Tor's ControlSocket, which allows Vidalia to talk to Tor via a Unix domain socket, and could possibly be enabled by default in Tor's distribution packages. Vidalia can then authenticate to Tor using filesystem-based (cookie) authentication if the user running Vidalia is also in the distribution-specific tor group.
This project will first involve adding support for Tor's ControlSocket to Vidalia. The student will then develop and test this support on various distributions to make sure it behaves in a predictable and consistent manner on all of them.
The next challenge would be to find an intuitive and usable way for Vidalia to be able to change Tor's configuration (torrc) even though it is located in /etc/tor/torrc and thus immutable. In Debian and Ubuntu we handle this with the aforementioned /etc/default/tor.vidalia but this functionality could (or should) be less distribution-specific.
The best idea we've come up with so far is to feed Tor a new configuration via the ControlSocket when Vidalia starts, but that's bad because if the user is not using the latest Debian/Ubuntu packages, they may not have disabled Tor's ability to run on boot and will end up with a configuration that is different from what they want. The second best idea we've come up with is for Vidalia to write out a temporary torrc file and ask the user to manually move it to /etc/tor/torrc, but that's bad because users shouldn't have to mess with files directly.
A person undertaking this project should have prior knowledge of various Linux distributions and their packaging mechanisms as well as some C++ development experience. Previous experience with Qt is helpful, but not required.

comment:4 in reply to:  2 Changed 9 years ago by chiiph

I'll start working on adding ControlSocket support to Vidalia, and we'll see from there how to use that to make a better app for Debian and Ubuntu, either way, it's a good idea to have Vidalia know how to deal with each possibility.

comment:5 Changed 9 years ago by dererk

Just an idea, expanded from here (note that some part of it has been already implemented):
http://bugs.debian.org/552556

What I've in mind that could solve part of the problem presented here, is for Tor to include configuration from a /etc/tor/tor.d/ likeish director, in the same fashion as other software (Apache, DBus, Logrotate, PAM, Nginx, Rsyslog, Saned, Tomcat, Xinetd, between others) does,

In that way, Tor could provide (or vidalia install) a file like /etc/tor/tor.d/vidalia, that could contain vidalia's config coming through whatever control method used. That would play nicely with Debian policy and most others.

This approach makes sense for upgrades too, as you don't mess (and shouldn't) with the main tor configuration, upgrades will go through nicely.

Again, it's just an idea.

comment:6 Changed 9 years ago by chiiph

I have a working implementation of ControlSocket that's still a work in progress though. I have to debug it a little bit more, but I'm posting this to see if I can get more eyes on it.

It still lacks the proper documentation, so that's why I won't post a patch in here yet, but the code can be accessed through my git mirror/fork of Vidalia in http://github.com/chiiph/vidalia in the branch controlsocket.

The only visual change is in the Advanced tab in Settings, under the hood there are a lot of changes, specially in the torcontrol subdirectory (obviously).

comment:7 Changed 9 years ago by chiiph

Another problem is that Vidalia when attaches to an already running Tor, when you close Vidalia, it stops Tor too but not as it should (calling /etc/init.d/tor stop, or something like that), either way it shouldn't stop the instance.
I'll fix this issue in the git repo I referenced earlier.

Changed 9 years ago by chiiph

Attachment: controlsocket.patch added

Patch for ControlSocket implementation

comment:8 Changed 9 years ago by edmanm

Here's my comments on the patch, broken down by severity.

  1. [MAJOR] You've created a circular dependency between src/vidalia and src/torcontrol. src/torcontrol should NOT depend on anything in src/vidalia. You don't actually need to instantiate a TorSettings object in the ControlConnection class. You could simply pass the appropriate arguments to the connect() method based on whichever control method is configured.
  1. [MINOR] "Control" is a little too vague of a class name. Perhaps something like AbstractControlMethod, AbstractControlConnection, AbstractControlBase, or similar would be more descriptive of what that class actually does.
  1. [TRIVIAL] More 'stdset=0' stuff lurks in the .ui files.
  1. [MAJOR] The code common to both ControlSocket.cpp and ControlPort.cpp needs to be refactored out into your base class. It makes no sense to have huge chunks of identical code in both places, particularly since they both inherit from the same abstract base class.
  1. [MINOR] The "ControlMethod" setting should be a string, rather than just an enum number. If someone is looking at vidalia.conf, "ControlMethod=ControlSocket" is much clearer than "ControlMethod=1". See how the authentication method settings are handled as a reference. TorSettings::getControlMethod() should still return the appropriate enum value, but the conversion happens "behind the scenes" in the TorSettings class.
  1. [TRIVIAL] It looks like your added accessors getControlMethod() and getSocketPath() should be marked const.
  1. [TRIVIAL] The ControlMethod enum value passed as an argument to TorSettings::setControlMethod() doesn't actually need to be passed by const reference.
  1. [TRIVIAL] We try to prepend "_" to private member variables of a class, so they're easy to identify in their respective .cpp files. So, your "sock" variable in the ControlSocket class should be "_sock" (or even "_socket").
  1. [MINOR] It doesn't look like you're setting a default value for SETTING_SOCKET_PATH in the TorSettings constructor, even if it's only an empty string.

I'd like to review your revised patch again before you commit it.

comment:9 in reply to:  8 Changed 9 years ago by chiiph

Replying to edmanm:

  1. [MAJOR] The code common to both ControlSocket.cpp and ControlPort.cpp needs to be refactored out into your base class. It makes no sense to have huge chunks of identical code in both places, particularly since they both inherit from the same abstract base class.

The main problem with this is that the only parent class in common that QLocalSocket and QTcpSocket have is QIODevice, but it has abstract definitions of the most important methods, so I can't make the AbstractControlMethod class implement them using a QIODevice that later on is instanciated with a QLocalSocket or QTcpSocket, because they all use readLine or methods like that that are pure virtual in QIODevice. Can you see any alternative? I had a lot of problems with this when I was planning the whole hierarchy change to add ControlSocket.

Changed 9 years ago by chiiph

Attachment: controlsocket-v2.patch added

New patch. Changes regarding Matt's comments

comment:10 Changed 9 years ago by chiiph

Resolution: implemented
Status: newclosed

The patch has been merged in svn revision 4492.

Note: See TracTickets for help on using tickets.