fix(clusterplugin): write the cluster config through a safe open

sdk/clusterplugin/plugin.go

@@ -2,12 +2,16 @@ package clusterplugin
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
+	"syscall"
 
 	"github.com/kairos-io/kairos/v4/sdk/bus"
 	"github.com/mudler/go-pluggable"
 	yip "github.com/mudler/yip/pkg/schema"
+	"github.com/twpayne/go-vfs/v4"
 	"gopkg.in/yaml.v3"
 )
 
@@ -55,30 +59,108 @@ func (p ClusterPlugin) onBoot(event *pluggable.Event) pluggable.EventResponse {
 		configFilePath = config.Cluster.ClusterConfigPath
 	}
 
-	// open our cloud configuration file for writing
-	f, err := filesystem.OpenFile(configFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600)
-	if err != nil {
-		response.Error = fmt.Sprintf("failed to parse boot event: %s", err.Error())
+	if err := writeClusterConfig(configFilePath, cc); err != nil {
+		response.Error = err.Error()
 		return response
 	}
 
+	return response
+}
+
+// writeClusterConfig writes the provider's cloud-config to configFilePath.
+//
+// The file holds the cluster token in cleartext and is written as root on
+// every boot, at a path on the persistent partition, so the open has to
+// survive whatever is sitting there already (kairos-io/kairos#4867):
+//
+//   - the parent directory is created first. A provider that points
+//     cluster_config_path at a directory the image does not ship, or a node
+//     whose /usr/local/cloud-config has not been created yet, used to fail the
+//     whole boot event on ENOENT.
+//   - O_NOFOLLOW refuses a symlink at the last path element. Without it a
+//     symlink planted there redirects the write, and O_TRUNC empties whatever
+//     it points at. Only the last element is protected, so a symlinked
+//     cloud-config directory keeps working.
+//   - O_NONBLOCK returns instead of parking in open(2) when the path is a FIFO
+//     with no reader, and the fstat that follows rejects every file type that
+//     is not a regular file. The flag does not change writes to a regular file.
+//   - the mode is set after the open. O_CREATE applies perm only when it
+//     creates the file, so a file pre-created by another user kept its own mode
+//     and then received the cluster token.
+//
+// Planting any of these needs write access to a persistent path, so this is
+// robustness rather than a privilege boundary. What it buys is that the failure
+// is reported, instead of being a truncated stranger's file, a boot that hangs
+// forever, or a token anyone can read.
+func writeClusterConfig(configFilePath string, cc yip.YipConfig) error {
+	dir := filepath.Dir(configFilePath)
+	if err := vfs.MkdirAll(filesystem, dir, 0700); err != nil {
+		return fmt.Errorf("failed to create the cluster config directory %s: %w", dir, err)
+	}
+
+	f, err := filesystem.OpenFile(configFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0600)
+	if err != nil {
+		// Name what is in the way. Both of these arrive as a bare errno that
+		// says nothing about the path holding something other than a file.
+		switch {
+		case errors.Is(err, syscall.ELOOP):
+			return fmt.Errorf("refusing to write the cluster config to %s: it is a symlink, not a regular file", configFilePath)
+		case errors.Is(err, syscall.ENXIO):
+			return fmt.Errorf("refusing to write the cluster config to %s: it is a named pipe, not a regular file", configFilePath)
+		}
+
+		return fmt.Errorf("failed to open the cluster config file %s: %w", configFilePath, err)
+	}
+
 	defer f.Close()
 
-	// write the cloud configuration header
-	_, err = f.WriteString("#cloud-config\n")
+	stat, err := f.Stat()
 	if err != nil {
-		response.Error = fmt.Sprintf("failed to parse boot event: %s", err.Error())
-		return response
+		return fmt.Errorf("failed to stat the cluster config file %s: %w", configFilePath, err)
+	}
+
+	if !stat.Mode().IsRegular() {
+		return fmt.Errorf("refusing to write the cluster config to %s: it is %s, not a regular file", configFilePath, fileTypeName(stat.Mode()))
+	}
+
+	if stat.Mode().Perm() != 0600 {
+		if err := f.Chmod(0600); err != nil {
+			return fmt.Errorf("failed to restrict the cluster config file %s to 0600: %w", configFilePath, err)
+		}
+	}
+
+	// write the cloud configuration header
+	if _, err := f.WriteString("#cloud-config\n"); err != nil {
+		return fmt.Errorf("failed to write the cluster config file %s: %w", configFilePath, err)
 	}
 
 	// encode the provider's configuration
-	err = yaml.NewEncoder(f).Encode(cc)
-	if err != nil {
-		response.Error = fmt.Sprintf("failed to parse boot event: %s", err.Error())
-		return response
+	if err := yaml.NewEncoder(f).Encode(cc); err != nil {
+		return fmt.Errorf("failed to encode the cluster config into %s: %w", configFilePath, err)
 	}
 
-	return response
+	return nil
+}
+
+// fileTypeName names the file type in an error somebody has to act on, since
+// "not a regular file" on its own does not say what to go and look for.
+func fileTypeName(mode os.FileMode) string {
+	switch {
+	case mode&os.ModeNamedPipe != 0:
+		return "a named pipe"
+	case mode&os.ModeSocket != 0:
+		return "a socket"
+	case mode&os.ModeCharDevice != 0:
+		return "a character device"
+	case mode&os.ModeDevice != 0:
+		return "a block device"
+	case mode&os.ModeSymlink != 0:
+		return "a symlink"
+	case mode.IsDir():
+		return "a directory"
+	default:
+		return "an irregular file"
+	}
 }
 
 func (p ClusterPlugin) Run(extraPlugins ...pluggable.FactoryPlugin) error {

sdk/clusterplugin/plugin_test.go

@@ -0,0 +1,212 @@
+package clusterplugin
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"syscall"
+	"time"
+
+	"github.com/kairos-io/kairos/v4/sdk/bus"
+	"github.com/mudler/go-pluggable"
+	yip "github.com/mudler/yip/pkg/schema"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+// The cluster config the provider hands back. Its content only has to be
+// recognisable in the file afterwards.
+func testProviderConfig() yip.YipConfig {
+	return yip.YipConfig{Name: "cluster-token-in-here"}
+}
+
+// bootEvent builds the payload agent.boot sends, with cluster_config_path
+// pointing at path.
+func bootEvent(path string) *pluggable.Event {
+	cfg := fmt.Sprintf("#cloud-config\ncluster:\n  cluster_token: sekret\n  role: worker\n  cluster_config_path: %s\n", path)
+	data, err := json.Marshal(bus.EventPayload{Config: cfg})
+	Expect(err).ToNot(HaveOccurred())
+
+	return &pluggable.Event{Name: bus.EventBoot, Data: string(data)}
+}
+
+// callWithin runs f and fails the spec if it has not returned by bound, so
+// that a blocking open fails the run instead of hanging it forever.
+func callWithin(bound time.Duration, f func() error) error {
+	done := make(chan error, 1)
+	go func() {
+		defer GinkgoRecover()
+		done <- f()
+	}()
+
+	select {
+	case err := <-done:
+		return err
+	case <-time.After(bound):
+		Fail(fmt.Sprintf("writeClusterConfig did not return within %s", bound))
+		return nil
+	}
+}
+
+var _ = Describe("writeClusterConfig", func() {
+	var root string
+
+	BeforeEach(func() {
+		root = GinkgoT().TempDir()
+	})
+
+	It("creates the directory the cluster config goes in", func() {
+		// A node whose /usr/local/cloud-config does not exist yet used to
+		// fail the whole boot event here, on ENOENT.
+		path := filepath.Join(root, "cloud-config", "cluster.kairos.yaml")
+
+		Expect(writeClusterConfig(path, testProviderConfig())).To(Succeed())
+
+		content, err := os.ReadFile(path)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(string(content)).To(HavePrefix("#cloud-config\n"))
+		Expect(string(content)).To(ContainSubstring("cluster-token-in-here"))
+
+		info, err := os.Stat(path)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(info.Mode().Perm()).To(Equal(os.FileMode(0600)))
+	})
+
+	It("writes the config once the directory is already there", func() {
+		path := filepath.Join(root, "cluster.kairos.yaml")
+
+		Expect(writeClusterConfig(path, testProviderConfig())).To(Succeed())
+		// A second boot rewrites it in place.
+		Expect(writeClusterConfig(path, testProviderConfig())).To(Succeed())
+
+		content, err := os.ReadFile(path)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(string(content)).To(HavePrefix("#cloud-config\n"))
+	})
+
+	It("refuses a symlink at the config path, and leaves its target alone", func() {
+		victim := filepath.Join(root, "victim")
+		Expect(os.WriteFile(victim, []byte("do not truncate me"), 0644)).To(Succeed())
+
+		path := filepath.Join(root, "cluster.kairos.yaml")
+		Expect(os.Symlink(victim, path)).To(Succeed())
+
+		err := writeClusterConfig(path, testProviderConfig())
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring(path))
+		Expect(err.Error()).To(ContainSubstring("it is a symlink"))
+
+		// O_TRUNC never reached the target.
+		content, err := os.ReadFile(victim)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(string(content)).To(Equal("do not truncate me"))
+	})
+
+	It("does not block on a named pipe nobody is reading", func() {
+		path := filepath.Join(root, "cluster.kairos.yaml")
+		Expect(syscall.Mkfifo(path, 0600)).To(Succeed())
+
+		err := callWithin(10*time.Second, func() error {
+			return writeClusterConfig(path, testProviderConfig())
+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("it is a named pipe"))
+	})
+
+	It("refuses a named pipe somebody is reading, instead of writing the token into it", func() {
+		path := filepath.Join(root, "cluster.kairos.yaml")
+		Expect(syscall.Mkfifo(path, 0600)).To(Succeed())
+
+		// A reader makes the write-side open succeed, so only the fstat
+		// stands between the cluster token and the pipe.
+		r, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NONBLOCK, 0)
+		Expect(err).ToNot(HaveOccurred())
+		defer r.Close()
+
+		err = callWithin(10*time.Second, func() error {
+			return writeClusterConfig(path, testProviderConfig())
+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("a named pipe"))
+		Expect(err.Error()).To(ContainSubstring("not a regular file"))
+
+		// Nothing was handed to the pipe. The write side is closed again by
+		// now, so the read reports EOF rather than EAGAIN; either way it
+		// carries no bytes.
+		buf := make([]byte, 4096)
+		n, _ := r.Read(buf)
+		Expect(n).To(Equal(0))
+	})
+
+	It("refuses a directory at the config path", func() {
+		path := filepath.Join(root, "cluster.kairos.yaml")
+		Expect(os.Mkdir(path, 0700)).To(Succeed())
+
+		err := writeClusterConfig(path, testProviderConfig())
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("takes the mode down to 0600 on a file somebody else created", func() {
+		// O_CREATE applies its perm argument only when it creates the file,
+		// so a pre-created 0666 file used to receive the cluster token and
+		// stay world readable.
+		path := filepath.Join(root, "cluster.kairos.yaml")
+		Expect(os.WriteFile(path, []byte("planted"), 0666)).To(Succeed())
+		Expect(os.Chmod(path, 0666)).To(Succeed())
+
+		Expect(writeClusterConfig(path, testProviderConfig())).To(Succeed())
+
+		info, err := os.Stat(path)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(info.Mode().Perm()).To(Equal(os.FileMode(0600)))
+
+		content, err := os.ReadFile(path)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(string(content)).ToNot(ContainSubstring("planted"))
+	})
+})
+
+var _ = Describe("onBoot", func() {
+	var plugin ClusterPlugin
+
+	BeforeEach(func() {
+		plugin = ClusterPlugin{Provider: func(cluster Cluster) yip.YipConfig {
+			return testProviderConfig()
+		}}
+	})
+
+	It("says what went wrong with the file, not that the event failed to parse", func() {
+		// Every error past the JSON unmarshal used to read
+		// "failed to parse boot event", which sends the reader to the
+		// payload rather than to the filesystem.
+		root := GinkgoT().TempDir()
+		path := filepath.Join(root, "cluster.kairos.yaml")
+		Expect(os.Mkdir(path, 0700)).To(Succeed())
+
+		response := plugin.onBoot(bootEvent(path))
+
+		Expect(response.Error).ToNot(BeEmpty())
+		Expect(response.Error).ToNot(ContainSubstring("failed to parse boot event"))
+		Expect(response.Error).To(ContainSubstring(path))
+	})
+
+	It("still reports a payload that is not JSON as a parse failure", func() {
+		response := plugin.onBoot(&pluggable.Event{Name: bus.EventBoot, Data: "not json"})
+
+		Expect(response.Error).To(ContainSubstring("failed to parse boot event"))
+	})
+
+	It("writes the provider config through to the path the cluster asked for", func() {
+		root := GinkgoT().TempDir()
+		path := filepath.Join(root, "cloud-config", "cluster.kairos.yaml")
+
+		response := plugin.onBoot(bootEvent(path))
+		Expect(response.Error).To(BeEmpty())
+
+		content, err := os.ReadFile(path)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(string(content)).To(ContainSubstring("cluster-token-in-here"))
+	})
+})

sdk/clusterplugin/suite_test.go

@@ -0,0 +1,13 @@
+package clusterplugin
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestClusterPlugin(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Cluster Plugin Suite")
+}