[trac1069] adapted resolver ACL config and processing to the generalized

framework.  it now accepts "action only" rule, so the test test in that
case was updated accordingly.

src/bin/resolver/Makefile.am

@@ -59,6 +59,7 @@ nodist_b10_resolver_SOURCES = resolver_messages.cc resolver_messages.h
 b10_resolver_LDADD =  $(top_builddir)/src/lib/dns/libdns++.la
 b10_resolver_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la
 b10_resolver_LDADD += $(top_builddir)/src/lib/cc/libcc.la
+b10_resolver_LDADD += $(top_builddir)/src/lib/acl/libdnsacl.la
 b10_resolver_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la
 b10_resolver_LDADD += $(top_builddir)/src/lib/asiodns/libasiodns.la
 b10_resolver_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la

src/bin/resolver/resolver.cc

@@ -26,7 +26,7 @@
 
 #include <exceptions/exceptions.h>
 
-#include <acl/acl.h>
+#include <acl/dns.h>
 #include <acl/loader.h>
 
 #include <asiodns/asiodns.h>
@@ -82,7 +82,9 @@ public:
         client_timeout_(4000),
         lookup_timeout_(30000),
         retries_(3),
-        query_acl_(new Resolver::ClientACL(REJECT)),
+        // we apply "reject all" (implicit default of the loader) ACL by
+        // default:
+        query_acl_(acl::dns::getLoader().load(Element::fromJSON("[]"))),
         rec_query_(NULL)
     {}
 
@@ -160,11 +162,11 @@ public:
                                          OutputBufferPtr buffer,
                                          DNSServer* server);
 
-    const Resolver::ClientACL& getQueryACL() const {
+    const Resolver::QueryACL& getQueryACL() const {
         return (*query_acl_);
     }
 
-    void setQueryACL(shared_ptr<const Resolver::ClientACL> new_acl) {
+    void setQueryACL(shared_ptr<const Resolver::QueryACL> new_acl) {
         query_acl_ = new_acl;
     }
 
@@ -192,7 +194,7 @@ public:
 
 private:
     /// ACL on incoming queries
-    shared_ptr<const Resolver::ClientACL> query_acl_;
+    shared_ptr<const Resolver::QueryACL> query_acl_;
 
     /// Object to handle upstream queries
     RecursiveQuery* rec_query_;
@@ -514,8 +516,10 @@ ResolverImpl::processNormalQuery(const IOMessage& io_message,
     const RRClass qclass = question->getClass();
 
     // Apply query ACL
-    Client client(io_message);
+    const Client client(io_message);
-    const BasicAction query_action(getQueryACL().execute(client));
+    const BasicAction query_action(
+        getQueryACL().execute(acl::dns::RequestContext(
+                                  client.getRequestSourceIPAddress())));
     if (query_action == isc::acl::REJECT) {
         LOG_INFO(resolver_logger, RESOLVER_QUERY_REJECTED)
             .arg(question->getName()).arg(qtype).arg(qclass).arg(client);
@@ -574,32 +578,6 @@ ResolverImpl::processNormalQuery(const IOMessage& io_message,
     return (RECURSION);
 }
 
-namespace {
-// This is a simplified ACL parser for the initial implementation with minimal
-// external dependency.  For a longer term we'll switch to a more generic
-// loader with allowing more complicated ACL syntax.
-shared_ptr<const Resolver::ClientACL>
-createQueryACL(isc::data::ConstElementPtr acl_config) {
-    if (!acl_config) {
-        return (shared_ptr<const Resolver::ClientACL>());
-    }
-
-    shared_ptr<Resolver::ClientACL> new_acl(
-        new Resolver::ClientACL(REJECT));
-    BOOST_FOREACH(ConstElementPtr rule, acl_config->listValue()) {
-        ConstElementPtr action = rule->get("action");
-        ConstElementPtr from = rule->get("from");
-        if (!action || !from) {
-            isc_throw(BadValue, "query ACL misses mandatory parameter");
-        }
-        new_acl->append(shared_ptr<IPCheck<Client> >(
-                            new IPCheck<Client>(from->stringValue())),
-                        defaultActionLoader(action));
-    }
-    return (new_acl);
-}
-}
-
 ConstElementPtr
 Resolver::updateConfig(ConstElementPtr config) {
     LOG_DEBUG(resolver_logger, RESOLVER_DBG_CONFIG, RESOLVER_CONFIG_UPDATED)
@@ -616,8 +594,10 @@ Resolver::updateConfig(ConstElementPtr config) {
         ConstElementPtr listenAddressesE(config->get("listen_on"));
         AddressList listenAddresses(parseAddresses(listenAddressesE,
                                                       "listen_on"));
-        shared_ptr<const ClientACL> query_acl(createQueryACL(
+        const ConstElementPtr query_acl_cfg(config->get("query_acl"));
-                                                  config->get("query_acl")));
+        const shared_ptr<const QueryACL> query_acl =
+            query_acl_cfg ? acl::dns::getLoader().load(query_acl_cfg) :
+            shared_ptr<const QueryACL>();
         bool set_timeouts(false);
         int qtimeout = impl_->query_timeout_;
         int ctimeout = impl_->client_timeout_;
@@ -777,13 +757,13 @@ Resolver::getListenAddresses() const {
     return (impl_->listen_);
 }
 
-const Resolver::ClientACL&
+const Resolver::QueryACL&
 Resolver::getQueryACL() const {
     return (impl_->getQueryACL());
 }
 
 void
-Resolver::setQueryACL(shared_ptr<const ClientACL> new_acl) {
+Resolver::setQueryACL(shared_ptr<const QueryACL> new_acl) {
     if (!new_acl) {
         isc_throw(InvalidParameter, "NULL pointer is passed to setQueryACL");
     }

src/bin/resolver/resolver.h

@@ -21,10 +21,9 @@
 
 #include <boost/shared_ptr.hpp>
 
-#include <acl/acl.h>
-
 #include <cc/data.h>
 #include <config/ccsession.h>
+#include <acl/dns.h>
 #include <dns/message.h>
 #include <util/buffer.h>
 
@@ -41,12 +40,6 @@
 
 #include <resolve/resolver_interface.h>
 
-namespace isc {
-namespace server_common {
-class Client;
-}
-}
-
 class ResolverImpl;
 
 /**
@@ -247,12 +240,12 @@ public:
     int getRetries() const;
 
     // Shortcut typedef used for query ACL.
-    typedef isc::acl::ACL<isc::server_common::Client> ClientACL;
+    typedef isc::acl::ACL<isc::acl::dns::RequestContext> QueryACL;
 
     /// Get the query ACL.
     ///
     /// \exception None
-    const ClientACL& getQueryACL() const;
+    const QueryACL& getQueryACL() const;
 
     /// Set the new query ACL.
     ///
@@ -265,7 +258,7 @@ public:
     /// \exception InvalidParameter The given pointer is NULL
     ///
     /// \param new_acl The new ACL to replace the existing one.
-    void setQueryACL(boost::shared_ptr<const ClientACL> new_acl);
+    void setQueryACL(boost::shared_ptr<const QueryACL> new_acl);
 
 private:
     ResolverImpl* impl_;

src/bin/resolver/tests/Makefile.am

@@ -39,6 +39,7 @@ run_unittests_LDADD += $(top_builddir)/src/lib/dns/libdns++.la
 run_unittests_LDADD += $(top_builddir)/src/lib/asiodns/libasiodns.la
 run_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la
 run_unittests_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la
+run_unittests_LDADD += $(top_builddir)/src/lib/acl/libdnsacl.la
 run_unittests_LDADD += $(top_builddir)/src/lib/cc/libcc.la
 run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la
 run_unittests_LDADD += $(top_builddir)/src/lib/xfr/libxfr.la

src/bin/resolver/tests/resolver_config_unittest.cc

@@ -43,6 +43,7 @@
 using namespace std;
 using boost::scoped_ptr;
 using namespace isc::acl;
+using isc::acl::dns::RequestContext;
 using namespace isc::data;
 using namespace isc::testutils;
 using namespace isc::asiodns;
@@ -57,19 +58,22 @@ protected:
     DNSService dnss;
     Resolver server;
     scoped_ptr<const IOEndpoint> endpoint;
-    scoped_ptr<const IOMessage> request;
+    scoped_ptr<const IOMessage> query_message;
     scoped_ptr<const Client> client;
+    scoped_ptr<const RequestContext> request;
     ResolverConfig() : dnss(ios, NULL, NULL, NULL) {
         server.setDNSService(dnss);
         server.setConfigured();
     }
-    const Client& createClient(const string& source_addr) {
+    const RequestContext& createRequest(const string& source_addr) {
         endpoint.reset(IOEndpoint::create(IPPROTO_UDP, IOAddress(source_addr),
                                           53210));
-        request.reset(new IOMessage(NULL, 0, IOSocket::getDummyUDPSocket(),
+        query_message.reset(new IOMessage(NULL, 0,
-                                    *endpoint));
+                                          IOSocket::getDummyUDPSocket(),
-        client.reset(new Client(*request));
+                                          *endpoint));
-        return (*client);
+        client.reset(new Client(*query_message));
+        request.reset(new RequestContext(client->getRequestSourceIPAddress()));
+        return (*request);
     }
     void invalidTest(const string &JSON, const string& name);
 };
@@ -253,15 +257,15 @@ TEST_F(ResolverConfig, invalidTimeoutsConfig) {
 
 TEST_F(ResolverConfig, defaultQueryACL) {
     // If no configuration is loaded, the default ACL should reject everything.
-    EXPECT_EQ(REJECT, server.getQueryACL().execute(createClient("192.0.2.1")));
+    EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("192.0.2.1")));
     EXPECT_EQ(REJECT, server.getQueryACL().execute(
-                  createClient("2001:db8::1")));
+                  createRequest("2001:db8::1")));
 
     // The following would be allowed if the server had loaded the default
     // configuration from the spec file.  In this context it should not have
     // happened, and they should be rejected just like the above cases.
-    EXPECT_EQ(REJECT, server.getQueryACL().execute(createClient("127.0.0.1")));
+    EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("127.0.0.1")));
-    EXPECT_EQ(REJECT, server.getQueryACL().execute(createClient("::1")));
+    EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("::1")));
 }
 
 TEST_F(ResolverConfig, emptyQueryACL) {
@@ -269,9 +273,9 @@ TEST_F(ResolverConfig, emptyQueryACL) {
     ConstElementPtr config(Element::fromJSON("{ \"query_acl\": [] }"));
     ConstElementPtr result(server.updateConfig(config));
     EXPECT_EQ(result->toWire(), isc::config::createAnswer()->toWire());
-    EXPECT_EQ(REJECT, server.getQueryACL().execute(createClient("192.0.2.1")));
+    EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("192.0.2.1")));
     EXPECT_EQ(REJECT, server.getQueryACL().execute(
-                  createClient("2001:db8::1")));
+                  createRequest("2001:db8::1")));
 }
 
 TEST_F(ResolverConfig, queryACLIPv4) {
@@ -282,9 +286,9 @@ TEST_F(ResolverConfig, queryACLIPv4) {
                                "     \"from\": \"192.0.2.1\"} ] }"));
     ConstElementPtr result(server.updateConfig(config));
     EXPECT_EQ(result->toWire(), isc::config::createAnswer()->toWire());
-    EXPECT_EQ(ACCEPT, server.getQueryACL().execute(createClient("192.0.2.1")));
+    EXPECT_EQ(ACCEPT, server.getQueryACL().execute(createRequest("192.0.2.1")));
     EXPECT_EQ(REJECT, server.getQueryACL().execute(
-                  createClient("2001:db8::1")));
+                  createRequest("2001:db8::1")));
 }
 
 TEST_F(ResolverConfig, queryACLIPv6) {
@@ -295,9 +299,9 @@ TEST_F(ResolverConfig, queryACLIPv6) {
                                "     \"from\": \"2001:db8::1\"} ] }"));
     ConstElementPtr result(server.updateConfig(config));
     EXPECT_EQ(result->toWire(), isc::config::createAnswer()->toWire());
-    EXPECT_EQ(REJECT, server.getQueryACL().execute(createClient("192.0.2.1")));
+    EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("192.0.2.1")));
     EXPECT_EQ(ACCEPT, server.getQueryACL().execute(
-                  createClient("2001:db8::1")));
+                  createRequest("2001:db8::1")));
 }
 
 TEST_F(ResolverConfig, multiEntryACL) {
@@ -317,14 +321,15 @@ TEST_F(ResolverConfig, multiEntryACL) {
                                "] }"));
     ConstElementPtr result(server.updateConfig(config));
     EXPECT_EQ(result->toWire(), isc::config::createAnswer()->toWire());
-    EXPECT_EQ(ACCEPT, server.getQueryACL().execute(createClient("192.0.2.1")));
+    EXPECT_EQ(ACCEPT, server.getQueryACL().execute(createRequest("192.0.2.1")));
-    EXPECT_EQ(REJECT, server.getQueryACL().execute(createClient("192.0.2.2")));
+    EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("192.0.2.2")));
     EXPECT_EQ(DROP, server.getQueryACL().execute(
-                  createClient("2001:db8::1")));
+                  createRequest("2001:db8::1")));
     EXPECT_EQ(REJECT, server.getQueryACL().execute(
-                  createClient("2001:db8::2"))); // match the default rule
+                  createRequest("2001:db8::2"))); // match the default rule
 }
 
+
 int
 getResultCode(ConstElementPtr result) {
     int rcode;
@@ -332,6 +337,22 @@ getResultCode(ConstElementPtr result) {
     return (rcode);
 }
 
+TEST_F(ResolverConfig, queryACLActionOnly) {
+    // "action only" rule will be accepted by the loader, which can
+    // effectively change the default action.
+    ConstElementPtr config(Element::fromJSON(
+                               "{ \"query_acl\": "
+                               "  [ {\"action\": \"ACCEPT\","
+                               "     \"from\": \"192.0.2.1\"},"
+                               "    {\"action\": \"DROP\"} ] }"));
+    EXPECT_EQ(0, getResultCode(server.updateConfig(config)));
+    EXPECT_EQ(ACCEPT, server.getQueryACL().execute(createRequest("192.0.2.1")));
+
+    // We reject non matching queries by default, but the last resort
+    // rule should have changed the action in that case to "DROP".
+    EXPECT_EQ(DROP, server.getQueryACL().execute(createRequest("192.0.2.2")));
+}
+
 TEST_F(ResolverConfig, badQueryACL) {
     // Most of these cases shouldn't happen in practice because the syntax
     // check should be performed before updateConfig().  But we check at
@@ -346,10 +367,6 @@ TEST_F(ResolverConfig, badQueryACL) {
                   server.updateConfig(
                       Element::fromJSON("{ \"query_acl\":"
                                         " [ {\"from\": \"192.0.2.1\"} ] }"))));
-    EXPECT_EQ(1, getResultCode(
-                  server.updateConfig(
-                      Element::fromJSON("{ \"query_acl\":"
-                                        " [ {\"action\": \"DROP\"} ] }"))));
     // invalid "action"
     EXPECT_EQ(1, getResultCode(
                   server.updateConfig(
@@ -361,7 +378,6 @@ TEST_F(ResolverConfig, badQueryACL) {
                       Element::fromJSON("{ \"query_acl\":"
                                         " [ {\"action\": \"BADACTION\","
                                         "    \"from\": \"192.0.2.1\"}]}"))));
-
     // invalid "from"
     EXPECT_EQ(1, getResultCode(
                   server.updateConfig(

src/bin/resolver/tests/resolver_unittest.cc

@@ -157,7 +157,7 @@ TEST_F(ResolverTest, setQueryACL) {
     // valid cases are tested through other tests.  We only explicitly check
     // an invalid case: passing a NULL shared pointer.
     EXPECT_THROW(server.setQueryACL(
-                     boost::shared_ptr<const Resolver::ClientACL>()),
+                     boost::shared_ptr<const Resolver::QueryACL>()),
                  isc::InvalidParameter);
 }