From 845e8f943f47ed3b88e19ff8351e6f248a9d6ab9 Mon Sep 17 00:00:00 2001 From: exvito Date: Thu, 2 Apr 2015 14:24:54 +0100 Subject: [PATCH 1/6] Issue #2334 - HTTPAuthDigest - Making it thread-safe The existing code counts the number of 401 responses in the num_401_calls authenticator attribute. This is in place so as to ensure the necessary auth header is sent, while avoiding infinite 401 loops (issue #547). This commit makes num_401_calls an instance of threading.local() (previously an integer), using num_401_calls.value as the counter. It ensures that concurrent authentication requests get each their own counter and behave as expected (otherwise every other concurrent request would have its authentication fail). --- requests/auth.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/requests/auth.py b/requests/auth.py index d1c48251..289da366 100644 --- a/requests/auth.py +++ b/requests/auth.py @@ -11,6 +11,7 @@ import os import re import time import hashlib +import threading from base64 import b64encode @@ -67,7 +68,7 @@ class HTTPDigestAuth(AuthBase): self.nonce_count = 0 self.chal = {} self.pos = None - self.num_401_calls = 1 + self.num_401_calls = threading.local() def build_digest_header(self, method, url): @@ -157,7 +158,7 @@ class HTTPDigestAuth(AuthBase): def handle_redirect(self, r, **kwargs): """Reset num_401_calls counter on redirects.""" if r.is_redirect: - self.num_401_calls = 1 + self.num_401_calls.value = 1 def handle_401(self, r, **kwargs): """Takes the given response and tries digest-auth, if needed.""" @@ -166,12 +167,12 @@ class HTTPDigestAuth(AuthBase): # Rewind the file position indicator of the body to where # it was to resend the request. r.request.body.seek(self.pos) - num_401_calls = getattr(self, 'num_401_calls', 1) + num_401_calls = getattr(self.num_401_calls, 'value', 1) s_auth = r.headers.get('www-authenticate', '') if 'digest' in s_auth.lower() and num_401_calls < 2: - self.num_401_calls += 1 + self.num_401_calls.value += 1 pat = re.compile(r'digest ', flags=re.IGNORECASE) self.chal = parse_dict_header(pat.sub('', s_auth, count=1)) @@ -191,7 +192,7 @@ class HTTPDigestAuth(AuthBase): return _r - self.num_401_calls = 1 + self.num_401_calls.value = 1 return r def __call__(self, r): @@ -208,4 +209,6 @@ class HTTPDigestAuth(AuthBase): self.pos = None r.register_hook('response', self.handle_401) r.register_hook('response', self.handle_redirect) + self.num_401_calls.value = 1 + return r From e65360dbaf252a136a011adf639e5dce880a2412 Mon Sep 17 00:00:00 2001 From: exvito Date: Thu, 2 Apr 2015 21:29:51 +0100 Subject: [PATCH 2/6] Issue #2334 - HTTPDigestAuth - Replace getattr utilization Following Lukasa + kennethreitz suggestion. --- requests/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requests/auth.py b/requests/auth.py index 289da366..5c5332c8 100644 --- a/requests/auth.py +++ b/requests/auth.py @@ -167,7 +167,7 @@ class HTTPDigestAuth(AuthBase): # Rewind the file position indicator of the body to where # it was to resend the request. r.request.body.seek(self.pos) - num_401_calls = getattr(self.num_401_calls, 'value', 1) + num_401_calls = self.num_401_calls.value s_auth = r.headers.get('www-authenticate', '') if 'digest' in s_auth.lower() and num_401_calls < 2: From e8d9bc55bc9fae82e3ba934d3f7a93e94a72d094 Mon Sep 17 00:00:00 2001 From: exvito Date: Fri, 3 Apr 2015 14:21:29 +0100 Subject: [PATCH 3/6] Issue #2334 - HTTPDigestAuth - All state now in thread local storage Following feedback from tardyp and @vincentxb. --- requests/auth.py | 65 ++++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/requests/auth.py b/requests/auth.py index 5c5332c8..7555114e 100644 --- a/requests/auth.py +++ b/requests/auth.py @@ -64,19 +64,24 @@ class HTTPDigestAuth(AuthBase): def __init__(self, username, password): self.username = username self.password = password - self.last_nonce = '' - self.nonce_count = 0 - self.chal = {} - self.pos = None - self.num_401_calls = threading.local() + # Keep state in per-thread local storage + self.tl = threading.local() + self.init_per_thread_state() + + def init_per_thread_state(self): + self.tl.last_nonce = '' + self.tl.nonce_count = 0 + self.tl.chal = {} + self.tl.pos = None + self.tl.num_401_calls = None def build_digest_header(self, method, url): - realm = self.chal['realm'] - nonce = self.chal['nonce'] - qop = self.chal.get('qop') - algorithm = self.chal.get('algorithm') - opaque = self.chal.get('opaque') + realm = self.tl.chal['realm'] + nonce = self.tl.chal['nonce'] + qop = self.tl.chal.get('qop') + algorithm = self.tl.chal.get('algorithm') + opaque = self.tl.chal.get('opaque') if algorithm is None: _algorithm = 'MD5' @@ -114,12 +119,12 @@ class HTTPDigestAuth(AuthBase): HA1 = hash_utf8(A1) HA2 = hash_utf8(A2) - if nonce == self.last_nonce: - self.nonce_count += 1 + if nonce == self.tl.last_nonce: + self.tl.nonce_count += 1 else: - self.nonce_count = 1 - ncvalue = '%08x' % self.nonce_count - s = str(self.nonce_count).encode('utf-8') + self.tl.nonce_count = 1 + ncvalue = '%08x' % self.tl.nonce_count + s = str(self.tl.nonce_count).encode('utf-8') s += nonce.encode('utf-8') s += time.ctime().encode('utf-8') s += os.urandom(8) @@ -139,7 +144,7 @@ class HTTPDigestAuth(AuthBase): # XXX handle auth-int. return None - self.last_nonce = nonce + self.tl.last_nonce = nonce # XXX should the partial digests be encoded too? base = 'username="%s", realm="%s", nonce="%s", uri="%s", ' \ @@ -158,23 +163,23 @@ class HTTPDigestAuth(AuthBase): def handle_redirect(self, r, **kwargs): """Reset num_401_calls counter on redirects.""" if r.is_redirect: - self.num_401_calls.value = 1 + self.tl.num_401_calls = 1 def handle_401(self, r, **kwargs): """Takes the given response and tries digest-auth, if needed.""" - if self.pos is not None: + if self.tl.pos is not None: # Rewind the file position indicator of the body to where # it was to resend the request. - r.request.body.seek(self.pos) - num_401_calls = self.num_401_calls.value + r.request.body.seek(self.tl.pos) + num_401_calls = self.tl.num_401_calls s_auth = r.headers.get('www-authenticate', '') if 'digest' in s_auth.lower() and num_401_calls < 2: - self.num_401_calls.value += 1 + self.tl.num_401_calls += 1 pat = re.compile(r'digest ', flags=re.IGNORECASE) - self.chal = parse_dict_header(pat.sub('', s_auth, count=1)) + self.tl.chal = parse_dict_header(pat.sub('', s_auth, count=1)) # Consume content and release the original connection # to allow our new request to reuse the same one. @@ -192,23 +197,29 @@ class HTTPDigestAuth(AuthBase): return _r - self.num_401_calls.value = 1 + self.tl.num_401_calls = 1 return r def __call__(self, r): + # When called from a thread other than the one that __init__'ed us + # per-thread state may be missing: initialize it if that's the case. + try: + self.tl.last_nonce + except AttributeError: + self.init_per_thread_state() # If we have a saved nonce, skip the 401 - if self.last_nonce: + if self.tl.last_nonce: r.headers['Authorization'] = self.build_digest_header(r.method, r.url) try: - self.pos = r.body.tell() + self.tl.pos = r.body.tell() except AttributeError: # In the case of HTTPDigestAuth being reused and the body of # the previous request was a file-like object, pos has the # file position of the previous body. Ensure it's set to # None. - self.pos = None + self.tl.pos = None r.register_hook('response', self.handle_401) r.register_hook('response', self.handle_redirect) - self.num_401_calls.value = 1 + self.tl.num_401_calls = 1 return r From 142e24de7b456d09b88bbbe0b9c978755254ca95 Mon Sep 17 00:00:00 2001 From: Pierre Tardy Date: Fri, 3 Apr 2015 15:38:59 +0200 Subject: [PATCH 4/6] digestauth: threadsafe test Signed-off-by: Pierre Tardy --- test_requests.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test_requests.py b/test_requests.py index 15406a22..22af2995 100755 --- a/test_requests.py +++ b/test_requests.py @@ -32,6 +32,11 @@ try: except ImportError: import io as StringIO +try: + from multiprocessing.pool import ThreadPool +except ImportError: + ThreadPool = None + if is_py3: def u(s): return s @@ -411,6 +416,21 @@ class RequestsTestCase(unittest.TestCase): r = requests.get(url, auth=auth) assert '"auth"' in r.request.headers['Authorization'] + def test_DIGESTAUTH_THREADED(self): + + auth = HTTPDigestAuth('user', 'pass') + url = httpbin('digest-auth', 'auth', 'user', 'pass') + session = requests.Session() + session.auth=auth + + def do_request(i): + r = session.get(url) + assert '"auth"' in r.request.headers['Authorization'] + return 1 + if ThreadPool is not None: + pool = ThreadPool(processes=50) + pool.map(do_request, range(100)) + def test_POSTBIN_GET_POST_FILES(self): url = httpbin('post') From 36dea434065fa174105ac220152d94f7a39827b5 Mon Sep 17 00:00:00 2001 From: exvito Date: Fri, 3 Apr 2015 17:04:33 +0100 Subject: [PATCH 5/6] Issue #2334 - HTTPDigestAuth - Improved per-thread state init Inspired in @tardyp approach. --- requests/auth.py | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/requests/auth.py b/requests/auth.py index 7555114e..f19f9dff 100644 --- a/requests/auth.py +++ b/requests/auth.py @@ -66,14 +66,16 @@ class HTTPDigestAuth(AuthBase): self.password = password # Keep state in per-thread local storage self.tl = threading.local() - self.init_per_thread_state() def init_per_thread_state(self): - self.tl.last_nonce = '' - self.tl.nonce_count = 0 - self.tl.chal = {} - self.tl.pos = None - self.tl.num_401_calls = None + # Ensure state is initialized just once per-thread + if not hasattr(self.tl, 'init'): + self.tl.init = True + self.tl.last_nonce = '' + self.tl.nonce_count = 0 + self.tl.chal = {} + self.tl.pos = None + self.tl.num_401_calls = None def build_digest_header(self, method, url): @@ -201,12 +203,8 @@ class HTTPDigestAuth(AuthBase): return r def __call__(self, r): - # When called from a thread other than the one that __init__'ed us - # per-thread state may be missing: initialize it if that's the case. - try: - self.tl.last_nonce - except AttributeError: - self.init_per_thread_state() + # Initialize per-thread state, if needed + self.init_per_thread_state() # If we have a saved nonce, skip the 401 if self.tl.last_nonce: r.headers['Authorization'] = self.build_digest_header(r.method, r.url) From 5a69137ac871c38f4597b574e93de07d9ff9655f Mon Sep 17 00:00:00 2001 From: exvito Date: Sat, 4 Apr 2015 14:25:08 +0100 Subject: [PATCH 6/6] Issue #2334 - HTTPDigestAuth - Renamed thread local attribute Per @sigmavirus24 suggestion: private and more readable. --- requests/auth.py | 61 ++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/requests/auth.py b/requests/auth.py index f19f9dff..3268300d 100644 --- a/requests/auth.py +++ b/requests/auth.py @@ -65,25 +65,25 @@ class HTTPDigestAuth(AuthBase): self.username = username self.password = password # Keep state in per-thread local storage - self.tl = threading.local() + self._thread_local = threading.local() def init_per_thread_state(self): # Ensure state is initialized just once per-thread - if not hasattr(self.tl, 'init'): - self.tl.init = True - self.tl.last_nonce = '' - self.tl.nonce_count = 0 - self.tl.chal = {} - self.tl.pos = None - self.tl.num_401_calls = None + if not hasattr(self._thread_local, 'init'): + self._thread_local.init = True + self._thread_local.last_nonce = '' + self._thread_local.nonce_count = 0 + self._thread_local.chal = {} + self._thread_local.pos = None + self._thread_local.num_401_calls = None def build_digest_header(self, method, url): - realm = self.tl.chal['realm'] - nonce = self.tl.chal['nonce'] - qop = self.tl.chal.get('qop') - algorithm = self.tl.chal.get('algorithm') - opaque = self.tl.chal.get('opaque') + realm = self._thread_local.chal['realm'] + nonce = self._thread_local.chal['nonce'] + qop = self._thread_local.chal.get('qop') + algorithm = self._thread_local.chal.get('algorithm') + opaque = self._thread_local.chal.get('opaque') if algorithm is None: _algorithm = 'MD5' @@ -121,12 +121,12 @@ class HTTPDigestAuth(AuthBase): HA1 = hash_utf8(A1) HA2 = hash_utf8(A2) - if nonce == self.tl.last_nonce: - self.tl.nonce_count += 1 + if nonce == self._thread_local.last_nonce: + self._thread_local.nonce_count += 1 else: - self.tl.nonce_count = 1 - ncvalue = '%08x' % self.tl.nonce_count - s = str(self.tl.nonce_count).encode('utf-8') + self._thread_local.nonce_count = 1 + ncvalue = '%08x' % self._thread_local.nonce_count + s = str(self._thread_local.nonce_count).encode('utf-8') s += nonce.encode('utf-8') s += time.ctime().encode('utf-8') s += os.urandom(8) @@ -146,7 +146,7 @@ class HTTPDigestAuth(AuthBase): # XXX handle auth-int. return None - self.tl.last_nonce = nonce + self._thread_local.last_nonce = nonce # XXX should the partial digests be encoded too? base = 'username="%s", realm="%s", nonce="%s", uri="%s", ' \ @@ -165,23 +165,22 @@ class HTTPDigestAuth(AuthBase): def handle_redirect(self, r, **kwargs): """Reset num_401_calls counter on redirects.""" if r.is_redirect: - self.tl.num_401_calls = 1 + self._thread_local.num_401_calls = 1 def handle_401(self, r, **kwargs): """Takes the given response and tries digest-auth, if needed.""" - if self.tl.pos is not None: + if self._thread_local.pos is not None: # Rewind the file position indicator of the body to where # it was to resend the request. - r.request.body.seek(self.tl.pos) - num_401_calls = self.tl.num_401_calls + r.request.body.seek(self._thread_local.pos) s_auth = r.headers.get('www-authenticate', '') - if 'digest' in s_auth.lower() and num_401_calls < 2: + if 'digest' in s_auth.lower() and self._thread_local.num_401_calls < 2: - self.tl.num_401_calls += 1 + self._thread_local.num_401_calls += 1 pat = re.compile(r'digest ', flags=re.IGNORECASE) - self.tl.chal = parse_dict_header(pat.sub('', s_auth, count=1)) + self._thread_local.chal = parse_dict_header(pat.sub('', s_auth, count=1)) # Consume content and release the original connection # to allow our new request to reuse the same one. @@ -199,25 +198,25 @@ class HTTPDigestAuth(AuthBase): return _r - self.tl.num_401_calls = 1 + self._thread_local.num_401_calls = 1 return r def __call__(self, r): # Initialize per-thread state, if needed self.init_per_thread_state() # If we have a saved nonce, skip the 401 - if self.tl.last_nonce: + if self._thread_local.last_nonce: r.headers['Authorization'] = self.build_digest_header(r.method, r.url) try: - self.tl.pos = r.body.tell() + self._thread_local.pos = r.body.tell() except AttributeError: # In the case of HTTPDigestAuth being reused and the body of # the previous request was a file-like object, pos has the # file position of the previous body. Ensure it's set to # None. - self.tl.pos = None + self._thread_local.pos = None r.register_hook('response', self.handle_401) r.register_hook('response', self.handle_redirect) - self.tl.num_401_calls = 1 + self._thread_local.num_401_calls = 1 return r