Opened 5 years ago

Closed 5 years ago

#12606 closed defect (fixed)

Refactor obfs4proxy (and the support protocol code)

Reported by: yawning Owned by: yawning
Priority: Medium Milestone:
Component: Circumvention/Pluggable transport Version:
Severity: Keywords: obfs4
Cc: asn Actual Points:
Parent ID: #12130 Points:
Reviewer: Sponsor:

Description

While the current code works, it is very "A C programmer is writing Go". This mostly applies to the support protocol code, and refactoring it to make more use of channels/goroutines would simplify the code.

This ticket also encompases "general cleanup" to the code as I am not quite happy with some parts of the code.

Child Tickets

Attachments (1)

obfs4_conns.log (5.4 KB) - added by asn 5 years ago.
obfs4 logs

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by yawning

Component: ObfsproxyPluggable transport

comment:2 Changed 5 years ago by asn

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?

comment:3 Changed 5 years ago by asn

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

comment:4 in reply to:  2 Changed 5 years ago by yawning

Status: newaccepted

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.

comment:5 Changed 5 years ago by yawning

Status: acceptedneeds_review

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.

Changed 5 years ago by asn

Attachment: obfs4_conns.log added

obfs4 logs

comment:6 Changed 5 years ago by asn

Also, it would be nice if obfs4proxy's logging was less heavy volume. attachment: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.

comment:7 in reply to:  6 ; Changed 5 years ago by yawning

Replying to asn:

Also, it would be nice if obfs4proxy's logging was less heavy volume. attachment: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.

comment:8 in reply to:  7 Changed 5 years ago by yawning

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

comment:9 Changed 5 years ago by 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?

comment:10 in reply to:  9 Changed 5 years ago by yawning

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.

comment:11 Changed 5 years ago by 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?
  • 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.

comment:12 in reply to:  11 Changed 5 years ago by yawning

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.

comment:13 Changed 5 years ago by yawning

Resolution: fixed
Status: needs_reviewclosed

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.

Note: See TracTickets for help on using tickets.