o/devicestate: remodel policy/checks changes (canonical#13480)

* fail early if remodeling to an old model revision
* allow offline update remodels without serial

* o/devicestate: try to make some things clearer/simpler

thanks @bboozzoo and @andrewphelpsj

overlord/devicestate/devicestate.go

@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2016-2022 Canonical Ltd
+ * Copyright (C) 2016-2024 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -116,6 +116,23 @@ func findSerial(st *state.State, device *auth.DeviceState) (*asserts.Serial, err
 	return a.(*asserts.Serial), nil
 }
 
+// findKnownRevisionOfModel returns the model assertion revision if any in the
+// assertion database for the given model, otherwise it returns -1.
+func findKnownRevisionOfModel(st *state.State, mod *asserts.Model) (modRevision int, err error) {
+	a, err := assertstate.DB(st).Find(asserts.ModelType, map[string]string{
+		"series":   release.Series,
+		"brand-id": mod.BrandID(),
+		"model":    mod.Model(),
+	})
+	if errors.Is(err, &asserts.NotFoundError{}) {
+		return -1, nil
+	}
+	if err != nil {
+		return 0, err
+	}
+	return a.Revision(), nil
+}
+
 // auto-refresh
 func canAutoRefresh(st *state.State) (bool, error) {
 	// we need to be seeded first
@@ -1205,11 +1222,31 @@ func Remodel(st *state.State, new *asserts.Model, localSnaps []*snap.SideInfo, p
 		return nil, err
 	}
 
+	prevRev, err := findKnownRevisionOfModel(st, new)
+	if err != nil {
+		return nil, err
+	}
+	if new.Revision() < prevRev {
+		return nil, fmt.Errorf("cannot remodel to older revision %d of model %s/%s than last revision %d known to the device", new.Revision(), new.BrandID(), new.Model(), prevRev)
+	}
+
+	// TODO: we need dedicated assertion language to permit for
+	// model transitions before we allow cross vault
+	// transitions.
+
+	remodelKind := ClassifyRemodel(current, new)
+
 	if _, err := findSerial(st, nil); err != nil {
-		if errors.Is(err, state.ErrNoState) {
+		if !errors.Is(err, state.ErrNoState) {
+			return nil, err
+		}
+
+		if len(localSnaps) > 0 && remodelKind == UpdateRemodel {
+			// it is allowed to remodel without serial for
+			// offline remodels that are update only
+		} else {
 			return nil, fmt.Errorf("cannot remodel without a serial")
 		}
-		return nil, err
 	}
 
 	if current.Series() != new.Series() {
@@ -1240,12 +1277,6 @@ func Remodel(st *state.State, new *asserts.Model, localSnaps []*snap.SideInfo, p
 		return nil, errors.New("cannot remodel from UC18+ (using snapd snap) system back to UC16 system (using core snap)")
 	}
 
-	// TODO: we need dedicated assertion language to permit for
-	// model transitions before we allow cross vault
-	// transitions.
-
-	remodelKind := ClassifyRemodel(current, new)
-
 	// TODO: should we restrict remodel from one arch to another?
 	// There are valid use-cases here though, i.e. amd64 machine that
 	// remodels itself to/from i386 (if the HW can do both 32/64 bit)

overlord/devicestate/devicestate_remodel_test.go

@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2016-2023 Canonical Ltd
+ * Copyright (C) 2016-2024 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -272,6 +272,41 @@ func (s *deviceMgrRemodelSuite) TestRemodelCheckGrade(c *C) {
 	}
 }
 
+func (s *deviceMgrRemodelSuite) TestRemodelCannotUseOldModel(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+	s.state.Set("seeded", true)
+
+	// set a model assertion
+	cur := map[string]interface{}{
+		"brand":        "canonical",
+		"model":        "pc-model",
+		"architecture": "amd64",
+		"kernel":       "pc-kernel",
+		"gadget":       "pc",
+	}
+	s.makeModelAssertionInState(c, "canonical", "pc-model", map[string]interface{}{
+		"architecture": "amd64",
+		"kernel":       "pc-kernel",
+		"gadget":       "pc",
+		"revision":     "2",
+	})
+	// no serial assertion, no serial in state
+	devicestatetest.SetDevice(s.state, &auth.DeviceState{
+		Brand: "canonical",
+		Model: "pc-model",
+	})
+
+	newModelHdrs := map[string]interface{}{
+		"revision": "1",
+	}
+	mergeMockModelHeaders(cur, newModelHdrs)
+	new := s.brands.Model("canonical", "pc-model", newModelHdrs)
+	chg, err := devicestate.Remodel(s.state, new, nil, nil)
+	c.Check(chg, IsNil)
+	c.Check(err, ErrorMatches, "cannot remodel to older revision 1 of model canonical/pc-model than last revision 2 known to the device")
+}
+
 func (s *deviceMgrRemodelSuite) TestRemodelRequiresSerial(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()
@@ -4512,6 +4547,16 @@ func (s *deviceMgrRemodelSuite) TestUC20RemodelLocalNonEssentialUpdate(c *C) {
 		&uc20RemodelLocalNonEssentialCase{isUpdate: true})
 }
 
+func (s *deviceMgrRemodelSuite) TestUC20RemodelLocalNonEssentialInstallNoSerial(c *C) {
+	s.testUC20RemodelLocalNonEssential(c,
+		&uc20RemodelLocalNonEssentialCase{isUpdate: false, noSerial: true})
+}
+
+func (s *deviceMgrRemodelSuite) TestUC20RemodelLocalNonEssentialUpdateNoSerial(c *C) {
+	s.testUC20RemodelLocalNonEssential(c,
+		&uc20RemodelLocalNonEssentialCase{isUpdate: true, noSerial: true})
+}
+
 func (s *deviceMgrRemodelSuite) TestUC20RemodelLocalNonEssentialInstallExtraSnap(c *C) {
 	// We check that it is fine to pass down a snap that is not used,
 	// although we might change the behavior in the future.
@@ -4529,6 +4574,7 @@ func (s *deviceMgrRemodelSuite) TestUC20RemodelLocalNonEssentialUpdateExtraSnap(
 type uc20RemodelLocalNonEssentialCase struct {
 	isUpdate    bool
 	notUsedSnap bool
+	noSerial    bool
 }
 
 func (s *deviceMgrRemodelSuite) testUC20RemodelLocalNonEssential(c *C, tc *uc20RemodelLocalNonEssentialCase) {
@@ -4577,11 +4623,16 @@ func (s *deviceMgrRemodelSuite) testUC20RemodelLocalNonEssential(c *C, tc *uc20R
 		"snaps":        snaps,
 	})
 	s.makeSerialAssertionInState(c, "canonical", "pc-model", "serial")
-	devicestatetest.SetDevice(s.state, &auth.DeviceState{
+	deviceState := auth.DeviceState{
 		Brand:  "canonical",
 		Model:  "pc-model",
 		Serial: "serial",
-	})
+	}
+	if tc.noSerial {
+		deviceState.Serial = ""
+		deviceState.KeyID = "device-key-id"
+	}
+	devicestatetest.SetDevice(s.state, &deviceState)
 
 	oldSeededTs := time.Now().AddDate(0, 0, -1)
 	s.state.Set("seeded-systems", []devicestate.SeededSystem{