pr feedback: wait-path only supports paths under sandbox mounts

Per pr feedback, only container paths under sandbox mounts are
supported as wait-paths. The support for other 2 scenarios can be
added as needed.

Add positive and negative CRI tests.

Signed-off-by: Maksim An <maksiman@microsoft.com>

internal/tools/securitypolicy/helpers/helpers.go

@@ -137,7 +137,13 @@ func PolicyContainersFromConfigs(containerConfigs []securitypolicy.ContainerConf
 			workingDir = containerConfig.WorkingDir
 		}
 
-		container, err := securitypolicy.NewContainer(containerConfig.Command, layerHashes, envRules, workingDir)
+		container, err := securitypolicy.NewContainer(
+			containerConfig.Command,
+			layerHashes,
+			envRules,
+			workingDir,
+			containerConfig.ExpectedMounts,
+		)
 		if err != nil {
 			return nil, err
 		}

pkg/securitypolicy/securitypolicy.go

@@ -211,15 +211,16 @@ type ExpectedMounts struct {
 
 // NewContainer creates a new Container instance from the provided values
 // or an error if envRules validation fails.
-func NewContainer(command, layers []string, envRules []EnvRuleConfig, workingDir string) (*Container, error) {
+func NewContainer(command, layers []string, envRules []EnvRuleConfig, workingDir string, eMounts []string) (*Container, error) {
 	if err := validateEnvRules(envRules); err != nil {
 		return nil, err
 	}
 	return &Container{
-		Command:    newCommandArgs(command),
-		Layers:     newLayers(layers),
-		EnvRules:   newEnvRules(envRules),
-		WorkingDir: workingDir,
+		Command:        newCommandArgs(command),
+		Layers:         newLayers(layers),
+		EnvRules:       newEnvRules(envRules),
+		WorkingDir:     workingDir,
+		ExpectedMounts: newExpectedMounts(eMounts),
 	}, nil
 }
 
@@ -279,6 +280,16 @@ func newLayers(ls []string) Layers {
 	}
 }
 
+func newExpectedMounts(em []string) ExpectedMounts {
+	mounts := map[string]string{}
+	for i, m := range em {
+		mounts[strconv.Itoa(i)] = m
+	}
+	return ExpectedMounts{
+		Elements: mounts,
+	}
+}
+
 // Custom JSON marshalling to add `lenth` field that matches the number of
 // elements present in the `elements` field.
 func (c Containers) MarshalJSON() ([]byte, error) {

pkg/securitypolicy/securitypolicyenforcer.go

@@ -10,12 +10,10 @@ import (
 	"strings"
 	"sync"
 
-	"github.com/google/go-cmp/cmp"
-	oci "github.com/opencontainers/runtime-spec/specs-go"
-
-	"github.com/Microsoft/hcsshim/internal/guestpath"
 	"github.com/Microsoft/hcsshim/internal/hooks"
 	"github.com/Microsoft/hcsshim/pkg/annotations"
+	"github.com/google/go-cmp/cmp"
+	oci "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 type SecurityPolicyEnforcer interface {
@@ -503,15 +501,22 @@ func possibleIndicesForID(containerID string, mapping map[int]map[string]struct{
 // the expected mounts appear prior container start. At the moment enforcement
 // is expected to take place inside LCOW UVM.
 //
-// Supported scenarios:
-//   1. expected mount is provided as a path inside the sandbox, and it should resolve inside UVM
-//      e.g, "sandbox://path/on/the/host", which will correspond to "/run/gcs/c/<podID>/sandboxMounts/path/on/the/host"
-//   2. expected mount is provided as a path under a sandbox mount path inside container, e.g.,
-//      sandbox mount is at path "/sandbox/mount" and wait path is "/sandbox/mount/wait/path", which
-//      corresponds to "/run/gcs/c/<podID>/sandboxMounts/path/on/the/host/wait/path"
-//   3. expected mount is provided as an arbitrary path inside container, and it should resolve
-//      inside UVM, e.g., "/arbitrary/container/path", which corresponds to
-//      "/run/gcs/c/<containerID>/rootfs/arbitrary/container/path"
+// Expected mount is provided as a path under a sandbox mount path inside
+// container, e.g., sandbox mount is at path "/path/in/container" and wait path
+// is "/path/in/container/wait/path", which corresponds to
+// "/run/gcs/c/<podID>/sandboxMounts/path/on/the/host/wait/path"
+//
+// Iterates through container mounts to identify the correct sandbox
+// mount where the wait path is nested under. The mount spec will
+// be something like:
+// {
+//    "source": "/run/gcs/c/<podID>/sandboxMounts/path/on/host",
+//    "destination": "/path/in/container"
+// }
+// The wait path will be "/path/in/container/wait/path". To find the corresponding
+// sandbox mount do a prefix match on wait path against all container mounts
+// Destination and resolve the full path inside UVM. For example above it becomes
+// "/run/gcs/c/<podID>/sandboxMounts/path/on/host/wait/path"
 func (pe *StandardSecurityPolicyEnforcer) EnforceExpectedMountsPolicy(containerID string, spec *oci.Spec) error {
 	pe.mutex.Lock()
 	defer pe.mutex.Unlock()
@@ -550,35 +555,17 @@ func (pe *StandardSecurityPolicyEnforcer) EnforceExpectedMountsPolicy(containerI
 
 	var wPaths []string
 	for _, mount := range wMounts {
-		// By default, handle scenario #3 and resolve container path to the actual path inside UVM.
-		wp := filepath.Join(guestpath.LCOWRootPrefixInUVM, containerID, guestpath.RootfsPath, mount)
-		if strings.HasPrefix(mount, guestpath.SandboxMountPrefix) {
-			// This covers case #1, and we replace sandbox mount prefix with the sandbox
-			// mounts path inside UVM
-			sandboxPath := strings.TrimPrefix(mount, guestpath.SandboxMountPrefix)
-			wp = filepath.Join(guestpath.LCOWRootPrefixInUVM, "sandboxMounts", sandboxPath)
-		} else {
-			// This covers case #2. Iterate through container mounts to identify the
-			// correct sandbox mount where the wait path is nested under. The mount
-			// spec will be something like:
-			// {
-			//    "source": "/run/gcs/c/<podID>/sandboxMounts/path/on/host",
-			//    "destination": "/sandbox/mount"
-			// }
-			// The wait path will be "/sandbox/mount/wait/path". To find the corresponding
-			// sandbox mount do a prefix match on wait path against all container mounts Destination
-			// and resolve the full path inside UVM. For example above it becomes
-			// "/run/gcs/c/<podID>/sandboxMounts/path/on/host/wait/path"
-			for _, m := range spec.Mounts {
-				if strings.HasPrefix(mount, m.Destination) {
-					wp = filepath.Join(m.Source, strings.TrimPrefix(mount, m.Destination))
-					break
-				}
-			}
-			if wp == "" {
-				return fmt.Errorf("invalid mount path: %q", mount)
+		var wp string
+		for _, m := range spec.Mounts {
+			// prefix matching to find correct sandbox mount
+			if strings.HasPrefix(mount, m.Destination) {
+				wp = filepath.Join(m.Source, strings.TrimPrefix(mount, m.Destination))
+				break
 			}
 		}
+		if wp == "" {
+			return fmt.Errorf("invalid mount path: %q", mount)
+		}
 		wPaths = append(wPaths, filepath.Clean(wp))
 	}
 

test/cri-containerd/policy_test.go

@@ -5,6 +5,7 @@ package cri_containerd
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/Microsoft/hcsshim/internal/tools/securitypolicy/helpers"
@@ -17,6 +18,15 @@ var (
 	validPolicyAlpineCommand = []string{"ash", "-c", "echo 'Hello'"}
 )
 
+type configOpt func(*securitypolicy.ContainerConfig) error
+
+func withExpectedMounts(em []string) configOpt {
+	return func(conf *securitypolicy.ContainerConfig) error {
+		conf.ExpectedMounts = append(conf.ExpectedMounts, em...)
+		return nil
+	}
+}
+
 func securityPolicyFromContainers(containers []securitypolicy.ContainerConfig) (string, error) {
 	pc, err := helpers.PolicyContainersFromConfigs(containers)
 	if err != nil {
@@ -117,3 +127,127 @@ func Test_RunSimpleAlpineContainer_WithPolicy_Allowed(t *testing.T) {
 	startContainer(t, client, ctx, containerID)
 	stopContainer(t, client, ctx, containerID)
 }
+
+func Test_RunContainers_WithSyncHooks_Positive(t *testing.T) {
+	requireFeatures(t, featureLCOW, featureLCOWIntegrity)
+
+	type config struct {
+		name              string
+		waiterSideEffect  func(containerConfig *securitypolicy.ContainerConfig)
+		shouldError       bool
+		expectedErrString string
+	}
+
+	for _, testConfig := range []config{
+		{
+			name:             "ValidWaitPath",
+			waiterSideEffect: nil,
+			shouldError:      false,
+		},
+		{
+			// This is a long test that will wait for a timeout
+			name: "InvalidWaitPath",
+			waiterSideEffect: func(cfg *securitypolicy.ContainerConfig) {
+				cfg.ExpectedMounts = []string{"/mnt/shared/container-B/wrong-sync-file"}
+			},
+			shouldError:       true,
+			expectedErrString: "timeout while waiting for path",
+		},
+	} {
+		t.Run(testConfig.name, func(t *testing.T) {
+			// create container #1 that writes a file
+			touchCmdArgs := []string{"ash", "-c", "touch /mnt/shared/container-A/sync-file && while true; do echo hello; sleep 1; done"}
+			configWriter := securitypolicy.ContainerConfig{
+				ImageName: "alpine:latest",
+				Command:   touchCmdArgs,
+			}
+			// create container #2 that waits for a path to appear
+			echoCmdArgs := []string{"ash", "-c", "while true; do echo hello2; sleep 1; done"}
+			configWaiter := securitypolicy.ContainerConfig{
+				ImageName:      "alpine:latest",
+				Command:        echoCmdArgs,
+				ExpectedMounts: []string{"/mnt/shared/container-B/sync-file"},
+			}
+			if testConfig.waiterSideEffect != nil {
+				testConfig.waiterSideEffect(&configWaiter)
+			}
+
+			// create appropriate policies for the two containers
+			containerConfigs := append(helpers.DefaultContainerConfigs(), configWriter, configWaiter)
+			policyContainers, err := helpers.PolicyContainersFromConfigs(containerConfigs)
+			if err != nil {
+				t.Fatalf("failed to create security policy containers: %s", err)
+			}
+			policy := securitypolicy.NewSecurityPolicy(false, policyContainers)
+			policyString, err := policy.EncodeToString()
+			if err != nil {
+				t.Fatalf("failed to generate security policy string: %s", err)
+			}
+
+			client := newTestRuntimeClient(t)
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			// create pod with security policy
+			podRequest := sandboxRequestWithPolicy(t, policyString)
+			podID := runPodSandbox(t, client, ctx, podRequest)
+			defer removePodSandbox(t, client, ctx, podID)
+			defer stopPodSandbox(t, client, ctx, podID)
+
+			sbMountWriter := v1alpha2.Mount{
+				HostPath:      "sandbox://host/path",
+				ContainerPath: "/mnt/shared/container-A",
+				Readonly:      false,
+			}
+			// start containers async and make sure that both of the start
+			requestWriter := getCreateContainerRequest(
+				podID,
+				"alpine-writer",
+				"alpine:latest",
+				touchCmdArgs,
+				podRequest.Config,
+			)
+			requestWriter.Config.Mounts = append(requestWriter.Config.Mounts, &sbMountWriter)
+
+			sbMountWaiter := v1alpha2.Mount{
+				HostPath:      "sandbox://host/path",
+				ContainerPath: "/mnt/shared/container-B",
+				Readonly:      false,
+			}
+			requestWaiter := getCreateContainerRequest(
+				podID,
+				"alpine-waiter",
+				"alpine:latest",
+				echoCmdArgs,
+				podRequest.Config,
+			)
+			requestWaiter.Config.Mounts = append(requestWaiter.Config.Mounts, &sbMountWaiter)
+
+			cidWriter := createContainer(t, client, ctx, requestWriter)
+			cidWaiter := createContainer(t, client, ctx, requestWaiter)
+
+			startContainer(t, client, ctx, cidWriter)
+			defer removeContainer(t, client, ctx, cidWriter)
+			defer stopContainer(t, client, ctx, cidWriter)
+
+			if !testConfig.shouldError {
+				startContainer(t, client, ctx, cidWaiter)
+				defer removeContainer(t, client, ctx, cidWaiter)
+				defer stopContainer(t, client, ctx, cidWaiter)
+			} else {
+				_, err := client.StartContainer(ctx, &v1alpha2.StartContainerRequest{
+					ContainerId: cidWaiter,
+				})
+				if err == nil {
+					defer removeContainer(t, client, ctx, cidWaiter)
+					defer stopContainer(t, client, ctx, cidWaiter)
+					t.Fatalf("should fail, succeeded instead")
+				} else {
+					if !strings.Contains(err.Error(), testConfig.expectedErrString) {
+						t.Fatalf("expected error: %q, got: %q", testConfig.expectedErrString, err)
+					}
+				}
+			}
+		})
+	}
+}