TLS connections should not "succeed" when certificate verification fails

[glyph]

Consider this example:

from typing import Any

from twisted.internet.endpoints import HostnameEndpoint, wrapClientTLS
from twisted.internet.protocol import Factory, Protocol
from twisted.internet.ssl import optionsForClientTLS
from twisted.internet.task import react


@react
async def main(reactor: Any) -> None:
    print("connecting")
    proto = await wrapClientTLS(
        optionsForClientTLS("expired.badssl.com"),
        HostnameEndpoint(reactor, "expired.badssl.com", 443),
    ).connect(Factory.forProtocol(Protocol))
    print("connected", proto)

On current Twisted, this happily gives you a "connected" protocol, despite the fact that the certificate has not been verified yet.

This is not correct. It should fail with a service identity verification error.

cc @exarkun @adiroiban re: #12074

[exarkun]

I think I agree with something like this issue but I'm not sure exactly what. Recall that twisted.internet.interfaces.IHandshakeListener exists and its handshakeCompleted will only be called after the handshake completes (successfully). I believe this interface was introduced as a way to expose the _real TLS connection established event (feel free to sprinkle scare quotes over whichever of those words you like) while also preserving the existing behavior of calling connectionMade as soon as the transport below TLS yielded a connection.

Backwards compatibility is great and remains one of Twisted's strengths but it is also true that this is an unfortunate stumbling block for using TLS in Twisted (something that is otherwise more pleasant than is the case when using many other libraries).

The introduction of wrapClientTLS would probably have been a great time to also introduce a transport behavior change such that connectionMade got called only when handshakeCompleted would be called. Given that didn't happen, what's the right path from here?

• Change the foundational TLS support in Twisted to call connnectionMade only when it now calls handshakeCompleted? Quite backwards incompatible, though it probably does what the majority of users want.
• Change only wrapClientTLS in this way? Also incompatible. Also introduces a divergence in behavior between the two layers which is pretty gross.
• Introduce a new API like wrapClientTLS but with the new behavior? Deprecate wrapClientTLS to get everyone on to it? Technically a nice compatibility story there, except it does require that everyone change their code.
• Deprecate support for protocols that don't implement IHandshakeListener in wrapClientTLS and assume that if developers are forced to implement that in their protocol, they'll also make correct choices about which events to handle to make their apps correct? This actually lets at least some folks who are already doing it right off the hook with no work, while only making other folks change their probably-broken code. Kinda cool. Except it might be annoying to implement since you can't see the protocol until somewhat late in the process. Also, part of the promise of endpoints is transport agnosticism, but now you are forced to put TLS details into your protocol and it's no longer transport agnostic.
• Push the handshake notification into a different part of the interface somehow? What if wrapClientTLS took another argument that got called when the handshake succeeded and ... I dunno, did something? I'm not sure right now what it could do that would help. Maybe something other than a callable would work?
• ... ?

[adiroiban]

I have never used the wrapClientTLS API.

Maybe have an API like wrapClientTLS(connectionCreator, wrappedEndpoint, waitHandshake=True)

The signature can be wrapClientTLS(connectionCreator, wrappedEndpoint, waitHandshake=None)
If waitHandshake is None we can raise a deprecation warning that in the future this will wait for handshake, and handled it as waitHandshake=False that is, for now, don't wait for handshake.

Later we can remove the deprecationg warning and have the signature as wrapClientTLS(connectionCreator, wrappedEndpoint, waitHandshake=True)

---

I am happy to have a better IHandshakeListener, but I am not sure how it should be.

Below is my boilerplate code that I use for any TLS connection.
I have the waitHandshakeCompleted, but it would be nice to have an API like handshakeCompleted() that is called when handshake is done.... so that I don't have to explicitly wait for handshake.

But, it's important to forward the TLS error to the connectionLost() API.

from typing import Any

from twisted.internet import defer
from twisted.internet.endpoints import HostnameEndpoint, wrapClientTLS
from twisted.internet.interfaces import ISSLTransport, IHandshakeListener
from twisted.internet.protocol import Factory, Protocol
from twisted.internet.ssl import optionsForClientTLS
from twisted.internet.task import react
from zope.interface.declarations import implementer


@implementer(IHandshakeListener)
class ClientProtocol(Protocol):

    def __init__(self):
        self._tls_handshake_completed = defer.Deferred()

    def handshakeCompleted(self):
        self._tls_handshake_completed.callback(None)

    def connectionLost(self, reason):
        self._tlsConnectionLost(reason)

    def waitHandshakeCompleted(self):
        return self._tls_handshake_completed

    def _tlsConnectionLost(self, failure):
        if not self._tls_handshake_completed:
            return
        if self._tls_handshake_completed.called:
            return

        # Once the connection is lost, there shouldn't be any other
        # process waiting for the deferred, so it's safe to ignore the error,
        # that we are triggering here.
        # We push the error here to trigger emitting the events.
        self._tls_handshake_completed.addErrback(lambda f: None)
        self._tls_handshake_completed.errback(failure)


@react
async def main(reactor: Any) -> None:
    print("connecting")
    proto = await wrapClientTLS(
        optionsForClientTLS("expired.badssl.com"),
        HostnameEndpoint(reactor, "expired.badssl.com", 443),
    ).connect(Factory.forProtocol(ClientProtocol))
    print("connected", proto)
    await proto.waitHandshakeCompleted()

[glyph]

Maybe have an API like wrapClientTLS(connectionCreator, wrappedEndpoint, waitHandshake=True)

Possibly the right signature here would be simply secureHostnameEndpoint to avoid the duplication here between optionsForClientTLS and HostnameEndpoint hostname parameters, which must, after all, match.

Basically, put these lines into their own public function, rather than hiding them behind clientFromString:

twisted/src/twisted/internet/endpoints.py

Lines 2294 to 2304 in 88151eb

	return wrapClientTLS(
	optionsForClientTLS(
	host,
	trustRoot=_parseTrustRootPath(trustRoots),
	clientCertificate=_privateCertFromPaths(certificate, privateKey),
	),
	clientFromString(reactor, endpoint)
	if endpoint is not None
	else HostnameEndpoint(reactor, _idnaBytes(host), port, timeout, bindAddress),
	)

[glyph]

The introduction of wrapClientTLS would probably have been a great time to also introduce a transport behavior change such that connectionMade got called only when handshakeCompleted would be called. Given that didn't happen, what's the right path from here?

The formal compatibility policy that Twisted attempts to guarantee is not "all behavior is the same", but "we aren't going to make public names start raising exceptions (particularly TypeError and AttributeError) without giving you a soft warning first".

Any code making a TLS connection with an endpoint already has to handle failures from .connect. Pushing errors forward in the interface is an incompatible change, but it's one where it cannot really break a correctly-written program. So I think that we can make this behavior change without a lot of concern over forward compatibility.

This covers the developer-facing part of the process, but there is also an operational concern. We do have other facilities we can play with in order to allow users to roll back the behavior if it causes issues for them, e.g. an environment variable.

That said, I do think that we are missing something like secureHostnameEndpoint anyway, and a convenient new API that offers other benefits might be a good way to introduce this as a default.

It does seem like adding a new waitForHandshake parameter to wrapClientTLS would be straightforward enough just to get the initial implementation done though, since there's probably enough work there to keep someone busy for a little while. Once that exists, we could separately consider a strategy for evolving the defaults with a smooth compatibility transition.