user: fix ToHost treating a non-root uid equal to the remapped-root base as the remapped root

user/idtools.go

@@ -117,22 +117,14 @@ func (i IdentityMapping) RootPair() (int, int) {
 	return uid, gid
 }
 
-// ToHost returns the host UID and GID for the container uid, gid.
-// Remapping is only performed if the ids aren't already the remapped root ids
+// ToHost returns the host UID and GID for the container uid and gid.
+// An empty mapping is treated as identity.
 func (i IdentityMapping) ToHost(uid, gid int) (int, int, error) {
-	var err error
-	ruid, rgid := i.RootPair()
-
-	if uid != ruid {
-		ruid, err = toHost(uid, i.UIDMaps)
-		if err != nil {
-			return ruid, rgid, err
-		}
-	}
-
-	if gid != rgid {
-		rgid, err = toHost(gid, i.GIDMaps)
+	ruid, err := toHost(uid, i.UIDMaps)
+	if err != nil {
+		return -1, -1, err
 	}
+	rgid, err := toHost(gid, i.GIDMaps)
 	return ruid, rgid, err
 }
 

user/idtools_unix_test.go

@@ -358,6 +358,91 @@ func TestGetRootUIDGID(t *testing.T) {
 	}
 }
 
+func TestToHost(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		idMap   []IDMap
+		uid     int
+		gid     int
+		wantUID int
+		wantGID int
+	}{
+		{
+			name:    "standard remap, container root",
+			idMap:   []IDMap{{ID: 0, ParentID: 100000, Count: 65536}},
+			uid:     0,
+			gid:     0,
+			wantUID: 100000,
+			wantGID: 100000,
+		},
+		{
+			name:    "standard remap, non-root user",
+			idMap:   []IDMap{{ID: 0, ParentID: 100000, Count: 65536}},
+			uid:     1000,
+			gid:     1000,
+			wantUID: 101000,
+			wantGID: 101000,
+		},
+		{
+			// Regression test: when the remapped-root base is below the
+			// container uid range, a non-root uid that equals the base must
+			// still be translated, and must not be treated as the remapped
+			// root (which would make the file owned by root inside the
+			// container).
+			name:    "low base remap, non-root uid equal to base",
+			idMap:   []IDMap{{ID: 0, ParentID: 1000, Count: 65536}},
+			uid:     1000,
+			gid:     1000,
+			wantUID: 2000,
+			wantGID: 2000,
+		},
+		{
+			name:    "low base remap, container root",
+			idMap:   []IDMap{{ID: 0, ParentID: 1000, Count: 65536}},
+			uid:     0,
+			gid:     0,
+			wantUID: 1000,
+			wantGID: 1000,
+		},
+		{
+			name:    "no remap (empty mapping)",
+			idMap:   nil,
+			uid:     1000,
+			gid:     1000,
+			wantUID: 1000,
+			wantGID: 1000,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			m := IdentityMapping{UIDMaps: tc.idMap, GIDMaps: tc.idMap}
+			uid, gid, err := m.ToHost(tc.uid, tc.gid)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if uid != tc.wantUID {
+				t.Errorf("uid: got %d, want %d", uid, tc.wantUID)
+			}
+			if gid != tc.wantGID {
+				t.Errorf("gid: got %d, want %d", gid, tc.wantGID)
+			}
+		})
+	}
+}
+
+func TestToHostRemappedRoot(t *testing.T) {
+	// ToHost expects container IDs. A host ID, such as the remapped root
+	// one, is not treated as already remapped, and so it is either mapped
+	// again (if it is in the container range) or results in an error.
+	idMap := []IDMap{{ID: 0, ParentID: 100000, Count: 65536}}
+	m := IdentityMapping{UIDMaps: idMap, GIDMaps: idMap}
+	ruid, rgid := m.RootPair()
+
+	uid, gid, err := m.ToHost(ruid, rgid)
+	if err == nil {
+		t.Fatalf("expected an error, got uid %d, gid %d", uid, gid)
+	}
+}
+
 func TestToContainer(t *testing.T) {
 	uidMap := []IDMap{
 		{