From 7e2e6736378ba0e77eecc060317032641290de12 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Tue, 22 Oct 2024 11:35:47 -0300 Subject: [PATCH 01/14] add utility function for path expansion --- libvirt/util/expandenv.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 libvirt/util/expandenv.go diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go new file mode 100644 index 000000000..3345c2a71 --- /dev/null +++ b/libvirt/util/expandenv.go @@ -0,0 +1,16 @@ +package util + +// ExpandEnvExt expands environment variables and resolves ~ to the home directory +// this is a drop-in replacement for os.ExpandEnv but is additionall '~' aware +func ExpandEnvExt(path string) string { + path = os.ExpandEnv(path) + if strings.HasPrefix(path, "~") { + home, err := os.UserHomeDir() + if err != nil { + return path // return path as-is if unable to resolve home directory + } + // Replace ~ with home directory + path = filepath.Join(home, strings.TrimPrefix(path, "~")) + } + return path +} From e82596467fbfe0ec509e1693c6e6eb13b8993121 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 04:41:08 -0300 Subject: [PATCH 02/14] add unit test for ExpandEnvExt utility function --- libvirt/util/expandenv_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 libvirt/util/expandenv_test.go diff --git a/libvirt/util/expandenv_test.go b/libvirt/util/expandenv_test.go new file mode 100644 index 000000000..27b0865eb --- /dev/null +++ b/libvirt/util/expandenv_test.go @@ -0,0 +1,23 @@ +package util + +import ( + "testing" + "strings" + + "github.com/stretchr/testify/assert" +) + +func TestExpandEnvExt(t *testing.T) { + userHomeDir = func() (string, error) { + return "/home/mock", nil + } + expandEnv = func(s string) string { + return strings.Replace(s, "${HOME}", "/home/mock", 1) + } + + + assert.Equal(t, "foo/bar/baz", ExpandEnvExt("foo/bar/baz")) + assert.Equal(t, "/home/mock/foo/bar/baz", ExpandEnvExt("~/foo/bar/baz")) + assert.Equal(t, "/home/mock/foo/bar/baz", ExpandEnvExt("${HOME}/foo/bar/baz")) + assert.Equal(t, "~foo/bar/baz", ExpandEnvExt("~foo/bar/baz")) +} From f1950b49259ff07d011b95171fd7ff3aba7e91a2 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 04:41:35 -0300 Subject: [PATCH 03/14] update expandenv.go to use variables that can be mock patched --- libvirt/util/expandenv.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index 3345c2a71..6234fcdd6 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -1,11 +1,22 @@ package util +import ( + "os" + "path/filepath" + "strings" +) + +var ( + userHomeDir = os.UserHomeDir + expandEnv = os.ExpandEnv +) + // ExpandEnvExt expands environment variables and resolves ~ to the home directory // this is a drop-in replacement for os.ExpandEnv but is additionall '~' aware func ExpandEnvExt(path string) string { - path = os.ExpandEnv(path) + path = expandEnv(path) if strings.HasPrefix(path, "~") { - home, err := os.UserHomeDir() + home, err := userHomeDir() if err != nil { return path // return path as-is if unable to resolve home directory } From f8b55be20ccbf121576c2792a27877f1388a626c Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 04:42:47 -0300 Subject: [PATCH 04/14] DRY up code by making use of util.ExpandEnvExt --- libvirt/uri/ssh.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/libvirt/uri/ssh.go b/libvirt/uri/ssh.go index 6c617ceff..6a36d2a67 100644 --- a/libvirt/uri/ssh.go +++ b/libvirt/uri/ssh.go @@ -6,13 +6,14 @@ import ( "net" "os" "os/user" - "path/filepath" "strings" "github.com/kevinburke/ssh_config" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" "golang.org/x/crypto/ssh/knownhosts" + + "github.com/dmacvicar/terraform-provider-libvirt/libvirt/util" ) const ( @@ -78,13 +79,7 @@ func (u *ConnectionURI) parseAuthMethods(target string, sshcfg *ssh_config.Confi case "privkey": for _, keypath := range sshKeyPaths { log.Printf("[DEBUG] Reading ssh key '%s'", keypath) - path := os.ExpandEnv(keypath) - if strings.HasPrefix(path, "~/") { - home, err := os.UserHomeDir() - if err == nil { - path = filepath.Join(home, path[2:]) - } - } + path := util.ExpandEnvExt(keypath) sshKey, err := os.ReadFile(path) if err != nil { log.Printf("[ERROR] Failed to read ssh key '%s': %v", keypath, err) @@ -116,7 +111,7 @@ func (u *ConnectionURI) parseAuthMethods(target string, sshcfg *ssh_config.Confi // construct the whole ssh connection, which can consist of multiple hops if using proxy jumps, // the ssh configuration file is loaded once and passed along to each host connection. func (u *ConnectionURI) dialSSH() (net.Conn, error) { - sshConfigFile, err := os.Open(os.ExpandEnv(defaultSSHConfigFile)) + sshConfigFile, err := os.Open(util.ExpandEnvExt(defaultSSHConfigFile)) if err != nil { log.Printf("[WARN] Failed to open ssh config file: %v", err) } @@ -212,11 +207,11 @@ func (u *ConnectionURI) dialHost(target string, sshcfg *ssh_config.Config, depth ssh.KeyAlgoECDSA521, } if !skipVerify { - kh, err := knownhosts.New(os.ExpandEnv(knownHostsPath)) + kh, err := knownhosts.New(util.ExpandEnvExt(knownHostsPath)) if err != nil { return nil, fmt.Errorf("failed to read ssh known hosts: %w", err) } - log.Printf("[DEBUG] Using known hosts file '%s' for target '%s'", os.ExpandEnv(knownHostsPath), target) + log.Printf("[DEBUG] Using known hosts file '%s' for target '%s'", util.ExpandEnvExt(knownHostsPath), target) hostKeyCallback = func(hostname string, remote net.Addr, key ssh.PublicKey) error { err := kh(net.JoinHostPort(hostName, port), remote, key) From 14c3ddb9c5ceb01472505aabb1b94db538ad2ec4 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 05:36:11 -0300 Subject: [PATCH 05/14] remove spurious import --- libvirt/uri/ssh.go | 1 - 1 file changed, 1 deletion(-) diff --git a/libvirt/uri/ssh.go b/libvirt/uri/ssh.go index 97cbd8776..870558a96 100644 --- a/libvirt/uri/ssh.go +++ b/libvirt/uri/ssh.go @@ -5,7 +5,6 @@ import ( "log" "net" "os" - "os/user" "strings" "github.com/kevinburke/ssh_config" From 5d2759f3d40bd13d8c491c360417660f35b6f048 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 05:42:39 -0300 Subject: [PATCH 06/14] fix comment spelling mistake --- libvirt/util/expandenv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index 6234fcdd6..42bbdf0c5 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -12,7 +12,7 @@ var ( ) // ExpandEnvExt expands environment variables and resolves ~ to the home directory -// this is a drop-in replacement for os.ExpandEnv but is additionall '~' aware +// this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware func ExpandEnvExt(path string) string { path = expandEnv(path) if strings.HasPrefix(path, "~") { From d286cf5f168ffb3bee021d1ddec17cef68b3617c Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 05:43:19 -0300 Subject: [PATCH 07/14] fix bug with tilde interpretation (~foo != ~/foo) --- libvirt/util/expandenv.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index 42bbdf0c5..c84aab732 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -15,13 +15,13 @@ var ( // this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware func ExpandEnvExt(path string) string { path = expandEnv(path) - if strings.HasPrefix(path, "~") { + if strings.HasPrefix(path, "~/") { home, err := userHomeDir() if err != nil { return path // return path as-is if unable to resolve home directory } // Replace ~ with home directory - path = filepath.Join(home, strings.TrimPrefix(path, "~")) + path = filepath.Join(home, strings.TrimPrefix(path, "~/")) } return path } From ceee215ad3738cf9dd187d01669082191cc0b9bf Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 05:49:54 -0300 Subject: [PATCH 08/14] add test case for if os.UserHomeDir returns an error --- libvirt/util/expandenv_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libvirt/util/expandenv_test.go b/libvirt/util/expandenv_test.go index 27b0865eb..c29b14a07 100644 --- a/libvirt/util/expandenv_test.go +++ b/libvirt/util/expandenv_test.go @@ -3,6 +3,7 @@ package util import ( "testing" "strings" + "fmt" "github.com/stretchr/testify/assert" ) @@ -20,4 +21,11 @@ func TestExpandEnvExt(t *testing.T) { assert.Equal(t, "/home/mock/foo/bar/baz", ExpandEnvExt("~/foo/bar/baz")) assert.Equal(t, "/home/mock/foo/bar/baz", ExpandEnvExt("${HOME}/foo/bar/baz")) assert.Equal(t, "~foo/bar/baz", ExpandEnvExt("~foo/bar/baz")) + + userHomeDir = func() (string, error) { + return "", fmt.Errorf("some failure") + } + + // failure to get home expansion should leave string unchanged + assert.Equal(t, "~/foo/bar/baz", ExpandEnvExt("~/foo/bar/baz")) } From 4b260c860d478b45ec495579ae850173f4ae905a Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 05:55:21 -0300 Subject: [PATCH 09/14] restore previous redaction --- libvirt/uri/ssh.go | 1 + 1 file changed, 1 insertion(+) diff --git a/libvirt/uri/ssh.go b/libvirt/uri/ssh.go index 870558a96..1a9da85e4 100644 --- a/libvirt/uri/ssh.go +++ b/libvirt/uri/ssh.go @@ -113,6 +113,7 @@ func (u *ConnectionURI) parseAuthMethods(target string, sshcfg *ssh_config.Confi // construct the whole ssh connection, which can consist of multiple hops if using proxy jumps, // the ssh configuration file is loaded once and passed along to each host connection. func (u *ConnectionURI) dialSSH() (net.Conn, error) { + var sshcfg* ssh_config.Config = nil sshConfigFile, err := os.Open(util.ExpandEnvExt(defaultSSHConfigFile)) if err != nil { From 2f9959290c0fe292025bab8e9861ae619e16115d Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 06:00:31 -0300 Subject: [PATCH 10/14] fix comment style --- libvirt/util/expandenv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index c84aab732..56f45c187 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -12,7 +12,7 @@ var ( ) // ExpandEnvExt expands environment variables and resolves ~ to the home directory -// this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware +// this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware. func ExpandEnvExt(path string) string { path = expandEnv(path) if strings.HasPrefix(path, "~/") { From 206bf268ab60236e15250794e5fdb6dc78081b4a Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 06:04:53 -0300 Subject: [PATCH 11/14] update unit tests so that they are platform independent --- libvirt/util/expandenv_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/libvirt/util/expandenv_test.go b/libvirt/util/expandenv_test.go index c29b14a07..11a89e330 100644 --- a/libvirt/util/expandenv_test.go +++ b/libvirt/util/expandenv_test.go @@ -1,9 +1,10 @@ package util import ( - "testing" - "strings" "fmt" + "path/filepath" + "strings" + "testing" "github.com/stretchr/testify/assert" ) @@ -17,15 +18,15 @@ func TestExpandEnvExt(t *testing.T) { } - assert.Equal(t, "foo/bar/baz", ExpandEnvExt("foo/bar/baz")) - assert.Equal(t, "/home/mock/foo/bar/baz", ExpandEnvExt("~/foo/bar/baz")) - assert.Equal(t, "/home/mock/foo/bar/baz", ExpandEnvExt("${HOME}/foo/bar/baz")) - assert.Equal(t, "~foo/bar/baz", ExpandEnvExt("~foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("foo/bar/baz"), ExpandEnvExt("foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("/home/mock/foo/bar/baz"), ExpandEnvExt("~/foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("/home/mock/foo/bar/baz"), ExpandEnvExt("${HOME}/foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("~foo/bar/baz"), ExpandEnvExt("~foo/bar/baz")) userHomeDir = func() (string, error) { return "", fmt.Errorf("some failure") } // failure to get home expansion should leave string unchanged - assert.Equal(t, "~/foo/bar/baz", ExpandEnvExt("~/foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("~/foo/bar/baz"), ExpandEnvExt("~/foo/bar/baz")) } From a5ef98dce153525a9af47a75050dc1f5463211b5 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 10:02:22 -0300 Subject: [PATCH 12/14] bugfix: be platform independent when dealing with filenames previously this code would hand back the path as specified by the caller if it failed (a path which can be a user specified string). However, this would mean that the returned entity would be a string or a filepath depending on the codepath. This behaviour previously existed in the codebase, however it is not clear if it caused any issues anywhere in production. This PR, including the associated unit test shows that under at least some circumstances (github ci) the outcomes are unexpected. --- libvirt/util/expandenv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index 56f45c187..590c9b89e 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -14,7 +14,7 @@ var ( // ExpandEnvExt expands environment variables and resolves ~ to the home directory // this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware. func ExpandEnvExt(path string) string { - path = expandEnv(path) + path = filepath.Clean(expandEnv(path)) if strings.HasPrefix(path, "~/") { home, err := userHomeDir() if err != nil { From bf8e95a101c185ae6bad632f555c845717eaf2e3 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Wed, 23 Oct 2024 10:18:45 -0300 Subject: [PATCH 13/14] bugfix: make unittests pass on windows --- libvirt/util/expandenv.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index 590c9b89e..9a24a4508 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -15,13 +15,20 @@ var ( // this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware. func ExpandEnvExt(path string) string { path = filepath.Clean(expandEnv(path)) - if strings.HasPrefix(path, "~/") { + tilde := filepath.FromSlash("~/") + + // note to maintainers: tilde without a following slash character is simply + // interpreted as part of the filename (e.g. ~foo/bar != ~/foo/bar). However, + // when running on windows, the filepath will be represented by backslashes ('\'), + // therefore we need to convert "~/" to the platform specific format to test for + // it, otherwise on windows systems the prefix test will always fail. + if strings.HasPrefix(path, tilde) { home, err := userHomeDir() if err != nil { return path // return path as-is if unable to resolve home directory } // Replace ~ with home directory - path = filepath.Join(home, strings.TrimPrefix(path, "~/")) + path = filepath.Join(home, strings.TrimPrefix(path, tilde)) } return path } From d366da8c397aaf36be42f2f96d7cfd8631bb2e03 Mon Sep 17 00:00:00 2001 From: Memet Bilgin Date: Mon, 28 Oct 2024 02:46:54 -0300 Subject: [PATCH 14/14] rename ExpandEnvExt to ExpandPath --- libvirt/uri/ssh.go | 8 ++++---- libvirt/util/expandenv.go | 4 ++-- libvirt/util/expandenv_test.go | 12 ++++++------ 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/libvirt/uri/ssh.go b/libvirt/uri/ssh.go index 1a9da85e4..2a42ba540 100644 --- a/libvirt/uri/ssh.go +++ b/libvirt/uri/ssh.go @@ -81,7 +81,7 @@ func (u *ConnectionURI) parseAuthMethods(target string, sshcfg *ssh_config.Confi case "privkey": for _, keypath := range sshKeyPaths { log.Printf("[DEBUG] Reading ssh key '%s'", keypath) - path := util.ExpandEnvExt(keypath) + path := util.ExpandPath(keypath) sshKey, err := os.ReadFile(path) if err != nil { log.Printf("[ERROR] Failed to read ssh key '%s': %v", keypath, err) @@ -114,7 +114,7 @@ func (u *ConnectionURI) parseAuthMethods(target string, sshcfg *ssh_config.Confi // the ssh configuration file is loaded once and passed along to each host connection. func (u *ConnectionURI) dialSSH() (net.Conn, error) { var sshcfg* ssh_config.Config = nil - sshConfigFile, err := os.Open(util.ExpandEnvExt(defaultSSHConfigFile)) + sshConfigFile, err := os.Open(util.ExpandPath(defaultSSHConfigFile)) if err != nil { log.Printf("[WARN] Failed to open ssh config file: %v", err) @@ -216,11 +216,11 @@ func (u *ConnectionURI) dialHost(target string, sshcfg *ssh_config.Config, depth ssh.KeyAlgoECDSA521, } if !skipVerify { - kh, err := knownhosts.New(util.ExpandEnvExt(knownHostsPath)) + kh, err := knownhosts.New(util.ExpandPath(knownHostsPath)) if err != nil { return nil, fmt.Errorf("failed to read ssh known hosts: %w", err) } - log.Printf("[DEBUG] Using known hosts file '%s' for target '%s'", util.ExpandEnvExt(knownHostsPath), target) + log.Printf("[DEBUG] Using known hosts file '%s' for target '%s'", util.ExpandPath(knownHostsPath), target) hostKeyCallback = func(hostname string, remote net.Addr, key ssh.PublicKey) error { err := kh(net.JoinHostPort(hostName, port), remote, key) diff --git a/libvirt/util/expandenv.go b/libvirt/util/expandenv.go index 9a24a4508..ab5450232 100644 --- a/libvirt/util/expandenv.go +++ b/libvirt/util/expandenv.go @@ -11,9 +11,9 @@ var ( expandEnv = os.ExpandEnv ) -// ExpandEnvExt expands environment variables and resolves ~ to the home directory +// ExpandPath expands environment variables and resolves ~ to the home directory // this is a drop-in replacement for os.ExpandEnv but is additionally '~' aware. -func ExpandEnvExt(path string) string { +func ExpandPath(path string) string { path = filepath.Clean(expandEnv(path)) tilde := filepath.FromSlash("~/") diff --git a/libvirt/util/expandenv_test.go b/libvirt/util/expandenv_test.go index 11a89e330..016f4957f 100644 --- a/libvirt/util/expandenv_test.go +++ b/libvirt/util/expandenv_test.go @@ -9,7 +9,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestExpandEnvExt(t *testing.T) { +func TestExpandPath(t *testing.T) { userHomeDir = func() (string, error) { return "/home/mock", nil } @@ -18,15 +18,15 @@ func TestExpandEnvExt(t *testing.T) { } - assert.Equal(t, filepath.FromSlash("foo/bar/baz"), ExpandEnvExt("foo/bar/baz")) - assert.Equal(t, filepath.FromSlash("/home/mock/foo/bar/baz"), ExpandEnvExt("~/foo/bar/baz")) - assert.Equal(t, filepath.FromSlash("/home/mock/foo/bar/baz"), ExpandEnvExt("${HOME}/foo/bar/baz")) - assert.Equal(t, filepath.FromSlash("~foo/bar/baz"), ExpandEnvExt("~foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("foo/bar/baz"), ExpandPath("foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("/home/mock/foo/bar/baz"), ExpandPath("~/foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("/home/mock/foo/bar/baz"), ExpandPath("${HOME}/foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("~foo/bar/baz"), ExpandPath("~foo/bar/baz")) userHomeDir = func() (string, error) { return "", fmt.Errorf("some failure") } // failure to get home expansion should leave string unchanged - assert.Equal(t, filepath.FromSlash("~/foo/bar/baz"), ExpandEnvExt("~/foo/bar/baz")) + assert.Equal(t, filepath.FromSlash("~/foo/bar/baz"), ExpandPath("~/foo/bar/baz")) }