Merge lp:~benji/launchpadlib/keyring into lp:launchpadlib

[Benji York]

Make launchpadlib attempt to store/load authentication credentials
in/from the Gnome keyring.

[lp:~benji/launchpadlib/keyring]

98. By Benji York <benji@benji-laptop> on 2010-09-22

use the more customary cStringIO instead of StringIO

99. By Benji York <benji@benji-laptop> on 2010-09-22

reword docstring

100. By Benji York <benji@benji-laptop> on 2010-09-22

add comment about replacing the socket module

[Leonard Richardson (leonardr)]

Looks good. I like the fake keyring. A few suggestions that I mentioned on IRC:

Add a context manager for setting and resetting the fake keyring.

See if you can move the keyring code into credentials.py.

Rework the docstring about the hierarchy of "places to look for/store credentials": we check the keyring first, then a provided file, then the default launchpadlib location.

And actually, it might make more sense to check the provided file *before* checking the keyring. (Though if the key isn't found, I think it still makes sense to write it to the keyring rather than the provided file.)

Also make sure to mention the change in the NEWS.

[lp:~benji/launchpadlib/keyring]

101. By Benji York <benji@benji-laptop> on 2010-09-22

use a context manager to encapsulate a reoccurring pattern

[Leonard Richardson (leonardr)]

review: Approve

[lp:~benji/launchpadlib/keyring]

102. By Benji York <benji@benji-laptop> on 2010-09-22

a couple of small tweaks

103. By Benji York <benji@benji-laptop> on 2010-09-22

add NEWS

[Robert Collins (lifeless)]

This is spectacularly risky unless you keep per-application access
restrictions. I haven't checked the code to see if thats what you're
doing.

It's-not-paranoia-if-they-are-out-to-get-you'ly-yrs
Rob

[Benji York (benji)]

On Thu, Sep 23, 2010 at 5:25 AM, Robert Collins
<email address hidden> wrote:
> This is spectacularly risky unless you keep per-application access
> restrictions.

Can you unpack that a bit? I don't follow.
--
Benji York

[Robert Collins (lifeless)]

See the thread about this overall conceptual change on the mailling list.

[Leonard Richardson (leonardr)]

I agree that you should wait to land this until the overall change is resolved, just to avoid confusion. But there are, currently, no per-application access restrictions in launchpadlib. It just looks like there are. Tokens are stored in standard locations in files on disk, and any running application can look for a token that meets its needs and use it. This was something we planned to *fix* by moving the tokens into the GNOME keyring, but it turns out the GNOME keyring is no different in this regard.

IMO this change is a net gain for security, even if we don't make the larger change to the way tokens work in a desktop context. Storing OAuth tokens on disk means that if someone steals your hard drive, they can see your tokens, unless you encrypted your home directory. Storing tokens in a keyring ensures that they are always stored encrypted on disk.

[Robert Collins (lifeless)]

On Fri, Sep 24, 2010 at 9:09 AM, Leonard Richardson
<email address hidden> wrote:
> I agree that you should wait to land this until the overall change is resolved, just to avoid confusion. But there are, currently, no per-application access restrictions in launchpadlib. It just looks like there are. Tokens are stored in standard locations in files on disk, and any running application can look for a token that meets its needs and use it. This was something we planned to *fix* by moving the tokens into the GNOME keyring, but it turns out the GNOME keyring is no different in this regard.

Applications manually store their keys; they can handoff to a personal
wallet with different characteristics to gnome keyring.

> IMO this change is a net gain for security, even if we don't make the larger change to the way tokens work in a desktop context. Storing OAuth tokens on disk means that if someone steals your hard drive, they can see your tokens, unless you encrypted your home directory. Storing tokens in a keyring ensures that they are always stored encrypted on disk.

In the Ubuntu context we ship with per file encryption available by default.

[Leonard Richardson (leonardr)]

> Applications manually store their keys; they can handoff to a personal
> wallet with different characteristics to gnome keyring.

Applications *may* manually store their keys, but almost every one I've seen uses Launchpad.login_with(), which stores the key on disk.

This branch changes login_with() to store the keys in the GNOME keyring instead of on disk. Applications that don't use login_with() are still free to do whatever they want.

[Leonard Richardson (leonardr)]

I'm un-approving this branch because Sidnei da Silva on launchpad-dev mentioned the keyring package (http://pypi.python.org/pypi/keyring), a general keyring library that abstracts away the difference between the GNOME keyring, the KDE wallet, etc. I was planning to add KDE support in a separate branch, but let's make keyring a dependency of launchpadlib and start out with KDE support.

review: Needs Fixing

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2010-08-23 19:51:55 +0000
+++ src/launchpadlib/NEWS.txt	2010-09-22 21:58:38 +0000
@@ -2,6 +2,11 @@
 NEWS for launchpadlib
 =====================
 
+1.7.0 (2010-09-23)
+==================
+
+- Store authorization tokens in the Gnome keyring, when available.
+
 1.6.5 (2010-08-23)
 ==================
 

=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2010-08-23 19:51:55 +0000
+++ src/launchpadlib/launchpad.py	2010-09-22 21:58:38 +0000
@@ -22,7 +22,9 @@
     ]
 
 import os
+import socket
 import stat
+import cStringIO
 import urlparse
 
 from lazr.uri import URI
@@ -33,6 +35,13 @@
     AuthorizeRequestTokenWithBrowser)
 from launchpadlib import uris
 
+# The Gnome keyring module (part of gtk) is optional
+try:
+    import gnomekeyring
+except ImportError:
+    gnomekeyring = None
+
+
 # Import some constants for backwards compatibility. This way, old
 # scripts that have 'from launchpad import EDGE_SERVICE_ROOT' will still
 # work.
@@ -96,6 +105,85 @@
     collection_of = 'distribution'
 
 
+def get_keyring_attr(consumer_name):
+    token_name = '%s (launchpadlib on %s)' % (
+        consumer_name, socket.gethostname())
+    return {
+        'key-type': 'launchpadlib credentials',
+        'token-name': token_name,
+        }
+
+
+def query_keyring_for_credentials(consumer_name):
+    """Load the credentials from the the Gnome keyring, if available."""
+    if gnomekeyring is None:
+        return None
+    if not gnomekeyring.is_available():
+        return None
+
+    try:
+        items = gnomekeyring.find_items_sync(
+            gnomekeyring.ITEM_GENERIC_SECRET, get_keyring_attr(consumer_name))
+    except gnomekeyring.NoMatchError:
+        return None
+    except gnomekeyring.CancelledError:
+        # The user pressed "Cancel" when prompted to unlock their keyring.
+        return None
+
+    assert len(items) == 1, 'no more than one entry should ever match'
+    credentials = Credentials()
+    credentials.load(cStringIO.StringIO(items[0].secret))
+    return credentials
+
+
+def attempt_to_save_credentials_in_keyring(credentials, consumer_name):
+    """Save the credentials to the the Gnome keyring, if available."""
+    if gnomekeyring is None:
+        return False
+    if not gnomekeyring.is_available():
+        return False
+
+    # Get the credentials as a string.
+    sio = cStringIO.StringIO()
+    credentials.save(sio)
+    value = sio.getvalue()
+
+    # If the login keyring is locked (e.g., they have auto login enabled) a
+    # dialog box prompting for their keyring password will be displayed.
+    # TODO make this return False if the user cancels
+    try:
+        gnomekeyring.item_create_sync(
+            'login', gnomekeyring.ITEM_GENERIC_SECRET, consumer_name,
+            get_keyring_attr(consumer_name), value, True)
+    except gnomekeyring.CancelledError:
+        # The user pressed "Cancel" when prompted to unlock their keyring.
+        return False
+
+    return True
+
+
+def save_credentials(credentials, consumer_credentials_path, consumer_name):
+    if attempt_to_save_credentials_in_keyring(credentials, consumer_name):
+        # It worked.  Nothing more to do.
+        return
+    else:
+        # We couldn't get the credentials into the key
+        credentials.save_to_path(consumer_credentials_path)
+        os.chmod(consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
+
+
+def load_credentials(consumer_name, consumer_credentials_path):
+    keyring_credentials = query_keyring_for_credentials(consumer_name)
+    if keyring_credentials is not None:
+        return keyring_credentials
+    elif os.path.exists(consumer_credentials_path):
+        credentials = Credentials.load_from_path(consumer_credentials_path)
+        attempt_to_save_credentials_in_keyring(credentials, consumer_name)
+        return credentials
+    else:
+        return None
+
+
 class Launchpad(ServiceRoot):
     """Root Launchpad API class.
 
@@ -240,17 +328,17 @@
         """Log in to Launchpad with possibly cached credentials.
 
         This is a convenience method for either setting up new login
-        credentials, or re-using existing ones. When a login token is generated
-        using this method, the resulting credentials will be saved in
-        `credentials_file`, or if not given, into the `launchpadlib_dir`
-        directory. If the same `credentials_file`/`launchpadlib_dir` is passed
-        in a second time, the credentials in for the consumer will be used
-        automatically.
-
-        Each consumer has their own credentials per service root in
-        `launchpadlib_dir`. `launchpadlib_dir` is also used for caching
-        fetched objects. The cache is per service root, and shared by
-        all consumers.
+        credentials, or re-using existing ones. When a login token is
+        generated using this method, the resulting credentials will be stored
+        in one of these locations (given in order of priority): the Gnome
+        keyring (if available), `credentials_file` (if provided), or as a last
+        resort the `launchpadlib_dir` directory.
+
+        Subsequent calls to this method will load the credentials from one of
+        the aformentioned locations in the same priority order.
+
+        `launchpadlib_dir` is also used for caching fetched objects. The cache
+        is per service root, and shared by all consumers.
 
         See `Launchpad.get_token_and_login()` for more information about
         how new tokens are generated.
@@ -274,30 +362,35 @@
         (service_root, launchpadlib_dir, cache_path,
          service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
         credentials_path = os.path.join(service_root_dir, 'credentials')
+
         if not os.path.exists(credentials_path):
             os.makedirs(credentials_path, 0700)
+
         if credentials_file is None:
             consumer_credentials_path = os.path.join(credentials_path,
                 consumer_name)
         else:
             consumer_credentials_path = credentials_file
-        if os.path.exists(consumer_credentials_path):
-            credentials = Credentials.load_from_path(
-                consumer_credentials_path)
-            launchpad = cls(
-                credentials, service_root=service_root, cache=cache_path,
-                timeout=timeout, proxy_info=proxy_info, version=version)
-        else:
+
+        credentials = load_credentials(
+            consumer_name, consumer_credentials_path)
+
+        if credentials is None:
             launchpad = cls.get_token_and_login(
                 consumer_name, service_root=service_root, cache=cache_path,
                 timeout=timeout, proxy_info=proxy_info,
                 authorizer_class=authorizer_class,
                 allow_access_levels=allow_access_levels,
                 max_failed_attempts=max_failed_attempts, version=version)
+            # New credentials were created, so save them for future use.
             if launchpad is not None:
-                launchpad.credentials.save_to_path(consumer_credentials_path)
-                os.chmod(
-                    consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
+                save_credentials(
+                    launchpad.credentials, consumer_credentials_path,
+                    consumer_name)
+        else:
+            launchpad = cls(
+                credentials, service_root=service_root, cache=cache_path,
+                timeout=timeout, proxy_info=proxy_info, version=version)
         return launchpad
 
     @classmethod
@@ -336,3 +429,4 @@
         if not os.path.exists(cache_path):
             os.makedirs(cache_path, 0700)
         return (service_root, launchpadlib_dir, cache_path, service_root_dir)
+

=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2010-08-23 19:51:55 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2010-09-22 21:58:38 +0000
@@ -20,16 +20,28 @@
 
 import os
 import shutil
+import socket
 import stat
 import tempfile
+import textwrap
 import unittest
+from contextlib import contextmanager
 
 from lazr.restfulclient.resource import ServiceRoot
 
 from launchpadlib.credentials import (
-    AccessToken, AuthorizeRequestTokenWithBrowser, Credentials)
-from launchpadlib.launchpad import Launchpad
+    AccessToken,
+    AuthorizeRequestTokenWithBrowser,
+    Credentials,
+    )
+from launchpadlib.launchpad import (
+    Launchpad,
+    attempt_to_save_credentials_in_keyring,
+    get_keyring_attr,
+    query_keyring_for_credentials,
+    )
 from launchpadlib import uris
+import launchpadlib.launchpad
 
 class NoNetworkLaunchpad(Launchpad):
     """A Launchpad instance for tests with no network access.
@@ -152,9 +164,15 @@
     """Tests for Launchpad.login_with()."""
 
     def setUp(self):
+        # For these tests we want to disable retrieving or storing credentials
+        # in the Gnome keyring.
+        self._saved_gnomekeyring = launchpadlib.launchpad.gnomekeyring
+        launchpadlib.launchpad.gnomekeyring = None
         self.temp_dir = tempfile.mkdtemp()
 
     def tearDown(self):
+        # Restore the gnomekeyring module that we disabled in setUp.
+        launchpadlib.launchpad.gnomekeyring = self._saved_gnomekeyring
         shutil.rmtree(self.temp_dir)
 
     def test_dirs_created(self):
@@ -383,7 +401,7 @@
             ValueError, NoNetworkLaunchpad.login_with, 'app name', 'foo')
 
     def test_separate_credentials_file(self):
-        my_credentials_path = os.path.join(self.temp_dir, 'my_creds') 
+        my_credentials_path = os.path.join(self.temp_dir, 'my_creds')
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
             credentials_file=my_credentials_path,
@@ -405,5 +423,194 @@
         self.assertFalse(launchpad.get_token_and_login_called)
 
 
+class FauxSocketModule:
+    """A socket module replacement that provides a fake hostname."""
+
+    def gethostname(self):
+        return 'HOSTNAME'
+
+
+class FauxCancelledError(Exception):
+    """A substitute for gnomekeyring.CancelledError."""
+
+
+class FauxNoMatchError(Exception):
+    """A substitute for gnomekeyring.NoMatchError."""
+
+
+class BaseFauxKeyringModule:
+    """A base to build fake keyring modules with various behaviors."""
+
+    CancelledError = FauxCancelledError
+    NoMatchError = FauxNoMatchError
+    ITEM_GENERIC_SECRET = 42
+
+    def is_available(self):
+        return True
+
+
+class UnavailableKeyring:
+    """A keyring that behaves as if the Gnome keyring daemon is not running."""
+    def is_available(self):
+        return False
+
+
+class CancelledKeyring(BaseFauxKeyringModule):
+    """A keyring that behaves as if the user canceled password entry."""
+
+    def find_items_sync(self, type, attr):
+        raise self.CancelledError
+
+    def item_create_sync(self, *args):
+        raise self.CancelledError
+
+
+class NoCredentialsKeyring(BaseFauxKeyringModule):
+    """A keyring that doesn't contain the credentials."""
+
+    def find_items_sync(self, type, attr):
+        raise self.NoMatchError
+
+
+class GoodKeyring(BaseFauxKeyringModule):
+    """A keyring that behaves as if the load and store operations succeed."""
+
+    def item_create_sync(self, *args):
+        pass
+
+    def find_items_sync(self, type, attr):
+        credentials_data = textwrap.dedent("""\
+            [1]
+            consumer_secret = CONSUMER SECRET
+            access_token = ACCESS TOKEN
+            consumer_key = CONSUMER NAME
+            access_secret = ACCESS SECRET""")
+
+        class FauxKeyringItem:
+            secret = credentials_data
+
+        return [FauxKeyringItem()]
+
+
+@contextmanager
+def fake_keyring(fake):
+    original = launchpadlib.launchpad.gnomekeyring
+    launchpadlib.launchpad.gnomekeyring = fake
+    yield
+    launchpadlib.launchpad.gnomekeyring = original
+
+
+class TestLoadFromKeyring(unittest.TestCase):
+
+    def setUp(self):
+        # launchpadlib.launchpad uses the socket module to look up the
+        # hostname, obviously that can vary so we replace the socket module
+        # with a fake that returns a fake hostname.
+        launchpadlib.launchpad.socket = FauxSocketModule()
+        self.temp_dir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.temp_dir)
+        launchpadlib.launchpad.socket = socket
+
+    def test_get_keyring_attr(self):
+        # Gnome keyring entries are identified by a set of "attributes".  We
+        # construct the attributes such that each application will have a
+        # unique entry that shouldn't conflict with an applications other uses
+        # of the keyring.
+        self.assertEqual(
+            get_keyring_attr('CONSUMER NAME'),
+            {'key-type': 'launchpadlib credentials',
+             'token-name': 'CONSUMER NAME (launchpadlib on HOSTNAME)'})
+
+    def test_no_keyring_module(self):
+        # If the gnomekeyring module cannot be imported (and is therefore set
+        # to None) an attempt to load credentials from the keyring will return
+        # None instead.
+        with fake_keyring(None):
+            self.assertEqual(
+                query_keyring_for_credentials('CONSUMER NAME'), None)
+
+    def test_no_keyring_available(self):
+        # If the gnomekeyring module can be imported but the keyring daemon is
+        # not running, an attempt to load credentials from the keyring will
+        # return None instead.
+
+        with fake_keyring(UnavailableKeyring()):
+            self.assertEqual(
+                query_keyring_for_credentials('CONSUMER NAME'), None)
+
+    def test_keyring_does_not_have_credentials(self):
+        # If the keyring doesn't have the credentials stored in it, an attempt
+        # to load credentials from the keyring will return None.
+        with fake_keyring(NoCredentialsKeyring()):
+            self.assertEqual(
+                query_keyring_for_credentials('CONSUMER NAME'), None)
+
+    def test_user_canceled_password_prompt(self):
+        # If the user cancels the dialog asking for their password to unlock
+        # the keyring the keyring will return None.
+        with fake_keyring(CancelledKeyring()):
+            self.assertEqual(
+                query_keyring_for_credentials('CONSUMER NAME'), None)
+
+    def test_successful_credentials_lookup(self):
+        # If serialized credentials are found in the keyring, a Credentials
+        # object is constructed and returned.
+        with fake_keyring(GoodKeyring()):
+            credentials  = query_keyring_for_credentials('CONSUMER NAME')
+            self.assertNotEqual(credentials, None)
+            self.assertEqual(
+                credentials.user_agent_params['oauth_consumer'],
+                'CONSUMER NAME')
+            self.assertEqual(credentials.access_token.key, 'ACCESS TOKEN')
+
+
+class FauxCredentials:
+    """A stand-in for the launchpadlib Credentials object."""
+
+    def save(self, writable_file):
+        writable_file.write('faux credentials')
+
+
+class TestSaveToKeyring(unittest.TestCase):
+
+    def setUp(self):
+        # launchpadlib.launchpad uses the socket module to look up the
+        # hostname, obviously that can vary so we replace the socket module
+        # with a fake that returns a fake hostname.
+        launchpadlib.launchpad.socket = FauxSocketModule()
+        self.temp_dir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.temp_dir)
+        launchpadlib.launchpad.socket = socket
+
+    def test_no_keyring_available(self):
+        # If the gnomekeyring module can be imported but the keyring daemon is
+        # not running, an attempt to save credentials from the keyring will
+        # return False to indicate that the data was not saved.
+        with fake_keyring(UnavailableKeyring()):
+            result = attempt_to_save_credentials_in_keyring(
+                None, 'CONSUMER NAME')
+            self.assertEqual(result, False)
+
+    def test_user_canceled_password_prompt(self):
+        # If the user cancels the dialog asking for their password to unlock
+        # the keyring the credentials will not be saved in the keyring.
+        with fake_keyring(CancelledKeyring()):
+            result = attempt_to_save_credentials_in_keyring(
+                FauxCredentials(), 'CONSUMER NAME')
+            self.assertEqual(result, False)
+
+    def test_successful_save(self):
+        # If the credentials were successfully written to the keyring, True is
+        # returned.
+        with fake_keyring(GoodKeyring()):
+            result = attempt_to_save_credentials_in_keyring(
+                FauxCredentials(), 'CONSUMER NAME')
+            self.assertEqual(result, True)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)