Harden persistent local volumes (issue #39)

cmd/configcheck/main.go

@@ -3,9 +3,13 @@
 // writes or cloud calls. It is intended to run in GitOps CI so a bad runtime
 // configuration is rejected before expensive image builds.
 //
+// With --node-config it instead validates a hand-authored node config, the
+// direct-Git alternative to control-plane-managed placement.
+//
 // Usage:
 //
 //	configcheck --input-dir <dir> [--require-remote-routing]
+//	configcheck --node-config <file>
 //
 // Exit status is non-zero on any validation error, and on any promoted warning
 // when --require-remote-routing is set.
@@ -16,20 +20,32 @@ import (
 	"fmt"
 	"os"
 
+	"github.com/artemnikitin/firework/internal/config"
 	"github.com/artemnikitin/firework/internal/enricher"
+	"github.com/artemnikitin/firework/internal/volume"
 )
 
 func main() {
 	inputDir := flag.String("input-dir", "", "path to the GitOps input directory to validate")
+	nodeConfig := flag.String("node-config", "", "path to a hand-authored node config to validate (direct-Git mode)")
 	requireRemoteRouting := flag.Bool("require-remote-routing", false,
 		"treat a routed service without a valid first port_forwards host port as a validation failure")
 	flag.Parse()
 
-	if *inputDir == "" {
-		fmt.Fprintln(os.Stderr, "configcheck: --input-dir is required")
+	if (*inputDir == "") == (*nodeConfig == "") {
+		fmt.Fprintln(os.Stderr, "configcheck: exactly one of --input-dir or --node-config is required")
 		os.Exit(2)
 	}
 
+	if *nodeConfig != "" {
+		if err := runNodeConfig(*nodeConfig); err != nil {
+			fmt.Fprintln(os.Stderr, "configcheck: "+err.Error())
+			os.Exit(1)
+		}
+		fmt.Println("configcheck: OK")
+		return
+	}
+
 	if err := run(*inputDir, *requireRemoteRouting); err != nil {
 		fmt.Fprintln(os.Stderr, "configcheck: "+err.Error())
 		os.Exit(1)
@@ -60,3 +76,37 @@ func run(inputDir string, requireRemoteRouting bool) error {
 	fmt.Printf("validated %d node config(s)\n", len(result.NodeConfigs))
 	return nil
 }
+
+// runNodeConfig validates a hand-authored node config. Its warnings are
+// advisory: nothing can verify a resize_generation without history, so the
+// check reports the shape that is almost always a mistake rather than failing.
+func runNodeConfig(path string) error {
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return fmt.Errorf("reading node config: %w", err)
+	}
+	nc, err := config.ParseNodeConfig(data)
+	if err != nil {
+		return err
+	}
+	// Parsing only proves the YAML is well formed. A hand-authored node config
+	// is the direct-Git equivalent of an enriched one, so it gets the same
+	// semantic validation the control plane applies before rendering —
+	// otherwise this command reports OK for a config with no node name, no
+	// image or kernel, zero compute, or a negative volume size, which defeats
+	// the point of running it in CI.
+	if err := enricher.ValidateOutput(nc); err != nil {
+		return fmt.Errorf("validation failed:\n%v", err)
+	}
+	// ValidateOutput covers generic service fields. The volume contract is
+	// enforced separately, by the agent's own rules, so a config cannot pass
+	// here and then fail to start on the node.
+	if err := volume.ValidateNodeVolumes(nc); err != nil {
+		return fmt.Errorf("validation failed:\n%v", err)
+	}
+	for _, warning := range config.NodeConfigWarnings(nc) {
+		fmt.Fprintf(os.Stderr, "warning [volume_size_without_generation]: %s\n", warning)
+	}
+	fmt.Printf("validated node config %s with %d service(s)\n", nc.Node, len(nc.Services))
+	return nil
+}

cmd/configcheck/main_test.go

@@ -1,9 +1,13 @@
 package main
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
+
+	"github.com/artemnikitin/firework/internal/config"
 )
 
 func writeTenant(t *testing.T, root, tenant, body string) {
@@ -79,3 +83,148 @@ metadata:
 		t.Fatal("expected failure with --require-remote-routing")
 	}
 }
+
+func TestRunNodeConfig_WarnsOnVolumeSizeWithoutGeneration(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "node.yaml")
+	if err := os.WriteFile(path, []byte(`node: node-1
+services:
+  - name: db
+    image: /images/db.ext4
+    kernel: /images/vmlinux
+    vcpus: 1
+    memory_mb: 512
+    volumes:
+      - name: data
+        type: local
+        mount_path: /var/lib/db
+        size_bytes: 10737418240
+        bound_node: node-1
+`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := runNodeConfig(path); err != nil {
+		t.Fatalf("an absent generation is advisory, not a failure: %v", err)
+	}
+
+	nc, err := config.ParseNodeConfig([]byte(`node: node-1
+services:
+  - name: db
+    volumes:
+      - name: data
+        type: local
+        mount_path: /var/lib/db
+        size_bytes: 10737418240
+`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	warnings := config.NodeConfigWarnings(nc)
+	if len(warnings) != 1 || !strings.Contains(warnings[0], "resize_generation") {
+		t.Fatalf("expected a resize_generation warning, got %#v", warnings)
+	}
+
+	nc.Services[0].Volumes[0].ResizeGeneration = 1
+	if got := config.NodeConfigWarnings(nc); len(got) != 0 {
+		t.Fatalf("a declared generation must not warn, got %#v", got)
+	}
+}
+
+// --node-config must validate, not just parse: a plainly invalid node config
+// reporting OK defeats the point of running this in CI.
+func TestNodeConfigIsSemanticallyValidated(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "node.yaml")
+	if err := os.WriteFile(path, []byte(`node: ""
+services:
+  - name: broken
+    vcpus: 0
+    memory_mb: 0
+    volumes:
+      - name: data
+        type: local
+        mount_path: /var/lib/db
+        size_bytes: -1
+`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := runNodeConfig(path); err == nil {
+		t.Fatal("a node config with no name, no image/kernel, zero compute and a negative volume size must not validate")
+	}
+}
+
+// ValidateOutput covers generic service fields, not the volume invariants the
+// agent enforces. A local volume with no bound_node is unusable, and
+// configcheck must say so rather than printing OK.
+func TestLocalVolumeWithoutBoundNodeIsRejected(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "node.yaml")
+	if err := os.WriteFile(path, []byte(`node: node-1
+services:
+  - name: db
+    image: /images/db.ext4
+    kernel: /images/vmlinux
+    vcpus: 1
+    memory_mb: 512
+    volumes:
+      - name: data
+        type: local
+        mount_path: /var/lib/db
+        size_bytes: 10737418240
+        resize_generation: 1
+`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := runNodeConfig(path); err == nil {
+		t.Fatal("a local volume with no bound_node is unusable and must not validate")
+	}
+}
+
+// The volume contract is checked through the agent's own rules, so the two
+// cannot drift into a config that passes CI and then fails to start.
+func TestNodeConfigVolumeContractMatchesTheAgent(t *testing.T) {
+	valid := `node: node-1
+services:
+  - name: db
+    image: /images/db.ext4
+    kernel: /images/vmlinux
+    vcpus: 1
+    memory_mb: 512
+    volumes:
+      - name: data
+        type: local
+        mount_path: %s
+        size_bytes: 10737418240
+        bound_node: %s
+        resize_generation: 1
+`
+	tests := []struct {
+		name      string
+		mountPath string
+		boundNode string
+		wantErr   bool
+	}{
+		{name: "valid", mountPath: "/var/lib/db", boundNode: "node-1"},
+		{name: "reserved mount path", mountPath: "/proc/db", boundNode: "node-1", wantErr: true},
+		{name: "relative mount path", mountPath: "var/lib/db", boundNode: "node-1", wantErr: true},
+		// A bound_node naming another node is only probably wrong — the agent
+		// matches its stable node_id, which need not equal the config key — so
+		// it warns rather than failing.
+		{name: "bound elsewhere warns only", mountPath: "/var/lib/db", boundNode: "node-2"},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			path := filepath.Join(t.TempDir(), "node.yaml")
+			if err := os.WriteFile(path, []byte(fmt.Sprintf(valid, test.mountPath, test.boundNode)), 0o644); err != nil {
+				t.Fatal(err)
+			}
+			err := runNodeConfig(path)
+			if test.wantErr && err == nil {
+				t.Fatal("expected the volume contract to reject this config")
+			}
+			if !test.wantErr && err != nil {
+				t.Fatalf("expected the config to validate, got %v", err)
+			}
+		})
+	}
+}

cmd/fc-init/main.go

@@ -380,7 +380,7 @@ func ensureWritablePaths(paths, volumePaths []string, uid, gid int) error {
 		if overlapsVolumePath(path, volumePaths) {
 			continue
 		}
-		if err := chownPathRecursive(path, uid, gid); err != nil {
+		if err := chownPathRecursive(path, volumePaths, uid, gid); err != nil {
 			if os.IsNotExist(err) {
 				continue
 			}
@@ -393,29 +393,50 @@ func ensureWritablePaths(paths, volumePaths []string, uid, gid int) error {
 	return nil
 }
 
+// overlapsVolumePath reports whether a declared writable path is at or below a
+// volume root. A path *above* a volume root does not overlap: the volume is a
+// separate filesystem mounted inside it, and skipping the parent would leave a
+// non-root guest process unable to write to its own directory.
 func overlapsVolumePath(path string, volumePaths []string) bool {
 	for _, volumePath := range volumePaths {
-		if path == volumePath || strings.HasPrefix(path, volumePath+"/") || strings.HasPrefix(volumePath, path+"/") {
+		if path == volumePath || strings.HasPrefix(path, volumePath+"/") {
 			return true
 		}
 	}
 	return false
 }
 
-func chownPathRecursive(path string, uid, gid int) error {
+// chownFn is a seam so the walk can be observed in tests without running as
+// root. Production always uses os.Lchown.
+var chownFn = os.Lchown
+
+// chownPathRecursive walks a writable path, pruning any volume mount point it
+// reaches. A mounted volume carries its own ownership on its own ext4
+// filesystem; descending into it is both wrong and potentially expensive.
+func chownPathRecursive(path string, volumePaths []string, uid, gid int) error {
 	info, err := os.Lstat(path)
 	if err != nil {
 		return err
 	}
 	if !info.IsDir() || info.Mode()&os.ModeSymlink != 0 {
-		return os.Lchown(path, uid, gid)
+		return chownFn(path, uid, gid)
 	}
 
+	pruned := make(map[string]struct{}, len(volumePaths))
+	for _, volumePath := range volumePaths {
+		pruned[volumePath] = struct{}{}
+	}
 	return filepath.WalkDir(path, func(p string, d fs.DirEntry, walkErr error) error {
 		if walkErr != nil {
 			return walkErr
 		}
-		return os.Lchown(p, uid, gid)
+		if _, isVolume := pruned[p]; isVolume {
+			if d != nil && d.IsDir() {
+				return fs.SkipDir
+			}
+			return nil
+		}
+		return chownFn(p, uid, gid)
 	})
 }