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

don't fail upon error while chdir()ing *before* loading config #3237

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

Conversation

svmhdvn
Copy link

@svmhdvn svmhdvn commented Jul 9, 2024

When gunicorn is invoked by root using # su -m <nonprivileged_user> -c gunicorn --config <absolute_path_to_config_file> --chdir <some_world_accessible_directory>, the following sequence of steps occurs:

  1. su runs gunicorn as the <nonprivileged_user>, BUT with PWD=/root due to the -m flag
  2. Due to e908ec3 , gunicorn takes its current working directory from the PWD envvar (QUESTION: is this the right behaviour? It seems to me that gunicorn should just use os.getcwd() every time.)
  3. gunicorn then reaches the code from this patch
  4. the config parser correctly parses cfg.chdir = <some_world_accessible_directory>, but hasn't yet set self.cfg = cfg
  5. gunicorn fails with self.chdir: ERROR: permission denied after trying to chdir() to /root

This patch makes this event a warning instead of a failure and continues onwards with searching for the config file. Afterwards, we will chdir() to the correct user-configured location as normal.

@svmhdvn svmhdvn force-pushed the chdir-fix branch 2 times, most recently from a8a59c4 to 96e866c Compare July 9, 2024 21:56
@pajod
Copy link
Contributor

pajod commented Jul 9, 2024

The primary recommendation here is to not run gunicorn from a shell in the first place. We only suffer abominations like systemd because they are so much better (at launching unprivileged daemons) than most after-the-fact cleanup code will do.


I certainly do not like not-quite-consistency between

  • config search path after arbiter (re-)exec
  • app search path after worker init

either, but I do not see how ignoring one user-specified config is better than ignoring the other, so I do not like the patch as-is.

I am leaning towards either entirely ignoring the PWD env for determining config path/module purposes (since the arbiter has switched us to the directory already), or dropping conflicting values known to confuse us (thus never inheriting such to the workers). Either should be possibly while exclusively breaking undocumented symlink behaviour.

If that is too intrusive.. maybe we should just start to respect the configuration specified on the command line, even where doing so breaks currently working get_default_config_file behaviour in the edge case. A warning could still fire for the likely unintended env/cwd mismatch then.

@benoitc
Copy link
Owner

benoitc commented Aug 6, 2024

@pajod gunicorn is supposed to be run from shell if it's wanted.

I think however that we should exit when such error happen instead of continue since it can be a security issue. @svmhdvn can you rework your patch for it.

@benoitc benoitc self-assigned this Aug 6, 2024
@svmhdvn
Copy link
Author

svmhdvn commented Aug 6, 2024

I won't be using gunicorn for a while now so I don't have the time to change and test the patch, however someone can definitely take my work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants