From: Chris Hofstaedtler Date: Fri, 8 Feb 2019 10:51:31 +0000 (+0100) Subject: Auth. API: improve RRset validation X-Git-Tag: auth-4.2.0-beta1~26^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=646bcd7d364688f9556b8d92d4918e4211b47e27;p=thirdparty%2Fpdns.git Auth. API: improve RRset validation Refuse to load RRset arrays that contain more than one of CNAME or DNAME. Expand refusal to load CNAMEs to pre-existing RRsets also to DNAMEs, and vice versa. Refuse to load zones given in BIND format for the same reasons. Apply hostname correctness check also to BIND format zones. --- diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc index 4ef6708339..b2ab3a2d86 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -56,6 +56,11 @@ static void patchZone(HttpRequest* req, HttpResponse* resp); static void storeChangedPTRs(UeberBackend& B, vector& new_ptrs); static void makePtr(const DNSResourceRecord& rr, DNSResourceRecord* ptr); +// QTypes that MUST NOT have multiple records of the same type in a given RRset. +static const std::set onlyOneEntryTypes = { QType::CNAME, QType::DNAME, QType::SOA }; +// QTypes that MUST NOT be used with any other QType on the same name. +static const std::set exclusiveEntryTypes = { QType::CNAME, QType::DNAME }; + AuthWebServer::AuthWebServer() : d_tid(0), d_start(time(nullptr)), @@ -505,7 +510,6 @@ static void validateGatheredRRType(const DNSResourceRecord& rr) { } static void gatherRecords(const Json container, const DNSName& qname, const QType qtype, const int ttl, vector& new_records, vector& new_ptrs) { - static const std::set onlyOneEntryTypes = { QType::CNAME, QType::SOA }; UeberBackend B; DNSResourceRecord rr; rr.qname = qname; @@ -515,10 +519,6 @@ static void gatherRecords(const Json container, const DNSName& qname, const QTyp validateGatheredRRType(rr); const auto& items = container["records"].array_items(); - if (onlyOneEntryTypes.count(qtype.getCode()) != 0 && items.size() > 1) { - throw ApiException("RRset for "+rr.qname.toString()+"/"+rr.qtype.getName()+" has more than one record"); - } - for(const auto& record : items) { string content = stringFromJson(record, "content"); rr.disabled = boolFromJson(record, "disabled"); @@ -1299,21 +1299,44 @@ static void gatherRecordsFromZone(const std::string& zonestring, vector& records) { +static void checkNewRecords(vector& records) { sort(records.begin(), records.end(), [](const DNSResourceRecord& rec_a, const DNSResourceRecord& rec_b) -> bool { /* we need _strict_ weak ordering */ return std::tie(rec_a.qname, rec_a.qtype, rec_a.content) < std::tie(rec_b.qname, rec_b.qtype, rec_b.content); } ); + DNSResourceRecord previous; for(const auto& rec : records) { - if (previous.qtype == rec.qtype && previous.qname == rec.qname && previous.content == rec.content) { - throw ApiException("Duplicate record in RRset " + rec.qname.toString() + " IN " + rec.qtype.getName() + " with content \"" + rec.content + "\""); + if (previous.qname == rec.qname) { + if (previous.qtype == rec.qtype) { + if (onlyOneEntryTypes.count(rec.qtype.getCode()) != 0) { + throw ApiException("RRset "+rec.qname.toString()+" IN "+rec.qtype.getName()+" has more than one record"); + } + if (previous.content == rec.content) { + throw ApiException("Duplicate record in RRset " + rec.qname.toString() + " IN " + rec.qtype.getName() + " with content \"" + rec.content + "\""); + } + } else if (exclusiveEntryTypes.count(rec.qtype.getCode()) != 0 || exclusiveEntryTypes.count(previous.qtype.getCode()) != 0) { + throw ApiException("RRset "+rec.qname.toString()+" IN "+rec.qtype.getName()+": Conflicts with another RRset"); + } + } + + // Check if the DNSNames that should be hostnames, are hostnames + try { + checkHostnameCorrectness(rec); + } catch (const std::exception& e) { + throw ApiException("RRset "+rec.qname.toString()+" IN "+rec.qtype.getName() + " " + e.what()); } + previous = rec; } } @@ -1577,7 +1600,7 @@ static void apiServerZones(HttpRequest* req, HttpResponse* resp) { } } - checkDuplicateRecords(new_records); + checkNewRecords(new_records); if (boolFromJson(document, "dnssec", false)) { checkDefaultDNSSECAlgos(); @@ -1936,15 +1959,8 @@ static void patchZone(HttpRequest* req, HttpResponse* resp) { if (rr.qtype.getCode() == QType::SOA && rr.qname==zonename) { soa_edit_done = increaseSOARecord(rr, soa_edit_api_kind, soa_edit_kind); } - - // Check if the DNSNames that should be hostnames, are hostnames - try { - checkHostnameCorrectness(rr); - } catch (const std::exception& e) { - throw ApiException("RRset "+qname.toString()+" IN "+qtype.getName() + " " + e.what()); - } } - checkDuplicateRecords(new_records); + checkNewRecords(new_records); } if (replace_comments) { @@ -1963,10 +1979,10 @@ static void patchZone(HttpRequest* req, HttpResponse* resp) { if (qtype.getCode() == 0) { ent_present = true; } - if (qtype.getCode() == QType::CNAME && rr.qtype.getCode() != QType::CNAME) { - throw ApiException("RRset "+qname.toString()+" IN "+qtype.getName()+": Conflicts with pre-existing non-CNAME RRset"); - } else if (qtype.getCode() != QType::CNAME && rr.qtype.getCode() == QType::CNAME) { - throw ApiException("RRset "+qname.toString()+" IN "+qtype.getName()+": Conflicts with pre-existing CNAME RRset"); + if (qtype.getCode() != rr.qtype.getCode() + && (exclusiveEntryTypes.count(qtype.getCode()) != 0 + || exclusiveEntryTypes.count(rr.qtype.getCode()) != 0)) { + throw ApiException("RRset "+qname.toString()+" IN "+qtype.getName()+": Conflicts with pre-existing RRset"); } } diff --git a/regression-tests.api/requirements.txt b/regression-tests.api/requirements.txt index 2400fe710b..ca23d7f08a 100644 --- a/regression-tests.api/requirements.txt +++ b/regression-tests.api/requirements.txt @@ -1,2 +1,3 @@ requests==2.20.0 nose==1.3.0 +parameterized diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index a0e08f9b83..9c72fdc109 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -3,6 +3,7 @@ import json import time import unittest from copy import deepcopy +from parameterized import parameterized from pprint import pprint from test_helper import ApiTestCase, unique_zone_name, is_auth, is_recursor, get_db_records, pdnsutil_rectify @@ -697,7 +698,11 @@ class AuthZones(ApiTestCase, AuthZonesHelperMixin): self.assertEquals(data['name'], 'example.com.') def test_import_zone_broken(self): - payload = {} + payload = { + 'name': 'powerdns-broken.com', + 'kind': 'Master', + 'nameservers': [], + } payload['zone'] = """ ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 58571 flags: qr aa rd; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1 @@ -717,9 +722,6 @@ powerdns-broken.com. 3600 IN MX 0 xs.powerdns.com. powerdns-broken.com. 3600 IN NS powerdnssec1.ds9a.nl. powerdns-broken.com. 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 1343746984 10800 3600 604800 10800 """ - payload['name'] = 'powerdns-broken.com.' - payload['kind'] = 'Master' - payload['nameservers'] = [] r = self.session.post( self.url("/api/v1/servers/localhost/zones"), data=json.dumps(payload), @@ -728,17 +730,17 @@ powerdns-broken.com. 86400 IN SOA powerdnssec1.ds9a.nl. ahu def test_import_zone_axfr_outofzone(self): # Ensure we don't create out-of-zone records - name = unique_zone_name() - payload = {} + payload = { + 'name': unique_zone_name(), + 'kind': 'Master', + 'nameservers': [], + } payload['zone'] = """ -NAME 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 1343746984 10800 3600 604800 10800 -NAME 3600 IN NS powerdnssec2.ds9a.nl. +%NAME% 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 1343746984 10800 3600 604800 10800 +%NAME% 3600 IN NS powerdnssec2.ds9a.nl. example.org. 3600 IN AAAA 2001:888:2000:1d::2 -NAME 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 1343746984 10800 3600 604800 10800 -""".replace('NAME', name) - payload['name'] = name - payload['kind'] = 'Master' - payload['nameservers'] = [] +%NAME% 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 1343746984 10800 3600 604800 10800 +""".replace('%NAME%', payload['name']) r = self.session.post( self.url("/api/v1/servers/localhost/zones"), data=json.dumps(payload), @@ -747,7 +749,12 @@ NAME 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 134374 self.assertEqual(r.json()['error'], 'RRset example.org. IN AAAA: Name is out of zone') def test_import_zone_axfr(self): - payload = {} + payload = { + 'name': 'powerdns.com.', + 'kind': 'Master', + 'nameservers': [], + 'soa_edit_api': '', # turn off so exact SOA comparison works. + } payload['zone'] = """ ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 58571 ;; flags: qr aa rd; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1 @@ -767,10 +774,6 @@ powerdns.com. 3600 IN MX 0 xs.powerdns.com. powerdns.com. 3600 IN NS powerdnssec1.ds9a.nl. powerdns.com. 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.nl. 1343746984 10800 3600 604800 10800 """ - payload['name'] = 'powerdns.com.' - payload['kind'] = 'Master' - payload['nameservers'] = [] - payload['soa_edit_api'] = '' # turn off so exact SOA comparison works. r = self.session.post( self.url("/api/v1/servers/localhost/zones"), data=json.dumps(payload), @@ -806,7 +809,12 @@ powerdns.com. 86400 IN SOA powerdnssec1.ds9a.nl. ahu.ds9a.n self.assertEqual(dbrec['content'], 'powerdnssec1.ds9a.nl') def test_import_zone_bind(self): - payload = {} + payload = { + 'name': 'example.org.', + 'kind': 'Master', + 'nameservers': [], + 'soa_edit_api': '', # turn off so exact SOA comparison works. + } payload['zone'] = """ $TTL 86400 ; 24 hours could have been written as 24h or 1d ; $TTL used for all RRs without explicit TTL value @@ -829,10 +837,6 @@ ftp IN CNAME www.example.org. ;ftp server definition bill IN A 192.168.0.3 fred IN A 192.168.0.4 """ - payload['name'] = 'example.org.' - payload['kind'] = 'Master' - payload['nameservers'] = [] - payload['soa_edit_api'] = '' # turn off so exact SOA comparison works. r = self.session.post( self.url("/api/v1/servers/localhost/zones"), data=json.dumps(payload), @@ -865,6 +869,26 @@ fred IN A 192.168.0.4 eq_zone_rrsets(data['rrsets'], expected) + def test_import_zone_bind_cname_apex(self): + payload = { + 'name': unique_zone_name(), + 'kind': 'Master', + 'nameservers': [], + } + payload['zone'] = """ +$ORIGIN %NAME% +@ IN SOA ns1.example.org. hostmaster.example.org. (2002022401 3H 15 1W 3H) +@ IN NS ns1.example.org. +@ IN NS ns2.smokeyjoe.com. +@ IN CNAME www.example.org. +""".replace('%NAME%', payload['name']) + r = self.session.post( + self.url("/api/v1/servers/localhost/zones"), + data=json.dumps(payload), + headers={'content-type': 'application/json'}) + self.assertEquals(r.status_code, 422) + self.assertIn('Conflicts with another RRset', r.json()['error']) + def test_export_zone_json(self): name, payload, zone = self.create_zone(nameservers=['ns1.foo.com.', 'ns2.foo.com.'], soa_edit_api='') # export it @@ -1258,12 +1282,16 @@ fred IN A 192.168.0.4 self.assertEquals(r.status_code, 422) self.assertIn('unknown type', r.json()['error']) - def test_rrset_cname_and_other(self): + @parameterized.expand([ + ('CNAME', ), + ('DNAME', ), + ]) + def test_rrset_exclusive_and_other(self, qtype): name, payload, zone = self.create_zone() rrset = { 'changetype': 'replace', 'name': name, - 'type': 'CNAME', + 'type': qtype, 'ttl': 3600, 'records': [ { @@ -1276,14 +1304,18 @@ fred IN A 192.168.0.4 r = self.session.patch(self.url("/api/v1/servers/localhost/zones/" + name), data=json.dumps(payload), headers={'content-type': 'application/json'}) self.assertEquals(r.status_code, 422) - self.assertIn('Conflicts with pre-existing non-CNAME RRset', r.json()['error']) + self.assertIn('Conflicts with pre-existing RRset', r.json()['error']) - def test_rrset_other_and_cname(self): + @parameterized.expand([ + ('CNAME', ), + ('DNAME', ), + ]) + def test_rrset_other_and_exclusive(self, qtype): name, payload, zone = self.create_zone() rrset = { 'changetype': 'replace', 'name': 'sub.'+name, - 'type': 'CNAME', + 'type': qtype, 'ttl': 3600, 'records': [ { @@ -1312,22 +1344,27 @@ fred IN A 192.168.0.4 r = self.session.patch(self.url("/api/v1/servers/localhost/zones/" + name), data=json.dumps(payload), headers={'content-type': 'application/json'}) self.assertEquals(r.status_code, 422) - self.assertIn('Conflicts with pre-existing CNAME RRset', r.json()['error']) - - def test_rrset_multiple_cnames(self): + self.assertIn('Conflicts with pre-existing RRset', r.json()['error']) + + @parameterized.expand([ + ('SOA', ['ns1.example.org. test@example.org. 10 10800 3600 604800 3600', 'ns2.example.org. test@example.org. 10 10800 3600 604800 3600']), + ('CNAME', ['01.example.org.', '02.example.org.']), + ('DNAME', ['01.example.org.', '02.example.org.']), + ]) + def test_rrset_single_qtypes(self, qtype, contents): name, payload, zone = self.create_zone() rrset = { 'changetype': 'replace', 'name': 'sub.'+name, - 'type': 'CNAME', + 'type': qtype, 'ttl': 3600, 'records': [ { - "content": "01.example.org.", + "content": contents[0], "disabled": False }, { - "content": "02.example.org.", + "content": contents[1], "disabled": False } ] @@ -1336,7 +1373,7 @@ fred IN A 192.168.0.4 r = self.session.patch(self.url("/api/v1/servers/localhost/zones/" + name), data=json.dumps(payload), headers={'content-type': 'application/json'}) self.assertEquals(r.status_code, 422) - self.assertIn('/CNAME has more than one record', r.json()['error']) + self.assertIn('IN ' + qtype + ' has more than one record', r.json()['error']) def test_create_zone_with_leading_space(self): # Actual regression.