diff --git a/internal/grpc/imageupgrade.go b/internal/grpc/imageupgrade.go index 3a8f5df..a263c27 100644 --- a/internal/grpc/imageupgrade.go +++ b/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) } diff --git a/internal/imageupgrade/imageupgrade.go b/internal/imageupgrade/imageupgrade.go index d38706b..044dbcc 100644 --- a/internal/imageupgrade/imageupgrade.go +++ b/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 } diff --git a/internal/imageupgrade/imageupgrade_test.go b/internal/imageupgrade/imageupgrade_test.go index 390644a..7c43a09 100644 --- a/internal/imageupgrade/imageupgrade_test.go +++ b/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) diff --git a/internal/init/imageupgrade.go b/internal/init/imageupgrade.go index 0489096..07a3715 100644 --- a/internal/init/imageupgrade.go +++ b/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 diff --git a/internal/init/imageupgrade_test.go b/internal/init/imageupgrade_test.go index 868f6d3..6b2055e 100644 --- a/internal/init/imageupgrade_test.go +++ b/internal/init/imageupgrade_test.go @@ -333,6 +333,113 @@ func TestUpgrader_ActivateRebootsOnTheRightCommonName(t *testing.T) { } } +// Stage, Rollback and Activate share one lock (#373): two upgrade calls at +// the same time could otherwise both write BOOTX64.NEW.EFI, and a caller made +// while another is already running gets FailedPrecondition rather than +// queuing behind it or racing it. +// +// This drives the overlap with real goroutines rather than only holding the +// lock by hand, so it proves the lock is actually wired into Stage, not just +// that TryLock itself works. +func TestUpgrader_StageSerializesAgainstAConcurrentStage(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + started := make(chan struct{}) + release := make(chan struct{}) + blockingMount := func(readWrite bool, fn func(root string) error) error { + return esp.mount(readWrite, func(root string) error { + close(started) + <-release + + return fn(root) + }) + } + u, err := newImageUpgrader(imageUpgradeOptions{ + CACN: func() string { return testCACN }, + Mount: blockingMount, + Reboot: func() { t.Fatal("must not reboot") }, + Release: rel.cert, + Running: digestOf(old), + Version: "v1.2.3", + }) + if err != nil { + t.Fatalf("newImageUpgrader: %v", err) + } + + image := []byte("the new image") + sig := rel.sign(t, image) + firstDone := make(chan error, 1) + go func() { + _, stageErr := u.Stage(context.Background(), image, sig) + firstDone <- stageErr + }() + + <-started + if _, err := u.Stage(context.Background(), image, sig); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("concurrent Stage err = %v, want ErrUpgradeInProgress", err) + } + close(release) + if err := <-firstDone; err != nil { + t.Fatalf("the first Stage: %v", err) + } +} + +// A caller must not be able to Stage while a Rollback the test holds the lock +// for is in flight (or vice versa); checked here against the lock directly, +// which exercises the same guard the goroutine test above exercises through +// real concurrency. +func TestUpgrader_StageRefusesWhileAnUpgradeIsInProgress(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + u := newTestUpgrader(t, esp, rel, old, func() { t.Fatal("must not reboot") }) + u.mu.Lock() + defer u.mu.Unlock() + + image := []byte("the new image") + if _, err := u.Stage(context.Background(), image, rel.sign(t, image)); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("err = %v, want ErrUpgradeInProgress", err) + } + if esp.mountedReadWrite() { + t.Error("a refused concurrent Stage still mounted the ESP") + } +} + +func TestUpgrader_RollbackRefusesWhileAnUpgradeIsInProgress(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + u := newTestUpgrader(t, esp, rel, old, func() { t.Fatal("must not reboot") }) + u.mu.Lock() + defer u.mu.Unlock() + + if _, err := u.Rollback(context.Background()); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("err = %v, want ErrUpgradeInProgress", err) + } + if esp.mountedReadWrite() { + t.Error("a refused concurrent Rollback still mounted the ESP") + } +} + +func TestUpgrader_ActivateRefusesWhileAnUpgradeIsInProgress(t *testing.T) { + rel := newReleaseKey(t) + old := []byte("the running image") + esp := newESPDir(t, old) + u := newTestUpgrader(t, esp, rel, old, func() { t.Fatal("must not reboot") }) + + image := []byte("the new image") + if _, err := u.Stage(context.Background(), image, rel.sign(t, image)); err != nil { + t.Fatalf("Stage: %v", err) + } + u.mu.Lock() + defer u.mu.Unlock() + + if err := u.Activate(context.Background(), testCACN); !errors.Is(err, grpc.ErrUpgradeInProgress) { + t.Fatalf("err = %v, want ErrUpgradeInProgress", err) + } +} + // On a node that has not been upgraded, running and active are the same image // and nothing is outstanding. func TestUpgrader_StatusOnAnUnupgradedNode(t *testing.T) {