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

Check UID unconditionally on non-Windows platforms, if os.getuid is available #10566

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

n1000
Copy link
Contributor

@n1000 n1000 commented Oct 10, 2021

On BSD systems, pip warns about executing it as root, even though it is not being executed as root.

Fixes issue #10565.

@uranusjr
Copy link
Member

When do they set UID to 0 when not running as root? We should detect the exact scenario instead of blindly excluding *BSD in the check.

@n1000
Copy link
Contributor Author

n1000 commented Oct 11, 2021

Hi @uranusjr, thanks for taking a look at the PR. Adding the additional OR conditions enables the calling of getuid() on the BSD's. Today with the code as it is, the getuid() call is only performed on Linux and Mac OS.

I'm not sure the history of why Linux and Mac OS are explicitly checked -- maybe on some platforms getuid() returns a nonsensical value -- but on the BSDs getuid() works as expected and should be called.

I guess one other change that could be done in addition to the one I've implemented here, is to print the warning only if getuid() was called to start with. Today the warning was being printed even without any getuid() call.

In other words, it could be restructured as follows:

    if (sys.platform == "darwin" or sys.platform == "linux" or
        sys.platform.startswith("openbsd") or sys.platform.startswith("freebsd") or
        sys.platform.startswith("netbsd")):
        if os.getuid() == 0:
            logger.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"
            )

This way, if some unknown platform is detected and we don't even call getuid(), it won't print the warning at least (unlike today where the warning is printed, even if the getuid() call is completely skipped).

@n1000
Copy link
Contributor Author

n1000 commented Oct 11, 2021

I actually strongly prefer the alternative approach I mentioned in my comment above, and have likewise updated the pull request.
I can revert it back to the original depending on other folks preferences / feedback...

@pfmoore
Copy link
Member

pfmoore commented Oct 11, 2021

So this seems bizarre. If pip install is run on any platform apart from win32, cygwin, darwin or linux, it unconditionally warns that you're running as root? Even when you're not? How has this never been reported before?

Rather than adding some extra platforms to the list here, why would we not just check getuid on all platforms that have it?

@n1000
Copy link
Contributor Author

n1000 commented Oct 11, 2021

Thanks for the feedback @pfmoore , I have updated the PR accordingly.

Note that although the native Windows version of python does not implement os.getuid(), the cygwin version does.

The old code was specifically excluding cases where sys.platform is win32 or cygwin from this root check, and for now I left this same exclusion in place.

@n1000
Copy link
Contributor Author

n1000 commented Oct 11, 2021

So this seems bizarre. If pip install is run on any platform apart from win32, cygwin, darwin or linux, it unconditionally warns that you're running as root? Even when you're not? How has this never been reported before?

The version of pip included in OpenBSD 6.9 and FreeBSD 13.0 is 20.3.4, which did not include this warning message. On those platforms this issue would not be seen unless the user does a pip upgrade (ie. pip install pip --user --upgrade). Maybe that is why it was not reported earlier.

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.

The logic looks good to me, a stylistic note that I feel surprisingly strongly about. :)

src/pip/_internal/cli/req_command.py Outdated Show resolved Hide resolved
news/10565.bugfix.rst Outdated Show resolved Hide resolved
news/10565.bugfix.rst Outdated Show resolved Hide resolved
@pradyunsg
Copy link
Member

pradyunsg commented Oct 12, 2021

I'll note that BSD falls outside of our regular "supported platforms" matrix. I'm on board for merging this, specifically since this is actually simplifying the code, and it is a low cost fix for the users of those platforms.

But, it is worth nothing none the less, should someone decide to argue that since we accepted PRs like this means that we support BSD upstream. We don't.

It just works out well in this case. :)

@pradyunsg pradyunsg changed the title Check uid on OpenBSD, NetBSD, FreeBSD before warning about root Check uid unconditionally on non-Windows platforms, if os.getuid is available Oct 12, 2021
@pradyunsg pradyunsg changed the title Check uid unconditionally on non-Windows platforms, if os.getuid is available Check UID unconditionally on non-Windows platforms, if os.getuid is available Oct 12, 2021
@n1000 n1000 requested a review from pradyunsg October 12, 2021 20:01
@n1000
Copy link
Contributor Author

n1000 commented Oct 12, 2021

Updated the news file as requested.

…vailable

Check the uid using getuid() on platforms other than Linux and Mac OS before
warning about running pip as root.

Before this change, pip was emitting this warning even when it was run as a
non-root user.
@n1000
Copy link
Contributor Author

n1000 commented Oct 13, 2021

Changed the commit title also.

@pradyunsg pradyunsg added this to the 21.3.1 milestone Oct 13, 2021
Copy link
Member

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

@n1000 This is a good proposal!

@uranusjr uranusjr merged commit 9d25d2a into pypa:main Oct 13, 2021
@n1000 n1000 deleted the dont_warn_on_bsd branch October 14, 2021 04:36
pradyunsg pushed a commit to pradyunsg/pip that referenced this pull request Oct 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants