From f6a282680c7e5814e89e742ec2fc501840ac0569 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 19 Aug 2020 10:50:16 +0200 Subject: [PATCH] Add unit test for matching on netmask. Also: fix the test to remove an entire entry. That should only be done if there are no custom records left after the cleanup. Old code was too eager and removed the entry whenever there was only one custom record left. We could be trying to remove a non-matching one. --- pdns/filterpo.cc | 21 +++-- pdns/recursordist/test-filterpo_cc.cc | 123 ++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 9 deletions(-) diff --git a/pdns/filterpo.cc b/pdns/filterpo.cc index 3c39947b03..4966b73d7a 100644 --- a/pdns/filterpo.cc +++ b/pdns/filterpo.cc @@ -380,11 +380,6 @@ bool DNSFilterEngine::Zone::rmNameTrigger(std::unordered_map& ma /* for custom types, we might have more than one type, and then we need to remove only the right ones. */ - if (existing.d_custom.size() <= 1) { - map.erase(found); - return true; - } - bool result = false; for (auto& toRemove : pol.d_custom) { for (auto it = existing.d_custom.begin(); it != existing.d_custom.end(); ++it) { @@ -396,6 +391,12 @@ bool DNSFilterEngine::Zone::rmNameTrigger(std::unordered_map& ma } } + // No records left for this trigger? + if (existing.d_custom.size() == 0) { + map.erase(found); + return true; + } + return result; } @@ -416,10 +417,6 @@ bool DNSFilterEngine::Zone::rmNetmaskTrigger(NetmaskTree& nmt, const Net /* for custom types, we might have more than one type, and then we need to remove only the right ones. */ - if (existing.d_custom.size() <= 1) { - nmt.erase(nm); - return true; - } bool result = false; for (auto& toRemove : pol.d_custom) { @@ -432,6 +429,12 @@ bool DNSFilterEngine::Zone::rmNetmaskTrigger(NetmaskTree& nmt, const Net } } + // No records left for this trigger? + if (existing.d_custom.size() == 0) { + nmt.erase(nm); + return true; + } + return result; } diff --git a/pdns/recursordist/test-filterpo_cc.cc b/pdns/recursordist/test-filterpo_cc.cc index c95738cafb..83d24f32bc 100644 --- a/pdns/recursordist/test-filterpo_cc.cc +++ b/pdns/recursordist/test-filterpo_cc.cc @@ -417,6 +417,129 @@ BOOST_AUTO_TEST_CASE(test_filter_policies_local_data) } } +BOOST_AUTO_TEST_CASE(test_filter_policies_local_data_netmask) +{ + DNSFilterEngine dfe; + + std::string zoneName("Unit test policy local data using netmasks"); + auto zone = std::make_shared(); + zone->setName(zoneName); + + const DNSName name("foo.example.com"); + const Netmask nm1("192.168.1.0/24"); + + zone->addClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.4")})); + zone->addClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.5")})); + zone->addClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::AAAA, QClass::IN, "::1234")})); + BOOST_CHECK_EQUAL(zone->size(), 1U); + + dfe.addZone(zone); + + { // A query should match two records + const auto matchingPolicy = dfe.getQueryPolicy(name, ComboAddress("192.168.1.1"), std::unordered_map(), DNSFilterEngine::maximumPriority); + BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ClientIP); + BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Custom); + auto records = matchingPolicy.getCustomRecords(DNSName(), QType::A); + BOOST_CHECK_EQUAL(records.size(), 2U); + const auto& record1 = records.at(0); + BOOST_CHECK(record1.d_type == QType::A); + BOOST_CHECK(record1.d_class == QClass::IN); + auto content1 = std::dynamic_pointer_cast(record1.d_content); + BOOST_CHECK(content1 != nullptr); + BOOST_CHECK_EQUAL(content1->getCA().toString(), "1.2.3.4"); + + const auto& record2 = records.at(1); + BOOST_CHECK(record2.d_type == QType::A); + BOOST_CHECK(record2.d_class == QClass::IN); + auto content2 = std::dynamic_pointer_cast(record2.d_content); + BOOST_CHECK(content2 != nullptr); + BOOST_CHECK_EQUAL(content2->getCA().toString(), "1.2.3.5"); + } + + { // AAAA query should match 1 record + const auto matchingPolicy = dfe.getQueryPolicy(name, ComboAddress("192.168.1.1"), std::unordered_map(), DNSFilterEngine::maximumPriority); + BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ClientIP); + BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Custom); + auto records = matchingPolicy.getCustomRecords(DNSName(), QType::AAAA); + BOOST_CHECK_EQUAL(records.size(), 1U); + const auto& record1 = records.at(0); + BOOST_CHECK(record1.d_type == QType::AAAA); + BOOST_CHECK(record1.d_class == QClass::IN); + auto content1 = std::dynamic_pointer_cast(record1.d_content); + BOOST_CHECK(content1 != nullptr); + BOOST_CHECK_EQUAL(content1->getCA().toString(), "::1234"); + } + + // Try to zap 1 nonexisting record + zone->rmClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.1.1.1")})); + + // Zap a record using a wider netmask + zone->rmClientTrigger(Netmask("192.168.0.0/16"), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.4")})); + + // Zap a record using a narrow netmask + zone->rmClientTrigger(Netmask("192.168.1.1/32"), DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.4")})); + + // Zap 1 existing record + zone->rmClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.5")})); + + { // A query should match one record now + const auto matchingPolicy = dfe.getQueryPolicy(name, ComboAddress("192.168.1.1"), std::unordered_map(), DNSFilterEngine::maximumPriority); + BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ClientIP); + BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Custom); + auto records = matchingPolicy.getCustomRecords(DNSName(), QType::A); + BOOST_CHECK_EQUAL(records.size(), 1U); + const auto& record1 = records.at(0); + BOOST_CHECK(record1.d_type == QType::A); + BOOST_CHECK(record1.d_class == QClass::IN); + auto content1 = std::dynamic_pointer_cast(record1.d_content); + BOOST_CHECK(content1 != nullptr); + BOOST_CHECK_EQUAL(content1->getCA().toString(), "1.2.3.4"); + } + { // AAAA query should still match one record + const auto matchingPolicy = dfe.getQueryPolicy(name, ComboAddress("192.168.1.1"), std::unordered_map(), DNSFilterEngine::maximumPriority); + BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ClientIP); + BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Custom); + auto records = matchingPolicy.getCustomRecords(DNSName(), QType::AAAA); + BOOST_CHECK_EQUAL(records.size(), 1U); + const auto& record1 = records.at(0); + BOOST_CHECK(record1.d_type == QType::AAAA); + BOOST_CHECK(record1.d_class == QClass::IN); + auto content1 = std::dynamic_pointer_cast(record1.d_content); + BOOST_CHECK(content1 != nullptr); + BOOST_CHECK_EQUAL(content1->getCA().toString(), "::1234"); + } + + // Zap one more A record + zone->rmClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.4")})); + + // Zap now nonexisting record + zone->rmClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::A, QClass::IN, "1.2.3.4")})); + + { // AAAA query should still match one record + const auto matchingPolicy = dfe.getQueryPolicy(name, ComboAddress("192.168.1.1"), std::unordered_map(), DNSFilterEngine::maximumPriority); + BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::ClientIP); + BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::Custom); + auto records = matchingPolicy.getCustomRecords(DNSName(), QType::AAAA); + BOOST_CHECK_EQUAL(records.size(), 1U); + const auto& record1 = records.at(0); + BOOST_CHECK(record1.d_type == QType::AAAA); + BOOST_CHECK(record1.d_class == QClass::IN); + auto content1 = std::dynamic_pointer_cast(record1.d_content); + BOOST_CHECK(content1 != nullptr); + BOOST_CHECK_EQUAL(content1->getCA().toString(), "::1234"); + } + + // Zap AAAA record + zone->rmClientTrigger(nm1, DNSFilterEngine::Policy(DNSFilterEngine::PolicyKind::Custom, DNSFilterEngine::PolicyType::ClientIP, 0, nullptr, {DNSRecordContent::mastermake(QType::AAAA, QClass::IN, "::1234")})); + + { // there should be no match left + const auto matchingPolicy = dfe.getQueryPolicy(name, ComboAddress("192.168.1.1"), std::unordered_map(), DNSFilterEngine::maximumPriority); + BOOST_CHECK(matchingPolicy.d_type == DNSFilterEngine::PolicyType::None); + BOOST_CHECK(matchingPolicy.d_kind == DNSFilterEngine::PolicyKind::NoAction); + } + +} + BOOST_AUTO_TEST_CASE(test_multiple_filter_policies) { DNSFilterEngine dfe; -- 2.47.2