diff --git a/internal/config/config.go b/internal/config/config.go index 231da2f..d84dd8b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,7 @@ import ( "fmt" "os" "regexp" + "syscall" ) var nodePattern = regexp.MustCompile(`^nd_[A-Za-z0-9_-]{8,64}$`) @@ -150,11 +151,19 @@ func loadFile(path string) (File, error) { return cfg, nil } -// Save writes the config back with 0600, atomically. +// Save writes the config back with 0600, atomically, preserving the file's owner. // // Atomic (temp file + rename) because the pairing flow writes the device list at moments when the // agent may be concurrently read by systemd restarts: a half-written config is a config that fails // to load, which turns a completed pairing into an uncommandable node. +// +// Ownership is preserved because the deployment document tells the operator to run `--pair` with +// sudo, and the installed config belongs to the `msa-agent` user. Writing a temp file as root and +// renaming it over the config would leave it root-owned 0600, and the systemd unit — which runs as +// `msa-agent` — could then no longer read it: the pairing would appear to succeed and the node would +// come up unable to load its own configuration. The chown is only attempted when the process can +// perform it (root), and a failure is an error rather than a warning, because silently leaving the +// file unreadable by the service is the failure this is here to prevent. func Save(path string, cfg File) error { if err := cfg.Validate(true); err != nil { return fmt.Errorf("config: refusing to save an invalid file: %w", err) @@ -164,13 +173,37 @@ func Save(path string, cfg File) error { return err } encoded = append(encoded, '\n') + uid, gid, hadOwner := ownerOf(path) temp := path + ".tmp" if err := os.WriteFile(temp, encoded, 0o600); err != nil { return err } + if hadOwner && os.Geteuid() == 0 { + if err := os.Chown(temp, uid, gid); err != nil { + // The temp file is removed so a failed save leaves no stray copy of the config — it + // holds the agent's private keys and the node password. + os.Remove(temp) + return fmt.Errorf("config: preserving ownership of %s: %w", path, err) + } + } return os.Rename(temp, path) } +// ownerOf reports the uid and gid a rewrite of path must keep. ok is false when the file does not +// exist yet, which is the first-write case: there is no owner to preserve, and the file is created +// by whoever is running. +func ownerOf(path string) (uid, gid int, ok bool) { + info, err := os.Stat(path) + if err != nil { + return 0, 0, false + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0, 0, false + } + return int(stat.Uid), int(stat.Gid), true +} + // NewRelayToken mints a device bearer for the relay: 24 random bytes, hex. Hex because the relay // only requires a bearer string and hex never contains a character that needs escaping when it // travels through JSON or headers. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 44ef263..f2835b8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,8 +1,11 @@ package config import ( + "fmt" "os" "path/filepath" + "strings" + "syscall" "testing" ) @@ -66,6 +69,45 @@ func TestSaveRoundTripsAndIsSixHundred(t *testing.T) { } } +// A rewrite must keep the file's owner. The deployment document tells the operator to run --pair +// with sudo while the installed config belongs to the msa-agent user; without preserving ownership +// the rename would leave a root-owned 0600 file that the systemd unit cannot read, so the pairing +// would look successful and the node would come up unable to load its own configuration. +// +// The test cannot become root, so it asserts the property that is observable without privileges: +// the owner after a save is the same as the owner before it, and the temp file does not survive. +func TestSavePreservesTheFileOwner(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := valid() + if err := Save(path, cfg); err != nil { + t.Fatalf("first Save: %v", err) + } + before := owner(t, path) + // A second save goes through the ownership-preserving path. The inode necessarily changes — the + // write is a temp file plus a rename — so what is compared is the owner, which is the thing the + // rename would otherwise reset. + cfg.Devices = append(cfg.Devices, Device{ + ID: "ap_second0001", SignKey: cfg.Devices[0].SignKey, RelayToken: strings.Repeat("b", 48), + }) + if err := Save(path, cfg); err != nil { + t.Fatalf("second Save: %v", err) + } + after := owner(t, path) + if before != after { + t.Fatalf("the rewrite changed the owner from %v to %v", before, after) + } + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Fatal("a temp file was left behind") + } + reloaded, err := Load(path) + if err != nil { + t.Fatalf("Load after rewrite: %v", err) + } + if len(reloaded.Devices) != 2 { + t.Fatalf("the rewrite lost a device: %d", len(reloaded.Devices)) + } +} + // A save must not be able to write a file that the next boot would refuse: the validation runs // before the rename, so a caller building a device list with a bad id gets an error, not a // corrupted file. @@ -138,3 +180,17 @@ func TestLoadReportsMissingAndMalformedFiles(t *testing.T) { t.Fatal("a malformed file was accepted") } } + +// owner reads the uid and gid of a path, which is what a rewrite must preserve. +func owner(t *testing.T, path string) string { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Skip("stat_t is not available on this platform") + } + return fmt.Sprintf("%d:%d", stat.Uid, stat.Gid) +}