Closed SchoolGuy closed 1 year ago
According to the GitHub Docs there is no way that you can access the GitHub workspace with a different user. As such the application becomes untestable when requiring orthos
. As such I will remove this check. The application has no requirement for a specific user, technically it might even be able to run as root
. However since it is strongly recommended to run this with the user orthos
I will emit a warning into the log.
Making this application untestable per design with our chosen CI provider is not something that is acceptable.
@watologo1 I know you said that this application requires the orthos
user. However, I was unable to find documentation of why this is the case. It is of course for security measures strongly recommended to not run this as root
. Of course, the user orthos
makes sense in the context of our systemd unit. It is of course easier to map the files this application created if the username is identical to the application name.
All these things still don't explain why it is required to run this application with this specific user. Since we lose a lot if we can't test it inside GitHub Actions I would like to see at least one strong argument why the application can only run with this one specific user (which will have different UIDs anyway on different systems depending on the configuration of it).
The application in its current state doesn't seem to break in any way that is related to the user that executes the Python code. Looking at said code I can only see that the setting is used during startup of the taskmanager and the wsgi application itself.
Edit: Fixing up the tests will be part of the next follow-up PR.
The problem is that you cannot create users/groups inside github test environment? Not sure how to easily fix/adopt this. There should at least always be the same non-root user running orthos. Which in the test env would be another (runner or similar?). Would it be possible to have something like:
/etc/orthos2/service.conf
USER=orthos
GROUP=orthos
/usr/lib/systemd/system/orthos2.service
EnvironmentFile=-/etc/orthos2/service.conf
User=${USER}
Group=${GROUP}
But this also needs fixing up here: /etc/nginx/nginx.conf
user orthos orthos;
and in the spec file and manage.py
wrapper.
There are other services also running with explicit usernames, there should be a way to do this in the test env?
Let's talk about this later...
The question also is how fully blown up you need the test env... If you do not install via rpm, the spec file part is not needed.
If you do not start nginx, etc. you might come away with another user. Not sure about orthos2 and orthos2_taskmanager services and how much you can really test without having them running. To have the full service tested, another additional mechanism would be needed at some point of time then...
@watologo1 There is no problem creating these users. This works just fine. According to the docs however (which I linked above), there is no way to access the code with those custom users. Making it impossible to test with the orthos
user.
The goal here is not to make the application run as any user in production. All the stuff about unit files and nginx can stay as it has before. We just want to be able to develop the code without the user being present and to do stuff like:
python3 manage.py runserver 0.0.0.0:8000
The goal here is not to make the application run as any user in production. All the stuff about unit files and nginx can stay as it has before. We just want to be able to develop the code without the user being present and to do stuff like:
python3 manage.py runserver 0.0.0.0:8000
Yep, makes sense. But this should work already with manage.py added again, right? Ah no, it does not, you still run into: ./orthos2/settings.py
RUN_AS_USER = 'orthos'
CUR_USER = getpwuid(os.getuid())[0]
if CUR_USER != RUN_AS_USER:
logging.error("You must run as user: %s, not as user: %s", RUN_AS_USER, CUR_USER)
exit(1)
I still would keep this, because if you run manage.py accidentially as root your productions system might be busted. Whatabout an additional environment variable that gets check here: Something like:
RUN_AS_USER = os.environ.get('ORTHOS_USER')
if not RUN_AS_USER:
RUN_AS_USER = 'orthos'
...
Then you can do: ORTHOS_USER=runner; python3 manage.py ...
@watologo1 That seems like a very good compromise indeed. I will get right to implementing it.
Nice. Not sure why setting env before the command did not work... You should revert this commit again: Loosen required runtime user account check
@watologo1 Done. The title of the commit is still true IMHO. If you want I can rephrase it to something like "allow for using custom user" or something in that direction.
Do you still want to modify something, otherwise I would merge or add my review...
@watologo1 No I think this PR is ready for review. I would prefer if you give a review and only then we merge.
After merging #210 the linters failed. Since we want to have our code to have a clean state, this PR makes the linters green.
The tests will stay red for now as this requires more changes and as such these should be handled in a dedicated PR.