Skip to content

Commit 2b6af52

Browse files
rawaharsrawahars
committed
Rectified the logic for user inherit functionality for HPCs
Signed-off-by: Harsh Rawat <[email protected]>
1 parent f3aa11f commit 2b6af52

File tree

4 files changed

+106
-64
lines changed

4 files changed

+106
-64
lines changed

‎cmd/containerd-shim-runhcs-v1/pod.go‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -516,18 +516,12 @@ func (p *pod) updateConfigForHostProcessContainer(s *specs.Spec) error {
516516
)
517517
}
518518

519-
if isHypervisorIsolatedPrivilegedSandbox {
520-
if isHypervisorIsolatedPrivilegedContainer && s.Annotations[annotations.HostProcessInheritUser] == "true" {
521-
// For privileged containers within the sandbox, if the annotation to inherit user is set
522-
// we will set the user to NT AUTHORITY\SYSTEM.
523-
s.Process.User.Username = `NT AUTHORITY\SYSTEM`
524-
}
525-
526-
if !isHypervisorIsolatedPrivilegedContainer && s.Annotations[annotations.HostProcessInheritUser] != "" {
527-
// If the hypervisor isolated sandbox was privileged but the container is non-privileged, then
528-
// we will explicitly disable the annotation which allows privileged user with the exec.
529-
s.Annotations[annotations.HostProcessInheritUser] = "false"
530-
}
519+
if isHypervisorIsolatedPrivilegedSandbox &&
520+
!isHypervisorIsolatedPrivilegedContainer &&
521+
s.Annotations[annotations.HostProcessInheritUser] != "" {
522+
// If the hypervisor isolated sandbox was privileged but the container is non-privileged, then
523+
// we will explicitly disable the annotation which allows privileged user with the exec.
524+
s.Annotations[annotations.HostProcessInheritUser] = "false"
531525
}
532526

533527
return nil

‎cmd/containerd-shim-runhcs-v1/pod_test.go‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,6 @@ func getWCOWSpecsWithAnnotations(annotation map[string]string, isIsolated bool,
407407
}
408408

409409
func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) {
410-
ntAuthorityUser := `NT AUTHORITY\SYSTEM`
411-
412410
testCases := []struct {
413411
testName string
414412
podSpec *specs.Spec
@@ -470,7 +468,7 @@ func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) {
470468
annotations.HostProcessContainer: "true",
471469
annotations.HostProcessInheritUser: "true",
472470
annotations.HostProcessRootfsLocation: "C:\\test",
473-
}, true, ntAuthorityUser),
471+
}, true, ""),
474472
expectedError: "",
475473
},
476474
{
@@ -500,21 +498,6 @@ func Test_pod_UpdateConfigForHostProcessContainer(t *testing.T) {
500498
expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{}, true, ""),
501499
expectedError: "",
502500
},
503-
{
504-
testName: "set user process for inherit annotation (isolated hpc)",
505-
podSpec: getWCOWSpecsWithAnnotations(map[string]string{
506-
annotations.HostProcessContainer: "true",
507-
annotations.HostProcessInheritUser: "true",
508-
}, true, ""),
509-
containerSpec: getWCOWSpecsWithAnnotations(map[string]string{
510-
annotations.HostProcessContainer: "true",
511-
}, true, ""),
512-
expectedContainerSpec: getWCOWSpecsWithAnnotations(map[string]string{
513-
annotations.HostProcessContainer: "true",
514-
annotations.HostProcessInheritUser: "true",
515-
}, true, ntAuthorityUser),
516-
expectedError: "",
517-
},
518501
{
519502
testName: "no changes in user process for normal containers (isolated hpc)",
520503
podSpec: getWCOWSpecsWithAnnotations(map[string]string{

‎cmd/containerd-shim-runhcs-v1/task_hcs.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ func newHcsTask(
214214
// For Isolated Job containers, we need special handling wherein if the command line
215215
// is not specified, then we add 'cmd /C' by default.
216216
if oci.IsIsolatedJobContainer(s) {
217-
handleProcessArgsForIsolatedJobContainer(s.Process)
217+
handleProcessArgsForIsolatedJobContainer(s, s.Process)
218218
}
219219

220220
// Default to an infinite timeout (zero value)
@@ -370,7 +370,7 @@ func (ht *hcsTask) CreateExec(ctx context.Context, req *task.ExecProcessRequest,
370370
}
371371

372372
if oci.IsIsolatedJobContainer(ht.taskSpec) {
373-
handleProcessArgsForIsolatedJobContainer(spec)
373+
handleProcessArgsForIsolatedJobContainer(ht.taskSpec, spec)
374374
}
375375

376376
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
993993
return ht.c.Modify(ctx, modification)
994994
}
995995

996-
func handleProcessArgsForIsolatedJobContainer(specs *specs.Process) {
996+
func handleProcessArgsForIsolatedJobContainer(taskSpec *specs.Spec, specs *specs.Process) {
997997
if specs.CommandLine != "" && !strings.HasPrefix(strings.TrimSpace(strings.ToLower(specs.CommandLine)), "cmd") {
998998
specs.CommandLine = fmt.Sprintf("cmd /c %s", specs.CommandLine)
999999
}
10001000
if len(specs.Args) > 0 && strings.TrimSpace(strings.ToLower(specs.Args[0])) != "cmd" {
10011001
specs.Args = append([]string{"cmd", "/c"}, specs.Args...)
10021002
}
1003+
1004+
// HostProcessInheritUser is set to explicit true or false during container create.
1005+
if taskSpec.Annotations[annotations.HostProcessInheritUser] == "true" {
1006+
// For privileged containers within the sandbox, if the annotation to inherit user is set
1007+
// we will set the user to NT AUTHORITY\SYSTEM.
1008+
specs.User.Username = `NT AUTHORITY\SYSTEM`
1009+
}
10031010
}
10041011

10051012
func isMountTypeSupported(hostPath, mountType string) bool {

‎cmd/containerd-shim-runhcs-v1/task_hcs_test.go‎

Lines changed: 89 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"testing"
1111
"time"
1212

13+
"github.com/Microsoft/hcsshim/pkg/annotations"
1314
"github.com/containerd/errdefs"
1415
"github.com/opencontainers/runtime-spec/specs-go"
1516
)
@@ -322,96 +323,153 @@ func Test_hcsTask_DeleteExec_2ndExecID_ExitedState_Success(t *testing.T) {
322323
}
323324

324325
func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) {
326+
ntAuthorityUser := `NT AUTHORITY\SYSTEM`
327+
testUserName := "testUser"
328+
325329
tests := []struct {
326-
name string
327-
specs *specs.Process
328-
expectedCmdLine string
329-
expectedArgs []string
330+
name string
331+
taskAnnotations map[string]string
332+
specs *specs.Process
333+
expectedCmdLine string
334+
expectedArgs []string
335+
initialUsername string
336+
expectedUsername string
330337
}{
331338
{
332339
name: "CommandLine starts with 'cmd' (lowercase) – unchanged",
333340
specs: &specs.Process{CommandLine: "cmd /c dir"},
334341
expectedCmdLine: "cmd /c dir",
335-
expectedArgs: nil,
336342
},
337343
{
338344
name: "CommandLine starts with 'CMD' (uppercase) – unchanged",
339345
specs: &specs.Process{CommandLine: "CMD /C whoami"},
340346
expectedCmdLine: "CMD /C whoami",
341-
expectedArgs: nil,
342347
},
343348
{
344349
name: "CommandLine starts with 'cmd.exe' – unchanged",
345350
specs: &specs.Process{CommandLine: "cmd.exe /c ipconfig"},
346351
expectedCmdLine: "cmd.exe /c ipconfig",
347-
expectedArgs: nil,
348352
},
349353
{
350354
name: "CommandLine plain – gets prefixed with 'cmd /c '",
351355
specs: &specs.Process{CommandLine: "echo hello"},
352356
expectedCmdLine: "cmd /c echo hello",
353-
expectedArgs: nil,
354357
},
355358
{
356359
name: "CommandLine mixed case 'CmD' – unchanged",
357360
specs: &specs.Process{CommandLine: "CmD /c ping 127.0.0.1"},
358361
expectedCmdLine: "CmD /c ping 127.0.0.1",
359-
expectedArgs: nil,
360362
},
361363
{
362-
name: "Args plain – gets ['cmd','/c',...] prefix",
363-
specs: &specs.Process{Args: []string{"echo", "hello"}},
364-
expectedCmdLine: "",
365-
expectedArgs: []string{"cmd", "/c", "echo", "hello"},
364+
name: "CommandLine has leading spaces before 'cmd' – unchanged",
365+
specs: &specs.Process{CommandLine: " cmd /c echo spaced"},
366+
expectedCmdLine: " cmd /c echo spaced",
366367
},
367368
{
368-
name: "Args already start with 'CMD' (uppercase) – unchanged",
369-
specs: &specs.Process{Args: []string{"CMD", "/C", "echo", "hi"}},
370-
expectedCmdLine: "",
371-
expectedArgs: []string{"CMD", "/C", "echo", "hi"},
369+
name: "CommandLine has leading spaces before 'CMD' – unchanged",
370+
specs: &specs.Process{CommandLine: " CMD /C echo spaced"},
371+
expectedCmdLine: " CMD /C echo spaced",
372372
},
373373
{
374-
name: "Args already start with 'cmd' (lowercase) – unchanged",
375-
specs: &specs.Process{Args: []string{"cmd", "/c", "type", "file.txt"}},
376-
expectedCmdLine: "",
377-
expectedArgs: []string{"cmd", "/c", "type", "file.txt"},
374+
name: "CommandLine whitespace-only – gets prefixed preserving spaces",
375+
specs: &specs.Process{CommandLine: " "},
376+
expectedCmdLine: "cmd /c ",
377+
},
378+
{
379+
name: "Args plain – gets ['cmd','/c',...] prefix",
380+
specs: &specs.Process{Args: []string{"echo", "hello"}},
381+
expectedArgs: []string{"cmd", "/c", "echo", "hello"},
382+
},
383+
{
384+
name: "Args already start with 'CMD' (uppercase) – unchanged",
385+
specs: &specs.Process{Args: []string{"CMD", "/C", "echo", "hi"}},
386+
expectedArgs: []string{"CMD", "/C", "echo", "hi"},
387+
},
388+
{
389+
name: "Args already start with 'cmd' (lowercase) – unchanged",
390+
specs: &specs.Process{Args: []string{"cmd", "/c", "type", "file.txt"}},
391+
expectedArgs: []string{"cmd", "/c", "type", "file.txt"},
392+
},
393+
{
394+
name: "Args first element mixed case 'Cmd' – unchanged",
395+
specs: &specs.Process{Args: []string{"Cmd", "/c", "echo", "hi"}},
396+
expectedArgs: []string{"Cmd", "/c", "echo", "hi"},
397+
},
398+
{
399+
name: "Args first element has leading/trailing spaces ' CMD ' – unchanged (trimmed comparison)",
400+
specs: &specs.Process{Args: []string{" CMD ", "/C", "echo", "trimmed"}},
401+
expectedArgs: []string{" CMD ", "/C", "echo", "trimmed"},
378402
},
379403
{
380404
name: "Empty CommandLine and empty Args – unchanged",
381405
specs: &specs.Process{},
382406
expectedCmdLine: "",
383-
expectedArgs: nil,
384407
},
385408
{
386409
name: "Empty CommandLine and empty slice Args – unchanged (empty slice preserved)",
387410
specs: &specs.Process{Args: []string{}},
388411
expectedCmdLine: "",
389412
expectedArgs: []string{},
390413
},
414+
// --- User inheritance behavior ---
391415
{
392-
name: "CommandLine has leading spaces before 'cmd' – treated as not starting with cmd - unchanged",
393-
specs: &specs.Process{CommandLine: " cmd /c echo spaced"},
394-
expectedCmdLine: " cmd /c echo spaced",
395-
expectedArgs: nil,
416+
name: "HostProcessInheritUser=true – sets Username to NT AUTHORITY\\SYSTEM",
417+
taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "true"},
418+
specs: &specs.Process{},
419+
expectedUsername: ntAuthorityUser,
396420
},
397421
{
398-
name: "Args first element mixed case 'Cmd' – unchanged",
399-
specs: &specs.Process{Args: []string{"Cmd", "/c", "echo", "hi"}},
400-
expectedCmdLine: "",
401-
expectedArgs: []string{"Cmd", "/c", "echo", "hi"},
422+
name: "HostProcessInheritUser=false – does not set Username",
423+
taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "false"},
424+
specs: &specs.Process{User: specs.User{Username: testUserName}},
425+
expectedUsername: testUserName,
426+
},
427+
{
428+
name: "HostProcessInheritUser missing – does not set Username",
429+
taskAnnotations: map[string]string{},
430+
specs: &specs.Process{User: specs.User{Username: testUserName}},
431+
expectedUsername: testUserName,
432+
},
433+
{
434+
name: "HostProcessInheritUser=true – overrides preexisting Username",
435+
taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "true"},
436+
specs: &specs.Process{User: specs.User{Username: testUserName}},
437+
expectedUsername: ntAuthorityUser,
438+
},
439+
{
440+
name: "HostProcessInheritUser=false – preserves preexisting Username",
441+
taskAnnotations: map[string]string{annotations.HostProcessInheritUser: "false"},
442+
specs: &specs.Process{User: specs.User{Username: testUserName}},
443+
expectedUsername: testUserName,
444+
},
445+
{
446+
name: "Nil annotation map – safely no change to Username",
447+
// Note: Annotations=nil is fine; indexing a nil map returns zero value
448+
taskAnnotations: nil,
449+
specs: &specs.Process{User: specs.User{Username: testUserName}},
450+
expectedUsername: testUserName,
402451
},
403452
}
404453

405454
for _, tt := range tests {
406455
t.Run(tt.name, func(t *testing.T) {
407-
handleProcessArgsForIsolatedJobContainer(tt.specs)
456+
taskSpec := &specs.Spec{Annotations: tt.taskAnnotations}
457+
// Ensure initial Username is set if provided
458+
if tt.initialUsername != "" {
459+
tt.specs.User.Username = tt.initialUsername
460+
}
461+
462+
handleProcessArgsForIsolatedJobContainer(taskSpec, tt.specs)
408463

409464
if tt.specs.CommandLine != tt.expectedCmdLine {
410465
t.Errorf("CommandLine mismatch: got: %q want: %q", tt.specs.CommandLine, tt.expectedCmdLine)
411466
}
412467
if !reflect.DeepEqual(tt.specs.Args, tt.expectedArgs) {
413468
t.Errorf("Args mismatch: got: %#v want: %#v", tt.specs.Args, tt.expectedArgs)
414469
}
470+
if tt.specs.User.Username != tt.expectedUsername {
471+
t.Errorf("Username mismatch: got: %q want: %q", tt.specs.User.Username, tt.expectedUsername)
472+
}
415473
})
416474
}
417475
}

0 commit comments

Comments
 (0)