Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add local user account creation for host process containers #1286

Merged
merged 3 commits into from
Feb 26, 2022

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jan 26, 2022

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.

@marosset
Copy link
Member

marosset commented Feb 4, 2022

I tried out these changes in a K8s cluster and everything is working as expected.

On one of the Windows nodes I performed the following actions

  1. Create a new local group
net localgroup hpc-users /add
  1. Create a new share that hpc-users has access to
mkdir c:\hpc-user-share
icacls c:\hpc-user-share /inheritance:r
icacls c:\hpc-user-share /grant:r SYSTEM:(OI)(CI)(F)
icacls c:\hpc-user-share /grant:r BUILTIN\Administrators:(OI)(CI)(F)
icacls c:\hpc-user-share /grant:r hpc-users:(OI)(CI)(F)

icacls c:\hpc-user-share
c:\hpc-user-share 1576k8s00000000\hcp-users:(OI)(CI)(F)
                  BUILTIN\Administrators:(OI)(CI)(F)
                  NT AUTHORITY\SYSTEM:(OI)(CI)(F)
  1. Create a share that hpc-users does not have access to
mkdir c:\admin-share
icacls c:\admin-share /inheritance:r
icacls c:\admin-share /grant:r SYSTEM:(OI)(CI)(F)
icacls c:\admin-share /grant:r BUILTIN\Administrators:(OI)(CI)(F)

C:\Users\azureuser>icacls c:\admin-share
c:\admin-share BUILTIN\Administrators:(OI)(CI)(F)
               NT AUTHORITY\SYSTEM:(OI)(CI)(F)

(not a step but here are the ACLs for c:\windows)

icacls c:\windows
c:\windows NT SERVICE\TrustedInstaller:(F)
           NT SERVICE\TrustedInstaller:(CI)(IO)(F)
           NT AUTHORITY\SYSTEM:(M)
           NT AUTHORITY\SYSTEM:(OI)(CI)(IO)(F)
           BUILTIN\Administrators:(M)
           BUILTIN\Administrators:(OI)(CI)(IO)(F)
           BUILTIN\Users:(RX)
           BUILTIN\Users:(OI)(CI)(IO)(GR,GE)
           CREATOR OWNER:(OI)(CI)(IO)(F)
           APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES:(RX)
           APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES:(OI)(CI)(IO)(GR,GE)
           APPLICATION PACKAGE AUTHORITY\ALL RESTRICTED APPLICATION PACKAGES:(RX)
           APPLICATION PACKAGE AUTHORITY\ALL RESTRICTED APPLICATION PACKAGES:(OI)(CI)(IO)

I think created a pod with the following deployment file

apiVersion: v1
kind: Pod
metadata:
  name: test-account-test
spec:
  securityContext:
    windowsOptions:
      hostProcess: true
      runAsUserName: hpc-users
  hostNetwork: true
  containers:
  - name: test
    image: mcr.microsoft.com/windows/nanoserver:1809
    command:
      - "ping"
      - "-t"
      - "127.0.0.1"
  nodeSelector:
    "kubernetes.io/os": windows

Exec'ed into it and ran the following actions.
Everything behaved as I would expect

C:\C\dc5fa129a454d485e76f291b905c62ea3b096ef742051cd7dac592dfffb420ed>whoami
1576k8s00000000\dc5fa129a454d485e76f

C:\C\dc5fa129a454d485e76f291b905c62ea3b096ef742051cd7dac592dfffb420ed>cd c:\hpc-user-share

c:\hpc-user-share>echo "" > temp

c:\hpc-user-share>cd c:\admin-share
Access is denied.

c:\hpc-user-share>cd c:\Windows

c:\Windows>echo "" > temp
Access is denied.

@dcantah dcantah marked this pull request as ready for review February 4, 2022 22:23
@dcantah dcantah requested a review from a team as a code owner February 4, 2022 22:23
@dcantah
Copy link
Contributor Author

dcantah commented Feb 8, 2022

@ambarve @helsaawy This should be ready for review. Edit: Spoke too soon 🤣

Edit 2: Needed a rebase :)

cmd/containerd-shim-runhcs-v1/delete.go Outdated Show resolved Hide resolved
cmd/containerd-shim-runhcs-v1/delete.go Outdated Show resolved Hide resolved
internal/jobcontainers/jobcontainer.go Show resolved Hide resolved
@dcantah
Copy link
Contributor Author

dcantah commented Feb 22, 2022

@ambarve ptal when you get a chance

@dcantah
Copy link
Contributor Author

dcantah commented Feb 22, 2022

Actually this needs a quick rebase

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 <dcanter@microsoft.com>
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 <dcanter@microsoft.com>
* 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 <dcanter@microsoft.com>
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants