HTTPDigestAuth on session not thread-safe

HTTPDigestAuth is not thread-safe when used on a session object. This script demonstrates the problem:

``` python
import Queue
import sys
import threading

import requests

s = requests.Session()
s.auth = requests.auth.HTTPDigestAuth('user', 'passwd')

def tprint(astr):
    t = threading.current_thread().name
    sys.stdout.write('%s: %s\n' % (t, astr))

def worker():
    resp = s.get('http://httpbin.org/digest-auth/auth/user/passwd')
    tprint(resp.status_code)

t1 = threading.Thread(target=worker)
t1.start()

t2 = threading.Thread(target=worker)
t2.start()

t1.join()
t2.join()
```

With debugging tprint()s added to auth.HTTPDigestAuth.handle_401(), the script generates output like this:

```
$ python threads_digest_1.py 
Thread-1: handle_401: num_401_calls: 1
Thread-1: handle_401: s_auth: Digest qop=auth, nonce="929169e53bab111e1c06e7a5207fe63b", realm="me@kennethreitz.com", opaque="3e5078f698656ced77637bf72881e06b"
Thread-2: handle_401: num_401_calls: 2
Thread-2: handle_401: s_auth: Digest qop=auth, nonce="f9bf0aa7be1fb59e85cbe0000f67a56a", realm="me@kennethreitz.com", opaque="b0d6dee4bc0030c5bdefc2c76342479d"
Thread-2: 401
Thread-1: 200
$
```


Session's aren't threadsafe so I'm not sure I understand why this is suprising.


This is surprising because "thread-safe" is listed as one of the features of Requests, and there is nothing in the documentation to indicate that parts of it are not thread-safe.


+1 with achorrath, I faced this issue and it's been quite hard to tackle down. Either clearly state that requests is not thread-safe from the documentation or provide a thread-safe session object.

Thanks.


So the session isn't the problem here it's the digest instance. There's no good way for the digest auth instance to be thread safe when it doesn't have the same context that a session would have.


Hi,

Before anything, here's a thanks for a great project to the authors and contributors. :)

Chiming in to share my experience which I believe is directly related to this case.
- I faced what I believe to be the same underlying issue while working with multi-threaded NTLM authentication.
- Seeing that the NTLM authenticator was not reusing connections correctly, I raised PR https://github.com/requests/requests-ntlm/pull/57 against requests-ntlm.
- While testing harder and digging deeper I found out that my PR improved things, but was not enough to ensure correct multi-threaded NTLM authentication behaviour.
- Given that NTLM does authentication on a per-connection basis, just like Digest auth, I suspected I would see the same types of failures with a simple multi-threaded Digest auth client.
- I created one and reproduced the issue.
- I came here looking for information: that's when I found this issue.

My suspicion about the underlying cause:
- In `handle_401`, the Digest authenticator does a `r.connection.send(...)` to "re-send" the original request with the computed digest header in-place.
- `r.connection.send` is, in fact, `HTTPAdapter.send` (in the example above, could be something else with a different adapter).
- `HTTPAdapter.send` does a `conn = self.get_connection(...)`.
- This ends up calling `PoolManager.connection_from_url(...)`.
- This is the culprit: the PoolManager has no way of knowing how to return the correct connection given the fact that its argument is just the url.

My suspicion about a possible fix:
(note: I'm not very familiar with the existing codebase. I may be making some mistakes and I'll gladly have them pointed out by someone.)
- If my reasoning above is correct, the only way to ensure correct multi-threaded per-connection authentication at the requests API level is to somehow allow `handle_401` to "re-send" the request over the same connection.
- This may require a different API given I don't think `HTTPAdapter.send`'s arguments may not comprise enough information so as to ensure the same connection is used.
- Is this currently possible in any way? I don't think so (again, I'm not very familiar wth urllib3 API and innards). Thus an eventual fix may need to adapt / improve (while keeping backwards compatilibity, of course) the requests/urllib3 API and eventually, urllib3 internal APIs.

Again thanks in advance for any helpful input.


So, we can do this if we _really_ want to by pulling the raw `httplib` connection object out and using it. That feels really nasty.


Update, correcting myself:
- Went reading RFC2617.
- Figured out that my understanding about Digest auth was wrong: Digest auth does not authenticate connections (contrarily to NTLM), it does authenticate requests.
- Given this, please disregard my considerations above concerning HTTPAdapters, Pools, etc.
- Digged deeper into this particular case:
  - The culprit seems to be the fact that `HTTPDigestAuth.handle_401` only produces the `Authorization` header if it has been called less than 2 times (see ...`and num_401_calls < 2:` around line 172 in auth.py)
  - With two near-simultaneous threads, the first one entering `handle_401` will correctly produce the necessary auth headers, the second one will not.
- My hack test:
  - Commented out the `and num_401_calls < 2` in auth.py.
  - Ran a variation of the test above successfully with 2, 20 and 200 threads (with the original code, I would only get 50% of the requests authenticated).
- Possible fix:
  - The underlying `and num_401_calls < 2` seems to stem from #547 which, rightfully so, tries to prevent infinite auth failure loops. This means throwing the 401 call count away would lead to regressions.
  - A primitive approach would count 401 calls on a per-thread basis: not sure if feasible, not sure if elegant.

Here are my 2c.

PS: @Lukasa, given this, your comment about the (agreed, really nasty) possibility of pulling the raw `httplib` connection object would not apply here, but maybe feasible (at least for quick-hack tests) in https://github.com/requests/requests-ntlm. However, I'm not sure how to grab that object: any pointer? (lest's move this discussion away from this issue, shall we?) :)

More PS: For completeness, I dump my test code (easier to raise up thread count)

``` python
#!/usr/bin/env python
# ------------------------------------------------------------------------------
# vim: tabstop=8 expandtab shiftwidth=4 softtabstop=4
# ------------------------------------------------------------------------------

import logging
import threading

import requests


class ClientThread(threading.Thread):

    def __init__(self, session, url, *args, **kwargs):
        threading.Thread.__init__(self, *args, **kwargs)
        self.log = logging.getLogger('client.%s' % self.name)
        self.session = session
        self.url = url
        self.success = None

    def run(self, *args, **kwargs):
        self.response = self.session.get(self.url)
        self.log.info('self.response.status_code=%r' % self.response.status_code)
        self.log.debug('self.response.text=%r', self.response.text)
        self.success = self.response.status_code == requests.codes.ok


if __name__ == '__main__':

    LOG_LEVEL = logging.INFO

    N_THREADS = 2

    URL = 'http://httpbin.org/digest-auth/auth/user/pass'
    AUTH_USERNAME = 'user'
    AUTH_PASSWORD = 'pass'

    logging.basicConfig(level=LOG_LEVEL)

    session = requests.session()
    session.auth = requests.auth.HTTPDigestAuth(AUTH_USERNAME, AUTH_PASSWORD)

    threads = [ClientThread(session, URL) for t in range(N_THREADS)]
    for t in threads:
        t.start()
    for t in threads:
        t.join()
    n_success = len([t.success for t in threads if t.success])

    log = logging.getLogger('client')
    log_level = logging.WARN if n_success!=N_THREADS else logging.INFO
    log.log(log_level, 'success rate %i/%i', n_success, N_THREADS)
```


Update:
- I submited PR #2523 with a possible solution based on a per-thread num_401_calls count.
- @achorrath , @vincentxb : Can you give it a run and confirm correct multi-threaded operation?


Greetings all. Thanks for taking care of this @exvito. My only concern is about nonce and nonce_count values in a multi-threaded context. More generally, how do you deal with two threads being simultaneously in the handle_401() method?


@vincentxb : thanks for your feedback. You are correct. @tardyp made the same comment in #2523. I'll go ahead and extend the per-thread storage to the other state attributes.


Anything holding back these patches? 'm using a local patchset for requests_ntlm in production to work around the connection recycle problem (see https://github.com/requests/requests-ntlm/pull/51) but I would prefer to use the official repo.


This has been fixed for almost a year now.