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

[global] Add the ability to run command as user #3648

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

arif-ali
Copy link
Member


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
  • Are all passwords or private data gathered by this PR obfuscated?

@arif-ali
Copy link
Member Author

arif-ali commented May 20, 2024

An initial idea to discuss

Once we migrate to python3.9, we can use the user argument for Popen and not have to do the setuid(). I get the same results with that, when used with python3.10 on ubuntu 22.04.

Once gotcha I've had so far, if a command is run, to establish some kind of connection, to then run commands afterwards, that doesn't seem to work.

So, any thoughts on this would be great, or if someone else has a better way/idea, happy to abandon this PR

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3648
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@TurboTurtle
Copy link
Member

TurboTurtle commented May 21, 2024

Once we migrate to python3.9, we can use the user argument for Popen and not have to do the setuid().

My immediate thought was to hold this implementation until the next minor version, but if it requires 3.9 then there's not much use to that, given that we've planned to go to 3.8 instead, with 3.9 coming in early/mid 2025 (per #3533).

I haven't yet played around with this, but something that comes to mind is do the os.setuid() and os.setgid() calls not persist after the first invocation? Just thinking out loud here, wouldn't we need to reset to root after each non-root command in order to avoid running all following commands as the non-root user?

The docs mention this sets the current process' id, so I imagine this is not thread-safe?

@arif-ali
Copy link
Member Author

I haven't yet played around with this, but something that comes to mind is do the os.setuid() and os.setgid() calls not persist after the first invocation? Just thinking out loud here, wouldn't we need to reset to root after each non-root command in order to avoid running all following commands as the non-root user?

The docs mention this sets the current process' id, so I imagine this is not thread-safe?

based on my testing on the updated PR I have for #3646, I ran through a full sos, and I only saw the commands that it needed to run as the user, actually do it. My understanding was that the setuid() for that process, i.e. the Popen being used, so hence didn't think we would need to change others.

Happy to hold off for others to do similar testing, and see how it goes. I've got an update on my system for docstrings, so can incorporate anything else that is required here.

@TurboTurtle
Copy link
Member

Oh! I didn't realize that was in _child_prep_fn() at first. Ok, that makes sense to me now.

@arif-ali arif-ali force-pushed the sos-arif-run-cmd-user branch from 32fdf8a to 91673eb Compare May 21, 2024 20:43
@arif-ali
Copy link
Member Author

There is one gotcha with this at the moment especially with the other PR I am working with

one of the commands only works with su -u <user> -c "command" but doesn't with the runas=user, so there must be some subtleties on what will and won't work. I've tried a few different ideas with Popen but haven't got there, if anyone has any ideas on what I can try, then that would be awesome.

Ultimately for that plugin, I only need to run those as exec_cmd, and one-offs, so I am not precious about it, especially if the command collection actually works without the extra su -u <user>, especially it looks so much more cleaner too :)

@arif-ali arif-ali marked this pull request as ready for review May 21, 2024 21:56
@arif-ali
Copy link
Member Author

arif-ali commented Jun 1, 2024

So, the issue I have with the current implementation is a corner case because of the way the command for sunbeam interacts. Spent a lot of time outside of sos and bare python code to see if there was a way to get that specific command to work, but to no avail.

So, this should be good to go, if we can get some reviews.

Ideally would like to get this, and once this is merged need to work on the sunbeam to be ready for the next z-release

@arif-ali arif-ali requested review from TurboTurtle and pmoravec June 1, 2024 08:38
Copy link
Contributor

@pmoravec pmoravec left a comment

Choose a reason for hiding this comment

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

ACK.

Maybe it makes sense to log the user somewhere, e.g. in collecting output of log in https://github.com/sosreport/sos/blob/main/sos/report/plugins/__init__.py#L3082, but it might be too detailed information already..? I dont know.. Leaving on @arif-ali to decide.

@TurboTurtle
Copy link
Member

Maybe it makes sense to log the user somewhere

This was my thought as well. Code looks good, but I think we should explicitly highlight when we're collecting as non-root, since we run as root.

@arif-ali arif-ali force-pushed the sos-arif-run-cmd-user branch from 91673eb to a0caed1 Compare June 2, 2024 22:01
@arif-ali
Copy link
Member Author

arif-ali commented Jun 2, 2024

After reviewing the logs, I found 2 places it was useful to add. Hopefully this fits with what you guys were anticipating

@arif-ali arif-ali force-pushed the sos-arif-run-cmd-user branch from a0caed1 to baf41b2 Compare June 3, 2024 09:09
@arif-ali
Copy link
Member Author

arif-ali commented Jun 3, 2024

tried re-running the centos-stream-8 rpm build, maybe an infra issue 🤷🏽

@arif-ali
Copy link
Member Author

arif-ali commented Jun 3, 2024

Oh, CentOS Stream 8 is now EOL, maybe that's why the CI is not working today? Do we need to update that before we do new CI tests now, as all CI's will fail now

/cc @pmoravec @jcastill

@jcastill
Copy link
Member

jcastill commented Jun 3, 2024

We can probably remove the references to CentOS Stream 8 now. Let me open a PR so we can discuss this separately and then if/when it's accepted, we can retry this one.

@arif-ali arif-ali force-pushed the sos-arif-run-cmd-user branch from baf41b2 to 1389052 Compare June 4, 2024 16:37
@TurboTurtle TurboTurtle merged commit 6a2845a into sosreport:main Jun 6, 2024
31 checks passed
arif-ali added a commit to arif-ali/sos that referenced this pull request Jun 7, 2024
If the chdir would have been set as well as runas, the chdir would not
have taken effect, so updating to reflect the correct behaviour

Related: sosreport#3648

Signed-off-by: Arif Ali <[email protected]>
TurboTurtle pushed a commit that referenced this pull request Jun 7, 2024
If the chdir would have been set as well as runas, the chdir would not
have taken effect, so updating to reflect the correct behaviour

Related: #3648

Signed-off-by: Arif Ali <[email protected]>
@arif-ali arif-ali deleted the sos-arif-run-cmd-user branch October 2, 2024 13:47
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.

4 participants