-
Notifications
You must be signed in to change notification settings - Fork 450
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
feat: add Unix forwarding server implementations #196
base: master
Are you sure you want to change the base?
Conversation
@henrybarreto, could you take a look at this to see if it makes sense to support it in @shellhub-io? |
I'd love to see this PR merged in so that Tailscale can support it on their end. Since they want to start moving back to the original copy of this repo, this PR is now blocking support for stuff like GPG agent forwarding through Tailscale SSH. See tailscale/tailscale#12081 (comment). While I was working on my PR over at Tailscale, I made a couple changes to @@ -128,12 +127,27 @@ func (h *ForwardedUnixHandler) HandleSSHRequest(ctx Context, srv *Server, req *g
return false, nil
}
+ // https://github.com/coder/coder/blob/main/agent/agentssh/forward.go
+ // Remove existing socket if it exists. We do not use os.Remove() here
+ // so that directories are kept. Note that it's possible that we will
+ // overwrite a regular file here. Both of these behaviors match OpenSSH,
+ // however, which is why we unlink.
+ err = unlink(addr)
+ if err != nil && !errors.Is(err, fs.ErrNotExist) {
+ // TODO: log
+ return false, nil
+ }
+
ln, err := net.Listen("unix", addr)
if err != nil {
// TODO: log unix listen failure
return false, nil
}
+ if err := os.Chmod(addr, os.FileMode(0777)); err != nil {
+ // TODO: log permission change failure
+ return false, nil
+ }
+
// The listener needs to successfully start before it can be added to
// the map, so we don't have to worry about checking for an existing
// listener as you can't listen on the same socket twice.
@@ -202,3 +216,15 @@ func (h *ForwardedUnixHandler) HandleSSHRequest(ctx Context, srv *Server, req *g
return false, nil
}
}
+
+// https://github.com/coder/coder/blob/main/agent/agentssh/forward.go
+// unlink removes files and unlike os.Remove, directories are kept.
+func unlink(path string) error {
+ // Ignore EINTR like os.Remove, see ignoringEINTR in os/file_posix.go
+ // for more details.
+ for {
+ err := syscall.Unlink(path)
+ if !errors.Is(err, syscall.EINTR) {
+ return err
+ }
+ }
+} |
Before we can merge, we'll need to resolve the conflict. Could you please take a look at it? |
836adc0
to
07e1ffa
Compare
@Xenfo I applied your patch minus the chmod thing. I don't think it's a good default to chmod to 777. |
Yeah I had also brought that concern up in the Tailscale PR. I'm not really sure what could be done better here since I'm not too familiar with the library or proper permissions. Ideally they would be set so that user that SSHed is able to use it. If there isn't a better default permission, it's essential that there's some way to set the permissions (maybe through a separate function that can be called by the library user?) for Tailscale since it runs as root. Any user not SSHing as root would not be able to access the sockets that are forwarded. |
In my opinion, this responsibility should lie with the user of the |
I'll make some changes later today. |
Hey @deansheather if you need any help I'm willing to give this a go. @gustavosbarreto is this what you're suggesting? ReverseUnixForwardingCallback: ssh.ReverseUnixForwardingCallback(func(ctx Context, socketPath string) net.Listener {
ln, err := net.Listen("unix", addr)
if err != nil {
return nil
}
if err := os.Chmod(addr, os.FileMode(0777)); err != nil {
return nil
}
return ln
}), If so then how would the check for allowing reverse forwarding be done? If we check if the callback returns |
I forgot to do it. If you want to do it, go ahead, but please keep my details in co-authored-by |
I opened deansheather#1 which implements it for remote forwarding, I'm unsure if this is also necessary for local forwarding. Lmk if the double callback looks fine. |
f86b780
to
222d4f7
Compare
Thanks for your changes @samchouse @gustavosbarreto the reverse forwarding callback now looks like: // ReverseUnixForwardingCallback is a hook for allowing reverse unix forwarding
// (streamlocal-forward@openssh.com). Returning ErrRejected will reject the
// request.
type ReverseUnixForwardingCallback func(ctx Context, socketPath string) (net.Listener, error) |
Looking for this functionality very much also |
I've given the PR a quick read - it all looks good to me, even though I don't know the specifics of how forwarding works with openssh, this all seems fairly logical. I see there were changes pushed last month. Would you say this is ready to merge as-is, or are there any leftover changes you'd like to make first? I'd be happy to merge this when I get the go-ahead from you, @deansheather. |
I think it's all ready to go |
Thanks @deansheather, am looking for this functionality as well to enable local->remote Yubikey GPG signing w/ Tailscale SSH |
I wonder if maybe the Forward variant of the unix socket code should also require that the callback returns a |
I've been looking around https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL?annotate=HEAD, section 2.4 ("connection: Unix domain socket forwarding") and I'm a bit confused. Direct is a channel request, streamlocal is a global request, but it's a little unclear how they're supposed to work: I assume And
|
The SSH implementation is very similar to local and reverse TCP forwarding and this implementation was based off the existing TCP implementation.
Reverse forwarding is for forwarding client connections that originate from a new unix socket on the server to some listener on the client. Typically used to allow programs like curl running on the server to dial a socket on the client machine. In the original implementation of this PR the callback just returned
If we change the callback to return a net.Conn it can be used to e.g. dial a different socket path or emulate a fake connection (e.g. you could handle the request in process without creating a real socket). I would propose changing the API to: // LocalUnixForwardingCallback is a hook for allowing unix forwarding
// (direct-streamlocal@openssh.com). Returning ErrRejected will reject the request.
type LocalUnixForwardingCallback func(ctx Context, socketPath string) (net.Conn, error) |
Thanks for taking the time to respond and clear that up - I think that change makes sense. Let me know when it's ready and I can merge it. :) |
f9faa02
to
d54a674
Compare
Adds optional (disabled by default) implementations of local->remote and remote->local Unix forwarding through OpenSSH's protocol extensions: - streamlocal-forward@openssh.com - cancel-streamlocal-forward@openssh.com - forwarded-streamlocal@openssh.com - direct-streamlocal@openssh.com Adds tests for Unix forwarding, reverse Unix forwarding and reverse TCP forwarding. Co-authored-by: Samuel Corsi-House <chouse.samuel@gmail.com>
d54a674
to
00e25e8
Compare
I've made the changes. I'd also like to get someone from my company to review it first since we've made some changes to our version of this code in the last 22 odd months 🤣 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, I especially like the new abstraction with callbacks for allowing custom handling of the connection.
My only nit is unix request handling which doesn't currently allow for our use-case in coder/coder
(the SimpleUnixReverseForwardingCallback
function does, but the handler doesn't).
I also think documentation on how to enable this would be good to add either here or a follow-up PR, essentially you need to both set RequestHandlers and callback functions, which may be a bit confusing to users. Since the default is reject, would it make sense to add these request handlers to DefaultRequestHandlers
?
addr := reqPayload.SocketPath | ||
h.Lock() | ||
_, ok := h.forwards[addr] | ||
h.Unlock() | ||
if ok { | ||
// TODO: log failure | ||
return false, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Granted, we haven't introduced a similar option to SSH StreamLocalBindUnlink
here, and the default is no
for OpenSSH, but in coder/coder
we want its yes
-behavior. (And my personal opinion is that it's the more useful default.)
If yes, the same socket can be forwarded by multiple sessions, in this case each session should maintain the connections that were opened while it was active. Otherwise the following scenario can't be supported:
- Open SSH connection (a) with forwarded socket (
/tmp/my.sock
) - (a) Open
/tmp/my.sock
- Open SSH connection (b)
- (b) Open
/tmp/my.sock
- Open SSH connection (c) with forwarded socket (
/tmp/my.sock
, overwrite) - (c) Open
/tmp/my.sock
- Close connection (a)
- (a) closed (b) Socket closed (c) Socket remains open
In the current implementation, (c) socket can't remain open as the socket wasn't overwritten.
Also: Consider returning true
here instead as a default (see motivation in below link).
return nil, fmt.Errorf("failed to remove existing file in socket path %q: %w", socketPath, err) | ||
} | ||
|
||
ln, err := net.Listen("unix", socketPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use context aware dialer in the other callback, should we use (&net.ListenConfig{}).Listen(ctx, "unix", addr)
here as well?
Adds optional (disabled by default) implementations of local->remote and remote->local Unix forwarding through OpenSSH's protocol extensions:
Adds tests for Unix forwarding, reverse Unix forwarding and reverse TCP forwarding.
I recently had to write this code for my job so we could support GPG forwarding and couldn't find an implementation here so I thought I'd contribute it upstream so others can use it easily.