Merge pull request #6039 from mvo5/no-classic-for-strict

snapstate: do not allow classic mode for strict snaps

client/client.go

@@ -427,6 +427,7 @@ const (
 	ErrorKindSnapNeedsDevMode       = "snap-needs-devmode"
 	ErrorKindSnapNeedsClassic       = "snap-needs-classic"
 	ErrorKindSnapNeedsClassicSystem = "snap-needs-classic-system"
+	ErrorKindSnapNotClassic         = "snap-not-classic"
 	ErrorKindNoUpdateAvailable      = "snap-no-update-available"
 
 	ErrorKindRevisionNotAvailable     = "snap-revision-not-available"

cmd/snap/cmd_snap_op_test.go

@@ -1325,6 +1325,25 @@ func (s *SnapOpSuite) TestTryMissingOpt(c *check.C) {
 	}
 }
 
+func (s *SnapOpSuite) TestInstallConfinedAsClassic(c *check.C) {
+	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
+		c.Check(r.Method, check.Equals, "POST")
+		w.WriteHeader(400)
+		fmt.Fprintf(w, `{
+  "type": "error",
+  "result": {
+    "message":"error from server",
+    "value": "some-snap",
+    "kind": "snap-not-classic"
+  },
+  "status-code": 400
+}`)
+	})
+
+	_, err := snap.Parser(snap.Client()).ParseArgs([]string{"install", "--classic", "some-snap"})
+	c.Assert(err, check.ErrorMatches, `snap "some-snap" is not compatible with --classic`)
+}
+
 func (s *SnapSuite) TestInstallChannelDuplicationError(c *check.C) {
 	_, err := snap.Parser(snap.Client()).ParseArgs([]string{"install", "--edge", "--beta", "some-snap"})
 	c.Assert(err, check.ErrorMatches, "Please specify a single channel")

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 compatible with --classic`)
 	case client.ErrorKindLoginRequired:
 		usesSnapName = false
 		u, _ := user.Current()

daemon/api_test.go

@@ -7605,6 +7605,7 @@ func (s *apiSuite) TestErrToResponse(c *check.C) {
 	nie := &snap.NotInstalledError{Snap: "foo"}
 	cce := &snapstate.ChangeConflictError{Snap: "foo"}
 	ndme := &snapstate.SnapNeedsDevModeError{Snap: "foo"}
+	nc := &snapstate.SnapNotClassicError{Snap: "foo"}
 	nce := &snapstate.SnapNeedsClassicError{Snap: "foo"}
 	ncse := &snapstate.SnapNeedsClassicSystemError{Snap: "foo"}
 	netoe := fakeNetError{message: "other"}
@@ -7631,6 +7632,7 @@ func (s *apiSuite) TestErrToResponse(c *check.C) {
 		{aie, makeErrorRsp(errorKindSnapAlreadyInstalled, aie, "foo")},
 		{nie, makeErrorRsp(errorKindSnapNotInstalled, nie, "foo")},
 		{ndme, makeErrorRsp(errorKindSnapNeedsDevMode, ndme, "foo")},
+		{nc, makeErrorRsp(errorKindSnapNotClassic, nc, "foo")},
 		{nce, makeErrorRsp(errorKindSnapNeedsClassic, nce, "foo")},
 		{ncse, makeErrorRsp(errorKindSnapNeedsClassicSystem, ncse, "foo")},
 		{cce, SnapChangeConflict(cce)},

daemon/response.go

@@ -176,6 +176,7 @@ const (
 	errorKindSnapNeedsDevMode       = errorKind("snap-needs-devmode")
 	errorKindSnapNeedsClassic       = errorKind("snap-needs-classic")
 	errorKindSnapNeedsClassicSystem = errorKind("snap-needs-classic-system")
+	errorKindSnapNotClassic         = errorKind("snap-not-classic")
 
 	errorKindBadQuery = errorKind("bad-query")
 
@@ -545,6 +546,9 @@ func errToResponse(err error, snaps []string, fallback func(format string, v ...
 		case *snapstate.SnapNeedsClassicSystemError:
 			kind = errorKindSnapNeedsClassicSystem
 			snapName = err.Snap
+		case *snapstate.SnapNotClassicError:
+			kind = errorKindSnapNotClassic
+			snapName = err.Snap
 		case net.Error:
 			if err.Timeout() {
 				kind = errorKindNetworkTimeout

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

@@ -126,9 +126,21 @@ func (e *SnapNeedsClassicSystemError) Error() string {
 	return fmt.Sprintf("snap %q requires classic confinement which is only available on classic systems", e.Snap)
 }
 
+type SnapNotClassicError struct {
+	Snap string
+}
+
+func (e *SnapNotClassicError) Error() string {
+	return fmt.Sprintf("snap %q is not a classic confined snap", e.Snap)
+}
+
 // 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 &SnapNotClassicError{Snap: info.InstanceName()}
+	}
+
 	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, `snap "hello" is not a 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,9 +1202,6 @@ func infoForUpdate(st *state.State, snapst *SnapState, name, channel string, rev
 		if err != nil {
 			return nil, err
 		}
-		if err := validateInfoAndFlags(info, snapst, flags); err != nil {
-			return nil, err
-		}
 		if ValidateRefreshes != nil && !flags.IgnoreValidation {
 			_, err := ValidateRefreshes(st, []*snap.Info{info}, nil, userID)
 			if err != nil {

overlord/snapstate/snapstate_test.go

@@ -20,6 +20,7 @@
 package snapstate_test
 
 import (
+	"bytes"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -510,9 +511,10 @@ 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, `snap "some-snap" is not a classic confined snap`)
+	c.Check(err, FitsTypeOf, &snapstate.SnapNotClassicError{})
 }
 
 func (s *snapmgrTestSuite) TestInstallFailsWhenClassicSnapsAreNotSupported(c *C) {
@@ -1832,21 +1834,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 +1858,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 +1946,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)
 }
 
@@ -9177,10 +9181,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()
 
@@ -9190,7 +9194,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")
@@ -9247,7 +9258,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) {