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: containers/crun#1147 Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe · Feb 20, 2023 · af8d649 · af8d649
1 parent b55df7f
commit af8d649
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 44 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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") {