From a7120b50b1a57edc5b96abda7a47ed23ec94b5ad Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Thu, 14 Nov 2024 16:44:54 +0100
Subject: [PATCH] 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 <pholzing@redhat.com>
---
 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 {