test: ✅🐛 Source audits for renames and transports

cmd/agentop/cmd_config_migrate.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/rossoctl/cortex/core/config"
@@ -224,22 +225,40 @@ func loadCandidate(updated string) (cfg *config.Config, loaded bool, err error)
 // unparseable config would take the proxy down on next start, so the original
 // survives any failure.
 func replaceConfig(path, updated string, verify func(*config.Config) error) error {
-	tmp := path + ".tmp"
-	if werr := os.WriteFile(tmp, []byte(updated), 0o600); werr != nil {
+	// CreateTemp, not path+".tmp": a fixed name is a symlink an attacker can
+	// plant, and the file being replaced is the one the proxy boots from.
+	tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp*")
+	if err != nil {
+		return err
+	}
+	tmpName := tmp.Name()
+	defer func() { _ = os.Remove(tmpName) }() // no-op once the rename succeeds
+	if _, werr := tmp.WriteString(updated); werr != nil {
+		_ = tmp.Close()
 		return werr
 	}
-	cfg, verr := config.Load(tmp)
+	// Sync before the rename: a crash after it must not leave the config the
+	// proxy boots from empty.
+	if serr := tmp.Sync(); serr != nil {
+		_ = tmp.Close()
+		return serr
+	}
+	if cerr := tmp.Close(); cerr != nil {
+		return cerr
+	}
+	if err := os.Chmod(tmpName, 0o600); err != nil {
+		return err
+	}
+	cfg, verr := config.Load(tmpName)
 	if verr != nil {
-		_ = os.Remove(tmp)
 		return fmt.Errorf("the migrated config would not parse (%w); left %s alone", verr, path)
 	}
 	if verify != nil {
 		if verr := verify(cfg); verr != nil {
-			_ = os.Remove(tmp)
 			return fmt.Errorf("%w; left %s alone", verr, path)
 		}
 	}
-	return os.Rename(tmp, path)
+	return os.Rename(tmpName, path)
 }
 
 // insertListenerKeys adds keys to the listener block, matching whatever

cmd/agentop/cmd_config_migrate_pricing_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"math"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -304,7 +305,9 @@ func TestReplaceConfig_VerifyFailureKeepsTheOriginal(t *testing.T) {
 	if readFile(t, p) != before {
 		t.Error("the file was replaced despite the failed verify")
 	}
-	if _, serr := os.Stat(p + ".tmp"); !os.IsNotExist(serr) {
-		t.Errorf("temp file left behind (stat err %v)", serr)
+	// The temp name is CreateTemp's (".<base>.tmp<random>"), so the check is a
+	// directory glob: a stat of a fixed path+".tmp" sees nothing either way.
+	if left, _ := filepath.Glob(filepath.Join(filepath.Dir(p), "."+filepath.Base(p)+".tmp*")); len(left) > 0 {
+		t.Errorf("temp file left behind: %v", left)
 	}
 }

cmd/agentop/cmd_script.go

@@ -451,17 +451,42 @@ func writeRecord(dir string, rec installRecord) error {
 	return writeAtomic(filepath.Join(dir, scriptRecord), append(b, '\n'), 0o600)
 }
 
-// writeAtomic writes b to path through a temp file whose name a slot ignores.
+// writeAtomic writes b to path through a CreateTemp'd file whose dot-prefixed
+// name a slot ignores, synced before the rename: this also writes the install
+// record, and a crash that empties THAT file wedges both install and remove on
+// a folder neither can make sense of.
 func writeAtomic(path string, b []byte, mode os.FileMode) error {
-	tmp := filepath.Join(filepath.Dir(path), "."+filepath.Base(path)+".tmp")
-	if err := os.WriteFile(tmp, b, mode); err != nil {
+	f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp*")
+	if err != nil {
+		return err
+	}
+	tmp := f.Name()
+	ok := false
+	defer func() {
+		if !ok {
+			_ = os.Remove(tmp)
+		}
+	}()
+	if _, err := f.Write(b); err != nil {
+		_ = f.Close()
+		return err
+	}
+	if err := f.Chmod(mode); err != nil {
+		_ = f.Close()
 		return err
 	}
-	if err := os.Chmod(tmp, mode); err != nil {
-		os.Remove(tmp)
+	if err := f.Sync(); err != nil {
+		_ = f.Close()
 		return err
 	}
-	return os.Rename(tmp, path)
+	if err := f.Close(); err != nil {
+		return err
+	}
+	if err := os.Rename(tmp, path); err != nil {
+		return err
+	}
+	ok = true
+	return nil
 }
 
 // removeFiles deletes what rec lists, the record, then dir when it is empty.

cmd/agentop/userconfig.go

@@ -139,6 +139,11 @@ func saveUserConfig(path string, s tui.UserSettings) error {
 	if _, err = writeAll(f, fileHeader); err == nil {
 		_, err = writeAll(f, body)
 	}
+	// Sync before the rename, so a crash cannot leave the renamed settings
+	// file empty and the next start reporting a malformed config.
+	if err == nil {
+		err = f.Sync()
+	}
 	if cerr := f.Close(); err == nil {
 		err = cerr
 	}

cmd/agentop/userconfig_test.go

@@ -282,8 +282,10 @@ func TestSaveUserConfig_LeavesNoTempFile(t *testing.T) {
 	if err := saveUserConfig(path, tui.UserSettings{Usage: tui.UsageSettings{Metric: "x"}}); err != nil {
 		t.Fatal(err)
 	}
-	if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) {
-		t.Errorf("tempfile survived the save (stat err = %v)", err)
+	// The temp name is CreateTemp's (".<base>.<random>.tmp"), so the check is a
+	// directory glob: a stat of a fixed path+".tmp" sees nothing either way.
+	if left, _ := filepath.Glob(filepath.Join(filepath.Dir(path), "."+filepath.Base(path)+".*.tmp")); len(left) > 0 {
+		t.Errorf("tempfile survived the save: %v", left)
 	}
 }