Skip to content

Commit 5ed0afd

Browse files
authored
refactor(ssh): redesign SSH client with improved resource management and security (oomol-lab#75)
This commit completely redesigns the SSH client implementation with better architecture, resource management, and security features. ## Key Changes ### SSH Package Redesign - Implement layered architecture: Client -> Session -> Executor - Add proper lifecycle management with sync.Once and RWMutex - Introduce dedicated config types (ClientConfig, SessionConfig, ExecOptions) - Fix race conditions in keepalive loop by using local variable copies - Resolve "already closed" connection errors with isErrorIsConnectionAlreadyClosed() - Add comprehensive package documentation (doc.go) ### Security Improvements - Use shellescape.QuoteCommand() to prevent command injection vulnerabilities - Properly escape shell arguments in command execution ### Resource Management - Add automatic keepalive with configurable intervals (default: 5s) - Implement proper defer-based cleanup for all resources - Fix pipe management: close writers (not readers) to signal EOF correctly - Add WaitGroup-based I/O synchronization to prevent goroutine leaks ### Terminal Handling (pkg/system/terminal.go) - Fix signal handler leak by adding defer signal.Stop(ch) - Improve terminal state management with wrapper types - Add terminal type and size auto-detection - Enhance PTY resize handling with proper context cancellation ### API Changes - Migrate from old monolithic ssh.Cfg to layered Client/Session/Executor - Remove manual cleanup callbacks in favor of defer-based patterns - cmd/attach.go: Refactor to use new SSH API with proper PTY/non-PTY handling - pkg/server/exec.go: Fix resource management by closing client in goroutine - pkg/vmconfig: Update SSH key generation and connection probing ### Dependencies - Add al.essio.dev/pkg/shellescape for safe command escaping - Add github.com/google/shlex for command parsing - Add github.com/mattn/go-isatty for reliable terminal detection ### Miscellaneous - Improve server mode logging with String() method - Update libkrun bindings (libkrun_v2.go replaces libkrun.go) - Remove unused install_name_tool.sh script line ## Testing - Verified attach command works in both PTY and non-PTY modes - Confirmed REST API exec functionality - Tested SSH connection probing during VM startup - Validated proper resource cleanup under various scenarios
1 parent 754f1cd commit 5ed0afd

File tree

19 files changed

+2627
-748
lines changed

19 files changed

+2627
-748
lines changed

cmd/attach.go

Lines changed: 61 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"path/filepath"
1313

1414
"github.com/urfave/cli/v3"
15+
gossh "golang.org/x/crypto/ssh"
1516
)
1617

1718
var AttachConsole = cli.Command{
@@ -30,63 +31,90 @@ var AttachConsole = cli.Command{
3031
},
3132
}
3233

33-
func attachConsole(ctx context.Context, command *cli.Command) error {
34+
func attachConsole(ctx context.Context, command *cli.Command) (err error) {
35+
// Parse and validate arguments
3436
rootfs := command.Args().First()
3537
if rootfs == "" {
3638
return fmt.Errorf("no rootfs specified, please provide the rootfs path, e.g. %s /path/to/rootfs", command.Name)
3739
}
3840

41+
if command.Args().Len() < 2 {
42+
return fmt.Errorf("no cmdline specified, please provide the cmdline, e.g. %s /path/to/rootfs /bin/bash", command.Name)
43+
}
44+
45+
// Load VM configuration
3946
vmc, err := define.LoadVMCFromFile(filepath.Join(rootfs, define.VMConfigFile))
4047
if err != nil {
4148
return err
4249
}
4350

51+
// Extract command line arguments
52+
cmdline := command.Args().Tail()
53+
54+
// Parse gvproxy endpoint
4455
endpoint, err := url.Parse(vmc.GVproxyEndpoint)
4556
if err != nil {
4657
return fmt.Errorf("failed to parse gvproxy endpoint: %w", err)
4758
}
4859

49-
if command.Args().Len() < 2 {
50-
return fmt.Errorf("no cmdline specified, please provide the cmdline, e.g. %s /path/to/rootfs /bin/bash", command.Name)
51-
}
52-
53-
cmdline := command.Args().Tail()
54-
55-
// First we maka a ssh client configure, remember it just a configure store the information the ssh client actually needed
56-
cfg := ssh.NewCfg(define.DefaultGuestAddr, define.DefaultGuestUser, define.DefaultGuestSSHDPort, vmc.SSHInfo.HostSSHKeyPairFile)
57-
defer cfg.CleanUp.CleanIfErr(&err)
60+
// Configure SSH client
61+
clientCfg := ssh.NewClientConfig(
62+
define.DefaultGuestAddr,
63+
uint16(define.DefaultGuestSSHDPort),
64+
define.DefaultGuestUser,
65+
vmc.SSHInfo.HostSSHKeyPairFile,
66+
).WithGVProxySocket(endpoint.Path)
5867

59-
// Set the cmdline
60-
cfg.SetCmdLine(cmdline[0], cmdline[1:])
61-
62-
if command.Bool(define.FlagPTY) {
63-
cfg.SetPty(true)
68+
// Create SSH client
69+
client, err := ssh.NewClient(ctx, clientCfg)
70+
if err != nil {
71+
return fmt.Errorf("failed to connect to %s: %w", endpoint.Path, err)
6472
}
73+
defer client.Close()
6574

66-
// Connect to the ssh server over gvproxy vsock, we use the gvproxy endpoint to connect to the ssh server
67-
if err = cfg.Connect(ctx, endpoint.Path); err != nil {
68-
return fmt.Errorf("failed to connect to %s: %w", endpoint.Path, err)
75+
// Create session
76+
session, err := client.NewSession(ctx)
77+
if err != nil {
78+
return err
6979
}
80+
defer session.Close()
7081

71-
keepAliveCtx, keepAliveCancel := context.WithCancel(ctx)
72-
defer keepAliveCancel()
73-
go func() {
74-
ssh.StartKeepAlive(keepAliveCtx, cfg.SSHClient)
75-
}()
82+
// Check if PTY mode is enabled
83+
enablePTY := command.Bool(define.FlagPTY)
7684

77-
// make stdout/stderr pipe, so we can get the output of the cmdline in realtime
78-
if err = cfg.WriteOutputTo(os.Stdout, os.Stderr); err != nil {
79-
return fmt.Errorf("failed to make std pipe: %w", err)
80-
}
85+
if enablePTY {
86+
// PTY mode: directly assign I/O streams, then request PTY
87+
if err := session.SetStdin(os.Stdin); err != nil {
88+
return err
89+
}
90+
if err := session.SetStdout(os.Stdout); err != nil {
91+
return fmt.Errorf("failed to setup stdout: %w", err)
92+
}
93+
if err := session.SetStderr(os.Stderr); err != nil {
94+
return fmt.Errorf("failed to setup stderr: %w", err)
95+
}
8196

82-
// if enable pty, we need to request a pty
83-
if cfg.IsPty() {
84-
resetFunc, err := cfg.RequestPTY(ctx)
85-
if err != nil {
97+
if err := session.RequestPTY(ctx, "", 0, 0); err != nil {
8698
return fmt.Errorf("failed to request pty: %w", err)
8799
}
88-
defer resetFunc()
100+
} else {
101+
// Non-PTY mode: use pipes with async I/O copying
102+
if err := session.SetupPipes(nil, os.Stdout, os.Stderr); err != nil {
103+
return fmt.Errorf("failed to setup pipes: %w", err)
104+
}
89105
}
90106

91-
return cfg.Run(ctx)
107+
// Set up context cancellation to send SIGTERM
108+
runCtx, cancel := context.WithCancel(ctx)
109+
defer cancel()
110+
111+
go func() {
112+
<-runCtx.Done()
113+
if ctx.Err() != nil {
114+
_ = session.Signal(gossh.SIGTERM)
115+
}
116+
}()
117+
118+
// Execute command
119+
return session.Run(ctx, cmdline...)
92120
}

cmd/common.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ func showVersionAndOSInfo() error {
2727
version.WriteString("unknown")
2828
}
2929

30+
version.WriteString("-")
31+
3032
if define.CommitID != "" {
3133
version.WriteString(define.CommitID)
3234
} else {

go.mod

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ require (
77
github.com/charmbracelet/keygen v0.5.4-0.20250811135526-3a78fe71a39a
88
github.com/crc-org/vfkit v0.6.2-0.20250929125513-88ab5cdcf444
99
github.com/gofrs/flock v0.12.1
10+
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
1011
github.com/google/uuid v1.6.0
12+
github.com/mattn/go-isatty v0.0.20
1113
github.com/oomol-lab/sysproxy v0.0.0-20250609040630-bacc3356fc00
1214
github.com/pkg/errors v0.9.1
1315
github.com/shirou/gopsutil/v4 v4.25.8
@@ -21,6 +23,7 @@ require (
2123
)
2224

2325
require (
26+
al.essio.dev/pkg/shellescape v1.6.1-0.20250626071713-b09271781b1a // indirect
2427
github.com/Code-Hex/go-infinity-channel v1.0.0 // indirect
2528
github.com/containers/common v0.64.2 // indirect
2629
github.com/ebitengine/purego v0.8.4 // indirect

go.sum

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
al.essio.dev/pkg/shellescape v1.6.1-0.20250626071713-b09271781b1a h1:G3lRbkj9f9M0wjN4kgGGsGaXL7mbLDfRrSE2Hn0zbO8=
2+
al.essio.dev/pkg/shellescape v1.6.1-0.20250626071713-b09271781b1a/go.mod h1:6sIqp7X2P6mThCQ7twERpZTuigpr6KbZWtls1U8I890=
13
github.com/Code-Hex/go-infinity-channel v1.0.0 h1:M8BWlfDOxq9or9yvF9+YkceoTkDI1pFAqvnP87Zh0Nw=
24
github.com/Code-Hex/go-infinity-channel v1.0.0/go.mod h1:5yUVg/Fqao9dAjcpzoQ33WwfdMWmISOrQloDRn3bsvY=
35
github.com/Code-Hex/vz/v3 v3.7.1 h1:EN1yNiyrbPq+dl388nne2NySo8I94EnPppvqypA65XM=
@@ -35,6 +37,8 @@ github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
3537
github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
3638
github.com/google/gopacket v1.1.19 h1:ves8RnFZPGiFnTS0uPQStjwru6uO6h+nlr9j6fL7kF8=
3739
github.com/google/gopacket v1.1.19/go.mod h1:iJ8V8n6KS+z2U1A8pUwu8bW5SyEMkXJB8Yo/Vo+TKTo=
40+
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4=
41+
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ=
3842
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
3943
github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
4044
github.com/insomniacslk/dhcp v0.0.0-20250417080101-5f8cf70e8c5f h1:dd33oobuIv9PcBVqvbEiCXEbNTomOHyj3WFuC5YiPRU=
@@ -49,6 +53,8 @@ github.com/linuxkit/virtsock v0.0.0-20220523201153-1a23e78aa7a2 h1:DZMFueDbfz6PN
4953
github.com/linuxkit/virtsock v0.0.0-20220523201153-1a23e78aa7a2/go.mod h1:SWzULI85WerrFt3u+nIm5F9l7EvxZTKQvd0InF3nmgM=
5054
github.com/lufia/plan9stats v0.0.0-20240909124753-873cd0166683 h1:7UMa6KCCMjZEMDtTVdcGu0B1GmmC7QJKiCCjyTAWQy0=
5155
github.com/lufia/plan9stats v0.0.0-20240909124753-873cd0166683/go.mod h1:ilwx/Dta8jXAgpFYFvSWEMwxmbWXyiUHkd5FwyKhb5k=
56+
github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
57+
github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
5258
github.com/mdlayher/packet v1.1.2 h1:3Up1NG6LZrsgDVn6X4L9Ge/iyRyxFEFD9o6Pr3Q1nQY=
5359
github.com/mdlayher/packet v1.1.2/go.mod h1:GEu1+n9sG5VtiRE4SydOmX5GTwyyYlteZiFU+x0kew4=
5460
github.com/mdlayher/socket v0.5.1 h1:VZaqt6RkGkt2OE9l3GcC6nZkqD3xKeQLyfleW/uBcos=
@@ -119,6 +125,7 @@ golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7w
119125
golang.org/x/sys v0.0.0-20201204225414-ed752295db88/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
120126
golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
121127
golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
128+
golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
122129
golang.org/x/sys v0.37.0 h1:fdNQudmxPjkdUTPnLn5mdQv7Zwvbvpaxqs831goi9kQ=
123130
golang.org/x/sys v0.37.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks=
124131
golang.org/x/term v0.36.0 h1:zMPR+aF8gfksFprF/Nc/rd1wRS1EI6nDBGyWAvDzx2Q=

0 commit comments

Comments
 (0)