From d60757cca685359d935abfc98c5953bb8108999c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 16 Feb 2024 14:41:14 +0100 Subject: [PATCH 1/3] pkg/machine: cleanup MakeSSHURL Remove unnecessary type redirection and just make it a normal function. Also unexport it and move the test as it does not need to be public and remove the default value assignments from the struct. Signed-off-by: Paul Holzinger --- pkg/machine/config_test.go | 67 ---------------------- pkg/machine/connection/add.go | 8 +-- pkg/machine/connection/connection.go | 36 +++++------- pkg/machine/connection/connection_test.go | 68 +++++++++++++++++++++++ 4 files changed, 86 insertions(+), 93 deletions(-) create mode 100644 pkg/machine/connection/connection_test.go diff --git a/pkg/machine/config_test.go b/pkg/machine/config_test.go index 869804ddcb..3450a111f1 100644 --- a/pkg/machine/config_test.go +++ b/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() diff --git a/pkg/machine/connection/add.go b/pkg/machine/connection/add.go index 5a5e150e7a..fe0ed7f3fa 100644 --- a/pkg/machine/connection/add.go +++ b/pkg/machine/connection/add.go @@ -15,10 +15,10 @@ 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} + uris := []*url.URL{uri, uriRoot} names := []string{name, name + "-root"} // The first connection defined when connections is empty will become the default @@ -28,7 +28,7 @@ func AddSSHConnectionsToPodmanSocket(uid, port int, identityPath, name, remoteUs } for i := 0; i < 2; i++ { - if err := AddConnection(&uris[i], names[i], identityPath, opts.IsDefault && i == 0); err != nil { + if err := AddConnection(uris[i], names[i], identityPath, opts.IsDefault && i == 0); err != nil { return err } } diff --git a/pkg/machine/connection/connection.go b/pkg/machine/connection/connection.go index 11f2ba7b18..94dc26569c 100644 --- a/pkg/machine/connection/connection.go +++ b/pkg/machine/connection/connection.go @@ -15,7 +15,7 @@ import ( const LocalhostIP = "127.0.0.1" -func AddConnection(uri fmt.Stringer, name, identity string, isDefault bool) error { +func AddConnection(uri *url.URL, name, identity string, isDefault bool) error { if len(identity) < 1 { return errors.New("identity must be defined") } @@ -108,27 +108,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 } diff --git a/pkg/machine/connection/connection_test.go b/pkg/machine/connection/connection_test.go new file mode 100644 index 0000000000..3cdabf485f --- /dev/null +++ b/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") + }) + } +} From 30a18fc02d157b205891c08aed3a3f251c5f33cc Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 16 Feb 2024 14:58:17 +0100 Subject: [PATCH 2/3] pkg/machine: make only one AddConnection() call This function has to read/write the connections file as such it should only ever be called once otherwise we read/write the same file twice which makes no sense. Also cleanup the fucntion a bit and make it private as there are no external callers. Signed-off-by: Paul Holzinger --- pkg/machine/connection/add.go | 22 +++++++------- pkg/machine/connection/connection.go | 43 ++++++++++++++++------------ 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/pkg/machine/connection/add.go b/pkg/machine/connection/add.go index fe0ed7f3fa..c87550065f 100644 --- a/pkg/machine/connection/add.go +++ b/pkg/machine/connection/add.go @@ -2,7 +2,6 @@ package connection import ( "fmt" - "net/url" "strconv" "github.com/containers/podman/v5/pkg/machine/define" @@ -18,19 +17,22 @@ func AddSSHConnectionsToPodmanSocket(uid, port int, identityPath, name, remoteUs 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) } diff --git a/pkg/machine/connection/connection.go b/pkg/machine/connection/connection.go index 94dc26569c..4c3394a8d2 100644 --- a/pkg/machine/connection/connection.go +++ b/pkg/machine/connection/connection.go @@ -15,33 +15,40 @@ import ( const LocalhostIP = "127.0.0.1" -func AddConnection(uri *url.URL, 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") - } + 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: uri.String(), - IsMachine: true, - Identity: identity, - } + dst := config.Destination{ + URI: con.uri.String(), + IsMachine: true, + Identity: identity, + } - if isDefault { - cfg.Connection.Default = name - } + if isDefault && i == 0 { + cfg.Connection.Default = con.name + } - if cfg.Connection.Connections == nil { - cfg.Connection.Connections = map[string]config.Destination{ - name: dst, + 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 From 2846027dc69205e9da67fccc59612cf78b0b4bd0 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 16 Feb 2024 15:51:50 +0100 Subject: [PATCH 3/3] machine init: validate machine name and username Validate the names with our name regex that we also use for containers/pods. While we technically do not need to be that strict, I think it makes sense to match containers. The most important bit of this validation is that we exclude the use of / and \ which breaks all our file paths as we just use this in the name an when machine write the file it ends up being in a subdir which breaks the reading side. Also other special characters could cause trouble for the URL parsing in the machine connection URL. Signed-off-by: Paul Holzinger --- cmd/podman/machine/init.go | 9 +++++++++ pkg/machine/e2e/init_test.go | 12 ++++++++++++ 2 files changed, 21 insertions(+) diff --git a/cmd/podman/machine/init.go b/cmd/podman/machine/init.go index d897822bbe..88f358d764 100644 --- a/cmd/podman/machine/init.go +++ b/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 { diff --git a/pkg/machine/e2e/init_test.go b/pkg/machine/e2e/init_test.go index 25b0f307e8..83f0b5c9a9 100644 --- a/pkg/machine/e2e/init_test.go +++ b/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()