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

apparmor: clobber docker-default profile on start #41954

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Feb 1, 2021

In the process of making docker-default reloading far less expensive,
567ef8e ("daemon: switch to 'ensure' workflow for AppArmor
profiles") mistakenly made the initial profile load at dockerd start-up
lazy. As a result, if you have a running Docker daemon and upgrade it to
a new one with an updated AppArmor profile the new profile will not take
effect (because the old one is still loaded). The fix for this is quite
trivial, and just requires us to clobber the profile on start-up.

Carries #40615 and #37353
Fixes: 567ef8e ("daemon: switch to 'ensure' workflow for AppArmor profiles")
Signed-off-by: Aleksa Sarai asarai@suse.de

@cyphar
Copy link
Contributor Author

cyphar commented Feb 1, 2021

Seems as though it has the same failures as before, so I'll need to look into those...

@dirkmueller
Copy link

@cyphar needs a rebase

@cyphar cyphar force-pushed the apparmor-clobber-profile-on-start branch from e08d45e to 24e2cde Compare June 9, 2021 09:43
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

LGTM

// DefaultApparmorProfile returns the name of the default apparmor profile
func DefaultApparmorProfile() string {
// DefaultAppArmorProfile returns the name of the default apparmor profile
func DefaultAppArmorProfile() string {
if apparmor.HostSupports() {
Copy link
Member

Choose a reason for hiding this comment

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

For the benefit of other reviewers, I was slightly concerned about this getting invoked in so many places (especially nested, as below), but the underlying implementation uses a sync.Once + cached bool so it's really efficient. 👍

https://github.com/containerd/containerd/blob/a545df345ee6b2e0cb6b8ae2c4fd11b7738f9185/pkg/apparmor/apparmor_linux.go#L27-L46

@tianon
Copy link
Member

tianon commented Jun 9, 2021

(Although noting the CI failures are the same consistent failures from the last two PRs, so very likely related 😩)

@cyphar
Copy link
Contributor Author

cyphar commented Jun 10, 2021

Yeah I haven't had a chance to debug the PR failure. Will figure that out sometime next week (we've been running with this patch in production for more than a year or two, so it's a bit strange that CI is failing in weird places).

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 16, 2021
@cyphar cyphar force-pushed the apparmor-clobber-profile-on-start branch from 24e2cde to 40112f4 Compare June 29, 2022 14:02
@cyphar cyphar force-pushed the apparmor-clobber-profile-on-start branch from 40112f4 to 3d3af96 Compare July 6, 2023 09:14
In the process of making docker-default reloading far less expensive,
567ef8e ("daemon: switch to 'ensure' workflow for AppArmor
profiles") mistakenly made the initial profile load at dockerd start-up
lazy. As a result, if you have a running Docker daemon and upgrade it to
a new one with an updated AppArmor profile the new profile will not take
effect (because the old one is still loaded). The fix for this is quite
trivial, and just requires us to clobber the profile on start-up.

Fixes: 567ef8e ("daemon: switch to 'ensure' workflow for AppArmor profiles")
Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar cyphar force-pushed the apparmor-clobber-profile-on-start branch from 3d3af96 to a11305b Compare July 7, 2023 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security/apparmor status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants