From af8d649da7ff2fa2bcae1c9e7d169bba62e7013c Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivan@redhat.com>
Date: Mon, 20 Feb 2023 14:05:04 +0100
Subject: [PATCH] libpod: always use direct mapping

always use the direct mapping when writing the mappings for an
idmapped mount.  crun was previously using the reverse mapping, which
is not correct and it is being addressed here:

https://github.com/containers/crun/pull/1147

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
---
 libpod/container_internal.go        |  2 +-
 libpod/container_internal_common.go | 36 ++++++++---------------------
 libpod/container_internal_test.go   | 34 +++++++++++++--------------
 test/e2e/run_userns_test.go         |  2 ++
 4 files changed, 30 insertions(+), 44 deletions(-)

diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index 82f82bcbae..6927ff77da 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -1525,7 +1525,7 @@ func (c *Container) mountStorage() (_ string, deferredErr error) {
 	mountPoint := c.config.Rootfs
 
 	if c.config.RootfsMapping != nil {
-		uidMappings, gidMappings, err := parseIDMapMountOption(c.config.IDMappings, *c.config.RootfsMapping, false)
+		uidMappings, gidMappings, err := parseIDMapMountOption(c.config.IDMappings, *c.config.RootfsMapping)
 		if err != nil {
 			return "", err
 		}
diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go
index 4d1cabb7d5..70562c6c98 100644
--- a/libpod/container_internal_common.go
+++ b/libpod/container_internal_common.go
@@ -74,7 +74,7 @@ func parseOptionIDs(option string) ([]idtools.IDMap, error) {
 	return ret, nil
 }
 
-func parseIDMapMountOption(idMappings stypes.IDMappingOptions, option string, invert bool) ([]spec.LinuxIDMapping, []spec.LinuxIDMapping, error) {
+func parseIDMapMountOption(idMappings stypes.IDMappingOptions, option string) ([]spec.LinuxIDMapping, []spec.LinuxIDMapping, error) {
 	uidMap := idMappings.UIDMap
 	gidMap := idMappings.GIDMap
 	if strings.HasPrefix(option, "idmap=") {
@@ -101,33 +101,17 @@ func parseIDMapMountOption(idMappings stypes.IDMappingOptions, option string, in
 	uidMappings := make([]spec.LinuxIDMapping, len(uidMap))
 	gidMappings := make([]spec.LinuxIDMapping, len(gidMap))
 	for i, uidmap := range uidMap {
-		if invert {
-			uidMappings[i] = spec.LinuxIDMapping{
-				HostID:      uint32(uidmap.ContainerID),
-				ContainerID: uint32(uidmap.HostID),
-				Size:        uint32(uidmap.Size),
-			}
-		} else {
-			uidMappings[i] = spec.LinuxIDMapping{
-				HostID:      uint32(uidmap.HostID),
-				ContainerID: uint32(uidmap.ContainerID),
-				Size:        uint32(uidmap.Size),
-			}
+		uidMappings[i] = spec.LinuxIDMapping{
+			HostID:      uint32(uidmap.HostID),
+			ContainerID: uint32(uidmap.ContainerID),
+			Size:        uint32(uidmap.Size),
 		}
 	}
 	for i, gidmap := range gidMap {
-		if invert {
-			gidMappings[i] = spec.LinuxIDMapping{
-				HostID:      uint32(gidmap.ContainerID),
-				ContainerID: uint32(gidmap.HostID),
-				Size:        uint32(gidmap.Size),
-			}
-		} else {
-			gidMappings[i] = spec.LinuxIDMapping{
-				HostID:      uint32(gidmap.HostID),
-				ContainerID: uint32(gidmap.ContainerID),
-				Size:        uint32(gidmap.Size),
-			}
+		gidMappings[i] = spec.LinuxIDMapping{
+			HostID:      uint32(gidmap.HostID),
+			ContainerID: uint32(gidmap.ContainerID),
+			Size:        uint32(gidmap.Size),
 		}
 	}
 	return uidMappings, gidMappings, nil
@@ -304,7 +288,7 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 		for _, o := range m.Options {
 			if o == "idmap" || strings.HasPrefix(o, "idmap=") {
 				var err error
-				m.UIDMappings, m.GIDMappings, err = parseIDMapMountOption(c.config.IDMappings, o, true)
+				m.UIDMappings, m.GIDMappings, err = parseIDMapMountOption(c.config.IDMappings, o)
 				if err != nil {
 					return nil, err
 				}
diff --git a/libpod/container_internal_test.go b/libpod/container_internal_test.go
index 50103d944c..d8a4b04d7b 100644
--- a/libpod/container_internal_test.go
+++ b/libpod/container_internal_test.go
@@ -65,49 +65,49 @@ func TestParseIDMapMountOption(t *testing.T) {
 		UIDMap: uidMap,
 		GIDMap: gidMap,
 	}
-	uids, gids, err := parseIDMapMountOption(options, "idmap", true)
+	uids, gids, err := parseIDMapMountOption(options, "idmap")
 	assert.Nil(t, err)
 	assert.Equal(t, len(uids), 1)
 	assert.Equal(t, len(gids), 1)
 
-	assert.Equal(t, uids[0].ContainerID, uint32(1000))
-	assert.Equal(t, uids[0].HostID, uint32(0))
+	assert.Equal(t, uids[0].HostID, uint32(1000))
+	assert.Equal(t, uids[0].ContainerID, uint32(0))
 	assert.Equal(t, uids[0].Size, uint32(10000))
 
-	assert.Equal(t, gids[0].ContainerID, uint32(2000))
-	assert.Equal(t, gids[0].HostID, uint32(0))
+	assert.Equal(t, gids[0].HostID, uint32(2000))
+	assert.Equal(t, gids[0].ContainerID, uint32(0))
 	assert.Equal(t, gids[0].Size, uint32(10000))
 
-	uids, gids, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10", true)
+	uids, gids, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10")
 	assert.Nil(t, err)
 	assert.Equal(t, len(uids), 2)
 	assert.Equal(t, len(gids), 1)
 
-	assert.Equal(t, uids[0].ContainerID, uint32(1))
-	assert.Equal(t, uids[0].HostID, uint32(0))
+	assert.Equal(t, uids[0].HostID, uint32(1))
+	assert.Equal(t, uids[0].ContainerID, uint32(0))
 	assert.Equal(t, uids[0].Size, uint32(10))
 
-	assert.Equal(t, uids[1].ContainerID, uint32(11))
-	assert.Equal(t, uids[1].HostID, uint32(10))
+	assert.Equal(t, uids[1].HostID, uint32(11))
+	assert.Equal(t, uids[1].ContainerID, uint32(10))
 	assert.Equal(t, uids[1].Size, uint32(10))
 
-	assert.Equal(t, gids[0].ContainerID, uint32(3))
-	assert.Equal(t, gids[0].HostID, uint32(0))
+	assert.Equal(t, gids[0].HostID, uint32(3))
+	assert.Equal(t, gids[0].ContainerID, uint32(0))
 	assert.Equal(t, gids[0].Size, uint32(10))
 
-	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10;foobar=bar", true)
+	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10;foobar=bar")
 	assert.NotNil(t, err)
 
-	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#0-12", true)
+	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#0-12")
 	assert.NotNil(t, err)
 
-	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#0-12--12", true)
+	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#0-12--12")
 	assert.NotNil(t, err)
 
-	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#-1-12-12", true)
+	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#-1-12-12")
 	assert.NotNil(t, err)
 
-	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#0--12-0", true)
+	_, _, err = parseIDMapMountOption(options, "idmap=uids=0-1-10#10-11-10;gids=0-3-10#0--12-0")
 	assert.NotNil(t, err)
 }
 
diff --git a/test/e2e/run_userns_test.go b/test/e2e/run_userns_test.go
index 1da50f4d7e..75ff186ca5 100644
--- a/test/e2e/run_userns_test.go
+++ b/test/e2e/run_userns_test.go
@@ -109,6 +109,8 @@ var _ = Describe("Podman UserNS support", func() {
 	})
 
 	It("podman uidmapping and gidmapping with an idmapped volume", func() {
+		Skip("it depends on a breaking change in crun: https://github.com/containers/crun/pull/1147")
+
 		session := podmanTest.Podman([]string{"run", "--uidmap=0:1:500", "--gidmap=0:200:5000", "-v", "my-foo-volume:/foo:Z,idmap", "alpine", "stat", "-c", "#%u:%g#", "/foo"})
 		session.WaitWithDefaultTimeout()
 		if strings.Contains(session.ErrorToString(), "Operation not permitted") {