Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IllegalStateException in DnsClient when DNS server responds with DNAME record #4908

Merged

Conversation

mnylen
Copy link
Contributor

@mnylen mnylen commented Oct 13, 2023

Motivation:

When trying to resolve any record type using DnsClient, if the DNS server replies back with a DNAME record, the handler passed to DnsClient#resolve*() methods is never called. This is because internally, DnsClientImpl uses RecordDecoder which currently throws IllegalStateException if it encounters a record type it does not know how to decode. DnsClientImpl does not catch this exception, causing the query to never be completed.

If you try to do this:

dnsClient.resolveMX("some-domain-with-dname-record.com", System::out::println);

The handler that would print the result is never called, instead this is logged:

WARNING: An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
java.lang.IllegalStateException: Unsupported resource record type [id: DNAME(39)].
    at io.vertx.core.dns.impl.decoder.RecordDecoder.decode(RecordDecoder.java:223)
    at io.vertx.core.dns.impl.DnsClientImpl$Query.handle(DnsClientImpl.java:336)
    at io.vertx.core.dns.impl.DnsClientImpl$1.channelRead0(DnsClientImpl.java:89)
    at io.vertx.core.dns.impl.DnsClientImpl$1.channelRead0(DnsClientImpl.java:83)
    at ...

In this PR, this is fixed by making RecordDecoder throw DecoderException when the DNS server replies back with any record type it doesn't know how to decode. Then, DnsClientImpl is made to catch this and fail the query when this exception is encountered during decoding. Another potential fix would be add DNAME record decoder to the list of supported decoders in RecordDecoder, but even though it would fix this specific issue, I figured it's better to have the try..catch as that would allow handling any other unsupported record types in similar fashion.

Note that DNAME (Delegation Name) records are valid DNS records and are used as a means of redirecting the whole domain (including all of it's subdomains) to another domain. So very similar to CNAME, but with CNAME the alias applies to only single subdomain, whereas with DNAME it applies to the name and all of its subnames. Properly handling these would likely involve following those redirects. This PR does not attempt to do that, it just fixes the issue where the error is not propagated to the caller.

Unfortunately testing this scenario requires extending DnsMessageEncoder from apacheds-protocol-dns in a way I'm not happy about, as the encoder map used by the class is final and also unmodifidable. The class does not provide any usable extension points either, so subclasses would need to rewrite significant portions. I briefly checked the option of upgrading to newer 2.X versions, but the issue remains there as well. I'll welcome any suggestions if there is a better way than outright copying the class to vert.x as I've done here.

There are also potential other issues with DnsClientImpl that I didn't have time to look at too deeply. For example, if the DNS server returns malformed supported record type that the RecordDecoder can't parse, the exception is catched and logged, but RecordDecoder will in this case return null. DnsClientImpl does not check for null value, so there is potential for it to throw NullPointerException in this scenario. This would lead to a similar issue where the handler would never be called. Unfortunately extending FakeDNSServer to support returning malformed records (as it uses standards conforming apacheds-protocol-dns underneath) is not trivial, so fixing this would require either taking a leap of faith with no tests, or perhaps rewriting portions of FakeDNSServer.

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

Currently the DnsMessageEncoder coming from apacheds-protocol-dns does
not support extending the list of encoders, as the list of encoders is
stored in a unmodifiable map set to final.

The DnsMessageEncoder also does not provide any good extension point,
as the method that looks up the encoder is private, and thus can't
be overridden by a subclass.

Thus the only way to do this is to copy the implementation to vert.x
test package, and extend it directly by modifying the source.
The new test times out after a while, because the handler is never
called due to RecordDecoder throwing IllegalStateException.
When no decoder is found for a record type, RecordDecoder now throws DecoderException
instead of IllegalStateException.

The DnsClientImpl then catches this exception and fails the query, as
otherwise the handler is never called.
@mnylen mnylen force-pushed the fix-dns-client-illegal-state-exception branch from 7cefba5 to 419f897 Compare October 13, 2023 19:44
@vietj vietj added this to the 5.0.0 milestone Oct 25, 2023
@vietj vietj merged commit 9e02ae0 into eclipse-vertx:master Nov 6, 2023
6 checks passed
@vietj
Copy link
Member

vietj commented Nov 6, 2023

Merged it will be back-ported to 4.5.0, note there is a reimplement of the
DnsClient in 5.0 with the DnsNameResolver of Netty, that will change the behaviour : DNAME records will be decoded but simply ignored

@vietj
Copy link
Member

vietj commented Nov 6, 2023

thanks for the contribution @mnylen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants