/snapd

many: fix handling of jail mode in security setup #2310

Closed
wants to merge 6 commits into
from

Conversation

Reviewers

@jdstrand jdstrand

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@zyga @jdstrand
@zyga
Contributor

zyga commented Nov 21, 2016 •
Edited 1 time

This branch is based on #2312 and #2315 and fixes jail mode flag handling when setting up security.

Fixes: https://bugs.launchpad.net/snappy/+bug/1641885

zyga added some commits Nov 21, 2016

@zyga
many: use snap.ConfinementType rather than bool devmode 
This large patch semi-mechanically changes all of devMode bool flag to
snap.ConfinementType with values such as DevmodeConfinemetn (old true)
and StrictConfinement (old false).

The intent is to have more than two states (so that we can have classic
confinement later) and so that the effective confinement is conveyed,
regardless of flags such as devmode and jailmode elsewhere.

The code in the interface manager is kept as-is (same logic as before,
just trivial adjustments to let it compile) so that a subsequent branch
can use EffectiveConfinement and fix the jailmode bug neatly.

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

All checks have passed

5 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@mvo5
xenial-amd64 — autopkgtest finished (success)
Details
@mvo5
xenial-i386 — autopkgtest finished (success)
Details
@mvo5
yakkety-amd64 — autopkgtest finished (success)
Details
@mvo5
zesty-amd64 — autopkgtest finished (success)
Details
26c1aa9
@zyga
overlord/snapstate: add Flags.EffectiveConfinement 
This patch adds a helper function that returns the effective
confinement, given declared snap confinement and flags such as devmode
or jailmode.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
d1e525b
@zyga
tests: add a dummy app to basic-devmode snap 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
1eae22d
@zyga
tests: add regression test for LP: 1641885 
This patch adds a (currently failing) regression test for a jalimode
bug.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
359b468
@zyga
overlord: use EffectiveConfinement instead of DevModeAllowed 
This patch changes all calls to setup snap security that were done with
DevModeAllowed to use EffectiveConfinement. The former was incorrectly
marking all jailmode snaps to use devmode confinement as the function
really meant "is devmode a valid flag" not "is devmode the effective
confinement".

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

@zyga zyga changed the title from Lp jailmode 1641885 to many: fix handling of jail mode in security setup Nov 21, 2016

overlord/snapstate/flags.go
+ if f.JailMode {
+ // jailmode flag overrides devmode flag
+ return snap.StrictConfinement
+ }
@jdstrand

jdstrand Nov 21, 2016

Contributor

At a minimum, the comment here is not right and is confusing, but does this f.JailMode check make sense within the context of snap.StrictConfinement or can it just be removed?

@zyga

zyga Nov 21, 2016

Contributor

I think that having both flags (jail and dev mode) set is an edge case but I think that we should respect the jail mode flag just to err on the strict side. I don't think the code actually allows such a combination.

FYI: please let's continue to review this function in #2312 where it is coming from.

@zyga
Merge remote-tracking branch 'origin/master' into lp-jailmode-1641885

All checks have passed

5 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@mvo5
xenial-amd64 — autopkgtest finished (success)
Details
@mvo5
xenial-i386 — autopkgtest finished (success)
Details
@mvo5
yakkety-amd64 — autopkgtest finished (success)
Details
@mvo5
zesty-amd64 — autopkgtest finished (success)
Details
8cf2f70
@zyga
Contributor

zyga commented Nov 23, 2016

I'll close this pull request and open a new one with different fix based on ConfinementOptions

@zyga zyga closed this Nov 23, 2016

@zyga zyga referenced this pull request Nov 23, 2016

Merged

overlord/ifacestate: don't setup jailmode snaps with devmode confinement #2344

@zyga zyga deleted the zyga:lp-jailmode-1641885 branch Aug 22, 2017

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