[SSSD] [PATCH] AD: add entry_cache_gpo_timeout option

Jakub Hrozek jhrozek at redhat.com
Thu Jul 31 16:53:07 UTC 2014


On Wed, Jul 30, 2014 at 08:39:56AM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > On Thu, Jul 24, 2014 at 06:53:24AM -0400, Yassir Elley wrote:
> > > Hi,
> > > 
> > > The attached patch adds support for an "entry_cache_gpo_timeout" option,
> > > which allows an admin to specify how many seconds the backend should
> > > consider locally stored gpo policy files to be valid before asking the
> > > backend again. This is an additional performance enhancement (along with
> > > comparing gpo versions to avoid unnecessary downloads). The timeout is set
> > > when a gpo entry is stored in the cache (which only happens after policy
> > > files have been downloaded).
> > > 
> > > During the processing of a gpo, if the gpo's cache timeout has not elapsed,
> > > the backend does not interact with the gpo_child at all (and therefore
> > > doesn't download any files from the smb server). Rather, it re-uses the
> > > policy files that are already stored locally. Obviously, no gpo version
> > > comparisons need to be made in this case. If the timeout has elapsed (or
> > > there is no gpo entry in the cache), then the backend does interact with
> > > the gpo child in the usual manner (including comparing gpo versions to
> > > avoid unnecessary downloads). Note that the entry_cache_gpo_timeout option
> > > is *not* an expiration for the data in the gpo cache entry (which includes
> > > the cached_gpt_version). Indeed, the cached_gpt_version is never
> > > considered to be "expired".
> > > 
> > > Note that this patch does not add support for offline mode, which will be
> > > implemented in a subsequent patch.
> > > 
> > > Regards,
> > > Yassir.
> > 
> > Hi,
> > 
> > I was wondering if it's a good idea to introduce the option as a global
> > one, present in sssd.conf or a sssd-ad option? See for instance
> > ipa_hbac_refresh in man sssd-ipa, I think that's quite natural
> > counterpart.
> > 
> > Also, the default timeout is way too big, for access control, I think we
> > should go with a lower timeout, the one used is for identity lookups
> > only where we care more about speed than precision.
> > 
> > Some more comments inline:
> > 
> > > From 8aff4445df5cf42e09cd8b521cbd5ff4d6e1a666 Mon Sep 17 00:00:00 2001
> > > From: Yassir Elley <yelley at redhat.com>
> > > Date: Tue, 22 Jul 2014 14:19:35 -0400
> > > Subject: [PATCH] AD-GPO: add entry_cache_gpo_timeout option
> > > 
> > > ---
> > >  src/confdb/confdb.c                  |  11 ++
> > >  src/confdb/confdb.h                  |   2 +
> > >  src/config/SSSDConfig/__init__.py.in |   1 +
> > >  src/config/SSSDConfigTest.py         |   2 +
> > >  src/config/etc/sssd.api.conf         |   1 +
> > >  src/db/sysdb.h                       |  14 +-
> > >  src/db/sysdb_gpo.c                   |  46 +++++-
> > >  src/man/sssd.conf.5.xml              |  13 ++
> > 
> > [...]
> > 
> > > @@ -1305,6 +1311,12 @@ ad_gpo_cse_step(struct tevent_req *req)
> > >      struct tevent_req *subreq;
> > >      struct ad_gpo_access_state *state;
> > >      int i = 0;
> > > +    struct ldb_result *res;
> > > +    errno_t ret;
> > > +    bool found_in_cache = false;
> > > +    bool send_to_child = true;
> > > +    int cached_gpt_version = 0;
> > > +    time_t policy_file_timeout = 0;
> > >  
> > >      state = tevent_req_data(req, struct ad_gpo_access_state);
> > >  
> > > @@ -1327,14 +1339,69 @@ ad_gpo_cse_step(struct tevent_req *req)
> > >      DEBUG(SSSDBG_TRACE_FUNC, "smb_path: %s\n",
> > >      cse_filtered_gpo->smb_path);
> > >      DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n",
> > >      cse_filtered_gpo->gpo_guid);
> > >  
> > > +    /* retrieve gpo cache entry; set cached_gpt_version to -1 if
> > > unavailable */
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "retrieving GPO from cache [%s]\n",
> > > +          cse_filtered_gpo->gpo_guid);
> > > +    ret = sysdb_gpo_get_gpo_by_guid(state,
> > > +                                    state->domain,
> > > +                                    cse_filtered_gpo->gpo_guid,
> > > +                                    &res);
> > > +    if (ret == EOK) {
> > > +        found_in_cache = true;
> > > +    } else {
> > > +        switch (ret) {
> > > +        case ENOENT:
> > > +            DEBUG(SSSDBG_TRACE_FUNC, "ENOENT\n");
> > > +            cached_gpt_version = -1;
> > > +            break;
> > > +        default:
> > > +            DEBUG(SSSDBG_FATAL_FAILURE, "Could not read GPO from cache:
> > > [%s]\n",
> > > +                  strerror(ret));
> > > +            return ret;
> > > +        }
> > > +    }
> > 
> > I think the above is too complex, what about:
> > 
> >      if (ret == EOK) {
> >           /*
> >            * Note: if the timeout is valid, then we can later avoid
> >            downloading
> >            * the GPT.INI file, as well as any policy files (i.e. we don't
> >            need
> >            * to interact with the gpo_child at all). However, even if the
> >            timeout
> >            * is not valid, while we will have to interact with the gpo child
> >            to
> >            * download the GPT.INI file, we may still be able to avoid
> >            downloading
> >            * the policy files (if the cached_gpt_version is the same as the
> >            * GPT.INI version). In other words, the timeout is *not* an
> >            expiration
> >            * for the entire cache entry; the cached_gpt_version never
> >            expires.
> > _          */
> >          cached_gpt_version = ldb_msg_find_attr_as_int(res->msgs[0],
> >                                                         SYSDB_GPO_VERSION_ATTR,
> >                                                         0);
> >                                                                                                                                                                                         
> >           policy_file_timeout = ldb_msg_find_attr_as_uint64
> >               (res->msgs[0], SYSDB_GPO_TIMEOUT_ATTR, 0);
> >                                                                                                                                                                                         
> >           if (policy_file_timeout >= time(NULL)) {
> >               send_to_child = false;
> >           }
> >      } else if (ret == ENOENT) {
> >          DEBUG(SSSDBG_TRACE_FUNC, "ENOENT\n");
> >          cached_gpt_version = -1;
> >      } else {
> >          DEBUG(SSSDBG_FATAL_FAILURE, "Could not read GPO from cache: [%s]\n",
> >                strerror(ret));
> >          return ret;
> >       }
> > 
> > That way you don't need the found_in_cache variable at all.
> > 
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "found_in_cache: %d\n", found_in_cache);
> > > +    if (found_in_cache) {
> > > +
> > > +        /*
> > > +         * Note: if the timeout is valid, then we can later avoid
> > > downloading
> > > +         * the GPT.INI file, as well as any policy files (i.e. we don't
> > > need
> > > +         * to interact with the gpo_child at all). However, even if the
> > > timeout
> > > +         * is not valid, while we will have to interact with the gpo child
> > > to
> > > +         * download the GPT.INI file, we may still be able to avoid
> > > downloading
> > > +         * the policy files (if the cached_gpt_version is the same as the
> > > +         * GPT.INI version). In other words, the timeout is *not* an
> > > expiration
> > > +         * for the entire cache entry; the cached_gpt_version never
> > > expires.
> > > +         */
> > > +
> > > +        cached_gpt_version = ldb_msg_find_attr_as_int(res->msgs[0],
> > > +
> > > SYSDB_GPO_VERSION_ATTR,
> > > +                                                      0);
> > > +
> > > +        policy_file_timeout = ldb_msg_find_attr_as_uint64
> > > +            (res->msgs[0], SYSDB_GPO_TIMEOUT_ATTR, 0);
> > > +
> > > +        if (policy_file_timeout >= time(NULL)) {
> > > +            send_to_child = false;
> > > +        }
> > > +    }
> > > +
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "send_to_child: %d\n", send_to_child);
> > > +    DEBUG(SSSDBG_TRACE_FUNC, "cached_gpt_version: %d\n",
> > > cached_gpt_version);
> > > +
> > > +    cse_filtered_gpo->send_to_child = send_to_child;
> > > +
> > >      subreq = ad_gpo_process_cse_send(state,
> > >                                       state->ev,
> > > -                                     state->domain,
> > > +                                     send_to_child,
> > > +                                     state->domain->name,
> > > +                                     cse_filtered_gpo->gpo_guid,
> > >                                       cse_filtered_gpo->smb_server,
> > >                                       cse_filtered_gpo->smb_share,
> > >                                       cse_filtered_gpo->smb_path,
> > >                                       GP_EXT_GUID_SECURITY_SUFFIX,
> > > -                                     cse_filtered_gpo->gpo_guid);
> > > +                                     cached_gpt_version);
> > >  
> > >      tevent_req_set_callback(subreq, ad_gpo_cse_done, req);
> > >      return EAGAIN;
> > > @@ -1356,10 +1423,12 @@ ad_gpo_cse_done(struct tevent_req *subreq)
> > >      struct ad_gpo_access_state *state;
> > >      int ret;
> > >      int sysvol_gpt_version;
> > > +    const char *policy_filename = NULL;
> > >      char **allowed_sids;
> > >      int allowed_size;
> > >      char **denied_sids;
> > >      int denied_size;
> > > +    time_t now;
> > >  
> > >      req = tevent_req_callback_data(subreq, struct tevent_req);
> > >      state = tevent_req_data(req, struct ad_gpo_access_state);
> > > @@ -1372,8 +1441,7 @@ ad_gpo_cse_done(struct tevent_req *subreq)
> > >      DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n", gpo_guid);
> > >  
> > >      ret = ad_gpo_process_cse_recv(subreq, state, &sysvol_gpt_version,
> > > -                                  &allowed_size, &allowed_sids,
> > > -                                  &denied_size, &denied_sids);
> > > +                                  &policy_filename);
> > >  
> > >      talloc_zfree(subreq);
> > >  
> > > @@ -1383,17 +1451,35 @@ ad_gpo_cse_done(struct tevent_req *subreq)
> > >          goto done;
> > >      }
> > >  
> > > -    DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n",
> > > sysvol_gpt_version);
> > > +    /* only store to cache (and refresh timeout) if we interacted with
> > > child */
> > > +    if (cse_filtered_gpo->send_to_child) {
> > >  
> > > -    ret = sysdb_gpo_store_gpo(state->domain, gpo_guid,
> > > sysvol_gpt_version);
> > > +        DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n",
> > > sysvol_gpt_version);
> > > +
> > > +        now = time(NULL);
> > > +        ret = sysdb_gpo_store_gpo(state->domain, gpo_guid,
> > > sysvol_gpt_version,
> > > +                                  state->domain->gpo_timeout, now);
> > > +        if (ret != EOK) {
> > > +            DEBUG(SSSDBG_OP_FAILURE, "Unable to store gpo cache entry:
> > > [%d](%s}\n",
> > > +                  ret, sss_strerror(ret));
> > > +            goto done;
> > > +        }
> > > +    }
> > 
> > What about moving the above to the ad_gpo_process_cse* request? That way
> > you could always store the GPO data after the child has finished and could
> > avoid branching in the caller.
> > 
> > The rest looks good to me and the patches seem to work fine.
> 
> I agree with all your comments and have attached a revised patch. With regard to the timeout option, I have changed this to be an sssd-ad option. I have changed the name to ad_gpo_cache_timeout and have modelled it after ipa_hbac_refresh (including the 5 second default timeout).
> 
> Regards,
> Yassir.

The code now looks good to me and I've verified that if you log in twice
within the ad_gpo_cache_timeout interval, the gpo_child is not called at
all.

ACK



More information about the sssd-devel mailing list