feat: upgrade SSH implementation to support private keys, ssh-agent, …#1
Open
akramhossain-dev wants to merge 1 commit into
Open
Conversation
…and host key verification
Reviewer's GuideRefactors the monolithic main and SSH logic into modular packages, adds a new SSH subsystem with private key, ssh-agent, and host key verification support, extends server configuration to store key/agent options with encrypted fields, and wires the TUI to use the new configuration and SSH layers while adding tests and contributor documentation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In the SSH authentication setup (both SSHCommand.Run and GetSSHClient), failing to load the configured private key immediately aborts the connection even if password or SSH agent auth could still work; consider treating key load failures as non-fatal and falling back to other available auth methods instead of returning an error.
- The server form validation in tui.go assumes that an empty private key path and disabled SSH agent requires a password, but still allows a non-empty key path with an empty password; if passphrase-less keys are intended, it would be helpful to clarify and enforce the expected combinations of privateKeyPath, password, and UseSSHAgent consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the SSH authentication setup (both SSHCommand.Run and GetSSHClient), failing to load the configured private key immediately aborts the connection even if password or SSH agent auth could still work; consider treating key load failures as non-fatal and falling back to other available auth methods instead of returning an error.
- The server form validation in tui.go assumes that an empty private key path and disabled SSH agent requires a password, but still allows a non-empty key path with an empty password; if passphrase-less keys are intended, it would be helpful to clarify and enforce the expected combinations of privateKeyPath, password, and UseSSHAgent consistently.
## Individual Comments
### Comment 1
<location path="ssh/ssh.go" line_range="41" />
<code_context>
-func (s *SSHCommand) SetStdout(w io.Writer) { s.stdout = w }
-func (s *SSHCommand) SetStderr(w io.Writer) { s.stderr = w }
-
-func (s *SSHCommand) Run() error {
- config := &ssh.ClientConfig{
- User: s.User,
</code_context>
<issue_to_address>
**issue (bug_risk):** Default stdin/stdout/stderr handling in SSHCommand can leave nil pipes while still manipulating the real TTY.
In `Run`, you call `term.MakeRaw` on the TTY (either `s.stdin` if it’s an `*os.File`, or `os.Stdin`), but then assign `session.Stdin = s.stdin` directly and never fall back to `os.Stdin` when `s.stdin` is nil. The same issue exists for stdout/stderr. As a result, if the caller doesn’t call `SetStdin/Stdout/Stderr`, the session may get nil stdio while the local terminal is still put into raw mode, and the FD you configure may not match the stream wired to SSH.
Instead, derive `in/out/errw` from the fields with sensible defaults to `os.Stdin/Stdout/Stderr`, assign those to `session.*`, and use the same `in` when choosing the FD for `term.MakeRaw`/`GetSize` so the configured TTY always corresponds to the actual session streams.
</issue_to_address>
### Comment 2
<location path="ssh/ssh.go" line_range="231" />
<code_context>
+}
+
+// loadPrivateKey loads a private key from disk with optional passphrase decryption.
+func loadPrivateKey(path string, passphrase string) (ssh.Signer, error) {
+ if strings.HasPrefix(path, "~/") {
+ home, err := os.UserHomeDir()
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Passing a wrong passphrase silently falls back to parsing the key unencrypted.
In `loadPrivateKey`, when a passphrase is provided and `ParsePrivateKeyWithPassphrase` fails, you immediately fall back to `ssh.ParsePrivateKey` and discard the error. This makes a wrong passphrase look the same as an unencrypted key and can hide misconfigurations.
If you need to support both encrypted and unencrypted keys, please distinguish "not an encrypted key" from actual decryption failures, and return the decryption error when a passphrase is supplied and the unencrypted parse also fails. For example:
```go
if passphrase != "" {
signer, err := ssh.ParsePrivateKeyWithPassphrase(keyBytes, []byte(passphrase))
if err == nil {
return signer, nil
}
// if it's clearly a "unencrypted key" error, fall through; otherwise return
}
return ssh.ParsePrivateKey(keyBytes)
```
This prevents silently accepting an incorrect passphrase.
</issue_to_address>
### Comment 3
<location path="ssh/ssh.go" line_range="318" />
<code_context>
+}
+
+// mustGetHostKeyCallback is a helper that falls back to insecure check ONLY if known_hosts cannot be loaded.
+func mustGetHostKeyCallback() ssh.HostKeyCallback {
+ cb, err := getHostKeyCallback()
+ if err != nil {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Falling back to InsecureIgnoreHostKey on error weakens host key verification.
`mustGetHostKeyCallback` currently returns `ssh.InsecureIgnoreHostKey()` when `getHostKeyCallback` fails (e.g. unresolved home dir, bad known_hosts permissions). In those cases host key verification is effectively disabled, undermining the TOFU model and exposing users to MITM if their environment is misconfigured.
Instead of falling back to `InsecureIgnoreHostKey`, consider:
- Propagating the error and failing connection setup, with a clear error from callers (e.g. `GetSSHClient` / `SSHCommand.Run`), or
- Returning a host key callback that always errors with a descriptive message about fixing `$HOME` / `known_hosts`.
This keeps the security guarantees consistent with the rest of the codebase.
Suggested implementation:
```golang
// mustGetHostKeyCallback returns a host key callback.
//
// If loading known_hosts fails, it returns a callback that always
// fails host key verification with a descriptive error, instead of
// disabling verification via InsecureIgnoreHostKey.
func mustGetHostKeyCallback() ssh.HostKeyCallback {
cb, err := getHostKeyCallback()
if err != nil {
return func(hostname string, remote net.Addr, key ssh.PublicKey) error {
return fmt.Errorf(
"host key verification failed for %s: unable to load known_hosts: %w. "+
"Please ensure $HOME is set and ~/.ssh/known_hosts is readable",
hostname,
err,
)
}
}
return cb
}
```
1. Ensure `net` is imported in `ssh/ssh.go`:
- Add `net` to the import list if it is not already present: `import "net"`.
2. If there are unit/integration tests that expect `mustGetHostKeyCallback` to return `ssh.InsecureIgnoreHostKey()` on failure, update them to expect the new erroring callback behavior instead.
3. Callers (e.g. `GetSSHClient`, `SSHCommand.Run`) should surface the error returned from the host key callback to the user so the descriptive message about fixing `$HOME` / `known_hosts` is visible.
</issue_to_address>
### Comment 4
<location path="ssh/ssh.go" line_range="173" />
<code_context>
+}
+
+// GetSSHClient establishes an SSH client connection (useful for SFTP).
+func GetSSHClient(server config.Server) (*ssh.Client, error) {
+ auths := []ssh.AuthMethod{}
+
</code_context>
<issue_to_address>
**suggestion:** Connections with no usable auth methods result in a generic ssh.Dial error instead of an explicit configuration error.
Both GetSSHClient and SSHCommand.Run build the auths slice incrementally but never handle the "no usable auth" case. If UseSSHAgent is true but SSH_AUTH_SOCK is unset and no password/private key is provided, auths stays empty and ssh.Dial fails with an opaque error.
Consider detecting this explicitly and returning a clearer configuration error, e.g.:
```go
if len(auths) == 0 {
return nil, fmt.Errorf("no SSH authentication methods configured (check agent, private key path, or password)")
}
```
Also, in the private-key branch you return immediately on loadPrivateKey error even if agent auth was already added. If agent auth is intended as a fallback, key-load failures could be treated as non-fatal when another auth method is available.
Suggested implementation:
```golang
// GetSSHClient establishes an SSH client connection (useful for SFTP).
func GetSSHClient(server config.Server) (*ssh.Client, error) {
auths := []ssh.AuthMethod{}
// Optional: SSH agent auth
if server.UseSSHAgent {
if authMethod, err := getSSHAgentAuth(); err == nil && authMethod != nil {
auths = append(auths, authMethod)
}
}
// Optional: password auth
if server.Password != "" {
auths = append(auths, ssh.Password(server.Password))
}
// Optional: private key auth
if server.PrivateKeyPath != "" {
signer, err := loadPrivateKey(server.PrivateKeyPath, server.Passphrase)
if err != nil {
// If we already have at least one usable auth method (e.g. agent or password),
// treat private key load failures as non-fatal so they can act as a fallback.
if len(auths) == 0 {
return nil, fmt.Errorf("failed to load private key %q: %w", server.PrivateKeyPath, err)
}
} else {
auths = append(auths, ssh.PublicKeys(signer))
}
}
// Explicitly detect misconfiguration where no usable auth methods were added.
if len(auths) == 0 {
return nil, fmt.Errorf("no SSH authentication methods configured (check agent, private key path, or password)")
}
clientConfig := &ssh.ClientConfig{
User: server.User,
Auth: auths,
HostKeyCallback: ssh.InsecureIgnoreHostKey(), // or your existing host key callback
Timeout: server.Timeout,
}
addr := net.JoinHostPort(server.Host, strconv.Itoa(server.Port))
client, err := ssh.Dial("tcp", addr, clientConfig)
if err != nil {
return nil, err
}
return client, nil
}
```
1. Ensure the following imports exist (or adjust to match your existing helpers/types):
- `fmt`
- `net`
- `strconv`
- `golang.org/x/crypto/ssh`
2. Replace `getSSHAgentAuth`, `loadPrivateKey`, and the `config.Server` fields (`UseSSHAgent`, `Password`, `PrivateKeyPath`, `Passphrase`, `User`, `Timeout`, `Host`, `Port`) with the actual helpers/fields used in your codebase, preserving the logic:
- Build `auths` incrementally.
- Only treat private key load errors as fatal when `len(auths) == 0`.
- Before calling `ssh.Dial`, fail fast with a clear error if `len(auths) == 0`.
3. Apply the same “no usable auth methods” check and non-fatal private-key error handling to `SSHCommand.Run`:
- After constructing its `auths` slice, add the `len(auths) == 0` configuration error.
- In its private-key branch, only return an error on key load failure if `len(auths) == 0`; otherwise log/ignore and continue with the other auth methods.
</issue_to_address>
### Comment 5
<location path="config/config_test.go" line_range="86" />
<code_context>
+ }
+}
+
+func TestSaveAndLoadServers(t *testing.T) {
+ // Create a temp directory for configuration files
+ tempDir, err := os.MkdirTemp("", "sshmanager_config_test")
</code_context>
<issue_to_address>
**suggestion (testing):** Extend SaveServers/LoadServers tests to cover more edge cases and backwards compatibility
To strengthen coverage of config persistence, consider adding tests for:
1. **Empty server list**: Call `SaveServers(nil)` and `SaveServers([]Server{})`, then `LoadServers()` and verify it returns an empty slice without error. This confirms the atomic write/temp file path behaves correctly with no servers.
2. **Backward compatibility**: Create an older-style `servers.json` missing `private_key_path` and `use_ssh_agent` (e.g., hand-crafted JSON), then call `LoadServers()` and assert the new fields use their zero values (`PrivateKeyPath == ""`, `UseSSHAgent == false`) while existing fields are still loaded/decrypted correctly.
These cases will help ensure the new fields and migration from previous config formats are safely handled.
Suggested implementation:
```golang
func TestSaveAndLoadServers(t *testing.T) {
// Create a temp directory for configuration files
tempDir, err := os.MkdirTemp("", "sshmanager_config_test")
if err != nil {
t.Fatalf("failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)
// Override config directory for testing
ConfigDirOverride = tempDir
defer func() {
ConfigDirOverride = ""
}()
t.Run("empty server list with nil slice", func(t *testing.T) {
// Save with a nil slice
if err := SaveServers(nil); err != nil {
t.Fatalf("SaveServers(nil) returned error: %v", err)
}
servers, err := LoadServers()
if err != nil {
t.Fatalf("LoadServers() after SaveServers(nil) returned error: %v", err)
}
if len(servers) != 0 {
t.Errorf("expected 0 servers after SaveServers(nil), got %d", len(servers))
}
})
t.Run("empty server list with empty slice", func(t *testing.T) {
// Save with an empty slice
if err := SaveServers([]Server{}); err != nil {
t.Fatalf("SaveServers([]Server{}) returned error: %v", err)
}
servers, err := LoadServers()
if err != nil {
t.Fatalf("LoadServers() after SaveServers([]Server{}) returned error: %v", err)
}
if len(servers) != 0 {
t.Errorf("expected 0 servers after SaveServers([]Server{}), got %d", len(servers))
}
})
t.Run("backward compatibility with legacy config missing new fields", func(t *testing.T) {
// Construct a legacy-style servers.json missing private_key_path and use_ssh_agent
legacyJSON := `[
{
"name": "legacy-server",
"host": "example.com",
"port": 22,
"username": "legacyuser",
"password_encrypted": "dGVzdF9wYXNzd29yZA=="
}
]`
// Determine the servers config path in the overridden config dir.
// NOTE: this assumes the production code writes servers.json directly under the config dir.
serversConfigPath := filepath.Join(tempDir, "servers.json")
if err := os.WriteFile(serversConfigPath, []byte(legacyJSON), 0o600); err != nil {
t.Fatalf("failed to write legacy servers.json: %v", err)
}
servers, err := LoadServers()
if err != nil {
t.Fatalf("LoadServers() for legacy config returned error: %v", err)
}
if len(servers) != 1 {
t.Fatalf("expected 1 server from legacy config, got %d", len(servers))
}
s := servers[0]
if s.Name != "legacy-server" {
t.Errorf("expected Name %q, got %q", "legacy-server", s.Name)
}
if s.Host != "example.com" {
t.Errorf("expected Host %q, got %q", "example.com", s.Host)
}
if s.Port != 22 {
t.Errorf("expected Port %d, got %d", 22, s.Port)
}
if s.Username != "legacyuser" {
t.Errorf("expected Username %q, got %q", "legacyuser", s.Username)
}
// New fields should have their zero values when missing in legacy JSON
if s.PrivateKeyPath != "" {
t.Errorf("expected PrivateKeyPath to be empty for legacy config, got %q", s.PrivateKeyPath)
}
if s.UseSSHAgent {
t.Errorf("expected UseSSHAgent to be false for legacy config, got %v", s.UseSSHAgent)
}
})
}
```
1. This test uses `filepath.Join` and assumes the servers config file is named `servers.json` directly under the config directory. If your production code uses a different filename or helper (e.g., `getServersConfigPath()`), update `serversConfigPath := filepath.Join(tempDir, "servers.json")` to use the same logic as the production code.
2. The legacy JSON field `password_encrypted` should match the actual JSON field used in your `Server` struct (e.g., `encrypted_password`, `password`, etc.). Adjust the field name and base64 value to align with your struct tags / encryption format so that `LoadServers()` can correctly deserialize and decrypt it.
3. If `LoadServers()` expects additional mandatory fields (beyond those shown), extend `legacyJSON` to include them with representative values while still omitting `private_key_path` and `use_ssh_agent` to test backward compatibility.
4. Ensure `Server` has fields `PrivateKeyPath` and `UseSSHAgent` with the expected zero values (`""` and `false`) so these assertions compile and behave as intended.
</issue_to_address>
### Comment 6
<location path="docs/CONTRIBUTING.md" line_range="15" />
<code_context>
+* **`main.go`**: The entrypoint of the application. It bootstraps and runs the application by calling `tui.Run()`.
+* **`config/`**: Manages server profiles storage (JSON), AES-256-GCM encryption/decryption of credentials, and atomic file I/O operations.
+* **`ssh/`**: Contains core SSH logic including PTY setup, SSH agent connections, private key parsing, keep-alive heartbeats, and TOFU host key verification.
+* **`tui/`**: Implements the user interface using the Charm Bracelet Bubble Tea framework:
+ * `sftp_browser.go`: Handles concurrent file transfers (upload/download queue and progress).
+ * `styles.go`: Defines the Lipgloss stylesheets, colors, and layout configurations.
</code_context>
<issue_to_address>
**issue (typo):** Correct the naming of the Charmbracelet Bubble Tea framework.
The framework name should be "Bubble Tea" by "Charmbracelet" (one word). Please adjust the phrasing accordingly for accuracy.
```suggestion
* **`tui/`**: Implements the user interface using the Bubble Tea framework by Charmbracelet:
```
</issue_to_address>
### Comment 7
<location path="tui/tui.go" line_range="429" />
<code_context>
- return m, cmd
-}
-
-func (m MainModel) updateSFTPBrowser(msg tea.Msg) (tea.Model, tea.Cmd) {
- b := m.sftpBrowser
- if b == nil {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting layout calculations, mode-specific handlers, and key translation tables into small helper functions to make the TUI logic easier to follow and maintain without changing behavior.
The added code centralizes a lot of responsibilities in `MainModel` and `updateSFTPBrowser`, which increases complexity and duplication. You can keep all functionality but reduce cognitive load with a few localized refactors:
---
### 1. Extract browser layout math into small helpers
`viewSFTPBrowser` and `handleTerminalResize` both compute layout dimensions. Centralizing this makes behavior easier to reason about and test:
```go
// Keep in this package but separate from MainModel methods
func (b *SFTPBrowser) layoutSize(width, height int) (leftWidth, rightWidth, panelHeight, termWidth, termHeight int) {
availHeight := height - 8
if availHeight < 10 {
availHeight = 10
}
termHeight = availHeight * b.terminalHeightPct / 100
if termHeight < 5 {
termHeight = 5
}
if termHeight > availHeight-6 {
termHeight = availHeight - 6
}
panelHeight = availHeight - termHeight
if panelHeight < 6 {
panelHeight = 6
}
availWidth := width - 10
if availWidth < 40 {
availWidth = 40
}
leftWidth = availWidth * b.localWidthPct / 100
rightWidth = availWidth - leftWidth
if leftWidth < 20 {
leftWidth = 20
}
if rightWidth < 20 {
rightWidth = 20
}
termWidth = width - 8
if termWidth < 20 {
termWidth = 20
}
return
}
```
Use it in `viewSFTPBrowser`:
```go
leftPanelWidth, rightPanelWidth, panelHeight, termWidth, termHeight :=
b.layoutSize(m.width, m.height)
```
And in `handleTerminalResize`:
```go
_, _, _, termWidth, termHeight := b.layoutSize(m.width, m.height)
if b.terminalEmu != nil {
b.terminalEmu.Resize(termWidth, termHeight)
}
if b.terminalSession != nil {
_ = b.terminalSession.WindowChange(termHeight, termWidth)
}
```
This removes repeated arithmetic and keeps layout rules in one place.
---
### 2. Flatten `updateSFTPBrowser` by routing modes early
You already have implicit modes (`deleteConfirm`, `activePanel == TerminalPanel`, `isBusy`). A small dispatcher reduces nested branching without changing behavior:
```go
func (m MainModel) updateSFTPBrowser(msg tea.Msg) (tea.Model, tea.Cmd) {
b := m.sftpBrowser
if b == nil {
m.state = StateServerList
return m, nil
}
keyMsg, ok := msg.(tea.KeyMsg)
if !ok {
return m, nil
}
// Clear status
if !b.isBusy && b.activePanel != TerminalPanel &&
(strings.HasPrefix(b.statusMsg, "Successfully") || strings.HasPrefix(b.statusMsg, "Error")) {
b.statusMsg = ""
}
// Mode routing
if b.deleteConfirm {
return m.handleDeleteConfirm(keyMsg)
}
if b.activePanel == TerminalPanel {
return m.handleTerminalKeys(keyMsg)
}
if b.isBusy {
return m.handleBusyKeys(keyMsg)
}
// Global resize keys
if cmd := m.handleResizeKeys(keyMsg); cmd != nil {
return m, cmd
}
// Default browser keys
return m.handleBrowserKeys(keyMsg)
}
```
Then move existing logic into small helpers (still in the same file):
```go
func (m MainModel) handleDeleteConfirm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
b := m.sftpBrowser
switch msg.String() {
case "y", "Y":
b.deleteConfirm = false
b.isBusy = true
b.statusMsg = fmt.Sprintf("Deleting %s...", b.deleteItem.Name)
isLocal := b.activePanel == LocalPanel
path := b.deletePath
name := b.deleteItem.Name
return m, func() tea.Msg {
var err error
if isLocal {
err = os.RemoveAll(path)
} else {
err = removeRemoteAll(b.sftpClient, b.sshClient, path)
}
if err != nil {
return sftpProgressMsg{
Done: true,
Err: fmt.Errorf("failed to delete %s: %w", name, err),
}
}
return sftpProgressMsg{
Done: true,
Message: fmt.Sprintf("Successfully deleted %s", name),
}
}
case "n", "N", "esc", "q":
b.deleteConfirm = false
}
return m, nil
}
```
You can similarly extract `handleTerminalKeys`, `handleBusyKeys`, and `handleBrowserKeys` by moving existing switch branches into dedicated functions. This keeps behavior identical but makes each mode’s logic self-contained.
---
### 3. Simplify terminal key translation with a map
`keyMsgToTerminalInput` is a large switch with mostly constant mappings. A small map/table makes it more maintainable:
```go
var ctrlKeyMap = map[tea.KeyType]string{
tea.KeyCtrlA: "\x01",
tea.KeyCtrlB: "\x02",
tea.KeyCtrlC: "\x03",
tea.KeyCtrlD: "\x04",
tea.KeyCtrlE: "\x05",
tea.KeyCtrlF: "\x06",
tea.KeyCtrlG: "\x07",
tea.KeyCtrlH: "\x08",
tea.KeyCtrlJ: "\x0a",
tea.KeyCtrlK: "\x0b",
tea.KeyCtrlL: "\x0c",
tea.KeyCtrlN: "\x0e",
tea.KeyCtrlO: "\x0f",
tea.KeyCtrlP: "\x10",
tea.KeyCtrlQ: "\x11",
tea.KeyCtrlR: "\x12",
tea.KeyCtrlS: "\x13",
// KeyCtrlT deliberately omitted to keep current behavior
tea.KeyCtrlU: "\x15",
tea.KeyCtrlV: "\x16",
tea.KeyCtrlW: "\x17",
tea.KeyCtrlX: "\x18",
tea.KeyCtrlY: "\x19",
tea.KeyCtrlZ: "\x1a",
}
func keyMsgToTerminalInput(msg tea.KeyMsg) string {
switch msg.Type {
case tea.KeyRunes:
return string(msg.Runes)
case tea.KeySpace:
return " "
case tea.KeyEnter:
return "\r"
case tea.KeyBackspace:
return "\x7f"
case tea.KeyTab:
return "\t"
case tea.KeyEscape:
return "\x1b"
case tea.KeyUp:
return "\x1b[A"
case tea.KeyDown:
return "\x1b[B"
case tea.KeyRight:
return "\x1b[C"
case tea.KeyLeft:
return "\x1b[D"
}
if seq, ok := ctrlKeyMap[msg.Type]; ok {
return seq
}
return ""
}
```
This keeps all current behavior (including the special case for `KeyCtrlT`) but makes future changes simpler.
---
These changes are small, preserve all functionality, and directly reduce local complexity and duplication within the TUI file without requiring a larger architectural split right now.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return &SSHCommand{ | ||
| Host: server.Host, | ||
| Port: server.Port, | ||
| User: server.User, |
There was a problem hiding this comment.
issue (bug_risk): Default stdin/stdout/stderr handling in SSHCommand can leave nil pipes while still manipulating the real TTY.
In Run, you call term.MakeRaw on the TTY (either s.stdin if it’s an *os.File, or os.Stdin), but then assign session.Stdin = s.stdin directly and never fall back to os.Stdin when s.stdin is nil. The same issue exists for stdout/stderr. As a result, if the caller doesn’t call SetStdin/Stdout/Stderr, the session may get nil stdio while the local terminal is still put into raw mode, and the FD you configure may not match the stream wired to SSH.
Instead, derive in/out/errw from the fields with sensible defaults to os.Stdin/Stdout/Stderr, assign those to session.*, and use the same in when choosing the FD for term.MakeRaw/GetSize so the configured TTY always corresponds to the actual session streams.
| } | ||
|
|
||
| // loadPrivateKey loads a private key from disk with optional passphrase decryption. | ||
| func loadPrivateKey(path string, passphrase string) (ssh.Signer, error) { |
There was a problem hiding this comment.
🚨 suggestion (security): Passing a wrong passphrase silently falls back to parsing the key unencrypted.
In loadPrivateKey, when a passphrase is provided and ParsePrivateKeyWithPassphrase fails, you immediately fall back to ssh.ParsePrivateKey and discard the error. This makes a wrong passphrase look the same as an unencrypted key and can hide misconfigurations.
If you need to support both encrypted and unencrypted keys, please distinguish "not an encrypted key" from actual decryption failures, and return the decryption error when a passphrase is supplied and the unencrypted parse also fails. For example:
if passphrase != "" {
signer, err := ssh.ParsePrivateKeyWithPassphrase(keyBytes, []byte(passphrase))
if err == nil {
return signer, nil
}
// if it's clearly a "unencrypted key" error, fall through; otherwise return
}
return ssh.ParsePrivateKey(keyBytes)This prevents silently accepting an incorrect passphrase.
| } | ||
|
|
||
| // mustGetHostKeyCallback is a helper that falls back to insecure check ONLY if known_hosts cannot be loaded. | ||
| func mustGetHostKeyCallback() ssh.HostKeyCallback { |
There was a problem hiding this comment.
🚨 suggestion (security): Falling back to InsecureIgnoreHostKey on error weakens host key verification.
mustGetHostKeyCallback currently returns ssh.InsecureIgnoreHostKey() when getHostKeyCallback fails (e.g. unresolved home dir, bad known_hosts permissions). In those cases host key verification is effectively disabled, undermining the TOFU model and exposing users to MITM if their environment is misconfigured.
Instead of falling back to InsecureIgnoreHostKey, consider:
- Propagating the error and failing connection setup, with a clear error from callers (e.g.
GetSSHClient/SSHCommand.Run), or - Returning a host key callback that always errors with a descriptive message about fixing
$HOME/known_hosts.
This keeps the security guarantees consistent with the rest of the codebase.
Suggested implementation:
// mustGetHostKeyCallback returns a host key callback.
//
// If loading known_hosts fails, it returns a callback that always
// fails host key verification with a descriptive error, instead of
// disabling verification via InsecureIgnoreHostKey.
func mustGetHostKeyCallback() ssh.HostKeyCallback {
cb, err := getHostKeyCallback()
if err != nil {
return func(hostname string, remote net.Addr, key ssh.PublicKey) error {
return fmt.Errorf(
"host key verification failed for %s: unable to load known_hosts: %w. "+
"Please ensure $HOME is set and ~/.ssh/known_hosts is readable",
hostname,
err,
)
}
}
return cb
}- Ensure
netis imported inssh/ssh.go:- Add
netto the import list if it is not already present:import "net".
- Add
- If there are unit/integration tests that expect
mustGetHostKeyCallbackto returnssh.InsecureIgnoreHostKey()on failure, update them to expect the new erroring callback behavior instead. - Callers (e.g.
GetSSHClient,SSHCommand.Run) should surface the error returned from the host key callback to the user so the descriptive message about fixing$HOME/known_hostsis visible.
| } | ||
|
|
||
| // GetSSHClient establishes an SSH client connection (useful for SFTP). | ||
| func GetSSHClient(server config.Server) (*ssh.Client, error) { |
There was a problem hiding this comment.
suggestion: Connections with no usable auth methods result in a generic ssh.Dial error instead of an explicit configuration error.
Both GetSSHClient and SSHCommand.Run build the auths slice incrementally but never handle the "no usable auth" case. If UseSSHAgent is true but SSH_AUTH_SOCK is unset and no password/private key is provided, auths stays empty and ssh.Dial fails with an opaque error.
Consider detecting this explicitly and returning a clearer configuration error, e.g.:
if len(auths) == 0 {
return nil, fmt.Errorf("no SSH authentication methods configured (check agent, private key path, or password)")
}Also, in the private-key branch you return immediately on loadPrivateKey error even if agent auth was already added. If agent auth is intended as a fallback, key-load failures could be treated as non-fatal when another auth method is available.
Suggested implementation:
// GetSSHClient establishes an SSH client connection (useful for SFTP).
func GetSSHClient(server config.Server) (*ssh.Client, error) {
auths := []ssh.AuthMethod{}
// Optional: SSH agent auth
if server.UseSSHAgent {
if authMethod, err := getSSHAgentAuth(); err == nil && authMethod != nil {
auths = append(auths, authMethod)
}
}
// Optional: password auth
if server.Password != "" {
auths = append(auths, ssh.Password(server.Password))
}
// Optional: private key auth
if server.PrivateKeyPath != "" {
signer, err := loadPrivateKey(server.PrivateKeyPath, server.Passphrase)
if err != nil {
// If we already have at least one usable auth method (e.g. agent or password),
// treat private key load failures as non-fatal so they can act as a fallback.
if len(auths) == 0 {
return nil, fmt.Errorf("failed to load private key %q: %w", server.PrivateKeyPath, err)
}
} else {
auths = append(auths, ssh.PublicKeys(signer))
}
}
// Explicitly detect misconfiguration where no usable auth methods were added.
if len(auths) == 0 {
return nil, fmt.Errorf("no SSH authentication methods configured (check agent, private key path, or password)")
}
clientConfig := &ssh.ClientConfig{
User: server.User,
Auth: auths,
HostKeyCallback: ssh.InsecureIgnoreHostKey(), // or your existing host key callback
Timeout: server.Timeout,
}
addr := net.JoinHostPort(server.Host, strconv.Itoa(server.Port))
client, err := ssh.Dial("tcp", addr, clientConfig)
if err != nil {
return nil, err
}
return client, nil
}- Ensure the following imports exist (or adjust to match your existing helpers/types):
fmtnetstrconvgolang.org/x/crypto/ssh
- Replace
getSSHAgentAuth,loadPrivateKey, and theconfig.Serverfields (UseSSHAgent,Password,PrivateKeyPath,Passphrase,User,Timeout,Host,Port) with the actual helpers/fields used in your codebase, preserving the logic:- Build
authsincrementally. - Only treat private key load errors as fatal when
len(auths) == 0. - Before calling
ssh.Dial, fail fast with a clear error iflen(auths) == 0.
- Build
- Apply the same “no usable auth methods” check and non-fatal private-key error handling to
SSHCommand.Run:- After constructing its
authsslice, add thelen(auths) == 0configuration error. - In its private-key branch, only return an error on key load failure if
len(auths) == 0; otherwise log/ignore and continue with the other auth methods.
- After constructing its
| } | ||
| } | ||
|
|
||
| func TestSaveAndLoadServers(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Extend SaveServers/LoadServers tests to cover more edge cases and backwards compatibility
To strengthen coverage of config persistence, consider adding tests for:
-
Empty server list: Call
SaveServers(nil)andSaveServers([]Server{}), thenLoadServers()and verify it returns an empty slice without error. This confirms the atomic write/temp file path behaves correctly with no servers. -
Backward compatibility: Create an older-style
servers.jsonmissingprivate_key_pathanduse_ssh_agent(e.g., hand-crafted JSON), then callLoadServers()and assert the new fields use their zero values (PrivateKeyPath == "",UseSSHAgent == false) while existing fields are still loaded/decrypted correctly.
These cases will help ensure the new fields and migration from previous config formats are safely handled.
Suggested implementation:
func TestSaveAndLoadServers(t *testing.T) {
// Create a temp directory for configuration files
tempDir, err := os.MkdirTemp("", "sshmanager_config_test")
if err != nil {
t.Fatalf("failed to create temp dir: %v", err)
}
defer os.RemoveAll(tempDir)
// Override config directory for testing
ConfigDirOverride = tempDir
defer func() {
ConfigDirOverride = ""
}()
t.Run("empty server list with nil slice", func(t *testing.T) {
// Save with a nil slice
if err := SaveServers(nil); err != nil {
t.Fatalf("SaveServers(nil) returned error: %v", err)
}
servers, err := LoadServers()
if err != nil {
t.Fatalf("LoadServers() after SaveServers(nil) returned error: %v", err)
}
if len(servers) != 0 {
t.Errorf("expected 0 servers after SaveServers(nil), got %d", len(servers))
}
})
t.Run("empty server list with empty slice", func(t *testing.T) {
// Save with an empty slice
if err := SaveServers([]Server{}); err != nil {
t.Fatalf("SaveServers([]Server{}) returned error: %v", err)
}
servers, err := LoadServers()
if err != nil {
t.Fatalf("LoadServers() after SaveServers([]Server{}) returned error: %v", err)
}
if len(servers) != 0 {
t.Errorf("expected 0 servers after SaveServers([]Server{}), got %d", len(servers))
}
})
t.Run("backward compatibility with legacy config missing new fields", func(t *testing.T) {
// Construct a legacy-style servers.json missing private_key_path and use_ssh_agent
legacyJSON := `[
{
"name": "legacy-server",
"host": "example.com",
"port": 22,
"username": "legacyuser",
"password_encrypted": "dGVzdF9wYXNzd29yZA=="
}
]`
// Determine the servers config path in the overridden config dir.
// NOTE: this assumes the production code writes servers.json directly under the config dir.
serversConfigPath := filepath.Join(tempDir, "servers.json")
if err := os.WriteFile(serversConfigPath, []byte(legacyJSON), 0o600); err != nil {
t.Fatalf("failed to write legacy servers.json: %v", err)
}
servers, err := LoadServers()
if err != nil {
t.Fatalf("LoadServers() for legacy config returned error: %v", err)
}
if len(servers) != 1 {
t.Fatalf("expected 1 server from legacy config, got %d", len(servers))
}
s := servers[0]
if s.Name != "legacy-server" {
t.Errorf("expected Name %q, got %q", "legacy-server", s.Name)
}
if s.Host != "example.com" {
t.Errorf("expected Host %q, got %q", "example.com", s.Host)
}
if s.Port != 22 {
t.Errorf("expected Port %d, got %d", 22, s.Port)
}
if s.Username != "legacyuser" {
t.Errorf("expected Username %q, got %q", "legacyuser", s.Username)
}
// New fields should have their zero values when missing in legacy JSON
if s.PrivateKeyPath != "" {
t.Errorf("expected PrivateKeyPath to be empty for legacy config, got %q", s.PrivateKeyPath)
}
if s.UseSSHAgent {
t.Errorf("expected UseSSHAgent to be false for legacy config, got %v", s.UseSSHAgent)
}
})
}- This test uses
filepath.Joinand assumes the servers config file is namedservers.jsondirectly under the config directory. If your production code uses a different filename or helper (e.g.,getServersConfigPath()), updateserversConfigPath := filepath.Join(tempDir, "servers.json")to use the same logic as the production code. - The legacy JSON field
password_encryptedshould match the actual JSON field used in yourServerstruct (e.g.,encrypted_password,password, etc.). Adjust the field name and base64 value to align with your struct tags / encryption format so thatLoadServers()can correctly deserialize and decrypt it. - If
LoadServers()expects additional mandatory fields (beyond those shown), extendlegacyJSONto include them with representative values while still omittingprivate_key_pathanduse_ssh_agentto test backward compatibility. - Ensure
Serverhas fieldsPrivateKeyPathandUseSSHAgentwith the expected zero values (""andfalse) so these assertions compile and behave as intended.
| * **`main.go`**: The entrypoint of the application. It bootstraps and runs the application by calling `tui.Run()`. | ||
| * **`config/`**: Manages server profiles storage (JSON), AES-256-GCM encryption/decryption of credentials, and atomic file I/O operations. | ||
| * **`ssh/`**: Contains core SSH logic including PTY setup, SSH agent connections, private key parsing, keep-alive heartbeats, and TOFU host key verification. | ||
| * **`tui/`**: Implements the user interface using the Charm Bracelet Bubble Tea framework: |
There was a problem hiding this comment.
issue (typo): Correct the naming of the Charmbracelet Bubble Tea framework.
The framework name should be "Bubble Tea" by "Charmbracelet" (one word). Please adjust the phrasing accordingly for accuracy.
Suggested change
| * **`tui/`**: Implements the user interface using the Charm Bracelet Bubble Tea framework: | |
| * **`tui/`**: Implements the user interface using the Bubble Tea framework by Charmbracelet: |
| return m, cmd | ||
| } | ||
|
|
||
| func (m MainModel) updateSFTPBrowser(msg tea.Msg) (tea.Model, tea.Cmd) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting layout calculations, mode-specific handlers, and key translation tables into small helper functions to make the TUI logic easier to follow and maintain without changing behavior.
The added code centralizes a lot of responsibilities in MainModel and updateSFTPBrowser, which increases complexity and duplication. You can keep all functionality but reduce cognitive load with a few localized refactors:
1. Extract browser layout math into small helpers
viewSFTPBrowser and handleTerminalResize both compute layout dimensions. Centralizing this makes behavior easier to reason about and test:
// Keep in this package but separate from MainModel methods
func (b *SFTPBrowser) layoutSize(width, height int) (leftWidth, rightWidth, panelHeight, termWidth, termHeight int) {
availHeight := height - 8
if availHeight < 10 {
availHeight = 10
}
termHeight = availHeight * b.terminalHeightPct / 100
if termHeight < 5 {
termHeight = 5
}
if termHeight > availHeight-6 {
termHeight = availHeight - 6
}
panelHeight = availHeight - termHeight
if panelHeight < 6 {
panelHeight = 6
}
availWidth := width - 10
if availWidth < 40 {
availWidth = 40
}
leftWidth = availWidth * b.localWidthPct / 100
rightWidth = availWidth - leftWidth
if leftWidth < 20 {
leftWidth = 20
}
if rightWidth < 20 {
rightWidth = 20
}
termWidth = width - 8
if termWidth < 20 {
termWidth = 20
}
return
}Use it in viewSFTPBrowser:
leftPanelWidth, rightPanelWidth, panelHeight, termWidth, termHeight :=
b.layoutSize(m.width, m.height)And in handleTerminalResize:
_, _, _, termWidth, termHeight := b.layoutSize(m.width, m.height)
if b.terminalEmu != nil {
b.terminalEmu.Resize(termWidth, termHeight)
}
if b.terminalSession != nil {
_ = b.terminalSession.WindowChange(termHeight, termWidth)
}This removes repeated arithmetic and keeps layout rules in one place.
2. Flatten updateSFTPBrowser by routing modes early
You already have implicit modes (deleteConfirm, activePanel == TerminalPanel, isBusy). A small dispatcher reduces nested branching without changing behavior:
func (m MainModel) updateSFTPBrowser(msg tea.Msg) (tea.Model, tea.Cmd) {
b := m.sftpBrowser
if b == nil {
m.state = StateServerList
return m, nil
}
keyMsg, ok := msg.(tea.KeyMsg)
if !ok {
return m, nil
}
// Clear status
if !b.isBusy && b.activePanel != TerminalPanel &&
(strings.HasPrefix(b.statusMsg, "Successfully") || strings.HasPrefix(b.statusMsg, "Error")) {
b.statusMsg = ""
}
// Mode routing
if b.deleteConfirm {
return m.handleDeleteConfirm(keyMsg)
}
if b.activePanel == TerminalPanel {
return m.handleTerminalKeys(keyMsg)
}
if b.isBusy {
return m.handleBusyKeys(keyMsg)
}
// Global resize keys
if cmd := m.handleResizeKeys(keyMsg); cmd != nil {
return m, cmd
}
// Default browser keys
return m.handleBrowserKeys(keyMsg)
}Then move existing logic into small helpers (still in the same file):
func (m MainModel) handleDeleteConfirm(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
b := m.sftpBrowser
switch msg.String() {
case "y", "Y":
b.deleteConfirm = false
b.isBusy = true
b.statusMsg = fmt.Sprintf("Deleting %s...", b.deleteItem.Name)
isLocal := b.activePanel == LocalPanel
path := b.deletePath
name := b.deleteItem.Name
return m, func() tea.Msg {
var err error
if isLocal {
err = os.RemoveAll(path)
} else {
err = removeRemoteAll(b.sftpClient, b.sshClient, path)
}
if err != nil {
return sftpProgressMsg{
Done: true,
Err: fmt.Errorf("failed to delete %s: %w", name, err),
}
}
return sftpProgressMsg{
Done: true,
Message: fmt.Sprintf("Successfully deleted %s", name),
}
}
case "n", "N", "esc", "q":
b.deleteConfirm = false
}
return m, nil
}You can similarly extract handleTerminalKeys, handleBusyKeys, and handleBrowserKeys by moving existing switch branches into dedicated functions. This keeps behavior identical but makes each mode’s logic self-contained.
3. Simplify terminal key translation with a map
keyMsgToTerminalInput is a large switch with mostly constant mappings. A small map/table makes it more maintainable:
var ctrlKeyMap = map[tea.KeyType]string{
tea.KeyCtrlA: "\x01",
tea.KeyCtrlB: "\x02",
tea.KeyCtrlC: "\x03",
tea.KeyCtrlD: "\x04",
tea.KeyCtrlE: "\x05",
tea.KeyCtrlF: "\x06",
tea.KeyCtrlG: "\x07",
tea.KeyCtrlH: "\x08",
tea.KeyCtrlJ: "\x0a",
tea.KeyCtrlK: "\x0b",
tea.KeyCtrlL: "\x0c",
tea.KeyCtrlN: "\x0e",
tea.KeyCtrlO: "\x0f",
tea.KeyCtrlP: "\x10",
tea.KeyCtrlQ: "\x11",
tea.KeyCtrlR: "\x12",
tea.KeyCtrlS: "\x13",
// KeyCtrlT deliberately omitted to keep current behavior
tea.KeyCtrlU: "\x15",
tea.KeyCtrlV: "\x16",
tea.KeyCtrlW: "\x17",
tea.KeyCtrlX: "\x18",
tea.KeyCtrlY: "\x19",
tea.KeyCtrlZ: "\x1a",
}
func keyMsgToTerminalInput(msg tea.KeyMsg) string {
switch msg.Type {
case tea.KeyRunes:
return string(msg.Runes)
case tea.KeySpace:
return " "
case tea.KeyEnter:
return "\r"
case tea.KeyBackspace:
return "\x7f"
case tea.KeyTab:
return "\t"
case tea.KeyEscape:
return "\x1b"
case tea.KeyUp:
return "\x1b[A"
case tea.KeyDown:
return "\x1b[B"
case tea.KeyRight:
return "\x1b[C"
case tea.KeyLeft:
return "\x1b[D"
}
if seq, ok := ctrlKeyMap[msg.Type]; ok {
return seq
}
return ""
}This keeps all current behavior (including the special case for KeyCtrlT) but makes future changes simpler.
These changes are small, preserve all functionality, and directly reduce local complexity and duplication within the TUI file without requiring a larger architectural split right now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…and host key verification
Summary by Sourcery
Modularize the application and enhance SSH connectivity to support keys, SSH agent, and safer host key verification while updating the TUI and config handling accordingly.
New Features:
Enhancements:
Documentation:
Tests: