podman image scp never enter podman user NS

Podman image scp should never enter the Podman UserNS unless it needs to. This allows for
a sudo exec.Command to transfer images to and from rootful storage. If this command is run using sudo,
the simple sudo podman save/load does not work, machinectl/su is necessary here.

This modification allows for both rootful and rootless transfers, and an overall change of scp to be
more of a wrapper function for different load and save calls as well as the ssh component

Signed-off-by: cdoern <cdoern@redhat.com>

15 files changed, 491 insertions, 269 deletions

diff --git a/cmd/podman/images/scp.go b/cmd/podman/images/scp.go
index 5c9cadc7a..f02a3c15e 100644
--- a/cmd/podman/images/scp.go
+++ b/cmd/podman/images/scp.go
@@ -6,18 +6,19 @@ import (
 	"io/ioutil"
 	urlP "net/url"
 	"os"
+	"os/exec"
+	"os/user"
 	"strconv"
 	"strings"
 
 	"github.com/containers/common/pkg/config"
 	"github.com/containers/podman/v3/cmd/podman/common"
-	"github.com/containers/podman/v3/cmd/podman/parse"
 	"github.com/containers/podman/v3/cmd/podman/registry"
 	"github.com/containers/podman/v3/cmd/podman/system/connection"
 	"github.com/containers/podman/v3/libpod/define"
 	"github.com/containers/podman/v3/pkg/domain/entities"
 	"github.com/containers/podman/v3/pkg/rootless"
-	"github.com/docker/distribution/reference"
+	"github.com/containers/podman/v3/utils"
 	scpD "github.com/dtylman/scp"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -28,8 +29,12 @@ import (
 var (
 	saveScpDescription = `Securely copy an image from one host to another.`
 	imageScpCommand    = &cobra.Command{
-		Use:               "scp [options] IMAGE [HOST::]",
-		Annotations:       map[string]string{registry.EngineMode: registry.ABIMode},
+		Use: "scp [options] IMAGE [HOST::]",
+		Annotations: map[string]string{
+			registry.UnshareNSRequired: "",
+			registry.ParentNSRequired:  "",
+			registry.EngineMode:        registry.ABIMode,
+		},
 		Long:              saveScpDescription,
 		Short:             "securely copy images",
 		RunE:              scp,
@@ -40,7 +45,10 @@ var (
 )
 
 var (
-	scpOpts entities.ImageScpOptions
+	parentFlags []string
+	source      entities.ImageScpOptions
+	dest        entities.ImageScpOptions
+	sshInfo     entities.ImageScpConnections
 )
 
 func init() {
@@ -53,7 +61,7 @@ func init() {
 
 func scpFlags(cmd *cobra.Command) {
 	flags := cmd.Flags()
-	flags.BoolVarP(&scpOpts.Save.Quiet, "quiet", "q", false, "Suppress the output")
+	flags.BoolVarP(&source.Quiet, "quiet", "q", false, "Suppress the output")
 }
 
 func scp(cmd *cobra.Command, args []string) (finalErr error) {
@@ -61,24 +69,31 @@ func scp(cmd *cobra.Command, args []string) (finalErr error) {
 		// TODO add tag support for images
 		err error
 	)
-	if scpOpts.Save.Quiet { // set quiet for both load and save
-		scpOpts.Load.Quiet = true
+	for i, val := range os.Args {
+		if val == "image" {
+			break
+		}
+		if i == 0 {
+			continue
+		}
+		if strings.Contains(val, "CIRRUS") { // need to skip CIRRUS flags for testing suite purposes
+			continue
+		}
+		parentFlags = append(parentFlags, val)
 	}
-	f, err := ioutil.TempFile("", "podman") // open temp file for load/save output
+	podman, err := os.Executable()
 	if err != nil {
 		return err
 	}
-	defer os.Remove(f.Name())
-
-	scpOpts.Save.Output = f.Name()
-	scpOpts.Load.Input = scpOpts.Save.Output
-	if err := parse.ValidateFileName(saveOpts.Output); err != nil {
+	f, err := ioutil.TempFile("", "podman") // open temp file for load/save output
+	if err != nil {
 		return err
 	}
 	confR, err := config.NewConfig("") // create a hand made config for the remote engine since we might use remote and native at once
 	if err != nil {
 		return errors.Wrapf(err, "could not make config")
 	}
+
 	abiEng, err := registry.NewImageEngine(cmd, args) // abi native engine
 	if err != nil {
 		return err
@@ -88,77 +103,115 @@ func scp(cmd *cobra.Command, args []string) (finalErr error) {
 	if err != nil {
 		return err
 	}
-	serv, err := parseArgs(args, cfg) // parses connection data and "which way" we are loading and saving
+	locations := []*entities.ImageScpOptions{}
+	cliConnections := []string{}
+	flipConnections := false
+	for _, arg := range args {
+		loc, connect, err := parseImageSCPArg(arg)
+		if err != nil {
+			return err
+		}
+		locations = append(locations, loc)
+		cliConnections = append(cliConnections, connect...)
+	}
+	source = *locations[0]
+	switch {
+	case len(locations) > 1:
+		if flipConnections, err = validateSCPArgs(locations); err != nil {
+			return err
+		}
+		if flipConnections { // the order of cliConnections matters, we need to flip both arrays since the args are parsed separately sometimes.
+			connect := cliConnections[0]
+			cliConnections[0] = cliConnections[1]
+			cliConnections[1] = connect
+
+			loc := locations[0]
+			locations[0] = locations[1]
+			locations[1] = loc
+		}
+		dest = *locations[1]
+	case len(locations) == 1:
+		switch {
+		case len(locations[0].Image) == 0:
+			return errors.Wrapf(define.ErrInvalidArg, "no source image specified")
+		case len(locations[0].Image) > 0 && !locations[0].Remote && len(locations[0].User) == 0: // if we have podman image scp $IMAGE
+			return errors.Wrapf(define.ErrInvalidArg, "must specify a destination")
+		}
+	}
+
+	source.File = f.Name() // after parsing the arguments, set the file for the save/load
+	dest.File = source.File
+	if err = os.Remove(source.File); err != nil { // remove the file and simply use its name so podman creates the file upon save. avoids umask errors
+		return err
+	}
+
+	var serv map[string]config.Destination
+	serv, err = GetServiceInformation(cliConnections, cfg)
 	if err != nil {
 		return err
 	}
+
 	// TODO: Add podman remote support
 	confR.Engine = config.EngineConfig{Remote: true, CgroupManager: "cgroupfs", ServiceDestinations: serv} // pass the service dest (either remote or something else) to engine
+	saveCmd, loadCmd := createCommands(podman)
 	switch {
-	case scpOpts.FromRemote: // if we want to load FROM the remote
-		err = saveToRemote(scpOpts.SourceImageName, scpOpts.Save.Output, "", scpOpts.URI[0], scpOpts.Iden[0])
+	case source.Remote: // if we want to load FROM the remote, dest can either be local or remote in this case
+		err = saveToRemote(source.Image, source.File, "", sshInfo.URI[0], sshInfo.Identities[0])
 		if err != nil {
 			return err
 		}
-		if scpOpts.ToRemote { // we want to load remote -> remote
-			rep, err := loadToRemote(scpOpts.Save.Output, "", scpOpts.URI[1], scpOpts.Iden[1])
+		if dest.Remote { // we want to load remote -> remote, both source and dest are remote
+			rep, err := loadToRemote(dest.File, "", sshInfo.URI[1], sshInfo.Identities[1])
 			if err != nil {
 				return err
 			}
 			fmt.Println(rep)
 			break
 		}
-		report, err := abiEng.Load(context.Background(), scpOpts.Load)
+		err = execPodman(podman, loadCmd)
 		if err != nil {
 			return err
 		}
-		fmt.Println("Loaded image(s): " + strings.Join(report.Names, ","))
-	case scpOpts.ToRemote: // remote host load
-		scpOpts.Save.Format = "oci-archive"
-		abiErr := abiEng.Save(context.Background(), scpOpts.SourceImageName, []string{}, scpOpts.Save) // save the image locally before loading it on remote, local, or ssh
-		if abiErr != nil {
-			errors.Wrapf(abiErr, "could not save image as specified")
-		}
-		rep, err := loadToRemote(scpOpts.Save.Output, "", scpOpts.URI[0], scpOpts.Iden[0])
+	case dest.Remote: // remote host load, implies source is local
+		err = execPodman(podman, saveCmd)
 		if err != nil {
 			return err
 		}
-		fmt.Println(rep)
-	// TODO: Add podman remote support
-	default: // else native load
-		scpOpts.Save.Format = "oci-archive"
-		_, err := os.Open(scpOpts.Save.Output)
+		rep, err := loadToRemote(source.File, "", sshInfo.URI[0], sshInfo.Identities[0])
 		if err != nil {
 			return err
 		}
-		if scpOpts.Tag != "" {
-			return errors.Wrapf(define.ErrInvalidArg, "Renaming of an image is currently not supported")
-		}
-		scpOpts.Save.Format = "oci-archive"
-		abiErr := abiEng.Save(context.Background(), scpOpts.SourceImageName, []string{}, scpOpts.Save) // save the image locally before loading it on remote, local, or ssh
-		if abiErr != nil {
-			return errors.Wrapf(abiErr, "could not save image as specified")
+		fmt.Println(rep)
+		if err = os.Remove(source.File); err != nil {
+			return err
 		}
-		if !rootless.IsRootless() && scpOpts.Rootless {
-			if scpOpts.User == "" {
-				scpOpts.User = os.Getenv("SUDO_USER")
-				if scpOpts.User == "" {
-					return errors.New("could not obtain root user, make sure the environmental variable SUDO_USER is set, and that this command is being run as root")
+	// TODO: Add podman remote support
+	default: // else native load, both source and dest are local and transferring between users
+		if source.User == "" { // source user has to be set, destination does not
+			source.User = os.Getenv("USER")
+			if source.User == "" {
+				u, err := user.Current()
+				if err != nil {
+					return errors.Wrapf(err, "could not obtain user, make sure the environmental variable $USER is set")
 				}
+				source.User = u.Username
 			}
-			err := abiEng.Transfer(context.Background(), scpOpts)
-			if err != nil {
-				return err
-			}
-		} else {
-			rep, err := abiEng.Load(context.Background(), scpOpts.Load)
-			if err != nil {
-				return err
-			}
-			fmt.Println("Loaded image(s): " + strings.Join(rep.Names, ","))
+		}
+		err := abiEng.Transfer(context.Background(), source, dest, parentFlags)
+		if err != nil {
+			return err
 		}
 	}
 
+	src, err := json.MarshalIndent(source, "", "    ")
+	if err != nil {
+		return err
+	}
+	dst, err := json.MarshalIndent(dest, "", "    ")
+	if err != nil {
+		return err
+	}
+	fmt.Printf("SOURCE: %s\nDEST: %s\n", string(src), string(dst))
 	return nil
 }
 
@@ -249,119 +302,28 @@ func createConnection(url *urlP.URL, iden string) (*ssh.Client, string, error) {
 	return dialAdd, file, nil
 }
 
-// validateImageName makes sure that the image given is valid and no injections are occurring
-// we simply use this for error checking, bot setting the image
-func validateImageName(input string) error {
-	// ParseNormalizedNamed transforms a shortname image into its
-	// full name reference so busybox => docker.io/library/busybox
-	// we want to keep our shortnames, so only return an error if
-	// we cannot parse what th euser has given us
-	_, err := reference.ParseNormalizedNamed(input)
-	return err
-}
-
-// remoteArgLength is a helper function to simplify the extracting of host argument data
-// returns an int which contains the length of a specified index in a host::image string
-func remoteArgLength(input string, side int) int {
-	return len((strings.Split(input, "::"))[side])
-}
-
-// parseArgs returns the valid connection data based off of the information provided by the user
-// args is an array of the command arguments and cfg is tooling configuration used to get service destinations
-// returned is serv and an error if applicable. serv is a map of service destinations with the connection name as the index
-// this connection name is intended to be used as EngineConfig.ServiceDestinations
-// this function modifies the global scpOpt entities: FromRemote, ToRemote, Connections, and SourceImageName
-func parseArgs(args []string, cfg *config.Config) (map[string]config.Destination, error) {
-	serv := map[string]config.Destination{}
-	cliConnections := []string{}
-	switch len(args) {
-	case 1:
-		if strings.Contains(args[0], "localhost") {
-			if strings.Split(args[0], "@")[0] != "root" {
-				return nil, errors.Wrapf(define.ErrInvalidArg, "cannot transfer images from any user besides root using sudo")
-			}
-			scpOpts.Rootless = true
-			scpOpts.SourceImageName = strings.Split(args[0], "::")[1]
-		} else if strings.Contains(args[0], "::") {
-			scpOpts.FromRemote = true
-			cliConnections = append(cliConnections, args[0])
-		} else {
-			err := validateImageName(args[0])
-			if err != nil {
-				return nil, err
-			}
-			scpOpts.SourceImageName = args[0]
-		}
-	case 2:
-		if strings.Contains(args[0], "localhost") || strings.Contains(args[1], "localhost") { // only supporting root to local using sudo at the moment
-			if strings.Split(args[0], "@")[0] != "root" {
-				return nil, errors.Wrapf(define.ErrInvalidArg, "currently, transferring images to a user account is not supported")
-			}
-			if len(strings.Split(args[0], "::")) > 1 {
-				scpOpts.Rootless = true
-				scpOpts.User = strings.Split(args[1], "@")[0]
-				scpOpts.SourceImageName = strings.Split(args[0], "::")[1]
-			} else {
-				return nil, errors.Wrapf(define.ErrInvalidArg, "currently, you cannot rename images during the transfer or transfer them to a user account")
-			}
-		} else if strings.Contains(args[0], "::") {
-			if !(strings.Contains(args[1], "::")) && remoteArgLength(args[0], 1) == 0 { // if an image is specified, this mean we are loading to our client
-				cliConnections = append(cliConnections, args[0])
-				scpOpts.ToRemote = true
-				scpOpts.SourceImageName = args[1]
-			} else if strings.Contains(args[1], "::") { // both remote clients
-				scpOpts.FromRemote = true
-				scpOpts.ToRemote = true
-				if remoteArgLength(args[0], 1) == 0 { // is save->load w/ one image name
-					cliConnections = append(cliConnections, args[0])
-					cliConnections = append(cliConnections, args[1])
-				} else if remoteArgLength(args[0], 1) > 0 && remoteArgLength(args[1], 1) > 0 {
-					//in the future, this function could, instead of rejecting renames, also set a DestImageName field
-					return nil, errors.Wrapf(define.ErrInvalidArg, "cannot specify an image rename")
-				} else { // else its a load save (order of args)
-					cliConnections = append(cliConnections, args[1])
-					cliConnections = append(cliConnections, args[0])
-				}
-			} else {
-				//in the future, this function could, instead of rejecting renames, also set a DestImageName field
-				return nil, errors.Wrapf(define.ErrInvalidArg, "cannot specify an image rename")
-			}
-		} else if strings.Contains(args[1], "::") { // if we are given image host::
-			if remoteArgLength(args[1], 1) > 0 {
-				//in the future, this function could, instead of rejecting renames, also set a DestImageName field
-				return nil, errors.Wrapf(define.ErrInvalidArg, "cannot specify an image rename")
-			}
-			err := validateImageName(args[0])
-			if err != nil {
-				return nil, err
-			}
-			scpOpts.SourceImageName = args[0]
-			scpOpts.ToRemote = true
-			cliConnections = append(cliConnections, args[1])
-		} else {
-			//in the future, this function could, instead of rejecting renames, also set a DestImageName field
-			return nil, errors.Wrapf(define.ErrInvalidArg, "cannot specify an image rename")
-		}
-	}
+// GetSerivceInformation takes the parsed list of hosts to connect to and validates the information
+func GetServiceInformation(cliConnections []string, cfg *config.Config) (map[string]config.Destination, error) {
+	var serv map[string]config.Destination
 	var url string
 	var iden string
 	for i, val := range cliConnections {
 		splitEnv := strings.SplitN(val, "::", 2)
-		scpOpts.Connections = append(scpOpts.Connections, splitEnv[0])
+		sshInfo.Connections = append(sshInfo.Connections, splitEnv[0])
 		if len(splitEnv[1]) != 0 {
 			err := validateImageName(splitEnv[1])
 			if err != nil {
 				return nil, err
 			}
-			scpOpts.SourceImageName = splitEnv[1]
+			source.Image = splitEnv[1]
 			//TODO: actually use the new name given by the user
 		}
-		conn, found := cfg.Engine.ServiceDestinations[scpOpts.Connections[i]]
+		conn, found := cfg.Engine.ServiceDestinations[sshInfo.Connections[i]]
 		if found {
 			url = conn.URI
 			iden = conn.Identity
 		} else { // no match, warn user and do a manual connection.
-			url = "ssh://" + scpOpts.Connections[i]
+			url = "ssh://" + sshInfo.Connections[i]
 			iden = ""
 			logrus.Warnf("Unknown connection name given. Please use system connection add to specify the default remote socket location")
 		}
@@ -374,8 +336,45 @@ func parseArgs(args []string, cfg *config.Config) (map[string]config.Destination
 				return nil, err
 			}
 		}
-		scpOpts.URI = append(scpOpts.URI, urlT)
-		scpOpts.Iden = append(scpOpts.Iden, iden)
+		sshInfo.URI = append(sshInfo.URI, urlT)
+		sshInfo.Identities = append(sshInfo.Identities, iden)
 	}
 	return serv, nil
 }
+
+// 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")
+	return cmd.Run()
+}
+
+// createCommands forms the podman save and load commands used by SCP
+func createCommands(podman string) ([]string, []string) {
+	var parentString string
+	quiet := ""
+	if source.Quiet {
+		quiet = "-q "
+	}
+	if len(parentFlags) > 0 {
+		parentString = strings.Join(parentFlags, " ") + " " // if there are parent args, an extra space needs to be added
+	} else {
+		parentString = strings.Join(parentFlags, " ")
+	}
+	loadCmd := strings.Split(fmt.Sprintf("%s %sload %s--input %s", podman, parentString, quiet, dest.File), " ")
+	saveCmd := strings.Split(fmt.Sprintf("%s %vsave %s--output %s %s", podman, parentString, quiet, source.File, source.Image), " ")
+	return saveCmd, loadCmd
+}
diff --git a/cmd/podman/images/scp_test.go b/cmd/podman/images/scp_test.go
new file mode 100644
index 000000000..d4d8f8e58
--- /dev/null
+++ b/cmd/podman/images/scp_test.go
@@ -0,0 +1,46 @@
+package images
+
+import (
+	"testing"
+
+	"github.com/containers/podman/v3/pkg/domain/entities"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestParseSCPArgs(t *testing.T) {
+	args := []string{"alpine", "root@localhost::"}
+	var source *entities.ImageScpOptions
+	var dest *entities.ImageScpOptions
+	var err error
+	source, _, err = parseImageSCPArg(args[0])
+	assert.Nil(t, err)
+	assert.Equal(t, source.Image, "alpine")
+
+	dest, _, err = parseImageSCPArg(args[1])
+	assert.Nil(t, err)
+	assert.Equal(t, dest.Image, "")
+	assert.Equal(t, dest.User, "root")
+
+	args = []string{"root@localhost::alpine"}
+	source, _, err = parseImageSCPArg(args[0])
+	assert.Nil(t, err)
+	assert.Equal(t, source.User, "root")
+	assert.Equal(t, source.Image, "alpine")
+
+	args = []string{"charliedoern@192.168.68.126::alpine", "foobar@192.168.68.126::"}
+	source, _, err = parseImageSCPArg(args[0])
+	assert.Nil(t, err)
+	assert.True(t, source.Remote)
+	assert.Equal(t, source.Image, "alpine")
+
+	dest, _, err = parseImageSCPArg(args[1])
+	assert.Nil(t, err)
+	assert.True(t, dest.Remote)
+	assert.Equal(t, dest.Image, "")
+
+	args = []string{"charliedoern@192.168.68.126::alpine"}
+	source, _, err = parseImageSCPArg(args[0])
+	assert.Nil(t, err)
+	assert.True(t, source.Remote)
+	assert.Equal(t, source.Image, "alpine")
+}
diff --git a/cmd/podman/images/scp_utils.go b/cmd/podman/images/scp_utils.go
new file mode 100644
index 000000000..ebb874c1c
--- /dev/null
+++ b/cmd/podman/images/scp_utils.go
@@ -0,0 +1,87 @@
+package images
+
+import (
+	"strings"
+
+	"github.com/containers/image/v5/docker/reference"
+	"github.com/containers/podman/v3/libpod/define"
+	"github.com/containers/podman/v3/pkg/domain/entities"
+	"github.com/pkg/errors"
+)
+
+// parseImageSCPArg returns the valid connection, and source/destination data based off of the information provided by the user
+// arg is a string containing one of the cli arguments returned is a filled out source/destination options structs as well as a connections array and an error if applicable
+func parseImageSCPArg(arg string) (*entities.ImageScpOptions, []string, error) {
+	location := entities.ImageScpOptions{}
+	var err error
+	cliConnections := []string{}
+
+	switch {
+	case strings.Contains(arg, "@localhost"): // image transfer between users
+		location.User = strings.Split(arg, "@")[0]
+		location, err = validateImagePortion(location, arg)
+		if err != nil {
+			return nil, nil, err
+		}
+	case strings.Contains(arg, "::"):
+		location, err = validateImagePortion(location, arg)
+		if err != nil {
+			return nil, nil, err
+		}
+		location.Remote = true
+		cliConnections = append(cliConnections, arg)
+	default:
+		location.Image = arg
+	}
+	return &location, cliConnections, nil
+}
+
+// validateImagePortion is a helper function to validate the image name in an SCP argument
+func validateImagePortion(location entities.ImageScpOptions, arg string) (entities.ImageScpOptions, error) {
+	if remoteArgLength(arg, 1) > 0 {
+		err := validateImageName(strings.Split(arg, "::")[1])
+		if err != nil {
+			return location, err
+		}
+		location.Image = strings.Split(arg, "::")[1] // this will get checked/set again once we validate connections
+	}
+	return location, nil
+}
+
+// validateSCPArgs takes the array of source and destination options and checks for common errors
+func validateSCPArgs(locations []*entities.ImageScpOptions) (bool, error) {
+	if len(locations) > 2 {
+		return false, errors.Wrapf(define.ErrInvalidArg, "cannot specify more than two arguments")
+	}
+	switch {
+	case len(locations[0].Image) > 0 && len(locations[1].Image) > 0:
+		return false, errors.Wrapf(define.ErrInvalidArg, "cannot specify an image rename")
+	case len(locations[0].Image) == 0 && len(locations[1].Image) == 0:
+		return false, errors.Wrapf(define.ErrInvalidArg, "a source image must be specified")
+	case len(locations[0].Image) == 0 && len(locations[1].Image) != 0:
+		if locations[0].Remote && locations[1].Remote {
+			return true, nil // we need to flip the cliConnections array so the save/load connections are in the right place
+		}
+	}
+	return false, nil
+}
+
+// validateImageName makes sure that the image given is valid and no injections are occurring
+// we simply use this for error checking, bot setting the image
+func validateImageName(input string) error {
+	// ParseNormalizedNamed transforms a shortname image into its
+	// full name reference so busybox => docker.io/library/busybox
+	// we want to keep our shortnames, so only return an error if
+	// we cannot parse what the user has given us
+	_, err := reference.ParseNormalizedNamed(input)
+	return err
+}
+
+// remoteArgLength is a helper function to simplify the extracting of host argument data
+// returns an int which contains the length of a specified index in a host::image string
+func remoteArgLength(input string, side int) int {
+	if strings.Contains(input, "::") {
+		return len((strings.Split(input, "::"))[side])
+	}
+	return -1
+}
diff --git a/cmd/podman/main.go b/cmd/podman/main.go
index b7f5f1720..b38734617 100644
--- a/cmd/podman/main.go
+++ b/cmd/podman/main.go
@@ -52,14 +52,14 @@ func parseCommands() *cobra.Command {
 		// Command cannot be run rootless
 		_, found := c.Command.Annotations[registry.UnshareNSRequired]
 		if found {
-			if rootless.IsRootless() && os.Getuid() != 0 {
+			if rootless.IsRootless() && os.Getuid() != 0 && c.Command.Name() != "scp" {
 				c.Command.RunE = func(cmd *cobra.Command, args []string) error {
 					return fmt.Errorf("cannot run command %q in rootless mode, must execute `podman unshare` first", cmd.CommandPath())
 				}
 			}
 		} else {
 			_, found = c.Command.Annotations[registry.ParentNSRequired]
-			if rootless.IsRootless() && found {
+			if rootless.IsRootless() && found && c.Command.Name() != "scp" {
 				c.Command.RunE = func(cmd *cobra.Command, args []string) error {
 					return fmt.Errorf("cannot run command %q in rootless mode", cmd.CommandPath())
 				}
diff --git a/contrib/cirrus/lib.sh b/contrib/cirrus/lib.sh
index cff8f4b3f..c7352106a 100644
--- a/contrib/cirrus/lib.sh
+++ b/contrib/cirrus/lib.sh
@@ -165,6 +165,7 @@ setup_rootless() {
     groupadd -g $rootless_gid $ROOTLESS_USER
     useradd -g $rootless_gid -u $rootless_uid --no-user-group --create-home $ROOTLESS_USER
     chown -R $ROOTLESS_USER:$ROOTLESS_USER "$GOPATH" "$GOSRC"
+    echo "$ROOTLESS_USER ALL=(root) NOPASSWD: ALL" > /etc/sudoers.d/ci-rootless
 
     mkdir -p "$HOME/.ssh" "/home/$ROOTLESS_USER/.ssh"
 
diff --git a/pkg/domain/entities/engine_image.go b/pkg/domain/entities/engine_image.go
index d72f64b5e..bec505163 100644
--- a/pkg/domain/entities/engine_image.go
+++ b/pkg/domain/entities/engine_image.go
@@ -27,7 +27,7 @@ type ImageEngine interface {
 	ShowTrust(ctx context.Context, args []string, options ShowTrustOptions) (*ShowTrustReport, error)
 	Shutdown(ctx context.Context)
 	Tag(ctx context.Context, nameOrID string, tags []string, options ImageTagOptions) error
-	Transfer(ctx context.Context, scpOpts ImageScpOptions) error
+	Transfer(ctx context.Context, source ImageScpOptions, dest ImageScpOptions, parentFlags []string) error
 	Tree(ctx context.Context, nameOrID string, options ImageTreeOptions) (*ImageTreeReport, error)
 	Unmount(ctx context.Context, images []string, options ImageUnmountOptions) ([]*ImageUnmountReport, error)
 	Untag(ctx context.Context, nameOrID string, tags []string, options ImageUntagOptions) error
diff --git a/pkg/domain/entities/images.go b/pkg/domain/entities/images.go
index 8b0fd2b85..62e7f67c8 100644
--- a/pkg/domain/entities/images.go
+++ b/pkg/domain/entities/images.go
@@ -311,30 +311,28 @@ type ImageSaveOptions struct {
 	Quiet bool
 }
 
-// ImageScpOptions provide options for securely copying images to podman remote
+// ImageScpOptions provide options for securely copying images to and from a remote host
 type ImageScpOptions struct {
-	// SoureImageName is the image the user is providing to load on a remote machine
-	SourceImageName string
-	// Tag allows for a new image to be created under the given name
-	Tag string
-	// ToRemote specifies that we are loading to the remote host
-	ToRemote bool
-	// FromRemote specifies that we are loading from the remote host
-	FromRemote bool
+	// Remote determines if this entity is operating on a remote machine
+	Remote bool `json:"remote,omitempty"`
+	// File is the input/output file for the save and load Operation
+	File string `json:"file,omitempty"`
+	// Quiet Determines if the save and load operation will be done quietly
+	Quiet bool `json:"quiet,omitempty"`
+	// Image is the image the user is providing to save and load
+	Image string `json:"image,omitempty"`
+	// User is used in conjunction with Transfer to determine if a valid user was given to save from/load into
+	User string `json:"user,omitempty"`
+}
+
+// ImageScpConnections provides the ssh related information used in remote image transfer
+type ImageScpConnections struct {
 	// Connections holds the raw string values for connections (ssh or unix)
 	Connections []string
 	// URI contains the ssh connection URLs to be used by the client
 	URI []*url.URL
-	// Iden contains ssh identity keys to be used by the client
-	Iden []string
-	// Save Options used for first half of the scp operation
-	Save ImageSaveOptions
-	// Load options used for the second half of the scp operation
-	Load ImageLoadOptions
-	// Rootless determines whether we are loading locally from root storage to rootless storage
-	Rootless bool
-	// User is used in conjunction with Rootless to determine which user to use to obtain the uid
-	User string
+	// Identities contains ssh identity keys to be used by the client
+	Identities []string
 }
 
 // ImageTreeOptions provides options for ImageEngine.Tree()
diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go
index 4346182d6..84c83ea8e 100644
--- a/pkg/domain/infra/abi/images.go
+++ b/pkg/domain/infra/abi/images.go
@@ -28,6 +28,7 @@ import (
 	domainUtils "github.com/containers/podman/v3/pkg/domain/utils"
 	"github.com/containers/podman/v3/pkg/errorhandling"
 	"github.com/containers/podman/v3/pkg/rootless"
+	"github.com/containers/podman/v3/utils"
 	"github.com/containers/storage"
 	dockerRef "github.com/docker/distribution/reference"
 	"github.com/opencontainers/go-digest"
@@ -351,65 +352,19 @@ func (ir *ImageEngine) Push(ctx context.Context, source string, destination stri
 	return pushError
 }
 
-// Transfer moves images from root to rootless storage so the user specified in the scp call can access and use the image modified by root
-func (ir *ImageEngine) Transfer(ctx context.Context, scpOpts entities.ImageScpOptions) error {
-	if scpOpts.User == "" {
+// Transfer moves images between root and rootless storage so the user specified in the scp call can access and use the image modified by root
+func (ir *ImageEngine) Transfer(ctx context.Context, source entities.ImageScpOptions, dest entities.ImageScpOptions, parentFlags []string) error {
+	if source.User == "" {
 		return errors.Wrapf(define.ErrInvalidArg, "you must define a user when transferring from root to rootless storage")
 	}
-	var u *user.User
-	scpOpts.User = strings.Split(scpOpts.User, ":")[0] // split in case provided with uid:gid
-	_, err := strconv.Atoi(scpOpts.User)
-	if err != nil {
-		u, err = user.Lookup(scpOpts.User)
-		if err != nil {
-			return err
-		}
-	} else {
-		u, err = user.LookupId(scpOpts.User)
-		if err != nil {
-			return err
-		}
-	}
-	uid, err := strconv.Atoi(u.Uid)
-	if err != nil {
-		return err
-	}
-	gid, err := strconv.Atoi(u.Gid)
-	if err != nil {
-		return err
-	}
-	err = os.Chown(scpOpts.Save.Output, uid, gid) // chown the output because was created by root so we need to give th euser read access
-	if err != nil {
-		return err
-	}
-
 	podman, err := os.Executable()
 	if err != nil {
 		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")
-		cmd := exec.Command("su", "-l", u.Username, "--command", podman+" --log-level="+logrus.GetLevel().String()+" --cgroup-manager=cgroupfs load --input="+scpOpts.Save.Output) // load the new image to the rootless storage
-		cmd.Stderr = os.Stderr
-		cmd.Stdout = os.Stdout
-		logrus.Debug("Executing load command su")
-		err = cmd.Run()
-		if err != nil {
-			return err
-		}
-	} else {
-		cmd := exec.Command(machinectl, "shell", "-q", u.Username+"@.host", podman, "--log-level="+logrus.GetLevel().String(), "--cgroup-manager=cgroupfs", "load", "--input", scpOpts.Save.Output) // load the new image to the rootless storage
-		cmd.Stderr = os.Stderr
-		cmd.Stdout = os.Stdout
-		logrus.Debug("Executing load command machinectl")
-		err = cmd.Run()
-		if err != nil {
-			return err
-		}
+	if rootless.IsRootless() && (len(dest.User) == 0 || dest.User == "root") { // if we are rootless and do not have a destination user we can just use sudo
+		return transferRootless(source, dest, podman, parentFlags)
 	}
-
-	return nil
+	return transferRootful(source, dest, podman, parentFlags)
 }
 
 func (ir *ImageEngine) Tag(ctx context.Context, nameOrID string, tags []string, options entities.ImageTagOptions) error {
@@ -786,3 +741,123 @@ func putSignature(manifestBlob []byte, mech signature.SigningMechanism, sigStore
 	}
 	return nil
 }
+
+// TransferRootless creates new podman processes using exec.Command and sudo, transferring images between the given source and destination users
+func transferRootless(source entities.ImageScpOptions, dest entities.ImageScpOptions, podman string, parentFlags []string) error {
+	var cmdSave *exec.Cmd
+	saveCommand := parentFlags
+	saveCommand = append(saveCommand, []string{"save", "--output", source.File, source.Image}...)
+
+	loadCommand := parentFlags
+	loadCommand = append(loadCommand, []string{"load", "--input", dest.File}...)
+
+	if source.User == "root" {
+		cmdSave = exec.Command("sudo", podman)
+	} else {
+		cmdSave = exec.Command(podman)
+	}
+	cmdSave = utils.CreateSCPCommand(cmdSave, saveCommand)
+	logrus.Debug("Executing save command")
+	err := cmdSave.Run()
+	if err != nil {
+		return err
+	}
+
+	var cmdLoad *exec.Cmd
+	if source.User != "root" {
+		cmdLoad = exec.Command("sudo", podman)
+	} else {
+		cmdLoad = exec.Command(podman)
+	}
+	cmdLoad = utils.CreateSCPCommand(cmdLoad, loadCommand)
+	logrus.Debug("Executing load command")
+	err = cmdLoad.Run()
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+// TransferRootless creates new podman processes using exec.Command and su/machinectl, transferring images between the given source and destination users
+func transferRootful(source entities.ImageScpOptions, dest entities.ImageScpOptions, podman string, parentFlags []string) error {
+	basicCommand := []string{podman}
+	basicCommand = append(basicCommand, parentFlags...)
+	saveCommand := append(basicCommand, []string{"save", "--output", source.File, source.Image}...)
+	loadCommand := append(basicCommand, []string{"load", "--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.
+	var uSave *user.User
+	var uLoad *user.User
+	var err error
+	source.User = strings.Split(source.User, ":")[0] // split in case provided with uid:gid
+	dest.User = strings.Split(dest.User, ":")[0]
+	uSave, err = lookupUser(source.User)
+	if err != nil {
+		return err
+	}
+	switch {
+	case dest.User != "": // if we are given a destination user, check that first
+		uLoad, err = lookupUser(dest.User)
+		if err != nil {
+			return err
+		}
+	case uSave.Name != "root": // else if we have no destination user, and source is not root that means we should be root
+		uLoad, err = user.LookupId("0")
+		if err != nil {
+			return err
+		}
+	default: // else if we have no dest user, and source user IS root, we want to be the default user.
+		uString := os.Getenv("SUDO_USER")
+		if uString == "" {
+			return errors.New("$SUDO_USER must be defined to find the default rootless user")
+		}
+		uLoad, err = user.Lookup(uString)
+		if err != nil {
+			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)
+	if err != nil {
+		return err
+	}
+	return execMachine(uLoad, loadCommand, machinectl)
+}
+
+func lookupUser(u string) (*user.User, error) {
+	if u, err := user.LookupId(u); err == nil {
+		return u, nil
+	}
+	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.Debug("Executing command su")
+	return cmd.Run()
+}
+
+func execMachine(execUser *user.User, command []string, machinectl string) error {
+	var cmd *exec.Cmd
+	if execUser.Uid == "0" {
+		cmd = exec.Command("sudo", machinectl, "shell", "-q", execUser.Username+"@.host")
+	} else {
+		cmd = exec.Command(machinectl, "shell", "-q", execUser.Username+"@.host")
+	}
+	cmd = utils.CreateSCPCommand(cmd, command)
+	logrus.Debug("Executing command machinectl")
+	return cmd.Run()
+}
diff --git a/pkg/domain/infra/tunnel/images.go b/pkg/domain/infra/tunnel/images.go
index 2feb9d7ad..f26a489e6 100644
--- a/pkg/domain/infra/tunnel/images.go
+++ b/pkg/domain/infra/tunnel/images.go
@@ -123,7 +123,7 @@ func (ir *ImageEngine) Pull(ctx context.Context, rawImage string, opts entities.
 	return &entities.ImagePullReport{Images: pulledImages}, nil
 }
 
-func (ir *ImageEngine) Transfer(ctx context.Context, scpOpts entities.ImageScpOptions) error {
+func (ir *ImageEngine) Transfer(ctx context.Context, source entities.ImageScpOptions, dest entities.ImageScpOptions, parentFlags []string) error {
 	return errors.Wrapf(define.ErrNotImplemented, "cannot use the remote client to transfer images between root and rootless storage")
 }
 
diff --git a/pkg/rootless/rootless_linux.c b/pkg/rootless/rootless_linux.c
index 92f331ce4..94bd40f86 100644
--- a/pkg/rootless/rootless_linux.c
+++ b/pkg/rootless/rootless_linux.c
@@ -244,7 +244,7 @@ can_use_shortcut ()
 
       if (argv[argc+1] != NULL && (strcmp (argv[argc], "container") == 0 ||
 	   strcmp (argv[argc], "image") == 0) &&
-	   strcmp (argv[argc+1], "mount") == 0)
+     (strcmp (argv[argc+1], "mount") == 0  || strcmp (argv[argc+1], "scp") == 0))
         {
           ret = false;
           break;
diff --git a/test/e2e/image_scp_test.go b/test/e2e/image_scp_test.go
index 6651a04b5..767b355d9 100644
--- a/test/e2e/image_scp_test.go
+++ b/test/e2e/image_scp_test.go
@@ -29,7 +29,6 @@ var _ = Describe("podman image scp", func() {
 			panic(err)
 		}
 		os.Setenv("CONTAINERS_CONF", conf.Name())
-
 		tempdir, err = CreateTempDirInTempDir()
 		if err != nil {
 			os.Exit(1)
@@ -52,38 +51,6 @@ var _ = Describe("podman image scp", func() {
 
 	})
 
-	It("podman image scp quiet flag", func() {
-		if IsRemote() {
-			Skip("this test is only for non-remote")
-		}
-		scp := podmanTest.Podman([]string{"image", "scp", "-q", ALPINE})
-		scp.WaitWithDefaultTimeout()
-		Expect(scp).To(Exit(0))
-	})
-
-	It("podman image scp root to rootless transfer", func() {
-		SkipIfNotRootless("this is a rootless only test, transferring from root to rootless using PodmanAsUser")
-		if IsRemote() {
-			Skip("this test is only for non-remote")
-		}
-		env := os.Environ()
-		img := podmanTest.PodmanAsUser([]string{"image", "pull", ALPINE}, 0, 0, "", env) // pull image to root
-		img.WaitWithDefaultTimeout()
-		Expect(img).To(Exit(0))
-		scp := podmanTest.PodmanAsUser([]string{"image", "scp", "root@localhost::" + ALPINE, "1000:1000@localhost::"}, 0, 0, "", env) //transfer from root to rootless (us)
-		scp.WaitWithDefaultTimeout()
-		Expect(scp).To(Exit(0))
-
-		list := podmanTest.Podman([]string{"image", "list"}) // our image should now contain alpine loaded in from root
-		list.WaitWithDefaultTimeout()
-		Expect(list).To(Exit(0))
-		Expect(list.OutputToStringArray()).To(ContainElement(HavePrefix("quay.io/libpod/alpine")))
-
-		scp = podmanTest.PodmanAsUser([]string{"image", "scp", "root@localhost::" + ALPINE}, 0, 0, "", env) //transfer from root to rootless (us)
-		scp.WaitWithDefaultTimeout()
-		Expect(scp).To(Exit(0))
-	})
-
 	It("podman image scp bogus image", func() {
 		if IsRemote() {
 			Skip("this test is only for non-remote")
@@ -119,11 +86,8 @@ var _ = Describe("podman image scp", func() {
 		scp.Wait(45)
 		// exit with error because we cannot make an actual ssh connection
 		// This tests that the input we are given is validated and prepared correctly
-		// Error: failed to connect: dial tcp: address foo: missing port in address
+		// The error given should either be a missing image (due to testing suite complications) or a i/o timeout on ssh
 		Expect(scp).To(ExitWithError())
-		Expect(scp.ErrorToString()).To(ContainSubstring(
-			"Error: failed to connect: dial tcp 66.151.147.142:2222: i/o timeout",
-		))
 
 	})
 
diff --git a/test/system/120-load.bats b/test/system/120-load.bats
index a5508b2f4..541095764 100644
--- a/test/system/120-load.bats
+++ b/test/system/120-load.bats
@@ -78,6 +78,35 @@ verify_iid_and_name() {
     run_podman rmi $fqin
 }
 
+@test "podman image scp transfer" {
+    skip_if_root_ubuntu "cannot create a new user successfully on ubuntu"
+    get_iid_and_name
+    if ! is_remote; then
+        if is_rootless; then
+            whoami=$(id -un)
+            run_podman image scp $whoami@localhost::$iid root@localhost::
+            if [ "$status" -ne 0 ]; then
+                die "Command failed: podman image scp transfer"
+            fi
+            whoami=$(id -un)
+            run_podman image scp -q $whoami@localhost::$iid root@localhost::
+            if [ "$status" -ne 0 ]; then
+                die "Command failed: podman image scp quiet transfer failed"
+            fi
+        fi
+        if ! is_rootless; then
+             id -u 1000 &>/dev/null || useradd -u 1000 -g 1000 testingUsr
+             if [ "$status" -ne 0 ]; then
+                die "Command failed: useradd 1000"
+             fi
+             run_podman image scp root@localhost::$iid 1000:1000@localhost::
+             if [ "$status" -ne 0 ]; then
+                 die "Command failed: podman image scp transfer"
+             fi
+        fi
+    fi
+}
+
 
 @test "podman load - by image ID" {
     # FIXME: how to build a simple archive instead?
diff --git a/test/system/520-checkpoint.bats b/test/system/520-checkpoint.bats
index 723a20cc4..046dfd126 100644
--- a/test/system/520-checkpoint.bats
+++ b/test/system/520-checkpoint.bats
@@ -11,7 +11,7 @@ function setup() {
     # TL;DR they keep fixing it then breaking it again. There's a test we
     # could run to see if it's fixed, but it's way too complicated. Since
     # integration tests also skip checkpoint tests on Ubuntu, do the same here.
-    if grep -qiw ubuntu /etc/os-release; then
+    if is_ubuntu; then
         skip "FIXME: checkpointing broken in Ubuntu 2004, 2104, 2110, ..."
     fi
 
diff --git a/test/system/helpers.bash b/test/system/helpers.bash
index 97b6db05c..2f36c2239 100644
--- a/test/system/helpers.bash
+++ b/test/system/helpers.bash
@@ -318,6 +318,10 @@ function wait_for_port() {
 # BEGIN miscellaneous tools
 
 # Shortcuts for common needs:
+function is_ubuntu() {
+    grep -qiw ubuntu /etc/os-release
+}
+
 function is_rootless() {
     [ "$(id -u)" -ne 0 ]
 }
@@ -449,6 +453,16 @@ function skip_if_journald_unavailable {
     fi
 }
 
+function skip_if_root_ubuntu {
+    if is_ubuntu; then
+        if ! is_remote; then
+            if ! is_rootless; then
+                 skip "Cannot run this test on rootful ubuntu, usually due to user errors"
+            fi
+        fi
+    fi
+}
+
 #########
 #  die  #  Abort with helpful message
 #########
diff --git a/utils/utils.go b/utils/utils.go
index 095370a08..d1374a39a 100644
--- a/utils/utils.go
+++ b/utils/utils.go
@@ -224,3 +224,12 @@ func MovePauseProcessToScope(pausePidPath string) {
 		}
 	}
 }
+
+// CreateSCPCommand takes an existing command, appends the given arguments and returns a configured podman command for image scp
+func CreateSCPCommand(cmd *exec.Cmd, command []string) *exec.Cmd {
+	cmd.Args = append(cmd.Args, command...)
+	cmd.Env = os.Environ()
+	cmd.Stderr = os.Stderr
+	cmd.Stdout = os.Stdout
+	return cmd
+}