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

Fix cron module logic regarding working from non-root user #51873

Conversation

Oloremo
Copy link
Contributor

@Oloremo Oloremo commented Feb 27, 2019

What does this PR do?

Fix cron module\state execution in case of Salt running from non-root user

What issues does this PR fix or reference?

Fixes #51872

Previous Behavior

cron state trying to run commands using the privilege escalation via runas arg in different modules.

New Behavior

If the current Salt process owner UID equals specified user in cron state it won't do a privilege escalation.

Tests written?

No

Commits signed with GPG?

Yes

@garethgreenaway
Copy link
Member

@garethgreenaway garethgreenaway commented Feb 27, 2019

@Oloremo Thanks for the PR. I think it might make more sense to break out the other if...else situations as you did for the raw_cron function in the others, to ensure that the Solaris/AIX functionality remains consistent.

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Feb 27, 2019

@garethgreenaway I felt like I didn't break any logic and raw_cron func is just a bit different from the other, but if you like I can make them in the same style

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Mar 1, 2019

Ok, so I tried to do some unification of the logic and updated tests according to it.
Problem is now I have new kind of condition which isn't covered by tests - when I check if salt running from a root user:

_check_instance_uid_match('root'):

And I'm not sure how to mock it properly to test this condition.

The second problem I hesitated to remove the final else and commented it with
# Edge cases here, let's try do a runas but in fact, I'm pretty sure that if the user hit this condition - it will result in an error since it's not the same user and not the root user so in that case we probably don't have enough permissions to do this operation anyway.

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Mar 13, 2019

@garethgreenaway mind to look at this PR again?

@dwoz dwoz requested a review from garethgreenaway Apr 29, 2019
@garethgreenaway garethgreenaway merged commit a0bb6a4 into saltstack:2018.3 Apr 29, 2019
15 checks passed
@Oloremo Oloremo deleted the cron-module-fix-for-non-root-execution branch Apr 29, 2019
@waynew waynew added this to PR needs port to master in PRs to port to master Oct 24, 2019
@waynew waynew moved this from PR needs port to master to PR has port to master in PRs to port to master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs to port to master
  
PR has port to master
Development

Successfully merging this pull request may close these issues.

None yet

2 participants