interfaces/apparmor: allow access to core snap #2413

Merged
merged 4 commits into from Dec 12, 2016

Conversation

Projects
None yet
3 participants
Contributor

zyga commented Dec 6, 2016

This patch changes the default apparmor template to allow read access to
the core snap (at any revision). This is required to allow classic snaps
that use the dynamic linker or libraries when they were forcibly
confined with jailmode.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

interfaces/apparmor: allow access to core snap
This patch changes the default apparmor template to allow read access to
the core snap (at any revision). This is required to allow classic snaps
that use the dynamic linker or libraries when they were forcibly
confined with jailmode.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga added the Critical label Dec 6, 2016

Contributor

jdstrand commented Dec 6, 2016

Based on conversations with @zyga, jailmode with --classic has been defined as strict mode plus read-only access to all of the core snap. As such, please:

  • document the confinement story somewhere
  • remove this rule from the default template and add it conditionally as a snippet 'if isClassic and isJailmode'

This makes the intent of the default template clear that it is (still) a subset of core and makes policy audits clearer.

Contributor

zyga commented Dec 6, 2016

Thanks for the review @jdstrand. I'll document the whole confinement story on the wiki. As for the extra core rule, I can do that via a fake snippet. I'll ping you for a second look.

Contributor

jdstrand commented Dec 6, 2016

Thanks! I think it will also help us future-proof a bit-- I have a suspicion that we'll want to add another rule or two for classic that may not be appropriate for regular strict mode and this will facilitate that.

interfaces/apparmor: add internal jailmode+classic snippet
This patch tweaks earlier code so that when both jailmode and classic
confinement is used a special snippet is internally added. This snippet
provides read only access to the core snap so that the dynamic linker
and runtime libraries can be accessed.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Per our discussion in the channel, LGTM as long as this is opened up only if --jailmode is actually used. We currently don't allow normal snaps to read all of core in its usual mount point, so it wouldn't make a lot of sense to allow them to read it only in an alternative path in general (either it's okay to read, or it's not).

Contributor

niemeyer commented Dec 12, 2016

Sorry, to be clear, --jailmode and --classic, not just --jailmode.

interfaces/apparmor/backend.go
+ // Add a special internal snippet for snaps using classic confinement
+ // and jailmode together. This snippet provides access to the core snap
+ // so that the dynamic linker and shared libraries can be used.
+ tagSnippets = append(tagSnippets, classicJailmodeSnippet)
@niemeyer

niemeyer Dec 12, 2016

Contributor

I think it'd make more sense to have this before the custom snippets.

zyga added some commits Dec 12, 2016

interfaces/apparmor: swap custom and jailmode snippets
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

LGTM with added comment from feedback

+ } else {
+ tagSnippets = snippets[securityTag]
+ }
+ return bytes.Join(tagSnippets, []byte("\n"))
@jdstrand

jdstrand Dec 12, 2016

Contributor

Thank you for this addition. @niemeyer was right to point out that we only want to give read access to the core snap when using --classic with --jailmode (thanks for that!). With strict, we don't want this rule. With just --classic, we don't need this rule. With --devmode plus --jailmode, we don't want this rule. Only with --classic plus --jailmode do we need it.

LGTM but please add this comment above 'if opts.Classic && opts.JailMode {':

# Only add the classicJailmodeSnippet when using --classic plus --jailmode. Why?
# - normal strict mode shouldn't have it since we don't want to subvert the default
#   template
# - --devmode with or without --jailmode shouldn't have it since it wouldn't
#   accurately represent complain-mode strict
# - --classic without --jailmode doesn't need it since it uses different (non-strict)
#   policy
# - --classic with --jailmode is defined as strict policy plus read-only access to the
#   core snap

@zyga zyga merged commit 8ea03cb into snapcore:master Dec 12, 2016

5 of 6 checks passed

trusty-amd64 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
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:core-snap-access branch Aug 22, 2017

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