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/apparmor: add apparmor support code #635
Conversation
niemeyer
reviewed
Mar 10, 2016
| + "strings" | ||
| +) | ||
| + | ||
| +// AddOrReplaceProfile loads an apparmor profile from the given file. |
zyga
Mar 10, 2016
Contributor
It started as such, then I went berserk and made it super obvious what happens on clash but since there will be one call to this function I don't think it will hurt :)
niemeyer
Mar 10, 2016
Contributor
Can we please actually call it LoadProfile? All the terminology on the package is following that. It's fine to have to look at the documentation if one wants minutia about how profiles are loaded.There's certainly much more at play than simply whether it's added or replaced.
zyga
Mar 10, 2016
Contributor
Oh sure, I did mean that the simplified rename won't hurt since there's just one place where this is used and whoever reads that can check it out from there. Fixed
niemeyer
reviewed
Mar 10, 2016
| + for { | ||
| + var name, mode string | ||
| + if _, err := fmt.Fscanf(file, "%s %s\n", &name, &mode); err != nil { | ||
| + if err.Error() == "EOF" { |
niemeyer
reviewed
Mar 10, 2016
| + defer file.Close() | ||
| + for { | ||
| + var name, mode string | ||
| + if _, err := fmt.Fscanf(file, "%s %s\n", &name, &mode); err != nil { |
niemeyer
Mar 10, 2016
Contributor
Rather than nesting, this tends to be more readable:
_, err := fmt.Fscanf(...)
if err == io.EOF {
break
}
if err != nil {
return nil, err
}
niemeyer
reviewed
Mar 10, 2016
| + } | ||
| + // Eliminate '(' ')' around the mode. | ||
| + mode = strings.TrimPrefix(mode, "(") | ||
| + mode = strings.TrimSuffix(mode, ")") |
niemeyer
Mar 10, 2016
Contributor
mode = strings.Trim(mode, "()")
I'd also drop the comment as it's redundant with the code.
niemeyer
reviewed
Mar 10, 2016
| +// | ||
| +// Snappy manages apparmor profiles named *.snap. Other profiles might exist on | ||
| +// the system (via snappy dimension) and those are filtered-out. | ||
| +func LoadedProfiles() (profiles []Profile, err error) { |
niemeyer
Mar 10, 2016
Contributor
A nice convention is to not name the result variables unless it's either important for the logic in the function, or clarifying what the results are (e.g. common when returning bare strings or ints, as we can't tell upfront what these are). In the case above ([]Profile, error) is super clear.
|
Several Go-related trivial nitpicks, but nothing critical. LGTM |
zyga
added some commits
Mar 10, 2016
jdstrand
reviewed
Mar 10, 2016
| +// If there was a profile with the same name before, that profile is replaced. | ||
| +func LoadProfile(fname string) error { | ||
| + return exec.Command("apparmor_parser", "--replace", fname).Run() | ||
| +} |
jdstrand
Mar 10, 2016
Contributor
While this works, it is not updating the apparmor cache file and isn't tuned for armhf (LP: #1383858). This is the full command you want:
apparmor_parser --replace --write-cache -O no-expr-simplify --cache-loc=/var/cache/apparmor
You might want to also consider whether you want --skip-read-cache. The above will avoid loading the profile if its mtime is newer than the profile. In general, I think you should avoid second guessing the apparmor caching operations and use its facilities, but thought it at least worth mentioning.
zyga
Mar 10, 2016
Contributor
As for http://pad.lv/1383858 -- what do I need to do on snappy side to avoid slowness?
EDIT: I'm blind, I see -O no-expr-simplify now.
jdstrand
reviewed
Mar 10, 2016
| + Name string | ||
| + // Mode is either "enforcing" or "complaining". | ||
| + Mode string | ||
| +} |
jdstrand
Mar 10, 2016
Contributor
AppArmor upstream terminology would use 'enforce' mode and 'complain' mode. This isn't a big deal but it would be nice to be consistent with upstream terminology. In addition, 'enforce' and 'complain' are what the kernel exposes in /sys/kernel/security/apparmor/profiles.
jdstrand
reviewed
Mar 10, 2016
| +// The operation is done with: apparmor_parser --remove $name | ||
| +func (profile *Profile) Unload() error { | ||
| + return exec.Command("apparmor_parser", "--remove", profile.Name).Run() | ||
| +} |
jdstrand
Mar 10, 2016
Contributor
FYI, this unloads the profile from the kernel, but doesn't remove the cache file in /var/cache/apparmor which means that on reboot what is in /var/cache/apparmor will be loaded. I think it is probably correct to not remove the cache file in Unload(), but I didn't see a corresponding Remove() function that would remove both /var/lib/snappy/apparmor/profiles/ and /var/cache/apparmor/.
zyga
Mar 10, 2016
Contributor
I was planning to call Unload() in the interface manager's Ensure() method that would iterate over all snappy-originated profiles and remove those that have no corresponding snap anymore (in other words, after removing a snap).
File handling is something I'm still working on, I'll most likely two sets of information: all the directories that we manage and all the files we want to have in those directories and, again, in Ensure() I'll remove any unexpected files (and rewrite any files that have unexpected content).
jdstrand
reviewed
Mar 10, 2016
| + | ||
| +// LoadedProfiles interrogates the kernel and returns a list of loaded apparmor profiles. | ||
| +// | ||
| +// Snappy manages apparmor profiles named *.snap. Other profiles might exist on |
jdstrand
Mar 10, 2016
Contributor
This isn't yet true based on profileAttachForApp() in https://github.com/ubuntu-core/snappy/pull/617/files#diff-f2feb1d579afc2fb12782291cb5ece77R98. You aren't appending .snap to the profile name. Changing this has ripple effects because what is in /sys/kernel/security/apparmor/profiles ends up as the security label on processes running under this profile which means security policy has to be updated to account for this, an extra 5 chars of kernel memory is being used for every loaded profile and there are implications for Ubuntu Personal. The policy updates are not insignificant-- in addition to adjusting the variables in interfaces/security.go and the policy in interfaces/apparmor.go, you're going to have to keep track of when to append '.snap' to all rules where we do label matching (ie, dbus, ptrace, signal, unix and in the future, network). The launcher will also need a corresponding update.
I'm not saying it isn't doable, but adding .snap has ripple effects. I suggest not appending .snap to the name and rethink why this is needed and see if there is another way to do this.
zyga
Mar 10, 2016
Contributor
This is done so that we have a clear way to identify profiles managed by snappy. We discussed this with Gustavo last week / this week as well.
I'm aware that nothing currently creates *.snap profiles but it is my desire to switch over interfaces/security.go to do so tomorrow.
I'd like to understand required changes better, I know that generated wrappers will have to change (so that we call it with $snap[.$app].snap as the policy name) but I'm not sure what else needs changing.
jdstrand
Mar 10, 2016
Contributor
On Thu, 2016-03-10 at 12:27 -0800, Zygmunt Krynicki wrote:
+// Unload removes a profile from the running kernel.
+//
+// The operation is done with: apparmor_parser --remove $name
+func (profile *Profile) Unload() error {
- return exec.Command("apparmor_parser", "--remove",
profile.Name).Run()
+}
+// profilesPath contains information about the currently loaded
apparmor profiles.
+const realProfilesPath = "/sys/kernel/security/apparmor/profiles"
+
+var profilesPath = realProfilesPath
+
+// LoadedProfiles interrogates the kernel and returns a list of
loaded apparmor profiles.
+//
+// Snappy manages apparmor profiles named *.snap. Other profiles
might exist on
This is done so that we have a clear way to identify profiles managed
by snappy. We discussed this with Gustavo last week / this week as
well.I'm not sure this is the best way to do this. The kernel only has what
is loaded. The authoritative list of what snappy maintains is what is
in /var/lib/snappy/apparmor/profiles.
For example, if you remove a profile from the kernel, snappy is still
managing the profile even though it wouldn't show up
in /sys/kernel/security/apparmor/profiles. Plus, as is obvious from
what .snap is trying to handle, there are many other profiles you have
to filter out and this is buggy because a non-snappy managed profile
may end in .snap, especially on a system where the user has traditional
admin control, such as snappy dimension on Ubuntu classic. Ie, what
will happen if a user creates the profile 'oh.snap' on her system for
some home-grown software?
I'm aware that nothing currently creates *.snap profiles but it is my
desire to switch overinterfaces/security.goto do so tomorrow.I'd like to understand required changes better, I know that generated
wrappers will have to change (so that we call it with
$snap[.$app].snapas the policy name) but I'm not sure what else
needs changing.
- The launcher is expecting a profile name. Adding '.snap' to the
profile name means we can either change the launcher to append it or we
can change the /snaps/bin generation and the systemd unit generation. I
prefer the latter. - profileAttachForApp() needs to change to append .snap
- varsForApp() needs to change so that encoded '.snap' (_2esnap)
appears in APP_ID_DBUS
I prefer you abstract this out into a 'profileNameForApp()' (or
similar) that the above 3 points can use rather than sprinkling .snap
in various places.
I audited all existing snappy and Touch policy. In general, the snap
policy today should be ok because our policy doesn't care about the
version part of the profile name (which .snap then becomes part of) or
we use the apparmor var @{profile_name}. Touch has some rules that
would be affected by this change but not many. Touch trusted helpers I
think typically don't care about the version part of the profile name,
so they should be ok.
Moving forward, we might have to consider '.snap' as part of the label
for new policy when writing rules for new interfaces (eg, that use
dbus, unix, signal and ptrace and in the future, network).
That should be it but I maintain that adding .snap adds complexity
where I think it shouldn't necessarily be and the implementation is
prone to bugs.
Jamie Strandboge | http://www.canonical.com
zyga
Mar 10, 2016
Contributor
- The launcher is expecting a profile name. Adding '.snap' to the
profile name means we can either change the launcher to append it or we
can change the /snaps/bin generation and the systemd unit generation. I
prefer the latter.
ZK: I'll patch the launcher generator to use .snap suffix.
- profileAttachForApp() needs to change to append .snap
ZK: yep, I'll write one, authoritative function that computes a profile name for snap + app and use it everywhere.
- varsForApp() needs to change so that encoded '.snap' (_2esnap)
appears in APP_ID_DBUS
ZK: Noted, I'll make that happen in the other branch ( #617 )
I prefer you abstract this out into a 'profileNameForApp()' (or
similar) that the above 3 points can use rather than sprinkling .snap
in various places.
ZK: Yeah, I already planned to do this while fixing hw-assign. I'll make that happen separately so that we can hunt-kill all remaining duplications of the code that computes this.
Moving forward, we might have to consider '.snap' as part of the label
for new policy when writing rules for new interfaces (eg, that use
dbus, unix, signal and ptrace and in the future, network).
ZK: I'm not sure I understand this completely. Yes, .snap suffix will show up in udev rules (because those are raw snippets) but apparmor and seccomp don't really care about the name (as far as the content of the actual profiles is concerned). Please correct me, I'm sure I'm missing something.
That should be it but I maintain that adding .snap adds complexity
where I think it shouldn't necessarily be and the implementation is
prone to bugs.
ZK: The motivation to add .snap is to ensure that we can implement Ensure() reliably. Ensure is supposed to inspect the state of the system, compute a desired state of the system and fix any issues. One aspects of doing that is to ensure we get rid of unused in-kernel apparmor profiles. Since the snap manager can remove a snap without telling us we need to notice that there are files and profiles in the air that don't have a corresponding snap and act on them accordingly. I agree that there's a potential clash with 3rd party environments that use .snap suffix somewhere but I think this is not unreasonable as an assumption to take, that we kind-of manage that pseudo naming space (not a kernel namespace). If you know of another method that we could use to do this then I'd love to know what that is.
jdstrand
Mar 11, 2016
Contributor
On Thu, 2016-03-10 at 15:01 -0800, Zygmunt Krynicki wrote:
- The launcher is expecting a profile name. Adding '.snap' to the
profile name means we can either change the launcher to append it or
we
can change the /snaps/bin generation and the systemd unit generation.
I
prefer the latter.ZK: I'll patch the launcher generator to use .snap suffix.
Thanks, I greatly preferred this-- with 'profile .snap {}' as the
apparmor profile, '.snap' is the profile name so I think using
the same profile everywhere like this makes sense.
...
Moving forward, we might have to consider '.snap' as part of the
label
for new policy when writing rules for new interfaces (eg, that use
dbus, unix, signal and ptrace and in the future, network).ZK: I'm not sure I understand this completely. Yes, .snap suffix will
show up in udev rules (because those are raw snippets) but apparmor
and seccomp don't really care about the name (as far as the content
of the actual profiles is concerned). Please correct me, I'm sure I'm
missing something.
As mentioned, the .snap suffix is part of the profile name now
because it is what you specify as 'profile .snap'. Because it
is in the profile name and the profile name composes the in kernel
security label, certain apparmor rules can be written to use the label.
These rules are signal, ptrace, unix, dbus and when fine-grained
network mediation hits, network. See 'man apaprmor.d' for details.
Whether or not we use the full security label or a part of it is up to
the rule writer and what we want in any given situation. I was saying
that when it is warranted (as it is in a couple of places in Touch
policy), we'll have to always remember to add '.snap'.
That should be it but I maintain that adding .snap adds complexity
where I think it shouldn't necessarily be and the implementation is
prone to bugs.ZK: The motivation to add .snap is to ensure that we can implement
Ensure() reliably. Ensure is supposed to inspect the state of the
system, compute a desired state of the system and fix any issues.
But as mentioned the chosen implementation has a flaw with 'oh.snap'.
Snappy will think it is managing oh.snap when it isn't.
One aspects of doing that is to ensure we get rid of unused in-
kernel apparmor profiles. Since the snap manager can remove a snap
without telling us we need to notice that there are files and
profiles in the air that don't have a corresponding snap and act on
them accordingly.
Why would the snap manager not use the rest api (and therefore the
snappy api and all this code under the hood)? Ie, how can a snap
manager ever remove snaps without snappy knowing it? Surely we aren't
going to support a snap manager unmount and rm -f'ing things on its
own?
I agree that there's a potential clash with 3rd party environments
that use .snap suffix somewhere but I think this is not unreasonable
as an assumption to take, that we kind-of manage that pseudo naming
space (not a kernel namespace). If you know of another method that we
could use to do this then I'd love to know what that is.Use the filesystem in /var/lib/snappy/apparmor/profiles as your
database, not the kernel. You already are in many senses since on
reboot it is the filesystem that will have the profiles you load into
the kernel. Even if you are of the opinion that bootstrapping is
separate from that, whatever you bootstrap from is your actual database
and not the kernel. You can Ensure() that profiles are loaded in the
kernel based on the profiles that exist in the filesystem. You can
Ensure() that a profile is unloaded from the kernel if the file exists
but the profile is not loaded.
Jamie Strandboge | http://www.canonical.com
zyga
Mar 11, 2016
Contributor
jdstrand: But as mentioned the chosen implementation has a flaw with 'oh.snap'.
Snappy will think it is managing oh.snap when it isn't.
ZK: We need a compromise. There are no "namespaces" for apparmor profiles. We're broadly allocating *.snap in order to make one. Unless there are existing "oh.snap" profiles in the wild I think this is exactly what we should do, in the absence of better kernel resource management, to send a strong signal that snappy is using apparmor in a certain way and that you should not name your profiles *.snap
jdstrand
Mar 11, 2016
Contributor
On Fri, 2016-03-11 at 00:43 -0800, Zygmunt Krynicki wrote:
+// Unload removes a profile from the running kernel.
+//
+// The operation is done with: apparmor_parser --remove $name
+func (profile *Profile) Unload() error {
- return exec.Command("apparmor_parser", "--remove",
profile.Name).Run()
+}
+// profilesPath contains information about the currently loaded
apparmor profiles.
+const realProfilesPath = "/sys/kernel/security/apparmor/profiles"
+
+var profilesPath = realProfilesPath
+
+// LoadedProfiles interrogates the kernel and returns a list of
loaded apparmor profiles.
+//
+// Snappy manages apparmor profiles named .snap. Other profiles
might exist on
Why would the snap manager not use the rest api (and therefore the
snappy api and all this code under the hood)? Ie, how can a snap
manager *ever remove snaps without snappy knowing it? Surely we
aren't
going to support a snap manager unmount and rm -f'ing things on its
own?ZK: The snap manager is part of the state engine. It lives in snapd
itself (same process). Our design has a simple, fixed pipeline, with
snap manager doing some stuff and then interface manager reacting as
appropriate to change the security side of things.
Ok, good, that is what I figured and hoped, but I'm confused by this
statement then: "Since the snap manager can remove a snap without
telling us we need to notice that there are files and profiles in the
air that don't have a corresponding snap and act on them accordingly"
Jamie Strandboge | http://www.canonical.com
jdstrand
Mar 11, 2016
Contributor
On Fri, 2016-03-11 at 00:45 -0800, Zygmunt Krynicki wrote:
+// Unload removes a profile from the running kernel.
+//
+// The operation is done with: apparmor_parser --remove $name
+func (profile *Profile) Unload() error {
- return exec.Command("apparmor_parser", "--remove",
profile.Name).Run()
+}
+// profilesPath contains information about the currently loaded
apparmor profiles.
+const realProfilesPath = "/sys/kernel/security/apparmor/profiles"
+
+var profilesPath = realProfilesPath
+
+// LoadedProfiles interrogates the kernel and returns a list of
loaded apparmor profiles.
+//
+// Snappy manages apparmor profiles named *.snap. Other profiles
might exist on
Use the filesystem in /var/lib/snappy/apparmor/profiles as your
database, not the kernel. You already are in many senses since on
reboot it is the filesystem that will have the profiles you load into
the kernel. Even if you are of the opinion that bootstrapping is
separate from that, whatever you bootstrap from is your actual
database
and not the kernel. You can Ensure() that profiles are loaded in the
kernel based on the profiles that exist in the filesystem. You can
Ensure() that a profile is unloaded from the kernel if the file
exists
but the profile is not loaded.ZK: This makes no sense. Yes we manage the /var/lib/snappy/apparmor/*
space but I don't see how this is related. My concern is solely about
profiles loaded into the kernel.When the snap manager changes a snap (e.g. updates it to a newer
version that has different apps inside) we must generate new profiles
(which is easy) and discard old profiles. Since profiles are a
runtime property of the kernel we need a way to discover what
profiles exist, and, to the best of our knowledge, which profiles to
remove/var/lib/snappy/apparmor/* is directly related because those (and
/var/cache/apparmor/) are what are loaded in the kernel to add to the
list that is in /sys/kernel/security/apparmor/profiles. Put another
way: you can always compare what is in the filesystem to what is loaded
in the kernel. Ie, ls /var/lib/snappy/apparmor/ to get the full list
of profiles, then look in /sys/kernel/security/apparmor/profiles and
see what is loaded and unloaded and makes decisions from there. It is
very easy to then see what exists in the kernel that you are
responsible for and which profiles are not in the kernel (that perhaps
should be). The current state then is-- see what in
/var/lib/snappy/apparmor/* is loaded in the kernel.
Jamie Strandboge | http://www.canonical.com
jdstrand
Mar 11, 2016
Contributor
On Fri, 2016-03-11 at 00:47 -0800, Zygmunt Krynicki wrote:
+// Unload removes a profile from the running kernel.
+//
+// The operation is done with: apparmor_parser --remove $name
+func (profile *Profile) Unload() error {
- return exec.Command("apparmor_parser", "--remove",
profile.Name).Run()
+}
+// profilesPath contains information about the currently loaded
apparmor profiles.
+const realProfilesPath = "/sys/kernel/security/apparmor/profiles"
+
+var profilesPath = realProfilesPath
+
+// LoadedProfiles interrogates the kernel and returns a list of
loaded apparmor profiles.
+//
+// Snappy manages apparmor profiles named *.snap. Other profiles
might exist on
But as mentioned the chosen implementation has a flaw with 'oh.snap'.
Snappy will think it is managing oh.snap when it isn't.ZK: We need a compromise. There are no "namespaces" for apparmor
profiles. We're broadly allocating*.snapin order to make one.
Unless there are existing "oh.snap" profiles in the wild I think this
is exactly what we should do, in the absence of better kernel
resource management, to send a strong signal that snappy is using
apparmor in a certain way and that you should not name your profiles
*.snapI discussed this with the security team and none of us like the
approach of appending ".snap". I think the compromise is looking at the
filesystem in the manner I described previously instead of using a
string to attempt namespacing.
AppArmor will have the concept of policy namespaces in support of LXD
for 16.04. The feature hasn't landed yet and is coming in hot. I
discussed this feature at length with the AppArmor team and I don't
advise using it for 16.04 because there are many security policy
implications to using them (ie, it is far more than just creating a
namespace and loading into it) and we are going to have to properly
design their use in snappy.
Jamie Strandboge | http://www.canonical.com
jdstrand
Mar 16, 2016
Contributor
We discussed this in a meeting today and came to the conclusion that while the security doesn't like using names as a poor-man's policy namespace, what we are doing today is actually a form of namespacing with the APP_ID tuplet and therefore adjusting the profile name is not appreciably different from what we are doing now. Policy namespaces are available, but this would require quite a bit of work to properly design in snappy and get right (there is a card for this).
Conclusion: rather than rushing to implement something that may require potentially lots more development work (bugs, snapd, SDoC, etc), use snap.<snap>.<app> for 16.04 and then investigate/understand the use of policy namespaces after 16.04.
jdstrand
reviewed
Mar 10, 2016
| + return nil, err | ||
| + } | ||
| + mode = strings.Trim(mode, "()") | ||
| + if strings.HasSuffix(name, ".snap") { |
zyga
added some commits
Mar 10, 2016
|
Code looks good but I think we need to hold back until Jamie is happy. |
|
Just coming here to third the “make jamie happiest” thing :-) |
mvo5
added
the
Reviewed
label
Mar 14, 2016
zyga
added some commits
Mar 14, 2016
jdstrand
reviewed
Mar 22, 2016
| +// LoadProfile loads an apparmor profile from the given file. | ||
| +// | ||
| +// If no such profile was previously loaded then it is simply added to the kernel. | ||
| +// If there was a profile with the same name before, that profile is replaced. |
jdstrand
Mar 22, 2016
Contributor
This is fine. A comment saying why we are using no-expr-simplify would be good. Eg:
// Use no-expr-simplify since expr-simplify is actually slower on armhf (LP: #1383858)
|
Please see my comment about adding a comment. Otherwise, we discussed this at length in a hangout and using 'snap.' is ok. +1 for full PR. |
zyga commentedMar 10, 2016
This is stacked on #634
This branch adds several utility functions for working with AppArmor.
They allow loading apparmor profiles into the kernel, listing loaded
profiles (those managed by snappy) and removing loaded profiles that are
no longer needed.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com