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

cmdmod: fix runas and group in run_chroot #53992

Merged
merged 3 commits into from
Apr 20, 2020
Merged

Conversation

aplanas
Copy link
Contributor

@aplanas aplanas commented Jul 24, 2019

The parameters runas and group for cmdmod.run() will change the efective
user and group before executing the command. But in a chroot environment is
expected that the change happends inside the chroot, not outside, as the
user and groups are refering to objects that can only exist inside the
environment.

This patch add the userspec parameter to the chroot command, to change
the user in the correct place.

@waynew
Copy link
Contributor

waynew commented Jul 24, 2019

Hey, thanks for the PR!

Do we have any existing chroot tests? This seems like something we'd want to protect against regressions.

@aplanas
Copy link
Contributor Author

aplanas commented Jul 24, 2019

There is not unit test for run_chroot, but I will do one for this case.

@cmcmarrow
Copy link
Contributor

@aplanas you just need to write test for your changes. But if your willing to go up and beyond that would be greatly appreciated.

@cmcmarrow cmcmarrow added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Aug 31, 2019
@aplanas
Copy link
Contributor Author

aplanas commented Aug 31, 2019

@aplanas you just need to write test for your changes. But if your willing to go up and beyond that would be greatly appreciated.

I am sorry, I do not understand. I provided the different tests in this same PR: 42640ec d900035

@aplanas
Copy link
Contributor Author

aplanas commented Sep 4, 2019

ping @cmcmarrow

@waynew waynew removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 6, 2019
waynew
waynew previously approved these changes Sep 6, 2019
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@waynew
Copy link
Contributor

waynew commented Sep 6, 2019

@aplanas Looks like it's out of date, so perhaps a rebase would be good :shipit:

@aplanas
Copy link
Contributor Author

aplanas commented Sep 12, 2019

@waynew rebased!

@aplanas aplanas changed the base branch from develop to master October 14, 2019 12:12
@aplanas
Copy link
Contributor Author

aplanas commented Oct 14, 2019

Rebased into master

@DmitryKuzmenko
Copy link
Contributor

@aplanas could you please resolve merge conflicts?

@aplanas
Copy link
Contributor Author

aplanas commented Apr 7, 2020

On it

@aplanas aplanas force-pushed the fix_run_chroot branch 3 times, most recently from f848d97 to 91d3a2a Compare April 9, 2020 13:14
@aplanas aplanas force-pushed the fix_run_chroot branch 2 times, most recently from 219dcce to 56ee80e Compare April 16, 2020 09:27
The parameters runas and group for cmdmod.run() will change the efective
user and group before executing the command. But in a chroot environment is
expected that the change happends inside the chroot, not outside, as the
user and groups are refering to objects that can only exist inside the
environment.

This patch add the userspec parameter to the chroot command, to change
the user in the correct place.
@dwoz dwoz merged commit f750dc0 into saltstack:master Apr 20, 2020
@aplanas aplanas deleted the fix_run_chroot branch April 20, 2020 08:41
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants