From f5c3596ef502c0b0ddee1611ab1b544b50c2196f Mon Sep 17 00:00:00 2001 From: Maksim An Date: Tue, 15 Mar 2022 15:10:46 -0700 Subject: [PATCH] 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 --- .../tools/securitypolicy/helpers/helpers.go | 8 +- pkg/securitypolicy/securitypolicy.go | 21 ++- pkg/securitypolicy/securitypolicyenforcer.go | 67 ++++----- test/cri-containerd/policy_test.go | 134 ++++++++++++++++++ .../tools/securitypolicy/helpers/helpers.go | 8 +- .../pkg/securitypolicy/securitypolicy.go | 21 ++- .../securitypolicy/securitypolicyenforcer.go | 67 ++++----- 7 files changed, 234 insertions(+), 92 deletions(-) diff --git a/internal/tools/securitypolicy/helpers/helpers.go b/internal/tools/securitypolicy/helpers/helpers.go index 42f6fd8a70..103b2bae3a 100644 --- a/internal/tools/securitypolicy/helpers/helpers.go +++ b/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 } diff --git a/pkg/securitypolicy/securitypolicy.go b/pkg/securitypolicy/securitypolicy.go index d7ec30bdd6..207a2779b3 100644 --- a/pkg/securitypolicy/securitypolicy.go +++ b/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) { diff --git a/pkg/securitypolicy/securitypolicyenforcer.go b/pkg/securitypolicy/securitypolicyenforcer.go index 3024f8f5cc..b435080e9e 100644 --- a/pkg/securitypolicy/securitypolicyenforcer.go +++ b/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//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//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//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//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//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//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//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//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)) } diff --git a/test/cri-containerd/policy_test.go b/test/cri-containerd/policy_test.go index 0a1abacbfb..d5e99e64a5 100644 --- a/test/cri-containerd/policy_test.go +++ b/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) + } + } + } + }) + } +} diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/tools/securitypolicy/helpers/helpers.go b/test/vendor/github.com/Microsoft/hcsshim/internal/tools/securitypolicy/helpers/helpers.go index 42f6fd8a70..103b2bae3a 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/tools/securitypolicy/helpers/helpers.go +++ b/test/vendor/github.com/Microsoft/hcsshim/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 } diff --git a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go index d7ec30bdd6..207a2779b3 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicy.go +++ b/test/vendor/github.com/Microsoft/hcsshim/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) { diff --git a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go b/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go index 3024f8f5cc..b435080e9e 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/pkg/securitypolicy/securitypolicyenforcer.go +++ b/test/vendor/github.com/Microsoft/hcsshim/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//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//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//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//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//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//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//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//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)) }