[SSSD] [PATCH] AD: support gpo processing in offline mode

Jakub Hrozek jhrozek at redhat.com
Tue Aug 5 10:14:47 UTC 2014


On Thu, Jul 31, 2014 at 02:40:02PM -0400, Yassir Elley wrote:
> Hi,
> 
> The attached patch adds support for gpo processing in offline mode. While the code for online mode uses LDAP to determine which gpo-guids are applicable (and then uses SMB to retrieve policy files), the code in offline mode simply retrieves all gpo-guids from the cache (and then retrieves locally cached per-gpo-guid policy files). Note that neither version checking nor the ad_gpo_cache_timeout option are relevant when in offline mode.
> 
> Unresolved issues
> * if there are no gpo-guids in the cache, the code currently denies
>   access; i suspect we should be allowing access instead; agree?

If all the request ran into completion /and/ yet we found no GPOs, then
it means there are no GPOs on the server side, right?

If that's true, then I agree. I don't think there is any equivalent of
IPA's allow_all, correct?

> * i don't think offline callbacks are needed, but i'm unclear about whether
> online callbacks are needed; i suspect they are not needed for the access
> provider (b/c I don't see them being used by the ad_access_filter code);
> should we trigger a fresh round of gpo processing when transitioning from
> offline to online?

This is something I'll test, but I suspect that we don't need additional
callbacks. Usually there would be authentication attempt before the
access control that would flip the offline state to online.

> > 
> Regards,
> Yassir.

> From 99f1f669b20d9f101916ebcf70bd463343624fa1 Mon Sep 17 00:00:00 2001
> From: Yassir Elley <yelley at redhat.com>
> Date: Mon, 21 Jul 2014 15:56:56 -0400
> Subject: [PATCH] AD-GPO: Support offline mode
> 
> ---
>  src/db/sysdb.h            |   7 ++-
>  src/db/sysdb_gpo.c        |  58 +++++++++++++++++++-

It would be nice if you could split the sysdb changes into a separate
patch. ACK to the sysdb changes themselves, though.

>  src/providers/ad/ad_gpo.c | 134 ++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 176 insertions(+), 23 deletions(-)
> 

[...]

> index c26011d672886faadaf2bb60c4537c2de8bf6532..ab92c9a9e859c4fd8077118467948c50cc524f67 100644
> --- a/src/providers/ad/ad_gpo.c
> +++ b/src/providers/ad/ad_gpo.c
> @@ -989,16 +989,67 @@ immediately:
>      return req;
>  }
>  
> +static errno_t
> +process_offline_gpo(TALLOC_CTX *mem_ctx,
> +                    const char *user,
> +                    enum gpo_access_control_mode gpo_mode,
> +                    struct sss_domain_info *domain,
> +                    struct ldb_message *gpo_cache_entry)
> +{
> +    const char *policy_filename = NULL;
> +    const char *cached_gpo_guid;
> +    char **allowed_sids;
> +    int allowed_size;
> +    char **denied_sids;
> +    int denied_size;
> +    int ret;
> +
> +    cached_gpo_guid = ldb_msg_find_attr_as_string(gpo_cache_entry,
> +                                                  SYSDB_GPO_GUID_ATTR, NULL);

I think you should check the cached_gpo_guid for validity here and bail
out if cached_gpo_guid == NULL.

> +
> +    policy_filename = talloc_asprintf(mem_ctx,
> +                                      GPO_CACHE_PATH"/%s/Policies/%s%s",
> +                                      domain->name,
> +                                      cached_gpo_guid,
> +                                      GP_EXT_GUID_SECURITY_SUFFIX);

There is a missing NULL check here as well.

> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "policy_filename:%s\n", policy_filename);
> +
> +    ret = ad_gpo_parse_policy_file(mem_ctx,
> +                                   policy_filename,
> +                                   &allowed_sids,
> +                                   &allowed_size,
> +                                   &denied_sids,
> +                                   &denied_size);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "Cannot parse policy file: [%s][%d][%s]\n",
> +              policy_filename, ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    ret = ad_gpo_access_check
> +        (mem_ctx, gpo_mode, user, domain,
> +         allowed_sids, allowed_size, denied_sids, denied_size);
> +
> + done:
> +    return ret;
> +}
> +
>  static void
>  ad_gpo_connect_done(struct tevent_req *subreq)
>  {
>      struct tevent_req *req;
>      struct ad_gpo_access_state *state;
> -    char* filter;
> +    char *filter;
>      char *sam_account_name;
>      char *domain_dn;
>      int dp_error;
>      errno_t ret;
> +    struct ldb_result *res;
> +    struct ldb_message *gpo_cache_entry;
> +    int i;
>  
>      const char *attrs[] = {AD_AT_DN, AD_AT_UAC, NULL};
>  
> @@ -1009,20 +1060,60 @@ ad_gpo_connect_done(struct tevent_req *subreq)
>      talloc_zfree(subreq);
>  
>      if (ret != EOK) {
> -        /* TBD: handle (dp_error == DP_ERR_OFFLINE) case */
> +        if (dp_error != DP_ERR_OFFLINE) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  "Failed to connect to AD server: [%d](%s)\n",
> +                  ret, strerror(ret));
> +            goto done;
> +        } else {

Could you move the whole "else" block into its own function?

> +            DEBUG(SSSDBG_TRACE_FUNC, "Preparing for offline operation.\n");
>  
> -        DEBUG(SSSDBG_OP_FAILURE,
> -              "Failed to connect to AD server: [%d](%s)\n",
> -               ret, sss_strerror(ret));
> +            ret = sysdb_gpo_get_gpos(state, state->domain, &res);
>  

The rest looks good to me! Given I already had some formatting comments,
I also tested the patch and it works fine.



More information about the sssd-devel mailing list