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

Support for optionally running http server as non-root #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maglar0
Copy link

@maglar0 maglar0 commented Dec 26, 2022

I don't know best practices etc on how to deal with things like this, so maybe this change is not the right way forward, but it does seem to work to run part of it as non-root. From what I gather when googling, it is impossible to run smartctl as non-root, so that part is still run as root (the only part run as root).

@matusnovak
Copy link
Owner

Hi @maglar0

The changes look good to me, but what is the use case to run a non-root version of this exporter?

I understand that there is a general preference to run things like these as user instead of a root (which I completely agree with), but since you are still running part of the script as root, doesn't that violate the idea of these changes?

@maglar0
Copy link
Author

maglar0 commented Jan 18, 2023

There is a flaw in this pull request in that the root part will execute anything you ask it to. So don't merge this. I hope I have time to fix it in the future, but I kinda did changes in a totally different direction after this in trying to not wake up the hard drives if they are in standby mode.

The general idea is to run as little as possible as root, in this case in particular we might not want to run the http parser etc as root, thus minimizing attack surface. But again, my implementation is flawed.

@ngosang ngosang added the invalid This doesn't seem right label Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants