snapstate: do not allow classic mode for strict snaps #6039

mvo5 · 2018-10-24T22:51:27Z

Right now it is possible to install a strictly confined snap as in
classic confinement mode with:

snap install --classic test-snapd-tools

This should not work.

Right now it is possible to install a strictly confined snap as in classic confinement mode with: ``` snap install --classic test-snapd-tools ``` This should not work.

zyga

+1

zyga

Some unit tests are broken now:


FAIL: snapstate_test.go:483: snapmgrTestSuite.TestInstallClassicConfinementFiltering

snapstate_test.go:501:
    c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"classic confinment requested for a non classic confined snap"} ("classic confinment requested for a non classic confined snap")

zyga

I tried to quickly fix the failure but I was unable to, I think this requires a deeper look. Some of the tests seem to indicate that we wanted the feature to operate this way too.

chipaca · 2018-10-26T09:38:21Z

Well, when we added classic I seem to recall we went over the combinations discussing which we wanted to work and which we didn't :-) I get it that we got one of them wrong, but that means we probably need to fix tests as well

chipaca · 2018-10-26T09:38:52Z

Also also: we need an error kind, so the clients can catch the error and offer suggestions / helpful error messages instead of the arcane server message

…don't support classic Mock classic snap support so that all unit test can are run everywhere Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo · 2018-10-26T11:49:37Z

overlord/snapstate/check_snap.go

@@ -129,6 +129,10 @@ func (e *SnapNeedsClassicSystemError) Error() string {
 // determine whether the flags (and system overrides thereof) are
 // compatible with the given *snap.Info
 func validateFlagsForInfo(info *snap.Info, snapst *SnapState, flags Flags) error {
+	if flags.Classic && !info.NeedsClassic() {
+		return fmt.Errorf("classic confinment requested for a non classic confined snap")


confinement

bboozzoo · 2018-10-26T11:50:19Z

I'll try to push fixes for the unit tests

Signed-off-by: Maciej Borzecki <[email protected]>

…end unit tests Allow upgrading from classic to strict on update, but keep blocking the transition from strict to classic. Update unit tests for installation and updates. Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo · 2018-10-26T14:58:22Z

@mvo5 the unit tests should pass now. I updated the handling of classic flag when the snap is transitioning from classic to strict or reverse on refresh. Hopefully, I haven't broken anything while doing so.

Needs re-review

zyga · 2018-10-26T15:53:00Z

The test failures in spread look real:

    - google:amazon-linux-2-64:tests/main/install-errors:noreexec

…ting classic Signed-off-by: Maciej Borzecki <[email protected]>

zyga

+1

Please fix the typo before merging.

overlord/snapstate/snapstate.go

zyga · 2018-10-26T18:52:52Z

2018-10-26 17:53:12 Error executing google:amazon-linux-2-64:tests/main/refresh:classic_fake : 
-----
+ . /home/gopath/src/github.com/snapcore/snapd/tests/lib/systems.sh
+ '[' fake = fake ']'
+ is_core_system
+ [[ amazon-linux-2-64 == ubuntu-core-* ]]
+ return 1
+ '[' true = false ']'
+ [[ test-snapd-classic-confinement =~ classic ]]
+ case "$SPREAD_SYSTEM" in
+ echo 'When the snap is refreshed'
When the snap is refreshed
+ snap refresh --channel=edge test-snapd-classic-confinement
error: cannot perform the following tasks:
- Mount snap "test-snapd-classic-confinement" (3) (snap "test-snapd-classic-confinement" requires classic confinement)

Signed-off-by: Maciej Borzecki <[email protected]>

Co-Authored-By: mvo5 <[email protected]>

mvo5

Nice! Just one question inline about infoToUpdate. Also wondering we should add an errot type for this, iirc @chipaca suggested this.

mvo5 · 2018-10-29T14:24:41Z

overlord/snapstate/snapstate.go

@@ -731,6 +731,11 @@ func UpdateMany(ctx context.Context, st *state.State, names []string, userID int

 	params := func(update *snap.Info) (string, Flags, *SnapState) {
 		snapst := stateByInstanceName[update.InstanceName()]
+		updateFlags := snapst.Flags
+		if !update.NeedsClassic() && updateFlags.Classic {


mvo5 · 2018-10-29T14:25:20Z

overlord/snapstate/snapstate.go

@@ -1118,7 +1123,12 @@ func Update(st *state.State, name, channel string, revision snap.Revision, userI
 	}

 	params := func(update *snap.Info) (string, Flags, *SnapState) {
-		return channel, flags, &snapst
+		updateFlags := flags
+		if !update.NeedsClassic() && updateFlags.Classic {


Slightly sad that our code requires to duplicate this with the code above.

Actually given that infoForUpdate is also changed (below) - do we need this change here or is the change in infoForUpdate sufficient?

Snap info is verified twice, once in infoForUpdate() and then once again in doUpdate(). I'm beginning to think that maybe verifying flags in infoForUpdate() is actually superfluous.

@bboozzoo Thanks for checking this! Would be great if you could double check, the less code the better :)

Dropped the part in infoForUpdate().

Validation of snap.Info with current set of flags is already done in doUpdate() for all code paths. No need to do it earlier when obtaining snap.Info for update. Signed-off-by: Maciej Borzecki <[email protected]>

mvo5

👍 (can't approve this as I create the PR)

pedronis · 2018-11-07T11:18:05Z

I agree with @chipaca about wanting an error kind for this, client.ErrKindSnapNotClassic would fit the mold I think (to live parallel to the *SnapNeeds* errors etc)

bboozzoo · 2018-11-07T11:40:24Z

I can look into adding the error once I'm back.

Introduce a separate error kind for a case when an attempt to install a non classic snap with classic confinement is made. Signed-off-by: Maciej Borzecki <[email protected]>

Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo · 2018-11-09T10:10:55Z

@chipaca can you take another look?

mvo5 · 2018-11-09T10:20:37Z

Fwiw, looks good to me, thanks for the changes!

chipaca · 2018-11-09T17:50:18Z

cmd/snap/error.go

@@ -191,6 +191,8 @@ usually confined to, which may put your system at risk.

 If you understand and want to proceed repeat the command including --classic.
 `)
+	case client.ErrorKindSnapNotClassic:
+		msg = i18n.G(`snap %q is not a classic confined snap`)


Not a blocker, but I think this is not a good error message for a user.

pedronis · 2018-11-09T18:26:49Z

<pedronis> Chipaca: given that #6039 is introducing that error, maybe that fact that is not good is a blocker :)
<pedronis> Chipaca: this is 2.37 material, if you have suggestion for a better error, please suggest
<mborzecki> Chipaca: pedronis: how about `classic confinment requested, but snap %q is not a classic confined snap` ?
<mborzecki> the original version by mvo was `classic confinment requested for a non classic confined snap', maybe a variant of this could work
<pedronis> mborzecki: I think Chipaca is suggesting that it's one of those longer messages, explaining a bit the concept as well
<pedronis> I'm honestly a bit too tired right now to make a suggestion
<pedronis> like the ones for SnapNeeds*
<pedronis> maybe the error needs to carry the actual confinement to help produce a better error
<pedronis> message
<pedronis> as well

pedronis · 2018-11-09T18:44:54Z

<Chipaca> mborzecki: pedronis: i'm very tired so not sure this suggestion is any good, but something like "snap %q is not compatible with --classic" would spell out exactly what the user did wrong

chipaca

Extra +1 for good luck

Signed-off-by: Maciej Borzecki <[email protected]>

mvo5 · 2018-11-13T09:50:57Z

@robert-ancell This PR adds a new error kind that may be relevant for snapd-glib

robert-ancell · 2018-11-13T20:11:52Z

@mvo thanks! In canonical/snapd-glib@57a93bb

snapstate: do not allow classic mode for strict snaps

c611736

Right now it is possible to install a strictly confined snap as in classic confinement mode with: ``` snap install --classic test-snapd-tools ``` This should not work.

zyga approved these changes Oct 25, 2018

View reviewed changes

zyga reviewed Oct 25, 2018

View reviewed changes

zyga previously requested changes Oct 25, 2018

View reviewed changes

bboozzoo added 2 commits October 26, 2018 11:51

Merge remote-tracking branch 'origin/master' into bboozzoo/pr-6039

21c6993

overlord/snapstate: run tests for classic snaps even on systems that …

9dbe291

…don't support classic Mock classic snap support so that all unit test can are run everywhere Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo reviewed Oct 26, 2018

View reviewed changes

bboozzoo added 3 commits October 26, 2018 16:43

Merge remote-tracking branch 'origin/master' into bboozzoo/pr-6039

b55a5db

overlord/snapstate: fix typo

a82c8ba

Signed-off-by: Maciej Borzecki <[email protected]>

tests/main/install-errors: fix error matching, fix systems not suppor…

a960454

…ting classic Signed-off-by: Maciej Borzecki <[email protected]>

zyga approved these changes Oct 26, 2018

View reviewed changes

zyga reviewed Oct 26, 2018

View reviewed changes

overlord/snapstate/snapstate.go Outdated Show resolved Hide resolved

tests/lib/fakestore: return confinement in snap details

b9c8127

Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo force-pushed the no-classic-for-strict branch from 131723d to b9c8127 Compare October 29, 2018 09:20

Update overlord/snapstate/snapstate.go

e0c146b

Co-Authored-By: mvo5 <[email protected]>

mvo5 commented Oct 29, 2018

View reviewed changes

overlord/snapstate: drop superfluous info and flags validation

563744b

Validation of snap.Info with current set of flags is already done in doUpdate() for all code paths. No need to do it earlier when obtaining snap.Info for update. Signed-off-by: Maciej Borzecki <[email protected]>

mvo5 commented Nov 5, 2018

View reviewed changes

bboozzoo added 2 commits November 9, 2018 09:48

Merge remote-tracking branch 'origin/master' into bboozzoo/pr-6039

4b243d1

many: separate error for a non classic confined snap

0afa031

Introduce a separate error kind for a case when an attempt to install a non classic snap with classic confinement is made. Signed-off-by: Maciej Borzecki <[email protected]>

tests/main/install-errors: update to match new error message

c21e3a2

Signed-off-by: Maciej Borzecki <[email protected]>

pedronis requested a review from chipaca November 9, 2018 10:10

chipaca approved these changes Nov 9, 2018

View reviewed changes

snap: improve ErrorKindSnapNotClassic message

be6bd59

chipaca approved these changes Nov 12, 2018

View reviewed changes

tests/main/install-errors: update to match snap error changes

99eab3a

Signed-off-by: Maciej Borzecki <[email protected]>

mvo5 merged commit ba566fe into canonical:master Nov 13, 2018

jstevans mentioned this pull request Mar 7, 2019

Add camera plug to Snap build config sindresorhus/caprine#813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snapstate: do not allow classic mode for strict snaps #6039

snapstate: do not allow classic mode for strict snaps #6039

mvo5 commented Oct 24, 2018

zyga left a comment

zyga left a comment

zyga left a comment

chipaca commented Oct 26, 2018

chipaca commented Oct 26, 2018

bboozzoo Oct 26, 2018

bboozzoo commented Oct 26, 2018

bboozzoo commented Oct 26, 2018

zyga commented Oct 26, 2018

zyga left a comment •

edited

Loading

zyga commented Oct 26, 2018

mvo5 left a comment

mvo5 Oct 29, 2018

mvo5 Oct 29, 2018

mvo5 Oct 29, 2018

bboozzoo Oct 29, 2018

mvo5 Oct 29, 2018

bboozzoo Oct 30, 2018

mvo5 left a comment

pedronis commented Nov 7, 2018 •

edited

Loading

bboozzoo commented Nov 7, 2018

bboozzoo commented Nov 9, 2018

mvo5 commented Nov 9, 2018

chipaca Nov 9, 2018

pedronis commented Nov 9, 2018 •

edited

Loading

pedronis commented Nov 9, 2018

chipaca left a comment

mvo5 commented Nov 13, 2018 •

edited by pedronis

Loading

robert-ancell commented Nov 13, 2018

snapstate: do not allow classic mode for strict snaps #6039

snapstate: do not allow classic mode for strict snaps #6039

Conversation

mvo5 commented Oct 24, 2018

zyga left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

chipaca commented Oct 26, 2018

chipaca commented Oct 26, 2018

Choose a reason for hiding this comment

bboozzoo commented Oct 26, 2018

bboozzoo commented Oct 26, 2018

zyga commented Oct 26, 2018

zyga left a comment • edited Loading

Choose a reason for hiding this comment

zyga commented Oct 26, 2018

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

pedronis commented Nov 7, 2018 • edited Loading

bboozzoo commented Nov 7, 2018

bboozzoo commented Nov 9, 2018

mvo5 commented Nov 9, 2018

Choose a reason for hiding this comment

pedronis commented Nov 9, 2018 • edited Loading

pedronis commented Nov 9, 2018

chipaca left a comment

Choose a reason for hiding this comment

mvo5 commented Nov 13, 2018 • edited by pedronis Loading

robert-ancell commented Nov 13, 2018

zyga left a comment •

edited

Loading

pedronis commented Nov 7, 2018 •

edited

Loading

pedronis commented Nov 9, 2018 •

edited

Loading

mvo5 commented Nov 13, 2018 •

edited by pedronis

Loading