Process review comments and extend unit tests

diff --git a/pdns/filterpo.cc b/pdns/filterpo.cc
index 0a3f0f3b4f009f6269a78e54e1964b51268a82cc..a1161745e9dd10a947ee46da6a3b08beacd9f5b6 100644 (file)
--- a/pdns/filterpo.cc
+++ b/pdns/filterpo.cc
@@ -50,34 +50,42 @@ bool DNSFilterEngine::Zone::findExactQNamePolicy(const DNSName& qname, DNSFilter
 
 bool DNSFilterEngine::Zone::findExactNSPolicy(const DNSName& qname, DNSFilterEngine::Policy& pol) const
 {
-  return findExactNamedPolicy(d_propolName, qname, pol);
+  if (findExactNamedPolicy(d_propolName, qname, pol)) {
+    pol.d_trigger = qname;
+    pol.d_trigger.appendRawLabel(rpzNSDnameName);
+    return true;
+  }
+  return false;
 }
 
-bool DNSFilterEngine::Zone::findNSIPPolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const
+bool DNSFilterEngine::Zone::findNSIPPolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const
 {
   if (const auto fnd = d_propolNSAddr.lookup(addr)) {
-    key = fnd->first;
     pol = fnd->second;
+    pol.d_trigger = Zone::maskToRPZ(fnd->first);
+    pol.d_trigger.appendRawLabel(rpzNSIPName);
     return true;
   }
   return false;
 }
 
-bool DNSFilterEngine::Zone::findResponsePolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const
+bool DNSFilterEngine::Zone::findResponsePolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const
 {
   if (const auto fnd = d_postpolAddr.lookup(addr)) {
-    key = fnd->first;
     pol = fnd->second;
+    pol.d_trigger = Zone::maskToRPZ(fnd->first);
+    pol.d_trigger.appendRawLabel(rpzIPName);
     return true;
   }
   return false;
 }
 
-bool DNSFilterEngine::Zone::findClientPolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const
+bool DNSFilterEngine::Zone::findClientPolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const
 {
   if (const auto fnd = d_qpolAddr.lookup(addr)) {
-    key = fnd->first;
     pol = fnd->second;
+    pol.d_trigger = Zone::maskToRPZ(fnd->first);
+    pol.d_trigger.appendRawLabel(rpzClientIPName);
     return true;
   }
   return false;
@@ -182,8 +190,6 @@ bool DNSFilterEngine::getProcessingPolicy(const DNSName& qname, const std::unord
     }
     if (z->findExactNSPolicy(qname, pol)) {
       // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
-      pol.d_trigger = qname;
-      pol.d_trigger.appendRawLabel(rpzNSDnameName);
       pol.d_hit = qname.toStringNoDot();
       return true;
     }
@@ -191,8 +197,6 @@ bool DNSFilterEngine::getProcessingPolicy(const DNSName& qname, const std::unord
     for (const auto& wc : wcNames) {
       if (z->findExactNSPolicy(wc, pol)) {
         // cerr<<"Had a hit on the nameserver ("<<qname<<") used to process the query"<<endl;
-        pol.d_trigger = wc;
-        pol.d_trigger.appendRawLabel(rpzNSDnameName);
         pol.d_hit = qname.toStringNoDot();
         return true;
       }
@@ -216,10 +220,8 @@ bool DNSFilterEngine::getProcessingPolicy(const ComboAddress& address, const std
     }
 
     Netmask key;
-    if(z->findNSIPPolicy(address, key, pol)) {
+    if(z->findNSIPPolicy(address, pol)) {
       //      cerr<<"Had a hit on the nameserver ("<<address.toString()<<") used to process the query"<<endl;
-      pol.d_trigger = Zone::maskToRPZ(key);
-      pol.d_trigger.appendRawLabel(rpzNSIPName);
       pol.d_hit = address.toString();
       return true;
     }
@@ -240,8 +242,9 @@ bool DNSFilterEngine::getClientPolicy(const ComboAddress& ca, const std::unorder
     }
 
     Netmask key;
-    if (z->findClientPolicy(ca, key, pol)) {
+    if (z->findClientPolicy(ca, pol)) {
       // cerr<<"Had a hit on the IP address ("<<ca.toString()<<") of the client"<<endl;
+      pol.d_hit = ca.toString();
       return true;
     }
   }
@@ -360,7 +363,7 @@ bool DNSFilterEngine::getPostPolicy(const DNSRecord& record, const std::unordere
     }
 
     Netmask key;
-    if (z->findResponsePolicy(ca, key, pol)) {
+    if (z->findResponsePolicy(ca, pol)) {
       pol.d_trigger = Zone::maskToRPZ(key);
       pol.d_trigger.appendRawLabel(rpzIPName);
       pol.d_hit = ca.toString();

diff --git a/pdns/filterpo.hh b/pdns/filterpo.hh
index 47f1d9a6de95c4a3f2787f4405ea832c41e3faa1..971aabd4760d48a0e419c06b79d8c0c93cd4589d 100644 (file)
--- a/pdns/filterpo.hh
+++ b/pdns/filterpo.hh
@@ -263,9 +263,9 @@ public:
 
     bool findExactQNamePolicy(const DNSName& qname, DNSFilterEngine::Policy& pol) const;
     bool findExactNSPolicy(const DNSName& qname, DNSFilterEngine::Policy& pol) const;
-    bool findNSIPPolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const;
-    bool findResponsePolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const;
-    bool findClientPolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const;
+    bool findNSIPPolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const;
+    bool findResponsePolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const;
+    bool findClientPolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const;
 
     bool hasClientPolicies() const
     {

diff --git a/pdns/recursordist/test-filterpo_cc.cc b/pdns/recursordist/test-filterpo_cc.cc
index 065fe4c3cb94bb13700a3de31758e00fa0a0b427..5cb8cfb2fe45bcf9c1136c2c90d48a8b20ab93b8 100644 (file)
--- a/pdns/recursordist/test-filterpo_cc.cc
+++ b/pdns/recursordist/test-filterpo_cc.cc
@@ -33,19 +33,19 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
   const DNSName blockedWildcardName("*.wildcard-blocked.");
   const ComboAddress responseIP("192.0.2.254");
   BOOST_CHECK_EQUAL(zone->size(), 0U);
-  zone->addClientTrigger(Netmask(clientIP, 32), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ClientIP));
+  zone->addClientTrigger(Netmask(clientIP, 31), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ClientIP));
   BOOST_CHECK_EQUAL(zone->size(), 1U);
   zone->addQNameTrigger(blockedName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::QName));
   BOOST_CHECK_EQUAL(zone->size(), 2U);
   zone->addQNameTrigger(blockedWildcardName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::QName));
   BOOST_CHECK_EQUAL(zone->size(), 3U);
-  zone->addNSIPTrigger(Netmask(nsIP, 32), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSIP));
+  zone->addNSIPTrigger(Netmask(nsIP, 31), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSIP));
   BOOST_CHECK_EQUAL(zone->size(), 4U);
   zone->addNSTrigger(nsName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSDName));
   BOOST_CHECK_EQUAL(zone->size(), 5U);
   zone->addNSTrigger(nsWildcardName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSDName));
   BOOST_CHECK_EQUAL(zone->size(), 6U);
-  zone->addResponseTrigger(Netmask(responseIP, 32), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ResponseIP));
+  zone->addResponseTrigger(Netmask(responseIP, 31), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ResponseIP));
   BOOST_CHECK_EQUAL(zone->size(), 7U);
 
   size_t zoneIdx = dfe.addZone(zone);
@@ -81,6 +81,8 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     const auto matchingPolicy = dfe.getProcessingPolicy(DNSName("sub.sub.wildcard.wolf."), std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::NSDName);
     BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Drop);
+    BOOST_CHECK_EQUAL(matchingPolicy.d_trigger, DNSName("*.wildcard.wolf.rpz-nsdname"));
+    BOOST_CHECK_EQUAL(matchingPolicy.d_hit, "sub.sub.wildcard.wolf");
 
     /* looking for wildcard.wolf. should not match *.wildcard-blocked. */
     const auto notMatchingPolicy = dfe.getProcessingPolicy(DNSName("wildcard.wolf."), std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
@@ -92,6 +94,8 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     /* except if we look exactly for the wildcard */
     BOOST_CHECK(zone->findExactNSPolicy(nsWildcardName, zonePolicy));
     BOOST_CHECK(zonePolicy == matchingPolicy);
+    BOOST_CHECK_EQUAL(zonePolicy.d_trigger, DNSName("*.wildcard.wolf.rpz-nsdname"));
+    BOOST_CHECK_EQUAL(zonePolicy.d_hit, nsWildcardName.toStringNoDot());
   }
 
   {
@@ -107,20 +111,18 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     const auto matchingPolicy = dfe.getProcessingPolicy(nsIP, std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::NSIP);
     BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Drop);
-    Netmask key;
     DNSFilterEngine::Policy zonePolicy;
-    BOOST_CHECK(zone->findNSIPPolicy(nsIP, key, zonePolicy));
-    BOOST_CHECK(key == nsIP);
+    BOOST_CHECK(zone->findNSIPPolicy(nsIP, zonePolicy));
     BOOST_CHECK(zonePolicy == matchingPolicy);
+    BOOST_CHECK_EQUAL(zonePolicy.d_trigger, DNSName("31.0.2.0.192.rpz-nsip"));
   }
 
   {
     /* allowed NS IP */
     const auto matchingPolicy = dfe.getProcessingPolicy(ComboAddress("192.0.2.142"), std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::None);
-    Netmask key;
     DNSFilterEngine::Policy zonePolicy;
-    BOOST_CHECK(zone->findNSIPPolicy(ComboAddress("192.0.2.142"), key, zonePolicy) == false);
+    BOOST_CHECK(zone->findNSIPPolicy(ComboAddress("192.0.2.142"), zonePolicy) == false);
   }
 
   {
@@ -131,6 +133,8 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     DNSFilterEngine::Policy zonePolicy;
     BOOST_CHECK(zone->findExactQNamePolicy(blockedName, zonePolicy));
     BOOST_CHECK(zonePolicy == matchingPolicy);
+    BOOST_CHECK_EQUAL(zonePolicy.d_trigger, blockedName);
+    BOOST_CHECK_EQUAL(zonePolicy.d_hit, blockedName.toStringNoDot());
 
     /* but a subdomain should not be blocked (not a wildcard, and this is not suffix domain matching */
     matchingPolicy = dfe.getQueryPolicy(DNSName("sub") + blockedName, std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
@@ -143,6 +147,8 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     const auto matchingPolicy = dfe.getQueryPolicy(DNSName("sub.sub.wildcard-blocked."), std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::QName);
     BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Drop);
+    BOOST_CHECK_EQUAL(matchingPolicy.d_trigger, blockedWildcardName);
+    BOOST_CHECK_EQUAL(matchingPolicy.d_hit, "sub.sub.wildcard-blocked");
 
     /* looking for wildcard-blocked. should not match *.wildcard-blocked. */
     const auto notMatchingPolicy = dfe.getQueryPolicy(DNSName("wildcard-blocked."), std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
@@ -154,6 +160,8 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     /* except if we look exactly for the wildcard */
     BOOST_CHECK(zone->findExactQNamePolicy(blockedWildcardName, zonePolicy));
     BOOST_CHECK(zonePolicy == matchingPolicy);
+    BOOST_CHECK_EQUAL(zonePolicy.d_trigger, blockedWildcardName);
+    BOOST_CHECK_EQUAL(zonePolicy.d_hit, blockedWildcardName.toStringNoDot());
   }
 
   {
@@ -161,20 +169,18 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     const auto matchingPolicy = dfe.getClientPolicy(clientIP, std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ClientIP);
     BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Drop);
-    Netmask key;
     DNSFilterEngine::Policy zonePolicy;
-    BOOST_CHECK(zone->findClientPolicy(clientIP, key, zonePolicy));
-    BOOST_CHECK(key == clientIP);
+    BOOST_CHECK(zone->findClientPolicy(clientIP, zonePolicy));
     BOOST_CHECK(zonePolicy == matchingPolicy);
+    BOOST_CHECK_EQUAL(zonePolicy.d_trigger, DNSName("31.128.2.0.192.rpz-client-ip"));
   }
 
   {
     /* not blocked */
     const auto matchingPolicy = dfe.getClientPolicy(ComboAddress("192.0.2.142"), std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::None);
-    Netmask key;
     DNSFilterEngine::Policy zonePolicy;
-    BOOST_CHECK(zone->findClientPolicy(ComboAddress("192.0.2.142"), key, zonePolicy) == false);
+    BOOST_CHECK(zone->findClientPolicy(ComboAddress("192.0.2.142"), zonePolicy) == false);
     BOOST_CHECK(zone->findExactQNamePolicy(DNSName("totally.legit."), zonePolicy) == false);
   }
 
@@ -186,11 +192,10 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     const auto matchingPolicy = dfe.getPostPolicy({dr}, std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ResponseIP);
     BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Drop);
-    Netmask key;
     DNSFilterEngine::Policy zonePolicy;
-    BOOST_CHECK(zone->findResponsePolicy(responseIP, key, zonePolicy));
-    BOOST_CHECK(key == responseIP);
+    BOOST_CHECK(zone->findResponsePolicy(responseIP, zonePolicy));
     BOOST_CHECK(zonePolicy == matchingPolicy);
+    BOOST_CHECK_EQUAL(zonePolicy.d_trigger, DNSName("31.254.2.0.192.rpz-ip"));
   }
 
   {
@@ -200,25 +205,24 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_basic)
     dr.d_content = DNSRecordContent::mastermake(QType::A, QClass::IN, "192.0.2.142");
     const auto matchingPolicy = dfe.getPostPolicy({dr}, std::unordered_map<std::string, bool>(), DNSFilterEngine::maximumPriority);
     BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::None);
-    Netmask key;
     DNSFilterEngine::Policy zonePolicy;
-    BOOST_CHECK(zone->findResponsePolicy(ComboAddress("192.0.2.142"), key, zonePolicy) == false);
+    BOOST_CHECK(zone->findResponsePolicy(ComboAddress("192.0.2.142"), zonePolicy) == false);
   }
 
   BOOST_CHECK_EQUAL(zone->size(), 7U);
-  zone->rmClientTrigger(Netmask(clientIP, 32), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ClientIP));
+  zone->rmClientTrigger(Netmask(clientIP, 31), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ClientIP));
   BOOST_CHECK_EQUAL(zone->size(), 6U);
   zone->rmQNameTrigger(blockedName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::QName));
   BOOST_CHECK_EQUAL(zone->size(), 5U);
   zone->rmQNameTrigger(blockedWildcardName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::QName));
   BOOST_CHECK_EQUAL(zone->size(), 4U);
-  zone->rmNSIPTrigger(Netmask(nsIP, 32), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSIP));
+  zone->rmNSIPTrigger(Netmask(nsIP, 31), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSIP));
   BOOST_CHECK_EQUAL(zone->size(), 3U);
   zone->rmNSTrigger(nsName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSDName));
   BOOST_CHECK_EQUAL(zone->size(), 2U);
   zone->rmNSTrigger(nsWildcardName, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::NSDName));
   BOOST_CHECK_EQUAL(zone->size(), 1U);
-  zone->rmResponseTrigger(Netmask(responseIP, 32), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ResponseIP));
+  zone->rmResponseTrigger(Netmask(responseIP, 31), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Drop, DNSFilterEngine::PolicyType::ResponseIP));
   BOOST_CHECK_EQUAL(zone->size(), 0U);
 
   /* DNSFilterEngine::clear() calls clear() on all zones, but keeps the zones */