Skip to content
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

If caller specifies label overrides, don't override security options #30652

Merged
merged 1 commit into from Mar 28, 2017

Conversation

rhatdan
Copy link
Contributor

@rhatdan rhatdan commented Feb 1, 2017

If a caller specifies an SELinux type or MCS Label and still wants to
share an IPC Namespace or the host namespace, we should allow them.
Currently we are ignoring the label specification if ipcmod=container
or pidmode=host.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

daemon/create.go Outdated
@@ -158,7 +158,18 @@ func (daemon *Daemon) create(params types.ContainerCreateConfig, managed bool) (
return container, nil
}

func (daemon *Daemon) generateSecurityOpt(ipcMode containertypes.IpcMode, pidMode containertypes.PidMode, privileged bool) ([]string, error) {
func (daemon *Daemon) generateSecurityOpt(hostConfig *containertypes.HostConfig) ([]string, error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Remove empty line here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runcom
Copy link
Member

runcom commented Feb 15, 2017

LGTM ping @cpuguy83 @mrunalp

@cpuguy83
Copy link
Member

Not sure I feel comfortable reviewing this change...

@justincormack ?

@thaJeztah
Copy link
Member

ping @justincormack PTAL

If a caller specifies an SELinux type or MCS Label and still wants to
share an IPC Namespace or the host namespace, we should allow them.
Currently we are ignoring the label specification if ipcmod=container
or pidmode=host.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 17, 2017

Any chance we can get people to look at this? This is a serious problem with kubernetes use of docker.

@cpuguy83
Copy link
Member

Is this function really more of a getSelinuxLabel? If so, LGTM.
generateSecurityOpt is throwing me off because --security-opt is used for a lot more.

@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 17, 2017

I think we could rename it to generateSELinuxLabel, or duplicateSELinuxLabel.
The idea is that docker generates SELinux labels on the fly for container separation, but if the container is joining an existing containers IPC Namespace you want to use the same label as you are joining, if you are using the pid host namespace, then you will need to run with no labels, since SELinux isolation will break stuff.

BUT if the caller has specified an SELinux Label to use, docker should just use the label, figuring the caller knows what it wants.

This is important for POD situations, where you could potentially want to containers sharing content but running with different SELinux labels.

Imaging you have a daemon container, but you another container to the pod that you want to have limited access, it can not use the network, or it can look at the process but not examine any content. Bottom line it gives better flexibility to the caller of the docker-engine to specify the labels that it wants.

@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 28, 2017

What do you guys want me to do with this? Change the function names or allow it to go in as is?

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐯
I guess we can go as is.

@cpuguy83 cpuguy83 merged commit b6cb416 into moby:master Mar 28, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants