The Samba-Bugzilla – Attachment 18063 Details for
Bug 15401
Azure AD Sync infinite loop in initial user sync phase
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch in master backported to Samba 4.17, 4.18, 4.19 (v2)
drs-spin-v2.patch (text/plain), 59.12 KB, created by
Andrew Bartlett
on 2023-08-17 20:33:53 UTC
(
hide
)
Description:
Patch in master backported to Samba 4.17, 4.18, 4.19 (v2)
Filename:
MIME Type:
Creator:
Andrew Bartlett
Created:
2023-08-17 20:33:53 UTC
Size:
59.12 KB
patch
obsolete
>From 1c1fe6af5db9f156eb3f9abb5441cde86f5195b0 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 24 Jul 2023 11:35:45 +1200 >Subject: [PATCH 01/18] s4-rpc_server/drsuapi: Add tmp_highest_usn tracking to > replication log > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 0d9ea6c559317e19642662220c089e2d59ef3ecd) >--- > source4/rpc_server/drsuapi/getncchanges.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 74b173c3965..9eb75c8502a 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -3507,9 +3507,10 @@ allowed: > getnc_state->last_dn = talloc_move(getnc_state, &msg->dn); > } > >- DEBUG(8,(__location__ ": %s object %s\n", >+ DEBUG(8,(__location__ ": %s object %s new tmp_highest_usn=%" PRIu64 "\n", > new_objs ? "replicating" : "skipping send of", >- ldb_dn_get_linearized(msg->dn))); >+ ldb_dn_get_linearized(msg->dn), >+ r->out.ctr->ctr6.new_highwatermark.tmp_highest_usn)); > > getnc_state->total_links += (getnc_state->la_count - old_la_index); > >-- >2.25.1 > > >From 6fe70c105a8050daa521a80e8a333d856eb7422a Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 12:18:24 +1200 >Subject: [PATCH 02/18] s4-rpc_server/drsuapi: Improve debugging of invalid DNs > >This is still unreachable, so but improve the logging >to give more detail in this area anyway. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit d0c1ce53add2fd3b3a4186581f4e214029cbcf1a) >--- > source4/rpc_server/drsuapi/getncchanges.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 9eb75c8502a..d730a1d18f5 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -2977,8 +2977,9 @@ allowed: > * This can't fail as we have done this above > * implicitly but not got the DN out > */ >- DBG_ERR("Bad DN '%s'\n", >- drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot)); >+ DBG_ERR("Bad DN '%s' as Naming Context for GetNCChanges: %s\n", >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >+ ldb_strerror(ret)); > return WERR_DS_DRA_INVALID_PARAMETER; > } > if (ldb_dn_compare(new_dn, getnc_state->ncRoot_dn) != 0) { >-- >2.25.1 > > >From 229d55897ba6ec1eb15bf02fe73fc1dc378ff427 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 17:18:39 +1200 >Subject: [PATCH 03/18] s4-rpc_server/drsuapi: Improve debug message for > drs_ObjectIdentifier_to_dn_and_nc_root() failure > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit a12bcce89d26ae05bbaeed560cf8fcc7b5bcfdab) >--- > source4/rpc_server/drsuapi/getncchanges.c | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index d730a1d18f5..977a0b75069 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -3022,6 +3022,9 @@ allowed: > &ncRoot_dn, > NULL); > if (ret != LDB_SUCCESS) { >+ DBG_ERR("Bad DN '%s' as Naming Context or EXOP DN for GetNCChanges: %s\n", >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >+ ldb_strerror(ret)); > return WERR_DS_DRA_BAD_DN; > } > >-- >2.25.1 > > >From 57b6f6ac2eaa0655dd618c044f4d156d5fa8543e Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 14:59:49 +1200 >Subject: [PATCH 04/18] s4-dsdb: Improve logging for > drs_ObjectIdentifier_to_dn_and_nc_root() > >At this layer we can make a reasonable assumption about being able >to read ldb_errstring() to print that for extra useful debugging. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 63843a22c8db73d459bee61e73bb1f0d31e3d427) >--- > source4/dsdb/common/dsdb_dn.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > >diff --git a/source4/dsdb/common/dsdb_dn.c b/source4/dsdb/common/dsdb_dn.c >index c86848fd697..b34d1e6052a 100644 >--- a/source4/dsdb/common/dsdb_dn.c >+++ b/source4/dsdb/common/dsdb_dn.c >@@ -554,6 +554,18 @@ int drs_ObjectIdentifier_to_dn_and_nc_root(TALLOC_CTX *mem_ctx, > new_dn, > normalised_dn, > nc_root); >+ if (ret != LDB_SUCCESS) { >+ /* >+ * dsdb_normalise_dn_and_find_nc_root() sets LDB error >+ * strings, and the functions it calls do also >+ */ >+ DBG_NOTICE("Failed to find DN \"%s\" -> \"%s\" for normalisation: %s (%s)\n", >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, nc), >+ ldb_dn_get_extended_linearized(mem_ctx, new_dn, 1), >+ ldb_errstring(ldb), >+ ldb_strerror(ret)); >+ } >+ > TALLOC_FREE(new_dn); > return ret; > } >-- >2.25.1 > > >From 3e098c149784541cb8e31d29e0738d6d78354762 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 14:22:52 +1200 >Subject: [PATCH 05/18] s4-rpc_server/drsuapi: Remove rudundant check for valid > and non-NULL ncRoot_dn > >This check was valuable before aee2039e63ceeb5e69a0461fb77e0f18278e4dc4 >but now only checks things we know to be true, as the value has come >from Samba via drs_ObjectIdentifier_to_dn_and_nc_root() either on this >or a previous call. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 0550e469eda4022659718ae9a56f5deaa9f9a307) >--- > source4/rpc_server/drsuapi/getncchanges.c | 7 ------- > 1 file changed, 7 deletions(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 977a0b75069..aaf8d8519ab 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -3136,13 +3136,6 @@ allowed: > TALLOC_FREE(res); > } > >- if (!ldb_dn_validate(getnc_state->ncRoot_dn) || >- ldb_dn_is_null(getnc_state->ncRoot_dn)) { >- DEBUG(0,(__location__ ": Bad DN '%s'\n", >- drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot))); >- return WERR_DS_DRA_INVALID_PARAMETER; >- } >- > ncRoot->guid = getnc_state->ncRoot_guid; > > /* we need the session key for encrypting password attributes */ >-- >2.25.1 > > >From 8d93be91b0308babee8877452fed5c75faa3228e Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 24 Jul 2023 11:40:46 +1200 >Subject: [PATCH 06/18] s4-torture/drs: Save the server dnsname on the > DcConnection object > >This object is used to hold one of many possible connections and >it is helpful for debugging and uniqueness to know which DC is being >connected to. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit c30bb8769ff2c4eba2d8f8a2bd3a56946b7d9d5e) >--- > source4/torture/drs/python/getncchanges.py | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py >index 1d25c58c51d..b101e9e2a08 100644 >--- a/source4/torture/drs/python/getncchanges.py >+++ b/source4/torture/drs/python/getncchanges.py >@@ -1229,3 +1229,4 @@ class DcConnection: > (self.drs, self.drs_handle) = drs_base._ds_bind(dnsname_dc) > (self.default_hwm, utdv) = drs_base._get_highest_hwm_utdv(ldb_dc) > self.default_utdv = utdv >+ self.dnsname_dc = dnsname_dc >-- >2.25.1 > > >From beef540b06e0a08e51f1722021270478bda995a2 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 24 Jul 2023 11:36:36 +1200 >Subject: [PATCH 07/18] s4-torture/drs: Create temp OU with a unique name per > test > >It is always better to keep the testing OUs unique if possible. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 628eab11b3c2e82875bf602e363b781d3e5eb96d) >--- > source4/torture/drs/python/getncchanges.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py >index b101e9e2a08..2095bc61a77 100644 >--- a/source4/torture/drs/python/getncchanges.py >+++ b/source4/torture/drs/python/getncchanges.py >@@ -49,7 +49,8 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): > self.set_test_ldb_dc(self.ldb_dc2) > > self.ou = str(samba.tests.create_test_ou(self.test_ldb_dc, >- "getncchanges")) >+ "getncchanges." + self.id().rsplit(".", 1)[1])) >+ > self.base_dn = self.test_ldb_dc.get_default_basedn() > > self.default_conn = DcConnection(self, self.ldb_dc2, self.dnsname_dc2) >-- >2.25.1 > > >From dc928a3f48da7d7fdae04dd35578766f6439f6f8 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 24 Jul 2023 11:37:19 +1200 >Subject: [PATCH 08/18] s4-torture/drs: Use addCleanup() in getchanges.py for > OU handling > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 40f831e67e1f312b1db52c74c119899245d03e32) >--- > source4/torture/drs/python/getncchanges.py | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > >diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py >index 2095bc61a77..46598311f79 100644 >--- a/source4/torture/drs/python/getncchanges.py >+++ b/source4/torture/drs/python/getncchanges.py >@@ -51,21 +51,13 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): > self.ou = str(samba.tests.create_test_ou(self.test_ldb_dc, > "getncchanges." + self.id().rsplit(".", 1)[1])) > >+ self.addCleanup(self.ldb_dc2.delete, self.ou, ["tree_delete:1"]) >+ > self.base_dn = self.test_ldb_dc.get_default_basedn() > > self.default_conn = DcConnection(self, self.ldb_dc2, self.dnsname_dc2) > self.set_dc_connection(self.default_conn) > >- 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 e: >- (enum, string) = e.args >- if enum == ldb.ERR_NO_SUCH_OBJECT: >- pass >- > def init_test_state(self): > self.rxd_dn_list = [] > self.rxd_links = [] >-- >2.25.1 > > >From db1fb48b261fffcab9d2e6cc5ab0b01dbe223083 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 12:20:32 +1200 >Subject: [PATCH 09/18] s4-torture/drs: Add a test matching Azure AD Connect > REPL_OBJ behaviour > >Azure AD Connect will send a GUID but no DummyDN. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit db16366b0bbefcdb91a0b36c903ed63456a081b8) >--- > source4/torture/drs/python/getnc_exop.py | 25 +++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > >diff --git a/source4/torture/drs/python/getnc_exop.py b/source4/torture/drs/python/getnc_exop.py >index 7121b1ba35f..0f12d9b27d7 100644 >--- a/source4/torture/drs/python/getnc_exop.py >+++ b/source4/torture/drs/python/getnc_exop.py >@@ -295,6 +295,29 @@ class DrsReplicaSyncTestCase(drs_base.DrsBaseTestCase): > (enum, estr) = e1.args > self.assertEqual(enum, werror.WERR_DS_DRA_BAD_NC) > >+ def test_valid_GUID_only_REPL_OBJ(self): >+ dc_guid_1 = self.ldb_dc1.get_invocation_id() >+ drs, drs_handle = self._ds_bind(self.dnsname_dc1, ip=self.url_dc1) >+ >+ res = self.ldb_dc1.search(base=self.ou, scope=SCOPE_BASE, >+ attrs=["objectGUID"]) >+ >+ guid = misc.GUID(res[0]["objectGUID"][0]) >+ >+ req8 = self._exop_req8(dest_dsa=None, >+ invocation_id=dc_guid_1, >+ nc_dn_str="", >+ nc_guid=guid, >+ exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ) >+ >+ try: >+ (level, ctr) = drs.DsGetNCChanges(drs_handle, 8, req8) >+ except WERRORError as e1: >+ (enum, estr) = e1.args >+ self.fail(f"Failed to call GetNCChanges with EXOP_REPL_OBJ and a GUID: {estr}") >+ >+ self.assertEqual(ctr.first_object.object.identifier.guid, guid) >+ > def test_DummyDN_valid_GUID_REPL_OBJ(self): > dc_guid_1 = self.ldb_dc1.get_invocation_id() > drs, drs_handle = self._ds_bind(self.dnsname_dc1, ip=self.url_dc1) >@@ -314,7 +337,7 @@ class DrsReplicaSyncTestCase(drs_base.DrsBaseTestCase): > (level, ctr) = drs.DsGetNCChanges(drs_handle, 8, req8) > except WERRORError as e1: > (enum, estr) = e1.args >- self.fail(f"Failed to call GetNCChanges with EXOP_REPL_OBJ and a GUID: {estr}") >+ self.fail(f"Failed to call GetNCChanges with EXOP_REPL_OBJ, DummyDN and a GUID: {estr}") > > self.assertEqual(ctr.first_object.object.identifier.guid, guid) > >-- >2.25.1 > > >From e5eae1b2440fe436ca5f3a220b808fca1d7bde81 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 26 Jun 2023 16:25:32 +1200 >Subject: [PATCH 10/18] s4-torture/drs: Add test demonstrating that a > GetNCChanges REPL_OBJ will not reset the replication cookie > >This demonstrates the behaviour used by the "Azure AD Connect" cloud sync tool. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit b323169d6ff8357f7c999ae346137166c98218ac) >--- > selftest/knownfail.d/getncchanges | 1 + > source4/torture/drs/python/getncchanges.py | 55 ++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > >diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges >index 5ef1bc98bef..b033b091c72 100644 >--- a/selftest/knownfail.d/getncchanges >+++ b/selftest/knownfail.d/getncchanges >@@ -4,3 +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\) >+samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_do_full_repl_mix_no_overlap >\ No newline at end of file >diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py >index 46598311f79..5b402e81c88 100644 >--- a/source4/torture/drs/python/getncchanges.py >+++ b/source4/torture/drs/python/getncchanges.py >@@ -1213,6 +1213,61 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): > # The NC should be the first object returned due to GET_ANC > self.assertEqual(ctr.first_object.object.identifier.guid, guid) > >+ def _test_do_full_repl_no_overlap(self, mix=True, get_anc=False): >+ self.default_hwm = drsuapi.DsReplicaHighWaterMark() >+ >+ # We set get_anc=True so we can assert the BASE DN will be the >+ # first object >+ ctr6 = self._repl_send_request(get_anc=get_anc) >+ guid_list_1 = self._get_ctr6_object_guids(ctr6) >+ >+ if mix: >+ dc_guid_1 = self.ldb_dc1.get_invocation_id() >+ >+ req8 = self._exop_req8(dest_dsa=None, >+ invocation_id=dc_guid_1, >+ nc_dn_str=self.ldb_dc1.get_default_basedn(), >+ exop=drsuapi.DRSUAPI_EXOP_REPL_OBJ) >+ >+ (level, ctr_repl_obj) = self.drs.DsGetNCChanges(self.drs_handle, 8, req8) >+ >+ self.assertEqual(ctr_repl_obj.extended_ret, drsuapi.DRSUAPI_EXOP_ERR_SUCCESS) >+ >+ repl_obj_guid_list = self._get_ctr6_object_guids(ctr_repl_obj) >+ >+ self.assertEqual(len(repl_obj_guid_list), 1) >+ >+ # This should be the first object in the main replication due >+ # to get_anc=True above in one case, and a rule that the NC must be first regardless otherwise >+ self.assertEqual(repl_obj_guid_list[0], guid_list_1[0]) >+ >+ self.last_ctr = ctr6 >+ ctr6 = self._repl_send_request(get_anc=True) >+ guid_list_2 = self._get_ctr6_object_guids(ctr6) >+ >+ self.assertNotEqual(guid_list_1, guid_list_2) >+ >+ def test_do_full_repl_no_overlap_get_anc(self): >+ """ >+ Make sure that a full replication on an nc succeeds to the goal despite needing multiple passes >+ """ >+ self._test_do_full_repl_no_overlap(mix=False, get_anc=True) >+ >+ def test_do_full_repl_no_overlap(self): >+ """ >+ Make sure that a full replication on an nc succeeds to the goal despite needing multiple passes >+ """ >+ self._test_do_full_repl_no_overlap(mix=False) >+ >+ def test_do_full_repl_mix_no_overlap(self): >+ """ >+ Make sure that a full replication on an nc succeeds to the goal despite needing multiple passes >+ >+ Assert this is true even if we do a REPL_OBJ in between the replications >+ >+ """ >+ self._test_do_full_repl_no_overlap(mix=True) >+ > > class DcConnection: > """Helper class to track a connection to another DC""" >-- >2.25.1 > > >From b795d74db101313d94102c0c5b14c320dfca9b3d Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 24 Jul 2023 12:05:18 +1200 >Subject: [PATCH 11/18] s4-torture/drs: Add test showing that if present in the > set the NC root leads and tmp_highest_usn moves > >The NC root, on any replication when it appears, is the first object to be >replicated, including for all subsequent chunks in the replication. > >However the tmp_highest_usn is not updated by that USN, it must >only be updated for the non-NC changes (to match Windows exactly), >or at least only updated with the non-NC changes until it would >naturally appear. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 87414955212143b8502b4c02aca150bc72cb8de5) >--- > selftest/knownfail.d/getncchanges | 7 +- > source4/torture/drs/python/getncchanges.py | 147 +++++++++++++++++++++ > 2 files changed, 153 insertions(+), 1 deletion(-) > >diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges >index b033b091c72..e9a86ca2ad2 100644 >--- a/selftest/knownfail.d/getncchanges >+++ b/selftest/knownfail.d/getncchanges >@@ -4,4 +4,9 @@ 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\) >-samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_do_full_repl_mix_no_overlap >\ No newline at end of file >+samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_do_full_repl_mix_no_overlap >+samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_nc_change_only\( >+samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first\( >+samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_mid\( >+samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_start_zero\( >+samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_start_zero_nc_change\( >diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py >index 5b402e81c88..580d8cc66f3 100644 >--- a/source4/torture/drs/python/getncchanges.py >+++ b/source4/torture/drs/python/getncchanges.py >@@ -1268,6 +1268,153 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): > """ > self._test_do_full_repl_no_overlap(mix=True) > >+ def nc_change(self): >+ old_base_msg = self.default_conn.ldb_dc.search(base=self.base_dn, >+ scope=SCOPE_BASE, >+ attrs=["oEMInformation"]) >+ rec_cleanup = {"dn": self.base_dn, >+ "oEMInformation": old_base_msg[0]["oEMInformation"][0]} >+ m_cleanup = ldb.Message.from_dict(self.default_conn.ldb_dc, >+ rec_cleanup, >+ ldb.FLAG_MOD_REPLACE) >+ >+ self.addCleanup(self.default_conn.ldb_dc.modify, m_cleanup) >+ >+ rec = {"dn": self.base_dn, >+ "oEMInformation": f"Tortured by Samba's getncchanges.py {self.id()} against {self.default_conn.dnsname_dc}"} >+ m = ldb.Message.from_dict(self.default_conn.ldb_dc, rec, ldb.FLAG_MOD_REPLACE) >+ self.default_conn.ldb_dc.modify(m) >+ >+ def _test_repl_nc_is_first(self, start_at_zero=True, nc_change=True, ou_change=True, mid_change=False): >+ """Tests that the NC is always replicated first, but does not move the >+ tmp_highest_usn at that point, just like 'early' GET_ANC objects. >+ """ >+ >+ # create objects, twice more than the page size of 133 >+ objs = self.create_object_range(0, 300, prefix="obj") >+ >+ if nc_change: >+ self.nc_change() >+ >+ if mid_change: >+ # create even moire objects >+ objs = self.create_object_range(301, 450, prefix="obj2") >+ >+ base_msg = self.default_conn.ldb_dc.search(base=self.base_dn, >+ scope=SCOPE_BASE, >+ attrs=["uSNChanged", >+ "objectGUID"]) >+ >+ base_guid = misc.GUID(base_msg[0]["objectGUID"][0]) >+ base_usn = int(base_msg[0]["uSNChanged"][0]) >+ >+ if ou_change: >+ # Make one more modification. We want to assert we have >+ # caught up to the base DN, but Windows both promotes the NC >+ # to the front and skips including it in the tmp_highest_usn, >+ # so we make a later modification that will be to show we get >+ # this change. >+ rec = {"dn": self.ou, >+ "postalCode": "0"} >+ m = ldb.Message.from_dict(self.default_conn.ldb_dc, rec, ldb.FLAG_MOD_REPLACE) >+ self.default_conn.ldb_dc.modify(m) >+ >+ ou_msg = self.default_conn.ldb_dc.search(base=self.ou, >+ scope=SCOPE_BASE, >+ attrs=["uSNChanged", >+ "objectGUID"]) >+ >+ ou_guid = misc.GUID(ou_msg[0]["objectGUID"][0]) >+ ou_usn = int(ou_msg[0]["uSNChanged"][0]) >+ >+ # Check some predicates about USN ordering that the below tests will rely on >+ if ou_change and nc_change: >+ self.assertGreater(ou_usn, base_usn); >+ elif not ou_change and nc_change: >+ self.assertGreater(base_usn, ou_usn); >+ >+ ctr6 = self.repl_get_next() >+ >+ guid_list_1 = self._get_ctr6_object_guids(ctr6) >+ if nc_change or start_at_zero: >+ self.assertEqual(base_guid, misc.GUID(guid_list_1[0])) >+ self.assertIn(str(base_guid), guid_list_1) >+ self.assertNotIn(str(base_guid), guid_list_1[1:]) >+ else: >+ self.assertNotEqual(base_guid, misc.GUID(guid_list_1[0])) >+ self.assertNotIn(str(base_guid), guid_list_1) >+ >+ self.assertTrue(ctr6.more_data) >+ >+ if not ou_change and nc_change: >+ self.assertLess(ctr6.new_highwatermark.tmp_highest_usn, base_usn) >+ >+ i = 0 >+ while not self.replication_complete(): >+ i = i + 1 >+ last_tmp_highest_usn = ctr6.new_highwatermark.tmp_highest_usn >+ ctr6 = self.repl_get_next() >+ guid_list_2 = self._get_ctr6_object_guids(ctr6) >+ if len(guid_list_2) > 0: >+ self.assertNotEqual(last_tmp_highest_usn, ctr6.new_highwatermark.tmp_highest_usn) >+ >+ if (nc_change or start_at_zero) and base_usn > last_tmp_highest_usn: >+ self.assertEqual(base_guid, misc.GUID(guid_list_2[0]), >+ f"pass={i} more_data={ctr6.more_data} base_usn={base_usn} tmp_highest_usn={ctr6.new_highwatermark.tmp_highest_usn} last_tmp_highest_usn={last_tmp_highest_usn}") >+ self.assertIn(str(base_guid), guid_list_2, >+ f"pass {i}·more_data={ctr6.more_data} base_usn={base_usn} tmp_highest_usn={ctr6.new_highwatermark.tmp_highest_usn} last_tmp_highest_usn={last_tmp_highest_usn}") >+ else: >+ self.assertNotIn(str(base_guid), guid_list_2, >+ f"pass {i}·more_data={ctr6.more_data} base_usn={base_usn} tmp_highest_usn={ctr6.new_highwatermark.tmp_highest_usn} last_tmp_highest_usn={last_tmp_highest_usn}") >+ >+ if ou_change: >+ # The modification to the base OU should be in the final chunk >+ self.assertIn(str(ou_guid), guid_list_2) >+ self.assertGreaterEqual(ctr6.new_highwatermark.highest_usn, >+ ou_usn) >+ else: >+ # Show that the NC root change does not show up in the >+ # highest_usn. We either get the change before or after >+ # it. >+ self.assertNotEqual(ctr6.new_highwatermark.highest_usn, >+ base_usn) >+ self.assertEqual(ctr6.new_highwatermark.highest_usn, >+ ctr6.new_highwatermark.tmp_highest_usn) >+ >+ self.assertFalse(ctr6.more_data) >+ >+ def test_repl_nc_is_first_start_zero_nc_change(self): >+ self.default_hwm = drsuapi.DsReplicaHighWaterMark() >+ self._test_repl_nc_is_first(start_at_zero=True, nc_change=True, ou_change=True) >+ >+ def test_repl_nc_is_first_start_zero(self): >+ # Get the NC change in the middle of the replication stream, certainly not at the start or end >+ self.nc_change() >+ self.default_hwm = drsuapi.DsReplicaHighWaterMark() >+ self._test_repl_nc_is_first(start_at_zero=True, nc_change=False, ou_change=False) >+ >+ def test_repl_nc_is_first_mid(self): >+ # This is a modification of the next test, that Samba >+ # will pass as it will always include the NC in the >+ # tmp_highest_usn at the point where it belongs >+ self._test_repl_nc_is_first(start_at_zero=False, >+ nc_change=True, >+ ou_change=True, >+ mid_change=True) >+ >+ def test_repl_nc_is_first(self): >+ # This is a modification of the next test, that Samba >+ # will pass as it will always include the NC in the >+ # tmp_highest_usn at the point where it belongs >+ self._test_repl_nc_is_first(start_at_zero=False, nc_change=True, ou_change=True) >+ >+ def test_repl_nc_is_first_nc_change_only(self): >+ # This shows that the NC change is not reflected in the tmp_highest_usn >+ self._test_repl_nc_is_first(start_at_zero=False, nc_change=True, ou_change=False) >+ >+ def test_repl_nc_is_first_no_change(self): >+ # The NC should not be present in this replication >+ self._test_repl_nc_is_first(start_at_zero=False, nc_change=False, ou_change=False) > > class DcConnection: > """Helper class to track a connection to another DC""" >-- >2.25.1 > > >From d98524528699681dfbf45c5a23d9274d1d66008f Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Mon, 26 Jun 2023 16:53:10 +1200 >Subject: [PATCH 12/18] s4-rpc_server/drsuapi: Only keep and invalidate > replication cycle state for normal replication > >This changes the GetNCChanges server to use a per-call state for >extended operations like RID_ALLOC or REPL_OBJ and only maintain >and (more importantly) invalidate the state during normal replication. > >This allows REPL_OBJ to be called during a normal replication cycle >that continues using after that call, continuing with the same >highwatermark cookie. > >Azure AD will do a sequence of (roughly) > >* Normal replication (objects 1..100) >* REPL_OBJ (of 1 object) >* Normal replication (objects 101..200) > >However, if there are more than 100 (in this example) objects in the >domain, and the second replication is required, the objects 1..100 >are sent, as the replication state was invalidated by the REPL_OBJ call. > >RN: Improve GetNChanges to address some (but not all "Azure AD Connect") >syncronisation tool looping during the initial user sync phase. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 99579e706312192f46df33d55949db7f1475d0d0) >--- > selftest/knownfail.d/getncchanges | 1 - > source4/rpc_server/drsuapi/dcesrv_drsuapi.h | 2 +- > source4/rpc_server/drsuapi/getncchanges.c | 103 ++++++++++++++++---- > 3 files changed, 84 insertions(+), 22 deletions(-) > >diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges >index e9a86ca2ad2..e92c86cb306 100644 >--- a/selftest/knownfail.d/getncchanges >+++ b/selftest/knownfail.d/getncchanges >@@ -4,7 +4,6 @@ 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\) >-samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_do_full_repl_mix_no_overlap > samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_nc_change_only\( > samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first\( > samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_mid\( >diff --git a/source4/rpc_server/drsuapi/dcesrv_drsuapi.h b/source4/rpc_server/drsuapi/dcesrv_drsuapi.h >index 38375b5ce58..18ac997583c 100644 >--- a/source4/rpc_server/drsuapi/dcesrv_drsuapi.h >+++ b/source4/rpc_server/drsuapi/dcesrv_drsuapi.h >@@ -35,7 +35,7 @@ struct drsuapi_bind_state { > struct GUID remote_bind_guid; > struct drsuapi_DsBindInfoCtr *remote_info; > struct drsuapi_DsBindInfoCtr *local_info; >- struct drsuapi_getncchanges_state *getncchanges_state; >+ struct drsuapi_getncchanges_state *getncchanges_full_repl_state; > }; > > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index aaf8d8519ab..15e959f1639 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -1711,6 +1711,7 @@ static const char *collect_objects_attrs[] = { "uSNChanged", > */ > static WERROR getncchanges_collect_objects(struct drsuapi_bind_state *b_state, > TALLOC_CTX *mem_ctx, >+ struct drsuapi_getncchanges_state *getnc_state, > struct drsuapi_DsGetNCChangesRequest10 *req10, > struct ldb_dn *search_dn, > const char *extra_filter, >@@ -1719,7 +1720,6 @@ static WERROR getncchanges_collect_objects(struct drsuapi_bind_state *b_state, > int ret; > char* search_filter; > enum ldb_scope scope = LDB_SCOPE_SUBTREE; >- struct drsuapi_getncchanges_state *getnc_state = b_state->getncchanges_state; > bool critical_only = false; > > if (req10->replica_flags & DRSUAPI_DRS_CRITICAL_ONLY) { >@@ -1773,6 +1773,7 @@ static WERROR getncchanges_collect_objects(struct drsuapi_bind_state *b_state, > */ > static WERROR getncchanges_collect_objects_exop(struct drsuapi_bind_state *b_state, > TALLOC_CTX *mem_ctx, >+ struct drsuapi_getncchanges_state *getnc_state, > struct drsuapi_DsGetNCChangesRequest10 *req10, > struct drsuapi_DsGetNCChangesCtr6 *ctr6, > struct ldb_dn *search_dn, >@@ -1932,7 +1933,13 @@ static WERROR getncchanges_collect_objects_exop(struct drsuapi_bind_state *b_sta > /* TODO: implement extended op specific collection > * of objects. Right now we just normal procedure > * for collecting objects */ >- return getncchanges_collect_objects(b_state, mem_ctx, req10, search_dn, extra_filter, search_res); >+ return getncchanges_collect_objects(b_state, >+ mem_ctx, >+ getnc_state, >+ req10, >+ search_dn, >+ extra_filter, >+ search_res); > } > } > >@@ -2712,7 +2719,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_ > WERROR werr; > struct dcesrv_handle *h; > struct drsuapi_bind_state *b_state; >- struct drsuapi_getncchanges_state *getnc_state; >+ struct drsuapi_getncchanges_state *getnc_state = NULL; > struct drsuapi_DsGetNCChangesRequest10 *req10; > uint32_t options; > uint32_t link_count = 0; >@@ -2962,7 +2969,31 @@ allowed: > ZERO_STRUCT(req10->highwatermark); > } > >- getnc_state = b_state->getncchanges_state; >+ /* >+ * An extended operation is "special single-response cycle" >+ * per MS-DRSR 4.1.10.1.1 "Start and Finish" so we don't need >+ * to guess if this is a continuation of any long-term >+ * state. >+ * >+ * Otherwise, maintain (including marking as stale, which is >+ * what the below is for) the replication state. >+ * >+ * Note that point 5 "The server implementation MAY declare >+ * the supplied values ... as too stale to use." would allow >+ * resetting the state at almost any point, Microsoft Azure AD >+ * Connect will switch back and forth between a REPL_OBJ and a >+ * full replication, so we must not reset the statue during >+ * extended operations. >+ */ >+ if (req10->extended_op == DRSUAPI_EXOP_NONE && >+ b_state->getncchanges_full_repl_state != NULL) { >+ /* >+ * Knowing that this is not an extended operation, we >+ * can access (and validate) the full replication >+ * state >+ */ >+ getnc_state = b_state->getncchanges_full_repl_state; >+ } > > /* see if a previous replication has been abandoned */ > if (getnc_state) { >@@ -2975,7 +3006,9 @@ allowed: > if (ret != LDB_SUCCESS) { > /* > * This can't fail as we have done this above >- * implicitly but not got the DN out >+ * implicitly but not got the DN out, but >+ * print a good error message regardless just >+ * in case. > */ > DBG_ERR("Bad DN '%s' as Naming Context for GetNCChanges: %s\n", > drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >@@ -2988,7 +3021,7 @@ allowed: > ldb_dn_get_linearized(getnc_state->ncRoot_dn), > ldb_dn_get_linearized(getnc_state->last_dn))); > TALLOC_FREE(getnc_state); >- b_state->getncchanges_state = NULL; >+ b_state->getncchanges_full_repl_state = NULL; > } > } > >@@ -3002,10 +3035,15 @@ allowed: > (ret > 0) ? "older" : "newer", > ldb_dn_get_linearized(getnc_state->last_dn))); > TALLOC_FREE(getnc_state); >- b_state->getncchanges_state = NULL; >+ b_state->getncchanges_full_repl_state = NULL; > } > } > >+ /* >+ * This is either a new replication cycle, or an extended >+ * operation. A new cycle is triggered above by the >+ * TALLOC_FREE() which sets getnc_state to NULL. >+ */ > if (getnc_state == NULL) { > struct ldb_result *res = NULL; > const char *attrs[] = { >@@ -3114,12 +3152,33 @@ allowed: > return WERR_DS_DRA_NOT_SUPPORTED; > } > >- /* Initialize the state we'll store over the replication cycle */ >- getnc_state = talloc_zero(b_state, struct drsuapi_getncchanges_state); >+ /* >+ * Initialize the state, initially for the remainder >+ * of this call (EXOPs) >+ * >+ * An extended operation is a "special single-response >+ * cycle" per MS-DRSR 4.1.10.1.1 "Start and Finish" >+ * >+ */ >+ getnc_state = talloc_zero(mem_ctx, struct drsuapi_getncchanges_state); > if (getnc_state == NULL) { > return WERR_NOT_ENOUGH_MEMORY; > } >- b_state->getncchanges_state = getnc_state; >+ >+ if (req10->extended_op == DRSUAPI_EXOP_NONE) { >+ /* >+ * Promote the memory to being a store of >+ * long-term state that we will use over the >+ * replication cycle for full replication >+ * requests >+ * >+ * Store the state in a clearly named location >+ * for pulling back only during full >+ * replications >+ */ >+ b_state->getncchanges_full_repl_state >+ = talloc_steal(b_state, getnc_state); >+ } > > getnc_state->ncRoot_dn = ncRoot_dn; > talloc_steal(getnc_state, ncRoot_dn); >@@ -3200,11 +3259,13 @@ allowed: > } > > if (req10->extended_op == DRSUAPI_EXOP_NONE) { >- werr = getncchanges_collect_objects(b_state, mem_ctx, req10, >+ werr = getncchanges_collect_objects(b_state, mem_ctx, >+ getnc_state, req10, > search_dn, extra_filter, > &search_res); > } else { >- werr = getncchanges_collect_objects_exop(b_state, mem_ctx, req10, >+ werr = getncchanges_collect_objects_exop(b_state, mem_ctx, >+ getnc_state, req10, > &r->out.ctr->ctr6, > search_dn, extra_filter, > &search_res); >@@ -3656,7 +3717,11 @@ allowed: > r->out.ctr->ctr6.nc_linked_attributes_count = getnc_state->total_links; > } > >- if (!r->out.ctr->ctr6.more_data) { >+ if (req10->extended_op != DRSUAPI_EXOP_NONE) { >+ r->out.ctr->ctr6.uptodateness_vector = NULL; >+ r->out.ctr->ctr6.nc_object_count = 0; >+ ZERO_STRUCT(r->out.ctr->ctr6.new_highwatermark); >+ } else if (!r->out.ctr->ctr6.more_data) { > > /* this is the last response in the replication cycle */ > r->out.ctr->ctr6.new_highwatermark = getnc_state->final_hwm; >@@ -3668,9 +3733,13 @@ allowed: > * that the RPC message we're sending contains links stored in > * getnc_state. mem_ctx is local to this RPC call, so the memory > * will get freed after the RPC message is sent on the wire. >+ * >+ * We must not do this for an EXOP, as that should not >+ * end the replication state, which is why that is >+ * checked first above. > */ > talloc_steal(mem_ctx, getnc_state); >- b_state->getncchanges_state = NULL; >+ b_state->getncchanges_full_repl_state = NULL; > } else { > ret = drsuapi_DsReplicaHighWaterMark_cmp(&r->out.ctr->ctr6.old_highwatermark, > &r->out.ctr->ctr6.new_highwatermark); >@@ -3692,12 +3761,6 @@ allowed: > getnc_state->last_hwm = r->out.ctr->ctr6.new_highwatermark; > } > >- if (req10->extended_op != DRSUAPI_EXOP_NONE) { >- r->out.ctr->ctr6.uptodateness_vector = NULL; >- r->out.ctr->ctr6.nc_object_count = 0; >- ZERO_STRUCT(r->out.ctr->ctr6.new_highwatermark); >- } >- > TALLOC_FREE(repl_chunk); > > DEBUG(r->out.ctr->ctr6.more_data?4:2, >-- >2.25.1 > > >From 728bc773261a295d8e2338f41e846992d0d45ec5 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 17:06:13 +1200 >Subject: [PATCH 13/18] s4-rpc_server/drsuapi: Fix indentation in > GetNCChanges() > >This avoids the indentation correction being in the previous patch. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit fe7418e1765b79f60945b787536b4d84a548fe02) >--- > source4/rpc_server/drsuapi/getncchanges.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 15e959f1639..92fa7ba7575 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -3726,7 +3726,7 @@ allowed: > /* this is the last response in the replication cycle */ > r->out.ctr->ctr6.new_highwatermark = getnc_state->final_hwm; > r->out.ctr->ctr6.uptodateness_vector = talloc_move(mem_ctx, >- &getnc_state->final_udv); >+ &getnc_state->final_udv); > > /* > * Free the state info stored for the replication cycle. Note >-- >2.25.1 > > >From 6e856365525c4c76ce14b0b734794538790f8392 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 14:39:18 +1200 >Subject: [PATCH 14/18] s4-rpc_server/drsuapi: Avoid modification to ncRoot > input variable in GetNCChanges > >This tries to avoid it appearing that ncRoot is a value that can >be trusted and used internally by not updating it and instead leaving >it just as an input/echo-back value. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 548f141f11e89d335d8f9d74ab6925fa6b90fb84) >--- > source4/rpc_server/drsuapi/getncchanges.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 92fa7ba7575..c3806f9e6de 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -3185,7 +3185,6 @@ allowed: > > getnc_state->ncRoot_guid = samdb_result_guid(res->msgs[0], > "objectGUID"); >- ncRoot->guid = getnc_state->ncRoot_guid; > > /* find out if we are to replicate Schema NC */ > ret = ldb_dn_compare_base(ldb_get_schema_basedn(sam_ctx), >@@ -3195,8 +3194,6 @@ allowed: > TALLOC_FREE(res); > } > >- ncRoot->guid = getnc_state->ncRoot_guid; >- > /* we need the session key for encrypting password attributes */ > status = dcesrv_auth_session_key(dce_call, &session_key); > if (!NT_STATUS_IS_OK(status)) { >@@ -3378,11 +3375,19 @@ allowed: > if (r->out.ctr->ctr6.naming_context == NULL) { > return WERR_NOT_ENOUGH_MEMORY; > } >+ >+ /* >+ * Match Windows and echo back the original values from the request, even if >+ * they say DummyDN for the string NC >+ */ > *r->out.ctr->ctr6.naming_context = *ncRoot; > > /* find the SID if there is one */ > dsdb_find_sid_by_dn(sam_ctx, getnc_state->ncRoot_dn, &r->out.ctr->ctr6.naming_context->sid); > >+ /* Set GUID */ >+ r->out.ctr->ctr6.naming_context->guid = getnc_state->ncRoot_guid; >+ > dsdb_get_oid_mappings_drsuapi(schema, true, mem_ctx, &ctr); > r->out.ctr->ctr6.mapping_ctr = *ctr; > >-- >2.25.1 > > >From 18ae38009e73042d3d2aeec1aad753620c2a4f70 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 14:43:39 +1200 >Subject: [PATCH 15/18] s4-rpc_server/drsuapi: Rename ncRoot -> > untrusted_ncRoot to avoid misuse > >Because of the requirement to echo back the original string, we can >not force this to be a trustworthy value. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 2ed9815eeacfcf3a58871bafe0212398cc34c39e) >--- > source4/rpc_server/drsuapi/getncchanges.c | 30 ++++++++++++++--------- > 1 file changed, 19 insertions(+), 11 deletions(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index c3806f9e6de..3e65c13f126 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -2708,7 +2708,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_ > dcesrv_call_session_info(dce_call); > struct imessaging_context *imsg_ctx = > dcesrv_imessaging_context(dce_call->conn); >- struct drsuapi_DsReplicaObjectIdentifier *ncRoot; >+ struct drsuapi_DsReplicaObjectIdentifier *untrusted_ncRoot; > int ret; > uint32_t i, k; > struct dsdb_schema *schema; >@@ -2830,8 +2830,8 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_ > } > > /* Perform access checks. */ >- ncRoot = req10->naming_context; >- if (ncRoot == NULL) { >+ untrusted_ncRoot = req10->naming_context; >+ if (untrusted_ncRoot == NULL) { > DEBUG(0,(__location__ ": Request for DsGetNCChanges with no NC\n")); > return WERR_DS_DRA_INVALID_PARAMETER; > } >@@ -3000,7 +3000,7 @@ allowed: > struct ldb_dn *new_dn; > ret = drs_ObjectIdentifier_to_dn_and_nc_root(getnc_state, > sam_ctx, >- ncRoot, >+ untrusted_ncRoot, > &new_dn, > NULL); > if (ret != LDB_SUCCESS) { >@@ -3011,7 +3011,7 @@ allowed: > * in case. > */ > DBG_ERR("Bad DN '%s' as Naming Context for GetNCChanges: %s\n", >- drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, untrusted_ncRoot), > ldb_strerror(ret)); > return WERR_DS_DRA_INVALID_PARAMETER; > } >@@ -3056,12 +3056,12 @@ allowed: > > ret = drs_ObjectIdentifier_to_dn_and_nc_root(mem_ctx, > sam_ctx, >- ncRoot, >+ untrusted_ncRoot, > &ncRoot_dn, > NULL); > if (ret != LDB_SUCCESS) { > DBG_ERR("Bad DN '%s' as Naming Context or EXOP DN for GetNCChanges: %s\n", >- drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, untrusted_ncRoot), > ldb_strerror(ret)); > return WERR_DS_DRA_BAD_DN; > } >@@ -3380,7 +3380,7 @@ allowed: > * Match Windows and echo back the original values from the request, even if > * they say DummyDN for the string NC > */ >- *r->out.ctr->ctr6.naming_context = *ncRoot; >+ *r->out.ctr->ctr6.naming_context = *untrusted_ncRoot; > > /* find the SID if there is one */ > dsdb_find_sid_by_dn(sam_ctx, getnc_state->ncRoot_dn, &r->out.ctr->ctr6.naming_context->sid); >@@ -3615,7 +3615,15 @@ allowed: > struct drsuapi_DsReplicaUpdateRefsRequest1 ureq; > DEBUG(3,("UpdateRefs on getncchanges for %s\n", > GUID_string(mem_ctx, &req10->destination_dsa_guid))); >- ureq.naming_context = ncRoot; >+ >+ /* >+ * We pass the pre-validation NC root here as >+ * drsuapi_UpdateRefs() has to check its own input >+ * values due to being called from >+ * dcesrv_drsuapi_DsReplicaUpdateRefs() >+ */ >+ >+ ureq.naming_context = untrusted_ncRoot; > ureq.dest_dsa_dns_name = samdb_ntds_msdcs_dns_name(sam_ctx, mem_ctx, > &req10->destination_dsa_guid); > if (!ureq.dest_dsa_dns_name) { >@@ -3638,7 +3646,7 @@ allowed: > &ureq); > if (!W_ERROR_IS_OK(werr)) { > DEBUG(0,(__location__ ": Failed UpdateRefs on %s for %s in DsGetNCChanges - %s\n", >- drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, untrusted_ncRoot), > ureq.dest_dsa_dns_name, > win_errstr(werr))); > } >@@ -3772,7 +3780,7 @@ allowed: > ("DsGetNCChanges with uSNChanged >= %llu flags 0x%08x on %s gave %u objects (done %u/%u) %u links (done %u/%u (as %s))\n", > (unsigned long long)(req10->highwatermark.highest_usn+1), > req10->replica_flags, >- drs_ObjectIdentifier_to_debug_string(mem_ctx, ncRoot), >+ drs_ObjectIdentifier_to_debug_string(mem_ctx, untrusted_ncRoot), > r->out.ctr->ctr6.object_count, > i, r->out.ctr->ctr6.more_data?getnc_state->num_records:i, > r->out.ctr->ctr6.linked_attributes_count, >-- >2.25.1 > > >From 17993c4c036a5a936dff4b482aadff2366084287 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Tue, 27 Jun 2023 17:01:28 +1200 >Subject: [PATCH 16/18] s4-rpc_server/drsuapi: Update getnc_state to be != NULL > >This is closer to our READDME.Coding style > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 2aba9e230ea62efcbd829f6f073894dfa3180c91) >--- > source4/rpc_server/drsuapi/getncchanges.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 3e65c13f126..f932cb03131 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -2996,7 +2996,7 @@ allowed: > } > > /* see if a previous replication has been abandoned */ >- if (getnc_state) { >+ if (getnc_state != NULL) { > struct ldb_dn *new_dn; > ret = drs_ObjectIdentifier_to_dn_and_nc_root(getnc_state, > sam_ctx, >@@ -3025,7 +3025,7 @@ allowed: > } > } > >- if (getnc_state) { >+ if (getnc_state != NULL) { > ret = drsuapi_DsReplicaHighWaterMark_cmp(&getnc_state->last_hwm, > &req10->highwatermark); > if (ret != 0) { >-- >2.25.1 > > >From a75214fb8cdca98d32588a7a0da49014d3921d9a Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Wed, 28 Jun 2023 15:57:47 +1200 >Subject: [PATCH 17/18] s4-rpc_server/drsuapi: Ensure logs show DN for > replicated objects, not (null) > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15407 >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 17359afa627a3086ec8d6862f007a3479574a8b4) >--- > source4/rpc_server/drsuapi/getncchanges.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index f932cb03131..448217c03d6 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -3567,7 +3567,15 @@ allowed: > getncchanges_chunk_add_objects(repl_chunk, new_objs); > > talloc_free(getnc_state->last_dn); >- getnc_state->last_dn = talloc_move(getnc_state, &msg->dn); >+ /* >+ * talloc_steal() as we still need msg->dn to >+ * be a valid pointer for the log on the next >+ * line. >+ * >+ * msg only remains in scope for the next 25 >+ * lines or so anyway. >+ */ >+ getnc_state->last_dn = talloc_steal(getnc_state, msg->dn); > } > > DEBUG(8,(__location__ ": %s object %s new tmp_highest_usn=%" PRIu64 "\n", >-- >2.25.1 > > >From 2d7a2dd1b44c63f287ae0667613d175b582422c9 Mon Sep 17 00:00:00 2001 >From: Andrew Bartlett <abartlet@samba.org> >Date: Wed, 26 Jul 2023 14:27:16 +1200 >Subject: [PATCH 18/18] s4-rpc_server/drsupai: Avoid looping with Azure AD > Connect by not incrementing temp_highest_usn for the NC root > >We send the NC root first, as a special case for every chunk >that we send until the natural point where it belongs. > >We do not bump the tmp_highest_usn in the highwatermark that >the client and server use (it is meant to be an opauqe cookie) >until the 'natural' point where the object appears, similar >to the cache for GET_ANC. > >The issue is that without this, because the NC root was sorted >first in whatever chunk it appeared in but could have a 'high' >highwatermark, Azure AD Connect will send back the same >new_highwatermark->tmp_highest_usn, and due to a bug, >a zero reserved_usn, which makes Samba discard it. > >The reserved_usn is now much less likely to ever be set because >the tmp_higest_usn is now always advancing. > >RN: Avoid infinite loop in initial user sync with Azure AD Connect > when synchronising a large Samba AD domain. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401 > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >Reviewed-by: Stefan Metzmacher <metze@samba.org> >(cherry picked from commit 79ca6ef28a6f94965cb030c4a7da8c1b9db7150b) >--- > selftest/knownfail.d/getncchanges | 5 +- > source4/rpc_server/drsuapi/getncchanges.c | 117 ++++++++++++++++------ > 2 files changed, 87 insertions(+), 35 deletions(-) > >diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges >index e92c86cb306..bda9b31a1b1 100644 >--- a/selftest/knownfail.d/getncchanges >+++ b/selftest/knownfail.d/getncchanges >@@ -4,8 +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\) >+# Samba chooses to always increment the USN for the NC root at the point where it would otherwise show up. > samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_nc_change_only\( >-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first\( >-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_mid\( >-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_start_zero\( >-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_start_zero_nc_change\( >diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c >index 448217c03d6..8864e79f10e 100644 >--- a/source4/rpc_server/drsuapi/getncchanges.c >+++ b/source4/rpc_server/drsuapi/getncchanges.c >@@ -62,6 +62,7 @@ struct drsuapi_getncchanges_state { > bool is_get_anc; > bool broken_samba_4_5_get_anc_emulation; > bool is_get_tgt; >+ bool send_nc_root_first; > uint64_t min_usn; > uint64_t max_usn; > struct drsuapi_DsReplicaHighWaterMark last_hwm; >@@ -1038,18 +1039,6 @@ static int site_res_cmp_usn_order(struct drsuapi_changed_objects *m1, > struct drsuapi_changed_objects *m2, > struct drsuapi_getncchanges_state *getnc_state) > { >- int ret; >- >- ret = ldb_dn_compare(getnc_state->ncRoot_dn, m1->dn); >- if (ret == 0) { >- return -1; >- } >- >- ret = ldb_dn_compare(getnc_state->ncRoot_dn, m2->dn); >- if (ret == 0) { >- return 1; >- } >- > if (m1->usn == m2->usn) { > return ldb_dn_compare(m2->dn, m1->dn); > } >@@ -2124,7 +2113,7 @@ static WERROR getncchanges_get_sorted_array(const struct drsuapi_DsReplicaLinked > * @param new_objs if parents are added, this gets updated to point to a chain > * of parent objects (with the parents first and the child last) > */ >-static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemEx *child_obj, >+static WERROR getncchanges_add_ancestors(const struct GUID *parent_object_guid, > struct ldb_dn *child_dn, > TALLOC_CTX *mem_ctx, > struct ldb_context *sam_ctx, >@@ -2147,7 +2136,7 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE > DSDB_SECRET_ATTRIBUTES, > NULL }; > >- next_anc_guid = child_obj->parent_object_guid; >+ next_anc_guid = parent_object_guid; > > while (next_anc_guid != NULL) { > struct drsuapi_DsReplicaObjectListItemEx *anc_obj = NULL; >@@ -2156,19 +2145,24 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE > struct ldb_dn *anc_dn = NULL; > > /* >- * Don't send an object twice. (If we've sent the object, then >- * we've also sent all its parents as well) >+ * For the GET_ANC case (but not the 'send NC root >+ * first' case), don't send an object twice. >+ * >+ * (If we've sent the object, then we've also sent all >+ * its parents as well) > */ >- werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache, >- next_anc_guid); >- if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) { >- return WERR_OK; >- } >- if (W_ERROR_IS_OK(werr)) { >- return WERR_INTERNAL_ERROR; >- } >- if (!W_ERROR_EQUAL(werr, WERR_OBJECT_NOT_FOUND)) { >- return werr; >+ if (getnc_state->obj_cache) { >+ werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache, >+ next_anc_guid); >+ if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) { >+ return WERR_OK; >+ } >+ if (W_ERROR_IS_OK(werr)) { >+ return WERR_INTERNAL_ERROR; >+ } >+ if (!W_ERROR_EQUAL(werr, WERR_OBJECT_NOT_FOUND)) { >+ return werr; >+ } > } > > anc_obj = talloc_zero(mem_ctx, >@@ -2222,11 +2216,18 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE > /* > * Regardless of whether we actually use it or not, > * we add it to the cache so we don't look at it again >+ * >+ * The only time we are here without >+ * getnc_state->obj_cache is for the forced addition >+ * of the NC root to the start of the reply, this we >+ * want to add each and every call.. > */ >- werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache, >- next_anc_guid); >- if (!W_ERROR_IS_OK(werr)) { >- return werr; >+ if (getnc_state->obj_cache) { >+ werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache, >+ next_anc_guid); >+ if (!W_ERROR_IS_OK(werr)) { >+ return werr; >+ } > } > > /* >@@ -2355,7 +2356,8 @@ static WERROR getncchanges_get_obj_to_send(const struct ldb_message *msg, > */ > if (getnc_state->is_get_anc > && !getnc_state->broken_samba_4_5_get_anc_emulation) { >- werr = getncchanges_add_ancestors(obj, msg->dn, mem_ctx, >+ werr = getncchanges_add_ancestors(obj->parent_object_guid, >+ msg->dn, mem_ctx, > sam_ctx, getnc_state, > schema, session_key, > req10, local_pas, >@@ -3287,6 +3289,12 @@ allowed: > if (changes[i].usn > getnc_state->max_usn) { > getnc_state->max_usn = changes[i].usn; > } >+ >+ if (req10->extended_op == DRSUAPI_EXOP_NONE && >+ GUID_equal(&changes[i].guid, &getnc_state->ncRoot_guid)) >+ { >+ getnc_state->send_nc_root_first = true; >+ } > } > > if (req10->extended_op == DRSUAPI_EXOP_NONE) { >@@ -3427,6 +3435,36 @@ allowed: > uint32_t_ptr_cmp); > } > >+ /* >+ * If we have the NC root in this replication, send it >+ * first regardless. However, don't bump the USN now, >+ * treat it as if it was sent early due to GET_ANC >+ * >+ * This is triggered for each call, so every page of responses >+ * gets the NC root as the first object, up to the point where >+ * it naturally occurs in the replication. >+ */ >+ >+ if (getnc_state->send_nc_root_first) { >+ struct drsuapi_DsReplicaObjectListItemEx *new_objs = NULL; >+ >+ werr = getncchanges_add_ancestors(&getnc_state->ncRoot_guid, >+ NULL, mem_ctx, >+ sam_ctx, getnc_state, >+ schema, &session_key, >+ req10, local_pas, >+ machine_dn, &new_objs); >+ >+ if (!W_ERROR_IS_OK(werr)) { >+ return werr; >+ } >+ >+ getncchanges_chunk_add_objects(repl_chunk, new_objs); >+ >+ DEBUG(8,(__location__ ": replicating NC root %s\n", >+ ldb_dn_get_linearized(getnc_state->ncRoot_dn))); >+ } >+ > /* > * Check in case we're still processing the links from an object in the > * previous chunk. We want to send the links (and any targets needed) >@@ -3465,6 +3503,23 @@ allowed: > TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx); > uint32_t old_la_index; > >+ /* >+ * Once we get to the 'natural' place to send the NC >+ * root, stop sending it at the front of each reply >+ * and make sure to suppress sending it now >+ * >+ * We don't just 'continue' here as we must send links >+ * and unlike Windows we want to update the >+ * tmp_highest_usn >+ */ >+ >+ if (getnc_state->send_nc_root_first && >+ GUID_equal(&getnc_state->guids[i], &getnc_state->ncRoot_guid)) >+ { >+ getnc_state->send_nc_root_first = false; >+ obj_already_sent = true; >+ } >+ > msg_dn = ldb_dn_new_fmt(tmp_ctx, sam_ctx, "<GUID=%s>", > GUID_string(tmp_ctx, &getnc_state->guids[i])); > W_ERROR_HAVE_NO_MEMORY(msg_dn); >-- >2.25.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
metze
:
review+
Actions:
View
Attachments on
bug 15401
:
18057
| 18063