fix: read KEY_ENCRYPTION_KEY in archive-only verify; never leave a partial archive

internal/cmd/backup.go

@@ -42,11 +42,16 @@ func newBackupCmd() *cobra.Command {
 			}
 			defer client.Disconnect(context.Background())
 
-			w, closeOut, name, err := backupDestination(out, g.Database)
+			w, dest, name, err := backupDestination(out, g.Database)
 			if err != nil {
 				return err
 			}
-			defer closeOut()
+			committed := false
+			defer func() {
+				if !committed {
+					dest.Cleanup()
+				}
+			}()
 
 			m, err := backup.Dump(ctx, backup.DumpOptions{
 				Client:         client,
@@ -60,6 +65,10 @@ func newBackupCmd() *cobra.Command {
 			if err != nil {
 				return err
 			}
+			if err := dest.Commit(); err != nil {
+				return err
+			}
+			committed = true
 
 			// Progress goes to stderr so --out - stays a clean pipe.
 			var docs int64
@@ -86,19 +95,59 @@ func newBackupCmd() *cobra.Command {
 }
 
 // backupDestination resolves --out to a writer, a closer and a name to print.
-func backupDestination(out, database string) (io.Writer, func(), string, error) {
+func backupDestination(out, database string) (io.Writer, destination, string, error) {
 	if out == "-" {
-		return os.Stdout, func() {}, "stdout", nil
+		return os.Stdout, stdoutDestination{}, "stdout", nil
 	}
 	name := archiveName(database, time.Now().UTC())
 	path := filepath.Join(out, name)
-	f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
+	// Written under a temporary name and renamed on success, the same
+	// discipline the agent uses for authorized_keys: a failed backup must not
+	// leave a partial file named exactly like a good archive.
+	tmp := path + ".partial"
+	f, err := os.OpenFile(tmp, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
 	if err != nil {
-		return nil, nil, "", fmt.Errorf("create %s: %w", path, err)
+		return nil, nil, "", fmt.Errorf("create %s: %w", tmp, err)
 	}
-	return f, func() { f.Close() }, path, nil
+	d := &fileDestination{f: f, tmp: tmp, final: path}
+	return f, d, path, nil
 }
 
+// fileDestination finishes a file-backed backup. Commit renames the temporary
+// file into place; Cleanup removes it if Commit was never called.
+type fileDestination struct {
+	f     *os.File
+	tmp   string
+	final string
+}
+
+func (d *fileDestination) Commit() error {
+	if err := d.f.Close(); err != nil {
+		return fmt.Errorf("close %s: %w", d.tmp, err)
+	}
+	if err := os.Rename(d.tmp, d.final); err != nil {
+		return fmt.Errorf("rename %s: %w", d.tmp, err)
+	}
+	return nil
+}
+
+func (d *fileDestination) Cleanup() {
+	d.f.Close()
+	os.Remove(d.tmp)
+}
+
+// destination is how the two --out modes finish. stdout commits by doing
+// nothing; there is no partial file to clean up either.
+type destination interface {
+	Commit() error
+	Cleanup()
+}
+
+type stdoutDestination struct{}
+
+func (stdoutDestination) Commit() error { return nil }
+func (stdoutDestination) Cleanup()      {}
+
 // archiveName is sortable and carries no colon, because an operator will copy
 // these onto a Windows share sooner or later and a colon is not a legal
 // filename character there.

internal/cmd/inspect_test.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"bytes"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -68,3 +69,48 @@ func TestRenderManifestFlagsAMissingFingerprint(t *testing.T) {
 		t.Fatalf("a null fingerprint must explain the consequence, got:\n%s", out)
 	}
 }
+
+// TestBackupDestinationDoesNotLeaveAPartialArchive covers the failure path: a
+// backup that errors must not leave a file named exactly like a good archive.
+func TestBackupDestinationDoesNotLeaveAPartialArchive(t *testing.T) {
+	dir := t.TempDir()
+	w, dest, path, err := backupDestination(dir, "vantage")
+	if err != nil {
+		t.Fatalf("backupDestination: %v", err)
+	}
+	if _, err := w.Write([]byte("half an archive")); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+	dest.Cleanup()
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatalf("readdir: %v", err)
+	}
+	if len(entries) != 0 {
+		t.Fatalf("a failed backup left %v behind", entries)
+	}
+	if _, err := os.Stat(path); !os.IsNotExist(err) {
+		t.Fatalf("%s exists after a failed backup", path)
+	}
+}
+
+func TestBackupDestinationRenamesOnCommit(t *testing.T) {
+	dir := t.TempDir()
+	w, dest, path, err := backupDestination(dir, "vantage")
+	if err != nil {
+		t.Fatalf("backupDestination: %v", err)
+	}
+	if _, err := w.Write([]byte("a whole archive")); err != nil {
+		t.Fatalf("write: %v", err)
+	}
+	if err := dest.Commit(); err != nil {
+		t.Fatalf("Commit: %v", err)
+	}
+	if _, err := os.Stat(path); err != nil {
+		t.Fatalf("committed archive is not at %s: %v", path, err)
+	}
+	if _, err := os.Stat(path + ".partial"); !os.IsNotExist(err) {
+		t.Fatal("the temporary file was left behind")
+	}
+}

internal/cmd/root.go

@@ -93,10 +93,16 @@ func resolveGlobals(c *cobra.Command) (*globalOpts, error) {
 	return &globalOpts{
 		MongoURI: uri,
 		Database: db,
-		KeyHex:   strings.TrimSpace(os.Getenv("KEY_ENCRYPTION_KEY")),
+		KeyHex:   keyFromEnv(),
 	}, nil
 }
 
+// keyFromEnv reads KEY_ENCRYPTION_KEY. It is separate from resolveGlobals
+// because verify needs the key even when there is no database to resolve.
+func keyFromEnv() string {
+	return strings.TrimSpace(os.Getenv("KEY_ENCRYPTION_KEY"))
+}
+
 // databaseFromURI reads the database out of the URI path. sitesvc takes its
 // database name this way too, so an operator who has configured one has
 // configured both.

internal/cmd/verify.go

@@ -31,7 +31,12 @@ func newVerifyCmd() *cobra.Command {
 			}
 			defer archive.Close()
 
-			opt := backup.VerifyOptions{Archive: archive}
+			// The key is read unconditionally. Archive-only mode — no MONGO_URI,
+			// which is what a scheduled check uses — must still compare the key
+			// in hand against the archive's fingerprint; leaving it unset there
+			// reported "KEY_ENCRYPTION_KEY is not set" for a key that was set
+			// and correct.
+			opt := backup.VerifyOptions{Archive: archive, KeyHex: keyFromEnv()}
 
 			// A database is optional here. resolveGlobals fails without a URI or
 			// without a resolvable database name, and either error is a signal to
@@ -45,7 +50,6 @@ func newVerifyCmd() *cobra.Command {
 				defer client.Disconnect(context.Background())
 				opt.Client = client
 				opt.Database = g.Database
-				opt.KeyHex = g.KeyHex
 			} else {
 				fmt.Fprintf(c.ErrOrStderr(),
 					"note: %v, so this checks the archive and the key only\n", gerr)

internal/cmd/verify_test.go

@@ -0,0 +1,90 @@
+package cmd
+
+import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"gitea.hostxtra.co.uk/mrhid6/vantage/shared/backup"
+)
+
+const testKeyHex = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+
+// writeArchive builds a minimal, valid archive without touching a database.
+func writeArchive(t *testing.T, keyHex string) string {
+	t.Helper()
+	path := filepath.Join(t.TempDir(), "archive.tar.gz")
+	f, err := os.Create(path)
+	if err != nil {
+		t.Fatalf("create: %v", err)
+	}
+	defer f.Close()
+
+	w := backup.NewWriter(f)
+	entry, err := w.WriteCollection("servers", nil)
+	if err != nil {
+		t.Fatalf("WriteCollection: %v", err)
+	}
+	fp, err := backup.FingerprintHex(keyHex)
+	if err != nil {
+		t.Fatalf("FingerprintHex: %v", err)
+	}
+	if err := w.Close(backup.Manifest{
+		FormatVersion:  backup.FormatVersion,
+		CreatedAt:      time.Now().UTC(),
+		MongoDB:        "vantage",
+		KeyFingerprint: &fp,
+		Collections:    []backup.CollectionEntry{entry},
+	}); err != nil {
+		t.Fatalf("Close: %v", err)
+	}
+	return path
+}
+
+// TestVerifyReadsTheKeyWithoutADatabase is the regression guard for the key
+// being read only on the branch that resolved a database: archive-only mode is
+// what a scheduled check runs, and it reported "KEY_ENCRYPTION_KEY is not set"
+// for a key that was set and correct.
+func TestVerifyReadsTheKeyWithoutADatabase(t *testing.T) {
+	path := writeArchive(t, testKeyHex)
+	t.Setenv("MONGO_URI", "")
+	t.Setenv("MONGO_DB", "")
+	t.Setenv("KEY_ENCRYPTION_KEY", testKeyHex)
+
+	root := NewRoot("test")
+	var out, errBuf bytes.Buffer
+	root.SetOut(&out)
+	root.SetErr(&errBuf)
+	root.SetArgs([]string{"verify", path})
+
+	if err := root.Execute(); err != nil {
+		t.Fatalf("verify failed with the correct key and no database: %v\n%s%s",
+			err, out.String(), errBuf.String())
+	}
+	if !strings.Contains(out.String(), "Key match     yes") {
+		t.Fatalf("archive-only verify did not compare the key:\n%s", out.String())
+	}
+}
+
+func TestVerifyReportsAKeyMismatchWithoutADatabase(t *testing.T) {
+	path := writeArchive(t, testKeyHex)
+	t.Setenv("MONGO_URI", "")
+	t.Setenv("MONGO_DB", "")
+	t.Setenv("KEY_ENCRYPTION_KEY", strings.Repeat("ab", 32))
+
+	root := NewRoot("test")
+	var out, errBuf bytes.Buffer
+	root.SetOut(&out)
+	root.SetErr(&errBuf)
+	root.SetArgs([]string{"verify", path})
+
+	if err := root.Execute(); err == nil {
+		t.Fatalf("verify accepted the wrong key:\n%s", out.String())
+	}
+	if !strings.Contains(out.String(), "key mismatch") {
+		t.Fatalf("output does not name the mismatch:\n%s", out.String())
+	}
+}