s4-rpc_server/drsupai: Avoid looping with Azure AD Connect by not incrementing temp_h...

We send the NC root first, as a special case for every chunk
that we send until the natural point where it belongs.

We do not bump the tmp_highest_usn in the highwatermark that
the client and server use (it is meant to be an opauqe cookie)
until the 'natural' point where the object appears, similar
to the cache for GET_ANC.

The issue is that without this, because the NC root was sorted
first in whatever chunk it appeared in but could have a 'high'
highwatermark, Azure AD Connect will send back the same
new_highwatermark->tmp_highest_usn, and due to a bug,
a zero reserved_usn, which makes Samba discard it.

The reserved_usn is now much less likely to ever be set because
the tmp_higest_usn is now always advancing.

RN: Avoid infinite loop in initial user sync with Azure AD Connect
 when synchronising a large Samba AD domain.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15401

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>
(cherry picked from commit 79ca6ef28a6f94965cb030c4a7da8c1b9db7150b)

Autobuild-User(v4-17-test): Jule Anger <janger@samba.org>
Autobuild-Date(v4-17-test): Mon Aug 21 08:42:32 UTC 2023 on sn-devel-184

diff --git a/selftest/knownfail.d/getncchanges b/selftest/knownfail.d/getncchanges
index e92c86cb306d3449cfeda9bff838e19f2abf7abc..bda9b31a1b15496c71f0f534ec8baf1529109f96 100644 (file)
--- a/selftest/knownfail.d/getncchanges
+++ b/selftest/knownfail.d/getncchanges
@@ -4,8 +4,5 @@ samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegri
 samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_chain\(promoted_dc\)
 samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_and_anc\(promoted_dc\)
 samba4.drs.getncchanges.python\(promoted_dc\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_get_tgt_multivalued_links\(promoted_dc\)
+# Samba chooses to always increment the USN for the NC root at the point where it would otherwise show up.
 samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_nc_change_only\(
-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first\(
-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_mid\(
-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_start_zero\(
-samba4.drs.getncchanges.python\(.*\).getncchanges.DrsReplicaSyncIntegrityTestCase.test_repl_nc_is_first_start_zero_nc_change\(

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 448217c03d672686263cc7982d9d507fab55c0e6..8864e79f10ea9df702f71e6af4d76777a4e59c78 100644 (file)
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -62,6 +62,7 @@ struct drsuapi_getncchanges_state {
        bool is_get_anc;
        bool broken_samba_4_5_get_anc_emulation;
        bool is_get_tgt;
+       bool send_nc_root_first;
        uint64_t min_usn;
        uint64_t max_usn;
        struct drsuapi_DsReplicaHighWaterMark last_hwm;
@@ -1038,18 +1039,6 @@ static int site_res_cmp_usn_order(struct drsuapi_changed_objects *m1,
                                  struct drsuapi_changed_objects *m2,
                                  struct drsuapi_getncchanges_state *getnc_state)
 {
-       int ret;
-
-       ret = ldb_dn_compare(getnc_state->ncRoot_dn, m1->dn);
-       if (ret == 0) {
-               return -1;
-       }
-
-       ret = ldb_dn_compare(getnc_state->ncRoot_dn, m2->dn);
-       if (ret == 0) {
-               return 1;
-       }
-
        if (m1->usn == m2->usn) {
                return ldb_dn_compare(m2->dn, m1->dn);
        }
@@ -2124,7 +2113,7 @@ static WERROR getncchanges_get_sorted_array(const struct drsuapi_DsReplicaLinked
  * @param new_objs if parents are added, this gets updated to point to a chain
  * of parent objects (with the parents first and the child last)
  */
-static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemEx *child_obj,
+static WERROR getncchanges_add_ancestors(const struct GUID *parent_object_guid,
                                         struct ldb_dn *child_dn,
                                         TALLOC_CTX *mem_ctx,
                                         struct ldb_context *sam_ctx,
@@ -2147,7 +2136,7 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
                                            DSDB_SECRET_ATTRIBUTES,
                                            NULL };
 
-       next_anc_guid = child_obj->parent_object_guid;
+       next_anc_guid = parent_object_guid;
 
        while (next_anc_guid != NULL) {
                struct drsuapi_DsReplicaObjectListItemEx *anc_obj = NULL;
@@ -2156,19 +2145,24 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
                struct ldb_dn *anc_dn = NULL;
 
                /*
-                * Don't send an object twice. (If we've sent the object, then
-                * we've also sent all its parents as well)
+                * For the GET_ANC case (but not the 'send NC root
+                * first' case), don't send an object twice.
+                *
+                * (If we've sent the object, then we've also sent all
+                * its parents as well)
                 */
-               werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
-                                                      next_anc_guid);
-               if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
-                       return WERR_OK;
-               }
-               if (W_ERROR_IS_OK(werr)) {
-                       return WERR_INTERNAL_ERROR;
-               }
-               if (!W_ERROR_EQUAL(werr, WERR_OBJECT_NOT_FOUND)) {
-                       return werr;
+               if (getnc_state->obj_cache) {
+                       werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
+                                                              next_anc_guid);
+                       if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
+                               return WERR_OK;
+                       }
+                       if (W_ERROR_IS_OK(werr)) {
+                               return WERR_INTERNAL_ERROR;
+                       }
+                       if (!W_ERROR_EQUAL(werr, WERR_OBJECT_NOT_FOUND)) {
+                               return werr;
+                       }
                }
 
                anc_obj = talloc_zero(mem_ctx,
@@ -2222,11 +2216,18 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
                /*
                 * Regardless of whether we actually use it or not,
                 * we add it to the cache so we don't look at it again
+                *
+                * The only time we are here without
+                * getnc_state->obj_cache is for the forced addition
+                * of the NC root to the start of the reply, this we
+                * want to add each and every call..
                 */
-               werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
-                                                   next_anc_guid);
-               if (!W_ERROR_IS_OK(werr)) {
-                       return werr;
+               if (getnc_state->obj_cache) {
+                       werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
+                                                           next_anc_guid);
+                       if (!W_ERROR_IS_OK(werr)) {
+                               return werr;
+                       }
                }
 
                /*
@@ -2355,7 +2356,8 @@ static WERROR getncchanges_get_obj_to_send(const struct ldb_message *msg,
         */
        if (getnc_state->is_get_anc
            && !getnc_state->broken_samba_4_5_get_anc_emulation) {
-               werr = getncchanges_add_ancestors(obj, msg->dn, mem_ctx,
+               werr = getncchanges_add_ancestors(obj->parent_object_guid,
+                                                 msg->dn, mem_ctx,
                                                  sam_ctx, getnc_state,
                                                  schema, session_key,
                                                  req10, local_pas,
@@ -3287,6 +3289,12 @@ allowed:
                        if (changes[i].usn > getnc_state->max_usn) {
                                getnc_state->max_usn = changes[i].usn;
                        }
+
+                       if (req10->extended_op == DRSUAPI_EXOP_NONE &&
+                           GUID_equal(&changes[i].guid, &getnc_state->ncRoot_guid))
+                       {
+                               getnc_state->send_nc_root_first = true;
+                       }
                }
 
                if (req10->extended_op == DRSUAPI_EXOP_NONE) {
@@ -3427,6 +3435,36 @@ allowed:
                               uint32_t_ptr_cmp);
        }
 
+       /*
+        * If we have the NC root in this replication, send it
+        * first regardless.  However, don't bump the USN now,
+        * treat it as if it was sent early due to GET_ANC
+        *
+        * This is triggered for each call, so every page of responses
+        * gets the NC root as the first object, up to the point where
+        * it naturally occurs in the replication.
+        */
+
+       if (getnc_state->send_nc_root_first) {
+               struct drsuapi_DsReplicaObjectListItemEx *new_objs = NULL;
+
+               werr = getncchanges_add_ancestors(&getnc_state->ncRoot_guid,
+                                                 NULL, mem_ctx,
+                                                 sam_ctx, getnc_state,
+                                                 schema, &session_key,
+                                                 req10, local_pas,
+                                                 machine_dn, &new_objs);
+
+               if (!W_ERROR_IS_OK(werr)) {
+                       return werr;
+               }
+
+               getncchanges_chunk_add_objects(repl_chunk, new_objs);
+
+               DEBUG(8,(__location__ ": replicating NC root %s\n",
+                        ldb_dn_get_linearized(getnc_state->ncRoot_dn)));
+       }
+
        /*
         * Check in case we're still processing the links from an object in the
         * previous chunk. We want to send the links (and any targets needed)
@@ -3465,6 +3503,23 @@ allowed:
                TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
                uint32_t old_la_index;
 
+               /*
+                * Once we get to the 'natural' place to send the NC
+                * root, stop sending it at the front of each reply
+                * and make sure to suppress sending it now
+                *
+                * We don't just 'continue' here as we must send links
+                * and unlike Windows we want to update the
+                * tmp_highest_usn
+                */
+
+               if (getnc_state->send_nc_root_first &&
+                   GUID_equal(&getnc_state->guids[i], &getnc_state->ncRoot_guid))
+               {
+                       getnc_state->send_nc_root_first = false;
+                       obj_already_sent = true;
+               }
+
                msg_dn = ldb_dn_new_fmt(tmp_ctx, sam_ctx, "<GUID=%s>",
                                        GUID_string(tmp_ctx, &getnc_state->guids[i]));
                W_ERROR_HAVE_NO_MEMORY(msg_dn);