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 and only install via an extra #9820

Draft
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

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

disable keyring per default

I think the majority of Poetry users do not use/need keyring. Therefore, changing the default of keyring.enabled from true to false avoids issues like #8623 and #8761. In case, someone stores credentials with keyring disabled/unavailable, we have already warned that credentials will be stored in plain text. I extended this warning so it tells the user that they should consider using keyring.

keyring as an extra

Further, I moved keyring to an extra because it is responsible for one third of our dependencies on Linux (and still a quarter on Windows)! Without keyring, there are 13 dependencies less on Linux and 9 dependencies less on Windows.

Why care about the number of dependencies?

The installation of Poetry is about 1-2 seconds faster and about 18 MB smaller without keyring. Maybe not much, but especially installation speed might still be relevant (at least nice) in CI pipelines.

And the other side of the coin?

We have to make sure that Poetry still works (and keeps working) without keyring. There must not be an accidental use of keyring anywhere in the code!

I have taken a few measures that should ensure that no accidental use of keyring slips in in the future:

  • disallow global keyring imports in src via TID253
  • restrict keyring imports to one module (password_manager.py) via TID251
  • extended test_password_manager.py::test_disabled_keyring_never_called to test_password_manager.py::test_disabled_or_unavailable_keyring_never_called

@radoering radoering added the impact/docs Contains or requires documentation changes label Nov 1, 2024
Copy link

github-actions bot commented Nov 1, 2024

Deploy preview for website ready!

✅ Preview
https://website-he51zpgfm-python-poetry.vercel.app

Built with commit 16a6c18.
This pull request is being automatically deployed with vercel-action

@abn
Copy link
Member

abn commented Nov 15, 2024

@radoering I am a bit conflicted here. On the one hand I can understand the odd issues we have had regarding keyring. On the other hand however, I am also aware that one of the original reasons we implemented keyring as default was to encourage good credential hygiene by avoiding storing plaintext credentials into config files where possible. It is not a silver bullet, but a better security posture. The question I guess, is if security considerations are getting in the way of ux without much value gained.

The other rationales, like the dependency numbers and sizes etc, I do not think really applies here. And I reckon the config flag disabling keyring by default is likely a better solution.

And further majority of keyring issues I have seen has been due to the rather badly setup debian environments or containers.

@radoering
Copy link
Member Author

I am a bit conflicted here.

I can fully understand. It took me a while to get to the point where I thought it is better to deactivate it per default.

The question I guess, is if security considerations are getting in the way of ux without much value gained.

I think we have reached this point because users run into keyring issues even when installing from sources without any authentication. Especially less experienced / new users are not able to identify and work around this issue quickly.

The other rationales, like the dependency numbers and sizes etc, I do not think really applies here.

Less dependencies -> less issues. 😉

I still like the extra but this is not a hill I want to die on.

@Secrus If I remember correctly, you are in the "reduce number of dependencies" team. Do you have a strong opinion on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
2 participants