Dynamic Port Forwarding #2261
Dynamic Port Forwarding #2261
Conversation
russjones
force-pushed
the
rjones/socks5
branch
from
3fe9802
to
01c2f45
Compare
lib/client/api.go
Outdated
| @@ -476,6 +512,11 @@ func (c *Config) LoadProfile(profileDir string, proxyName string) error { | |||
| log.Warnf("Error parsing user profile: %v", err) | |||
| } | |||
|
|
|||
| c.DynamicForwardedPorts, err = ParseDynamicPortForwardSpec(cp.DynamicForwardedPorts) | |||
| if err != nil { | |||
| log.Warnf("Error parsing user profile: %v", err) | |||
There was a problem hiding this comment.
missing dot. Also it's a warning, but log message says error what is not consistent.
lib/client/api.go
Outdated
| @@ -834,6 +875,15 @@ func (tc *TeleportClient) startPortForwarding(nodeClient *NodeClient) error { | |||
| go nodeClient.listenAndForward(socket, net.JoinHostPort(fp.DestHost, strconv.Itoa(fp.DestPort))) | |||
| } | |||
| } | |||
| if len(tc.Config.DynamicForwardedPorts) > 0 { | |||
| for _, fp := range tc.Config.DynamicForwardedPorts { | |||
| socket, err := net.Listen("tcp", net.JoinHostPort(fp.SrcIP, strconv.Itoa(fp.SrcPort))) | |||
There was a problem hiding this comment.
if you created 3 listeners and failed to bind on 4th listener, remaining 3 listeners will linger and leak? Can you make sure the code closes the listeners on client close or error return (I would use contexts)
There was a problem hiding this comment.
I think a better UX would be to just log that we failed to bind to a particular address but continue to bind to the ones that we can.
What do you think?
lib/client/api.go
Outdated
|
|
||
| // ToStringSpec returns the same string spec which can be parsed by | ||
| // ParseDynamicPortForwardSpec. | ||
| func (fp DynamicForwardedPorts) ToStringSpec() (retval []string) { |
lib/client/api.go
Outdated
|
|
||
| // ParseDynamicPortForwardSpec parses parameter to -D flag, i.e. strings like "remote.host:3000" | ||
| // The opposite of this function (spec generation) is DynamicForwardPorts.ToStringSpec. | ||
| func ParseDynamicPortForwardSpec(spec []string) (ports DynamicForwardedPorts, err error) { |
There was a problem hiding this comment.
let's not use named return values in go in larger functions without a good reason:
https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters
Naked returns are okay if the function is a handful of lines. Once it's a medium sized function, be explicit with your return values. Corollary: it's not worth it to name result parameters just because it enables you to use named returns. Clarity of docs is always more important than saving a line or two in your function.
lib/client/api.go
Outdated
| for i, str := range spec { | ||
| parts := strings.Split(str, ":") | ||
| if len(parts) > 2 { | ||
| return nil, fmt.Errorf(errTemplate, str) |
There was a problem hiding this comment.
replace fmt.Errorf with trace here and everywhere else. Error message should be compliant with our style (should not start with upper case and have dots, it's not a full sentence.)
lib/client/client.go
Outdated
| log.Debugf("SOCKS5 proxy forwarding requests to %v.", remoteAddr) | ||
|
|
||
| // Proxy the connection. | ||
| go c.proxyConnection(conn, remoteAddr) |
| limitations under the License. | ||
| */ | ||
|
|
||
| package socks |
There was a problem hiding this comment.
missing package level documentation
|
|
||
| "github.com/gravitational/teleport" | ||
|
|
||
| "github.com/gravitational/trace" |
There was a problem hiding this comment.
logrus and trace are both external deps, so extra line is not necessary
lib/utils/socks/socks.go
Outdated
| } | ||
| return int(len), nil | ||
| default: | ||
| return 0, trace.BadParameter("unsupported address type") |
There was a problem hiding this comment.
include the type that is not supported in the error
lib/utils/socks/socks.go
Outdated
| // Write the response to the client. Similar to OpenSSH only success is written. | ||
| // https://github.com/openssh/openssh-portable/blob/5d14019/channels.c#L1442-L1452 | ||
| func writeReply(conn net.Conn) error { | ||
| // Write success reply. |
There was a problem hiding this comment.
redundant comment line, it's clear what the function does
russjones
force-pushed
the
rjones/socks5
branch
4 times, most recently
from
feb062d
to
afe29b6
Compare
lib/client/client.go
Outdated
| log.Errorf("Connection attempt %v: %v", attempt, err) | ||
| // failed to establish an outbound connection? try again: | ||
| time.Sleep(time.Millisecond * time.Duration(100*attempt)) | ||
| func (client *NodeClient) proxyConnection(ctx context.Context, incoming net.Conn, remoteAddr string) { |
There was a problem hiding this comment.
I find it confusing to have two identical functions with similar names proxyConn and proxyConnection with only difference being the first one returning error vs. the other one not.
Just use the local closure instead of proxyConnection function.
lib/client/client.go
Outdated
| // and proxy them to 'remoteAddr' | ||
|
|
||
| // Proxy the connection to the remote address. | ||
| go c.proxyConnection(ctx, conn, remoteAddr) |
lib/client/client.go
Outdated
| log.Debugf("SOCKS5 proxy forwarding requests to %v.", remoteAddr) | ||
|
|
||
| // Proxy the connection. | ||
| go c.proxyConnection(ctx, conn, remoteAddr) |
russjones
force-pushed
the
rjones/socks5
branch
from
afe29b6
to
70aaa24
Compare
Implements the -D flag of ssh, but only with a SOCKS5 proxy. OpenSSH also supports a SOCKS4 proxy. This commit also fixes a bug in the server which prevented it from forwarding raw IPv6 sockets, as the addresses were incorrectly escaped.
and golang.org/x/net/proxy.
Purpose
Build off the work of @steven-aerts in #1694 and add support for dynamic port forwarding (SOCKS5).
Implementation
-Dflag to tsh.direct-tcpipchannel.Related Issues
Fixes #1693
Fixes #1694