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

Add a warning when run as root (e.g., sudo pip) #9394

Merged
merged 5 commits into from
Mar 6, 2021

Conversation

winsonluk
Copy link
Contributor

@winsonluk winsonluk commented Dec 29, 2020

This change discourages the usage of pip as root by showing a warning when pip install is run with sudo.

$ sudo pip3 install
WARNING: Running pip as root is not recommended. This could result in the corruption of the package directories when a package manager is used. See #6409 for more details.
ERROR: You must give at least one requirement to install (see "pip help install")

Fixes #6409
...and potentially helps helps prevent hundreds of future issues related to sudo pip: https://github.com/pypa/pip/issues?q=is%3Aissue+%22sudo+pip%22

@hroncok
Copy link
Contributor

hroncok commented Dec 29, 2020

The Fedora's patch: https://src.fedoraproject.org/rpms/python-pip/blob/master/f/emit-a-warning-when-running-with-root-privileges.patch

@winsonluk
Copy link
Contributor Author

winsonluk commented Dec 29, 2020

The Fedora's patch: https://src.fedoraproject.org/rpms/python-pip/blob/master/f/emit-a-warning-when-running-with-root-privileges.patch

that's some good stuff, thanks @hroncok. i added the venv check and updated the warning message with the tip

@pradyunsg
Copy link
Member

/cc @dstufft since last I checked he was opposed to just doing this.

@pradyunsg pradyunsg added state: needs discussion This needs some more discussion type: enhancement Improvements to functionality labels Jan 2, 2021
@pradyunsg
Copy link
Member

There's still one sticking point with this: docker containers give you root by default.

I don't think we should be calling out the use of pip in root shells like that.

@hroncok
Copy link
Contributor

hroncok commented Jan 18, 2021

I think we should. Even in docker, installing with pip as root can lead to serious problems.

@pradyunsg
Copy link
Member

Perhaps. But that's still more disruptive than I'd like.

I haven't seen a Dockerfile that's using pip that even tries to install as non-root. Is it even possible to use it as non-root from a Dockerfile?

@hroncok
Copy link
Contributor

hroncok commented Jan 18, 2021

It is possible, however not that straightforward. I get your point.

@uranusjr
Copy link
Member

uranusjr commented Jan 18, 2021

Is this really that disruptive? It’s “just” a warning, people ignore warnings all the time when they feel like it. Using root in a Docker container is ultimately still not best practice, and it’s arguably still not a bad idea to show the warning in this situation.

If it’s really that important, we can have --disable-root-check for people to set if they think they know what they’re doing.

@hroncok
Copy link
Contributor

hroncok commented Jan 18, 2021

--root / is for people who think they know they are doing ;)

@winsonluk
Copy link
Contributor Author

winsonluk commented Jan 18, 2021

Yeah, most people don't use Docker, so I think the benefit of preventing mis-installations exceeds whatever disruption a one-line message causes to Docker users.

Plus this is just a logger.info, not a logger.warning.

@xavfernandez
Copy link
Member

Perhaps. But that's still more disruptive than I'd like.

I haven't seen a Dockerfile that's using pip that even tries to install as non-root. Is it even possible to use it as non-root from a Dockerfile?

I can confirm that using virtual environments inside docker is definitely a thing

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Alrighty. Consider me convinced that this is a good idea overall.

This is the first time I'm looking at the code in this PR and... I have some opinions about how we should implement this. :)

news/6409.bugfix.rst Outdated Show resolved Hide resolved
src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/uninstall.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/install.py Outdated Show resolved Hide resolved
@pradyunsg pradyunsg changed the title Sudo warning Add a warning when run as root (eg: sudo pip) Jan 19, 2021
@winsonluk winsonluk changed the base branch from master to docs-quickstart January 30, 2021 21:21
@winsonluk winsonluk changed the base branch from docs-quickstart to master January 30, 2021 21:21
@winsonluk
Copy link
Contributor Author

Changes requested by @pradyunsg have been implemented.

src/pip/_internal/cli/req_command.py Outdated Show resolved Hide resolved
src/pip/_internal/cli/req_command.py Show resolved Hide resolved
src/pip/_internal/cli/req_command.py Show resolved Hide resolved
@winsonluk winsonluk requested a review from pradyunsg January 31, 2021 12:12
"It is possible to break packages installed for your "
"operating system or by different package managers. "
"You should use a virtual environment rather than trying "
"to run as root. This may help resolve permission issues.")
Copy link
Member

Choose a reason for hiding this comment

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

I would try making the message shorter. The longer the warning is, the less likely users would read it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is shorter, yet OTOH the user has no idea what to do instead:

logger.warning("Running pip as root may destroy your system.")`

Do you think providing a link would work? I.e.:

logger.warning("Running pip as root may destroy your system. See https://bit.ly/pip-root for details.")`

Copy link
Member

Choose a reason for hiding this comment

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

A link to the documentation feels like a good compromise to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's two situations I can think of where someone would run pip with sudo:

  1. Unintentionally: it's often easy to forget which commands need sudo and which don't. In this case, the "Running pip as root may break packages" part of the message should be enough to dissuade them; I don't think we'll need to link an essay.
  2. Intentionally: A user may run pip without sudo, encounter a permission error, and retry with sudo. We should discourage this by suggesting virtual environments. This is where a link could help imo.

How's this?

Running pip as root may break packages and permissions. Permission errors should be fixed with venv: https://docs.python.org/3/library/venv.html 

Copy link
Member

Choose a reason for hiding this comment

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

Either of the followings would be a better link IMO:

The library documentation page covers more on the programmatic interface and isn’t very useful for the audience we target here.

Copy link
Member

Choose a reason for hiding this comment

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

I like @hroncok's link-based suggestion. I'd say use https://pip.pypa.io/warnings/root-usage for that right now. We can setup a redirect in our RTD configuration, to make it point to the right place once this is merged.

It'll also defer the discussion on what the right place is, which I like. 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think we should explicitly call out permission errors and venv in the warning message, since that's by far the most common misuse of sudo.

So instead of a generic https://pip.pypa.io/warnings/root-usage redirect, how about we specify https://pip.pypa.io/warnings/venv and tentatively redirect it to https://docs.python.org/3/tutorial/venv.html?

Updated warning message:
Running pip as root may break packages and permissions. Permission errors should be fixed with venv: https://pip.pypa.io/warnings/venv

Copy link
Member

Choose a reason for hiding this comment

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

How about "Permission errors should be fixed with venv" -> "If you need to install packages not supplied by your system package manager, use a virtual environment: https://pip.pypa.io/warnings/venv"? Focus on what the user wants to do, rather than simply on fixing the error they got.

Copy link
Member

@pradyunsg pradyunsg Feb 2, 2021

Choose a reason for hiding this comment

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

I think we should call this out for what they're doing here, and provide better guidance on what they should be doing in the link (using a virtualenv, why, what the risks are etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shorter message with a focus on the user:

    logger.warning("Running pip as root may break packages and permissions. "                       
                   "You should install packages reliably by using venv: "                                             
                   "https://pip.pypa.io/warnings/venv") 

I agree that technical details belong in the link.

@winsonluk winsonluk requested a review from uranusjr February 1, 2021 02:11
@sbidoul
Copy link
Member

sbidoul commented Feb 20, 2021

I just stumbled upon on this part of the pip code base which warns about permissions and reckons that the user might be using sudo. I thought this might be relevant to the conversation here.

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 21, 2021
@winsonluk
Copy link
Contributor Author

I just stumbled upon on this part of the pip code base which warns about permissions and reckons that the user might be using sudo. I thought this might be relevant to the conversation here.

Good call out, this warning often appears when users run as root too. But I think it's useful to keep both cache and root warnings separate, since each case can still exist independently:

  • The user may not be running as root, but the cache may still be owned by another user; or,
  • The user may be running as root, but the cache and its parent directory are both owned by root.

@winsonluk
Copy link
Contributor Author

@uranusjr @pradyunsg @pfmoore Rebase is complete. We still need to set up a redirect from https://pip.pypa.io/warnings/venv to https://docs.python.org/3/tutorial/venv.html before merging, but that should be the last step!

@pradyunsg pradyunsg added the state: blocked Can not be done until something else is done label Feb 21, 2021
@winsonluk
Copy link
Contributor Author

#9680 configures the redirect. would appreciate a review :)

@pradyunsg pradyunsg removed the state: blocked Can not be done until something else is done label Mar 3, 2021
@pradyunsg
Copy link
Member

pip.pypa.io/warnings/venv to docs.python.org/3/tutorial/venv.html

Configured the redirect.

@winsonluk
Copy link
Contributor Author

Thanks @pradyunsg, this should be good to merge then.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Could you do a rebase of this PR? I'd like to make sure it still passes the current linter setup.

@winsonluk
Copy link
Contributor Author

Could you do a rebase of this PR? I'd like to make sure it still passes the current linter setup.

@pradyunsg Sure, rebased without issues.

@pradyunsg pradyunsg merged commit c6933d5 into pypa:master Mar 6, 2021
@pradyunsg
Copy link
Member

Thanks for this PR! ^.^

@graue70
Copy link

graue70 commented Jul 28, 2021

I'm adding the current version of the warning message so this PR with some explanations can be found more easily:

WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv

@graue70
Copy link

graue70 commented Jul 28, 2021

I think we should. Even in docker, installing with pip as root can lead to serious problems.

Using root in a Docker container is ultimately still not best practice, and it’s arguably still not a bad idea to show the warning in this situation.

I can confirm that using virtual environments inside docker is definitely a thing

Both the documentation of the official python docker images and the official docker docs install pip packages with sudo. I can't find any information on the web as to how it should be done instead. Would you please explain how I'm supposed to get rid of the warning when using a Dockerfile? @hroncok @uranusjr @xavfernandez

@uranusjr
Copy link
Member

Processes In Containers Should Not Run As Root

Deploying Python Applications with Docker - A Suggestion (It covers a ton of stuff, but your can search for virtualenv to find the relevant parts.)

Maybe we should write a documentation page on this, and point https://pip.pypa.io/warnings/venv to that page instead.

@alexchamberlain
Copy link

I think we should. Even in docker, installing with pip as root can lead to serious problems.

@hroncok What issues were you thinking of here? Generally, I recommend people use Docker (well any container) or virtualenv, not both.

@hroncok
Copy link
Contributor

hroncok commented Aug 26, 2021

For example, if your docker container image is based on a distribution that uses Python for some critical software (let's say the distribution's package manager is written in Python), using pip with root can essentially turn that package manager unusable and depending on the container image usage, this might break use cases of people building stuff on top of it.

@potiuk
Copy link
Contributor

potiuk commented Aug 26, 2021

It is totally legitimate case to use pip with root user. I can give you a very good example

In our Production Dockerflie Image for Apache Airflow we have two segments:

Segment 1) installs all the packages for Apache Airlfow including all the packages that need "build-essentials" - i.e. compilation support. This is all done as "root" user and installed with --user flag, so that all the python packages but also .so libraries are placed in ${HOME}/.local of the user (which happens to be root). Using root user helps us to simplify the build segment "USER" management - we do not have to switch between different users to install debian packages. Hell - even more. That segment has NO OTHER USER than root. There is no need for that. And I would not like to add such user because PIP

Then we have segment 2) which uses "root" user to install all "debian" packages, but then we switch to "airflow" user - and first thing we do - we copy the ".local" folder of the "root" user from segment 1 to segment 2 - changing the ownership of all files to "airflow" while copying (nice feature of COPY command":

COPY --chown=airflow:root --from=airflow-build-image /root/.local "${AIRFLOW_USER_HOME_DIR}/.local"

This results in 25-30% smaller images - ca. 250 MB (no need to keep build-essentials and -dev packages around). Also any kind of secrets you might need in segment 1 (for example .pypirc containing passwords to your registry) are not present in the final "segment 2" even if they needed to be copied to "segment 1" in order to be able to install "private packages".

I wonder if the best solution here is:

  • warn users by default - this will fulfil it's need of warning the users who are not fully aware of the consequences of running pip as root.

  • provide a flag for "power users" to disable the warning. There are quite a number of people who "know what they do" when running PIP as root. And those people should be able to disable it. It could be difficult. Might require some woodo-magic - whatever. It can be complex to disable the warning. Those people will figure it out. And will be able to install pip using root user without the negative consequences.

I would really love to be able to have "warning-free" docker build of Airflow Image. It's not possible currently (or maybe you need stronger magic).

@hroncok
Copy link
Contributor

hroncok commented Aug 26, 2021

provide a flag for "power users" to disable the warning.

So, technically, the Fedora patch did allow you to use --root / if you knew what you were doing. This PR does not contain that and I think it was a legitimate thing to have. Thoughts?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
state: needs discussion This needs some more discussion type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discourage usage of pip as root