New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 19, 2016

Conversation

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

This comment has been minimized.

Show comment
Hide comment
@hqhq

hqhq May 5, 2016

Contributor

For reference, it'll Closes: #10163

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

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 5, 2016

Contributor

This is ready to test/review.

Contributor

mrunalp commented May 5, 2016

This is ready to test/review.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass May 5, 2016

Collaborator

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

Collaborator

tiborvass commented May 5, 2016

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

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 5, 2016

Contributor

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

Contributor

mrunalp commented May 5, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 6, 2016

Member

@mrunalp its merged 😄 👍

Member

thaJeztah commented May 6, 2016

@mrunalp its merged 😄 👍

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 6, 2016

Contributor

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

Contributor

mrunalp commented May 6, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 6, 2016

Member

@mrunalp done as well \o/

Member

thaJeztah commented May 6, 2016

@mrunalp done as well \o/

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 6, 2016

Contributor

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

Contributor

mrunalp commented May 6, 2016

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

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 May 9, 2016

Contributor

LGTM

Contributor

LK4D4 commented May 9, 2016

LGTM

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime May 11, 2016

Contributor

Ping @mlaventure!

Contributor

icecrime commented May 11, 2016

Ping @mlaventure!

Show outdated Hide outdated daemon/create.go
return nil, err
}
return label.DupSecOpt(c.ProcessLabel), nil

This comment has been minimized.

@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?

@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?

This comment has been minimized.

@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

@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

This comment has been minimized.

@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.

@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.

This comment has been minimized.

@thaJeztah

thaJeztah May 12, 2016

Member

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

@thaJeztah

thaJeztah May 12, 2016

Member

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

This comment has been minimized.

@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

@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

This comment has been minimized.

@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")?

@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")?

This comment has been minimized.

@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.

@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.

This comment has been minimized.

@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.

@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.

This comment has been minimized.

@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

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

This comment has been minimized.

@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
@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
if pidLabel != nil && ipcLabel != nil {
for i := 0; i < len(pidLabel); i++ {
if pidLabel[i] != ipcLabel[i] {

This comment has been minimized.

@mlaventure

mlaventure May 13, 2016

Contributor

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

@mlaventure

mlaventure May 13, 2016

Contributor

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

This comment has been minimized.

@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.

@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.

This comment has been minimized.

@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"

?

@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"

?

This comment has been minimized.

@mrunalp

mrunalp May 14, 2016

Contributor

@mlaventure No, that can't happen.

@mrunalp

mrunalp May 14, 2016

Contributor

@mlaventure No, that can't happen.

This comment has been minimized.

@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

@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

This comment has been minimized.

@rhatdan

rhatdan May 14, 2016

Contributor

This works for me.

@rhatdan

rhatdan May 14, 2016

Contributor

This works for me.

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 16, 2016

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2016

Member

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

Member

thaJeztah commented May 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2016

Member

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

Member

thaJeztah commented May 17, 2016

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

@Random-Liu

This comment has been minimized.

Show comment
Hide comment

Random-Liu commented May 17, 2016

Add support for --pid=container:<id>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 17, 2016

Contributor

@thaJeztah I have pushed updated commit with docs included.

Contributor

mrunalp commented May 17, 2016

@thaJeztah I have pushed updated commit with docs included.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2016

Member

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

Member

thaJeztah commented May 17, 2016

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

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 17, 2016

Contributor

LGTM

Contributor

crosbymichael commented May 17, 2016

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 17, 2016

Member

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

Member

thaJeztah commented May 17, 2016

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

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp May 19, 2016

Contributor

This is all green!

Contributor

mrunalp commented May 19, 2016

This is all green!

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester May 19, 2016

Member

LGTM 🐸

Member

vdemeester commented May 19, 2016

LGTM 🐸

@vdemeester vdemeester merged commit ebeb5a0 into moby: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

@jimmyxian jimmyxian referenced this pull request Jun 6, 2016

Open

Keep track of the missing API in Swarm #2333

4 of 23 tasks complete

@pmorie pmorie referenced this pull request Jun 9, 2016

Closed

Signal API proposal #26884

@leseb leseb referenced this pull request Jun 16, 2016

Merged

daemon: new directory mode #306

stepanstipl added a commit to stepanstipl/docker-py that referenced this pull request Aug 31, 2016

Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see moby/moby#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>

stepanstipl added a commit to stepanstipl/docker-py that referenced this pull request Sep 1, 2016

Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see moby/moby#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>

stepanstipl added a commit to stepanstipl/docker-py that referenced this pull request Sep 1, 2016

Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see moby/moby#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>

WeiZhang555 pushed a commit to WeiZhang555/moby that referenced this pull request Nov 25, 2016

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

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

WeiZhang555 pushed a commit to WeiZhang555/moby that referenced this pull request Nov 25, 2016

Merge branch 'backport_pidns' into 'huawei-1.11.2'
Add support for --pid=container:<id>

Backported from: moby#22481

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


See merge request docker/docker!147

bfirsh added a commit to docker/docker-py that referenced this pull request Nov 28, 2016

Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see moby/moby#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>

shin- added a commit to docker/docker-py that referenced this pull request Nov 28, 2016

Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see moby/moby#22481).

Signed-off-by: Stepan Stipl <stepan@stipl.net>

shin- added a commit to docker/docker-py that referenced this pull request Dec 8, 2016

Allow custom PID mode for the container
Docker added support for sharing PID namespaces with other containers
since version 1.12 (see moby/moby#22481).

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