Menu

#531 Tunneled connections

open
None
4
2007-08-11
2007-07-20
Bas Wijnen
No

This patch implements tunneled connections (or actually, it contains the code from the earlier patch, changed to apply to the new code).

There are a few functions which allow connections to be tunneled over other connections, instead of using a new tcp connect function. The good thing about this is that it is guaranteed to work: if you have a connection, you may not be able to create a new one (due to firewalls, for example), but you can use the one you have.

This patch doesn't actually use the functions yet. I have tested it previously, and it worked. This patch obviously doesn't break anything either, since no new code is actually called. I would like to have it included anyway, so that the functionality can be used later.

There has been some discussion about whether tunneling is acceptable, in particular for the meta-server (which should remain low-traffic). That discussion is not relevant for this patch, because it is easy to add an option to allow (or disallow) tunneling. That option must however be implemented in the parts that call these functions, so they are of no concern here.

Discussion

  • Bas Wijnen

    Bas Wijnen - 2007-07-20
     
  • Roland Clobus

    Roland Clobus - 2007-07-20

    Logged In: YES
    user_id=831677
    Originator: NO

    This patch contains a few times 'FIXME'. It is not ready yet?

     
  • Roland Clobus

    Roland Clobus - 2007-07-20
    • assigned_to: nobody --> shevek
     
  • Bas Wijnen

    Bas Wijnen - 2007-07-21
     
  • Bas Wijnen

    Bas Wijnen - 2007-07-21
    • priority: 5 --> 4
    • assigned_to: shevek --> nobody
     
  • Bas Wijnen

    Bas Wijnen - 2007-07-21

    Logged In: YES
    user_id=42389
    Originator: YES

    Thanks for reminding, I saw it and forgot it. :-)

    Those things are now also implemented: tunnels with duplicate ids (on the same parent) can never happen, and when a parent closes, all tunneled connections are also closed.

    It shouldn't have much effect yet, but there are some changes to the current code and there is no benefit for the user yet, so I suggest adding it after the release. I've lowered the priority for that reason.
    File Added: tunnel2.patch

     
  • Roland Clobus

    Roland Clobus - 2007-08-08
    • assigned_to: nobody --> shevek
     
  • Roland Clobus

    Roland Clobus - 2007-08-08

    Logged In: YES
    user_id=831677
    Originator: NO

    I've read the patch and have several questions:
    * network.h
    Session *parent, *prev, *next: Are you implementing your own version of GList?
    * network.c (net_write)
    I have trouble understanding the code in net_write: what do all those memcpy's do? Can't you use a g_strdup_printf instead?
    * network.c (read_ready)
    When bad formed data is sent, the while loop will not terminate. That could be a security bug.
    * network.c (net_new)
    No initialization for ses->prev is needed?
    * network.c (net_add)
    You use g_malloc instead of g_malloc0 as net_new does. Intentionally?
    * network.c (net_add_new)
    Instead of using a static variable, can't you always start at zero when looking for a usable id?
    * network.h (net_add and net_add_new)
    Which of the two would I normally use to create a tunneled connection? Could the other function become a file static function?

    Do you have a script/code snippet that allows me to see what this patch will actually do?

     
  • Bas Wijnen

    Bas Wijnen - 2007-08-11

    Logged In: YES
    user_id=42389
    Originator: YES

    Session *parent, *prev, *next: Yes, I suppose this is my own version of GList. I find creating linked lists so trivial that it doesn't usually occur to me to use helper functions for it. ;-) Given the rest of the code, it's probably a good idea to rewrite it to use GList though.

    net_write: I wrote this before I knew about g_strdup_printf. It will indeed become much more readable with that.

    read_ready: The loop will always terminate at end-of-string. In case of malformed data, an empty string (which is also malformed data) is sent to the state machine. That seems acceptable to me.

    In net_new, a normal connection is initialised. For normal connections, parent is NULL and next is the first child. prev is only used for tunneled connections. So it does indeed not need to be initialised there. However, this code will change anyway when adapting it to GList.

    net_add uses g_malloc because I didn't see a reason to use g_malloc0. Is there any? We don't want to implicitly initialise values that way IMO.

    net_add_new: I used a static variable for speed reasons (in the usual case, this is O(1) while starting from 0 is O(n)). However, speed is not actually a concern here at all, so I don't really mind starting from 0. That does have the benefit of shorter strings and thus slightly less network traffic.

    About net_add and net_add_new: Perhaps they should be named better. :-) To make a tunneled connection, both sides should first agree that the connection should be made. This can be done for example by a meta-server telling a server that it wants to tunnel a client connection. (For this, a new command is needed in the meta-server protocol.) To do this, the meta-server will call net_add_new. That will result in a session with an id. This id is sent in the request, "client-tunnel 3", for example. The server will then call net_add to complete the connection. It will use the id it received from the meta-server.

    How about net_tunnel_connect and net_tunnel_accept, to mirror the tcp/ip function names?

    I don't currently have code using the tunnels, but if you look at patch #1464509 (closed), you can see an example.

    I'll rewrite the patch and post a new version.

     
  • Bas Wijnen

    Bas Wijnen - 2007-08-11
     
  • Bas Wijnen

    Bas Wijnen - 2007-08-11

    Logged In: YES
    user_id=42389
    Originator: YES

    Here's the new version. It uses the GList and g_strdup_printf (which doesn't clean up things as much as I expected, but a bit anyway). I also changed the TUNNEL_CHAR to '=', which looks a bit like a tunnel. :-)

    I also renamed the functions to be called connect and accept.
    File Added: tunnel3.patch

     
  • Bas Wijnen

    Bas Wijnen - 2007-08-11
    • assigned_to: shevek --> rclobus
     

Log in to post a comment.