cmd/snap-confine,packaging: import snapd-generated policy #3807

Merged
merged 5 commits into from Sep 20, 2017

Conversation

Projects
None yet
6 participants
Contributor

zyga commented Aug 25, 2017

This patch allows snap-confine's apparmor profile to be extended with
additional site policy that is generated by snapd. The purpose of the
site policy is so that it can be used to generate some workarounds for
things like $HOME on NFS (the fact of which is not transparent to
confinement, sadly) or encrypted home filesystem. as well as other
quirks that need it specifically in snap-confine's profile (snapd can
already influence profiles of specific snap applications).

It is important to note that the packaging must ensure the directory
/var/lib/snapd/apparmor/snap-confine.d must exist or the profile will
fail to compile. I did this for Ubuntu and openSUSE and will coordinate
to make sure this happens in Debian. Other distributions are not
affected as they don't enable AppArmor yet.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

cmd/snap-confine,packaging: import snapd-generated policy
This patch allows snap-confie's apparmor profile to be extended with
additional site policy that is generated by snapd. The purpose of the
site policy is so that it can be used to generate some workarounds for
things like $HOME on NFS (the fact of which is not transparent to
confinement, sadly) or encrypted home filesystem. as well as other
quirks that need it specifically in snap-confine's profile (snapd can
already influence profiles of specific snap applications).

It is important to note that the packaging must ensure the directory
/var/lib/snapd/apparmor/snap-confine.d *must* exist or the profile will
fail to compile. I did this for Ubuntu and openSUSE and will coordinate
to make sure this happens in Debian. Other distributions are not
affected as they don't enable AppArmor yet.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

This looks fine.

mvo5 approved these changes Aug 25, 2017

cmd: install snap-confine.d directory unconditionally
This directory is sourced by the apparmor profile for snap-confine and
it should be present even if apparmor is disabled on compile time.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Collaborator

mvo5 commented Aug 25, 2017

Ubuntu core tests fail with:

+ su -l -c test-snapd-system-observe-consumer.consumer test

snap-confine has elevated permissions and is not confined but should be. Refusing to continue to avoid permission escalation attacks

I assume this is because this dir is missing on the core-image?

Contributor

zyga commented Aug 25, 2017

@mvo5 - yes, this is because adding a subdirectory under /var/lib/snapd/apparmor/snap-confine.d is actually harder than it should be. I'm considering what I can do to avoid that.

Unfortunately apparmor_parser will abort if the directory is not present, I didn't anticipate it would be hard to add a new directory to that structure.

the problem here is that /var/lib/snapd in writable-paths is set to "transition" mode which by definition will not add new directories or files from the readonly rootfs when they appear, we could switch to "synced" mode but this needs testing if/how existing files in the rw path are handled properly (theoretically this should all "just work" but the dir is to important to blindly do that switch).

@zyga zyga added the Blocked label Aug 30, 2017

Contributor

zyga commented Aug 30, 2017

I'm marking this as blocked because we have no clear path to adding that directory early at system startup on existing core devices. At the same time this PR is a prerequisite for a priority Canonical commercial project. @mvo5 please advice on suggested action.

@ogra1 ogra1 referenced this pull request in snapcore/core-build Sep 1, 2017

Merged

switch /etc/systemd/system to "synced" mode #17

ogra1 added a commit to snapcore/core-build that referenced this pull request Sep 7, 2017

ogra1 added a commit to snapcore/core-build that referenced this pull request Sep 7, 2017

Member

chipaca commented Sep 12, 2017

@zyga I believe this should now be unblocked

Codecov Report

❗️ No coverage uploaded for pull request base (master@1c9ec2a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3807   +/-   ##
=========================================
  Coverage          ?   76.78%           
=========================================
  Files             ?      416           
  Lines             ?    36978           
  Branches          ?        0           
=========================================
  Hits              ?    28395           
  Misses            ?     6672           
  Partials          ?     1911

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c9ec2a...5970b70. Read the comment docs.

Collaborator

mvo5 commented Sep 15, 2017

This should be ok to merge now

Collaborator

mvo5 commented Sep 20, 2017

@zyga zyga removed the Blocked label Sep 20, 2017

Contributor

zyga commented Sep 20, 2017

The failure, whatever it was, got resolved after merging master.

@zyga zyga requested a review from jdstrand Sep 20, 2017

@mvo5 mvo5 merged commit d1ba97c into snapcore:master Sep 20, 2017

5 of 7 checks passed

artful-amd64 autopkgtest finished (failure)
Details
artful-i386 autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:feature/snap-confine-profile-extensions branch Sep 22, 2017

@zyga zyga referenced this pull request Sep 22, 2017

Merged

cmd: update "make hack" #3952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment