libcontainer/userns: simplify, and separate from "user" package.

[thaJeztah]

follow-up to #2850 (only last commit is new) merged

This makes libcontainer/userns self-dependent, largely returning to the original implementation from lxc. The uiMapInUserNS is kept as a separate function for unit-testing and fuzzing.

[kolyshkin]

centos7 failure is a flake; filed #2907

CI restarted.

[thaJeztah]

Looks like it passed this time ✅

[thaJeztah]

Ok, rebase fixed that; all ✅ again 😅

[kolyshkin]

LGTM overall, left a single nit about ignoring error from Sscanf (not super important)

[thaJeztah]

Made this change per suggestion in #2868 (comment)

Looking at the logic now, I'm wondering if return true is "correct" if we have an error; is it ok to assume that we are in userns if we receive an error, or should we assume the reverse?

I guess if this file is not parsable, it means things are non-functional anyway, so perhaps not an issue (but looking from the other perspective; if the file-mapping is not-parsable, we likely won't be able to be using user-namespaces, in which case false would make more sense)

@AkihiroSuda @kolyshkin wdyt?

[thaJeztah]

Alternative would be:

func uidMapInUserNS(uidMap string) bool {
	var a, b, c int64
	_, err := fmt.Sscanf(uidMap, "%d %d %d", &a, &b, &c)
	if err != nil {
		return false
	}

	/*
	 * We assume we are in the initial user namespace if we have a full
	 * range - 4294967295 uids starting at uid 0.
	 */
	if a == 0 && b == 0 && c == 4294967295 {
		return false
	}
	return true
}

[kolyshkin]

One other way would be to panic() if we can't parse this. That is too radical I think.

As to whether return true or false in case we're not sure, I'd say emit a warning and return false as e.g. cgroups/fs drivers use true to skip devices setting, and we don't want that.

So I like your last version, which can be further simplified by doing

initns := a == 0 && b == 0 && c == 4294967295
return !initns

I'd also replace the comment ("We assume we are ...") with a reference to user_namespaces(7):

// As per user_namespaces(7), /proc/self/uid_map of
// the initial user namespace shows 0 0 4294967295.

[lifubang]

Yes, I think in lxc, they don't check uidMap == "", so they return true when got an EOF error.
But we checked this, so return false is correct, I think it will never happen.

[thaJeztah]

@kolyshkin updated per your suggestions, PTAL

[thaJeztah]

rebased to get a fresh run of CI

[kolyshkin]

@thaJeztah this needs another rebase because of a recent regression

[thaJeztah]

👍 rebased to trigger CI again

[thaJeztah]

rebased again; @kolyshkin @AkihiroSuda PTAL

[thaJeztah]

@kolyshkin @AkihiroSuda PTAL

[kolyshkin]

LGTM, thanks!

[lifubang]

This link is gone now. Now it's in here: https://github.com/lxc/incus/blob/main/shared/util.go#L678-L700

[thaJeztah]

Thx; updated to use a permalink

[thaJeztah]

@kolyshkin @lifubang good to go?

[AkihiroSuda]

@thaJeztah
Maybe we have discussed before, but maybe we can consider moving this to https://github.com/moby/sys ?

[thaJeztah]

@AkihiroSuda hm.. good one, not sure if we discussed that option. I know containerd has similar code (could've been a fork of this package?)

But now that containerd already depends on moby/sys/user, perhaps it would make sense to move it; if we do, we should probably check if containerd has fixes/changes that we want to integrate (and vice-versa).