Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#8402 closed defect (implemented)

Tor should help its transport proxy use a proxy, if needed.

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge pt flashproxy, tbb-needs, tbb-helpdesk-frequent, tbb-tor-backported-3.6.1 026-triaged-1
Cc: dcf@…, yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If a censored user wants to use both a (normal) proxy and a pluggable transport proxy, Tor should psas the credentials of the (normal) proxy to the pluggable transport proxy.

The proxy chain should look like this:
Tor (Client) -> Transport Proxy (Client) -> SOCKS/HTTP Proxy -> Internet -> Transport Proxy (Server) -> Tor (Bridge)

Arturo prepared a related proposal in:
https://lists.torproject.org/pipermail/tor-dev/2012-February/003318.html
We should clean it up, see if anything is missing, and start implementing it in Tor.

Child Tickets

TicketStatusOwnerSummaryComponent
#8403closedtorspec: Merge the "Pluggable Transport through SOCKS proxy" proposalCore Tor/Tor
#8956closedasnobfsproxy should use a SOCKS proxy if Tor wants it toObfuscation/Obfsproxy
#11409closedasnObfsproxy through HTTP proxyObfuscation/Obfsproxy
#11674closedDocument TOR_PT_PROXY in 'pt-spec.txt'.Core Tor/Tor

Attachments (2)

0001-Allow-ClientTransportPlugins-to-use-proxies.patch (12.5 KB) - added by yawning 4 years ago.
Patch to implement the tor side of things
0001-Allow-ClientTransportPlugins-to-use-proxies_ver2.patch (14.0 KB) - added by yawning 4 years ago.
Version 2 of the patch that supports SIGHUP

Download all attachments as: .zip

Change History (40)

comment:1 Changed 4 years ago by dcf

Cc: dcf@… added
Keywords: flashproxy added

comment:4 Changed 4 years ago by asn

Parent ID: #5195

comment:5 Changed 4 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:6 Changed 4 years ago by yawning

Cc: yawning added

comment:7 Changed 4 years ago by dcf

meek-client is capable of using an upstream HTTP proxy (since here), but you have to set it up manually, either with a command-line option

Bridge meek 0.0.2.0:1
ClientTransportPlugin meek exec ./meek-client --url=http://meek.bamsoftware.com:7002/ --http-proxy=http://127.0.0.1:8787/

or with a bridge line arg

Bridge meek 0.0.2.0:1 url=http://meek.bamsoftware.com:7002/ http-proxy=http://127.0.0.1:8787
ClientTransportPlugin meek exec ./meek-client

comment:8 Changed 4 years ago by yawning

I looked into implementing support for this and do not believe it will be difficult, but I think the proposal requires PT_MANAGED_TRANSPORT_VER bump.

My rationale for this is when it is necessary for users to use a SOCKS proxy (in a heavily censored enviornment for instance), tor should fail without attempting to send traffic over the network instead of attempting a direct connection that will fail, and possibly bring the wrath of the IT department (or whatever) down on the user.

Unless the version is bumped up, pyptlib/liballium will ignore this portion of the config protocol.

Thoughts? (Also if we are going to bump the version, are there any other things like this that should go into version 2 of the pt config protocol?)

comment:9 in reply to:  8 Changed 4 years ago by dcf

Replying to yawning:

I looked into implementing support for this and do not believe it will be difficult, but I think the proposal requires PT_MANAGED_TRANSPORT_VER bump.

That's not necessarily so. We could add a client stdout message that means "I have understood and will obey your proxy request," and tor could avoid sending anything through the PT until it gets that message.

comment:10 Changed 4 years ago by yawning

Well, that's what bumping the version would do.

I was thinking along the lines of "tor could only attempt to negotiate the v2 protocol if any of the various *Proxy options are set in the torrc". That may get messy in the future, but I'm not sure about how much more will get added to the PT spec, so it's hard to figure out what would be better.

comment:11 Changed 4 years ago by yawning

A ha, I'm blind, sorry.

If the pluggable transport proxy detects that the TOR_PT_PROXY
environment variable is set, it attempts connecting to it. On
success it writes to stdout: "PROXY true".
On failure it writes: "PROXY-ERROR <errormessage>".

So dcf's variant of this is already in the proposal. I'm not sure if attempting to connect to it is required (Would it be ok to just spit out "PROXY true" to acknowledge that it will use the proxy?), as that is potentially identifiable behaviour, but I don't have any other issues with the design and will probably look into adding this to pyptlib/liballium and adding support for this to obfsproxy.

Changed 4 years ago by yawning

Patch to implement the tor side of things

comment:12 Changed 4 years ago by yawning

Status: newneeds_revision

A patch against tor master that adds this functionality. The only pluggable transport currently capable of using it is my obfsproxy branch linked of #8956.

I'm not sure what is supposed to happen if the user edits the torrc to specify a different proxy (eg changing from nothing to SOCKS5, or SOCKS5 to HTTPS). The existing code probably won't detect this as something worthy of restarting all the pluggable transports, so I think I need to add code to detect this in or/transports.c/proxy_needs_restart(), along with caching the previously configured proxy settings.

Changed 4 years ago by yawning

Version 2 of the patch that supports SIGHUP

comment:13 Changed 4 years ago by yawning

Status: needs_revisionneeds_review

comment:14 Changed 4 years ago by asn

Nice!

A few comments from a preliminary review. I would like to review it once again:

  • As we discussed in IRC, unit tests for the non-trivial additions would be great.
  • Maybe we could functionify the new duplicate code in get_proxy_addrport(). I know that Nick hates duplicate code, and I share his sentiments.
  • This is more of a comment to the original proposal, but isn't PROXY true a bit off in a protocol that doesn't have any other false/true strings? Maybe PROXY DONE is more appropriate? Maybe not.
  • acked_proxy is a bit of a deceiving name. Maybe we should change the variable name to imply some connection to the proxy?

Other than that, patch looks very promising!

comment:15 Changed 4 years ago by asn

( Also a git branch would be preferrable to a patch ;) )

comment:16 in reply to:  14 ; Changed 3 years ago by yawning

Status: needs_reviewneeds_revision

Replying to asn:

A few comments from a preliminary review. I would like to review it once again:

  • As we discussed in IRC, unit tests for the non-trivial additions would be great.

Will do.

  • Maybe we could functionify the new duplicate code in get_proxy_addrport(). I know that Nick hates duplicate code, and I share his sentiments.

Ditto. I would have thought of something clever to do, but I was focused on getting an initial revision that worked.

  • This is more of a comment to the original proposal, but isn't PROXY true a bit off in a protocol that doesn't have any other false/true strings? Maybe PROXY DONE is more appropriate? Maybe not.

I would be ok with this, and agree that it makes sense (I considered doing it when I was writing, but decided to implement that portion of the spec as is).

  • acked_proxy is a bit of a deceiving name. Maybe we should change the variable name to imply some connection to the proxy?

Hmmm, I was going to change it to proxy_acked to match proxy_uri as far as naming goes, if there is something better for "the pt claims it will use the specified proxy", then I'm open to suggestions.

(Is what I said in comment #11 correct? The proposal should be changed to not require the pluggable transport to verify that the proxy is actually usable during the config right? Apart from trying to connect to something, that's not possible with SOCKS4 or 5 in a way that isn't at least somewhat suspicious.)

comment:17 in reply to:  16 ; Changed 3 years ago by asn

Replying to yawning:

Replying to asn:

A few comments from a preliminary review. I would like to review it once again:

  • As we discussed in IRC, unit tests for the non-trivial additions would be great.

Will do.

  • Maybe we could functionify the new duplicate code in get_proxy_addrport(). I know that Nick hates duplicate code, and I share his sentiments.

Ditto. I would have thought of something clever to do, but I was focused on getting an initial revision that worked.

  • This is more of a comment to the original proposal, but isn't PROXY true a bit off in a protocol that doesn't have any other false/true strings? Maybe PROXY DONE is more appropriate? Maybe not.

I would be ok with this, and agree that it makes sense (I considered doing it when I was writing, but decided to implement that portion of the spec as is).

Hm, if you also like the suggestion, let's do it then?

  • acked_proxy is a bit of a deceiving name. Maybe we should change the variable name to imply some connection to the proxy?

Hmmm, I was going to change it to proxy_acked to match proxy_uri as far as naming goes, if there is something better for "the pt claims it will use the specified proxy", then I'm open to suggestions.

Hm, not sure. How about supports_proxy? So that you can do if (mp.supports_proxy) {?
Doesn't look terribly bad. Probably can be improved.

(Is what I said in comment #11 correct? The proposal should be changed to not require the pluggable transport to verify that the proxy is actually usable during the config right? Apart from trying to connect to something, that's not possible with SOCKS4 or 5 in a way that isn't at least somewhat suspicious.)

Yes, I think that's reasonable. The PROXY true message is there so that Tor is assured that the PT understands that it has to use a proxy for any outgoing connections. It doesn't seem necessary to test the proxy for correctness at configure time.

comment:18 in reply to:  17 Changed 3 years ago by yawning

Status: needs_revisionneeds_review

https://github.com/yawning/tor/tree/bug_8402

Per discussion:

  • Changes now live in a branch.
  • PROXY true -> PROXY DONE
  • mp->acked_proxy -> mp->proxy_supported (I use proxy_uri, so flipped the suggested name to be consistent).
  • Duplicate code in get_proxy_addrport() moved into get_bridge_pt_addrport().
  • Now with unit tests for get_pt_proxy_uri().

I didn't end up writing a test for the duplicate code I moved into a function because I consider that trivial (existing code that was present in the pt/bridge if case moved into a separate function).

I also pushed the one line update to my pyptlib branch that makes it emit PROXY DONE and it seems to be working as expected.

comment:19 Changed 3 years ago by asn

This LGTM now. I need to test it too. I will do it tonight.

Thanks!

comment:20 Changed 3 years ago by asn

This worked fine in my tests. I tested obfsproxy in managed-mode with the proper tor/obfsproxy/pyptlib branches.

comment:21 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

I haven't had a chance for a full review yet, but I wanted to see if it still merged. It did, but when I tried to build it, I got a bunch of these:

src/test/test_pt.c:452:24: error: assigning to 'char *' from 'const char [15]'
      discards qualifiers
      [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
  options->Socks4Proxy = "192.0.2.1:1080";

If you haven't, please make sure you're building with --enable-gcc-warnings.

comment:22 Changed 3 years ago by yawning

Status: needs_revisionneeds_review

My bad, didn't know I had to do something special to enable all the warnings. Branch updated.

comment:23 Changed 3 years ago by asn

Re-tested and still works fine.
No warnings from --enable-gcc-warnings either.

(Maybe --enable-gcc-warnings should be on by default?)

comment:24 Changed 3 years ago by mikeperry

Keywords: tbb-needs tbb-helpdesk-frequent added

comment:25 Changed 3 years ago by nickm

noted on IRC:

11:48 < nickm> What I would be worried about is the case where:
11:48 < nickm> You have ClientTransportPlugin entries in your torrc
11:48 < nickm> You have a proxy
11:49 < nickm> but either you are not using bridges, or you are using  some 
               bridges with no transport set.
11:49 < nickm> (It's allowed to have ClientTransportPlugins even if you're not 
               using them, I hope)

(I haven't reviewed to make sure that this case works yet; it is worth testing.)

comment:26 Changed 3 years ago by yawning

That case works as expected, with the usual does not provide any needed transports and will not be launched spam in the log, and the configured proxy being used to bootstrap with the native tor wire protocol.

comment:27 Changed 3 years ago by yawning

I found some more minor issues in my branch for this and as a result revised it a bit.

Changes since the last time I checked it:

ClientTransportPlugins with associated Bridge lines that were killed due to lacking proxy support were being ignored, when it should have caused a failure. The code now correctly deals with this, and is additionally closer to the existing code (Instead of using a helper routine, code is just moved and altered slightly).

log_failed_proxy_connection() is calling get_proxy_type() despite having the actual proxy in a local variable. In a world where there can be 2 proxy types configured (PT + other), this is bad and leads to a log that is misleading.

comment:28 Changed 3 years ago by mikeperry

Keywords: tbb-tor-backported-3.6.1 added

We backported the maint-2.4 version of this (34004139ee9380c5c468d28037520d02681dd7cf) to include it in TBB 3.6.1 (as it will solve #11658, even if no PTs upgrade to support proxies themselves).

comment:29 Changed 3 years ago by dcf

(As I implement this for meek-client...) 232-pluggable-transports-through-proxy.txt says:

If the pluggable transport proxy detects that the TOR_PT_PROXY environment variable is set, it attempts connecting to it. On success it writes to stdout: "PROXY DONE". On failure it writes: "PROXY-ERROR <errormessage>".

I wonder if it would be better, instead of

PROXY DONE

to emit

PROXY <url>

for example

PROXY socks5://tor:test1234@198.51.100.1:8000

The reason for this is that it acts as positive confirmation: "You asked me to use a proxy; this is how I understood you."

Something similar exists with TOR_PT_SERVER_BINDADDR and SMETHOD. Tor can ask:

TOR_PT_SERVER_BINDADDR=dummy-127.0.0.1:1111

and the PT can give positive confirmation:

SMETHOD dummy 127.0.0.1:1111

The PT can even disobey tor's requested port, and inform tor that it is instead listening on a different port. This is what happens, for example, if you use the --port option in websocket-server and others.

SMETHOD dummy 127.0.0.1:9999

comment:30 Changed 3 years ago by nickm

Priority: normalmajor

Bumping to major; the TBB / PT teams really want this.

comment:31 Changed 3 years ago by asn

So, should we do David's suggestion? I think we all agree that it's better than the informationless PROXY DONE message that we currently do.

Yawning mentioned the problem that there are already versions of flashproxy/FTE/meek that implement the old proposal (only send PROXY DONE) and Tor would get confused if we changed that to PROXY socks5://tor:test1234@198.51.100.1:8000.

Maybe we could change it to PROXY DONE socks5://tor:test1234@198.51.100.1:8000 and treat the last part as optional but SHOULD do?

comment:32 in reply to:  31 Changed 3 years ago by dcf

Replying to asn:

So, should we do David's suggestion? I think we all agree that it's better than the informationless PROXY DONE message that we currently do.

Yawning mentioned the problem that there are already versions of flashproxy/FTE/meek that implement the old proposal (only send PROXY DONE) and Tor would get confused if we changed that to PROXY socks5://tor:test1234@198.51.100.1:8000.

Maybe we could change it to PROXY DONE socks5://tor:test1234@198.51.100.1:8000 and treat the last part as optional but SHOULD do?

It would be better for the PROXY line to have more information, but at this point it may not be worth breaking the interface of deployed programs. There's a de-facto standard now that there's code in the wild.

My use case was a narrow one. It was: I want to have a --proxy command-line option in meek-client. The command line overrides anything set through the PT protocol. (The --port option in e.g. websocket-server works the same way: it overrides TOR_PT_SERVER_BINDADDR and listens on the port you give it; then informs tor that it was ignored through the SMETHOD line.) I want to be able to at least inform tor that I am ignoring its proxy setting by telling it the proxy I'm actually going to use.

The use case is not totally coherent because meek-client can also take a proxy parameter per-bridgeline, which overrides the command line and the PT protocol (see man page). (If it seems weird to use a non-global HTTP proxy, i.e., not just use HTTPSProxy for all transports, think of the use case of chaining one proxy to another, like meek-in-Lantern.) So it's possible that even if the PROXY line gives a transport URL, the actual proxy could be different, set dynamically with a SOCKS arg.

comment:33 Changed 3 years ago by nickm

  • I think we want to allow the user to specify a lot of ClientTransportPlugins, and have that not matter unless they actually use one for a bridge? If that's so, the new behavior in get_proxy_type() and parse_client_transport_line() might need work.
  • I think I merged another branch to refactor parse_client_transport_line(). I should check whether this conflicts badly now.
  • The documentation in get_pt_proxy_uri() should document that it returns a newly allocated string or NULL.
  • Do we verify elsewhere that our Socks5 username and password don't have any characters that cause trouble in a URI? Should we? (Same question for HTTPSProxyAuthenticator.)
  • Do we verify elsewhere that our Socks5 username and password are either both set or neither set?
  • We usually prefer "unsigned int x : 1" for one-bit booleans: 1 is a less surprising result for true than -1 is.

comment:34 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

comment:35 Changed 3 years ago by nickm

Keywords: 026-triaged-1 added

comment:36 in reply to:  33 Changed 3 years ago by yawning

Status: needs_revisionneeds_review

Replying to nickm:

  • I think we want to allow the user to specify a lot of ClientTransportPlugins, and have that not matter unless they actually use one for a bridge? If that's so, the new behavior in get_proxy_type() and parse_client_transport_line() might need work.

This use case works fine, since that is what Tor Browser does with it's default configuration (Lots of ClientTransportPlugins pre-configured).

  • I think I merged another branch to refactor parse_client_transport_line(). I should check whether this conflicts badly now.

I will fix this if needed, just let me know.

  • The documentation in get_pt_proxy_uri() should document that it returns a newly allocated string or NULL.

Fixed.

  • Do we verify elsewhere that our Socks5 username and password don't have any characters that cause trouble in a URI? Should we? (Same question for HTTPSProxyAuthenticator.)

We do not do verification for either Socks5 or HTTPS. No one's complained about this yet. Fixing it correctly would be modifying the proposal/pt-spec to specify using URL (percent) encoding, and adding code to do so (either for only the problematic characters, or the full RFC 3986 reserved set).

I'm somewhat inclined to say "leave this till it's actually a problem for someone" since adding the encoder would be quite a bit more code.

  • Do we verify elsewhere that our Socks5 username and password are either both set or neither set?

options.c:options_validate() handles both checks.

  • We usually prefer "unsigned int x : 1" for one-bit booleans: 1 is a less surprising result for true than -1 is.

Fixed.

The missing changes file has also been added (edit to suit taste).

https://github.com/Yawning/tor/commit/cae44838fecc550f2df16de44b0ef297ecb509a6

comment:37 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good to me then. Merged and done!

comment:38 Changed 3 years ago by nickm

[Had to apply 43a47ae7265e0030e0e275fbf01caf1c313710e3 from Yawning in an attempt to make the tests happy on Windows]]

Note: See TracTickets for help on using tickets.