Improve validatation for networks.yaml settings

Signed-off-by: Jan Dubois <[email protected]>

Author: jandubois

cmd/limactl/sudoers.go

@@ -16,8 +16,9 @@ func newSudoersCommand() *cobra.Command {
 		Args:  cobra.MaximumNArgs(1),
 		RunE:  sudoersAction,
 	}
+	configFile, _ := networks.ConfigFile()
 	sudoersCommand.Flags().Bool("check", false,
-		"check that the sudoers file is up-to-date with $LIMA_HOME/_config/networks.yaml")
+		fmt.Sprintf("check that the sudoers file is up-to-date with %q", configFile))
 	return sudoersCommand
 }
 
@@ -30,39 +31,44 @@ func sudoersAction(cmd *cobra.Command, args []string) error {
 		return err
 	}
 	if check {
-		var file string
-		switch len(args) {
-		case 0:
-			config, err := networks.Config()
-			if err != nil {
-				return err
-			}
-			file = config.Paths.Sudoers
-			if file == "" {
-				return errors.New("no sudoers file defined in ~/.lima/_config/networks.yaml")
-			}
-		case 1:
-			file = args[0]
-		default:
-			return errors.New("can check only a single sudoers file")
-		}
+		return verifySudoAccess(args)
+	}
+	sudoers, err := networks.Sudoers()
+	if err != nil {
+		return err
+	}
+	fmt.Print(sudoers)
+	return nil
+}
+
+func verifySudoAccess(args []string) error {
+	var file string
+	switch len(args) {
+	case 0:
 		config, err := networks.Config()
 		if err != nil {
 			return err
 		}
-		if err := config.Validate(); err != nil {
-			return err
-		}
-		if err := config.VerifySudoAccess(file); err != nil {
-			return err
+		file = config.Paths.Sudoers
+		if file == "" {
+			configFile, _ := networks.ConfigFile()
+			return fmt.Errorf("no sudoers file defined in %q", configFile)
 		}
-		fmt.Printf("%q is up-to-date (or sudo doesn't require a password)\n", file)
-		return nil
+	case 1:
+		file = args[0]
+	default:
+		return errors.New("can check only a single sudoers file")
 	}
-	sudoers, err := networks.Sudoers()
+	config, err := networks.Config()
 	if err != nil {
 		return err
 	}
-	fmt.Print(sudoers)
+	if err := config.Validate(); err != nil {
+		return err
+	}
+	if err := config.VerifySudoAccess(file); err != nil {
+		return err
+	}
+	fmt.Printf("%q is up-to-date (or sudo doesn't require a password)\n", file)
 	return nil
-}
+}

pkg/networks/config.go

@@ -23,20 +23,28 @@ var cache struct {
 	err    error
 }
 
-// load() loads the _config/networks.yaml file.
-func load() {
+func ConfigFile() (string, error) {
+	configDir, err := dirnames.LimaConfigDir()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(configDir, filenames.NetworksConfig), nil
+}
+
+// loadCache loads the _config/networks.yaml file into the cache.
+func loadCache() {
 	cache.Do(func() {
-		var configDir string
-		configDir, cache.err = dirnames.LimaConfigDir()
+		var configFile string
+		configFile, cache.err = ConfigFile()
 		if cache.err != nil {
 			return
 		}
-		configFile := filepath.Join(configDir, filenames.NetworksConfig)
 		_, cache.err = os.Stat(configFile)
 		if cache.err != nil {
 			if !errors.Is(cache.err, os.ErrNotExist) {
 				return
 			}
+			configDir := filepath.Dir(configFile)
 			cache.err = os.MkdirAll(configDir, 0700)
 			if cache.err != nil {
 				cache.err = fmt.Errorf("could not create %q directory: %w", configDir, cache.err)
@@ -61,15 +69,15 @@ func load() {
 
 // Config returns the network config from the _config/networks.yaml file.
 func Config() (NetworksConfig, error) {
-	if runtime.GOOS == "darwin" {
-		load()
-		return cache.config, cache.err
+	if runtime.GOOS != "darwin" {
+		return NetworksConfig{}, errors.New("networks.yaml configuration is only supported on macOS right now")
 	}
-	return NetworksConfig{}, errors.New("networks.yaml configuration is only supported on macOS right now")
+	loadCache()
+	return cache.config, cache.err
 }
 
 func VDESock(name string) (string, error) {
-	load()
+	loadCache()
 	if cache.err != nil {
 		return "", cache.err
 	}

pkg/networks/reconcile/reconcile.go

@@ -9,6 +9,8 @@ import (
 	"runtime"
 	"strings"
 	"sync"
+	"syscall"
+	"time"
 
 	"github.com/lima-vm/lima/pkg/networks"
 	"github.com/lima-vm/lima/pkg/store"
@@ -24,12 +26,10 @@ func Reconcile(ctx context.Context, newInst string) error {
 	if err != nil {
 		return err
 	}
-
 	instances, err := store.Instances()
 	if err != nil {
 		return err
 	}
-
 	activeNetwork := make(map[string]bool, 3)
 	for _, instName := range instances {
 		instance, err := store.Inspect(instName)
@@ -80,17 +80,50 @@ func sudo(user, group, command string) error {
 	return nil
 }
 
-func startDaemon(config *networks.NetworksConfig, ctx context.Context, name, daemon string) error {
+func makeVarRun(config *networks.NetworksConfig) error {
 	err := sudo("root", "wheel", config.MkdirCmd())
 	if err != nil {
 		return err
 	}
 
-	networksDir, _ := dirnames.LimaNetworksDir()
+	// Check that VarRun is daemon-group writable. If we don't report it here, the error would only be visible
+	// in the vde_switch daemon log. This has not been checked by networks.Validate() because only the VarRun
+	// directory itself needs to be daemon-group writable, any parents just need to be daemon-group executable.
+	fi, err := os.Stat(config.Paths.VarRun)
+	if err != nil {
+		return err
+	}
+	stat, ok := fi.Sys().(*syscall.Stat_t)
+	if !ok {
+		// should never happen
+		return fmt.Errorf("could not retrieve stat buffer for %q", config.Paths.VarRun)
+	}
+	if fi.Mode()&020 == 0 || stat.Gid != networks.DaemonGID {
+		return fmt.Errorf("%q doesn't seem to be writable by the daemon (gid:%d) group",
+			config.Paths.VarRun, networks.DaemonGID)
+	}
+	return nil
+}
+
+func startDaemon(config *networks.NetworksConfig, ctx context.Context, name, daemon string) error {
+	if err := makeVarRun(config); err != nil {
+		return err
+	}
+	networksDir, err := dirnames.LimaNetworksDir()
+	if err != nil {
+		return err
+	}
 	if err := os.MkdirAll(networksDir, 0755); err != nil {
 		return err
 	}
 
+	args := []string{"--user", config.DaemonUser(daemon), "--group", config.DaemonGroup(daemon), "--non-interactive"}
+	args = append(args, strings.Split(config.StartCmd(name, daemon), " ")...)
+	cmd := exec.CommandContext(ctx, "sudo", args...)
+	// set directory to a path the daemon user has read access to because vde_switch calls getcwd() which
+	// can fail when called from directories like ~/Downloads, which has 700 permissions
+	cmd.Dir = config.Paths.VarRun
+
 	stdoutPath := config.LogFile(name, daemon, "stdout")
 	stderrPath := config.LogFile(name, daemon, "stderr")
 	if err := os.RemoveAll(stdoutPath); err != nil {
@@ -99,25 +132,16 @@ func startDaemon(config *networks.NetworksConfig, ctx context.Context, name, dae
 	if err := os.RemoveAll(stderrPath); err != nil {
 		return err
 	}
-	stdoutW, err := os.Create(stdoutPath)
+
+	cmd.Stdout, err = os.Create(stdoutPath)
 	if err != nil {
 		return err
 	}
-	// no defer stdoutW.Close()
-	stderrW, err := os.Create(stderrPath)
+	cmd.Stderr, err = os.Create(stderrPath)
 	if err != nil {
 		return err
 	}
-	// no defer stderrW.Close()
 
-	args := []string{"--user", config.DaemonUser(daemon), "--group", config.DaemonGroup(daemon), "--non-interactive"}
-	args = append(args, strings.Split(config.StartCmd(name, daemon), " ")...)
-	cmd := exec.CommandContext(ctx, "sudo", args...)
-	// set directory to a path the daemon user has read access to because vde_switch calls getcwd() which
-	// can fail when called from directories like ~/Downloads, which has 700 permissions
-	cmd.Dir = config.Paths.VarRun
-	cmd.Stdout = stdoutW
-	cmd.Stderr = stderrW
 	logrus.Debugf("Starting %q daemon for %q network: %v", daemon, name, cmd.Args)
 	if err := cmd.Start(); err != nil {
 		return err
@@ -174,7 +198,20 @@ func stopNetwork(config *networks.NetworksConfig, name string) error {
 				return err
 			}
 		}
-		// TODO: wait for VMNet to terminate before stopping Switch, otherwise the socket may not get deleted
+		// wait for VMNet to terminate (up to 5s) before stopping Switch, otherwise the socket may not get deleted
+		if daemon == networks.VMNet {
+			startWaiting := time.Now()
+			for {
+				if pid, _ := store.ReadPIDFile(config.PIDFile(name, daemon)); pid == 0 {
+					break
+				}
+				if time.Since(startWaiting) > 5 * time.Second {
+					logrus.Infof("%q daemon for %q network still running after 5 seconds", daemon, name)
+					break
+				}
+				time.Sleep(500 * time.Millisecond)
+			}
+		}
 	}
 	return nil
 }

pkg/networks/validate.go

@@ -25,7 +25,7 @@ func (config *NetworksConfig) Validate() error {
 		if name == "varRun" {
 			path = findBaseDirectory(path)
 		}
-		err := validatePath(path)
+		err := validatePath(path, name == "varRun")
 		if err != nil {
 			// sudoers file does not need to exist; otherwise `limactl sudoers` couldn't bootstrap
 			if name == "sudoers" && errors.Is(err, os.ErrNotExist) {
@@ -34,21 +34,27 @@ func (config *NetworksConfig) Validate() error {
 			return fmt.Errorf("networks.yaml field `paths.%s` error: %w", name, err)
 		}
 	}
-	// TODO: validate network definitions
+	// TODO(jandubois): validate network definitions
 	return nil
 }
 
 // findBaseDirectory removes non-existing directories from the end of the path.
 func findBaseDirectory(path string) string {
-	if _, err := os.Lstat(path); errors.Is(err, os.ErrNotExist)	{
+	if _, err := os.Lstat(path); errors.Is(err, os.ErrNotExist) {
 		if path != "/" {
 			return findBaseDirectory(filepath.Dir(path))
 		}
 	}
 	return path
 }
 
-func validatePath(path string) error {
+const (
+	RootUID   = 0
+	WheelGID  = 0
+	DaemonGID = 1
+)
+
+func validatePath(path string, allowDaemonGroupWritable bool) error {
 	if path == "" {
 		return nil
 	}
@@ -67,28 +73,35 @@ func validatePath(path string) error {
 		file = "dir"
 	}
 	// TODO: should we allow symlinks when both the link and the target are secure?
-	// E.g. on macOS /var is a symlink to /private/var
+	// E.g. on macOS /var is a symlink to /private/var, /etc to /private/etc
 	if (fi.Mode() & fs.ModeSymlink) != 0 {
 		return fmt.Errorf("%s %q is a symlink", file, path)
 	}
-	if stat, ok := fi.Sys().(*syscall.Stat_t); ok {
-		if stat.Uid != 0 {
-			return fmt.Errorf("%s %q is not owned by 0 (root), but by %d", file, path, stat.Uid)
-		}
-		if (fi.Mode() & 020) != 0 {
-			if stat.Gid != 0 && stat.Gid != 1 {
-				return fmt.Errorf("%s %q is group-writable and group is neither 0 (wheel) nor 1 (daemon), but is %d", file, path, stat.Gid)
-			}
-		}
-		if (fi.Mode() & 002) != 0 {
-			return fmt.Errorf("%s %q is world-writable", file, path)
-		}
-	} else {
+	stat, ok := fi.Sys().(*syscall.Stat_t)
+	if !ok {
 		// should never happen
 		return fmt.Errorf("could not retrieve stat buffer for %q", path)
 	}
+	if stat.Uid != RootUID {
+		return fmt.Errorf(`%s %q is not owned by root (uid: %d), but by uid %d`, file, path, RootUID, stat.Uid)
+	}
+	if allowDaemonGroupWritable {
+		if fi.Mode()&020 != 0 && stat.Gid != WheelGID && stat.Gid != DaemonGID {
+			return fmt.Errorf(`%s %q is group-writable and group is neither "wheel" (gid: %d) nor "daemon" (guid: %d), but is gid: %d`,
+				file, path, WheelGID, DaemonGID, stat.Gid)
+		}
+		if fi.Mode().IsDir() && fi.Mode()&1 == 0 && (fi.Mode()&0010 == 0 || stat.Gid != DaemonGID) {
+			return fmt.Errorf(`%s %q is not executable by the "daemon" (gid: %d)" group`, file, path, DaemonGID)
+		}
+	} else if fi.Mode()&020 != 0 && stat.Gid != WheelGID {
+		return fmt.Errorf(`%s %q is group-writable and group is not "wheel" (gid: %d), but is gid: %d`,
+			file, path, WheelGID, stat.Gid)
+	}
+	if fi.Mode()&002 != 0 {
+		return fmt.Errorf("%s %q is world-writable", file, path)
+	}
 	if path != "/" {
-		return validatePath(filepath.Dir(path))
+		return validatePath(filepath.Dir(path), allowDaemonGroupWritable)
 	}
 	return nil
 }