Use Endpoints API for Twisted Connection

Description

Some input from Glyph on the Twisted connection implementation:

Thehigh-level “endpoints” API within Twisted is far more flexible and robust than hard-coding the low-level connectTCP.

For a very basic example, HostnameEndpoint would let you transparently support IPv6 addresses and use the "happy eyeballs" algorithm to establish connections more quickly.

At a higher level, you could use clientFromString to allow users to configure arbitrary endpoints from Twisted's endpoint plugin system, including TLS endpoints with client certificates, Tor connections, SSH tunnels, subprocesses and so on.

Need to look into this and see if there's any benefit to be had, while remaining compatible with the other reactor implementations.

Environment

None

Pull Requests

None

Activity

Show:
Alan Boudreault
May 20, 2020, 9:01 AM

Hey Glyph! We don´t have any plan for this twisted reactor enhancement. You might also be interested to look the reactor code again and see if this ticket is still valid.. I know we improved that reactor some months ago to use TCP4ClientEndpoint/SSL4ClientEndpoint and allow a user to pass its own SSLContext.

https://github.com/datastax/python-driver/blob/master/cassandra/io/twistedreactor.py

Glyph
May 20, 2020, 9:16 AM

Hey Alan;

TCP4ClientEndpoint and SSL4ClientEndpoint, as their names suggest, don’t support IPv6; and the built-in _SSLCreator has a number of issues, some of which are security problems:

  • It forces an old, insecure version of TLS

  • it incorrectly implements hostname verification (hostnames in TLS are stored in subject alternative names, using commonName is deprecated and shouldn’t be relied upon)

  • it relies on the info callback, so upgrades to OpenSSL could easily break or skip its verification

So this ticket remains valid; you should be using HostnameEndpoint and optionsForClientTLS for a variety of reasons. Happily, now that you’re using the endpoint interfaces, this mostly just means deleting code for you :).

Alan Boudreault
May 20, 2020, 10:22 AM
Edited

Indeed, the twisted reactor has some issues at the moment. Unfortunately, I don´t know much twisted myself and I rarely interact with users/customers using it. That´s why this ticket has never been prioritized. It would be nice to get a user contribution

Some more comments:

We can definitively fix this one to automatically select the higher server version. (Like ssl.PROTOCOL_TLS)

True. But we need to preserve the current behavior in 3.x. Would be a good candidate to deprecate the old way and use optionsForClientTLS in the next major.

True. I think this one is only required to support the old way to do hostname verification...

Glyph
May 24, 2020, 5:14 PM

It would be nice to get a user contribution

Sadly I don’t use Cassandra at my day job any more, or I’d love to fix this .

We can definitively fix this one to automatically select the higher server version. (Like ssl.PROTOCOL_TLS)

optionsForClientTLS has a raiseMinimumToso you can specify a minimum version (it will always use the equivalent of PROTOCOL_TLS, you can’t configure that ).

we need to preserve the current behavior in 3.x

Ah, if it’s intentional, that makes sense.

True. I think this one is only required to support the old way to do hostname verification...

Yeah I believe Twisted uses the info callback as well, given OpenSSL’s paucity of reasonable extension points - but we have comprehensive integration testing with lots of versions of dependencies and a commitment to keep it up to date behind the CertificateOptions-based APIs, including optionsForClientTLS. It’s not so much that it’s a bad idea behaviorally as that it’s the sort of interface you need to be tracking carefully.

Alan Boudreault
May 25, 2020, 2:48 AM

Thanks Glyph for your comments. I will mark this for 4.0 so we don't forget to refactor/fix this reactor.

Assignee

Unassigned

Reporter

Adam Holmberg

Fix versions

Labels

None

PM Priority

None

External issue ID

None

Doc Impact

None

Reviewer

None

Size

None

Pull Request

None

Sprint

Py P-DEF

Priority

Minor