From 68dbc7703a8249f4fe392d4a12156b4fc398bfe1 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 14 Aug 2020 16:02:53 +0200 Subject: [PATCH] Correctly limit queries done to resolve unresolved nameservers (#10478) Motivation: We need limit the number of queries done to resolve unresolved nameserver as otherwise we may never fail the resolve when there is a missconfigured dns server that lead to a "resolve loop". Modifications: - When resolve unresolved nameservers ensure we use the allowedQueries as max upper limit for queries so it will eventually fail - Add unit tests Result: No more possibility to fail in a loop while resolve. This is related to https://github.com/netty/netty/issues/10420 --- .../dns/DnsAddressResolveContext.java | 8 +-- .../netty/resolver/dns/DnsNameResolver.java | 6 ++- .../resolver/dns/DnsRecordResolveContext.java | 14 ++--- .../netty/resolver/dns/DnsResolveContext.java | 19 ++++--- .../resolver/dns/DnsNameResolverTest.java | 52 +++++++++++++++++++ 5 files changed, 80 insertions(+), 19 deletions(-) diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java index d802c0c08b..df7b52b491 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsAddressResolveContext.java @@ -34,11 +34,11 @@ final class DnsAddressResolveContext extends DnsResolveContext { DnsAddressResolveContext(DnsNameResolver parent, Promise originalPromise, String hostname, DnsRecord[] additionals, - DnsServerAddressStream nameServerAddrs, DnsCache resolveCache, + DnsServerAddressStream nameServerAddrs, int allowedQueries, DnsCache resolveCache, AuthoritativeDnsServerCache authoritativeDnsServerCache, boolean completeEarlyIfPossible) { super(parent, originalPromise, hostname, DnsRecord.CLASS_IN, - parent.resolveRecordTypes(), additionals, nameServerAddrs); + parent.resolveRecordTypes(), additionals, nameServerAddrs, allowedQueries); this.resolveCache = resolveCache; this.authoritativeDnsServerCache = authoritativeDnsServerCache; this.completeEarlyIfPossible = completeEarlyIfPossible; @@ -49,9 +49,9 @@ final class DnsAddressResolveContext extends DnsResolveContext { String hostname, int dnsClass, DnsRecordType[] expectedTypes, DnsRecord[] additionals, - DnsServerAddressStream nameServerAddrs) { + DnsServerAddressStream nameServerAddrs, int allowedQueries) { return new DnsAddressResolveContext(parent, originalPromise, hostname, additionals, nameServerAddrs, - resolveCache, authoritativeDnsServerCache, completeEarlyIfPossible); + allowedQueries, resolveCache, authoritativeDnsServerCache, completeEarlyIfPossible); } @Override diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java index 62cce5ec63..cf485903d1 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsNameResolver.java @@ -816,7 +816,8 @@ public class DnsNameResolver extends InetNameResolver { // It was not A/AAAA question or there was no entry in /etc/hosts. final DnsServerAddressStream nameServerAddrs = dnsServerAddressStreamProvider.nameServerAddressStream(hostname); - new DnsRecordResolveContext(this, promise, question, additionals, nameServerAddrs).resolve(promise); + new DnsRecordResolveContext(this, promise, question, additionals, nameServerAddrs, maxQueriesPerResolve) + .resolve(promise); return promise; } @@ -1058,7 +1059,8 @@ public class DnsNameResolver extends InetNameResolver { final DnsServerAddressStream nameServerAddrs = dnsServerAddressStreamProvider.nameServerAddressStream(hostname); new DnsAddressResolveContext(this, originalPromise, hostname, additionals, nameServerAddrs, - resolveCache, authoritativeDnsServerCache, completeEarlyIfPossible) + maxQueriesPerResolve, resolveCache, + authoritativeDnsServerCache, completeEarlyIfPossible) .resolve(promise); } diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java index ed88ae7492..97f4d9b470 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsRecordResolveContext.java @@ -28,17 +28,18 @@ import io.netty.util.concurrent.Promise; final class DnsRecordResolveContext extends DnsResolveContext { DnsRecordResolveContext(DnsNameResolver parent, Promise originalPromise, DnsQuestion question, - DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs) { + DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs, int allowedQueries) { this(parent, originalPromise, question.name(), question.dnsClass(), new DnsRecordType[] { question.type() }, - additionals, nameServerAddrs); + additionals, nameServerAddrs, allowedQueries); } private DnsRecordResolveContext(DnsNameResolver parent, Promise originalPromise, String hostname, int dnsClass, DnsRecordType[] expectedTypes, DnsRecord[] additionals, - DnsServerAddressStream nameServerAddrs) { - super(parent, originalPromise, hostname, dnsClass, expectedTypes, additionals, nameServerAddrs); + DnsServerAddressStream nameServerAddrs, + int allowedQueries) { + super(parent, originalPromise, hostname, dnsClass, expectedTypes, additionals, nameServerAddrs, allowedQueries); } @Override @@ -46,9 +47,10 @@ final class DnsRecordResolveContext extends DnsResolveContext { String hostname, int dnsClass, DnsRecordType[] expectedTypes, DnsRecord[] additionals, - DnsServerAddressStream nameServerAddrs) { + DnsServerAddressStream nameServerAddrs, + int allowedQueries) { return new DnsRecordResolveContext(parent, originalPromise, hostname, dnsClass, - expectedTypes, additionals, nameServerAddrs); + expectedTypes, additionals, nameServerAddrs, allowedQueries); } @Override diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java index 31d17e54b1..28212547ce 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java @@ -88,7 +88,6 @@ abstract class DnsResolveContext { private final String hostname; private final int dnsClass; private final DnsRecordType[] expectedTypes; - private final int maxAllowedQueries; final DnsRecord[] additionals; private final Set>> queriesInProgress = @@ -102,7 +101,7 @@ abstract class DnsResolveContext { DnsResolveContext(DnsNameResolver parent, Promise originalPromise, String hostname, int dnsClass, DnsRecordType[] expectedTypes, - DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs) { + DnsRecord[] additionals, DnsServerAddressStream nameServerAddrs, int allowedQueries) { assert expectedTypes.length > 0; this.parent = parent; @@ -113,8 +112,7 @@ abstract class DnsResolveContext { this.additionals = additionals; this.nameServerAddrs = requireNonNull(nameServerAddrs, "nameServerAddrs"); - maxAllowedQueries = parent.maxQueriesPerResolve(); - allowedQueries = maxAllowedQueries; + this.allowedQueries = allowedQueries; } static final class DnsResolveContextException extends RuntimeException { @@ -156,7 +154,7 @@ abstract class DnsResolveContext { String hostname, int dnsClass, DnsRecordType[] expectedTypes, DnsRecord[] additionals, - DnsServerAddressStream nameServerAddrs); + DnsServerAddressStream nameServerAddrs, int allowedQueries); /** * Converts the given {@link DnsRecord} into {@code T}. @@ -258,7 +256,8 @@ abstract class DnsResolveContext { void doSearchDomainQuery(String hostname, Promise> nextPromise) { DnsResolveContext nextContext = newResolverContext(parent, originalPromise, hostname, dnsClass, - expectedTypes, additionals, nameServerAddrs); + expectedTypes, additionals, nameServerAddrs, + parent.maxQueriesPerResolve()); nextContext.internalResolve(hostname, nextPromise); } @@ -488,8 +487,13 @@ abstract class DnsResolveContext { DnsCache resolveCache = resolveCache(); if (!DnsNameResolver.doResolveAllCached(nameServerName, additionals, resolverPromise, resolveCache, parent.resolvedInternetProtocolFamiliesUnsafe())) { + new DnsAddressResolveContext(parent, originalPromise, nameServerName, additionals, - parent.newNameServerAddressStream(nameServerName), resolveCache, + parent.newNameServerAddressStream(nameServerName), + // Resolving the unresolved nameserver must be limited by allowedQueries + // so we eventually fail + allowedQueries, + resolveCache, redirectAuthoritativeDnsServerCache(authoritativeDnsServerCache()), false) .resolve(resolverPromise); } @@ -964,6 +968,7 @@ abstract class DnsResolveContext { } // No resolved address found. + final int maxAllowedQueries = parent.maxQueriesPerResolve(); final int tries = maxAllowedQueries - allowedQueries; final StringBuilder buf = new StringBuilder(64); diff --git a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java index e3f5a07a7d..f15df51e74 100644 --- a/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java +++ b/resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java @@ -1768,6 +1768,58 @@ public class DnsNameResolverTest { } } + @Test + public void testNsLoopFailsResolveWithAuthoritativeDnsServerCache() throws Exception { + testNsLoopFailsResolve(new DefaultAuthoritativeDnsServerCache()); + } + + @Test + public void testNsLoopFailsResolveWithoutAuthoritativeDnsServerCache() throws Exception { + testNsLoopFailsResolve(NoopAuthoritativeDnsServerCache.INSTANCE); + } + + private void testNsLoopFailsResolve(AuthoritativeDnsServerCache authoritativeDnsServerCache) throws Exception { + final String domain = "netty.io"; + final String ns1Name = "ns1." + domain; + final String ns2Name = "ns2." + domain; + + TestDnsServer testDnsServer = new TestDnsServer(new HashSet( + Collections.singletonList(domain))) { + + @Override + protected DnsMessage filterMessage(DnsMessage message) { + // Just always return NS records only without any additional records (glue records). + // Because of this the resolver will never be able to resolve and so fail eventually at some + // point. + for (QuestionRecord record: message.getQuestionRecords()) { + if (record.getDomainName().equals(domain)) { + message.getAdditionalRecords().clear(); + message.getAnswerRecords().clear(); + message.getAuthorityRecords().add(TestDnsServer.newNsRecord(domain, ns1Name)); + message.getAuthorityRecords().add(TestDnsServer.newNsRecord(domain, ns2Name)); + } + } + return message; + } + }; + testDnsServer.start(); + DnsNameResolverBuilder builder = newResolver(); + + final DnsNameResolver resolver = builder.resolveCache(NoopDnsCache.INSTANCE) + .authoritativeDnsServerCache(authoritativeDnsServerCache) + .nameServerProvider(new SingletonDnsServerAddressStreamProvider(testDnsServer.localAddress())).build(); + + try { + assertThat(resolver.resolve(domain).await().cause(), + Matchers.instanceOf(UnknownHostException.class)); + assertThat(resolver.resolveAll(domain).await().cause(), + Matchers.instanceOf(UnknownHostException.class)); + } finally { + resolver.close(); + testDnsServer.stop(); + } + } + private static InetSocketAddress unresolved(InetSocketAddress address) { return InetSocketAddress.createUnresolved(address.getHostString(), address.getPort()); }