Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snap: use snap-confine from the core snap #2810
Conversation
mvo5
added some commits
Feb 7, 2017
All checks have failed
jdstrand
reviewed
Feb 8, 2017
When not OnClassic, there is no problem, right? You must reboot if the os snap is refreshed and that gives you everything. Prior to reboot, all the old stuff is still used, so no problem.
| + | ||
| + cmd := []string{} | ||
| + if release.OnClassic && osutil.FileExists(snapConfinePathInCore) { | ||
| + cmd = append(cmd, snapConfinePathInCore) |
jdstrand
Feb 8, 2017
Contributor
Unconditionally using snapConfinePathInCore seems problematic. Do we unconditionally use everything else from core there? What about reverts?
| +// profile for snap-confine when we are in re-exec mode | ||
| +func (m *SnapManager) ensureSnapConfineApparmor() error { | ||
| + if !release.OnClassic { | ||
| + return nil |
jdstrand
Feb 8, 2017
Contributor
Adding a comment above !release.OnClassic I think is wise. Eg:
// On all-snaps we always use the mounted snapd, snap-confine, apparmor, etc
// but on classic we use the snapd and snap-confine from the latest core snap.
// As such, nothing to do when not OnClassic
| + return nil | ||
| + } | ||
| + | ||
| + // FIXME2: cleanup old entries here |
jdstrand
Feb 8, 2017
•
Edited 1 time
-
jdstrand
Feb 8, 2017
Contributor
-
Feb 8, 2017
You could cleanup only old profiles where the corresponding core snap is no longer on the system. Leaving the old ones might help the revert scenario.
On the other hand, the code would likely be simpler if instead only ever have the current core snap profile and remove all the others (that happens to mirror what we do with /var/lib/snapd/apparmor/profiles where we only have the profile for the current revision).
| + // FIXME2: cleanup old entries here | ||
| + | ||
| + // FIXME: what to do for debian? debian has a less powerful apparmor | ||
| + // so we can not use the ubuntu profile? |
jdstrand
Feb 8, 2017
Contributor
You can use the Ubuntu profile, you just need to add two rules:
# Required when using unpatched upstream kernel
capability sys_ptrace,
# Debian compiles snap-confine without AppArmor, so allow running
# snaps unconfined
/usr/lib/snapd/snap-exec uxr,
mvo5
Feb 8, 2017
Collaborator
Hm, that ties in with the suggestion from @zyga to have something more clever here than the dumb strings.Replace()
| + | ||
| + if err := ioutil.WriteFile(apparmorProfilePath, []byte(apparmorProfileForCore), 0644); err != nil { | ||
| + return err | ||
| + } |
jdstrand
Feb 8, 2017
Contributor
Please add a comment:
// /etc/apparmor.d is read/write OnClassic, so write out the new core's profile there
| + | ||
| + if output, err := exec.Command("apparmor_parser", "--replace", "--write-cache", apparmorProfilePath).CombinedOutput(); err != nil { | ||
| + return osutil.OutputErr(output, err) | ||
| + } |
jdstrand
Feb 8, 2017
Contributor
This may break with alternate cores that don't have apparmor enabled. I forget OTOH how the parser handles that-- you probably want to double check that.
mvo5
Feb 8, 2017
Collaborator
Hm that is a good point! Should we use if release.ReleaseInfo.ForceDevMode() == true {retur nil} on top of this function?
zyga
reviewed
Feb 8, 2017
Some comments, I think the direction is good but there's a lot of things that we need to fix before this can land.
| @@ -175,9 +177,17 @@ func runSnapConfine(info *snap.Info, securityTag, snapApp, command, hook string, | ||
| logger.Noticef("WARNING: cannot create user data directory: %s", err) | ||
| } | ||
| - cmd := []string{ | ||
| - filepath.Join(dirs.LibExecDir, "snap-confine"), | ||
| + // use snap-confine from core if it is available |
zyga
Feb 8, 2017
Contributor
We also need to use the dynamic linker from core as snap-confine dynamically links to libraries that are not available on the host (e.g. xfs, apparmor).
| @@ -551,6 +556,45 @@ func (m *SnapManager) ensureUbuntuCoreTransition() error { | ||
| return nil | ||
| } | ||
| +// ensureSnapConfineApparmor ensures that we have a valid apparmor | ||
| +// profile for snap-confine when we are in re-exec mode | ||
| +func (m *SnapManager) ensureSnapConfineApparmor() error { |
zyga
Feb 8, 2017
Contributor
Question: what will keep us from keeping unbound number of old profiles around?
Perhaps we could generate the profile in /run and load it from there (the actual path is irrelevant as long as it is loaded into the kernel). This doesn't prevent unbound in-memory profiles but at least a "reboot" will fix that.
mvo5
Feb 8, 2017
Collaborator
This is fixed now, as @jdstrand suggested the code will only keep the "current" profile around and loaded and will discard older profiles.
| + | ||
| + // FIXME: what to do for debian? debian has a less powerful apparmor | ||
| + // so we can not use the ubuntu profile? | ||
| + apparmorProfile, err := ioutil.ReadFile(filepath.Join(root, "/etc/apparmor.d/usr.lib.snapd.snap-confine")) |
zyga
Feb 8, 2017
Contributor
As mentioned on telegram earlier, I think that we should ship the snap-confine.apparmor.in file and run sed on it to obtain the correct profile. We could also extend this mechanism to feed in os-release (or even apparmor version available at runtime) to generate a profile that will work in a particular environment. Using the .in file is easier for this than the resulting file from the package.
mvo5
Feb 8, 2017
Collaborator
I would prefer to do that in a follow-up branch, mostly because AIUI we currently ship a working /etc/apparmor.d/usr.lib.snapd.snap-confine only on Ubuntu. We also ship it on Debian but AIUI its broken there. And the other distros do not use it (yet). So it seems ideal to do a followup each time we add this file for a new distro (unless my premise is wrong of course).
| + return err | ||
| + } | ||
| + | ||
| + if output, err := exec.Command("apparmor_parser", "--replace", "--write-cache", apparmorProfilePath).CombinedOutput(); err != nil { |
zyga
Feb 8, 2017
Contributor
I think you should switch to interfaces.apparmor here. There are some extra option that we pass, most notably -O no-expr-simplify.
mvo5
Feb 8, 2017
Collaborator
The difference is the cache dir that is used. For the system profiles its /etc/apparmor.d/cache and for the snapd profiles its /var/cache/apparmor/. The /etc/init.d/apparmor init script uses the different cachedir.
mvo5
Feb 8, 2017
Collaborator
Having said that, of course we can refactor to something like interfaces.apparmor.AddProfileWithCachedir() or somesuch.
jdstrand
Feb 8, 2017
Contributor
Those options are less interesting for a single profile like snap-confine where all this happens as part of the snap refresh. -O no-expr-simplify is important on ARM with a lot of profiles that use our template, etc, but the gain will not be significant for this one profile. IMHO, this can be fixed up in a future commit if it keeps this PR simple.
| @@ -560,6 +604,12 @@ func (m *SnapManager) Ensure() error { | ||
| m.runner.Ensure() | ||
| + // needs to run after the runner |
zyga
Feb 8, 2017
Contributor
Keep in mind that snap-confine can be invoked before snapd boots. Could we perhaps trigger this from snap-run (on demand)?
We could do the same for all the "stale" objects:
- security profiles for snaps
- launchers (well, only if they run snap-run today)
- snap-confine's own profile
If we detect that something is not up-to-date we reach out to snapd and call some API like ensure-ephemeral-state. Just an idea but I think we could use it to avoid costly stuff from happening on boot.
mvo5
Feb 8, 2017
Collaborator
This should be fine (unless I miss something). The generated apparmor profile is loaded during early boot automatically via /etc/init.d/apparmor. This ensures its available early on and its not more costly than any other of the profiles in that dir.
jdstrand
Feb 8, 2017
Contributor
I agree with @mvo5 that this shouldn't be needed due to how apparmor is setup during boot. @zyga, are we missing something?
|
Also as a potential consideration that does not have to be fixed for this PR, /etc/apparmor.d/abstractions on the classic host and in the core snap could be different. We've been lucky so far that they haven't been different, but it is easy to imagine scenarios where they might be with different core snaps or we build the series 16 core with a different apparmor than is in xenial-updates. |
mvo5
added some commits
Feb 8, 2017
All checks have failed
All checks have failed
Some checks were not successful
All checks have passed
| + | ||
| + echo "Copy the suid root snap-confine over so that it runs outside " | ||
| + echo "of the auto-genreated profile" | ||
| + cp -a /snap/${core_name}/current/usr/lib/snapd/snap-confine . |
Some checks were not successful
zyga
approved these changes
Feb 13, 2017
LGTM and iterate on what needs to be done. We have more trouble without this than with the imperfections this introduces.
mvo5
added this to the
2.23 milestone
Feb 14, 2017
jdstrand
referenced this pull request
Feb 14, 2017
Merged
interfaces: misc updates for network-control, firewall-control, unity7 and default policy #2842
| + // not using apparmor.UnloadProfile() because it uses a | ||
| + // different cachedir | ||
| + if output, err := exec.Command("apparmor_parser", "-R", filepath.Base(path)).CombinedOutput(); err != nil { | ||
| + logger.Noticef("cannot remove apparmor %s: %q", path, output) |
niemeyer
Feb 14, 2017
Contributor
"cannot unload apparmor profile %s: %v", filepath.Base(path), osutil.OutputErr(output, err)
| + // distros with apparmor around snap-confine, i.e. | ||
| + // ship `snap-confine.apparmor.in` and use that as the base | ||
| + // because core is ubuntu and classic host might be anything | ||
| + apparmorProfile, err := ioutil.ReadFile(filepath.Join(root, "/etc/apparmor.d/usr.lib.snapd.snap-confine")) |
niemeyer
Feb 14, 2017
Contributor
This is a location in classic, which means it depends on the packaging, right? At the top of this PR we have a snapConfinePath variable being used which assumes this location is not hardcoded.
Same concern below and perhaps elsewhere. Need to watch out for the difference between a path in core, which we're sure about, and in the outer system, which we're not.
mvo5
Feb 15, 2017
Collaborator
The apparmor profile we read as input is a location in the core snap, the "root" variable name is probably misleading here, I changed it to coreRoot to make this clearer.
| + | ||
| + // /etc/apparmor.d is read/write OnClassic, so write out the | ||
| + // new core's profile there | ||
| + if err := ioutil.WriteFile(apparmorProfilePath, []byte(apparmorProfileForCore), 0644); err != nil { |
| + | ||
| + // not using apparmor.LoadProfile() because it uses a different cachedir | ||
| + if output, err := exec.Command("apparmor_parser", "--replace", "--write-cache", apparmorProfilePath, "--cache-loc", dirs.SystemApparmorCacheDir).CombinedOutput(); err != nil { | ||
| + return osutil.OutputErr(output, err) |
niemeyer
Feb 14, 2017
Contributor
return fmt.Errorf("cannot replace snap-confine apparmor profile: %v", osutil.OutputErr(output, err))
| + | ||
| + root := filepath.Join(dirs.SnapMountDir, "/core/current/") | ||
| + snapConfineInCore, err := filepath.EvalSymlinks(filepath.Join(root, "/usr/lib/snapd/snap-confine")) | ||
| + // in flight |
niemeyer
Feb 14, 2017
Contributor
Or maybe still on ubuntu-core? Or updating? Maybe there's no core at all at this point? Comment seems to not help much. If we know exactly why this should happen here, then we need to be clear why for our own benefit later. Otherwise, I suggest just dropping it so we're not creating a false impression.
| @@ -560,6 +660,12 @@ func (m *SnapManager) Ensure() error { | ||
| m.runner.Ensure() | ||
| + // needs to run after the runner |
niemeyer
Feb 14, 2017
Contributor
Why it needs to run after the runner? Otherwise it's a riddle for the next reader.
mvo5
Feb 15, 2017
Collaborator
Sorry for this! Comment updated and it is hopefully more descriptive now :)
| @@ -560,6 +660,12 @@ func (m *SnapManager) Ensure() error { | ||
| m.runner.Ensure() | ||
| + // needs to run after the runner | ||
| + if err := m.ensureSnapConfineApparmor(); err != nil { | ||
| + // FIMXE: this will spam the world as ensure runs often |
niemeyer
Feb 14, 2017
Contributor
Why are we doing that non-stop? I mean reading/writing/updating the profile, rather than just logging. That sounds unnecessary and somewhat unfortunate to be running so frequently. Shouldn't we be doing that only when updating core, which is the moment when we update snap-confine itself and the apparmor profile that comes with it?
mvo5
Feb 15, 2017
Collaborator
We have some options here, I'm happy to rework it. The implementation is done like this because:
a) on startup we need to check if we have an apparmor profile for snap-confine. we may upgrade/re-exec from an older version of snapd so we need to check this, the ensure loop seems to be a good place for this
b) if we need to have this code anyway it can be reused for the case when the core updates. please note that it will not actually re-generate profiles all the time. if the profile exists, it will just do nothing.
We can of course change that to:
a) check once on startup if the profile is there
b) write the snap-confine profile in e.g. the setup-security task of core (as a special case there).
Happy to rework if you think the later design is cleaner (or anyother design that is cleaner!).
| + // needs to run after the runner | ||
| + if err := m.ensureSnapConfineApparmor(); err != nil { | ||
| + // FIMXE: this will spam the world as ensure runs often | ||
| + logger.Noticef("ensureSnapConfineApparmor failed with: %s", err) |
| + // apparmor, etc but on classic we use the snapd and | ||
| + // snap-confine from the latest core snap. As such, nothing | ||
| + // to do when not OnClassic | ||
| + if !release.OnClassic { |
niemeyer
Feb 14, 2017
Contributor
One more detail: where are we deciding to not do this logic at all because the distribution at hand doesn't support apparmor?
mvo5
Feb 15, 2017
Collaborator
We don't have that right now, I fixed it and use if release.ReleaseInfo.ForceDevMode() { return nil } now.
mvo5
added some commits
Feb 14, 2017
Some checks were not successful
All checks have passed
Some checks were not successful
| + } | ||
| + // On releases that do not support apparmor there is no need to | ||
| + // write an apparmor profile for snap-confine | ||
| + if release.ReleaseInfo.ForceDevMode() { |
zyga
Feb 15, 2017
Contributor
This is not super-strictly-speaking true. Can you please add a comment like this:
// TODO: switch to runtime-detected flag that tells us if apparmor is enabled.
// Write a profile for snap-confine unconditionally but only attempt to load it
// if apparmor is enabled.
mvo5
modified the milestones:
2.24,
2.23
Feb 16, 2017
mvo5
referenced this pull request
Feb 16, 2017
Merged
interfaces/default: allow mknod for regular files, pipes and sockets (LP: #1636540) #2749
pedronis
requested changes
Feb 17, 2017
•
yes, where/when and how to run apparmor profile regen is a bit unclear to me atm
| @@ -560,6 +666,13 @@ func (m *SnapManager) Ensure() error { | ||
| m.runner.Ensure() | ||
| + // run after the runner so if we get a new core snap, we immediately | ||
| + // generate the matching apparmor profile for snap-confine from core |
pedronis
Feb 17, 2017
Contributor
indeed I don't understand this comment, also I don't see any kind of locks ? it feels also strange to do all this in snapmgr and not ifacemgr
Some checks were not successful
|
After discussing the security profile generation for the snap.core.$revno.usr.lib.snapd.snap-confine part, there will be some changes needed:
|
|
Closing for now as this needs some rework (see last comment) |
mvo5
closed this
Feb 23, 2017
| - if su test -c "sh -c \"SNAP_NAME=test-snapd-tools /snap/${core_name}/current/usr/lib/snapd/snap-confine snap.test-snapd-tools.cmd /bin/true 2>/dev/null\""; then | ||
| + | ||
| + echo "Copy the suid root snap-confine over so that it runs outside " | ||
| + echo "of the auto-genreated profile" |
|
@mvo5 - ping re #2810 (comment) |
mvo5 commentedFeb 8, 2017
•
Edited 1 time
-
mvo5
Feb 8, 2017
Not quite ready yet, tests and spread tests are missing but this allows early comments.
Missing: