Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(61)

Issue 507033: When talking to a SOCKS v5 proxy, default to sending addresses as raw domains... (Closed)

Created:
11 years ago by eroman
Modified:
9 years, 6 months ago
Reviewers:
wtc
CC:
chromium-reviews_googlegroups.com, darin (slow to review), Paweł Hajdan Jr.
Visibility:
Public.

Description

When talking to a SOCKS v5 proxy, default to sending addresses as raw domains rather than IP addresses. Before, we would default to client-side DNS resolution (sending IP addresses to the proxy) for both v4 and v5. However if you are using a v5 server, it is most likely that you want to do the resolves on the proxy-side. And in fact if you are using a SOCKS 5 proxy to anonymize your browsing, you definitely don't want that as the default policy. Embedders of the network stack can select the alternate policy by passing a non-NULL Host resolver into SOCKS5ClientSocket. BUG=29914 TEST=HttpNetworkTransactionTest.SOCKS5_HTTP_GET, HttpNetworkTransactionTest.SOCKS5_SSL_GET Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34903

Patch Set 1 #

Total comments: 5

Patch Set 2 : Address wtc's comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+112 lines, -25 lines) Patch
M net/http/http_network_transaction.cc View 1 1 chunk +1 line, -1 line 1 comment Download
M net/http/http_network_transaction_unittest.cc View 1 2 chunks +21 lines, -4 lines 0 comments Download
M net/socket/socks5_client_socket.h View 1 2 chunks +19 lines, -2 lines 0 comments Download
M net/socket/socks5_client_socket.cc View 3 chunks +12 lines, -4 lines 0 comments Download
M net/socket/socks5_client_socket_unittest.cc View 13 chunks +58 lines, -13 lines 0 comments Download
M net/socket_stream/socket_stream.cc View 1 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 4 (0 generated)
eroman
11 years ago (2009-12-17 04:43:40 UTC) #1
wtc
LGTM. Thanks for implementing this change quickly. We should consider dropping client-side hostname resolution support ...
11 years ago (2009-12-17 22:24:17 UTC) #2
eroman
Addressed all comments. > We should consider dropping client-side hostname > resolution support for SOCK5 ...
11 years ago (2009-12-17 22:49:50 UTC) #3
wtc
11 years ago (2009-12-17 23:23:04 UTC) #4
LGTM.

http://codereview.chromium.org/507033/diff/5001/6002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/507033/diff/5001/6002#newcode655
net/http/http_network_transaction.cc:655: s = new SOCKS5ClientSocket(s,
req_info, NULL /*use proxy-side resolving*/);
Nit: unless you're trying to fit in 80 chars, it looks nicer
to add spaces after /* and before */.

http://codereview.chromium.org/507033/diff/5001/6006
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/507033/diff/5001/6006#newcode702
net/socket_stream/socket_stream.cc:702: s = new SOCKS5ClientSocket(s, req_info,
NULL /*use proxy-side resolving*/);
Maybe you can just say "proxy-side resolving" to deal with
the 80-char limit.

Powered by Google App Engine
This is Rietveld 408576698