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: Check if apparmor_parser is available #44902
Conversation
a57b80c
to
32095c3
Compare
225c728
to
e433ffc
Compare
(CI failure unrelated - will go away after #44930 is merged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some simple correctness issues, but a step in the right direction. I feel as though fundamentally this package is not well architected/abstracted and while a couple tweaks can make the initial patch good, I'd like to explore some more refactoring/restructuring to better separate concerns, calls to apparmor_parser
, and make the error flow better.
Even wondering if we still need the version at all. I see it was added in 8cf8924 (#17002) (October 2015), updated in f8db9a0, and later fixed in 4bf7a84 (#20305). The original reason was described in #17002;
Which seems to be "older versions" in 2015, so very unlikely any of that still applies. |
3cb4e43
to
aacfa17
Compare
Notes from today's maintainer's call: We're changing this PR to be a simple reproduction of the old containerd-provided logic (just moving it upwards in the call stack) so we can be confident we're bug-for-bug compatible with the old behavior. We will then look at a refactor (probably backported to 23.0) that makes this code maintainable. |
daemon/apparmor_default.go
Outdated
} | ||
}) | ||
|
||
if apparmor.HostSupports() && hasAppArmorParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move apparmor.HostSupports()
inside the sync.Once
as that is what the original logic did, and we additionally need to actually check apparmor is enabled in the kernel before claiming that it is in a log message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess apparmor.HostSupports()
also uses a sync.Once()
on its own, so maybe not a huge difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly wondering if we should put the log.Warn
outside of the sync.Once though; maybe we want it to log on every invocation instead of only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just wrap twice with a second sync.Once
, as that's the best way to emulate the existing behavior (only checking for the binary once at this call site) we have. Similarly, I think that it makes more sense to log inside the sync.Once, as:
- the logging is net-new and 'good enough' to tide us over until a followup refactor
- it allows for the most minimal change/lets us preserve the exact logic from containerd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, thanks for catching this 😄
As for logging - I think it makes sense to do it inside the sync.Once
.
Doing it every time will simply spam the daemon log. And it could be a good thing as it definitely attracts more attention than one log line somewhere in the beginning.
However in this case it seems that in most cases where the cause of not having the apparmor_parser
is just the characteristic of the environment rather than user's error - and he can't do anything about it.
`hostSupports` doesn't check if the apparmor_parser is available. It's possible in some environments that the apparmor will be enabled but the tool to load the profile is not available which will cause the ensureDefaultAppArmorProfile to fail completely. This patch checks if the apparmor_parser is available. Otherwise the function returns early, but still logs a warning to the daemon log. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
aacfa17
to
ab3fa46
Compare
CI failure is a flaky test; tracked in #38587 (I'll give CI a kick later)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! I think @neersighted's comment has been addressed in the latest iteration, and asked @rumpl to do a review to keep me honest; I'll go ahead and merge this one 👍 |
Post-merge LGTM from me! |
Can AppArmor rules be enforced without the userland utilities installed? I think the warning is misleading, respectively is unnecessary noise for admins which want to debug actual issues:
A warning doesn't hurt much, but when real issues are faced and investigated, then every log entry with warning severity or above might bring admins on a wrong track. So I'd personally try to keep those at a minimum when related admin intentions are obvious. |
A warning is useful, as the 23.0.0 release of Moby has revealed that plenty of users are running an AppArmor-by-default kernel, on a distro with AppArmor support, and do not have We're logging now as it's not necessarily an error since the daemon used to just silently continue without AppArmor in this scenario. However, it's often unexpected that AppArmor will not be enforced, even it it otherwise could. Environments that should support AppArmor, but will not out of the box:
Environments that can not support AppArmor, but could have an AppArmor kernel:
Based on this, we discussed in our maintainers' call a migration to a trinary state:
This was discussed as being part of the core design for a successor to #41442, but is out of scope for this initial patch for a regression. |
Not sure whether I understand everything. Where shall this I know what this PR is about and ran into the error myself. I just think it can be perfectly silently continued without using AppArmor if the userland utilities, respectively Another question is whether to add a section to the docs (if not there already) about the security benefits of AppArmor, how Docker is using it and how to enable it on some common distros. |
It's not yet determined, but probably a flag/in the daemon JSON. The intention is to 'reintroduce the failure,' but in a deliberate way. That is to say, to force the user to specify whether or not they want AppArmor instead of silently continuing. AppArmor can be used all the way down the nested container stack, e.g. Debian containers on a Debian host. As Debian as a system is AppArmor-by-default, silently continuing without AppArmor because the user may have forgotten the package when building their Docker-in-Docker image is a rather sharp edge. So again, this warning is just a debugging aid for the maintainers, and we are preserving the behavior of 20.10 with this PR. The maintainers are quite conscious of the need to make the AppArmor support better documented, solve some of the sharp edges/murky areas of the current implementation, and to attempt to make what is an import security feature require explicit intent from the administrator instead of attempting to guess what the user wants. In any case, this is not the forum to discuss it -- I'd suggest that you keep an eye out for the design proposal I mentioned when it materializes. It most likely will mention/close out the linked PR, so that's probably the place to watch. |
Thanks for the explanation and insights.
Probably then reducing severity to "debug" would be appropriate? Because
this I think is not something you need to worry about. If anything related to AppArmor does not work but is expected to work, the first thing one would check is whether related system packages are installed, isn't it? My main concern here is that more (especially higher severity) logs can help, but can also complicate/lengthen debugging when turning focus into wrong directions. And so far I see this being the case with this warning, since it is expected (and must be ignored) in a a significant number of cases (as the reason for this PR showed). However,
so enough of my 3 (or more) cents on this topic 🙂. |
A warning is very appropriate as we would have used AppArmor if we had support. A warning doesn't mean something is broken necessarily; in this case we are indicating that "We would use AppArmor, but you don't have the userspace binary, so we are continuing as if the kernel had it disabled" which is an appropriate usage of a warning-level log. The absence of warnings in the logs does not imply a properly functioning install, and the presence of warnings does not imply a broken daemon. It's simply a log level meant to signify "you might want to look at this and consider if you expect it, because you are not on the default/happy path." Moreover, log messages at the end of the day are primarily a debugging aid and are aimed more at the maintainer/administrator debugging than a user who is deploying a system. That is what documentation is for, and as you have noted, there is room for expansion. |
Also re:
This is patently not the case; the vast majority of users should have AppArmor installed when their kernel supports it, and a lack of AppArmor was caused by a historical quirk of the get.docker.com script. AppArmor in the kernel without userspace support is a unusual/more niche situation, and deserves to be noted as such to help with troubleshooting. There are very legitimate use cases for it (mixed userland with OS containers like systemd-nspawn, LXC/LXD; Docker-in-Docker) but when that happens, it is worth logging. Moreover, the user can opt out of AppArmor (as it is only Likewise, logging today is useful to make that situation apparent when reviewing daemon logs (as we would otherwise expect AppArmor to be present on a Debian/Ubuntu kernel reported in |
Closing the discussion about the warning, you have some valid points there. For the |
I think that design discussion is sufficiently out of scope, so it should be addressed to the design we spitballed in our last meeting if it makes it to the proposal/implementation stage. However, the current thinking is that that AppArmor should be opt-out if it is available/possible, so that a missing TL;DR: Building the kernel with AppArmor on by default is a statement of intent, and we should respect that statement of intent as much as possible. |
Having AppArmor userland tools not installed is also a statement of intent, especially as those are a |
This reverts commit 1acca8b. 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: * moby/moby#44900 * moby/moby#44902 * moby/moby#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 <bneergaard@mirantis.com>
This reverts commit 1acca8b. 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: * moby/moby#44900 * moby/moby#44902 * moby/moby#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 <bneergaard@mirantis.com> (cherry picked from commit a326510) Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This reverts commit 1acca8b. 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: * moby/moby#44900 * moby/moby#44902 * moby/moby#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 <bneergaard@mirantis.com>
This reverts commit 1acca8b. 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: * moby/moby#44900 * moby/moby#44902 * moby/moby#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 <bneergaard@mirantis.com>
This reverts commit 1acca8b. 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: * moby/moby#44900 * moby/moby#44902 * moby/moby#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 <bneergaard@mirantis.com>
#42276 replaced the function we used to check whether AppArmor is supported on the host from the one in
runc/libcontainer/apparmor
tocontainerd/pkg/apparmor
.In containerd v1.6.1 this function has dropped a check for the
apparmor_parser
binary: containerd/containerd#5519It's possible in some environments that the apparmor will be enabled but the tool to load the profile is not available which will cause the
ensureDefaultAppArmorProfile
to fail completely.This patch checks the apparmor_parser is available, if it's not, then the function returns early, but still logs a warning to the daemon log.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)