The Wayback Machine - https://web.archive.org/web/20201029135201/https://github.com/gliderlabs/ssh/pull/38
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

Support for local port forwarding #38

Merged
merged 3 commits into from Apr 28, 2017

Conversation

@notnoopci
Copy link
Contributor

@notnoopci notnoopci commented Apr 27, 2017

Permit use of local port forwarding (e.g. ssh -L 8080:example.com:80 ssh-server).

Due to our needs (at CircleCI) so far, I'm restricting this to local port forwarding. If it matches conventions or pattern, we can contribute remote port forwarding and SOCKS as well in a follow up PR.

Few considerations made to call out:

  • Defaults to disallowing port forwarding. With the library being used in contexts without providing shell, defaulting to allowing it exposes a security vector to existing library consumers. It also feels like the right default regardless IMO
  • I used verbose LocalPortForwarding name in the public API thinking it matches the high level nature of the library. Open for ideas there.
  • Not sure what the testing pattern here is - so could use feedback
@notnoopci notnoopci force-pushed the notnoopci:local-port-forwarding branch from c6c9ddd to 2a9c1a2 Apr 27, 2017
@@ -42,6 +42,9 @@ type PasswordHandler func(ctx Context, password string) bool
// PtyCallback is a hook for allowing PTY sessions.
type PtyCallback func(ctx Context, pty Pty) bool

// LocalPortForwardingCallback is a hook for allowing port forwarding
type LocalPortForwardingCallback func(ctx Context, destinationHost string, destinationPort uint32) bool

This comment has been minimized.

@notnoopci

notnoopci Apr 27, 2017
Author Contributor

I chose plain parameters here rather than a typed struct, mostly because I'm unhappy with the names - and it's easier to change parameter names than struct field names and I don't envision more parameters so far. Happy to change it if I get definitive names.

This comment has been minimized.

@progrium

progrium Apr 28, 2017
Contributor

How are you unhappy with the names? Are these what are used in the protocol spec?

@notnoopci notnoopci force-pushed the notnoopci:local-port-forwarding branch from 35aca29 to 2de60e8 Apr 27, 2017
var dialer net.Dialer
dconn, err := dialer.DialContext(ctx, "tcp", dest)
if err != nil {
newChan.Reject(gossh.ConnectionFailed, err.Error())

This comment has been minimized.

@notnoopci

notnoopci Apr 27, 2017
Author Contributor

The rejects reasons and messages are are approximate and don't match the behavior of OpenSSH server - don't see expected rejection reasons in the spec for these cases.

@progrium
Copy link
Contributor

@progrium progrium commented Apr 28, 2017

Overall this looks great. Simple and follows our conventions so far. I'd love to see some tests. Also we'd love to see remote forwarding and SOCKS, but keep in mind we'll probably change the public API a bit afterwards to simplify and would love to have your feedback in that process.

@notnoopci
Copy link
Contributor Author

@notnoopci notnoopci commented Apr 28, 2017

Updated with tests! Thanks!

Also, expect an attribution notice for CircleCI 2.0 very soon!

@progrium progrium merged commit 20a4547 into gliderlabs:master Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.