diff --git a/vantagectl/internal/cmd/backup.go b/vantagectl/internal/cmd/backup.go index 538f71f..c70ffdc 100644 --- a/vantagectl/internal/cmd/backup.go +++ b/vantagectl/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. diff --git a/vantagectl/internal/cmd/inspect_test.go b/vantagectl/internal/cmd/inspect_test.go index 81125fc..a6bed5a 100644 --- a/vantagectl/internal/cmd/inspect_test.go +++ b/vantagectl/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") + } +} diff --git a/vantagectl/internal/cmd/root.go b/vantagectl/internal/cmd/root.go index 7d0b859..c4f9b11 100644 --- a/vantagectl/internal/cmd/root.go +++ b/vantagectl/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. diff --git a/vantagectl/internal/cmd/verify.go b/vantagectl/internal/cmd/verify.go index 758947b..d1a55c8 100644 --- a/vantagectl/internal/cmd/verify.go +++ b/vantagectl/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) diff --git a/vantagectl/internal/cmd/verify_test.go b/vantagectl/internal/cmd/verify_test.go new file mode 100644 index 0000000..72e66e3 --- /dev/null +++ b/vantagectl/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()) + } +}