From 82ec6ba815adcb3ab057ef8a65f6f620a1ad5611 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 14 Jan 2019 08:17:44 +0100 Subject: [PATCH] Correctly detect and handle CNAME loops. (#8691) Motivation: We do not correctly detect loops when follow CNAMEs and so may try to follow it without any success. Modifications: - Correctly detect CNAME loops - Do not cache CNAME entries which point to itself - Add unit test. Result: Fixes https://github.com/netty/netty/issues/8687. --- .../netty/resolver/dns/DnsResolveContext.java | 21 +++++++-- .../resolver/dns/DnsNameResolverTest.java | 47 +++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) 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 e75f49c8d4..847bfb5b9f 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 @@ -48,6 +48,7 @@ import java.util.AbstractList; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.IdentityHashMap; import java.util.Iterator; import java.util.List; @@ -651,10 +652,11 @@ abstract class DnsResolveContext { // Make sure the record is for the questioned domain. if (!recordName.equals(questionName)) { + Map cnamesCopy = new HashMap(cnames); // Even if the record's name is not exactly same, it might be an alias defined in the CNAME records. String resolved = questionName; do { - resolved = cnames.get(resolved); + resolved = cnamesCopy.remove(resolved); if (recordName.equals(resolved)) { break; } @@ -749,8 +751,12 @@ abstract class DnsResolveContext { String mapping = domainName.toLowerCase(Locale.US); // Cache the CNAME as well. - cache.cache(hostnameWithDot(name), hostnameWithDot(mapping), r.timeToLive(), loop); - cnames.put(name, mapping); + String nameWithDot = hostnameWithDot(name); + String mappingWithDot = hostnameWithDot(mapping); + if (!nameWithDot.equalsIgnoreCase(mappingWithDot)) { + cache.cache(nameWithDot, mappingWithDot, r.timeToLive(), loop); + cnames.put(name, mapping); + } } return cnames != null? cnames : Collections.emptyMap(); @@ -875,12 +881,21 @@ abstract class DnsResolveContext { private void followCname(DnsQuestion question, String cname, DnsQueryLifecycleObserver queryLifecycleObserver, Promise> promise) { + Set cnames = null; for (;;) { // Resolve from cnameCache() until there is no more cname entry cached. String mapping = cnameCache().get(hostnameWithDot(cname)); if (mapping == null) { break; } + if (cnames == null) { + // Detect loops. + cnames = new HashSet(2); + } + if (!cnames.add(cname)) { + // Follow CNAME from cache would loop. Lets break here. + break; + } cname = mapping; } 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 70c8b6b52e..1326a7975e 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 @@ -96,6 +96,7 @@ import static io.netty.handler.codec.dns.DnsRecordType.AAAA; import static io.netty.handler.codec.dns.DnsRecordType.CNAME; import static io.netty.resolver.dns.DnsServerAddresses.sequential; import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; @@ -2135,6 +2136,52 @@ public class DnsNameResolverTest { } } + @Test + public void testFollowCNAMELoop() throws IOException { + expectedException.expect(UnknownHostException.class); + TestDnsServer dnsServer2 = new TestDnsServer(new RecordStore() { + + @Override + public Set getRecords(QuestionRecord question) { + Set records = new LinkedHashSet(4); + + records.add(new TestDnsServer.TestResourceRecord("x." + question.getDomainName(), + RecordType.A, Collections.singletonMap( + DnsAttribute.IP_ADDRESS.toLowerCase(), "10.0.0.99"))); + records.add(new TestDnsServer.TestResourceRecord( + "cname2.netty.io", RecordType.CNAME, + Collections.singletonMap( + DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname.netty.io"))); + records.add(new TestDnsServer.TestResourceRecord( + "cname.netty.io", RecordType.CNAME, + Collections.singletonMap( + DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname2.netty.io"))); + records.add(new TestDnsServer.TestResourceRecord( + question.getDomainName(), RecordType.CNAME, + Collections.singletonMap( + DnsAttribute.DOMAIN_NAME.toLowerCase(), "cname.netty.io"))); + return records; + } + }); + dnsServer2.start(); + DnsNameResolver resolver = null; + try { + DnsNameResolverBuilder builder = newResolver() + .recursionDesired(false) + .resolvedAddressTypes(ResolvedAddressTypes.IPV4_ONLY) + .maxQueriesPerResolve(16) + .nameServerProvider(new SingletonDnsServerAddressStreamProvider(dnsServer2.localAddress())); + + resolver = builder.build(); + resolver.resolveAll("somehost.netty.io").syncUninterruptibly().getNow(); + } finally { + dnsServer2.stop(); + if (resolver != null) { + resolver.close(); + } + } + } + @Test public void testSearchDomainQueryFailureForSingleAddressTypeCompletes() { expectedException.expect(UnknownHostException.class);