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

pip_audit, test: warn on Python path confusion #451

Merged
merged 10 commits into from
Dec 29, 2022

Conversation

woodruffw
Copy link
Member

Closes #450.

Signed-off-by: William Woodruff william@trailofbits.com

Closes #450.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw added the component:dep-sources Dependency sources label Dec 27, 2022
@woodruffw woodruffw requested review from di and tetsuo-cpp December 27, 2022 19:16
@woodruffw woodruffw self-assigned this Dec 27, 2022
@woodruffw
Copy link
Member Author

NB: This doesn't work quite as expected yet:

$ pip-audit
WARNING:pip_audit._dependency_source.pip:pip-audit will run pip against /Users/william/.pyenv/versions/3.11.0/bin/python3.11, but you have /Users/william/.pyenv/versions/3.11.0/bin/python in your PATH (usually indicating a virtual environment). This may result in unintuitive audits, since your local environment will not be audited. You can forcefully override this behavior by setting PIPAPI_PYTHON_LOCATION to the location of your local Python interpreter.
Found 2 known vulnerabilities in 2 packages
Name       Version   ID                  Fix Versions
---------- --------- ------------------- ------------
certifi    2022.9.24 GHSA-43fp-rhv2-5gv8 2022.12.7
setuptools 65.5.0    GHSA-r9hx-vwmv-q579 65.5.1

(In this case it can't see that the two global versions are actually the same.)

@woodruffw
Copy link
Member Author

I can induce the expected warning, however:

$ ~/.pyenv/versions/3.11.0/bin/pip-audit
WARNING:pip_audit._dependency_source.pip:pip-audit will run pip against /Users/william/.pyenv/versions/3.11.0/bin/python3.11, but you have /Users/william/devel/pip-audit/env/bin/python in your PATH (usually indicating a virtual environment). This may result in unintuitive audits, since your local environment will not be audited. You can forcefully override this behavior by setting PIPAPI_PYTHON_LOCATION to the location of your local Python interpreter.
Found 2 known vulnerabilities in 2 packages
Name       Version   ID                  Fix Versions
---------- --------- ------------------- ------------
certifi    2022.9.24 GHSA-43fp-rhv2-5gv8 2022.12.7
setuptools 65.5.0    GHSA-r9hx-vwmv-q579 65.5.1

@woodruffw woodruffw marked this pull request as draft December 27, 2022 19:23
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw marked this pull request as ready for review December 27, 2022 19:45
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Member Author

7f943b5 should be a better approach -- rather than checking whether sys.executable diverges from the $PATH Python, we check whether we're in a virtual environment but are executing with a sys.executable outside of that environment.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw
Copy link
Member Author

Example output, formatted for readability (global pip-audit, local virtual environment):

$ ~/.pyenv/versions/3.11.0/bin/pip-audit 
WARNING:pip_audit._dependency_source.pip:pip-audit will run pip against
/Users/william/.pyenv/versions/3.11.0/bin/python3.11, but you have a virtual environment 
loaded at /Users/william/devel/pip-audit/env. This may result in unintuitive audits, since 
your local environment will not be audited. You can forcefully override this behavior by 
setting PIPAPI_PYTHON_LOCATION to the location of your local Python interpreter.

Found 2 known vulnerabilities in 2 packages
Name       Version   ID                  Fix Versions
---------- --------- ------------------- ------------
certifi    2022.9.24 GHSA-43fp-rhv2-5gv8 2022.12.7
setuptools 65.5.0    GHSA-r9hx-vwmv-q579 65.5.1

@woodruffw
Copy link
Member Author

Thinking about it some more: maybe we should just do what the user expects here (i.e. configure PIPAPI_PYTHON_LOCATION for them, based on VIRTUAL_ENV).

We could still emit a warning, but this would avoid users having to do their own manual workaround and would probably match ordinary user expectations (similar to how mypy or any other tool can be installed globally but used within a virtual environment).

@tetsuo-cpp
Copy link
Contributor

tetsuo-cpp commented Dec 28, 2022

Thinking about it some more: maybe we should just do what the user expects here (i.e. configure PIPAPI_PYTHON_LOCATION for them, based on VIRTUAL_ENV).

We could still emit a warning, but this would avoid users having to do their own manual workaround and would probably match ordinary user expectations (similar to how mypy or any other tool can be installed globally but used within a virtual environment).

Seems reasonable to me. If you find a file at ${VIRTUAL_ENV}/bin/python3, you can use that. I'll push a commit for that.

os.environ["PIPAPI_PYTHON_LOCATION"] = str(venv_python)
else:
logger.warning(
f"pip-audit will run pip against {effective_python}, but you have "
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about editing this to include the fact that we tried to look for a symlink in bin/ but wasn't sure how to phrase it as this is already getting complicated. I don't think it's all that important to communicate to the user since the corrective action doesn't change.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also maybe just fail here?

Copy link
Member Author

@woodruffw woodruffw Dec 28, 2022

Choose a reason for hiding this comment

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

Yeah, I think just failing is probably appropriate -- the lack of a bin here suggests that the user intended the load a virtual environment (based on the env setting) but somehow ended up with a broken one, so we shouldn't try to guess at the correct behavior.

@woodruffw
Copy link
Member Author

woodruffw commented Dec 28, 2022

Hmm, there's another issue here: this check:

        if _PIP_VERSION < _MINIMUM_RELIABLE_PIP_VERSION:
            logger.warning(
                f"pip {_PIP_VERSION} is very old, and may not provide reliable "
                "dependency information! You are STRONGLY encouraged to upgrade to a "
                "newer version of pip."
            )

...doesn't actually do what we (or the user) expect, since _PIP_VERSION is just pip_api.PIP_VERSION and is therefore loaded eagerly on pip_api's import. So it doesn't see the updated PIPAPI_PYTHON_LOCATION.

@woodruffw
Copy link
Member Author

Based on the above, I think we just want to go with the initial "warn" approach for now.

That'll allow us to get a bugfix out, and from there we can work on actually using the active virtual environment by default (since it'll require us to rethink how we use pip_api and possibly make some upstream changes).

@woodruffw
Copy link
Member Author

For posterity, here's the tweaked approach based on @tetsuo-cpp's work (mostly tweaked to respect PIPAPI_PYTHON_LOCATION if the user has already overridden it):

        # NOTE: By default `pip_api` invokes `pip` through `sys.executable`, like so:
        #
        #    {sys.executable} -m pip [args ...]
        #
        # This is the right decision 99% of the time, but it can result in unintuitive audits
        # for users who have installed `pip-audit` globally but are trying to audit
        # a loaded virtual environment, since `pip-audit`'s `sys.executable` will be the global
        # Python and not the virtual environment's Python.
        #
        # Rather than auditing the global environment (based on how `pip-audit` has been
        # installed), we detect the user's intent by checking for an active virtual
        # environment. If there's an active virtual environment and the user hasn't
        # already overriden `PIPAPI_PYTHON_LOCATION`, then we point `pip_api` at the
        # right Python path in that virtual environment.
        effective_python = os.getenv("PIPAPI_PYTHON_LOCATION")
        if effective_python is None:
            effective_python = sys.executable

            venv_prefix = os.getenv("VIRTUAL_ENV")
            if venv_prefix is not None and not effective_python.startswith(venv_prefix):
                venv_python = Path(venv_prefix) / "bin/python"

                if venv_python.exists():
                    os.environ["PIPAPI_PYTHON_LOCATION"] = str(venv_python)
                else:
                    raise PipSourceError(
                        f"pip-audit found a virtual environment at {venv_prefix}, but "
                        "couldn't find a Python interpreter in that virtual environment"
                    )

        if _PIP_VERSION < _MINIMUM_RELIABLE_PIP_VERSION:
            logger.warning(
                f"pip {_PIP_VERSION} is very old, and may not provide reliable "
                "dependency information! You are STRONGLY encouraged to upgrade to a "
                "newer version of pip."
            )

woodruffw and others added 2 commits December 28, 2022 11:46
* _cli: refactor logging

This is inspired by the refactor in
sigstore/sigstore-python#372.

Signed-off-by: William Woodruff <william@trailofbits.com>

* README: update `pip-audit --help`

Signed-off-by: William Woodruff <william@trailofbits.com>

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw force-pushed the ww/python-path-confusion branch from 8c14570 to da2ad6f Compare December 28, 2022 16:47
@tetsuo-cpp
Copy link
Contributor

Based on the above, I think we just want to go with the initial "warn" approach for now.

Good idea. I completely missed the issue with PIP_VERSION. Let's just merge this for now.

Copy link
Contributor

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

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

LGTM!

@woodruffw woodruffw merged commit 533ecf9 into main Dec 29, 2022
@woodruffw woodruffw deleted the ww/python-path-confusion branch December 29, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:dep-sources Dependency sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running globally installed pip-audit from within a virtualenv produces unintuitive results
2 participants