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

Use keyring to store/retrieve password if it is available #83

Merged
merged 4 commits into from Nov 11, 2013

Conversation

sebastinas
Copy link
Contributor

keyring allows to access keyrings like gnome-keyring, kwallet, some OS X key chain, etc. and, if they should not be available, provides an encrypted keyring itself. The pull request implements the following:

If keyring is available, check if a password is stored and use it. Should the stored password be wrong, the password is deleted from the keyring and user is asked to enter the password. If no password is stored in the keyring, the user is asked to enter it and then it will be stored in the keyring for the next time.

Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
@JonnyJD
Copy link
Owner

JonnyJD commented Oct 29, 2013

I personally don't use keyrings, since that often makes you dependent on it (you don't actually remember your passwords). I have many friends telling me they can only login from their machine and even some getting new accounts for everything when they break or lose that machine..

Anyways, nothing I should decide for others. A lot better than putting a plaintext password as a parameter.
I'll test a bit and then merge.

keyring 2.0 is quite new (and not in debian stable), but in general it looks like it works with many keyring providers, which is cool.

What happens with keyring < 2.0?

Somewhat related:
I will release isrcsubmit 2.0 soon,
but there will probably a release candidate, since there have been some changes since the last beta that should get more testing. (tests added, this keyring thing, documentation added/changed)

@sebastinas
Copy link
Contributor Author

The requirement for >= 2.0 comes from delete_password. delete_password got introduced in 1.1 (I think), but only 2.0 and later guarantee that delete_password is really implemented in the backends. It should be possible to work around this by checking if keyring.delete_password exists and handling the exception if a backend doesn't implement delete_keyring. I'll come up with something.

@Freso
Copy link
Collaborator

Freso commented Oct 29, 2013

FWIW, as long as the keyring is optional, I like it.

Instead of removing passwords that failed to authenticate from the keyring, just
ignore passwords from the keyring until the user successfully authenticated and
the password has been overridden.

This change makes the keyring support usable with any version of keyring.

Signed-off-by: Sebastian Ramacher <sebastian+dev@ramacher.at>
@JonnyJD
Copy link
Owner

JonnyJD commented Nov 1, 2013

I tested it now and it does work very good (testing with the EncryptedFile backend on my system, using PyCrypto).

What I am missing is the possibility to disable using the keyring for isrcsubmit.
See also: https://bitbucket.org/kang/python-keyring-lib/issue/125

There are multiple possible workarounds:

  • Not installing python-keyring if you don't want to use it (no problem for me atm, other that I maintain the python3 version of keyring in the AUR now)
  • asking the user every time if the keyring should be used (quite a big fuzz just for weird guys like me)
  • having command line parameter to suppress using the keyring (not sure if anybody would actually use that, since you either have to type it all the time or add another layer of indirection)
  • having a config file just for isrcsubmit
  • and of course: actually using the keyring, but deleting the keyring with cron to not forget your password :-P

Not installing python-keyring is probably the best. I just want to find out if there is an even better option.

@sebastinas
Copy link
Contributor Author

I would implement a combination of command line option for it together with a config file for isrcsubmit. The config file could be used to hold other command line options too (say the username, server, etc.). If you think something like that would be a good idea (and it is not much code with ConfigParser and optparse) I could implement it, but would do that in a separate pull request.

JonnyJD added a commit that referenced this pull request Nov 11, 2013
see pull request #83
@JonnyJD JonnyJD merged commit 2c84c86 into JonnyJD:master Nov 11, 2013
@JonnyJD
Copy link
Owner

JonnyJD commented Nov 11, 2013

Yes. I merged this first so others can use it in the git version.

Having a command line option and a config file wouldn't hurt.
If you have an easy way to fix something like that up, please do. I don't have experience with that.
I hope it doesn't get too complex (where to look for config files, overriding/changing/saving?)

@JonnyJD JonnyJD mentioned this pull request Dec 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants