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. |
ainghazal
force-pushed
the
feat/obfs4-dialer-support
branch
2 times, most recently
from
448280e to
ed19197
Compare
.gitignore
Outdated
| @@ -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.
obfs4/common.go
Outdated
| // 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
obfs4/obfs4.go
Outdated
| @@ -23,6 +23,24 @@ import ( | |||
| "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.
vpn/client.go
Outdated
|
|
||
| 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
| @@ -6,6 +6,9 @@ package vpn | |||
| // | |||
|
|
|||
| 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. | |||
| @@ -71,6 +74,12 @@ const ( | |||
|
|
|||
| // 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.
ainghazal
force-pushed
the
feat/obfs4-dialer-support
branch
from
d927246 to
028dca7
Compare
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.