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

Jakub Hrozek jhrozek at redhat.com
Wed Sep 18 17:26:36 UTC 2013


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
to the done label using one of the previous if-s. But when reading the
function again, I found a bad indentation, so attached is a revised
patch.
-------------- next part --------------
>From 3a7ff3275dcb156ee7d08ab8efb734eb4b744f8a 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 ++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 62 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
-- 
1.8.3.1



More information about the sssd-devel mailing list