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

disable keyring per default #9866

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

radoering
Copy link
Member

Pull Request Check List

Resolves: #8623
Resolves: #8761
Closes: #9820

Basically, #9820 without the extra part.

  • Added tests for changed code.
  • Updated documentation for changed code.

@abn
Copy link
Member

abn commented Nov 22, 2024

I am wondering if we simply follow what pip's behavior is here.

https://pip.pypa.io/en/stable/topics/authentication/#keyring-support

The default value auto respects --no-input and does not query keyring at all if the option is used; otherwise it tries the import, subprocess, and disabled providers (in this order) and uses the first one that works.

The majority of the issues occur when keyring is used in a non-interactive session (ci/containers). To me, this is the best balance.

@radoering
Copy link
Member Author

Do we auto-detect a non-interactive session? If users have to pass --no-interaction, I am afraid this will not help at all because users will still run into this issue.

@abn
Copy link
Member

abn commented Nov 22, 2024

We do check. For example in https://github.com/python-poetry/poetry/blob/main/src%2Fpoetry%2Fconsole%2Fcommands%2Finit.py (sorry for the bad link on mobile).

@radoering
Copy link
Member Author

The majority of the issues occur when keyring is used in a non-interactive session

Actually, I do not think that this is true. It seems like many users experience this issue when being on a headless system but there are also reports, which mention a GUI. Just some examples:

#8761 (comment)
#8761 (comment)
#8761 (comment)
#8623 (comment)
#8623 (comment)

@abn
Copy link
Member

abn commented Nov 22, 2024

#8761 (comment)

The issue here really is when a scenario occurs where your login did not correctly unlock your system keyring. This is for example - first login using fingerprint, login password was changed without changing your keyring unlock credentials etc. This should really be solved in UX using a pre-flight check and detecting explicitly - a kerying exists and is locked - if interactive ask for password and passthrough.

This and many other of the linked comments also is the case for ssh access (eg: git via ssh) if your ssh key is locked.

#8623 (comment)
#8623 (comment)

Will be resolved if we detect headless.

#8761 (comment)
#8761 (comment)

Will potentially be resolved if we detect headless sessions and we ignore locked keyrings (or treat that as disabled). But I would definitely like to reproduce this somewhere as I have been unsuccessful in doing so till date.

To be clear I am not saying we don't want to do this, but I would like to at least make an informed choice here. Often times a lot of the comments around this are stale given that we have resolved issues through the releases. And keyring implementation is definitely better as well. And we cannot be held responsible for fixing broken distros.

@neersighted pinging you as well since you have had some insights on the topic as well?

PS: I am also locking this conversation to maintainers to avoid bike shedding.

@python-poetry python-poetry locked and limited conversation to collaborators Nov 22, 2024
@radoering
Copy link
Member Author

I can confirm that something like this happened to me some weeks ago at work. It was not poetry init for me but poetry install. I only used sources without authentication in this project and I have never used keyring on that machine and still the dialog to unlock keyring popped up. Since I had no time to investigate the issue and I knew what to do I just aborted, ran poetry config keyring.enabled false and poetry install again. This is only a minor annoyance for me but it is hard for new users to grasp what is going on.

Considering the discussions on Discord, it seems that @Secrus and @finswimmer also lean toward disabling keyring support. (Personally I would even go for #9820, but it seems like this one might be the best compromise.)

@abn I do not expect any groundbreaking improvements in our keyring support before Poetry 2.0. Thus, the question remains if we want to use the major version bump to change the default.

@abn
Copy link
Member

abn commented Dec 4, 2024

Apologies for the delay in responding here. Over the last couple of days, I had a think about the concerns that have been raised on the various issues that have been raised related to the use of keyring.

The main reasons why I think having keyring enabled being the default is for the following reasons.

  1. Does not leave credentials on disk in plain text files.
  2. Allows for easy/seamless integration for Azure/GCP/AWS credential helpers.
  3. It is a rather widely adopted mechanism to enable a better security posture.

One could argue that (1) does not provide any additional security as most disks are encrypted in the professional context due to organisational security postures and if that is assumed the threat vector really is a compromised login credential - which does not prevent access to the keyring backend unless the backend uses a different credential other than the login credential. Following this logic, and if this was the only matter, I would say we disable by default.

Further, it is of concern that users without private sources are impacted. This also supports potentially changing the default, but my opinion is that we must try and address the issues first.

While it's difficult to predict the exact impact, I suspect disabling keyring by default will only swap out one complaining user group with another. I do not know which one will be bigger, or what the impact radius for such a change would be. It might simply increase the noise in our issues.

What I do not want to do, is make a change now hastily (as we are close to a 2.0 release, and we want to get that out) and then have to switch things back later for the above reason. I'd rather us try to improve the status quo with concentrated effort, and make a call in the next major release (which I suspect we want to do more frequently going forward). I am willing to take on the responsibility for this effort to bring if we agree we can wait another major release cycle to switch the behaviour.

I would appreciate insights from @Secrus and @finswimmer and any one else from @python-poetry/core. If rest of the team feels strongly about disabling it now, I will support that decision. Ultimately, I want to make the best decision for our users and the project.

@radoering
Copy link
Member Author

I am wondering if we simply follow what pip's behavior is here.

I took a look at what pip does. An interesting difference seems to be that pip only uses keyring after receiving a 401: https://github.com/pypa/pip/blob/667acf4e5e95783900c0b6f29c7520a69c178b5b/src/pip/_internal/network/auth.py#L452

However, this method also has drawbacks. See pypa/pip#11721 and pypa/pip#12496 (comment) for details.

What I do not want to do, is make a change now hastily (as we are close to a 2.0 release, and we want to get that out) and then have to switch things back later for the above reason.

I would not call it "hasty" since the keyring issues are over a year old. Of course, we will not know what the feedback will be until we change the setting...

I am willing to take on the responsibility for this effort to bring if we agree we can wait another major release cycle to switch the behaviour.

I have the feeling that keyring is something for experienced users and that it is not a big deal for this type of users if keyring support is disabled by default and they have to turn it on. However, I do not care that much about this topic to postpone 2.0 further so I am fine with changing the default as well as dropping it for 2.0.

@radoering radoering force-pushed the keyring-disabled-per-default branch from a09fed1 to 6bad3d1 Compare December 14, 2024 09:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants