Docker inspect "AppArmorProfile" field now shows "docker-default" when AppArmor is enabled and no other profile was defined by RobSkye · Pull Request #27083 · moby/moby

RobSkye · 2016-09-30T23:58:09Z

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.

thaJeztah · 2016-10-01T00:05:08Z

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

RobSkye · 2016-10-01T00:07:04Z

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

thaJeztah · 2016-10-01T00:10:23Z

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

thaJeztah · 2016-10-01T00:12:08Z

Guess this fixes #25935

RobSkye · 2016-10-01T00:17:19Z

Yes.

tonistiigi

Please squash the commits as well.

tonistiigi · 2016-10-07T17:58:44Z

daemon/container_linux.go

there is const defaultApparmorProfile

Also need to handle --privileged. See https://github.com/docker/docker/blob/master/daemon/oci_linux.go#L709-L714

Even with privileged fixed this is a slight behavioral change. If apparmor is enabled after container was created this var will not change. I think if this is only called on inspecting single container we can lazy load the value like we do in container startup.

Even with privileged fixed this is a slight behavioral change. If apparmor is enabled after container was created this var will not change. I think if this is only called on inspecting single container we can lazy load the value like we do in container startup.

Yes, I agree on that. Maybe docker inspect is not the best place to show security information because one thing is the definition of the security you want and the other the security you really have in place at execution time.

Having said that, we can check in daemon.containerStart if the container apparmor options are compatible with the actual execution environment and if not, change the value to unconfined.

I'm going to commit and push this ASAP and..I will squash the commits, sorry about that :p

thaJeztah · 2016-10-10T16:57:08Z

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

RobSkye · 2016-10-11T16:03:26Z

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:

If you do an inspect after an apparmor change and before the start of the container, still shows the old AppArmorProfile.
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".
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
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.

tonistiigi · 2016-10-12T17:31:49Z

daemon/container_linux.go

Don't understand this. Why is unconfined profile switched back to default.

That is actually an error. I was trying to restore "docker-default" to the unconfined containers if the containers were created after apparmor was enabled but i didn't notice the effect of enabling apparmor to the containers with --security-opt app-armor:unconfined.

I'm gonna think on how manage this correctly.

RobSkye · 2016-10-12T22:03:21Z

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 · 2016-11-03T16:39:25Z

@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 · 2016-12-01T18:16:26Z

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 · 2016-12-04T15:32:53Z

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

thaJeztah · 2017-01-12T19:52:54Z

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

tonistiigi · 2017-01-12T21:14:08Z

daemon/container_linux.go

I'd call this saveApparmorConfig

tonistiigi · 2017-01-12T21:29:39Z

daemon/container_linux.go

This shows unconfined when apparmor is disabled.

Yes, do you want to show "Disabled" instead?

I think empty would do. unconfined implies that special privileged profile was applied(at least to me).

LK4D4 · 2017-01-19T19:12:10Z

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

…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 · 2017-01-30T15:44:12Z

LGTM
ping @tonistiigi

tonistiigi

LGTM

GordonTheTurtle added the status/0-triage label Sep 30, 2016

RobSkye closed this Sep 30, 2016

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

RobSkye reopened this Oct 1, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Oct 1, 2016

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

tonistiigi reviewed Oct 7, 2016

View reviewed changes

tonistiigi reviewed Oct 12, 2016

View reviewed changes

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Dec 19, 2016

thaJeztah assigned LK4D4 and tonistiigi Jan 12, 2017

tonistiigi requested changes Jan 12, 2017

View reviewed changes

LK4D4 added the status/2-code-review label Jan 30, 2017

LK4D4 removed the status/1-design-review label Jan 30, 2017

LK4D4 approved these changes Jan 30, 2017

View reviewed changes

tonistiigi approved these changes Jan 30, 2017

View reviewed changes

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

GordonTheTurtle added this to the 1.14.0 milestone Jan 30, 2017

thaJeztah added the impact/changelog label Jan 30, 2017

spawnflagger mentioned this pull request May 5, 2017

Impossible to see contents of AppArmor Profile "docker-default" #33060

Open

Conversation

RobSkye commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 1, 2016

Uh oh!

RobSkye commented Oct 1, 2016

Uh oh!

thaJeztah commented Oct 1, 2016

Uh oh!

thaJeztah commented Oct 1, 2016

Uh oh!

RobSkye commented Oct 1, 2016

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 10, 2016

Uh oh!

RobSkye commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobSkye commented Oct 12, 2016

Uh oh!

LK4D4 commented Nov 3, 2016

Uh oh!

mlaventure commented Dec 1, 2016

Uh oh!

RobSkye commented Dec 4, 2016

Uh oh!

thaJeztah commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jan 19, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RobSkye commented Sep 30, 2016 •

edited

Loading