Move AppArmor policy to contrib & deb packaging #13144

ewindisch · 2015-05-12T03:20:05Z

The automatic installation of AppArmor policies prevents the
management of custom, site-specific apparmor policies for the
default container profile. Furthermore, this change will allow
a future policy for the engine itself to be written without demanding
the engine be able to arbitrarily create and manage AppArmor policies.

This change also introduces tests for the AppArmor policy and
removes a couple ineffective rules.

Signed-off-by: Eric Windisch [email protected]

Wraps the engine itself with an AppArmor policy. This restricts what may be done via a reexec, or through applications we call out to, such as 'xz'. Significantly, this policy restricts the policies to which a container may be spawned into. By default, users will be able to transition to an unconfined policy or any policy prefaced with 'docker-'. Local operators may add new local policies prefaced with 'docker-' without needing to modify this policy. Operators choosing to disable privileged containers will need to modify this policy to remove access to change_policy to unconfined. Builds on top of PR moby#13144. Signed-off-by: Eric Windisch <[email protected]>

jessfraz · 2015-05-12T15:37:09Z

This will need to be added to the new debs, I can send u a PR or we can update after merge

crosbymichael · 2015-05-12T20:47:48Z

@tianon

this change is fine with me

jessfraz · 2015-05-12T20:48:59Z

is there 2 prs open for this?

EDIT nm i get it

jessfraz · 2015-05-12T20:49:21Z

LGTM

tianon · 2015-05-19T15:57:23Z

hack/make/ubuntu

"Suggests" would be the appropriate dependency here, not "Recommends"

Actually, checking back with the debian policy manual, I really prefer this is a Recommends. It's not simply suggested to use an LSM with Docker. Recommends is also what we use for the debian packages.

Do we actually have documentation that explains how to use an LSM effectively in combination with Docker? Without telling users how to actually use this in the real world, we're just blindly setting it and hoping it helps (and with our default profile, making sure it helps for our own desired "use cases" that we think the majority of people use Docker for/in).

@tianon I agree better documentation should be written, but AppArmor simply works out of the box. I haven't seen an Ubuntu machine where Docker and AppArmor weren't just working straight of the box. While documentation would be nice, it isn't necessary for this to be effective for users. It's a good, working default.

It can be argued that breaking out of a container is not something that users should care about, but to the degree that users do care about this, using an LSM is essential. With all kernels to date, it is trivial to break out of a container where an LSM is not used. Using an LSM cannot eliminate that threat, especially against kernel RCEs, but does makes it much more difficult to execute a local privilege escalation.

That said, I believe the policy could be locked down further, both by default, and by local preference. I have been developing better policies in order to improve the default, which I intend to introduce in a separate PR. The work I've done on building a hardened policy would be good material for documentation.

@tianon you make the recommendation on #13156 that a Debian user using the "ubuntu" FPM packages might get AppArmor installed when they don't want it. I suppose that's true. However, they should want AppArmor or SELinux and I think recommending them is reasonable. If the argument is that the user might prefer SELinux and this package is not flexible enough... perhaps we should have an lxc-docker-lsm package that specifies the recommendation with provided-by lxc-docker-lsm-apparmor lxc-docker-lsm-selinux? Then, installs could happen with no LSM (it's still optional), or could satisfy the apt recommendation by either LSM module.

This seems complicated, but seems to be within the model of Debian packaging semantics.

Yes, that's exactly what I mean. In a properly-configured system, dependencies from Recommends get installed automatically. On Debian systems, AppArmor is not installed by default, but the very act of installing Docker will bring it in, which is a very disruptive change. I think this is properly solved by deprecating this "hack/make/ubuntu" package entirely in favor of our new packages where this dependency is already properly added to Recommends for Ubuntu-based platforms.

tianon · 2015-05-19T15:59:06Z

This also needs to happen in hack/make/build-deb via dh-apparmor, which needs to be added to the list of packages in contrib/builder/deb/generate.sh and then the appropriate bits added in hack/make/.build-deb (http://manpages.debian.org/dh_apparmor).

tianon · 2015-05-19T16:00:33Z

Also, this could be considered a regression, since this is happening the way it is today specifically so that people on systems that use AppArmor can install using the static binary without adding any extra files, so this change needs to be considered carefully.

I'm personally +1 on the change, but it is a departure from our traditional stance of "only need this one binary".

tianon · 2015-05-19T16:03:54Z

hack/make/ubuntu

Is the extra space here intentional or accidental? (/etc/apparmor.d/ docker)

Also, shouldn't this only be run if aa-status --enabled 2>/dev/null ?

(ref https://sources.debian.net/src/apparmor/2.9.2-3/debian/debhelper/postinst-apparmor/#L18)

The space is intentional.

The 'aa-status --enabled' is entirely reasonable. Libcontainer was using the check for the binary, which is a terrible way to check, but I was keeping for a minimal change.

ewindisch · 2015-05-19T16:24:36Z

I'm going to update this with suggestions from Tianon.

@tianon FWIW, the SELinux policy isn't included either. One big advantage of having this in packaging is that it becomes possible for users/deployers to roll out a customized default policy. That said, the potential for a dynamic per-container policy (a la process labels) could change how we look at this...

Finally, if we don't want to (immediately) move the policy into packaging then perhaps we merge this into libcontainer? docker-archive/libcontainer#579

tianon · 2015-05-19T16:27:35Z

Yes, I understand and agree with the benefits of this being in the packaging. All I'm saying is that this is different from what we've done previously, so we need to consider carefully whether "just download this one static binary" is still a realistic way to install Docker.

Wraps the engine itself with an AppArmor policy. This restricts what may be done via a reexec, or through applications we call out to, such as 'xz'. Significantly, this policy restricts the policies to which a container may be spawned into. By default, users will be able to transition to an unconfined policy or any policy prefaced with 'docker-'. Local operators may add new local policies prefaced with 'docker-' without needing to modify this policy. Operators choosing to disable privileged containers will need to modify this policy to remove access to change_policy to unconfined. Builds on top of PR moby#13144. Signed-off-by: Eric Windisch <[email protected]>

ewindisch · 2015-05-29T15:01:02Z

Now with tests! (and rebased)

Wraps the engine itself with an AppArmor policy. This restricts what may be done via a reexec, or through applications we call out to, such as 'xz'. Significantly, this policy restricts the policies to which a container may be spawned into. By default, users will be able to transition to an unconfined policy or any policy prefaced with 'docker-'. Local operators may add new local policies prefaced with 'docker-' without needing to modify this policy. Operators choosing to disable privileged containers will need to modify this policy to remove access to change_policy to unconfined. Builds on top of PR moby#13144. Signed-off-by: Eric Windisch <[email protected]>

ewindisch · 2015-06-02T15:36:24Z

Made this less controversial / safer to merge:

Keeps it as a 'suggests' on Debian (although I'd like to change this later...)
Does not change the AppArmor policy from what is currently in libcontainer

I'll submit subsequent PRs to tighten the policy and add further tests.

tianon · 2015-06-02T16:06:58Z

LGTM (although eyebrow raise at 8 commits 😄)

ewindisch · 2015-06-02T18:58:18Z

@tianon I could squash if it's worthwhile. shrug

thaJeztah · 2015-06-02T19:28:25Z

Well, we tell every other contributor to squash, so...

The automatic installation of AppArmor policies prevents the management of custom, site-specific apparmor policies for the default container profile. Furthermore, this change will allow a future policy for the engine itself to be written without demanding the engine be able to arbitrarily create and manage AppArmor policies. - Add deb package suggests for apparmor. - Ubuntu postinst use aa-status & fix policy path - Add the policies to the debian packages. - Add apparmor tests for writing proc files Additional restrictions against modifying files in proc are enforced by AppArmor. Ensure that AppArmor is preventing access to these files, not simply Docker's configuration of proc. - Remove /proc/k?mem from AA policy The path to mem and kmem are in /dev, not /proc and cannot be restricted successfully through AppArmor. The device cgroup will need to be sufficient here. - Load contrib/apparmor during integration tests Note that this is somewhat dirty because we cannot restore the host to its original configuration. However, it should be noted that prior to this patch series, the Docker daemon itself was loading apparmor policy from within the tests, so this is no dirtier or uglier than the status-quo. Signed-off-by: Eric Windisch <[email protected]>

ewindisch · 2015-06-02T19:36:41Z

squashed

thaJeztah · 2015-06-02T19:38:31Z

💓 thanks!

tianon · 2015-06-08T21:53:31Z

hack/make/.build-deb/docker-engine.install

@jfrazelle you were planning to take over adding the dh-apparmor bits to actually plumb this into the maintscripts automatically post-merge, right?

@tianon are you just asking use of dh_apparmor here?

Right -- just installing the file isn't enough; you've got to have this line (which is perfect), and a dh_apparmor line in "rules" ( http://manpages.debian.org/dh_apparmor; something like "dh_apparmor --profile-name=bin.foo" probably in "override_dh_install"). On top of that, we need to add "dh-apparmor" in "contrib/builder/deb/generate.sh". https://codesearch.debian.net/results/dh_apparmor is a useful way to see examples of how other packages invoke this same helper.

tianon · 2015-06-08T21:54:36Z

LGTM -- @jfrazelle you decide who's going to add the rest of the build-deb bits 😉 (I'm happy to do it or to defer to you)

ewindisch · 2015-06-15T19:05:33Z

@jfrazelle @tianon happy to look into it myself, not sure what other build-deb bits you're looking for...

calavera · 2015-06-18T17:55:34Z

it needs a rebase 😔

crosbymichael · 2015-07-07T23:53:13Z

@ewindisch ping!

reopen when you have this rebased and ready to be looked at again.

thanks

ewindisch · 2015-07-13T19:19:20Z

@crosbymichael rebase and fixes for @unclejack applied.

thaJeztah · 2015-07-13T19:22:27Z

@ewindisch oh! Looks like you'll have to create a new pull-request, because GitHub doesn't update PRs if they are closed (and I can no longer re-open because of that) :'(

GordonTheTurtle added the status/0-triage label May 12, 2015

ewindisch force-pushed the apparmor-policy branch 3 times, most recently from 0992a8a to 97c4f9d Compare May 12, 2015 04:08

ewindisch mentioned this pull request May 12, 2015

Add AppArmor policy for the engine #13156

Closed

jessfraz added status/2-code-review and removed status/0-triage labels May 13, 2015

tianon reviewed May 19, 2015
View reviewed changes

ewindisch force-pushed the apparmor-policy branch 2 times, most recently from 6dae1f6 to 52715f8 Compare May 29, 2015 14:59

ewindisch force-pushed the apparmor-policy branch 2 times, most recently from a803f41 to a649a65 Compare June 2, 2015 15:28

ewindisch force-pushed the apparmor-policy branch from a649a65 to c9d53ff Compare June 2, 2015 15:38

ewindisch force-pushed the apparmor-policy branch from c9d53ff to f5f70d5 Compare June 2, 2015 19:36

tianon reviewed Jun 8, 2015
View reviewed changes

ewindisch mentioned this pull request Jun 21, 2015

Do not generate/load AppArmor profiles docker-archive/libcontainer#645

Merged

crosbymichael closed this Jul 7, 2015

ewindisch mentioned this pull request Jul 13, 2015

Move AppArmor policy to contrib & deb packaging #14609

Merged

jessfraz added the area/security/apparmor label Oct 18, 2015

Move AppArmor policy to contrib & deb packaging #13144

Move AppArmor policy to contrib & deb packaging #13144

Uh oh!

Conversation

ewindisch commented May 12, 2015

Uh oh!

jessfraz commented May 12, 2015

Uh oh!

crosbymichael commented May 12, 2015

Uh oh!

jessfraz commented May 12, 2015

Uh oh!

jessfraz commented May 12, 2015

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon Jun 1, 2015 via email

Choose a reason for hiding this comment

Uh oh!

tianon commented May 19, 2015

Uh oh!

tianon commented May 19, 2015

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!

ewindisch commented May 19, 2015

Uh oh!

tianon commented May 19, 2015 via email

Uh oh!

ewindisch commented May 29, 2015

Uh oh!

ewindisch commented Jun 2, 2015

Uh oh!

tianon commented Jun 2, 2015

Uh oh!

ewindisch commented Jun 2, 2015

Uh oh!

thaJeztah commented Jun 2, 2015

Uh oh!

ewindisch commented Jun 2, 2015

Uh oh!

thaJeztah commented Jun 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon Jun 15, 2015 via email

Choose a reason for hiding this comment

Uh oh!

tianon commented Jun 8, 2015

Uh oh!

ewindisch commented Jun 15, 2015

Uh oh!

calavera commented Jun 18, 2015

Uh oh!

crosbymichael commented Jul 7, 2015

Uh oh!

ewindisch commented Jul 13, 2015

Uh oh!

thaJeztah commented Jul 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects