Proposal 232 (TOR_PT_PROXY) support for goptlib

added component::circumvention/pluggable transport goptlib owner::dcf priority::medium resolution::fixed status::closed type::project labels

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.

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.

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.

Trac:
Status: new to needs_review

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.

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.

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):

Here's obfs4proxy's implementation:

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.)

Here's a goptlib branch ready to merge: https://gitweb.torproject.org/pluggable-transports/goptlib.git/commit/?h=bug12125&id=d433318f1635b9d14b29236df7556cb42378932b And here is the adaptation to the new interface in meek-client: https://gitweb.torproject.org/pluggable-transports/meek.git/commit/?h=bug12125&id=bff262d015426ca5687dbdfbed035bd4f8bdd20a I will merge this soon if there are no objections.

Replying to dcf:

Here's a goptlib branch ready to merge: https://gitweb.torproject.org/pluggable-transports/goptlib.git/commit/?h=bug12125&id=d433318f1635b9d14b29236df7556cb42378932b And here is the adaptation to the new interface in meek-client: https://gitweb.torproject.org/pluggable-transports/meek.git/commit/?h=bug12125&id=bff262d015426ca5687dbdfbed035bd4f8bdd20a I will merge this soon if there are no objections.

Merged in goptlib d433318f16 and meek eafbc1d735.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

mentioned in issue #28849 (moved)

moved to tpo/anti-censorship/pluggable-transports/trac#12125 (closed)

Proposal 232 (TOR_PT_PROXY) support for goptlib

Child items 0

Activity