Add User Namespace support to libcontainer. #23

mrunalp · 2014-06-16T18:53:38Z

I have split the libcontainer changes from this PR moby/moby#6111 into this PR.
This PR depends upon this change -- moby/moby#6417

I want to get feedback on the implementation and the UI. For using this see moby/moby#6111 changes to the docker side.

The changes allow one to specify UID/GID Mappings and also the user on host that will be mapped to root within the container. The UI can be tweaked on feedback.

Docker-DCO-1.1-Signed-off-by: Mrunal Patel mrunalp@gmail.com (github: mrunalp)

tianon · 2014-06-17T03:22:39Z

I'm not the maintainer (IANTM), but this seems too docker-specific IMO. Isn't libcontainer supposed to just expose the primitives, essentially?

mrunalp · 2014-06-17T05:42:06Z

@tianon Fair enough. I will make some changes to make it more generic.

tianon · 2014-06-17T06:24:16Z

I think it's probably still worth waiting for a maintainer to confirm
before doing too much more work, isn't it? :)

glyn · 2014-06-17T07:45:51Z

Would it be possible to include unit tests? I know libcontainer is light on unit tests, but...

mrunalp · 2014-06-17T18:04:32Z

@vmarmol @crosbymichael review please :) I think I can take out docker specific terminology and use more generic terms.

crosbymichael · 2014-06-17T19:08:08Z

I think we should just expose this via generic uid and gid mappings don't you?

mrunalp · 2014-06-17T19:19:22Z

@crosbymichael Agree on the general case.
There are two features here:

Generic UID/GID mappings that can just be set and will be applied by libcontainer.
Docker use case of mapping a host user to root. Here we need to know the uid/gid since we change to that user before calling clone and perhaps a flag to indicate that user wants to do that.. Also, there are two choices about the mapping for this feature, either we set it in libcontainer or have the user set it. Any thoughts?

crosbymichael · 2014-06-17T20:09:25Z

@mrunalp why would the root -> "docker" root usecase be any different from the generic uid/gid mappings? Wouldn't it just be one entry for the mapping?

mrunalp · 2014-06-17T20:58:03Z

@crosbymichael There is the mapping and then there is the user that you are going to run as and that's what the DockerRootUid DockerRootGid (which will be renamed) are for. Do we want to use those variables or should be use the User field?

From your response, I take it that we want to make the user supply the mappings for this case as well and I think it makes sense.

crosbymichael · 2014-06-17T21:01:30Z

Well since libcontainer is the low level part we can have the user specify the exact mapping that they want. Docker can provide the root -> docker-root abstraction

vmarmol · 2014-06-17T22:29:20Z

I agree with @crosbymichael and @tianon. We should really be exposing a mapping:

type UidRange struct {
  Start  int
  Length int
}

type UidMapping struct {
  Internal UidRange
  External UidRange
}

UserMapping []UidMapping

dineshs-altiscale · 2014-06-17T22:39:28Z

Yup. Kernel represents it somewhat more concisely as a triplet (hostUid, containerUid, size):

https://github.com/dotcloud/docker/pull/4572/files#diff-679e6137274897a7bfcc7688f0c6f089R361

Using same format saves a few lines of code here and there.

vmarmol · 2014-06-17T22:40:33Z

@dineshs-altiscale lets go with that :)

mrunalp · 2014-06-17T22:46:22Z

@dineshs-altiscale @vmarmol @crosbymichael Thanks for the feedback. I will incorporate the changes and update the PR.

mrunalp · 2014-06-19T00:44:28Z

@crosbymichael @vmarmol I have updated the PR.

mrunalp · 2014-06-19T16:51:44Z

Here is how this could be used from docker -- mrunalp/docker@6218272

vmarmol · 2014-06-20T16:14:33Z

container.go

@@ -66,6 +66,18 @@ type Container struct {

 	// The device nodes that should be automatically created within the container upon container start.  Note, make sure that the node is marked as allowed in the cgroup as well!
 	DeviceNodes []*devices.Device `json:"device_nodes,omitempty"`
+
+	// UserNsUid specifies the uid to run as when user namespace mappings are specified
+	UserNsUid int `json:"user_ns_uid,omitempty"`


I do want us to start making a distinction between runtime config and container config. Runtime config are the things that we can change if we were to run another process in the container. This is one example of that. I don't think it affects your PR, but it is something for @rjnagal, @crosbymichael, and me to think about :)

mrunalp · 2014-06-23T16:55:14Z

@crosbymichael @vmarmol You guys had a chance to review?

dineshs-altiscale · 2014-06-23T18:33:15Z

container.go

+	HostUid      int `json:"host_uid,omitempty"`
+	ContainerUid int `json:"container_uid,omitempty"`
+	Size         int `json:"size,omitempty"`
+}


UIDs are 32 bit unsigned: https://github.com/dotcloud/docker/pull/6602/files#diff-2fb7c74bdba3137445a0c8ca02bba557R124

Making that change.

dineshs-altiscale · 2014-06-23T19:06:44Z

This is missing key things like dropping capabilities and clearing KEEP_CAPS.

mrunalp · 2014-06-23T19:09:01Z

@dineshs-altiscale Good point. What capabilities do we want to retain in the user namespace? We should be able to retain more than what we do in the non-user namespace case.

mrunalp · 2014-06-23T19:32:59Z

@dineshs-altiscale capabilities could be dropped in the parent to match non-user namespace case, but I think it makes sense to allow full capabilities in the child which is the point of user ns anyway, right? Please let me know if that is a wrong assumption. From whatever I have read the capabilities are only appicable to the new child user ns. If there are any capabilities that could allow the child process to escalate onto host then we could drop those.

dineshs-altiscale · 2014-06-23T19:54:24Z

We should begin with the same list used for non user namespace case as a conservative starting point and defer a closer audit of capabilities for a later PR.

Also, it's good to keep the same model (-u <username>) currently supported for non user namespace case. It also handles supplementary groups.

In general, reusing FinalizeNamespace avoids code duplication and handles these cases correctly.

vmarmol · 2014-06-23T20:31:49Z

+1 to using the same caps for now

vmarmol · 2014-06-23T20:33:36Z

namespaces/init.go

@@ -95,17 +102,155 @@ func Init(container *libcontainer.Container, uncleanRootfs, consolePath string,
 		return fmt.Errorf("get parent death signal %s", err)
 	}

-	if err := FinalizeNamespace(container); err != nil {
-		return fmt.Errorf("finalize namespace %s", err)
+	userNsMode := len(container.UidMappings) > 0 || len(container.GidMappings) > 0


More than this, we should check if a new NS_USER was specifed.

I will add that.

mrunalp · 2014-06-23T20:54:15Z

@dineshs-altiscale -u user refers to a user inside the container whereas the options added here are for the host uid/host gid. Also, we cannot really use FinalizeNamespace or any other golang function in the child after making a raw syscall to CLONE. There was a discussion around this on #docker-dev.

mrunalp · 2014-06-23T21:50:37Z

@vmarmol @dineshs-altiscale @crosbymichael As far as capabilities in user ns go -- first paragraph here -- http://lwn.net/Articles/528117/ :) Of course, there are/will be vulnerabilities, but they will be fixed sooner the more people try out full caps in a container and report them. Also, this could be started off as an experimental feature in docker.

vmarmol · 2014-06-23T22:30:33Z

namespaces/init.go

+		}
+	}
+
+	syscall.ForkLock.Lock()


Wondering whether we should do all this in C. We're gonna have to migrate it there anyways. WDYT?

@vmarmol I agree that this is much easier to do in C as we are working around go here. Question is where are the bits so I can contribute :) and also when is it expected to be released?

This is starting slowly. The first thing is to nail down the API and then start the merge. If we use cgo for now, it'll be a drop-in replacement when the time comes.

Yeah if you're not too against it (and sorry for the extra work!) but can we write this in C? @crosbymichael let me know if you disagree :)

mrunalp · 2014-06-23T23:01:29Z

Pushed change to use uint32 for UIDs/GIDs and use NEWUSER as a flag in init.go

dineshs-altiscale · 2014-06-23T23:28:26Z

Specifying container user is intuitive and aligns with non user namespace usage model. Implementation difficulty with raw syscalls cannot really be a reason for artificially adding new config to container.go and making the usage harder.

+1 that these things are much easier to do in C when Go is not changing stack under you. But for now, since you know the mappings, you could convert container.User to its host equivalent and use it.

vmarmol · 2014-06-23T23:28:41Z

namespaces/init.go

@@ -95,17 +102,155 @@ func Init(container *libcontainer.Container, uncleanRootfs, consolePath string,
 		return fmt.Errorf("get parent death signal %s", err)
 	}

-	if err := FinalizeNamespace(container); err != nil {
-		return fmt.Errorf("finalize namespace %s", err)
+	userNsMode := container.Namespaces["NEWUSER"]


nit: userNsEnabled

dineshs-altiscale · 2014-06-23T23:34:01Z

Need to close FDs in the child as well -- there could be some FDs not marked close-on-exec that should not get through to the child. Forklock only takes care of the ones that are marked close-on-exec. With fchdir and other VFS calls that take FD rather than filename, an open FD to a host file can be as bad as having access to host file system.

mrunalp · 2014-06-23T23:46:59Z

@dineshs-altiscale I would argue that using uid/gid is more apt in libcontainer since there need not even be a user created on the host to be able to use this feature :) (and that was the driving reason behind using that as I went back and forth on it).

dineshs-altiscale · 2014-06-24T03:26:37Z

Not sure I follow that, but I don't think -u and UID maps should be mixed up.

mrunalp · 2014-06-24T22:32:04Z

@vmarmol @crosbymichael Rebased the PR and made modifications to address feedback except the -u flag. I think overloading the meaning of -u to lookup users on host or within a container namespace depending on userns/non-userns case isn't a good idea.

crosbymichael · 2014-06-24T22:37:59Z

@mrunalp do you have a sample_config for us to test this out with nsinit?

If not you can just copy the minimal.json file and add the mappings

mrunalp · 2014-06-24T23:06:06Z

@crosbymichael I have been maintaining changes to docker to test this. Need to modify create.go and driver.go -- mrunalp/docker@37dcb1a

Here is a sample container.json -- https://gist.github.com/mrunalp/833decb50fc9ae443106

crosbymichael · 2014-06-25T00:03:40Z

namespaces/init.go

-		return fmt.Errorf("finalize namespace %s", err)
+	userNsEnabled := container.Namespaces["NEWUSER"]
+
+	if userNsEnabled {


Can you break this into two separate functions just for readability?

mrunalp · 2014-06-25T00:42:25Z

@crosbymichael Pushed changes to address all of your comments.

Docker-DCO-1.1-Signed-off-by: Mrunal Patel <mrunalp@gmail.com> (github: mrunalp)

crosbymichael · 2014-07-24T00:54:26Z

I think we can close this one because #53 looks much better and I don't want to split the conversation.

vmarmol reviewed Jun 20, 2014
View reviewed changes

dineshs-altiscale reviewed Jun 23, 2014
View reviewed changes

vmarmol reviewed Jun 23, 2014
View reviewed changes

crosbymichael reviewed Jun 25, 2014
View reviewed changes

Add User Namespace support to libcontainer.

da786ae

Docker-DCO-1.1-Signed-off-by: Mrunal Patel <mrunalp@gmail.com> (github: mrunalp)

crosbymichael closed this Jul 24, 2014

dineshs-altiscale mentioned this pull request Sep 5, 2014

Proposal: Support for user namespaces moby/moby#7906

Closed

Add User Namespace support to libcontainer. #23

Add User Namespace support to libcontainer. #23

Conversation

mrunalp commented Jun 16, 2014

tianon commented Jun 17, 2014

mrunalp commented Jun 17, 2014

tianon commented Jun 17, 2014

glyn commented Jun 17, 2014

mrunalp commented Jun 17, 2014

crosbymichael commented Jun 17, 2014

mrunalp commented Jun 17, 2014

crosbymichael commented Jun 17, 2014

mrunalp commented Jun 17, 2014

crosbymichael commented Jun 17, 2014

vmarmol commented Jun 17, 2014

dineshs-altiscale commented Jun 17, 2014

vmarmol commented Jun 17, 2014

mrunalp commented Jun 17, 2014

mrunalp commented Jun 19, 2014

mrunalp commented Jun 19, 2014

Choose a reason for hiding this comment

mrunalp commented Jun 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dineshs-altiscale commented Jun 23, 2014

mrunalp commented Jun 23, 2014

mrunalp commented Jun 23, 2014

dineshs-altiscale commented Jun 23, 2014

vmarmol commented Jun 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Jun 23, 2014

mrunalp commented Jun 23, 2014

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

mrunalp commented Jun 23, 2014

dineshs-altiscale commented Jun 23, 2014

Choose a reason for hiding this comment

dineshs-altiscale commented Jun 23, 2014

mrunalp commented Jun 23, 2014

dineshs-altiscale commented Jun 24, 2014

mrunalp commented Jun 24, 2014

crosbymichael commented Jun 24, 2014

mrunalp commented Jun 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Jun 25, 2014

crosbymichael commented Jul 24, 2014