Refactor obfs4proxy (and the support protocol code)

added component::circumvention/pluggable transport obfs4 owner::yawning parent::12130 priority::medium resolution::fixed status::closed type::defect labels

Trac:
Component: Obfsproxy to Pluggable transport

Code looks good. Couldn't find any obvious issues.

Some notes:

It would be nice if findMarkMac() had a unittest.
You might want to consider stealing the integration tester of obfsproxy, even if it's just for your own testing. It has really helped over the years to find some dump code changes that break transports.
In serverSetup() maybe you want to continue in:

		ln, err := net.ListenTCP("tcp", bindaddr.Addr)
		if err != nil {
			pt.SmethodError(name, err.Error())
		}

Maybe you want to add a link to obfs2/obfs3 specs?
Installation instructions are needed.
Maybe now is a good time to establish a ChangeLog too?

I still need to set it up in my bridge, and then I will give my blessings.

Some of this is per discussion from a while back on IRC.

Replying to asn:

It would be nice if findMarkMac() had a unittest.

I could do this, but the code is well exercised via the exaustive tests in handshake_ntor_test.go, so I will hold off on this for now.

You might want to consider stealing the integration tester of obfsproxy, even if it's just for your own testing. It has really helped over the years to find some dump code changes that break transports.

I have some plans for a better integration tester, worst comes to worst I will do this just for the sake of being able to maintain the code (I don't think this is a release blocker though).

In serverSetup() maybe you want to continue in: {{{ ln, err := net.ListenTCP("tcp", bindaddr.Addr) if err != nil { pt.SmethodError(name, err.Error()) } }}}

Fixed a while ago.

Maybe you want to add a link to obfs2/obfs3 specs?

Hmmm. I could, but the protocols are deprecated/will be deprecated so no one should be writing new implementations of them. I'll do a final pass on the README.md prior to tagging and I'll link to obfsproxy's copies from there.

Installation instructions are needed.

See above about updating the documentation prior to tagging.

Maybe now is a good time to establish a ChangeLog too?

Done.

There's one more thing I want to do (make IAT parameters part of the bridge line, so people can chose to run bridges with more paranoia), but as far as I am aware apart from that, updating the documentation, and tagging a release the coding is done.

Trac:
Status: new to accepted

Ok, in the latest series of commits I went and:

Made the JSON parse failure errors more descriptive.
Fixed a really dumb issue that was preventing the server side PRNG seed from being applied on the client.
Added another bridge line argument "iat-mode", which allows the bridge to impose it's will on the client wrt to IAT obfuscation (disabled by default).
Added the option to trade off even more performance for obfuscation when using IAT obfuscation by always sampling the length of the packet to send as opposed to only padding out the tail.

As far as I know, only updating the README.md remains for things I need to do to obfs4proxy, and separate bugs for such things will suffice as they arise. Setting to needs_review to get interested parties signoffs.

Trac:
Status: accepted to needs_review

Trac:
obfs4_conns.log

obfs4 logs

Also, it would be nice if obfs4proxy's logging was less heavy volume. obfs4_conns.log has a sample of what the logs look like in my bridge.

As a bridge op, I regularly check obfspoxy logs to see if any new handshake log warnings have triggered (perhaps active scanning?), but if the log is so busy it's easy to lose information. Log severities or more strategic logging would be helpful.

Also that ERROR "handshake failed" is probably someone who disconnected during the handshake, not something more serious, right?

fwiw I still haven't reviewed your new changes.

Replying to asn:

Also, it would be nice if obfs4proxy's logging was less heavy volume. obfs4_conns.log has a sample of what the logs look like in my bridge.

As a bridge op, I regularly check obfspoxy logs to see if any new handshake log warnings have triggered (perhaps active scanning?), but if the log is so busy it's easy to lose information. Log severities or more strategic logging would be helpful.

Go lang's default logging package doesn't support log leveling, so I would need to do this on my own, which is why the logging currently is only really useful when enabled and the scrubber disabled. I figured that most bridge operators wouldn't enable logging except when debugging problems, for which disabling the scrubber would be ok.

(Insert rant about how the debian go packaging and tbb build process combine to provide a rather large disincentive for using an existing 3rd party library by making it a huge pain to increase the number of dependencies.)

Also that ERROR "handshake failed" is probably someone who disconnected during the handshake, not something more serious, right?

Probably, but because of the log scrubbing that I had to put in, it'll suppress the more interesting errors unless the log scrubber is disabled, so there's no way to tell at the moment. I'll revise the code to only suppress go network errors and see if that's better.

fwiw I still haven't reviewed your new changes.

Ok, no rush. I'm looking to tag the current code (with the logging changes) soon-ish as long as no major problems show up.

Replying to yawning:

Replying to asn:

Also that ERROR "handshake failed" is probably someone who disconnected during the handshake, not something more serious, right?

Probably, but because of the log scrubbing that I had to put in, it'll suppress the more interesting errors unless the log scrubber is disabled, so there's no way to tell at the moment. I'll revise the code to only suppress go network errors and see if that's better.

https://gitweb.torproject.org/pluggable-transports/obfs4.git/commitdiff/ce39988b11a56e763c8eedbbdab75a35b48366d4

I did a bit better and wrote a sanitizer for common net.Error types. If it gets passed a net.Error that it doesn't know about, it will pring network error: <TYPE OF ERROR> which is less than ideal, but I covered most of the likely ones, and what the user is probably most interested in are protocol errors (unsanitized) and net.OpErrors (handled).

New changes look good and my bridge seems to function fine with them.

I wonder if you could have used base32 instead of base16. Do implementations of base32 append padding?

Replying to asn:

New changes look good and my bridge seems to function fine with them.

I wonder if you could have used base32 instead of base16. Do implementations of base32 append padding?

I thought about using Base32, but alas the lengths of the fields I'm exporting would likewise require padding there. Once I fix the issues in the SOCKS5 stuff and the TBB integration branch I'll go back and add support for leveled logging, update the README and tag a release.

Some review up to 79e94103d6bf7685c78e991f0d1b93b0eecefa3d:

It doesn't matter much, but I prefer --version or -v over -version. I think that's what the GNU standard suggests. Can you support both -v and --version?
Here are the contents of a new log file after the log tiers were added:

2014/09/03 13:23:19 [WARN]: obfs3([scrubbed]:27078) - handshake failed: EOF
2014/09/03 13:23:19 [WARN]: obfs3([scrubbed]:27080) - handshake failed: EOF
2014/09/03 13:23:33 [WARN]: obfs3([scrubbed]:12889) - closed connection: read: connection reset by peer
2014/09/03 13:23:34 [WARN]: obfs3([scrubbed]:50767) - closed connection: read: connection reset by peer
2014/09/03 13:23:36 [WARN]: obfs3([scrubbed]:51263) - closed connection: read: connection reset by peer
2014/09/03 13:23:52 [WARN]: obfs3([scrubbed]:61503) - closed connection: read: connection reset by peer
2014/09/03 13:24:17 [WARN]: obfs3([scrubbed]:51515) - closed connection: write: connection reset by peer
2014/09/03 13:24:32 [WARN]: obfs3([scrubbed]:29223) - handshake failed: read: i/o timeout
2014/09/03 13:24:36 [WARN]: obfs3([scrubbed]:10791) - closed connection: read: connection reset by peer
2014/09/03 13:24:43 [WARN]: obfs3([scrubbed]:11820) - closed connection: read: connection reset by peer
2014/09/03 13:24:46 [WARN]: obfs3([scrubbed]:17999) - closed connection: read: connection reset by peer
2014/09/03 13:24:47 [WARN]: obfs3([scrubbed]:2171) - handshake failed: read: i/o timeout
2014/09/03 13:24:56 [WARN]: obfs3([scrubbed]:52261) - handshake failed: EOF
2014/09/03 13:24:57 [WARN]: obfs3([scrubbed]:52263) - handshake failed: EOF

Notice that it's full of innocent handshake failures warnings. This is suboptimal, I think.

Also, there should be a log prologue when obfs4proxy starts up, mentioning the obfs4proxy version. Otherwise, it's hard to know when obfs4proxy was restarted.

Replying to asn:

Some review up to 79e94103d6bf7685c78e991f0d1b93b0eecefa3d:

It doesn't matter much, but I prefer --version or -v over -version. I think that's what the GNU standard suggests. Can you support both -v and --version?

Both is kind of annoying but doable (as in, it would pollute the usage list and make things look messy) because of how primitive Go's flag package is. --version works as is, I just documented it as -version for brevity.

Here are the contents of a new log file after the log tiers were added: {{{ 2014/09/03 13:23:19 [WARN]: obfs3([scrubbed]:27078) - handshake failed: EOF 2014/09/03 13:23:19 [WARN]: obfs3([scrubbed]:27080) - handshake failed: EOF 2014/09/03 13:23:33 [WARN]: obfs3([scrubbed]:12889) - closed connection: read: connection reset by peer 2014/09/03 13:23:34 [WARN]: obfs3([scrubbed]:50767) - closed connection: read: connection reset by peer 2014/09/03 13:23:36 [WARN]: obfs3([scrubbed]:51263) - closed connection: read: connection reset by peer 2014/09/03 13:23:52 [WARN]: obfs3([scrubbed]:61503) - closed connection: read: connection reset by peer 2014/09/03 13:24:17 [WARN]: obfs3([scrubbed]:51515) - closed connection: write: connection reset by peer 2014/09/03 13:24:32 [WARN]: obfs3([scrubbed]:29223) - handshake failed: read: i/o timeout 2014/09/03 13:24:36 [WARN]: obfs3([scrubbed]:10791) - closed connection: read: connection reset by peer 2014/09/03 13:24:43 [WARN]: obfs3([scrubbed]:11820) - closed connection: read: connection reset by peer 2014/09/03 13:24:46 [WARN]: obfs3([scrubbed]:17999) - closed connection: read: connection reset by peer 2014/09/03 13:24:47 [WARN]: obfs3([scrubbed]:2171) - handshake failed: read: i/o timeout 2014/09/03 13:24:56 [WARN]: obfs3([scrubbed]:52261) - handshake failed: EOF 2014/09/03 13:24:57 [WARN]: obfs3([scrubbed]:52263) - handshake failed: EOF }}}

Notice that it's full of innocent handshake failures warnings. This is suboptimal, I think.

Hmm, not sure how I should disambiguate "people potentially doing evil things" vs "innocent handshake failures". I don't particularly see the log as being useful for anything but testing/debugging to be honest, so the idea still is "enable if needed to troubleshoot".

If you have a suggestion for how to disambiguate this in a way that's not "suppress common socket errors", please let me know.

Also, there should be a log prologue when obfs4proxy starts up, mentioning the obfs4proxy version. Otherwise, it's hard to know when obfs4proxy was restarted.

I'll do this right now.

Tag: https://gitweb.torproject.org/pluggable-transports/obfs4.git/tag/3cf955029e65023e046f1c2f2aa0681fe2d8e742

It's as good as any other project with a version number that reads 0.0.1 at this point. Closing. Further issues will get separate tickets.

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

closed

mentioned in issue #12903 (moved)

mentioned in issue #12910 (moved)

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

mentioned in issue tpo/anti-censorship/pluggable-transports/trac#12910 (closed)

Refactor obfs4proxy (and the support protocol code)

Child items ...

Activity