Merge lp:~macieksitarz/calibre/optional-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.

[Maciej Sitarz (macieksitarz)]

Resubmitted merge proposal with new branch.

I added the GUI option to use Keyring and also command line option in calibre-smtp.
Values from config file should be moved to keyring and removed from config when necessary.
Also from keyring to config and removed from keyring.

Additionally I fixed one minor bug with TLS/SSL/None. Changing radio buttons from None to SSL did not enable the "Save" option. The SSL button was not monitored for changes.

[Kovid Goyal (kovid)]

I'll review this in a bit, I have a local maxima in my workload at the moment.

[Kovid Goyal (kovid)]

I spent half a day looking into the keyring library and frankly, I am not impressed.

A) Windows

1) keyring does not work on my system. It uses environment variables to try to get the location of standard directories. This is broken. It needs to use the win32 API. And a library that reads config files from random places on the users system is not suitable for embedding. It would be trivial for an attacker to write a config file that would cause keyring to use a plain text file storage backend.

2) Examining the source code, it appears to use the ReadCred/WriteCred API to store secrets. This API means that *any* program that the user launches can trivially and without permission read any secrets that calibre stores. This is in no way better than plaintext storage.

B) Linux

1) It uses third party modules (PyKDE/gnomekeyring) to interface with the two major keyrings. These modules will not be available in a binary calibre install

2) The freedesktop.org based keyring backend (SecretService) does not work. Apparently the standard it is based on was never finalized and at least on my system the default collection alias does not exist, which causes the interface to not work. If its broken on my system, it's going to be broken on other systems. In addition, the SecretService API can require the user to respond to a prompt when storing a secret. The keyring backend does not support this.

3) The KDE Wallet based backend asks for permission by pretending ot be the KDE desktop, which is misleading to say the least. Not to mention that it relies on PyKDE instead of using the portable dbus interface.

C) OS X

Given my previous experiences, I did not even bother with this one.

Given this sad state of affairs, I have to decline this merge request. If you are worried about the safety of your email account, I suggest you create a dummy gmail/hotmail/whatever account and use it only for calibre. Much easier and *much* more secure.

[Kovid Goyal (kovid)]

Rejecting.

review: Disapprove

Unmerged revisions

14092. By Maciej Sitarz on 2013-01-13

Merge with trunk

14091. By Maciej Sitarz on 2013-01-13

Monitor both SSL and TLS radio buttons

Enables "Save" after changing from "None" to "SSL"

14090. By Maciej Sitarz on 2013-01-13

Keyring support for password storage

=== 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-13 18:50:27 +0000
@@ -7,13 +7,13 @@
 __docformat__ = 'restructuredtext en'
 
 import os, socket, time
-from binascii import unhexlify
 from functools import partial
 from threading import Thread
 from itertools import repeat
 
 from calibre.utils.smtp import (compose_mail, sendmail, extract_email_address,
         config as email_config)
+from calibre.utils.smtp import get_password
 from calibre.utils.filenames import ascii_filename
 from calibre.customize.ui import available_input_formats, available_output_formats
 from calibre.ebooks.metadata import authors_to_string
@@ -109,7 +109,7 @@
                         verbose=1,
                         relay=opts.relay_host,
                         username=opts.relay_username,
-                        password=unhexlify(opts.relay_password), port=opts.relay_port,
+                        password=get_password(opts, opts.use_keyring), 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-13 18:50:27 +0000
@@ -7,7 +7,6 @@
 __docformat__ = 'restructuredtext en'
 
 import cStringIO, sys
-from binascii import hexlify, unhexlify
 from functools import partial
 
 from PyQt4.Qt import QWidget, pyqtSignal, QDialog, Qt, QLabel, \
@@ -15,6 +14,8 @@
 
 from calibre.gui2.wizard.send_email_ui import Ui_Form
 from calibre.utils.smtp import config as smtp_prefs
+from calibre.utils.smtp import get_password
+from calibre.utils.smtp import delete_password_in_keyring
 from calibre.gui2.dialogs.test_email_ui import Ui_Dialog as TE_Dialog
 from calibre.gui2 import error_dialog, question_dialog
 
@@ -32,7 +33,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(opts, opts.use_keyring),
                         host=opts.relay_host, port=opts.relay_port, enc=opts.encryption))
 
     def test(self, *args):
@@ -127,11 +128,15 @@
         self.relay_port.valueChanged.connect(self.changed)
         if opts.relay_username:
             self.relay_username.setText(opts.relay_username)
+            relay_password = get_password(opts, opts.use_keyring)
+            if relay_password is not None:
+                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)
+        self.use_keyring.setCheckState(Qt.Checked if opts.use_keyring else Qt.Unchecked)
+        self.use_keyring.stateChanged.connect(self.changed)
         getattr(self, 'relay_'+opts.encryption.lower()).setChecked(True)
+        self.relay_ssl.toggled.connect(self.changed)
         self.relay_tls.toggled.connect(self.changed)
 
         for x in ('gmail', 'hotmail'):
@@ -151,7 +156,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 +174,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(opts, opts.use_keyring),
                 encryption=opts.encryption, port=opts.relay_port)
         except:
             import traceback
@@ -224,6 +229,7 @@
         username = unicode(self.relay_username.text()).strip()
         password = unicode(self.relay_password.text()).strip()
         host = unicode(self.relay_host.text()).strip()
+        use_keyring = True if self.use_keyring.checkState() else False
         enc_method = ('TLS' if self.relay_tls.isChecked() else 'SSL'
                 if self.relay_ssl.isChecked() else 'NONE')
         if host:
@@ -244,13 +250,29 @@
                     ' mailservers need a username and password. Are you sure?')):
                     return False
         conf = smtp_prefs()
+        conf_prev = smtp_prefs().parse()
         conf.set('from_', from_)
         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('use_keyring', use_keyring)
         conf.set('encryption', enc_method)
+
+        if use_keyring:
+            if not conf_prev.relay_username is None:
+                if username != conf_prev.relay_username:
+                    delete_password_in_keyring(conf_prev.relay_username)
+            if username:
+                import keyring
+                keyring.set_password('Calibre', username, password)
+        else:
+            from binascii import hexlify
+            conf.set('relay_password', hexlify(password) if password else None)
+
+        if use_keyring != conf_prev.use_keyring:
+            if use_keyring:
+                conf.set('relay_password', None)
+            if conf_prev.use_keyring:
+                delete_password_in_keyring(conf_prev.relay_username)
+
         return True
-
-
-

=== modified file 'src/calibre/gui2/wizard/send_email.ui'
--- src/calibre/gui2/wizard/send_email.ui	2010-11-25 16:31:29 +0000
+++ src/calibre/gui2/wizard/send_email.ui	2013-01-13 18:50:27 +0000
@@ -44,16 +44,6 @@
       <string>Mail &amp;Server</string>
      </property>
      <layout class="QGridLayout" name="gridLayout_3">
-      <item row="0" column="0" colspan="4">
-       <widget class="QLabel" name="label_16">
-        <property name="text">
-         <string>calibre can &lt;b&gt;optionally&lt;/b&gt; use a server to send mail</string>
-        </property>
-        <property name="wordWrap">
-         <bool>true</bool>
-        </property>
-       </widget>
-      </item>
       <item row="1" column="0">
        <widget class="QLabel" name="label_17">
         <property name="text">
@@ -145,7 +135,7 @@
         </property>
        </widget>
       </item>
-      <item row="4" column="0">
+      <item row="5" column="0">
        <widget class="QLabel" name="label_21">
         <property name="text">
          <string>&amp;Encryption:</string>
@@ -168,7 +158,7 @@
         </property>
        </widget>
       </item>
-      <item row="4" column="2">
+      <item row="5" column="2">
        <widget class="QRadioButton" name="relay_ssl">
         <property name="toolTip">
          <string>Use SSL encryption when connecting to the mail server.</string>
@@ -178,6 +168,16 @@
         </property>
        </widget>
       </item>
+      <item row="5" column="3">
+       <widget class="QRadioButton" name="relay_none">
+        <property name="toolTip">
+         <string>WARNING: Using no encryption is highly insecure</string>
+        </property>
+        <property name="text">
+         <string>&amp;None</string>
+        </property>
+       </widget>
+      </item>
       <item row="3" column="4">
        <spacer name="horizontalSpacer_3">
         <property name="orientation">
@@ -191,13 +191,20 @@
         </property>
        </spacer>
       </item>
-      <item row="4" column="3">
-       <widget class="QRadioButton" name="relay_none">
-        <property name="toolTip">
-         <string>WARNING: Using no encryption is highly insecure</string>
-        </property>
-        <property name="text">
-         <string>&amp;None</string>
+      <item row="0" column="0" colspan="5">
+       <widget class="QLabel" name="label_16">
+        <property name="text">
+         <string>calibre can &lt;b&gt;optionally&lt;/b&gt; use a server to send mail</string>
+        </property>
+        <property name="wordWrap">
+         <bool>true</bool>
+        </property>
+       </widget>
+      </item>
+      <item row="4" column="0" colspan="5">
+       <widget class="QCheckBox" name="use_keyring">
+        <property name="text">
+         <string>Store password securely in &amp;keyring</string>
         </property>
        </widget>
       </item>

=== 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-13 18:50:27 +0000
@@ -160,6 +160,8 @@
       'encryption method is SSL and 25 otherwise.')
     r('-u', '--username', help='Username for relay')
     r('-p', '--password', help='Password for relay')
+    r('-k', '--use-keyring', help='Use keyring to get password',
+            action='store_true', default=False)
     r('-e', '--encryption-method', default='TLS',
       choices=['TLS', 'SSL', 'NONE'],
       help='Encryption method to use when connecting to relay. Choices are '
@@ -179,6 +181,27 @@
     from email.utils import parseaddr
     return parseaddr(raw)[-1]
 
+
+def get_password(opts=None, use_keyring=False):
+    if use_keyring:
+        import keyring
+        if opts.relay_username:
+            return keyring.get_password('Calibre', opts.relay_username)
+        return None
+    else:
+        if opts:
+            from binascii import unhexlify
+            return unhexlify(opts.relay_password)
+        return None
+
+
+def delete_password_in_keyring(username):
+    import keyring
+    try:
+        keyring.delete_password('Calibre', username)
+    except:
+        pass
+
 def compose_mail(from_, to, text, subject=None, attachment=None,
         attachment_name=None):
     attachment_type = attachment_data = None
@@ -241,10 +264,17 @@
         if os.fork() != 0:
             return 0
 
+    if opts.use_keyring:
+        from calibre.utils.smtp import config as smtp_prefs
+        smtp_opts = smtp_prefs().parse()
+        password = get_password(smtp_opts, True)
+    else:
+        password = opts.password
+
     try:
         sendmail(msg, efrom, eto, localhost=opts.localhost, verbose=opts.verbose,
              timeout=opts.timeout, relay=opts.relay, username=opts.username,
-             password=opts.password, port=opts.port,
+             password=password, port=opts.port,
              encryption=opts.encryption_method)
     except:
         if outbox is not None:
@@ -265,6 +295,7 @@
     c.add_opt('relay_host')
     c.add_opt('relay_port', default=25)
     c.add_opt('relay_username')
+    c.add_opt('use_keyring', default=False, action='store_true')
     c.add_opt('relay_password')
     c.add_opt('encryption', default='TLS', choices=['TLS', 'SSL'])
     return c