diff --git a/user/idtools.go b/user/idtools.go index 0bed31e..62a6b05 100644 --- a/user/idtools.go +++ b/user/idtools.go @@ -118,21 +118,24 @@ func (i IdentityMapping) RootPair() (int, int) { } // 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 +// +// Every container id is translated through the id map. The container root +// (id 0) maps to the host remapped-root base (the ParentID of the map entry +// covering id 0), because toHost(0) resolves to that base--which is exactly +// what [IdentityMapping.RootPair] returns, so no special case is needed. +// An empty (nil) mapping is treated as identity. +// +// Callers must pass container-namespace ids. ToHost does not treat an id as +// "already remapped" based on the host remapped-root value: doing so would +// incorrectly leave a non-root container uid that happens to equal the host +// remapped-root base unmapped (and therefore owned by the remapped root inside +// the container). 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 ruid, 0, err } + rgid, err := toHost(gid, i.GIDMaps) return ruid, rgid, err } diff --git a/user/idtools_unix_test.go b/user/idtools_unix_test.go index db7fc42..16277de 100644 --- a/user/idtools_unix_test.go +++ b/user/idtools_unix_test.go @@ -358,6 +358,77 @@ 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 TestToContainer(t *testing.T) { uidMap := []IDMap{ {