Opened 8 years ago

Closed 8 years ago

#3613 closed enhancement (fixed)

obfsproxy: groundwork for new protocols

Reported by: zwol Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Please review and pull from https://github.com/zackw/obfsproxy/compare/master...more-protocols (you will probably find it easier to read individual changes in the series). The branch is *called* "more-protocols", but I haven't started writing new protocols yet; highlights of what you get are:

  • cleaner protocol-specific API
  • better way to define protocol vtables
  • imported the Tor container library
  • consistent use of "infallible" memory allocation
  • many of the unit tests have been improved
  • fixed some nasty memory leaks in obfs2 and minor ones elsewhere

I plan to keep hacking on this branch; the last commit I'm asking for review of at this time is https://github.com/zackw/obfsproxy/commit/2dd47ce6624d374932f7c212b1f719ad09143c5e (but nothing more is likely to show up till this evening).

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by rransom

Status: newneeds_review

comment:2 Changed 8 years ago by nickm

Looking through the branch with asn now...

As a general issue, it's best to split code changes from moving things around. (I think I already mentioned this and am just looking at older commits here, but just in case I should say it again.

Try to use the standard git commit message format where the first line is a *short* summary of what the commit does. I'm not insisting on a strict 60-char limit or whatever the stricter projects are using, but having the first line of the commit msg span more than one 80-char line is a little much.

It looks like after your assert() changes, there are some bare abort()s in logging (maybe elsewhere too?) It's better to use assert(0) or assert(0==1) so that we get a message printed with the line number. Also, maybe log_error() ought to abort() rather than exit(1), so that we can get stack traces.

I don't like how your patch that allegedly was going to change from DLL to smartlist also removed stuff like memory-poison-before-free and removed calls to listener_free() [replacing it with a partial implementation inline]. What's the point of that? Why not just call listener_free to free a listener?

Using smartlist_remove() to remove a connection from a list is a step backwards. That's an O(N) operation since it needs to do a linear search over all the connections. I guess it won't happen often, though.

Also, close_conn probably wants to be named close_and_free_conn() or something to make it clear that the connection is now unexistent after calling it.

The new asserts in input_event_cb() are dangerous. Libevent is allowed to add new event types in the future; if we get a BEV_EVENT_FOOBAR, we should just ignore it, not die with an assertion. The other event handling functions have a similar problem.

I think the next step is to review all the new callbacks logic and to try it out a bit, clean up the major issues from above, then merge.

comment:3 Changed 8 years ago by asn

Hi!
Just letting you know that for the next some days I'll be checking out your more-protocols branch to get fully acquainted with all your changes.

It would be great fun if you splitted the more-protocols branch to two or more branches so that they are more easily reviewed and merged. Specifically, split it to branches of "commits that fix/improve stuff" and branches of "commits that create new stuff".
For example:

  • branch that improves the current protocol API.
  • branch that improves the current network/connection API.
  • branch that implemenents steganography API.
  • branch that implements x_http and x_dsteg.

I have been preparing a "big" branch that I would like to merge some time semi-soon (#3474 [0]) and it's currently based on master. If it gets merged, your stuff will be unmergeable and I don't want to do that.
I think that a good way to resolve this, is to split your more-protocols branch, and then choose a resulting branch for me to use as #3474's parent branch.
For example, it seems like commit 19eda9f8 is the last commit before you start implementing the stego stuff, maybe I could use that point as my parent commit.
What do you think?

[0]: #3474 implements managed proxies, it currently looks like this:
`

Makefile.am | 6 +-
configure.ac | 2 -
src/external.c | 216 ----------------
src/external.h | 10 -
src/main.c | 321 +++++++++++++++++--------
src/main.h | 21 --
src/managed.c | 574 -------------------------------------------
src/managed.h | 35 ---
src/network.c | 17 +-
src/network.h | 2 -
src/protocol.c | 61 +----
src/protocol.h | 17 +-
src/protocols/dummy.c | 96 +-------
src/protocols/dummy.h | 7 +-
src/protocols/obfs2.c | 111 +--------
src/protocols/obfs2.h | 6 +-
src/protocols/obfs2_crypt.c | 8 +-
src/socks.c | 4 +-
src/test/unittest_obfs2.c | 75 ++++---
src/util.c | 39 +---
src/util.h | 24 --

`

comment:4 Changed 8 years ago by asn

Alright, I think I'm now up to date with all the design changes till 19eda9f8.
I'll now start looking at x_http and x_dsteg.

BTW, we should probably decide on code style, before we bastardize the whole code base. For example, connections.c functions are now commented on the header file. I don't like this [0], but if we decide to keep it like this, we should migrate the rest of the code as well.

obfs2 must be revived before merging your stuff. It serves as a nice basic transport example. I rolled back to 674d0735 and manually tested obfs2 and it seems to work.
I also used the old int_test.sh on it, and it worked nicely. I'm not sure why it doesn't work on tester.py; I haven't checked the code of tester.py.in that much.

I have a bug3613_shutdown branch in my repo which fixes a small issue while shutting down obfsproxy barbarically.

Thanks for your work!

[0]: I enjoy easily viewing the function documentation while checking out a function's code.

comment:5 Changed 8 years ago by asn

Oh yeah, another thing, you are now aborting if a single listener could not be established, instead of continuing with the other listeners. Do we like this?

Also, because of the log_error()s we don't free all memory in case something in main.c goes wrong. Do we like this?

comment:6 Changed 8 years ago by asn

FWIW, more-protocols up to and including 19eda9f8 (and with 60fa1ee8 cherry-picked) got merged.

comment:7 Changed 8 years ago by arma

Component: Pluggable transportObfsproxy

comment:8 Changed 8 years ago by asn

Resolution: fixed
Status: needs_reviewclosed

Closing this for now. Some parts got merged, the rest got removed from github. Let's open a new ticket if zack's stuff resurface.

Note: See TracTickets for help on using tickets.