[PATCH] AD: add entry_cache_gpo_timeout option

[Yassir Elley]

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.

[Jakub Hrozek]

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 
> 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.

[Yassir Elley]

----- 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 
> > 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.

[Jakub Hrozek]

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 
> > > 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

[Jakub Hrozek]

On Thu, Jul 31, 2014 at 06:53:07PM +0200, Jakub Hrozek wrote:
> 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

master: ff4b603cc14ea6ea15caaf89a03e927920124af4