dns
dns copied to clipboard
reactphp
Enable support for DNS over TLS (RFC 7858)
Provide support for DNS servers specified with the tls:// scheme by providing a SSL stream context when creating the TCP socket. Closes #80
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
@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 tls:// scheme for stream_socket_client() performs a blocking TLS handshake, so you'd have to use the tcp:// scheme instead and use stream_socket_enable_crypto() for the TLS handshake. On top of this, we employ a number of work arounds for TLS streams in ReactPHP, for example empty writes are possible due to different TLS and network buffer sizes.
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.
@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 :+1:
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?
@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 TcpTransportExecutor.php. This way it may be easier to test this new class and get the code coverage up to 100%.
What do you think about this?
@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 TcpTransportExecutor.php is, that it will eventually be refactored (because TLS and TCP are two different things) and could then lead to a BC-Break. This is avoidable if we move the TLS part of this change to its own class in this pull request.
As clue also mentioned above, we need to make sure that this is well tested.
...one of the most important aspects of this feature is going to be tests, tests, tests.
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.
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:
Classes: 100.00% (18/18)
Methods: 100.00% (64/64)
Lines: 100.00% (770/770)
Tests and functional changes are looking great :+1:
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.
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 TcpTransportExecutor and a TlsTcpTransportExecutor. That kept me thinking: Maybe we need some kind of Abstraction layer for this. We did a similar thing in socket with its Interfaces.
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 TcpExecutor. Both classes may have some noticeable overlap, but they both also contain somewhat different domain knowledge and different use cases. As such, I would rather keep this separate for now and then work out possible abstractions to avoid any duplication.
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! :+1:
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.
I'm up for it :+1: