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

Runas any user even when shell is limited like winrm #47621

Merged
merged 16 commits into from Aug 5, 2018

Conversation

@dwoz
Copy link
Contributor

@dwoz dwoz commented May 12, 2018

What does this PR do?

  • Fix runas when running under winrm.
  • Support for LOCAL SERVICE and NETWORK SERVICE system accounts.
  • Runas can now use system accounts from salt-call.
    (SYSTEM, LOCAL SERVICE and NETWORK SERVICE)
  • Runas can launch processes on behalf of users without a password.

What issues does this PR fix or reference?

  • Some tests fail when run over winrm because of restrictions.

Tests written?

No

Commits signed with GPG?

Yes

@ghost ghost self-requested a review May 12, 2018
@dwoz dwoz force-pushed the win_runas branch 2 times, most recently from c112b6a to 34608cb May 12, 2018
@dwoz dwoz changed the title WIP: Runas that works on limited shells for any user WIP: Runas any user even when shell is limited (winrm) May 12, 2018
@dwoz dwoz changed the title WIP: Runas any user even when shell is limited (winrm) WIP: Runas any user even when shell is limited like winrm May 12, 2018
@dwoz dwoz requested a review from twangboy May 12, 2018
@dwoz dwoz force-pushed the win_runas branch 7 times, most recently from da7506f to 92e93b7 May 12, 2018
@damon-atkins
Copy link
Collaborator

@damon-atkins damon-atkins commented May 15, 2018

Seems like a good idea. Please use a class for constants like in modules/reg.py

@dwoz dwoz changed the title WIP: Runas any user even when shell is limited like winrm Runas any user even when shell is limited like winrm May 23, 2018
@dwoz dwoz requested a review from s0undt3ch May 23, 2018
@dwoz
Copy link
Contributor Author

@dwoz dwoz commented May 23, 2018

This should fix

#47621

stdin_read, stdin_write = win32pipe.CreatePipe(security_attributes, 0)
stdin_read = make_inheritable(stdin_read)
stdin_read = salt.utils.winutil.make_inheritable(stdin_read)
Copy link
Contributor

@twangboy twangboy May 23, 2018

And here


stdout_read, stdout_write = win32pipe.CreatePipe(security_attributes, 0)
stdout_write = make_inheritable(stdout_write)
stdout_write = salt.utils.winutil.make_inheritable(stdout_write)
Copy link
Contributor

@twangboy twangboy May 23, 2018

same here

fd_err = msvcrt.open_osfhandle(stderr_read, os.O_RDONLY | os.O_TEXT)
with os.fdopen(fd_err, 'r') as f_err:
ret['stderr'] = f_err.read()
stderr_write = salt.utils.winutil.make_inheritable(stderr_write)
Copy link
Contributor

@twangboy twangboy May 23, 2018

and here


salt.utils.winutil.kernel32.CloseHandle(stdin_write.handle)
salt.utils.winutil.kernel32.CloseHandle(stdout_write.handle)
salt.utils.winutil.kernel32.CloseHandle(stderr_write.handle)
Copy link
Contributor

@twangboy twangboy May 23, 2018

And these


def runas(cmdLine, username, password=None, cwd=None, elevated=True):

impersonation_token = salt.utils.winutil.impersonate_sid(
Copy link
Contributor

@twangboy twangboy May 23, 2018

Shouldn't this be salt.winutil now?

salt.utils.winutil.elevate_token(user_token)

handle_reg = win32profile.LoadUserProfile(user_token, {'UserName': username})
salt.utils.winutil.grant_winsta_and_desktop(user_token)
Copy link
Contributor

@twangboy twangboy May 23, 2018

And here

win32process.CREATE_SUSPENDED
)

startup_info = salt.utils.winutil.STARTUPINFO(
Copy link
Contributor

@twangboy twangboy May 23, 2018

You get the idea...


env = win32profile.CreateEnvironmentBlock(user_token, False)

process_info = salt.utils.winutil.CreateProcessWithTokenW(
Copy link
Contributor

@twangboy twangboy May 23, 2018

and here

# salt.utils.winutil.kernel32.CloseHandle(user_token)
if impersonation_token:
win32security.RevertToSelf()
# salt.utils.winutil.kernel32.CloseHandle(impersonation_token)
Copy link
Contributor

@twangboy twangboy May 23, 2018

Can we remove these comments?
And the salt.winutil thing

salt/winutil.py Outdated
except block because it is only applicable on Windows platforms.
Much of what is here was adappted from the following:
Copy link
Contributor

@twangboy twangboy May 23, 2018

adapted

@damon-atkins
Copy link
Collaborator

@damon-atkins damon-atkins commented May 24, 2018

Some random thoughts.
Not sure module salt.win is the correct place. Maybe salt.utils.platform.win.somthing (if you break the file up). Maybe along the lines of how powershell is broken up e.g. salt.utils.platform.win.credentials class. Or even salt.platform.win.*
Get-<Name Of Python Class> e.g. Get-Credentials
Get-Module -ListAvailable | Format-Table Name, Description

There are also a few win_*.py in salt.utils

For example we have salt.utils.pkg.win

What I would like to see is a

salt.platform.file.posix
salt.platform.file.posix.linux
salt.platform.file.posix.solaris
salt.platform.cred.win
salt.platform.cred.posix
salt.platform.cred.posix.linux
salt.platform.cred.posix.solaris
salt.platform.cred.win
salt.platform.cred.aws

With a platform independent set of high level class methods where possible, with specific platform overrides.

@dwoz dwoz requested a review from cachedout May 24, 2018
@dwoz
Copy link
Contributor Author

@dwoz dwoz commented May 24, 2018

@damon-atkins, thanks so much for reviewing this. :)

salt.win came from discussions with @twangboy. The main driver for salt.win is to have a place for Windows specific stuff. For now salt.win only contains things related to logon. The plan is to expand on this module to make salt.win.logon, salt.win.service, salt.win.registry ect. I did not put the module under salt.utils to avoid having the loader try to import platform specifics automatically. I think anything under salt.utils gets touched by the loader automatically (I could be wrong on this). I'm down with salt.utils.platform provided the loader plays nicely. If not, maybe salt.platform works as a namespace for these things. Maybe @s0undt3ch or @cachedout have some opinions?

@dwoz dwoz requested a review from damon-atkins May 24, 2018
username, domain = split_username(username)
sid, domain, sidType = win32security.LookupAccountName(domain, username)
if domain == 'NT AUTHORITY':
log.warn("Logon system account: %s", username)
Copy link
Member

@s0undt3ch s0undt3ch May 24, 2018

Why warning?
Also, switch to log.warning, log.warn is deprecated in Py3.

Copy link
Contributor Author

@dwoz dwoz May 24, 2018

I agree warning is overkill. Should these 'logon user' log statements go away completely?

Copy link
Member

@s0undt3ch s0undt3ch May 24, 2018

If you think these will make sense when trying to debug an issue, leave then, but perhaps, at debug log level? Info?

Copy link
Contributor Author

@dwoz dwoz May 24, 2018

Windows creates an event log for the logons so I've just removed these log statements.

salt/win.py Outdated
@@ -0,0 +1,1164 @@
# -*- coding: utf-8 -*-
'''
Windows specific utility functions, this module should be imported in a try,
Copy link
Member

@s0undt3ch s0undt3ch May 24, 2018

If this is a utilities module it should be moved to salt.utils.

@dwoz dwoz force-pushed the win_runas branch 2 times, most recently from 94c3a62 to 2a1da92 May 24, 2018
@cachedout cachedout dismissed their stale review Jul 16, 2018

I've read this and it looks good so far as I can tell, but I simply don't know enough about the Windows ecosystem to be able to review this in any real depth.

@dwoz dwoz requested a review from UtahDave Jul 17, 2018
@twangboy
Copy link
Contributor

@twangboy twangboy commented Jul 19, 2018

I'm getting the following error on Py3 when I do a cmd.run running as a normal (non-admin) user. I haven't tried with an admin user. Here's the command I ran and the result on both Py2 and Py3.

root@master:/srv/salt/test# salt -t 300 dev* cmd.run whoami runas=testuser01 password=PassWord1!
dev(py2):
    win-8fgt3e045se\testuser01
dev3(py3):
    The minion function caused an exception: Traceback (most recent call last):
      File "c:\dev\salt\salt\minion.py", line 1619, in _thread_return
        return_data = minion_instance.executors[fname](opts, data, func, args, kwargs)
      File "c:\dev\salt\salt\executors\direct_call.py", line 12, in execute
        return func(*args, **kwargs)
      File "c:\dev\salt\salt\modules\cmdmod.py", line 1161, in run
        **kwargs)
      File "c:\dev\salt\salt\modules\cmdmod.py", line 411, in _run
        return win_runas(cmd, runas, password, cwd)
      File "c:\dev\salt\salt\utils\win_runas.py", line 62, in runas
        privs=['SeTcbPrivilege'],
      File "c:\dev\salt\salt\platform\win.py", line 1125, in impersonate_sid
        raise WindowsError("Impersonation failure")  # pylint: disable=undefined-variable
    OSError: Impersonation failure
root@shanelee-master:/srv/salt/test#

@dwoz dwoz force-pushed the win_runas branch 2 times, most recently from 77ef5cb to 8626bef Aug 1, 2018
@rallytime rallytime requested a review from cachedout Aug 2, 2018

A password is no longer required with ``runas`` under normal circumstances.
The password option is only needed if the minion process is run under a
restricted (non-administrator) account.
Copy link
Contributor

@cachedout cachedout Aug 3, 2018

As I recall, password is only needed if it's under a non-admin account AND it's requesting priv escalation, right? If so, I think that should be called out specifically.

Copy link
Contributor Author

@dwoz dwoz Aug 3, 2018

@cachedout If the minion happens to be running under a non admin. They will need to use a password anytime the use the runas argument to cmd.run. Maybe this is better?

 A password is no longer required with ``runas`` under normal circumstances.
The password option is only needed if the minion process is run under a
restricted (non-administrator) account. In the aforementioned case, a password
is only required when using the ``runas`` argument to run command as a different 
user.

@@ -685,7 +685,7 @@ def wrap(cls):
username
)
)
create_user = cls.run_function('user.add', [username])
create_user = cls.run_function('user.add', [username], **kwargs)
Copy link
Contributor

@cachedout cachedout Aug 3, 2018

Is the intention here just to pass groups? I'm wary of passing all kwargs unless that's truly needed.

Copy link
Contributor Author

@dwoz dwoz Aug 3, 2018

@cachedout Yes, this is to pass groups. I'll change it to be more specific.

Copy link
Contributor Author

@dwoz dwoz Aug 3, 2018

Actually, looking at this again. We are making the kwargs dictionary for the sole purpose of passing it to user.add. It is being used to pass timeout, groups, and password.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Aug 3, 2018

@dwoz This has a merge conflict with the release notes. Can you fix that up?

dwoz added 15 commits Aug 3, 2018
@dwoz
Copy link
Contributor Author

@dwoz dwoz commented Aug 3, 2018

@rallytime fixed.

@rallytime rallytime merged commit a9daa92 into saltstack:develop Aug 5, 2018
5 of 9 checks passed
@dwoz dwoz deleted the win_runas branch Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants