Attachment 8845 Details for Bug 41190

[patch] 41190-dns-correct-tombstoned-461.patch

41190-dns-correct-tombstoned-461.patch (text/plain), 13.37 KB, created by Lukas Oyen on 2017-05-12 15:17 CEST

(hide)

Description:

Filename:

MIME Type:

Creator: Lukas Oyen

Created: 2017-05-12 15:17 CEST

Size: 13.37 KB

patch

obsolete

>From 450197f65ae4af3a9ef34482b16895c12f04c6ae Mon Sep 17 00:00:00 2001
>From: Lukas Oyen <oyen@univention.de>
>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 <oyen@univention.de>
>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 <oyen@univention.de>
>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
>

Actions: View | Diff

Attachments on bug 41190: 7635 | 8828 | 8836 | 8845