From d9b7f0faad136b8019ad0fd5f77d72f42fe8f051 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 21 Sep 2026 19:10:29 +0000 Subject: [PATCH] fix(clusterplugin): write the cluster config through a safe open The cluster plugin wrote the provider's cloud-config with O_CREATE|O_WRONLY|O_TRUNC and nothing else, at a path that holds the cluster token in cleartext and is rewritten as root on every boot. It never created the parent directory, so a node whose cloud-config directory is not there yet, or a provider that points cluster_config_path somewhere the image does not ship, failed the whole boot event on ENOENT. The open itself trusted the path. A symlink redirected the write and O_TRUNC emptied whatever it pointed at. A FIFO parked the boot in open(2) for good. A file somebody else had already created kept its own mode, because O_CREATE applies perm only when it creates the file, and then received the cluster token. MkdirAll the parent, then open with O_NOFOLLOW and O_NONBLOCK, reject anything that is not a regular file, and set the mode to 0600 after the open rather than trusting O_CREATE to have done it. Only the last path element is protected by O_NOFOLLOW, so a symlinked cloud-config directory keeps working. Every error past the JSON unmarshal in onBoot also said "failed to parse boot event", which points the reader at the payload instead of at the filesystem. Each step now says what it was doing, and the two errnos that mean "the path is not a file" name what is in the way. Fixes kairos-io/kairos#4867 Signed-off-by: Ettore Di Giacinto --- sdk/clusterplugin/plugin.go | 108 ++++++++++++++-- sdk/clusterplugin/plugin_test.go | 212 +++++++++++++++++++++++++++++++ sdk/clusterplugin/suite_test.go | 13 ++ 3 files changed, 320 insertions(+), 13 deletions(-) create mode 100644 sdk/clusterplugin/plugin_test.go create mode 100644 sdk/clusterplugin/suite_test.go diff --git a/sdk/clusterplugin/plugin.go b/sdk/clusterplugin/plugin.go index 9b9cab4921..17861a0ddc 100644 --- a/sdk/clusterplugin/plugin.go +++ b/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 { diff --git a/sdk/clusterplugin/plugin_test.go b/sdk/clusterplugin/plugin_test.go new file mode 100644 index 0000000000..776f6a8a60 --- /dev/null +++ b/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")) + }) +}) diff --git a/sdk/clusterplugin/suite_test.go b/sdk/clusterplugin/suite_test.go new file mode 100644 index 0000000000..68abb30c59 --- /dev/null +++ b/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") +}