(Probably needs some tests.)

I was under the impression that the current way to customize AppArmor profiles was to ship a /etc/apparmor.d/docker-default profile. What does this PR add that you can't already achieve with that? Wouldn't that allow distributions to work with docker without patching the binary?

This patch also fixes a few minor bugs in the AppArmor profile implementation (notably, Docker would not forcefully load its built-in profile at start time -- meaning a Docker update wouldn't actually update the profile).

Maybe better to send fixes as a separate PR?

We've hit this before and it's a little annoyed that dockerd manages its own profile instead of writing a /etc/apparmor.d profile as part of its deb configuration. We use a local addition for abstractions/base and because Docker manually shells out to apparmor_parser, restarting the apparmor unit doesn't reload docker-default. We've had to manually nuke the docker-default profile by interacting with the securityfs instead to force dockerd to pick up our changes.

Is there some history on why dockerd manages its own profile instead of including it as part of its packaging?

I was under the impression that the current way to customize AppArmor profiles was to ship a /etc/apparmor.d/docker-default profile.

That is not /quite/ right and actually this patch would make this no longer true at all (because of the forcefully load patch). Right now, Docker will try to insert it's own profile if it isn't already loaded -- though it is true that adding a profile file to /etc/apparmor.d/docker-default does mean AppArmor will load it for you (meaning Docker won't push it's own -- at least before this PR). But if you end up with the wrong ordering of service loads or some other such rubbish you may end up with the profile being inserted by Docker instead (docker doesn't directly check the existence of a docker-default profile in /etc/apparmor.d.

And as mentioned this behaviour is actually a bug because we need to forcefully push the profile on Docker daemon start because otherwise updates will silently result in old AppArmor profiles being used (meaning security or correctness fixes won't be applied until after reboot).

Another thing this patch allows is for you to more directly control and know what the default profile is, which currently isn't possible because we don't write to /etc/apparmor.d (in fairness this is a fairly contrived way of adding this feature -- but it's done to mirror the seccomp default profile design) which you note here:

We've hit this before and it's a little annoyed that dockerd manages its own profile instead of writing a /etc/apparmor.d profile as part of its deb configuration

This is intentional (though I agree it is annoying -- and I say that as the person who originally implemented this behaviour in #26518). This is working around a design limitation of AppArmor which was found in #29097 (in particular when it comes to the apparmor.service and unattended upgrades) -- in short, when AppArmor reloads profiles in certain scenarios it unloads the profile. This causes all running containers on the system to become permanently unconfined. I don't know if AppArmor has stopped doing this behaviour since then, but the underlying design (removing a profile makes all processes under that profile unconfined) is still the case today. There were other reasons why we implemented this (some distributions need to have a read-only /etc partition), but that was the primary one.

EDIT: I actually can't find the original issue describing the AppArmor reload behaviour, but I am fairly sure that this was a problem we hit at some point in the past...?

Is there some history on why dockerd manages its own profile instead of including it as part of its packaging?

See above, though the reason for not including it in packaging and needing to dynamically generate it is that certain AppArmor profile features are dependent on which version of apparmor_parser you have on a system. So we actually use Go templates to generate the full profile so that Docker can work on any distribution with any version of apparmor_parser as well as different layouts of the AppArmor system "headers". (We also auto-detect the name of the profile the Docker daemon runs as.)

Breaking the ability to override docker-default by shipping it yourself sounds like a bad idea. We've used this in the past when there have been bugs in docker's apparmor parsing.

I remember the issue of containers dropping to unconfined, but I can't reproduce with my currently install apparmor unit. Even if you update a profile, it appears that existing processes running as the profile don't become unconfined:

$ cat test.sh 
#!/bin/bash -e

apparmor_parser --version

# Setup profile for /tmp/test.
echo "profile test /tmp/test flags=(complain) {
}" | sudo tee /etc/apparmor.d/tmp.test > /dev/null
sudo systemctl restart apparmor

# Write /tmp/test bash script and run it.
echo "#!/bin/bash -e
sleep infinity" > /tmp/test
chmod +x /tmp/test
/tmp/test &
test_pid="$!"

# Print the current profile.
echo "Script running as: $( cat "/proc/${test_pid}/attr/current" )"

# Modify profile, reload it, then see what /tmp/test is running as.
echo "profile test /tmp/test flags=(complain) {
  /** rw,
}" | sudo tee /etc/apparmor.d/tmp.test > /dev/null
sudo systemctl restart apparmor
echo "Script running as: $( cat "/proc/${test_pid}/attr/current" )"

# Cleanup profile and script.
kill "$test_pid"
sudo rm /etc/apparmor.d/tmp.test
sudo systemctl restart apparmor
$ ./test.sh 
AppArmor parser version 2.13.4
Copyright (C) 1999-2008 Novell Inc.
Copyright 2009-2018 Canonical Ltd.
Script running as: test (complain)
Script running as: test (complain)

Also you can run code during configuration of debs and rpms to do templating :)

I remember the issue of containers dropping to unconfined, but I can't reproduce with my currently install apparmor unit.

This may have been fixed since then and to be honest probably was some weird interaction with the systemd apparmor "service" on Ubuntu/Debian (which internally might have done some wacky stuff to /etc/apparmor.d and then done a full reload of all profiles -- I'm not sure). AppArmor has supported replacing profiles in a way where you don't unconfine processes since basically forever -- and assuming that this issue no longer exists I'd be happy to go back to using /etc/apparmor.d (as long as we handle EROFS nicely).

Breaking the ability to override docker-default by shipping it yourself sounds like a bad idea.

I don't want to break things, but right now Docker is broken with regards to upgrades and we need to fix it somehow. Future updates to Docker will involve changes to the built-in docker-default AppArmor profile and we need to have a way for such upgrades to happen. Right now Docker basically requires a reboot because it will not forcefully load the profile after an upgrade.

In addition the fact that docker-default overrides worked before (the way you describe) was a coincidence -- it was a consequence of the change I wrote in #29130. I'm not against custom profiles at all, it's just that Docker wasn't written to support it in that way and the fact it worked was not intentional.

Fixing the issue of upgrades requires clobbering the existing profile if the existing profile is an old Docker built-in profile -- the catch is it's not possible to tell whether a profile is a built-in or custom one without any other information from the system administrator (regardless of whether we start writing to /etc/apparmor.d). While it is not ideal the --apparmor-profile change in this PR allows you to actually indicate that you have a custom profile and Docker shouldn't mess with it (though in fairness we would end up running apparmor_parser and inserting it for you -- we could change that behaviour).

We've used this in the past when there have been bugs in docker's apparmor parsing. [emphasis added]

Can you clarify what you mean by that? Docker doesn't parse AppArmor profiles. (But I do agree that being able to modify the docker-default profile is a useful feature -- hence this PR.)

Also you can run code during configuration of debs and rpms to do templating :)

Most distributions don't use the packaging scripts in contrib (because they don't match distribution packaging policy) and as a result you would end up with default security profiles being balkanised between different entities having to keep them up to date. Mercifully, the docker-default profile doesn't change too often but imagine if we had a similar policy for the seccomp profile -- every new syscall (meaning more or less every upstream kernel release) would require every distribution to keep track of the upstream changes to the default seccomp profile.

If we supported /etc/apparmor.d setup, I'm sure some distributions might make use of this feature, but I don't think it's a good idea to require it -- Docker should be able to set up AppArmor if the system hasn't already done it.

I'm fine with this change for the reasons you state, but frankly there's no way we can use Docker's apparmor integration after this. Though our setup is a little unique :) We won't give dockerd the permissions to modify apparmor state. Being able to provide a docker-default profile ourselves was a nice out, so you might want to ensure other distributions weren't using that feature.

I could implement some kind of "don't you dare touch docker-default" mode where you tell Docker that it isn't in control of the profile (basically doing what you do now except Docker wouldn't even attempt to install the profile if it's missing). My point was that we need some way that you can tell Docker "no really, I'm in control here" -- because the current state is not really compatible with that requirement.

Sorry, didn't mean to be so dramatic with that. I think that I'll handle the docker-default issues with a feature or workaround like #41553, that shouldn't impact this PR.

This PR lgtm. Is there a place to document this change?

Are you sure you don't want something like --apparmor-profile=NONE (or something) which would indicate that Docker should try to use docker-default but without loading it itself?

I think it'd be easier for us to break the daemon's access to the AppArmor securityfs to disable AppArmor integration. I believe we can make runc transition to a container profile using AppArmor if we really wanted to :)

Either way we'd need to control the dockerd profile, since we'd both want to restrict change_profile to only docker-default and we need to block access to our enterprise specific paths. I think it's easier to skip the --apparmor-profile=NONE feature for now.