openssh unix dev - Mar 2005 - [PATCH] controlling remote port forwarding over control path

Hi,

the attached patch implements adding and canceling of remote port
forwardings by communicating with a running ssh client via a control
socket.

Thus, one can do this:

  ssh -MNfS ~/.ssh/ctl remotehost

and then:

  ssh -S ~/.ssh/ctl -O add-rforward 2000:forward:80 localhost

to add a new remote forwarding or

  ssh -S ~/.ssh/ctl -O cancel-rforward localhost:2000 localhost

to remove it.

The patch is against openssh-SNAP-20050302 so the new fine tuned 
forwarding code is already contained.


While working on the patch a few questions/inconveniences have emerged:

1) why is mux_command in ssh.c not part of Options?

2) the current implementation allows -O to occur only once. So, to add 
or remove multiple channels ssh has to be called multiple times. Would 
it make sense to extend the code to allow it to occur multiple times?

  ssh -S ~/.ssh/ctl \
      -O add-rforward 2000:forward:80 \
      -O add-rforward 2001:forward:80 \
      localhost

3) permitted_opens in channels.c is a real problem. The current code 
allocates a new element from the end of this array while adding a new 
forwarding. But when the forwarding is cancelled the element is not 
really freed. It is marked somehow to be not in use but the current 
code cannot reuse it.

4) again permitted_opens. channel_request_rforward_cancel() identifies 
the local side of a forwarding only by 
permitted_opens[i].host_to_connect and permitted_opens[i].listen_port. 
Since a forwarding is really a quadruple this looks a little fragile to 
me. In fact you can try to remove a forwarding by specifying only a 
port number

  ssh -S ~/.ssh/ctl -O cancel-rforward 2000 localhost

This matches an element of permitted_opens and resets it but it does not 
match an open channel at the server side. So the listening socket is 
not closed. Now when someone tries to connect to that port the server 
forwards the connection to the client. Here it does not match an 
element of permitted_opens. Hence

  WARNING: Server requests forwarding for unknown listen_port 2000

is printed and the connection is closed.

I have not yet changed this behaviour because it is the same code that 
is used when adding or canceling forwardings with the ssh command line 
("~C", then "-R2000:forward:80", then "~C", then
"-KR2000" yields the
same result). But I think it's rather a bug than a feature.

Doesn't it make more sense to represent forwardings as quadruples 
(remotehost, remoteport, localhost, localport) also at the client side?

5) I think I have to implement -O add-lforward and -O cancel-lforward, 
too.

6) Also -O list-forwards would be useful, wouldn't it?

Torsten
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openssh.patch
Type: text/x-diff
Size: 5214 bytes
Desc: not available
Url :
http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20050304/d71166d1/attachment.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url :
http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20050304/d71166d1/attachment-0001.bin

Torsten Foertsch wrote:
> Hi,
> 
> the attached patch implements adding and canceling of remote port
> forwardings by communicating with a running ssh client via a control
> socket.
Cool, I was planning on doing something like this.

However, instead of a commandline like:

 >   ssh -S ~/.ssh/ctl -O add-rforward 2000:forward:80 localhost

Which is not very getopt()ish. Could I suggest:

ssh -S ~/.ssh/ctl -O add-rforward localhost 2000:forward:80

(i.e. place the forarding arguments where the command goes normally)

This has the advantage of being easier to extend to multiple forwarding
specifications:

ssh -S ~/.ssh/ctl -O add-rforward xxx 2222:host1:22 2223:host2:22 ...
> While working on the patch a few questions/inconveniences have emerged:
> 
> 1) why is mux_command in ssh.c not part of Options?
It didn't seem like a good place to put it - it is a runtime control
thing like -N or -f. What do you have in mind?
> 2) the current implementation allows -O to occur only once. So, to add 
> or remove multiple channels ssh has to be called multiple times. Would 
> it make sense to extend the code to allow it to occur multiple times?
See above.
> 3) permitted_opens in channels.c is a real problem. The current code 
> allocates a new element from the end of this array while adding a new 
> forwarding. But when the forwarding is cancelled the element is not 
> really freed. It is marked somehow to be not in use but the current 
> code cannot reuse it.
Yes, it is a small leak but, worse, it makes it easier to hit the
SSH_MAX_FORWARDS_PER_DIRECTION limit. Perhaps this list would be better
off wrapped in functions to add/delete/check entries.
> 4) again permitted_opens. channel_request_rforward_cancel() identifies 
> the local side of a forwarding only by 
> permitted_opens[i].host_to_connect and permitted_opens[i].listen_port. 
> Since a forwarding is really a quadruple this looks a little fragile to 
> me. In fact you can try to remove a forwarding by specifying only a 
> port number
> 
>   ssh -S ~/.ssh/ctl -O cancel-rforward 2000 localhost
> 
> This matches an element of permitted_opens and resets it but it does not 
> match an open channel at the server side. So the listening socket is 
> not closed. Now when someone tries to connect to that port the server 
> forwards the connection to the client. Here it does not match an 
> element of permitted_opens. Hence
> 
>   WARNING: Server requests forwarding for unknown listen_port 2000
> 
> is printed and the connection is closed.
> 
> I have not yet changed this behaviour because it is the same code that 
> is used when adding or canceling forwardings with the ssh command line 
> ("~C", then "-R2000:forward:80", then "~C",
then "-KR2000" yields the
> same result). But I think it's rather a bug than a feature.
> 
> Doesn't it make more sense to represent forwardings as quadruples 
> (remotehost, remoteport, localhost, localport) also at the client side?
There are a couple of things here:

1. Deleting remote forwardings by port makes sense unless you have them
bound to multiple addresses. I don't think it is necessary to use
quadruples to specify which remote forward to kill, because a remote
forward can be uniquely identified using just the bind_address and port.
Though maybe it is less confusing...

2. If we do wildcard deletes, then we should do them properly and kill
all the relevant channels. So the behaviour you see on channel delete is
a separate bug.
> 5) I think I have to implement -O add-lforward and -O cancel-lforward, 
> too.
That would be handy too :) Don't forget -O add-dforward :)
> 6) Also -O list-forwards would be useful, wouldn't it?
Yes.

Could you create a bug on http://bugzilla.mindrot.org and attach your
patch? We won't be able to get to this until after the imminent release,
but I would like to see something like this added.

Thanks,
Damien Miller