FileCache reliability fixes.

Avoid incorrect cache lookups (or invalidations) when a username is a
proper prefix of some other username.

Avoid cache poisoning if usernames contain embedded '::' separators or
newlines.

Avoid exceptions on a race condition if the cache file disappears after
we check for its existence but before we open it for reading.

CHANGES.txt

@@ -4,7 +4,16 @@ CHANGES
 1.5.1 (unreleased)
 ------------------
 
-- Nothing changed yet.
+- FileCache reliability fixes:
+
+  + Avoid incorrect cache lookups (or invalidations) when a username is a
+    proper prefix of some other username.
+
+  + Avoid cache poisoning if usernames contain embedded '::' separators or
+    newlines.
+
+  + Avoid exceptions on a race condition if the cache file disappears after
+    we check for its existence but before we open it for reading.
 
 
 1.5.0 (2012-10-09)

src/cipher/googlepam/pam_google.py

@@ -26,6 +26,7 @@
 import optparse
 import os
 import time
+import errno
 
 from gdata.apps.groups.service import GroupsService
 from gdata.apps.service import AppsService, AppsForYourDomainException
@@ -87,16 +88,23 @@ def __init__(self, pam):
         self._filename = self.pam.config.get(self.SECTION_NAME, 'file')
 
     def _get_user_info(self, username):
-        if not os.path.exists(self._filename):
-            return
-        with open(self._filename, 'r') as file:
-            for line in file:
-                if line.startswith(username):
-                    username, created, pw_hash = line.strip().split('::', 2)
-                    return UserInfo(float(created), pw_hash)
+        try:
+            with open(self._filename, 'r') as file:
+                for line in file:
+                    if line.startswith(username + '::'):
+                        username, created, pw_hash = line.strip().split('::', 2)
+                        return UserInfo(float(created), pw_hash)
+        except IOError, e:
+            pass
         return None
 
     def _add_user_info(self, username, password):
+        if '::' in username or '\n' in username:
+            # let's not break our cache file, mmkay?
+            # also, it would be a Bad Idea if we let people stuff their
+            # own username + passwordhash combos into the cache file by
+            # stuffing them into the username
+            return
         with open(self._filename, 'a') as file:
             file.write('%s::%f::%s\n' %(
                     username,
@@ -107,11 +115,15 @@ def _add_user_info(self, username, password):
     def _del_user_info(self, username):
         if not os.path.exists(self._filename):
             return
-        with open(self._filename, 'r') as file:
-            lines = [line for line in file
-                     if not line.startswith(username)]
-        with open(self._filename, 'w') as file:
-            file.writelines(lines)
+        try:
+            with open(self._filename, 'r') as file:
+                lines = [line for line in file
+                         if not line.startswith(username + '::')]
+        except IOError:
+            pass
+        else:
+            with open(self._filename, 'w') as file:
+                file.writelines(lines)
 
     def clear(self):
         os.remove(self._filename)

src/cipher/googlepam/tests/test_doc.py

@@ -15,13 +15,24 @@
 """
 import doctest
 import os
+import tempfile
+import ConfigParser
+import shutil
 
 from cipher.googlepam import pam_google
 from gdata.apps.service import AppsForYourDomainException
 from gdata.service import BadAuthentication, CaptchaRequired
 
 HERE = os.path.dirname(__file__)
 
+class GooglePAMStub(object):
+    def __init__(self, config={}):
+        self.config = ConfigParser.ConfigParser()
+        for section, d in config.items():
+            self.config.add_section(section)
+            for k, v in d.items():
+                self.config.set(section, k, v)
+
 class FakePamMessage(object):
     def __init__(self, flags, prompt):
         pass
@@ -317,6 +328,10 @@ def doctest_FileCache():
       >>> pam._cache.authenticate('user', 'bad')
       False
 
+    Regression test: truncated usernames do not match
+
+      >>> pam._cache.authenticate('use', 'pwd')
+
     When the cache entry times out, the cache behaves as it has no entry:
 
       >>> pam._cache.lifespan = 0
@@ -328,6 +343,112 @@ def doctest_FileCache():
       >>> pam._cache.clear()
     """
 
+def doctest_FileCache_get_user_info_file_does_not_exist():
+    """Test for FileCache._get_user_info
+
+        >>> pam = GooglePAMStub({'file-cache': {
+        ...     'file': 'nosuchfile',
+        ...     'lifespan': '600',
+        ... }})
+        >>> fc = pam_google.FileCache(pam)
+        >>> fc._get_user_info('bob')
+
+    """
+
+def doctest_FileCache_get_user_info_file_goes_away():
+    """Test for FileCache._get_user_info
+
+        >>> orig_exists = os.path.exists
+
+        >>> pam = GooglePAMStub({'file-cache': {
+        ...     'file': 'nosuchfile',
+        ...     'lifespan': '600',
+        ... }})
+        >>> fc = pam_google.FileCache(pam)
+        >>> os.path.exists = lambda filename: True
+
+    Look-before-you-leap is prone to race conditions: the file might be
+    deleted after you check for its existence
+
+        >>> fc._get_user_info('bob')
+
+        >>> os.path.exists = orig_exists
+
+    """
+
+def doctest_FileCache_add_user_info():
+    r"""Test for FileCache._add_user_info
+
+        >>> tempdir = tempfile.mkdtemp(prefix='cipher.googlepam-test-')
+        >>> cachefile = os.path.join(tempdir, 'cache')
+
+        >>> pam = GooglePAMStub({'file-cache': {
+        ...     'file': cachefile,
+        ...     'lifespan': '600',
+        ... }})
+        >>> fc = pam_google.FileCache(pam)
+        >>> fc._add_user_info('bob', 's3cr3t')
+        >>> print open(cachefile).read().strip()
+        bob::...::...
+
+    you can't poison the cache with trick usernames
+
+        >>> fc._add_user_info('bob::0::mypasswordhash', 's3cr3t')
+        >>> fc._add_user_info('fred\nroot', 's3cr3t')
+        >>> print len(open(cachefile).readlines())
+        1
+
+        >>> shutil.rmtree(tempdir)
+
+    """
+
+def doctest_FileCache_del_user_info_file_goes_away():
+    """Test for FileCache._del_user_info
+
+        >>> orig_exists = os.path.exists
+
+        >>> pam = GooglePAMStub({'file-cache': {
+        ...     'file': '/nosuchfile',
+        ...     'lifespan': '600',
+        ... }})
+        >>> fc = pam_google.FileCache(pam)
+        >>> os.path.exists = lambda filename: True
+
+    Look-before-you-leap is prone to race conditions: the file might be
+    deleted after you check for its existence
+
+        >>> fc._del_user_info('bob')
+
+        >>> os.path.exists = orig_exists
+
+    """
+
+def doctest_FileCache_del_user_info_prefix_safety():
+    """Test for FileCache._del_user_info
+
+        >>> tempdir = tempfile.mkdtemp(prefix='cipher.googlepam-test-')
+        >>> cachefile = os.path.join(tempdir, 'cache')
+
+        >>> pam = GooglePAMStub({'file-cache': {
+        ...     'file': cachefile,
+        ...     'lifespan': '600',
+        ... }})
+        >>> fc = pam_google.FileCache(pam)
+        >>> fc._add_user_info('bob', 's3cr3t')
+
+    Now we try to delete 'bo', which is a prefix of 'bob'
+
+        >>> fc._del_user_info('bo')
+
+    and we should still have the 'bob' line in the cache file
+
+        >>> print len(open(cachefile).readlines())
+        1
+
+        >>> shutil.rmtree(tempdir)
+
+    """
+
 def doctest_MemcacheCache():
     """class MemcacheCache