Open gigi206 opened 5 years ago
@gigi206 Thank you for reporting this issue.
@saltstack/team-windows Any of you have any thoughts on this one? Thanks.
@gigi206, can you make sure that the user running start has write access to the directory for the minion_cache?
To ensure the user can write to the minion cache, the section "Add the New User to the Access Control List for the Salt Folder" at https://docs.saltstack.com/en/latest/topics/installation/windows.html should help, just make sure you're applying it to the minion cache directory.
Yes, all rights are good for the minion_cache.
If I edit function copy_security in salt\utils\win_dacl.py file like below, it seems to works :
def copy_security(source,
target,
obj_type='file',
copy_owner=True,
copy_group=True,
copy_dacl=True,
copy_sacl=True):
return True
As shown in the error message, GetNamedSecurityInfo required an admin privilege.
maintainers: it seems like according to https://docs.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-getnamedsecurityinfoa, the SE_SECURITY_NAME
privilege needs to be added to the token that's adjusted in salt.platform.win
. You can search for AdjustTokenPrivileges
to find it and then add the priv to the loop above it.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.
The fix for this issue was discussed and resolved and is literally just waiting for a maintainer to pick it up and add the required SE_SECURITY_NAME
privilege.
Thank you for updating this issue. It is no longer marked as stale.
@frugunder, if you want to fix bugs please review the super simple fix mentioned in this discussion.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.
Not stale
Thank you for updating this issue. It is no longer marked as stale.
Not only is it not stale, but a really simple fix too.
The above suggested fix is incorrect. You can't enable privileges the user does not have.
The token contains all the privileges the user has. Some are enabled, others are dormant. The elevate_token
function in salt.platform.win
enables all privileges the user has. This is the equivalent of the UAC dialog box when you're doing something from an account that is a member of the Administrators group.
By default, not all privileges are enabled (unless you're logged in as THE Administrator). That function loops through each privilege held by the token and enables it. If the user doesn't have the privilege to begin with, it cannot be enabled.
You must run salt with a user that has permissions to its directories. By default this is Administrators. If you're an administrator, you must run salt in an elevated prompt.
The failure in the stack trace of the original issue is in the copy_security
function in salt.utils.win_dacl
. Just above the failure there are the following lines:
new_privs = set()
luid = win32security.LookupPrivilegeValue('', 'SeTakeOwnershipPrivilege')
new_privs.add((luid, win32con.SE_PRIVILEGE_ENABLED))
luid = win32security.LookupPrivilegeValue('', 'SeRestorePrivilege')
new_privs.add((luid, win32con.SE_PRIVILEGE_ENABLED))
luid = win32security.LookupPrivilegeValue('', 'SeSecurityPrivilege')
new_privs.add((luid, win32con.SE_PRIVILEGE_ENABLED))
# Get the current token
p_handle = win32api.GetCurrentProcess()
t_handle = win32security.OpenProcessToken(
p_handle,
win32security.TOKEN_ALL_ACCESS | win32con.TOKEN_ADJUST_PRIVILEGES)
You could try adding the following 2 lines after the las new_privs.add
to see if it will add a new permission. I'm not sure it will work:
luid = win32security.LookupPrivilegeValue('', 'SeSecurityName')
new_privs.add((luid, win32con.SE_PRIVILEGE_ENABLED))
If this works, could you please submit a PR?
The above suggested fix is incorrect. You can't enable privileges the user does not have.
The token contains all the privileges the user has. Some are enabled, others are dormant. The
elevate_token
function insalt.platform.win
enables all privileges the user has. This is the equivalent of the UAC dialog box when you're doing something from an account that is a member of the Administrators group.
I think you're looking at the wrong thing here because salt.platform.win.elevate_token
is executed only by salt.utils.win_runas
and is related to that particular commit. So, the process' token is not getting all of its available privileges set with elevate_token
prior to running GetNamedSecurityInfo
.
If elevate_token
was actually being called at process startup, then that would make sense...but that's definitely not in OPs case. So specifically these privileges that the OP enabled for their service account are still "dormant". You can literally set a breakpoint at kernelbase!AdjustTokenPrivileges
and watch it not get hit when running salt until you explicitly try and call a function that explicitly sets privs.
This is why many of the implementors of the functionality in salt.utils.win_dacl
are explicitly opening the process token and explicitly assigning the necessary privileges prior to making the API calls. If elevate_token
was guaranteed to have been called for the process token, then the explicit setting of tokens is entirely unnecessary.
And no, the UAC dialog is not the same and isn't even considered a security boundary for that matter as explicitly mentioned by Microsoft (they don't offer security bounties for UAC bypasses).
By default, not all privileges are enabled (unless you're logged in as THE Administrator). That function loops through each privilege held by the token and enables it. If the user doesn't have the privilege to begin with, it cannot be enabled.
You must run salt with a user that has permissions to its directories. By default this is Administrators. If you're an administrator, you must run salt in an elevated prompt.
According to the OP, the discussion from 4 months ago, and my reproduction from 4 months ago, this was the case. I don't recall if it was the same crash since it was so long ago and I just relocated and don't have any of my equipment setup, but you can simply grep for calls to GetNamedSecurityInfo
and find a handful of code paths that don't set the token privileges prior to making the call.
The right thing to do would be to set all available token privileges at startup (instead of on-demand) so that one can cull out all of the repeated code, or better yet to mention in docs at https://docs.saltstack.com/en/master/ref/configuration/nonroot.html that running the service as a non-administrative in Windows is although possible, it's unsupported until someone has actually tested it.
As you guys have to unfortunately go through a struggle fixing critical issues upon each release, I think you guys should be culling features and I'm for fixing this w/ docs, but its up to @gigi206 on how they want to proceed.
(edited to remove a stray 'Y')
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.
Not stale
Thank you for updating this issue. It is no longer marked as stale.
Description of Issue
When I run saltstack with admin rights it works.
But if I run saltstack without admin rights, I have the following issue:
GetNamedSecurityInfo needs to be run with admin rights
Steps to Reproduce Issue
I think just import from a jinja file like that :
Versions Report