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

[okhowang]

fixes #241

ToHost translated container ids to host ids, but special-cased the
container root by skipping the id-map lookup whenever the container uid
matched the host remapped-root base (RootPair). That comparison was
wrong: the input is a container-namespace id, so it must never be
compared against a host value.

When the subuid/subgid base is below 65536 (e.g. 'rootless:1000:65536',
used to align dind-rootless container uids), a non-root container uid
that equals the base (e.g. 1000) was incorrectly left unmapped and
therefore appeared as root inside the container.

Drop the special case entirely and always translate every id through
toHost. toHost(0) already resolves to the host remapped-root base (which
is exactly what RootPair returns), so the container root is handled
correctly without any guard. An empty (nil) mapping is treated as
identity.

Add a regression test covering standard, low-base, and empty mappings.

[kolyshkin]

The fix looks correct to me, but @thaJeztah's concern is valid: there is one caller in moby that depends on the current behavior.

The classic builder's COPY --from copies files from another build stage or image, whose layers (with graphdriver and userns-remap) are already owned by host (remapped) IDs, yet it remaps them again via Archiver.CopyWithTar / CopyFileWithTar → ToHost. Today this works for root-owned files only because of the special case being removed here (non-root files already fail with container ID 100100 cannot be mapped to a host ID). With this PR, it would fail for all files.

I've opened moby/moby#53859 to fix that on the moby side. Other ToHost callers in moby and buildkit pass container IDs, so they only benefit from this fix.

A few nits for this PR:

1. On a uid mapping error, ToHost now returns ruid, 0, err, i.e. gid 0 (root). Better to return -1, -1, err, consistent with ToContainer (and with what was suggested in [userns-remap] Bug: ToHost incorrectly maps non-root container UID to root when it collides with RootPair #241).
2. The doc comment for ToHost can be much shorter, something like "ToHost returns the host UID and GID for the container uid and gid. An empty mapping is treated as identity."
3. It'd be nice to have a test case documenting the behavior change, i.e. that passing the (already remapped) host root ID now results in an error.

[kolyshkin]

Better to return -1, -1, err here, consistent with ToContainer (and with what was suggested in #241).

[kolyshkin]

This does not need to be that long. Something like

Suggested change

	// 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).
	// ToHost returns the host UID and GID for the container uid and gid.
	// An empty mapping is treated as identity.

would do just fine.

[kolyshkin]

3. It'd be nice to have a test case documenting the behavior change, i.e. that passing the (already remapped) host root ID now results in an error.

Something like this:

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)
      }
}

[kolyshkin]

AFAIK this one is ready; PTAL @vvoland @thaJeztah

[kolyshkin]

(of course, user release notes should come with an explanation about this one)

[vvoland]

Any suggestions for the release notes?

[thaJeztah]

Should we assert the -1 values returned here?

[kolyshkin]

Yes, makes sense to add something like

          if uid != -1 || gid != -1 {
                  t.Errorf("expected -1, -1 on error, got %d, %d", uid, gid)
          }

[kolyshkin]

...or not -- the caller is usually supposed to ignore values if a function returns an error.

[thaJeztah]

Perhaps the -1 is worth calling out. Of course, a caller shouldn't consume returned values if there's an error, but just to emphasise that they really shouldn't? 😅

[kolyshkin]

I think we should assert it (in the test, see your previous comment) but not document it, as it's minor implementation detail, can be changed, and is not always true (when git mapping fails we we return ruid, -1, err now). So I would leave the doc as is.

[kolyshkin]

Any suggestions for the release notes?

I can make a release and write it, but something like this should do:

• IdentityMapping.ToHost now always translates container IDs via the ID map.
  Previously, a non-root container UID/GID equal to the remapped-root
  base host ID (e.g. 1000 with a rootless:1000:65536 subuid range) was left
  untranslated and thus ended up owned by root inside the container. (#241, #242)

[kolyshkin]

Any suggestions for the release notes?

I can make a release and write it, but something like this should do:

• IdentityMapping.ToHost now always translates container IDs via the ID map.
  Previously, a non-root container UID/GID equal to the remapped-root
  base host ID (e.g. 1000 with a rootless:1000:65536 subuid range) was left
  untranslated and thus ended up owned by root inside the container. (#241, #242)

Maybe also say that the only user of such weird feature was moby, and add a link to moby/moby#53859