From 8add91de238774607677e16b7876b43e3e37d7f1 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Mon, 29 May 2017 17:06:55 +1200 Subject: [PATCH 01/20] drs: support sync-forced for 'samba-tool drs replicate --local' The sync-forced option wasn't being passed into the replication request when the --local option was used. This meant if outbound replication were disabled on the target DC, then the replicate --local command would fail. Signed-off-by: Tim Beale Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 314b96e18371e92cde8172c3f3739b52970ee2d7) --- python/samba/drs_utils.py | 6 +++++- python/samba/netcmd/drs.py | 9 ++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/python/samba/drs_utils.py b/python/samba/drs_utils.py index 8624f3f..1719af6 100644 --- a/python/samba/drs_utils.py +++ b/python/samba/drs_utils.py @@ -200,7 +200,7 @@ class drs_Replicate(object): def replicate(self, dn, source_dsa_invocation_id, destination_dsa_guid, schema=False, exop=drsuapi.DRSUAPI_EXOP_NONE, rodc=False, - replica_flags=None, full_sync=True): + replica_flags=None, full_sync=True, sync_forced=False): '''replicate a single DN''' # setup for a GetNCChanges call @@ -262,6 +262,10 @@ class drs_Replicate(object): drsuapi.DRSUAPI_DRS_SPECIAL_SECRET_PROCESSING) else: req8.replica_flags |= drsuapi.DRSUAPI_DRS_WRIT_REP + + if sync_forced: + req8.replica_flags |= drsuapi.DRSUAPI_DRS_SYNC_FORCED + req8.max_object_count = 402 req8.max_ndr_size = 402116 req8.extended_op = exop diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py index b9b876a..e1886b9 100644 --- a/python/samba/netcmd/drs.py +++ b/python/samba/netcmd/drs.py @@ -241,7 +241,8 @@ class cmd_drs_kcc(Command): -def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False, single_object=False): +def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False, single_object=False, + sync_forced=False): '''replicate from a source DC to the local SAM''' self.server = SOURCE_DC @@ -284,7 +285,7 @@ def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False, single_object=Fals (num_objects, num_links) = repl.replicate(NC, source_dsa_invocation_id, destination_dsa_guid, rodc=rodc, full_sync=full_sync, - exop=exop) + exop=exop, sync_forced=sync_forced) except Exception, e: raise CommandError("Error replicating DN %s" % NC, e) self.samdb.transaction_commit() @@ -332,7 +333,9 @@ class cmd_drs_replicate(Command): self.creds = credopts.get_credentials(self.lp, fallback_machine=True) if local: - drs_local_replicate(self, SOURCE_DC, NC, full_sync=full_sync, single_object=single_object) + drs_local_replicate(self, SOURCE_DC, NC, full_sync=full_sync, + single_object=single_object, + sync_forced=sync_forced) return if local_online: -- 1.9.1 From 74fcf311f6bf990f8d607c80557e8a6527dd5483 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Wed, 7 Jun 2017 16:56:18 +1200 Subject: [PATCH 02/20] drs_utils: HWM in 'samba-tool drs replicate --local' always zero The code to check for the 'repsFrom' highwatermark didn't have any effect because the hwm variable was overwritten (initialized to all zeroes) further down. Using a zero HWM probably wouldn't have impacted functionality because we were still correctly using the uptodatenessvector, which should avoid a full replication. This was introduced in commit e2ba17d26af42974e5d, presumably by accident. Signed-off-by: Tim Beale Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 4bd8467018eee181588c13490edbea26b488ee36) --- python/samba/drs_utils.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/python/samba/drs_utils.py b/python/samba/drs_utils.py index 1719af6..b9ed059 100644 --- a/python/samba/drs_utils.py +++ b/python/samba/drs_utils.py @@ -211,7 +211,13 @@ class drs_Replicate(object): req8.naming_context = drsuapi.DsReplicaObjectIdentifier() req8.naming_context.dn = dn + # Default to a full replication if we don't find an upToDatenessVector udv = None + hwm = drsuapi.DsReplicaHighWaterMark() + hwm.tmp_highest_usn = 0 + hwm.reserved_usn = 0 + hwm.highest_usn = 0 + if not full_sync: res = self.samdb.search(base=dn, scope=ldb.SCOPE_BASE, attrs=["repsFrom"]) @@ -238,12 +244,6 @@ class drs_Replicate(object): udv.cursors = cursors_v1 udv.count = len(cursors_v1) - # If we can't find an upToDateVector, or where told not to, replicate fully - hwm = drsuapi.DsReplicaHighWaterMark() - hwm.tmp_highest_usn = 0 - hwm.reserved_usn = 0 - hwm.highest_usn = 0 - req8.highwatermark = hwm req8.uptodateness_vector = udv -- 1.9.1 From 43e430defa4e3deb628df730f9de73a6cc4dd146 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Mon, 19 Jun 2017 10:26:48 +1200 Subject: [PATCH 03/20] 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 Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (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; -- 1.9.1 From 9a6c9c3f165929ae58c13c082d2fa88c386f137c Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Wed, 7 Jun 2017 11:13:52 +1200 Subject: [PATCH 04/20] getnc_exop.py: Fix typo in function name This drove me crazy when I tried to search for it. Signed-off-by: Tim Beale Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit dddcf80660dd883ee24a1dfa13fd5cfe4cd49e62) --- source4/torture/drs/python/drs_base.py | 2 +- source4/torture/drs/python/getnc_exop.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py index f07592d..f19efca 100644 --- a/source4/torture/drs/python/drs_base.py +++ b/source4/torture/drs/python/drs_base.py @@ -166,7 +166,7 @@ class DrsBaseTestCase(SambaToolCmdTest): utdv.cursors = cursors return (hwm, utdv) - def _get_indentifier(self, ldb_conn, dn): + def _get_identifier(self, ldb_conn, dn): res = ldb_conn.search(dn, scope=ldb.SCOPE_BASE, attrs=["objectGUID", "objectSid"]) id = drsuapi.DsReplicaObjectIdentifier() diff --git a/source4/torture/drs/python/getnc_exop.py b/source4/torture/drs/python/getnc_exop.py index 26786be..caa7826 100644 --- a/source4/torture/drs/python/getnc_exop.py +++ b/source4/torture/drs/python/getnc_exop.py @@ -146,20 +146,20 @@ class DrsReplicaSyncTestCase(drs_base.DrsBaseTestCase): "dn": ou1, "objectclass": "organizationalUnit" }) - ou1_id = self._get_indentifier(self.ldb_dc1, ou1) + ou1_id = self._get_identifier(self.ldb_dc1, ou1) ou2 = "OU=get_anc2,%s" % ou1 self.ldb_dc1.add({ "dn": ou2, "objectclass": "organizationalUnit" }) - ou2_id = self._get_indentifier(self.ldb_dc1, ou2) + ou2_id = self._get_identifier(self.ldb_dc1, ou2) dc3 = "CN=test_anc_dc_%u,%s" % (random.randint(0, 4294967295), ou2) self.ldb_dc1.add({ "dn": dc3, "objectclass": "computer", "userAccountControl": "%d" % (samba.dsdb.UF_ACCOUNTDISABLE | samba.dsdb.UF_SERVER_TRUST_ACCOUNT) }) - dc3_id = self._get_indentifier(self.ldb_dc1, dc3) + dc3_id = self._get_identifier(self.ldb_dc1, dc3) req8 = self._exop_req8(dest_dsa=None, invocation_id=self.ldb_dc1.get_invocation_id(), @@ -201,20 +201,20 @@ class DrsReplicaSyncTestCase(drs_base.DrsBaseTestCase): "dn": ou1, "objectclass": "organizationalUnit" }) - ou1_id = self._get_indentifier(self.ldb_dc1, ou1) + ou1_id = self._get_identifier(self.ldb_dc1, ou1) ou2 = "OU=get_anc2,%s" % ou1 self.ldb_dc1.add({ "dn": ou2, "objectclass": "organizationalUnit" }) - ou2_id = self._get_indentifier(self.ldb_dc1, ou2) + ou2_id = self._get_identifier(self.ldb_dc1, ou2) dc3 = "CN=test_anc_dc_%u,%s" % (random.randint(0, 4294967295), ou2) self.ldb_dc1.add({ "dn": dc3, "objectclass": "computer", "userAccountControl": "%d" % (samba.dsdb.UF_ACCOUNTDISABLE | samba.dsdb.UF_SERVER_TRUST_ACCOUNT) }) - dc3_id = self._get_indentifier(self.ldb_dc1, dc3) + dc3_id = self._get_identifier(self.ldb_dc1, dc3) (hwm1, utdv1) = self._check_replication([ou1,ou2,dc3], drsuapi.DRSUAPI_DRS_WRIT_REP) @@ -325,7 +325,7 @@ class DrsReplicaSyncTestCase(drs_base.DrsBaseTestCase): "dn": cn3, "objectclass": "container", }) - cn3_id = self._get_indentifier(self.ldb_dc1, cn3) + cn3_id = self._get_identifier(self.ldb_dc1, cn3) (hwm5, utdv5) = self._check_replication([dc3,ou1,ou2,self.ou,cn3], drsuapi.DRSUAPI_DRS_WRIT_REP) -- 1.9.1 From 82d6bfe06beb887708d6898633cc37eed713d5f3 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Mon, 12 Jun 2017 11:20:54 +1200 Subject: [PATCH 05/20] getncchanges.c: Remove unused null_scope variable This was added in 4cc6b5a69b1f94d96a73ac1 but the very next commit (f1c6bab60e52624f5f3) removed where it was set, which meant the variable was always false and seemingly pointless. Signed-off-by: Tim Beale Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 50b638d15cceafc4f3e78d1fab8c7ed0a6e44a1f) --- source4/rpc_server/drsuapi/getncchanges.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c index 32580bd..77d881f 100644 --- a/source4/rpc_server/drsuapi/getncchanges.c +++ b/source4/rpc_server/drsuapi/getncchanges.c @@ -2022,7 +2022,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_ uint32_t link_total = 0; uint32_t link_given = 0; struct ldb_dn *search_dn = NULL; - bool am_rodc, null_scope=false; + bool am_rodc; enum security_user_level security_level; struct ldb_context *sam_ctx; struct dom_sid *user_sid; @@ -2553,7 +2553,6 @@ allowed: for (i=getnc_state->num_processed; inum_records && - !null_scope && (r->out.ctr->ctr6.object_count < max_objects) && !max_wait_reached; i++) { -- 1.9.1 From 7554c814c46b06b47d89a1a383db5d82301fcae4 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 20 Jun 2017 13:14:43 +1200 Subject: [PATCH 06/20] repl: Remove old TODO This TODO was added in 2007 before we supported linked attributes. It's no longer relevant. Signed-off-by: Tim Beale Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 69f593ec5a89411fb3e2cbf62f2092c3073fa0f7) --- source4/dsdb/repl/replicated_objects.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/source4/dsdb/repl/replicated_objects.c b/source4/dsdb/repl/replicated_objects.c index e5fec1f..862fafa 100644 --- a/source4/dsdb/repl/replicated_objects.c +++ b/source4/dsdb/repl/replicated_objects.c @@ -835,8 +835,6 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb, return WERR_NOT_ENOUGH_MEMORY; } - /* TODO: handle linked attributes */ - /* wrap the extended operation in a transaction See [MS-DRSR] 3.3.2 Transactions */ -- 1.9.1 From de0e72d533f83a07a0b8f1aea901ee802231b0a6 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 30 May 2017 15:08:44 +1200 Subject: [PATCH 07/20] werror: Add WERR_DS_DRA_RECYCLED_TARGET When the DRS client encounters a linked attribute with an unknown target object, it should return a RECYCLED_TARGET error, which should result in the client resending the GETNCChanges request with the GET_TGT flag set. This error code is currently documented by Microsoft under System Error Codes (8200-8999). I contacted them and they will also add it to the MS-ERREF doc in future. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit d13e7b92f85e5623c5cf1309f13d59141dc7d888) --- libcli/util/werror.h | 1 + 1 file changed, 1 insertion(+) diff --git a/libcli/util/werror.h b/libcli/util/werror.h index d3d3327..0370a06 100644 --- a/libcli/util/werror.h +++ b/libcli/util/werror.h @@ -100,6 +100,7 @@ typedef uint32_t WERROR; #define WERR_INVALID_PRIMARY_GROUP W_ERROR(0x0000051C) #define WERR_DS_DRA_SECRETS_DENIED W_ERROR(0x000021B6) +#define WERR_DS_DRA_RECYCLED_TARGET W_ERROR(0x000021BF) #define WERR_DNS_ERROR_KEYMASTER_REQUIRED W_ERROR(0x0000238D) #define WERR_DNS_ERROR_NOT_ALLOWED_ON_SIGNED_ZONE W_ERROR(0x0000238E) -- 1.9.1 From 24ee8e6b3dd23fd8759bd9e867e81cc8867e7333 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Wed, 14 Jun 2017 10:51:59 +1200 Subject: [PATCH 08/20] replmd: Split checking link attr target into new function We want to re-use this code to check that the linked attribute's target object exists *before* we try to commit the transaction. This will allow us to re-request the block with the GET_TGT flag set. This splits checking the target object exists into a separate function. Minor changes of note: - the 'parent' argument was passed to replmd_process_linked_attribute() as NULL, so I've just replaced where it was used in the refactored code with NULL. - I've tweaked the "Failed to find GUID" error message slightly to display the attribute ID rather than the attribute name (saves repeating lookups and/or passing extra arguments). - Tweaked the replmd_deletion_state() logic - it only made sense to call it in the code block where we actually found the target Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 04ce638a1f7166b3fe75b52efd0a522248196fd4) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 171 ++++++++++++++---------- 1 file changed, 102 insertions(+), 69 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 0ed24cb..53e2c12 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -6627,6 +6627,105 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct return replmd_replicated_apply_next(ar); } +/** + * Checks that the target object for a linked attribute exists. + * If it exists, its GUID and deletion-state are returned + */ +static int replmd_check_target_exists(struct ldb_module *module, + struct dsdb_dn *dsdb_dn, + struct la_entry *la_entry, + struct ldb_dn *source_dn, + struct GUID *guid, + enum deletion_state *target_deletion_state) +{ + struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la; + struct ldb_context *ldb = ldb_module_get_ctx(module); + struct ldb_result *target_res; + TALLOC_CTX *tmp_ctx = talloc_new(la_entry); + const char *attrs[] = { "isDeleted", "isRecycled", NULL }; + NTSTATUS ntstatus; + int ret; + bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE) ? true : false; + + *target_deletion_state = OBJECT_REMOVED; + + ntstatus = dsdb_get_extended_dn_guid(dsdb_dn->dn, guid, "GUID"); + + if (!NT_STATUS_IS_OK(ntstatus) && !active) { + + /* + * This strange behaviour (allowing a NULL/missing + * GUID) originally comes from: + * + * commit e3054ce0fe0f8f62d2f5b2a77893e7a1479128bd + * Author: Andrew Tridgell + * Date: Mon Dec 21 21:21:55 2009 +1100 + * + * s4-drs: cope better with NULL GUIDS from DRS + * + * It is valid to get a NULL GUID over DRS for a deleted forward link. We + * need to match by DN if possible when seeing if we should update an + * existing link. + * + * Pair-Programmed-With: Andrew Bartlett + */ + ret = dsdb_module_search_dn(module, tmp_ctx, &target_res, + dsdb_dn->dn, attrs, + DSDB_FLAG_NEXT_MODULE | + DSDB_SEARCH_SHOW_RECYCLED | + DSDB_SEARCH_SEARCH_ALL_PARTITIONS | + DSDB_SEARCH_SHOW_DN_IN_STORAGE_FORMAT, + NULL); + } else if (!NT_STATUS_IS_OK(ntstatus)) { + ldb_asprintf_errstring(ldb, "Failed to find GUID in linked attribute 0x%x blob for %s from %s", + la->attid, + ldb_dn_get_linearized(dsdb_dn->dn), + ldb_dn_get_linearized(source_dn)); + talloc_free(tmp_ctx); + return LDB_ERR_OPERATIONS_ERROR; + } else { + ret = dsdb_module_search(module, tmp_ctx, &target_res, + NULL, LDB_SCOPE_SUBTREE, + attrs, + DSDB_FLAG_NEXT_MODULE | + DSDB_SEARCH_SHOW_RECYCLED | + DSDB_SEARCH_SEARCH_ALL_PARTITIONS | + DSDB_SEARCH_SHOW_DN_IN_STORAGE_FORMAT, + NULL, + "objectGUID=%s", + GUID_string(tmp_ctx, guid)); + } + + if (ret != LDB_SUCCESS) { + ldb_asprintf_errstring(ldb, "Failed to re-resolve GUID %s: %s\n", + GUID_string(tmp_ctx, guid), + ldb_errstring(ldb)); + talloc_free(tmp_ctx); + return ret; + } + + 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))); + } else if (target_res->count != 1) { + ldb_asprintf_errstring(ldb, "More than one object found matching objectGUID %s\n", + GUID_string(tmp_ctx, guid)); + ret = LDB_ERR_OPERATIONS_ERROR; + } else { + struct ldb_message *target_msg = target_res->msgs[0]; + + dsdb_dn->dn = talloc_steal(dsdb_dn, target_msg->dn); + + /* Get the object's state (i.e. Not Deleted, Tombstone, etc) */ + replmd_deletion_state(module, target_msg, + target_deletion_state, NULL); + } + + talloc_free(tmp_ctx); + return ret; +} + /* process one linked attribute structure */ @@ -6638,7 +6737,6 @@ static int replmd_process_linked_attribute(struct ldb_module *module, struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la; struct ldb_context *ldb = ldb_module_get_ctx(module); struct ldb_message *msg; - struct ldb_message *target_msg = NULL; TALLOC_CTX *tmp_ctx = talloc_new(la_entry); const struct dsdb_schema *schema = dsdb_get_schema(ldb, tmp_ctx); int ret; @@ -6649,17 +6747,15 @@ static int replmd_process_linked_attribute(struct ldb_module *module, WERROR status; time_t t = time(NULL); struct ldb_result *res; - struct ldb_result *target_res; const char *attrs[4]; - const char *attrs2[] = { "isDeleted", "isRecycled", NULL }; struct parsed_dn *pdn_list, *pdn, *next; struct GUID guid = GUID_zero(); - NTSTATUS ntstatus; bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false; enum deletion_state deletion_state = OBJECT_NOT_DELETED; enum deletion_state target_deletion_state = OBJECT_NOT_DELETED; + /* linked_attributes[0]: &objs->linked_attributes[i]: struct drsuapi_DsReplicaLinkedAttribute @@ -6779,74 +6875,14 @@ linked_attributes[0]: return LDB_ERR_OPERATIONS_ERROR; } - ntstatus = dsdb_get_extended_dn_guid(dsdb_dn->dn, &guid, "GUID"); - if (!NT_STATUS_IS_OK(ntstatus) && !active) { - /* - * This strange behaviour (allowing a NULL/missing - * GUID) originally comes from: - * - * commit e3054ce0fe0f8f62d2f5b2a77893e7a1479128bd - * Author: Andrew Tridgell - * Date: Mon Dec 21 21:21:55 2009 +1100 - * - * s4-drs: cope better with NULL GUIDS from DRS - * - * It is valid to get a NULL GUID over DRS for a deleted forward link. We - * need to match by DN if possible when seeing if we should update an - * existing link. - * - * Pair-Programmed-With: Andrew Bartlett - */ - - ret = dsdb_module_search_dn(module, tmp_ctx, &target_res, - dsdb_dn->dn, attrs2, - DSDB_FLAG_NEXT_MODULE | - DSDB_SEARCH_SHOW_RECYCLED | - DSDB_SEARCH_SEARCH_ALL_PARTITIONS | - DSDB_SEARCH_SHOW_DN_IN_STORAGE_FORMAT, - parent); - } else if (!NT_STATUS_IS_OK(ntstatus)) { - ldb_asprintf_errstring(ldb, "Failed to find GUID in linked attribute blob for %s on %s from %s", - old_el->name, - ldb_dn_get_linearized(dsdb_dn->dn), - ldb_dn_get_linearized(msg->dn)); - talloc_free(tmp_ctx); - return LDB_ERR_OPERATIONS_ERROR; - } else { - ret = dsdb_module_search(module, tmp_ctx, &target_res, - NULL, LDB_SCOPE_SUBTREE, - attrs2, - DSDB_FLAG_NEXT_MODULE | - DSDB_SEARCH_SHOW_RECYCLED | - DSDB_SEARCH_SEARCH_ALL_PARTITIONS | - DSDB_SEARCH_SHOW_DN_IN_STORAGE_FORMAT, - parent, - "objectGUID=%s", - GUID_string(tmp_ctx, &guid)); - } + ret = replmd_check_target_exists(module, dsdb_dn, la_entry, msg->dn, + &guid, &target_deletion_state); if (ret != LDB_SUCCESS) { - ldb_asprintf_errstring(ldb_module_get_ctx(module), "Failed to re-resolve GUID %s: %s\n", - GUID_string(tmp_ctx, &guid), - ldb_errstring(ldb_module_get_ctx(module))); talloc_free(tmp_ctx); return ret; } - 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))); - } 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)); - talloc_free(tmp_ctx); - return LDB_ERR_OPERATIONS_ERROR; - } 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 @@ -6854,9 +6890,6 @@ linked_attributes[0]: * 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; -- 1.9.1 From 456bc3bc9ced09b98483a2f535d7daeb9f859b7e Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Wed, 14 Jun 2017 11:35:36 +1200 Subject: [PATCH 09/20] drs: Fail replication transaction instead of dropping links If the DRS client received a linked attribute that it couldn't resolve the target for, then it would just ignore that link and keep going. That link would then be lost forever (although a full-sync would resolve this). Instead of silently ignoring the link, fail the transaction. This *can* happen on Samba, but it is unusual. The target object and linked-attribute would need to be added while a replication is still in progress. It can also happen fairly easily when talking to a Windows DC. There are two import exceptions to this: 1). Linked attributes that span partitions. We can never guarantee that we will have received the target object, because it may be in a partition we haven't replicated yet. Samba doesn't have a great way of handling this currently, but we shouldn't fail the replication (because that breaks basic join tests). Just skip that linked attribute and hope that a subsequent full-sync will fix it. (I queried Microsoft and they said resolving cross-partition linked attributes is a implementation-specific problem to solve. GET_TGT won't resolve it) 2). When the replication involves a subset of objects, e.g. critical-only. In these cases, we don't increase the highwater-mark, so it is probably not such a dire problem if we don't add the link. In the case of critical-only, we will do a subsequent full sync which will then add the links. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit f69596cd21d8106afdf494f39d1fcebea2b0f5dd) --- source4/dsdb/repl/drepl_out_helpers.c | 4 ++ source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 92 ++++++++++++++++++++++++- source4/dsdb/samdb/samdb.h | 2 +- source4/libnet/libnet_vampire.c | 1 + 4 files changed, 95 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 53e2c12..dc16674 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); @@ -6628,6 +6638,46 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct } /** + * 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; +} + +/** * Checks that the target object for a linked attribute exists. * If it exists, its GUID and deletion-state are returned */ @@ -6705,9 +6755,45 @@ static int replmd_check_target_exists(struct ldb_module *module, } 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))); + + /* + * 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(source_dn))); + + } else if (replmd_objects_have_same_nc(ldb, tmp_ctx, source_dn, + dsdb_dn->dn)) { + ldb_asprintf_errstring(ldb, "Unknown target %s GUID %s linked from %s\n", + ldb_dn_get_linearized(dsdb_dn->dn), + GUID_string(tmp_ctx, guid), + ldb_dn_get_linearized(source_dn)); + ret = LDB_ERR_NO_SUCH_OBJECT; + } else { + + /* + * TODO: + * We don't handle cross-partition links well here (we + * could potentially lose them), but don't fail the + * replication. + */ + DEBUG(2,("Failed to resolve cross-partition link between %s and %s\n", + ldb_dn_get_linearized(source_dn), + ldb_dn_get_linearized(dsdb_dn->dn))); + } } else if (target_res->count != 1) { ldb_asprintf_errstring(ldb, "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 */ -- 1.9.1 From 103e7d9e695a1d82afea21ce5b406f72e0340ffc Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 15 Jun 2017 14:09:27 +1200 Subject: [PATCH 10/20] drs: Check target object is known after applying objects Currently we only check that the target object is known at the end of the transaction (i.e. the .prepare_commit hook). It's too late at this point to resend the request with GET_TGT. Move this processing earlier on, after we've applied all the objects (i.e. off the .extended hook). In reality, we need to perform the checks at both points. I've split the common code that gets the source/target details out of the la_entry into a helper function. It's not the greatest function ever, but seemed to make more sense than duplicating the code. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 67617d470028b45c722c598944a17591cab1e6ba) --- source4/dsdb/repl/replicated_objects.c | 11 +- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 178 ++++++++++++++++++------ 2 files changed, 145 insertions(+), 44 deletions(-) diff --git a/source4/dsdb/repl/replicated_objects.c b/source4/dsdb/repl/replicated_objects.c index 862fafa..dd84570 100644 --- a/source4/dsdb/repl/replicated_objects.c +++ b/source4/dsdb/repl/replicated_objects.c @@ -884,12 +884,15 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb, dsdb_reference_schema(ldb, cur_schema, false); } - if (!W_ERROR_EQUAL(objects->error, WERR_DS_DRA_MISSING_PARENT)) { - DEBUG(1,("Failed to apply records: %s: %s\n", - ldb_errstring(ldb), ldb_strerror(ret))); - } else { + if (W_ERROR_EQUAL(objects->error, WERR_DS_DRA_RECYCLED_TARGET)) { + DEBUG(3,("Missing target while attempting to apply records: %s\n", + ldb_errstring(ldb))); + } else if (W_ERROR_EQUAL(objects->error, WERR_DS_DRA_MISSING_PARENT)) { DEBUG(3,("Missing parent while attempting to apply records: %s\n", ldb_errstring(ldb))); + } else { + DEBUG(1,("Failed to apply records: %s: %s\n", + ldb_errstring(ldb), ldb_strerror(ret))); } ldb_transaction_cancel(ldb); TALLOC_FREE(tmp_ctx); diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index dc16674..c04358f 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -114,6 +114,7 @@ static int replmd_check_upgrade_links(struct ldb_context *ldb, struct parsed_dn *dns, uint32_t count, struct ldb_message_element *el, const char *ldap_oid); +static int replmd_verify_linked_attributes(struct replmd_replicated_request *ar); enum urgent_situation { REPL_URGENT_ON_CREATE = 1, @@ -6057,7 +6058,18 @@ static int replmd_replicated_apply_next(struct replmd_replicated_request *ar) struct GUID_txt_buf guid_str_buf; if (ar->index_current >= ar->objs->num_objects) { - /* done with it, go to next stage */ + + /* + * Now that we've applied all the objects, check that the new + * linked attributes only reference objects we know about + */ + ret = replmd_verify_linked_attributes(ar); + + if (ret != LDB_SUCCESS) { + return ret; + } + + /* done applying objects, move on to the next stage */ return replmd_replicated_uptodate_vector(ar); } @@ -6812,35 +6824,29 @@ static int replmd_check_target_exists(struct ldb_module *module, return ret; } -/* - process one linked attribute structure +/** + * Extracts the key details about the source/target object for a + * linked-attribute entry. + * This returns the following details: + * @param ret_attr the schema details for the linked attribute + * @param source_msg the search result for the source object + * @param target_dsdb_dn the unpacked DN info for the target object */ -static int replmd_process_linked_attribute(struct ldb_module *module, - struct replmd_private *replmd_private, +static int replmd_extract_la_entry_details(struct ldb_module *module, struct la_entry *la_entry, - struct ldb_request *parent) + TALLOC_CTX *mem_ctx, + const struct dsdb_attribute **ret_attr, + struct ldb_message **source_msg, + struct dsdb_dn **target_dsdb_dn) { struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la; struct ldb_context *ldb = ldb_module_get_ctx(module); - struct ldb_message *msg; - TALLOC_CTX *tmp_ctx = talloc_new(la_entry); - const struct dsdb_schema *schema = dsdb_get_schema(ldb, tmp_ctx); + const struct dsdb_schema *schema = dsdb_get_schema(ldb, mem_ctx); int ret; const struct dsdb_attribute *attr; - struct dsdb_dn *dsdb_dn; - uint64_t seq_num = 0; - struct ldb_message_element *old_el; WERROR status; - time_t t = time(NULL); struct ldb_result *res; const char *attrs[4]; - struct parsed_dn *pdn_list, *pdn, *next; - struct GUID guid = GUID_zero(); - bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false; - - enum deletion_state deletion_state = OBJECT_NOT_DELETED; - enum deletion_state target_deletion_state = OBJECT_NOT_DELETED; - /* linked_attributes[0]: @@ -6885,7 +6891,6 @@ linked_attributes[0]: la->attid, GUID_buf_string(&la->identifier->guid, &guid_str)); - talloc_free(tmp_ctx); return LDB_ERR_OPERATIONS_ERROR; } @@ -6894,28 +6899,130 @@ linked_attributes[0]: attrs[2] = "isRecycled"; attrs[3] = NULL; - /* get the existing message from the db for the object with - this GUID, returning attribute being modified. We will then - use this msg as the basis for a modify call */ - ret = dsdb_module_search(module, tmp_ctx, &res, NULL, LDB_SCOPE_SUBTREE, attrs, + /* + * get the existing message from the db for the object with + * this GUID, returning attribute being modified. We will then + * use this msg as the basis for a modify call + */ + ret = dsdb_module_search(module, mem_ctx, &res, NULL, LDB_SCOPE_SUBTREE, attrs, DSDB_FLAG_NEXT_MODULE | DSDB_SEARCH_SEARCH_ALL_PARTITIONS | DSDB_SEARCH_SHOW_RECYCLED | DSDB_SEARCH_SHOW_DN_IN_STORAGE_FORMAT | DSDB_SEARCH_REVEAL_INTERNALS, - parent, - "objectGUID=%s", GUID_string(tmp_ctx, &la->identifier->guid)); + NULL, + "objectGUID=%s", GUID_string(mem_ctx, &la->identifier->guid)); if (ret != LDB_SUCCESS) { - talloc_free(tmp_ctx); return ret; } if (res->count != 1) { ldb_asprintf_errstring(ldb, "DRS linked attribute for GUID %s - DN not found", - GUID_string(tmp_ctx, &la->identifier->guid)); - talloc_free(tmp_ctx); + GUID_string(mem_ctx, &la->identifier->guid)); return LDB_ERR_NO_SUCH_OBJECT; } - msg = res->msgs[0]; + + *source_msg = res->msgs[0]; + + /* the value blob for the attribute holds the target object DN */ + status = dsdb_dn_la_from_blob(ldb, attr, schema, mem_ctx, la->value.blob, target_dsdb_dn); + if (!W_ERROR_IS_OK(status)) { + ldb_asprintf_errstring(ldb, "Failed to parsed linked attribute blob for %s on %s - %s\n", + attr->lDAPDisplayName, + ldb_dn_get_linearized(res->msgs[0]->dn), + win_errstr(status)); + return LDB_ERR_OPERATIONS_ERROR; + } + + *ret_attr = attr; + + return LDB_SUCCESS; +} + +/** + * Verifies that the source and target objects are known for each linked + * attribute in the current transaction. + */ +static int replmd_verify_linked_attributes(struct replmd_replicated_request *ar) +{ + int ret = LDB_SUCCESS; + struct la_entry *la; + struct ldb_module *module = ar->module; + TALLOC_CTX *tmp_ctx = talloc_new(ar); + struct replmd_private *replmd_private = + talloc_get_type(ldb_module_get_private(module), struct replmd_private); + + for (la = DLIST_TAIL(replmd_private->la_list); la; la = DLIST_PREV(la)) { + struct ldb_message *src_msg; + const struct dsdb_attribute *attr; + struct dsdb_dn *tgt_dsdb_dn; + enum deletion_state del_state = OBJECT_NOT_DELETED; + struct GUID guid = GUID_zero(); + + ret = replmd_extract_la_entry_details(module, la, tmp_ctx, &attr, + &src_msg, &tgt_dsdb_dn); + + if (ret != LDB_SUCCESS) { + break; + } + + ret = replmd_check_target_exists(module, tgt_dsdb_dn, la, + src_msg->dn, &guid, &del_state); + + /* + * When we fail to find the target object, the error code we pass + * back here is really important. It flags back to the callers to + * retry this request with DRSUAPI_DRS_GET_TGT + */ + if (ret == LDB_ERR_NO_SUCH_OBJECT) { + ret = replmd_replicated_request_werror(ar, WERR_DS_DRA_RECYCLED_TARGET); + } + + if (ret != LDB_SUCCESS) { + break; + } + } + + talloc_free(tmp_ctx); + return ret; +} + +/* + process one linked attribute structure + */ +static int replmd_process_linked_attribute(struct ldb_module *module, + struct replmd_private *replmd_private, + struct la_entry *la_entry, + struct ldb_request *parent) +{ + struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la; + struct ldb_context *ldb = ldb_module_get_ctx(module); + struct ldb_message *msg; + TALLOC_CTX *tmp_ctx = talloc_new(la_entry); + const struct dsdb_schema *schema = dsdb_get_schema(ldb, tmp_ctx); + int ret; + const struct dsdb_attribute *attr; + struct dsdb_dn *dsdb_dn; + uint64_t seq_num = 0; + struct ldb_message_element *old_el; + time_t t = time(NULL); + struct parsed_dn *pdn_list, *pdn, *next; + struct GUID guid = GUID_zero(); + bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false; + + enum deletion_state deletion_state = OBJECT_NOT_DELETED; + enum deletion_state target_deletion_state = OBJECT_NOT_DELETED; + + /* + * get the attribute being modified, the search result for the source object, + * and the target object's DN details + */ + ret = replmd_extract_la_entry_details(module, la_entry, tmp_ctx, &attr, + &msg, &dsdb_dn); + + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } /* * Check for deleted objects per MS-DRSR 4.1.10.6.13 @@ -6924,7 +7031,6 @@ linked_attributes[0]: * any existing link, that should have happened when the * object deletion was replicated or initiated. */ - replmd_deletion_state(module, msg, &deletion_state, NULL); if (deletion_state >= OBJECT_RECYCLED) { @@ -6953,14 +7059,6 @@ linked_attributes[0]: return ret; } - status = dsdb_dn_la_from_blob(ldb, attr, schema, tmp_ctx, la->value.blob, &dsdb_dn); - if (!W_ERROR_IS_OK(status)) { - ldb_asprintf_errstring(ldb, "Failed to parsed linked attribute blob for %s on %s - %s\n", - old_el->name, ldb_dn_get_linearized(msg->dn), win_errstr(status)); - talloc_free(tmp_ctx); - return LDB_ERR_OPERATIONS_ERROR; - } - ret = replmd_check_target_exists(module, dsdb_dn, la_entry, msg->dn, &guid, &target_deletion_state); -- 1.9.1 From dedce122c8aa1f62f6f359ef5b49443b450cb2a7 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 15 Jun 2017 16:52:33 +1200 Subject: [PATCH 11/20] drepl: Support GET_TGT on periodic replication client - Update IDL comments to include Microsoft reference doc - Add support for sending v10 GetNCChanges request (needed for the GET_TGT flag, which is in the new 'more_flags' field) - Update to also set the GET_TGT flag in the same place we were setting GET_ANC (I split this logic out into a separate function). - The state struct now needs to hold a 'more_flags' field as well (this flag is different to the GET_ANC replica flag) Note that using the GET_TGT when replicating from a Windows DC could be highly inefficient. Because Samba keeps the GET_TGT flag set throughout the replication cycle, it will basically receive a repeated object from Windows for every single linked attribute that it receives. I believe Windows behaviour only expects the client to set the GET_TGT flag when it actually needs to (i.e. when it receives a target object it doesn't know about), rather than throughout the replication cycle. However, this approach won't work with Samba-to-Samba replication, because when the server receives the GET_TGT flag it restarts the replication cycle from scratch. So if we only set the GET_TGT flag when the client encountered an unknown target then Samba-to-Samba could potentially get into an endless replication loop. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit cc201c2c4f5d2dfd18f68143d001c5ba02c00b32) --- librpc/idl/drsuapi.idl | 4 +- source4/dsdb/repl/drepl_out_helpers.c | 97 +++++++++++++++++++++++++++++------ source4/dsdb/repl/drepl_out_pull.c | 1 + source4/dsdb/repl/drepl_service.h | 2 + 4 files changed, 86 insertions(+), 18 deletions(-) diff --git a/librpc/idl/drsuapi.idl b/librpc/idl/drsuapi.idl index 2eb1d65..51ef567 100644 --- a/librpc/idl/drsuapi.idl +++ b/librpc/idl/drsuapi.idl @@ -70,7 +70,9 @@ interface drsuapi } drsuapi_DrsUpdate; /*****************/ - /* Function 0x00 */ + /* Function 0x00 drsuapi_DsBind() */ + + /* MS-DRSR 5.39 DRS_EXTENSIONS_INT */ typedef [bitmap32bit] bitmap { DRSUAPI_SUPPORTED_EXTENSION_BASE = 0x00000001, DRSUAPI_SUPPORTED_EXTENSION_ASYNC_REPLICATION = 0x00000002, diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c index ad4801a..6e24e7f 100644 --- a/source4/dsdb/repl/drepl_out_helpers.c +++ b/source4/dsdb/repl/drepl_out_helpers.c @@ -555,7 +555,28 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req) } r->in.bind_handle = &drsuapi->bind_handle; - if (drsuapi->remote_info28.supported_extensions & DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V8) { + + if (drsuapi->remote_info28.supported_extensions & DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V10) { + r->in.level = 10; + r->in.req->req10.destination_dsa_guid = service->ntds_guid; + r->in.req->req10.source_dsa_invocation_id= rf1->source_dsa_invocation_id; + r->in.req->req10.naming_context = &partition->nc; + r->in.req->req10.highwatermark = highwatermark; + r->in.req->req10.uptodateness_vector = uptodateness_vector; + r->in.req->req10.replica_flags = replica_flags; + r->in.req->req10.max_object_count = 133; + r->in.req->req10.max_ndr_size = 1336811; + r->in.req->req10.extended_op = state->op->extended_op; + r->in.req->req10.fsmo_info = state->op->fsmo_info; + r->in.req->req10.partial_attribute_set = pas; + r->in.req->req10.partial_attribute_set_ex= NULL; + r->in.req->req10.mapping_ctr.num_mappings= mappings == NULL ? 0 : mappings->num_mappings; + r->in.req->req10.mapping_ctr.mappings = mappings == NULL ? NULL : mappings->mappings; + + /* the only difference to v8 is the more_flags */ + r->in.req->req10.more_flags = state->op->more_flags; + + } else if (drsuapi->remote_info28.supported_extensions & DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V8) { r->in.level = 8; r->in.req->req8.destination_dsa_guid = service->ntds_guid; r->in.req->req8.source_dsa_invocation_id= rf1->source_dsa_invocation_id; @@ -693,6 +714,53 @@ static void dreplsrv_op_pull_source_get_changes_done(struct tevent_req *subreq) dreplsrv_op_pull_source_apply_changes_trigger(req, r, ctr_level, ctr1, ctr6); } +/** + * If processing a chunk of replication data fails, check if it is due to a + * problem that can be fixed by setting extra flags in the GetNCChanges request, + * i.e. GET_ANC or GET_TGT. + * @returns NT_STATUS_OK if the request was retried, and an error code if not + */ +static NTSTATUS dreplsrv_op_pull_retry_with_flags(struct tevent_req *req, + WERROR error_code) +{ + struct dreplsrv_op_pull_source_state *state; + NTSTATUS nt_status = NT_STATUS_OK; + + state = tevent_req_data(req, struct dreplsrv_op_pull_source_state); + + /* + * Check if we failed to apply the records due to a missing parent or + * target object. If so, try again and ask for any mising parent/target + * objects to be included this time. + */ + if (W_ERROR_EQUAL(error_code, WERR_DS_DRA_RECYCLED_TARGET)) { + + if (state->op->more_flags & DRSUAPI_DRS_GET_TGT) { + DEBUG(1,("Missing target object despite setting DRSUAPI_DRS_GET_TGT flag\n")); + nt_status = NT_STATUS_INVALID_NETWORK_RESPONSE; + } else { + state->op->more_flags |= DRSUAPI_DRS_GET_TGT; + DEBUG(1,("Missing target object when we didn't set the DRSUAPI_DRS_GET_TGT flag, retrying\n")); + dreplsrv_op_pull_source_get_changes_trigger(req); + } + } else if (W_ERROR_EQUAL(error_code, WERR_DS_DRA_MISSING_PARENT)) { + + if (state->op->options & DRSUAPI_DRS_GET_ANC) { + DEBUG(1,("Missing parent object despite setting DRSUAPI_DRS_GET_ANC flag\n")); + nt_status = NT_STATUS_INVALID_NETWORK_RESPONSE; + } else { + state->op->options |= DRSUAPI_DRS_GET_ANC; + DEBUG(4,("Missing parent object when we didn't set the DRSUAPI_DRS_GET_ANC flag, retrying\n")); + dreplsrv_op_pull_source_get_changes_trigger(req); + } + } else { + nt_status = werror_to_ntstatus(WERR_BAD_NET_RESP); + } + + return nt_status; +} + + static void dreplsrv_update_refs_trigger(struct tevent_req *req); static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req, @@ -937,25 +1005,20 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req if (!W_ERROR_IS_OK(status)) { /* - * If we failed to apply the records due to a missing - * parent, try again after asking for the parent - * records first. Because we don't update the - * highwatermark, we start this part of the cycle - * again. + * Check if this error can be fixed by resending the GetNCChanges + * request with extra flags set (i.e. GET_ANC/GET_TGT) */ - if (((state->op->options & DRSUAPI_DRS_GET_ANC) == 0) - && W_ERROR_EQUAL(status, WERR_DS_DRA_MISSING_PARENT)) { - state->op->options |= DRSUAPI_DRS_GET_ANC; - DEBUG(4,("Missing parent object when we didn't set the DRSUAPI_DRS_GET_ANC flag, retrying\n")); - dreplsrv_op_pull_source_get_changes_trigger(req); + nt_status = dreplsrv_op_pull_retry_with_flags(req, status); + + if (NT_STATUS_IS_OK(nt_status)) { + + /* + * We resent the request. Don't update the highwatermark, + * we'll start this part of the cycle again. + */ return; - } else if (((state->op->options & DRSUAPI_DRS_GET_ANC)) - && W_ERROR_EQUAL(status, WERR_DS_DRA_MISSING_PARENT)) { - DEBUG(1,("Missing parent object despite setting DRSUAPI_DRS_GET_ANC flag\n")); - nt_status = NT_STATUS_INVALID_NETWORK_RESPONSE; - } else { - nt_status = werror_to_ntstatus(WERR_BAD_NET_RESP); } + DEBUG(0,("Failed to commit objects: %s/%s\n", win_errstr(status), nt_errstr(nt_status))); tevent_req_nterror(req, nt_status); diff --git a/source4/dsdb/repl/drepl_out_pull.c b/source4/dsdb/repl/drepl_out_pull.c index 8af6412..78442a9 100644 --- a/source4/dsdb/repl/drepl_out_pull.c +++ b/source4/dsdb/repl/drepl_out_pull.c @@ -126,6 +126,7 @@ WERROR dreplsrv_schedule_partition_pull_source(struct dreplsrv_service *s, op->callback = callback; op->cb_data = cb_data; op->schedule_time = time(NULL); + op->more_flags = 0; DLIST_ADD_END(s->ops.pending, op); diff --git a/source4/dsdb/repl/drepl_service.h b/source4/dsdb/repl/drepl_service.h index 6b85ad6..5fc894f 100644 --- a/source4/dsdb/repl/drepl_service.h +++ b/source4/dsdb/repl/drepl_service.h @@ -130,6 +130,8 @@ struct dreplsrv_out_operation { enum drsuapi_DsExtendedError extended_ret; dreplsrv_extended_callback_t callback; void *cb_data; + /* more replication flags - used by DsReplicaSync GET_TGT */ + uint32_t more_flags; }; struct dreplsrv_notify_operation { -- 1.9.1 From 29e76ff39ed215cc58ec89f72e502c2087bf2bcc Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Fri, 16 Jun 2017 09:49:16 +1200 Subject: [PATCH 12/20] replmd: Set GET_ANC if Windows sends a link with unknown source object Windows replication can send the linked attribute before it sends the source object. The MS-DRSR spec says that in this case the client should resend the GetNCChanges request with the GET_ANC flag set. In my testing this resolves the problem - Windows will include the source object for the linked attribute in the same replication chunk. This problem doesn't happen with Samba-to-Samba replication, because the source object for the linked attribute is guaranteed to have already been sent. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit f87332eb35638cc38f83c580d4623ab978088601) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index c04358f..fde9b72 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -6961,6 +6961,17 @@ static int replmd_verify_linked_attributes(struct replmd_replicated_request *ar) ret = replmd_extract_la_entry_details(module, la, tmp_ctx, &attr, &src_msg, &tgt_dsdb_dn); + /* + * When we fail to find the source object, the error code we pass + * back here is really important. It flags back to the callers to + * retry this request with DRSUAPI_DRS_GET_ANC. This case should + * never happen if we're replicating from a Samba DC, but it is + * needed to talk to a Windows DC + */ + if (ret == LDB_ERR_NO_SUCH_OBJECT) { + ret = replmd_replicated_request_werror(ar, WERR_DS_DRA_MISSING_PARENT); + } + if (ret != LDB_SUCCESS) { break; } -- 1.9.1 From 5214d264df575e40a4a6626f5ab532cc3b6dfd05 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Fri, 16 Jun 2017 12:54:32 +1200 Subject: [PATCH 13/20] drs_utils: Add GET_TGT support to 'samba-tool drs replicate --local' Update drs_Replicate.replicate() so it handles being passed the GET_TGT flag (more_flags). To do this, we need to always use a v10 GetNCChanges request (v8 and v10 are essentially the same except for the more_flags). If the replicate_chunk() call into the C bindings throws an error, check to see whether the error could be fixed by setting the GET_TGT flag, and re-send the request if so. Unfortunately because WERR_DS_DRA_RECYCLED_TARGET isn't documented with the other AD error codes, I've left it hardcoded for now (Microsoft should be fixing up their Docs). Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit f812c29d4000ef35fa5d7f6007606ca53c5ed37a) --- python/samba/drs_utils.py | 101 ++++++++++++++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 35 deletions(-) diff --git a/python/samba/drs_utils.py b/python/samba/drs_utils.py index b9ed059..bed8817 100644 --- a/python/samba/drs_utils.py +++ b/python/samba/drs_utils.py @@ -21,6 +21,8 @@ from samba.dcerpc import drsuapi, misc, drsblobs from samba.net import Net from samba.ndr import ndr_unpack from samba import dsdb +from samba import werror +from samba import WERRORError import samba, ldb @@ -198,18 +200,37 @@ class drs_Replicate(object): raise RuntimeError("Must not set GUID 00000000-0000-0000-0000-000000000000 as invocation_id") self.replication_state = self.net.replicate_init(self.samdb, lp, self.drs, invocation_id) + def _should_retry_with_get_tgt(self, error_code, req): + + # If the error indicates we fail to resolve a target object for a + # linked attribute, then we should retry the request with GET_TGT + # (if we support it and haven't already tried that) + + # TODO fix up the below line when we next update werror_err_table.txt + # and pull in the new error-code + # return (error_code == werror.WERR_DS_DRA_RECYCLED_TARGET and + return (error_code == 0x21bf and + (req.more_flags & drsuapi.DRSUAPI_DRS_GET_TGT) == 0 and + self.supported_extensions & drsuapi.DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V10) + def replicate(self, dn, source_dsa_invocation_id, destination_dsa_guid, schema=False, exop=drsuapi.DRSUAPI_EXOP_NONE, rodc=False, - replica_flags=None, full_sync=True, sync_forced=False): + replica_flags=None, full_sync=True, sync_forced=False, more_flags=0): '''replicate a single DN''' # setup for a GetNCChanges call - req8 = drsuapi.DsGetNCChangesRequest8() + if self.supported_extensions & drsuapi.DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V10: + req = drsuapi.DsGetNCChangesRequest10() + req.more_flags = more_flags + req_level = 10 + else: + req_level = 8 + req = drsuapi.DsGetNCChangesRequest8() - req8.destination_dsa_guid = destination_dsa_guid - req8.source_dsa_invocation_id = source_dsa_invocation_id - req8.naming_context = drsuapi.DsReplicaObjectIdentifier() - req8.naming_context.dn = dn + req.destination_dsa_guid = destination_dsa_guid + req.source_dsa_invocation_id = source_dsa_invocation_id + req.naming_context = drsuapi.DsReplicaObjectIdentifier() + req.naming_context.dn = dn # Default to a full replication if we don't find an upToDatenessVector udv = None @@ -244,49 +265,46 @@ class drs_Replicate(object): udv.cursors = cursors_v1 udv.count = len(cursors_v1) - req8.highwatermark = hwm - req8.uptodateness_vector = udv + req.highwatermark = hwm + req.uptodateness_vector = udv if replica_flags is not None: - req8.replica_flags = replica_flags + req.replica_flags = replica_flags elif exop == drsuapi.DRSUAPI_EXOP_REPL_SECRET: - req8.replica_flags = 0 + req.replica_flags = 0 else: - req8.replica_flags = (drsuapi.DRSUAPI_DRS_INIT_SYNC | - drsuapi.DRSUAPI_DRS_PER_SYNC | - drsuapi.DRSUAPI_DRS_GET_ANC | - drsuapi.DRSUAPI_DRS_NEVER_SYNCED | - drsuapi.DRSUAPI_DRS_GET_ALL_GROUP_MEMBERSHIP) + req.replica_flags = (drsuapi.DRSUAPI_DRS_INIT_SYNC | + drsuapi.DRSUAPI_DRS_PER_SYNC | + drsuapi.DRSUAPI_DRS_GET_ANC | + drsuapi.DRSUAPI_DRS_NEVER_SYNCED | + drsuapi.DRSUAPI_DRS_GET_ALL_GROUP_MEMBERSHIP) if rodc: - req8.replica_flags |= ( - drsuapi.DRSUAPI_DRS_SPECIAL_SECRET_PROCESSING) + req.replica_flags |= ( + drsuapi.DRSUAPI_DRS_SPECIAL_SECRET_PROCESSING) else: - req8.replica_flags |= drsuapi.DRSUAPI_DRS_WRIT_REP + req.replica_flags |= drsuapi.DRSUAPI_DRS_WRIT_REP if sync_forced: - req8.replica_flags |= drsuapi.DRSUAPI_DRS_SYNC_FORCED + req.replica_flags |= drsuapi.DRSUAPI_DRS_SYNC_FORCED - req8.max_object_count = 402 - req8.max_ndr_size = 402116 - req8.extended_op = exop - req8.fsmo_info = 0 - req8.partial_attribute_set = None - req8.partial_attribute_set_ex = None - req8.mapping_ctr.num_mappings = 0 - req8.mapping_ctr.mappings = None + req.max_object_count = 402 + req.max_ndr_size = 402116 + req.extended_op = exop + req.fsmo_info = 0 + req.partial_attribute_set = None + req.partial_attribute_set_ex = None + req.mapping_ctr.num_mappings = 0 + req.mapping_ctr.mappings = None if not schema and rodc: - req8.partial_attribute_set = drs_get_rodc_partial_attribute_set(self.samdb) + req.partial_attribute_set = drs_get_rodc_partial_attribute_set(self.samdb) - if self.supported_extensions & drsuapi.DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V8: - req_level = 8 - req = req8 - else: + if not self.supported_extensions & drsuapi.DRSUAPI_SUPPORTED_EXTENSION_GETCHGREQ_V8: req_level = 5 req5 = drsuapi.DsGetNCChangesRequest5() for a in dir(req5): if a[0] != '_': - setattr(req5, a, getattr(req8, a)) + setattr(req5, a, getattr(req, a)) req = req5 num_objects = 0 @@ -295,8 +313,21 @@ class drs_Replicate(object): (level, ctr) = self.drs.DsGetNCChanges(self.drs_handle, req_level, req) if ctr.first_object is None and ctr.object_count != 0: raise RuntimeError("DsGetNCChanges: NULL first_object with object_count=%u" % (ctr.object_count)) - self.net.replicate_chunk(self.replication_state, level, ctr, - schema=schema, req_level=req_level, req=req) + + try: + self.net.replicate_chunk(self.replication_state, level, ctr, + schema=schema, req_level=req_level, req=req) + except WERRORError as e: + # Check if retrying with the GET_TGT flag set might resolve this error + if self._should_retry_with_get_tgt(e[0], req): + + print("Missing target object - retrying with DRS_GET_TGT") + req.more_flags |= drsuapi.DRSUAPI_DRS_GET_TGT + + # try sending the request again + continue + else: + raise e num_objects += ctr.object_count -- 1.9.1 From ea45aef2875aca3a68b47c9ea848ec62e265f3ac Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 22 Jun 2017 10:43:31 +1200 Subject: [PATCH 14/20] replmd: Move where we store linked attributes There was a bug in my previous patch where the code would verify *all* links in the list, rather than just the ones that are new. And it would do this for every replication chunk it received, regardless of whether there were actually any links in that chunk. The problem is by the time we want to verify the attributes, we don't actually know which attributes are new. We can fix this by moving where we store the linked attributes from the start of processing the replication chunk to the end of processing the chunk. We can then verify the new linked attributes at the same time we store them. Longer-term we may want to try to apply the linked attribute at this point. This would save looking up the source/target objects twice, but it makes things a bit more complicated (attributes will usually apply at this point *most* of the time, but not *all* the time). Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 18ea167adef59b80982f13135947e58cb56a4f16) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 193 +++++++++++++----------- 1 file changed, 103 insertions(+), 90 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index fde9b72..0a53d1d 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -114,7 +114,8 @@ static int replmd_check_upgrade_links(struct ldb_context *ldb, struct parsed_dn *dns, uint32_t count, struct ldb_message_element *el, const char *ldap_oid); -static int replmd_verify_linked_attributes(struct replmd_replicated_request *ar); +static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar, + struct la_entry *la); enum urgent_situation { REPL_URGENT_ON_CREATE = 1, @@ -6042,6 +6043,70 @@ static int replmd_replicated_apply_search_callback(struct ldb_request *req, return LDB_SUCCESS; } +/** + * Stores the linked attributes received in the replication chunk - these get + * applied at the end of the transaction. We also check that each linked + * attribute is valid, i.e. source and target objects are known. + */ +static int replmd_store_linked_attributes(struct replmd_replicated_request *ar) +{ + int ret = LDB_SUCCESS; + uint32_t i; + bool incomplete_subset; + struct ldb_module *module = ar->module; + struct replmd_private *replmd_private = + talloc_get_type(ldb_module_get_private(module), struct replmd_private); + struct ldb_context *ldb; + + ldb = ldb_module_get_ctx(module); + incomplete_subset = (ar->objs->dsdb_repl_flags & DSDB_REPL_FLAG_OBJECT_SUBSET); + + DEBUG(4,("linked_attributes_count=%u\n", ar->objs->linked_attributes_count)); + + /* save away the linked attributes for the end of the transaction */ + for (i = 0; i < ar->objs->linked_attributes_count; i++) { + struct la_entry *la_entry; + + if (replmd_private->la_ctx == NULL) { + replmd_private->la_ctx = talloc_new(replmd_private); + } + la_entry = talloc(replmd_private->la_ctx, struct la_entry); + if (la_entry == NULL) { + ldb_oom(ldb); + return LDB_ERR_OPERATIONS_ERROR; + } + la_entry->la = talloc(la_entry, struct drsuapi_DsReplicaLinkedAttribute); + if (la_entry->la == NULL) { + talloc_free(la_entry); + ldb_oom(ldb); + return LDB_ERR_OPERATIONS_ERROR; + } + *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); + talloc_steal(la_entry->la, la_entry->la->value.blob); + + ret = replmd_verify_linked_attribute(ar, la_entry); + + if (ret != LDB_SUCCESS) { + break; + } + + DLIST_ADD(replmd_private->la_list, la_entry); + } + + return ret; +} + static int replmd_replicated_uptodate_vector(struct replmd_replicated_request *ar); static int replmd_replicated_apply_next(struct replmd_replicated_request *ar) @@ -6060,10 +6125,10 @@ static int replmd_replicated_apply_next(struct replmd_replicated_request *ar) if (ar->index_current >= ar->objs->num_objects) { /* - * Now that we've applied all the objects, check that the new - * linked attributes only reference objects we know about + * Now that we've applied all the objects, check the new linked + * attributes and store them (we apply them in .prepare_commit) */ - ret = replmd_verify_linked_attributes(ar); + ret = replmd_store_linked_attributes(ar); if (ret != LDB_SUCCESS) { return ret; @@ -6547,10 +6612,6 @@ static int replmd_extended_replicated_objects(struct ldb_module *module, struct struct replmd_replicated_request *ar; 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); ldb = ldb_module_get_ctx(module); @@ -6606,45 +6667,6 @@ 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)); - - /* save away the linked attributes for the end of the - transaction */ - for (i=0; iobjs->linked_attributes_count; i++) { - struct la_entry *la_entry; - - if (replmd_private->la_ctx == NULL) { - replmd_private->la_ctx = talloc_new(replmd_private); - } - la_entry = talloc(replmd_private->la_ctx, struct la_entry); - if (la_entry == NULL) { - ldb_oom(ldb); - return LDB_ERR_OPERATIONS_ERROR; - } - la_entry->la = talloc(la_entry, struct drsuapi_DsReplicaLinkedAttribute); - if (la_entry->la == NULL) { - talloc_free(la_entry); - ldb_oom(ldb); - return LDB_ERR_OPERATIONS_ERROR; - } - *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); - talloc_steal(la_entry->la, la_entry->la->value.blob); - - DLIST_ADD(replmd_private->la_list, la_entry); - } return replmd_replicated_apply_next(ar); } @@ -6939,58 +6961,49 @@ linked_attributes[0]: } /** - * Verifies that the source and target objects are known for each linked - * attribute in the current transaction. + * Verifies the source and target objects are known for a linked attribute */ -static int replmd_verify_linked_attributes(struct replmd_replicated_request *ar) +static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar, + struct la_entry *la) { int ret = LDB_SUCCESS; - struct la_entry *la; + TALLOC_CTX *tmp_ctx = talloc_new(la); struct ldb_module *module = ar->module; - TALLOC_CTX *tmp_ctx = talloc_new(ar); - struct replmd_private *replmd_private = - talloc_get_type(ldb_module_get_private(module), struct replmd_private); - - for (la = DLIST_TAIL(replmd_private->la_list); la; la = DLIST_PREV(la)) { - struct ldb_message *src_msg; - const struct dsdb_attribute *attr; - struct dsdb_dn *tgt_dsdb_dn; - enum deletion_state del_state = OBJECT_NOT_DELETED; - struct GUID guid = GUID_zero(); - - ret = replmd_extract_la_entry_details(module, la, tmp_ctx, &attr, - &src_msg, &tgt_dsdb_dn); + struct ldb_message *src_msg; + const struct dsdb_attribute *attr; + struct dsdb_dn *tgt_dsdb_dn; + enum deletion_state del_state = OBJECT_NOT_DELETED; + struct GUID guid = GUID_zero(); - /* - * When we fail to find the source object, the error code we pass - * back here is really important. It flags back to the callers to - * retry this request with DRSUAPI_DRS_GET_ANC. This case should - * never happen if we're replicating from a Samba DC, but it is - * needed to talk to a Windows DC - */ - if (ret == LDB_ERR_NO_SUCH_OBJECT) { - ret = replmd_replicated_request_werror(ar, WERR_DS_DRA_MISSING_PARENT); - } + ret = replmd_extract_la_entry_details(module, la, tmp_ctx, &attr, + &src_msg, &tgt_dsdb_dn); - if (ret != LDB_SUCCESS) { - break; - } + /* + * When we fail to find the source object, the error code we pass + * back here is really important. It flags back to the callers to + * retry this request with DRSUAPI_DRS_GET_ANC. This case should + * never happen if we're replicating from a Samba DC, but it is + * needed to talk to a Windows DC + */ + if (ret == LDB_ERR_NO_SUCH_OBJECT) { + ret = replmd_replicated_request_werror(ar, WERR_DS_DRA_MISSING_PARENT); + } - ret = replmd_check_target_exists(module, tgt_dsdb_dn, la, - src_msg->dn, &guid, &del_state); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } - /* - * When we fail to find the target object, the error code we pass - * back here is really important. It flags back to the callers to - * retry this request with DRSUAPI_DRS_GET_TGT - */ - if (ret == LDB_ERR_NO_SUCH_OBJECT) { - ret = replmd_replicated_request_werror(ar, WERR_DS_DRA_RECYCLED_TARGET); - } + ret = replmd_check_target_exists(module, tgt_dsdb_dn, la, + src_msg->dn, &guid, &del_state); - if (ret != LDB_SUCCESS) { - break; - } + /* + * When we fail to find the target object, the error code we pass + * back here is really important. It flags back to the callers to + * retry this request with DRSUAPI_DRS_GET_TGT + */ + if (ret == LDB_ERR_NO_SUCH_OBJECT) { + ret = replmd_replicated_request_werror(ar, WERR_DS_DRA_RECYCLED_TARGET); } talloc_free(tmp_ctx); -- 1.9.1 From 1f2e8c942afbef12bb106b3c7a3ff56fae4e3c7f Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Wed, 19 Jul 2017 16:18:20 +1200 Subject: [PATCH 15/20] replmd: Avoid dropping links if link target is deleted The server-side can potentially send the linked attribute before the target-object. This happens on Microsoft, and will happen on Samba once server-side GET_TGT support is added. In these cases there is a hole where the Samba client can silently drop the linked attribute. If the old copy of the target object was deleted/recycled, then the client can receive the new linked attribute before it realizes the target has now been reincarnated. It silently ignores the linked attribute, thinking its receiving out of date information, when really it's the client's copy of the target object that's out of date. In this case we want to retry with the GET_TGT flag set, which will force the updated version of the target object to be sent along with the linked attribute. This deleted/recycled target case is the main reason that Windows added the GET_TGT flag. If the server sends all the links at the end, instead of along with the source object, then this case can still be hit. If so, it will cause the server to restart the replication from the beginning again. This is probably preferential to silently dropping links. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit ab12aed7e13a9c9663ba04b6dd4f02acb7bff380) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 52 ++++++++++++++++--------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 0a53d1d..8e2c64b 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -6713,14 +6713,16 @@ static bool replmd_objects_have_same_nc(struct ldb_context *ldb, /** * Checks that the target object for a linked attribute exists. - * If it exists, its GUID and deletion-state are returned + * @param guid returns the target object's GUID (is returned)if it exists) + * @param ignore_link set to true if the linked attribute should be ignored + * (i.e. the target doesn't exist, but that it's OK to skip the link) */ static int replmd_check_target_exists(struct ldb_module *module, struct dsdb_dn *dsdb_dn, struct la_entry *la_entry, struct ldb_dn *source_dn, struct GUID *guid, - enum deletion_state *target_deletion_state) + bool *ignore_link) { struct drsuapi_DsReplicaLinkedAttribute *la = la_entry->la; struct ldb_context *ldb = ldb_module_get_ctx(module); @@ -6729,10 +6731,10 @@ static int replmd_check_target_exists(struct ldb_module *module, const char *attrs[] = { "isDeleted", "isRecycled", NULL }; NTSTATUS ntstatus; int ret; + enum deletion_state target_deletion_state = OBJECT_REMOVED; bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE) ? true : false; - *target_deletion_state = OBJECT_REMOVED; - + *ignore_link = false; ntstatus = dsdb_get_extended_dn_guid(dsdb_dn->dn, guid, "GUID"); if (!NT_STATUS_IS_OK(ntstatus) && !active) { @@ -6808,6 +6810,7 @@ static int replmd_check_target_exists(struct ldb_module *module, ": Failed to find target %s linked from %s\n", ldb_dn_get_linearized(dsdb_dn->dn), ldb_dn_get_linearized(source_dn))); + *ignore_link = true; } else if (replmd_objects_have_same_nc(ldb, tmp_ctx, source_dn, dsdb_dn->dn)) { @@ -6827,6 +6830,7 @@ static int replmd_check_target_exists(struct ldb_module *module, DEBUG(2,("Failed to resolve cross-partition link between %s and %s\n", ldb_dn_get_linearized(source_dn), ldb_dn_get_linearized(dsdb_dn->dn))); + *ignore_link = true; } } else if (target_res->count != 1) { ldb_asprintf_errstring(ldb, "More than one object found matching objectGUID %s\n", @@ -6839,7 +6843,23 @@ static int replmd_check_target_exists(struct ldb_module *module, /* Get the object's state (i.e. Not Deleted, Tombstone, etc) */ replmd_deletion_state(module, target_msg, - target_deletion_state, NULL); + &target_deletion_state, NULL); + + /* + * Check for deleted objects as per MS-DRSR 4.1.10.6.14 + * ProcessLinkValue(). Link updates should not be sent for + * recycled and tombstone objects (deleting the links should + * happen when we delete the object). This probably means our + * copy of the target object isn't up to date. + */ + if (target_deletion_state >= OBJECT_RECYCLED) { + ldb_asprintf_errstring(ldb, + "Deleted target %s GUID %s linked from %s\n", + ldb_dn_get_linearized(dsdb_dn->dn), + GUID_string(tmp_ctx, guid), + ldb_dn_get_linearized(source_dn)); + ret = LDB_ERR_NO_SUCH_OBJECT; + } } talloc_free(tmp_ctx); @@ -6972,8 +6992,8 @@ static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar, struct ldb_message *src_msg; const struct dsdb_attribute *attr; struct dsdb_dn *tgt_dsdb_dn; - enum deletion_state del_state = OBJECT_NOT_DELETED; struct GUID guid = GUID_zero(); + bool dummy; ret = replmd_extract_la_entry_details(module, la, tmp_ctx, &attr, &src_msg, &tgt_dsdb_dn); @@ -6995,7 +7015,7 @@ static int replmd_verify_linked_attribute(struct replmd_replicated_request *ar, } ret = replmd_check_target_exists(module, tgt_dsdb_dn, la, - src_msg->dn, &guid, &del_state); + src_msg->dn, &guid, &dummy); /* * When we fail to find the target object, the error code we pass @@ -7032,9 +7052,8 @@ static int replmd_process_linked_attribute(struct ldb_module *module, struct parsed_dn *pdn_list, *pdn, *next; struct GUID guid = GUID_zero(); bool active = (la->flags & DRSUAPI_DS_LINKED_ATTRIBUTE_FLAG_ACTIVE)?true:false; - + bool ignore_link; enum deletion_state deletion_state = OBJECT_NOT_DELETED; - enum deletion_state target_deletion_state = OBJECT_NOT_DELETED; /* * get the attribute being modified, the search result for the source object, @@ -7049,7 +7068,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module, } /* - * Check for deleted objects per MS-DRSR 4.1.10.6.13 + * Check for deleted objects per MS-DRSR 4.1.10.6.14 * 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 @@ -7084,7 +7103,7 @@ static int replmd_process_linked_attribute(struct ldb_module *module, } ret = replmd_check_target_exists(module, dsdb_dn, la_entry, msg->dn, - &guid, &target_deletion_state); + &guid, &ignore_link); if (ret != LDB_SUCCESS) { talloc_free(tmp_ctx); @@ -7092,15 +7111,12 @@ static int replmd_process_linked_attribute(struct ldb_module *module, } /* - * 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. + * there are some cases where the target object doesn't exist, but it's + * OK to ignore the linked attribute */ - if (target_deletion_state >= OBJECT_RECYCLED) { + if (ignore_link) { talloc_free(tmp_ctx); - return LDB_SUCCESS; + return ret; } /* see if this link already exists */ -- 1.9.1 From 57fd1716a4edd3a8e8c013d1f6764de1aa202c19 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 20 Jul 2017 11:14:27 +1200 Subject: [PATCH 16/20] replmd: Don't fail cycle if we get link for deleted object with GET_TGT We are going to end up supporting 2 different server schemes: A. the old/default behaviour of sending all the linked attributes last, at the end of the replication cycle. B. the new/Microsoft way of sending the linked attributes interleaved with the source/target objects. Normally if we're talking to a server using the old scheme-A, we won't ever use the GET_TGT flag. However, there are a couple of cases where it can happen: - A link to a new object was added during the replication cycle. - An object was deleted while the replication was in progress (and the linked attribute got queued before the object was deleted). Talking to an Samba DC running the old scheme will just cause it to start the replication cycle from scratch again, which is fairly harmless. However, there is a chance that the same thing can happen again, in which case the replication cycle will fail (because GET_TGT was already set). Even if we're using the new scheme (B), we could still potentially hit this case, as we can still queue up linked attributes between requests (group memberships can be larger than what can fit into a single replication chunk). If GET_TGT is set in the GetNcChanges request, then the local copy of the target object should always be up-to-date when we process the linked attribute. So if we still think the target object is deleted/recycled at this point, then it's safe to ignore the linked attribute (because we know our local copy is up-to-date). This logic matches the MS spec logic in ProcessLinkValue(). Not failing the replication cycle may be beneficial if we're trying to do a full-sync of a large database. Otherwise it might be time-consuming and frustrating to repeat the sync unnecessarily. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 89cf5c3f76e214fbd06ce461470eb64b338c5144) --- source4/dsdb/repl/drepl_out_helpers.c | 4 ++ source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 51 +++++++++++++++---------- source4/dsdb/samdb/samdb.h | 1 + source4/libnet/libnet_vampire.c | 4 ++ 4 files changed, 39 insertions(+), 21 deletions(-) diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c index 6e24e7f..6630804 100644 --- a/source4/dsdb/repl/drepl_out_helpers.c +++ b/source4/dsdb/repl/drepl_out_helpers.c @@ -868,6 +868,10 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req dsdb_repl_flags |= DSDB_REPL_FLAG_OBJECT_SUBSET; } + if (state->op->more_flags & DRSUAPI_DRS_GET_TGT) { + dsdb_repl_flags |= DSDB_REPL_FLAG_TARGETS_UPTODATE; + } + if (state->op->extended_op != DRSUAPI_EXOP_NONE) { ret = dsdb_find_nc_root(service->samdb, partition, partition->dn, &nc_root); diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 8e2c64b..228622c 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -74,7 +74,7 @@ struct replmd_private { struct la_entry { struct la_entry *next, *prev; struct drsuapi_DsReplicaLinkedAttribute *la; - bool incomplete_replica; + uint32_t dsdb_repl_flags; }; struct replmd_replicated_request { @@ -6052,14 +6052,12 @@ static int replmd_store_linked_attributes(struct replmd_replicated_request *ar) { int ret = LDB_SUCCESS; uint32_t i; - bool incomplete_subset; struct ldb_module *module = ar->module; struct replmd_private *replmd_private = talloc_get_type(ldb_module_get_private(module), struct replmd_private); struct ldb_context *ldb; ldb = ldb_module_get_ctx(module); - incomplete_subset = (ar->objs->dsdb_repl_flags & DSDB_REPL_FLAG_OBJECT_SUBSET); DEBUG(4,("linked_attributes_count=%u\n", ar->objs->linked_attributes_count)); @@ -6082,13 +6080,7 @@ static int replmd_store_linked_attributes(struct replmd_replicated_request *ar) return LDB_ERR_OPERATIONS_ERROR; } *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; + la_entry->dsdb_repl_flags = ar->objs->dsdb_repl_flags; /* we need to steal the non-scalars so they stay around until the end of the transaction */ @@ -6793,17 +6785,19 @@ static int replmd_check_target_exists(struct ldb_module *module, if (target_res->count == 0) { /* + * 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 + * * 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 (la_entry->dsdb_repl_flags & DSDB_REPL_FLAG_OBJECT_SUBSET) { /* - * 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 + * We don't increase the highwater-mark in the object + * subset cases, so subsequent replications should * resolve any missing links */ DEBUG(2,(__location__ @@ -6853,12 +6847,27 @@ static int replmd_check_target_exists(struct ldb_module *module, * copy of the target object isn't up to date. */ if (target_deletion_state >= OBJECT_RECYCLED) { - ldb_asprintf_errstring(ldb, - "Deleted target %s GUID %s linked from %s\n", - ldb_dn_get_linearized(dsdb_dn->dn), - GUID_string(tmp_ctx, guid), - ldb_dn_get_linearized(source_dn)); - ret = LDB_ERR_NO_SUCH_OBJECT; + + if (la_entry->dsdb_repl_flags & DSDB_REPL_FLAG_TARGETS_UPTODATE) { + + /* + * target should already be uptodate so there's no + * point retrying - it's probably just bad timing + */ + *ignore_link = true; + DEBUG(0, ("%s is deleted but up to date. " + "Ignoring link from %s\n", + ldb_dn_get_linearized(dsdb_dn->dn), + ldb_dn_get_linearized(source_dn))); + + } else { + ldb_asprintf_errstring(ldb, + "Deleted target %s GUID %s linked from %s", + ldb_dn_get_linearized(dsdb_dn->dn), + GUID_string(tmp_ctx, guid), + ldb_dn_get_linearized(source_dn)); + ret = LDB_ERR_NO_SUCH_OBJECT; + } } } diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h index 2abaf4a..01eb1f3 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -65,6 +65,7 @@ struct dsdb_control_current_partition { #define DSDB_REPL_FLAG_ADD_NCNAME 4 #define DSDB_REPL_FLAG_EXPECT_NO_SECRETS 8 #define DSDB_REPL_FLAG_OBJECT_SUBSET 16 +#define DSDB_REPL_FLAG_TARGETS_UPTODATE 32 #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 d89256b..aa0ec91 100644 --- a/source4/libnet/libnet_vampire.c +++ b/source4/libnet/libnet_vampire.c @@ -647,6 +647,10 @@ WERROR libnet_vampire_cb_store_chunk(void *private_data, is_exop = true; } req_replica_flags = c->req10->replica_flags; + + if (c->req10->more_flags & DRSUAPI_DRS_GET_TGT) { + dsdb_repl_flags |= DSDB_REPL_FLAG_TARGETS_UPTODATE; + } break; default: return WERR_INVALID_PARAMETER; -- 1.9.1 From d04b81f174736f0c619b119af88be03d74f84e58 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Mon, 24 Jul 2017 16:20:58 +1200 Subject: [PATCH 17/20] 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. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit fae5df891c11f642cbede9e4e3d845c49c5f86b8) --- source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 14 +++++++------- testprogs/blackbox/dbcheck.sh | 8 ++++++++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c index 228622c..d5234d5 100644 --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c @@ -374,8 +374,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; } @@ -6816,15 +6817,14 @@ static int replmd_check_target_exists(struct ldb_module *module, } else { /* - * TODO: - * We don't handle cross-partition links well here (we - * could potentially lose them), but don't fail the - * replication. + * 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. */ DEBUG(2,("Failed to resolve cross-partition link between %s and %s\n", ldb_dn_get_linearized(source_dn), ldb_dn_get_linearized(dsdb_dn->dn))); - *ignore_link = true; } } else if (target_res->count != 1) { ldb_asprintf_errstring(ldb, "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 -- 1.9.1 From 6a734d3ef71405c24c355467897ab6b26a1e2419 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 6 Jun 2017 18:06:22 +1200 Subject: [PATCH 18/20] getncchanges.py: Add a new test for replication This adds a new test to check that if objects are modified during a replication, then those objects don't wind up missing from the replication data. Note that when this scenario occurs, samba returns the objects in a different order to Windows. This test doesn't care what order the replicated objects get returned in, so long as they all have been received by the end of the test. As part of this, I've refactored _check_replication() in drs_base.py so it can be reused in new tests. In these cases, the objects are split up over multiple different chunks. So asserting that the objects are returned in a specific order makes it difficult to run the same test on both Samba and Windows. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 4cfc29688584ca69c43abb770d1e721d1eab1480) --- source4/selftest/tests.py | 10 ++ source4/torture/drs/python/drs_base.py | 70 ++++++++++---- source4/torture/drs/python/getncchanges.py | 147 +++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 19 deletions(-) create mode 100644 source4/torture/drs/python/getncchanges.py diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 4bcbdc6..a73ba60 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -853,6 +853,16 @@ for env in ['vampire_dc', 'promoted_dc', 'vampire_2000_dc']: environ={'DC1': "$DC_SERVER", 'DC2': '$%s_SERVER' % env.upper()}, extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) +# A side-effect of the getncchanges tests is that they will create hundreds of +# tombstone objects, so run them last to avoid interferring with (and slowing +# down) the other DRS tests +for env in ['vampire_dc', 'promoted_dc']: + 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 ['ad_dc_ntvfs']: planoldpythontestsuite(env, "repl_rodc", extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py index f19efca..f313abd 100644 --- a/source4/torture/drs/python/drs_base.py +++ b/source4/torture/drs/python/drs_base.py @@ -176,17 +176,15 @@ class DrsBaseTestCase(SambaToolCmdTest): id.dn = str(res[0].dn) return id - def _check_replication(self, expected_dns, replica_flags, expected_links=[], - drs_error=drsuapi.DRSUAPI_EXOP_ERR_NONE, drs=None, drs_handle=None, - highwatermark=None, uptodateness_vector=None, - more_flags=0, more_data=False, - dn_ordered=True, links_ordered=True, - max_objects=133, exop=0, - dest_dsa=drsuapi.DRSUAPI_DS_BIND_GUID_W2K3, - source_dsa=None, invocation_id=None, nc_dn_str=None, - nc_object_count=0, nc_linked_attributes_count=0): + def _get_replication(self, replica_flags, + drs_error=drsuapi.DRSUAPI_EXOP_ERR_NONE, drs=None, drs_handle=None, + highwatermark=None, uptodateness_vector=None, + more_flags=0, max_objects=133, exop=0, + dest_dsa=drsuapi.DRSUAPI_DS_BIND_GUID_W2K3, + source_dsa=None, invocation_id=None, nc_dn_str=None): """ - Makes sure that replication returns the specific error given. + Builds a DsGetNCChanges request based on the information provided + and returns the response received from the DC. """ if source_dsa is None: source_dsa = self.ldb_dc1.get_ntds_GUID() @@ -230,12 +228,51 @@ class DrsBaseTestCase(SambaToolCmdTest): self.assertEqual(level, 6, "expected level 6 response!") self.assertEqual(ctr.source_dsa_guid, misc.GUID(source_dsa)) self.assertEqual(ctr.source_dsa_invocation_id, misc.GUID(invocation_id)) - ctr6 = ctr - self.assertEqual(ctr6.extended_ret, drs_error) + self.assertEqual(ctr.extended_ret, drs_error) + + return ctr + + def _check_replication(self, expected_dns, replica_flags, expected_links=[], + drs_error=drsuapi.DRSUAPI_EXOP_ERR_NONE, drs=None, drs_handle=None, + highwatermark=None, uptodateness_vector=None, + more_flags=0, more_data=False, + dn_ordered=True, links_ordered=True, + max_objects=133, exop=0, + dest_dsa=drsuapi.DRSUAPI_DS_BIND_GUID_W2K3, + source_dsa=None, invocation_id=None, nc_dn_str=None, + nc_object_count=0, nc_linked_attributes_count=0): + """ + Makes sure that replication returns the specific error given. + """ + + # send a DsGetNCChanges to the DC + ctr6 = self._get_replication(replica_flags, + drs_error, drs, drs_handle, + highwatermark, uptodateness_vector, + more_flags, max_objects, exop, dest_dsa, + source_dsa, invocation_id, nc_dn_str) + + # check the response is what we expect self._check_ctr6(ctr6, expected_dns, expected_links, - nc_object_count=nc_object_count) + nc_object_count=nc_object_count, more_data=more_data, + dn_ordered=dn_ordered) return (ctr6.new_highwatermark, ctr6.uptodateness_vector) + + def _get_ctr6_dn_list(self, ctr6): + """ + Returns the DNs contained in a DsGetNCChanges response. + """ + dn_list = [] + next_object = ctr6.first_object + for i in range(0, ctr6.object_count): + dn_list.append(next_object.object.identifier.dn) + next_object = next_object.next_object + self.assertEqual(next_object, None) + + return dn_list + + def _check_ctr6(self, ctr6, expected_dns=[], expected_links=[], dn_ordered=True, links_ordered=True, more_data=False, nc_object_count=0, @@ -250,12 +287,7 @@ class DrsBaseTestCase(SambaToolCmdTest): self.assertEqual(ctr6.nc_linked_attributes_count, nc_linked_attributes_count) self.assertEqual(ctr6.drs_error[0], drs_error) - ctr6_dns = [] - next_object = ctr6.first_object - for i in range(0, ctr6.object_count): - ctr6_dns.append(next_object.object.identifier.dn) - next_object = next_object.next_object - self.assertEqual(next_object, None) + ctr6_dns = self._get_ctr6_dn_list(ctr6) i = 0 for dn in expected_dns: diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py new file mode 100644 index 0000000..d1d6b2b --- /dev/null +++ b/source4/torture/drs/python/getncchanges.py @@ -0,0 +1,147 @@ +#!/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 . +# + +# +# 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 + +from samba.dcerpc import drsuapi + +class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): + def setUp(self): + super(DrsReplicaSyncIntegrityTestCase, self).setUp() + self.base_dn = self.ldb_dc1.get_default_basedn() + self.ou = "OU=uptodateness_test,%s" % self.base_dn + self.ldb_dc1.add({ + "dn": self.ou, + "objectclass": "organizationalUnit"}) + (self.drs, self.drs_handle) = self._ds_bind(self.dnsname_dc1) + (self.default_hwm, self.default_utdv) = self._get_highest_hwm_utdv(self.ldb_dc1) + self._debug = True + + def tearDown(self): + super(DrsReplicaSyncIntegrityTestCase, self).tearDown() + # tidyup groups and users + try: + self.ldb_dc1.delete(self.ou, ["tree_delete:1"]) + except ldb.LdbError as (enum, string): + if enum == ldb.ERR_NO_SUCH_OBJECT: + pass + + def add_object(self, dn): + """Adds an OU object""" + self.ldb_dc1.add({"dn": dn, "objectclass": "organizationalunit"}) + res = self.ldb_dc1.search(base=dn, scope=SCOPE_BASE) + self.assertEquals(len(res), 1) + + 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.ldb_dc1, dn) + m[attr] = ldb.MessageElement(value, ldb.FLAG_MOD_ADD, attr) + self.ldb_dc1.modify(m) + + def create_object_range(self, start, end, prefix=""): + """ + Creates a block of objects. Object names are numbered sequentially, + using the optional prefix supplied. + """ + dn_list = [] + + # Create the parents first, then the children. + # This makes it easier to see in debug when GET_ANC takes effect + # because the parent/children become interleaved (by default, + # this approach means the objects are organized into blocks of + # parents and blocks of children together) + for x in range(start, end): + ou = "OU=test_ou_%s%d,%s" % (prefix, x, self.ou) + self.add_object(ou) + dn_list.append(ou) + + return dn_list + + def assert_expected_data(self, received_list, expected_list): + """ + Asserts that we received all the DNs that we expected and + none are missing. + """ + + # Note that with GET_ANC Windows can end up sending the same parent + # object multiple times, so this might be noteworthy but doesn't + # warrant failing the test + if (len(received_list) != len(expected_list)): + print("Note: received %d objects but expected %d" %(len(received_list), + len(expected_list))) + + # Check that we received every object that we were expecting + for dn in expected_list: + self.assertTrue(dn in received_list, "DN '%s' missing from replication." % dn) + + def test_repl_integrity(self): + """ + Modify the objects being replicated while the replication is still + in progress and check that no object loss occurs. + """ + + # The server behaviour differs between samba and Windows. Samba returns + # the objects in the original order (up to the pre-modify HWM). Windows + # incorporates the modified objects and returns them in the new order + # (i.e. modified objects last), up to the post-modify HWM. The Microsoft + # docs state the Windows behaviour is optional. + + # Create a range of objects to replicate. + expected_dn_list = self.create_object_range(0, 400) + (orig_hwm, unused) = self._get_highest_hwm_utdv(self.ldb_dc1) + + # We ask for the first page of 100 objects. + # For this test, we don't care what order we receive the objects in, + # so long as by the end we've received everything + rxd_dn_list = [] + ctr6 = self._get_replication(drsuapi.DRSUAPI_DRS_WRIT_REP, max_objects=100) + rxd_dn_list = self._get_ctr6_dn_list(ctr6) + + # Modify some of the second page of objects. This should bump the highwatermark + for x in range(100, 200): + self.modify_object(expected_dn_list[x], "displayName", "OU%d" % x) + + (post_modify_hwm, unused) = self._get_highest_hwm_utdv(self.ldb_dc1) + self.assertTrue(post_modify_hwm.highest_usn > orig_hwm.highest_usn) + + # Get the remaining blocks of data + while ctr6.more_data: + ctr6 = self._get_replication(drsuapi.DRSUAPI_DRS_WRIT_REP, max_objects=100, + highwatermark=ctr6.new_highwatermark, + uptodateness_vector=ctr6.uptodateness_vector) + rxd_dn_list += self._get_ctr6_dn_list(ctr6) + + # Check we still receive all the objects we're expecting + self.assert_expected_data(rxd_dn_list, expected_dn_list) + + -- 1.9.1 From 7f40472f94ad85176219d01898c1272c6c15ae09 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 6 Jun 2017 18:21:40 +1200 Subject: [PATCH 19/20] getncchanges.py: Add GET_ANC replication test case This test: - creates blocks of parent/child objects - modifies the parents, so the child gets received first in the replication (which means the client has to use GET_ANC) - checks that we always receive the parent before the child (if not, it either retries with GET_ANC, or asserts if GET_ANC is already set) - modifies the parent objects to change their USN while the replication is in progress - checks that all expected objects are received by the end of the test I've added a repl_get_next() function to help simulate a client's behaviour - if it encounters an object it doesn't know the parent of, then it retries with GET_ANC. Also added some debug to drs_base.py that developers can turn on to make it easier to see what objects we're actually receiving in the responses. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 9f0ae6e44d0f6def6be7c44067c7fcfdf0d42db2) --- source4/torture/drs/python/drs_base.py | 25 ++++ source4/torture/drs/python/getncchanges.py | 195 +++++++++++++++++++++++++++-- 2 files changed, 212 insertions(+), 8 deletions(-) diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py index f313abd..9ae1f0f 100644 --- a/source4/torture/drs/python/drs_base.py +++ b/source4/torture/drs/python/drs_base.py @@ -68,6 +68,9 @@ class DrsBaseTestCase(SambaToolCmdTest): self.dnsname_dc1 = self.info_dc1["dnsHostName"][0] self.dnsname_dc2 = self.info_dc2["dnsHostName"][0] + # for debugging the test code + self._debug = False + def tearDown(self): super(DrsBaseTestCase, self).tearDown() @@ -176,6 +179,27 @@ class DrsBaseTestCase(SambaToolCmdTest): id.dn = str(res[0].dn) return id + def _ctr6_debug(self, ctr6): + """ + Displays basic info contained in a DsGetNCChanges response. + Having this debug code allows us to see the difference in behaviour + between Samba and Windows easier. Turn on the self._debug flag to see it. + """ + + if self._debug: + print("------------ recvd CTR6 -------------") + + next_object = ctr6.first_object + for i in range(0, ctr6.object_count): + print("Obj %d: %s %s" %(i, next_object.object.identifier.dn[:22], + next_object.object.identifier.guid)) + next_object = next_object.next_object + + print("Linked Attributes: %d" % ctr6.linked_attributes_count) + print("HWM: %d" %(ctr6.new_highwatermark.highest_usn)) + print("Tmp HWM: %d" %(ctr6.new_highwatermark.tmp_highest_usn)) + print("More data: %d" %(ctr6.more_data)) + def _get_replication(self, replica_flags, drs_error=drsuapi.DRSUAPI_EXOP_ERR_NONE, drs=None, drs_handle=None, highwatermark=None, uptodateness_vector=None, @@ -224,6 +248,7 @@ class DrsBaseTestCase(SambaToolCmdTest): uptodateness_vector_v1.cursors = cursors req10.uptodateness_vector = uptodateness_vector_v1 (level, ctr) = drs.DsGetNCChanges(drs_handle, 10, req10) + self._ctr6_debug(ctr) self.assertEqual(level, 6, "expected level 6 response!") self.assertEqual(ctr.source_dsa_guid, misc.GUID(source_dsa)) diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py index d1d6b2b..2f914d8 100644 --- a/source4/torture/drs/python/getncchanges.py +++ b/source4/torture/drs/python/getncchanges.py @@ -44,7 +44,14 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): "objectclass": "organizationalUnit"}) (self.drs, self.drs_handle) = self._ds_bind(self.dnsname_dc1) (self.default_hwm, self.default_utdv) = self._get_highest_hwm_utdv(self.ldb_dc1) - self._debug = True + + # 100 is the minimum max_objects that Microsoft seems to honour + # (the max honoured is 400ish), so we use that in these tests + self.max_objects = 100 + self.last_ctr = None + + # store whether we used GET_ANC flags in the requests + self.used_get_anc = False def tearDown(self): super(DrsReplicaSyncIntegrityTestCase, self).tearDown() @@ -68,13 +75,23 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): m[attr] = ldb.MessageElement(value, ldb.FLAG_MOD_ADD, attr) self.ldb_dc1.modify(m) - def create_object_range(self, start, end, prefix=""): + def create_object_range(self, start, end, prefix="", + children=None, parent_list=None): """ Creates a block of objects. Object names are numbered sequentially, - using the optional prefix supplied. + using the optional prefix supplied. If the children parameter is + supplied it will create a parent-child hierarchy and return the + top-level parents separately. """ dn_list = [] + # Use dummy/empty lists if we're not creating a parent/child hierarchy + if children is None: + children = [] + + if parent_list is None: + parent_list = [] + # Create the parents first, then the children. # This makes it easier to see in debug when GET_ANC takes effect # because the parent/children become interleaved (by default, @@ -85,6 +102,16 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): self.add_object(ou) dn_list.append(ou) + # keep track of the top-level parents (if needed) + parent_list.append(ou) + + # create the block of children (if needed) + for x in range(start, end): + for child in children: + ou = "OU=test_ou_child%s%d,%s" % (child, x, parent_list[x]) + self.add_object(ou) + dn_list.append(ou) + return dn_list def assert_expected_data(self, received_list, expected_list): @@ -124,7 +151,7 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # For this test, we don't care what order we receive the objects in, # so long as by the end we've received everything rxd_dn_list = [] - ctr6 = self._get_replication(drsuapi.DRSUAPI_DRS_WRIT_REP, max_objects=100) + ctr6 = self.repl_get_next(rxd_dn_list) rxd_dn_list = self._get_ctr6_dn_list(ctr6) # Modify some of the second page of objects. This should bump the highwatermark @@ -135,13 +162,165 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): self.assertTrue(post_modify_hwm.highest_usn > orig_hwm.highest_usn) # Get the remaining blocks of data - while ctr6.more_data: - ctr6 = self._get_replication(drsuapi.DRSUAPI_DRS_WRIT_REP, max_objects=100, - highwatermark=ctr6.new_highwatermark, - uptodateness_vector=ctr6.uptodateness_vector) + while not self.replication_complete(): + ctr6 = self.repl_get_next(rxd_dn_list) rxd_dn_list += self._get_ctr6_dn_list(ctr6) # Check we still receive all the objects we're expecting self.assert_expected_data(rxd_dn_list, expected_dn_list) + def is_parent_known(self, dn, known_dn_list): + """ + Returns True if the parent of the dn specified is in known_dn_list + """ + + # we can sometimes get system objects like the RID Manager returned. + # Ignore anything that is not under the test OU we created + if self.ou not in dn: + return True + + # Remove the child portion from the name to get the parent's DN + name_substrings = dn.split(",") + del name_substrings[0] + + parent_dn = ",".join(name_substrings) + + # check either this object is a parent (it's parent is the top-level + # test object), or its parent has been seen previously + return parent_dn == self.ou or parent_dn in known_dn_list + + def repl_get_next(self, initial_objects, get_anc=False): + """ + Requests the next block of replication data. This tries to simulate + client behaviour - if we receive a replicated object that we don't know + the parent of, then re-request the block with the GET_ANC flag set. + """ + + # we're just trying to mimic regular client behaviour here, so just + # use the highwatermark in the last response we received + if self.last_ctr: + highwatermark = self.last_ctr.new_highwatermark + uptodateness_vector = self.last_ctr.uptodateness_vector + else: + # this is the initial replication, so we're starting from the start + highwatermark = None + uptodateness_vector = None + + # we'll add new objects as we discover them, so take a copy to modify + known_objects = initial_objects[:] + + # Ask for the next block of replication data + replica_flags = drsuapi.DRSUAPI_DRS_WRIT_REP + + if get_anc: + replica_flags = drsuapi.DRSUAPI_DRS_WRIT_REP | drsuapi.DRSUAPI_DRS_GET_ANC + self.used_get_anc = True + + ctr6 = self._get_replication(replica_flags, + max_objects=self.max_objects, + highwatermark=highwatermark, + uptodateness_vector=uptodateness_vector) + + # check that we know the parent for every object received + rxd_dn_list = self._get_ctr6_dn_list(ctr6) + + for i in range(0, len(rxd_dn_list)): + + dn = rxd_dn_list[i] + + if self.is_parent_known(dn, known_objects): + + # the new DN is now known so add it to the list. + # It may be the parent of another child in this block + known_objects.append(dn) + else: + # If we've already set the GET_ANC flag then it should mean + # we receive the parents before the child + self.assertFalse(get_anc, "Unknown parent for object %s" % dn) + + print("Unknown parent for %s - try GET_ANC" % dn) + + # try the same thing again with the GET_ANC flag set this time + return self.repl_get_next(get_anc=True) + + # store the last successful result so we know what HWM to request next + self.last_ctr = ctr6 + + return ctr6 + + def replication_complete(self): + """Returns True if the current/last replication cycle is complete""" + + if self.last_ctr is None or self.last_ctr.more_data: + return False + else: + return True + + def test_repl_integrity_get_anc(self): + """ + Modify the parent objects being replicated while the replication is still + in progress (using GET_ANC) and check that no object loss occurs. + """ + + # Note that GET_ANC behaviour varies between Windows and Samba. + # On Samba GET_ANC results in the replication restarting from the very + # beginning. After that, Samba remembers GET_ANC and also sends the + # parents in subsequent requests (regardless of whether GET_ANC is + # specified in the later request). + # Windows only sends the parents if GET_ANC was specified in the last + # request. It will also resend a parent, even if it's already sent the + # parent in a previous response (whereas Samba doesn't). + + # Create a small block of 50 parents, each with 2 children (A and B) + # This is so that we receive some children in the first block, so we + # can resend with GET_ANC before we learn too many parents + parent_dn_list = [] + expected_dn_list = self.create_object_range(0, 50, prefix="parent", + children=("A", "B"), + parent_list=parent_dn_list) + + # create the remaining parents and children + expected_dn_list += self.create_object_range(50, 150, prefix="parent", + children=("A", "B"), + parent_list=parent_dn_list) + + # We've now got objects in the following order: + # [50 parents][100 children][100 parents][200 children] + + # Modify the first parent so that it's now ordered last by USN + # This means we set the GET_ANC flag pretty much straight away + # because we receive the first child before the first parent + self.modify_object(parent_dn_list[0], "displayName", "OU0") + + # modify a later block of parents so they also get reordered + for x in range(50, 100): + self.modify_object(parent_dn_list[x], "displayName", "OU%d" % x) + + # Get the first block of objects - this should resend the request with + # GET_ANC set because we won't know about the first child's parent. + # On samba GET_ANC essentially starts the sync from scratch again, so + # we get this over with early before we learn too many parents + rxd_dn_list = [] + ctr6 = self.repl_get_next(rxd_dn_list) + rxd_dn_list = self._get_ctr6_dn_list(ctr6) + + # modify the last chunk of parents. They should now have a USN higher + # than the highwater-mark for the replication cycle + for x in range(100, 150): + self.modify_object(parent_dn_list[x], "displayName", "OU%d" % x) + + # Get the remaining blocks of data - this will resend the request with + # GET_ANC if it encounters an object it doesn't have the parent for. + while not self.replication_complete(): + ctr6 = self.repl_get_next(rxd_dn_list) + rxd_dn_list += self._get_ctr6_dn_list(ctr6) + + # The way the test objects have been created should force + # self.repl_get_next() to use the GET_ANC flag. If this doesn't + # actually happen, then the test isn't doing its job properly + self.assertTrue(self.used_get_anc, + "Test didn't use the GET_ANC flag as expected") + + # Check we get all the objects we're expecting + self.assert_expected_data(rxd_dn_list, expected_dn_list) -- 1.9.1 From aeaf9f653f51091c43eb3374240e8fd1d924d443 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Wed, 12 Jul 2017 10:16:00 +1200 Subject: [PATCH 20/20] getncchanges.py: Add test for GET_ANC and linked attributes Add a basic test that when we use GET_ANC and the parents have linked attributes, then we receive all the expected links and all the expected objects by the end of the test. This extends the test code to track what linked attributes get received and check whether they match what's present on the DC. Also made some minor cleanups to store the received objects/links each time we successfully receive a GETNCChanges response (this saves the test case having to repeat this code every time). Note that although this test involves linked attributes, it shouldn't exercise the GET_TGT case at all. Signed-off-by: Tim Beale Reviewed-by: Garming Sam Reviewed-by: Andrew Bartlett BUG: https://bugzilla.samba.org/show_bug.cgi?id=12972 (cherry picked from commit 0bb4d28272914736a46dc5cdb0bef766ba7a4ab8) --- source4/torture/drs/python/drs_base.py | 46 ++++++--- source4/torture/drs/python/getncchanges.py | 150 ++++++++++++++++++++++++----- 2 files changed, 157 insertions(+), 39 deletions(-) diff --git a/source4/torture/drs/python/drs_base.py b/source4/torture/drs/python/drs_base.py index 9ae1f0f..6083e43 100644 --- a/source4/torture/drs/python/drs_base.py +++ b/source4/torture/drs/python/drs_base.py @@ -179,6 +179,27 @@ class DrsBaseTestCase(SambaToolCmdTest): id.dn = str(res[0].dn) return id + def _get_ctr6_links(self, ctr6): + """ + Unpacks the linked attributes from a DsGetNCChanges response + and returns them as a list. + """ + ctr6_links = [] + for lidx in range(0, ctr6.linked_attributes_count): + l = ctr6.linked_attributes[lidx] + try: + target = ndr_unpack(drsuapi.DsReplicaObjectIdentifier3, + l.value.blob) + except: + target = ndr_unpack(drsuapi.DsReplicaObjectIdentifier3Binary, + l.value.blob) + al = AbstractLink(l.attid, l.flags, + l.identifier.guid, + target.guid, target.dn) + ctr6_links.append(al) + + return ctr6_links + def _ctr6_debug(self, ctr6): """ Displays basic info contained in a DsGetNCChanges response. @@ -196,6 +217,11 @@ class DrsBaseTestCase(SambaToolCmdTest): next_object = next_object.next_object print("Linked Attributes: %d" % ctr6.linked_attributes_count) + ctr6_links = self._get_ctr6_links(ctr6) + for link in ctr6_links: + print("Link Tgt %s... <-- Src %s" + %(link.targetDN[:22], link.identifier)) + print("HWM: %d" %(ctr6.new_highwatermark.highest_usn)) print("Tmp HWM: %d" %(ctr6.new_highwatermark.tmp_highest_usn)) print("More data: %d" %(ctr6.more_data)) @@ -325,21 +351,9 @@ class DrsBaseTestCase(SambaToolCmdTest): else: self.assertTrue(dn in ctr6_dns, "Couldn't find DN '%s' anywhere in ctr6 response." % dn) - ctr6_links = [] + # Extract the links from the response + ctr6_links = self._get_ctr6_links(ctr6) expected_links.sort() - lidx = 0 - for lidx in range(0, ctr6.linked_attributes_count): - l = ctr6.linked_attributes[lidx] - try: - target = ndr_unpack(drsuapi.DsReplicaObjectIdentifier3, - l.value.blob) - except: - target = ndr_unpack(drsuapi.DsReplicaObjectIdentifier3Binary, - l.value.blob) - al = AbstractLink(l.attid, l.flags, - l.identifier.guid, - target.guid) - ctr6_links.append(al) lidx = 0 for el in expected_links: @@ -420,13 +434,15 @@ class DrsBaseTestCase(SambaToolCmdTest): class AbstractLink: - def __init__(self, attid, flags, identifier, targetGUID): + def __init__(self, attid, flags, identifier, targetGUID, + targetDN=""): self.attid = attid self.flags = flags self.identifier = str(identifier) self.selfGUID_blob = ndr_pack(identifier) self.targetGUID = str(targetGUID) self.targetGUID_blob = ndr_pack(targetGUID) + self.targetDN = targetDN def __repr__(self): return "AbstractLink(0x%08x, 0x%08x, %s, %s)" % ( diff --git a/source4/torture/drs/python/getncchanges.py b/source4/torture/drs/python/getncchanges.py index 2f914d8..7d48133 100644 --- a/source4/torture/drs/python/getncchanges.py +++ b/source4/torture/drs/python/getncchanges.py @@ -45,6 +45,9 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): (self.drs, self.drs_handle) = self._ds_bind(self.dnsname_dc1) (self.default_hwm, self.default_utdv) = self._get_highest_hwm_utdv(self.ldb_dc1) + self.rxd_dn_list = [] + self.rxd_links = [] + # 100 is the minimum max_objects that Microsoft seems to honour # (the max honoured is 400ish), so we use that in these tests self.max_objects = 100 @@ -114,11 +117,12 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): return dn_list - def assert_expected_data(self, received_list, expected_list): + def assert_expected_data(self, expected_list): """ Asserts that we received all the DNs that we expected and none are missing. """ + received_list = self.rxd_dn_list # Note that with GET_ANC Windows can end up sending the same parent # object multiple times, so this might be noteworthy but doesn't @@ -150,9 +154,7 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # We ask for the first page of 100 objects. # For this test, we don't care what order we receive the objects in, # so long as by the end we've received everything - rxd_dn_list = [] - ctr6 = self.repl_get_next(rxd_dn_list) - rxd_dn_list = self._get_ctr6_dn_list(ctr6) + self.repl_get_next() # Modify some of the second page of objects. This should bump the highwatermark for x in range(100, 200): @@ -163,11 +165,10 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # Get the remaining blocks of data while not self.replication_complete(): - ctr6 = self.repl_get_next(rxd_dn_list) - rxd_dn_list += self._get_ctr6_dn_list(ctr6) + self.repl_get_next() # Check we still receive all the objects we're expecting - self.assert_expected_data(rxd_dn_list, expected_dn_list) + self.assert_expected_data(expected_dn_list) def is_parent_known(self, dn, known_dn_list): """ @@ -189,12 +190,8 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # test object), or its parent has been seen previously return parent_dn == self.ou or parent_dn in known_dn_list - def repl_get_next(self, initial_objects, get_anc=False): - """ - Requests the next block of replication data. This tries to simulate - client behaviour - if we receive a replicated object that we don't know - the parent of, then re-request the block with the GET_ANC flag set. - """ + def _repl_send_request(self, get_anc=False): + """Sends a GetNCChanges request for the next block of replication data.""" # we're just trying to mimic regular client behaviour here, so just # use the highwatermark in the last response we received @@ -202,13 +199,10 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): highwatermark = self.last_ctr.new_highwatermark uptodateness_vector = self.last_ctr.uptodateness_vector else: - # this is the initial replication, so we're starting from the start + # this is the first replication chunk highwatermark = None uptodateness_vector = None - # we'll add new objects as we discover them, so take a copy to modify - known_objects = initial_objects[:] - # Ask for the next block of replication data replica_flags = drsuapi.DRSUAPI_DRS_WRIT_REP @@ -216,14 +210,30 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): replica_flags = drsuapi.DRSUAPI_DRS_WRIT_REP | drsuapi.DRSUAPI_DRS_GET_ANC self.used_get_anc = True - ctr6 = self._get_replication(replica_flags, + # return the response from the DC + return self._get_replication(replica_flags, max_objects=self.max_objects, highwatermark=highwatermark, uptodateness_vector=uptodateness_vector) + def repl_get_next(self, get_anc=False): + """ + Requests the next block of replication data. This tries to simulate + client behaviour - if we receive a replicated object that we don't know + the parent of, then re-request the block with the GET_ANC flag set. + """ + + # send a request to the DC and get the response + ctr6 = self._repl_send_request(get_anc=get_anc) + # check that we know the parent for every object received rxd_dn_list = self._get_ctr6_dn_list(ctr6) + # we'll add new objects as we discover them, so take a copy of the + # ones we already know about, so we can modify the list safely + known_objects = self.rxd_dn_list[:] + + # check that we know the parent for every object received for i in range(0, len(rxd_dn_list)): dn = rxd_dn_list[i] @@ -246,6 +256,10 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # store the last successful result so we know what HWM to request next self.last_ctr = ctr6 + # store the objects and links we received + self.rxd_dn_list += self._get_ctr6_dn_list(ctr6) + self.rxd_links += self._get_ctr6_links(ctr6) + return ctr6 def replication_complete(self): @@ -300,9 +314,7 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # GET_ANC set because we won't know about the first child's parent. # On samba GET_ANC essentially starts the sync from scratch again, so # we get this over with early before we learn too many parents - rxd_dn_list = [] - ctr6 = self.repl_get_next(rxd_dn_list) - rxd_dn_list = self._get_ctr6_dn_list(ctr6) + self.repl_get_next() # modify the last chunk of parents. They should now have a USN higher # than the highwater-mark for the replication cycle @@ -312,8 +324,7 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): # Get the remaining blocks of data - this will resend the request with # GET_ANC if it encounters an object it doesn't have the parent for. while not self.replication_complete(): - ctr6 = self.repl_get_next(rxd_dn_list) - rxd_dn_list += self._get_ctr6_dn_list(ctr6) + self.repl_get_next() # The way the test objects have been created should force # self.repl_get_next() to use the GET_ANC flag. If this doesn't @@ -322,5 +333,96 @@ class DrsReplicaSyncIntegrityTestCase(drs_base.DrsBaseTestCase): "Test didn't use the GET_ANC flag as expected") # Check we get all the objects we're expecting - self.assert_expected_data(rxd_dn_list, expected_dn_list) + self.assert_expected_data(expected_dn_list) + + def assert_expected_links(self, objects_with_links, link_attr="managedBy"): + """ + Asserts that a GetNCChanges response contains any expected links + for the objects it contains. + """ + received_links = self.rxd_links + + num_expected = len(objects_with_links) + + self.assertTrue(len(received_links) == num_expected, + "Received %d links but expected %d" + %(len(received_links), num_expected)) + + for dn in objects_with_links: + self.assert_object_has_link(dn, link_attr, received_links) + + def assert_object_has_link(self, dn, link_attr, received_links): + """ + Queries the object in the DB and asserts there is a link in the + GetNCChanges response that matches. + """ + + # Look up the link attribute in the DB + # The extended_dn option will dump the GUID info for the link + # attribute (as a hex blob) + res = self.ldb_dc1.search(ldb.Dn(self.ldb_dc1, dn), attrs=[link_attr], + controls=['extended_dn:1:0'], scope=ldb.SCOPE_BASE) + + # We didn't find the expected link attribute in the DB for the object. + # Something has gone wrong somewhere... + self.assertTrue(link_attr in res[0], "%s in DB doesn't have attribute %s" + %(dn, link_attr)) + + # find the received link in the list and assert that the target and + # source GUIDs match what's in the DB + for val in res[0][link_attr]: + # Work out the expected source and target GUIDs for the DB link + target_dn = ldb.Dn(self.ldb_dc1, val) + targetGUID_blob = target_dn.get_extended_component("GUID") + sourceGUID_blob = res[0].dn.get_extended_component("GUID") + + found = False + + for link in received_links: + if link.selfGUID_blob == sourceGUID_blob and \ + link.targetGUID_blob == targetGUID_blob: + + found = True + + if self._debug: + print("Link %s --> %s" %(dn[:25], link.targetDN[:25])) + break + + self.assertTrue(found, "Did not receive expected link for DN %s" % dn) + + def test_repl_get_anc_link_attr(self): + """ + A basic GET_ANC test where the parents have linked attributes + """ + + # Create a block of 100 parents and 100 children + parent_dn_list = [] + expected_dn_list = self.create_object_range(0, 100, prefix="parent", + children=("A"), + parent_list=parent_dn_list) + + # Add links from the parents to the children + for x in range(0, 100): + self.modify_object(parent_dn_list[x], "managedBy", expected_dn_list[x + 100]) + + # add some filler objects at the end. This allows us to easily see + # which chunk the links get sent in + expected_dn_list += self.create_object_range(0, 100, prefix="filler") + + # We've now got objects in the following order: + # [100 x children][100 x parents][100 x filler] + + # Get the replication data - because the block of children come first, + # this should retry the request with GET_ANC + while not self.replication_complete(): + self.repl_get_next() + + self.assertTrue(self.used_get_anc, + "Test didn't use the GET_ANC flag as expected") + + # Check we get all the objects we're expecting + self.assert_expected_data(expected_dn_list) + + # Check we received links for all the parents + self.assert_expected_links(parent_dn_list) -- 1.9.1