[patch] RFC: put server tunnel name in environment

[Alex Bligh]

Peter,

--On 21 September 2009 20:44:56 +0100 Alex Bligh <alex at alex.org.uk> wrote:

>> Alex Bligh wrote:
>>> Trying again, with the patch attached as text/plain
>>
>> The patch seems racy. What if a second tunnel request comes in
>> between the set_tunnel() and get_tunnel() for the first request?
>
> Is it possible to have more than one tunnel request per forked
> child?

I've been thinking some more about this.

I think it depends what you mean by "racy". There is no traditional
code race whereby the allocated string can be freed, as the
code concerned is one single-threaded process (as per the
reasons I explained before).

What can happen is the string could have the wrong content if the following
code flows were to happen:

1. Environment is set up (and interactive / non-interactive setup
   launched) before SSH_MSG_CHANNEL_OPEN / tun at openssh.com is received.
   Here do_setup_env() is called before server_request_tun(), and
   the environment string will be empty.

   Firstly, this doesn't happen in practice, at least when openssh
   is the client (and the tunnel extension seems to be openssh only),
   because the sessions are first sent up, then no-more-sessions comes
   in, then the interactive session / whatever launches.

   Secondly, once the interactive session is running, clearly its
   environment can't be altered by its parent anyway. So if SSH_TUNNEL
   is documented as "the tunnel opened prior to the / at the time of
   the start of the session", this would be accurate.

2. Multiple calls to server_request_tun() before the call to do_setup_env()
   call (i.e. requests to open multiple tunnels).

   This can't happen with an openssh client because the client does not
   support opening multiple tunnels, even when their IDs are specified.
   EG
      machine1# ssh -w 0:0 -w 1:1 machine2
   will not do what you might have thought. It will in fact only set up
   tun1 devices at either end (and not tun0 devices).

   Now the protocol definition says:

	OpenSSH supports layer 2 and layer 3 tunnelling via the
	"tun at openssh.com" channel type. This channel type supports
	forwarding of network packets with datagram boundaries intact
	between endpoints equipped with interfaces like the BSD tun(4)
	device. Tunnel forwarding channels are requested by the client with
	the following packet:

   It's unclear to me whether that last plural is implying more than one
   tunnel can be set up (in theory), or is merely talking in generic terms.
   My reading of the code suggests that the server would seem to support
   this, but the client doesn't.

   One fix for this problem would be merely to describe the environment
   variable as "the last tunnel set up prior to the interactive session".
   A better approach, I think, would be simply to concatenate tunnel names
   onto the string. This would also handle closures, as the client knows
   exactly how many tunnels it has successfully opened and closed, and
   if the server never removes any from the string, the client can simply
   count through open requests to reliably get the ID of the n'th tunnel
   that it opened. This would would be a simple change to tun_setname()
   in the patch to concatenate, rather than free.

Does that address your raciness concerns?

-- 
Alex Bligh