diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index 6a8f5ef62c..6f2d3b2d5f 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -201,7 +201,6 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques parent.Close() return nil, err } - } else if oci.IsJobContainer(s) { // If we're making a job container fake a task (i.e reuse the wcowPodSandbox logic) p.sandboxTask = newWcowPodSandboxTask(ctx, events, req.ID, req.Bundle, parent, "") @@ -223,7 +222,7 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques }); err != nil { return nil, err } - p.jobContainer = true + return &p, nil } else if !isWCOW { return nil, errors.Wrap(errdefs.ErrFailedPrecondition, "oci spec does not contain WCOW or LCOW spec") @@ -338,11 +337,6 @@ type pod struct { // It MUST be treated as read only in the lifetime of the pod. host *uvm.UtilityVM - // jobContainer specifies whether this pod is for WCOW job containers only. - // - // It MUST be treated as read only in the lifetime of the pod. - jobContainer bool - // spec is the OCI runtime specification for the pod sandbox container. spec *specs.Spec @@ -367,24 +361,9 @@ func (p *pod) CreateTask(ctx context.Context, req *task.CreateTaskRequest, s *sp return nil, errors.Wrapf(errdefs.ErrAlreadyExists, "task with id: '%s' already exists id pod: '%s'", req.ID, p.id) } - if p.jobContainer { - // This is a short circuit to make sure that all containers in a pod will have - // the same IP address/be added to the same compartment. - // - // There will need to be OS work needed to support this scenario, so for now we need to block on - // this. - if !oci.IsJobContainer(s) { - return nil, errors.New("cannot create a normal process isolated container if the pod sandbox is a job container") - } - // Pass through some annotations from the pod spec that if specified will need to be made available - // to every container as well. Kubernetes only passes annotations to RunPodSandbox so there needs to be - // a way for individual containers to get access to these. - oci.SandboxAnnotationsPassThrough( - p.spec.Annotations, - s.Annotations, - annotations.HostProcessInheritUser, - annotations.HostProcessRootfsLocation, - ) + err = p.updateConfigForHostProcessContainer(s) + if err != nil { + return nil, err } ct, sid, err := oci.GetSandboxTypeAndID(s.Annotations) @@ -501,3 +480,49 @@ func (p *pod) DeleteTask(ctx context.Context, tid string) error { return nil } + +func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { + if !oci.IsIsolatedJobContainer(p.spec) && oci.IsIsolatedJobContainer(s) { + return errors.New("cannot create a host process container inside sandbox which has missing annotation: microsoft.com/hostprocess-container") + } + + isProcessIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) + isHypervisorIsolatedPrivilegedSandbox := oci.IsIsolatedJobContainer(p.spec) + isProcessIsolatedPrivilegedContainer := oci.IsJobContainer(s) + isHypervisorIsolatedPrivilegedContainer := oci.IsIsolatedJobContainer(s) + + if isProcessIsolatedPrivilegedSandbox || (isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer) { + if isProcessIsolatedPrivilegedSandbox && !isProcessIsolatedPrivilegedContainer { + // This is a short circuit to make sure that all containers in a pod will have + // the same IP address/be added to the same compartment. + // + // There will need to be OS work needed to support this scenario, so for now we need to block on + // this. + + // IsJobContainer returns true if there was no hypervisor isolation and request was for HPCs. + // Therefore, in this request we check if the pod was a process isolated HPC pod but the + // container spec is not job container spec. + return errors.New("cannot create a normal process isolated container if the pod sandbox is a job container running on host") + } + + // Pass through some annotations from the pod spec that if specified will need to be made available + // to every container as well. Kubernetes only passes annotations to RunPodSandbox so there needs to be + // a way for individual containers to get access to these. + oci.SandboxAnnotationsPassThrough( + p.spec.Annotations, + s.Annotations, + annotations.HostProcessInheritUser, + annotations.HostProcessRootfsLocation, + ) + } + + if isHypervisorIsolatedPrivilegedSandbox && + !isHypervisorIsolatedPrivilegedContainer && + s.Annotations[annotations.HostProcessInheritUser] != "" { + // If the hypervisor isolated sandbox was privileged but the container is non-privileged, then + // we will explicitly disable the annotation which allows privileged user with the exec. + s.Annotations[annotations.HostProcessInheritUser] = "false" + } + + return nil +} diff --git a/cmd/containerd-shim-runhcs-v1/pod_test.go b/cmd/containerd-shim-runhcs-v1/pod_test.go index 816bd800ed..4423108b8f 100644 --- a/cmd/containerd-shim-runhcs-v1/pod_test.go +++ b/cmd/containerd-shim-runhcs-v1/pod_test.go @@ -6,10 +6,12 @@ import ( "context" "fmt" "math/rand" + "reflect" "strconv" "sync" "testing" + "github.com/Microsoft/hcsshim/pkg/annotations" task "github.com/containerd/containerd/api/runtime/task/v2" "github.com/containerd/errdefs" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -388,3 +390,157 @@ func Test_pod_DeleteTask_TaskID_Not_Created(t *testing.T) { err := p.KillTask(context.Background(), strconv.Itoa(rand.Int()), "", 0xf, true) verifyExpectedError(t, nil, err, errdefs.ErrNotFound) } + +func getWCOWSpecsWithAnnotations(annotation map[string]string, isIsolated bool, processUser string) *specs.Spec { + spec := &specs.Spec{ + Windows: &specs.Windows{}, + Annotations: annotation, + Process: &specs.Process{ + User: specs.User{Username: processUser}, + }, + } + + if isIsolated { + spec.Windows.HyperV = &specs.WindowsHyperV{} + } + return spec +} + +func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) { + testCases := []struct { + testName string + podSpec *specs.Spec + containerSpec *specs.Spec + expectedContainerSpec *specs.Spec + expectedError string + }{ + { + testName: "privileged container in unprivileged pod (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + expectedContainerSpec: nil, + expectedError: "cannot create a host process container inside sandbox which has missing annotation: microsoft.com/hostprocess-container", + }, + { + testName: "privileged container in privileged pod (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + expectedError: "", + }, + { + testName: "normal container in privileged pod (process hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, false, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, false, ""), + expectedContainerSpec: nil, + expectedError: "cannot create a normal process isolated container if the pod sandbox is a job container running on host", + }, + { + testName: "normal container in privileged pod (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, false, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, false, ""), + expectedError: "", + }, + { + testName: "annotations passthrough (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + annotations.HostProcessRootfsLocation: "C:\\test", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + annotations.HostProcessRootfsLocation: "C:\\test", + }, true, ""), + expectedError: "", + }, + { + testName: "annotations passthrough (process hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + annotations.HostProcessRootfsLocation: "C:\\test", + }, false, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, false, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + annotations.HostProcessRootfsLocation: "C:\\test", + }, false, ""), + expectedError: "", + }, + { + testName: "no annotation passthrough for normal containers (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessInheritUser: "true", + annotations.HostProcessRootfsLocation: "C:\\test", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""), + expectedError: "", + }, + { + testName: "no changes in user process for normal containers (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""), + expectedError: "", + }, + { + testName: "set inherit user annotation to false for normal containers (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessInheritUser: "true", + }, true, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessInheritUser: "false", + }, true, ""), + expectedError: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + p := &pod{spec: tc.podSpec} + + err := p.updateConfigForHostProcessContainer(tc.containerSpec) + if err == nil && tc.expectedError != "" { + t.Fatalf("should have failed with '%s'", tc.expectedError) + } + if err != nil && err.Error() != tc.expectedError { + t.Fatalf("should have failed with '%s', actual: '%s'", tc.expectedError, err.Error()) + } + + if tc.expectedContainerSpec != nil { + equal := reflect.DeepEqual(tc.containerSpec, tc.expectedContainerSpec) + if !equal { + t.Fatalf("containerSpec expected: %+v, got: %+v", tc.expectedContainerSpec, tc.containerSpec) + } + } + }) + } +} diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index ae4b391cd6..4b3f9ef73f 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -148,6 +148,8 @@ func createContainer( return nil, nil, err } + // For Job Container which runs on host, we create the same using shim logic. + // If the request was for job container within UVM, then the request is passed along to GCS. if oci.IsJobContainer(s) { opts := jobcontainers.CreateOptions{WCOWLayers: wcowLayers} container, resources, err = jobcontainers.Create(ctx, id, s, opts) @@ -209,6 +211,12 @@ func newHcsTask( shimOpts = v.(*runhcsopts.Options) } + // For Isolated Job containers, we need special handling wherein if the command line + // is not specified, then we add 'cmd /C' by default. + if oci.IsIsolatedJobContainer(s) { + handleProcessArgsForIsolatedJobContainer(s, s.Process) + } + // Default to an infinite timeout (zero value) var ioRetryTimeout time.Duration if shimOpts != nil { @@ -361,6 +369,10 @@ func (ht *hcsTask) CreateExec(ctx context.Context, req *task.ExecProcessRequest, return errors.Wrapf(errdefs.ErrFailedPrecondition, "exec: '' in task: '%s' must be running to create additional execs", ht.id) } + if oci.IsIsolatedJobContainer(ht.taskSpec) { + handleProcessArgsForIsolatedJobContainer(ht.taskSpec, spec) + } + io, err := cmd.NewUpstreamIO(ctx, req.ID, req.Stdout, req.Stderr, req.Stdin, req.Terminal, ht.ioRetryTimeout) if err != nil { return err @@ -981,6 +993,22 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st return ht.c.Modify(ctx, modification) } +func handleProcessArgsForIsolatedJobContainer(taskSpec *specs.Spec, specs *specs.Process) { + if specs.CommandLine != "" && !strings.HasPrefix(strings.TrimSpace(strings.ToLower(specs.CommandLine)), "cmd") { + specs.CommandLine = fmt.Sprintf("cmd /c %s", specs.CommandLine) + } + if len(specs.Args) > 0 && strings.TrimSpace(strings.ToLower(specs.Args[0])) != "cmd" { + specs.Args = append([]string{"cmd", "/c"}, specs.Args...) + } + + // HostProcessInheritUser is set to explicit true or false during container create. + if taskSpec.Annotations[annotations.HostProcessInheritUser] == "true" { + // For privileged containers within the sandbox, if the annotation to inherit user is set + // we will set the user to NT AUTHORITY\SYSTEM. + specs.User.Username = `NT AUTHORITY\SYSTEM` + } +} + func isMountTypeSupported(hostPath, mountType string) bool { // currently we only support mounting of host volumes/directories switch mountType { diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go index 95fd4f386e..397abf8cb8 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go @@ -5,11 +5,14 @@ package main import ( "context" "math/rand" + "reflect" "strconv" "testing" "time" + "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/containerd/errdefs" + "github.com/opencontainers/runtime-spec/specs-go" ) func setupTestHcsTask(t *testing.T) (*hcsTask, *testShimExec, *testShimExec) { @@ -318,3 +321,150 @@ func Test_hcsTask_DeleteExec_2ndExecID_ExitedState_Success(t *testing.T) { } verifyDeleteSuccessValues(t, pid, status, at, second) } + +func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { + ntAuthorityUser := `NT AUTHORITY\SYSTEM` + testUserName := "testUser" + + tests := []struct { + name string + taskAnnotations map[string]string + specs *specs.Process + expectedCmdLine string + expectedArgs []string + expectedUsername string + }{ + { + name: "CommandLine starts with 'cmd' (lowercase) – unchanged", + specs: &specs.Process{CommandLine: "cmd /c dir"}, + expectedCmdLine: "cmd /c dir", + }, + { + name: "CommandLine starts with 'CMD' (uppercase) – unchanged", + specs: &specs.Process{CommandLine: "CMD /C whoami"}, + expectedCmdLine: "CMD /C whoami", + }, + { + name: "CommandLine starts with 'cmd.exe' – unchanged", + specs: &specs.Process{CommandLine: "cmd.exe /c ipconfig"}, + expectedCmdLine: "cmd.exe /c ipconfig", + }, + { + name: "CommandLine plain – gets prefixed with 'cmd /c '", + specs: &specs.Process{CommandLine: "echo hello"}, + expectedCmdLine: "cmd /c echo hello", + }, + { + name: "CommandLine mixed case 'CmD' – unchanged", + specs: &specs.Process{CommandLine: "CmD /c ping 127.0.0.1"}, + expectedCmdLine: "CmD /c ping 127.0.0.1", + }, + { + name: "CommandLine has leading spaces before 'cmd' – unchanged", + specs: &specs.Process{CommandLine: " cmd /c echo spaced"}, + expectedCmdLine: " cmd /c echo spaced", + }, + { + name: "CommandLine has leading spaces before 'CMD' – unchanged", + specs: &specs.Process{CommandLine: " CMD /C echo spaced"}, + expectedCmdLine: " CMD /C echo spaced", + }, + { + name: "CommandLine whitespace-only – gets prefixed preserving spaces", + specs: &specs.Process{CommandLine: " "}, + expectedCmdLine: "cmd /c ", + }, + { + name: "Args plain – gets ['cmd','/c',...] prefix", + specs: &specs.Process{Args: []string{"echo", "hello"}}, + expectedArgs: []string{"cmd", "/c", "echo", "hello"}, + }, + { + name: "Args already start with 'CMD' (uppercase) – unchanged", + specs: &specs.Process{Args: []string{"CMD", "/C", "echo", "hi"}}, + expectedArgs: []string{"CMD", "/C", "echo", "hi"}, + }, + { + name: "Args already start with 'cmd' (lowercase) – unchanged", + specs: &specs.Process{Args: []string{"cmd", "/c", "type", "file.txt"}}, + expectedArgs: []string{"cmd", "/c", "type", "file.txt"}, + }, + { + name: "Args first element mixed case 'Cmd' – unchanged", + specs: &specs.Process{Args: []string{"Cmd", "/c", "echo", "hi"}}, + expectedArgs: []string{"Cmd", "/c", "echo", "hi"}, + }, + { + name: "Args first element has leading/trailing spaces ' CMD ' – unchanged (trimmed comparison)", + specs: &specs.Process{Args: []string{" CMD ", "/C", "echo", "trimmed"}}, + expectedArgs: []string{" CMD ", "/C", "echo", "trimmed"}, + }, + { + name: "Empty CommandLine and empty Args – unchanged", + specs: &specs.Process{}, + expectedCmdLine: "", + }, + { + name: "Empty CommandLine and empty slice Args – unchanged (empty slice preserved)", + specs: &specs.Process{Args: []string{}}, + expectedCmdLine: "", + expectedArgs: []string{}, + }, + // --- User inheritance behavior --- + { + name: "HostProcessInheritUser=true – sets Username to NT AUTHORITY\\SYSTEM", + taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "true"}, + specs: &specs.Process{}, + expectedUsername: ntAuthorityUser, + }, + { + name: "HostProcessInheritUser=false – does not set Username", + taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "false"}, + specs: &specs.Process{User: specs.User{Username: testUserName}}, + expectedUsername: testUserName, + }, + { + name: "HostProcessInheritUser missing – does not set Username", + taskAnnotations: map[string]string{}, + specs: &specs.Process{User: specs.User{Username: testUserName}}, + expectedUsername: testUserName, + }, + { + name: "HostProcessInheritUser=true – overrides preexisting Username", + taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "true"}, + specs: &specs.Process{User: specs.User{Username: testUserName}}, + expectedUsername: ntAuthorityUser, + }, + { + name: "HostProcessInheritUser=false – preserves preexisting Username", + taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "false"}, + specs: &specs.Process{User: specs.User{Username: testUserName}}, + expectedUsername: testUserName, + }, + { + name: "Nil annotation map – safely no change to Username", + // Note: Annotations=nil is fine; indexing a nil map returns zero value + taskAnnotations: nil, + specs: &specs.Process{User: specs.User{Username: testUserName}}, + expectedUsername: testUserName, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + taskSpec := &specs.Spec{Annotations: tt.taskAnnotations} + + handleProcessArgsForIsolatedJobContainer(taskSpec, tt.specs) + + if tt.specs.CommandLine != tt.expectedCmdLine { + t.Errorf("CommandLine mismatch: got: %q want: %q", tt.specs.CommandLine, tt.expectedCmdLine) + } + if !reflect.DeepEqual(tt.specs.Args, tt.expectedArgs) { + t.Errorf("Args mismatch: got: %#v want: %#v", tt.specs.Args, tt.expectedArgs) + } + if tt.specs.User.Username != tt.expectedUsername { + t.Errorf("Username mismatch: got: %q want: %q", tt.specs.User.Username, tt.expectedUsername) + } + }) + } +} diff --git a/internal/hcs/schema2/container.go b/internal/hcs/schema2/container.go index 39a54432c0..36906d5109 100644 --- a/internal/hcs/schema2/container.go +++ b/internal/hcs/schema2/container.go @@ -33,4 +33,6 @@ type Container struct { AssignedDevices []Device `json:"AssignedDevices,omitempty"` AdditionalDeviceNamespace *ContainerDefinitionDevice `json:"AdditionalDeviceNamespace,omitempty"` + + IsolationType string `json:"IsolationType,omitempty"` } diff --git a/internal/hcs/schema2/storage.go b/internal/hcs/schema2/storage.go index 2627af9132..86cf6fca8e 100644 --- a/internal/hcs/schema2/storage.go +++ b/internal/hcs/schema2/storage.go @@ -18,4 +18,7 @@ type Storage struct { Path string `json:"Path,omitempty"` QoS *StorageQoS `json:"QoS,omitempty"` + + // Path to the root of the container's filesystem. This is useful in case of HostProcess containers where the container rootfs is different from C:\. + PrivilegedContainerRootPath string `json:"PrivilegedContainerRootPath,omitempty"` } diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index e2aa0776cb..a5f7b3101e 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/Microsoft/go-winio/pkg/fs" + "github.com/Microsoft/hcsshim/internal/ospath" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" @@ -490,6 +491,21 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter v2Container.RegistryChanges = &hcsschema.RegistryChanges{ AddValues: registryAdd, } + + if oci.IsIsolatedJobContainer(coi.Spec) { + v2Container.IsolationType = "HostProcess" + + // If the customer specified a custom rootfs path then use that instead of default c:\hpc. + if customRootFsPath, ok := coi.Spec.Annotations[annotations.HostProcessRootfsLocation]; ok { + customRootFsPathSanitized, err := ospath.Sanitize(customRootFsPath, ospath.DisallowedUVMMountPrefixes) + if err != nil { + return nil, nil, err + } + + v2Container.Storage.PrivilegedContainerRootPath = customRootFsPathSanitized + } + } + return v1, v2Container, nil } diff --git a/internal/oci/util.go b/internal/oci/util.go index 259b0e4e37..75383c32a1 100644 --- a/internal/oci/util.go +++ b/internal/oci/util.go @@ -21,6 +21,19 @@ func IsIsolated(s *specs.Spec) bool { } // IsJobContainer checks if `s` is asking for a Windows job container. +// This request is for a shim based process isolated HPCs. func IsJobContainer(s *specs.Spec) bool { - return IsWCOW(s) && s.Annotations[annotations.HostProcessContainer] == "true" + return s.Linux == nil && + s.Windows != nil && + s.Windows.HyperV == nil && + s.Annotations[annotations.HostProcessContainer] == "true" +} + +// IsIsolatedJobContainer checks if `s` is asking for a Windows job container. +// This request is for running HPC within guest. +func IsIsolatedJobContainer(s *specs.Spec) bool { + return s.Linux == nil && + s.Windows != nil && + s.Windows.HyperV != nil && + s.Annotations[annotations.HostProcessContainer] == "true" } diff --git a/internal/oci/util_test.go b/internal/oci/util_test.go index cd88fa19a4..1bfac0a1d1 100644 --- a/internal/oci/util_test.go +++ b/internal/oci/util_test.go @@ -3,6 +3,7 @@ package oci import ( "testing" + "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/opencontainers/runtime-spec/specs-go" ) @@ -152,3 +153,165 @@ func Test_IsIsolated_Neither(t *testing.T) { t.Fatal("should have not have returned isolated for neither config") } } + +// ----------------------------------------------------------------------------- +// IsJobContainer tests +// ----------------------------------------------------------------------------- + +func Test_IsJobContainer(t *testing.T) { + tests := []struct { + name string + spec *specs.Spec + expected bool + }{ + { + name: "WCOW process-isolated with HostProcess=true", + spec: &specs.Spec{ + Windows: &specs.Windows{}, + Annotations: map[string]string{annotations.HostProcessContainer: "true"}, + }, + expected: true, + }, + { + name: "WCOW process-isolated with HostProcess=false", + spec: &specs.Spec{ + Windows: &specs.Windows{}, + Annotations: map[string]string{annotations.HostProcessContainer: "false"}, + }, + expected: false, + }, + { + name: "WCOW process-isolated with HostProcess missing", + spec: &specs.Spec{ + Windows: &specs.Windows{}, + }, + expected: false, + }, + { + name: "WCOW Hyper-V isolated with HostProcess=true (not a JobContainer)", + spec: &specs.Spec{ + Windows: &specs.Windows{ + HyperV: &specs.WindowsHyperV{}, + }, + Annotations: map[string]string{annotations.HostProcessContainer: "true"}, + }, + expected: false, + }, + { + name: "LCOW without Windows (not a JobContainer)", + spec: &specs.Spec{ + Linux: &specs.Linux{}, + }, + expected: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + actual := IsJobContainer(tt.spec) + if actual != tt.expected { + t.Fatalf("IsJobContainer() = %v, expected %v", actual, tt.expected) + } + }) + } +} + +// ----------------------------------------------------------------------------- +// IsIsolatedJobContainer tests +// ----------------------------------------------------------------------------- + +func Test_IsIsolatedJobContainer(t *testing.T) { + tests := []struct { + name string + spec *specs.Spec + expected bool + }{ + { + name: "WCOW Hyper-V isolated with HostProcess=true", + spec: &specs.Spec{ + Windows: &specs.Windows{ + HyperV: &specs.WindowsHyperV{}, + }, + Annotations: map[string]string{annotations.HostProcessContainer: "true"}, + }, + expected: true, + }, + { + name: "WCOW Hyper-V isolated with HostProcess=false", + spec: &specs.Spec{ + Windows: &specs.Windows{ + HyperV: &specs.WindowsHyperV{}, + }, + Annotations: map[string]string{annotations.HostProcessContainer: "false"}, + }, + expected: false, + }, + { + name: "WCOW Hyper-V isolated with HostProcess missing", + spec: &specs.Spec{ + Windows: &specs.Windows{ + HyperV: &specs.WindowsHyperV{}, + }, + }, + expected: false, + }, + { + name: "WCOW process-isolated with HostProcess=true (not isolated job)", + spec: &specs.Spec{ + Windows: &specs.Windows{}, + Annotations: map[string]string{annotations.HostProcessContainer: "true"}, + }, + expected: false, + }, + { + name: "LCOW without Windows (not an isolated job container)", + spec: &specs.Spec{ + Linux: &specs.Linux{}, + }, + expected: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + actual := IsIsolatedJobContainer(tt.spec) + if actual != tt.expected { + t.Fatalf("IsIsolatedJobContainer() = %v, expected %v", actual, tt.expected) + } + }) + } +} + +// ----------------------------------------------------------------------------- +// Cross-property tests (consistency / mutual exclusivity) +// ----------------------------------------------------------------------------- + +func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) { + // Process-isolated WCOW HostProcess=true + processJob := &specs.Spec{ + Windows: &specs.Windows{}, + Annotations: map[string]string{annotations.HostProcessContainer: "true"}, + } + if !IsJobContainer(processJob) { + t.Fatal("expected IsJobContainer to be true for process-isolated HostProcess=true") + } + if IsIsolatedJobContainer(processJob) { + t.Fatal("expected IsIsolatedJobContainer to be false for process-isolated HostProcess=true") + } + + // Hyper-V isolated WCOW HostProcess=true + hyperVJob := &specs.Spec{ + Windows: &specs.Windows{ + HyperV: &specs.WindowsHyperV{}, + }, + Annotations: map[string]string{annotations.HostProcessContainer: "true"}, + } + if IsJobContainer(hyperVJob) { + t.Fatal("expected IsJobContainer to be false for Hyper-V isolated HostProcess=true") + } + if !IsIsolatedJobContainer(hyperVJob) { + t.Fatal("expected IsIsolatedJobContainer to be true for Hyper-V isolated HostProcess=true") + } +} diff --git a/internal/ospath/join.go b/internal/ospath/join.go deleted file mode 100644 index c025460768..0000000000 --- a/internal/ospath/join.go +++ /dev/null @@ -1,14 +0,0 @@ -package ospath - -import ( - "path" - "path/filepath" -) - -// Join joins paths using the target OS's path separator. -func Join(os string, elem ...string) string { - if os == "windows" { - return filepath.Join(elem...) - } - return path.Join(elem...) -} diff --git a/internal/ospath/path.go b/internal/ospath/path.go new file mode 100644 index 0000000000..cc20cdb073 --- /dev/null +++ b/internal/ospath/path.go @@ -0,0 +1,67 @@ +package ospath + +import ( + "errors" + "fmt" + "os" + "path" + "path/filepath" + "strings" +) + +var ( + errUnsafePath = errors.New("unsafe path detected") + + // DisallowedUVMMountPrefixes represents common locations within UVM + // where we do not want mounts to happen from customer perspective. + DisallowedUVMMountPrefixes = []string{ + `C:\Windows`, + `C:\mounts`, + `C:\EFI`, + `C:\SandboxMounts`, + } +) + +// Join joins paths using the target OS's path separator. +func Join(os string, elem ...string) string { + if os == "windows" { + return filepath.Join(elem...) + } + return path.Join(elem...) +} + +// Sanitize validates and normalizes a Windows path. +func Sanitize(path string, disallowedPrefixes []string) (string, error) { + if path == "" { + return "", errUnsafePath + } + + // Normalize the path. + cleanPath := filepath.Clean(path) + + // Reject UNC paths (\\server\share or //server/share) + if strings.HasPrefix(cleanPath, `\\`) || strings.HasPrefix(cleanPath, `//`) { + return "", errUnsafePath + } + + // Check if the path is not in the disallowed paths. + for _, prefix := range disallowedPrefixes { + if strings.HasPrefix(cleanPath, prefix) { + return "", errUnsafePath + } + } + + // Reject if the path already exists (file/dir/symlink/junction). + // Use Lstat so we do not follow symlinks. + var err error + if _, err = os.Lstat(cleanPath); err == nil { + // Path exists + return "", fmt.Errorf("%w: path %q already exists", errUnsafePath, cleanPath) + } + if !os.IsNotExist(err) { + // Unexpected error (e.g., permission issues) + return "", fmt.Errorf("%w: error checking existence for %q: %w", errUnsafePath, cleanPath, err) + } + + return cleanPath, nil +} diff --git a/internal/ospath/path_test.go b/internal/ospath/path_test.go new file mode 100644 index 0000000000..90368712bf --- /dev/null +++ b/internal/ospath/path_test.go @@ -0,0 +1,84 @@ +package ospath + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSanitize(t *testing.T) { + // Create a temp folder and file to simulate "already exists" + existingDir := t.TempDir() + existingFile := filepath.Join(existingDir, "exists.txt") + if err := os.WriteFile(existingFile, []byte("data"), 0644); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + path string + disallowedPrefixes []string + expectedPath string + expectedErrPrefix string + }{ + { + name: "valid path", + path: filepath.Join(existingDir, "test"), + expectedPath: filepath.Join(existingDir, "test"), + }, + { + name: "empty path", + path: "", + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "path traversal", + path: `C:\foo\..\Windows`, + disallowedPrefixes: []string{`C:\Windows`}, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "UNC path", + path: `\\server\share`, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "disallowed prefix", + path: `C:\Windows\System32`, + disallowedPrefixes: []string{`C:\Windows`}, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "existing folder", + path: existingDir, + expectedErrPrefix: "already exists", + }, + { + name: "existing file", + path: existingFile, + expectedErrPrefix: "already exists", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Sanitize(tt.path, tt.disallowedPrefixes) + + if tt.expectedErrPrefix != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.expectedErrPrefix) + } + if !strings.Contains(err.Error(), tt.expectedErrPrefix) { + t.Fatalf("expected error to contain %q, got %v", tt.expectedErrPrefix, err) + } + } else if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if !strings.EqualFold(got, tt.expectedPath) { + t.Errorf("expected path %q, got %q", tt.expectedPath, got) + } + }) + } +}