Support multiple users/groups mapped for the rootless case #1529

giuseppe · 2017-07-20T17:38:37Z

Take advantage of the newuidmap/newgidmap tools to allow multiple users/groups to be mapped into the new user namespace in the rootless case.

This is what I've tried here (only the relevant bits showed for config.json):

	"args": [
	    "cat", "/proc/self/uid_map"
	],
...
        "uidMappings": [
            {
                "hostID": 1000,
                "containerID": 0,
                "size": 1
            },
            {
                "hostID": 110000,
                "containerID": 1,
                "size": 255
            }
        ],
        "gidMappings": [
            {
                "hostID": 1000,
                "containerID": 0,
                "size": 1
            }
        ],

$ whoami
gscrivano

$ grep ^gscrivano /etc/subuid
gscrivano:110000:65536

$ runc run foo
         0       1000          1
         1     110000        255

teddyking · 2017-07-21T08:46:34Z

Hey @giuseppe, I opened a PR looking to add this same functionality a couple of months ago: #1403. There's a discussion ongoing in the linked PR, would be great to hear your thoughts on that as well. It looks like we were both heading down the same path here.

cyphar

I have a couple of comments, as well as some semantic questions that we need to discuss.

cyphar · 2017-07-22T10:27:55Z

libcontainer/configs/validate/rootless.go

-// mount verifies that the user isn't trying to set up any mounts they don't have
-// the rights to do. In addition, it makes sure that no mount has a `uid=` or
-// `gid=` option that doesn't resolve to root.
-func rootlessMount(config *configs.Config) error {


I'm not convinced that dropping this makes sense. Even in the multiple-uid case, really we should be making it so that we emit an error if the uid=... or gid=... options reference unmapped users. The errors are far too cryptic if you ignore it.

cyphar · 2017-07-22T10:30:02Z

libcontainer/configs/validate/rootless_test.go

-	config.Mounts[0].Data = "gid=0"
-	if err := validator.Validate(config); err != nil {
-		t.Errorf("Expected error to not occur when setting gid=0 in mount options: %+v", err)
-	}


... so all of these removed tests should stay, and we should add new tests making sure that multiple-mapping cases are still treated sanely.

cyphar · 2017-07-22T10:30:22Z

libcontainer/nsenter/nsexec.c

@@ -192,22 +194,74 @@ static void update_setgroups(int pid, enum policy_t setgroup)
 	}
 }

-static void update_uidmap(int pid, char *map, size_t map_len)
+static int attempt_mapping_tool(const char *app, int pid, char *map, size_t map_len)


Please format this (all of your changes) in the Linux kernel style (sans the 80-column restriction).

cyphar · 2017-07-22T10:30:49Z

libcontainer/nsenter/nsexec.c

+	size_t i;
+	int child;
+
+	sprintf (pid_fmt, "%d", pid);


asprintf?

IMO it would be wasteful to allocate dynamic memory when we know it will be just to format an int

It's just because I don't like that you've hard-coded the length of pid_fmt to be 11.

Personally I like @teddyking's approach to the formatting a bit more (we send most of the information from Go with a netlink message rather than formatting everything in C). I prefer that because the context in which this C code is run is quite fragile and I get less worried if we do things in Go when possible. I'll read through his PR again and point out which bits seem good to me.

AFAICS #1403 is doing a very similar thing for preparing the argv for newuidmap/newgidmap

The main difference is that here I am not using strtok and parsing the string inline without allocating any memory. I've avoided input validation here as it would be superfluous since newuidmap/newgidmap do that already.

cyphar · 2017-07-22T10:32:53Z

libcontainer/nsenter/nsexec.c

+	}
+	else {
+		int status;
+		while (waitpid (child, &status, 0) < 0 && errno == EINTR);


do waitpid(child, &status, 0); while (errno == EINTR);

If not, just put the ; on a bare newline so it's more clear it's an empty loop.

yes, fixed and add an error check

cyphar · 2017-07-22T10:35:18Z

libcontainer/nsenter/nsexec.c

 {
 	if (map == NULL || map_len <= 0)
 		return;

-	if (write_file(map, map_len, "/proc/%d/uid_map", pid) < 0)
+	if (write_file(map, map_len, "/proc/%d/uid_map", pid) < 0) {
+		if(errno == EPERM && is_rootless && attempt_mapping_tool ("/usr/bin/newuidmap", pid, map, map_len) == 0)


I'm not sure how I feel about hard-coding the /usr/bin/newuidmap. At this point, $PATH should still be trustworthy (so execvp could work), but maybe @crosbymichael knows better than me.

I've changed it to execvp so that PATH is honored.

One thing we'd quite like to bring over from #1403 is the ability to specify --newuidmap and --newgidmap binary locations. It's seems to be in line with the --criuPath and it's our preferred way to be explicit in Garden. We can also wait for this to get merged and PR the flags separately if y'all want to keep this PR smaller, but it would be good to get buy in for that approach now.

cyphar · 2017-07-22T10:38:42Z

main.go

@@ -60,6 +60,14 @@ func main() {
 	}
 	v = append(v, fmt.Sprintf("spec: %s", specs.Version))
 	app.Version = strings.Join(v, "\n")
+
+	root := os.Getenv("XDG_RUNTIME_DIR")


This is not correct, as it will mean that if you run runc as root with XDG_RUNTIME_DIR set, then the spawned containers won't be visible by other runc management processes. Instead, this should be conditional based on whether you're an unprivileged user. Even then I'm still not convinced, because what if someone sets up /run/runc to be like /tmp (sticky-bit and world-writeable) in order for all users to access the same store of container state?

I agree that runc should obey XDG_RUNTIME_DIR, but I'm not entirely convinced that this is correct (especially since changing XDG_RUNTIME_DIR will make it more difficult for other people to manage the runc containers spawned by someone with XDG_RUNTIME_DIR set.

@crosbymichael do you have any ideas on how this should work nicely?

I've changed it to be used only when geteuid() !=0. I think a shared directory for all users is a very uncommon use case, while most users would probably prefer to have runc working out of the box and behave like other applications that use XDG_RUNTIME_DIR. The first case is still possible though by overriding the root directory, and I think it is preferable to require extra configuration for an uncommon use like a shared directory for all users.

cyphar · 2017-07-22T10:42:55Z

libcontainer/nsenter/nsexec.c

-					update_uidmap(child, config.uidmap, config.uidmap_len);
-					update_gidmap(child, config.gidmap, config.gidmap_len);
+					update_uidmap(child, config.is_rootless, config.uidmap, config.uidmap_len);
+					update_gidmap(child, config.is_rootless, config.gidmap, config.gidmap_len);


Why not always try to use newuidmap and newgidmap rather than making it conditional on is_rootless? (Though maybe you're right here.)

I am using newuidmap and newgidmap only after an EPERM. When running as root that should never happen

Right, so then the is_rootless check is unecessary (which is what I meant)?

sure I can remove that. I've pushed a new version where I drop it.

giuseppe · 2017-07-22T20:25:49Z

@teddyking yes, we got to something very similar :) An extra point for considering this the right approach, and for the maintainers to pick what they prefer from both implementations

giuseppe · 2017-08-30T09:22:23Z

is there anything I can do to get this moving forward?

cyphar · 2017-08-30T10:54:17Z

Yeah, there's a few things left that we can take from #1403. I'll list them as a checklist below.

Switch to passing the ($PATH expanded) path of newuidmap and newgidmap via netlink. I don't think they need to be user-visible options at the moment, but I don't like hardcoding them in nsexec.
Add some tests for this functionality, if possible (you could use what Use newuidmap to allow multiple ID mappings when running rootless #1403 has as inspiration). If you don't feel like doing it, I can work on it after this is merged.

I'll also add some in-line comments. Sorry for not reviewing this in a while, I've been looking at the other rootless-container-related PR for cgroup handling most recently.

cyphar · 2017-08-30T10:54:57Z

libcontainer/nsenter/nsexec.c

+	return -1;
+}
+
+static void update_uidmap(int pid, uint8_t is_rootless, char *map, size_t map_len)


is_rootless is still here, even though it isn't used anymore.

cyphar · 2017-08-30T10:55:09Z

libcontainer/nsenter/nsexec.c

 }

-static void update_gidmap(int pid, char *map, size_t map_len)
+static void update_gidmap(int pid, uint8_t is_rootless, char *map, size_t map_len)


... and it's still here too.

cyphar · 2017-08-30T10:58:04Z

libcontainer/nsenter/nsexec.c

 		bail("failed to update /proc/%d/gid_map", pid);
+	}


Completely stylistic, but I think this is more readable (same applies for update_uidmap):

if (write_file(map, map_len, "/proc/%d/gid_map", pid) < 0) { if (errno != EPERM) bail("failed to update /proc/%d/gid_map", pid); if (try_mapping_tool("newgidmap", pid, map, map_len)) bail("failed to use newgid map on %d", pid); }

cyphar · 2017-08-30T11:00:02Z

main.go

+	if os.Geteuid() != 0 {
+		runtimeDir := os.Getenv("XDG_RUNTIME_DIR")
+		root = runtimeDir + "/runc"
+	}


This doesn't handle the case where XDG_RUNTIME_DIR is unspecified safely. If it's not set, just use /run/runc as usual.

cyphar · 2017-08-30T11:04:32Z

libcontainer/configs/validate/rootless.go

-			if strings.HasPrefix(opt, "uid=") && opt != "uid=0" {
-				return fmt.Errorf("cannot specify uid= mount options in rootless containers where argument isn't 0")
+			if strings.HasPrefix(opt, "uid=") {
+				uid, _ := strconv.Atoi(strings.Replace(opt, "uid=", "", 1))


I don't like the ignoring of errors, nor strings.Replace here. Can we just do something like

var uid int n, err := fmt.Sscanf(opt, "uid=%d", &uid) if n != 1 || err != nil { // Ignore unknown mount options. continue } if !hasIDMapping(uid, config.UidMappings) { return fmt.Errorf("cannot specify uid= mount options for unmapped uid in rootless containers") }

And the same for the other option.

cyphar · 2017-08-30T11:06:48Z

libcontainer/nsenter/nsexec.c

+		 * Convert the map string into a list of argument that
+		 * newuidmap/newgidmap can understand.
+		 */
+		for (i = 0; i < map_len - 1; ) {


Is it possible to do this with strtok?

cyphar · 2017-08-30T11:07:28Z

libcontainer/nsenter/nsexec.c

+		int argc = 0;
+		size_t i;
+
+		sprintf (pid_fmt, "%d", pid);


While it would never be an issue, use snprintf(pid_fmt, 16, ...).

cyphar · 2017-08-30T11:10:48Z

Also I believe this needs a rebase, because the test failures are happening due to shfmt (which we no longer use).

giuseppe · 2017-09-05T16:10:10Z

@cyphar Thanks for the fixes! They LGTM

cyphar · 2017-09-07T02:42:43Z

@giuseppe I've pulled some of the changes from #1403, and rewrote some others to work better in the test framework. Now all of this code is tested as an integration suite run. 😸

cyphar · 2017-09-07T02:42:57Z

LGTM.

/cc @opencontainers/runc-maintainers

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Take advantage of the newuidmap/newgidmap tools to allow multiple users/groups to be mapped into the new user namespace in the rootless case. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> [ rebased to handle intelrdt changes. ] Signed-off-by: Aleksa Sarai <asarai@suse.de>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

After quite a bit of debugging, I found that previous versions of this patchset did not include newgidmap in a rootless setting. Fix this by passing it whenever group mappings are applied, and also providing some better checking for try_mapping_tool. This commit also includes some stylistic improvements. Signed-off-by: Aleksa Sarai <asarai@suse.de>

With the addition of our new{uid,gid}map support, we used to call execvp(3) from inside nsexec. This would mean that the path resolution for the binaries would happen in nsexec. Move the resolution to the initial setup code, and pass the absolute path to nsexec. Signed-off-by: Aleksa Sarai <asarai@suse.de>

Now that rootless containers have support for multiple uid and gid mappings, allow --user to work as expected. If the user is not mapped, an error occurs (as usual). Signed-off-by: Aleksa Sarai <asarai@suse.de>

This is necessary in order to add proper opportunistic tests, and is a placeholder until we add tests for new{uid,gid}map configurations. Signed-off-by: Aleksa Sarai <asarai@suse.de>

Enable several previously disabled tests (for the idmap execution mode) for rootless containers, in addition to making all tests use the additional mappings. At the moment there's no strong need to add any additional tests purely for rootless_idmap. Signed-off-by: Aleksa Sarai <asarai@suse.de>

cyphar · 2017-09-09T02:46:34Z

LGTM (again, needed rebase).

/cc @opencontainers/runc-maintainers

crosbymichael · 2017-09-11T18:01:13Z

LGTM

AkihiroSuda · 2018-01-15T07:11:37Z

Added runc spec support for optionally generating a config with multiple uidMappings and gidMappings: #1692

giuseppe force-pushed the rootless-improvements branch 3 times, most recently from aa29e64 to 98ff8cf Compare July 21, 2017 08:08

giuseppe force-pushed the rootless-improvements branch from 98ff8cf to 4a40d13 Compare July 21, 2017 10:31

cyphar reviewed Jul 22, 2017

View reviewed changes

giuseppe force-pushed the rootless-improvements branch 2 times, most recently from 4d4023d to 77f8908 Compare July 22, 2017 20:15

cyphar mentioned this pull request Jul 23, 2017

Use newuidmap to allow multiple ID mappings when running rootless #1403

Closed

giuseppe force-pushed the rootless-improvements branch 4 times, most recently from 07b33c0 to be19b80 Compare July 26, 2017 11:38

dqminh added this to the 1.1.0 milestone Jul 27, 2017

cyphar added the rootless-containers label Aug 11, 2017

giuseppe force-pushed the rootless-improvements branch from be19b80 to e89e473 Compare August 21, 2017 07:34

giuseppe force-pushed the rootless-improvements branch from e89e473 to a0270be Compare August 28, 2017 13:58

cyphar reviewed Aug 30, 2017

View reviewed changes

giuseppe force-pushed the rootless-improvements branch 2 times, most recently from 7b00799 to 4769977 Compare August 31, 2017 08:16

cyphar force-pushed the rootless-improvements branch 9 times, most recently from 5cb0e9c to 959a954 Compare September 7, 2017 02:25

cyphar mentioned this pull request Sep 7, 2017

Support cgroups with limits as rootless #1540

Merged

giuseppe and others added 8 commits September 9, 2017 12:44

main: honor XDG_RUNTIME_DIR for rootless containers

fdf85e3

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

tests: fix for rootless multiple uids/gids

3282f5a

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

init: support setting -u with rootless containers

1a5fdc1

Now that rootless containers have support for multiple uid and gid mappings, allow --user to work as expected. If the user is not mapped, an error occurs (as usual). Signed-off-by: Aleksa Sarai <asarai@suse.de>

tests: generalise rootless runner

d0aec23

This is necessary in order to add proper opportunistic tests, and is a placeholder until we add tests for new{uid,gid}map configurations. Signed-off-by: Aleksa Sarai <asarai@suse.de>

cyphar force-pushed the rootless-improvements branch from 959a954 to eb5bd4f Compare September 9, 2017 02:46

crosbymichael merged commit 8b47a24 into opencontainers:master Sep 11, 2017

cyphar mentioned this pull request Oct 22, 2017

tests: add various !terminal tests #1357

Merged

cyphar mentioned this pull request Nov 13, 2017

Additional user namespace checks #1649

Closed

AkihiroSuda mentioned this pull request Jan 15, 2018

rootless: optional support for generating config with subuid map #1692

Closed

cyphar mentioned this pull request Feb 24, 2018

VERSION: release v1.0.0-rc5 #1739

Merged

Support multiple users/groups mapped for the rootless case #1529

Support multiple users/groups mapped for the rootless case #1529

Conversation

giuseppe commented Jul 20, 2017

teddyking commented Jul 21, 2017

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Jul 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammartin Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Jul 22, 2017

giuseppe commented Aug 30, 2017

cyphar commented Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Aug 30, 2017

giuseppe commented Sep 5, 2017

cyphar commented Sep 7, 2017

cyphar commented Sep 7, 2017 • edited by caniszczyk Loading

cyphar commented Sep 9, 2017 • edited by caniszczyk Loading

crosbymichael commented Sep 11, 2017 • edited by caniszczyk Loading

AkihiroSuda commented Jan 15, 2018

cyphar Jul 22, 2017 •

edited

Loading

williammartin Jul 24, 2017 •

edited

Loading

cyphar commented Aug 30, 2017 •

edited

Loading

cyphar commented Sep 7, 2017 •

edited by caniszczyk

Loading

cyphar commented Sep 9, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Sep 11, 2017 •

edited by caniszczyk

Loading