apparmor: support AppArmor 2.13 by jdstrand · Pull Request #6549 · canonical/snapd

jdstrand · 2019-02-27T22:43:28Z

apparmor: support unified cache directories
apparmor 2.13 as included in Debian/Ubuntu and their derivatives moved
the system profile cache directory out of /etc/apparmor.d/cache into a
unified cache directory in /var/cache/apparmor. Detect and adjust for
this.
apparmor: support 2.13+ cache directory structure
apparmor 2.13 moved away from a single flat cache directory to a forest
of cache directories (among other things, this helps with boot
performance when going back and forth between kernels with different
feature sets). Detect the cache forest and adjust to remove cache files
from the forest.
interfaces/docker-support: add accesses for apparmor cache forest
apparmor: remove code that pretends to unload apparmor profiles
unloadProfiles() is currently called with a list of profile names and
it invokes apparmor_parser --remove on those names. Unfortunately,
apparmor_parser --remove requires full paths to the profiles, not just a
profile name, so the profiles were not removed. While we could prepend
dirs.SnapAppArmorDir to the profile names, by the time unloadProfiles()
is called it is too late because the profiles on disk have already been
removed. This was never caught before because apparmor_parser less than
2.13 would return 0 when given profiles that didn't exist and so the
error check would never fire.

The code removal preserves the actual current behavior (ie, where no
apparmor profiles are removed from the kernel) for all versions
of apparmor. The previous code was somewhat misguided for the current
implementation of snapd since 'snap remove' does not guarantee there are
no processes running for the removed snap. While unloadProfiles() could
be fixed to unload the profiles, this could result in a snap process
being moved to unconfined upon 'snap remove'. Comments are added to show
how to remove a profile using sysfs but with a warning that we should
not do this until all processes are guaranteed to be killed as part of
'snap remove'.

Note: Debian Buster already has apparmor 2.13 and I'm working on uploading 2.13 to Ubuntu 19.04.

apparmor 2.13 as included in Debian/Ubuntu and their derivatives moved the system profile cache directory out of /etc/apparmor.d/cache into a unified cache directory in /var/cache/apparmor. Detect and adjust for this.

apparmor 2.13 moved away from a single flat cache directory to a forest of cache directories (among other things, this helps with boot performance when going back and forth between kernels with different feature sets). Detect the cache forest and adjust to remove cache files from the forest.

unloadProfiles() is currently called with a list of profile names and it invokes apparmor_parser --remove on those names. Unfortunately, apparmor_parser --remove requires full paths to the profiles, not just a profile name, so the profiles were not removed. While we could prepend dirs.SnapAppArmorDir to the profile names, by the time unloadProfiles() is called it is too late because the profiles on disk have already been removed. This was never caught before because apparmor_parser less than 2.13 would return 0 when given profiles that didn't exist and so the error check would never fire. The code removal preserves the actual current behavior for all versions of apparmor. The previous code was somewhat misguided for the current implementation of snapd since 'snap remove' does not guarantee there are no processes running for the removed snap. While unloadProfiles() could be fixed to unload the profiles, this could result in a snap process being moved to unconfined upon 'snap remove'. Comments are added to show how to remove a profile using sysfs but with a warning that we should not do this until all processes are guaranteed to be killed as part of 'snap remove'.

pedronis

some initial comments

pedronis · 2019-03-01T09:39:37Z

+				// - usr.lib.snapd.snap-confine
+				// - snap.core.NNNN.usr.lib.snapd.snap-confine
+				// - snap-confine.core.NNNN
+				// TODO: also the "snapd" snap here soon


we now have "snapd" snap based systems, it might well be time to implement the TODOs and not leave them as TODO. let's see what @mvo thinks

I agree with pedronis

we now have "snapd" snap based systems, it might well be time to implement the TODOs and not leave them as TODO. let's see what @mvo thinks

Please note that it is very important that this makes 2.38 since I can't update AppArmor in the archive much later than that due to distro timelines. Since this TODO was already there and unrelated to this PR, I suggest it be done in a separate PR.

zyga

First pass. I will look again with some more thoughts.

What is the urgency with apparmor 2.13? When do we have to support it? What happens if we miss the time window?

zyga · 2019-03-01T10:09:18Z

+	// With 2.13+, snap profiles are not expected to be in every
+	// subdirectory, so don't error on ENOENT but otherwise if we get an
+	// error, something weird happened so stop processing.
+	if li, err := filepath.Glob(filepath.Join(cacheDir, "*/.features")); err == nil && len(li) > 0 { // 2.13+


Hmm...

I'm not very happy about this. Is there any tool that would be compatible across apparmor versions that would reliably remove items from the cache?

@zyga - No. There is the libapparmor API (C library) but it only supports working with the current cache, not the historic caches. This is no different from previous behavior where didn't use some API and just managed the caches ourselves; it is just that there are now more cache directories (one per kernel abi). Note that I discussed this with apparmor upstream and the .features file is guaranteed to exist in each cache directory since it is required for the parser to use it. I keyed off this file instead of doing a simple glob on the filenames since I didn't want to interfere with other directories that might be in there (of which there shouldn't be any, but was being safe). FWIW, dh_apparmor is going to be modified to do the exact same thing here.

zyga · 2019-03-01T10:14:34Z

+				// - usr.lib.snapd.snap-confine
+				// - snap.core.NNNN.usr.lib.snapd.snap-confine
+				// - snap-confine.core.NNNN
+				// TODO: also the "snapd" snap here soon


I agree with pedronis

jdstrand · 2019-03-01T14:18:18Z

What is the urgency with apparmor 2.13? When do we have to support it? What happens if we miss the time window?

@zyga - the world is moving ahead. Debian has it today. SUSE is picking it up. Ubuntu needs it now to verify the features and to make the planned change to AppArmor 3 in 19.10 less disruptive.

jdstrand · 2019-03-01T15:02:03Z

@pedronis and @zyga - all comments have been addressed. This is ready for another review. Please continue to keep in mind for 2.38.

zyga · 2019-03-04T09:56:18Z


 	SystemApparmorDir = filepath.Join(rootdir, "/etc/apparmor.d")
 	SystemApparmorCacheDir = filepath.Join(rootdir, "/etc/apparmor.d/cache")
+	if !osutil.IsWritable(SystemApparmorCacheDir) {


Is this a relevant condition or is mere presence / absence sufficient. Note that snapd will usually always satisfy this condition since it runs as root and is not confined by apparmor.

Presence, not sure why I changed it to IsWritable (that isn't what Samuele even suggested...).

zyga · 2019-03-04T09:57:20Z

+// (ie, forcibly stop all running processes from the snap). Otherwise, any
+// running processes will become unconfined. Since we don't have this guarantee
+// yet, leave the profiles loaded in the kernel but remove the cache files from
+// the system so the policy is gone on the next reboot. LP: #1818241


This is fine.

Will you tackle this or shall I? I was thinking about fixing it a few times before but this time is as good as any.

I was not planning on tackling this. I'm of the opinion that we should not unload the profiles because a) it is the current behavior, b) that is what people are used to with non-snaps and c) I don't want to introduce complexity that might result in a snap command going unconfined. I understand that the snapd team is interested in removing the profiles, so I don't object to someone else doing it.

I think we can explore doing this after the lifecycle tracking work gives us the tools to do this. You can count on me trying at least :)

zyga · 2019-03-04T11:05:38Z

+	}
+	*/
+
+	// AppArmor 2.13 and higher has a cache forest while 2.12 and lower has


I was thinking about this fragment and given that I don't fully understand the 2.13 format yet I wanted to change how we handle this code. I opened a mostly-documentation RFC pull request at #6559

I was thinking about this fragment and given that I don't fully understand the 2.13 format yet I wanted to change how we handle this code.

I don't understand: you say you don't understand the format but yet you want to change the code?

I had a similar reaction and felt like this should be refactored. But given how small and isolated this is I think its ok for this PR and we can improve/iterate in a followup.

Refactoring in a followup is of course fine, but yeah, I didn't do that precisely because this only needed to be done in this one spot. I also suspect the cache forest to be in place for many years (ie, I don't foresee it changing, so less of a reason to abstract this out).

zyga · 2019-03-04T11:06:08Z

+// - var.lib.snapd.snap.core.NNNN.usr.lib.snapd.snap-confine
+// - snap-confine.core.NNNN
+// TODO: also the "snapd" snap here soon
+func profileIsRemovableOnCoreSetup(fn string) bool {


Could you add a comment that explains why a profile is or is not removable in the general sense. I understand that this is so that snapd boots but I don't know if I'm right and I suspect most people will quickly forget the rationale unless it's written down.

This part of why I didn't have it broken out into a separate function before because all this is related to the comment in Setup() below, where this is called. I'll (essentially) just dupe the comment from below.

Aha, I see now. Thank you!

mvo5

Looks good, one question and suggestion inline but no blockers, all of this can be done in follow-ups later.

mvo5 · 2019-03-04T15:02:26Z

+	fname := filepath.Join(subdir, "profile")
 	ioutil.WriteFile(fname, []byte("blob"), 0600)
 	err = apparmor.UnloadProfiles([]string{"profile"}, dirs.AppArmorCacheDir)
 	c.Assert(err, IsNil)


We could test here that the .features file is still there (i.e. that UnloadProfiles() leaves files alone that its not supposed to touch and maybe that the dir itself is still there. But given that its really just a tiny improvement doing that later in a followup is fine.

I'll do this now.

mvo5 · 2019-03-04T15:25:37Z


+// Determine if a profile filename is removable during core refresh/rollback.
+// On systems with unified cache directory, don't remove the snap profiles,
+// only system and snap-confine profiles. snap-confine profiles are like the


What is an example for a system profile here?

What is an example for a system profile here?

sbin.dhclient, usr.bin.man, etc. Ie, anything either a distro package manager installs or something the admin adds.

jdstrand · 2019-03-04T15:54:06Z

@mvo5 and @zyga - I believe all comments are addressed.

pedronis

+1

zyga · 2019-03-05T06:06:36Z

I'll do one more pass. Please postpone merging until later today.

zyga

Thank you for iterating on this. I'm +0.9 and you can give +0.1 depending on how you answer the questions I posted. Perhaps there's nothing more needed, perhaps there is. Please see inline.

zyga · 2019-03-05T10:40:44Z

+/etc/apparmor.d/cache/ r,            # apparmor 2.12 and below
 /etc/apparmor.d/cache/.features r,
 /etc/apparmor.d/{,cache/}docker* rw,
+/var/cache/apparmor/{,*/} r,         # apparmor 2.13 and higher


This is very useful. Thank you.

@zyga - to follow on our conversation on irc, it was probably this that was confusing. The docker-support interface deals with what is in the wild, not what is possible. The part of this 2nd rule that is 2.13 specific is the */ part, not the /var/cache/apparmor part. I guess I could've made the comment more clear, but I was trying to be concise (maybe too much so...).

zyga · 2019-03-05T10:55:44Z

+// - snap.core.NNNN.usr.lib.snapd.snap-confine
+// - var.lib.snapd.snap.core.NNNN.usr.lib.snapd.snap-confine
+// - snap-confine.core.NNNN
+// TODO: also the "snapd" snap here soon


About the snapd.snap, the snap, to the best of my knowledge, has no apps or hooks in the traditional sense. Is there something specific about snapd snap you were thinking about?

There will be a auto-generated profile for snap-confine when we pull it from the snapd snap, something like snap.snapd.NNNN.usr.lib.snapd.snap-confine

Aha, that's very useful insight. Thank you for the comment.

zyga · 2019-03-05T11:06:57Z

+// TODO: also the "snapd" snap here soon
+func profileIsRemovableOnCoreSetup(fn string) bool {
+	bn := path.Base(fn)
+	if strings.HasPrefix(bn, ".") {


What's the rationale behind this? This is skipping files with names starting with dot, is this about the .features file?

Yes, the features file. I noticed yesterday it was being removing. The parser will recreate it, but we shouldn't remove it.

zyga · 2019-03-05T11:07:40Z

+	bn := path.Base(fn)
+	if strings.HasPrefix(bn, ".") {
+		return false
+	} else if strings.HasPrefix(bn, "snap") && !strings.HasPrefix(bn, "snap-confine.core.") && !strings.Contains(bn, "usr.lib.snapd.snap-confine") {


Should the first test check for snap.?

I think "snap" is covering both "snap." and "snap-update-ns.", maybe it should be more explicit and use both

The idea is that we want to skip all policy for the snaps themselves. There are a bunch of ways to do this and I picked what I deemed to be the simplest to understand and was future-proofed and picked the idea of "skip everything that starts with 'snap' since snapd manages that, except for snap-confine which we want to regenerate".

There could be something called snapshot... for example that would also match though?

@pedronis - yes, it would match and the profile would not be regenerated for this profile. Considering that this only fires on core I opted for simpler to understand and didn't worry about non-snapd things that might start with 'snap*' because those would only come from the distro and the distro doesn't have anything in the core/snapd snap (and hard to imagine ever since it is so small) that would start with snap that doesn't come from snapd. I'll also mention that one of the reasons I didn't break this out in the first place and had it down in the workaround code was so that this point was more clear. I was asked to break the check into a function, which I did, but it makes it harder to understand the context it is operating under imho.

mmh, the question is a bit what is more likely:

the something with snap... get added to core

that we add some more profiles related to snaps but forget to change this bit

@pedronis - sure. The latter is the one that is more possible which then you must ask which is more likely:

the profiles that are added are something we don't want removed (ie, they are like snap.*, snap-update-ns.*)

the profiles that are added are something we want removed (ie, they are like snap-confine)

I felt the former was more likely and so coded it as such. Problem is, if you code it one way and add the other at a later date, then you did the wrong thing.

zyga · 2019-03-05T11:09:07Z

+	bn := path.Base(fn)
+	if strings.HasPrefix(bn, ".") {
+		return false
+	} else if strings.HasPrefix(bn, "snap") && !strings.HasPrefix(bn, "snap-confine.core.") && !strings.Contains(bn, "usr.lib.snapd.snap-confine") {


Should we do usr.libexec.snapd as well?

This will only run on core devices (not classic) so we should never have libexec.snapd here (even with a fedora base snapd will still come from the snapd snap with the "usr/lib" layout).

mvo5 · 2019-03-05T11:55:22Z

Merging as none of the feedback is a blocker, we may do a followup with the comments from zyga.

jdstrand · 2019-03-05T13:35:15Z

Thanks for the merge and all the reviews!

Jamie Strandboge added 4 commits February 27, 2019 22:19

apparmor: support unified cache directories

725ce46

apparmor 2.13 as included in Debian/Ubuntu and their derivatives moved the system profile cache directory out of /etc/apparmor.d/cache into a unified cache directory in /var/cache/apparmor. Detect and adjust for this.

interfaces/docker-support: add accesses for apparmor cache forest

756dfeb

jdstrand added this to the 2.38 milestone Feb 27, 2019

pedronis self-requested a review February 28, 2019 07:27

pedronis reviewed Mar 1, 2019

View reviewed changes

zyga reviewed Mar 1, 2019

View reviewed changes

Jamie Strandboge added 2 commits March 1, 2019 14:30

address review comments thanks to pedronis

95c4c31

address review comments thanks to zyga

9cebe98

zyga reviewed Mar 4, 2019

View reviewed changes

check for directory existence, not file writable (thanks zyga)

31ae830

pedronis requested a review from mvo5 March 4, 2019 14:28

mvo5 approved these changes Mar 4, 2019

View reviewed changes

Jamie Strandboge added 2 commits March 4, 2019 15:33

also check that .features exists (thanks to mvo)

a1a2f29

adjust comment for profileIsRemovableOnCoreSetup()

51cd8d3

profileIsRemovableOnCoreSetup(): dot files shouldn't be removed

96d33ef

pedronis approved these changes Mar 4, 2019

View reviewed changes

casing of function name in doc comment

750732e

zyga self-requested a review March 5, 2019 06:06

zyga reviewed Mar 5, 2019

View reviewed changes

mvo5 merged commit 4fbeeb5 into canonical:master Mar 5, 2019

jdstrand deleted the support-aa-2.13 branch April 26, 2019 15:28

Uh oh!

Conversation

jdstrand commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

zyga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jdstrand Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdstrand commented Mar 1, 2019

jdstrand commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand commented Mar 4, 2019

pedronis left a comment

Choose a reason for hiding this comment

zyga commented Mar 5, 2019

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Mar 5, 2019

jdstrand commented Mar 5, 2019

Labels

4 participants

jdstrand commented Feb 27, 2019 •

edited

Loading

jdstrand Mar 1, 2019 •

edited

Loading

jdstrand Mar 4, 2019 •

edited

Loading

jdstrand Mar 4, 2019 •

edited

Loading

jdstrand Mar 5, 2019 •

edited

Loading

jdstrand Mar 5, 2019 •

edited

Loading