Enable support for DNS over TLS (RFC 7858)

[lucasnetau]

https://tools.ietf.org/html/rfc7858

Reworking of previous PR #200, Closes #80

Implement TLS as a layered class extending the TcpTransportExecutor

• TlsTransportExecutor sets up sane defaults for TLS context options and port number (if not provided) and applies workarounds for ancient PHP bugs.
• TlsTransportExecutor overrides handleWritable to initialise TLS on the connection then passes control to TcpTransportExecutor.
• TcpTransportExecutor::$socket visibility is changed to protected so that TlsTransportExecutor can enable crypto.
• TcpTransportExecutor::readChunk and TcpTransportExecutor::writeChunk (added) visibility is protected to allow TlsTransportExecutor to set workarounds for ancient PHP version bugs.
• TcpTransportExecutor support passing in Stream Context (https://www.php.net/manual/en/context.php) parameters via the query part of the nameserver URI.

[lucasnetau]

@clue @SimonFrings I've refactored this patch so that TlsTransportExecutor is basically a wrapper around TcpTransportExecutor with the TLS components added in to the constructor and handleWrite. Everything else relies on TcpTransportExecutors implementation.

I was prompted to rewrite this after a recent reminder in a comment on a ticket in phpstan (phpstan/phpstan-src#2467 (comment)).

Please accept this for inclusion in reactphp/dns.

[lucasnetau]

@clue @SimonFrings any comments?

[SimonFrings]

Hey @lucasnetau, my apologies for the delayed response. We're currently swamped with work, making it challenging to dedicate enough time to properly address these tickets. The good news is, that we're currently in the middle of expanding our team in order to get more things done, so you can expect more frequent answers after September.

I'll do my best to prioritize and review this ticket when time permits (though there are several others in the queue 😅).

[lucasnetau]

@SimonFrings any movement?

[clue]

@lucasnetau Again thank you very much for looking into this and filing this high-quality PR!

I think we all agree that having support for DNS over TLS (DoT) would be fantastic and you've clearly put some effort into this. As much as I love the overall idea, I hate I have to critique some of its implementation details (some of which I've clearly contributed to).

In particular, I can see why the current TlsTransportExecutor implementation extends the existing TcpTransportExecutor. Unfortunately, it looks like an oversight on my end as the TcpTransportExecutor should have been marked final to discourage inheritance and promote composition instead (see #134, #145 and #148). Accordingly, I'm not sure we would want to merge a changeset that takes advantage of this oversight and I would rather work towards a cleaner solution instead.

I understand this will have a noticeable impact on the current implementation, so I'm not exactly sure this is something that can reasonably be changed as part of this PR. That said, at the same time, I hate to suggest closing this one and to ask you to file (yet another) PR.

I'd like to avoid wasting more of your time, so perhaps it makes sense to set up a quick call to discuss implementation approaches in person? If you're interested, you can check my profile to schedule a call.

Thank you for your contribution!

[lucasnetau]

This is entirely frustrating process with artificial blocks and hypothetical what-ifs put in place.

The first PR combined TLS and TCP transport together because TLS is just an additional handshake on top of TCP, it makes sense to have them combined as they are tightly coupled (both in protocol since DoT is TLS+TCP, and in the PHP way of needing to construct the TCP connection first then upgrade the crypto to ensure it is async). In that PR I was requested to pull out the TLS components into a seperate class.

This PR went the inheritance route since TLS only needs to setup sane defaults and override the handleWrite to implement the TLS handshake.

Forcing Composition for this functionality is not possible since the TcpExecutor does not expose the raw socket on which to perform the TLS handshake.

The only remaining thing would be to implement an abstract SocketExecutor which is then extended by Udp, Tcp, and Tls executor code which looks like large amount of refactoring and would still see a heap of code duplication between Tcp and Tls since they share 99% of their implementation.

[SimonFrings]

@lucasnetau I can understand your frustration with this, but we're not trying to intentionally block this, we just want to introduce this into the project the right way. I still think this is a great feature addition and I really appreciate all the work you put into this, so maybe we can revisit this in the future together.