Re: [PATCH] controlling remote port forwarding over control path - openssh

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

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
http://www.mindrot.org/mailman/listi...enssh-unix-dev