From 80ff106e4c3db91070826986bc02b368ec3e64be Mon Sep 17 00:00:00 2001 From: mountford Date: Fri, 15 Sep 2017 08:00:36 -0700 Subject: [PATCH] Prohibit some RDAP domain and nameserver lookups by nameserver name We had been allowing lookups by nameserver name using a wildcard and suffix if the suffix was a domain name. That's ok if the domain name is one we manage, but doesn't work efficiently otherwise. A lookup of ns*.zombo.com would require us to search for all nameservers beginning with ns (which could be almost all of them), then loop through until we found those ending with .zombo.com. So we are going to prohibit suffixes after the TLD unless the suffix is an in-bailiwick domain. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=168835732 --- .../registry/rdap/RdapDomainSearchAction.java | 66 ++++++++----------- .../rdap/RdapNameserverSearchAction.java | 52 ++++++++------- .../rdap/RdapDomainSearchActionTest.java | 19 ++++-- .../rdap/RdapNameserverSearchActionTest.java | 9 ++- 4 files changed, 76 insertions(+), 70 deletions(-) diff --git a/java/google/registry/rdap/RdapDomainSearchAction.java b/java/google/registry/rdap/RdapDomainSearchAction.java index 1e15ea868..c99fdb8f0 100644 --- a/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/java/google/registry/rdap/RdapDomainSearchAction.java @@ -261,51 +261,43 @@ public class RdapDomainSearchAction extends RdapActionBase { } // Handle queries with a wildcard. } else { - // If there is a suffix, it must be a domain. If it happens to be a domain that we manage, - // we can look up the domain and look through the subordinate hosts. This is more efficient, - // and lets us permit wildcard searches with no initial string. + // If there is a suffix, it must be a domain that we manage. That way, we can look up the + // domain and search through the subordinate hosts. This is more efficient, and lets us permit + // wildcard searches with no initial string. if (partialStringQuery.getSuffix() != null) { DomainResource domainResource = loadByForeignKey( DomainResource.class, partialStringQuery.getSuffix(), now); - if (domainResource != null) { - ImmutableList.Builder> builder = new ImmutableList.Builder<>(); - for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { - // We can't just check that the host name starts with the initial query string, because - // then the query ns.exam*.example.com would match against nameserver ns.example.com. - if (partialStringQuery.matches(fqhn)) { - Key hostKey = loadAndGetKey(HostResource.class, fqhn, now); - if (hostKey != null) { - builder.add(hostKey); - } else { - logger.warningfmt("Host key unexpectedly null"); - } + if (domainResource == null) { + // Don't allow wildcards with suffixes which are not domains we manage. That would risk a + // table scan in some easily foreseeable cases. + throw new UnprocessableEntityException( + "A suffix in a lookup by nameserver name must be an in-bailiwick domain"); + } + ImmutableList.Builder> builder = new ImmutableList.Builder<>(); + for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { + // We can't just check that the host name starts with the initial query string, because + // then the query ns.exam*.example.com would match against nameserver ns.example.com. + if (partialStringQuery.matches(fqhn)) { + Key hostKey = loadAndGetKey(HostResource.class, fqhn, now); + if (hostKey != null) { + builder.add(hostKey); + } else { + logger.warningfmt("Host key unexpectedly null"); } } - return builder.build(); } - } - // If there's no suffix, or it isn't a domain we manage, query the host resources. Query the - // resources themselves, rather than the foreign key indexes, because then we have an index on - // fully qualified host name and deletion time, so we can check the deletion status in the - // query itself. There are no pending deletes for hosts, so we can call queryUndeleted. In - // this case, the initial string must be present, to avoid querying every host in the system. - // This restriction is enforced by queryUndeleted(). + return builder.build(); + // If there's no suffix, query the host resources. Query the resources themselves, rather than + // the foreign key indexes, because then we have an index on fully qualified host name and + // deletion time, so we can check the deletion status in the query itself. There are no + // pending deletes for hosts, so we can call queryUndeleted. In this case, the initial string + // must be present, to avoid querying every host in the system. This restriction is enforced + // by queryUndeleted(). // TODO (b/24463238): figure out how to limit the size of these queries effectively - Iterable> keys = - queryUndeleted(HostResource.class, "fullyQualifiedHostName", partialStringQuery, 1000) - .keys(); - // queryUndeleted() ignores suffixes, so if one was specified, we must filter on the partial - // string query. - if (partialStringQuery.getSuffix() == null) { - return keys; } else { - ImmutableList.Builder> filteredKeys = new ImmutableList.Builder<>(); - for (Key key : keys) { - if (partialStringQuery.matches(key.getName())) { - filteredKeys.add(key); - } - } - return filteredKeys.build(); + return queryUndeleted( + HostResource.class, "fullyQualifiedHostName", partialStringQuery, 1000) + .keys(); } } } diff --git a/java/google/registry/rdap/RdapNameserverSearchAction.java b/java/google/registry/rdap/RdapNameserverSearchAction.java index de6db742c..5cc3d416f 100644 --- a/java/google/registry/rdap/RdapNameserverSearchAction.java +++ b/java/google/registry/rdap/RdapNameserverSearchAction.java @@ -35,11 +35,13 @@ import google.registry.rdap.RdapJsonFormatter.OutputDataType; import google.registry.request.Action; import google.registry.request.HttpException.BadRequestException; import google.registry.request.HttpException.NotFoundException; +import google.registry.request.HttpException.UnprocessableEntityException; import google.registry.request.Parameter; import google.registry.request.auth.Auth; import google.registry.util.Clock; import google.registry.util.Idn; import java.net.InetAddress; +import java.util.ArrayList; import java.util.List; import javax.inject.Inject; import org.joda.time.DateTime; @@ -140,38 +142,40 @@ public class RdapNameserverSearchAction extends RdapActionBase { hostResource, false, rdapLinkBase, rdapWhoisServer, now, OutputDataType.FULL))); // Handle queries with a wildcard. } else { - // If there is a suffix, it should be a domain. If it happens to be a domain that we manage, - // we can look up the domain and look through the subordinate hosts. This is more efficient, - // and lets us permit wildcard searches with no initial string. + // If there is a suffix, it should be a domain that we manage, so we can look up the domain + // and search through the subordinate hosts. This is more efficient, and lets us permit + // wildcard searches with no initial string. if (partialStringQuery.getSuffix() != null) { DomainResource domainResource = loadByForeignKey(DomainResource.class, partialStringQuery.getSuffix(), now); - ImmutableList.Builder hostListBuilder = new ImmutableList.Builder<>(); - if (domainResource != null) { - for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { - // We can't just check that the host name starts with the initial query string, because - // then the query ns.exam*.example.com would match against nameserver ns.example.com. - if (partialStringQuery.matches(fqhn)) { - HostResource hostResource = loadByForeignKey(HostResource.class, fqhn, now); - if (hostResource != null) { - hostListBuilder.add(hostResource); + if (domainResource == null) { + // Don't allow wildcards with suffixes which are not domains we manage. That would risk a + // table scan in many easily foreseeable cases. The user might ask for ns*.zombo.com, + // forcing us to query for all hosts beginning with ns, then filter for those ending in + // .zombo.com. It might well be that 80% of all hostnames begin with ns, leading to + // inefficiency. + throw new UnprocessableEntityException( + "A suffix after a wildcard in a nameserver lookup must be an in-bailiwick domain"); + } + List hostList = new ArrayList<>(); + for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { + // We can't just check that the host name starts with the initial query string, because + // then the query ns.exam*.example.com would match against nameserver ns.example.com. + if (partialStringQuery.matches(fqhn)) { + HostResource hostResource = loadByForeignKey(HostResource.class, fqhn, now); + if (hostResource != null) { + hostList.add(hostResource); + if (hostList.size() > rdapResultSetMaxSize) { + break; } } } - } else { - // If we don't recognize the domain, call queryUndeleted and filter. - // TODO(mountford): figure out how to size this correctly - for (HostResource hostResource : - queryUndeleted( - HostResource.class, "fullyQualifiedHostName", partialStringQuery, 1000)) { - if (partialStringQuery.matches(hostResource.getFullyQualifiedHostName())) { - hostListBuilder.add(hostResource); - } - } } - return makeSearchResults(hostListBuilder.build(), now); + return makeSearchResults(hostList, now); // Handle queries with a wildcard, but no suffix. There are no pending deletes for hosts, so - // we can call queryUndeleted. + // we can call queryUndeleted. Unlike the above problem with suffixes, we can safely search + // for nameservers beginning with a particular suffix, because we need only fetch the first + // rdapResultSetMaxSize entries, and ignore the rest. } else { return makeSearchResults( // Add 1 so we can detect truncation. diff --git a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java index 55e062f70..2b54e2e3f 100644 --- a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java @@ -815,8 +815,8 @@ public class RdapDomainSearchActionTest { } @Test - public void testNameserverMatchWithWildcardAndTldSuffix_notFound() throws Exception { - generateActualJson(RequestType.NS_LDH_NAME, "ns2.cat*.lol"); + public void testNameserverMatchWithWildcardAndDomainSuffix_notFound() throws Exception { + generateActualJson(RequestType.NS_LDH_NAME, "ns5*.cat.lol"); assertThat(response.getStatus()).isEqualTo(404); } @@ -849,6 +849,12 @@ public class RdapDomainSearchActionTest { assertThat(response.getStatus()).isEqualTo(422); } + @Test + public void testNameserverMatchWithWildcardAndInvalidSuffix_unprocessable() throws Exception { + generateActualJson(RequestType.NS_LDH_NAME, "ns*.google.com"); + assertThat(response.getStatus()).isEqualTo(422); + } + @Test public void testNameserverMatch_ns2_cat_lol_found() throws Exception { generateActualJson(RequestType.NS_LDH_NAME, "ns2.cat.lol"); @@ -887,9 +893,9 @@ public class RdapDomainSearchActionTest { } @Test - public void testNameserverMatch_nsstar_test_notFound() throws Exception { + public void testNameserverMatch_nsstar_test_unprocessable() throws Exception { generateActualJson(RequestType.NS_LDH_NAME, "ns*.1.test"); - assertThat(response.getStatus()).isEqualTo(404); + assertThat(response.getStatus()).isEqualTo(422); } @Test @@ -949,10 +955,11 @@ public class RdapDomainSearchActionTest { } @Test - public void testNameserverMatchDeletedNameserverWithWildcardAndTld_notFound() throws Exception { + public void testNameserverMatchDeletedNameserverWithWildcardAndSuffix_notFound() + throws Exception { persistResource( hostNs1CatLol.asBuilder().setDeletionTime(clock.nowUtc().minusDays(1)).build()); - assertThat(generateActualJson(RequestType.NS_LDH_NAME, "ns1.cat*.lol")) + assertThat(generateActualJson(RequestType.NS_LDH_NAME, "ns1*.cat.lol")) .isEqualTo(generateExpectedJson("No matching nameservers found", "rdap_error_404.json")); assertThat(response.getStatus()).isEqualTo(404); } diff --git a/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java b/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java index d94421c07..6904da33d 100644 --- a/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java @@ -239,10 +239,13 @@ public class RdapNameserverSearchActionTest { } @Test - public void testNonexistentDomainSuffix_notFound() throws Exception { + public void testNonexistentDomainSuffix_unprocessable() throws Exception { assertThat(generateActualJsonWithName("exam*.foo.bar")) - .isEqualTo(generateExpectedJson("No nameservers found", "rdap_error_404.json")); - assertThat(response.getStatus()).isEqualTo(404); + .isEqualTo( + generateExpectedJson( + "A suffix after a wildcard in a nameserver lookup must be an in-bailiwick domain", + "rdap_error_422.json")); + assertThat(response.getStatus()).isEqualTo(422); } @Test