From a3265102d9318fac00c2425f46b2d532bacc3afb Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Fri, 10 Feb 2023 10:05:56 -0700 Subject: [PATCH 1/2] Revert "Don't check for apparmor_parser to be present" This reverts commit 1acca8bba36e99684ee3489ea4a42609194ca6b9. As stated in the Godoc, this function is intended to check for presence of `apparmor_parser`. Changing this regressed the public API of containerd, and directly contradicts the way that this function is consumed inside of containerd itself: * https://github.com/containerd/containerd/blob/fdfdc9bfc0f865a43c88171110615d1510fad3bc/pkg/apparmor/apparmor.go#L20 * https://github.com/containerd/containerd/blob/fdfdc9bfc0f865a43c88171110615d1510fad3bc/pkg/cri/sbserver/helpers_linux.go#L85 * https://github.com/containerd/containerd/blob/fdfdc9bfc0f865a43c88171110615d1510fad3bc/pkg/cri/server/helpers_linux.go#L144 This has lead to a number of painful regressions and attempted fixes in Moby: * https://github.com/moby/moby/issues/44900 * https://github.com/moby/moby/pull/44902 * https://github.com/moby/moby/issues/44970 While reverting this late into the life of 1.6 and at the start of the life of 1.7 is likely painful, I think this is ultimately the best path to take, as containerd is subject to the same failure to start containers with an AppArmor kernel when `apparmor_parser` is missing as Moby. Signed-off-by: Bjorn Neergaard --- pkg/apparmor/apparmor_linux.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/apparmor/apparmor_linux.go b/pkg/apparmor/apparmor_linux.go index ab54df8eab67..a54c91145fe1 100644 --- a/pkg/apparmor/apparmor_linux.go +++ b/pkg/apparmor/apparmor_linux.go @@ -35,8 +35,10 @@ func hostSupports() bool { checkAppArmor.Do(func() { // see https://github.com/opencontainers/runc/blob/0d49470392206f40eaab3b2190a57fe7bb3df458/libcontainer/apparmor/apparmor_linux.go if _, err := os.Stat("/sys/kernel/security/apparmor"); err == nil && os.Getenv("container") == "" { - buf, err := os.ReadFile("/sys/module/apparmor/parameters/enabled") - appArmorSupported = err == nil && len(buf) > 1 && buf[0] == 'Y' + if _, err = os.Stat("/sbin/apparmor_parser"); err == nil { + buf, err := os.ReadFile("/sys/module/apparmor/parameters/enabled") + appArmorSupported = err == nil && len(buf) > 1 && buf[0] == 'Y' + } } }) return appArmorSupported From d33a43cc2329b6d1514eb89397bc7ad8c8c878ff Mon Sep 17 00:00:00 2001 From: Bjorn Neergaard Date: Fri, 10 Feb 2023 10:13:48 -0700 Subject: [PATCH 2/2] pkg/apparmor: clarify Godoc Signed-off-by: Bjorn Neergaard --- pkg/apparmor/apparmor.go | 12 ++++++------ pkg/apparmor/apparmor_linux.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/apparmor/apparmor.go b/pkg/apparmor/apparmor.go index ef64e5af66ef..293f8ba499b5 100644 --- a/pkg/apparmor/apparmor.go +++ b/pkg/apparmor/apparmor.go @@ -16,13 +16,13 @@ package apparmor -// HostSupports returns true if apparmor is enabled for the host, // On non-Linux returns false -// On Linux returns true if apparmor_parser is enabled, and if we +// HostSupports returns true if apparmor is enabled for the host: +// - On Linux returns true if apparmor is enabled, apparmor_parser is +// present, and if we are not running docker-in-docker. +// - On non-Linux returns false. // -// are not running docker-in-docker. -// -// It is a modified version of libcontainer/apparmor.IsEnabled(), which does not -// check for apparmor_parser to be present, or if we're running docker-in-docker. +// This is derived from libcontainer/apparmor.IsEnabled(), with the addition +// of checks for apparmor_parser to be present and docker-in-docker. func HostSupports() bool { return hostSupports() } diff --git a/pkg/apparmor/apparmor_linux.go b/pkg/apparmor/apparmor_linux.go index a54c91145fe1..c96de6a2688e 100644 --- a/pkg/apparmor/apparmor_linux.go +++ b/pkg/apparmor/apparmor_linux.go @@ -29,8 +29,8 @@ var ( // hostSupports returns true if apparmor is enabled for the host, if // apparmor_parser is enabled, and if we are not running docker-in-docker. // -// It is a modified version of libcontainer/apparmor.IsEnabled(), which does not -// check for apparmor_parser to be present, or if we're running docker-in-docker. +// This is derived from libcontainer/apparmor.IsEnabled(), with the addition +// of checks for apparmor_parser to be present and docker-in-docker. func hostSupports() bool { checkAppArmor.Do(func() { // see https://github.com/opencontainers/runc/blob/0d49470392206f40eaab3b2190a57fe7bb3df458/libcontainer/apparmor/apparmor_linux.go