From f76b015122e2fcecc2a6c9507d71c6e2d63327c3 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Fri, 5 Sep 2025 00:33:21 +0530 Subject: [PATCH 01/13] Added support for hypervisor isolated hpc This commit adds support for running HostProcess containers with hypervisor isolated workloads. Therefore, these HPCs will run as privileged processes within the UVM. Some of the key aspects of this feature are- - A pod needs to be marked privileged by passing in annotation 'microsoft.com/hostprocess-container' - Any container which has 'microsoft.com/hostprocess-container' annotation will be launched as HostProcess Container. - Other containers will be launched as normal process-isolated containers within the UVM - HPC within UVM will not have access to any network since the sandbox (UVM) does not have any network. - GCS will create the HPCs based on the specs. The behaviour is similar to existing HPCs for process-isolated case. - Annotations for Inherit user will lead to the HPC running as System user within UVM. - Annotation for custom rootfs will lead to the container filesystem being virtualized in a directory with that custom name. If not provided, `C:\hpc` is used by default. Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 83 ++++++++++++++++------- cmd/containerd-shim-runhcs-v1/task_hcs.go | 24 +++++++ internal/hcs/schema2/container.go | 2 + internal/hcs/schema2/storage.go | 3 + internal/hcsoci/hcsdoc_wcow.go | 10 +++ internal/oci/util.go | 15 +++- 6 files changed, 110 insertions(+), 27 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index 76b126b007..95d7515626 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -171,7 +171,7 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques var parent *uvm.UtilityVM var lopts *uvm.OptionsLCOW - if oci.IsIsolated(s) { + if oci.IsIsolated(s) || oci.IsIsolatedJobContainer(s) { // Create the UVM parent opts, err := oci.SpecToUVMCreateOpts(ctx, s, fmt.Sprintf("%s@vm", req.ID), owner) if err != nil { @@ -202,7 +202,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, "") @@ -224,7 +223,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") @@ -339,11 +338,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 @@ -368,24 +362,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) @@ -502,3 +481,55 @@ 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 { + if isHypervisorIsolatedPrivilegedContainer && s.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. + s.Process.User.Username = `NT AUTHORITY\SYSTEM` + } + + if !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/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index 9fbb1faf35..270d219119 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -149,6 +149,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) @@ -212,6 +214,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.Process) + } + // Default to an infinite timeout (zero value) var ioRetryTimeout time.Duration if shimOpts != nil { @@ -364,6 +372,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(spec) + } + io, err := cmd.NewUpstreamIO(ctx, req.ID, req.Stdout, req.Stderr, req.Stdin, req.Terminal, ht.ioRetryTimeout) if err != nil { return err @@ -1015,6 +1027,18 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st return ht.c.Modify(ctx, modification) } +func handleProcessArgsForIsolatedJobContainer(specs *specs.Process) { + if specs.CommandLine != "" { + if !strings.HasPrefix(strings.ToLower(specs.CommandLine), "cmd") { + specs.CommandLine = fmt.Sprintf("cmd /c %s", specs.CommandLine) + } + } else { + if strings.ToLower(specs.Args[0]) != "cmd" { + specs.Args = append([]string{"cmd", "/c"}, specs.Args...) + } + } +} + func isMountTypeSupported(hostPath, mountType string) bool { // currently we only support mounting of host volumes/directories switch mountType { diff --git a/internal/hcs/schema2/container.go b/internal/hcs/schema2/container.go index 39a54432c0..0914478c70 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"` + + ContainerType string `json:"ContainerType,omitempty"` } diff --git a/internal/hcs/schema2/storage.go b/internal/hcs/schema2/storage.go index 2627af9132..99a6edff77 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:\. + ContainerRootPath string `json:"ContainerRootPath,omitempty"` } diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index d1d1a44c85..d084241ead 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -499,6 +499,16 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter v2Container.RegistryChanges = &hcsschema.RegistryChanges{ AddValues: registryAdd, } + + if oci.IsIsolatedJobContainer(coi.Spec) { + v2Container.ContainerType = "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 { + v2Container.Storage.ContainerRootPath = customRootFsPath + } + } + 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" } From 717bd1fce2f45c407663aef0a25afadb8bf19f29 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Mon, 8 Sep 2025 13:02:23 +0530 Subject: [PATCH 02/13] Added unit tests for HostProcess Container helper methods This commit adds thorough unit tests for: - IsJobContainer - IsIsolatedJobContainer These tests explicitly verify the expected behavior for Windows HostProcess containers across isolation modes and OS targets. Signed-off-by: Harsh Rawat --- internal/oci/util_test.go | 163 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) 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") + } +} From ac5a240177dc489efd5ed271d35b451e33a30836 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Mon, 8 Sep 2025 15:25:38 +0530 Subject: [PATCH 03/13] Added unit tests for updateConfigForHostProcessContainer method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds thorough unit tests for the updateConfigForHostProcessContainer method. We test the following functionality- - Reject privileged container in unprivileged sandbox (isolated HPC) — return error when container requests microsoft.com/hostprocess-container=true but the pod lacks it. - Allow privileged container in privileged sandbox (isolated HPC) — ensure no unexpected mutations when no passthrough annotations are present. - Block normal process-isolated container in process-isolated HPC pod — enforce constraint that all containers in a process HPC pod must also be job containers (error on mixing). - Allow normal hypervisor-isolated container in hypervisor-isolated HPC pod. - Passthrough annotations to privileged containers — propagate HostProcessInheritUser and HostProcessRootfsLocation from pod → container for: hypervisor-isolated HPC pods with privileged containers, and process-isolated HPC pods with privileged containers. - No passthrough for normal containers — verify annotations aren’t propagated when the container is non-privileged. - Set SYSTEM user for hypervisor-isolated privileged containers — when HostProcessInheritUser=true, set Process.User.Username to NT AUTHORITY\SYSTEM. - Do not change user for normal containers — ensure container user remains unchanged even if the pod has HostProcessInheritUser=true. - Force HostProcessInheritUser to "false" for normal containers — in hypervisor-isolated privileged pods, if a non-privileged container sets the inherit annotation, flip it to "false". Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod_test.go | 173 ++++++++++++++++++++++ 1 file changed, 173 insertions(+) diff --git a/cmd/containerd-shim-runhcs-v1/pod_test.go b/cmd/containerd-shim-runhcs-v1/pod_test.go index 816bd800ed..34e144d04a 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,174 @@ 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) { + ntAuthorityUser := `NT AUTHORITY\SYSTEM` + + 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, ntAuthorityUser), + 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: "set user process for inherit annotation (isolated hpc)", + podSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + }, true, ""), + containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + annotations.HostProcessInheritUser: "true", + }, true, ntAuthorityUser), + 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) + } + } + }) + } +} From 6a5350da63097ca6cf99666526df1168070dcd7c Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 8 Oct 2025 02:20:10 +0530 Subject: [PATCH 04/13] Added unit tests for handleProcessArgsForIsolatedJobContainer Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/task_hcs.go | 13 +-- .../task_hcs_test.go | 97 +++++++++++++++++++ 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index 270d219119..da46ed06db 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -1028,14 +1028,11 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st } func handleProcessArgsForIsolatedJobContainer(specs *specs.Process) { - if specs.CommandLine != "" { - if !strings.HasPrefix(strings.ToLower(specs.CommandLine), "cmd") { - specs.CommandLine = fmt.Sprintf("cmd /c %s", specs.CommandLine) - } - } else { - if strings.ToLower(specs.Args[0]) != "cmd" { - specs.Args = append([]string{"cmd", "/c"}, specs.Args...) - } + 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...) } } diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go index 95fd4f386e..f29e20f891 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go @@ -5,11 +5,13 @@ package main import ( "context" "math/rand" + "reflect" "strconv" "testing" "time" "github.com/containerd/errdefs" + "github.com/opencontainers/runtime-spec/specs-go" ) func setupTestHcsTask(t *testing.T) (*hcsTask, *testShimExec, *testShimExec) { @@ -318,3 +320,98 @@ func Test_hcsTask_DeleteExec_2ndExecID_ExitedState_Success(t *testing.T) { } verifyDeleteSuccessValues(t, pid, status, at, second) } + +func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { + tests := []struct { + name string + specs *specs.Process + expectedCmdLine string + expectedArgs []string + }{ + { + name: "CommandLine starts with 'cmd' (lowercase) – unchanged", + specs: &specs.Process{CommandLine: "cmd /c dir"}, + expectedCmdLine: "cmd /c dir", + expectedArgs: nil, + }, + { + name: "CommandLine starts with 'CMD' (uppercase) – unchanged", + specs: &specs.Process{CommandLine: "CMD /C whoami"}, + expectedCmdLine: "CMD /C whoami", + expectedArgs: nil, + }, + { + name: "CommandLine starts with 'cmd.exe' – unchanged", + specs: &specs.Process{CommandLine: "cmd.exe /c ipconfig"}, + expectedCmdLine: "cmd.exe /c ipconfig", + expectedArgs: nil, + }, + { + name: "CommandLine plain – gets prefixed with 'cmd /c '", + specs: &specs.Process{CommandLine: "echo hello"}, + expectedCmdLine: "cmd /c echo hello", + expectedArgs: nil, + }, + { + 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", + expectedArgs: nil, + }, + { + name: "Args plain – gets ['cmd','/c',...] prefix", + specs: &specs.Process{Args: []string{"echo", "hello"}}, + expectedCmdLine: "", + expectedArgs: []string{"cmd", "/c", "echo", "hello"}, + }, + { + name: "Args already start with 'CMD' (uppercase) – unchanged", + specs: &specs.Process{Args: []string{"CMD", "/C", "echo", "hi"}}, + expectedCmdLine: "", + expectedArgs: []string{"CMD", "/C", "echo", "hi"}, + }, + { + name: "Args already start with 'cmd' (lowercase) – unchanged", + specs: &specs.Process{Args: []string{"cmd", "/c", "type", "file.txt"}}, + expectedCmdLine: "", + expectedArgs: []string{"cmd", "/c", "type", "file.txt"}, + }, + { + name: "Empty CommandLine and empty Args – unchanged", + specs: &specs.Process{}, + expectedCmdLine: "", + expectedArgs: nil, + }, + { + name: "Empty CommandLine and empty slice Args – unchanged (empty slice preserved)", + specs: &specs.Process{Args: []string{}}, + expectedCmdLine: "", + expectedArgs: []string{}, + }, + { + name: "CommandLine has leading spaces before 'cmd' – treated as not starting with cmd - unchanged", + specs: &specs.Process{CommandLine: " cmd /c echo spaced"}, + expectedCmdLine: " cmd /c echo spaced", + expectedArgs: nil, + }, + { + name: "Args first element mixed case 'Cmd' – unchanged", + specs: &specs.Process{Args: []string{"Cmd", "/c", "echo", "hi"}}, + expectedCmdLine: "", + expectedArgs: []string{"Cmd", "/c", "echo", "hi"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handleProcessArgsForIsolatedJobContainer(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) + } + }) + } +} From bf8fd0337948efdef09f9effcaeed6a75e120c27 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 8 Oct 2025 14:15:26 +0530 Subject: [PATCH 05/13] Rectified the schema for custom rootfs for HCS HPCs Signed-off-by: Harsh Rawat --- internal/hcs/schema2/storage.go | 2 +- internal/hcsoci/hcsdoc_wcow.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/hcs/schema2/storage.go b/internal/hcs/schema2/storage.go index 99a6edff77..86cf6fca8e 100644 --- a/internal/hcs/schema2/storage.go +++ b/internal/hcs/schema2/storage.go @@ -20,5 +20,5 @@ type Storage struct { 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:\. - ContainerRootPath string `json:"ContainerRootPath,omitempty"` + PrivilegedContainerRootPath string `json:"PrivilegedContainerRootPath,omitempty"` } diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index d084241ead..0f7f26610f 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -505,7 +505,7 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter // 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 { - v2Container.Storage.ContainerRootPath = customRootFsPath + v2Container.Storage.PrivilegedContainerRootPath = customRootFsPath } } From 311a8bbfeca0d27f2403bdc4ad8cd6a1dffa3945 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 8 Oct 2025 18:08:30 +0530 Subject: [PATCH 06/13] Rectified the logic for user inherit functionality for HPCs Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 18 +-- cmd/containerd-shim-runhcs-v1/pod_test.go | 19 +-- cmd/containerd-shim-runhcs-v1/task_hcs.go | 13 +- .../task_hcs_test.go | 120 +++++++++++++----- 4 files changed, 106 insertions(+), 64 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index 95d7515626..d55a6e36cd 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -517,18 +517,12 @@ func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { ) } - if isHypervisorIsolatedPrivilegedSandbox { - if isHypervisorIsolatedPrivilegedContainer && s.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. - s.Process.User.Username = `NT AUTHORITY\SYSTEM` - } - - if !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" - } + 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 34e144d04a..4423108b8f 100644 --- a/cmd/containerd-shim-runhcs-v1/pod_test.go +++ b/cmd/containerd-shim-runhcs-v1/pod_test.go @@ -407,8 +407,6 @@ func getWCOWSpecsWithAnnotations(annotation map[string]string, isIsolated bool, } func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) { - ntAuthorityUser := `NT AUTHORITY\SYSTEM` - testCases := []struct { testName string podSpec *specs.Spec @@ -470,7 +468,7 @@ func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) { annotations.HostProcessContainer: "true", annotations.HostProcessInheritUser: "true", annotations.HostProcessRootfsLocation: "C:\\test", - }, true, ntAuthorityUser), + }, true, ""), expectedError: "", }, { @@ -500,21 +498,6 @@ func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) { expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""), expectedError: "", }, - { - testName: "set user process for inherit annotation (isolated hpc)", - podSpec: getWCOWSpecsWithAnnotations(map[string]string{ - annotations.HostProcessContainer: "true", - annotations.HostProcessInheritUser: "true", - }, true, ""), - containerSpec: getWCOWSpecsWithAnnotations(map[string]string{ - annotations.HostProcessContainer: "true", - }, true, ""), - expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ - annotations.HostProcessContainer: "true", - annotations.HostProcessInheritUser: "true", - }, true, ntAuthorityUser), - expectedError: "", - }, { testName: "no changes in user process for normal containers (isolated hpc)", podSpec: getWCOWSpecsWithAnnotations(map[string]string{ diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index da46ed06db..992149275d 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -217,7 +217,7 @@ func newHcsTask( // 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.Process) + handleProcessArgsForIsolatedJobContainer(s, s.Process) } // Default to an infinite timeout (zero value) @@ -373,7 +373,7 @@ func (ht *hcsTask) CreateExec(ctx context.Context, req *task.ExecProcessRequest, } if oci.IsIsolatedJobContainer(ht.taskSpec) { - handleProcessArgsForIsolatedJobContainer(spec) + handleProcessArgsForIsolatedJobContainer(ht.taskSpec, spec) } io, err := cmd.NewUpstreamIO(ctx, req.ID, req.Stdout, req.Stderr, req.Stdin, req.Terminal, ht.ioRetryTimeout) @@ -1027,13 +1027,20 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st return ht.c.Modify(ctx, modification) } -func handleProcessArgsForIsolatedJobContainer(specs *specs.Process) { +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 { diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go index f29e20f891..bae6605756 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/containerd/errdefs" "github.com/opencontainers/runtime-spec/specs-go" ) @@ -322,65 +323,87 @@ func Test_hcsTask_DeleteExec_2ndExecID_ExitedState_Success(t *testing.T) { } func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { + ntAuthorityUser := `NT AUTHORITY\SYSTEM` + testUserName := "testUser" + tests := []struct { - name string - specs *specs.Process - expectedCmdLine string - expectedArgs []string + name string + taskAnnotations map[string]string + specs *specs.Process + expectedCmdLine string + expectedArgs []string + initialUsername string + expectedUsername string }{ { name: "CommandLine starts with 'cmd' (lowercase) – unchanged", specs: &specs.Process{CommandLine: "cmd /c dir"}, expectedCmdLine: "cmd /c dir", - expectedArgs: nil, }, { name: "CommandLine starts with 'CMD' (uppercase) – unchanged", specs: &specs.Process{CommandLine: "CMD /C whoami"}, expectedCmdLine: "CMD /C whoami", - expectedArgs: nil, }, { name: "CommandLine starts with 'cmd.exe' – unchanged", specs: &specs.Process{CommandLine: "cmd.exe /c ipconfig"}, expectedCmdLine: "cmd.exe /c ipconfig", - expectedArgs: nil, }, { name: "CommandLine plain – gets prefixed with 'cmd /c '", specs: &specs.Process{CommandLine: "echo hello"}, expectedCmdLine: "cmd /c echo hello", - expectedArgs: nil, }, { 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", - expectedArgs: nil, }, { - name: "Args plain – gets ['cmd','/c',...] prefix", - specs: &specs.Process{Args: []string{"echo", "hello"}}, - expectedCmdLine: "", - expectedArgs: []string{"cmd", "/c", "echo", "hello"}, + name: "CommandLine has leading spaces before 'cmd' – unchanged", + specs: &specs.Process{CommandLine: " cmd /c echo spaced"}, + expectedCmdLine: " cmd /c echo spaced", }, { - name: "Args already start with 'CMD' (uppercase) – unchanged", - specs: &specs.Process{Args: []string{"CMD", "/C", "echo", "hi"}}, - expectedCmdLine: "", - expectedArgs: []string{"CMD", "/C", "echo", "hi"}, + name: "CommandLine has leading spaces before 'CMD' – unchanged", + specs: &specs.Process{CommandLine: " CMD /C echo spaced"}, + expectedCmdLine: " CMD /C echo spaced", }, { - name: "Args already start with 'cmd' (lowercase) – unchanged", - specs: &specs.Process{Args: []string{"cmd", "/c", "type", "file.txt"}}, - expectedCmdLine: "", - expectedArgs: []string{"cmd", "/c", "type", "file.txt"}, + 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: "", - expectedArgs: nil, }, { name: "Empty CommandLine and empty slice Args – unchanged (empty slice preserved)", @@ -388,23 +411,55 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { expectedCmdLine: "", expectedArgs: []string{}, }, + // --- User inheritance behavior --- { - name: "CommandLine has leading spaces before 'cmd' – treated as not starting with cmd - unchanged", - specs: &specs.Process{CommandLine: " cmd /c echo spaced"}, - expectedCmdLine: " cmd /c echo spaced", - expectedArgs: nil, + name: "HostProcessInheritUser=true – sets Username to NT AUTHORITY\\SYSTEM", + taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "true"}, + specs: &specs.Process{}, + expectedUsername: ntAuthorityUser, }, { - name: "Args first element mixed case 'Cmd' – unchanged", - specs: &specs.Process{Args: []string{"Cmd", "/c", "echo", "hi"}}, - expectedCmdLine: "", - expectedArgs: []string{"Cmd", "/c", "echo", "hi"}, + 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) { - handleProcessArgsForIsolatedJobContainer(tt.specs) + taskSpec := &specs.Spec{Annotations: tt.taskAnnotations} + // Ensure initial Username is set if provided + if tt.initialUsername != "" { + tt.specs.User.Username = tt.initialUsername + } + + handleProcessArgsForIsolatedJobContainer(taskSpec, tt.specs) if tt.specs.CommandLine != tt.expectedCmdLine { t.Errorf("CommandLine mismatch: got: %q want: %q", tt.specs.CommandLine, tt.expectedCmdLine) @@ -412,6 +467,9 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { 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) + } }) } } From e43d8d26026a0871b612ae8391b4f93ed7bf08cf Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Sat, 1 Nov 2025 02:46:01 +0530 Subject: [PATCH 07/13] Addressed PR feedback Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 2 +- .../task_hcs_test.go | 5 -- internal/hcsoci/hcsdoc_wcow.go | 8 +- internal/ospath/join.go | 14 ---- internal/ospath/path.go | 67 +++++++++++++++ internal/ospath/path_test.go | 84 +++++++++++++++++++ 6 files changed, 159 insertions(+), 21 deletions(-) delete mode 100644 internal/ospath/join.go create mode 100644 internal/ospath/path.go create mode 100644 internal/ospath/path_test.go diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index d55a6e36cd..b54b8ba1d0 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -171,7 +171,7 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques var parent *uvm.UtilityVM var lopts *uvm.OptionsLCOW - if oci.IsIsolated(s) || oci.IsIsolatedJobContainer(s) { + if oci.IsIsolated(s) { // Create the UVM parent opts, err := oci.SpecToUVMCreateOpts(ctx, s, fmt.Sprintf("%s@vm", req.ID), owner) if err != nil { diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go index bae6605756..397abf8cb8 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go @@ -332,7 +332,6 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { specs *specs.Process expectedCmdLine string expectedArgs []string - initialUsername string expectedUsername string }{ { @@ -454,10 +453,6 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { taskSpec := &specs.Spec{Annotations: tt.taskAnnotations} - // Ensure initial Username is set if provided - if tt.initialUsername != "" { - tt.specs.User.Username = tt.initialUsername - } handleProcessArgsForIsolatedJobContainer(taskSpec, tt.specs) diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index 0f7f26610f..cd366df0dc 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" @@ -505,7 +506,12 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter // 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 { - v2Container.Storage.PrivilegedContainerRootPath = customRootFsPath + customRootFsPathSanitized, err := ospath.Sanitize(customRootFsPath, ospath.DisallowedUVMMountPrefixes) + if err != nil { + return nil, nil, err + } + + v2Container.Storage.PrivilegedContainerRootPath = customRootFsPathSanitized } } 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) + } + }) + } +} From de2dcdf3eb81ffb88b06d4b57f76d458387c032d Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Fri, 21 Nov 2025 21:59:43 +0530 Subject: [PATCH 08/13] Schema changes inline with HCS Signed-off-by: Harsh Rawat --- internal/hcs/schema2/container.go | 2 +- internal/hcsoci/hcsdoc_wcow.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/hcs/schema2/container.go b/internal/hcs/schema2/container.go index 0914478c70..36906d5109 100644 --- a/internal/hcs/schema2/container.go +++ b/internal/hcs/schema2/container.go @@ -34,5 +34,5 @@ type Container struct { AdditionalDeviceNamespace *ContainerDefinitionDevice `json:"AdditionalDeviceNamespace,omitempty"` - ContainerType string `json:"ContainerType,omitempty"` + IsolationType string `json:"IsolationType,omitempty"` } diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index cd366df0dc..65d2b1b539 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -502,7 +502,7 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter } if oci.IsIsolatedJobContainer(coi.Spec) { - v2Container.ContainerType = "HostProcess" + 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 { From 4916a30e73b55e63e6be21340c2ae4b2a2e888ef Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 20 May 2026 13:57:52 +0530 Subject: [PATCH 09/13] review comments 2 Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 29 +++++--- cmd/containerd-shim-runhcs-v1/task_hcs.go | 23 +++++- internal/hcs/schema2/container.go | 15 +++- internal/hcsoci/hcsdoc_wcow.go | 6 +- internal/oci/util.go | 15 +--- internal/oci/util_test.go | 88 +++-------------------- test/internal/container/container.go | 2 +- 7 files changed, 70 insertions(+), 108 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index b54b8ba1d0..ffc86a05dc 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -482,16 +482,26 @@ func (p *pod) DeleteTask(ctx context.Context, tid string) error { return nil } +// updateConfigForHostProcessContainer validates and adjusts a container spec +// against the pod sandbox spec for HostProcess (HPC) scenarios: +// - Rejects a hypervisor-isolated HPC inside a non-HPC sandbox. +// - Rejects a non-HPC container inside a process-isolated HPC sandbox +// (all containers in such a pod must share the host job/network). +// - Propagates HPC pod-level annotations (e.g. inherit-user, rootfs +// location) onto each container, since CRI only delivers them at pod +// create time. +// - Forces HostProcessInheritUser=false on a non-privileged container in a +// privileged hypervisor-isolated sandbox so it can't inherit SYSTEM. func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { - if !oci.IsIsolatedJobContainer(p.spec) && oci.IsIsolatedJobContainer(s) { + isProcessIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && !oci.IsIsolated(p.spec) + isHypervisorIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && oci.IsIsolated(p.spec) + isProcessIsolatedPrivilegedContainer := oci.IsJobContainer(s) && !oci.IsIsolated(s) + isHypervisorIsolatedPrivilegedContainer := oci.IsJobContainer(s) && oci.IsIsolated(s) + + if !isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer { 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 @@ -499,10 +509,9 @@ func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { // // 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. + // + // If the pod sandbox is a process-isolated HPC then the container must also be a + // process-isolated HPC. return errors.New("cannot create a normal process isolated container if the pod sandbox is a job container running on host") } diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index 992149275d..63334070e9 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -151,7 +151,7 @@ func createContainer( // 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) { + if oci.IsJobContainer(s) && !oci.IsIsolated(s) { opts := jobcontainers.CreateOptions{WCOWLayers: wcowLayers} container, resources, err = jobcontainers.Create(ctx, id, s, opts) if err != nil { @@ -216,7 +216,7 @@ func newHcsTask( // 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) { + if oci.IsJobContainer(s) && oci.IsIsolated(s) { handleProcessArgsForIsolatedJobContainer(s, s.Process) } @@ -372,7 +372,7 @@ 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) { + if oci.IsJobContainer(ht.taskSpec) && oci.IsIsolated(ht.taskSpec) { handleProcessArgsForIsolatedJobContainer(ht.taskSpec, spec) } @@ -1027,6 +1027,23 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st return ht.c.Modify(ctx, modification) } +// handleProcessArgsForIsolatedJobContainer adjusts the process spec for a +// HostProcess container running inside a hypervisor-isolated UVM. +// +// Why the "cmd /c" wrap: +// - For a regular WCOW container, the guest creates the workload process +// from within the container silo, so the silo's filesystem view (including +// the bind-mounted container rootfs) is in scope for path resolution. +// - For a HostProcess container the guest creates the process from outside +// the silo and only attaches it to the silo's job object after creation. +// Executable-name resolution therefore uses the launcher's view (System32, +// Windows, PATH) and cannot see binaries that live only inside the +// container rootfs, nor resolve shell builtins like `echo`, `dir`, `set`, +// redirection, etc. Anything not present as a real .exe on the standard +// search path would fail with ERROR_FILE_NOT_FOUND. +// - cmd.exe always exists under System32 (so the launcher can find it) and, +// once joined to the silo's job, runs inside the container's filesystem +// view where it can resolve builtins and any rootfs-relative binary. 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) diff --git a/internal/hcs/schema2/container.go b/internal/hcs/schema2/container.go index 36906d5109..a60f6bb273 100644 --- a/internal/hcs/schema2/container.go +++ b/internal/hcs/schema2/container.go @@ -9,6 +9,19 @@ package hcsschema +// IsolationType describes the isolation mode of a container. +type IsolationType string + +const ( + // IsolationTypeProcess is a fully process-isolated, namespace-isolated + // Windows Server container. + IsolationTypeProcess IsolationType = "Process" + + // IsolationTypeHostProcess is a privileged (Windows) HostProcess + // container that shares the host namespace. + IsolationTypeHostProcess IsolationType = "HostProcess" +) + type Container struct { GuestOs *GuestOs `json:"GuestOs,omitempty"` @@ -34,5 +47,5 @@ type Container struct { AdditionalDeviceNamespace *ContainerDefinitionDevice `json:"AdditionalDeviceNamespace,omitempty"` - IsolationType string `json:"IsolationType,omitempty"` + IsolationType IsolationType `json:"IsolationType,omitempty"` } diff --git a/internal/hcsoci/hcsdoc_wcow.go b/internal/hcsoci/hcsdoc_wcow.go index 65d2b1b539..3da8bd27be 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -501,8 +501,9 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter AddValues: registryAdd, } - if oci.IsIsolatedJobContainer(coi.Spec) { - v2Container.IsolationType = "HostProcess" + if oci.IsJobContainer(coi.Spec) && oci.IsIsolated(coi.Spec) { + // Set the isolation type to host process. + v2Container.IsolationType = hcsschema.IsolationTypeHostProcess // 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 { @@ -511,6 +512,7 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter return nil, nil, err } + // Set the path for container root filesystem. v2Container.Storage.PrivilegedContainerRootPath = customRootFsPathSanitized } } diff --git a/internal/oci/util.go b/internal/oci/util.go index 75383c32a1..259b0e4e37 100644 --- a/internal/oci/util.go +++ b/internal/oci/util.go @@ -21,19 +21,6 @@ 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 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" + return IsWCOW(s) && s.Annotations[annotations.HostProcessContainer] == "true" } diff --git a/internal/oci/util_test.go b/internal/oci/util_test.go index 1bfac0a1d1..4a1c35824f 100644 --- a/internal/oci/util_test.go +++ b/internal/oci/util_test.go @@ -188,14 +188,14 @@ func Test_IsJobContainer(t *testing.T) { expected: false, }, { - name: "WCOW Hyper-V isolated with HostProcess=true (not a JobContainer)", + 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: false, + expected: true, }, { name: "LCOW without Windows (not a JobContainer)", @@ -218,77 +218,11 @@ func Test_IsJobContainer(t *testing.T) { } // ----------------------------------------------------------------------------- -// 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) +// Cross-property tests: IsJobContainer combined with IsIsolated to differentiate +// process-isolated HPC from hypervisor-isolated HPC. // ----------------------------------------------------------------------------- -func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) { +func Test_JobContainer_IsolationDifferentiation(t *testing.T) { // Process-isolated WCOW HostProcess=true processJob := &specs.Spec{ Windows: &specs.Windows{}, @@ -297,8 +231,8 @@ func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) { 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") + if IsIsolated(processJob) { + t.Fatal("expected IsIsolated to be false for process-isolated HostProcess=true") } // Hyper-V isolated WCOW HostProcess=true @@ -308,10 +242,10 @@ func Test_JobContainer_And_IsolatedJobContainer_MutualExclusion(t *testing.T) { }, Annotations: map[string]string{annotations.HostProcessContainer: "true"}, } - if IsJobContainer(hyperVJob) { - t.Fatal("expected IsJobContainer to be false for Hyper-V isolated HostProcess=true") + if !IsJobContainer(hyperVJob) { + t.Fatal("expected IsJobContainer to be true for Hyper-V isolated HostProcess=true") } - if !IsIsolatedJobContainer(hyperVJob) { - t.Fatal("expected IsIsolatedJobContainer to be true for Hyper-V isolated HostProcess=true") + if !IsIsolated(hyperVJob) { + t.Fatal("expected IsIsolated to be true for Hyper-V isolated HostProcess=true") } } diff --git a/test/internal/container/container.go b/test/internal/container/container.go index 0d3a9a0a5b..fd8958cea6 100644 --- a/test/internal/container/container.go +++ b/test/internal/container/container.go @@ -48,7 +48,7 @@ func Create( tb.Fatalf("layer parsing failed: %s", err) } - if oci.IsJobContainer(spec) { + if oci.IsJobContainer(spec) && !oci.IsIsolated(spec) { c, r, err = jobcontainers.Create(ctx, name, spec, jobcontainers.CreateOptions{WCOWLayers: wcowLayers}) } else { co := &hcsoci.CreateOptions{ From 9dfb4cbde6b2aa0525559bc41c6fb887ccfeb95f Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 20 May 2026 14:48:27 +0530 Subject: [PATCH 10/13] reject task creation if the UVM does not support HPCs Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 6 ++++++ internal/gcs/guestcaps.go | 4 ++++ internal/hcs/schema1/schema1.go | 1 + internal/uvm/capabilities.go | 11 +++++++++++ 4 files changed, 22 insertions(+) diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index ffc86a05dc..3b3ed64135 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -502,6 +502,12 @@ func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { return errors.New("cannot create a host process container inside sandbox which has missing annotation: microsoft.com/hostprocess-container") } + // If the UVM does not support HPCs, then we reject HPC calls. + if isHypervisorIsolatedPrivilegedContainer && + p.host != nil && !p.host.HostProcessContainerSupported() { + return fmt.Errorf("UVM does not support HostProcess containers") + } + if isProcessIsolatedPrivilegedSandbox || (isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer) { if isProcessIsolatedPrivilegedSandbox && !isProcessIsolatedPrivilegedContainer { // This is a short circuit to make sure that all containers in a pod will have diff --git a/internal/gcs/guestcaps.go b/internal/gcs/guestcaps.go index 58b1f8ebb7..f2e2b866e2 100644 --- a/internal/gcs/guestcaps.go +++ b/internal/gcs/guestcaps.go @@ -104,3 +104,7 @@ func (w *WCOWGuestDefinedCapabilities) IsDeleteContainerStateSupported() bool { func (w *WCOWGuestDefinedCapabilities) IsLogForwardingSupported() bool { return w.LogForwardingSupported } + +func (w *WCOWGuestDefinedCapabilities) IsHostProcessContainerSupported() bool { + return w.HostProcessContainerSupported +} diff --git a/internal/hcs/schema1/schema1.go b/internal/hcs/schema1/schema1.go index e0a3010f8d..24162ca158 100644 --- a/internal/hcs/schema1/schema1.go +++ b/internal/hcs/schema1/schema1.go @@ -222,6 +222,7 @@ type GuestDefinedCapabilities struct { DeleteContainerStateSupported bool `json:",omitempty"` UpdateContainerSupported bool `json:",omitempty"` LogForwardingSupported bool `json:",omitempty"` + HostProcessContainerSupported bool `json:",omitempty"` } // GuestConnectionInfo is the structure of an iterm return by a GuestConnection call on a utility VM diff --git a/internal/uvm/capabilities.go b/internal/uvm/capabilities.go index 42125a2501..f2d5977781 100644 --- a/internal/uvm/capabilities.go +++ b/internal/uvm/capabilities.go @@ -21,6 +21,17 @@ func (uvm *UtilityVM) DeleteContainerStateSupported() bool { return uvm.guestCaps.IsDeleteContainerStateSupported() } +// HostProcessContainerSupported returns if the WCOW UVM supports +// running host process containers. +func (uvm *UtilityVM) HostProcessContainerSupported() bool { + if uvm.OS() != "windows" { + return false + } + + wcaps := gcs.GetWCOWCapabilities(uvm.gc.Capabilities()) + return wcaps != nil && wcaps.HostProcessContainerSupported +} + // Capabilities returns the protocol version and the guest defined capabilities. // This should only be used for testing. func (uvm *UtilityVM) Capabilities() (uint32, gcs.GuestDefinedCapabilities) { From d2096364f3f569d1986acffe67abae95cc6569ad Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Thu, 21 May 2026 01:34:20 +0530 Subject: [PATCH 11/13] allow HPCs in any UVM Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index 3b3ed64135..78b0fb53cd 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -484,31 +484,27 @@ func (p *pod) DeleteTask(ctx context.Context, tid string) error { // updateConfigForHostProcessContainer validates and adjusts a container spec // against the pod sandbox spec for HostProcess (HPC) scenarios: -// - Rejects a hypervisor-isolated HPC inside a non-HPC sandbox. +// - Rejects a hypervisor-isolated HPC when the parent UVM does not +// advertise HostProcess container support. // - Rejects a non-HPC container inside a process-isolated HPC sandbox // (all containers in such a pod must share the host job/network). // - Propagates HPC pod-level annotations (e.g. inherit-user, rootfs // location) onto each container, since CRI only delivers them at pod // create time. -// - Forces HostProcessInheritUser=false on a non-privileged container in a -// privileged hypervisor-isolated sandbox so it can't inherit SYSTEM. +// - Clears HostProcessInheritUser on any non-hypervisor-isolated-HPC +// container so a propagated annotation can't grant it SYSTEM. func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { isProcessIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && !oci.IsIsolated(p.spec) - isHypervisorIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && oci.IsIsolated(p.spec) isProcessIsolatedPrivilegedContainer := oci.IsJobContainer(s) && !oci.IsIsolated(s) isHypervisorIsolatedPrivilegedContainer := oci.IsJobContainer(s) && oci.IsIsolated(s) - if !isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer { - return errors.New("cannot create a host process container inside sandbox which has missing annotation: microsoft.com/hostprocess-container") - } - - // If the UVM does not support HPCs, then we reject HPC calls. + // Reject hypervisor-isolated HPCs when the UVM doesn't support them. if isHypervisorIsolatedPrivilegedContainer && p.host != nil && !p.host.HostProcessContainerSupported() { return fmt.Errorf("UVM does not support HostProcess containers") } - if isProcessIsolatedPrivilegedSandbox || (isHypervisorIsolatedPrivilegedSandbox && isHypervisorIsolatedPrivilegedContainer) { + if isProcessIsolatedPrivilegedSandbox || 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. @@ -532,9 +528,7 @@ func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { ) } - if isHypervisorIsolatedPrivilegedSandbox && - !isHypervisorIsolatedPrivilegedContainer && - s.Annotations[annotations.HostProcessInheritUser] != "" { + if !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" From 930adcd5edda2bd66b340da53260e8f08f6aaca5 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Thu, 21 May 2026 13:03:08 +0530 Subject: [PATCH 12/13] HPC: tighten inherit-user scoping, harden cmd-wrapping, and fix path sanitizer - pod: clear HostProcessInheritUser on all non-HPC containers. - task_hcs: make handleProcessArgsForIsolatedJobContainer nil-safe and detect cmd/cmd.exe via filepath.Base so absolute paths are recognised and tools like `cmdkey` aren't mis-identified as cmd. - ospath.Sanitize: compare disallowed prefixes case-insensitively with a path-separator boundary (C:\WindowsBackup no longer matches C:\Windows), and drop the os.Lstat "already exists" check. - uvm.HostProcessContainerSupported: guard against a nil guest connection and use IsHostProcessContainerSupported(). - Update tests to cover the new behaviour. Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/pod.go | 10 ++--- cmd/containerd-shim-runhcs-v1/pod_test.go | 6 ++- cmd/containerd-shim-runhcs-v1/task_hcs.go | 30 ++++++++++--- .../task_hcs_test.go | 38 ++++++++++++++++ internal/ospath/path.go | 24 ++++------- internal/ospath/path_test.go | 43 +++++++++++-------- internal/uvm/capabilities.go | 4 +- 7 files changed, 104 insertions(+), 51 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go index 78b0fb53cd..c0f6b72597 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -491,8 +491,8 @@ func (p *pod) DeleteTask(ctx context.Context, tid string) error { // - Propagates HPC pod-level annotations (e.g. inherit-user, rootfs // location) onto each container, since CRI only delivers them at pod // create time. -// - Clears HostProcessInheritUser on any non-hypervisor-isolated-HPC -// container so a propagated annotation can't grant it SYSTEM. +// - Clears HostProcessInheritUser on any non-HPC container so a stray or +// propagated annotation can't grant it SYSTEM. func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { isProcessIsolatedPrivilegedSandbox := oci.IsJobContainer(p.spec) && !oci.IsIsolated(p.spec) isProcessIsolatedPrivilegedContainer := oci.IsJobContainer(s) && !oci.IsIsolated(s) @@ -528,9 +528,9 @@ func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error { ) } - if !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. + // Non-HPC containers must never carry HostProcessInheritUser - it would + // otherwise grant SYSTEM to a non-privileged exec. + if !oci.IsJobContainer(s) && s.Annotations[annotations.HostProcessInheritUser] != "" { s.Annotations[annotations.HostProcessInheritUser] = "false" } diff --git a/cmd/containerd-shim-runhcs-v1/pod_test.go b/cmd/containerd-shim-runhcs-v1/pod_test.go index 4423108b8f..c11d4959a7 100644 --- a/cmd/containerd-shim-runhcs-v1/pod_test.go +++ b/cmd/containerd-shim-runhcs-v1/pod_test.go @@ -420,8 +420,10 @@ func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) { 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", + expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{ + annotations.HostProcessContainer: "true", + }, true, ""), + expectedError: "", }, { testName: "privileged container in privileged pod (isolated hpc)", diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index 63334070e9..6b11194542 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -1044,19 +1044,35 @@ func (ht *hcsTask) requestAddContainerMount(ctx context.Context, resourcePath st // - cmd.exe always exists under System32 (so the launcher can find it) and, // once joined to the silo's job, runs inside the container's filesystem // view where it can resolve builtins and any rootfs-relative binary. -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) +func handleProcessArgsForIsolatedJobContainer(taskSpec *specs.Spec, p *specs.Process) { + if p == nil { + return + } + + // Match "cmd" / "cmd.exe" as a complete first token, including absolute + // paths like `C:\Windows\System32\cmd.exe`. + if p.CommandLine != "" { + fields := strings.Fields(p.CommandLine) + base := "" + if len(fields) > 0 { + base = strings.ToLower(filepath.Base(fields[0])) + } + if base != "cmd" && base != "cmd.exe" { + p.CommandLine = fmt.Sprintf("cmd /c %s", p.CommandLine) + } } - if len(specs.Args) > 0 && strings.TrimSpace(strings.ToLower(specs.Args[0])) != "cmd" { - specs.Args = append([]string{"cmd", "/c"}, specs.Args...) + if len(p.Args) > 0 { + base := strings.ToLower(filepath.Base(strings.TrimSpace(p.Args[0]))) + if base != "cmd" && base != "cmd.exe" { + p.Args = append([]string{"cmd", "/c"}, p.Args...) + } } // HostProcessInheritUser is set to explicit true or false during container create. - if taskSpec.Annotations[annotations.HostProcessInheritUser] == "true" { + if taskSpec != nil && 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` + p.User.Username = `NT AUTHORITY\SYSTEM` } } diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go index 397abf8cb8..beb58ffc50 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs_test.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs_test.go @@ -410,6 +410,40 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { expectedCmdLine: "", expectedArgs: []string{}, }, + { + name: "CommandLine 'cmdkey ...' – not cmd, gets prefixed", + specs: &specs.Process{CommandLine: "cmdkey /list"}, + expectedCmdLine: "cmd /c cmdkey /list", + }, + { + name: "CommandLine 'cmdtool foo' – not cmd, gets prefixed", + specs: &specs.Process{CommandLine: "cmdtool foo"}, + expectedCmdLine: "cmd /c cmdtool foo", + }, + { + name: "Args starts with 'cmd.exe' – unchanged", + specs: &specs.Process{Args: []string{"cmd.exe", "/c", "echo", "hi"}}, + expectedArgs: []string{"cmd.exe", "/c", "echo", "hi"}, + }, + { + name: "Args starts with 'cmdkey' – not cmd, gets prefixed", + specs: &specs.Process{Args: []string{"cmdkey", "/list"}}, + expectedArgs: []string{"cmd", "/c", "cmdkey", "/list"}, + }, + { + name: "CommandLine absolute path to cmd.exe – unchanged", + specs: &specs.Process{CommandLine: `C:\Windows\System32\cmd.exe /c echo hi`}, + expectedCmdLine: `C:\Windows\System32\cmd.exe /c echo hi`, + }, + { + name: "Args[0] absolute path to cmd.exe – unchanged", + specs: &specs.Process{Args: []string{`C:\Windows\System32\cmd.exe`, "/c", "echo", "hi"}}, + expectedArgs: []string{`C:\Windows\System32\cmd.exe`, "/c", "echo", "hi"}, + }, + { + name: "Nil Process – no panic", + specs: nil, + }, // --- User inheritance behavior --- { name: "HostProcessInheritUser=true – sets Username to NT AUTHORITY\\SYSTEM", @@ -456,6 +490,10 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) { handleProcessArgsForIsolatedJobContainer(taskSpec, tt.specs) + if tt.specs == nil { + return + } + if tt.specs.CommandLine != tt.expectedCmdLine { t.Errorf("CommandLine mismatch: got: %q want: %q", tt.specs.CommandLine, tt.expectedCmdLine) } diff --git a/internal/ospath/path.go b/internal/ospath/path.go index cc20cdb073..dbb44b78c0 100644 --- a/internal/ospath/path.go +++ b/internal/ospath/path.go @@ -2,8 +2,6 @@ package ospath import ( "errors" - "fmt" - "os" "path" "path/filepath" "strings" @@ -44,24 +42,18 @@ func Sanitize(path string, disallowedPrefixes []string) (string, error) { return "", errUnsafePath } - // Check if the path is not in the disallowed paths. + // Reject if cleanPath equals or is under any disallowed prefix. Compare + // case-insensitively (Windows) and enforce a path-separator boundary so + // `C:\Windows` does not block `C:\WindowsBackup`. + lowerPath := strings.ToLower(cleanPath) for _, prefix := range disallowedPrefixes { - if strings.HasPrefix(cleanPath, prefix) { + lowerPrefix := strings.ToLower(filepath.Clean(prefix)) + if lowerPath == lowerPrefix || + strings.HasPrefix(lowerPath, lowerPrefix+`\`) || + strings.HasPrefix(lowerPath, lowerPrefix+`/`) { 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 index 90368712bf..cb060792b8 100644 --- a/internal/ospath/path_test.go +++ b/internal/ospath/path_test.go @@ -1,20 +1,11 @@ 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 @@ -24,8 +15,8 @@ func TestSanitize(t *testing.T) { }{ { name: "valid path", - path: filepath.Join(existingDir, "test"), - expectedPath: filepath.Join(existingDir, "test"), + path: `C:\custom\hpc`, + expectedPath: `C:\custom\hpc`, }, { name: "empty path", @@ -33,7 +24,7 @@ func TestSanitize(t *testing.T) { expectedErrPrefix: errUnsafePath.Error(), }, { - name: "path traversal", + name: "path traversal normalizes into disallowed", path: `C:\foo\..\Windows`, disallowedPrefixes: []string{`C:\Windows`}, expectedErrPrefix: errUnsafePath.Error(), @@ -44,20 +35,34 @@ func TestSanitize(t *testing.T) { expectedErrPrefix: errUnsafePath.Error(), }, { - name: "disallowed prefix", + name: "disallowed prefix - subpath", path: `C:\Windows\System32`, disallowedPrefixes: []string{`C:\Windows`}, expectedErrPrefix: errUnsafePath.Error(), }, { - name: "existing folder", - path: existingDir, - expectedErrPrefix: "already exists", + name: "disallowed prefix - exact match", + path: `C:\Windows`, + disallowedPrefixes: []string{`C:\Windows`}, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "disallowed prefix - case insensitive", + path: `C:\windows\System32`, + disallowedPrefixes: []string{`C:\Windows`}, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "similarly-named sibling - allowed", + path: `C:\WindowsBackup`, + disallowedPrefixes: []string{`C:\Windows`}, + expectedPath: `C:\WindowsBackup`, }, { - name: "existing file", - path: existingFile, - expectedErrPrefix: "already exists", + name: "no disallowed prefixes - allowed", + path: `C:\hpc`, + disallowedPrefixes: nil, + expectedPath: `C:\hpc`, }, } diff --git a/internal/uvm/capabilities.go b/internal/uvm/capabilities.go index f2d5977781..f8bbbcfdcf 100644 --- a/internal/uvm/capabilities.go +++ b/internal/uvm/capabilities.go @@ -24,12 +24,12 @@ func (uvm *UtilityVM) DeleteContainerStateSupported() bool { // HostProcessContainerSupported returns if the WCOW UVM supports // running host process containers. func (uvm *UtilityVM) HostProcessContainerSupported() bool { - if uvm.OS() != "windows" { + if uvm.OS() != "windows" || uvm.gc == nil { return false } wcaps := gcs.GetWCOWCapabilities(uvm.gc.Capabilities()) - return wcaps != nil && wcaps.HostProcessContainerSupported + return wcaps != nil && wcaps.IsHostProcessContainerSupported() } // Capabilities returns the protocol version and the guest defined capabilities. From bcac42cf6e3a30cc413921754a88b9cde144bc38 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Fri, 22 May 2026 16:49:18 +0530 Subject: [PATCH 13/13] address review comments 2 Signed-off-by: Harsh Rawat --- cmd/containerd-shim-runhcs-v1/task_hcs.go | 11 ++++++----- internal/hcs/schema2/container.go | 4 ---- internal/ospath/path.go | 9 +++++++++ internal/ospath/path_test.go | 16 ++++++++++++++++ 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index 6b11194542..a59618137b 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -1049,9 +1049,11 @@ func handleProcessArgsForIsolatedJobContainer(taskSpec *specs.Spec, p *specs.Pro return } - // Match "cmd" / "cmd.exe" as a complete first token, including absolute - // paths like `C:\Windows\System32\cmd.exe`. - if p.CommandLine != "" { + // Wrap the process invocation with "cmd /c" so it runs via cmd.exe. Only mutate + // whichever of CommandLine/Args will actually be used (CommandLine wins if set), + // and skip if it already invokes cmd. + switch { + case p.CommandLine != "": fields := strings.Fields(p.CommandLine) base := "" if len(fields) > 0 { @@ -1060,8 +1062,7 @@ func handleProcessArgsForIsolatedJobContainer(taskSpec *specs.Spec, p *specs.Pro if base != "cmd" && base != "cmd.exe" { p.CommandLine = fmt.Sprintf("cmd /c %s", p.CommandLine) } - } - if len(p.Args) > 0 { + case len(p.Args) > 0: base := strings.ToLower(filepath.Base(strings.TrimSpace(p.Args[0]))) if base != "cmd" && base != "cmd.exe" { p.Args = append([]string{"cmd", "/c"}, p.Args...) diff --git a/internal/hcs/schema2/container.go b/internal/hcs/schema2/container.go index a60f6bb273..2272b35fb0 100644 --- a/internal/hcs/schema2/container.go +++ b/internal/hcs/schema2/container.go @@ -13,10 +13,6 @@ package hcsschema type IsolationType string const ( - // IsolationTypeProcess is a fully process-isolated, namespace-isolated - // Windows Server container. - IsolationTypeProcess IsolationType = "Process" - // IsolationTypeHostProcess is a privileged (Windows) HostProcess // container that shares the host namespace. IsolationTypeHostProcess IsolationType = "HostProcess" diff --git a/internal/ospath/path.go b/internal/ospath/path.go index dbb44b78c0..7dae37f9cf 100644 --- a/internal/ospath/path.go +++ b/internal/ospath/path.go @@ -42,6 +42,15 @@ func Sanitize(path string, disallowedPrefixes []string) (string, error) { return "", errUnsafePath } + // Require an absolute Windows path (drive letter + separator) so relative paths like + // `..\..\Windows\System32` cannot bypass the absolute-prefix checks below. + if len(cleanPath) < 3 || cleanPath[1] != ':' || + (cleanPath[2] != '\\' && cleanPath[2] != '/') || + ((cleanPath[0] < 'a' || cleanPath[0] > 'z') && + (cleanPath[0] < 'A' || cleanPath[0] > 'Z')) { + return "", errUnsafePath + } + // Reject if cleanPath equals or is under any disallowed prefix. Compare // case-insensitively (Windows) and enforce a path-separator boundary so // `C:\Windows` does not block `C:\WindowsBackup`. diff --git a/internal/ospath/path_test.go b/internal/ospath/path_test.go index cb060792b8..fa9891c237 100644 --- a/internal/ospath/path_test.go +++ b/internal/ospath/path_test.go @@ -64,6 +64,22 @@ func TestSanitize(t *testing.T) { disallowedPrefixes: nil, expectedPath: `C:\hpc`, }, + { + name: "relative parent traversal", + path: `..\..\Windows\System32`, + disallowedPrefixes: []string{`C:\Windows`}, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "drive-relative path", + path: `C:foo\bar`, + expectedErrPrefix: errUnsafePath.Error(), + }, + { + name: "rooted not absolute", + path: `\foo`, + expectedErrPrefix: errUnsafePath.Error(), + }, } for _, tt := range tests {