From cb51a0b2b319c890eff9edca052acfe34cb3b2c8 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Fri, 5 Sep 2025 00:33:21 +0530 Subject: [PATCH 1/8] 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 6a8f5ef62c..29c37eaa7a 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 { @@ -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,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 ae4b391cd6..9babf4a871 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.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(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,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 e2aa0776cb..53ccfd8594 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -490,6 +490,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 4ca4190ef4c09c9fb504de800424bbaa4262b698 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Mon, 8 Sep 2025 13:02:23 +0530 Subject: [PATCH 2/8] 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 3326ce0d5b5ce3e05bb0acaa2ced9f9efc3cdeb2 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Mon, 8 Sep 2025 15:25:38 +0530 Subject: [PATCH 3/8] 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 ffad9b739582edb0270f3da9923261b29ae89e2f Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 8 Oct 2025 02:20:10 +0530 Subject: [PATCH 4/8] 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 9babf4a871..28e40de7d5 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -994,14 +994,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 3514a559f201b3e67b85760a6cd40e8102fe7182 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 8 Oct 2025 14:15:26 +0530 Subject: [PATCH 5/8] 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 53ccfd8594..5288ab3cb9 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -496,7 +496,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 4772e1b44bf59e16cab98c4a124895e380dbce4e Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Wed, 8 Oct 2025 18:08:30 +0530 Subject: [PATCH 6/8] 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 29c37eaa7a..8dc5b1506c 100644 --- a/cmd/containerd-shim-runhcs-v1/pod.go +++ b/cmd/containerd-shim-runhcs-v1/pod.go @@ -516,18 +516,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 28e40de7d5..4b3f9ef73f 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -214,7 +214,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) @@ -370,7 +370,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) @@ -993,13 +993,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 827645fe0ac5c8c2be815b282be1bc6d64a9f10f Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Sat, 1 Nov 2025 02:46:01 +0530 Subject: [PATCH 7/8] 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 8dc5b1506c..6f2d3b2d5f 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 5288ab3cb9..813e5cd1ca 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" @@ -496,7 +497,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 eda509e5dffc2b2398729e7e47fa7139bd253d42 Mon Sep 17 00:00:00 2001 From: Harsh Rawat Date: Fri, 21 Nov 2025 21:59:43 +0530 Subject: [PATCH 8/8] 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 813e5cd1ca..a5f7b3101e 100644 --- a/internal/hcsoci/hcsdoc_wcow.go +++ b/internal/hcsoci/hcsdoc_wcow.go @@ -493,7 +493,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 {