-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Warning when PlaintextKeyring backend is used #25
Comments
Thank you for using keyring, Tethik. Don't you think, that the name of the class is warning enough? Since PlaintextKeyring has such a low priority, and is flagged as "no encryption" as well, I find it hard to believe that using this module by accident is possible, don't you? Please note, that even using the encrypting part of this module isn't sufficiently secure from a security sensitive point of view. Given the sheer hashing power¹, that is available today, pbkdf2 isn't up to the task, if there's a slight chance, that the encoded file is falling into the wrong hands... Where should we draw the line? On the other hand, there might be good applications for a plain password storage as well, where such log messages would be undesirable and disruptive. ¹) Probably looking for new opportunities soon, if the world turns away from bitcoin. |
I do think it's worth at least a mention in the readme that the backends in this module are generally discouraged for production use. |
Actually I never imported the class specifically, so the class name "PlaintextKeyring" never appeared to me as I implemented my script using this module. Only once I wanted to check where it actually stored the data did I discover that it was plaintext. Basically what I did was install the parent repo I think there is something to be said still for keeping secure defaults and giving user feedback here. If I install and use a keyring package as a developer I do expect it to not fall back to plaintext. I install it specifically because I look for a better place to store tokens and secrets that is not just a config file in the home directory. For my use case then I'd strongly prefer it if this package error-ed instead. Then I would be alerted to fix the potential security issue. Another idea could be to have a default whitelist of modules. Forcing implementing developers opt-in when they want to use the less secure modules. |
There is an admittedly buried feature whereby a user of keyring can force imperatively to use only recommended keyrings. At one point, we discussed forcing users to opt-in to the insecure keyrings, and the creation of this library was the mechanism we chose. Perhaps it's time the keyring project itself put more warnings around its mention of keyrings.alt also. |
…l security concerns. Ref jaraco/keyrings.alt#25
The issue linked for that buried feature is exactly the same concern that I have here. Basically the same discussion too. Interesting. Thanks for the updated documentation, I think that is a step in the right direction. You could make it so that all modules inside this "alt" package are explicitly opt-in only, while keeping the parent package with the default modules opt-out. If I get the time I may dig into this package deeper. Because I really like the idea. Has there been any work on threat modeling this library? Specifically what threats do we try to protect against and which are out of scope? As someone who likes to sometimes pwn things one of the first ideas that comes to mind is e.g. the priority system and trying to manipulate it by making whatever backend it uses unavailable. But that assumes that the attacker might have some sort of execution or read locking capability (e.g. as another user on the system or as a malicious script) which may be out of scope. |
It helps, sure :) |
My intention, by making it a separate library that must be explicitly installed, the modules are effectively explicitly opt-in only. There is also the 'priority' designation, which while arbitrary, does provide a signal about how recommended a given backend is.
Basically, no. The primary intention is to provide an interface to the best implementations of secure password storage across platforms, but that led to a proliferation of non-secure implementations as well. We welcome further analysis and suggestions. |
…ds, using entry points. Deprecate reliance on loading without entry points (with a warning presented). Ref #310. Dan Sully (1): Use 'entrypoints' instead of pkg_resources to avoid import time overhead. Hans-Peter Jansen (1): More specific DE hints Jakub Wilk (1): Fix typo in fail.Keyring method name Jason R. Coombs (26): Disable pytest-sugar until Teemu/pytest-sugar#133 is addressed. Bring back pytest-sugar with a minimum version to support Pytest 3.4. Add link to keyrings.cryptfile. Push keyrings.alt to bottom and signal security concerns. Ref jaraco/keyrings.alt#25 Link to the third party backends rather than mentioning only keyrings.alt. It's macOS now Merge pull request #305 from jwilk-forks/spelling Save the pip cache across builds. Ref pypa/setuptools#1279. Add workaround for build failures on Python 3.7 (yaml/pyyaml#126). Run flake8 with tests. Add flake8 config to ignore common exclusions. Add comments to testing and docs extras to aid with merges. Add appveyor badge (commented). Disable RTD by default. Merge https://github.com/jaraco/skeleton Feed the hobgoblins (delint). Homebrew now installs Python 3 with pip by default for 'python' Now load all backends, including the native backends, using entry points. Deprecate reliance on loading without entry points (with a warning presented). Ref #310. Merge pull request #312 from dsully/master Update changelog. Ref #312. Try running ensurepip to see if macos tests will run Prefer 'macos' to 'osx' Merge branch 'master' into feature/310-all-backends-by-entry-points Try linking Python Add Python's bins to the path pyinstaller hook may now rely entirely on the entry points Ensure 'keyring.backends' is used consistently. On second thought, use a major version bump for clarity. Merge branch 'feature/310-all-backends-by-entry-points' Update changelog. Ref #314. 12.0.1 ------ * #314: No changes except to rebuild. 12.0.0 ------ * #310: Keyring now loads all backends through entry points. For most users, this release will be fully compatible. Some users may experience compatibility issues if entrypoints is not installed (as declared) or the metadata on which entrypoints relies is unavailable. For that reason, the package is released (NEWS truncated at 15 lines)
Users may install this library expecting security by default, but then actually having their secrets stored in plaintext.
I think it would be good to at least have some sort of warning (e.g. via logging) if PlaintextKeyring is used.
The text was updated successfully, but these errors were encountered: