Opened 3 months ago

Last modified 3 weeks ago

#31518 needs_revision enhancement

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 (15)

comment:1 Changed 3 months ago by haxxpop

Status: assignedneeds_review

comment:2 Changed 3 months ago by dgoulet

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

comment:3 Changed 3 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 2 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 2 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 2 months ago by nickm (previous) (diff)

comment:6 Changed 2 months ago by nickm

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

comment:7 Changed 2 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 2 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 2 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 2 months ago by haxxpop

Status: needs_revisionneeds_review

comment:11 Changed 2 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

comment:12 Changed 2 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 3 weeks ago by haxxpop

Status: needs_revisionneeds_review

comment:14 in reply to:  12 Changed 3 weeks 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 3 weeks 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.

Note: See TracTickets for help on using tickets.