Port SOCKS5 authentication from muon. #290

riastradh-brave · 2018-07-27T01:30:37Z

This ports the patches, makes them work, and adds unit tests, in separate commits. (If the unit tests should go again, they can be removed -- much easier this time since all the patches are in different files.)

fix brave/brave-browser#358

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.

Test Plan:

After a build, say a Debug build:

Build and run the unit tests:
1. ninja -C src/out/Debug net:net_unittests
2. ./src/out/Debug/net_unittests --gtest_filter='ProxyServerTest.*:SOCKS5ClientSocketTest.*'
Manually confirm the behaviour with Tor:
1. Launch a tor process listening on (say) 9250, e.g. by starting browser-laptop and opening a private tab with Tor.
2. Launch two independent Brave instances with different proxy server usernames and passwords, e.g.:
  - ./src/out/Debug/brave --user-data-dir=/tmp/test1 --proxy-server=socks5://foo:bar@127.0.0.1:9250 &
  - ./src/out/Debug/brave --user-data-dir=/tmp/test2 --proxy-server=socks5://baz:quux@127.0.0.1:9250 &
3. Load https://check.torproject.org in the two browsers and confirm that they have different IP addresses.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

bridiver · 2018-07-27T02:42:08Z

patches/net-base-host_port_pair.cc.patch

+ namespace net {
+
+ HostPortPair::HostPortPair() : port_(0) {}
+#if 0                           // brave override


these should use BRAVE_CHROMIUM_BUILD

5e8611a27cd370eb31c4b0e911a38e9fc0341c9f

bridiver · 2018-07-27T02:49:10Z

chromium_src/net/socket/socks5_client_socket_auth.cc

+    std::unique_ptr<ClientSocketHandle> transport_socket,
+    const HostResolver::RequestInfo& req_info,
+    const NetworkTrafficAnnotationTag& traffic_annotation,
+    const HostPortPair& proxy_host_port)


I'm sure we talked about doing a subclass/friend for HostPortPair, but I can't remember why we didn't. The only place we create the auth version is in our own code so it seems like it should work?

Would presumably require making several of HostPortPair's member functions virtual. Not sure what the tradeoff is of the extra members vs making those functions virtual is. Originally I didn't touch HostPortPair at all, but rather put it into ProxyServer.

bridiver · 2018-07-27T02:50:05Z

patches/net-BUILD.gn.patch

+     "ssl/token_binding.h",
+     "third_party/quic/core/quic_error_codes.cc",
+     "third_party/quic/core/quic_error_codes.h",
+    "//brave/chromium_src/net/base/host_port_pair_auth.cc",


we can actually add these using a patch on the cc files. For instance: create a chromium_src patch for host_port_part.cc, include the original host_port_part.cc file and then include host_port_pair_auth.cc

cb908db81cf50cf6eafbdbed8ffe7cd683188010

bridiver · 2018-07-27T02:51:13Z

patches/net-BUILD.gn.patch

+     "third_party/quic/core/quic_error_codes.cc",
+     "third_party/quic/core/quic_error_codes.h",
+    "//brave/chromium_src/net/base/host_port_pair_auth.cc",
+    "//brave/chromium_src/net/base/url_auth_util.cc",


same thing applies to these

614d17f6149feded4eb20f9049a9050ba1a5cfd2
a9a045e0dd404ef76cc00a727f41118cc4ecc17d

riastradh-brave · 2018-07-27T22:07:53Z

I eliminated the BUILD.gn and net_log_event_type_list.h patches using overrides.

I could go a little further with overrides, but in some cases I'm reluctant to do that because if the code I'm overriding has changed enough, I want there to be a merge conflict that requires human attention.

I could replace the definition of ParseHostAndPort, but there are various callers beyond proxy_server.cc. I think it is safer to define a new function with the semantics we need there and patch proxy_server.cc to use it -- again, if the caller in that .cc file changes, it probably deserves human attention.

I could reduce the diff in host_port_pair.h a tiny bit by using a subclass for the version with an authority, but that would require making a number of member functions virtual, including several that are written in-line in host_port_pair.h, which strikes me as a net negative.

bridiver · 2018-07-30T20:05:09Z

I spent some time going through this with @riastradh-brave and unfortunately we couldn't use the preprocessor method to subclass HostPortPair because it gets passed by value (copy constructor)

- Work around Chromium's new in-line constructor size limits. - Get base::StringPiece forward declaration in url_auth_util.h. - Use `#if !defined(BRAVE_CHROMIUM_BUILD)', not #if 0. - Override and include, rather than patch, net_log_event_type_list.h. - Override and include host_port_pair.cc to reduce BUILD.gn diffs. - Override and include url_util.cc rather than add url_auth_util.cc. - Override and include socks5_client_socket.cc. - Don't patch HostPortPair(host, port) constructor. Instead, use a different constructor in proxy_server.cc. This adds another patch to our maintenance burden, but it avoids changing the semantics of (and adding parsing overhead to) the constructor that is used in a bazillion places all over Chromium.

Merge muon socks5 auth changes.

ec75f16

riastradh-brave self-assigned this Jul 27, 2018

riastradh-brave requested review from bridiver and darkdh July 27, 2018 01:30

bridiver reviewed Jul 27, 2018

View reviewed changes

riastradh-brave force-pushed the riastradh-socks5-auth branch from c4eb758 to 9d1c936 Compare July 30, 2018 19:51

bridiver previously approved these changes Jul 30, 2018

View reviewed changes

riastradh-brave dismissed bridiver’s stale review via c1da167 July 30, 2018 20:12

riastradh-brave force-pushed the riastradh-socks5-auth branch from 9d1c936 to c1da167 Compare July 30, 2018 20:12

bridiver approved these changes Jul 30, 2018

View reviewed changes

riastradh-brave merged commit 2248b86 into master Jul 30, 2018

bbondy mentioned this pull request Jul 31, 2018

Revert "Port SOCKS5 authentication from muon." #300

Merged

This was referenced Jul 31, 2018

Rework socks5 authentication. #302

Closed

Rework socks5 authentication. #311

Closed

riastradh-brave mentioned this pull request Aug 18, 2018

Implement SOCKS5 authentication. #336

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port SOCKS5 authentication from muon. #290

Port SOCKS5 authentication from muon. #290

riastradh-brave commented Jul 27, 2018 •

edited

Loading

bridiver Jul 27, 2018

riastradh-brave Jul 27, 2018

bridiver Jul 27, 2018

riastradh-brave Jul 27, 2018

bridiver Jul 27, 2018 •

edited

Loading

riastradh-brave Jul 27, 2018

bridiver Jul 27, 2018

riastradh-brave Jul 27, 2018

riastradh-brave commented Jul 27, 2018

bridiver commented Jul 30, 2018

Port SOCKS5 authentication from muon. #290

Port SOCKS5 authentication from muon. #290

Conversation

riastradh-brave commented Jul 27, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

bridiver Jul 27, 2018

Choose a reason for hiding this comment

riastradh-brave Jul 27, 2018

Choose a reason for hiding this comment

bridiver Jul 27, 2018

Choose a reason for hiding this comment

riastradh-brave Jul 27, 2018

Choose a reason for hiding this comment

bridiver Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

riastradh-brave Jul 27, 2018

Choose a reason for hiding this comment

bridiver Jul 27, 2018

Choose a reason for hiding this comment

riastradh-brave Jul 27, 2018

Choose a reason for hiding this comment

riastradh-brave commented Jul 27, 2018

bridiver commented Jul 30, 2018

riastradh-brave commented Jul 27, 2018 •

edited

Loading

bridiver Jul 27, 2018 •

edited

Loading