Prevent failed host-dir VMs from restarting (#463)

* Fix host directory policy boundary checks

* Fix host directory policy test lint

.golangci.yml

@@ -9,7 +9,6 @@ formatters:
     - gofmt
     - gofumpt
     - goimports
-    - golines
     - swaggo
 
 linters:
@@ -41,6 +40,7 @@ linters:
 
     # Style linters that are total nuts.
     - wsl
+    - wsl_v5
     - funlen
 
     # Enough parallelism for now.
@@ -70,6 +70,18 @@ linters:
     # Not all errors need to be checked
     - errcheck
 
+    # It's OK to not initialize some struct fields
+    - exhaustruct
+
+    # We'll control the variable name length ourselves
+    - varnamelen
+
+    # Inline error handling keeps assignment and checking together
+    - noinlineerr
+
+    # Avoid unrelated style churn for now
+    - funcorder
+
 issues:
   # Don't hide multiple issues that belong to one class since GitHub annotations can handle them all nicely.
   max-issues-per-linter: 0

api/openapi.yaml

@@ -791,7 +791,9 @@ components:
             model: macstudio
         hostDirs:
           type: array
-          description: Directories on the Orchard Worker host to mount to a VM
+          description: |
+            Directories on the Orchard Worker host to mount to a VM.
+            Requires running Orchard Controller with `--insecure-allow-host-dirs`.
           items:
             type: object
             properties:

internal/command/controller/run.go

@@ -30,6 +30,7 @@ var addressPprof string
 var debug bool
 var noTLS bool
 var sshNoClientAuth bool
+var insecureAllowHostDirs bool
 var experimentalRPCV2 bool
 var noExperimentalRPCV2 bool
 var experimentalPingInterval time.Duration
@@ -74,6 +75,8 @@ func newRunCommand() *cobra.Command {
 	cmd.Flags().BoolVar(&sshNoClientAuth, "insecure-ssh-no-client-auth", false,
 		"allow SSH clients to connect to the controller's SSH server without authentication, "+
 			"thus only authenticating on the target worker/VM's SSH server")
+	cmd.Flags().BoolVar(&insecureAllowHostDirs, "insecure-allow-host-dirs", false,
+		"allow unsafe path-based local host directory sharing")
 	cmd.Flags().BoolVar(&experimentalRPCV2, "experimental-rpc-v2", false,
 		"enable experimental RPC v2 (https://github.com/cirruslabs/orchard/issues/235)")
 	_ = cmd.Flags().MarkHidden("experimental-rpc-v2")
@@ -166,6 +169,10 @@ func runController(cmd *cobra.Command, args []string) (err error) {
 		controllerOpts = append(controllerOpts, controller.WithSynthetic())
 	}
 
+	if insecureAllowHostDirs {
+		controllerOpts = append(controllerOpts, controller.WithInsecureAllowHostDirs())
+	}
+
 	var controllerCert tls.Certificate
 
 	if !noTLS {

internal/command/dev/dev.go

@@ -35,6 +35,7 @@ var experimentalRPCV2 bool
 var addressPprof string
 var synthetic bool
 var workers int
+var insecureAllowHostDirs bool
 
 func NewCommand() *cobra.Command {
 	command := &cobra.Command{
@@ -57,6 +58,8 @@ func NewCommand() *cobra.Command {
 	command.Flags().BoolVar(&synthetic, "synthetic", false,
 		"do not instantiate real Tart VM, use synthetic in-memory VMs suitable for load testing")
 	command.Flags().IntVar(&workers, "workers", 1, "number of workers to start")
+	command.Flags().BoolVar(&insecureAllowHostDirs, "insecure-allow-host-dirs", false,
+		"allow unsafe path-based local host directory sharing")
 
 	return command
 }
@@ -105,6 +108,10 @@ func runDev(cmd *cobra.Command, args []string) error {
 		additionalControllerOpts = append(additionalControllerOpts, controller.WithExperimentalRPCV2())
 	}
 
+	if insecureAllowHostDirs {
+		additionalControllerOpts = append(additionalControllerOpts, controller.WithInsecureAllowHostDirs())
+	}
+
 	group, ctx := errgroup.WithContext(cmd.Context())
 
 	var additionalWorkerOpts []worker.Option

internal/controller/api_vms.go

@@ -158,6 +158,10 @@ func (controller *Controller) updateVMSpec(ctx *gin.Context) responder.Responder
 		return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided"))
 	}
 
+	if responder := controller.validateHostDirs(userVM.HostDirs); responder != nil {
+		return responder
+	}
+
 	name := ctx.Param("name")
 
 	return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder {
@@ -540,6 +544,14 @@ func (controller *Controller) validateHostDirs(hostDirs []v1.HostDir) responder.
 		return nil
 	}
 
+	if !controller.insecureAllowHostDirs {
+		return responder.JSON(
+			http.StatusBadRequest,
+			NewErrorResponse("host directory sharing is disabled; "+
+				"restart the controller with --insecure-allow-host-dirs to enable this unsafe feature"),
+		)
+	}
+
 	// Retrieve cluster settings
 	var clusterSettings *v1.ClusterSettings
 	var err error

internal/controller/controller.go

@@ -46,6 +46,7 @@ type Controller struct {
 	listener                           net.Listener
 	httpServer                         *http.Server
 	insecureAuthDisabled               bool
+	insecureAllowHostDirs              bool
 	scheduler                          *scheduler.Scheduler
 	store                              storepkg.Store
 	logger                             *zap.SugaredLogger
@@ -195,6 +196,14 @@ func New(opts ...Option) (*Controller, error) {
 		return nil, err
 	}
 
+	// When no "--insecure-allow-host-dirs" is present,
+	// fail the VMs that have "hostDirs" set
+	if !controller.insecureAllowHostDirs {
+		if err := controller.failVMsWithHostDirs(); err != nil {
+			return nil, err
+		}
+	}
+
 	return controller, nil
 }
 
@@ -305,6 +314,34 @@ func (controller *Controller) DeleteServiceAccount(name string) error {
 	})
 }
 
+func (controller *Controller) failVMsWithHostDirs() error {
+	return controller.store.Update(func(txn storepkg.Transaction) error {
+		vms, err := txn.ListVMs()
+		if err != nil {
+			return err
+		}
+
+		for _, vm := range vms {
+			permanentlyFailed := vm.TerminalState() &&
+				vm.RestartPolicy == v1.RestartPolicyNever
+
+			if permanentlyFailed || len(vm.HostDirs) == 0 {
+				continue
+			}
+
+			vm.Status = v1.VMStatusFailed
+			vm.StatusMessage = "host directories are used, but host directory sharing is disabled"
+			vm.RestartPolicy = v1.RestartPolicyNever
+
+			if err := txn.SetVM(vm); err != nil {
+				return err
+			}
+		}
+
+		return nil
+	})
+}
+
 func (controller *Controller) Run(ctx context.Context) error {
 	// Run the scheduler so that each VM will eventually
 	// be assigned to a specific Worker

internal/controller/option.go

@@ -48,6 +48,12 @@ func WithInsecureAuthDisabled() Option {
 	}
 }
 
+func WithInsecureAllowHostDirs() Option {
+	return func(controller *Controller) {
+		controller.insecureAllowHostDirs = true
+	}
+}
+
 func WithSwaggerDocs() Option {
 	return func(controller *Controller) {
 		controller.enableSwaggerDocs = true

internal/tests/integration_test.go

@@ -372,12 +372,26 @@ func TestVMGarbageCollection(t *testing.T) {
 	}), "failed to wait for the VM %s to be garbage-collected", vmName)
 }
 
+func TestHostDirsDisabledByDefault(t *testing.T) {
+	devClient, _, _ := devcontroller.StartIntegrationTestEnvironment(t)
+
+	err := devClient.VMs().Create(context.Background(), &v1.VM{
+		Meta:     v1.Meta{Name: "test-host-dirs-disabled"},
+		Image:    imageconstant.DefaultMacosImage,
+		HostDirs: []v1.HostDir{{Name: "src", Path: "/Users/ci/src"}},
+	})
+	require.Error(t, err)
+}
+
 func TestHostDirs(t *testing.T) {
 	if runtime.GOOS != "darwin" {
 		t.Skip("HostDirs is only supported on macOS with Tart")
 	}
 
-	devClient, _, _ := devcontroller.StartIntegrationTestEnvironment(t)
+	devClient, _, _ := devcontroller.StartIntegrationTestEnvironmentWithAdditionalOpts(t,
+		false, []controller.Option{controller.WithInsecureAllowHostDirs()},
+		false, nil,
+	)
 
 	dirToMount := t.TempDir()
 
@@ -449,7 +463,10 @@ func TestHostDirsInvalidPolicy(t *testing.T) {
 		t.Skip("HostDirs is only supported on macOS with Tart")
 	}
 
-	devClient, _, _ := devcontroller.StartIntegrationTestEnvironment(t)
+	devClient, _, _ := devcontroller.StartIntegrationTestEnvironmentWithAdditionalOpts(t,
+		false, []controller.Option{controller.WithInsecureAllowHostDirs()},
+		false, nil,
+	)
 
 	dirToMount := t.TempDir()
 

pkg/resource/v1/host_dir_policy.go

@@ -58,10 +58,10 @@ func (policy HostDirPolicy) Validate(path string, readOnly bool) bool {
 		return false
 	}
 
-	return strings.HasPrefix(
-		strings.TrimSuffix(path, "/"),
-		strings.TrimSuffix(policy.PathPrefix, "/"),
-	)
+	path = strings.TrimSuffix(path, "/")
+	pathPrefix := strings.TrimSuffix(policy.PathPrefix, "/")
+
+	return path == pathPrefix || strings.HasPrefix(path, pathPrefix+"/")
 }
 
 func (policy HostDirPolicy) String() string {

pkg/resource/v1/host_dir_policy_test.go

@@ -44,6 +44,107 @@ func TestHostDirPolicyValidate(t *testing.T) {
 	require.False(t, policy.Validate("/..", true))
 }
 
+func TestHostDirPolicyValidatePathBoundary(t *testing.T) {
+	const (
+		localPathPrefix = "/src/"
+		githubURLPrefix = "https://github.com"
+	)
+
+	testCases := []struct {
+		name       string
+		pathPrefix string
+		path       string
+		allowed    bool
+	}{
+		{
+			name:       "local policy allows its exact path",
+			pathPrefix: localPathPrefix,
+			path:       "/src",
+			allowed:    true,
+		},
+		{
+			name:       "local policy allows descendants",
+			pathPrefix: localPathPrefix,
+			path:       "/src/project",
+			allowed:    true,
+		},
+		{
+			name:       "local policy without trailing slash rejects sibling sharing its prefix",
+			pathPrefix: "/src",
+			path:       "/src-private",
+			allowed:    false,
+		},
+		{
+			name:       "local policy rejects sibling sharing its prefix",
+			pathPrefix: localPathPrefix,
+			path:       "/src-private",
+			allowed:    false,
+		},
+		{
+			name:       "local root policy allows descendants",
+			pathPrefix: "/",
+			path:       "/src/project",
+			allowed:    true,
+		},
+		{
+			name:       "local root policy rejects remote URLs",
+			pathPrefix: "/",
+			path:       "https://github.com/archive.tar.gz",
+			allowed:    false,
+		},
+		{
+			name:       "URL policy allows its exact host",
+			pathPrefix: githubURLPrefix + "/",
+			path:       githubURLPrefix,
+			allowed:    true,
+		},
+		{
+			name:       "URL policy allows paths on its host",
+			pathPrefix: githubURLPrefix,
+			path:       "https://github.com/actions/archive.tar.gz",
+			allowed:    true,
+		},
+		{
+			name:       "URL policy rejects lookalike host",
+			pathPrefix: githubURLPrefix,
+			path:       "https://github.com.attacker.com/archive.tar.gz",
+			allowed:    false,
+		},
+		{
+			name:       "URL policy with trailing slash rejects lookalike host",
+			pathPrefix: githubURLPrefix + "/",
+			path:       "https://github.com.attacker.com/archive.tar.gz",
+			allowed:    false,
+		},
+		{
+			name:       "URL policy rejects host concealed by userinfo",
+			pathPrefix: githubURLPrefix,
+			path:       "https://github.com@attacker.example/archive.tar.gz",
+			allowed:    false,
+		},
+		{
+			name:       "URL path policy allows descendants",
+			pathPrefix: "https://github.com/actions",
+			path:       "https://github.com/actions/runner/archive.tar.gz",
+			allowed:    true,
+		},
+		{
+			name:       "URL path policy rejects sibling sharing its prefix",
+			pathPrefix: "https://github.com/actions",
+			path:       "https://github.com/actions-private/archive.tar.gz",
+			allowed:    false,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			policy := v1.HostDirPolicy{PathPrefix: testCase.pathPrefix, ReadOnly: false}
+
+			require.Equal(t, testCase.allowed, policy.Validate(testCase.path, false))
+		})
+	}
+}
+
 func TestHostDirPolicyValidateReadOnly(t *testing.T) {
 	policy := &v1.HostDirPolicy{PathPrefix: "/Users/ci/src", ReadOnly: true}