snapstate: do not allow classic mode for strict snaps

overlord/snapstate/backend_test.go

@@ -196,6 +196,8 @@ func (f *fakeStore) snap(spec snapSpec, user *auth.UserState) (*snap.Info, error
 		typ = snap.TypeGadget
 	case "some-snapd":
 		typ = snap.TypeSnapd
+	case "some-snap-now-classic":
+		confinement = "classic"
 	}
 
 	if spec.Name == "snap-unknown" {
@@ -263,6 +265,10 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) {
 		name = "services-snap"
 	case "some-snap-id":
 		name = "some-snap"
+	case "some-snap-now-classic-id":
+		name = "some-snap-now-classic"
+	case "some-snap-was-classic-id":
+		name = "some-snap-was-classic"
 	case "core-snap-id":
 		name = "core"
 		typ = snap.TypeOS
@@ -305,6 +311,9 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) {
 	case "channel-for-devmode":
 		confinement = snap.DevModeConfinement
 	}
+	if name == "some-snap-now-classic" {
+		confinement = "classic"
+	}
 
 	info := &snap.Info{
 		Type: typ,
@@ -589,31 +598,38 @@ func (f *fakeSnappyBackend) OpenSnapFile(snapFilePath string, si *snap.SideInfo)
 		op.sinfo = *si
 	}
 
-	var name string
+	var info *snap.Info
 	if !osutil.IsDirectory(snapFilePath) {
-		name = filepath.Base(snapFilePath)
+		name := filepath.Base(snapFilePath)
 		split := strings.Split(name, "_")
 		if len(split) >= 2 {
 			// <snap>_<rev>.snap
 			// <snap>_<instance-key>_<rev>.snap
 			name = split[0]
 		}
+
+		info = &snap.Info{SuggestedName: name, Architectures: []string{"all"}}
+		if name == "some-snap-now-classic" {
+			info.Confinement = "classic"
+		}
 	} else {
 		// for snap try only
 		snapf, err := snap.Open(snapFilePath)
 		if err != nil {
 			return nil, nil, err
 		}
 
-		info, err := snap.ReadInfoFromSnapFile(snapf, si)
+		info, err = snap.ReadInfoFromSnapFile(snapf, si)
 		if err != nil {
 			return nil, nil, err
 		}
-		name = info.SuggestedName
 	}
 
+	if info == nil {
+		return nil, nil, fmt.Errorf("internal error: no mocked snap for %q", snapFilePath)
+	}
 	f.appendOp(&op)
-	return &snap.Info{SuggestedName: name, Architectures: []string{"all"}}, f.emptyContainer, nil
+	return info, f.emptyContainer, nil
 }
 
 func (f *fakeSnappyBackend) SetupSnap(snapFilePath, instanceName string, si *snap.SideInfo, p progress.Meter) (snap.Type, error) {

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 confinement requested for a non classic confined snap")
+	}
+
 	switch c := info.Confinement; c {
 	case snap.StrictConfinement, "":
 		// strict is always fine

overlord/snapstate/check_snap_test.go

@@ -540,6 +540,27 @@ confinement: classic
 	c.Assert(err, ErrorMatches, ".* requires classic confinement which is only available on classic systems")
 }
 
+func (s *checkSnapSuite) TestCheckSnapErrorClassicModeForStrictOrDevmode(c *C) {
+	const yaml = `name: hello
+version: 1.10
+confinement: strict
+`
+	info, err := snap.InfoFromSnapYaml([]byte(yaml))
+	c.Assert(err, IsNil)
+
+	var openSnapFile = func(path string, si *snap.SideInfo) (*snap.Info, snap.Container, error) {
+		c.Check(path, Equals, "snap-path")
+		c.Check(si, IsNil)
+		return info, emptyContainer(c), nil
+	}
+	restore := snapstate.MockOpenSnapFile(openSnapFile)
+	defer restore()
+
+	err = snapstate.CheckSnap(s.st, "snap-path", "hello", nil, nil, snapstate.Flags{Classic: true})
+
+	c.Assert(err, ErrorMatches, "classic confinement requested for a non classic confined snap")
+}
+
 func (s *checkSnapSuite) TestCheckSnapKernelUpdate(c *C) {
 	reset := release.MockOnClassic(false)
 	defer reset()

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 {
+			// allow updating from classic to strict
+			updateFlags.Classic = false
+		}
 		return snapst.Channel, snapst.Flags, snapst
 
 	}
@@ -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 {
+			// allow updating from classic to strict
+			updateFlags.Classic = false
+		}
+		return channel, updateFlags, &snapst
 	}
 
 	_, tts, err := doUpdate(context.TODO(), st, []string{name}, updates, params, userID, &flags)
@@ -1192,6 +1202,12 @@ func infoForUpdate(st *state.State, snapst *SnapState, name, channel string, rev
 		if err != nil {
 			return nil, err
 		}
+		if !info.NeedsClassic() && flags.Classic {
+			// thw new revision is no longer classic, but the one
+			// installed is, allow such transition so do not fail
+			// validation
+			flags.Classic = false
+		}
 		if err := validateInfoAndFlags(info, snapst, flags); err != nil {
 			return nil, err
 		}

overlord/snapstate/snapstate_test.go

@@ -20,6 +20,7 @@
 package snapstate_test
 
 import (
+	"bytes"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -510,9 +511,9 @@ func (s *snapmgrTestSuite) TestInstallClassicConfinementFiltering(c *C) {
 	_, err = snapstate.Install(s.state, "some-snap", "channel-for-classic", snap.R(0), s.user.ID, snapstate.Flags{Classic: true})
 	c.Assert(err, IsNil)
 
-	// if a snap is *not* classic, you can still install it with --classic
+	// if a snap is *not* classic, you cannot install it with --classic
 	_, err = snapstate.Install(s.state, "some-snap", "channel-for-strict", snap.R(0), s.user.ID, snapstate.Flags{Classic: true})
-	c.Assert(err, IsNil)
+	c.Assert(err, ErrorMatches, "classic confinement requested for a non classic confined snap")
 }
 
 func (s *snapmgrTestSuite) TestInstallFailsWhenClassicSnapsAreNotSupported(c *C) {
@@ -1832,21 +1833,20 @@ func (s *snapmgrTestSuite) TestUpdateClassicConfinementFiltering(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 
-	snapstate.Set(s.state, "some-snap", &snapstate.SnapState{
+	snapstate.Set(s.state, "some-snap-now-classic", &snapstate.SnapState{
 		Active:   true,
-		Channel:  "channel-for-classic",
-		Sequence: []*snap.SideInfo{{RealName: "some-snap", SnapID: "some-snap-id", Revision: snap.R(7)}},
+		Sequence: []*snap.SideInfo{{RealName: "some-snap-now-classic", SnapID: "some-snap-now-classic-id", Revision: snap.R(7)}},
 		Current:  snap.R(7),
 		SnapType: "app",
 	})
 
 	// updated snap is classic, refresh without --classic, do nothing
 	// TODO: better error message here
-	_, err := snapstate.Update(s.state, "some-snap", "", snap.R(0), s.user.ID, snapstate.Flags{})
+	_, err := snapstate.Update(s.state, "some-snap-now-classic", "", snap.R(0), s.user.ID, snapstate.Flags{})
 	c.Assert(err, ErrorMatches, `.* requires classic confinement`)
 
 	// updated snap is classic, refresh with --classic
-	ts, err := snapstate.Update(s.state, "some-snap", "", snap.R(0), s.user.ID, snapstate.Flags{Classic: true})
+	ts, err := snapstate.Update(s.state, "some-snap-now-classic", "", snap.R(0), s.user.ID, snapstate.Flags{Classic: true})
 	c.Assert(err, IsNil)
 
 	chg := s.state.NewChange("refresh", "refresh snap")
@@ -1857,9 +1857,12 @@ func (s *snapmgrTestSuite) TestUpdateClassicConfinementFiltering(c *C) {
 	s.settle(c)
 	s.state.Lock()
 
+	c.Assert(chg.Err(), IsNil)
+	c.Assert(chg.IsReady(), Equals, true)
+
 	// verify snap is in classic
 	var snapst snapstate.SnapState
-	err = snapstate.Get(s.state, "some-snap", &snapst)
+	err = snapstate.Get(s.state, "some-snap-now-classic", &snapst)
 	c.Assert(err, IsNil)
 	c.Check(snapst.Classic, Equals, true)
 }
@@ -1942,21 +1945,21 @@ func (s *snapmgrTestSuite) TestUpdateStrictFromClassic(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
 
-	snapstate.Set(s.state, "some-snap", &snapstate.SnapState{
+	snapstate.Set(s.state, "some-snap-was-classic", &snapstate.SnapState{
 		Active:   true,
 		Channel:  "channel",
-		Sequence: []*snap.SideInfo{{RealName: "some-snap", SnapID: "some-snap-id", Revision: snap.R(7)}},
+		Sequence: []*snap.SideInfo{{RealName: "some-snap-was-classic", SnapID: "some-snap-was-classic-id", Revision: snap.R(7)}},
 		Current:  snap.R(7),
 		SnapType: "app",
 		Flags:    snapstate.Flags{Classic: true},
 	})
 
 	// snap installed with --classic, update does not need classic, refresh works without --classic
-	_, err := snapstate.Update(s.state, "some-snap", "", snap.R(0), s.user.ID, snapstate.Flags{})
+	_, err := snapstate.Update(s.state, "some-snap-was-classic", "", snap.R(0), s.user.ID, snapstate.Flags{})
 	c.Assert(err, IsNil)
 
 	// snap installed with --classic, update does not need classic, refresh works with --classic
-	_, err = snapstate.Update(s.state, "some-snap", "", snap.R(0), s.user.ID, snapstate.Flags{Classic: true})
+	_, err = snapstate.Update(s.state, "some-snap-was-classic", "", snap.R(0), s.user.ID, snapstate.Flags{Classic: true})
 	c.Assert(err, IsNil)
 }
 
@@ -9127,10 +9130,10 @@ func (s *snapmgrTestSuite) TestTrySetsTryModeClassic(c *C) {
 	restore := maybeMockClassicSupport(c)
 	defer restore()
 
-	s.testTrySetsTryMode(snapstate.Flags{Classic: true}, c)
+	s.testTrySetsTryMode(snapstate.Flags{Classic: true}, c, "confinement: classic\n")
 }
 
-func (s *snapmgrTestSuite) testTrySetsTryMode(flags snapstate.Flags, c *C) {
+func (s *snapmgrTestSuite) testTrySetsTryMode(flags snapstate.Flags, c *C, extraYaml ...string) {
 	s.state.Lock()
 	defer s.state.Unlock()
 
@@ -9140,7 +9143,14 @@ func (s *snapmgrTestSuite) testTrySetsTryMode(flags snapstate.Flags, c *C) {
 	tryYaml := filepath.Join(d, "meta", "snap.yaml")
 	err := os.MkdirAll(filepath.Dir(tryYaml), 0755)
 	c.Assert(err, IsNil)
-	err = ioutil.WriteFile(tryYaml, []byte("name: foo\nversion: 1.0"), 0644)
+	buf := bytes.Buffer{}
+	buf.WriteString("name: foo\nversion: 1.0\n")
+	if len(extraYaml) > 0 {
+		for _, extra := range extraYaml {
+			buf.WriteString(extra)
+		}
+	}
+	err = ioutil.WriteFile(tryYaml, buf.Bytes(), 0644)
 	c.Assert(err, IsNil)
 
 	chg := s.state.NewChange("try", "try snap")
@@ -9197,7 +9207,7 @@ func (s *snapmgrTestSuite) TestTryUndoRemovesTryFlagLeavesJailMode(c *C) {
 func (s *snapmgrTestSuite) TestTryUndoRemovesTryFlagLeavesClassic(c *C) {
 	restore := maybeMockClassicSupport(c)
 	defer restore()
-	s.testTrySetsTryMode(snapstate.Flags{Classic: true}, c)
+	s.testTrySetsTryMode(snapstate.Flags{Classic: true}, c, "confinement: classic\n")
 }
 
 func (s *snapmgrTestSuite) testTryUndoRemovesTryFlag(flags snapstate.Flags, c *C) {

tests/main/install-errors/task.yaml

@@ -76,3 +76,17 @@ execute: |
         exit 1
     fi
     MATCH 'characters that look like dashes but are not' < stderr.out
+
+    #shellcheck source=tests/lib/snaps.sh
+    . "$TESTSLIB"/snaps.sh
+    #shellcheck source=tests/lib/strings.sh
+    . "$TESTSLIB"/strings.sh
+
+    if is_classic_confinement_supported ; then
+        echo "Installing a strict snap in classic mode does not work"
+        if snap install --classic test-snapd-busybox-static 2>stderr.out; then
+            echo "snap install ––classic test-snapd-busybox-static should have failed but did not"
+            exit 1
+        fi
+        str_to_one_line "$(cat stderr.out)" | MATCH 'classic confinement requested for a non classic confined snap'
+    fi