Tor control channel #4802

riastradh-brave · 2020-03-02T17:17:57Z

Resolves brave/brave-browser#359
Resolves brave/brave-browser#649
Resolves brave/brave-browser#1113

Tor control channel spec:
https://gitweb.torproject.org/torspec.git/plain/control-spec.txt

This PR implements access to the Tor control socket, which lets us monitor the status of the tor daemon and control it without restarting it. This is a necessary step for brave/brave-browser#649, as well as getting the progress bar that browser-laptop used to have, along with other amenities like displaying where the exit nodes you're using are.
Also it improves tor crashes or tor launcher crashes with cleanup method before launch.

Work to do before merge:

Finish management of event subscriptions.
Monitor bootstrap progress, network liveness, and circuit establishment events.
Get the SOCKS proxy port from the tor daemon.
Work out how to change the SOCKS proxy configuration driven by what the tor daemon says, rather than choosing it up front and telling both the profile and the daemon what to use.

Follow-up issues:

More tests for these new codes Add more tests for TorLauncherFactory and TorControl brave-browser#11759
Switch to a local socket (file system namespace) instead of an IP loopback socket (127.0.0.1 namespace) as in Use a local socket for tor socks proxy and control channel where available brave-browser#650.
Incorporate output from GETINFO VERSION somewhere, perhaps in brave://version or in brave://tor as in Expose Tor log in brave://tor brave-browser#8106.

Submitter Checklist:

Test Plan:

OS-chosen port for tor

Open Tor window
Check tor listen port number
On Mac and Linux: lsof -nP -iTCP -sTCP:LISTEN
On Windows: netstat -ano -p tcp -b (requires admin privileges because of -b)
There are two port, one is control port (which should be same as user_dir/tor/watch/controlport) another is what we really care about. (should not be 9250, 9260, 9270, 9280, or 9290)
Close Tor window
Open Tor window
Check tor listen port again which should be different than step 2

Tor initialization progress indicator

Launch Brave with clean profile (so we can see longer init progress)
Open tor window and paste https://check.torproject.org/ immediately
The request shouldn't get through (tab keeps spinning)
There will tor initialization progress on NTP
When tor is connected to tor network, the previous request will continue and we should see connected to Tor network on https://check.torproject.org/

Tor process crash handling

Open Tor window
Kill tor progress (it looks like this, tor-VERSION-PLATFORM-brave-REVISION, eg tor-0.3.5.11-darwin-brave-0)
There will be a new tor process launched in 1s
Go back to tor window and go to https://check.torproject.org/, it should be connected to tor network

Tor launcher crash handling

Open Tor window
Go to https://check.torproject.org/
Open Brave's Task Manager and kill Tor Launcher

Go back to tor window and go to https://check.torproject.org/, it should be connected to tor network ((MacOS & Windows only, on Linux tor process will be killed when Tor Launcher dies)
Close Tor window and open a new tor window
Open Brave's Task Manager and check Tor Launcher is there with a new pid
Go to https://check.torproject.org/, it should be connected to tor network

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

bridiver · 2020-09-15T18:34:36Z

patches/sandbox-policy-win-sandbox_win.cc.patch

 --- a/sandbox/policy/win/sandbox_win.cc
 +++ b/sandbox/policy/win/sandbox_win.cc
 @@ -887,6 +887,7 @@ ResultCode SandboxWin::StartSandboxedProcess(
       launcher_process_command_line.HasSwitch(switches::kNoSandbox)) {
     base::LaunchOptions options;
     options.handles_to_inherit = handles_to_inherit;
-+    BraveLaunchOption(cmd_line, &options);
+    BraveLaunchOption(process_type, &options);


can we convert this to a define since you're changing it anyway?

sure, will do

bridiver · 2020-09-15T21:25:26Z

browser/tor/tor_control.cc

+#else
+  const char maxpid[] = "4194304";  // 0x400000
+#endif
+  const size_t kBufSiz = strlen(maxpid);


This doesn't look right, the pid string is not going to be 4294967292 characters long and this whole thing seems overly complicated. Why aren't we just reading the file as a string like normal?

That is strlen of it so it would be 10 characters long max on Windows and 7 characters long on POSIX

but yeah, will just read the file as a string

fixed in 12c003763561ef5dc1ef014d4769d24a84c58883

bridiver · 2020-09-15T21:31:02Z

browser/tor/tor_control.cc

+}
+
+// XXX DEBUG
+static std::string escapify(const char *buf, int len) {


this should be at the top in an anonymous namespace

fixed in 12c003763561ef5dc1ef014d4769d24a84c58883

bridiver · 2020-09-15T21:33:32Z

browser/tor/tor_launcher_factory.cc

+  KillTorProcess();
+  // Post delayed relaucn for control to stop
+  content::GetUIThreadTaskRunner({})
+    ->PostDelayedTask(FROM_HERE,


nit - 4 spaces for continuation of a line

fixed in 60f91be

Remove kTorProxyString without migration is becasue it was always default value

cause the cookies to be truncated if we store it as std::string

delegate

…ndency and it is fully encapsulated by TorLauncherFactory

…indow

… flow Also remove unused ReLaunch

1. Build failed 2. Not handling CRLF when reading port file 3. Not checking process validity before killing it Fix tor crash handler is not getting called

…nnected

…getting skipped

relaunch tor with 1s interval when tor crashes

…CHECK failure fix lint

and remove owner_task_runner to avoid confusion

darkdh force-pushed the riastradh-torcontrol-v2 branch from 88f5bdf to f0f8170 Compare July 30, 2020 21:35

darkdh self-assigned this Aug 6, 2020

darkdh force-pushed the riastradh-torcontrol-v2 branch 4 times, most recently from d7f8272 to 54647a3 Compare August 11, 2020 18:53

darkdh force-pushed the riastradh-torcontrol-v2 branch 6 times, most recently from 7a8c911 to 137c310 Compare August 26, 2020 19:41

darkdh force-pushed the riastradh-torcontrol-v2 branch 3 times, most recently from 72d7c33 to 7c2cfe7 Compare August 29, 2020 01:48

darkdh marked this pull request as ready for review August 31, 2020 17:13

darkdh requested a review from bridiver as a code owner August 31, 2020 17:13

darkdh requested a review from simonhong August 31, 2020 17:13

darkdh force-pushed the riastradh-torcontrol-v2 branch 2 times, most recently from af889e8 to 429c208 Compare August 31, 2020 17:18

darkdh changed the title ~~WIP: Tor control channel~~ Tor control channel Aug 31, 2020

darkdh force-pushed the riastradh-torcontrol-v2 branch 5 times, most recently from b4d9660 to 15d1a5e Compare September 12, 2020 04:58

bridiver reviewed Sep 15, 2020

View reviewed changes

darkdh added 23 commits September 17, 2020 15:02

Add GetSOCKSListeners and cleanup

a415a1f

Apply auto selected port from tor

44042fa

Remove kTorProxyString without migration is becasue it was always default value

Fix tor control sequence checker failure

40b96b4

Auth cookies might contains 0x00 in the middle of byte stream which will

f76bab7

cause the cookies to be truncated if we store it as std::string

Move TorControl from scoped_refptr to unique_ptr and observer_list to

a9ca802

delegate

Merge TorControl and TorControlImpl since we don't have to split depe…

f3e76ef

…ndency and it is fully encapsulated by TorLauncherFactory

Monitor tor bootstrap and fix event subscription after relaunch tor w…

f870d70

…indow

Cleanup old tor process which was not reaped by utility process

fe8c516

Use owner_task_runner for callbacks, delegate methods to simplify the…

d13ff83

… flow Also remove unused ReLaunch

Use WeakPtr

dae92ba

Fix windows issues:

3352cc9

1. Build failed 2. Not handling CRLF when reading port file 3. Not checking process validity before killing it Fix tor crash handler is not getting called

Prevent requests from getting through between tor launched and tor co…

0cb59cc

…nnected

Fix opening old control channel bug which causes event subscriptions …

28e011f

…getting skipped

Fix tor launcher hanging after tor crashes and

b746095

relaunch tor with 1s interval when tor crashes

Cleanup: lint, DCHECK, VLOG

f3d6aba

Add BraveOmniboxViewViewsTest.TorInitProgress and fix browser tests D…

c4cc2ee

…CHECK failure fix lint

Move TorControl under browser

058dd07

Add ReadLine test case

8554c22

Remove unused kTorExecutablePath

a1112da

Fix TorControl::Delegate callback lifetime issue

e65243b

and remove owner_task_runner to avoid confusion

Address review feedback

c667a2c

clang-format

03f860e

Showing tor status on NTP

81567f0

darkdh force-pushed the riastradh-torcontrol-v2 branch from 4f3a328 to 81567f0 Compare September 17, 2020 22:07

darkdh merged commit 3a0de8f into master Sep 17, 2020

darkdh deleted the riastradh-torcontrol-v2 branch September 17, 2020 23:55

darkdh added this to the 1.16.x - Nightly milestone Sep 17, 2020

This was referenced Oct 5, 2020

Adapt tor daemon controller logic from browser-laptop to brave-core brave/brave-browser#359

Closed

Use an OS-chosen port number for tor socks proxy brave/brave-browser#649

Closed

GeetaSarvadnya mentioned this pull request Oct 23, 2020

Tor process reaping on Windows brave/brave-browser#1113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tor control channel #4802

Tor control channel #4802

riastradh-brave commented Mar 2, 2020 •

edited by darkdh

Loading

bridiver Sep 15, 2020

darkdh Sep 15, 2020

darkdh Sep 16, 2020

bridiver Sep 15, 2020

darkdh Sep 15, 2020

darkdh Sep 15, 2020

darkdh Sep 16, 2020

bridiver Sep 15, 2020

darkdh Sep 16, 2020

bridiver Sep 15, 2020

darkdh Sep 16, 2020

Tor control channel #4802

Tor control channel #4802

Conversation

riastradh-brave commented Mar 2, 2020 • edited by darkdh Loading

Submitter Checklist:

Test Plan:

OS-chosen port for tor

Tor initialization progress indicator

Tor process crash handling

Tor launcher crash handling

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riastradh-brave commented Mar 2, 2020 •

edited by darkdh

Loading