Merge pull request #12867 from cdoern/scp

switch podman image scp from depending on machinectl to just os/exec

4 files changed, 64 insertions, 75 deletions

diff --git a/cmd/podman/images/scp.go b/cmd/podman/images/scp.go
index 1481e71c7..81dcda123 100644
--- a/cmd/podman/images/scp.go
+++ b/cmd/podman/images/scp.go
@@ -17,7 +17,6 @@ import (
 	"github.com/containers/podman/v4/cmd/podman/system/connection"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/pkg/domain/entities"
-	"github.com/containers/podman/v4/pkg/rootless"
 	"github.com/containers/podman/v4/utils"
 	scpD "github.com/dtylman/scp"
 	"github.com/pkg/errors"
@@ -337,21 +336,9 @@ func GetServiceInformation(cliConnections []string, cfg *config.Config) (map[str
 
 // execPodman executes the podman save/load command given the podman binary
 func execPodman(podman string, command []string) error {
-	if rootless.IsRootless() {
-		cmd := exec.Command(podman)
-		utils.CreateSCPCommand(cmd, command[1:])
-		logrus.Debug("Executing podman command")
-		return cmd.Run()
-	}
-	machinectl, err := exec.LookPath("machinectl")
-	if err != nil {
-		cmd := exec.Command("su", "-l", "root", "--command")
-		cmd = utils.CreateSCPCommand(cmd, []string{strings.Join(command, " ")})
-		return cmd.Run()
-	}
-	cmd := exec.Command(machinectl, "shell", "-q", "root@.host")
-	cmd = utils.CreateSCPCommand(cmd, command)
-	logrus.Debug("Executing load command machinectl")
+	cmd := exec.Command(podman)
+	utils.CreateSCPCommand(cmd, command[1:])
+	logrus.Debugf("Executing podman command: %q", cmd)
 	return cmd.Run()
 }
 
diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go
index b9c6d3ac7..0b1281aac 100644
--- a/pkg/domain/infra/abi/images.go
+++ b/pkg/domain/infra/abi/images.go
@@ -12,6 +12,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"syscall"
 
 	"github.com/containers/common/libimage"
 	"github.com/containers/common/pkg/config"
@@ -782,7 +783,7 @@ func transferRootless(source entities.ImageScpOptions, dest entities.ImageScpOpt
 	return cmdLoad.Run()
 }
 
-// TransferRootful creates new podman processes using exec.Command and su/machinectl, transferring images between the given source and destination users
+// TransferRootful creates new podman processes using exec.Command and a new uid/gid alongside a cleared environment
 func transferRootful(source entities.ImageScpOptions, dest entities.ImageScpOptions, podman string, parentFlags []string) error {
 	basicCommand := []string{podman}
 	basicCommand = append(basicCommand, parentFlags...)
@@ -794,12 +795,9 @@ func transferRootful(source entities.ImageScpOptions, dest entities.ImageScpOpti
 	}
 	saveCommand = append(saveCommand, []string{"--output", source.File, source.Image}...)
 	loadCommand = append(loadCommand, []string{"--input", dest.File}...)
-	save := []string{strings.Join(saveCommand, " ")}
-	load := []string{strings.Join(loadCommand, " ")}
 
-	// if executing using sudo or transferring between two users, the TransferRootless approach will not work, default to using machinectl or su as necessary.
-	// the approach using sudo is preferable and more straightforward. There is no reason for using sudo in these situations
-	// since the feature is meant to transfer from root to rootless an vice versa without explicit sudo evocaiton.
+	// if executing using sudo or transferring between two users, the TransferRootless approach will not work, the new process needs to be set up
+	// with the proper uid and gid as well as environmental variables.
 	var uSave *user.User
 	var uLoad *user.User
 	var err error
@@ -830,20 +828,11 @@ func transferRootful(source entities.ImageScpOptions, dest entities.ImageScpOpti
 			return err
 		}
 	}
-	machinectl, err := exec.LookPath("machinectl")
-	if err != nil {
-		logrus.Warn("defaulting to su since machinectl is not available, su will fail if no user session is available")
-		err = execSu(uSave, save)
-		if err != nil {
-			return err
-		}
-		return execSu(uLoad, load)
-	}
-	err = execMachine(uSave, saveCommand, machinectl)
+	err = execPodman(uSave, saveCommand)
 	if err != nil {
 		return err
 	}
-	return execMachine(uLoad, loadCommand, machinectl)
+	return execPodman(uLoad, loadCommand)
 }
 
 func lookupUser(u string) (*user.User, error) {
@@ -853,21 +842,37 @@ func lookupUser(u string) (*user.User, error) {
 	return user.Lookup(u)
 }
 
-func execSu(execUser *user.User, command []string) error {
-	cmd := exec.Command("su", "-l", execUser.Username, "--command")
-	cmd = utils.CreateSCPCommand(cmd, command)
-	logrus.Debugf("Executing via su: %q", cmd)
-	return cmd.Run()
-}
-
-func execMachine(execUser *user.User, command []string, machinectl string) error {
-	verb := machinectl
-	args := []string{"shell", "-q", execUser.Username + "@.host"}
-	if execUser.Uid == "0" {
-		args = append([]string{verb}, args...)
-		verb = "sudo"
+func execPodman(execUser *user.User, command []string) error {
+	cmdLogin, err := utils.LoginUser(execUser.Username)
+	if err != nil {
+		return err
+	}
+	defer func() error {
+		err := cmdLogin.Process.Kill()
+		if err != nil {
+			return err
+		}
+		return cmdLogin.Wait()
+	}()
+	cmd := exec.Command(command[0], command[1:]...)
+	cmd.Env = []string{"PATH=" + os.Getenv("PATH"), "TERM=" + os.Getenv("TERM")}
+	cmd.Stderr = os.Stderr
+	cmd.Stdout = os.Stdout
+	uid, err := strconv.ParseInt(execUser.Uid, 10, 32)
+	if err != nil {
+		return err
+	}
+	gid, err := strconv.ParseInt(execUser.Gid, 10, 32)
+	if err != nil {
+		return err
+	}
+	cmd.SysProcAttr = &syscall.SysProcAttr{
+		Credential: &syscall.Credential{
+			Uid:         uint32(uid),
+			Gid:         uint32(gid),
+			Groups:      nil,
+			NoSetGroups: false,
+		},
 	}
-	cmd := utils.CreateSCPCommand(exec.Command(verb, args...), command)
-	logrus.Debugf("Executing via machinectl: %q", cmd)
 	return cmd.Run()
 }
diff --git a/test/system/120-load.bats b/test/system/120-load.bats
index f5ba93d8a..8bf785081 100644
--- a/test/system/120-load.bats
+++ b/test/system/120-load.bats
@@ -104,10 +104,6 @@ verify_iid_and_name() {
     # If we can't sudo, we can't test.
     _sudo true || skip "cannot sudo to $notme"
 
-    # FIXME FIXME FIXME: it'd be reeeeeeally nice if we could pass --root
-    #                    to the non-self user, hence avoid vandalizing
-    #                    their storage.
-
     # Preserve digest of original image; we will compare against it later
     run_podman image inspect --format '{{.Digest}}' $IMAGE
     src_digest=$output
@@ -117,12 +113,8 @@ verify_iid_and_name() {
     run_podman tag $IMAGE $newname
 
     # Copy it there.
-    # FIXME: the first '.*' in the expect string below is unfortunate; it's
-    #        a workaround for Ubuntu which gripes:
-    #        "warning.*defaulting to su since machinectl is not available"
-    #        Reexamine this once #12829 is fixed
     run_podman image scp $newname ${notme}@localhost::
-    is "$output" ".*Copying blob .*Copying config.*Writing manifest.*Storing signatures"
+    is "$output" "Copying blob .*Copying config.*Writing manifest.*Storing signatures"
 
     # confirm that image was copied. FIXME: also try $PODMAN image inspect?
     _sudo $PODMAN image exists $newname
@@ -132,13 +124,6 @@ verify_iid_and_name() {
     run_podman image scp -q ${notme}@localhost::$newname
 
     expect="Loaded image(s): $newname"
-    # FIXME FIXME FIXME: ubuntu has no machinectl, emits useless warning message instead
-    if ! is_rootless; then
-        # FIXME: root on fedora uses machinectl, which emits useless \n and \r (#12829)
-        NL=$'\n'
-        CR=$'\r'
-        expect="$NL$expect$CR"
-    fi
     is "$output" "$expect" "-q silences output"
 
     # Confirm that we have it, and that its digest matches our original
@@ -150,17 +135,14 @@ verify_iid_and_name() {
     run_podman untag $IMAGE $newname
 
     # Negative test for nonexistent image.
-    # FIXME FIXME: cannot test on root, because it uses machinectl (#12829)
-    if is_rootless; then
-        # FIXME: error message is 2 lines, the 2nd being "exit status 125".
-        # FIXME: is that fixable, or do we have to live with it?
-        nope="nope.nope/nonesuch:notag"
-        run_podman 125 image scp ${notme}@localhost::$nope
-        is "$output" "Error: $nope: image not known.*" "Pulling nonexistent image"
-
-        run_podman 125 image scp $nope ${notme}@localhost::
-        is "$output" "Error: $nope: image not known.*" "Pushing nonexistent image"
-    fi
+    # FIXME: error message is 2 lines, the 2nd being "exit status 125".
+    # FIXME: is that fixable, or do we have to live with it?
+    nope="nope.nope/nonesuch:notag"
+    run_podman 125 image scp ${notme}@localhost::$nope
+    is "$output" "Error: $nope: image not known.*" "Pulling nonexistent image"
+
+    run_podman 125 image scp $nope ${notme}@localhost::
+    is "$output" "Error: $nope: image not known.*" "Pushing nonexistent image"
 
     # Negative test for copying to a different name
     run_podman 125 image scp $IMAGE ${notme}@localhost::newname:newtag
diff --git a/utils/utils.go b/utils/utils.go
index caf63c975..52586b937 100644
--- a/utils/utils.go
+++ b/utils/utils.go
@@ -238,3 +238,18 @@ func CreateSCPCommand(cmd *exec.Cmd, command []string) *exec.Cmd {
 	cmd.Stdout = os.Stdout
 	return cmd
 }
+
+// LoginUser starts the user process on the host so that image scp can use systemd-run
+func LoginUser(user string) (*exec.Cmd, error) {
+	sleep, err := exec.LookPath("sleep")
+	if err != nil {
+		return nil, err
+	}
+	machinectl, err := exec.LookPath("machinectl")
+	if err != nil {
+		return nil, err
+	}
+	cmd := exec.Command(machinectl, "shell", "-q", user+"@.host", sleep, "inf")
+	err = cmd.Start()
+	return cmd, err
+}