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 7 commits
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
1 change: 1 addition & 0 deletions pex/cli/commands/lock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,7 @@ def _sync(self):
pip_version=pip_configuration.version,
use_pip_config=pip_configuration.use_pip_config,
extra_pip_requirements=pip_configuration.extra_requirements,
keyring_provider=pip_configuration.keyring_provider,
result_type=InstallableType.INSTALLED_WHEEL_CHROOT,
)
)
Expand Down
9 changes: 9 additions & 0 deletions pex/pip/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def create(
password_entries=(), # type: Iterable[PasswordEntry]
use_pip_config=False, # type: bool
extra_pip_requirements=(), # type: Tuple[Requirement, ...]
keyring_provider=None, # type: Optional[str]
):
# type: (...) -> PackageIndexConfiguration
resolver_version = resolver_version or ResolverVersion.default(pip_version)
Expand All @@ -174,6 +175,7 @@ def create(
use_pip_config=use_pip_config,
extra_pip_requirements=extra_pip_requirements,
password_entries=password_entries,
keyring_provider=keyring_provider,
)

def __init__(
Expand All @@ -186,6 +188,7 @@ def __init__(
password_entries=(), # type: Iterable[PasswordEntry]
pip_version=None, # type: Optional[PipVersionValue]
extra_pip_requirements=(), # type: Tuple[Requirement, ...]
keyring_provider=None, # type: Optional[str]
):
# type: (...) -> None
self.resolver_version = resolver_version # type: ResolverVersion.Value
Expand All @@ -196,6 +199,7 @@ def __init__(
self.password_entries = password_entries # type: Iterable[PasswordEntry]
self.pip_version = pip_version # type: Optional[PipVersionValue]
self.extra_pip_requirements = extra_pip_requirements # type: Tuple[Requirement, ...]
self.keyring_provider = keyring_provider # type: Optional[str]


if TYPE_CHECKING:
Expand Down Expand Up @@ -393,6 +397,11 @@ def _spawn_pip_isolated(
# `~/.config/pip/pip.conf`.
pip_args.append("--isolated")

# Configure a keychain provider if so configured.
if package_index_configuration and package_index_configuration.keyring_provider:
pip_args.append("--keyring-provider")
pip_args.append(package_index_configuration.keyring_provider)

if log:
pip_args.append("--log")
pip_args.append(log)
Expand Down
2 changes: 2 additions & 0 deletions pex/resolve/configured_resolve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def resolve(
pip_version=lock.pip_version,
use_pip_config=pip_configuration.use_pip_config,
extra_pip_requirements=pip_configuration.extra_requirements,
keyring_provider=pip_configuration.keyring_provider,
result_type=result_type,
dependency_configuration=dependency_configuration,
)
Expand Down Expand Up @@ -130,6 +131,7 @@ def resolve(
resolver=ConfiguredResolver(pip_configuration=resolver_configuration),
use_pip_config=resolver_configuration.use_pip_config,
extra_pip_requirements=resolver_configuration.extra_requirements,
keyring_provider=resolver_configuration.keyring_provider,
result_type=result_type,
dependency_configuration=dependency_configuration,
)
2 changes: 2 additions & 0 deletions pex/resolve/configured_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def resolve_lock(
pip_version=pip_version or self.pip_configuration.version,
use_pip_config=self.pip_configuration.use_pip_config,
extra_pip_requirements=self.pip_configuration.extra_requirements,
keyring_provider=self.pip_configuration.keyring_provider,
result_type=result_type,
)
)
Expand Down Expand Up @@ -113,5 +114,6 @@ def resolve_requirements(
if extra_resolver_requirements is not None
else self.pip_configuration.extra_requirements
),
keyring_provider=self.pip_configuration.keyring_provider,
result_type=result_type,
)
7 changes: 7 additions & 0 deletions pex/resolve/lock_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ def __init__(
resolver=None, # type: Optional[Resolver]
use_pip_config=False, # type: bool
extra_pip_requirements=(), # type: Tuple[Requirement, ...]
keyring_provider=None, # type: Optional[str]
):
super(VCSArtifactDownloadManager, self).__init__(
pex_root=pex_root, file_lock_style=file_lock_style
Expand All @@ -117,6 +118,7 @@ def __init__(
self._resolver = resolver
self._use_pip_config = use_pip_config
self._extra_pip_requirements = extra_pip_requirements
self._keyring_provider = keyring_provider

def save(
self,
Expand All @@ -143,6 +145,7 @@ def save(
resolver=self._resolver,
use_pip_config=self._use_pip_config,
extra_pip_requirements=self._extra_pip_requirements,
keyring_provider=self._keyring_provider,
)
if len(downloaded_vcs.local_distributions) != 1:
return Error(
Expand Down Expand Up @@ -257,6 +260,7 @@ def resolve_from_lock(
pip_version=None, # type: Optional[PipVersionValue]
use_pip_config=False, # type: bool
extra_pip_requirements=(), # type: Tuple[Requirement, ...]
keyring_provider=None, # type: Optional[str]
result_type=InstallableType.INSTALLED_WHEEL_CHROOT, # type: InstallableType.Value
dependency_configuration=DependencyConfiguration(), # type: DependencyConfiguration
):
Expand Down Expand Up @@ -311,6 +315,7 @@ def resolve_from_lock(
password_entries=PasswordDatabase.from_netrc().append(password_entries).entries,
use_pip_config=use_pip_config,
extra_pip_requirements=extra_pip_requirements,
keyring_provider=keyring_provider,
),
max_parallel_jobs=max_parallel_jobs,
),
Expand All @@ -332,6 +337,7 @@ def resolve_from_lock(
resolver=resolver,
use_pip_config=use_pip_config,
extra_pip_requirements=extra_pip_requirements,
keyring_provider=keyring_provider,
)
for resolved_subset in subset_result.subsets
}
Expand Down Expand Up @@ -450,6 +456,7 @@ def resolve_from_lock(
password_entries=PasswordDatabase.from_netrc().append(password_entries).entries,
use_pip_config=use_pip_config,
extra_pip_requirements=extra_pip_requirements,
keyring_provider=keyring_provider,
),
compile=compile,
build_configuration=build_configuration,
Expand Down
2 changes: 2 additions & 0 deletions pex/resolve/lockfile/create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ def create(
),
use_pip_config=pip_configuration.use_pip_config,
extra_pip_requirements=pip_configuration.extra_requirements,
keyring_provider=pip_configuration.keyring_provider,
)

configured_resolver = ConfiguredResolver(pip_configuration=pip_configuration)
Expand Down Expand Up @@ -427,6 +428,7 @@ def create(
resolver=configured_resolver,
use_pip_config=pip_configuration.use_pip_config,
extra_pip_requirements=pip_configuration.extra_requirements,
keyring_provider=pip_configuration.keyring_provider,
dependency_configuration=dependency_configuration,
)
except resolvers.ResolveError as e:
Expand Down
1 change: 1 addition & 0 deletions pex/resolve/pre_resolved_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def resolve_from_dists(
password_entries=pip_configuration.repos_configuration.password_entries,
use_pip_config=pip_configuration.use_pip_config,
extra_pip_requirements=pip_configuration.extra_requirements,
keyring_provider=pip_configuration.keyring_provider,
)
build_and_install = BuildAndInstallRequest(
build_requests=[
Expand Down
1 change: 1 addition & 0 deletions pex/resolve/resolver_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ class PipConfiguration(object):
allow_version_fallback = attr.ib(default=True) # type: bool
use_pip_config = attr.ib(default=False) # type: bool
extra_requirements = attr.ib(default=()) # type Tuple[Requirement, ...]
keyring_provider = attr.ib(default=None) # type: Optional[str]


@attr.s(frozen=True)
Expand Down
11 changes: 11 additions & 0 deletions pex/resolve/resolver_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,18 @@ def register(
"See: https://pip.pypa.io/en/stable/topics/authentication/#keyring-support"
),
)

register_use_pip_config(parser)

parser.add_argument(
"--keyring-provider",
metavar="PROVIDER",
dest="keyring_provider",
type=str,
default=None,
help="`keyring` provider to configure `pip` to use.",
)

register_repos_options(parser)
register_network_options(parser)

Expand Down Expand Up @@ -681,6 +691,7 @@ def create_pip_configuration(
allow_version_fallback=options.allow_pip_version_fallback,
use_pip_config=get_use_pip_config_value(options),
extra_requirements=tuple(options.extra_pip_requirements),
keyring_provider=options.keyring_provider,
)


Expand Down
4 changes: 4 additions & 0 deletions pex/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,7 @@ def resolve(
resolver=None, # type: Optional[Resolver]
use_pip_config=False, # type: bool
extra_pip_requirements=(), # type: Tuple[Requirement, ...]
keyring_provider=None, # type: Optional[str]
result_type=InstallableType.INSTALLED_WHEEL_CHROOT, # type: InstallableType.Value
dependency_configuration=DependencyConfiguration(), # type: DependencyConfiguration
):
Expand Down Expand Up @@ -1108,6 +1109,7 @@ def resolve(
password_entries=password_entries,
use_pip_config=use_pip_config,
extra_pip_requirements=extra_pip_requirements,
keyring_provider=keyring_provider,
)

if not build_configuration.allow_wheels:
Expand Down Expand Up @@ -1276,6 +1278,7 @@ def download(
resolver=None, # type: Optional[Resolver]
use_pip_config=False, # type: bool
extra_pip_requirements=(), # type: Tuple[Requirement, ...]
keyring_provider=None, # type: Optional[str]
dependency_configuration=DependencyConfiguration(), # type: DependencyConfiguration
):
# type: (...) -> Downloaded
Expand Down Expand Up @@ -1322,6 +1325,7 @@ def download(
password_entries=password_entries,
use_pip_config=use_pip_config,
extra_pip_requirements=extra_pip_requirements,
keyring_provider=keyring_provider,
)
build_requests, download_results = _download_internal(
targets=targets,
Expand Down
43 changes: 41 additions & 2 deletions tests/test_pip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,21 @@ def test_no_duplicate_constraints_pex_warnings(
def package_index_configuration(
pip_version, # type: PipVersionValue
use_pip_config=False, # type: bool
keyring_provider=None, # type: Optional[str]
):
# type: (...) -> PackageIndexConfiguration
if pip_version is PipVersion.v23_2:
# N.B.: Pip 23.2 has a bug handling PEP-658 metadata with the legacy resolver; so we use the
# 2020 resolver to work around. See: https://github.com/pypa/pip/issues/12156
return PackageIndexConfiguration.create(
pip_version, resolver_version=ResolverVersion.PIP_2020, use_pip_config=use_pip_config
pip_version,
resolver_version=ResolverVersion.PIP_2020,
use_pip_config=use_pip_config,
keyring_provider=keyring_provider,
)
return PackageIndexConfiguration.create(use_pip_config=use_pip_config)
return PackageIndexConfiguration.create(
use_pip_config=use_pip_config, keyring_provider=keyring_provider
)


@pytest.mark.skipif(
Expand Down Expand Up @@ -395,6 +401,39 @@ def test_use_pip_config(
assert "invalid --python-version value: 'invalid'" in str(exc.value.stderr)


@applicable_pip_versions
def test_keyring_provider(
create_pip, # type: CreatePip
version, # type: PipVersionValue
current_interpreter, # type: PythonInterpreter
tmpdir, # type: Any
):
# type: (...) -> None

pip = create_pip(current_interpreter, version=version)

download_dir = os.path.join(str(tmpdir), "downloads")
assert not os.path.exists(download_dir)

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"
),
)
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()


@applicable_pip_versions
def test_extra_pip_requirements_pip_not_allowed(
create_pip, # type: CreatePip
Expand Down
Loading