Hi Michael,
I cannot test kerberos auth myself, but you are right that there is a
race condition that can play havoc with those state variables. I fixed
this by simply wrapping the code in question in a lock. This should have
minimal performance implications and makes the code safe to use.
Can you please test if the attached patch on top of the current 'next'
branch fixes things for you? You could either get the current next
branch and apply this patch
https://github.com/nicolas33/offlineimap/tarball/next
or pull from my git repo with
git clone g...@github.com:spaetz/offlineimap.git
and do
git checkout feature/kerberosracekondition
to get the branch with the patch already applied.
Thanks for testing
Sebastian
>From 627d9a73b328f43cb16b5a0d868577cf71b2e83f Mon Sep 17 00:00:00 2001
From: Sebastian Spaeth <sebast...@sspaeth.de>
Date: Wed, 9 Mar 2011 12:01:43 +0100
Subject: [PATCH] Fix race condition with kerberos authentication
There is a race condition that leads kerberos authentication to fail
when maxconnections > 1. Fix this by simply locking around the code in
question.
Signed-off-by: Sebastian Spaeth <sebast...@sspaeth.de>
---
diff | 272 +++++++++++++++++++++++++++++++++++++++++++++
offlineimap/imapserver.py | 2 +
2 files changed, 274 insertions(+), 0 deletions(-)
create mode 100644 diff
diff --git a/diff b/diff
new file mode 100644
index 0000000..fc90427
--- /dev/null
+++ b/diff
@@ -0,0 +1,272 @@
+diff --git a/offlineimap/imapserver.py b/offlineimap/imapserver.py
+index 7ead9a4..342b551 100644
+--- a/offlineimap/imapserver.py
++++ b/offlineimap/imapserver.py
+@@ -22,7 +22,6 @@ from offlineimap.ui import getglobalui
+ from threading import *
+ import thread, hmac, os, time
+ import base64
+-
+ from StringIO import StringIO
+ from platform import system
+
+@@ -112,15 +111,19 @@ class IMAPServer:
+ else:
+ self.port = 143
+ self.maxconnections = maxconnections
+- self.availableconnections = []
+- self.assignedconnections = []
+- self.lastowner = {}
+- self.semaphore = BoundedSemaphore(self.maxconnections)
++ self.avail_conn = {}
++ """containing imapobj that are available for reuse
++ {lastthread_id: imapobj, ...}"""
++ self.assigned_conn = []
++ """list of imapobj that are currently in use"""
++ self.maxconn_semaphore = BoundedSemaphore(self.maxconnections)
++ """Semaphore restricting the number of possible connections"""
+ self.connectionlock = Lock()
++ """lock protecting us from concurrently modifying
++ self.avail_conn or self.assigned_conn"""
+ self.reference = reference
+ self.gss_step = self.GSS_STATE_STEP
+ self.gss_vc = None
+- self.gssapi = False
+
+ def getpassword(self):
+ if self.goodpassword != None:
+@@ -150,19 +153,14 @@ class IMAPServer:
+ def releaseconnection(self, connection):
+ """Releases a connection, returning it to the pool."""
+ self.connectionlock.acquire()
+- self.assignedconnections.remove(connection)
+- self.availableconnections.append(connection)
++ self.assigned_conn.remove(connection)
++ self.avail_conn[thread.get_ident()] = connection
+ self.connectionlock.release()
+- self.semaphore.release()
+-
+- def md5handler(self, response):
+- challenge = response.strip()
+- self.ui.debug('imap', 'md5handler: got challenge %s' % challenge)
++ self.maxconn_semaphore.release()
+
+- passwd = self.getpassword()
+- retval = self.username + ' ' + hmac.new(passwd, challenge).hexdigest()
+- self.ui.debug('imap', 'md5handler: returning %s' % retval)
+- return retval
++ def login_cram_md5(self, imapobj):
++ self.ui.debug('imap', 'Attempting CRAM-MD5 authentication')
++ imapobj.login_cram_md5(self.username, self.getpassword())
+
+ def plainauth(self, imapobj):
+ self.ui.debug('imap', 'Attempting plain authentication')
+@@ -195,49 +193,55 @@ class IMAPServer:
+ response = ''
+ return base64.b64decode(response)
+
+- def acquireconnection(self):
+- """Fetches a connection from the pool, making sure to create a new one
+- if needed, to obey the maximum connection limits, etc.
+- Opens a connection to the server and returns an appropriate
+- object."""
+
+- self.semaphore.acquire()
+- self.connectionlock.acquire()
++ def get_available_connection(self):
++ """Return an imapobj of an already existing connection
++
++ :returns: imapobj if a connection can be reused or None if there
++ are none available."""
+ imapobj = None
++ if len(self.avail_conn) == 0: # None is available
++ return None
++
++ with self.connectionlock:
++ #lock protecting us from concurrently modifying
++ #self.avail_conn or self.assigned_conn
+
+- if len(self.availableconnections): # One is available.
+ # Try to find one that previously belonged to this thread
+- # as an optimization. Start from the back since that's where
+- # they're popped on.
+ threadid = thread.get_ident()
+- imapobj = None
+- for i in range(len(self.availableconnections) - 1, -1, -1):
+- tryobj = self.availableconnections[i]
+- if self.lastowner[tryobj] == threadid:
+- imapobj = tryobj
+- del(self.availableconnections[i])
+- break
++ if threadid in self.avail_conn:
++ imapobj = self.avail_conn.pop(threadid)
++ #otherwise get a random one
+ if not imapobj:
+- imapobj = self.availableconnections[0]
+- del(self.availableconnections[0])
+- self.assignedconnections.append(imapobj)
+- self.lastowner[imapobj] = thread.get_ident()
+- self.connectionlock.release()
++ oldthreadid, imapobj = self.avail_conn.popitem()
++
++ self.assigned_conn.append(imapobj)
++ return imapobj
++
++ def acquireconnection(self):
++ """Fetches a connection from the pool, making sure to create a new one
++ if needed, to obey the maximum connection limits, etc.
++ Opens a connection to the server and returns an appropriate
++ imapobj object."""
++ #see if we can get another connection, block if all are in use.
++ self.maxconn_semaphore.acquire()
++ # first try to recycle an existing connection
++ imapobj = self.get_available_connection()
++ if imapobj:
+ return imapobj
+-
+- self.connectionlock.release() # Release until need to modify data
+
+ """ Must be careful here that if we fail we should bail out gracefully
+ and release locks / threads so that the next attempt can try...
+ """
+ success = 0
+ try:
++ #Repeat in case we got a wrong password
+ while not success:
+ # Generate a new connection.
+ if self.tunnel:
+ self.ui.connecting('tunnel', self.tunnel)
+ imapobj = UsefulIMAP4_Tunnel(self.tunnel)
+- success = 1
++ success = 1 #mark success, so we can skip authentication
+ elif self.usessl:
+ self.ui.connecting(self.hostname, self.port)
+ imapobj = UsefulIMAP4_SSL(self.hostname, self.port,
+@@ -249,41 +253,38 @@ class IMAPServer:
+
+ imapobj.mustquote = imaplibutil.mustquote
+
+- if not self.tunnel:
++ # Authenticate: GSSAPI, MD5-CRAM, then PLAINTEXT
++ if not success and have_gss and \
++ 'AUTH=GSSAPI' in imapobj.capabilities:
++ self.ui.debug('imap',
++ 'Attempting GSSAPI authentication')
+ try:
+- # Try GSSAPI and continue if it fails
+- if 'AUTH=GSSAPI' in imapobj.capabilities and have_gss:
+- self.ui.debug('imap',
+- 'Attempting GSSAPI authentication')
+- try:
+- imapobj.authenticate('GSSAPI', self.gssauth)
+- except imapobj.error, val:
+- self.gssapi = False
+- self.ui.debug('imap',
+- 'GSSAPI Authentication failed')
+- else:
+- self.gssapi = True
+- #if we do self.password = None then the next attempt cannot try...
+- #self.password = None
+-
+- if not self.gssapi:
+- if 'AUTH=CRAM-MD5' in imapobj.capabilities:
+- self.ui.debug('imap',
+- 'Attempting CRAM-MD5 authentication')
+- try:
+- imapobj.authenticate('CRAM-MD5', self.md5handler)
+- except imapobj.error, val:
+- self.plainauth(imapobj)
+- else:
+- self.plainauth(imapobj)
+- # Would bail by here if there was a failure.
+- success = 1
+- self.goodpassword = self.password
++ imapobj.authenticate('GSSAPI', self.gssauth)
+ except imapobj.error, val:
+- self.passworderror = str(val)
+- raise
+- #self.password = None
++ self.ui.debug('imap',
++ 'GSSAPI Authentication failed')
++ else:
++ success = True
++ break
++
++ #cram md5 or plainaut?
++ if 'AUTH=CRAM-MD5' in imapobj.capabilities:
++ authmethod = self.login_cram_md5
++ else:
++ authmethod = self.plainauth
++
++ try:
++ authmethod(imapobj)
++ except imapobj.error as e:
++ self.passworderror = str(e)
++ self.ui.warn('imap',
++ 'Login failed: %s' % str(e))
++ raise
++ else:
++ success = True
++ self.goodpassword = self.password
+
++ #Authenticated. Retrieve self.delim and self.root if necessary
+ if self.delim == None:
+ listres = imapobj.list(self.reference, '""')[1]
+ if listres == [None] or listres == None:
+@@ -304,14 +305,13 @@ class IMAPServer:
+ self.root = imaputil.dequote(self.root)
+
+ self.connectionlock.acquire()
+- self.assignedconnections.append(imapobj)
+- self.lastowner[imapobj] = thread.get_ident()
++ self.assigned_conn.append(imapobj)
+ self.connectionlock.release()
+ return imapobj
+ except:
+ """If we are here then we did not succeed in getting a connection -
+ we should clean up and then re-raise the error..."""
+- self.semaphore.release()
++ self.maxconn_semaphore.release()
+
+ #Make sure that this can be retried the next time...
+ self.passworderror = None
+@@ -327,22 +327,21 @@ class IMAPServer:
+ to copy messages, then have them all wait for 3 available connections.
+ It's OK if we have maxconnections + 1 or 2 threads, which is what
+ this will help us do."""
+- threadutil.semaphorewait(self.semaphore)
++ threadutil.semaphorewait(self.maxconn_semaphore)
+
+ def close(self):
+ # Make sure I own all the semaphores. Let the threads finish
+ # their stuff. This is a blocking method.
+ self.connectionlock.acquire()
+- threadutil.semaphorereset(self.semaphore, self.maxconnections)
+- for imapobj in self.assignedconnections + self.availableconnections:
++ threadutil.semaphorereset(self.maxconn_semaphore, self.maxconnections)
++ #log out of both idle and assigned connections.
++ for imapobj in self.assigned_conn + self.avail_conn.values():
+ imapobj.logout()
+- self.assignedconnections = []
+- self.availableconnections = []
+- self.lastowner = {}
++ self.assigned_conn = []
++ self.avail_conn = {}
+ # reset kerberos state
+ self.gss_step = self.GSS_STATE_STEP
+ self.gss_vc = None
+- self.gssapi = False
+ self.connectionlock.release()
+
+ def keepalive(self, timeout, event):
+@@ -361,8 +360,8 @@ class IMAPServer:
+ return
+ self.ui.debug('imap', 'keepalive: acquiring connectionlock')
+ self.connectionlock.acquire()
+- numconnections = len(self.assignedconnections) + \
+- len(self.availableconnections)
++ numconnections = len(self.assigned_conn) + \
++ len(self.avail_conn)
+ self.connectionlock.release()
+ self.ui.debug('imap', 'keepalive: connectionlock released')
+ threads = []
diff --git a/offlineimap/imapserver.py b/offlineimap/imapserver.py
index 7ead9a4..b1f535f 100644
--- a/offlineimap/imapserver.py
+++ b/offlineimap/imapserver.py
@@ -116,6 +116,7 @@ class IMAPServer:
self.assignedconnections = []
self.lastowner = {}
self.semaphore = BoundedSemaphore(self.maxconnections)
+ self.gss_lock = Lock()
self.connectionlock = Lock()
self.reference = reference
self.gss_step = self.GSS_STATE_STEP
@@ -171,6 +172,7 @@ class IMAPServer:
def gssauth(self, response):
data = base64.b64encode(response)
try:
+ with self.gss_lock:
if self.gss_step == self.GSS_STATE_STEP:
if not self.gss_vc:
rc, self.gss_vc = kerberos.authGSSClientInit('imap@' +
--
1.7.1
