Merge pull request #21692 from Luap99/machine-cleanup

machine init: validate machine name and username

cmd/podman/machine/init.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/containers/common/pkg/completion"
 	"github.com/containers/podman/v5/cmd/podman/registry"
+	ldefine "github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/libpod/events"
 	"github.com/containers/podman/v5/pkg/machine"
 	"github.com/containers/podman/v5/pkg/machine/define"
@@ -136,6 +137,10 @@ func initMachine(cmd *cobra.Command, args []string) error {
 			return fmt.Errorf("machine name %q must be %d characters or less", args[0], maxMachineNameSize)
 		}
 		initOpts.Name = args[0]
+
+		if !ldefine.NameRegex.MatchString(initOpts.Name) {
+			return fmt.Errorf("invalid name %q: %w", initOpts.Name, ldefine.RegexError)
+		}
 	}
 
 	// The vmtype names need to be reserved and cannot be used for podman machine names
@@ -143,6 +148,10 @@ func initMachine(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("cannot use %q for a machine name", initOpts.Name)
 	}
 
+	if !ldefine.NameRegex.MatchString(initOpts.Username) {
+		return fmt.Errorf("invalid username %q: %w", initOpts.Username, ldefine.RegexError)
+	}
+
 	// Check if machine already exists
 	_, exists, err := shim.VMExists(initOpts.Name, []vmconfigs.VMProvider{provider})
 	if err != nil {

pkg/machine/config_test.go

@@ -3,79 +3,12 @@
 package machine
 
 import (
-	"net"
-	"net/url"
 	"path/filepath"
-	"reflect"
 	"testing"
 
-	"github.com/containers/podman/v5/pkg/machine/connection"
 	"github.com/stretchr/testify/assert"
 )
 
-func TestRemoteConnectionType_MakeSSHURL(t *testing.T) {
-	var (
-		host     = "foobar"
-		path     = "/path/to/socket"
-		rc       = "ssh"
-		username = "core"
-	)
-	type args struct {
-		host     string
-		path     string
-		port     string
-		userName string
-	}
-	tests := []struct {
-		name string
-		rc   connection.RemoteConnectionType
-		args args
-		want url.URL
-	}{
-		{
-			name: "Good no port",
-			rc:   "ssh",
-			args: args{
-				host:     host,
-				path:     path,
-				port:     "",
-				userName: username,
-			},
-			want: url.URL{
-				Scheme:     rc,
-				User:       url.User(username),
-				Host:       host,
-				Path:       path,
-				ForceQuery: false,
-			},
-		},
-		{
-			name: "Good with port",
-			rc:   "ssh",
-			args: args{
-				host:     host,
-				path:     path,
-				port:     "222",
-				userName: username,
-			},
-			want: url.URL{
-				Scheme:     rc,
-				User:       url.User(username),
-				Host:       net.JoinHostPort(host, "222"),
-				Path:       path,
-				ForceQuery: false,
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := tt.rc.MakeSSHURL(tt.args.host, tt.args.path, tt.args.port, tt.args.userName); !reflect.DeepEqual(got, tt.want) { //nolint: scopelint
-				t.Errorf("MakeSSHURL() = %v, want %v", got, tt.want) //nolint: scopelint
-			}
-		})
-	}
-}
-
 func TestGetSSHIdentityPath(t *testing.T) {
 	name := "p-test"
 	datadir, err := GetGlobalDataDir()

pkg/machine/connection/add.go

@@ -2,7 +2,6 @@ package connection
 
 import (
 	"fmt"
-	"net/url"
 	"strconv"
 
 	"github.com/containers/podman/v5/pkg/machine/define"
@@ -15,22 +14,25 @@ func AddSSHConnectionsToPodmanSocket(uid, port int, identityPath, name, remoteUs
 		fmt.Println("An ignition path was provided.  No SSH connection was added to Podman")
 		return nil
 	}
-	uri := SSHRemoteConnection.MakeSSHURL(LocalhostIP, fmt.Sprintf("/run/user/%d/podman/podman.sock", uid), strconv.Itoa(port), remoteUsername)
-	uriRoot := SSHRemoteConnection.MakeSSHURL(LocalhostIP, "/run/podman/podman.sock", strconv.Itoa(port), "root")
+	uri := makeSSHURL(LocalhostIP, fmt.Sprintf("/run/user/%d/podman/podman.sock", uid), strconv.Itoa(port), remoteUsername)
+	uriRoot := makeSSHURL(LocalhostIP, "/run/podman/podman.sock", strconv.Itoa(port), "root")
 
-	uris := []url.URL{uri, uriRoot}
-	names := []string{name, name + "-root"}
+	cons := []connection{
+		{
+			name: name,
+			uri:  uri,
+		},
+		{
+			name: name + "-root",
+			uri:  uriRoot,
+		},
+	}
 
 	// The first connection defined when connections is empty will become the default
 	// regardless of IsDefault, so order according to rootful
 	if opts.Rootful {
-		uris[0], names[0], uris[1], names[1] = uris[1], names[1], uris[0], names[0]
+		cons[0], cons[1] = cons[1], cons[0]
 	}
 
-	for i := 0; i < 2; i++ {
-		if err := AddConnection(&uris[i], names[i], identityPath, opts.IsDefault && i == 0); err != nil {
-			return err
-		}
-	}
-	return nil
+	return addConnection(cons, identityPath, opts.IsDefault)
 }

pkg/machine/connection/connection.go

@@ -15,33 +15,40 @@ import (
 
 const LocalhostIP = "127.0.0.1"
 
-func AddConnection(uri fmt.Stringer, name, identity string, isDefault bool) error {
+type connection struct {
+	name string
+	uri  *url.URL
+}
+
+func addConnection(cons []connection, identity string, isDefault bool) error {
 	if len(identity) < 1 {
 		return errors.New("identity must be defined")
 	}
 
 	return config.EditConnectionConfig(func(cfg *config.ConnectionsFile) error {
-		if _, ok := cfg.Connection.Connections[name]; ok {
-			return errors.New("cannot overwrite connection")
-		}
-
-		dst := config.Destination{
-			URI:       uri.String(),
-			IsMachine: true,
-			Identity:  identity,
-		}
-
-		if isDefault {
-			cfg.Connection.Default = name
-		}
-
-		if cfg.Connection.Connections == nil {
-			cfg.Connection.Connections = map[string]config.Destination{
-				name: dst,
+		for i, con := range cons {
+			if _, ok := cfg.Connection.Connections[con.name]; ok {
+				return fmt.Errorf("cannot overwrite connection %q", con.name)
+			}
+
+			dst := config.Destination{
+				URI:       con.uri.String(),
+				IsMachine: true,
+				Identity:  identity,
+			}
+
+			if isDefault && i == 0 {
+				cfg.Connection.Default = con.name
+			}
+
+			if cfg.Connection.Connections == nil {
+				cfg.Connection.Connections = map[string]config.Destination{
+					con.name: dst,
+				}
+				cfg.Connection.Default = con.name
+			} else {
+				cfg.Connection.Connections[con.name] = dst
 			}
-			cfg.Connection.Default = name
-		} else {
-			cfg.Connection.Connections[name] = dst
 		}
 
 		return nil
@@ -108,27 +115,19 @@ func RemoveFilesAndConnections(files []string, names ...string) {
 	}
 }
 
-type RemoteConnectionType string
-
-var SSHRemoteConnection RemoteConnectionType = "ssh"
-
-// MakeSSHURL
-func (rc RemoteConnectionType) MakeSSHURL(host, path, port, userName string) url.URL {
-	// TODO Should this function have input verification?
-	userInfo := url.User(userName)
-	uri := url.URL{
-		Scheme:     "ssh",
-		Opaque:     "",
-		User:       userInfo,
-		Host:       host,
-		Path:       path,
-		RawPath:    "",
-		ForceQuery: false,
-		RawQuery:   "",
-		Fragment:   "",
-	}
+// makeSSHURL creates a URL from the given input
+func makeSSHURL(host, path, port, userName string) *url.URL {
+	var hostname string
 	if len(port) > 0 {
-		uri.Host = net.JoinHostPort(uri.Hostname(), port)
+		hostname = net.JoinHostPort(host, port)
+	} else {
+		hostname = host
+	}
+	userInfo := url.User(userName)
+	return &url.URL{
+		Scheme: "ssh",
+		User:   userInfo,
+		Host:   hostname,
+		Path:   path,
 	}
-	return uri
 }

pkg/machine/connection/connection_test.go

@@ -0,0 +1,68 @@
+//go:build amd64 || arm64
+
+package connection
+
+import (
+	"net"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_makeSSHURL(t *testing.T) {
+	var (
+		host     = "foobar"
+		path     = "/path/to/socket"
+		rc       = "ssh"
+		username = "core"
+	)
+	type args struct {
+		host     string
+		path     string
+		port     string
+		userName string
+	}
+	tests := []struct {
+		name string
+		args args
+		want *url.URL
+	}{
+		{
+			name: "Good no port",
+			args: args{
+				host:     host,
+				path:     path,
+				port:     "",
+				userName: username,
+			},
+			want: &url.URL{
+				Scheme: rc,
+				User:   url.User(username),
+				Host:   host,
+				Path:   path,
+			},
+		},
+		{
+			name: "Good with port",
+			args: args{
+				host:     host,
+				path:     path,
+				port:     "222",
+				userName: username,
+			},
+			want: &url.URL{
+				Scheme: rc,
+				User:   url.User(username),
+				Host:   net.JoinHostPort(host, "222"),
+				Path:   path,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := makeSSHURL(tt.args.host, tt.args.path, tt.args.port, tt.args.userName)
+			assert.Equal(t, tt.want, got, "URL matches")
+		})
+	}
+}

pkg/machine/e2e/init_test.go

@@ -66,7 +66,19 @@ var _ = Describe("podman machine init", func() {
 		Expect(badInit).To(Exit(125))
 		Expect(badInit.errorToString()).To(ContainSubstring(want))
 
+		invalidName := "ab/cd"
+		session, err = mb.setName(invalidName).setCmd(&i).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(125))
+		Expect(session.errorToString()).To(ContainSubstring(`invalid name "ab/cd": names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*: invalid argument`))
+
+		i.username = "-/a"
+		session, err = mb.setName("").setCmd(&i).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(125))
+		Expect(session.errorToString()).To(ContainSubstring(`invalid username "-/a": names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*: invalid argument`))
 	})
+
 	It("simple init", func() {
 		i := new(initMachine)
 		session, err := mb.setCmd(i.withImagePath(mb.imagePath)).run()