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.

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 access requested although non-publishing operations do not need any credentials. #8761

Closed
4 tasks done
real-yfprojects opened this issue Dec 8, 2023 · 13 comments
Closed
4 tasks done
Labels
area/auth Related to the authenticator and keyring kind/bug Something isn't working as expected status/triage This issue needs to be triaged

Comments

@real-yfprojects
Copy link
Contributor

  • Poetry version: 1.8.0.dev0 (6f9de73)
  • Python version: 3.8.10
  • OS version and name: Kubuntu
  • pyproject.toml: Any minimal working configuration, no dependencies are required.
  • I am on the latest stable Poetry version, installed using a recommended method.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have consulted the FAQ and blog for any relevant entries or release notes.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option) and have included the output below.

Issue

In its essence this is the same as #1917 and closely linked to #8623.

Can you summarize the problem?

For any http request made by poetry it will hit the keyring for possible credentials to send along (presumably to support private repositories and registries). However this has the consequents that poetry needs keyring access for almost all operations although most operations do not require any credentials. This is bad. Quoting from #1917 (comment):

I think people should be careful about granting programs access to things they should not need to access. This behavior is an example of a problem that makes that hard to achieve.

Why was #1917 closed?

Very good question. The maintainers apparently thought fixing the symptoms would also fix the underlying problem. See #8078 (comment) and #8227. As a matter of fact the underlying issue now reappears in a different disguise.

What's the new symptom?

#8227 fixed the problem where poetry just exited when the keyring access was denied. In the current version it ignores the denial and sends the http request without credentials.
However for the next http request it will try to access the keyring again. For a command like poetry add pytest this will result in uncountable popups requesting keyring access that one has to deny (hit cancel in my case) one after another. This makes poetry unusable without granting keyring access. It is safe to say that #8227 practically didn't fix anything.

What would be an actual fix?

I can already imagine the maintainers putting forward fixes for this new problem instead of fixing the underlying issue.
The sane way of fixing this goes like that:
Prioritize the uses cases of the majority. Only a minority of packages need credentials for resolving dependencies. For those users the configuration file format could be adjusted so that they can specify when credentials are required. For all other users poetry shouldn't hit the keyring at all when running lock, install, add, ... However for poetry publish credentials may be requested for all users.

@real-yfprojects real-yfprojects added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Dec 8, 2023
@dimbleby
Copy link
Contributor

dimbleby commented Dec 8, 2023

close enough to duplicating #8623 that I doubt it's useful to track separately.

pull requests welcome, I assume

@real-yfprojects
Copy link
Contributor Author

real-yfprojects commented Dec 8, 2023

close enough to duplicating #8623 that I doubt it's useful to track separately.

Yeah, I decided to open this one anyway because I found #8623 lacking in some aspects. And at least in the title, focusses more on the symptoms again.

@radoering
Copy link
Member

Yeah, I decided to open this one anyway because I found #8623 lacking in some aspects.

I think I'm fine with this. Afaik, the other issue describes a specific regression and this issue is more general. So the other issue might be resolved without resolving this one.

I'm not that deep into this keyring topic, but I suppose #7038 (comment) might be a reason why we are trying to use keyring even if it's not necessary:

some indexes rate-limit users based on credentials (e.g. have lower rate limits for unauthenticated users). Always providing credentials, instead of waiting for an error and retrying, seems to be a better behavior as it generates fewer network requests.

I don't feel able to judge if we should stick to it or not.

@codethief
Copy link

codethief commented Jul 13, 2024

Just ran into this issue again: I did

mkdir new-project
cd my-project poetry init
poetry add <some dependency>

Unfortunately, this time I had forgotten to set PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring in my .envrc beforehand (as per #5250). As mentioned in the OP, this

[resulted] in uncountable popups requesting keyring access that one has to deny (hit cancel in my case) one after another

However, in my case, the prompt also prevented me (an i3 user) from switching to any other window or workspace. As a result, I had to switch to another TTY and killall poetry by hand because even after hitting ESC a hundred times, it didn't want to leave me alone. :\

I don't feel able to judge if we should stick to it or not.

In light of the above, I would vote for not sticking to this or at least making this behavior transparent to the user somehow before they invoke poetry add for the first time.

@tmplt
Copy link

tmplt commented Sep 30, 2024

This access attempt is prohibitive as I've just discovered poetry: I use GNOME, but not the keyring functionality, and on an interactive poetry init it asks me for a password. I thought it was asking me for my root password before I found this issue, prompting vendor security questions.

This is confusing and unexpected behavior.

@Secrus Secrus added the area/auth Related to the authenticator and keyring label Oct 13, 2024
@lersek
Copy link

lersek commented Oct 14, 2024

This issue persists with version 1.8.3. As others stated above, the (seemingly) infinite loop of keyring password requests effectively locks up the GUI; it's not even possible to switch to a character VT. ssh from another machine is needed for a killall poetry.

It wouldn't be nearly as bad if we could at least specify a non-default collection on the keyring.

The PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring workaround works though, at least.

lersek added a commit to lersek/scylla that referenced this issue Oct 15, 2024
Python-poetry is affected by bug
<python-poetry/poetry#8761>. Namely, if you have
"keyring" <https://pypi.org/project/keyring/> installed, poetry will try
to gain access to the Default collection in the (ex. GNOME) keyring, even
if poetry only needs read-only access to package repositories, and even if
those repos are public.

Consequently, you either unlock your Default collection for poetry
(unjustifiedly), or your GUI session gets effectively locked up, because
any time you hit Cancel on the keyring unlock dialog, poetry immediately
pops up another, and this dialog grabs the keyboard -- you cannot even
switch to a character VT, for killing poetry; you have to log in via ssh
for that.

This issue is not visible to users who don't use "keyring" (GNOME or
otherwise). For those who do, work around the problem by selecting the
"null" keyring back-end, in the environment of every poetry invocation.

Note: I have not regression-tested the workaround in a desktop environment
where "keyring" is unavailable to begin with.

Signed-off-by: Laszlo Ersek <[email protected]>
lersek added a commit to lersek/sphinx-scylladb-theme that referenced this issue Oct 15, 2024
Python-poetry is affected by bug
<python-poetry/poetry#8761>. Namely, if you have
"keyring" <https://pypi.org/project/keyring/> installed, poetry will try
to gain access to the Default collection in the (ex. GNOME) keyring, even
if poetry only needs read-only access to package repositories, and even if
those repos are public.

Consequently, you either unlock your Default collection for poetry
(unjustifiedly), or your GUI session gets effectively locked up, because
any time you hit Cancel on the keyring unlock dialog, poetry immediately
pops up another, and this dialog grabs the keyboard -- you cannot even
switch to a character VT, for killing poetry; you have to log in via ssh
for that.

This issue is not visible to users who don't use "keyring" (GNOME or
otherwise). For those who do, work around the problem by selecting the
"null" keyring back-end, in the environment of every poetry invocation.

Note: I have not regression-tested the workaround in a desktop environment
where "keyring" is unavailable to begin with.

Signed-off-by: Laszlo Ersek <[email protected]>
avikivity added a commit to scylladb/scylladb that referenced this issue Oct 16, 2024
…from Laszlo Ersek

- `docs/Makefile`: work around python-poetry issue python-poetry/poetry#8761
- `docs/README.md`: fix minimum poetry version

No backporting needed (docs development).

Closes #21118

* github.com:scylladb/scylladb:
  docs/README.md: fix minimum poetry version
  docs/Makefile: work around python-poetry issue #8761
@abn
Copy link
Member

abn commented Jan 17, 2025

Folks who have environments where you can reproduce this issue still with 2.0.1, can you please try the fix at #10062? This should significantly improve status quo. Note that the fix might not yet work for folks who have a dbus-session, keyring available and a system prompter is not available.

Using pipx

pipx install --suffix=@10062 'poetry @ git+https://github.com/python-poetry/poetry.git@refs/pull/10062/head'
poetry@10062 new foobar
cd foobar
poetry@10062 add -v pycowsay httpx

Using a container (podman | docker)

podman run --rm -i --entrypoint bash docker.io/python:latest <<EOF
set -xe
python -m pip install --disable-pip-version-check --root-user-action ignore -q git+https://github.com/python-poetry/poetry.git@refs/pull/10062/head
poetry new foobar
pushd foobar
poetry add -v pycowsay httpx
EOF

@abn
Copy link
Member

abn commented Jan 17, 2025

Prioritize the uses cases of the majority. Only a minority of packages need credentials for resolving dependencies. For those users the configuration file format could be adjusted so that they can specify when credentials are required. For all other users poetry shouldn't hit the keyring at all when running lock, install, add, ... However for poetry publish credentials may be requested for all users.

While the perspective is appreciated, I am not certain there is evidence either way to suggest what the majority is. Making it opt-in unfortunately has the consequence that users might never end up enabling it just by virtue of not being aware of it. As maintainers of dx tools, we need to cater for a wide spectrum of users constantly.

Access to keyring is already "on-demand" only, the problem right now is when the keyring is locked by a user the prompt might be required mid-way through handling a command. Especially if you consider that Poetry needs to check if a credential exist before a request - so simply assuming a non-publishing command will not require it, is an incomplete assumption.

Disabling it entirely was proposed in #9820. Which is deferred to the next major if we cannot make the ux error free enough.

The cleanest way forward right now, I believe, is to perform preflight validation of keyring etc. and investigations during #10062 revealed the GUI lockup was being caused due to a thread safety issue in the executor. Yes, another "symptom" - aka bug. Once this is merged, we will see if something else pops up.

There might be a middle-way as well, in that for cases where there are no sources defined and the command is not publish, we can dynamically set keyring to be disabled. But need to consider if that has any unexpected consequences.

@abn
Copy link
Member

abn commented Jan 21, 2025

I am going to close this as this is not really actionable for the following reasons.

  1. All installer commands (lock, update, install etc.) require access to credential store (irrespective of using your OS credential store or plain text file) - the latter is the default fallback).
  2. Keyring also enables a wide array of cases, like for example developers using Azure, GCP, AWS etc as private package indices. Each of those have keyring backend providers that automatically configure and make available service level short lived credentials. This is not a minority case.
  3. The developers having issues with keyring in their environment are almost always encountering them due to bugs in Poetry's usage (https://gitlab.gnome.org/GNOME/gnome-keyring/-/issues/160) ; or due to them having a broken environment/session (fingerprint login not unlocking keyring at login etc).
  4. While it may appear there are issues with the default, it must also be noted that Poetry is downloaded ~2 million times a day and hopefully being run more than that. So, for a large portion of users this is a non-issue.

For now keyring is enabled by default and is opt-out only. And there will be ongoing efforts to fix legitimate bugs that are reported.

The keyring prompt issue is being tracked in #8623 resolving via #10062.

As a final note, we sincerely appreciate you taking the time and effort to raise an issue with meaningful details.

@abn abn closed this as completed Jan 21, 2025
@trygveaa
Copy link

trygveaa commented Jan 21, 2025

All installer commands (lock, update, install etc.) require access to credential store (irrespective of using your OS credential store or plain text file) - the latter is the default fallback).

Why do you need the credential store for those actions when the project only has public dependencies? Can't you check if there are private dependencies when the command starts, and only request it if necessary?

@real-yfprojects
Copy link
Contributor Author

While it may appear there are issues with the default, it must also be noted that Poetry is downloaded ~2 million times a day and hopefully being run more than that. So, for a large portion of users this is a non-issue.

IMHO this is an issue for a vast majority of users as I imagine most projects to rely on public dependencies only. However most users aren't aware of this security flaw presented by poetry accessing the keyring without need since they have there keyring open all the time or simply accept any keyring request they encounter. Both isn't recommended and poetry shouldn't push resp. force users into such behaviour by setting unreasonable defaults. As frequently mentioned private dependencies could be detected e.g. using a keyword in the config file.

Frankly I don't understand your first point. From an external perspective the keyring isn't generally needed for install, ... but only in the case of private deps. This seems to be an artificial/internal requirement.

@abn
Copy link
Member

abn commented Jan 21, 2025

Why do you need the credential store for those actions when the project only has public dependencies? Can't you check if there are private dependencies when the command starts, and only request it if necessary?

It's not always that straightforward. If there was an additional metadata for sources that is propagated to the lockfile, maybe. And further, we will have defer downloads including, including metadata retrieval to after we go through all potential download candidates and then map it to sources etc. to identify if we need to access keyring. Beyond that there is also git credentials that use basic http (which could be an indirect direct origin dependency).

And the check for credentials is only done once for each repo during the whole process. The unlock attempt will only be done once too with the upcoming changes.

@abn
Copy link
Member

abn commented Jan 21, 2025

From an external perspective the keyring isn't generally needed for install, ... but only in the case of private deps. This seems to be an artificial/internal requirement.

You are welcome to your opinion there.

As I mentioned in the previous comment, without a new flag for each source it is not straightforward to guess if keyring will be required before the command execution.

The only case we can do so right now might be if all files listed in the lockfile are guaranteed to be from PyPI and there are no direct origin dependencies in the tree. Even then, that assumption might be too brittle.

You are welcome to submit a PR for that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth Related to the authenticator and keyring kind/bug Something isn't working as expected status/triage This issue needs to be triaged
Projects
None yet
9 participants