From 412498c11dea8d2933abf05dad73195ee23c50c8 Mon Sep 17 00:00:00 2001 From: Leon Hausdorf Date: Mon, 29 Sep 2025 11:41:42 +0200 Subject: [PATCH 1/2] enh: added default timeouts for ssh key deployment --- internal/deploy/ssh.go | 146 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 133 insertions(+), 13 deletions(-) diff --git a/internal/deploy/ssh.go b/internal/deploy/ssh.go index 71c2a25f..f9e15eb0 100644 --- a/internal/deploy/ssh.go +++ b/internal/deploy/ssh.go @@ -2,9 +2,12 @@ // Keymaster - SSH key management system // This source code is licensed under the MIT license found in the LICENSE file. -// package deploy provides functionality for connecting to remote hosts via SSH +// Package deploy provides functionality for connecting to remote hosts via SSH // and managing their authorized_keys files. This file contains the core SSH and // SFTP client logic for connecting, authenticating, and transferring files. +// +// It includes configurable timeout support and enhanced error classification +// to provide better user feedback when connections fail. package deploy // import "github.com/toeirei/keymaster/internal/deploy" import ( @@ -13,6 +16,7 @@ import ( "io" "net" "path" + "strings" "time" "github.com/pkg/sftp" @@ -20,26 +24,60 @@ import ( "golang.org/x/crypto/ssh" ) +// Default timeout values for SSH operations +const ( + // DefaultConnectionTimeout is the default timeout for establishing SSH connections + DefaultConnectionTimeout = 10 * time.Second + // DefaultCommandTimeout is the default timeout for executing commands + DefaultCommandTimeout = 30 * time.Second + // DefaultHostKeyTimeout is the default timeout for host key retrieval + DefaultHostKeyTimeout = 5 * time.Second + // DefaultSFTPTimeout is the default timeout for SFTP operations + DefaultSFTPTimeout = 60 * time.Second +) + +// ConnectionConfig holds timeout configuration for SSH connections +type ConnectionConfig struct { + ConnectionTimeout time.Duration + CommandTimeout time.Duration + SFTPTimeout time.Duration +} + +// DefaultConnectionConfig returns a ConnectionConfig with default timeout values +func DefaultConnectionConfig() *ConnectionConfig { + return &ConnectionConfig{ + ConnectionTimeout: DefaultConnectionTimeout, + CommandTimeout: DefaultCommandTimeout, + SFTPTimeout: DefaultSFTPTimeout, + } +} + // Deployer handles the connection and deployment to a remote host. type Deployer struct { client *ssh.Client sftp *sftp.Client + config *ConnectionConfig } // NewDeployer creates a new SSH connection and returns a Deployer. // For bootstrap connections, use NewBootstrapDeployer instead. func NewDeployer(host, user, privateKey string) (*Deployer, error) { - return newDeployerInternal(host, user, privateKey, false) + return NewDeployerWithConfig(host, user, privateKey, DefaultConnectionConfig(), false) } // NewBootstrapDeployer creates a new SSH connection for bootstrap operations. // It accepts any host key and saves it to the database for future connections. func NewBootstrapDeployer(host, user, privateKey string) (*Deployer, error) { - return newDeployerInternal(host, user, privateKey, true) + return NewDeployerWithConfig(host, user, privateKey, DefaultConnectionConfig(), true) +} + +// NewDeployerWithConfig creates a new SSH connection with custom timeout configuration. +func NewDeployerWithConfig(host, user, privateKey string, config *ConnectionConfig, isBootstrap bool) (*Deployer, error) { + return newDeployerInternal(host, user, privateKey, config, isBootstrap) } // newDeployerInternal is the internal implementation for creating deployers. -func newDeployerInternal(host, user, privateKey string, isBootstrap bool) (*Deployer, error) { +func newDeployerInternal(host, user, privateKey string, config *ConnectionConfig, isBootstrap bool) (*Deployer, error) { // Define the host key callback based on bootstrap mode. var hostKeyCallback ssh.HostKeyCallback @@ -107,13 +145,13 @@ func newDeployerInternal(host, user, privateKey string, isBootstrap bool) (*Depl if privateKey != "" { signer, err := ssh.ParsePrivateKey([]byte(privateKey)) if err == nil { - config := &ssh.ClientConfig{ + sshConfig := &ssh.ClientConfig{ User: user, Auth: []ssh.AuthMethod{ssh.PublicKeys(signer)}, HostKeyCallback: hostKeyCallback, - Timeout: 10 * time.Second, + Timeout: config.ConnectionTimeout, } - client, err = ssh.Dial("tcp", addr, config) + client, err = ssh.Dial("tcp", addr, sshConfig) if err == nil { // Success! We connected with the system key. sftpClient, sftpErr := sftp.NewClient(client) @@ -121,7 +159,10 @@ func newDeployerInternal(host, user, privateKey string, isBootstrap bool) (*Depl client.Close() return nil, fmt.Errorf("failed to create sftp client: %w", sftpErr) } - return &Deployer{client: client, sftp: sftpClient}, nil + return &Deployer{client: client, sftp: sftpClient, config: config}, nil + } else { + // Classify the error for better debugging + err = ClassifyConnectionError(host, err) } // If we provided a key and it failed, we will fall through to try the agent. } @@ -134,15 +175,16 @@ func newDeployerInternal(host, user, privateKey string, isBootstrap bool) (*Depl return nil, fmt.Errorf("no authentication method available (system key failed and no ssh agent found)") } - config := &ssh.ClientConfig{ + sshConfig := &ssh.ClientConfig{ User: user, Auth: []ssh.AuthMethod{ssh.PublicKeysCallback(agentClient.Signers)}, HostKeyCallback: hostKeyCallback, - Timeout: 10 * time.Second, + Timeout: config.ConnectionTimeout, } - client, err := ssh.Dial("tcp", addr, config) + client, err := ssh.Dial("tcp", addr, sshConfig) if err != nil { + err = ClassifyConnectionError(host, err) return nil, fmt.Errorf("connection with ssh agent failed: %w", err) } @@ -157,6 +199,7 @@ func newDeployerInternal(host, user, privateKey string, isBootstrap bool) (*Depl return &Deployer{ client: client, sftp: sftpClient, + config: config, }, nil } @@ -253,8 +296,84 @@ func (d *Deployer) GetAuthorizedKeys() ([]byte, error) { // in GetRemoteHostKey once the host key has been captured. var ErrHostKeySuccessfullyRetrieved = errors.New("keymaster: successfully retrieved host key") +// Error classification functions for better error handling + +// IsConnectionTimeoutError checks if the error is due to a connection timeout +func IsConnectionTimeoutError(err error) bool { + if err == nil { + return false + } + + errStr := err.Error() + // Check for various timeout-related error messages + return strings.Contains(errStr, "timeout") || + strings.Contains(errStr, "deadline exceeded") || + strings.Contains(errStr, "i/o timeout") +} + +// IsConnectionRefusedError checks if the error is due to connection being refused +func IsConnectionRefusedError(err error) bool { + if err == nil { + return false + } + + errStr := err.Error() + return strings.Contains(errStr, "connection refused") || + strings.Contains(errStr, "no route to host") +} + +// IsAuthenticationError checks if the error is due to authentication failure +func IsAuthenticationError(err error) bool { + if err == nil { + return false + } + + errStr := err.Error() + return strings.Contains(errStr, "authentication failed") || + strings.Contains(errStr, "permission denied") || + strings.Contains(errStr, "public key") || + strings.Contains(errStr, "unable to authenticate") +} + +// IsHostKeyError checks if the error is due to host key verification failure +func IsHostKeyError(err error) bool { + if err == nil { + return false + } + + errStr := err.Error() + return strings.Contains(errStr, "HOST KEY MISMATCH") || + strings.Contains(errStr, "unknown host key") || + strings.Contains(errStr, "host key verification failed") +} + +// ClassifyConnectionError provides a more descriptive error message based on the error type +func ClassifyConnectionError(host string, err error) error { + if err == nil { + return nil + } + + switch { + case IsConnectionTimeoutError(err): + return fmt.Errorf("connection to %s timed out (host may be unreachable or firewall blocking connection): %w", host, err) + case IsConnectionRefusedError(err): + return fmt.Errorf("connection to %s refused (SSH daemon may not be running or wrong port): %w", host, err) + case IsAuthenticationError(err): + return fmt.Errorf("authentication failed for %s (check SSH keys or credentials): %w", host, err) + case IsHostKeyError(err): + return fmt.Errorf("host key verification failed for %s (run 'keymaster trust-host %s' to accept): %w", host, host, err) + default: + return fmt.Errorf("failed to connect to %s: %w", host, err) + } +} + // GetRemoteHostKey connects to a host just to retrieve its public key. func GetRemoteHostKey(host string) (ssh.PublicKey, error) { + return GetRemoteHostKeyWithTimeout(host, DefaultHostKeyTimeout) +} + +// GetRemoteHostKeyWithTimeout connects to a host with a custom timeout to retrieve its public key. +func GetRemoteHostKeyWithTimeout(host string, timeout time.Duration) (ssh.PublicKey, error) { keyChan := make(chan ssh.PublicKey, 1) config := &ssh.ClientConfig{ @@ -266,7 +385,7 @@ func GetRemoteHostKey(host string) (ssh.PublicKey, error) { // Return a specific error to gracefully stop the handshake. return ErrHostKeySuccessfullyRetrieved }, - Timeout: 5 * time.Second, + Timeout: timeout, } addr := host @@ -283,7 +402,8 @@ func GetRemoteHostKey(host string) (ssh.PublicKey, error) { return <-keyChan, nil } // It's a different, real error (e.g., connection refused). - return nil, fmt.Errorf("failed to connect to %s: %w", host, err) + err = ClassifyConnectionError(host, err) + return nil, err } // This case should ideally not be reached if the callback returns an error. From 801cab66013226fce5392501bc12c47fb7ac5978 Mon Sep 17 00:00:00 2001 From: Leon Hausdorf Date: Mon, 29 Sep 2025 11:41:57 +0200 Subject: [PATCH 2/2] test: added ssh deployment tests --- internal/deploy/ssh_test.go | 169 ++++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 internal/deploy/ssh_test.go diff --git a/internal/deploy/ssh_test.go b/internal/deploy/ssh_test.go new file mode 100644 index 00000000..7ac671f4 --- /dev/null +++ b/internal/deploy/ssh_test.go @@ -0,0 +1,169 @@ +// Copyright (c) 2025 ToeiRei +// Keymaster - SSH key management system +// This source code is licensed under the MIT license found in the LICENSE file. + +package deploy + +import ( + "errors" + "testing" +) + +func TestDefaultConnectionConfig(t *testing.T) { + config := DefaultConnectionConfig() + + if config.ConnectionTimeout != DefaultConnectionTimeout { + t.Errorf("Expected ConnectionTimeout %v, got %v", DefaultConnectionTimeout, config.ConnectionTimeout) + } + + if config.CommandTimeout != DefaultCommandTimeout { + t.Errorf("Expected CommandTimeout %v, got %v", DefaultCommandTimeout, config.CommandTimeout) + } + + if config.SFTPTimeout != DefaultSFTPTimeout { + t.Errorf("Expected SFTPTimeout %v, got %v", DefaultSFTPTimeout, config.SFTPTimeout) + } +} + +func TestIsConnectionTimeoutError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + {"nil error", nil, false}, + {"timeout error", errors.New("connection timeout"), true}, + {"deadline exceeded", errors.New("deadline exceeded"), true}, + {"i/o timeout", errors.New("i/o timeout"), true}, + {"other error", errors.New("connection refused"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsConnectionTimeoutError(tt.err) + if result != tt.expected { + t.Errorf("IsConnectionTimeoutError(%v) = %v, expected %v", tt.err, result, tt.expected) + } + }) + } +} + +func TestIsConnectionRefusedError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + {"nil error", nil, false}, + {"connection refused", errors.New("connection refused"), true}, + {"no route to host", errors.New("no route to host"), true}, + {"other error", errors.New("timeout"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsConnectionRefusedError(tt.err) + if result != tt.expected { + t.Errorf("IsConnectionRefusedError(%v) = %v, expected %v", tt.err, result, tt.expected) + } + }) + } +} + +func TestIsAuthenticationError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + {"nil error", nil, false}, + {"authentication failed", errors.New("authentication failed"), true}, + {"permission denied", errors.New("permission denied"), true}, + {"public key error", errors.New("public key authentication failed"), true}, + {"other error", errors.New("timeout"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsAuthenticationError(tt.err) + if result != tt.expected { + t.Errorf("IsAuthenticationError(%v) = %v, expected %v", tt.err, result, tt.expected) + } + }) + } +} + +func TestIsHostKeyError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + {"nil error", nil, false}, + {"host key mismatch", errors.New("HOST KEY MISMATCH"), true}, + {"unknown host key", errors.New("unknown host key"), true}, + {"host key verification failed", errors.New("host key verification failed"), true}, + {"other error", errors.New("timeout"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsHostKeyError(tt.err) + if result != tt.expected { + t.Errorf("IsHostKeyError(%v) = %v, expected %v", tt.err, result, tt.expected) + } + }) + } +} + +func TestClassifyConnectionError(t *testing.T) { + host := "test-host" + + tests := []struct { + name string + err error + expectedMsg string + }{ + {"nil error", nil, ""}, + {"timeout error", errors.New("timeout"), "connection to test-host timed out"}, + {"connection refused", errors.New("connection refused"), "connection to test-host refused"}, + {"authentication failed", errors.New("authentication failed"), "authentication failed for test-host"}, + {"host key error", errors.New("HOST KEY MISMATCH"), "host key verification failed for test-host"}, + {"generic error", errors.New("some other error"), "failed to connect to test-host"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := ClassifyConnectionError(host, tt.err) + if tt.err == nil { + if result != nil { + t.Errorf("Expected nil for nil input, got %v", result) + } + return + } + + if result == nil { + t.Errorf("Expected error, got nil") + return + } + + if !contains(result.Error(), tt.expectedMsg) { + t.Errorf("Expected error message to contain '%s', got '%s'", tt.expectedMsg, result.Error()) + } + }) + } +} + +// Helper function for string containment check +func contains(s, substr string) bool { + return len(s) >= len(substr) && (substr == "" || stringContains(s, substr)) +} + +func stringContains(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +}