Make Torbutton work with Unix Domain Socket option
Even if it is only for *NIX systems first, we should modify Torbutton to work with the Unix Domain Socket option.
Activity
Trac:
Summary: Make Torbutton work with SocksSocket option to Make Torbutton work with Unix Domain Socket option
Description: Even if it is only for *NIX systems first, we should modify Torbutton to work with the SocksSocket option.to
Even if it is only for *NIX systems first, we should modify Torbutton to work with the Unix Domain Socket option.
As far I understand, this is up to
Make Tor Launcher work with Unix Domain Socket option(#14272 (moved)). What is Tor Button's role in this?
Replying to proper:
As far I understand, this is up to
Make Tor Launcher work with Unix Domain Socket option(#14272 (moved)). What is Tor Button's role in this?Torbutton includes code that interacts with tor via the control port (e.g., newnym, circuit display). If we disable all TCP in Tor Browser, control port communication will need to be via a Unix domain socket as well (so Torbutton will need to be modified).
Trac:
Cc: N/A to arthuredelstein, mcs, properTrac:
Cc: arthuredelstein, mcs, proper to arthuredelstein, mcs, proper, whonix-devel@whonix.org
Here is a patch for review: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-01&id=115d96e5ca5a861573a6efe08fbb3ba7a3ce149d For this to work correctly, you will need a Tor Launcher that includes the changes from #14272 (moved).
Trac:
Reviewer: N/A to N/A
Status: assigned to needs_review
Severity: N/A to Normal
Keywords: TorBrowserTeam201608 deleted, TorBrowserTeam201608R addedI mean, one that includes both changes, #14271 (moved) and #14272 (moved).
-
Replying to adrelanos:
Do you have a branch and/or downloadable version that I could use to check Whonix compatibility?
We do not have a packaged Tor Browser that includes these changes. You would need to build Torbutton from the bug14271-01 branch within https://git.torproject.org/user/brade/torbutton.git and build Tor Launcher from the bug14272-01 branch within https://git.torproject.org/user/brade/tor-launcher.git. But you may also want the patch from #19733 (moved) (it is not needed if the local Tor check is disabled within Torbutton).
But it probably makes sense to wait until after these changes are merged into master so that you can use a nightly Tor Browser build.
-
Here are some comments:
+// given sockFile or host and port.s/sockFile/socketFile/
let tlps = Cc["@torproject.org/torlauncher-protocol-service;1"] .getService(Ci.nsISupports).wrappedJSObject;We should do that once and not twice in
torbutton_init().-
Checking whether we should call
torbutton_local_tor_check()should check form_tb_control_socket_fileas well (not only form_tb_control_port) I guess? -
I am not so sure about
+ } catch(e) { + m_tb_control_port = 9151; + }What was your rationale for adding that now (given you omitted it earlier)? For one, the logs might be misleading showing a probably wrong port (I mean the setup is seriously troubling if we need to assign
9151in the catch clause at all) in an error with respect to the control connection. On the other hand, we might want to show that something is wrong with the help oftorbutton_do_tor_check()which would update the toolbar button in this case (if we get that far at all with a broken setup like the one in question).Trac:
Keywords: TorBrowserTeam201608R deleted, TorBrowserTeam201609 added
Status: needs_review to needs_revision -
Replying to gk:
Here are some comments:
{{{ +// given sockFile or host and port. }}} s/sockFile/socketFile/
Good catch.
{{{ let tlps = Cc["@torproject.org/torlauncher-protocol-service;1"] .getService(Ci.nsISupports).wrappedJSObject; }}} We should do that once and not twice in
torbutton_init().Agreed.
- Checking whether we should call
torbutton_local_tor_check()should check form_tb_control_socket_fileas well (not only form_tb_control_port) I guess?
We did add that check, although if you look at the patch with git's default of three lines of context it is not so obvious.
- I am not so sure about {{{
-
} catch(e) { -
m_tb_control_port = 9151; -
}
}}} What was your rationale for adding that now (given you omitted it earlier)? For one, the logs might be misleading showing a probably wrong port (I mean the setup is seriously troubling if we need to assign
9151in the catch clause at all) in an error with respect to the control connection. On the other hand, we might want to show that something is wrong with the help oftorbutton_do_tor_check()which would update the toolbar button in this case (if we get that far at all with a broken setup like the one in question).Kathy and I added the 9151 default to be consistent with how m_tb_control_host is handled (it was already defaulting to 127.0.0.1). Thinking about this some more and looking at the existing Torbutton code, it seems like there is some effort to disable features (e.g., New Identity, the local Tor check) when the port is missing. So maybe we should put things back how there were and make sure we consistently check for port, password, etc. before trying to do things in Torbutton that require authenticated control port access?
The circuit display code also includes code that defaults the port to 9151, so if we decide to continue with the concept that a lack of port can be used to disable code, we should remove the
|| 9151from this line in tor-circuit-display.js:myController = controller(socketFile, host, port || 9151, password,We can also add a check to skip the call to createTorCircuitDisplay() if port, password, etc. are missing (the existing code will log an error if initialization fails).What do you think?
Trac:
Status: needs_revision to needs_information -
Replying to mcs:
Replying to gk:
- Checking whether we should call
torbutton_local_tor_check()should check form_tb_control_socket_fileas well (not only form_tb_control_port) I guess?
We did add that check, although if you look at the patch with git's default of three lines of context it is not so obvious.
Hm. No, it is even obvious then. Must have been a different kind of code-blindness, my bad.
- I am not so sure about {{{
-
} catch(e) { -
m_tb_control_port = 9151; -
}
}}} What was your rationale for adding that now (given you omitted it earlier)? For one, the logs might be misleading showing a probably wrong port (I mean the setup is seriously troubling if we need to assign
9151in the catch clause at all) in an error with respect to the control connection. On the other hand, we might want to show that something is wrong with the help oftorbutton_do_tor_check()which would update the toolbar button in this case (if we get that far at all with a broken setup like the one in question).Kathy and I added the 9151 default to be consistent with how m_tb_control_host is handled (it was already defaulting to 127.0.0.1). Thinking about this some more and looking at the existing Torbutton code, it seems like there is some effort to disable features (e.g., New Identity, the local Tor check) when the port is missing. So maybe we should put things back how there were and make sure we consistently check for port, password, etc. before trying to do things in Torbutton that require authenticated control port access?
The circuit display code also includes code that defaults the port to 9151, so if we decide to continue with the concept that a lack of port can be used to disable code, we should remove the
|| 9151from this line in tor-circuit-display.js:myController = controller(socketFile, host, port || 9151, password,We can also add a check to skip the call to createTorCircuitDisplay() if port, password, etc. are missing (the existing code will log an error if initialization fails).What do you think?
I think we should not set
m_tb_control_portto9151now in the catch clause and open a new ticket for implementing a saner solution across all Torbutton code. We can discuss there what we want that solution to be.Trac:
Status: needs_information to assigned - Checking whether we should call
-
Replying to gk:
I think we should not set
m_tb_control_portto9151now in the catch clause and open a new ticket for implementing a saner solution across all Torbutton code. We can discuss there what we want that solution to be.That sounds like a good plan. I opened #20057 (moved). Here is a revised patch for this ticket: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-02&id=abb01118a8445c541bb359d85fe9af941536b43a
Trac:
Status: assigned to needs_review
Keywords: TorBrowserTeam201609 deleted, TorBrowserTeam201609R added Replying to mcs:
Replying to gk:
I think we should not set
m_tb_control_portto9151now in the catch clause and open a new ticket for implementing a saner solution across all Torbutton code. We can discuss there what we want that solution to be.That sounds like a good plan. I opened #20057 (moved). Here is a revised patch for this ticket: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-02&id=abb01118a8445c541bb359d85fe9af941536b43a
Sorry for the late review. This patch looks good to me -- nice work! I have a couple of minor stylistic suggestions that you may or may not want to adopt:
+// given socketFile or host and port. +io.asyncSocketStreams = function (socketFile, host, port) { + let socketTransport; + let sts = Cc["@mozilla.org/network/socket-transport-service;1"] + .getService(Components.interfaces.nsISocketTransportService), + UNBUFFERED = Ci.nsITransport.OPEN_UNBUFFERED; + + // Create an instance of a socket transport. + if (socketFile) { + socketTransport = sts.createUnixDomainTransport(socketFile); + } else { + socketTransport = sts.createTransport(null, 0, host, port, null); + } +Maybe move
let socketTransportdown to after the comment// Create an instance of a socket transport? Also, in that file, I had used blank lines to separate functions, though that's probably a personal eccentricity...Also, a somewhat bigger suggestion that could be part of this patch or left for another time. In
torbutton.js, we will now havevar m_tb_control_socket_file = null; // Set if using a UNIX domain socket. var m_tb_control_port = null; // Set if not using a socket. var m_tb_control_host = null; // Set if not using a socket. var m_tb_control_pass = null; var m_tb_control_desc = null;I imagine it might be cleaner to collect these into a single data object like
var m_tb_control = { socket_file, host, port, password, descriptor }Then we could factor out a single factory function from
torbutton_init()that generates this object. And we could use this object as a single argument for functions intor-control-port.jsandtor-circuit-display.jsas well.-
Replying to arthuredelstein:
Sorry for the late review. This patch looks good to me -- nice work! I have a couple of minor stylistic suggestions that you may or may not want to adopt: {{{ +// given socketFile or host and port. +io.asyncSocketStreams = function (socketFile, host, port) {
- let socketTransport;
- let sts = Cc["@mozilla.org/network/socket-transport-service;1"]
-
.getService(Components.interfaces.nsISocketTransportService), - UNBUFFERED = Ci.nsITransport.OPEN_UNBUFFERED;
- // Create an instance of a socket transport.
- if (socketFile) {
- socketTransport = sts.createUnixDomainTransport(socketFile);
- } else {
- socketTransport = sts.createTransport(null, 0, host, port, null);
- }
}}} Maybe move
let socketTransportdown to after the comment// Create an instance of a socket transport?Good idea.
Also, in that file, I had used blank lines to separate functions, though that's probably a personal eccentricity...
I am OK with accommodating eccentricities, but can you explain what we need to fix? I don't think we removed any blank lines, but maybe I am just not seeing it.
Also, a somewhat bigger suggestion that could be part of this patch or left for another time. In
torbutton.js, we will now have {{{ var m_tb_control_socket_file = null; // Set if using a UNIX domain socket. var m_tb_control_port = null; // Set if not using a socket. var m_tb_control_host = null; // Set if not using a socket. var m_tb_control_pass = null; var m_tb_control_desc = null;
}}} I imagine it might be cleaner to collect these into a single data object like {{{ var m_tb_control = { socket_file, host, port, password, descriptor } }}} Then we could factor out a single factory function fromtorbutton_init()that generates this object. And we could use this object as a single argument for functions intor-control-port.jsandtor-circuit-display.jsas well.Kathy and I like that suggestion, but let's do it as a follow up. I created #20102 (moved) for it.
Trac:
Status: needs_review to needs_information
Keywords: TorBrowserTeam201609R deleted, TorBrowserTeam201609 added -
#3967 (closed) was resolved as a duplicate.
mentioned in issue #19459 (moved)
mentioned in issue #20075 (moved)