From 598c62864d0a8a4e1b7dcda0cb7a3d5e380a5863 Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Tue, 24 Jan 2023 01:05:31 -0500 Subject: [PATCH] feat(core): Add known hosts callback check for ssh connections (#217) * feat(core): Add known hosts callback check for ssh connections * fix code & add tests --- go.mod | 1 + go.sum | 2 + proxmox/utils.go | 14 ++++ proxmox/utils_test.go | 34 +++++++++ proxmox/virtual_environment_client.go | 9 +-- proxmox/virtual_environment_nodes.go | 71 ++++++++++++------- .../resource_virtual_environment_file.go | 18 +---- 7 files changed, 101 insertions(+), 48 deletions(-) diff --git a/go.mod b/go.mod index 1e6a7ba4..f915feed 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/hashicorp/terraform-plugin-log v0.7.0 github.com/hashicorp/terraform-plugin-sdk/v2 v2.24.1 github.com/pkg/sftp v1.13.5 + github.com/skeema/knownhosts v1.1.0 github.com/stretchr/testify v1.8.1 golang.org/x/crypto v0.5.0 ) diff --git a/go.sum b/go.sum index c4259472..94a621f8 100644 --- a/go.sum +++ b/go.sum @@ -99,6 +99,8 @@ github.com/pkg/sftp v1.13.5 h1:a3RLUqkyjYRtBTZJZ1VRrKbN3zhuPLlUc3sphVz81go= github.com/pkg/sftp v1.13.5/go.mod h1:wHDZ0IZX6JcBYRK1TH9bcVq8G7TLpVHYIGJRFnmPfxg= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/skeema/knownhosts v1.1.0 h1:Wvr9V0MxhjRbl3f9nMnKnFfiWTJmtECJ9Njkea3ysW0= +github.com/skeema/knownhosts v1.1.0/go.mod h1:sKFq3RD6/TKZkSWn8boUbDC7Qkgcv+8XXijpFO6roag= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= diff --git a/proxmox/utils.go b/proxmox/utils.go index 4f4f6238..841029da 100644 --- a/proxmox/utils.go +++ b/proxmox/utils.go @@ -5,12 +5,26 @@ package proxmox import ( + "context" "fmt" + "io" "math" "strconv" "strings" + + "github.com/hashicorp/terraform-plugin-log/tflog" ) +func CloseOrLogError(ctx context.Context) func(io.Closer) { + return func(c io.Closer) { + if err := c.Close(); err != nil { + tflog.Error(ctx, "Failed to close", map[string]interface{}{ + "error": err, + }) + } + } +} + func ParseDiskSize(size *string) (int, error) { if size == nil { return 0, nil diff --git a/proxmox/utils_test.go b/proxmox/utils_test.go index 914f8297..14440ee9 100644 --- a/proxmox/utils_test.go +++ b/proxmox/utils_test.go @@ -1,7 +1,11 @@ package proxmox import ( + "context" + "fmt" "testing" + + "github.com/stretchr/testify/assert" ) func TestParseDiskSize(t *testing.T) { @@ -35,3 +39,33 @@ func TestParseDiskSize(t *testing.T) { }) } } + +func TestCloseOrLogError(t *testing.T) { + t.Parallel() + f := CloseOrLogError(context.Background()) + + c := &testCloser{} + b := &badCloser{} + func() { + defer f(c) + defer f(b) + assert.Equal(t, false, c.isClosed) + }() + + assert.Equal(t, true, c.isClosed) +} + +type testCloser struct { + isClosed bool +} + +func (t *testCloser) Close() error { + t.isClosed = true + return nil +} + +type badCloser struct{} + +func (t *badCloser) Close() error { + return fmt.Errorf("bad") +} diff --git a/proxmox/virtual_environment_client.go b/proxmox/virtual_environment_client.go index 0bce7ac1..75f4a15a 100644 --- a/proxmox/virtual_environment_client.go +++ b/proxmox/virtual_environment_client.go @@ -192,14 +192,7 @@ func (c *VirtualEnvironmentClient) DoRequest( return fErr } - defer func(Body io.ReadCloser) { - err := Body.Close() - if err != nil { - tflog.Error(ctx, "failed to close the response body", map[string]interface{}{ - "error": err.Error(), - }) - } - }(res.Body) + defer CloseOrLogError(ctx)(res.Body) err = c.ValidateResponseCode(res) if err != nil { diff --git a/proxmox/virtual_environment_nodes.go b/proxmox/virtual_environment_nodes.go index 9710a610..6faac9e3 100644 --- a/proxmox/virtual_environment_nodes.go +++ b/proxmox/virtual_environment_nodes.go @@ -8,12 +8,15 @@ import ( "context" "errors" "fmt" + "net" "net/url" + "os" "sort" "strings" "time" "github.com/hashicorp/terraform-plugin-log/tflog" + "github.com/skeema/knownhosts" "golang.org/x/crypto/ssh" ) @@ -24,33 +27,19 @@ func (c *VirtualEnvironmentClient) ExecuteNodeCommands( nodeName string, commands []string, ) error { + closeOrLogError := CloseOrLogError(ctx) + sshClient, err := c.OpenNodeShell(ctx, nodeName) if err != nil { return err } - - defer func(sshClient *ssh.Client) { - err := sshClient.Close() - if err != nil { - tflog.Error(ctx, "Failed to close ssh client", map[string]interface{}{ - "error": err, - }) - } - }(sshClient) + defer closeOrLogError(sshClient) sshSession, err := sshClient.NewSession() if err != nil { return err } - - defer func(sshSession *ssh.Session) { - err := sshSession.Close() - if err != nil { - tflog.Error(ctx, "Failed to close ssh session", map[string]interface{}{ - "error": err, - }) - } - }(sshSession) + defer closeOrLogError(sshSession) output, err := sshSession.CombinedOutput( fmt.Sprintf( @@ -203,15 +192,49 @@ func (c *VirtualEnvironmentClient) OpenNodeShell( ur := strings.Split(c.Username, "@") - sshConfig := &ssh.ClientConfig{ - User: ur[0], - Auth: []ssh.AuthMethod{ssh.Password(c.Password)}, - HostKeyCallback: ssh.InsecureIgnoreHostKey(), + sshHost := fmt.Sprintf("%s:22", *nodeAddress) + khPath := fmt.Sprintf("%s/.ssh/known_hosts", os.Getenv("HOME")) + kh, err := knownhosts.New(khPath) + if err != nil { + return nil, fmt.Errorf("failed to read %s: %w", khPath, err) } - sshClient, err := ssh.Dial("tcp", *nodeAddress+":22", sshConfig) + // Create a custom permissive hostkey callback which still errors on hosts + // with changed keys, but allows unknown hosts and adds them to known_hosts + cb := ssh.HostKeyCallback(func(hostname string, remote net.Addr, key ssh.PublicKey) error { + err := kh(hostname, remote, key) + if knownhosts.IsHostKeyChanged(err) { + return fmt.Errorf("REMOTE HOST IDENTIFICATION HAS CHANGED for host %s! This may indicate a MitM attack", hostname) + } + + if knownhosts.IsHostUnknown(err) { + f, ferr := os.OpenFile(khPath, os.O_APPEND|os.O_WRONLY, 0o600) + if ferr == nil { + defer CloseOrLogError(ctx)(f) + ferr = knownhosts.WriteKnownHost(f, hostname, remote, key) + } + if ferr == nil { + tflog.Info(ctx, fmt.Sprintf("Added host %s to known_hosts", hostname)) + } else { + tflog.Error(ctx, fmt.Sprintf("Failed to add host %s to known_hosts", hostname), map[string]interface{}{ + "error": err, + }) + } + return nil + } + return err + }) + + sshConfig := &ssh.ClientConfig{ + User: ur[0], + Auth: []ssh.AuthMethod{ssh.Password(c.Password)}, + HostKeyCallback: cb, + HostKeyAlgorithms: kh.HostKeyAlgorithms(sshHost), + } + + sshClient, err := ssh.Dial("tcp", sshHost, sshConfig) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to dial %s: %w", sshHost, err) } return sshClient, nil diff --git a/proxmoxtf/resource_virtual_environment_file.go b/proxmoxtf/resource_virtual_environment_file.go index 51158f69..2d560a70 100644 --- a/proxmoxtf/resource_virtual_environment_file.go +++ b/proxmoxtf/resource_virtual_environment_file.go @@ -256,14 +256,7 @@ func resourceVirtualEnvironmentFileCreate( if err != nil { return diag.FromErr(err) } - defer func(Body io.ReadCloser) { - err := Body.Close() - if err != nil { - tflog.Error(ctx, "Failed to close body", map[string]interface{}{ - "error": err, - }) - } - }(res.Body) + defer proxmox.CloseOrLogError(ctx)(res.Body) tempDownloadedFile, err := os.CreateTemp("", "download") if err != nil { @@ -659,14 +652,7 @@ func readURL( return } - defer func(Body io.ReadCloser) { - err := Body.Close() - if err != nil { - tflog.Error(ctx, "failed to close the response body", map[string]interface{}{ - "error": err.Error(), - }) - } - }(res.Body) + defer proxmox.CloseOrLogError(ctx)(res.Body) fileSize = res.ContentLength httpLastModified := res.Header.Get("Last-Modified")