fix(imageupgrade): serialise upgrade calls and sync the ESP directory

internal/grpc/imageupgrade.go

@@ -74,6 +74,13 @@ var ErrImageNotResealable = errors.New("grpc: the state key cannot be resealed f
 // package need not import the implementation.
 var ErrNoPreviousImage = errors.New("grpc: no previous image to roll back to")
 
+// ErrUpgradeInProgress is returned by an ImageUpgrader's Stage, Rollback or
+// Activate when another one of those three calls is already running on this
+// node. The handler maps it to FailedPrecondition: the request itself is
+// fine, but a second call in flight at the same time as a Stage could share
+// the write slot (BOOTX64.NEW.EFI) and leave a half-written boot image.
+var ErrUpgradeInProgress = errors.New("grpc: an image upgrade call is already in progress on this node")
+
 // ImageUpgrader replaces the node's CryptOS image without touching its
 // identity. It is wired on the mTLS and local servers of a running node; the
 // maintenance servers leave it nil so the image RPCs return Unimplemented
@@ -123,6 +130,10 @@ func (s *Server) StageImage(stream grpc.ClientStreamingServer[nodev1.StageImageR
 			return status.Errorf(codes.FailedPrecondition,
 				"StageImage: nothing was written: %v", err)
 		}
+		if errors.Is(err, ErrUpgradeInProgress) {
+			return status.Error(codes.FailedPrecondition,
+				"StageImage: another image upgrade call is already in progress on this node")
+		}
 		return status.Errorf(codes.Internal, "StageImage: %v", err)
 	}
 
@@ -209,6 +220,10 @@ func (s *Server) RollbackImage(ctx context.Context, _ *nodev1.RollbackImageReque
 		if errors.Is(err, ErrNoPreviousImage) {
 			return nil, status.Error(codes.FailedPrecondition, "RollbackImage: this node has no previous image retained")
 		}
+		if errors.Is(err, ErrUpgradeInProgress) {
+			return nil, status.Error(codes.FailedPrecondition,
+				"RollbackImage: another image upgrade call is already in progress on this node")
+		}
 		return nil, status.Errorf(codes.Internal, "RollbackImage: %v", err)
 	}
 
@@ -243,6 +258,10 @@ func (s *Server) ActivateImage(ctx context.Context, req *nodev1.ActivateImageReq
 		if errors.Is(err, reset.ErrConfirmMismatch) {
 			return nil, status.Error(codes.PermissionDenied, "ActivateImage: confirmation CN does not match the CA CN")
 		}
+		if errors.Is(err, ErrUpgradeInProgress) {
+			return nil, status.Error(codes.FailedPrecondition,
+				"ActivateImage: another image upgrade call is already in progress on this node")
+		}
 		return nil, status.Errorf(codes.Internal, "ActivateImage: %v", err)
 	}
 

internal/imageupgrade/imageupgrade.go

@@ -97,6 +97,13 @@ type FS interface {
 	Remove(rel string) error
 	Rename(from, to string) error
 	WriteFile(rel string, data []byte) error
+	// Sync fsyncs the ESP's mounted directory. A vfat rename is only a
+	// directory-entry update; WriteFile's own fsync covers a file's data, not
+	// that entry. Without this, a power loss right after the rename that swaps
+	// the active image can leave the directory entry unresolved -- neither
+	// surely the old image nor surely the new one -- rather than one of the
+	// two, which is what every caller of Stage and Rollback relies on.
+	Sync() error
 }
 
 // Stager installs verified images into the ESP slots.
@@ -198,6 +205,9 @@ func (s *Stager) Stage(image, signature []byte) (Status, error) {
 	if err := s.fs.Rename(incomingRelPath, ActiveRelPath); err != nil {
 		return Status{}, fmt.Errorf("imageupgrade: activate the incoming image: %w", err)
 	}
+	if err := s.fs.Sync(); err != nil {
+		return Status{}, fmt.Errorf("imageupgrade: sync the ESP directory: %w", err)
+	}
 
 	return s.Status()
 }
@@ -223,6 +233,9 @@ func (s *Stager) Rollback() error {
 	if err := s.fs.Rename(incomingRelPath, ActiveRelPath); err != nil {
 		return fmt.Errorf("imageupgrade: restore the previous image: %w", err)
 	}
+	if err := s.fs.Sync(); err != nil {
+		return fmt.Errorf("imageupgrade: sync the ESP directory: %w", err)
+	}
 
 	return nil
 }

internal/imageupgrade/imageupgrade_test.go

@@ -30,11 +30,15 @@ import (
 )
 
 // memFS is the ESP in memory. failWrite lets a test interrupt the sequence at
-// the point that decides whether a node is left bootable.
+// the point that decides whether a node is left bootable. ops records every
+// call in order (including Sync), so a test can assert the directory sync
+// happens after the rename that it is meant to make durable.
 type memFS struct {
 	files     map[string][]byte
 	failWrite map[string]error
+	failSync  error
 	writes    []string
+	ops       []string
 }
 
 func newMemFS() *memFS {
@@ -55,6 +59,7 @@ func (m *memFS) ReadFile(rel string) ([]byte, error) {
 func (m *memFS) Remove(rel string) error { delete(m.files, rel); return nil }
 
 func (m *memFS) Rename(from, to string) error {
+	m.ops = append(m.ops, "rename:"+from+"->"+to)
 	data, ok := m.files[from]
 	if !ok {
 		return errors.New("not found: " + from)
@@ -67,6 +72,7 @@ func (m *memFS) Rename(from, to string) error {
 
 func (m *memFS) WriteFile(rel string, data []byte) error {
 	m.writes = append(m.writes, rel)
+	m.ops = append(m.ops, "write:"+rel)
 	if err := m.failWrite[rel]; err != nil {
 		return err
 	}
@@ -75,6 +81,14 @@ func (m *memFS) WriteFile(rel string, data []byte) error {
 	return nil
 }
 
+// Sync stands in for fsyncing the ESP's mounted directory: the durability
+// barrier a vfat rename needs that a file's own fsync does not provide.
+func (m *memFS) Sync() error {
+	m.ops = append(m.ops, "sync")
+
+	return m.failSync
+}
+
 type release struct {
 	cert *x509.Certificate
 	key  *rsa.PrivateKey
@@ -249,6 +263,40 @@ func TestStage_OnANodeWithNoActiveImage(t *testing.T) {
 	}
 }
 
+// A vfat rename isn't durable across a power loss on its own: the directory
+// entry update needs its own fsync. Stage must call it right after the
+// rename that makes the incoming image the active one (#373).
+func TestStage_SyncsTheESPDirectoryAfterTheRename(t *testing.T) {
+	rel := newRelease(t)
+	fs := newMemFS()
+	fs.files[ActiveRelPath] = []byte("old image")
+	s, _ := New(fs, rel.cert)
+
+	image := []byte("new image")
+	if _, err := s.Stage(image, rel.sign(t, image)); err != nil {
+		t.Fatalf("Stage: %v", err)
+	}
+
+	wantLast := "rename:" + incomingRelPath + "->" + ActiveRelPath
+	if n := len(fs.ops); n < 2 || fs.ops[n-2] != wantLast || fs.ops[n-1] != "sync" {
+		t.Fatalf("ops = %v, want the activating rename immediately followed by a sync", fs.ops)
+	}
+}
+
+// If the directory sync fails, Stage must say so rather than reporting
+// success over a rename that might not survive a power loss.
+func TestStage_ReturnsAnErrorWhenTheDirectorySyncFails(t *testing.T) {
+	rel := newRelease(t)
+	fs := newMemFS()
+	fs.failSync = errors.New("input/output error")
+	s, _ := New(fs, rel.cert)
+
+	image := []byte("new image")
+	if _, err := s.Stage(image, rel.sign(t, image)); err == nil {
+		t.Fatal("Stage reported success despite a failed directory sync")
+	}
+}
+
 func TestRollback_RestoresThePreviousImage(t *testing.T) {
 	rel := newRelease(t)
 	fs := newMemFS()
@@ -268,6 +316,47 @@ func TestRollback_RestoresThePreviousImage(t *testing.T) {
 	}
 }
 
+// Rollback's final rename needs the same directory durability barrier as
+// Stage's (#373).
+func TestRollback_SyncsTheESPDirectoryAfterTheRename(t *testing.T) {
+	rel := newRelease(t)
+	fs := newMemFS()
+	fs.files[ActiveRelPath] = []byte("old image")
+	s, _ := New(fs, rel.cert)
+
+	image := []byte("bad image")
+	if _, err := s.Stage(image, rel.sign(t, image)); err != nil {
+		t.Fatalf("Stage: %v", err)
+	}
+	fs.ops = nil // only Rollback's own ops matter here
+
+	if err := s.Rollback(); err != nil {
+		t.Fatalf("Rollback: %v", err)
+	}
+
+	wantLast := "rename:" + incomingRelPath + "->" + ActiveRelPath
+	if n := len(fs.ops); n < 2 || fs.ops[n-2] != wantLast || fs.ops[n-1] != "sync" {
+		t.Fatalf("ops = %v, want the restoring rename immediately followed by a sync", fs.ops)
+	}
+}
+
+func TestRollback_ReturnsAnErrorWhenTheDirectorySyncFails(t *testing.T) {
+	rel := newRelease(t)
+	fs := newMemFS()
+	fs.files[ActiveRelPath] = []byte("old image")
+	s, _ := New(fs, rel.cert)
+
+	image := []byte("bad image")
+	if _, err := s.Stage(image, rel.sign(t, image)); err != nil {
+		t.Fatalf("Stage: %v", err)
+	}
+	fs.failSync = errors.New("input/output error")
+
+	if err := s.Rollback(); err == nil {
+		t.Fatal("Rollback reported success despite a failed directory sync")
+	}
+}
+
 func TestRollback_WithNothingRetained(t *testing.T) {
 	rel := newRelease(t)
 	s, _ := New(newMemFS(), rel.cert)

internal/init/imageupgrade.go

@@ -36,6 +36,7 @@ import (
 	"log"
 	"os"
 	"path/filepath"
+	"sync"
 	"time"
 
 	nodev1 "github.com/CryptOS-PKI/cryptos-node/gen/go/cryptos/node/v1"
@@ -99,6 +100,13 @@ type imageUpgradeOptions struct {
 // nodeImageUpgrader implements grpc.ImageUpgrader.
 type nodeImageUpgrader struct {
 	opts imageUpgradeOptions
+
+	// mu serializes Stage, Rollback and Activate (#373): all three choreograph
+	// the same ESP slots (BOOTX64.NEW.EFI is shared write scratch), so two of
+	// them in flight at once could corrupt a slot rather than merely race for
+	// it. TryLock rather than Lock: a caller made while another is already
+	// running is refused outright (FailedPrecondition), not queued behind it.
+	mu sync.Mutex
 }
 
 func newImageUpgrader(opts imageUpgradeOptions) (*nodeImageUpgrader, error) {
@@ -121,6 +129,11 @@ func newImageUpgrader(opts imageUpgradeOptions) (*nodeImageUpgrader, error) {
 // Stage verifies the image and installs it as the one the firmware will boot,
 // retaining the current image. It does not reboot.
 func (u *nodeImageUpgrader) Stage(ctx context.Context, image, signature []byte) (*nodev1.ImageStatus, error) {
+	if !u.mu.TryLock() {
+		return nil, cgrpc.ErrUpgradeInProgress
+	}
+	defer u.mu.Unlock()
+
 	// Verified before the ESP is mounted at all, let alone writable. An image
 	// the node cannot attribute never gets near the boot partition of a
 	// production CA, and the stager verifies again on its own behalf -- cheap
@@ -205,6 +218,11 @@ func (u *nodeImageUpgrader) reseal(ctx context.Context, esp dirFS, image []byte)
 // boots with, and a prune that fails does not undo the rollback: the header
 // keeps its old tokens, the rollback target among them, until the next stage.
 func (u *nodeImageUpgrader) Rollback(ctx context.Context) (*nodev1.ImageStatus, error) {
+	if !u.mu.TryLock() {
+		return nil, cgrpc.ErrUpgradeInProgress
+	}
+	defer u.mu.Unlock()
+
 	var st imageupgrade.Status
 	err := u.opts.Mount(true, func(root string) error {
 		esp := dirFS{root: root}
@@ -298,6 +316,11 @@ func (u *nodeImageUpgrader) pruneAfterRollback(ctx context.Context, esp dirFS, r
 // boot the same image the node is already running is an outage with nothing to
 // show for it -- far more likely a mistake than an intention.
 func (u *nodeImageUpgrader) Activate(ctx context.Context, confirmCommonName string) error {
+	if !u.mu.TryLock() {
+		return cgrpc.ErrUpgradeInProgress
+	}
+	defer u.mu.Unlock()
+
 	if err := reset.CheckConfirm(u.opts.CACN(), confirmCommonName); err != nil {
 		return err
 	}
@@ -397,6 +420,23 @@ func (d dirFS) WriteFile(rel string, data []byte) error {
 	return f.Close()
 }
 
+// Sync fsyncs the ESP root directory, the barrier a vfat rename needs: the
+// rename that swaps the active image updates a directory entry, which a
+// file's own fsync (WriteFile, above) does not cover.
+func (d dirFS) Sync() error {
+	f, err := os.Open(d.root)
+	if err != nil {
+		return err
+	}
+	if err := f.Sync(); err != nil {
+		_ = f.Close()
+
+		return err
+	}
+
+	return f.Close()
+}
+
 // digestFile returns the SHA-256 (hex) of rel under a mounted ESP, or an empty
 // string when it is not there. It is how a booting node learns which image it
 // is running: at startup the file on the boot path is, by definition, the one