Add support for --pid=container:<id> #22481

Merged
merged 1 commit into from May 19, 2016

Projects

None yet
@mrunalp
Contributor
mrunalp commented May 3, 2016

This is for #22479

I will take out the engine-api code once docker/engine-api#218 is merged.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

@hqhq
Contributor
hqhq commented May 5, 2016

For reference, it'll Closes: #10163

@mrunalp mrunalp changed the title from WIP: Add support for --pid=container:<id> to Add support for --pid=container:<id> May 5, 2016
@mrunalp
Contributor
mrunalp commented May 5, 2016

This is ready to test/review.

@tiborvass
Contributor

Maintainers (from today's review session) agree with the design, so moving to code review.

@mrunalp
Contributor
mrunalp commented May 5, 2016

@tiborvass Thanks for the update! We need to get docker/engine-api#218 reviewed/merged as well.

@thaJeztah
Member

@mrunalp its merged 😄 👍

@mrunalp
Contributor
mrunalp commented May 6, 2016

@thaJeztah yeah, now waiting for the vendor PR #22538 :)

@thaJeztah
Member

@mrunalp done as well \o/

@mrunalp
Contributor
mrunalp commented May 6, 2016

@thaJeztah Thanks! I have rebased this PR to pick up the engine-api update.

@LK4D4
Contributor
LK4D4 commented May 9, 2016

LGTM

@icecrime
Member

Ping @mlaventure!

@mlaventure mlaventure and 3 others commented on an outdated diff May 12, 2016
daemon/create.go
@@ -150,6 +150,14 @@ func (daemon *Daemon) generateSecurityOpt(ipcMode containertypes.IpcMode, pidMod
return label.DupSecOpt(c.ProcessLabel), nil
}
+ if pidContainer := pidMode.Container(); pidContainer != "" {
+ c, err := daemon.GetContainer(pidContainer)
+ if err != nil {
+ return nil, err
+ }
+
+ return label.DupSecOpt(c.ProcessLabel), nil
@mlaventure
mlaventure May 12, 2016 Contributor

If we have both ipcMode.IsContainer() and pidMode.IsContainer() are true and they both point to different containers with different labels we will never apply the second container labels.

I'm no selinux expert, but shouldn't those be merged somehow?

@mrunalp
mrunalp May 12, 2016 Contributor

We can't join two namespaces of two different containers unless transitively they have the same label.
Cc: @rhatdan

Sent from my iPhone

On May 11, 2016, at 8:32 PM, Kenfe-Mickaël Laventure notifications@github.com wrote:

In daemon/create.go:

@@ -150,6 +150,14 @@ func (daemon *Daemon) generateSecurityOpt(ipcMode containertypes.IpcMode, pidMod

    return label.DupSecOpt(c.ProcessLabel), nil
}
  • if pidContainer := pidMode.Container(); pidContainer != "" {
  •   c, err := daemon.GetContainer(pidContainer)
    
  •   if err != nil {
    
  •       return nil, err
    
  •   }
    
  •   return label.DupSecOpt(c.ProcessLabel), nil
    
    If we have both ipcMode.IsContainer() and pidMode.IsContainer() are true and they both point to different containers with different labels we will never apply the second container labels.

I'm no selinux expert, but shouldn't those be merged somehow?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@rhatdan
rhatdan May 12, 2016 Contributor

In order for two containers to share the same PID or same IPC Namespace, they have to run with the same SELinux label. Otherwise SELinux will break when a process in container1 tries to look at the processes in container2, or a process in container 1 tries to use an IPC created in container2. This is why when the second container is created it switches to the first containers labels. By merging the containers namespaces, you are saying that they share the same security structure, from an SELinux point of view.

@thaJeztah
thaJeztah May 12, 2016 Member

is ipc=<container-A>, pid=<container-B> a valid configuration, or should that produce an error?

@mlaventure
mlaventure May 12, 2016 Contributor

It's valid if they have the same selinux configuration it seems. Which
should check for this or prohibit it entirely to avoid runtime error later
on I'd say.

On Thu, May 12, 2016, 5:19 AM Sebastiaan van Stijn notifications@github.com
wrote:

In daemon/create.go
#22481 (comment):

@@ -150,6 +150,14 @@ func (daemon *Daemon) generateSecurityOpt(ipcMode containertypes.IpcMode, pidMod

    return label.DupSecOpt(c.ProcessLabel), nil
}
  • if pidContainer := pidMode.Container(); pidContainer != "" {
  •   c, err := daemon.GetContainer(pidContainer)
    
  •   if err != nil {
    
  •       return nil, err
    
  •   }
    
  •   return label.DupSecOpt(c.ProcessLabel), nil
    

is ipc=, pid= a valid configuration, or should
that produce an error?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/22481/files/0c5d257326331f2bcaf03a4b3147d3225acf09c7#r63009686

@thaJeztah
thaJeztah May 12, 2016 Member

Would it be bad to keep it simple for now, and require them to be the same container? (as in "no is temporary, yes is forever")?

@rhatdan
rhatdan May 12, 2016 Contributor

You could release note it. Not sure if their would be other problems. Combining containers together without combining their security attributes could lead to some weird errors. Right now we keep seccomp, apparmor and capabiltiies separate, but you could figure out ways where weird bugs or priv escalations can happen. Basically one process with limited privs in one container causing a process in a different container through IPC or shared pid to do something more privileged.

@mlaventure
mlaventure May 12, 2016 Contributor

Prohibiting joining ipc and pid from 2 different containers works for me as long as it's well documented.

We can extend it later to allow this case if both containers are using the same selinux configuration.

@mrunalp
mrunalp May 13, 2016 Contributor

I have pushed an update that compares the pid and ipc labels for the use case and allows them only if they are same.

@mrunalp
mrunalp May 13, 2016 Contributor

This also works for this example as the labels are the same transitively.

start container A
container B joins --ipc:container:A
container C joins --ipc:container:B --pid:container:A
@mlaventure mlaventure commented on the diff May 13, 2016
daemon/create.go
- return label.DupSecOpt(c.ProcessLabel), nil
+ pidLabel = label.DupSecOpt(c.ProcessLabel)
+ if ipcContainer == "" {
+ return pidLabel, err
+ }
+ }
+
+ if pidLabel != nil && ipcLabel != nil {
+ for i := 0; i < len(pidLabel); i++ {
+ if pidLabel[i] != ipcLabel[i] {
@mlaventure
mlaventure May 13, 2016 Contributor

What if the labels are identical but not in the same order?

@mrunalp
mrunalp May 13, 2016 Contributor

Could you clarify your question? If the labels are the same, then all the strings in the label array will match up.

@mlaventure
mlaventure May 13, 2016 Contributor

So we can't have a scenario where

ipcLabel[0] = "label1" ; ipcLabel[1] = "label2"
pidLabel[0] = "label2" ; pidLabel[1] = "label1"

?

@mrunalp
mrunalp May 14, 2016 Contributor

@mlaventure No, that can't happen.

@mlaventure
mlaventure May 14, 2016 Contributor

Ok, LGTM then! (IANAM)

On Fri, May 13, 2016, 8:33 PM Mrunal Patel notifications@github.com wrote:

In daemon/create.go
#22481 (comment):

  •   return label.DupSecOpt(c.ProcessLabel), nil
    
  •   pidLabel = label.DupSecOpt(c.ProcessLabel)
    
  •   if ipcContainer == "" {
    
  •       return pidLabel, err
    
  •   }
    
  • }
  • if pidLabel != nil && ipcLabel != nil {
  •   for i := 0; i < len(pidLabel); i++ {
    
  •       if pidLabel[i] != ipcLabel[i] {
    

@mlaventure https://github.com/mlaventure No, that can't happen.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/22481/files/4772a40138e8f88e77f04049c1c2dc4865508dc7#r63270556

@rhatdan
rhatdan May 14, 2016 Contributor

This works for me.

@mrunalp
Contributor
mrunalp commented May 16, 2016

@thaJeztah ping. who else needs to review this? :) Also looks like janky failure is a test environment issue that should go away on restart.

@thaJeztah
Member

ping @LK4D4 @crosbymichael PTAL for the latest changes, let's get this merged 👍

@thaJeztah thaJeztah added this to the 1.12.0 milestone May 17, 2016
@thaJeztah
Member
thaJeztah commented May 17, 2016 edited

actually; this needs changes to the documentation, so please move it to "docs review" if it LGTY

@mrunalp The documentation needs to be updated to show this is now possible; preferably
with an example (explaining why someone would use this as an option?). I think it needs a note as well, explaining that, although --pid and --net can use different containers, their labels should match, if SELinux is used (don't know if we can come up with an example for having different containers for that?)

Man pages;

Also, the completion scripts need to be updated; let me know if you want to try
updating those yourself, or if @albers and @sdurrheimer need to assist. I think
the same approach can be taken as for the --net option (although I couldn't
find completion for container-names in the zsh script);

@mrunalp mrunalp Add support for --pid=container:<id>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
fb43ef6
@mrunalp
Contributor
mrunalp commented May 17, 2016

@thaJeztah I have pushed updated commit with docs included.

@thaJeztah
Member

Thanks! docs LGTM, but we're not in docs review yet :-)

@crosbymichael
Member

LGTM

@thaJeztah
Member

moving to docs review with a LGTM from @mlaventure and @LK4D4

@Random-Liu Random-Liu referenced this pull request in kubernetes/kubernetes May 18, 2016
Open

Shared PID and UTS namespaces #1615

@mrunalp
Contributor
mrunalp commented May 19, 2016

This is all green!

@vdemeester
Member

LGTM 🐸

@vdemeester vdemeester merged commit ebeb5a0 into docker:master May 19, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 18655 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 5499 has succeeded
Details
janky Jenkins build Docker-PRs 27481 has succeeded
Details
userns Jenkins build Docker-PRs-userns 9663 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 26044 has succeeded
Details
windowsTP5 Jenkins build Docker-PRs-WoW-TP5 1765 has succeeded
Details
@stepanstipl stepanstipl added a commit to stepanstipl/docker-py that referenced this pull request Aug 31, 2016
@stepanstipl stepanstipl Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see docker/docker#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>
2b29d6f
@stepanstipl stepanstipl referenced this pull request in docker/docker-py Aug 31, 2016
Closed

Allow custom PID mode for the container #1177

@stepanstipl stepanstipl added a commit to stepanstipl/docker-py that referenced this pull request Sep 1, 2016
@stepanstipl stepanstipl Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see docker/docker#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>
b26556f
@stepanstipl stepanstipl added a commit to stepanstipl/docker-py that referenced this pull request Sep 1, 2016
@stepanstipl stepanstipl Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see docker/docker#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>
d60bed2
@WeiZhang555 WeiZhang555 pushed a commit to WeiZhang555/docker that referenced this pull request Nov 25, 2016
@mrunalp @hqhq mrunalp + hqhq Add support for --pid=container:<id>
Backported from: docker#22481

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>
5f221b9
@WeiZhang555 WeiZhang555 pushed a commit to WeiZhang555/docker that referenced this pull request Nov 25, 2016
@coolljt0725 coolljt0725 Merge branch 'backport_pidns' into 'huawei-1.11.2'
Add support for --pid=container:<id>

Backported from: docker#22481

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Qiang Huang <h.huangqiang@huawei.com>


See merge request docker/docker!147
68fe09a
@bfirsh bfirsh added a commit to docker/docker-py that referenced this pull request Nov 28, 2016
@stepanstipl @bfirsh stepanstipl + bfirsh Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see docker/docker#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>
e7a9ba2
@shin- shin- added a commit to docker/docker-py that referenced this pull request Nov 28, 2016
@stepanstipl @shin- stepanstipl + shin- Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see docker/docker#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>
7ef48c3
@shin- shin- added a commit to docker/docker-py that referenced this pull request Dec 8, 2016
@stepanstipl @shin- stepanstipl + shin- Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see docker/docker#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>
d19e8e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment