Opened 6 years ago

Closed 4 years ago

#12125 closed project (fixed)

Proposal 232 (TOR_PT_PROXY) support for goptlib

Reported by: dcf Owned by: dcf
Priority: Medium Milestone:
Component: Circumvention/Pluggable transport Version:
Severity: Keywords: goptlib
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should support proposal 232/#8402 (TOR_PT_PROXY) in goptlib.

Child Tickets

Change History (8)

comment:1 Changed 6 years ago by dcf

Here's my proposed interface.

We add two new functions:

func ProxyError(msg string) error {
        return doError("PROXY-ERROR", msg)
}

func ProxyDone() {
        line("PROXY", "DONE")
}

We add one new field to ClientInfo:

        ProxyURL *url.URL

ClientSetup fills in ProxyURL. It only parses the URL, and doesn't check, for example, that the scheme is that of a known proxy protocol. Leave that to the application; the application has to know what proxy types it supports anyway.

I have some prototypes of this API with tests in the meek-client commit at https://gitweb.torproject.org/pluggable-transports/meek.git/commitdiff/81206ad1ece0aa33d598ac2d265ef636a278e91d.

comment:2 Changed 6 years ago by yawning

Welp, I likewise spent the evening working on this for obfs4proxy (SOCKSv5 works through the magic of library code, I'll write SOCKSv4/HTTP support next).

My ProxyError/ProxyDone routines are basically identical. I went with having a ptGetProxy() routine and I currently do more validation (check vs known scheme types, validate that the rest of the URI is sane) since I stole the code I wrote for pyptlib, and need to have those checks anyway.

https://github.com/Yawning/obfs4/blob/master/obfs4proxy/pt_extras.go#L83

Changing my code to use your interface at a later date is no problem for me.

comment:3 in reply to:  2 ; Changed 6 years ago by dcf

Status: newneeds_review

Replying to yawning:

Welp, I likewise spent the evening working on this for obfs4proxy (SOCKSv5 works through the magic of library code, I'll write SOCKSv4/HTTP support next).

My ProxyError/ProxyDone routines are basically identical. I went with having a ptGetProxy() routine and I currently do more validation (check vs known scheme types, validate that the rest of the URI is sane) since I stole the code I wrote for pyptlib, and need to have those checks anyway.

https://github.com/Yawning/obfs4/blob/master/obfs4proxy/pt_extras.go#L83

Changing my code to use your interface at a later date is no problem for me.

It's good that you did this. now we have two clean-room implementations that we can compare against each other for bugs. They can both just mellow in their respective programs until we merge them to goptlib.

It looks like ProxyDone and ProxyError are easy and uncontroversial.

I'm thinking of doing even less validation of the URL, on the assumption that application authors know what they're doing and they're going to have to crawl the URL structure anyway to do anything useful. But the checks for credentials that won't work (like a password with socks4a, or too-long credentials with socks5) may be a good idea. On the other hand, I might like to let it fail at connect time, because the application has to check for errors at that time anyway.

validateAddrStr appears to allow only IP addresses, not host names. I guess prop 232 isn't clear whether host names need to be supported. I assumed they should be, without thinking about it. (The use case I'm thinking of is like a corporate proxy-only network, where you set your proxy to "http://proxy.megacorp.example.com:8000/". I was also doing tests locally with "localhost".) I suppose this should be clarified in the proposal.

comment:4 in reply to:  3 ; Changed 6 years ago by yawning

Replying to dcf:

I'm thinking of doing even less validation of the URL, on the assumption that application authors know what they're doing and they're going to have to crawl the URL structure anyway to do anything useful. But the checks for credentials that won't work (like a password with socks4a, or too-long credentials with socks5) may be a good idea. On the other hand, I might like to let it fail at connect time, because the application has to check for errors at that time anyway.

Hmmm, currently our connect time error reporting is all sorts of terrible, so I would rather PROXY-ERROR out on malformed URIs. As long as ProxyError is exposed, leaving this up to the application would be fine.

The proposal also says that the pluggable transports should connect to the proxy before PROXY DONEing, which implies that validation is done at config time, but none of the existing implementations actually do the "connect before reporting" thing (IIRC I discussed this with asn when writing the patch and we came to the conclusion that just validating would be better because opening network connections on startup that aren't used is rude, and possibly identifiable behaviour). This should be changed in the proposal.

validateAddrStr appears to allow only IP addresses, not host names. I guess prop 232 isn't clear whether host names need to be supported. I assumed they should be, without thinking about it. (The use case I'm thinking of is like a corporate proxy-only network, where you set your proxy to "http://proxy.megacorp.example.com:8000/". I was also doing tests locally with "localhost".) I suppose this should be clarified in the proposal.

This should be clarified in the proposal, yes. For what it's worth, as it stands now, the code will always pass an IP address in the URI because tor does a tor_addr_port_lookup on each of the variables when parsing the config file (the URI format does say ip). Allowing FQDNs would be more robust to changes on the tor side of the equation, but I am uncertain as to if this will change any time in the foreseeable future.

comment:5 in reply to:  4 Changed 6 years ago by dcf

Replying to yawning:

The proposal also says that the pluggable transports should connect to the proxy before PROXY DONEing, which implies that validation is done at config time, but none of the existing implementations actually do the "connect before reporting" thing (IIRC I discussed this with asn when writing the patch and we came to the conclusion that just validating would be better because opening network connections on startup that aren't used is rude, and possibly identifiable behaviour). This should be changed in the proposal.

I agree completely. It's a bug in the proposal. I've been ignoring that part, on the assumption that it will get changed.

validateAddrStr appears to allow only IP addresses, not host names. I guess prop 232 isn't clear whether host names need to be supported. I assumed they should be, without thinking about it. (The use case I'm thinking of is like a corporate proxy-only network, where you set your proxy to "http://proxy.megacorp.example.com:8000/". I was also doing tests locally with "localhost".) I suppose this should be clarified in the proposal.

This should be clarified in the proposal, yes. For what it's worth, as it stands now, the code will always pass an IP address in the URI because tor does a tor_addr_port_lookup on each of the variables when parsing the config file (the URI format does say ip). Allowing FQDNs would be more robust to changes on the tor side of the equation, but I am uncertain as to if this will change any time in the foreseeable future.

Good to know, thanks.

comment:6 Changed 5 years ago by dcf

So a variation on this has been shipping with meek in the 4.0-alpha-1 bundles. Should we work out the differences between meek's and obfs4proxy's implementations and make a patch? Or would you rather expose the obfs4 code to battle first?

Here's meek's implementation (remember that PtGetProxyURL will go away and be replaced by a field in ClientInfo):

And here is how you use it:

Here's obfs4proxy's implementation:

And here is how you use it:

I think the main difference is how much validation goptlib should do before letting the application see the URL, and what to do in some unspecified cases. (I'm leaning to "reject" for the commented test cases at https://gitweb.torproject.org/pluggable-transports/meek.git/blob/refs/tags/0.10:/meek-client/proxy_test.go#l43.)

comment:7 Changed 4 years ago by dcf

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

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