Merge lp:~macieksitarz/calibre/keyring-support into lp:calibre

[Maciej Sitarz]

This branch introduces python keyring support. Keyring's are used as a common secure password storage.

python-keyring:
http://pypi.python.org/pypi/keyring
https://bitbucket.org/kang/python-keyring-lib

It has wide platform support:

The keyring services supported by the Python keyring lib:

    OSXKeychain: supports the Keychain service in Mac OS X.
    KDEKWallet: supports the KDE's Kwallet service.
    GnomeKeyring: for Gnome 2 environment.
    SecretServiceKeyring: for newer GNOME and KDE environments.

Besides these native password storing services provided by operating systems. Python keyring lib also provides following build-in keyrings.

    Win32CryptoKeyring: for Windows 2k+.
    CryptedFileKeyring: a command line interface keyring base on PyCrypto.
    UncryptedFileKeyring: a keyring which leaves passwords directly in file.

[Maciej Sitarz (macieksitarz)]

Instead of using hexlified password stored in file keyring would be used.
It improves security and uses common keyrings depending on the platform and/or window manager.

[Maciej Sitarz (macieksitarz)]

One more thing about testing done.

I tested this on Linux using KWallet(KDE's keyring), but it should work properly on other OSes and window managers, as python-keyring-lib handles that internaly.

[Kovid Goyal (kovid)]

Some problems I see:

1) The patch must not break existing setups. Email cannot stop working on a calibre update.

2) The various keyrings depend on environment variables, X sessions, and the like. Those environemnt variables may not be present in all contexts calibre-smtp runs, for example, in cron jobs. That makes calibre-smtp significantly less robust.

3) I dont see how a keyring is fundamentally more or less secure than plaintext password storage. Once an attacher has read access to a user's data, decrypting password stores is possible by brute force attacks. Reversible password storage can simply *never* be secure. However, I do agree it makes an attackers' life a little less convenient.

4) Someone has to test this witht he various keyring implementations. That someone is not going to be me :)

Given all those problems, I am not willing to accept this patch. I will consider a patch that uses an optional keyring based password storage, for people that want to use it. But the default method will remain plaintext storage.

review: Disapprove

[Maciej Sitarz (macieksitarz)]

> 1) The patch must not break existing setups. Email cannot stop working on a
> calibre update.

If compatibility is necessary migration code could be added.

Algorithm like:
if relay_password exists in config:
    unhexlify password
    save password to keyring
    remove relay_password from config

> 2) The various keyrings depend on environment variables, X sessions, and the
> like. Those environemnt variables may not be present in all contexts calibre-
> smtp runs, for example, in cron jobs. That makes calibre-smtp significantly
> less robust.

Rigt now calibre-smtp is not using password from config file. Is there such functionality?
I'm asking because I also have a patch adding such functionality.

After applying the patch calibre-smtp will still have the -p/--password option, so one can use it in cron (after specifying plain text password).

> 3) I dont see how a keyring is fundamentally more or less secure than
> plaintext password storage. Once an attacher has read access to a user's data,
> decrypting password stores is possible by brute force attacks. Reversible
> password storage can simply *never* be secure. However, I do agree it makes an
> attackers' life a little less convenient.

Using strong password for the keyring will increase security. But you're right it won't be 100% secure.

> 4) Someone has to test this witht he various keyring implementations. That
> someone is not going to be me :)

Let's wait for volunteers...

> Given all those problems, I am not willing to accept this patch. I will
> consider a patch that uses an optional keyring based password storage, for
> people that want to use it. But the default method will remain plaintext
> storage.

The setting should be boolean "Use keyring for password storage" and visible in the SMTP settings window?

[Kovid Goyal (kovid)]

Yes, you would need to add a boolean option (checkbox) that says "Store password securely in keyring" to the GUI (opts.use_keyring) and similarly have a --use-keyring option for calibre-smtp. If opts.use_keyring is True, the code will try to get the password from the keyring, otherwise it will use opts.password.

Unmerged revisions

14073. By Maciej Sitarz on 2013-01-11

Merge from main branch

14072. By Maciej Sitarz on 2013-01-11

Add password keyring support

=== modified file 'src/calibre/gui2/email.py'
--- src/calibre/gui2/email.py	2012-12-20 04:18:34 +0000
+++ src/calibre/gui2/email.py	2013-01-11 13:32:25 +0000
@@ -7,7 +7,7 @@
 __docformat__ = 'restructuredtext en'
 
 import os, socket, time
-from binascii import unhexlify
+import keyring
 from functools import partial
 from threading import Thread
 from itertools import repeat
@@ -109,7 +109,8 @@
                         verbose=1,
                         relay=opts.relay_host,
                         username=opts.relay_username,
-                        password=unhexlify(opts.relay_password), port=opts.relay_port,
+                        password=keyring.get_password('Calibre',opts.relay_username),
+                        port=opts.relay_port,
                         encryption=opts.encryption,
                         debug_output=log.debug)
         finally:
 
=== modified file 'src/calibre/gui2/wizard/send_email.py'
--- src/calibre/gui2/wizard/send_email.py	2011-07-10 19:09:11 +0000
+++ src/calibre/gui2/wizard/send_email.py	2013-01-11 13:32:25 +0000
@@ -7,14 +7,13 @@
 __docformat__ = 'restructuredtext en'
 
 import cStringIO, sys
-from binascii import hexlify, unhexlify
 from functools import partial
 
 from PyQt4.Qt import QWidget, pyqtSignal, QDialog, Qt, QLabel, \
         QLineEdit, QDialogButtonBox, QGridLayout, QCheckBox
 
 from calibre.gui2.wizard.send_email_ui import Ui_Form
-from calibre.utils.smtp import config as smtp_prefs
+from calibre.utils.smtp import config as smtp_prefs, get_password_from_wallet
 from calibre.gui2.dialogs.test_email_ui import Ui_Dialog as TE_Dialog
 from calibre.gui2 import error_dialog, question_dialog
 
@@ -32,7 +31,7 @@
             self.to.setText(pa)
         if opts.relay_host:
             self.label.setText(_('Using: %(un)s:%(pw)s@%(host)s:%(port)s and %(enc)s encryption')%
-                    dict(un=opts.relay_username, pw=unhexlify(opts.relay_password),
+                    dict(un=opts.relay_username, pw=get_password_from_wallet(opts.relay_username),
                         host=opts.relay_host, port=opts.relay_port, enc=opts.encryption))
 
     def test(self, *args):
@@ -125,11 +124,14 @@
         self.relay_host.textChanged.connect(self.changed)
         self.relay_port.setValue(opts.relay_port)
         self.relay_port.valueChanged.connect(self.changed)
+        self.relay_username_old = None
         if opts.relay_username:
+            self.relay_username_old = opts.relay_username
             self.relay_username.setText(opts.relay_username)
+            relay_password = get_password_from_wallet(opts.relay_username)
+            if None != relay_password:
+                self.relay_password.setText(relay_password)
         self.relay_username.textChanged.connect(self.changed)
-        if opts.relay_password:
-            self.relay_password.setText(unhexlify(opts.relay_password))
         self.relay_password.textChanged.connect(self.changed)
         getattr(self, 'relay_'+opts.encryption.lower()).setChecked(True)
         self.relay_tls.toggled.connect(self.changed)
@@ -151,7 +153,7 @@
         to_set = pa is not None
         if self.set_email_settings(to_set):
             opts = smtp_prefs().parse()
-            if not opts.relay_password or question_dialog(self, _('OK to proceed?'),
+            if not opts.relay_username or question_dialog(self, _('OK to proceed?'),
                     _('This will display your email password on the screen'
                     '. Is it OK to proceed?'), show_copy_button=False):
                 TestEmail(pa, self).exec_()
@@ -169,7 +171,7 @@
             sendmail(msg, from_=opts.from_, to=[to],
                 verbose=3, timeout=30, relay=opts.relay_host,
                 username=opts.relay_username,
-                password=unhexlify(opts.relay_password),
+                password=get_password_from_wallet(opts.relay_username),
                 encryption=opts.encryption, port=opts.relay_port)
         except:
             import traceback
@@ -248,8 +250,20 @@
         conf.set('relay_host', host if host else None)
         conf.set('relay_port', self.relay_port.value())
         conf.set('relay_username', username if username else None)
-        conf.set('relay_password', hexlify(password))
         conf.set('encryption', enc_method)
+
+        if not self.relay_username_old is None:
+            if username != self.relay_username_old:
+                import keyring
+                try:
+                    keyring.delete_password('Calibre', self.relay_username_old)
+                except: pass
+        if username:
+            import keyring
+            keyring.set_password('Calibre', username, password)
+
+        self.relay_username_old = username
+
         return True
 
 
 
=== modified file 'src/calibre/utils/smtp.py'
--- src/calibre/utils/smtp.py	2013-01-11 09:35:10 +0000
+++ src/calibre/utils/smtp.py	2013-01-11 13:32:25 +0000
@@ -179,6 +179,12 @@
     from email.utils import parseaddr
     return parseaddr(raw)[-1]
 
+def get_password_from_wallet(username):
+    import keyring
+    if username:
+        return keyring.get_password('Calibre',username)
+    return None
+
 def compose_mail(from_, to, text, subject=None, attachment=None,
         attachment_name=None):
     attachment_type = attachment_data = None
@@ -265,7 +271,6 @@
     c.add_opt('relay_host')
     c.add_opt('relay_port', default=25)
     c.add_opt('relay_username')
-    c.add_opt('relay_password')
     c.add_opt('encryption', default='TLS', choices=['TLS', 'SSL'])
     return c