Isolate salt-pip's PYTHONPATH from the parent environment - #70157
Open
twangboy wants to merge 5 commits into
Open
Isolate salt-pip's PYTHONPATH from the parent environment#70157twangboy wants to merge 5 commits into
twangboy wants to merge 5 commits into
Conversation
twangboy
force-pushed
the
fix/70151/3008.x
branch
from
August 27, 2026 16:40
f9e00dc to
db15d9d
Compare
twangboy
force-pushed
the
fix/70151/3008.x
branch
from
August 27, 2026 19:24
db15d9d to
39f3174
Compare
twangboy
force-pushed
the
fix/70151/3008.x
branch
from
August 27, 2026 22:59
39f3174 to
58c6f89
Compare
_pip_environment() only ever adds env vars to the pip subprocess it builds; it never strips anything already present. --find-links is independent of --no-index by pip's own design (confirmed: `pip install --no-index --find-links <url> pkg` still fetches via find-links), so an inherited PIP_FIND_LINKS remained a live network path even with saltpip_no_index enabled. Add saltpip_allow_find_links (default True, matching current behavior) to strip an inherited PIP_FIND_LINKS when set to False. Deliberately independent of saltpip_no_index rather than folded into it: --find-links alongside a disabled index is pip's own documented air-gapped-install pattern, so forcibly stripping it whenever saltpip_no_index is on would break that legitimate use. PIP_INDEX_URL/PIP_EXTRA_INDEX_URL are left untouched by design: pip already ignores both once PIP_NO_INDEX=1 is set, so stripping them in that case has no effect, and filtering them outside that mode is a separate, unaddressed gap - noted in the docstring and docs rather than papered over with more options. Related to saltstack#70151
The test didn't mock os.environ.copy() like its sibling test does, so it picked up the real CI job's ambient environment. GitHub Actions' onedir test jobs already set PIP_DISABLE_PIP_VERSION_CHECK=1 at the job level for their own unrelated reasons, and _pip_environment() correctly leaves pre-existing env vars alone when the corresponding option is off - so that ambient value passed straight through and failed the test's "must not be present" assertion. Mock os.environ.copy to a clean, empty dict so the test controls its own baseline environment instead of depending on the host/CI runner being free of PIP_* vars. Reproduced the failure locally by setting PIP_DISABLE_PIP_VERSION_CHECK=1 before running pytest, confirmed the fix resolves it either way. Related to saltstack#70151
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
_pip_environment()insalt/scripts.pyprepended salt's ownextrasdirectory onto whateverPYTHONPATHthe calling process already had, rather than replacing it. Anything inherited there (e.g. leaked from an unrelated build/init process) stayed visible to salt's pip subprocess.Combined with
--force-reinstall, pip uninstalls whatever satisfies a requirement wherever it finds it on sys.path, not just in the target directory. An inheritedPYTHONPATHpointing at an unrelated Python installation could therefore causesalt-pipto delete a package belonging to that unrelated environment.PYTHONPATHis now set to just theextrasdirectory by default, matching the isolationsalt-pipalready documents. Extras packages remain importable at runtime via the onedir's.pthfile mechanism, which is independent ofPYTHONPATH, so this doesn't affect normal usage.Five new minion config options make this configurable rather than hardcoded, so a site can opt back into old behavior or lock
salt-pipdown further, instead of relying on every caller to pass the right flags by hand:saltpip_use_pythonpath(defaultFalse): restores the old prepend-onto-inherited-PYTHONPATHbehavior for sites that specifically need it. Isolated behavior stays the default.saltpip_no_deps,saltpip_no_index,saltpip_disable_pip_version_check(all defaultFalse, i.e. unchanged behavior): forcePIP_NO_DEPS/PIP_NO_INDEX/PIP_DISABLE_PIP_VERSION_CHECKonsalt-pip's subprocess, so an operator can guaranteesalt-pipnever resolves dependencies, queries an index, or checks for a newer pip release. Implemented as environment variables rather than CLI flags, sincesalt-pipproxies arbitrarypipsubcommands (list,show,uninstall, ...) andpipsilently ignores these env vars on subcommands that don't use them, whereas the equivalent CLI flags would error out on those subcommands.saltpip_allow_find_links(defaultTrue, matching current behavior):--find-linksis independent of--no-indexby pip's own design (confirmed empirically), sosaltpip_no_indexalone doesn't stop pip from fetching via an inherited/leakedPIP_FIND_LINKS. Set toFalseto strip it as well. Kept as a separate, independent option rather than folded intosaltpip_no_index, since--find-linksalongside a disabled index ispip's own documented air-gapped-install pattern — forcing it off wheneversaltpip_no_indexis on would break that legitimate use.Known, deliberately deferred gap: an inherited
PIP_INDEX_URL/PIP_EXTRA_INDEX_URLis left untouched by every option here.pipalready ignores both oncePIP_NO_INDEX=1is set, so stripping them in that case has no effect; filtering them whensaltpip_no_indexis off is a separate gap, documented as out of scope rather than addressed with more options (weighed against addingsaltpip_allow_index_url/saltpip_allow_extra_index_url, which would be silent no-ops wheneversaltpip_no_indexis enabled — not implemented). Direct URL/VCS requirements (pip install https://..., git+https://...) also bypass--no-indexinherently and can't be blocked by any pip flag.Documentation updates:
doc/topics/packaging/index.rstandsalt/modules/pip.py: the pip execution module/state defaults to the samesalt-pip/extrasbehavior on a onedir minion, how to target the system Python instead viabin_env, and an unsupported stop-gap procedure for patching a vulnerable bundled dependency directly.doc/ref/configuration/minion.rstanddoc/topics/packaging/index.rst: all five new config options, including why the network options are env-var-based and thesaltpip_allow_find_links/saltpip_no_indexindependence rationale.Test coverage added:
tests/pytests/unit/test_scripts.pyandtests/pytests/unit/test_salt_pip_user.pycovering the isolated-vs-opt-inPYTHONPATHbehavior, each network-lockdown option,saltpip_allow_find_links(including that it's unaffected bysaltpip_no_index's state), and each option flowing throughsalt_pip()end-to-end into the subprocess environment.tests/pytests/functional/cli/test_salt_pip.pyconfirming installing a salt extension that depends on a package already bundled in the onedir doesn't duplicate it intoextras, and thatsalt-pipdoesn't see or touch an unrelated ("system") Python's packages even under--force-reinstall.What issues does this PR fix or reference?
Fixes #70151
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes