-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Keyring support should require an --enable-keyring flag #8719
Comments
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. |
We generally see these kinds of requests alongside workflow-impacting changes:
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. |
Good points. Personally, I'd go with:
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 🙂 |
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). |
PRs to implement this are very welcome. |
Is there a chance the keyring support will be improved by vendoring the package? |
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? |
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? |
I agree that making it opt-in is likely the best solution. 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. For example, with git, you have an ability to configure credential helpers, one of them is As i understand from your previous comment the best alternative to bundling is to have a plugin system? |
@Jmennius That's how keyrings generally work -- not the We can't vendor |
@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 🙁 |
Thank you for your inputs, this clears up the situation for me at least. |
Is there any progress/time plan for this? I literally spend half a day to debug the "can not uninstall I don't want to blame anyone and appreciate the work you are putting into this great tool (without which Sorry for the rage, this had to go somewhere... |
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 🙂 |
Per the discussion in pypa#8719 (keyring support should be opt-in) > [...] defer importing keyring at all until we confirm the opt-in > pypa#8719 (comment)
Per the discussion in pypa#8719 (keyring support should be opt-in) > [...] defer importing keyring at all until we confirm the opt-in > pypa#8719 (comment)
@nbraud it seems more or less finished in your branch - would you dare to make a PR to get it reviewed? |
Apparently nbraud already submitted a PR at #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, ...)
|
Setting that environment variable didn't help in my case:
|
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. |
Looks like @piksel had python-keyring older than 19.3, where all keyrings are still attempted even when |
Indeed, Ubuntu 20.04 (LTS) uses:
which was installed as a dependency of |
What command I use for windows instead of |
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 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 Security wise it doesn't seem any more or less secure since adding a Some possible motivation:
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 #11215 the only PR that should be merged to close this issue? |
@pradyunsg @uranusjr any chance for some review of #11399 and/or #11215? 😁 |
(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 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. |
I like the option idea. |
#11698 is ready for review |
Bumps [pip](https://github.com/pypa/pip) from 23.0.1 to 23.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's changelog</a>.</em></p> <blockquote> <h1>23.1 (2023-04-15)</h1> <h2>Deprecations and Removals</h2> <ul> <li>Remove support for the deprecated <code>--install-options</code>. (<code>[#11358](pypa/pip#11358) <https://github.com/pypa/pip/issues/11358></code>_)</li> <li><code>--no-binary</code> does not imply <code>setup.py install</code> anymore. Instead a wheel will be built locally and installed. (<code>[#11451](pypa/pip#11451) <https://github.com/pypa/pip/issues/11451></code>_)</li> <li><code>--no-binary</code> does not disable the cache of locally built wheels anymore. It only means "don't download wheels". (<code>[#11453](pypa/pip#11453) <https://github.com/pypa/pip/issues/11453></code>_)</li> <li>Deprecate <code>--build-option</code> and <code>--global-option</code>. Users are invited to switch to <code>--config-settings</code>. (<code>[#11859](pypa/pip#11859) <https://github.com/pypa/pip/issues/11859></code>_)</li> <li>Using <code>--config-settings</code> with projects that don't have a <code>pyproject.toml</code> now print a deprecation warning. In the future the presence of config settings will automatically enable the default build backend for legacy projects and pass the setttings to it. (<code>[#11915](pypa/pip#11915) <https://github.com/pypa/pip/issues/11915></code>_)</li> <li>Remove <code>setup.py install</code> fallback when building a wheel failed for projects without <code>pyproject.toml</code>. (<code>[#8368](pypa/pip#8368) <https://github.com/pypa/pip/issues/8368></code>_)</li> <li>When the <code>wheel</code> package is not installed, pip now uses the default build backend instead of <code>setup.py install</code> for project without <code>pyproject.toml</code>. (<code>[#8559](pypa/pip#8559) <https://github.com/pypa/pip/issues/8559></code>_)</li> </ul> <h2>Features</h2> <ul> <li>Specify egg-link location in assertion message when it does not match installed location to provide better error message for debugging. (<code>[#10476](pypa/pip#10476) <https://github.com/pypa/pip/issues/10476></code>_)</li> <li>Present conflict information during installation after each choice that is rejected (pass <code>-vv</code> to <code>pip install</code> to show it) (<code>[#10937](pypa/pip#10937) <https://github.com/pypa/pip/issues/10937></code>_)</li> <li>Display dependency chain on each Collecting/Processing log line. (<code>[#11169](pypa/pip#11169) <https://github.com/pypa/pip/issues/11169></code>_)</li> <li>Support a per-requirement <code>--config-settings</code> option in requirements files. (<code>[#11325](pypa/pip#11325) <https://github.com/pypa/pip/issues/11325></code>_)</li> <li>The <code>--config-settings</code>/<code>-C</code> option now supports using the same key multiple times. When the same key is specified multiple times, all values are passed to the build backend as a list, as opposed to the previous behavior, where pip would only pass the last value if the same key was used multiple times. (<code>[#11681](pypa/pip#11681) <https://github.com/pypa/pip/issues/11681></code>_)</li> <li>Add <code>-C</code> as a short version of the <code>--config-settings</code> option. (<code>[#11786](pypa/pip#11786) <https://github.com/pypa/pip/issues/11786></code>_)</li> <li>Reduce the number of resolver rounds, since backjumping makes the resolver more efficient in finding solutions. This also makes pathological cases fail quicker. (<code>[#11908](pypa/pip#11908) <https://github.com/pypa/pip/issues/11908></code>_)</li> <li>Warn if <code>--hash</code> is used on a line without requirement in a requirements file. (<code>[#11935](pypa/pip#11935) <https://github.com/pypa/pip/issues/11935></code>_)</li> <li>Stop propagating CLI <code>--config-settings</code> to the build dependencies. They already did not propagate to requirements provided in requirement files. To pass the same config settings to several requirements, users should provide the requirements as CLI arguments. (<code>[#11941](pypa/pip#11941) <https://github.com/pypa/pip/issues/11941></code>_)</li> <li>Support wheel cache when using <code>--require-hashes</code>. (<code>[#5037](pypa/pip#5037) <https://github.com/pypa/pip/issues/5037></code>_)</li> <li>Add <code>--keyring-provider</code> flag. See the Authentication page in the documentation for more info. (<code>[#8719](pypa/pip#8719) <https://github.com/pypa/pip/issues/8719></code>_)</li> <li>In the case of virtual environments, configuration files are now also included from the base installation. (<code>[#9752](pypa/pip#9752) <https://github.com/pypa/pip/issues/9752></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fix grammar by changing "A new release of pip available:" to "A new release of pip is available:" in the notice used for indicating that. (<code>[#11529](pypa/pip#11529) <https://github.com/pypa/pip/issues/11529></code>_)</li> <li>Normalize paths before checking if installed scripts are on PATH. (<code>[#11719](pypa/pip#11719) <https://github.com/pypa/pip/issues/11719></code>_)</li> <li>Correct the way to decide if keyring is available. (<code>[#11774](pypa/pip#11774) <https://github.com/pypa/pip/issues/11774></code>_)</li> <li>More consistent resolution backtracking by removing legacy hack related to setuptools resolution (<code>[#11837](pypa/pip#11837) <https://github.com/pypa/pip/issues/11837></code>_)</li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/pip/commit/6424ac4600265490462015c2fc7f9a402dba9ed8"><code>6424ac4</code></a> Bump for release</li> <li><a href="https://github.com/pypa/pip/commit/868338f9f79b58eff34dafb168aed65480d080d5"><code>868338f</code></a> Update AUTHORS.txt</li> <li><a href="https://github.com/pypa/pip/commit/4f3a4f72697299da1a412cf10c919a989e0692f5"><code>4f3a4f7</code></a> Merge pull request <a href="https://redirect.github.com/pypa/pip/issues/11919">#11919</a> from sbidoul/deprecate-legacy-ignore-config-setting...</li> <li><a href="https://github.com/pypa/pip/commit/dbf4e6842c9603792f6d3944a5c9cec17bd0a92a"><code>dbf4e68</code></a> Merge pull request <a href="https://redirect.github.com/pypa/pip/issues/11897">#11897</a> from sbidoul/cache-hash-checking-sbi</li> <li><a href="https://github.com/pypa/pip/commit/efe2d27451d50b165df78093bf5885da713fbdf8"><code>efe2d27</code></a> Further refactor is_wheel_from_cache</li> <li><a href="https://github.com/pypa/pip/commit/4beca6b4c9c510b19dbb6180e962425b89e8c839"><code>4beca6b</code></a> Improve test</li> <li><a href="https://github.com/pypa/pip/commit/bd746e3136e5e1be2374a079bac66071dd967a8c"><code>bd746e3</code></a> Introduce ireq.cached_wheel_source_link</li> <li><a href="https://github.com/pypa/pip/commit/caafe6e87d4f2998a77b194297e1c204cf6e10c2"><code>caafe6e</code></a> Add a couple of asserts</li> <li><a href="https://github.com/pypa/pip/commit/a6ef6485be9512f18121298b058797c578f65d45"><code>a6ef648</code></a> Rename original_link_is_in_wheel_cache to is_wheel_from_cache</li> <li><a href="https://github.com/pypa/pip/commit/ff8c8e38887880ad81ffd7cfc6a8373213c087b7"><code>ff8c8e3</code></a> Cosmetics</li> <li>Additional commits viewable in <a href="https://github.com/pypa/pip/compare/23.0.1...23.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=23.0.1&new-version=23.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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, #8090Describe 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".
The text was updated successfully, but these errors were encountered: