[SSSD] [PATCH] AD: Failure to get flat name is not fatal

Jakub Hrozek jhrozek at redhat.com
Thu Sep 19 17:03:43 UTC 2013


On Thu, Sep 19, 2013 at 10:47:49AM +0200, Pavel Březina wrote:
> On 09/18/2013 07:26 PM, Jakub Hrozek wrote:
> >On Wed, Sep 18, 2013 at 04:42:38PM +0200, Pavel Březina wrote:
> >>On 09/17/2013 10:18 PM, Jakub Hrozek wrote:
> >>>On Fri, Sep 13, 2013 at 01:04:28PM +0200, Pavel Březina wrote:
> >>>>On 09/04/2013 08:16 AM, Jakub Hrozek wrote:
> >>>>>This patch depends on my other AD enumeration patches.
> >>>>>
> >>>>>https://fedorahosted.org/sssd/ticket/2067
> >>>>>
> >>>>>Some AD or AD-like servers do not contain the netlogon attribute in the
> >>>>>master domain name. Instead of failing completely, we should just abort
> >>>>>the master domain request and carry on. The only functionality we miss
> >>>>>would get getting users by domain flat name.
> >>>>
> >>>>Nack (minor)
> >>>>
> >>>
> >>>Hi,
> >>>
> >>>thank you for the review.
> >>>
> >>>>>    AD: Failure to get flat name is not fatal
> >>>>>
> >>>>>    https://fedorahosted.org/sssd/ticket/2067
> >>>>>
> >>>>>    Some AD or AD-like servers do not contain the netlogon attribute in the
> >>>>>    master domain name. Instead of failing completely, we should just abort
> >>>>>    the master domain request and carry on. The only functionality we miss
> >>>>>    would get getting users by domain flat name.
> >>>>             ^ be?
> >>>>
> >>>
> >>>Fixed.
> >>>
> >>>>
> >>>>>static void
> >>>>>ad_master_domain_netlogon_done(struct tevent_req *subreq)
> >>>>>{
> >>>>>    int ret;
> >>>>>    size_t reply_count;
> >>>>>    struct sysdb_attrs **reply = NULL;
> >>>>>
> >>>>>    struct tevent_req *req = tevent_req_callback_data(subreq,
> >>>>>                                                      struct tevent_req);
> >>>>>    struct ad_master_domain_state *state =
> >>>>>        tevent_req_data(req, struct ad_master_domain_state);
> >>>>>
> >>>>>    ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> >>>>>    talloc_zfree(subreq);
> >>>>>    if (ret != EOK) {
> >>>>>        DEBUG(SSSDBG_OP_FAILURE, ("sdap_get_generic_send request failed.\n"));
> >>>>>        tevent_req_error(req, ret);
> >>>>>        return;
> >>>>>    }
> >>>>>
> >>>>>    if (reply_count != 1) {
> >>>>>        if (reply_count == 0) {
> >>>>>            DEBUG(SSSDBG_MINOR_FAILURE, ("No netlogon data available. Flat name " \
> >>>>>                                         "might not be usable\n"));
> >>>>>        } else if (reply_count > 1) {
> >>>>>            DEBUG(SSSDBG_MINOR_FAILURE,
> >>>>>                  ("More than one netlogon info returned.\n"));
> >>>>>        }
> >>>>>        /* Not fatal. Just quit. */
> >>>>>        goto done;
> >>>>>    }
> >>>>
> >>>>Removing != 1 condition will make the code nicer.
> >>>
> >>>OK, I removed the condition.
> >>>
> >>>>
> >>>>>
> >>>>>    ret = netlogon_get_flat_name(state, reply[0], &state->flat);
> >>>>>    if (ret != EOK) {
> >>>>>        DEBUG(SSSDBG_MINOR_FAILURE, ("Could not get the flat name\n"));
> >>>>>        /* Not fatal. Just quit. */
> >>>>>        goto done;
> >>>>>    }
> >>>>>
> >>>>>    DEBUG(SSSDBG_TRACE_FUNC, ("Found flat name [%s].\n", state->flat));
> >>>>>done:
> >>>>>    tevent_req_done(req);
> >>>>>    return;
> >>>>>}
> >>>
> >>>New patches are attached.
> >>
> >>Hi,
> >>sorry, I should have nitpick this earlier. When you fail to obtain a
> >>flat name, logs contains:
> >>[ad_enumeration_master_done] (0x0400): Found flat name [(null)].
> >>
> >>We should print that we failed to get flat name but we will continue anyway.
> >
> >I don't think it will, we won't get to this line at all but rather jump
> 
> Yes it will, I copied the debug message from the logs :-)
> 
> This debug message is at three places:
> ad_domain_info.c:384 (area of your patch) this one is not printed
> ad_id.c:433
> ad_subdomains.c:374
> 
> The last two are written into logs since you return ret = EOK.

Ah, you're right, thanks. But since the information is available in the
domain info request and the debug messages are already printed there, I
simply removed the debug statements from ad_id.c and ad_subdomains.c

Thanks for the review, new patches are attached.
-------------- next part --------------
>From a0b3860e893d4f5ca670bbac3c5eaad3131dd441 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 4 Sep 2013 07:43:59 +0200
Subject: [PATCH] AD: Failure to get flat name is not fatal

https://fedorahosted.org/sssd/ticket/2067

Some AD or AD-like servers do not contain the netlogon attribute in the
master domain name. Instead of failing completely, we should just abort
the master domain request and carry on. The only functionality we miss
would be getting users by domain flat name.
---
 src/providers/ad/ad_domain_info.c | 148 ++++++++++++++++++++++----------------
 src/providers/ad/ad_id.c          |   3 -
 src/providers/ad/ad_subdomains.c  |   3 -
 3 files changed, 86 insertions(+), 68 deletions(-)

diff --git a/src/providers/ad/ad_domain_info.c b/src/providers/ad/ad_domain_info.c
index b0c8652c9009d761d8cadd0e8eae187b92ba50ff..eff2034d12261510ed7535dee7098a6e68f1f2c2 100644
--- a/src/providers/ad/ad_domain_info.c
+++ b/src/providers/ad/ad_domain_info.c
@@ -40,6 +40,79 @@
 
 #define MASTER_DOMAIN_SID_FILTER "objectclass=domain"
 
+static errno_t
+netlogon_get_flat_name(TALLOC_CTX *mem_ctx,
+                       struct sysdb_attrs *reply,
+                       char **_flat_name)
+{
+    errno_t ret;
+    struct ldb_message_element *el;
+    DATA_BLOB blob;
+    struct ndr_pull *ndr_pull = NULL;
+    enum ndr_err_code ndr_err;
+    struct netlogon_samlogon_response response;
+    const char *flat_name;
+
+    ret = sysdb_attrs_get_el(reply, AD_AT_NETLOGON, &el);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_el() failed\n"));
+        return ret;
+    }
+
+    if (el->num_values == 0) {
+        DEBUG(SSSDBG_OP_FAILURE, ("netlogon has no value\n"));
+        return ENOENT;
+    } else if (el->num_values > 1) {
+        DEBUG(SSSDBG_OP_FAILURE, ("More than one netlogon value?\n"));
+        return EIO;
+    }
+
+    blob.data = el->values[0].data;
+    blob.length = el->values[0].length;
+
+    ndr_pull = ndr_pull_init_blob(&blob, mem_ctx);
+    if (ndr_pull == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("ndr_pull_init_blob() failed.\n"));
+        return ENOMEM;
+    }
+
+    ndr_err = ndr_pull_netlogon_samlogon_response(ndr_pull, NDR_SCALARS,
+                                                  &response);
+    if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
+        DEBUG(SSSDBG_OP_FAILURE, ("ndr_pull_netlogon_samlogon_response() "
+                                  "failed [%d]\n", ndr_err));
+        ret = EBADMSG;
+        goto done;
+    }
+
+    if (!(response.ntver & NETLOGON_NT_VERSION_5EX)) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Wrong version returned [%x]\n",
+                                  response.ntver));
+        ret = EBADMSG;
+        goto done;
+    }
+
+    if (response.data.nt5_ex.domain_name != NULL &&
+        *response.data.nt5_ex.domain_name != '\0') {
+        flat_name = response.data.nt5_ex.domain_name;
+    } else {
+        DEBUG(SSSDBG_MINOR_FAILURE, ("No netlogon data available\n"));
+        ret = ENOENT;
+        goto done;
+    }
+
+    *_flat_name = talloc_strdup(mem_ctx, flat_name);
+    if (*_flat_name == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
+        ret = ENOMEM;
+        goto done;
+    }
+    ret = EOK;
+done:
+    talloc_free(ndr_pull);
+    return EOK;
+}
+
 struct ad_master_domain_state {
     struct tevent_context *ev;
     struct sdap_id_conn_ctx *conn;
@@ -238,11 +311,6 @@ ad_master_domain_netlogon_done(struct tevent_req *subreq)
     int ret;
     size_t reply_count;
     struct sysdb_attrs **reply = NULL;
-    struct ldb_message_element *el;
-    DATA_BLOB blob;
-    enum ndr_err_code ndr_err;
-    struct ndr_pull *ndr_pull = NULL;
-    struct netlogon_samlogon_response response;
 
     struct tevent_req *req = tevent_req_callback_data(subreq,
                                                       struct tevent_req);
@@ -253,78 +321,34 @@ ad_master_domain_netlogon_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE, ("sdap_get_generic_send request failed.\n"));
-        goto done;
+        tevent_req_error(req, ret);
+        return;
     }
 
+    /* Failure to get the flat name is not fatal. Just quit. */
     if (reply_count == 0) {
-        DEBUG(SSSDBG_TRACE_FUNC, ("No netlogon data available.\n"));
-        ret = ENOENT;
+        DEBUG(SSSDBG_MINOR_FAILURE, ("No netlogon data available. Flat name " \
+                                     "might not be usable\n"));
         goto done;
     } else if (reply_count > 1) {
-        DEBUG(SSSDBG_OP_FAILURE,
-              ("More than one netlogon info returned.\n"));
-        ret = EINVAL;
+        DEBUG(SSSDBG_MINOR_FAILURE,
+                ("More than one netlogon info returned.\n"));
         goto done;
     }
 
-    ret = sysdb_attrs_get_el(reply[0], AD_AT_NETLOGON, &el);
+    /* Exactly one flat name. Carry on */
+
+    ret = netlogon_get_flat_name(state, reply[0], &state->flat);
     if (ret != EOK) {
-        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_el() failed\n"));
+        DEBUG(SSSDBG_MINOR_FAILURE, ("Could not get the flat name\n"));
+        /* Not fatal. Just quit. */
         goto done;
     }
 
-    if (el->num_values == 0) {
-        DEBUG(SSSDBG_OP_FAILURE, ("netlogon has no value\n"));
-        ret = ENOENT;
-        goto done;
-    } else if (el->num_values > 1) {
-        DEBUG(SSSDBG_OP_FAILURE, ("More than one netlogon value?\n"));
-        ret = EIO;
-        goto done;
-    }
-
-    blob.data =  el->values[0].data;
-    blob.length = el->values[0].length;
-
-    ndr_pull = ndr_pull_init_blob(&blob, state);
-    if (ndr_pull == NULL) {
-        DEBUG(SSSDBG_OP_FAILURE, ("ndr_pull_init_blob() failed.\n"));
-        ret = ENOMEM;
-        goto done;
-    }
-
-    ndr_err = ndr_pull_netlogon_samlogon_response(ndr_pull, NDR_SCALARS,
-                                                  &response);
-    if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
-        DEBUG(SSSDBG_OP_FAILURE, ("ndr_pull_netlogon_samlogon_response() "
-                                  "failed [%d]\n", ndr_err));
-        ret = EBADMSG;
-        goto done;
-    }
-
-    if (!(response.ntver & NETLOGON_NT_VERSION_5EX)) {
-        DEBUG(SSSDBG_OP_FAILURE, ("Wrong version returned [%x]\n",
-                                  response.ntver));
-        ret = EBADMSG;
-        goto done;
-    }
-
-    if (response.data.nt5_ex.domain_name != NULL &&
-        *response.data.nt5_ex.domain_name != '\0') {
-        state->flat = talloc_strdup(state, response.data.nt5_ex.domain_name);
-        if (state->flat == NULL) {
-            DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
-            ret = ENOMEM;
-            goto done;
-        }
-    }
-
     DEBUG(SSSDBG_TRACE_FUNC, ("Found flat name [%s].\n", state->flat));
+done:
     tevent_req_done(req);
     return;
-
-done:
-    tevent_req_error(req, ret);
 }
 
 errno_t
diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
index 1d45440e197e79a60dbc038d2b0c0c8ffe823642..9ee639a75c386045d7e80d8a80b1237b1383417e 100644
--- a/src/providers/ad/ad_id.c
+++ b/src/providers/ad/ad_id.c
@@ -430,9 +430,6 @@ ad_enumeration_master_done(struct tevent_req *subreq)
         return;
     }
 
-    DEBUG(SSSDBG_TRACE_FUNC, ("Found flat name [%s].\n", flat_name));
-    DEBUG(SSSDBG_TRACE_FUNC, ("Found master SID [%s].\n", master_sid));
-
     ret = sysdb_master_domain_add_info(state->sdom->dom,
                                        flat_name, master_sid);
     if (ret != EOK) {
diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
index f15a013803f34ee36ac5ac70162f23525ab4be36..b95f4e46ad7f20382cbc07470e41ff56790f0263 100644
--- a/src/providers/ad/ad_subdomains.c
+++ b/src/providers/ad/ad_subdomains.c
@@ -371,9 +371,6 @@ static void ad_subdomains_master_dom_done(struct tevent_req *req)
         goto done;
     }
 
-    DEBUG(SSSDBG_TRACE_FUNC, ("Found flat name [%s].\n", ctx->flat_name));
-    DEBUG(SSSDBG_TRACE_FUNC, ("Found master SID [%s].\n", ctx->master_sid));
-
     ret = sysdb_master_domain_add_info(ctx->sd_ctx->be_ctx->domain,
                                        ctx->flat_name, ctx->master_sid);
     if (ret != EOK) {
-- 
1.8.3.1



More information about the sssd-devel mailing list