Concurrency issue in generate_request_header

[mkomitee]

In applications which use the same HTTPKerberosAuth object across multiple threads, there's a concurrency issue. Here's a reproducer:

import requests
import threading
import requests_kerberos

REQUESTS_PER_THREAD = 128
THREADS = 8
URL = 'http://example.com/'

KERB = requests_kerberos.HTTPKerberosAuth(requests_kerberos.DISABLED)

def worker():
    for _ in range(REQUESTS_PER_THREAD):
        response = requests.get(URL, auth=KERB)
        response.raise_for_status()

def main():
    threads = []
    for _ in range(THREADS):
        t = threading.Thread(target=worker)
        threads.append(t)
        t.start()

    for t in threads:
        t.join()

if __name__ == '__main__':
    main()

Eventually (and it won't take long at all) you'll see HTTPError: 401 Client Error: Unauthorized for url: http://example.com exceptions.

FYI, I've been able to reproduce it with as few as 2 threads and 1 request per thread, but set them to 8/128 to make it more reliable.

The issue stems from how we cache the context by urlparse(request.url).hostname in generate_request_header, and then always access it via that cache. If two threads are making concurrent requests to the same host, they'll stomp on one another, and kerberos.authGSSClientResponse will be called using a context that hasn't been used in a kerberos.authGSSClientStep invocation. The result is None. We then encode the header as "Negotiate None" which is invalid from the servers perspective.

One solution is to bind the context to a name local to the function and use that instead of the cached value, and that will fix client authentication, but mutual authentication -- the reason we cache the context objects -- will remain broken, and mutual authentication will fail if OPTIONAL or REQUIRED.

We can solve this by instead, caching the context in generate_request_header indexed by request (response.request), and then extracting it in authenticate_server using response.request. I'm not 100% sure, but we may also need to look through all of the requests in response.history to find the "right" one.

The problem with this approach, is that it will break winrm support, but the winrm functionality probably doesn't belong in here anyway, and by exposing a function to extract the context given a response object should be enough for the winrm users to implement that functionality themselves. I have a patch for this but it may be a bit before I can actually submit it since I'm using an older version of requests_kerberos which uses an older version of kerberos which doesn't work with the requests_kerberos on master.

[mkomitee]

I've verified that we don't need to look through the history, we can safely index the context dictionary by PreparedRequest.

To solve the winrm feature we can do one of two things:

1. instruct users that they should extract the established context from the auth object using auth_object.context[response.request]
2. in handle_other() add setattr(response, 'requests_kerberos_context', self.context.get(response.request)) and instruct users they should be able to use response.requests_kerberos_context.

[mdziemianko]

Hi, I just hit this problem while investigating issue with Elasticsearch client using requests-kerberos as a connection class. Is there any chance the PR with the fix being merged? I see it's been lingering for a few months now... happy to test it in practice:)