From 450197f65ae4af3a9ef34482b16895c12f04c6ae Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Tue, 9 May 2017 17:36:11 +0200 Subject: [PATCH 1/3] Bug #41190: dns: `dns_common_replace()` add `tombstone` parameter The old `dns_common_replace()` assumed a valid object state: whenever a `DNS_TYPE_TOMBSTON` marker is present, `dnsTombstoned` is set to `TRUE` and vice versa. A LDAP modification of the object, setting `dnsTombstoned=TRUE`, without adding the markeer, produces a state, where updates via the `bind9_dlz` module adds records, without deleting old obsolete ones. This patch introduces a `tombstone` parameter to `dns_common_replace()` (and the wrapper `dns_replace_records()` and the Python wrapper) that marks whether `dnsTombstoned=TRUE`. `dns_common_replace()` will now reset `dnsTombstoned=FALSE`, if no `DNS_TYPE_TOMBSTONE` marker is found, but valid records exist. Although not explicitly documented in [MS-DNSP], this is the behaviour of a Windows Server 2012. --- python/samba/samdb.py | 8 ++++---- source4/dns_server/dlz_bind9.c | 8 +++++--- source4/dns_server/dns_server.h | 1 + source4/dns_server/dns_update.c | 18 ++++++++++++------ source4/dns_server/dns_utils.c | 4 +++- source4/dns_server/dnsserver_common.c | 9 +++++---- source4/dns_server/dnsserver_common.h | 1 + source4/dns_server/pydns.c | 10 ++++++++-- 8 files changed, 39 insertions(+), 20 deletions(-) diff --git a/python/samba/samdb.py b/python/samba/samdb.py index 19dd8e9..b1c4094 100644 --- a/python/samba/samdb.py +++ b/python/samba/samdb.py @@ -935,20 +935,20 @@ accountExpires: %u '''Return the NDR database structures from a dnsRecord element''' return dsdb_dns.extract(el) - def dns_replace(self, dns_name, new_records): + def dns_replace(self, dns_name, new_records, tombstone=False): '''Do a DNS modification on the database, sets the NDR database structures on a DNS name ''' - return dsdb_dns.replace(self, dns_name, new_records) + return dsdb_dns.replace(self, dns_name, new_records, tombstone) - def dns_replace_by_dn(self, dn, new_records): + def dns_replace_by_dn(self, dn, new_records, tombstone=False): '''Do a DNS modification on the database, sets the NDR database structures on a LDB DN This routine is important because if the last record on the DN is removed, this routine will put a tombstone in the record. ''' - return dsdb_dns.replace_by_dn(self, dn, new_records) + return dsdb_dns.replace_by_dn(self, dn, new_records, tombstone) def garbage_collect_tombstones(self, dn, current_time, tombstone_lifetime=None): diff --git a/source4/dns_server/dlz_bind9.c b/source4/dns_server/dlz_bind9.c index 4c21a5e..964dbce 100644 --- a/source4/dns_server/dlz_bind9.c +++ b/source4/dns_server/dlz_bind9.c @@ -1637,7 +1637,7 @@ _PUBLIC_ isc_result_t dlz_addrdataset(const char *name, const char *rdatastr, vo /* modify the record */ werr = dns_common_replace(state->samdb, rec, dn, - needs_add, + needs_add, tombstoned, state->soa_serial, recs, num_recs); b9_reset_session_info(state); @@ -1720,7 +1720,8 @@ _PUBLIC_ isc_result_t dlz_subrdataset(const char *name, const char *rdatastr, vo /* modify the record */ werr = dns_common_replace(state->samdb, rec, dn, - false,/* needs_add */ + false, /* needs_add */ + false, /* tombstoned */ state->soa_serial, recs, num_recs); b9_reset_session_info(state); @@ -1804,7 +1805,8 @@ _PUBLIC_ isc_result_t dlz_delrdataset(const char *name, const char *type, void * /* modify the record */ werr = dns_common_replace(state->samdb, tmp_ctx, dn, - false,/* needs_add */ + false, /* needs_add */ + false, /* tombstoned */ state->soa_serial, recs, num_recs); b9_reset_session_info(state); diff --git a/source4/dns_server/dns_server.h b/source4/dns_server/dns_server.h index e623f97..5c8e649 100644 --- a/source4/dns_server/dns_server.h +++ b/source4/dns_server/dns_server.h @@ -97,6 +97,7 @@ WERROR dns_replace_records(struct dns_server *dns, TALLOC_CTX *mem_ctx, struct ldb_dn *dn, bool needs_add, + bool tombstoned, struct dnsp_DnssrvRpcRecord *records, uint16_t rec_count); WERROR dns_name2dn(struct dns_server *dns, diff --git a/source4/dns_server/dns_update.c b/source4/dns_server/dns_update.c index bb41a9c..240f8d0 100644 --- a/source4/dns_server/dns_update.c +++ b/source4/dns_server/dns_update.c @@ -456,7 +456,8 @@ static WERROR handle_one_update(struct dns_server *dns, rcount += 1; werror = dns_replace_records(dns, mem_ctx, dn, - needs_add, recs, rcount); + needs_add, tombstoned, + recs, rcount); W_ERROR_NOT_OK_RETURN(werror); return WERR_OK; @@ -517,7 +518,8 @@ static WERROR handle_one_update(struct dns_server *dns, } werror = dns_replace_records(dns, mem_ctx, dn, - needs_add, recs, rcount); + needs_add, tombstoned, + recs, rcount); W_ERROR_NOT_OK_RETURN(werror); return WERR_OK; @@ -538,14 +540,16 @@ static WERROR handle_one_update(struct dns_server *dns, recs[i] = recs[rcount]; werror = dns_replace_records(dns, mem_ctx, dn, - needs_add, recs, rcount); + needs_add, tombstoned, + recs, rcount); W_ERROR_NOT_OK_RETURN(werror); return WERR_OK; } werror = dns_replace_records(dns, mem_ctx, dn, - needs_add, recs, rcount+1); + needs_add, tombstoned, recs, + rcount+1); W_ERROR_NOT_OK_RETURN(werror); return WERR_OK; @@ -594,7 +598,8 @@ static WERROR handle_one_update(struct dns_server *dns, } werror = dns_replace_records(dns, mem_ctx, dn, - needs_add, recs, rcount); + needs_add, tombstoned, recs, + rcount); W_ERROR_NOT_OK_RETURN(werror); return WERR_OK; @@ -640,7 +645,8 @@ static WERROR handle_one_update(struct dns_server *dns, } werror = dns_replace_records(dns, mem_ctx, dn, - needs_add, recs, rcount); + needs_add, tombstoned, recs, + rcount); W_ERROR_NOT_OK_RETURN(werror); } diff --git a/source4/dns_server/dns_utils.c b/source4/dns_server/dns_utils.c index c728eaa..6f203ad 100644 --- a/source4/dns_server/dns_utils.c +++ b/source4/dns_server/dns_utils.c @@ -121,13 +121,15 @@ WERROR dns_replace_records(struct dns_server *dns, TALLOC_CTX *mem_ctx, struct ldb_dn *dn, bool needs_add, + bool tombstoned, struct dnsp_DnssrvRpcRecord *records, uint16_t rec_count) { /* TODO: Autogenerate this somehow */ uint32_t dwSerial = 110; return dns_common_replace(dns->samdb, mem_ctx, dn, - needs_add, dwSerial, records, rec_count); + needs_add, tombstoned, dwSerial, records, + rec_count); } bool dns_authoritative_for_zone(struct dns_server *dns, diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 7aac7e2..e46d2bb 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -303,6 +303,7 @@ WERROR dns_common_replace(struct ldb_context *samdb, TALLOC_CTX *mem_ctx, struct ldb_dn *dn, bool needs_add, + bool tombstoned, uint32_t serial, struct dnsp_DnssrvRpcRecord *records, uint16_t rec_count) @@ -312,7 +313,7 @@ WERROR dns_common_replace(struct ldb_context *samdb, int ret; WERROR werr; struct ldb_message *msg = NULL; - bool was_tombstoned = false; + bool tombstoned_marker = false; bool become_tombstoned = false; msg = ldb_msg_new(mem_ctx); @@ -352,7 +353,7 @@ WERROR dns_common_replace(struct ldb_context *samdb, if (records[i].wType == DNS_TYPE_TOMBSTONE) { if (records[i].data.timestamp != 0) { - was_tombstoned = true; + tombstoned_marker = true; } continue; } @@ -391,7 +392,7 @@ WERROR dns_common_replace(struct ldb_context *samdb, enum ndr_err_code ndr_err; struct timeval tv; - if (was_tombstoned) { + if (tombstoned && tombstoned_marker) { /* * This is already a tombstoned object. * Just leave it instead of updating the time stamp. @@ -417,7 +418,7 @@ WERROR dns_common_replace(struct ldb_context *samdb, become_tombstoned = true; } - if (was_tombstoned || become_tombstoned) { + if (tombstoned || tombstoned_marker || become_tombstoned) { ret = ldb_msg_add_empty(msg, "dNSTombstoned", LDB_FLAG_MOD_REPLACE, NULL); if (ret != LDB_SUCCESS) { diff --git a/source4/dns_server/dnsserver_common.h b/source4/dns_server/dnsserver_common.h index 57d5d9f..26e922a 100644 --- a/source4/dns_server/dnsserver_common.h +++ b/source4/dns_server/dnsserver_common.h @@ -53,6 +53,7 @@ WERROR dns_common_replace(struct ldb_context *samdb, TALLOC_CTX *mem_ctx, struct ldb_dn *dn, bool needs_add, + bool tombstoned, uint32_t serial, struct dnsp_DnssrvRpcRecord *records, uint16_t rec_count); diff --git a/source4/dns_server/pydns.c b/source4/dns_server/pydns.c index 9842f24..1a7d65c 100644 --- a/source4/dns_server/pydns.c +++ b/source4/dns_server/pydns.c @@ -195,6 +195,7 @@ static PyObject *py_dsdb_dns_replace(PyObject *self, PyObject *args) struct ldb_dn *dn; struct dnsp_DnssrvRpcRecord *records; uint16_t num_records; + bool tombstone = false; /* * TODO: This is a shocking abuse, but matches what the @@ -203,7 +204,8 @@ static PyObject *py_dsdb_dns_replace(PyObject *self, PyObject *args) */ static const int serial = 110; - if (!PyArg_ParseTuple(args, "OsO", &py_ldb, &dns_name, &py_dns_records)) { + if (!PyArg_ParseTuple(args, "OsO|b", &py_ldb, &dns_name, + &py_dns_records, &tombstone)) { return NULL; } PyErr_LDB_OR_RAISE(py_ldb, samdb); @@ -233,6 +235,7 @@ static PyObject *py_dsdb_dns_replace(PyObject *self, PyObject *args) frame, dn, false, /* Not adding a record */ + tombstone, serial, records, num_records); @@ -254,6 +257,7 @@ static PyObject *py_dsdb_dns_replace_by_dn(PyObject *self, PyObject *args) struct ldb_dn *dn; struct dnsp_DnssrvRpcRecord *records; uint16_t num_records; + bool tombstone = false; /* * TODO: This is a shocking abuse, but matches what the @@ -262,7 +266,8 @@ static PyObject *py_dsdb_dns_replace_by_dn(PyObject *self, PyObject *args) */ static const int serial = 110; - if (!PyArg_ParseTuple(args, "OOO", &py_ldb, &py_dn, &py_dns_records)) { + if (!PyArg_ParseTuple(args, "OOO|b", &py_ldb, &py_dn, &py_dns_records, + &tombstone)) { return NULL; } PyErr_LDB_OR_RAISE(py_ldb, samdb); @@ -282,6 +287,7 @@ static PyObject *py_dsdb_dns_replace_by_dn(PyObject *self, PyObject *args) frame, dn, false, /* Not adding a record */ + tombstone, serial, records, num_records); -- 2.7.4 From ec12a8666fbc455dcdb7c04b68839ca87e133b69 Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Tue, 9 May 2017 18:10:59 +0200 Subject: [PATCH 2/3] Bug #41190: dns: only skip record if `DNS_TYPE_TOMBSTONE` `dns_common_replace()` always sorts the records by `wType` and `dwTimeStamp`, therefor a `DNS_TYPE_TOMBSTONE` should always be the first record. But if `dnsTombstoned=TRUE`, without a `DNS_TYPE_TOMBSTONE` marker, a valid record is skipped. This patch only skips the a record, if `dnsTombstoned=TRUE` and `wType==DNS_TYPE_TOMBSTONE`. --- source4/dns_server/dlz_bind9.c | 2 +- source4/dns_server/dns_update.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/dns_server/dlz_bind9.c b/source4/dns_server/dlz_bind9.c index 964dbce..6b9af82 100644 --- a/source4/dns_server/dlz_bind9.c +++ b/source4/dns_server/dlz_bind9.c @@ -1593,7 +1593,7 @@ _PUBLIC_ isc_result_t dlz_addrdataset(const char *name, const char *rdatastr, vo return ISC_R_FAILURE; } - if (tombstoned) { + if (tombstoned && num_recs > 0 && recs[num_recs - 1].wType == DNS_TYPE_TOMBSTONE) { /* * we need to keep the existing tombstone record * and ignore it diff --git a/source4/dns_server/dns_update.c b/source4/dns_server/dns_update.c index 240f8d0..52ef0da 100644 --- a/source4/dns_server/dns_update.c +++ b/source4/dns_server/dns_update.c @@ -419,7 +419,7 @@ static WERROR handle_one_update(struct dns_server *dns, } W_ERROR_NOT_OK_RETURN(werror); - if (tombstoned) { + if (tombstoned && rcount > 0 && recs[rcount - 1].wType == DNS_TYPE_TOMBSTONE) { /* * we need to keep the existing tombstone record * and ignore it -- 2.7.4 From 5f4e51d7b2eb01aca93a139b4402726817c71b89 Mon Sep 17 00:00:00 2001 From: Lukas Oyen Date: Wed, 10 May 2017 16:51:01 +0200 Subject: [PATCH 3/3] Bug #41190: pytest dnsserver: fix tombstone test --- python/samba/tests/dcerpc/dnsserver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py index 9602066..58a82bf 100644 --- a/python/samba/tests/dcerpc/dnsserver.py +++ b/python/samba/tests/dcerpc/dnsserver.py @@ -192,7 +192,7 @@ class DnsserverTests(RpcInterfaceTestCase): dn, record = self.get_record_from_db(self.custom_zone, "testrecord") record.wType = dnsp.DNS_TYPE_TOMBSTONE - res = self.samdb.dns_replace_by_dn(dn, [record]) + res = self.samdb.dns_replace_by_dn(dn, [record], True) if res is not None: self.fail("Unable to update dns record to be tombstoned.") -- 2.7.4