[Checkins] SVN: cipher.googlepam/trunk/ FileCache reliability fixes.
Marius Gedminas
cvs-admin at zope.org
Wed Oct 10 14:10:52 UTC 2012
Log message for revision 127958:
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.
Changed:
U cipher.googlepam/trunk/CHANGES.txt
U cipher.googlepam/trunk/src/cipher/googlepam/pam_google.py
U cipher.googlepam/trunk/src/cipher/googlepam/tests/test_doc.py
-=-
Modified: cipher.googlepam/trunk/CHANGES.txt
===================================================================
--- cipher.googlepam/trunk/CHANGES.txt 2012-10-10 09:25:17 UTC (rev 127957)
+++ cipher.googlepam/trunk/CHANGES.txt 2012-10-10 14:10:48 UTC (rev 127958)
@@ -4,9 +4,18 @@
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)
------------------
Modified: cipher.googlepam/trunk/src/cipher/googlepam/pam_google.py
===================================================================
--- cipher.googlepam/trunk/src/cipher/googlepam/pam_google.py 2012-10-10 09:25:17 UTC (rev 127957)
+++ cipher.googlepam/trunk/src/cipher/googlepam/pam_google.py 2012-10-10 14:10:48 UTC (rev 127958)
@@ -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 @@
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 _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)
Modified: cipher.googlepam/trunk/src/cipher/googlepam/tests/test_doc.py
===================================================================
--- cipher.googlepam/trunk/src/cipher/googlepam/tests/test_doc.py 2012-10-10 09:25:17 UTC (rev 127957)
+++ cipher.googlepam/trunk/src/cipher/googlepam/tests/test_doc.py 2012-10-10 14:10:48 UTC (rev 127958)
@@ -15,6 +15,9 @@
"""
import doctest
import os
+import tempfile
+import ConfigParser
+import shutil
from cipher.googlepam import pam_google
from gdata.apps.service import AppsForYourDomainException
@@ -22,6 +25,14 @@
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 @@
>>> 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 @@
>>> 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
More information about the checkins
mailing list