Enable support for DNS over TLS (RFC 7858) #200
Conversation
|
Support for TLS >1.1 in PHP 5 was lacking before 5.6. Should the tests be marked as Skipped for these older versions?. There is no sense in enabling old SSL/TLSv1 support for a new function to provide security for DNS over TLS |
clue
changed the title
|
@lucasnetau Thank you very much for looking into this and addressing #80! This is definitely one of the harder features as fully async TLS support requires a substantial amount of code in PHP. In particular, using the This is all addressed as part of our Socket and Stream components, but we don't currently employ any of this logic in our DNS component to avoid a cyclic dependency (see also #96 and #200). I'd love to have a discussion to see if this cyclic dependency is worth it or whether we should copy the relevant bits and pieces and accept some code duplication. As another alternative, we may also implement this logic in a separate package which might help with the initial design phases until the full feature set is fleshed out. This could be beneficial as the TLS transport reuses the exact same message framing from the TCP/IP transport, so the only thing that would be changed is the initial TLS handshake. Once these design problems are addressed, one of the most important aspects of this feature is going to be tests, tests, tests. I invite you to take a look at the Socket component to see how many tests we have in place to cover common and not so common edge cases. Regarding legacy PHP: I'm not too concerned about legacy PHP support, but I would like to see some tests and documentation in place that cover what is possible and what isn't. For instance, legacy TLS/SSL may work on different PHP versions than newer TLS versions. Let us know if there's anything we can help with 👍 |
|
Hi @clue, That makes sense, I saw the stream_socket_enable_crypto() used in sockets so I'll take a look there. I wanted to keep the DoTLS implementation fairly strict with TLS version from the get go, since this will be a new feature and it makes no sense trying to secure the protocol with an insecure TLS version. In terms on the cyclic dependancy. I also have a working DNS over HTTPS Executor client, however I've used reactphp/https to do the leg work since it doesn't make much sense to copy/paste all the work done there into reactphp/dns. I was thinking it should be added as a 'suggest' in composer.json to make it opt in. |
|
@clue I've re-implemented the TLS setup using stream_socket_enable_crypto() The additional workarounds that you allude to, are these the two at the top of React\Socket\Connection? |
|
@clue I've ported over the various workarounds I found in reactphp/streams and socket to the DNS component. PHP less than 5.6 is not supported. The two major DoT providers Google and CloudFlare only support TLS1.2 and 1.3. Unsure what additional test you are thinking of. There are two tests now, one to check resolving over TLS, and the other handling the case where TLS is attempted against a non-TLS port. The rest of the code flow for TcpTransportExecutor is left as is, and previous tests are not failing. |
@lucasnetau Fine by me. You're already throwing on non-supported versions. We should make sure we also document this int he readme so users aren't caught by surprise. But I'm, very much in favour of doing it this way 👍 |
|
Updated README Updated test cases. HHVM failing on un-related tests to the code that has been modified. testQueryRejectsWhenClientKeepsSendingWhenServerClosesSocketWithoutCallingCustomErrorHandler seems flaky as it doesn't always fail on the HHVM test runs. Please let me know if anything else needs to. be done, or if this PR is ready to have the commit history squashed? |
lucasnetau
changed the title
|
@clue @WyriHaximus Is there anything more that needs to be done with this PR? |
|
Hey @lucasnetau, thanks for the amazing work on this so far 👍 The HHVM tests are currently failing, you can look into it but I don't expect a fix for this if it takes up to much time/work. It's also acceptable if you just skip the tests on HHVM so the overall test suite gets green. Maybe it would make sense to move the TLS part of this change to its own class instead of adding it to the What do you think about this? |
lucasnetau
force-pushed
the
dns-over-tls
branch
from
e774366 to
b7bad73
Compare
|
@SimonFrings I had some time and found the HHVM 3.30.12 Docker container. Both failures were due to HHVM never raising an error when the remote connection was closed. I've put in an extra check to stream_socket_get_name() to reliably detect the remote connection failure. MacOS is failing due to the GitHub blackout for the deprecated MacOS 10.15. I've squashed all commits to one feature commit. This should be ready to merge |
|
@lucasnetau I will look into MacOS, this is also the case for some other repositories, so expect a PR to fix this soon. After that the test suite should be green 👍 The problem with adding this to the As clue also mentioned above, we need to make sure that this is well tested.
To be honest it's an annoying topic but it assures that the added functionality works as expected. If people use ReactPHP in production (we do that too) it would not be safe to take the risk that something could break or isn't working properly. It is part of our identity to be reliable and trustworthy. |
|
For the record: Unrelated test failure for macOS is addressed in #205. |
|
I'm not sure I track on TLS and TCP being two different things in this instance. TLS is a layer on top of a reliable transport, commonly TCP, however DNS over TLS specifically layers on top of TCP (https://datatracker.ietf.org/doc/html/rfc7858). The only difference between a TcpTransportExecutor and a TlsTcpTransportExecutor would be the handshake that occurs at the beginning. I cannot see any reasoning why this would require refactoring at all, nor why refactoring would cause a BC-break. I've added extra test cases to get code coverage to 100% Summary: |
lucasnetau
force-pushed
the
dns-over-tls
branch
from
f054d14 to
7ba22c2
Compare
|
Tests and functional changes are looking great 👍
You're right with what you're saying here, this is more of a design choice. The more a class grows with complexity, the harder it gets to maintain it and it also runs into a higher risk of breaking at some point (tests should prevent that but there's never a 100% certainty). This can be prevented by the suggestion I made in my last comments. I agree with you that there is not much difference between a Don't get me wrong, your suggested changes are looking fine. We just have to keep in mind that this is something that will be part of this package for the next several years (maybe til the end of time itself), plus it also helps other people to contribute on top of your changes. |
|
Thanks @SimonFrings , I don't have much available time in the near future to do much further, so I'm hoping this PR is merged as is. My opinion is abstracting away is overkill for what is essentially a single tasked class. If BC breaks are required, perhaps those can be considered at a time that reactphp project drops support (and all the bug workarounds) for all the unmaintained php/hhvm versions. Might be a better question to ask in a seperate incident, but I have a DNS over HTTPS client based on React\Http\Browser, however as it has this dependancy it cannot be added easily to the reactphp/dns package, however at the same time it can't easily live outside of the package and be included due to the fact that the Factory is final and there is no way to add additional schemes |
|
@lucasnetau I understand that, definitely feels wrong to just throw this away. I am also not the only one to decide over this, I am interested in what @WyriHaximus and @clue have to say about this. |
|
@lucasnetau Thank you for your contribution so far, this definitely looks like a useful addition to the DNS component! 👍 That said, I agree with @SimonFrings that it would be preferable to have this implementation independent of the existing Would love to have this feature at some point, but I don't think it's high priority at the moment. If you can't work on this at the moment, perhaps somebody else can step in and help with this when time allows. Let's also ping @WyriHaximus. Anyway, keep it up! 👍 |
I'm up for it 👍 |
With PHP 8.2 coming out later this year, we should be reading for it's release to ensure all out code works on it. Refs: reactphp/event-loop#258
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here. Refs: reactphp/promise#223
* 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.
|
Closing in favour of #214 |
Provide support for DNS servers specified with the tls:// scheme by providing a SSL stream context when creating the TCP socket. Closes #80