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.

Sign up for GitHub

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 --keyring-provider flag to configure keyring-based authentication #2592

Merged
merged 23 commits into from
Jan 15, 2025
Merged

add --keyring-provider flag to configure keyring-based authentication #2592

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3207adc
add --with-extra-pip-arg flag
tdyas Nov 13, 2024
c74dc81
revert setuptools addition
tdyas Nov 13, 2024
1fc4eb7
--keychain-provider
tdyas Nov 13, 2024
2f21708
more spots
tdyas Nov 14, 2024
ef3a8f2
fix incorrect variable reference
tdyas Nov 14, 2024
ccc213a
name the option correctly
tdyas Nov 14, 2024
e2d0c26
fix test and comments
tdyas Nov 14, 2024
ba6d06d
Merge branch 'main' into extra_pip_args
tdyas Nov 19, 2024
31662b6
add integration test
tdyas Nov 19, 2024
0963e08
Merge branch 'main' into extra_pip_args
tdyas Jan 10, 2025
755657e
selected wrong side of the merge
tdyas Jan 10, 2025
5b8894f
more post-merge cleanup for lock_downloader
tdyas Jan 11, 2025
8b9a06e
do not pass `--keyring-provider` on Pip versions which do not support it
tdyas Jan 13, 2025
a070c68
improve pip test
tdyas Jan 13, 2025
5bacd6b
redo warning message
tdyas Jan 13, 2025
8dd5098
soften language and fix import
tdyas Jan 13, 2025
f903bf8
remove unused import
tdyas Jan 13, 2025
11c38c9
more cleanups
tdyas Jan 13, 2025
352eb4e
better help message
tdyas Jan 15, 2025
69f822d
little edit
tdyas Jan 15, 2025
9ae677d
fmt
tdyas Jan 15, 2025
8be4043
edit version language
tdyas Jan 15, 2025
668caa6
update warning to refer to `--keyring-provider`
tdyas Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pex/resolve/resolver_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def register(
dest="keyring_provider",
type=str,
default=None,
help="Keychain provider to configure `pip` to use.",
help="`keyring` provider to configure `pip` to use.",
)

register_repos_options(parser)
Expand Down
11 changes: 8 additions & 3 deletions tests/test_pip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,16 +415,21 @@ def test_keyring_provider(
download_dir = os.path.join(str(tmpdir), "downloads")
assert not os.path.exists(download_dir)

with ENV.patch(PIP_KEYCHAIN_PROVIDER="invalid") as env, environment_as(**env):
assert "invalid" == os.environ["PIP_KEYCHAIN_PROVIDER"]
with ENV.patch(PIP_KEYRING_PROVIDER="invalid") as env, environment_as(**env):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if you've seen /home/jsirois/dev/pex-tool/pex/tests/integration/test_keyring_support.py, but its probably what you eventually want to amend to test the "import" + --extra-pip-requirement and "subprocess" + PATH means of hooking the keyring provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't I just update that test to use the new --keyring-provider option instead of the existing --use-pip-config option? (Or paramterize the test to try both methods?)

Copy link
Member

Choose a reason for hiding this comment

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

~the latter is exactly what I was suggesting, yes.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully you've noticed though how tortured the very concept of a keyring provider is. There is a nasty bootstrap problem for any locked down org.

Copy link
Contributor Author

@tdyas tdyas Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, the whole shebang is a marvelous house of cards. The work in Pants using this support focuses on --keyring-provider=subprocess with a trampoline keyring script so Pants won't need to bootstrap keyring PyPi at all.

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely. I'll hint you 1st: how do you think Pex implements --pip-version any-not-vendored-version?

Copy link
Member

@jsirois jsirois Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The answer to that is fairly obvious and I'm sure you've already forehead smacked. That said, solving is thornier. I already had to deal with how to bootstrap --pip-version any-not-vendored-version here:

pex/pex/pip/installation.py

Lines 290 to 305 in 3361e79

bootstrap_pip_version = try_(
compatible_version(
targets,
PipVersion.VENDORED,
context="Bootstrapping Pip {version}".format(version=version),
warn=False,
)
)
if bootstrap_pip_version is not PipVersion.VENDORED and not extra_requirements:
return _pip_installation(
version=version,
iter_distribution_locations=_bootstrap_pip(version, interpreter=interpreter),
interpreter=interpreter,
fingerprint=_fingerprint(extra_requirements),
use_system_time=use_system_time,
)

The context there is using a new enough Python (3.12+) where distutils is gone from the stdlib and vendored Pip fails as a result. In that situation I had to find another way to bootstrap a specific Pip version without using vendored Pip. So, basically, iff using the current python to python -mvenv nets a venv with a Pip new enough to support --keyring-provider, the thorny bootstrap can be just the thorny problem you described, solved with --keyring-provider subprocess and some pre-arranged provider on the PATH. If not, you have an even thornier bootstrap problem than you expected.

Copy link
Member

Choose a reason for hiding this comment

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

Following up a little more, --keyring-provider was added for Pip 23.1. Python 3.10.15 and older come with ensurepip that yield at most Pip 23.0.1; so you can only count on Python>=3.11.4 (3.11 only hit bundled Pip>=23.1 here: python/cpython#103752) for bootstrapping any --pip-version >= 23.1 with --keyring-provider being passed as part of the bootstrap process.

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 think the intended user of my Pants change will be just fine with Python >= 3.11.4. Is that enough to ensure bootstrap of a pip supporting --keyring-provider though? I assume the pip bootstrap happens with the user Python chosen by Pants and not the Pants-specific PBS Python used by scie-pants?

Copy link
Member

Choose a reason for hiding this comment

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

I think the intended user of my Pants change will be just fine with Python >= 3.11.4. Is that enough to ensure bootstrap of a pip supporting --keyring-provider though?

Not with the code as-it-is, since the python -mvenv ... bootstrap trick is not used as aggressively as it could be. That code pointer I gave does highlight the existing mechanism though which should point the way to using it more aggressively.

In short though, knowing which version of Python you have is clearly trivial; then using that to fail fast or move forward with bootstrapping Pip via -mvenv and passing --keyring-provider after that is definitely in the realm of just putting in the work.

As I mentioned below though, I'll think on this whole situation a bit and report back. There may be some better way than either this or picking a random new Pip to vendor (then can't live without it feature with bootstrapping issue comes up in a newer Pip and surely vendoring a 3rd version is a no go, etc...).

assert "invalid" == os.environ["PIP_KEYRING_PROVIDER"]
job = pip.spawn_download_distributions(
download_dir=download_dir,
requirements=["ansicolors==1.1.8"],
package_index_configuration=package_index_configuration(
pip_version=version, keyring_provider="auto"
),
)
assert "--keyring-provider" in job._command, "\n".join(job._command)
keyring_arg = job._command.index("--keyring-provider")
if keyring_arg != -1:
assert job._command[keyring_arg : keyring_arg + 2] == ("--keyring-provider", "auto")
else:
pytest.fail("--keyring-provider was not present in the invoked pip command")

with pytest.raises(Job.Error) as exc:
job.wait()

Expand Down
Loading