Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add reverse port forwarding #72

Merged
merged 11 commits into from
Dec 29, 2018
Merged

add reverse port forwarding #72

merged 11 commits into from
Dec 29, 2018

Conversation

sunshineco
Copy link
Contributor

@sunshineco sunshineco commented Dec 24, 2018

This patch series implements reverse port forwarding (sharing client ports to the server) which complements existing forwarding (sharing server ports to the client). This is analogous to how OpenSSH's -R complements -L port forwarding.

Reverse port forwarding remotes are specified as R:<local-interface>:<local-port>:<remote-host>:<remote-port> and are subject to --authfile restrictions, just like normal remotes. A new server-side --reverse option controls whether R: remotes are accepted by the server (they are rejected by default).

I implemented these changes in isolation before ever seeing #29, which requests reverse port forwarding, or github.com/mcbernie/chisel which forks chisel to add @client endpoint notation, neither of which has seen any activity in over a year. The reverse port forwarding introduced by this series, on the other hand, works out-of-the-box, is available today, and is implemented with minimally invasive changes.

When keep-alive is enabled, Client.loop() creates a go-routine to
provide the functionality, however, the go-routine never exits, not even
when the client's SSH connection to the server has been torn down, thus
leaking the go-routine.

This typically isn't a problem for the command-line chisel client since
it is single-use; the SSH connection is torn down only when the client
is terminating anyway. However, such a go-routine leak could become
problematic if the client ever learns to manage multiple SSH connections
(perhaps, say, via an interactive mode) or when chisel is used as a
library by some other program which manages multiple connections.
Therefore, ensure that the go-routine terminates when the SSH connection
is torn down.
Aside from minor dependencies upon chclient.Client, chclient.tcpProxy is
sufficiently general to handle TCP port forwarding beyond the client
itself. For instance, when chisel learns to support reverse port
forwarding, in which case the proxy will be listening on the server
rather than the client, it will make sense to re-use the existing proxy
code rather than duplicating it. Therefore, divorce the proxy code from
chclient.Client, freeing it up for use by the server too.
chclient.tcpProxy is sufficiently general to handle TCP port
forwarding for both client and server. Therefore, move it to share/ so
it can be re-used by the server when chisel learns to support reverse
port forwarding, in which case the proxy will be listening on the
server rather than the client as with normal port forwarding.
Once started, a TCPProxy never stops listening on the specified port.
This typically isn't a problem for the command-line chisel client since
the only time a proxy should stop listening is when the client itself
terminates. However, it will become a problem when chisel eventually
supports reverse port forwarding since the proxy will instead be running
on the server side, and the server will need to tear down those
listeners when the client disconnects. Therefore, teach TCPProxy to
shutdown via context.Context cancellation.
Presently, only the server maintains connection statistics, however,
when reverse port forwarding is eventually supported by chisel, the
client will also want to do so. Rather than duplicating this
functionality in the client, generalize the existing functionality as
chshare.ConnStats, allowing it to be used by both client and server.
Aside from a minor dependency upon chserver.Server, the existing
Server.handleTCPStream() is sufficiently general for either the server
or client side. For instance, when chisel learns to support reverse
port forwarding, in which case the client, not the server, will be
handling the TCP stream, it will make sense to re-use existing
functionality rather than duplicating it. Therefore, divorce the TCP
stream handling code from chserver.Server, freeing it up for use by
the client too.
server/handleTCPStream() is sufficiently general to handle TCP stream
functionality for both the client and server. Therefore, move it to
share/ so it can be re-used by the client when chisel learns to
support reverse port forwarding, in which case TCP stream handling
will be done by the client rather than the server.
An upcoming enhancement will add reverse port forwarding support (client
sharing its ports to the server) to complement the existing port
forwarding (server sharing its ports to the client). As a first step,
introduce notation for specifying a remote for reverse port forwarding;
i.e. "R:<local-interface>:<local-port>:<remote-host>:<remote-port>".

At this stage, reverse port forwarding remotes are recognized but never
actually created. A subsequent change will flesh out the functionality.
Normal port forwarding shares server ports to the client, allowing the
client to access ports on the server (or ports on other machines
accessible from the server). Sometimes, however, it is necessary to
share ports in the opposite direction, allowing the server to access
ports on the client (or ports on other machines accessible from the
client). Reverse port forwarding is analogous to ssh's -R forwarding
which complements normal -L forwarding.

Reverse port forwarding remotes are specified as
"R:<local-interface>:<local-port>:<remote-host>:<remote-port>", where
<local-interface> and <local-port> refer to the server side, and
<remote-host> and <remote-port> refer to the client side. For instance,
"R:2222:localhost:22" forwards port 2222 on the server to port 22 on the
client.
Although reverse port forwarding (sharing client ports with the server)
should not generally leak any resources from the server to the client,
the facility may nevertheless be abused if the client is able to open a
server port which is otherwise meant for some other purpose on the
server. (This might happen, for instance, if a service on the server has
crashed or becomes somehow disabled, thus freeing the port which would
otherwise be occupied by the service.)

To mitigate such potential abuse, disable reverse port forwarding by
default and introduce server option --reverse to enable it explicitly.
Additionally, subject reverse port forwarding remotes to server-side
--authfile restrictions (for instance, "^R:0.0.0.0:7000$").
@matti
Copy link

matti commented Dec 25, 2018

Add also support for dynamic remote port allocation like in SSH? (eg. port 0 and then print out the dynamically allocated port in log)

@sunshineco
Copy link
Contributor Author

Add also support for dynamic remote port allocation like in SSH? (eg. port 0 and then print out the dynamically allocated port in log)

It looks like this could be done without major surgery since the server spins up the reverse-port-forwarding proxies before shipping its reply back to the client upon connection establishment, which means that the server can report all the dynamically-allocated ports to the client without changing the protocol (i.e. adding another client/server exchange). One small issue is that the client presently interprets any payload shipped back as an error and aborts the connection attempt. This will need to change to allow the server to ship back dynamically-allocated port numbers as non-error payload. Not a big deal, but something to take into consideration.

Anyhow, such functionality can be implemented atop the current pull request rather than re-rolling (though, I may take a stab at re-rolling this one if it is not merged in a timely fashion).

@jpillora jpillora merged commit a11a3dd into jpillora:master Dec 29, 2018
jpillora added a commit that referenced this pull request Dec 29, 2018
… potential race, USR2 to print go stats, many small cleanups
@jpillora
Copy link
Owner

Merged :) Thanks @sunshineco, I do indeed prefer this PR over the other ones for its simplicity. There's still room to add client --name <id> and @client-id remotes, though I'd want a good chunk of time to merge existing implementations / rewrite it.

RE: Leaking go routine: So you run chisel via the Go API, and call client.Start a lot?

@sunshineco
Copy link
Contributor Author

RE: Leaking go routine: So you run chisel via the Go API, and call client.Start a lot?

No, not yet, but I had it in mind to use chisel as a library, was glad to see it was already structured as such (with main.go just a thin wrapper over the libified client and server code), so when I saw the leak, I plugged it.

@tijme
Copy link

tijme commented Feb 13, 2019

I would love to see dynamic reverse port forwarding. Maybe I'll take a shot at it myself. 😄

Nice pull request @sunshineco!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants