Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces: enable partial apparmor support #3814
Conversation
zyga
added some commits
Aug 25, 2017
All checks have passed
zyga
requested a review
from
jdstrand
Aug 28, 2017
codecov-io
commented
Aug 28, 2017
•
Edited 1 time
-
codecov-io
Aug 30, 2017
-
Aug 30, 2017
Codecov Report
@@ Coverage Diff @@
## master #3814 +/- ##
==========================================
+ Coverage 75.86% 75.92% +0.06%
==========================================
Files 403 417 +14
Lines 34835 36025 +1190
==========================================
+ Hits 26427 27352 +925
- Misses 6535 6755 +220
- Partials 1873 1918 +45
Continue to review full report at Codecov.
|
| + // When apparmor partial then use the classic template as we cannot enable | ||
| + // effective confinement and we cannot rely on the devmode template which | ||
| + // may have things that local apparmor doesn't understand. | ||
| + level, _ := aa.ProbeKernel().Evaluate() |
mvo5
Aug 28, 2017
Collaborator
Why are we using aa.ProbeKernel().Evaluate()" instead of just "aa.Evaluate()" and hidding the "ProbeKernel()" as an implementation detail in aa ?
zyga
Aug 28, 2017
Contributor
Because later on we'll get the result of ProbeKernel from the backend itself and specific interfaces will be able to get it from there.
zyga
referenced this pull request
Aug 28, 2017
Closed
Allow snap-confine to be confined even with --disable-apparmor #3760
|
This looks sane from my outside perspective. Didn't Jamie ask for loud logging when the wide open policy is being used? I don't see that. Also, once this is done, I think snap-confine can lose the --disable-apparmor option entirely (in general, because we want snap-confine to be usable from the core snap on all systems, it can't really have compile time options!). |
jdstrand
reviewed
Aug 29, 2017
This looks fine-- looking forward to followup PRs since I'd prefer we not overuse the classic template for forced devmode. I do wonder why we just don't go to straight strict mode here-- IME, all partial is really important for in comparison with strict is logging that certain things aren't mediated.
| - if opts.Classic && !opts.JailMode { | ||
| + // When apparmor partial then use the classic template as we cannot enable | ||
| + // effective confinement and we cannot rely on the devmode template which | ||
| + // may have things that local apparmor doesn't understand. |
jdstrand
Aug 29, 2017
Contributor
This comment isn't quite right. Note that classicTemplate has bare rules for all the modern rules so an old parser will always choke on them. This means that partial confinement must have new tools, and those new tools know how to work with all kernels. As such, this should read as:
// When partial AppArmor is detected, use the classic template for now. We could
// use devmode, but that could generate confusing log entries for users running
// snaps on systems with partial AppArmor support.
| + | ||
| + // This should be logger.Noticef but due to ordering of initialization | ||
| + // calls, the logger is not ready at this point yet and the message goes | ||
| + // nowehre. Per advice from other snapd developers, we just print it |
| + // This should be logger.Noticef but due to ordering of initialization | ||
| + // calls, the logger is not ready at this point yet and the message goes | ||
| + // nowehre. Per advice from other snapd developers, we just print it | ||
| + // directly. |
jdstrand
Aug 29, 2017
Contributor
By printing directly, this means that it will end up in journalctl, which is important since we want syslog to show this as well. Please mention that here as part of your comment.
|
@mwhudson the |
zyga
added some commits
Aug 30, 2017
Some checks were not successful
|
Updated per review feedback. Would love to see this in 2.28 if we can pull it off @mvo5 :-) |
zyga
added this to the 2.28 milestone
Aug 30, 2017
jdstrand
approved these changes
Aug 30, 2017
Thanks for the updates!
When will you be implementing the change for using strict mode with logging instead of just using the classic template?
|
Soon, as time allows :) |
niemeyer
requested changes
Aug 30, 2017
This is going in a nice direction, thanks for bringing that up. That said, let's please talk through that API, and the logging also needs fixing.
| + // When partial AppArmor is detected, use the classic template for now. We could | ||
| + // use devmode, but that could generate confusing log entries for users running | ||
| + // snaps on systems with partial AppArmor support. | ||
| + level, _ := aa.ProbeKernel().Evaluate() |
niemeyer
Aug 30, 2017
Contributor
Agree with Michael: this interface is too complicated for how trivial this is. What is the expected result of probing for a kernel and what does it mean to Evaluate it?
How about: aa.SupportLevel() == aa.PartialSupport?
The fact we need to compute and cache should remain inside the apparmor package itself. The release package may help with some ideas on that regard.
Also, what would "partial" mean in this case, and why do we expect what is inside classic to work while what is in strict won't? It would be better to be more clear about what is supported on not, and if possible have that gradient explicit in the terminology.
As a side note, Michael is extremely polite.. when he raises something on my code or ideas, I think three times before dismissing it.
niemeyer
Aug 30, 2017
Contributor
In fact, that's all the apparmor package does right now. This would be better as release.ApparmorSupport instead, I think.
zyga
Aug 31, 2017
Contributor
Please bare with me, the result of ProbeKernel will be stored in the apparmor backend. From there we will call Evaluate in one spot and access available features in several spots. I can bring this patch earlier. Partial means, as it is documented, that apparmor is enabled but some of the required features are missing. As discussed with jdstrand classic is used because it works on every release of apparmor and allows us to take the first step. The next step will be to switch to the actual strict template and teach specific interfaces about any extra deficiencies or incompatibilities that resulted from how apparmor enforcement works when a particular feature is present or absent. We already have the gradient you are talking about - we can ask, through SupportsFeature if a given feature is enabled or not.
niemeyer
Sep 1, 2017
Contributor
@zyga I understood this proposal from your earlier comments. My points above already take it into account.
niemeyer
Sep 1, 2017
•
Edited 1 time
-
niemeyer
Sep 1, 2017
Contributor
-
Sep 1, 2017
release.ApparmorSupports(feature) sounds nice, btw.
| + // snapd.service. This aspect should be retained even after the switch to | ||
| + // user-warning. | ||
| + if featureLevel != aa.Full { | ||
| + fmt.Printf("WARNING: %s\n", summary) |
niemeyer
Aug 30, 2017
Contributor
Code that prints on import seems pretty bad. Let's please drop this and the long apologetic comment. :)
We can easily log in the real log, and in a proper place, if we want to.
zyga
Aug 31, 2017
Contributor
We cannot log the warning to the real log because of initialization order of our logger. This does the right thing because stdout is properly logged into the journal.
| + // Enable apparmor backend if there is any level of apparmor support, | ||
| + // including partial feature set. This will allow snap-confine to always | ||
| + // link to apparmor and check if it is enabled on boot, knowing that there | ||
| + // is always *some* profile to apply to each snap process. |
niemeyer
removed this from the 2.28 milestone
Aug 30, 2017
|
I'll clean up this PR shortly. |
zyga
added some commits
Sep 12, 2017
Some checks were not successful
All checks have passed
jdstrand
requested changes
Sep 13, 2017
I reviewed the new commits since my last review and have a few questions, so marking back to 'Request changes' for now.
| -// Evaluate checks if the apparmor module is enabled and if all the required features are available. | ||
| -func (ks *KernelSupport) Evaluate() (level FeatureLevel, summary string) { | ||
| +// SupportLevel checks if the apparmor module is enabled and if all the required features are available. | ||
| +func (ks *KernelSupport) SupportLevel() (level SupportLevel, summary string) { |
jdstrand
Sep 13, 2017
Contributor
It feels a little weird that this function is called SupportLevel() and it takes an argument of type SupportLevel. Should the function be called GetSupportLevel()?
| @@ -366,6 +370,9 @@ snippet | ||
| }} | ||
| func (s *backendSuite) TestCombineSnippets(c *C) { | ||
| + restore := release.MockAppArmorSupportLevel(aa.FullSupport) | ||
| + defer restore() | ||
| + | ||
| // NOTE: replace the real template with a shorter variant | ||
| restoreTemplate := apparmor.MockTemplate("\n" + |
jdstrand
Sep 13, 2017
Contributor
The mixture of 'aa.Foo' and 'apparmor.Foo' here and elsewhere in the PR seems weird to me. Is there a reason why you aren't consistently using one over the other?
| @@ -55,8 +55,24 @@ var ( | ||
| // ForceDevMode returns true if the distribution doesn't implement required | ||
| // security features for confinement and devmode is forced. | ||
| func (o *OS) ForceDevMode() bool { | ||
| - level, _ := apparmor.ProbeKernel().Evaluate() | ||
| - return level != apparmor.Full | ||
| + return AppArmorSupportLevel() != apparmor.FullSupport |
jdstrand
Sep 13, 2017
Contributor
I realize that you are returning the equivalent of before, but this (forcing devmode if not full support) seems inconsistent with interfaces/backends/backends.go init() where now add the apparmor backend if there is any level of support. Perhaps at least the comment needs to be adjusted?
niemeyer
Sep 13, 2017
Contributor
Let's please not change this here. ForceDevMode as an idea should hopefully die altogether, but this PR is lingering for too long on issues which are related to its implementation. Let's keep unrelated fixes for follow ups please.
jdstrand
Sep 13, 2017
Contributor
I agree it should be in a future PR, but felt the inconsistency should at least be pointed out (so we know to change it going forward! :) and that perhaps the comment should change.
| + | ||
| +// AppArmorSupportLevel quantifies how well apparmor is supported on the current kernel. | ||
| +func AppArmorSupportLevel() apparmor.SupportLevel { | ||
| + level, _ := aa.SupportLevel() |
niemeyer
Sep 13, 2017
Contributor
Sorry, but it feels super strange to have such trivial helpers to our own trivial helpers (!) that are sitting across two different packages, and having one of those two packages consistently entirely of the trivial helpers.
Since this is taking a bit longer than it should for such a simple change, here is a more clear path for LGTM:
- The level function becomes release.AppArmorLevel
- The type becomes release.AppArmorLevelType
- The summary becomes release.AppArmorSummary
- All of that inside release/apparmor{,_test}.go
- KernelSupport dies. All of that functionality fits inside a tiny init function that runs once during execution and stores single two global variables: one level, one summary.
- apparmor package dies. Its whole point is that funcionality today.
niemeyer
Sep 13, 2017
•
Edited 1 time
-
niemeyer
Sep 13, 2017
Contributor
-
Sep 13, 2017
Ah, and consts might be release.{No,Partial,Full}AppArmor.
|
Since @niemeyer addressed my questions, is driving the design and I agree with that design, I'll mark my bits as approved now. |
Some checks were not successful
|
This should be ready for a re-review. |
niemeyer
approved these changes
Sep 18, 2017
Thanks for sorting this out, @mvo5!
LGTM.. just re-running tests now.
|
The broken test looks unrelated, btw. Seems to be an unrelated Debian issue: |
Some checks were not successful
zyga commentedAug 28, 2017
•
Edited 1 time
-
zyga
Aug 28, 2017
This branch is inspired by an idea from @jdstrand to allow working with
mainline apparmor support by using the classic template (which gives broad
permissions) on such systems.
This has three direct consequences:
First, snap-confine can now be built with apparmor support on Debian and
OpenSUSE and those systems will gracefully degrade to devmode before all
apparmor features are available but will work with strict confinement as soon
as a compatible kernel is available.
The second advantage is that we may expand this concept to allow specific
interfaces to offer specific snippets if a given apparmor feature is present.
This would allow to create a level of confinement above devmode. Where, for
example, file rules are enforced on a wide number of systems. Certain
interfaces (e.g. those controlling IPC) would necessarily be more open than we
would like but it would improve the effective level of the sandbox for many
people.
Lastly this change will allow us to enable re-exec support on openSUSE as
snap-confine built from the core snap will now be compatible.
All those improvements will be proposed separately for consideration.
Signed-off-by: Zygmunt Krynicki me@zygoon.pl