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

Warning when PlaintextKeyring backend is used #25

Closed
Tethik opened this issue Feb 13, 2018 · 8 comments
Closed

Warning when PlaintextKeyring backend is used #25

Tethik opened this issue Feb 13, 2018 · 8 comments

Comments

@Tethik
Copy link

Tethik commented Feb 13, 2018

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.

@frispete
Copy link
Contributor

frispete commented Feb 14, 2018

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.

@jaraco
Copy link
Owner

jaraco commented Feb 14, 2018

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.

@jaraco jaraco closed this as completed in 4c1a907 Feb 14, 2018
@Tethik
Copy link
Author

Tethik commented Feb 14, 2018

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 keyring. Then I read in the README about this repo. Because I'm running Gnome I looked for and found the Gnome keyring in this keyring.alt repo. So I installed it. The parent README does not say that the contents of this repo are unsecure only that they are "alternate".

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.

@jaraco
Copy link
Owner

jaraco commented Feb 14, 2018

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.

jaraco added a commit to jaraco/keyring that referenced this issue Feb 14, 2018
@Tethik
Copy link
Author

Tethik commented Feb 15, 2018

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.

@frispete
Copy link
Contributor

Okay, let's clarify the purpose of the Secret Service backend in a prominent place.

@Tethik How about this. Would this phrasing prevented you to look any further?

@Tethik
Copy link
Author

Tethik commented Feb 15, 2018

It helps, sure :)

@jaraco
Copy link
Owner

jaraco commented Feb 15, 2018

You could make it so that all modules inside this "alt" package are explicitly opt-in only.

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.

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?

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.

clrpackages pushed a commit to clearlinux-pkgs/keyring that referenced this issue Apr 6, 2018
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants