feat: streamline support of obfs4 dialer#31
Conversation
|
this PR is missing tests - but let me know if you think there are possible improvements in the design. |
448280e to
ed19197
Compare
bassosimone
left a comment
bassosimone
left a comment
There was a problem hiding this comment.
It seems a good approach 👍
I've provided suggestions
bassosimone
left a comment
bassosimone
left a comment
There was a problem hiding this comment.
I think this PR still needs some extra tests. In the meanwhile, I provided an interim review.
| @@ -1,5 +1,5 @@ | |||
| minivpn | |||
| ./obfs4vpn | |||
| obfs4vpn | |||
There was a problem hiding this comment.
Why did this change? Could we have the binary at arbitrary directories?
There was a problem hiding this comment.
it actually should be removed, the cmd/obfs4vpn endpoint was deleted.
| // NewProxyNodeFromURI returns a configured proxy node. It accepts a string | ||
| // with all the parameters needed to establish a connection to the obfs4 | ||
| // proxy, in the form "obfs4://<ip>:<port>?cert=<deadbeef>&iat-mode=<int>" | ||
| func NewProxyNodeFromURI(uri string) (ProxyNode, error) { |
There was a problem hiding this comment.
I would return a *ProxyNode here
| return Node{ | ||
| return ProxyNode{ | ||
| Protocol: u.Scheme, | ||
| Addr: net.JoinHostPort(u.Hostname(), u.Port()), |
There was a problem hiding this comment.
You do not seem to be checking whether a port is defined here, so what happens if you pass a URL that does not have any port? Also, u.Host is equivalent to u.Hostname() when there is a port.
There was a problem hiding this comment.
Good question, and good catch. It should fail if no port is specified, there's no way to assign a default port.
There was a problem hiding this comment.
added test for the constructor, and checked for missing port, hostname
| "golang.org/x/net/proxy" | ||
| ) | ||
|
|
||
| type DialFunc func(string, string) (net.Conn, error) |
There was a problem hiding this comment.
Please, document new public types. Are not using a context-enabled dialer because other projects with which you want to be compatible do not define a context-enabled dialer as well?
There was a problem hiding this comment.
no, I think this was mostly because obfs4 passes a DialFunc. when integrating in OONI probably reusing OBFS4Dialer (with a DialContext method) would be the right thing to do...
There was a problem hiding this comment.
I've been looking at this again, and I think the reason I avoided going that way is that it entails quite a bit of code duplication (obfs4 transport).
There was a problem hiding this comment.
Did another take, I was wrong about the code duplication (probe-cli does, but I don't seem to need it here). I think I've addressed the context-enabled dialer problem now.
|
|
||
| const ( | ||
| // dialTimeoutInSeconds tells how long to wait on Dial | ||
| dialTimeoutInSeconds = 10 |
There was a problem hiding this comment.
Maybe this is a little too low; I'd use ~15 seconds here
| const ( | ||
| // Be very careful when altering the order of these event; other libraries | ||
| // might be trusting their values, so please release a new version and | ||
| // document the changes in that case. |
|
|
||
| // protoUDP is used for vpn in UDP mode. | ||
| protoUDP = proto("udp") | ||
|
|
There was a problem hiding this comment.
please, create a new const ( ) block for constants using a different type.
Before, there was a separate entry point for the obfs4 dialer. This commit will create the right dialer if a line with proxy-obfs4 is added to the openvpn config file (or the equivalent fields are set in Options). We also allow to pass any arbitrary dialer to the obfs4 base dialer. While doing this, some attention is given to the thought that we might want to support an arbitrary number of similar obfuscation proxies -that's why we're trying to reuse the abstractions from the gost project.
this is needed so that we can pass vpn.Dialer to HTTPConfig as part of the urlgetter experiment in OONI probe-cli.
d927246 to
028dca7
Compare
2 participants
Before, there was a separate entry point for the obfs4 dialer. This commit will create the right dialer if a line with proxy-obfs4 is added to the openvpn config file (or the equivalent fields are set in Options).
We also allow to pass any arbitrary dialer to the obfs4 base dialer.
While doing this, some attention is given to the thought that we might want to support an arbitrary number of similar obfuscation proxies -that's why we're trying to reuse the abstractions from the gost project.