From 8a5ec2c505392588827d9dad847fd797578d0ffb Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 14 Nov 2024 16:17:50 +0100 Subject: [PATCH 1/4] connection: ignore errors when parsing ssh_config The new ssh_Config feature doesn't work on my system because the lib fails to parse configs using Match[1]. However Fedora and RHEL based distros seem to ship /etc/ssh/ssh_config.d/50-redhat.conf which contains a Match line thus it always fails to parse and never uses the proper values from my home dir config. [1] https://github.com/kevinburke/ssh_config/issues/6 Signed-off-by: Paul Holzinger --- pkg/bindings/connection.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/bindings/connection.go b/pkg/bindings/connection.go index 0fce6c9a27..aa29062779 100644 --- a/pkg/bindings/connection.go +++ b/pkg/bindings/connection.go @@ -170,6 +170,7 @@ func sshClient(_url *url.URL, uri string, identity string, machine bool) (Connec // ssh_config alias := _url.Hostname() cfg := ssh_config.DefaultUserSettings + cfg.IgnoreErrors = true found := false if val := cfg.Get(alias, "User"); val != "" { userinfo = url.User(val) From a7120b50b1a57edc5b96abda7a47ed23ec94b5ad Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 14 Nov 2024 16:44:54 +0100 Subject: [PATCH 2/4] ssh_config: do not overwrite values from config file When we alreadty get a full URL with user, port and identity then we should not read the config file just to overwrite them with wrong values. This is a bad regression for user using * wildcard in their ssh_config as it makes podman machine unusable. Fixes: #24567 Fixes: e523734ab6 ("Add support for ssh_config for connection") Signed-off-by: Paul Holzinger --- pkg/bindings/connection.go | 61 +++++++++++++++++++++------------ pkg/machine/e2e/machine_test.go | 15 +++++++- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/pkg/bindings/connection.go b/pkg/bindings/connection.go index aa29062779..9a12cbebe3 100644 --- a/pkg/bindings/connection.go +++ b/pkg/bindings/connection.go @@ -147,20 +147,14 @@ func NewConnectionWithIdentity(ctx context.Context, uri string, identity string, func sshClient(_url *url.URL, uri string, identity string, machine bool) (Connection, error) { var ( - err error + err error + port int ) connection := Connection{ URI: _url, } userinfo := _url.User - if _url.User == nil { - u, err := user.Current() - if err != nil { - return connection, fmt.Errorf("current user could not be determined: %w", err) - } - userinfo = url.User(u.Username) - } - port := 22 + if _url.Port() != "" { port, err = strconv.Atoi(_url.Port()) if err != nil { @@ -172,29 +166,52 @@ func sshClient(_url *url.URL, uri string, identity string, machine bool) (Connec cfg := ssh_config.DefaultUserSettings cfg.IgnoreErrors = true found := false - if val := cfg.Get(alias, "User"); val != "" { - userinfo = url.User(val) - found = true + + if userinfo == nil { + if val := cfg.Get(alias, "User"); val != "" { + userinfo = url.User(val) + found = true + } + } + // not in url or ssh_config so default to current user + if userinfo == nil { + u, err := user.Current() + if err != nil { + return connection, fmt.Errorf("current user could not be determined: %w", err) + } + userinfo = url.User(u.Username) } + if val := cfg.Get(alias, "Hostname"); val != "" { uri = val found = true } - if val := cfg.Get(alias, "Port"); val != "" { - if val != ssh_config.Default("Port") { - port, err = strconv.Atoi(val) - if err != nil { - return connection, fmt.Errorf("port is not an int: %s: %w", val, err) + + if port == 0 { + if val := cfg.Get(alias, "Port"); val != "" { + if val != ssh_config.Default("Port") { + port, err = strconv.Atoi(val) + if err != nil { + return connection, fmt.Errorf("port is not an int: %s: %w", val, err) + } + found = true } - found = true } } - if val := cfg.Get(alias, "IdentityFile"); val != "" { - if val != ssh_config.Default("IdentityFile") { - identity = strings.Trim(val, "\"") - found = true + // not in ssh config or url so use default 22 port + if port == 0 { + port = 22 + } + + if identity == "" { + if val := cfg.Get(alias, "IdentityFile"); val != "" { + if val != ssh_config.Default("IdentityFile") { + identity = strings.Trim(val, "\"") + found = true + } } } + if found { logrus.Debugf("ssh_config alias found: %s", alias) logrus.Debugf(" User: %s", userinfo.Username()) diff --git a/pkg/machine/e2e/machine_test.go b/pkg/machine/e2e/machine_test.go index ee1d57c3d1..7ef0c60549 100644 --- a/pkg/machine/e2e/machine_test.go +++ b/pkg/machine/e2e/machine_test.go @@ -98,6 +98,19 @@ var _ = SynchronizedAfterSuite(func() {}, func() { } }) +// The config does not matter to much for our testing, however we +// would like to be sure podman machine is not effected by certain +// settings as we should be using full URLs anywhere. +// https://github.com/containers/podman/issues/24567 +const sshConfigContent = ` +Host * + User NOT_REAL + Port 9999 +Host 127.0.0.1 + User blah + IdentityFile ~/.ssh/id_ed25519 +` + func setup() (string, *machineTestBuilder) { // Set TMPDIR if this needs a new directory if value, ok := os.LookupEnv("TMPDIR"); ok { @@ -118,7 +131,7 @@ func setup() (string, *machineTestBuilder) { if err != nil { Fail(fmt.Sprintf("failed to create ssh config: %q", err)) } - if _, err := sshConfig.WriteString("IdentitiesOnly=yes"); err != nil { + if _, err := sshConfig.WriteString(sshConfigContent); err != nil { Fail(fmt.Sprintf("failed to write ssh config: %q", err)) } if err := sshConfig.Close(); err != nil { From cbb2820a7e1c0cb997b8809c6d5cd43f76e4fd7d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 14 Nov 2024 16:52:27 +0100 Subject: [PATCH 3/4] ssh_config: allow IdentityFile file with tilde The ssh_config can contain a path with ~/ to refer to the home dir like done on shells. Handle that special case and resolve the path correctly so it can be used. Signed-off-by: Paul Holzinger --- pkg/bindings/connection.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/bindings/connection.go b/pkg/bindings/connection.go index 9a12cbebe3..762e77d810 100644 --- a/pkg/bindings/connection.go +++ b/pkg/bindings/connection.go @@ -11,6 +11,7 @@ import ( "net/url" "os" "os/user" + "path/filepath" "strconv" "strings" "time" @@ -205,10 +206,15 @@ func sshClient(_url *url.URL, uri string, identity string, machine bool) (Connec if identity == "" { if val := cfg.Get(alias, "IdentityFile"); val != "" { - if val != ssh_config.Default("IdentityFile") { - identity = strings.Trim(val, "\"") - found = true + identity = strings.Trim(val, "\"") + if strings.HasPrefix(identity, "~/") { + homedir, err := os.UserHomeDir() + if err != nil { + return connection, fmt.Errorf("failed to find home dir: %w", err) + } + identity = filepath.Join(homedir, identity[2:]) } + found = true } } From 71f1f52894dd0b549cccf8f95af38bd45b61c914 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 14 Nov 2024 16:56:24 +0100 Subject: [PATCH 4/4] only read ssh_config for non machine connections For machine we know we have all the info we need so there is no reason to read and parse another file. Signed-off-by: Paul Holzinger --- pkg/bindings/connection.go | 105 +++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 50 deletions(-) diff --git a/pkg/bindings/connection.go b/pkg/bindings/connection.go index 762e77d810..abf7ab496e 100644 --- a/pkg/bindings/connection.go +++ b/pkg/bindings/connection.go @@ -162,68 +162,73 @@ func sshClient(_url *url.URL, uri string, identity string, machine bool) (Connec return connection, err } } - // ssh_config - alias := _url.Hostname() - cfg := ssh_config.DefaultUserSettings - cfg.IgnoreErrors = true - found := false - - if userinfo == nil { - if val := cfg.Get(alias, "User"); val != "" { - userinfo = url.User(val) - found = true + + // only parse ssh_config when we are not connecting to a machine + // For machine connections we always have the full URL in the + // system connection so reading the file is just unnecessary. + if !machine { + alias := _url.Hostname() + cfg := ssh_config.DefaultUserSettings + cfg.IgnoreErrors = true + found := false + + if userinfo == nil { + if val := cfg.Get(alias, "User"); val != "" { + userinfo = url.User(val) + found = true + } } - } - // not in url or ssh_config so default to current user - if userinfo == nil { - u, err := user.Current() - if err != nil { - return connection, fmt.Errorf("current user could not be determined: %w", err) + // not in url or ssh_config so default to current user + if userinfo == nil { + u, err := user.Current() + if err != nil { + return connection, fmt.Errorf("current user could not be determined: %w", err) + } + userinfo = url.User(u.Username) } - userinfo = url.User(u.Username) - } - if val := cfg.Get(alias, "Hostname"); val != "" { - uri = val - found = true - } + if val := cfg.Get(alias, "Hostname"); val != "" { + uri = val + found = true + } - if port == 0 { - if val := cfg.Get(alias, "Port"); val != "" { - if val != ssh_config.Default("Port") { - port, err = strconv.Atoi(val) - if err != nil { - return connection, fmt.Errorf("port is not an int: %s: %w", val, err) + if port == 0 { + if val := cfg.Get(alias, "Port"); val != "" { + if val != ssh_config.Default("Port") { + port, err = strconv.Atoi(val) + if err != nil { + return connection, fmt.Errorf("port is not an int: %s: %w", val, err) + } + found = true } - found = true } } - } - // not in ssh config or url so use default 22 port - if port == 0 { - port = 22 - } + // not in ssh config or url so use default 22 port + if port == 0 { + port = 22 + } - if identity == "" { - if val := cfg.Get(alias, "IdentityFile"); val != "" { - identity = strings.Trim(val, "\"") - if strings.HasPrefix(identity, "~/") { - homedir, err := os.UserHomeDir() - if err != nil { - return connection, fmt.Errorf("failed to find home dir: %w", err) + if identity == "" { + if val := cfg.Get(alias, "IdentityFile"); val != "" { + identity = strings.Trim(val, "\"") + if strings.HasPrefix(identity, "~/") { + homedir, err := os.UserHomeDir() + if err != nil { + return connection, fmt.Errorf("failed to find home dir: %w", err) + } + identity = filepath.Join(homedir, identity[2:]) } - identity = filepath.Join(homedir, identity[2:]) + found = true } - found = true } - } - if found { - logrus.Debugf("ssh_config alias found: %s", alias) - logrus.Debugf(" User: %s", userinfo.Username()) - logrus.Debugf(" Hostname: %s", uri) - logrus.Debugf(" Port: %d", port) - logrus.Debugf(" IdentityFile: %q", identity) + if found { + logrus.Debugf("ssh_config alias found: %s", alias) + logrus.Debugf(" User: %s", userinfo.Username()) + logrus.Debugf(" Hostname: %s", uri) + logrus.Debugf(" Port: %d", port) + logrus.Debugf(" IdentityFile: %q", identity) + } } conn, err := ssh.Dial(&ssh.ConnectionDialOptions{ Host: uri,