From: JINMEI Tatuya Date: Thu, 30 Jun 2011 22:15:04 +0000 (-0700) Subject: [trac1069] adapted resolver ACL config and processing to the generalized X-Git-Tag: perftcpdns_before_epoll~231^2~19^2~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=aedaf51b32a4b31d697b18ecb914af3889d13c2c;p=thirdparty%2Fkea.git [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. --- diff --git a/src/bin/resolver/Makefile.am b/src/bin/resolver/Makefile.am index bce8307515..4d6f5c36c2 100644 --- a/src/bin/resolver/Makefile.am +++ b/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 diff --git a/src/bin/resolver/resolver.cc b/src/bin/resolver/resolver.cc index be254b7332..1a481c2adb 100644 --- a/src/bin/resolver/resolver.cc +++ b/src/bin/resolver/resolver.cc @@ -26,7 +26,7 @@ #include -#include +#include #include #include @@ -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 new_acl) { + void setQueryACL(shared_ptr new_acl) { query_acl_ = new_acl; } @@ -192,7 +194,7 @@ public: private: /// ACL on incoming queries - shared_ptr query_acl_; + shared_ptr 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 BasicAction query_action(getQueryACL().execute(client)); + const Client client(io_message); + 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 -createQueryACL(isc::data::ConstElementPtr acl_config) { - if (!acl_config) { - return (shared_ptr()); - } - - shared_ptr 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 >( - new IPCheck(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 query_acl(createQueryACL( - config->get("query_acl"))); + const ConstElementPtr query_acl_cfg(config->get("query_acl")); + const shared_ptr query_acl = + query_acl_cfg ? acl::dns::getLoader().load(query_acl_cfg) : + shared_ptr(); 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 new_acl) { +Resolver::setQueryACL(shared_ptr new_acl) { if (!new_acl) { isc_throw(InvalidParameter, "NULL pointer is passed to setQueryACL"); } diff --git a/src/bin/resolver/resolver.h b/src/bin/resolver/resolver.h index 9c7812682c..656723d316 100644 --- a/src/bin/resolver/resolver.h +++ b/src/bin/resolver/resolver.h @@ -21,10 +21,9 @@ #include -#include - #include #include +#include #include #include @@ -41,12 +40,6 @@ #include -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 ClientACL; + typedef isc::acl::ACL 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 new_acl); + void setQueryACL(boost::shared_ptr new_acl); private: ResolverImpl* impl_; diff --git a/src/bin/resolver/tests/Makefile.am b/src/bin/resolver/tests/Makefile.am index c5196176f9..97a2ba6509 100644 --- a/src/bin/resolver/tests/Makefile.am +++ b/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 diff --git a/src/bin/resolver/tests/resolver_config_unittest.cc b/src/bin/resolver/tests/resolver_config_unittest.cc index b674d032d8..698e535417 100644 --- a/src/bin/resolver/tests/resolver_config_unittest.cc +++ b/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 endpoint; - scoped_ptr request; + scoped_ptr query_message; scoped_ptr client; + scoped_ptr 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(), - *endpoint)); - client.reset(new Client(*request)); - return (*client); + query_message.reset(new IOMessage(NULL, 0, + IOSocket::getDummyUDPSocket(), + *endpoint)); + 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(createClient("::1"))); + EXPECT_EQ(REJECT, server.getQueryACL().execute(createRequest("127.0.0.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(REJECT, server.getQueryACL().execute(createClient("192.0.2.2"))); + EXPECT_EQ(ACCEPT, server.getQueryACL().execute(createRequest("192.0.2.1"))); + 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( diff --git a/src/bin/resolver/tests/resolver_unittest.cc b/src/bin/resolver/tests/resolver_unittest.cc index 9bcc261f73..6eda44268c 100644 --- a/src/bin/resolver/tests/resolver_unittest.cc +++ b/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()), + boost::shared_ptr()), isc::InvalidParameter); }