From 54157202002e2b1a26880823aeaafbf894c0c15d Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Wed, 26 Jan 2022 13:39:31 -0800 Subject: [PATCH 1/3] Add local user account creation for host process containers This change is experimental for now. This allows a user the ability to pass a Windows group name as the username for the container. What happens in this case is: 1. Client provides a Windows group name as the username for the container 2. We validate that it's a group, and if so then make a temporary local account. 3. Add local account to the group passed in and run the container under the account. 4. On container exit delete the account. This allows a client to setup a group with whatever permissions/restrictions needed on it and have any host process containers run as a user in the group. The blocker for not just creating a user itself with the right permissions is there's no cri field to pass a user password, and passwordless logon seems to be blocked on Windows by default. Signed-off-by: Daniel Canter --- cmd/containerd-shim-runhcs-v1/delete.go | 26 +++- internal/jobcontainers/jobcontainer.go | 68 ++++++---- internal/jobcontainers/logon.go | 127 ++++++++++++++++-- internal/winapi/jobobject.go | 3 + internal/winapi/path.go | 1 + internal/winapi/system.go | 1 + internal/winapi/thread.go | 1 + internal/winapi/user.go | 94 +++++++++++++ internal/winapi/winapi.go | 2 +- internal/winapi/zsyscall_windows.go | 37 +++++ test/cri-containerd/jobcontainer_test.go | 112 +++++++++++++-- .../hcsshim/internal/winapi/jobobject.go | 3 + .../Microsoft/hcsshim/internal/winapi/path.go | 1 + .../hcsshim/internal/winapi/system.go | 1 + .../hcsshim/internal/winapi/thread.go | 1 + .../Microsoft/hcsshim/internal/winapi/user.go | 94 +++++++++++++ .../hcsshim/internal/winapi/winapi.go | 2 +- .../internal/winapi/zsyscall_windows.go | 37 +++++ 18 files changed, 561 insertions(+), 50 deletions(-) create mode 100644 internal/winapi/user.go create mode 100644 test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go diff --git a/cmd/containerd-shim-runhcs-v1/delete.go b/cmd/containerd-shim-runhcs-v1/delete.go index 5f471226e1..bfb74401ca 100644 --- a/cmd/containerd-shim-runhcs-v1/delete.go +++ b/cmd/containerd-shim-runhcs-v1/delete.go @@ -9,12 +9,14 @@ import ( "github.com/Microsoft/hcsshim/internal/hcs" "github.com/Microsoft/hcsshim/internal/oc" + "github.com/Microsoft/hcsshim/internal/winapi" "github.com/containerd/containerd/runtime/v2/task" "github.com/gogo/protobuf/proto" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/urfave/cli" "go.opencensus.io/trace" + "golang.org/x/sys/windows" ) // LimitedRead reads at max `readLimitBytes` bytes from the file at path `filePath`. If the file has @@ -49,8 +51,8 @@ The delete command will be executed in the container's bundle as its cwd. SkipArgReorder: true, Action: func(context *cli.Context) (err error) { // We cant write anything to stdout for this cmd other than the - // task.DeleteResponse by protcol. We can write to stderr which will be - // warning logged in containerd. + // task.DeleteResponse by protocol. We can write to stderr which will be + // logged as a warning in containerd. ctx, span := trace.StartSpan(gcontext.Background(), "delete") defer span.End() @@ -99,6 +101,26 @@ The delete command will be executed in the container's bundle as its cwd. } } + // For Host Process containers if a group name is passed as the user for the container the shim will create a + // temporary user for the container to run as and add it to the specified group. On container exit the account will + // be deleted, but if the shim crashed unexpectedly (panic, terminated etc.) then the account may still be around. + // The username will be the container ID so try and delete it here. The username character limit is 20, so we need to + // slice down the container ID a bit. + username := idFlag[:winapi.UserNameCharLimit] + usrNameUTF16, err := windows.UTF16PtrFromString(username) + if err == nil { + // Always try and delete the user, if it doesn't exist we'll get a specific error code that we can use to + // not log any warnings. + if err := winapi.NetUserDel( + nil, + usrNameUTF16, + ); err != nil && err != winapi.NERR_UserNotFound { + fmt.Fprintf(os.Stderr, "failed to delete user '%s': %v", username, err) + } + } else { + fmt.Fprintf(os.Stderr, "failed to encode user '%s' to UTF16: %v", username, err) + } + if data, err := proto.Marshal(&task.DeleteResponse{ ExitedAt: time.Now(), ExitStatus: 255, diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go index 706d4b8cea..8d19dc297f 100644 --- a/internal/jobcontainers/jobcontainer.go +++ b/internal/jobcontainers/jobcontainer.go @@ -54,16 +54,18 @@ type initProc struct { // JobContainer represents a lightweight container composed from a job object. type JobContainer struct { - id string - spec *specs.Spec // OCI spec used to create the container - job *jobobject.JobObject // Object representing the job object the container owns - sandboxMount string // Path to where the sandbox is mounted on the host - closedWaitOnce sync.Once - init initProc - startTimestamp time.Time - exited chan struct{} - waitBlock chan struct{} - waitError error + id string + spec *specs.Spec // OCI spec used to create the container + job *jobobject.JobObject // Object representing the job object the container owns + sandboxMount string // Path to where the sandbox is mounted on the host + closedWaitOnce sync.Once + init initProc + token windows.Token + localUserAccount string + startTimestamp time.Time + exited chan struct{} + waitBlock chan struct{} + waitError error } var _ cow.ProcessHost = &JobContainer{} @@ -205,21 +207,23 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_ return nil, errors.Wrapf(err, "failed to get application name from commandline %q", conf.CommandLine) } - var token windows.Token - if inheritUserTokenIsSet(c.spec.Annotations) { - token, err = openCurrentProcessToken() - if err != nil { - return nil, err - } - } else { - token, err = processToken(conf.User) - if err != nil { - return nil, errors.Wrap(err, "failed to create user process token") + // We've already done the user dance already, so this is most likely an exec being launched. Just use the token we + // already retrieved when we made the init process. + if c.token == 0 { + if inheritUserTokenIsSet(c.spec.Annotations) { + c.token, err = openCurrentProcessToken() + if err != nil { + return nil, err + } + } else { + c.token, err = c.processToken(ctx, conf.User) + if err != nil { + return nil, fmt.Errorf("failed to create user process token: %w", err) + } } } - defer token.Close() - env, err := defaultEnvBlock(token) + env, err := defaultEnvBlock(c.token) if err != nil { return nil, errors.Wrap(err, "failed to get default environment block") } @@ -257,7 +261,7 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_ commandLine, exec.WithDir(workDir), exec.WithEnv(env), - exec.WithToken(token), + exec.WithToken(c.token), exec.WithJobObject(c.job), exec.WithConPty(cpty), exec.WithProcessFlags(windows.CREATE_BREAKAWAY_FROM_JOB), @@ -314,11 +318,27 @@ func (c *JobContainer) Start(ctx context.Context) error { return nil } -// Close closes any open handles. +// Close free's up any resources (handles, temporary accounts). func (c *JobContainer) Close() error { if err := c.job.Close(); err != nil { return err } + + if err := c.token.Close(); err != nil { + return fmt.Errorf("failed to close token: %w", err) + } + + // Delete the containers local account if one was created + if c.localUserAccount != "" { + userName, err := windows.UTF16PtrFromString(c.localUserAccount) + if err != nil { + return fmt.Errorf("failed to encode user name %q to UTF16: %w", c.localUserAccount, err) + } + if err := winapi.NetUserDel(nil, userName); err != nil { + return fmt.Errorf("failed to delete local user account: %w", err) + } + } + c.closedWaitOnce.Do(func() { c.waitError = hcs.ErrAlreadyClosed close(c.waitBlock) diff --git a/internal/jobcontainers/logon.go b/internal/jobcontainers/logon.go index 01ac968c04..b655a86eff 100644 --- a/internal/jobcontainers/logon.go +++ b/internal/jobcontainers/logon.go @@ -1,35 +1,142 @@ package jobcontainers import ( + "context" "fmt" "strings" + "unsafe" + "github.com/Microsoft/go-winio/pkg/guid" + "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/winapi" "github.com/pkg/errors" "golang.org/x/sys/windows" ) -// processToken returns a user token for the user specified by `user`. This should be in the form -// of either a DOMAIN\username or just username. -func processToken(user string) (windows.Token, error) { +func randomPswd() (*uint16, error) { + g, err := guid.NewV4() + if err != nil { + return nil, err + } + return windows.UTF16PtrFromString(g.String()) +} + +func groupExists(groupName string) bool { + groupNameUTF16, err := windows.UTF16PtrFromString(groupName) + if err != nil { + return false + } + var p *byte + err = winapi.NetLocalGroupGetInfo( + nil, + groupNameUTF16, + 1, + &p, + ) + if err != nil { + return false + } + defer windows.NetApiBufferFree(p) + return true +} + +// makeLocalAccount creates a local account with the passed in username and a randomly generated password. +// If `groupName` is not an empty string the user will be added to group if it exists. +func makeLocalAccount(ctx context.Context, user, groupName string) (*uint16, error) { + // Create a local account with a random password + pswd, err := randomPswd() + if err != nil { + return nil, fmt.Errorf("failed to generate random password: %w", err) + } + + userUTF16, err := windows.UTF16PtrFromString(user) + if err != nil { + return nil, fmt.Errorf("failed to encode username to UTF16: %w", err) + } + + usr1 := &winapi.UserInfo1{ + Name: userUTF16, + Password: pswd, + Priv: winapi.USER_PRIV_USER, + Flags: winapi.UF_NORMAL_ACCOUNT | winapi.UF_DONT_EXPIRE_PASSWD, + } + if err := winapi.NetUserAdd( + nil, + 1, + (*byte)(unsafe.Pointer(usr1)), + nil, + ); err != nil { + return nil, fmt.Errorf("failed to create user %s: %w", user, err) + } + + log.G(ctx).WithField("username", user).Debug("Created local user account for job container") + + sid, _, _, err := windows.LookupSID("", user) + if err != nil { + return nil, fmt.Errorf("failed to lookup SID for user %q: %w", user, err) + } + + groupNameUTF16, err := windows.UTF16PtrFromString(groupName) + if err != nil { + return nil, fmt.Errorf("failed to encode group name to UTF16: %w", err) + } + sids := []winapi.LocalGroupMembersInfo0{{Sid: sid}} + if err := winapi.NetLocalGroupAddMembers( + nil, + groupNameUTF16, + 0, + (*byte)(unsafe.Pointer(&sids[0])), + 1, + ); err != nil { + return nil, fmt.Errorf("failed to add user %q to the %q group: %w", user, groupName, err) + } + + return pswd, nil +} + +// processToken verifies first whether userOrGroup is a username or group name. If it's a valid group name, +// a temporary local user account will be created and added to the group and then the token for the user will +// be returned. If it is not a group name then the user will logged into and the token will be returned. +func (c *JobContainer) processToken(ctx context.Context, userOrGroup string) (windows.Token, error) { var ( domain string userName string token windows.Token ) - split := strings.Split(user, "\\") + if userOrGroup == "" { + return 0, errors.New("empty username or group name passed") + } + + if groupExists(userOrGroup) { + userName = c.id[:winapi.UserNameCharLimit] + pswd, err := makeLocalAccount(ctx, userName, userOrGroup) + if err != nil { + return 0, fmt.Errorf("failed to create local account for container: %w", err) + } + if err := winapi.LogonUser( + windows.StringToUTF16Ptr(userName), + nil, + pswd, + winapi.LOGON32_LOGON_INTERACTIVE, + winapi.LOGON32_PROVIDER_DEFAULT, + &token, + ); err != nil { + return 0, fmt.Errorf("failed to logon user: %w", err) + } + c.localUserAccount = userName + return token, nil + } + + // Must be a user string, split it by domain and username + split := strings.Split(userOrGroup, "\\") if len(split) == 2 { domain = split[0] userName = split[1] } else if len(split) == 1 { userName = split[0] } else { - return 0, fmt.Errorf("invalid user string `%s`", user) - } - - if user == "" { - return 0, errors.New("empty user string passed") + return 0, fmt.Errorf("invalid user string `%s`", userOrGroup) } logonType := winapi.LOGON32_LOGON_INTERACTIVE @@ -46,7 +153,7 @@ func processToken(user string) (windows.Token, error) { winapi.LOGON32_PROVIDER_DEFAULT, &token, ); err != nil { - return 0, errors.Wrap(err, "failed to logon user") + return 0, fmt.Errorf("failed to logon user: %w", err) } return token, nil } diff --git a/internal/winapi/jobobject.go b/internal/winapi/jobobject.go index 35d5b11fda..b2ca47c02b 100644 --- a/internal/winapi/jobobject.go +++ b/internal/winapi/jobobject.go @@ -195,6 +195,7 @@ type JOBOBJECT_ASSOCIATE_COMPLETION_PORT struct { // JOBOBJECT_IO_RATE_CONTROL_INFORMATION **InfoBlocks, // ULONG *InfoBlockCount // ); +// //sys QueryIoRateControlInformationJobObject(jobHandle windows.Handle, volumeName *uint16, ioRateControlInfo **JOBOBJECT_IO_RATE_CONTROL_INFORMATION, infoBlockCount *uint32) (ret uint32, err error) = kernel32.QueryIoRateControlInformationJobObject // NTSTATUS @@ -203,6 +204,7 @@ type JOBOBJECT_ASSOCIATE_COMPLETION_PORT struct { // _In_ ACCESS_MASK DesiredAccess, // _In_ POBJECT_ATTRIBUTES ObjectAttributes // ); +// //sys NtOpenJobObject(jobHandle *windows.Handle, desiredAccess uint32, objAttributes *ObjectAttributes) (status uint32) = ntdll.NtOpenJobObject // NTSTATUS @@ -212,4 +214,5 @@ type JOBOBJECT_ASSOCIATE_COMPLETION_PORT struct { // _In_ ACCESS_MASK DesiredAccess, // _In_opt_ POBJECT_ATTRIBUTES ObjectAttributes // ); +// //sys NtCreateJobObject(jobHandle *windows.Handle, desiredAccess uint32, objAttributes *ObjectAttributes) (status uint32) = ntdll.NtCreateJobObject diff --git a/internal/winapi/path.go b/internal/winapi/path.go index 908920e872..c6a149b552 100644 --- a/internal/winapi/path.go +++ b/internal/winapi/path.go @@ -8,4 +8,5 @@ package winapi // LPWSTR lpBuffer, // LPWSTR *lpFilePart // ); +// //sys SearchPath(lpPath *uint16, lpFileName *uint16, lpExtension *uint16, nBufferLength uint32, lpBuffer *uint16, lpFilePath *uint16) (size uint32, err error) = kernel32.SearchPathW diff --git a/internal/winapi/system.go b/internal/winapi/system.go index 327f57d7c2..9e4009d873 100644 --- a/internal/winapi/system.go +++ b/internal/winapi/system.go @@ -12,6 +12,7 @@ const STATUS_INFO_LENGTH_MISMATCH = 0xC0000004 // ULONG SystemInformationLength, // PULONG ReturnLength // ); +// //sys NtQuerySystemInformation(systemInfoClass int, systemInformation uintptr, systemInfoLength uint32, returnLength *uint32) (status uint32) = ntdll.NtQuerySystemInformation type SYSTEM_PROCESS_INFORMATION struct { diff --git a/internal/winapi/thread.go b/internal/winapi/thread.go index 4724713e3e..f23141a836 100644 --- a/internal/winapi/thread.go +++ b/internal/winapi/thread.go @@ -9,4 +9,5 @@ package winapi // DWORD dwCreationFlags, // LPDWORD lpThreadId // ); +// //sys CreateRemoteThread(process windows.Handle, sa *windows.SecurityAttributes, stackSize uint32, startAddr uintptr, parameter uintptr, creationFlags uint32, threadID *uint32) (handle windows.Handle, err error) = kernel32.CreateRemoteThread diff --git a/internal/winapi/user.go b/internal/winapi/user.go new file mode 100644 index 0000000000..0718aabab1 --- /dev/null +++ b/internal/winapi/user.go @@ -0,0 +1,94 @@ +package winapi + +import ( + "syscall" + + "golang.org/x/sys/windows" +) + +const UserNameCharLimit = 20 + +const ( + USER_PRIV_GUEST uint32 = iota + USER_PRIV_USER + USER_PRIV_ADMIN +) + +const ( + UF_NORMAL_ACCOUNT = 0x00200 + UF_DONT_EXPIRE_PASSWD = 0x10000 +) + +const NERR_UserNotFound = syscall.Errno(0x8AD) + +// typedef struct _LOCALGROUP_MEMBERS_INFO_0 { +// PSID lgrmi0_sid; +// } LOCALGROUP_MEMBERS_INFO_0, *PLOCALGROUP_MEMBERS_INFO_0, *LPLOCALGROUP_MEMBERS_INFO_0; +type LocalGroupMembersInfo0 struct { + Sid *windows.SID +} + +// typedef struct _LOCALGROUP_INFO_1 { +// LPWSTR lgrpi1_name; +// LPWSTR lgrpi1_comment; +// } LOCALGROUP_INFO_1, *PLOCALGROUP_INFO_1, *LPLOCALGROUP_INFO_1; +type LocalGroupInfo1 struct { + Name *uint16 + Comment *uint16 +} + +// typedef struct _USER_INFO_1 { +// LPWSTR usri1_name; +// LPWSTR usri1_password; +// DWORD usri1_password_age; +// DWORD usri1_priv; +// LPWSTR usri1_home_dir; +// LPWSTR usri1_comment; +// DWORD usri1_flags; +// LPWSTR usri1_script_path; +// } USER_INFO_1, *PUSER_INFO_1, *LPUSER_INFO_1; +type UserInfo1 struct { + Name *uint16 + Password *uint16 + PasswordAge uint32 + Priv uint32 + HomeDir *uint16 + Comment *uint16 + Flags uint32 + ScriptPath *uint16 +} + +// NET_API_STATUS NET_API_FUNCTION NetLocalGroupGetInfo( +// [in] LPCWSTR servername, +// [in] LPCWSTR groupname, +// [in] DWORD level, +// [out] LPBYTE *bufptr +// ); +// +//sys NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) = netapi32.NetLocalGroupGetInfo + +// NET_API_STATUS NET_API_FUNCTION NetUserAdd( +// [in] LPCWSTR servername, +// [in] DWORD level, +// [in] LPBYTE buf, +// [out] LPDWORD parm_err +// ); +// +//sys NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) = netapi32.NetUserAdd + +// NET_API_STATUS NET_API_FUNCTION NetUserDel( +// [in] LPCWSTR servername, +// [in] LPCWSTR username +// ); +// +//sys NetUserDel(serverName *uint16, username *uint16) (status error) = netapi32.NetUserDel + +// NET_API_STATUS NET_API_FUNCTION NetLocalGroupAddMembers( +// [in] LPCWSTR servername, +// [in] LPCWSTR groupname, +// [in] DWORD level, +// [in] LPBYTE buf, +// [in] DWORD totalentries +// ); +// +//sys NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) = netapi32.NetLocalGroupAddMembers diff --git a/internal/winapi/winapi.go b/internal/winapi/winapi.go index 1d4ba3c4f8..2614851da2 100644 --- a/internal/winapi/winapi.go +++ b/internal/winapi/winapi.go @@ -2,4 +2,4 @@ // be thought of as an extension to golang.org/x/sys/windows. package winapi -//go:generate go run ..\..\mksyscall_windows.go -output zsyscall_windows.go console.go system.go net.go path.go thread.go iocp.go jobobject.go logon.go memory.go process.go processor.go devices.go filesystem.go errors.go +//go:generate go run ..\..\mksyscall_windows.go -output zsyscall_windows.go user.go console.go system.go net.go path.go thread.go iocp.go jobobject.go logon.go memory.go process.go processor.go devices.go filesystem.go errors.go diff --git a/internal/winapi/zsyscall_windows.go b/internal/winapi/zsyscall_windows.go index 4eb64b4c0c..537dc99b29 100644 --- a/internal/winapi/zsyscall_windows.go +++ b/internal/winapi/zsyscall_windows.go @@ -37,12 +37,17 @@ func errnoErr(e syscall.Errno) error { } var ( + modnetapi32 = windows.NewLazySystemDLL("netapi32.dll") modkernel32 = windows.NewLazySystemDLL("kernel32.dll") modntdll = windows.NewLazySystemDLL("ntdll.dll") modiphlpapi = windows.NewLazySystemDLL("iphlpapi.dll") modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") modcfgmgr32 = windows.NewLazySystemDLL("cfgmgr32.dll") + procNetLocalGroupGetInfo = modnetapi32.NewProc("NetLocalGroupGetInfo") + procNetUserAdd = modnetapi32.NewProc("NetUserAdd") + procNetUserDel = modnetapi32.NewProc("NetUserDel") + procNetLocalGroupAddMembers = modnetapi32.NewProc("NetLocalGroupAddMembers") procCreatePseudoConsole = modkernel32.NewProc("CreatePseudoConsole") procClosePseudoConsole = modkernel32.NewProc("ClosePseudoConsole") procResizePseudoConsole = modkernel32.NewProc("ResizePseudoConsole") @@ -73,6 +78,38 @@ var ( procRtlNtStatusToDosError = modntdll.NewProc("RtlNtStatusToDosError") ) +func NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) { + r0, _, _ := syscall.Syscall6(procNetLocalGroupGetInfo.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(bufptr)), 0, 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + +func NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) { + r0, _, _ := syscall.Syscall6(procNetUserAdd.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(unsafe.Pointer(parm_err)), 0, 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + +func NetUserDel(serverName *uint16, username *uint16) (status error) { + r0, _, _ := syscall.Syscall(procNetUserDel.Addr(), 2, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(username)), 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + +func NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) { + r0, _, _ := syscall.Syscall6(procNetLocalGroupAddMembers.Addr(), 5, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(totalEntries), 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + func createPseudoConsole(size uint32, hInput windows.Handle, hOutput windows.Handle, dwFlags uint32, hpcon *windows.Handle) (hr error) { r0, _, _ := syscall.Syscall6(procCreatePseudoConsole.Addr(), 5, uintptr(size), uintptr(hInput), uintptr(hOutput), uintptr(dwFlags), uintptr(unsafe.Pointer(hpcon)), 0) if int32(r0) < 0 { diff --git a/test/cri-containerd/jobcontainer_test.go b/test/cri-containerd/jobcontainer_test.go index 05397c8036..0bcccf16c6 100644 --- a/test/cri-containerd/jobcontainer_test.go +++ b/test/cri-containerd/jobcontainer_test.go @@ -16,6 +16,7 @@ import ( "github.com/Microsoft/go-winio/vhd" "github.com/Microsoft/hcsshim/hcn" + "github.com/Microsoft/hcsshim/internal/winapi" "github.com/Microsoft/hcsshim/pkg/annotations" runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" ) @@ -30,7 +31,11 @@ func getJobContainerPodRequestWCOW(t *testing.T) *runtime.RunPodSandboxRequest { ) } -func getJobContainerRequestWCOW(t *testing.T, podID string, podConfig *runtime.PodSandboxConfig, image string, mounts []*runtime.Mount) *runtime.CreateContainerRequest { +func getJobContainerRequestWCOW(t *testing.T, podID string, podConfig *runtime.PodSandboxConfig, image string, user string, mounts []*runtime.Mount) *runtime.CreateContainerRequest { + inheritUser := "true" + if user != "" { + inheritUser = "false" + } return &runtime.CreateContainerRequest{ Config: &runtime.ContainerConfig{ Metadata: &runtime.ContainerMetadata{ @@ -49,9 +54,13 @@ func getJobContainerRequestWCOW(t *testing.T, podID string, podConfig *runtime.P Mounts: mounts, Annotations: map[string]string{ annotations.HostProcessContainer: "true", - annotations.HostProcessInheritUser: "true", + annotations.HostProcessInheritUser: inheritUser, + }, + Windows: &runtime.WindowsContainerConfig{ + SecurityContext: &runtime.WindowsContainerSecurityContext{ + RunAsUsername: user, + }, }, - Windows: &runtime.WindowsContainerConfig{}, }, PodSandboxId: podID, SandboxConfig: podConfig, @@ -72,7 +81,7 @@ func Test_RunContainer_InheritUser_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -111,7 +120,7 @@ func Test_RunContainer_Hostname_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -131,6 +140,85 @@ func Test_RunContainer_Hostname_JobContainer_WCOW(t *testing.T) { } } +func makeLocalGroup(name string) error { + output, err := exec.Command("net", "localgroup", name, "/add").CombinedOutput() + if err != nil { + return fmt.Errorf("failed to create localgroup %s with %s, output %s", name, err, string(output)) + } + return nil +} + +func deleteLocalGroup(name string) error { + output, err := exec.Command("net", "localgroup", name, "/delete").CombinedOutput() + if err != nil { + return fmt.Errorf("failed to delete localgroup %s with %s, output %s", name, err, string(output)) + } + return nil +} + +// Checks if userName is present in the group `groupName` +func checkLocalGroupMember(groupName, userName string) error { + output, err := exec.Command("net", "localgroup", groupName).CombinedOutput() + if err != nil { + return fmt.Errorf("failed to check members for localgroup %s with %s, output %s", groupName, err, string(output)) + } + if !strings.Contains(string(output), userName) { + return fmt.Errorf("user %s not present in the local group %s", userName, groupName) + } + return nil +} + +func Test_RunContainer_GroupName_JobContainer_WCOW(t *testing.T) { + requireFeatures(t, featureWCOWProcess, featureHostProcess) + + pullRequiredImages(t, []string{imageWindowsNanoserver}) + client := newTestRuntimeClient(t) + + // This test validates that we can create a group, pass the group name to the container and have it run as a local user account in the group. + podctx := context.Background() + sandboxRequest := getJobContainerPodRequestWCOW(t) + + podID := runPodSandbox(t, client, podctx, sandboxRequest) + defer removePodSandbox(t, client, podctx, podID) + defer stopPodSandbox(t, client, podctx, podID) + + groupName := "jobcontainer_test" + // Make the local group the container will be creating a local account in. + if err := makeLocalGroup(groupName); err != nil { + t.Fatalf("failed to make local group: %s", err) + } + + defer func() { + if err := deleteLocalGroup(groupName); err != nil { + t.Fatalf("failed to delete local group: %s", err) + } + }() + + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, groupName, nil) + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + containerID := createContainer(t, client, ctx, containerRequest) + defer removeContainer(t, client, ctx, containerID) + startContainer(t, client, ctx, containerID) + defer stopContainer(t, client, ctx, containerID) + + execResponse := execSync(t, client, ctx, &runtime.ExecSyncRequest{ + ContainerId: containerID, + Cmd: []string{"whoami"}, + }) + containerStdout := strings.Trim(string(execResponse.Stdout), " \r\n") + expectedUserName := containerID[:winapi.UserNameCharLimit] + if !strings.Contains(containerStdout, expectedUserName) { + t.Fatalf("expected whoami to be %s. got %s", expectedUserName, containerStdout) + } + + // Check if user is in the group. + if err := checkLocalGroupMember(groupName, expectedUserName); err != nil { + t.Fatal(err) + } +} + func Test_RunContainer_HNS_JobContainer_WCOW(t *testing.T) { requireFeatures(t, featureWCOWProcess, featureHostProcess) @@ -144,7 +232,7 @@ func Test_RunContainer_HNS_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerHNS, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerHNS, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -191,7 +279,7 @@ func Test_RunContainer_VHD_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerVHD, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerVHD, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -242,7 +330,7 @@ func Test_RunContainer_ETW_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerETW, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerETW, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -297,7 +385,7 @@ func Test_RunContainer_HostVolumes_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -414,7 +502,7 @@ func Test_RunContainer_JobContainer_VolumeMount(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, test.mounts) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, "", test.mounts) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -490,7 +578,7 @@ func Test_RunContainer_JobContainer_Environment(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageWindowsNanoserver, "", nil) containerRequest.Config.Envs = test.env ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() @@ -620,7 +708,7 @@ func Test_DoubleQuoting_JobContainer_WCOW(t *testing.T) { defer removePodSandbox(t, client, podctx, podID) defer stopPodSandbox(t, client, podctx, podID) - containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerCmdline, nil) + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerCmdline, "", nil) ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/jobobject.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/jobobject.go index 35d5b11fda..b2ca47c02b 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/jobobject.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/jobobject.go @@ -195,6 +195,7 @@ type JOBOBJECT_ASSOCIATE_COMPLETION_PORT struct { // JOBOBJECT_IO_RATE_CONTROL_INFORMATION **InfoBlocks, // ULONG *InfoBlockCount // ); +// //sys QueryIoRateControlInformationJobObject(jobHandle windows.Handle, volumeName *uint16, ioRateControlInfo **JOBOBJECT_IO_RATE_CONTROL_INFORMATION, infoBlockCount *uint32) (ret uint32, err error) = kernel32.QueryIoRateControlInformationJobObject // NTSTATUS @@ -203,6 +204,7 @@ type JOBOBJECT_ASSOCIATE_COMPLETION_PORT struct { // _In_ ACCESS_MASK DesiredAccess, // _In_ POBJECT_ATTRIBUTES ObjectAttributes // ); +// //sys NtOpenJobObject(jobHandle *windows.Handle, desiredAccess uint32, objAttributes *ObjectAttributes) (status uint32) = ntdll.NtOpenJobObject // NTSTATUS @@ -212,4 +214,5 @@ type JOBOBJECT_ASSOCIATE_COMPLETION_PORT struct { // _In_ ACCESS_MASK DesiredAccess, // _In_opt_ POBJECT_ATTRIBUTES ObjectAttributes // ); +// //sys NtCreateJobObject(jobHandle *windows.Handle, desiredAccess uint32, objAttributes *ObjectAttributes) (status uint32) = ntdll.NtCreateJobObject diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/path.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/path.go index 908920e872..c6a149b552 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/path.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/path.go @@ -8,4 +8,5 @@ package winapi // LPWSTR lpBuffer, // LPWSTR *lpFilePart // ); +// //sys SearchPath(lpPath *uint16, lpFileName *uint16, lpExtension *uint16, nBufferLength uint32, lpBuffer *uint16, lpFilePath *uint16) (size uint32, err error) = kernel32.SearchPathW diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/system.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/system.go index 327f57d7c2..9e4009d873 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/system.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/system.go @@ -12,6 +12,7 @@ const STATUS_INFO_LENGTH_MISMATCH = 0xC0000004 // ULONG SystemInformationLength, // PULONG ReturnLength // ); +// //sys NtQuerySystemInformation(systemInfoClass int, systemInformation uintptr, systemInfoLength uint32, returnLength *uint32) (status uint32) = ntdll.NtQuerySystemInformation type SYSTEM_PROCESS_INFORMATION struct { diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/thread.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/thread.go index 4724713e3e..f23141a836 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/thread.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/thread.go @@ -9,4 +9,5 @@ package winapi // DWORD dwCreationFlags, // LPDWORD lpThreadId // ); +// //sys CreateRemoteThread(process windows.Handle, sa *windows.SecurityAttributes, stackSize uint32, startAddr uintptr, parameter uintptr, creationFlags uint32, threadID *uint32) (handle windows.Handle, err error) = kernel32.CreateRemoteThread diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go new file mode 100644 index 0000000000..0718aabab1 --- /dev/null +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go @@ -0,0 +1,94 @@ +package winapi + +import ( + "syscall" + + "golang.org/x/sys/windows" +) + +const UserNameCharLimit = 20 + +const ( + USER_PRIV_GUEST uint32 = iota + USER_PRIV_USER + USER_PRIV_ADMIN +) + +const ( + UF_NORMAL_ACCOUNT = 0x00200 + UF_DONT_EXPIRE_PASSWD = 0x10000 +) + +const NERR_UserNotFound = syscall.Errno(0x8AD) + +// typedef struct _LOCALGROUP_MEMBERS_INFO_0 { +// PSID lgrmi0_sid; +// } LOCALGROUP_MEMBERS_INFO_0, *PLOCALGROUP_MEMBERS_INFO_0, *LPLOCALGROUP_MEMBERS_INFO_0; +type LocalGroupMembersInfo0 struct { + Sid *windows.SID +} + +// typedef struct _LOCALGROUP_INFO_1 { +// LPWSTR lgrpi1_name; +// LPWSTR lgrpi1_comment; +// } LOCALGROUP_INFO_1, *PLOCALGROUP_INFO_1, *LPLOCALGROUP_INFO_1; +type LocalGroupInfo1 struct { + Name *uint16 + Comment *uint16 +} + +// typedef struct _USER_INFO_1 { +// LPWSTR usri1_name; +// LPWSTR usri1_password; +// DWORD usri1_password_age; +// DWORD usri1_priv; +// LPWSTR usri1_home_dir; +// LPWSTR usri1_comment; +// DWORD usri1_flags; +// LPWSTR usri1_script_path; +// } USER_INFO_1, *PUSER_INFO_1, *LPUSER_INFO_1; +type UserInfo1 struct { + Name *uint16 + Password *uint16 + PasswordAge uint32 + Priv uint32 + HomeDir *uint16 + Comment *uint16 + Flags uint32 + ScriptPath *uint16 +} + +// NET_API_STATUS NET_API_FUNCTION NetLocalGroupGetInfo( +// [in] LPCWSTR servername, +// [in] LPCWSTR groupname, +// [in] DWORD level, +// [out] LPBYTE *bufptr +// ); +// +//sys NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) = netapi32.NetLocalGroupGetInfo + +// NET_API_STATUS NET_API_FUNCTION NetUserAdd( +// [in] LPCWSTR servername, +// [in] DWORD level, +// [in] LPBYTE buf, +// [out] LPDWORD parm_err +// ); +// +//sys NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) = netapi32.NetUserAdd + +// NET_API_STATUS NET_API_FUNCTION NetUserDel( +// [in] LPCWSTR servername, +// [in] LPCWSTR username +// ); +// +//sys NetUserDel(serverName *uint16, username *uint16) (status error) = netapi32.NetUserDel + +// NET_API_STATUS NET_API_FUNCTION NetLocalGroupAddMembers( +// [in] LPCWSTR servername, +// [in] LPCWSTR groupname, +// [in] DWORD level, +// [in] LPBYTE buf, +// [in] DWORD totalentries +// ); +// +//sys NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) = netapi32.NetLocalGroupAddMembers diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/winapi.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/winapi.go index 1d4ba3c4f8..2614851da2 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/winapi.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/winapi.go @@ -2,4 +2,4 @@ // be thought of as an extension to golang.org/x/sys/windows. package winapi -//go:generate go run ..\..\mksyscall_windows.go -output zsyscall_windows.go console.go system.go net.go path.go thread.go iocp.go jobobject.go logon.go memory.go process.go processor.go devices.go filesystem.go errors.go +//go:generate go run ..\..\mksyscall_windows.go -output zsyscall_windows.go user.go console.go system.go net.go path.go thread.go iocp.go jobobject.go logon.go memory.go process.go processor.go devices.go filesystem.go errors.go diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go index 4eb64b4c0c..537dc99b29 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go @@ -37,12 +37,17 @@ func errnoErr(e syscall.Errno) error { } var ( + modnetapi32 = windows.NewLazySystemDLL("netapi32.dll") modkernel32 = windows.NewLazySystemDLL("kernel32.dll") modntdll = windows.NewLazySystemDLL("ntdll.dll") modiphlpapi = windows.NewLazySystemDLL("iphlpapi.dll") modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") modcfgmgr32 = windows.NewLazySystemDLL("cfgmgr32.dll") + procNetLocalGroupGetInfo = modnetapi32.NewProc("NetLocalGroupGetInfo") + procNetUserAdd = modnetapi32.NewProc("NetUserAdd") + procNetUserDel = modnetapi32.NewProc("NetUserDel") + procNetLocalGroupAddMembers = modnetapi32.NewProc("NetLocalGroupAddMembers") procCreatePseudoConsole = modkernel32.NewProc("CreatePseudoConsole") procClosePseudoConsole = modkernel32.NewProc("ClosePseudoConsole") procResizePseudoConsole = modkernel32.NewProc("ResizePseudoConsole") @@ -73,6 +78,38 @@ var ( procRtlNtStatusToDosError = modntdll.NewProc("RtlNtStatusToDosError") ) +func NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) { + r0, _, _ := syscall.Syscall6(procNetLocalGroupGetInfo.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(bufptr)), 0, 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + +func NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) { + r0, _, _ := syscall.Syscall6(procNetUserAdd.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(unsafe.Pointer(parm_err)), 0, 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + +func NetUserDel(serverName *uint16, username *uint16) (status error) { + r0, _, _ := syscall.Syscall(procNetUserDel.Addr(), 2, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(username)), 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + +func NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) { + r0, _, _ := syscall.Syscall6(procNetLocalGroupAddMembers.Addr(), 5, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(totalEntries), 0) + if r0 != 0 { + status = syscall.Errno(r0) + } + return +} + func createPseudoConsole(size uint32, hInput windows.Handle, hOutput windows.Handle, dwFlags uint32, hpcon *windows.Handle) (hr error) { r0, _, _ := syscall.Syscall6(procCreatePseudoConsole.Addr(), 5, uintptr(size), uintptr(hInput), uintptr(hOutput), uintptr(dwFlags), uintptr(unsafe.Pointer(hpcon)), 0) if int32(r0) < 0 { From 9d242097fc5a97e0ec9de87e17fcb404ffd450a3 Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Wed, 9 Feb 2022 14:11:32 -0800 Subject: [PATCH 2/3] PR feedback 1. Fix comment regarding when to create a new token 2. Make "go friendly" wrappers for the net functions so we don't have to windows.UTF16PtrFromString at the callsite everywhere. Signed-off-by: Daniel Canter --- cmd/containerd-shim-runhcs-v1/delete.go | 21 ++-- internal/jobcontainers/jobcontainer.go | 10 +- internal/jobcontainers/logon.go | 23 ++-- internal/winapi/user.go | 106 +++++++++++++++++- internal/winapi/zsyscall_windows.go | 8 +- .../Microsoft/hcsshim/internal/winapi/user.go | 106 +++++++++++++++++- .../internal/winapi/zsyscall_windows.go | 8 +- 7 files changed, 230 insertions(+), 52 deletions(-) diff --git a/cmd/containerd-shim-runhcs-v1/delete.go b/cmd/containerd-shim-runhcs-v1/delete.go index bfb74401ca..2f8c50e597 100644 --- a/cmd/containerd-shim-runhcs-v1/delete.go +++ b/cmd/containerd-shim-runhcs-v1/delete.go @@ -16,7 +16,6 @@ import ( "github.com/sirupsen/logrus" "github.com/urfave/cli" "go.opencensus.io/trace" - "golang.org/x/sys/windows" ) // LimitedRead reads at max `readLimitBytes` bytes from the file at path `filePath`. If the file has @@ -107,18 +106,14 @@ The delete command will be executed in the container's bundle as its cwd. // The username will be the container ID so try and delete it here. The username character limit is 20, so we need to // slice down the container ID a bit. username := idFlag[:winapi.UserNameCharLimit] - usrNameUTF16, err := windows.UTF16PtrFromString(username) - if err == nil { - // Always try and delete the user, if it doesn't exist we'll get a specific error code that we can use to - // not log any warnings. - if err := winapi.NetUserDel( - nil, - usrNameUTF16, - ); err != nil && err != winapi.NERR_UserNotFound { - fmt.Fprintf(os.Stderr, "failed to delete user '%s': %v", username, err) - } - } else { - fmt.Fprintf(os.Stderr, "failed to encode user '%s' to UTF16: %v", username, err) + + // Always try and delete the user, if it doesn't exist we'll get a specific error code that we can use to + // not log any warnings. + if err := winapi.NetUserDel( + "", + username, + ); err != nil && err != winapi.NERR_UserNotFound { + fmt.Fprintf(os.Stderr, "failed to delete user %q: %v", username, err) } if data, err := proto.Marshal(&task.DeleteResponse{ diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go index 8d19dc297f..4b544470d8 100644 --- a/internal/jobcontainers/jobcontainer.go +++ b/internal/jobcontainers/jobcontainer.go @@ -207,8 +207,8 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_ return nil, errors.Wrapf(err, "failed to get application name from commandline %q", conf.CommandLine) } - // We've already done the user dance already, so this is most likely an exec being launched. Just use the token we - // already retrieved when we made the init process. + // If we haven't grabbed a token yet this is the init process being launched. Skip grabbing another token afterwards if we've already + // done the work (c.token != 0), this would typically be for an exec being launched. if c.token == 0 { if inheritUserTokenIsSet(c.spec.Annotations) { c.token, err = openCurrentProcessToken() @@ -330,11 +330,7 @@ func (c *JobContainer) Close() error { // Delete the containers local account if one was created if c.localUserAccount != "" { - userName, err := windows.UTF16PtrFromString(c.localUserAccount) - if err != nil { - return fmt.Errorf("failed to encode user name %q to UTF16: %w", c.localUserAccount, err) - } - if err := winapi.NetUserDel(nil, userName); err != nil { + if err := winapi.NetUserDel("", c.localUserAccount); err != nil { return fmt.Errorf("failed to delete local user account: %w", err) } } diff --git a/internal/jobcontainers/logon.go b/internal/jobcontainers/logon.go index b655a86eff..68175dc5c7 100644 --- a/internal/jobcontainers/logon.go +++ b/internal/jobcontainers/logon.go @@ -22,18 +22,13 @@ func randomPswd() (*uint16, error) { } func groupExists(groupName string) bool { - groupNameUTF16, err := windows.UTF16PtrFromString(groupName) - if err != nil { - return false - } var p *byte - err = winapi.NetLocalGroupGetInfo( - nil, - groupNameUTF16, + if err := winapi.NetLocalGroupGetInfo( + "", + groupName, 1, &p, - ) - if err != nil { + ); err != nil { return false } defer windows.NetApiBufferFree(p) @@ -61,7 +56,7 @@ func makeLocalAccount(ctx context.Context, user, groupName string) (*uint16, err Flags: winapi.UF_NORMAL_ACCOUNT | winapi.UF_DONT_EXPIRE_PASSWD, } if err := winapi.NetUserAdd( - nil, + "", 1, (*byte)(unsafe.Pointer(usr1)), nil, @@ -76,14 +71,10 @@ func makeLocalAccount(ctx context.Context, user, groupName string) (*uint16, err return nil, fmt.Errorf("failed to lookup SID for user %q: %w", user, err) } - groupNameUTF16, err := windows.UTF16PtrFromString(groupName) - if err != nil { - return nil, fmt.Errorf("failed to encode group name to UTF16: %w", err) - } sids := []winapi.LocalGroupMembersInfo0{{Sid: sid}} if err := winapi.NetLocalGroupAddMembers( - nil, - groupNameUTF16, + "", + groupName, 0, (*byte)(unsafe.Pointer(&sids[0])), 1, diff --git a/internal/winapi/user.go b/internal/winapi/user.go index 0718aabab1..47aa63e34a 100644 --- a/internal/winapi/user.go +++ b/internal/winapi/user.go @@ -65,7 +65,34 @@ type UserInfo1 struct { // [out] LPBYTE *bufptr // ); // -//sys NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) = netapi32.NetLocalGroupGetInfo +//sys netLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) = netapi32.NetLocalGroupGetInfo + +// NetLocalGroupGetInfo is a slightly go friendlier wrapper around the NetLocalGroupGetInfo function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetLocalGroupGetInfo(serverName, groupName string, level uint32, bufPtr **byte) (err error) { + var ( + serverNameUTF16 *uint16 + groupNameUTF16 *uint16 + ) + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + if groupName != "" { + groupNameUTF16, err = windows.UTF16PtrFromString(groupName) + if err != nil { + return err + } + } + return netLocalGroupGetInfo( + serverNameUTF16, + groupNameUTF16, + level, + bufPtr, + ) +} // NET_API_STATUS NET_API_FUNCTION NetUserAdd( // [in] LPCWSTR servername, @@ -74,14 +101,57 @@ type UserInfo1 struct { // [out] LPDWORD parm_err // ); // -//sys NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) = netapi32.NetUserAdd +//sys netUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) = netapi32.NetUserAdd + +// NetUserAdd is a slightly go friendlier wrapper around the NetUserAdd function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetUserAdd(serverName string, level uint32, buf *byte, parm_err *uint32) (err error) { + var serverNameUTF16 *uint16 + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + return netUserAdd( + serverNameUTF16, + level, + buf, + parm_err, + ) +} // NET_API_STATUS NET_API_FUNCTION NetUserDel( // [in] LPCWSTR servername, // [in] LPCWSTR username // ); // -//sys NetUserDel(serverName *uint16, username *uint16) (status error) = netapi32.NetUserDel +//sys netUserDel(serverName *uint16, username *uint16) (status error) = netapi32.NetUserDel + +// NetUserDel is a slightly go friendlier wrapper around the NetUserDel function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetUserDel(serverName, userName string) (err error) { + var ( + serverNameUTF16 *uint16 + userNameUTF16 *uint16 + ) + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + if userName != "" { + userNameUTF16, err = windows.UTF16PtrFromString(userName) + if err != nil { + return err + } + } + return netUserDel( + serverNameUTF16, + userNameUTF16, + ) +} // NET_API_STATUS NET_API_FUNCTION NetLocalGroupAddMembers( // [in] LPCWSTR servername, @@ -91,4 +161,32 @@ type UserInfo1 struct { // [in] DWORD totalentries // ); // -//sys NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) = netapi32.NetLocalGroupAddMembers +//sys netLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) = netapi32.NetLocalGroupAddMembers + +// NetLocalGroupAddMembers is a slightly go friendlier wrapper around the NetLocalGroupAddMembers function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetLocalGroupAddMembers(serverName, groupName string, level uint32, buf *byte, totalEntries uint32) (err error) { + var ( + serverNameUTF16 *uint16 + groupNameUTF16 *uint16 + ) + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + if groupName != "" { + groupNameUTF16, err = windows.UTF16PtrFromString(groupName) + if err != nil { + return err + } + } + return netLocalGroupAddMembers( + serverNameUTF16, + groupNameUTF16, + level, + buf, + totalEntries, + ) +} diff --git a/internal/winapi/zsyscall_windows.go b/internal/winapi/zsyscall_windows.go index 537dc99b29..fc4a8f664f 100644 --- a/internal/winapi/zsyscall_windows.go +++ b/internal/winapi/zsyscall_windows.go @@ -78,7 +78,7 @@ var ( procRtlNtStatusToDosError = modntdll.NewProc("RtlNtStatusToDosError") ) -func NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) { +func netLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) { r0, _, _ := syscall.Syscall6(procNetLocalGroupGetInfo.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(bufptr)), 0, 0) if r0 != 0 { status = syscall.Errno(r0) @@ -86,7 +86,7 @@ func NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, b return } -func NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) { +func netUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) { r0, _, _ := syscall.Syscall6(procNetUserAdd.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(unsafe.Pointer(parm_err)), 0, 0) if r0 != 0 { status = syscall.Errno(r0) @@ -94,7 +94,7 @@ func NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) ( return } -func NetUserDel(serverName *uint16, username *uint16) (status error) { +func netUserDel(serverName *uint16, username *uint16) (status error) { r0, _, _ := syscall.Syscall(procNetUserDel.Addr(), 2, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(username)), 0) if r0 != 0 { status = syscall.Errno(r0) @@ -102,7 +102,7 @@ func NetUserDel(serverName *uint16, username *uint16) (status error) { return } -func NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) { +func netLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) { r0, _, _ := syscall.Syscall6(procNetLocalGroupAddMembers.Addr(), 5, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(totalEntries), 0) if r0 != 0 { status = syscall.Errno(r0) diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go index 0718aabab1..47aa63e34a 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/user.go @@ -65,7 +65,34 @@ type UserInfo1 struct { // [out] LPBYTE *bufptr // ); // -//sys NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) = netapi32.NetLocalGroupGetInfo +//sys netLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) = netapi32.NetLocalGroupGetInfo + +// NetLocalGroupGetInfo is a slightly go friendlier wrapper around the NetLocalGroupGetInfo function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetLocalGroupGetInfo(serverName, groupName string, level uint32, bufPtr **byte) (err error) { + var ( + serverNameUTF16 *uint16 + groupNameUTF16 *uint16 + ) + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + if groupName != "" { + groupNameUTF16, err = windows.UTF16PtrFromString(groupName) + if err != nil { + return err + } + } + return netLocalGroupGetInfo( + serverNameUTF16, + groupNameUTF16, + level, + bufPtr, + ) +} // NET_API_STATUS NET_API_FUNCTION NetUserAdd( // [in] LPCWSTR servername, @@ -74,14 +101,57 @@ type UserInfo1 struct { // [out] LPDWORD parm_err // ); // -//sys NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) = netapi32.NetUserAdd +//sys netUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) = netapi32.NetUserAdd + +// NetUserAdd is a slightly go friendlier wrapper around the NetUserAdd function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetUserAdd(serverName string, level uint32, buf *byte, parm_err *uint32) (err error) { + var serverNameUTF16 *uint16 + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + return netUserAdd( + serverNameUTF16, + level, + buf, + parm_err, + ) +} // NET_API_STATUS NET_API_FUNCTION NetUserDel( // [in] LPCWSTR servername, // [in] LPCWSTR username // ); // -//sys NetUserDel(serverName *uint16, username *uint16) (status error) = netapi32.NetUserDel +//sys netUserDel(serverName *uint16, username *uint16) (status error) = netapi32.NetUserDel + +// NetUserDel is a slightly go friendlier wrapper around the NetUserDel function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetUserDel(serverName, userName string) (err error) { + var ( + serverNameUTF16 *uint16 + userNameUTF16 *uint16 + ) + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + if userName != "" { + userNameUTF16, err = windows.UTF16PtrFromString(userName) + if err != nil { + return err + } + } + return netUserDel( + serverNameUTF16, + userNameUTF16, + ) +} // NET_API_STATUS NET_API_FUNCTION NetLocalGroupAddMembers( // [in] LPCWSTR servername, @@ -91,4 +161,32 @@ type UserInfo1 struct { // [in] DWORD totalentries // ); // -//sys NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) = netapi32.NetLocalGroupAddMembers +//sys netLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) = netapi32.NetLocalGroupAddMembers + +// NetLocalGroupAddMembers is a slightly go friendlier wrapper around the NetLocalGroupAddMembers function. Instead of taking in *uint16's, it takes in +// go strings and does the conversion internally. +func NetLocalGroupAddMembers(serverName, groupName string, level uint32, buf *byte, totalEntries uint32) (err error) { + var ( + serverNameUTF16 *uint16 + groupNameUTF16 *uint16 + ) + if serverName != "" { + serverNameUTF16, err = windows.UTF16PtrFromString(serverName) + if err != nil { + return err + } + } + if groupName != "" { + groupNameUTF16, err = windows.UTF16PtrFromString(groupName) + if err != nil { + return err + } + } + return netLocalGroupAddMembers( + serverNameUTF16, + groupNameUTF16, + level, + buf, + totalEntries, + ) +} diff --git a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go index 537dc99b29..fc4a8f664f 100644 --- a/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go +++ b/test/vendor/github.com/Microsoft/hcsshim/internal/winapi/zsyscall_windows.go @@ -78,7 +78,7 @@ var ( procRtlNtStatusToDosError = modntdll.NewProc("RtlNtStatusToDosError") ) -func NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) { +func netLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, bufptr **byte) (status error) { r0, _, _ := syscall.Syscall6(procNetLocalGroupGetInfo.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(bufptr)), 0, 0) if r0 != 0 { status = syscall.Errno(r0) @@ -86,7 +86,7 @@ func NetLocalGroupGetInfo(serverName *uint16, groupName *uint16, level uint32, b return } -func NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) { +func netUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) (status error) { r0, _, _ := syscall.Syscall6(procNetUserAdd.Addr(), 4, uintptr(unsafe.Pointer(serverName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(unsafe.Pointer(parm_err)), 0, 0) if r0 != 0 { status = syscall.Errno(r0) @@ -94,7 +94,7 @@ func NetUserAdd(serverName *uint16, level uint32, buf *byte, parm_err *uint32) ( return } -func NetUserDel(serverName *uint16, username *uint16) (status error) { +func netUserDel(serverName *uint16, username *uint16) (status error) { r0, _, _ := syscall.Syscall(procNetUserDel.Addr(), 2, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(username)), 0) if r0 != 0 { status = syscall.Errno(r0) @@ -102,7 +102,7 @@ func NetUserDel(serverName *uint16, username *uint16) (status error) { return } -func NetLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) { +func netLocalGroupAddMembers(serverName *uint16, groupName *uint16, level uint32, buf *byte, totalEntries uint32) (status error) { r0, _, _ := syscall.Syscall6(procNetLocalGroupAddMembers.Addr(), 5, uintptr(unsafe.Pointer(serverName)), uintptr(unsafe.Pointer(groupName)), uintptr(level), uintptr(unsafe.Pointer(buf)), uintptr(totalEntries), 0) if r0 != 0 { status = syscall.Errno(r0) From 4e62590b5deac37a85c3663163d5908b10389943 Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Fri, 25 Feb 2022 18:46:32 -0800 Subject: [PATCH 3/3] PR feedback * Make comment on makeLocalAccount more clear * Delete account if any errors occurred. * Don't return on first error in jobcontainer.Close() Signed-off-by: Daniel Canter --- internal/jobcontainers/jobcontainer.go | 15 ++++++++++++--- internal/jobcontainers/logon.go | 10 ++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go index 4b544470d8..b77ba9b1db 100644 --- a/internal/jobcontainers/jobcontainer.go +++ b/internal/jobcontainers/jobcontainer.go @@ -320,18 +320,24 @@ func (c *JobContainer) Start(ctx context.Context) error { // Close free's up any resources (handles, temporary accounts). func (c *JobContainer) Close() error { + // Do not return the first error so we can finish cleaning up. + + var closeErr bool if err := c.job.Close(); err != nil { - return err + log.G(context.Background()).WithError(err).WithField("cid", c.id).Warning("failed to close job object") + closeErr = true } if err := c.token.Close(); err != nil { - return fmt.Errorf("failed to close token: %w", err) + log.G(context.Background()).WithError(err).WithField("cid", c.id).Warning("failed to close token") + closeErr = true } // Delete the containers local account if one was created if c.localUserAccount != "" { if err := winapi.NetUserDel("", c.localUserAccount); err != nil { - return fmt.Errorf("failed to delete local user account: %w", err) + log.G(context.Background()).WithError(err).WithField("cid", c.id).Warning("failed to delete local account") + closeErr = true } } @@ -339,6 +345,9 @@ func (c *JobContainer) Close() error { c.waitError = hcs.ErrAlreadyClosed close(c.waitBlock) }) + if closeErr { + return errors.New("failed to close one or more job container resources") + } return nil } diff --git a/internal/jobcontainers/logon.go b/internal/jobcontainers/logon.go index 68175dc5c7..056d06e1b5 100644 --- a/internal/jobcontainers/logon.go +++ b/internal/jobcontainers/logon.go @@ -36,8 +36,9 @@ func groupExists(groupName string) bool { } // makeLocalAccount creates a local account with the passed in username and a randomly generated password. -// If `groupName` is not an empty string the user will be added to group if it exists. -func makeLocalAccount(ctx context.Context, user, groupName string) (*uint16, error) { +// The user specified by `user`` will added to the `groupName`. This function does not check if groupName exists, that must be handled +// the caller. +func makeLocalAccount(ctx context.Context, user, groupName string) (_ *uint16, err error) { // Create a local account with a random password pswd, err := randomPswd() if err != nil { @@ -63,6 +64,11 @@ func makeLocalAccount(ctx context.Context, user, groupName string) (*uint16, err ); err != nil { return nil, fmt.Errorf("failed to create user %s: %w", user, err) } + defer func() { + if err != nil { + _ = winapi.NetUserDel("", user) + } + }() log.G(ctx).WithField("username", user).Debug("Created local user account for job container")