Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

With forced TLSv1.3, still TLSv1.0 Client Hello is sent #8478

Closed
1 task done
C0D3D3V opened this issue Jul 2, 2024 · 6 comments
Closed
1 task done

With forced TLSv1.3, still TLSv1.0 Client Hello is sent #8478

C0D3D3V opened this issue Jul 2, 2024 · 6 comments
Labels

Comments

@C0D3D3V
Copy link

C0D3D3V commented Jul 2, 2024

Describe the bug

It looks like https://bbb-lb.uni-paderborn.de only configured TLS1.3. But aiohttp is not able to connect to this site. Requests does the request without problem. In Wireshark I see that aiohttp sends a TLS Client Hello with version 1.0, but requests sends a Client hello with version 1.3. The same is the case for curl and wget, curl sends an TLSv1.3 Client Hello, wget not, and so wget can not connect to the website.

Forcing requests to use TLS1.2 (so that it sends an TLSv1.2 Client hello), then it also can not connect to the server.

Is this considered a bug, or is this expected because the bbb-lb.uni-paderborn.de server is misconfigured?

To Reproduce

Simple Script for testing:

import asyncio

import aiohttp
import requests


async def async_fetch(url):
    async with aiohttp.ClientSession() as session:
        async with session.get(url) as response:
            return await response.text()


def fetch(url):
    response = requests.get(url)
    return response.text


async def main():
    url = "https://bbb-lb.uni-paderborn.de"
    content = fetch(url)
    print(f"Result sync: {content}")
    content = await async_fetch(url)
    print(f"Result async: {content}")


asyncio.run(main())

Expected behavior

Some option to allow aiohttp to connect to such misconfigured servers.

Logs/tracebacks

Connection timeout to host https://bbb-lb.uni-paderborn.de

Python Version

$ python --version
Python 3.12.3

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.9.5
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache 2
Location: /home/daniel/Desktop/other_repos/bbb-download/.venv/lib/python3.12/site-packages
Requires: aiosignal, attrs, frozenlist, multidict, yarl
Required-by: bbb-dl

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.0.5
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /home/daniel/Desktop/other_repos/bbb-download/.venv/lib/python3.12/site-packages
Requires: 
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.9.4
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: /home/daniel/Desktop/other_repos/bbb-download/.venv/lib/python3.12/site-packages
Requires: idna, multidict
Required-by: aiohttp

OS

Arch Linux 6.8.9-arch1-1

Related component

Client

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@C0D3D3V C0D3D3V added the bug label Jul 2, 2024
@C0D3D3V
Copy link
Author

C0D3D3V commented Jul 2, 2024

Mh strange, I thought I tested it, and it did not work, but forcing the ssl context of aiohttp to use tls1.3 worked.

Here is my code if anyone needs it (my sslhelper also allows some other options, but you do not need them for this task):

import asyncio
import os
import ssl
from functools import lru_cache

import aiohttp
import requests
import urllib3
from requests.adapters import HTTPAdapter
from requests.utils import DEFAULT_CA_BUNDLE_PATH, extract_zipped_paths


class SslHelper:
    warned_about_certifi = False

    @classmethod
    def load_default_certs(cls, ssl_context: ssl.SSLContext):
        cert_loc = extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH)

        if not cert_loc or not os.path.exists(cert_loc):
            if not cls.warned_about_certifi:
                print(f"Certifi could not find a suitable TLS CA certificate bundle, invalid path: {cert_loc}")
                cls.warned_about_certifi = True
            ssl_context.load_default_certs()
        else:
            if not os.path.isdir(cert_loc):
                ssl_context.load_verify_locations(cafile=cert_loc)
            else:
                ssl_context.load_verify_locations(capath=cert_loc)

    @classmethod
    @lru_cache(maxsize=16)
    def get_ssl_context(
        cls,
        skip_cert_verify: bool,
        allow_insecure_ssl: bool,
        use_all_ciphers: bool,
        tls_13_min: bool,
    ):
        if not skip_cert_verify:
            ssl_context = ssl.create_default_context(ssl.Purpose.SERVER_AUTH)
            cls.load_default_certs(ssl_context)
        else:
            ssl_context = (
                ssl._create_unverified_context()
            )  # pylint: disable=protected-access

        if allow_insecure_ssl:
            # This allows connections to legacy insecure servers
            # https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_options.html#SECURE-RENEGOTIATION
            # Be warned the insecure renegotiation allows an attack, see:
            # https://nvd.nist.gov/vuln/detail/CVE-2009-3555
            ssl_context.options |= 0x4  # set ssl.OP_LEGACY_SERVER_CONNECT bit
        if use_all_ciphers:
            ssl_context.set_ciphers("ALL")
        if tls_13_min:
            ssl_context.minimum_version = ssl.TLSVersion.TLSv1_3
            ssl_context.maximum_version = ssl.TLSVersion.TLSv1_3

        return ssl_context

    class CustomHttpAdapter(requests.adapters.HTTPAdapter):
        """
        Transport adapter that allows us to use custom ssl_context.
        See https://stackoverflow.com/a/71646353 for more details.
        """

        def __init__(self, ssl_context=None, **kwargs):
            self.ssl_context = ssl_context
            super().__init__(**kwargs)

        def init_poolmanager(self, connections, maxsize, block=False, **pool_kwargs):
            self.poolmanager = urllib3.poolmanager.PoolManager(
                num_pools=connections,
                maxsize=maxsize,
                block=block,
                ssl_context=self.ssl_context,
                **pool_kwargs,
            )

    @classmethod
    def custom_requests_session(
        cls,
        skip_cert_verify: bool,
        allow_insecure_ssl: bool,
        use_all_ciphers: bool,
        tls_13_min: bool,
    ):
        """
        Return a new requests session with custom SSL context
        """
        session = requests.Session()
        ssl_context = cls.get_ssl_context(
            skip_cert_verify, allow_insecure_ssl, use_all_ciphers, tls_13_min
        )
        session.mount("https://", cls.CustomHttpAdapter(ssl_context))
        session.verify = not skip_cert_verify
        return session


async def async_fetch(url):
    async with aiohttp.ClientSession() as session:
        ssl_context = SslHelper.get_ssl_context(False, False, False, True)
        async with session.get(url, ssl=ssl_context) as response:
            return await response.text()

def fetch(url):
    s = SslHelper.custom_requests_session(False, False, False, True)
    # response = requests.get(url)
    response = s.get(url, timeout=10)
    return response.text


async def main():
    url = "https://bbb-lb.uni-paderborn.de"
    content = fetch(url)
    print(f"Result sync: {content}")
    content = await async_fetch(url)
    print(f"Result async: {content}")


# Start the event loop
asyncio.run(main())

@C0D3D3V C0D3D3V closed this as completed Jul 2, 2024
@C0D3D3V
Copy link
Author

C0D3D3V commented Jul 2, 2024

Ufff, this only works because I first make the request via requests (because I used the same cached context for both). If you comment out

content = fetch(url)
print(f"Result sync: {content}")

it does not work

@C0D3D3V C0D3D3V reopened this Jul 2, 2024
@C0D3D3V C0D3D3V changed the title Maybe TLS1.3 bug With forced TLSv1.3, still TLSv1.0 Client Hello is send Jul 2, 2024
@C0D3D3V C0D3D3V changed the title With forced TLSv1.3, still TLSv1.0 Client Hello is send With forced TLSv1.3, still TLSv1.0 Client Hello is sent Jul 2, 2024
@C0D3D3V
Copy link
Author

C0D3D3V commented Jul 2, 2024

Simplified test script, with forced tlsv1.3

import asyncio
import os
import ssl

import aiohttp
import requests
import urllib3
from requests.adapters import HTTPAdapter
from requests.utils import DEFAULT_CA_BUNDLE_PATH, extract_zipped_paths


class SslHelper:

    @classmethod
    def load_default_certs(cls, ssl_context: ssl.SSLContext):
        cert_loc = extract_zipped_paths(DEFAULT_CA_BUNDLE_PATH)

        if not cert_loc or not os.path.exists(cert_loc):
            ssl_context.load_default_certs()
        else:
            if not os.path.isdir(cert_loc):
                ssl_context.load_verify_locations(cafile=cert_loc)
            else:
                ssl_context.load_verify_locations(capath=cert_loc)

    @classmethod
    def get_ssl_context(
        cls,
    ):
        ssl_context = ssl.create_default_context(ssl.Purpose.SERVER_AUTH)
        cls.load_default_certs(ssl_context)

        ssl_context.minimum_version = ssl.TLSVersion.TLSv1_3
        ssl_context.maximum_version = ssl.TLSVersion.TLSv1_3

        return ssl_context

    class CustomHttpAdapter(requests.adapters.HTTPAdapter):
        def __init__(self, ssl_context=None, **kwargs):
            self.ssl_context = ssl_context
            super().__init__(**kwargs)

        def init_poolmanager(self, connections, maxsize, block=False, **pool_kwargs):
            self.poolmanager = urllib3.poolmanager.PoolManager(
                num_pools=connections,
                maxsize=maxsize,
                block=block,
                ssl_context=self.ssl_context,
                **pool_kwargs,
            )


async def async_fetch(url):
    async with aiohttp.ClientSession() as session:
        ssl_context = SslHelper.get_ssl_context()
        async with session.get(url, ssl=ssl_context) as response:
            return await response.text()


async def main():
    url = "https://bbb-lb.uni-paderborn.de"
    content = await async_fetch(url)
    print(f"Result async: {content}")

asyncio.run(main())

@C0D3D3V
Copy link
Author

C0D3D3V commented Jul 4, 2024

After digging deeper, I found out, that the version fields are not the problem. Sorry for bothering you.

@C0D3D3V C0D3D3V closed this as completed Jul 4, 2024
@C0D3D3V
Copy link
Author

C0D3D3V commented Jul 4, 2024

For anyone who is interested, the problem is that no ALPN extension is used by aiohttp, thats why the (haproxy?) reverse proxy used by the university ignored the client hello. The requests library and curl default uses the ALPN extension. If you use the --no-alpn flag on curl it can also not connect.

requests uses urllib3, that automatically sets the alpn correct, see https://github.com/search?q=repo%3Aurllib3%2Furllib3%20set_alpn_protocols&type=code

If anyone needs to fix this too: We can get the same behavior if you set the alpn option in your ssl context like this: ssl_context.set_alpn_protocols(['http/1.1'])

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jul 4, 2024

As far as I can see, ALPN is initiated by a client. So, it's a bug in the proxy that it won't work without ALPN at all.

I wonder if it's worth setting this by default though? I'm not clear exactly what tradeoffs this might make, but from a quick read, I think the benefit is faster connection as the protocol is decided up-front without an extra roundtrip, while the drawback is advertising the protocol in cleartext (but, that doesn't really appear to be a security issue if we're just advertising HTTP versions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants