Add support for synchronous TLS-in-TLS connections.

[karpetrosyan]

Asyncio and Trio support TLS-in-TLS by default, but the standard ssl module does not, so this PR adds TLS-in-TLS implementation on top of the standard ssl module to fully support TLS-in-TLS backends.

TLS-in-TLS implementations are required for all backends in order to support HTTPS proxies.

Closes #721
Refs: #722 #714 #722

Todo

• Add tests for the backends, without TLS.
• Add tests for the backends, with TLS.
• Add failing tests for the backends, with TLS-in-TLS.
• Add TLS-in-TLS implementation.
• Add changelog.

[karpetrosyan]

We will most likely need to write these tests for each backend.

without tls

• test_connect_without_tls
• test_write_without_tls
• test_read_without_tls

with tls

• test_connect_with_tls
• test_write_with_tls
• test_read_with_tls

with tls in tls

• test_connect_with_tls_in_tls
• test_write_with_tls_in_tls
• test_need_with_tls_in_tls

[karpetrosyan]

This tests are simply smoke tests that do not require any assertion; we simply want to know if it runs or not. (maybe excluding read tests)

[karpetrosyan]

Now we have three failed tests, all of which are synchornous tls_in_tls tests that we do not yet support, but adding tls_in_tls support from #714 causes all tests to pass, so I believe it is ready for review.

[karpetrosyan]

Could you please review this? @tomchristie

It's a complicated PR that may be difficult to review, but I believe we need it in 0.18.0.

[tomchristie]

I don't think we need the typing_extensions.Self here. (?)
Can we just have this return NetworkStream.

The override point is close(), not the __enter__/__exit__ which will stay the same even for subclasses.

[karpetrosyan]

If we can inherit, I believe we should always use Self to avoid strange type issues, such as when the instance of SlowNetworkStream is NetworkStream.

[karpetrosyan]

Example:

import typing
from httpcore._backends.base import NetworkStream
from time import sleep

class SlowNetworkStream(NetworkStream):
    
    def read(self, max_bytes: int, timeout: float | None = None) -> bytes:
        sleep(100)

with SlowNetworkStream() as stream:
    reveal_type(stream)

OUTPUT test.py:11: note: Revealed type is "httpcore._backends.base.NetworkStream"

[tomchristie]

Ah okay right.
Could we use the TypeVar style, then?

Eg... in httpx

https://github.com/encode/httpx/blob/76c9cb65f2a159adb764c2236d139f85b46e1506/httpx/_client.py#L60
https://github.com/encode/httpx/blob/76c9cb65f2a159adb764c2236d139f85b46e1506/httpx/_client.py#L1263

Really prefer us avoiding introducing new third party packages wherever possible.

[karpetrosyan]

See https://github.com/encode/httpcore/blob/e31572e0371557d6163d2b7c28676e2b1727673b/httpcore/_backends/base.py

Initially, we used TypeVar, but it was too complicated.

[tomchristie]

Okay, so...

• I'd suggest we drop the typing_extensions here - I don't think we actually need that.
• I'd prefer we drop the operation_start timeout smarts, and just have the timeout apply at the SSL read layer. Yes there's a little bit of scope for a connection timeout to be slightly more relaxed than exactly correct, but I think that's an okay trade-off in order to keep things clean and simple.

Do those two changes seem acceptable to you?

Otherwise looking great. Thanks! 😊

[karpetrosyan]

I'd prefer we drop the operation_start timeout smarts, and just have the timeout apply at the SSL read layer. Yes there's a little bit of scope for a connection timeout to be slightly more relaxed than exactly correct, but I think that's an okay trade-off in order to keep things clean and simple.

If we do that, we will be able to hang out indefinitely even though a timeout has been set; do we want that?

[tomchristie]

If we do that, we will be able to hang out indefinitely even though a timeout has been set; do we want that?

I'm not suggesting that we remove the timeout, I'm suggesting that we keep it nice and simple and just call settimeout() once within that method. (In practice I think that'll actually be perfectly sufficient behaviour)

[karpetrosyan]

I'm not suggesting that we remove the timeout, I'm suggesting that we keep it nice and simple and just call settimeout() once within that method. (In practice I think that'll actually be perfectly sufficient behaviour)

I'm not sure how we can do that 😢 ; could you please provide an example?

[T-256]

I'm not suggesting that we remove the timeout, I'm suggesting that we keep it nice and simple and just call settimeout() once within that method. (In practice I think that'll actually be perfectly sufficient behaviour)

IMO the current behavior is correct.
if we use settimeout once, that it would applies for each socket calls, that is not what we want. we need to set timeout for total calls
.

[tomchristie]

This...

    def _perform_io(
        self,
        func: typing.Callable[..., typing.Any],
        timeout: typing.Optional[float] = None,
    ) -> typing.Any:
        ret = None

        self._sock.settimeout(timeout)

        while True:
            errno = None
            try:
                ret = func()
            except (ssl.SSLWantReadError, ssl.SSLWantWriteError) as e:
                errno = e.errno

            self._sock.sendall(self._outgoing.read())

            if errno == ssl.SSL_ERROR_WANT_READ:
                buf = self._sock.recv(self.TLS_RECORD_SIZE)

                if buf:
                    self._incoming.write(buf)
                else:
                    self._incoming.write_eof()  # pragma: no cover

            if errno is None:
                return ret

The timeouts aren't perfect if we're in TLS-in-TLS, but we probably don't care. (vs the cost of this method being complex to understand.)

[karpetrosyan]

Time problem of that solution is:
If timeout=5 is used, the server can send one byte every 4 seconds, causing us to hang indefinitely.

[T-256]

Time problem of that solution is: If timeout=5 is used, the server can send one byte every 4 seconds, causing us to hang indefinitely.

So, according to current timeout calculation, what kind of timeout it would be?
Do we need new timeout type?

[T-256]

And other question here is if self._sock already set the timeout, shouldn't we revert the actual value?

[karpetrosyan]

So, according to current timeout calculation, what kind of timeout it would be? Do we need new timeout type?

It has already been implemented, and it functions exactly like a standard TLS connection, with write timeouts for write operations, read timeouts for read operations, and so on.

@tomchristie suggests setting the timeout only once, but I'm afraid we won't be able to do so.

One TLS operation, such as read, may necessitate N socket read operations because we must read not only raw bytes, but also TLS layer information and encoded data.

If the socket timeout was only set once, it would be the same for each socket operation, so we need to decrease the timeout based on how long our socket operation takes.

So, if we do this, we may have to wait 50 seconds when the timeout is set to 5 or even hang indefinitely.

[karpetrosyan]

As a result, the problem I described is easily reproducible.

change the conftest.handle_tunnel_connection to this

def handle_tunnel_connection(client_sock: socket.socket) -> None:
    from time import sleep
    with client_sock, socket.create_connection(TLS_ADDRESS) as remote_socket:  # type: ignore
        while True:
            try:
                try:
                    client_sock.settimeout(TUNNEL_READ_WRITE_TIMEOUT)
                    buffer = client_sock.recv(1024)
                    remote_socket.sendall(buffer)
                except socket.timeout:  # pragma: no cover
                    pass

                try:
                    remote_socket.settimeout(TUNNEL_READ_WRITE_TIMEOUT)
                    buffer = remote_socket.recv(1024)

                    while buffer:
                        client_sock.sendall(buffer[:1])
                        buffer = buffer[1:]
                        sleep(1)
                except socket.timeout:  # pragma: no cover
                    pass
            except OSError:
                break

And run pytest -s tests/_backends/test_sync.py::test_read_with_tls_in_tls

The read timeout error should be raised with the current implementation.

But, with @tomchristie's implementation, the test will hang indefinitely..

[T-256]

And other question here is if self._sock already set the timeout, shouldn't we revert the actual value?

@karosis88
I mean, assume the socket before start_tls called, it had already timeout set. but when it goes to nested tls streams, the timeout is changing again and again. does each stream respect other stream timeout?

[karpetrosyan]

And other question here is if self._sock already set the timeout, shouldn't we revert the actual value?

@karosis88 I mean, assume the socket before start_tls called, it had already timeout set. but when it goes to nested tls streams, the timeout is changing again and again. does each stream respect other stream timeout?

Sorry for the delayed response. I don't think we'll have any problems with this because we're updating the timeout before each network operation to ensure that our network operations use the correct timeout. Please correct me if I am wrong.

[T-256]

IMO we should follow implementation in other libraries, we should not apply any changes on sockets as it could have errors in other instances using the same socket.

[karpetrosyan]

IMO we should follow implementation in other libraries, we should not apply any changes on sockets as it could have errors in other instances using the same socket.

I'm not sure what you mean, but if there is an issue, you can describe it and write some simple reproducible code so we can work through it.

[T-256]

I'm not sure what you mean, but if there is an issue, you can describe it and write some simple reproducible code so we can work through it.

I think in this PR the socket's timeout changes over time can cause problems in outer scopes where we set constant value for socket timeout.
But I agree to merge it and then solve those issues by re-set timeouts at those places.

[karpetrosyan]

@encode/maintainers Anyone? :(

[tomchristie]

I've been quietly reviewing this today, and I'm still of the "let's have read/write timeout here have semantics of socket-read/socket-write timeout" not "ssl-operation-read/ssl-operation-write timeout".

However... I'm okay with shelving my judgement on that.

I'm up for merging this and #722 once I'm able to test and verify. If anyone's able to provide a nice example of how to set up a (ideally python based, just because that'd work for me) proxy setup that we can verify TLS-in-TLS support against then that'd be helpful. Otherwise I'll dig into it myself and update the ticket when sorted.

[tomchristie]

I've issued #786 which is a minimal-viable-product alternative to this pull request.

It's a bit atypical in that I'm advocating for a bit of a relaxed approach to testing there, see the description for my motivation there.

[tomchristie]

Resolved in #786. (Credit to @karosis88)