requests icon indicating copy to clipboard operation
requests copied to clipboard

Multiple concurrent client certs broken with v2.32.3

Open jeffreytolar opened this issue 1 year ago • 9 comments

We use requests with multiple mTLS client certificates - each certificate is signed by the same CA, but they have different subjects - each subject has different permissions. Each distinct client cert is used by a different requests.Session Additionally, we make use of ThreadPoolExecutor to make many requests in parallel. When client certs are in use, urllib3 will load the cert into the SSL context, which, with concurrent requests, will cause the shared SSL context to get modified while it's in use.

The reproducer actually fails with an exception - when we first encountered this, we were seeing the wrong certs get used. (That was with different versions of python and openssl, however, and as mentioned above, they're all signed by the same CA, unlike the reproducer)

Expected Result

The reproducer below, when run with requests-2.31.0:

$ ./bin/python3 test.py
/tmp/py/test.py:24: DeprecationWarning: X509Extension support in pyOpenSSL is deprecated. You should use the APIs in cryptography.
  crypto.X509Extension(b"subjectAltName", False, b"DNS:localhost, IP:127.0.0.1")
/tmp/py/test.py:50: DeprecationWarning: ssl.PROTOCOL_TLSv1_2 is deprecated
  self.ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)
Server started on port 8443...
  OK client2: CN: client2 / URL: /client2
  OK client3: CN: client3 / URL: /client3
  OK client1: CN: client1 / URL: /client1

Actual Result

When run with requests-2.32.3:

$ ./bin/python3 test.py
/tmp/py/test.py:24: DeprecationWarning: X509Extension support in pyOpenSSL is deprecated. You should use the APIs in cryptography.
  crypto.X509Extension(b"subjectAltName", False, b"DNS:localhost, IP:127.0.0.1")
/tmp/py/test.py:50: DeprecationWarning: ssl.PROTOCOL_TLSv1_2 is deprecated
  self.ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)
Server started on port 8443...
FAIL client1: HTTPSConnectionPool(host='127.0.0.1', port=8443): Max retries exceeded with url: /client1 (Caused by SSLError(SSLError(116, '[X509: KEY_VALUES_MISMATCH] key values mismatch (_ssl.c:3926)')))
FAIL client3: HTTPSConnectionPool(host='127.0.0.1', port=8443): Max retries exceeded with url: /client3 (Caused by SSLError(SSLError(116, '[X509: KEY_VALUES_MISMATCH] key values mismatch (_ssl.c:3926)')))
  OK client2: CN: client2 / URL: /client2

Reproduction Steps

Gist: https://gist.github.com/jeffreytolar/ea05b3092df12dc6e5b518e58e6821ad ; this generates a few sets of key/certs, hackily sets the default CA bundle, and then makes a few concurrent requests, each using a distinct client cert.

With:

$ pip freeze
certifi==2024.2.2
cffi==1.16.0
charset-normalizer==3.3.2
cryptography==42.0.7
idna==3.7
pycparser==2.22
pyOpenSSL==24.1.0
requests==2.32.3
urllib3==2.2.1

(top level dependencies are pyOpenSSL and requests)

System Information

$ python -m requests.help
{
  "chardet": {
    "version": null
  },
  "charset_normalizer": {
    "version": "3.3.2"
  },
  "cryptography": {
    "version": "42.0.7"
  },
  "idna": {
    "version": "3.7"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.11.8"
  },
  "platform": {
    "release": "5.15.0-105-generic",
    "system": "Linux"
  },
  "pyOpenSSL": {
    "openssl_version": "30200010",
    "version": "24.1.0"
  },
  "requests": {
    "version": "2.32.3"
  },
  "system_ssl": {
    "version": "300000d0"
  },
  "urllib3": {
    "version": "2.2.1"
  },
  "using_charset_normalizer": true,
  "using_pyopenssl": true
}

jeffreytolar avatar May 29 '24 19:05 jeffreytolar

Thanks for the report, @jeffreytolar. It does looks like we're not checking the certs provided by the Session before opting into the default context, I've put together fe251aa94b2b7849e224e455c0aad0df53ad3d8e to disable the default context when certs are present.

However, with testing I'm seeing an exception with the cert being self-signed that wasn't present in 2.31.0. I'm looking into that further but would you mind checking the above patch against your current setup so we can decouple the two issues. If your issue is persisting after we've moved the default context out of the hot path, there may be something else at play with the recent CVE fix.

nateprewitt avatar May 29 '24 20:05 nateprewitt

So far it's looking like that patch is working in our main setup - thanks for the quick commit!

For the self-signed issue still in the reproducer, I think it's that 2.32.x isn't passing a CA bundle to urllib3, whereas 2.31 did that here: https://github.com/psf/requests/blob/147c8511ddbfa5e8f71bbf5c18ede0c4ceb3bba4/requests/adapters.py#L257-L258 ; that causes urllib3 to load the OS default, rather than using certifi

To restore the v2.31 behavior, I think maybe a elif verify is True: pool_kwargs["ca_certs (or ca_cert_dir)"] = extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH) might work in _urllib3_request_context ?

jeffreytolar avatar May 29 '24 22:05 jeffreytolar

So far it's looking like that patch is working in our main setup - thanks for the quick commit!

For the self-signed issue still in the reproducer, I think it's that 2.32.x isn't passing a CA bundle to urllib3, whereas 2.31 did that here: https://github.com/psf/requests/blob/147c8511ddbfa5e8f71bbf5c18ede0c4ceb3bba4/requests/adapters.py#L257-L258 ; that causes urllib3 to load the OS default, rather than using certifi

To restore the v2.31 behavior, I think maybe a elif verify is True: pool_kwargs["ca_certs (or ca_cert_dir)"] = extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH) might work in _urllib3_request_context ?

Yeah, I thought the optimization broke the zipped paths extraction but couldn't prove it easily

sigmavirus24 avatar May 29 '24 23:05 sigmavirus24

Ok, so that's the same issue reported here (https://github.com/psf/requests/pull/6710#issuecomment-2137095349) this morning. That explains why calling load_default_certs on the SSLContext fixes it.

Let me take a closer look tomorrow, I'm a little worried if we do it only for verify is True that we'll start the whole custom SSLContext issue over again. Thanks for pointing that out, @jeffreytolar!

nateprewitt avatar May 29 '24 23:05 nateprewitt

Any updates on a release including a fix for this @nateprewitt ?

jschlyter avatar Sep 05 '24 12:09 jschlyter

We got hit by this bug, it can happen in non-concurrent code. Once a client certificate has been passed, it keeps being used in subsequent calls. Possibly in totally unrelated code and libraries.

res_1 = requests.post(url_1, json=data, cert=cert)
res_2 = requests.post(url_2, json=data)  # <-- subsequent calls keep using `cert` 

My guess is a global state being mutated in the library. It might be related to this line in the 2.32.0 changelog, but we did not investigate further.

  • verify=True now reuses a global SSLContext which should improve request time variance between first and subsequent requests.

Rolling back to requests-2.31.0 fixes it for now.

Thank you for your work!

stackp avatar Feb 03 '25 10:02 stackp

We got hit by this bug, it can happen in non-concurrent code. Once a client certificate has been passed, it keeps being used in subsequent calls. Possibly in totally unrelated code and libraries.

res_1 = requests.post(url_1, json=data, cert=cert) res_2 = requests.post(url_2, json=data) # <-- subsequent calls keep using cert My guess is a global state being mutated in the library. It might be related to this line in the 2.32.0 changelog, but we did not investigate further.

  • verify=True now reuses a global SSLContext which should improve request time variance between first and subsequent requests.

Rolling back to requests-2.31.0 fixes it for now.

Thank you for your work!

I've encountered this bug as well. Here’s what I’ve discovered so far:

In requests-2.31.0, SSLContext was created dynamically: urllib3/connection.py#L755-L765

However, in requests-2.32.3, _preloaded_ssl_context is used instead:

The key issue is that once a client certificate is loaded into _preloaded_ssl_context, it persists for all subsequent requests—even if no certificate is provided. This appears to be an unintended side effect of global state reuse.

To work around this, I implemented a custom HTTPAdapter:

Option 1: Override get_connection_with_tls_context

This prevents unwanted certificate reuse but relies on a deprecated method:

class CustomHTTPAdapter(HTTPAdapter):

    def get_connection_with_tls_context(self, request, verify, proxies=None, cert=None):
        # keep in mind .get_connection() is now deprecated
        return self.get_connection(request.url, proxies)

Option 2: Reset SSLContext

This explicitly resets the SSLContext to avoid persistent client certificates:

class CustomHTTPAdapter(HTTPAdapter):

    @staticmethod
    def _get_reset_ssl_context() -> ssl.SSLContext:
        # Create a fresh SSLContext and load CA certificates
        ssl_context = create_urllib3_context()
        ssl_context.load_verify_locations(
            extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH)
        )
        return ssl_context

    def init_poolmanager(self, *args, **kwargs):
        kwargs['ssl_context'] = self._get_reset_ssl_context()
        return super().init_poolmanager(*args, **kwargs)

    def proxy_manager_for(self, *args, **kwargs):
        kwargs['ssl_context'] = self._get_reset_ssl_context()
        return super().proxy_manager_for(*args, **kwargs)

blueflyingpanda avatar Feb 15 '25 00:02 blueflyingpanda

We also got hit by the bug. in an open source parking data project.

Example code: We expect an 403 at both requests to https://certauth.cryptomix.com/json, as there is no cert in this request. With requests 2.31.0, I get an 403 at both requests. At 2.32.2 or 2.32.3, I get an "found client cert", which means that the client cert was sent. If you have worker services and a data gathering systems where just a few of the data sources use client certs, this ends up into very strange "sometimes it fails" issues.

Here's my isolation:

def main():
    print('sending request without client cert')
    requests.get('https://binary-butterfly.de')

    print('sending request to cert check service')
    response = requests.get('https://certauth.cryptomix.com/json')
    print(f'Status code: {response.status_code}')

    print('sending request with client cert')
    requests.get('https://binary-butterfly.de', cert=('certs/client.crt', 'certs/client.key'))

    print('sending request to cert check service again')
    response = requests.get('https://certauth.cryptomix.com/json')
    print(f'Status code: {response.status_code}')
    if response.status_code == 200:
        print(f'found client cert "{response.json()["SSL_CLIENT_S_DN_O"]}"')

if __name__ == '__main__':
    main()

@blueflyingpanda s solution works (thank you for this!), but means quite some change if you don't use sessions yet (was not necessary so far). Would be great to resolve this in general. Thank you!

the-infinity avatar Feb 21 '25 19:02 the-infinity

The #6767 PR *should* fix that issue when merged. Maybe writing adding some tests to the Requests library to track this issue would be appropriate?

Conobi avatar Jun 03 '25 10:06 Conobi