pfmoore commented 4 years ago

What's the problem this feature will solve? Keyring support is enabled silently when the keyring module is installed in the user's system. There's no way for the user to say whether or not they want keyring support, or to turn it off when it causes an issue. The pip tests don't test the behaviour of the keyring module (just the integration) and there have been a number of issues reported which are down to keyring behaviour: #8634, #8485, #8443, #8090

Describe the solution you'd like Add an --enable-keyring flag to pip that acts as an explicit "opt in" to keyring support. Users who want keyring enabled by default can set this via an environment variable or config file.

This would ensure that users have a clear understanding that they are opting into keyring usage, and would ensure that pip's behaviour can't be changed by unrelated changes to the user's system. It would also allow users having issues with keyring to easily disable it (by omitting the flag).

Also, if the user requests keyring support and the module is not available for some reason, it is easy to give an explicit message explaining the issue, whereas at the moment, pip just silently disables the feature with no explanation.

Alternative Solutions Having an option to disable keyring support would allow users to opt out if they hit issues. But it would do nothing to help users to have a clear understanding of when they have keyring support enabled. It also doesn't address the "silent behaviour change" issue.

Additional context When implemented, keyring support was actively debated, because the approach taken (enabling the feature if a particular package was available) is non-standard. Normally, pip vendors dependencies, and won't implement features that need modules that don't conform to its vendoring requirements. The keyring support was merged, but to be honest the concerns about the exception to our vendoring policy (see the thread starting here for details) were never really addressed. In my view, the current issues stem from that, and this issue is an attempt to contain the problem by making it explicitly "opt in".

pradyunsg commented 4 years ago

I'm strongly in favor of making the keyring support opt-in instead of opt-out.

Nearly all users I've interacted with have mentioned how the keyring support in pip is actually surprising rather than useful to them.

chrahunt commented 4 years ago

We generally see these kinds of requests alongside workflow-impacting changes:

this should be available in requirements.txt so that its usage can be standardized across a team or require fewer setup steps
a flag also needs to be present to disable it, so if it is enabled at a system config or user config level then it can be disabled on a per-query basis

I think it would be good to understand our position now enough to be able to help answer those questions later.

For the implementation of this, we would probably want to defer importing keyring at all until we confirm the opt-in. From discussions in at lease one keyring issue it's possible it could be made to do the backend check lazily, but there's no telling when that makes it into distributions. It would also be nice if we can say "just upgrade pip" to resolve issues like #8485.

pfmoore commented 4 years ago

Good points. Personally, I'd go with:

No, it's global only, and we explicitly don't support it in requirements.txt. That's to limit the complexity in general, and because there's no such support at the moment, so we shouldn't be adding yet more features as part of this issue, which is all about keeping the impact of keyring support limited.
Again, no. There's no way to disable it now short of uninstalling keyring or using whatever options keyring has to switch it off, why should we add one as part of this change?

For the implementation of this, we would probably want to defer importing keyring at all until we confirm the opt-in.

Probably, but only if it's easy to do. If it's complicated, then I'd say defer it to a separate feature request, and leave this as simply switching off the functionality.

(I should probably be clear here, I never actually liked the way the keyring feature was implemented, and if I had my way I'd just rip it out completely. But we're stuck with it now and my main aim is to limit the damage, and make it easier to not have it affect people who don't opt into it. I'm particularly offended by #8485 in this regard.)

Also, I should probably have added under Alternative Solutions - Implement a more robust plugin system for pip that allows reliable addition of "optional" features that rely on non-vendored libraries. But that's easy to say, and much harder to actually do, so I basically just consider keyring support as a demonstration of why such a thing is harder than it looks 🙂

Julian commented 4 years ago

Not sure if it's included in the issues alluded to here (or discussed in that original ticket that I haven't read yet), but similar issues to #6668 arise with the way keyring support works today as well.

Specifically -- because it's not vendored, if you're going to use e.g. tox to run some tests, and need keyring in that venv, it is difficult or impossible today to ensure keyring is installed at the right moment (so fetch keyring from PyPI, then proceed with tox installing everything else from potentially some other place that now needs keyring present).

pradyunsg commented 4 years ago

PRs to implement this are very welcome.

Jmennius commented 4 years ago

Is there a chance the keyring support will be improved by vendoring the package? My concern is mostly because of virtual environments - they break the experience as the system keyring package is not accessible inside a venv.

pfmoore commented 4 years ago

The support cannot be vendored because it includes a C extension somewhere in the dependency chain, and pip only ships as a "pure Python" universal wheel.

Jmennius commented 4 years ago

The support cannot be vendored because it includes a C extension somewhere in the dependency chain, and pip only ships as a "pure Python" universal wheel.

Understood, otherwise, you would consider vendoring it?

pfmoore commented 4 years ago

Personally, I'd be against doing so. The known issues with keyring, with long delays on import, people getting advised to disable it, etc, suggest that vendoring it would add an unacceptable amount to our support burden. Also, I don't know how keyring backends work - we wouldn't want to vendor those so how much would vendoring just keyring help?

Jmennius commented 4 years ago

I agree that making it opt-in is likely the best solution. But when you actually want to use it there is that barrier that keyring is actually a python package and is subject to related restictions (mostly because of venvs).

Usually, keyring backends work as daemons and are preinstalled on desktop operating systems. On Linux, communication with the daemon (Freedesktop SecretService API) is via DBus. DBus may be the reason there are long import delays (there is an option to start the service on the first request to it), and 25 seconds resembles some timeout. I will check upstream if I can help. Maybe, this is the reason people are having issues on Ubuntu WSL, maybe the daemon is just not installed (kind of headless installation, there is even a manual on how to install and use it in headless mode).

For example, with git, you have an ability to configure credential helpers, one of them is libsecret. All you have to do to opt-in is install a package (git-credential-libsecret on Fedora) or compile a single file (Ubuntu/Debian somehow don't package it but the source is included...) and then configure it in your gitconfig.

As i understand from your previous comment the best alternative to bundling is to have a plugin system?

pradyunsg commented 4 years ago

@Jmennius That's how keyrings generally work -- not the keyring package in Python.

We can't vendor keyring since adapters/plugins for it assume that the package is available under the keyring namespace, and if we vendor it, it won't be -- it'd be pip._vendor.keyring.

pfmoore commented 4 years ago

@Jmennius jaraco/keyring#403 is probably where you would find the best place to try to improve things.

Honestly, pip isn't set up for this type of thing - yes, a plugin system would be the best approach, but we don't have one because we don't have a stable programming API for plugins to use, etc, etc. It's an ongoing debate that we aren't likely to resolve in the short term

The way keyring is handled is essentially a "better than nothing" solution, but like any such compromise it's not ideal for anyone 🙁

Jmennius commented 4 years ago

Thank you for your inputs, this clears up the situation for me at least.

dquitmann-op commented 3 years ago

Is there any progress/time plan for this?

I literally spend half a day to debug the "can not uninstall cffi on windows" problem until I found https://github.com/pypa/pip/issues/8443 which in the end leads to here.

I don't want to blame anyone and appreciate the work you are putting into this great tool (without which python really would not be the same), but for this particular thing my feeling is: this should be rolled back as soon as possible (for your own sake). While keyring support might be nice, the issues (especially https://github.com/pypa/pip/issues/8443 breaking pip) speak for themselves, that this was not a thoughtful shot.

Sorry for the rage, this had to go somewhere...

uranusjr commented 3 years ago

I believe everyone basically agrees this needs to be done, but nobody is having enough interest to actually write the code. A PR would definitely be welcomed, and likely promptly reviewed, if you would bother to implement it 🙂

dumblob commented 3 years ago

@nbraud it seems more or less finished in your branch - would you dare to make a PR to get it reviewed?

yan12125 commented 3 years ago

would you dare to make a PR to get it reviewed?

Apparently nbraud already submitted a PR at https://github.com/pypa/pip/pull/9434.

By the way, before that PR is merged, this issue can work-arounded by specifying the "null" keyring backend if you have python-keyring 19.3.0 or newer (see discussions below). That prevents pip from using the system keyring (KWallet, GNOME Keyring, KeePassXC, ...)

PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring pip install ...

piksel commented 3 years ago

Setting that environment variable didn't help in my case:

$ PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring pip install -v fabulous

``` ^CTraceback (most recent call last): File "/usr/bin/pip", line 11, in load_entry_point('pip==20.0.2', 'console_scripts', 'pip')() File "/usr/lib/python3/dist-packages/pip/_internal/cli/main.py", line 73, in main command = create_command(cmd_name, isolated=("--isolated" in cmd_args)) File "/usr/lib/python3/dist-packages/pip/_internal/commands/__init__.py", line 96, in create_command module = importlib.import_module(module_path) File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "", line 1014, in _gcd_import File "", line 991, in _find_and_load File "", line 975, in _find_and_load_unlocked File "", line 671, in _load_unlocked File "", line 848, in exec_module File "", line 219, in _call_with_frames_removed File "/usr/lib/python3/dist-packages/pip/_internal/commands/install.py", line 24, in from pip._internal.cli.req_command import RequirementCommand File "/usr/lib/python3/dist-packages/pip/_internal/cli/req_command.py", line 19, in from pip._internal.network.session import PipSession File "/usr/lib/python3/dist-packages/pip/_internal/network/session.py", line 26, in from pip._internal.network.auth import MultiDomainBasicAuth File "/usr/lib/python3/dist-packages/pip/_internal/network/auth.py", line 36, in import keyring # noqa File "/usr/lib/python3/dist-packages/keyring/__init__.py", line 3, in from .core import ( File "/usr/lib/python3/dist-packages/keyring/core.py", line 189, in init_backend() File "/usr/lib/python3/dist-packages/keyring/core.py", line 93, in init_backend keyrings = filter(limit, backend.get_all_keyring()) File "/usr/lib/python3/dist-packages/keyring/util/__init__.py", line 21, in wrapper func.always_returns = func(*args, **kwargs) File "/usr/lib/python3/dist-packages/keyring/backend.py", line 210, in get_all_keyring return list(rings) File "/usr/lib/python3/dist-packages/keyring/util/__init__.py", line 31, in suppress_exceptions for callable in callables: File "/usr/lib/python3/dist-packages/keyring/util/properties.py", line 26, in __get__ return self.fget.__get__(None, owner)() File "/usr/lib/python3/dist-packages/keyring/backend.py", line 67, in viable cls.priority File "/usr/lib/python3/dist-packages/keyring/util/properties.py", line 26, in __get__ return self.fget.__get__(None, owner)() File "/usr/lib/python3/dist-packages/keyring/backends/kwallet.py", line 140, in priority return super(DBusKeyringKWallet4, cls).priority - 1 File "/usr/lib/python3/dist-packages/keyring/util/properties.py", line 26, in __get__ return self.fget.__get__(None, owner)() File "/usr/lib/python3/dist-packages/keyring/backends/kwallet.py", line 37, in priority bus = dbus.SessionBus(mainloop=DBusGMainLoop()) File "/usr/lib/python3/dist-packages/dbus/_dbus.py", line 212, in __new__ return Bus.__new__(cls, Bus.TYPE_SESSION, private=private, File "/usr/lib/python3/dist-packages/dbus/_dbus.py", line 102, in __new__ bus = BusConnection.__new__(subclass, bus_type, mainloop=mainloop) File "/usr/lib/python3/dist-packages/dbus/bus.py", line 124, in __new__ bus = cls._new_for_bus(address_or_type, mainloop=mainloop) File "/usr/lib/python3/dist-packages/dbus/exceptions.py", line 47, in __init__ def __init__(self, *args, **kwargs): KeyboardInterrupt ```

It still hangs indefinitely in keychain/dbus, but at least it lead me to the only way to enable pip again:

apt remove python3-keychain

uranusjr commented 3 years ago

Your installed version of keyring is likely too old to support the flag. If that’s not the case, please report the issue to keyring.

yan12125 commented 3 years ago

Looks like @piksel had python-keyring older than 19.3, where all keyrings are still attempted even when $PYTHON_KEYRING_BACKEND is set [1]. I've updated my previous comment to include version constraints.

[1] https://github.com/jaraco/keyring/pull/404

piksel commented 3 years ago

Indeed, Ubuntu 20.04 (LTS) uses:

python3-keyring/focal,focal 18.0.1-2ubuntu1 all

which was installed as a dependency of ubuntu-server

suhailxeaser commented 3 years ago

Setting that environment variable didn't help in my case:

$ PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring pip install -v fabulous It still hangs indefinitely in keychain/dbus, but at least it lead me to the only way to enable pip again:
apt remove python3-keychain

What command I use for windows instead of apt ?

Darsstar commented 2 years ago

FYI https://github.com/Darsstar/pip/tree/vendor-keyring-subprocess is a proof of concept that keyring could be vendored if we are OK interfacing with keyring via its CLI interface instead of its Python API. (GitHub Action "CI" succeeded.)

The C deps are not vendored because they are not strictly necessary when explicitly setting a different keyring backend. The top commit is the most interesting, HEAD^1 just vendors everything. (Well, vendor.txt might be interesting.)

It would also make keyring support opt-in without adding a new flag because the executable (currently keyring-subprocess) needs to be present on the PATH. (It does not prevent adding the flag, so it can be added in addition to vendoring.)

The longs delays on importing keyring mentioned earlier in this issue should be solved. They should have been causef because no backend is set/configured and it falling back to discovering backends registered as entry points. That is the last of the three fallbacks. The first fallback is looking at environment variables. The second is looking at a config file, if it exists. But the very first thing keyring checks is if keyring.set_keyring() has been called to explcitly set a keyring backend. That is what my POC does.

Security wise it doesn't seem any more or less secure since adding a keyring.py in the right location would also let someone execute their code. So it is probably just a matter of taste to keep using keyring's Python API instead of it's CLI API.

Some possible motivation:

I have only glaced at the pip as a zipapp issues/discussions, but keyring was mentioned in those.
- This is what pushed me from thinking "calling an executable on the PATH is probably a big no-no" to "well, I should probably throw the idea out there and have others make that decission instead of implicitly making it for them by not mentioning it".
I created keyring-subprocess to make bootstrapping easier for me and my colleagues by including a Virtualenv seeder, but Pipx and pyprojectx don't use virtualenv to create venvs. Having keyring-subprocess or something similar vendored into Pip (and have https://github.com/pypa/pip/issues/11020 be resolved) would be a massive win for us. (And I would get to remove the seeder implementation.)

I do appologive for the structure of this comment/post. Hopefully it is sufficient even though I'm not happy with it.

So I guess the only thing left is: is this something I should make a polished PR for or is https://github.com/pypa/pip/issues/11215 the only PR that should be merged to close this issue?

Darsstar commented 2 years ago

I turned the PoC into an actual PR: #11399. The test failure ~seems~ is unrelated to the changes in the PR.

SnoopJ commented 2 years ago

PRs to implement this are very welcome.

A PR would definitely be welcomed, and likely promptly reviewed, if you would bother to implement it :slightly_smiling_face:

@pradyunsg @uranusjr any chance for some review of #11399 and/or #11215? :grin:

Darsstar commented 1 year ago

(There is a question for maintainsers at the end.)

So I closed #11399 but @uranusjr made a comment relevant to this issue. Keyring can now be queried by importing and calling its API or the cli interface in a subprocess. Should we instead have more influence than just on/off? And instead be able to pick from disabled, import, subprocess and maybe try-import-then-subprocess (which I would name auto)

Ik have a branch for the first three options and can add the fouth. It is currently based on #11029 PR which is at the time of writing part of the 23.0 milestone.

Should I rebase that branch on main before making a PR? Should I include it in #11029 instead? Should I polish it and make a PR without rebasing? Or is a on/off switch still prefered even though new keyring related functionality got added?

PS. Sorry, I wasn't able to actually make it one question for guidance without it being to vague about the sort of answer I expect.

uranusjr commented 1 year ago

I like the option idea.

Darsstar commented 1 year ago

I like the option idea.

pypa / pip

Keyring support should require an --enable-keyring flag #8719

11698 is ready for review