Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3472 closed task (fixed)

Implementing the pluggable-transport spec (managed proxies)

Reported by: asn Owned by: asn
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: sjmurdoch Actual Points:
Parent ID: #3591 Points:
Reviewer: Sponsor:

Description

This ticket is for the development of the managed proxies spec of 180-pluggable-transport.txt.

Child Tickets

Change History (17)

comment:1 Changed 8 years ago by asn

Alright, I put my current progress in a bug3472_prealpha branch in
git://gitorious.org/mytor/mytor.git.

Basically:
This can handle managed proxy torrc lines, it can talk the 180 spec and it can spawn managed proxies.
It can't use a consistent port for server proxies yet [1], it can't speak the external OR protocol, it can't use the "*" method, it can't understand optional SOCKS arguments and it can't do any fancy extra-info stuff.

Comments on the design logic are much appreciated.
(There are also a couple of "ASN" questions floating around that need answering)

To try it put something like this in your torrcbridgetransport:

ServerTransportPlugin dummy exec /tmp/obfsproxy --managed

Then wait 'till in your log appears something like:

[warn] Server transport dummy at 127.0.0.1:35071.

Then take that "35071" part and put it in your torrcclienttransport like this:

Bridge dummy 127.0.0.1:35071
ClientTransportPlugin dummy exec /tmp/obfsproxy --managed

comment:2 Changed 8 years ago by asn

[1]: Since there is no consistent port support, I'm reporting the port used in the logs for now, like explained in the "To try it ..." section

comment:3 Changed 8 years ago by asn

Parent ID: #3591

comment:4 Changed 8 years ago by nickm

The filenames "pluggable_transports.[ch]" are pretty long. Can we just have that be "transports.[ch]"

The argument-parsing code in parse_client_transport_line is O(N2). That's ugly. Also, instead of having it launch the transports, it should probably just populate a data structure; there should be a separate function to launch the transports. (Rationale: it's easier to test the parsing.)

Also, what is the point of "x = tor_strdup(foo); tor_free(foo);" ? Why not just "x = foo;" ?

In 810a7a5fa09734, the ST_prefix is too overloaded. Also, can the other code that uses tor_spawn_background be made to use your read-a-line function? It's best if we can avoid duplicated code there.

I'll look more at the actual code later.

comment:5 in reply to:  4 Changed 8 years ago by asn

Replying to nickm:

The filenames "pluggable_transports.[ch]" are pretty long. Can we just have that be "transports.[ch]"

Done. I kept the prefix of public functions to pt_. I'm not sure if this is nice.

The argument-parsing code in parse_client_transport_line is O(N2). That's ugly. Also, instead of having it launch the transports, it should probably just populate a data structure; there should be a separate function to launch the transports. (Rationale: it's easier to test the parsing.)

Also, what is the point of "x = tor_strdup(foo); tor_free(foo);" ? Why not just "x = foo;" ?

Alright, I stopped the smartlist_del_keeporder() business, and the code looks nicer than I expected.

As far as testing is concerned, I didn't do it with a data structure - that's how I originally wanted to do it - because I noticed that no other config.c parsing function returns a data structure. I also thought that validate_only turns it into a testable function (even though at the moment proxy_argv is prepared only when !validate_only; this is easy to change.).

In 810a7a5fa09734, the ST_prefix is too overloaded. Also, can the other code that uses tor_spawn_background be made to use your read-a-line function? It's best if we can avoid duplicated code there.

Done. I tested log_from_pipe() using obfsproxy and it seems to function as intended.

I'll look more at the actual code later.

comment:6 Changed 8 years ago by nickm

Status: newneeds_review

comment:7 Changed 8 years ago by nickm

Looks workable!

A few things to revise, either by you or by me:

  • there is still an assert() or two that should be tor_assert().
  • I think there are some c99isms that snuck in; try building with --enable-gcc-warnings
  • log_warn() is only for problems; never for things that are working as expected.
  • connection_uses_transport: I kinda want to have this look at a field that's set at connection-launch time, so that if the set of transports or bridges changes between when we launch the connection and when we check, we will still get the same answer every time we check.
  • Please double-check that the argv pointers we pass to pt_managed_launch_client_proxy actually get freed someplace? Maybe try running this whole thing under valgrind to look for leaks; see doc/HACKING for info how.
  • If I HUP tor to reload the configuration, do I wind up re-launching all the proxies? I didn't see the code that prevents this from happening.

Stuff that doesn't need to get fixed now, but should get an XXXX comment to note that we should fix it later:

  • The parse_client_transports and parse_server_transports() methods share a lot of code.

Some stuff I'll have to deal with:

  • The environment stuff and other changes in util.c will probably conflict with the changes Steven had to make to get the spawn-process code to work on Windows. That's my problem, though, not yours.

comment:8 Changed 8 years ago by sjmurdoch

Cc: sjmurdoch added

comment:9 Changed 8 years ago by nickm

See #3656; that's where the action on this branch seems to be these days. :/

comment:10 Changed 8 years ago by nickm

Status: needs_reviewassigned

Merged #3656, but I had to disable the body of launch_managed_proxy since porting it to use the new tor_spawn_background() interface seemed nontrivial; that will need a new patch.

comment:11 Changed 8 years ago by nickm

Also we'll need a changes file.

comment:12 Changed 8 years ago by asn

Alright, I have something in my bug3472_act2 branch.

I don't consider it mergeable in its vanilla form, because it's leaking fds when subprocesses exit. We need a function that "closes" process handles to fix this.

What I still want to improve:

  • I want Windows and Unix to manage the environment of the launched proxy in the same way. Right now, Unix creates an envp that is used by execve(), while Windows sets the environment variables in the tor process, launches the process and unsets the managed proxy environment afterwards. "Fixing" the Windows behavior is not hard (setting lpEnvironment in CreateProcess()), but I didn't have enough time to tinker with it.
  • I want to make that function that closes/frees process_handle_t (we are currently leaking fds when the subprocess exits.)
  • I want to unify the two configure_proxy() and log_from_handle() platform implementations.
  • I am not sure if I like exposing the process_handle_t internals in util.h. I'm not sure if I like placing launch_managed_proxy() in util.c either. I think I might end up some helper functions, like tor_get_process_stdout() etc.
  • I might want to make use of process_handle_t.status (even though managed_proxy_t.status is good enough too).
  • I need to make a changes file.

comment:13 Changed 8 years ago by asn

Second attempt of comment:12 since I fubared its format.

Alright, I have something in my bug3472_act2 branch.

I don't consider it mergeable in its vanilla form, because it's
leaking fds when subprocesses exit. We need a function that "closes"
process handles to fix this.

What I still want to improve:

  • I want Windows and Unix to manage the environment of the launched
    proxy in the same way. Right now, Unix creates an envp that is used by
    execve(), while Windows sets the environment variables in the tor
    process, launches the process and unsets the managed proxy environment
    afterwards. "Fixing" the Windows behavior is not hard (setting
    lpEnvironment in CreateProcess()), but I didn't have enough time to
    tinker with it.
  • I want to make that function that closes/frees process_handle_t (we
    are currently leaking fds when the subprocess exits.)
  • I want to unify the two configure_proxy() and log_from_handle()
    platform implementations.
  • I am not sure if I like exposing the process_handle_t internals in
    util.h. I'm not sure if I like placing launch_managed_proxy() in
    util.c either. I think I might end up some helper functions, like
    tor_get_process_stdout() etc.
  • I might want to make use of process_handle_t.status (even though
    managed_proxy_t.status is good enough too).
  • I need to make a changes file.

comment:14 Changed 8 years ago by asn

Status: assignedneeds_review

I think it's ready.

Check out branch bug3472_act2.

Some pointers:

  • I did not unify configure_proxy() and log_from_handle(). I'm hesitant on touching tor_read_all_handle() and making sure that nothing breaks on the tor_check_port_forwarding() side.
  • I tried to avoid poking into the guts of process_handle_t, but I couldn't avoid it when I wanted to access the stdout handle/pipe. The correct solution to this (and to the above) is to fix tor_read_all_handle and tor_read_all_from_process_stdout().
  • I ended up copying the whole tor environment to the managed proxy when on Windows. For some reason, libevent wouldn't create an event_base with the minimal TOR_PT_*/HOME/PATH environment on Windows. I didn't want to mess with libevent too much, and copying the whole environment is not too messy/bad so I left it like this.

Steven if you could take a peek at my changes (especially the changes on util.[ch] and launch_managed_proxy() in transports.c) it would be great.

Putting this in needs_review.

comment:15 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged!

I made some of the changes to the process_handle interface I've been threatening for a while.

Also, I made some unit tests that didn't work for me start working.

Any more here? If so please reopen.

comment:16 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:17 Changed 7 years ago by nickm

Component: Tor BridgeTor
Note: See TracTickets for help on using tickets.