[SSSD] [PATCH] LDAP: Detect the presence of POSIX attributes

Jakub Hrozek jhrozek at redhat.com
Wed Feb 12 14:31:42 UTC 2014


On Wed, Feb 12, 2014 at 11:30:24AM +0100, Pavel Březina wrote:
> On 02/11/2014 09:33 PM, Jakub Hrozek wrote:
> >On Thu, Jan 30, 2014 at 11:17:06AM +0100, Sumit Bose wrote:
> >>On Wed, Jan 29, 2014 at 05:11:59PM +0100, Jakub Hrozek wrote:
> >>>On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
> >>>>On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
> >>>>>Hi,
> >>>>>
> >>>>>When the schema is set to AD and ID mapping is used, there is a one-time
> >>>>
> >>>>you wanted to say "when ID mapping is *not* used"
> >>>>
> >>>>>check ran when searching for users to detect the presence of POSIX attributes
> >>>>>in LDAP. If this check fails, the search fails as if no entry was found
> >>>>>and returns a special error code.
> >>>>>
> >>>>>If the AD identity lookup finds this error code, the GC is disabled for
> >>>>>the next search.
> >>>>>
> >>>>>The sdap_server_opts structure is filled every time a client connects to
> >>>>>a server so the posix check boolean is reset to false again on connecting
> >>>>>to the server.
> >>>>
> >>>>GC lookups should not be disabled permanently. Administrator can
> >>>>setup POSIX attribute propagation to GC any time and SSSD should
> >>>>notice it
> >>>>without restarting.
> >>>>
> >>>>So we should rather perform the check periodically and not only
> >>>>disable GC lookups, but also re-enable it if it succeeds.
> >>>
> >>>See the attached patch, I added a timeout of 3600 seconds for the next
> >>>detection. I don't think we want to have the timeout configurable but
> >>>maybe we want the timeout to be reset on changing the online status as
> >>>well?
> >>
> >>I think we do not have to reset it when changing the online status since
> >>having the POSIX IDs in the GC or not is a global feature of the forest
> >>and changes should be quite rare.
> >
> >After some discussion last week with Simo I decided to remove the timer
> >altogether. The decision to change AD schema is so rare it makes sense
> >to let admins restart a service on clients.
> >
> >>
> >>>
> >>>>
> >>>>>It might be better to move the check to where the rootDSE is retrieved,
> >>>>>but the check depends on several features that are not known to the code
> >>>>>that retrieves the rootDSE (or the connection code for example) such as what
> >>>>>the attribute mappings are or the authentication method that should be used.
> >>>>>
> >>>>>Currently this patch only runs the check when users are requested. It
> >>>>>would be trivial to add the same code (about 70 lines) to the group
> >>>>>request as well.
> >>
> >>I think it is not called when enumeration is the first task running. Can
> >>you add it here as well? And while you are on it it might be worth to
> >>add it to group lookups as well, just for completeness.
> >
> >Done. During the testing of the AD enumeration I found some other issues
> >that are on the list right now as separate patches.
> >
> >>
> >>Please find more comments inline.
> >>
> >>bye,
> >>Sumit
> >>
> >>>>>
> >>>>>Additionally, I wonder if the absence of POSIX attributes in GC should
> >>>>>be reported louder. Currently there is just MINOR_FAILURE.
> >>>>
> >>>>We have SSSDBG_IMPORTANT_INFO for such things.
> >>>
> >>>Done.
> >>>
> >>>>
> >>>>>We could go as far as report to syslog when a user or a group from
> >>>>>subdomains is requested and the GC was already disabled, but I wanted to
> >>>>>check with the other developers before implementing this.
> >>>>
> >>>>I haven't made up my mind on this question yet, but I'm more
> >>>>inclined to avoid using syslog for this situation. :-)
> >>>
> >>>Yes, me too.
> >>>
> >>>>
> >>>>>One additional question -- currently the AD ID provider always retries
> >>>>>in LDAP if search in GC didn't find the result. With the possibility of
> >>>>>detecting the POSIX attributes, do we want to remove this fallback if we
> >>>>>were able to run the detection and know that GC is available but doesn't
> >>>>>contain the POSIX attributes?
> >>>>>
> >>>>>I would say the fallback should stay, because there is still a chance
> >>>>>other required attribute will be missing, after all the user can
> >>>>>configure additional filter and this fallback only affects negative
> >>>>>searches.
> >>>>
> >>>>I'm not sure if I understand you. Isn't the whole point of this
> >>>>patch to actually avoid the fallback? I mean if we detect that GC
> >>>>doesn't have POSIX attributes we want to go directly to LDAP on next
> >>>>queries, don't we?
> >>>
> >>>I meant the other way. Currently even if we know that GC *has* POSIX
> >>>attributes and we don't find the user in GC, we still retry LDAP. I was
> >>>wondering whether we can remove this fallback if we know that POSIX
> >>>attributes are present. But I think we shouldn't remove the fallback for
> >>>the general case because admins can still configure a custom filter in
> >>>the search base and this custom filter can contain any attributes.
> >>
> >>> From c76a1058f1aea30b83f3dd133230a093b1de5b44 Mon Sep 17 00:00:00 2001
> >>>From: Jakub Hrozek <jhrozek at redhat.com>
> >>>Date: Mon, 16 Dec 2013 18:36:12 +0100
> >>>Subject: [PATCH] LDAP: Detect the presence of POSIX attributes
> >>>
> >>>When the schema is set to AD and ID mapping is used, there is a one-time
> >>
> >>                                                 ^ 'not' missing ?
> >
> >Right, fixed
> >
> >>
> >>>check ran when searching for users to detect the presence of POSIX
> >>>attributes in LDAP. If this check fails, the search fails as if no entry
> >>>was found and returns a special error code.
> >>>
> >>>The sdap_server_opts structure is filled every time a client connects to
> >>>a server so the posix check boolean is reset to false again on connecting
> >>>to the server.
> >>>
> >>>It might be better to move the check to where the rootDSE is retrieved,
> >>>but the check depends on several features that are not known to the code
> >>>that retrieves the rootDSE (or the connection code for example) such as what
> >>>the attribute mappings are or the authentication method that should be used.
> >>>---
> >>>  src/providers/ad/ad_common.h          |   3 +
> >>>  src/providers/ad/ad_id.c              |  45 ++++++++-
> >>>  src/providers/ad/ad_id.h              |   1 +
> >>>  src/providers/ipa/ipa_subdomains_id.c |   2 +-
> >>>  src/providers/ldap/ldap_id.c          |  79 ++++++++++++++-
> >>>  src/providers/ldap/sdap.h             |   1 +
> >>>  src/providers/ldap/sdap_async.c       | 185 ++++++++++++++++++++++++++++++++++
> >>>  src/providers/ldap/sdap_async.h       |   9 ++
> >>>  src/util/util_errors.c                |   1 +
> >>>  src/util/util_errors.h                |   1 +
> >>>  10 files changed, 320 insertions(+), 7 deletions(-)
> >>>
> >>>diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
> >>>index d370cef69124c127f41d7c4cbaa25713363e7752..d70d4656c1d1878a882f261e481a2507f8e58c55 100644
> >>>--- a/src/providers/ad/ad_common.h
> >>>+++ b/src/providers/ad/ad_common.h
> >>>@@ -69,6 +69,9 @@ struct ad_options {
> >>>      struct sdap_options *id;
> >>>      struct ad_id_ctx *id_ctx;
> >>>
> >>>+    /* POSIX detection */
> >>>+    time_t gc_check_timer;
> >>>+
> >>>      /* Auth and chpass Provider */
> >>>      struct krb5_ctx *auth_ctx;
> >>>
> >>>diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
> >>>index db921b11850c16cc7e818cad481705acbb4b9127..164855fb13a9ae88fcf244529b357824762178a8 100644
> >>>--- a/src/providers/ad/ad_id.c
> >>>+++ b/src/providers/ad/ad_id.c
> >>>@@ -27,6 +27,8 @@
> >>>  #include "providers/ldap/sdap_async_enum.h"
> >>>  #include "providers/ldap/sdap_idmap.h"
> >>>
> >>>+#define GC_POSIX_CHECK_TIMEOUT  3600
> >>>+
> >>>  struct ad_handle_acct_info_state {
> >>>      struct be_req *breq;
> >>>      struct be_acct_req *ar;
> >>>@@ -34,6 +36,7 @@ struct ad_handle_acct_info_state {
> >>>      struct sdap_id_conn_ctx **conn;
> >>>      struct sdap_domain *sdom;
> >>>      size_t cindex;
> >>>+    struct ad_options *ad_options;
> >>>
> >>>      int dp_error;
> >>>      const char *err;
> >>>@@ -47,6 +50,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
> >>>                           struct be_req *breq,
> >>>                           struct be_acct_req *ar,
> >>>                           struct sdap_id_ctx *ctx,
> >>>+                         struct ad_options *ad_options,
> >>>                           struct sdap_domain *sdom,
> >>>                           struct sdap_id_conn_ctx **conn)
> >>>  {
> >>>@@ -64,6 +68,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
> >>>      state->ctx = ctx;
> >>>      state->sdom = sdom;
> >>>      state->conn = conn;
> >>>+    state->ad_options = ad_options;
> >>>      state->cindex = 0;
> >>>
> >>>      ret = ad_handle_acct_info_step(req);
> >>>@@ -137,12 +142,29 @@ ad_handle_acct_info_done(struct tevent_req *subreq)
> >>>      if (sdap_err == EOK) {
> >>>          tevent_req_done(req);
> >>>          return;
> >>>+    } else if (sdap_err == ERR_NO_POSIX) {
> >>>+        if (dp_opt_get_bool(state->ad_options->basic, AD_ENABLE_GC)) {
> >>>+            DEBUG(SSSDBG_IMPORTANT_INFO, ("POSIX attributes were requested "
> >>>+                  "but are not present on the server side. Global Catalog "
> >>>+                  "lookups will be disabled for %d seconds\n",
> >>>+                  GC_POSIX_CHECK_TIMEOUT));
> >>>+
> >>>+            state->ad_options->gc_check_timer = time(NULL);
> >>
> >>I would like to suggest rename gc_check_timer to gc_next_check and set
> >>it to time(NULL)+GC_POSIX_CHECK_TIMEOUT here. This will save you an add
> >>operation later which might be called more often (ok, compared to
> >>calling time() the cycles spend for an add are negligible, but still the
> >>check might be easier to read).
> >
> >The timer is gone now :-)
> >
> >>
> >>>+
> >>>+            ret = dp_opt_set_bool(state->ad_options->basic,
> >>>+                                  AD_ENABLE_GC, false);
> >>>+            if (ret != EOK) {
> >>>+                DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+                      ("Could not turn off GC support\n"));
> >>>+                /* Not fatal */
> >>>+            }
> >>>+        }
> >>>      } else if (sdap_err != ENOENT) {
> >>>          tevent_req_error(req, EIO);
> >>>          return;
> >>>      }
> >>>
> >>>-    /* Ret is only ENOENT now. Try the next connection */
> >>>+    /* Ret is only ENOENT or ERR_NO_POSIX now. Try the next connection */
> >>>      state->cindex++;
> >>>      ret = ad_handle_acct_info_step(req);
> >>>      if (ret != EAGAIN) {
> >>>@@ -188,6 +210,25 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
> >>>                struct sss_domain_info *dom, struct be_acct_req *ar)
> >>>  {
> >>>      struct sdap_id_conn_ctx **clist;
> >>>+    time_t now;
> >>>+    errno_t ret;
> >>>+
> >>>+    /* If the last POSIX check was too far in the past, we might need to
> >>>+     * re-check again
> >>>+     */
> >>>+    now = time(NULL);
> >>>+    if (ad_ctx->ad_options->gc_check_timer &&
> >>>+            ad_ctx->ad_options->gc_check_timer + GC_POSIX_CHECK_TIMEOUT < now) {
> >>
> >>see above
> >>
> >>>+        ad_ctx->sdap_id_ctx->srv_opts->posix_checked = false;
> >>>+
> >>>+        ret = dp_opt_set_bool(ad_ctx->ad_options->basic,
> >>>+                              AD_ENABLE_GC, true);
> >>>+        if (ret != EOK) {
> >>>+            DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+                    ("Could not turn on GC support\n"));
> >>>+            /* Not fatal */
> >>>+        }
> >>>+    }
> >>>
> >>>      switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> >>>      case BE_REQ_USER: /* user */
> >>
> >>...
> >>
> >>>+static errno_t sdap_posix_check_parse(struct sdap_handle *sh,
> >>>+                                      struct sdap_msg *msg,
> >>>+                                      void *pvt)
> >>>+{
> >>>+    struct berval **vals;
> >>>+    struct sdap_posix_check_state *state =
> >>>+        talloc_get_type(pvt, struct sdap_posix_check_state);
> >>>+    char *dn;
> >>>+
> >>>+    dn = ldap_get_dn(sh->ldap, msg->msg);
> >>>+    if (dn == NULL) {
> >>>+        DEBUG(SSSDBG_TRACE_LIBS,
> >>>+              ("Search did not find any entry with POSIX attributes\n"));
> >>>+        goto done;
> >>>+    }
> >>>+    DEBUG(SSSDBG_TRACE_LIBS, ("Found [%s] with POSIX attributes\n", dn));
> >>>+    ldap_memfree(dn);
> >>>+
> >>>+    vals = ldap_get_values_len(sh->ldap, msg->msg,
> >>>+                               state->opts->user_map[SDAP_AT_USER_UID].name);
> >>>+    if (vals == NULL) {
> >>>+        vals = ldap_get_values_len(sh->ldap, msg->msg,
> >>>+                               state->opts->group_map[SDAP_AT_GROUP_GID].name);
> >>>+        if (vals == NULL) {
> >>>+            DEBUG(SSSDBG_TRACE_LIBS, ("Entry does not have POSIX attrs?\n"));
> >>>+            goto done;
> >>>+        }
> >>>+    }
> >>
> >>If you really want to check if one of the attributes is present you
> >>might also want to check if the value is really an integer?
> >
> >I'm not sure, this code specifically checks the presence of the
> >attributes, the semantics is checked later when the attributes are used.
> >But the performance hit is very small and the additional checks might
> >bring some nicer error messages, so I've added them.
> >
> >Thank you for the review a new patch is attached.
> 
> Ack.

Thank you both for the review, pushed to master and sssd-1-11



More information about the sssd-devel mailing list