#31518 closed enhancement (implemented)

HAProxy implementation in TCPProxy option.

Reported by: haxxpop Owned by: haxxpop
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: proxy tcp
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

Since SOCKS5 takes 2 round trips, SOCKS4 doesn't support IPv6 and the proxy has to support DNS resolving in HTTP CONNECT, I would like to implement the proxy protocol of HAProxy to support IPv6 and the proxy doesn't have to resolve domain names by itself.

According to the mail thread, https://lists.torproject.org/pipermail/tor-dev/2019-August/013968.html, people suggested to create a new torrc option called TCPProxy and have a protocol as a parameter.

Child Tickets

Change History (23)

comment:1 Changed 15 months ago by haxxpop

Status: assignedneeds_review

comment:2 Changed 15 months ago by dgoulet

Component: Core TorCore Tor/Tor
Keywords: proxy tcp added
Milestone: Tor: unspecified

comment:3 Changed 15 months ago by haxxpop

If you would like you can also run the haproxy server by running

go install github.com/haxxpop/proxy-server
proxy-server

comment:4 Changed 15 months ago by asn

Reviewer: nickm

Nick, I'm giving you this review but since it's bigger than all the other reviews we had this week feel free to do it over multiple review periods. Feel free to remove yourself if you don't have time to do this , and we will give it somewhere else.

comment:5 Changed 15 months ago by nickm

First question: on your note above, what do you mean by "the proxy has to support DNS resolving in HTTP CONNECT"? We *want* all the DNS resolution to happen in Tor. Tor should really only be passing IP addresses; does it pass hostnames to HTTP CONNECT proxies?

Second question: is it actually a problem in practice that SOCKS5 takes two round trips?

Last edited 15 months ago by nickm (previous) (diff)

comment:6 Changed 15 months ago by nickm

Third question: what is the canonical specification for the protocol that this implements?

comment:7 Changed 15 months ago by nickm

Oh, I misunderstood: this is a new proxy type for tor to use, not for tor to implement. I'll adjust the above questions accordingly.

comment:8 Changed 15 months ago by nickm

Status: needs_reviewneeds_revision

I've left some questions and suggestions on the pull request.

comment:9 in reply to:  5 Changed 15 months ago by haxxpop

Replying to nickm:

First question: on your note above, what do you mean by "the proxy has to support DNS resolving in HTTP CONNECT"? We *want* all the DNS resolution to happen in Tor. Tor should really only be passing IP addresses; does it pass hostnames to HTTP CONNECT proxies?

Yes Tor will be passing only IP addresses.

I meant that if I want to deploy my own proxy server and if it's HTTP CONNECT, it's supposed to support DNS resolving because it will be probably used by some other proxy client (which is not Tor). So it's more reasonable for me to deploy a haproxy instead of HTTP CONNECT proxy.

Second question: is it actually a problem in practice that SOCKS5 takes two round trips?

I'm not sure. I don't have much experience on this :D. But in theory, according to the speed of light, if the server is in the US and the client is in Thailand, it will take at least 80ms more if it's two round trips.

comment:10 Changed 15 months ago by haxxpop

Status: needs_revisionneeds_review

comment:11 Changed 15 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

comment:12 Changed 15 months ago by nickm

Status: needs_reviewneeds_revision

Okay, this is looking better and better! I think it will be ready for inclusion in 0.4.3.x.

I added a suggestion to rename the "WANT_ACK" state.

The other request I have is to see if you can write tests for more functions. Right now, only around 1/4 of the new lines in your patch have test coverage. Coveralls didn't add its report to the ticket for some reason, but you can see the coverage report at https://coveralls.io/builds/25644803

If there's something you can't test, or think a test isn't necessary for, please let me know and we can talk about it.

comment:13 Changed 13 months ago by haxxpop

Status: needs_revisionneeds_review

comment:14 in reply to:  12 Changed 13 months ago by haxxpop

The coverage is higher now. The rest of the new lines that is not covered are not actually new lines. They are from refactoring and belongs to socks4 and socks5.

comment:15 Changed 13 months ago by nickm

Status: needs_reviewneeds_revision

Looking much better! I'm glad to see the test coverage here.

I've left a couple of small comments on the ticket. Also, this needs a "changes" file, as described in doc/HACKING/CodingStandards.md. After that's done, I think this will be ready to merge.

comment:16 Changed 12 months ago by haxxpop

Status: needs_revisionneeds_review

comment:17 Changed 12 months ago by nickm

I'm still almost okay to merge this. Just two issues:

1) "maek check" doesn't pass, because of a lintChanges issue.

  1. I'm still wondering if it's okay to do connection_start_reading() unconditionally in connection_or_finished_connecting(). I left a response to your response on the PR there back in Oct 25: https://github.com/torproject/tor/pull/1248#discussion_r339032153

If I get your go-ahead to fix the lintChanges issue and make the connection_start_reading() call unconditional, I will fix those, make sure CI still passes, and merge.

Version 0, edited 12 months ago by nickm (next)

comment:18 Changed 12 months ago by nickm

Status: needs_reviewneeds_revision

comment:19 Changed 11 months ago by haxxpop

Status: needs_revisionneeds_review

comment:20 Changed 11 months ago by nickm

Okay, I've squashed the branch and made another branch that merges it to master and resolves the merge conflicts, so that we can let CI test it.

The branch is tcp_proxy_squashed_and_merged; the PR is https://github.com/torproject/tor/pull/1637

Also, please let me know whether the merge commit looks right to you.

comment:21 Changed 11 months ago by haxxpopcf

lgtm!

comment:22 Changed 11 months ago by nickm

Merged!

comment:23 Changed 11 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.