Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3656 closed defect (implemented)

Support spawning multiple protocols using the same managed proxy

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We currently can't spawn multiple protocols using a single managed proxy line.

This ticket tracks this effort.

For now, I have a spec patch in my protocol_180 torspec.git branch.
(git://gitorious.org/torspec/torspec.git)

Child Tickets

Change History (15)

comment:1 Changed 8 years ago by asn

Actually, check branch bug3656 in my torspec.git.

comment:2 Changed 8 years ago by asn

Status: newneeds_review

Alright, tor code can be found in my bug3656 branch in:
git://gitorious.org/mytor/mytor.git

I have obfsproxy code ready as well, but it's based on 1f022eac from zwol's more-protocol branch and I'll wait till #3613 gets resolved before pushing it.

(putting this in needs_review for spec and tor code)

comment:3 Changed 8 years ago by asn

Alright, obfsproxy code is merge ready as well.
It can be found at branch 'bug3656' in my obfsproxy.git.

comment:4 Changed 8 years ago by nickm

Component: Pluggable transportTor Bridge

comment:5 Changed 8 years ago by nickm

On the first patch here:

  • managed_proxy_has_argv/managed_proxy_get_by_argv/add_transport_to_proxy/get_bindaddr_for_proxy: constify if they're not already constified
  • smartlist_clear() before smartlist_free() is redundant.
  • would a comma-separated list of transports be easier than multiple lines here? I'm not sure the behavior is intuitive as it is. Is this how we designed it?
  • This design has gotten complicated enough that we could probably use a big comment someplace explaining the relationship between proxies, transports, and bridges; who manages which when; how stuff gets launched and checked; how stuff gets stopped; etc

In the future:

  • Please read your diffs as you make them; if they combine lots of cleanup stuff with new feature stuff with moving stuff around, see if you can do it in more diffs.
  • Please don't commit prints

General stuff applying to this entire branch series:

  • make check-spaces ; and make sure that --enable-gcc-warnings works
  • Have a look at the new functions added to .h files through this whole series. The ones that are only used in the file that declares them should be static functions in that file. The ones that are only used in the file that that declares them, plus in the unit tests, should use the #ifdef FOO_PRIVATE trick
  • Try this stuff under valgrind; there are enough new things allocated in one place and freed in another that I'm a little unconfident that I actually checked everything completely. (valgrind instructions in doc/HACKING are a must)

comment:6 Changed 8 years ago by asn

What I fixed:

  • 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
  • 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.
  • get_transport_in_state_by_name will fail badly if there are two transports A and B such that A's name is a prefix of B's.
  • You shouldn't need to be messing with this STRUCT_VAR_P stuff and config_find_option stuff if you know the name of the field you want to access -- just use C and say "get_or_state()->TransportProxies"
  • get_transport_bindaddr should probably check that the line actually starts with the name of the transport and a space.
  • When in doubt, prefer tor_addr_t
  • smartlist_clear() before smartlist_free() is redundant.
  • This design has gotten complicated enough that we could probably use a big comment someplace explaining the relationship between proxies, transports, and bridges; who manages which when; how stuff gets launched and checked; how stuff gets stopped; etc

What I questionably fixed:

  • log_warn() is only for problems; never for things that are working as expected.

I sanitized the logging severities. I didn't think about it too much and some of them might be spammy in real usage. I tried to keep LD_WARN to the minimum and mainly use LD_{INFO,NOTICE}.

  • managed_proxy_has_argv/managed_proxy_get_by_argv/add_transport_to_proxy/get_bindaddr_for_proxy: constify if they're not already constified.

I try to pay attention to constification and e8715b30 ended up being quite empty. Stuff like get_managed_proxy_by_argv_and_type()! that I would like to constify, give me a compiler warning if I do. (proxy_argv is not const in `pt_kickstart_proxy`).

  • make check-spaces ; and make sure that --enable-gcc-warnings works

I'm getting an: EOL@EOF:src/or/config.c:5995 warning in check-spaces. I tried to remove the EOL with three different editors and I couldn't. I gave up after a bit.

  • Have a look at the new functions added to .h files through this whole series. The ones that are only used in the file that declares them should be static functions in that file. The ones that are only used in the file that that declares them, plus in the unit tests, should use the #ifdef FOO_PRIVATE trick

I also try to pay attention to static functions and I couldn't find anything other than `get_transport_bindaddr()`. It wouldn't surprise me at all if I had forgotten a function or three though, so if you have anything in mind do say.

  • Try this stuff under valgrind; there are enough new things allocated in one place and freed in another that I'm a little unconfident that I actually checked everything completely. (valgrind instructions in doc/HACKING are a must)

Did that a hundred times with different torrc etc. I plugged all memleaks I managed to find, but some allocation are quite complex (and there are many edge cases as well) and I'm still afraid I might have left memleaks lying around...

  • Please don't commit prints

I always forget you are reviewing by reading diffs and I forget prints inside. Sorry. I killed all stray printf()s in 2e73f9b3.

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

Alright, this was a hard one. I had to hunt for edge cases all around the code and I didn't like it. I'm not very satisfied with the results but I couldn't find a better way to do it. It should detect all torrc modifications and react accordingly except when external and managed proxies are mixed together. More on this later.

What I didn't fix:

  • would a comma-separated list of transports be easier than multiple lines here? I'm not sure the behavior is intuitive as it is. Is this how we designed it?

It's true, comma-seperated transports will probably be more intuitive. It's in my TODO list.

  • validate_transports_in_state doesn't do what it's documented to do: It never returns -1

Hm, I'm not sure what I should be doing in the case state transports are not sane. I remember you saying that the state transports format should be more extensible and that we shouldn't trash the state for now. So, I'm just issuing an LD_WARN to the user and leaving the state as is.

  • For save_transport_to_state, would it be simpler to just clear the entire TransportProxies field, and re-add *every* transport?

I'm not sure, it could be simpler indeed. I didn't fix this because I would have to figure out when a transport is a server transport and `transport_t`s can't do that yet (very easy to implement thought).

I'm also semi-confident™ that the code logic is correct even thought it looks like spaggheti. This probably goes to the TODO list as well.

BUGS

There is a bug that I haven't fixed yet. When there is a transport that is supported by a managed proxy *and* an external proxy, the code will always register the external proxy version, even if the managed proxy is higher in the torrc.

This happens because managed proxies are configured in several `run_scheduled_events()` ticks, while external proxies are configured in parse_client_transport_line(). This means that external proxy transports hit the circuitbuild.c subsystem instantly, when managed proxies will conflict when they try to add their transports later on. Because of the first-come-first-served transport registration, external proxy transports will always be prefered over managed proxy transports.

I didn't fix this one because the fix will put even more edge case catching in the code and my heart is not prepared for that just yet. I would probably have to introduce 'priorities' to the transports based on their position in torrc. I would then have to respect that priority every time a transport tries to get registered in circuitbuild.c. That priority will also have to be respected in the case of a HUP, etc.

TODO

We probably have to introduce a LOG_PT and put the transports.c etc. logging in that domain.

We probably also have to introduce a unique identifier to every managed proxy so that logs become a bit more meaningful.

comment:7 Changed 8 years ago by asn

  1. By LOG_PT, I actually wanted to say LD_PT (as in Log Domain).
  2. A human readable managed proxy unique identifier could be the managed proxy filename (probably just the name of the binary, not the whole path), with some mangling if there are two identically named managed proxies in different paths.

comment:8 Changed 8 years ago by asn

Oh yeah another thing.
tor_terminate_process has *not* been tested in Microsoft Windows.

I'll revive my VM, try it and update this ticket when I do.

comment:9 in reply to:  6 Changed 8 years ago by nickm

Replying to asn:

What I fixed:

Thanks! I'll take another pass through.

I'm getting an: EOL@EOF:src/or/config.c:5995 warning in check-spaces. I tried to remove the EOL with three different editors and I couldn't. I gave up after a bit.

That warning means that there's supposed to be a blank line at the end of the file.

  • Try this stuff under valgrind; there are enough new things allocated in one place and freed in another that I'm a little unconfident that I actually checked everything completely. (valgrind instructions in doc/HACKING are a must)

Did that a hundred times with different torrc etc. I plugged all memleaks I managed to find, but some allocation are quite complex (and there are many edge cases as well) and I'm still afraid I might have left memleaks lying around...

Okay, we'll have to look more as we go.

The s

  • Please don't commit prints

I always forget you are reviewing by reading diffs and I forget prints inside. Sorry. I killed all stray printf()s in 2e73f9b3.

Even if I weren't reviewing by reading diffs, you still shouldn't commit prints. :)

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

Alright, this was a hard one. I had to hunt for edge cases all around the code and I didn't like it. I'm not very satisfied with the results but I couldn't find a better way to do it. It should detect all torrc modifications and react accordingly except when external and managed proxies are mixed together. More on this later.

I'll check it out. Let me know if you want any advice here.

What I didn't fix:

  • would a comma-separated list of transports be easier than multiple lines here? I'm not sure the behavior is intuitive as it is. Is this how we designed it?

It's true, comma-seperated transports will probably be more intuitive. It's in my TODO list.

Okay. I'd rather not merge the interface only to change it later: if we have one interface in a public release, we usually commit ourselves to keeping that interface working indefinitely. So if we fix it before merge, we don't have to keep the old thing working. I guess we could just label the interface as "unstable, will change" for an alpha or two if we really had to.

TODO

We probably have to introduce a LOG_PT and put the transports.c etc. logging in that domain.

LD_PT, you mean.

We probably also have to introduce a unique identifier to every managed proxy so that logs become a bit more meaningful.

Sounds smart.

comment:10 Changed 8 years ago by asn

I have an implementation of:

    would a comma-separated list of transports be easier than multiple lines here? I'm not sure the behavior is intuitive as it is. Is this how we designed it? 

in commit 96ca9e14 of the bug3656 branch.

I also made a branch mulprotos_single_line in my torspec.git to document this feature.

comment:11 in reply to:  8 Changed 8 years ago by asn

Replying to asn:

Oh yeah another thing.
tor_terminate_process has *not* been tested in Microsoft Windows.

I'll revive my VM, try it and update this ticket when I do.

I set to test tor_terminate_process() in Windows today, and then I realized that tor_spawn_background() does not work in Windows yet (using the old API), so I couldn't test it.

In any case, commit 3be9d76fa allows tor to compile on Windows again.

comment:12 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Hmmm.

I can no longer find anything clearly wrong here. If there are more problems, they'll need to come out in testing. Time to merge.

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.

Closing this ticket; bug 3472 is the most logical "turn spawn_background back on" place.

comment:13 Changed 8 years ago by nickm

Oh! Also, please review my merge carefully.

comment:14 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:15 Changed 7 years ago by nickm

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