From 4e16281c9fc431ee8c473ab413646be08da3e643 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Mon, 1 Jul 2019 14:16:13 +1200 Subject: [PATCH 1/2] tests: Add getncchanges test for cross-partition links + TGT This adds a test-case to highlight a bug in the client side GetNCChanges handling. These tests mostly exercise the server-side behaviour of sending the GetNCChanges, however, there's a bug in the client-side code when we try to handle a missing cross-partition link target *in combination* with the GET_TGT flag already having been set. The test is exercising the client-side code by using the 'samba-tool drs replicate' command. By adding a one-way link to a deleted target object, we force the client code to retry with the GET_TGT flag set. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14022 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett (cherry picked from commit dba9987bf500f82fbbcda1cd78c543a87f90cec5) [conflict in knownfail.d/getncchanges file, merged manually] --- selftest/knownfail.d/getncchanges | 2 ++ source4/torture/drs/python/getncchanges.py | 28 +++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges index 5ef1bc9..525b6df 100644 --- a/selftest/knownfail.d/getncchanges +++ b/selftest/knownfail.d/getncchanges @@ -4,3 +4,5 @@ samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegri samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_chain\(promoted_dc\) samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_and_anc\(promoted_dc\) samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_multivalued_links\(promoted_dc\) +# this fails due to a bug +samba4.drs.getncchanges.python.*.getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_integrity_cross_partition_links_with_tgt diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py index 9136d07..614f650 100644 --- a/source4/torture/drs/python/getncchanges.py +++ b/source4/torture/drs/python/getncchanges.py @@ -966,7 +966,7 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): self.assert_DCs_replication_is_consistent(peer_conn, all_objects, expected_links) - def test_repl_integrity_cross_partition_links(self): + def _test_repl_integrity_cross_partition_links(self, get_tgt=False): """ Checks that a cross-partition link to an unknown target object does not result in missing links. @@ -979,6 +979,18 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # stop replication so the peer gets the following objects in one go self._disable_all_repl(self.dnsname_dc2) + # optionally force the client-side to use GET_TGT locally, by adding a + # one-way link to a missing/deleted target object + if get_tgt: + missing_target = "OU=missing_tgt,%s" % self.ou + self.add_object(missing_target) + get_tgt_source = "CN=get_tgt_src,%s" % self.ou + self.add_object(get_tgt_source, + objectclass="msExchConfigurationContainer") + self.modify_object(get_tgt_source, "addressBookRoots2", + missing_target) + self.test_ldb_dc.delete(missing_target) + # create a link source object in the main NC la_source = "OU=cross_nc_src,%s" % self.ou self.add_object(la_source) @@ -1007,6 +1019,14 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): self.repl_get_next(get_tgt=True) self.set_dc_connection(self.default_conn) + + # delete the GET_TGT test object. We're not interested in asserting its + # links - it was just there to make the client use GET_TGT (and it + # creates an inconsistency because one DC correctly ignores the link, + # because it points to a deleted object) + if get_tgt: + self.test_ldb_dc.delete(get_tgt_source) + self.init_test_state() # Now sync across the partition containing the link target object @@ -1063,6 +1083,12 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): self.test_ldb_dc.delete(la_target) self._enable_all_repl(self.dnsname_dc2) + def test_repl_integrity_cross_partition_links(self): + self._test_repl_integrity_cross_partition_links(get_tgt=False) + + def test_repl_integrity_cross_partition_links_with_tgt(self): + self._test_repl_integrity_cross_partition_links(get_tgt=True) + def test_repl_get_tgt_multivalued_links(self): """Tests replication with multi-valued link attributes.""" -- 2.7.4 From f84f7d1b1e583765ef63617c7920d0d1aefe219e Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Mon, 1 Jul 2019 13:41:14 +1200 Subject: [PATCH 2/2] repl_md: Avoid dropping cross-partition links Cross-partition links could still be dropped if GET_TGT was already previously set for the replication. This was due to a slight error in the order of logic. We never want to ignore cross-partition links (regardless of whether the TARGETS_UPTODATE /GET_TGT flag is set). We should only be returning early in the GET_TGT case if the objects are both in the same partition. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14022 RN: When the AD domain contained a linked attribute that spanned partitions, DRS replication could drop the link. This dropped link could then result in subtle differences in behaviour between DCs, as some DCs would have the link and others wouldn't. When this issue occurred, the dropped link would be logged in a warning message: " is Unknown but up to date. Ignoring link from " This issue would not always occur - it depended a lot on the database contents. Typically, it would only potentially occur when joining a new DC to the domain (doing an ldapcmp after the join would also highlight the problem, if it occurred). This issue has now been resolved. Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett (cherry picked from commit 98848142cde51d4b280a6fb5cd95dc4bd2471e17) [conflict in knownfail.d/getncchanges file, merged manually] --- selftest/knownfail.d/getncchanges | 3 +-- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 31 +++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges index 525b6df..39452cd 100644 --- a/selftest/knownfail.d/getncchanges +++ b/selftest/knownfail.d/getncchanges @@ -4,5 +4,4 @@ samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegri samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_chain\(promoted_dc\) samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_and_anc\(promoted_dc\) samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_multivalued_links\(promoted_dc\) -# this fails due to a bug -samba4.drs.getncchanges.python.*.getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_integrity_cross_partition_links_with_tgt + diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 04a51ec..648b7f0 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -7230,27 +7230,28 @@ static int replmd_allow_missing_target(struct ldb_module *module, return LDB_SUCCESS; } - if (dsdb_repl_flags & DSDB_REPL_FLAG_TARGETS_UPTODATE) { + is_in_same_nc = dsdb_objects_have_same_nc(ldb, + mem_ctx, + source_dn, + target_dn); + if (is_in_same_nc) { /* - * target should already be up-to-date so there's no point in + * if the target is already be up-to-date there's no point in * retrying. This could be due to bad timing, or if a target * on a one-way link was deleted. We ignore the link rather * than failing the replication cycle completely */ - *ignore_link = true; - DBG_WARNING("%s is %s but up to date. Ignoring link from %s\n", - ldb_dn_get_linearized(target_dn), missing_str, - ldb_dn_get_linearized(source_dn)); - return LDB_SUCCESS; - } - - is_in_same_nc = dsdb_objects_have_same_nc(ldb, - mem_ctx, - source_dn, - target_dn); - if (is_in_same_nc) { - /* fail the replication and retry with GET_TGT */ + if (dsdb_repl_flags & DSDB_REPL_FLAG_TARGETS_UPTODATE) { + *ignore_link = true; + DBG_WARNING("%s is %s " + "but up to date. Ignoring link from %s\n", + ldb_dn_get_linearized(target_dn), missing_str, + ldb_dn_get_linearized(source_dn)); + return LDB_SUCCESS; + } + + /* otherwise fail the replication and retry with GET_TGT */ ldb_asprintf_errstring(ldb, "%s target %s GUID %s linked from %s\n", missing_str, ldb_dn_get_linearized(target_dn), -- 2.7.4