-
Notifications
You must be signed in to change notification settings - Fork 868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NetworkService migration for Tor #2647
Conversation
cb2c3d2
to
b3c9760
Compare
071b483
to
a473ce4
Compare
994a787
to
1fee089
Compare
const int kTorPasswordLength = 16; | ||
// Default tor circuit life time is 10 minutes | ||
constexpr base::TimeDelta kTenMins = base::TimeDelta::FromMinutes(10); | ||
constexpr base::TimeDelta kTenMins = base::TimeDelta::FromMinutes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like ten minutes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I was doing some testing and forgot to change back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
#include "net/proxy_resolution/proxy_resolution_service.h" | ||
#include "net/url_request/url_request_context.h" | ||
#include "net/proxy_resolution/proxy_config_with_annotation.h" | ||
#include "net/base/registry_controlled_domains/registry_controlled_domain.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong sorting (just apply clang-format
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 4985fce
proxy_url = std::string(scheme_ + "://" + host_ + ":" + port_); | ||
} | ||
std::string proxy_url = | ||
std::string(scheme_ + "://" + host_ + ":" + port_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace could be fixed by clang-format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use const std::string
and generally const
as much as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 4985fce
TorProxyConfigService::TorProxyConfigService( | ||
const std::string& tor_proxy, const std::string& username, | ||
TorProxyMap* tor_proxy_map) { | ||
ProxyConfigServiceTor::ProxyConfigServiceTor(const std::string& tor_proxy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we receive a string here instead of getting net::ProxyServer
? We could avoid all that parsing and use well-formed data structures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 51b3f3c
browser/ui/browser_commands.cc
Outdated
} // namespace | ||
|
||
namespace brave { | ||
|
||
void NewTorIdentityCallback(WebContents* current_tab, bool success) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why success
is unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 51b3f3c
case version_info::Channel::DEV: | ||
return std::string("socks5://127.0.0.1:9370"); | ||
case version_info::Channel::CANARY: | ||
return std::string("socks5://127.0.0.1:9380"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it possible to return GURL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is an uri not url, see ProxyServer::FromURI
and ProxyServer::ToURI
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, socks5
is not a valid scheme for GURL afaik
return std::make_unique<net::ProxyConfigServiceTor>( | ||
tor::GetTorProxyString()); | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#endif // BUILDFLAG ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 51b3f3c
GURL url = request_url.ReplaceComponents(replacements); | ||
tor_circuit_callback_ = std::move(callback); | ||
|
||
DCHECK_CURRENTLY_ON(content::BrowserThread::UI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typically such checks are placed in the very beginning of a method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 51b3f3c
base::Unretained(this), net::ERR_ABORTED, base::nullopt)); | ||
// Force lookup to erase the old circuit | ||
storage_partition->GetNetworkContext()-> | ||
LookUpProxyForURL(url, std::move(proxy_lookup_client_ptr)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 4985fce
generally looks good, but I will take more time to understand details. |
1fee089
to
d62bea4
Compare
if (current_state_ == STATE_NONE) | ||
ApplyProxyConfigIfAvailable(); | ||
|
||
+ if (IsTorProxy(config_->value()) && raw_url.ref() == "NewTorCircuit") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should hide this under ENABLE_TOR
, though not sure if it's possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to make //net not to rely on any brave stuff to avoid circular dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update patches similar to discussions about sync patches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I'd really like to find some way to not have tor-specific stuff in ProxyResolutionService, in particular the tor_proxy_map_. Can we create a class a singleton class in Brave that keeps a map of ProxyResolutionService
instance -> tor_proxy_map
and use that inside a chromium_src injected method/macro?
}; | ||
|
||
TEST_F(ProxyResolutionServiceTest, TorProxy) { | ||
MockAsyncProxyResolverFactory* factory = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: make_unique
here and std::move
below
const everywhere for local variables that do not change (proxy_uri
, site_url
, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MockAsyncProxyResolverFactory will be used later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't actually care about factory->pending_requests().empty()
so I will wrap it when constructing ProxyResolutionService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 58622cb
MockAsyncProxyResolverFactory* factory = | ||
new MockAsyncProxyResolverFactory(false); | ||
std::string proxy_uri("socks5://127.0.0.1:5566"); | ||
ProxyResolutionService service( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can avoid copypaste by initializing this stuff in the test class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 58622cb
ProxyConfigServiceTor tor_proxy_config_service(proxy_uri); | ||
ProxyConfigWithAnnotation fetched_config; | ||
tor_proxy_config_service.GetLatestProxyConfig(&fetched_config); | ||
tor_proxy_config_service.SetUsername( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments describing the key point would be highly appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 58622cb
bool success = proxy_info.has_value() && !proxy_info->is_direct(); | ||
if (tor_circuit_callback_) { | ||
std::move(tor_circuit_callback_).Run(success); | ||
binding_.Close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we close the binding in any case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 58622cb
&tor_proxy_map_, | ||
true); | ||
void TorProfileServiceImpl::OnSetNewTorCircuitComplete(bool success) { | ||
if (tor_circuit_callback_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in what case this is empty? tests? Probably worth adding a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is not used, removed in 58622cb
common/tor/tor_proxy_uri_helper.cc
Outdated
std::string GetTorProxyURI() { | ||
switch (chrome::GetChannel()) { | ||
case version_info::Channel::STABLE: | ||
return std::string("socks5://127.0.0.1:9350"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not introduce a named constant for 127.0.0.1
and maybe ports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 58622cb
EXPECT_EQ(host_port.port(), 5566); | ||
EXPECT_EQ(host_port.username(), isolation_key); | ||
|
||
// persistent circuit isolation until timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit for all comments throughout the PR: normally the should look like a sentence, i.e. start with a Capital Letter and end with a dot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 58622cb
LGTM with nits. My vote for merging this ASAP :) @bridiver @bsclifton @riastradh-brave WDYT? |
9256613
to
5eeba9f
Compare
@@ -12,13 +12,15 @@ | |||
#include "base/files/scoped_temp_dir.h" | |||
#include "base/strings/utf_string_conversions.h" | |||
#include "brave/browser/profiles/tor_unittest_profile_manager.h" | |||
#include "brave/browser/tor/tor_launcher_factory.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably keep all the tor stuff here inside ENABLE_TOR
since we don't currently have an implementation for android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same also applies for the tor-specific test files in gn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 03268dc482e62d0ff2c0573fb193528ca1508a28
@@ -33,3 +35,6 @@ std::unique_ptr<net::ProxyConfigService> CreateProxyConfigServiceTor( | |||
#endif | |||
|
|||
#include "../../../../../chrome/browser/net/proxy_config_monitor.cc" // NOLINT | |||
|
|||
// Required by lint | |||
#include "brave/chromium_src/chrome/browser/net/proxy_config_monitor.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go at the top, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we actually disable this lint check for the chromium_src dir in chromium_src/CPPLINT.cfg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 3bc4178
browser/tor/tor_profile_service.cc
Outdated
@@ -18,6 +16,11 @@ | |||
#include "components/prefs/pref_registry_simple.h" | |||
#include "components/prefs/pref_service.h" | |||
|
|||
#if BUILDFLAG(ENABLE_TOR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the entire TorProfileService be left out of gn if tor is not enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 9bb8d68
@@ -123,10 +124,12 @@ TorProfileServiceImpl::TorProfileServiceImpl(Profile* profile) | |||
tor_launcher_factory_ = TorLauncherFactory::GetInstance(); | |||
tor_launcher_factory_->AddObserver(this); | |||
|
|||
#if BUILDFLAG(ENABLE_TOR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TorProfileServiceImpl should definitely not be built at all if tor is not enabled in gn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 9bb8d68
d6fb3aa
to
48d0487
Compare
48d0487
to
08e5837
Compare
@darkdh this is marked as 0.68.x milestone, but I only see it in master (0.69.x). Are we planning to uplift to 0.68.x and 0.67.x? c76 bump is going to those branches and would need these changes to be there as well, I think. |
I must marked the milestone too early and train migration happened (it was meant to be nightly). |
NetworkService migration for Tor
NetworkService migration for Tor
brave-browser: a0b6c4d (75.0.3770.90)
fix brave/brave-browser#4312
When NetworkService is enabled, we only have access to profile-wise
proxy config. So we discard proxy config per url request path to have a
global proxy config for tor profile and insert username/password when it
is about to resolve proxy in net::ProxyResolutionService.
Note
dependency.
proxy config without setting proxy_config::prefs::kProxy which will be
interfered by other components(ex. DataReductionProxyService)
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Launch Brave with/without
--enable-features=NetworkService
and test following scenario separatelyNew Tor Identity
, it will trigger a auto refresh and ip should be different than step 2Reviewer Checklist:
After-merge Checklist:
changes has landed on.