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

Docker inspect "AppArmorProfile" field now shows "docker-default" when AppArmor is enabled and no other profile was defined #27083

Merged
merged 1 commit into from Jan 30, 2017

Conversation

@RobSkye
Contributor

RobSkye commented Sep 30, 2016

This pull request fixes #25935 showing "AppArmorProfile" in docker inspect filled with "docker-default" when AppArmor is enabled and no other profile was defined using --security-opt.

@RobSkye RobSkye closed this Sep 30, 2016

@RobSkye RobSkye changed the title from adde to Docker inspect "AppArmorProfile" field now shows "default-profile" when AppArmor is enabled and no other profile was defined Oct 1, 2016

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 1, 2016

Member

@RobSkye did you close this on purpose, or by accident?

Member

thaJeztah commented Oct 1, 2016

@RobSkye did you close this on purpose, or by accident?

@RobSkye RobSkye reopened this Oct 1, 2016

@RobSkye

This comment has been minimized.

Show comment
Hide comment
@RobSkye

RobSkye Oct 1, 2016

Contributor

By Accident...laptops and beds are bad combination....I'm editing the description too.

Contributor

RobSkye commented Oct 1, 2016

By Accident...laptops and beds are bad combination....I'm editing the description too.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 1, 2016

Member

GitHub buttons are also a disaster at times, haha, no worries, thanks!

Member

thaJeztah commented Oct 1, 2016

GitHub buttons are also a disaster at times, haha, no worries, thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 1, 2016

Member

Guess this fixes #25935

Member

thaJeztah commented Oct 1, 2016

Guess this fixes #25935

@RobSkye

This comment has been minimized.

Show comment
Hide comment
@RobSkye

RobSkye Oct 1, 2016

Contributor

Yes.

Contributor

RobSkye commented Oct 1, 2016

Yes.

@RobSkye RobSkye changed the title from Docker inspect "AppArmorProfile" field now shows "default-profile" when AppArmor is enabled and no other profile was defined to Docker inspect "AppArmorProfile" field now shows "docker-default" when AppArmor is enabled and no other profile was defined Oct 1, 2016

@tonistiigi

Please squash the commits as well.

Show outdated Hide outdated daemon/container_linux.go
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 10, 2016

Member

@RobSkye can you address @tonistiigi's comments, and squash your commits?

Member

thaJeztah commented Oct 10, 2016

@RobSkye can you address @tonistiigi's comments, and squash your commits?

@RobSkye

This comment has been minimized.

Show comment
Hide comment
@RobSkye

RobSkye Oct 11, 2016

Contributor

With the latest push, if you change apparmor configuration and apparmor completely disappear from the dockerhost (I have some comments about this below) the AppArmorProfile is updated with the expected configuration but still there is some issues we have to address:

  1. If you do an inspect after an apparmor change and before the start of the container, still shows the old AppArmorProfile.
  2. If you create the container with apparmor disabled but with --security-opt apparmor=someprofile, when you enable apparmor in dockerhost, AppArmorProfile will be "docker-default".
  3. Apparmor status is checked ONLY checking the existence of /sys/kernel/security/apparmor and is not a good method, you can have apparmor completely shutdown and still that directory exists. see https://github.com/docker/docker/blob/master/pkg/sysinfo/sysinfo_linux.go#L57
  4. You can run a container with --security-opt apparmor=someprofile and someprofile will be set in AppArmorProfile whether or not exists as an valid apparmor profile.

I don't know if addressing this issues are in the scope of the main issue. I' have the feeling that implementing a real-time check of the security configuration of containers will require more than a "slight behavioral change".

I have some ideas of adding security information in the "docker ps" command and a more detailed "docker security" showing all the security settings of a container.

I look forward to your feedback.

Contributor

RobSkye commented Oct 11, 2016

With the latest push, if you change apparmor configuration and apparmor completely disappear from the dockerhost (I have some comments about this below) the AppArmorProfile is updated with the expected configuration but still there is some issues we have to address:

  1. If you do an inspect after an apparmor change and before the start of the container, still shows the old AppArmorProfile.
  2. If you create the container with apparmor disabled but with --security-opt apparmor=someprofile, when you enable apparmor in dockerhost, AppArmorProfile will be "docker-default".
  3. Apparmor status is checked ONLY checking the existence of /sys/kernel/security/apparmor and is not a good method, you can have apparmor completely shutdown and still that directory exists. see https://github.com/docker/docker/blob/master/pkg/sysinfo/sysinfo_linux.go#L57
  4. You can run a container with --security-opt apparmor=someprofile and someprofile will be set in AppArmorProfile whether or not exists as an valid apparmor profile.

I don't know if addressing this issues are in the scope of the main issue. I' have the feeling that implementing a real-time check of the security configuration of containers will require more than a "slight behavioral change".

I have some ideas of adding security information in the "docker ps" command and a more detailed "docker security" showing all the security settings of a container.

I look forward to your feedback.

@RobSkye

This comment has been minimized.

Show comment
Hide comment
@RobSkye

RobSkye Oct 12, 2016

Contributor

I did a little refactor of the patch in the last commit. Now inspect always shows an empty AppArmorProfile when the container is stopped ( maybe empty is not the best value but this can be changed easily to undefined, unconfined or something more clarifying).

The correct value of AppArmorProfile is set at container startup using the actual status of apparmor, the privileged flag and the configured apparmor settings using --security-opt.

Contributor

RobSkye commented Oct 12, 2016

I did a little refactor of the patch in the last commit. Now inspect always shows an empty AppArmorProfile when the container is stopped ( maybe empty is not the best value but this can be changed easily to undefined, unconfined or something more clarifying).

The correct value of AppArmorProfile is set at container startup using the actual status of apparmor, the privileged flag and the configured apparmor settings using --security-opt.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Nov 3, 2016

Contributor

@RobSkye I'm not sure that I like an idea of clearing profile on stopped containers - it's breaking change, and indeed it might be useful for debugging. Otherwise, the patch is ok for me.

Contributor

LK4D4 commented Nov 3, 2016

@RobSkye I'm not sure that I like an idea of clearing profile on stopped containers - it's breaking change, and indeed it might be useful for debugging. Otherwise, the patch is ok for me.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Dec 1, 2016

Contributor

ping @RobSkye , needs a rebase.

I personally don't think showing the previous apparmor profile when inspecting old containers is an issue to be fixed otherwise. It properly reflect how this container was run.

Contributor

mlaventure commented Dec 1, 2016

ping @RobSkye , needs a rebase.

I personally don't think showing the previous apparmor profile when inspecting old containers is an issue to be fixed otherwise. It properly reflect how this container was run.

@RobSkye

This comment has been minimized.

Show comment
Hide comment
@RobSkye

RobSkye Dec 4, 2016

Contributor

Ok, i'll delete that part and rebase/commit.

Contributor

RobSkye commented Dec 4, 2016

Ok, i'll delete that part and rebase/commit.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 12, 2017

Member

ping @tonistiigi @LK4D4 PTAL what's the status on this one?

Member

thaJeztah commented Jan 12, 2017

ping @tonistiigi @LK4D4 PTAL what's the status on this one?

Show outdated Hide outdated daemon/container_linux.go
Show outdated Hide outdated daemon/container_linux.go
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 19, 2017

Contributor

ping @RobSkye
Would you mind to fix Tonis' comments? Then we can merge

Contributor

LK4D4 commented Jan 19, 2017

ping @RobSkye
Would you mind to fix Tonis' comments? Then we can merge

Added an apparmorEnabled boolean in the Daemon struct to indicate if …
…AppArmor is enabled or not. It is set in NewDaemon using sysInfo information.

Signed-off-by: Roberto Muñoz Fernández <robertomf@gmail.com>

Added an apparmorEnabled boolean in the Daemon struct to indicate if AppArmor is enabled or not. It is set in NewDaemon using sysInfo information.

Signed-off-by: Roberto Muñoz Fernández <robertomf@gmail.com>

gofmt'd

Signed-off-by: Roberto Muñoz Fernández <robertomf@gmail.com>

change the function name to something more adequate and changed the behaviour to show empty value on an apparmor disabled system.

Signed-off-by: Roberto Muñoz Fernández <robertomf@gmail.com>

go fmt

Signed-off-by: Roberto Muñoz Fernández <robertomf@gmail.com>
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

LGTM
ping @tonistiigi

Contributor

LK4D4 commented Jan 30, 2017

LGTM
ping @tonistiigi

@LK4D4

LK4D4 approved these changes Jan 30, 2017

@tonistiigi

LGTM

@tonistiigi tonistiigi merged commit 61b2cda into moby:master Jan 30, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30112 has succeeded
Details
janky Jenkins build Docker-PRs 38725 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9753 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment