New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rec: correct appliedPolicyTrigger value for IP matches #10842
Changes from 2 commits
b143b5f
9524d9c
562c1c1
f9de1f7
e4387f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,27 +53,30 @@ bool DNSFilterEngine::Zone::findExactNSPolicy(const DNSName& qname, DNSFilterEng | |
return findExactNamedPolicy(d_propolName, qname, pol); | ||
} | ||
|
||
bool DNSFilterEngine::Zone::findNSIPPolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const | ||
bool DNSFilterEngine::Zone::findNSIPPolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const | ||
{ | ||
if (const auto fnd = d_propolNSAddr.lookup(addr)) { | ||
key = fnd->first; | ||
pol = fnd->second; | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
bool DNSFilterEngine::Zone::findResponsePolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const | ||
bool DNSFilterEngine::Zone::findResponsePolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const | ||
{ | ||
if (const auto fnd = d_postpolAddr.lookup(addr)) { | ||
key = fnd->first; | ||
pol = fnd->second; | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
bool DNSFilterEngine::Zone::findClientPolicy(const ComboAddress& addr, DNSFilterEngine::Policy& pol) const | ||
bool DNSFilterEngine::Zone::findClientPolicy(const ComboAddress& addr, Netmask& key, DNSFilterEngine::Policy& pol) const | ||
{ | ||
if (const auto fnd = d_qpolAddr.lookup(addr)) { | ||
key = fnd->first; | ||
pol = fnd->second; | ||
return true; | ||
} | ||
|
@@ -212,10 +215,10 @@ bool DNSFilterEngine::getProcessingPolicy(const ComboAddress& address, const std | |
continue; | ||
} | ||
|
||
if(z->findNSIPPolicy(address, pol)) { | ||
Netmask key; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leftover? |
||
if(z->findNSIPPolicy(address, key, pol)) { | ||
// cerr<<"Had a hit on the nameserver ("<<address.toString()<<") used to process the query"<<endl; | ||
// XXX should use ns RPZ | ||
pol.d_trigger = Zone::maskToRPZ(address); | ||
pol.d_trigger = Zone::maskToRPZ(key); | ||
pol.d_trigger.appendRawLabel(rpzNSIPName); | ||
pol.d_hit = address.toString(); | ||
return true; | ||
|
@@ -236,7 +239,8 @@ bool DNSFilterEngine::getClientPolicy(const ComboAddress& ca, const std::unorder | |
continue; | ||
} | ||
|
||
if (z->findClientPolicy(ca, pol)) { | ||
Netmask key; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leftover as well? |
||
if (z->findClientPolicy(ca, key, pol)) { | ||
// cerr<<"Had a hit on the IP address ("<<ca.toString()<<") of the client"<<endl; | ||
rgacogne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return true; | ||
} | ||
|
@@ -355,8 +359,9 @@ bool DNSFilterEngine::getPostPolicy(const DNSRecord& record, const std::unordere | |
return false; | ||
} | ||
|
||
if (z->findResponsePolicy(ca, pol)) { | ||
pol.d_trigger = Zone::maskToRPZ(ca); | ||
Netmask key; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leftover? |
||
if (z->findResponsePolicy(ca, key, pol)) { | ||
pol.d_trigger = Zone::maskToRPZ(key); | ||
pol.d_trigger.appendRawLabel(rpzIPName); | ||
pol.d_hit = ca.toString(); | ||
return true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,17 +107,20 @@ 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, zonePolicy)); | ||
BOOST_CHECK(zone->findNSIPPolicy(nsIP, key, zonePolicy)); | ||
BOOST_CHECK(key == nsIP); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like all our tests are using an exact match (/32), perhaps we should test with at least one different mask? I know we do in the regression tests, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extended the tests and found another case where the trigger value was not set properly: Now I'm wondering if |
||
BOOST_CHECK(zonePolicy == matchingPolicy); | ||
} | ||
|
||
{ | ||
/* 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"), zonePolicy) == false); | ||
BOOST_CHECK(zone->findNSIPPolicy(ComboAddress("192.0.2.142"), key, zonePolicy) == false); | ||
} | ||
|
||
{ | ||
|
@@ -158,17 +161,20 @@ 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, zonePolicy)); | ||
BOOST_CHECK(zone->findClientPolicy(clientIP, key, zonePolicy)); | ||
BOOST_CHECK(key == clientIP); | ||
BOOST_CHECK(zonePolicy == matchingPolicy); | ||
} | ||
|
||
{ | ||
/* 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"), zonePolicy) == false); | ||
BOOST_CHECK(zone->findClientPolicy(ComboAddress("192.0.2.142"), key, zonePolicy) == false); | ||
BOOST_CHECK(zone->findExactQNamePolicy(DNSName("totally.legit."), zonePolicy) == false); | ||
} | ||
|
||
|
@@ -180,8 +186,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, zonePolicy)); | ||
BOOST_CHECK(zone->findResponsePolicy(responseIP, key, zonePolicy)); | ||
BOOST_CHECK(key == responseIP); | ||
BOOST_CHECK(zonePolicy == matchingPolicy); | ||
} | ||
|
||
|
@@ -192,8 +200,9 @@ 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"), zonePolicy) == false); | ||
BOOST_CHECK(zone->findResponsePolicy(ComboAddress("192.0.2.142"), key, zonePolicy) == false); | ||
} | ||
|
||
BOOST_CHECK_EQUAL(zone->size(), 7U); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to update the
d_trigger
field of the returned policy here, instead of exposing the internal key?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is a good idea.