Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit385d58c

Browse files
authored
fix(agent/agentssh): allow remote forwarding a socket multiple times (#11631)
* fix(agent/agentssh): allow remote forwarding a socket multiple timesFixes#11198Fixescoder/customers#407
1 parent08b4eb3 commit385d58c

File tree

3 files changed

+190
-37
lines changed

3 files changed

+190
-37
lines changed

‎agent/agentssh/agentssh.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func NewServer(ctx context.Context, logger slog.Logger, prometheusRegistry *prom
9999
}
100100

101101
forwardHandler:=&ssh.ForwardedTCPHandler{}
102-
unixForwardHandler:=&forwardedUnixHandler{log:logger}
102+
unixForwardHandler:=newForwardedUnixHandler(logger)
103103

104104
metrics:=newSSHServerMetrics(prometheusRegistry)
105105
s:=&Server{

‎agent/agentssh/forward.go‎

Lines changed: 74 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,14 @@ package agentssh
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
7+
"io/fs"
68
"net"
79
"os"
810
"path/filepath"
911
"sync"
12+
"syscall"
1013

1114
"github.com/gliderlabs/ssh"
1215
gossh"golang.org/x/crypto/ssh"
@@ -33,22 +36,29 @@ type forwardedStreamLocalPayload struct {
3336
typeforwardedUnixHandlerstruct {
3437
sync.Mutex
3538
log slog.Logger
36-
forwardsmap[string]net.Listener
39+
forwardsmap[forwardKey]net.Listener
40+
}
41+
42+
typeforwardKeystruct {
43+
sessionIDstring
44+
addrstring
45+
}
46+
47+
funcnewForwardedUnixHandler(log slog.Logger)*forwardedUnixHandler {
48+
return&forwardedUnixHandler{
49+
log:log,
50+
forwards:make(map[forwardKey]net.Listener),
51+
}
3752
}
3853

3954
func (h*forwardedUnixHandler)HandleSSHRequest(ctx ssh.Context,_*ssh.Server,req*gossh.Request) (bool, []byte) {
4055
h.log.Debug(ctx,"handling SSH unix forward")
41-
h.Lock()
42-
ifh.forwards==nil {
43-
h.forwards=make(map[string]net.Listener)
44-
}
45-
h.Unlock()
4656
conn,ok:=ctx.Value(ssh.ContextKeyConn).(*gossh.ServerConn)
4757
if!ok {
4858
h.log.Warn(ctx,"SSH unix forward request from client with no gossh connection")
4959
returnfalse,nil
5060
}
51-
log:=h.log.With(slog.F("remote_addr",conn.RemoteAddr()))
61+
log:=h.log.With(slog.F("session_id",ctx.SessionID()),slog.F("remote_addr",conn.RemoteAddr()))
5262

5363
switchreq.Type {
5464
case"streamlocal-forward@openssh.com":
@@ -62,14 +72,22 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
6272
addr:=reqPayload.SocketPath
6373
log=log.With(slog.F("socket_path",addr))
6474
log.Debug(ctx,"request begin SSH unix forward")
75+
76+
key:=forwardKey{
77+
sessionID:ctx.SessionID(),
78+
addr:addr,
79+
}
80+
6581
h.Lock()
66-
_,ok:=h.forwards[addr]
82+
_,ok:=h.forwards[key]
6783
h.Unlock()
6884
ifok {
69-
log.Warn(ctx,"SSH unix forward request for socket path that is already being forwarded (maybe to another client?)",
70-
slog.F("socket_path",addr),
71-
)
72-
returnfalse,nil
85+
// In cases where `ExitOnForwardFailure=yes` is set, returning false
86+
// here will cause the connection to be closed. To avoid this, and
87+
// to match OpenSSH behavior, we silently ignore the second forward
88+
// request.
89+
log.Warn(ctx,"SSH unix forward request for socket path that is already being forwarded on this session, ignoring")
90+
returntrue,nil
7391
}
7492

7593
// Create socket parent dir if not exists.
@@ -83,12 +101,20 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
83101
returnfalse,nil
84102
}
85103

86-
ln,err:=net.Listen("unix",addr)
104+
// Remove existing socket if it exists. We do not use os.Remove() here
105+
// so that directories are kept. Note that it's possible that we will
106+
// overwrite a regular file here. Both of these behaviors match OpenSSH,
107+
// however, which is why we unlink.
108+
err=unlink(addr)
109+
iferr!=nil&&!errors.Is(err,fs.ErrNotExist) {
110+
log.Warn(ctx,"remove existing socket for SSH unix forward request",slog.Error(err))
111+
returnfalse,nil
112+
}
113+
114+
lc:=&net.ListenConfig{}
115+
ln,err:=lc.Listen(ctx,"unix",addr)
87116
iferr!=nil {
88-
log.Warn(ctx,"listen on Unix socket for SSH unix forward request",
89-
slog.F("socket_path",addr),
90-
slog.Error(err),
91-
)
117+
log.Warn(ctx,"listen on Unix socket for SSH unix forward request",slog.Error(err))
92118
returnfalse,nil
93119
}
94120
log.Debug(ctx,"SSH unix forward listening on socket")
@@ -99,7 +125,7 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
99125
//
100126
// This is also what the upstream TCP version of this code does.
101127
h.Lock()
102-
h.forwards[addr]=ln
128+
h.forwards[key]=ln
103129
h.Unlock()
104130
log.Debug(ctx,"SSH unix forward added to cache")
105131

@@ -115,9 +141,7 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
115141
c,err:=ln.Accept()
116142
iferr!=nil {
117143
if!xerrors.Is(err,net.ErrClosed) {
118-
log.Warn(ctx,"accept on local Unix socket for SSH unix forward request",
119-
slog.Error(err),
120-
)
144+
log.Warn(ctx,"accept on local Unix socket for SSH unix forward request",slog.Error(err))
121145
}
122146
// closed below
123147
log.Debug(ctx,"SSH unix forward listener closed")
@@ -131,10 +155,7 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
131155
gofunc() {
132156
ch,reqs,err:=conn.OpenChannel("forwarded-streamlocal@openssh.com",payload)
133157
iferr!=nil {
134-
h.log.Warn(ctx,"open SSH unix forward channel to client",
135-
slog.F("socket_path",addr),
136-
slog.Error(err),
137-
)
158+
h.log.Warn(ctx,"open SSH unix forward channel to client",slog.Error(err))
138159
_=c.Close()
139160
return
140161
}
@@ -144,12 +165,11 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
144165
}
145166

146167
h.Lock()
147-
ln2,ok:=h.forwards[addr]
148-
ifok&&ln2==ln {
149-
delete(h.forwards,addr)
168+
ifln2,ok:=h.forwards[key];ok&&ln2==ln {
169+
delete(h.forwards,key)
150170
}
151171
h.Unlock()
152-
log.Debug(ctx,"SSH unix forward listener removed from cache",slog.F("path",addr))
172+
log.Debug(ctx,"SSH unix forward listener removed from cache")
153173
_=ln.Close()
154174
}()
155175

@@ -162,13 +182,22 @@ func (h *forwardedUnixHandler) HandleSSHRequest(ctx ssh.Context, _ *ssh.Server,
162182
h.log.Warn(ctx,"parse cancel-streamlocal-forward@openssh.com (SSH unix forward) request payload from client",slog.Error(err))
163183
returnfalse,nil
164184
}
165-
log.Debug(ctx,"request to cancel SSH unix forward",slog.F("path",reqPayload.SocketPath))
185+
log.Debug(ctx,"request to cancel SSH unix forward",slog.F("socket_path",reqPayload.SocketPath))
186+
187+
key:=forwardKey{
188+
sessionID:ctx.SessionID(),
189+
addr:reqPayload.SocketPath,
190+
}
191+
166192
h.Lock()
167-
ln,ok:=h.forwards[reqPayload.SocketPath]
193+
ln,ok:=h.forwards[key]
194+
delete(h.forwards,key)
168195
h.Unlock()
169-
ifok {
170-
_=ln.Close()
196+
if!ok {
197+
log.Warn(ctx,"SSH unix forward not found in cache")
198+
returntrue,nil
171199
}
200+
_=ln.Close()
172201
returntrue,nil
173202

174203
default:
@@ -209,3 +238,15 @@ func directStreamLocalHandler(_ *ssh.Server, _ *gossh.ServerConn, newChan gossh.
209238

210239
Bicopy(ctx,ch,dconn)
211240
}
241+
242+
// unlink removes files and unlike os.Remove, directories are kept.
243+
funcunlink(pathstring)error {
244+
// Ignore EINTR like os.Remove, see ignoringEINTR in os/file_posix.go
245+
// for more details.
246+
for {
247+
err:=syscall.Unlink(path)
248+
if!errors.Is(err,syscall.EINTR) {
249+
returnerr
250+
}
251+
}
252+
}

‎cli/ssh_test.go‎

Lines changed: 115 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,14 @@ import (
2626
"github.com/stretchr/testify/require"
2727
"golang.org/x/crypto/ssh"
2828
gosshagent"golang.org/x/crypto/ssh/agent"
29+
"golang.org/x/sync/errgroup"
2930
"golang.org/x/xerrors"
3031

3132
"cdr.dev/slog"
3233
"cdr.dev/slog/sloggers/slogtest"
3334

3435
"github.com/coder/coder/v2/agent"
36+
"github.com/coder/coder/v2/agent/agentssh"
3537
"github.com/coder/coder/v2/agent/agenttest"
3638
"github.com/coder/coder/v2/cli/clitest"
3739
"github.com/coder/coder/v2/cli/cliui"
@@ -738,8 +740,8 @@ func TestSSH(t *testing.T) {
738740
defercancel()
739741

740742
tmpdir:=tempDirUnixSocket(t)
741-
agentSock:=filepath.Join(tmpdir,"agent.sock")
742-
l,err:=net.Listen("unix",agentSock)
743+
localSock:=filepath.Join(tmpdir,"local.sock")
744+
l,err:=net.Listen("unix",localSock)
743745
require.NoError(t,err)
744746
deferl.Close()
745747
remoteSock:=filepath.Join(tmpdir,"remote.sock")
@@ -748,7 +750,7 @@ func TestSSH(t *testing.T) {
748750
"ssh",
749751
workspace.Name,
750752
"--remote-forward",
751-
fmt.Sprintf("%s:%s",remoteSock,agentSock),
753+
fmt.Sprintf("%s:%s",remoteSock,localSock),
752754
)
753755
clitest.SetupConfig(t,client,root)
754756
pty:=ptytest.New(t).Attach(inv)
@@ -771,6 +773,116 @@ func TestSSH(t *testing.T) {
771773
<-cmdDone
772774
})
773775

776+
// Test that we can forward a local unix socket to a remote unix socket and
777+
// that new SSH sessions take over the socket without closing active socket
778+
// connections.
779+
t.Run("RemoteForwardUnixSocketMultipleSessionsOverwrite",func(t*testing.T) {
780+
ifruntime.GOOS=="windows" {
781+
t.Skip("Test not supported on windows")
782+
}
783+
784+
t.Parallel()
785+
786+
client,workspace,agentToken:=setupWorkspaceForAgent(t)
787+
788+
_=agenttest.New(t,client.URL,agentToken)
789+
coderdtest.AwaitWorkspaceAgents(t,client,workspace.ID)
790+
791+
// Wait super super long so this doesn't flake on -race test.
792+
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitSuperLong*2)
793+
defercancel()
794+
795+
tmpdir:=tempDirUnixSocket(t)
796+
797+
localSock:=filepath.Join(tmpdir,"local.sock")
798+
l,err:=net.Listen("unix",localSock)
799+
require.NoError(t,err)
800+
deferl.Close()
801+
testutil.Go(t,func() {
802+
for {
803+
fd,err:=l.Accept()
804+
iferr!=nil {
805+
if!errors.Is(err,net.ErrClosed) {
806+
assert.NoError(t,err,"listener accept failed")
807+
}
808+
return
809+
}
810+
811+
testutil.Go(t,func() {
812+
deferfd.Close()
813+
agentssh.Bicopy(ctx,fd,fd)
814+
})
815+
}
816+
})
817+
818+
remoteSock:=filepath.Join(tmpdir,"remote.sock")
819+
820+
vardone []func()error
821+
fori:=0;i<2;i++ {
822+
id:=fmt.Sprintf("ssh-%d",i)
823+
inv,root:=clitest.New(t,
824+
"ssh",
825+
workspace.Name,
826+
"--remote-forward",
827+
fmt.Sprintf("%s:%s",remoteSock,localSock),
828+
)
829+
inv.Logger=inv.Logger.Named(id)
830+
clitest.SetupConfig(t,client,root)
831+
pty:=ptytest.New(t).Attach(inv)
832+
inv.Stderr=pty.Output()
833+
cmdDone:=tGo(t,func() {
834+
err:=inv.WithContext(ctx).Run()
835+
assert.NoError(t,err,"ssh command failed: %s",id)
836+
})
837+
838+
// Since something was output, it should be safe to write input.
839+
// This could show a prompt or "running startup scripts", so it's
840+
// not indicative of the SSH connection being ready.
841+
_=pty.Peek(ctx,1)
842+
843+
// Ensure the SSH connection is ready by testing the shell
844+
// input/output.
845+
pty.WriteLine("echo ping' 'pong")
846+
pty.ExpectMatchContext(ctx,"ping pong")
847+
848+
d:=&net.Dialer{}
849+
fd,err:=d.DialContext(ctx,"unix",remoteSock)
850+
require.NoError(t,err,id)
851+
852+
// Ping / pong to ensure the socket is working.
853+
_,err=fd.Write([]byte("hello world"))
854+
require.NoError(t,err,id)
855+
856+
buf:=make([]byte,11)
857+
_,err=fd.Read(buf)
858+
require.NoError(t,err,id)
859+
require.Equal(t,"hello world",string(buf),id)
860+
861+
done=append(done,func()error {
862+
// Redo ping / pong to ensure that the socket
863+
// connections still work.
864+
_,err:=fd.Write([]byte("hello world"))
865+
assert.NoError(t,err,id)
866+
867+
buf:=make([]byte,11)
868+
_,err=fd.Read(buf)
869+
assert.NoError(t,err,id)
870+
assert.Equal(t,"hello world",string(buf),id)
871+
872+
pty.WriteLine("exit")
873+
<-cmdDone
874+
returnnil
875+
})
876+
}
877+
878+
vareg errgroup.Group
879+
for_,d:=rangedone {
880+
eg.Go(d)
881+
}
882+
err=eg.Wait()
883+
require.NoError(t,err)
884+
})
885+
774886
t.Run("FileLogging",func(t*testing.T) {
775887
t.Parallel()
776888

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp