Tor profile service #357

darkdh · 2018-08-24T06:36:54Z

fix brave/brave-browser#361
fix brave/brave-browser#360
fix brave/brave-browser#376

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

Manual test
1. Open Tor window and check tor connectivity by going to https://check.torproject.org/
2. Use fresh profile and "Open Tor Window" will show up only when tor executable is downloaded (see cldoidikboihgcjfkhdeidbpclkineef for mac ,
  cpoalefficncklhjfpglfiplenlpccdb for Windows and
  biahpgbdmdkfgndcmfiipgcebobojjkp for Linux, in user dir)
3. When https://check.torproject.org/ shows you are connected to tor network, click "New Tor Identity" in app menu and page will refresh and shows different ip address
4. Quit Brave and do ps aux |grep <hash based on OS>, there shouldn't be any tor
  process left
5. Shield should work in both tor window and guest window
Unit test
1. BraveProfileManager
2. BraveTorNetworkDelegateHelper

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

darkdh · 2018-09-11T06:56:39Z

C70 rebased

bbondy · 2018-09-11T14:10:15Z

browser/tor/tor_profile_service_factory.cc

+tor::TorProfileService* TorProfileServiceFactory::GetForProfile(
+    Profile* profile) {
+  if (profile->IsOffTheRecord())
+    return NULL;


nit: prefer nullptr since that's the standard.

Brians-iMac-Pro-2:src brianbondy$ git grep return\ NULL | wc -l
3843
Brians-iMac-Pro-2:src brianbondy$ git grep return\ nullptr | wc -l
12074

bbondy · 2018-09-11T14:20:40Z

browser/tor/BUILD.gn

+    "//components/keyed_service/content",
+    "//components/keyed_service/core",
+    # for profile.h
+    "//components/domain_reliability",


I think alphabetical is preferred?

diracdeltas · 2018-09-11T16:45:50Z

if i understand correctly, this doesn't actually provide a Tor binary yet? FYI we will be updating to Tor 0.3.4 soon so i recommend testing with that: brave/tor_build_scripts#23

bbondy · 2018-09-11T16:46:31Z

Tor binary is already provided as a crx installation, works the same way extension install works.

bbondy · 2018-09-11T16:47:00Z

See Tor scripts here:
https://github.com/brave/brave-core-crx-packager/tree/master/scripts

darkdh · 2018-09-11T16:47:37Z

And we access it through TorClientUpdater #316

bbondy

Nothing major, awesome work!

bbondy · 2018-09-11T15:10:00Z

browser/tor/tor_proxy_config_service.cc

+void TorProxyConfigService::TorSetProxy(
+    net::ProxyResolutionService* service,
+    const std::string& tor_proxy,
+    const std::string& site_url,


Since we bind these parameters, this will cause various crashes. These must be std::string and not a const std::string&

bbondy · 2018-09-11T15:26:38Z

browser/tor/tor_proxy_config_service.cc

+      if (url.scheme.is_valid()) {
+        scheme_ =
+          std::string(tor_proxy.begin() + url.scheme.begin,
+                      tor_proxy.begin() + url.scheme.begin + url.scheme.len);


Can all of these be expressed more simply like this?
scheme_ = tor_proxy.substr(url.scheme.begin, url.scheme.len);

bbondy · 2018-09-11T15:31:46Z

browser/tor/tor_proxy_config_service.cc

+                      tor_proxy.begin() + url.port.begin + url.port.len);
+      }
+      std::string proxy_url;
+      if (tor_proxy_map && !username.empty()) {


What is the right thing to do if host_ is not filled or port_ is not filled?

it actually doesn't matter here because the config will remain empty but I will still do a early return when scheme_ or host_ or port_ is empty.

bbondy · 2018-09-11T15:35:53Z

browser/tor/tor_proxy_config_service.cc

+  // map, the old entry in the queue will cease to affect it because
+  // the timestamps won't match, and they will simultaneously create a
+  // new entry in the queue.
+  map_.erase(username);


Should we wrap this to check if it exists in the map first? I think it will cause a crash if it doesn't if there was no Get called.

I don't get it, if you are able to call TorProxyMap::Erase that means you have TorProxyMap instance which always has a std::map<std::string, std::pair<std::string, base::Time> > in it. Even if you specify non existent key, the erase operation just return 0. There shouldn't be any crashes.

bbondy · 2018-09-11T15:39:01Z

browser/tor/tor_proxy_config_service.h

+  static void TorSetProxy(
+    net::ProxyResolutionService* service,
+    const std::string& tor_proxy,
+    const std::string& site_url,


Update to std::string.

bbondy · 2018-09-11T19:07:46Z

patches/chrome-browser-ui-views-profiles-avatar_toolbar_button.cc.patch

+diff --git a/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc b/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
+index a053f001493dc3815ee9bb0a6d4526bfe65718dd..348a783485741370796073076cbbf78666911665 100644
+--- a/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc
+++ b/chrome/browser/ui/views/profiles/avatar_toolbar_button.cc


I prefer a subclass for the tooltip patch since I envision multiple different types of things here in the future.

bbondy · 2018-09-11T19:16:22Z

patches/services-service_manager-sandbox-win-sandbox_win.cc.patch

+           service_manager::switches::kNoSandbox)) {
+     base::LaunchOptions options;
+     options.handles_to_inherit = handles_to_inherit;
+    options.start_hidden = true;


are there any other process types started here which we don't want hidden?

for now, we only want to hide utility process type for audio service and tor launcher service specifically

do you know for sure nothing else uses this code path though?

seems like all child process on Window will enter this function but only non sandboxed child process would hit this block

and in that case, you will see the popup terminal window so that would be only audio service and tor launcher service for now

we can also enable service_manager::features::kAudioServiceSandbox and I can add this specifically for tor launcher service

Audio Service part is updated in the issue.
brave/brave-browser#714 (comment)

bbondy · 2018-09-11T19:22:25Z

patches/chrome-browser-extensions-chrome_process_manager_delegate.cc.patch

+   // artificial and background page shouldn't be created in it.
+   // http://crbug.com/329498
+-  return is_normal_session || profile->IsOffTheRecord();
+  return is_normal_session || profile->IsOffTheRecord() || profile->IsTorProfile();


Can we allow it for guest profiles here too? Or is there some problem why it's disallowed?

I think we can add this to fixed list if so:
brave/brave-browser#376

bbondy · 2018-09-11T19:28:04Z

app/brave_generated_resources.grd

+      <message name="IDS_PROFILES_OPEN_TOR_PROFILE_BUTTON" desc="Button in the avatar menu bubble view to open a tor window.">
+        Open Tor Window
+      </message>
+      <message name="IDS_PROFILES_TOR_PROFILE_NAME" desc="Name of the tor profile.">


nit: I know it's only a description, but capitalization might help a localizer to know it's a name.

bbondy · 2018-09-11T19:28:55Z

browser/tor/tor_profile_service_impl.cc

-void TorProfileServiceImpl::SetNewTorCircuit(const GURL& request_url) {}
+void TorProfileServiceImpl::SetNewTorCircuitOnIOThread(
+    const scoped_refptr<net::URLRequestContextGetter>& getter,
+    const std::string& host) {


Use std::string host here or it can introduce crashes.

bbondy · 2018-09-12T01:02:06Z

patches/chrome-test-base-chrome_unit_test_suite.cc.patch

+   // This needs to run after ChromeTestSuite::Initialize which calls content's
+   // intialization which calls base's which initializes ICU.
+   InitializeResourceBundle();
+  brave::InitializeResourceBundle();


Pls move to brave_unit_test_suite.cc after the Initialize call.

diracdeltas · 2018-09-12T22:09:06Z

defer my review to @riastradh-brave

diracdeltas · 2018-09-12T22:12:21Z

doesn't block but i opened brave/brave-core-crx-packager#24 for updating the tor version

darkdh · 2018-09-13T02:27:27Z

build and browser/unit tests pass on all platforms. Also manually verify tor function works

bbondy · 2018-09-13T02:33:03Z

chromium_src/services/service_manager/sandbox/win/sandbox_win.cc

+namespace {
+void BraveLaunchOption(base::CommandLine* cmd_line,
+                       base::LaunchOptions *options) {
+  // tor::swtiches::kTorExecutablePath


so why not just use tor::swtiches::kTorExecutablePath like the comment says?

that will have to patch the BUILD.gn and also leads to circular deps.
Probably can do when we migrate to components/tor

bbondy

incremental commits lgtm

on MacOS

Note: brave::InitializeResourceBundle cannot be called per unit test suites, it can only be called each unittest run or it will lead to duplicate resources

1. const std::string => std::string in bind function 2. Tor resource strings descrition 3. Use substr(...) 4. Early return when scheme_, host_ or port_ is empty 5. nits

to prevent it from generating new terminal window on Windows

riastradh-brave

LGTM, sorry this took so long! A lot of the logic is essentially the same as in muon. The organization into keyed services is cleaner, as we discussed with brianjohnson.

riastradh-brave · 2018-09-14T06:55:22Z

utility/tor/tor_launcher_impl.cc

+    args.AppendArg("--cookieauthentication");
+    args.AppendArg("1");
+    args.AppendArg("--cookieauthfile");
+    args.AppendArgPath(tor_watch_path.AppendASCII("control_auth_cookie"));


These are all basically the same as in muon, looks fine to me.

bbondy · 2018-09-14T15:07:31Z

🎉 👍 💯

Tor profile service

bbondy · 2018-09-14T15:13:15Z

0.55.x 0e45986
master 84a4955

bsclifton · 2018-09-14T16:21:29Z

😻

- interface follow-up of #357

Bump patch version for new internal build

darkdh added the pr/work-in-progress label Aug 24, 2018

darkdh self-assigned this Aug 24, 2018

bbondy changed the title ~~[WIP] Tor profile service~~ WIP: Tor profile service Sep 3, 2018

darkdh force-pushed the tor_profile_service branch 4 times, most recently from c43af3c to 187bcbc Compare September 11, 2018 06:56

darkdh changed the title ~~WIP: Tor profile service~~ Tor profile service Sep 11, 2018

darkdh requested a review from bbondy September 11, 2018 06:56

bbondy removed the pr/work-in-progress label Sep 11, 2018

bbondy requested review from diracdeltas and riastradh-brave September 11, 2018 13:49

bbondy reviewed Sep 11, 2018

View reviewed changes

bbondy requested changes Sep 11, 2018

View reviewed changes

darkdh force-pushed the tor_profile_service branch from 0d0dcb7 to 30abf5c Compare September 12, 2018 00:14

bbondy reviewed Sep 12, 2018

View reviewed changes

darkdh force-pushed the tor_profile_service branch 2 times, most recently from 13c5cb5 to 243c5f3 Compare September 12, 2018 04:58

darkdh force-pushed the tor_profile_service branch from fd1613b to 3e43852 Compare September 12, 2018 22:10

bbondy reviewed Sep 13, 2018

View reviewed changes

bbondy previously approved these changes Sep 13, 2018

View reviewed changes

darkdh added 11 commits September 13, 2018 10:50

Tor process cleanup in SuicideOnChannelErrorFilter when browser crashes

0c1a3b6

on MacOS

Add tor_network_delegate_helper unittest

d26237c

hide annoying windows of utility services on Windows

1669a56

Disable tor window menu item until tor executable is downloaded

dd4ce2f

C70 rebase changes

87a27cf

Add BraveProfileManager unittest

00d3025

Note: brave::InitializeResourceBundle cannot be called per unit test suites, it can only be called each unittest run or it will lead to duplicate resources

Address review comments:

951cb45

1. const std::string => std::string in bind function 2. Tor resource strings descrition 3. Use substr(...) 4. Early return when scheme_, host_ or port_ is empty 5. nits

Add LaunchOption.start_hidden for tor launcher service

5592af7

to prevent it from generating new terminal window on Windows

Allow background pages for guest profile including tor profile

bb1f66b

Overrides GetAvatarNameForProfile and GetAvatarButtonTextForProfile

3986508

Subclass ProfileChooserView

e81c259

darkdh force-pushed the tor_profile_service branch 2 times, most recently from 5735691 to e81c259 Compare September 13, 2018 19:42

riastradh-brave approved these changes Sep 14, 2018

View reviewed changes

bbondy approved these changes Sep 14, 2018

View reviewed changes

darkdh merged commit 84a4955 into master Sep 14, 2018

bbondy pushed a commit that referenced this pull request Sep 14, 2018

Merge pull request #357 from brave/tor_profile_service

0e45986

Tor profile service

bbondy added the merged/0.55.x label Sep 14, 2018

bsclifton deleted the tor_profile_service branch September 14, 2018 16:21

cezaraugusto mentioned this pull request Sep 14, 2018

new private tab with tor option #453

Merged

cezaraugusto added a commit that referenced this pull request Sep 15, 2018

private new tab ui with tor option

4906a8f

- interface follow-up of #357

riastradh-brave mentioned this pull request Sep 24, 2018

Adapt tor browser context from muon to brave-core brave/brave-browser#361

Closed

bbondy added this to the 0.55.x - Release milestone Jan 14, 2019

fmarier pushed a commit that referenced this pull request Oct 29, 2019

Merge pull request #357 from RyanJarv/master

69885b8

Bump patch version for new internal build

Tor profile service #357

Tor profile service #357

Conversation

darkdh commented Aug 24, 2018 • edited by srirambv Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

darkdh commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Sep 11, 2018

bbondy commented Sep 11, 2018

bbondy commented Sep 11, 2018

darkdh commented Sep 11, 2018

bbondy left a 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

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

Choose a reason for hiding this comment

diracdeltas commented Sep 12, 2018

diracdeltas commented Sep 12, 2018

darkdh commented Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy left a comment

Choose a reason for hiding this comment

riastradh-brave left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy commented Sep 14, 2018

bbondy commented Sep 14, 2018

bsclifton commented Sep 14, 2018

darkdh commented Aug 24, 2018 •

edited by srirambv

Loading

darkdh commented Sep 13, 2018 •

edited

Loading

riastradh-brave left a comment •

edited

Loading