Skip to content

Implement SOCKS5 authentication. #336

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

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Implement SOCKS5 authentication. #336

merged 2 commits into from
Aug 21, 2018

Conversation

riastradh-brave
Copy link
Contributor

@riastradh-brave riastradh-brave commented Aug 18, 2018

  • 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.

This is a replacement for #290, in which during squashing or something I lost two files (socks5_client_socket.cc/h) that were critical to the whole thing, and for #302, which was behind a Chromium update or something, and for #311, which had a larger set of patches than necessary.

fix brave/brave-browser#666

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:

On riastradh-socks5-auth-test-v3 (which only adds tests, doesn't touch anything used outside tests):

  • ninja -C src/out/Debug net:net_unittests
  • ./src/out/Debug/net_unittests --gtest_filter='ProxyServerTest.*:SOCKS5ClientSocketTest.*'

Reviewer Checklist:

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

- 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.
@darkdh
Copy link
Member

darkdh commented Aug 18, 2018

build pass on all platforms. However, ProxyServerTest.*:SOCKS5ClientSocketTest.* unit test only pass fro Mac and Linux
I saw these on Windows

5 tests crashed:
    SOCKS5ClientSocketTest.CompleteHandshake (../../net/socket/socks5_client_socket_unittest.cc:125)
    SOCKS5ClientSocketTest.ConnectAndDisconnectTwice (../../net/socket/socks5_client_socket_unittest.cc:196)
    SOCKS5ClientSocketTest.LargeHostNameFails (../../net/socket/socks5_client_socket_unittest.cc:232)
    SOCKS5ClientSocketTest.PartialReadWrites (../../net/socket/socks5_client_socket_unittest.cc:250)
    SOCKS5ClientSocketTest.Tag (../../net/socket/socks5_client_socket_unittest.cc:379)

Crash stacks are almost the same cause

Received fatal exception EXCEPTION_ACCESS_VIOLATION
Backtrace:
        std::char_traits<char>::assign [0x00007FFAD6EB48C8+24] (C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\iosfwd:517)
        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::assign [0x00007FFAD6EB4522+162] (C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\xstring:2441)
        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::operator= [0x00007FFAD6EB5EE0+144] (C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\xstring:2267)
        net::HostPortPair::operator= [0x00007FFAD6ECB51E+62] (C:\brave-browser\src\net\base\host_port_pair.h:21)
        net::ProxyServer::operator= [0x00007FFAD6FAE98C+60] (C:\brave-browser\src\net\base\proxy_server.h:25)
        net::HttpResponseInfo::operator= [0x00007FFAD6FAF8CF+111] (C:\brave-browser\src\net\http\http_response_info.cc:131)
        net::ClientSocketHandle::ResetErrorState [0x00007FFAD6FF556E+78] (C:\brave-browser\src\net\socket\client_socket_handle.cc:87)
        net::ClientSocketHandle::Reset [0x00007FFAD6FF5027+39] (C:\brave-browser\src\net\socket\client_socket_handle.cc:48)
        net::ClientSocketHandle::~ClientSocketHandle [0x00007FFAD6FF4F78+24] (C:\brave-browser\src\net\socket\client_socket_handle.cc:32)
        std::default_delete<net::ClientSocketHandle>::operator() [0x00007FFAD6FFE3EC+44] (C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\memory:1999)
        std::unique_ptr<net::ClientSocketHandle,std::default_delete<net::ClientSocketHandle> >::~unique_ptr [0x00007FFAD6FF466B+75] (C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.11.25503\include\memory:2204)
        net::SOCKS5ClientSocket::~SOCKS5ClientSocket [0x00007FFAD769866D+141] (C:\brave-browser\src\net\socket\socks5_client_socket.cc:52)
...

@darkdh
Copy link
Member

darkdh commented Aug 21, 2018

the above error turns out to be causing by sccache.
I did fresh build without using sccache and it works great and no test crash and startup crash
The build result sccache gave us is missing two members we patched username_ and password_
I inspect the object instance(HostPortPair) that being assigned, it only contains host_ and port_

@darkdh
Copy link
Member

darkdh commented Aug 21, 2018

All build pass, ProxyServerTest.*:SOCKS5ClientSocketTest.* unit tests pass and manually set --proxy-server with different credentials get different ip on https://check.torproject.org/ pass

@darkdh darkdh merged commit 0812b7a into master Aug 21, 2018
@bbondy bbondy deleted the riastradh-socks5-auth-v3 branch August 23, 2018 13:39
fmarier pushed a commit that referenced this pull request Oct 29, 2019
Add --disable_pdfjs_extension switch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reland SOCKS5 PR
2 participants