Attachment 13595 Details for Bug 12972 – Proposed change-set to fix the bug on 4.7

[patch] Proposed change-set to fix the bug on 4.7

4.7-bug-fix.txt (text/plain), 24.95 KB, created by Tim Beale on 2017-09-14 00:31:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tim Beale

Created: 2017-09-14 00:31:00 UTC

Size: 24.95 KB

patch

obsolete

>From d555e122ed7bd03bca974202fb52e583c390c3cd Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Wed, 13 Sep 2017 13:37:57 +1200
>Subject: [PATCH 1/5] getncchanges.py: Add a test for dropped cross-partition
> links
>
>Samba would drop linked attributes that span partitions if it didn't
>know about the target object. This patch adds a test that exposes
>the problem.
>
>I've backported getncchanges.py from master and stripped out the
>unnecessary tests/functionality. The functions in getncchanges.py may
>still look a bit overkill for a single test case
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> selftest/knownfail.d/getncchanges          |   3 +
> source4/selftest/tests.py                  |   5 +
> source4/torture/drs/python/getncchanges.py | 149 +++++++++++++++++++++++++++++
> 3 files changed, 157 insertions(+)
> create mode 100644 selftest/knownfail.d/getncchanges
> create mode 100644 source4/torture/drs/python/getncchanges.py
>
>diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges
>new file mode 100644
>index 0000000..303400f
>--- /dev/null
>+++ b/selftest/knownfail.d/getncchanges
>@@ -0,0 +1,3 @@
>+samba4.drs.getncchanges.python$vampire_dc$.getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_integrity_cross_partition_links$vampire_dc$
>+samba4.drs.getncchanges.python$promoted_dc$.getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_integrity_cross_partition_links$promoted_dc$
>+
>diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
>index 4bcbdc6..783676f 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -845,6 +845,11 @@ for env in ['vampire_dc', 'promoted_dc']:
>                            name="samba4.drs.linked_attributes_drs.python(%s)" % env,
>                            environ={'DC1': "$DC_SERVER", 'DC2': '$%s_SERVER' % env.upper()},
>                            extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
>+    planoldpythontestsuite(env, "getncchanges",
>+                           extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')],
>+                           name="samba4.drs.getncchanges.python(%s)" % env,
>+                           environ={'DC1': "$DC_SERVER", 'DC2': '$%s_SERVER' % env.upper()},
>+                           extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD'])
> 
> for env in ['vampire_dc', 'promoted_dc', 'vampire_2000_dc']:
>     planoldpythontestsuite(env, "repl_schema",
>diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py
>new file mode 100644
>index 0000000..a2be830
>--- /dev/null
>+++ b/source4/torture/drs/python/getncchanges.py
>@@ -0,0 +1,149 @@
>+#!/usr/bin/env python
>+# -*- coding: utf-8 -*-
>+#
>+# Tests various schema replication scenarios
>+#
>+# Copyright (C) Catalyst.Net Ltd. 2017
>+#
>+# This program is free software; you can redistribute it and/or modify
>+# it under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# This program is distributed in the hope that it will be useful,
>+# but WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>+#
>+
>+#
>+# Usage:
>+#  export DC1=dc1_dns_name
>+#  export DC2=dc2_dns_name
>+#  export SUBUNITRUN=$samba4srcdir/scripting/bin/subunitrun
>+#  PYTHONPATH="$PYTHONPATH:$samba4srcdir/torture/drs/python" $SUBUNITRUN getncchanges -U"$DOMAIN/$DC_USERNAME"%"$DC_PASSWORD"
>+#
>+
>+import drs_base
>+import samba.tests
>+import ldb
>+from ldb import SCOPE_BASE
>+import random
>+
>+from samba.dcerpc import drsuapi
>+
>+class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase):
>+    def setUp(self):
>+        super(DrsReplicaSyncIntegrityTestCase, self).setUp()
>+
>+        # for this test we modify DC2 and replicate to DC1
>+        self.test_ldb_dc = self.ldb_dc2
>+
>+        # add some randomness to the test OU. (Deletion of the last test's
>+        # objects can be slow to replicate out. So the OU created by a previous
>+        # testenv may still exist at this point).
>+        rand = random.randint(1, 10000000)
>+        self.base_dn = self.test_ldb_dc.get_default_basedn()
>+        self.ou = "OU=getncchanges%d_test,%s" %(rand, self.base_dn)
>+        self.test_ldb_dc.add({
>+            "dn": self.ou,
>+            "objectclass": "organizationalUnit"})
>+
>+    def tearDown(self):
>+        super(DrsReplicaSyncIntegrityTestCase, self).tearDown()
>+        # tidyup groups and users
>+        try:
>+            self.ldb_dc2.delete(self.ou, ["tree_delete:1"])
>+        except ldb.LdbError as (enum, string):
>+            if enum == ldb.ERR_NO_SUCH_OBJECT:
>+                pass
>+
>+        self._enable_all_repl(self.dnsname_dc2)
>+
>+    def add_object(self, dn, objectclass="organizationalunit"):
>+        """Adds an OU object"""
>+        self.test_ldb_dc.add({"dn": dn, "objectclass": objectclass})
>+        res = self.test_ldb_dc.search(base=dn, scope=SCOPE_BASE, attrs=['objectGUID'])
>+        self.assertEquals(len(res), 1)
>+        return self._GUID_string(res[0]["objectGUID"][0])
>+
>+    def modify_object(self, dn, attr, value):
>+        """Modifies an object's USN by adding an attribute value to it"""
>+        m = ldb.Message()
>+        m.dn = ldb.Dn(self.test_ldb_dc, dn)
>+        m[attr] = ldb.MessageElement(value, ldb.FLAG_MOD_ADD, attr)
>+        self.test_ldb_dc.modify(m)
>+
>+    def delete_attribute(self, dn, attr, value):
>+        """Deletes an attribute from an object"""
>+        m = ldb.Message()
>+        m.dn = ldb.Dn(self.test_ldb_dc, dn)
>+        m[attr] = ldb.MessageElement(value, ldb.FLAG_MOD_DELETE, attr)
>+        self.test_ldb_dc.modify(m)
>+
>+    def sync_DCs(self, nc_dn=None):
>+        # make sure DC1 has all the changes we've made to DC2
>+        self._net_drs_replicate(DC=self.dnsname_dc1, fromDC=self.dnsname_dc2, nc_dn=nc_dn)
>+
>+    def test_repl_integrity_cross_partition_links(self):
>+        """
>+        Checks that a cross-partition link to an unknown target object does
>+        not result in missing links.
>+        """
>+
>+        # check the peer DC is up-to-date, then connect (storing its HWM)
>+        self.sync_DCs()
>+
>+        # stop replication so the peer gets the following objects in one go
>+        self._disable_all_repl(self.dnsname_dc2)
>+
>+        # create a link source object in the main NC
>+        la_source = "OU=cross_nc_src,%s" % self.ou
>+        src_guid = self.add_object(la_source)
>+
>+        # create the link target (a server object) in the config NC
>+        rand = random.randint(1, 10000000)
>+        la_target = "CN=getncchanges-%d,CN=Servers,CN=Default-First-Site-Name," \
>+                    "CN=Sites,%s" %(rand, self.config_dn)
>+        self.add_object(la_target, objectclass="server")
>+
>+        # add a cross-partition link between the two
>+        self.modify_object(la_source, "managedBy", la_target)
>+
>+        # sync the NC containing the link source first, then the target/Config NC
>+        self.sync_DCs()
>+        self.sync_DCs(nc_dn=self.config_dn)
>+
>+        res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
>+                                   scope=SCOPE_BASE, attrs=["managedBy"])
>+        res2 = self.ldb_dc2.search(base="<GUID=%s>" % src_guid,
>+                                   scope=SCOPE_BASE, attrs=["managedBy"])
>+
>+        self.assertTrue("managedBy" in res1[0], "%s in DB is missing managedBy attribute"
>+                         % la_source)
>+        self.assertTrue("managedBy" in res2[0], "%s in DB is missing managedBy attribute"
>+                         % la_source)
>+
>+        # the cross-partition linked attribute has a missing backlink. Check
>+        # that we can still delete it successfully
>+        self.delete_attribute(la_source, "managedBy", la_target)
>+        self.sync_DCs()
>+
>+        res1 = self.ldb_dc1.search(base="<GUID=%s>" % src_guid,
>+                                   scope=SCOPE_BASE, attrs=["managedBy"])
>+        res2 = self.ldb_dc2.search(base="<GUID=%s>" % src_guid,
>+                                   scope=SCOPE_BASE, attrs=["managedBy"])
>+
>+        self.assertFalse("managedBy" in res1[0], "%s in DB still has managedBy attribute"
>+                         % la_source)
>+        self.assertFalse("managedBy" in res2[0], "%s in DB still has managedBy attribute"
>+                         % la_source)
>+
>+        # cleanup the server object we created in the Configuration partition
>+        self.test_ldb_dc.delete(la_source)
>+
>+
>+
>-- 
>2.7.4
>
>
>From ad39a33d2574413044325b2369ffb5e6a1ee7e7f Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Mon, 19 Jun 2017 10:26:48 +1200
>Subject: [PATCH 2/5] libnet: Initialize req_level in become_dc tests
>
>The net.api.become.dc tests would always pass the request into
>libnet_vampire_cb_store_chunk() with req_level=0, which meant that
>storing the chunk didn't use the correct replica_flags/exop.
>
>I noticed this problem when working on client-side support for GET_TGT.
>My changes relied on the critical-only request flag being passed down
>into replmd, but because the request flags weren't passed correctly, my
>changes caused the become_dc tests to fail.
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit 475a3206461f5458059f8f530b45f0b1ae636739)
>---
> source4/libnet/libnet_become_dc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source4/libnet/libnet_become_dc.c b/source4/libnet/libnet_become_dc.c
>index 43a3209..e9153a0 100644
>--- a/source4/libnet/libnet_become_dc.c
>+++ b/source4/libnet/libnet_become_dc.c
>@@ -2673,7 +2673,7 @@ static WERROR becomeDC_drsuapi_pull_partition_recv(struct libnet_BecomeDC_state
> 						   struct libnet_BecomeDC_Partition *partition,
> 						   struct drsuapi_DsGetNCChanges *r)
> {
>-	uint32_t req_level = 0;
>+	uint32_t req_level = r->in.level;
> 	struct drsuapi_DsGetNCChangesRequest5 *req5 = NULL;
> 	struct drsuapi_DsGetNCChangesRequest8 *req8 = NULL;
> 	struct drsuapi_DsGetNCChangesRequest10 *req10 = NULL;
>-- 
>2.7.4
>
>
>From ec4c97c9c600e369c294361784a05bd786b54ea0 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Wed, 13 Sep 2017 15:35:20 +1200
>Subject: [PATCH 3/5] drs: Refactor to distinguish missing link target cases
>
>This is mostly a cherry-pick of f69596cd21d8106afdf494f39d from master,
>except I've omitted the actual change in functionality (i.e. dropping
>the link).
>
>It adds the code that allows us to distinguish between the various cases
>where the link target can be missing:
>- We're replicating a CRITICAL_ONLY object and don't know about the
>  target yet.
>- We've got a cross-partition link and we haven't replicated the
>  partition containing the target yet.
>- Some other corner-case (in master, we'd retry with GET_TGT).
>
>There should be no change in functionality from this patch (apart from
>tweaking the debug message).
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> source4/dsdb/repl/drepl_out_helpers.c           |  4 ++
> source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 83 ++++++++++++++++++++++++-
> source4/dsdb/samdb/samdb.h                      |  2 +-
> source4/libnet/libnet_vampire.c                 |  1 +
> 4 files changed, 86 insertions(+), 4 deletions(-)
>
>diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c
>index 079edc8..ad4801a 100644
>--- a/source4/dsdb/repl/drepl_out_helpers.c
>+++ b/source4/dsdb/repl/drepl_out_helpers.c
>@@ -795,6 +795,10 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req
> 	if (state->op->options & DRSUAPI_DRS_SPECIAL_SECRET_PROCESSING) {
> 		dsdb_repl_flags |= DSDB_REPL_FLAG_EXPECT_NO_SECRETS;
> 	}
>+	if (state->op->options & DRSUAPI_DRS_CRITICAL_ONLY ||
>+	    state->op->extended_op != DRSUAPI_EXOP_NONE) {
>+		dsdb_repl_flags |= DSDB_REPL_FLAG_OBJECT_SUBSET;
>+	}
> 
> 	if (state->op->extended_op != DRSUAPI_EXOP_NONE) {
> 		ret = dsdb_find_nc_root(service->samdb, partition,
>diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>index 0ed24cb..37f5b8d 100644
>--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>@@ -74,6 +74,7 @@ struct replmd_private {
> struct la_entry {
> 	struct la_entry *next, *prev;
> 	struct drsuapi_DsReplicaLinkedAttribute *la;
>+	bool incomplete_replica;
> };
> 
> struct replmd_replicated_request {
>@@ -6535,6 +6536,7 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct
> 	struct ldb_control **ctrls;
> 	int ret;
> 	uint32_t i;
>+	bool incomplete_subset;
> 	struct replmd_private *replmd_private =
> 		talloc_get_type(ldb_module_get_private(module), struct replmd_private);
> 
>@@ -6592,6 +6594,7 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct
> 
> 	ar->controls = req->controls;
> 	req->controls = ctrls;
>+	incomplete_subset = (ar->objs->dsdb_repl_flags & DSDB_REPL_FLAG_OBJECT_SUBSET);
> 
> 	DEBUG(4,("linked_attributes_count=%u\n", objs->linked_attributes_count));
> 
>@@ -6616,6 +6619,13 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct
> 		}
> 		*la_entry->la = ar->objs->linked_attributes[i];
> 
>+		/*
>+		 * we may not be able to resolve link targets properly when
>+		 * dealing with subsets of objects, e.g. the source is a
>+		 * critical object and the target isn't
>+		 */
>+		la_entry->incomplete_replica = incomplete_subset;
>+
> 		/* we need to steal the non-scalars so they stay
> 		   around until the end of the transaction */
> 		talloc_steal(la_entry->la, la_entry->la->identifier);
>@@ -6627,6 +6637,46 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct
> 	return replmd_replicated_apply_next(ar);
> }
> 
>+/**
>+ * Returns True if the source and target DNs both have the same naming context,
>+ * i.e. they're both in the same partition.
>+ */
>+static bool replmd_objects_have_same_nc(struct ldb_context *ldb,
>+					TALLOC_CTX *mem_ctx,
>+					struct ldb_dn *source_dn,
>+					struct ldb_dn *target_dn)
>+{
>+	TALLOC_CTX *tmp_ctx;
>+	struct ldb_dn *source_nc;
>+	struct ldb_dn *target_nc;
>+	int ret;
>+	bool same_nc = true;
>+
>+	tmp_ctx = talloc_new(mem_ctx);
>+
>+	ret = dsdb_find_nc_root(ldb, tmp_ctx, source_dn, &source_nc);
>+	if (ret != LDB_SUCCESS) {
>+		DBG_ERR("Failed to find base DN for source %s\n",
>+			ldb_dn_get_linearized(source_dn));
>+		talloc_free(tmp_ctx);
>+		return true;
>+	}
>+
>+	ret = dsdb_find_nc_root(ldb, tmp_ctx, target_dn, &target_nc);
>+	if (ret != LDB_SUCCESS) {
>+		DBG_ERR("Failed to find base DN for target %s\n",
>+			ldb_dn_get_linearized(target_dn));
>+		talloc_free(tmp_ctx);
>+		return true;
>+	}
>+
>+	same_nc = (ldb_dn_compare(source_nc, target_nc) == 0);
>+
>+	talloc_free(tmp_ctx);
>+
>+	return same_nc;
>+}
>+
> /*
>   process one linked attribute structure
>  */
>@@ -6834,9 +6884,36 @@ linked_attributes[0]:
> 	}
> 
> 	if (target_res->count == 0) {
>-		DEBUG(2,(__location__ ": WARNING: Failed to re-resolve GUID %s - using %s\n",
>-			 GUID_string(tmp_ctx, &guid),
>-			 ldb_dn_get_linearized(dsdb_dn->dn)));
>+		bool same_partition;
>+
>+		/*
>+		 * TODO:
>+		 * When we implement Trusted Domains we need to consider
>+		 * whether they get treated as an incomplete replica here or not
>+		 */
>+		if (la_entry->incomplete_replica) {
>+
>+			/*
>+			 * If we're only replicating a subset of objects (e.g.
>+			 * critical-only, single-object), then an unknown target
>+			 * is probably not a critical problem. We don't increase
>+			 * the highwater-mark so subsequent replications should
>+			 * resolve any missing links
>+			 */
>+			DEBUG(2,(__location__
>+				 ": Failed to find target %s linked from %s\n",
>+				 ldb_dn_get_linearized(dsdb_dn->dn),
>+				 ldb_dn_get_linearized(msg->dn)));
>+			return LDB_SUCCESS;
>+		}
>+
>+		same_partition = replmd_objects_have_same_nc(ldb, tmp_ctx, msg->dn, dsdb_dn->dn);
>+
>+   		DEBUG(2,(__location__ ": WARNING: Failed to re-resolve %sGUID %s - using %s\n",
>+				 same_partition ? "" : "cross-partition ",
>+    			 GUID_string(tmp_ctx, &guid),
>+    			 ldb_dn_get_linearized(dsdb_dn->dn)));
>+
> 	} else if (target_res->count != 1) {
> 		ldb_asprintf_errstring(ldb_module_get_ctx(module), "More than one object found matching objectGUID %s\n",
> 				       GUID_string(tmp_ctx, &guid));
>diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h
>index c8658dc..2abaf4a 100644
>--- a/source4/dsdb/samdb/samdb.h
>+++ b/source4/dsdb/samdb/samdb.h
>@@ -64,7 +64,7 @@ struct dsdb_control_current_partition {
> #define DSDB_REPL_FLAG_PARTIAL_REPLICA     2
> #define DSDB_REPL_FLAG_ADD_NCNAME	   4
> #define DSDB_REPL_FLAG_EXPECT_NO_SECRETS   8
>-
>+#define DSDB_REPL_FLAG_OBJECT_SUBSET       16
> 
> #define DSDB_CONTROL_REPLICATED_UPDATE_OID "1.3.6.1.4.1.7165.4.3.3"
> 
>diff --git a/source4/libnet/libnet_vampire.c b/source4/libnet/libnet_vampire.c
>index 7f25a3a..d89256b 100644
>--- a/source4/libnet/libnet_vampire.c
>+++ b/source4/libnet/libnet_vampire.c
>@@ -660,6 +660,7 @@ WERROR libnet_vampire_cb_store_chunk(void *private_data,
> 		 */
> 		ZERO_STRUCT(s_dsa->highwatermark);
> 		uptodateness_vector = NULL;
>+		dsdb_repl_flags |= DSDB_REPL_FLAG_OBJECT_SUBSET;
> 	}
> 
> 	/* TODO: avoid hardcoded flags */
>-- 
>2.7.4
>
>
>From d991ee84785d19f918e374fef74981528abc55ad Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Wed, 13 Sep 2017 16:02:00 +1200
>Subject: [PATCH 4/5] replmd: Small refactor to make code flow more obvious
>
>When the 'Failed to re-resolve GUID' case was hit, target_msg would
>still be assigned to NULL. Execution would fall through, bail out
>early in replmd_deletion_state(), then return because the
>target_deletion_state >= OBJECT_RECYCLED.
>
>This code path wasn't very clear. Change it so the 're-resolve GUID'
>case just returns early. We can then move replmd_deletion_state() into
>the only code block that cares about it.
>
>This patch should not alter functionality.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 28 +++++++++++++------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>index 37f5b8d..9862601 100644
>--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>@@ -6914,6 +6914,8 @@ linked_attributes[0]:
>     			 GUID_string(tmp_ctx, &guid),
>     			 ldb_dn_get_linearized(dsdb_dn->dn)));
> 
>+		return LDB_SUCCESS;
>+
> 	} else if (target_res->count != 1) {
> 		ldb_asprintf_errstring(ldb_module_get_ctx(module), "More than one object found matching objectGUID %s\n",
> 				       GUID_string(tmp_ctx, &guid));
>@@ -6922,21 +6924,21 @@ linked_attributes[0]:
> 	} else {
> 		target_msg = target_res->msgs[0];
> 		dsdb_dn->dn = talloc_steal(dsdb_dn, target_msg->dn);
>-	}
> 
>-	/*
>-	 * Check for deleted objects per MS-DRSR 4.1.10.6.13
>-	 * ProcessLinkValue, because link updates are not applied to
>-	 * recycled and tombstone objects.  We don't have to delete
>-	 * any existing link, that should have happened when the
>-	 * object deletion was replicated or initiated.
>-	 */
>-	replmd_deletion_state(module, target_msg,
>-			      &target_deletion_state, NULL);
>+		/*
>+		 * Check for deleted objects per MS-DRSR 4.1.10.6.13
>+		 * ProcessLinkValue, because link updates are not applied to
>+		 * recycled and tombstone objects.  We don't have to delete
>+		 * any existing link, that should have happened when the
>+		 * object deletion was replicated or initiated.
>+		 */
>+		replmd_deletion_state(module, target_msg,
>+				      &target_deletion_state, NULL);
> 
>-	if (target_deletion_state >= OBJECT_RECYCLED) {
>-		talloc_free(tmp_ctx);
>-		return LDB_SUCCESS;
>+		if (target_deletion_state >= OBJECT_RECYCLED) {
>+			talloc_free(tmp_ctx);
>+			return LDB_SUCCESS;
>+		}
> 	}
> 
> 	/* see if this link already exists */
>-- 
>2.7.4
>
>
>From 31c5a190df5af0570f7b1e7cb2364570842c8d01 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Wed, 13 Sep 2017 16:24:09 +1200
>Subject: [PATCH 5/5] replmd: Try to add forward-link for unknown
> cross-partition links
>
>Previously Samba would just drop cross-partition links where the link
>target object is unknown. Instead, what we want to do is try to add the
>forward link for the GUID specified. We can't add the backlink because
>we don't know the target, however, dbcheck should be able to fix any
>missing backlinks.
>
>The new behaviour should now mean dbcheck will detect the problem and be
>able to fix it. It's still not ideal, but it's better than dropping the
>link completely.
>
>I've updated the log so that it has higher severity and tells the user
>what they need to do to fix it.
>
>These changes now mean that the selftests now detect an error - instead
>of completely dropping the serverReference, we now have a missing
>backlink. I've updated the selftests to fix up any missing
>serverReference backlinks before running dbcheck.
>
>(This is master commit fae5df891c11f642cbede9e backported to 4.7)
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> selftest/knownfail.d/getncchanges               |  3 ---
> source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 26 ++++++++++++++++++-------
> testprogs/blackbox/dbcheck.sh                   |  8 ++++++++
> 3 files changed, 27 insertions(+), 10 deletions(-)
> delete mode 100644 selftest/knownfail.d/getncchanges
>
>diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges
>deleted file mode 100644
>index 303400f..0000000
>--- a/selftest/knownfail.d/getncchanges
>+++ /dev/null
>@@ -1,3 +0,0 @@
>-samba4.drs.getncchanges.python$vampire_dc$.getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_integrity_cross_partition_links$vampire_dc$
>-samba4.drs.getncchanges.python$promoted_dc$.getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_integrity_cross_partition_links$promoted_dc$
>-
>diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>index 9862601..2c9e7b9 100644
>--- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>+++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
>@@ -372,8 +372,9 @@ static int replmd_process_backlink(struct ldb_module *module, struct la_backlink
> 	ret = dsdb_module_dn_by_guid(module, frame, &bl->target_guid, &target_dn, parent);
> 	if (ret != LDB_SUCCESS) {
> 		struct GUID_txt_buf guid_str;
>-		DEBUG(2,(__location__ ": WARNING: Failed to find target DN for linked attribute with GUID %s\n",
>-			 GUID_buf_string(&bl->target_guid, &guid_str)));
>+		DBG_WARNING("Failed to find target DN for linked attribute with GUID %s\n",
>+			    GUID_buf_string(&bl->target_guid, &guid_str));
>+		DBG_WARNING("Please run 'samba-tool dbcheck' to resolve any missing backlinks.\n");
> 		talloc_free(frame);
> 		return LDB_SUCCESS;
> 	}
>@@ -6909,12 +6910,23 @@ linked_attributes[0]:
> 
> 		same_partition = replmd_objects_have_same_nc(ldb, tmp_ctx, msg->dn, dsdb_dn->dn);
> 
>-   		DEBUG(2,(__location__ ": WARNING: Failed to re-resolve %sGUID %s - using %s\n",
>-				 same_partition ? "" : "cross-partition ",
>-    			 GUID_string(tmp_ctx, &guid),
>-    			 ldb_dn_get_linearized(dsdb_dn->dn)));
>+		if (same_partition) {
>+			DEBUG(2,(__location__ ": WARNING: Failed to re-resolve GUID %s - using %s\n",
>+					 GUID_string(tmp_ctx, &guid),
>+					 ldb_dn_get_linearized(dsdb_dn->dn)));
>+			return LDB_SUCCESS;
>+		} else {
> 
>-		return LDB_SUCCESS;
>+			/*
>+			 * The target of the cross-partition link is missing.
>+			 * Continue and try to at least add the forward-link.
>+			 * This isn't great, but if we can add a partial link
>+			 * then it's better than nothing.
>+			 */
>+			DBG_WARNING("Missing cross-partition target %s linked from %s\n",
>+				    ldb_dn_get_linearized(dsdb_dn->dn),
>+				    ldb_dn_get_linearized(msg->dn));
>+		}
> 
> 	} else if (target_res->count != 1) {
> 		ldb_asprintf_errstring(ldb_module_get_ctx(module), "More than one object found matching objectGUID %s\n",
>diff --git a/testprogs/blackbox/dbcheck.sh b/testprogs/blackbox/dbcheck.sh
>index 0f979ab..387ce70 100755
>--- a/testprogs/blackbox/dbcheck.sh
>+++ b/testprogs/blackbox/dbcheck.sh
>@@ -27,6 +27,13 @@ dbcheck_fix_stale_links() {
> 	$BINDIR/samba-tool dbcheck --quiet --fix --yes remove_plausible_deleted_DN_links --attrs="member msDS-NC-Replica-Locations msDS-NC-RO-Replica-Locations" --cross-ncs $ARGS
> }
> 
>+# This list of attributes can be freely extended
>+dbcheck_fix_crosspartition_backlinks() {
>+	# we may not know the target yet when we receive a cross-partition link,
>+	# which can result in a missing backlink
>+	$BINDIR/samba-tool dbcheck --quiet --fix --yes fix_all_missing_backlinks --attrs="serverReference" --cross-ncs $ARGS
>+}
>+
> # This test shows that this does not do anything to a current
> # provision (that would be a bug)
> dbcheck_reset_well_known_acls() {
>@@ -47,6 +54,7 @@ force_modules() {
> 
> dbcheck_fix_one_way_links
> dbcheck_fix_stale_links
>+dbcheck_fix_crosspartition_backlinks
> testit "dbcheck" dbcheck
> testit "reindex" reindex
> testit "fixed_attrs" fixed_attrs
>-- 
>2.7.4
>

Actions: View

Attachments on bug 12972: 13480 | 13581 | 13595 | 13605