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

Yassir Elley yelley at redhat.com
Mon Aug 11 00:18:46 UTC 2014



----- Original Message -----
> 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 we found no GPOs after running to completion, then it means that there are no *applicable* GPOs on the server side (taking into account the hostname, inheritance logic, dacl, supported-cse, etc). That's all that matters. There may well be other GPOs on the server side that were *not* applicable, but we don't care about those.

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

I think IPA's allow_all is simply the default HBAC rule that allows all access. For GPOs, in the absence of any Allow Logon Locally policy setting, the default is also to allow all access. In the case of IPA, as soon as the allow_all rule is removed, then the semantics become deny all (except for those allowed by additional rules). Similarly, as soon as a single user/group is added to the Allow Logon Locally policy setting, the semantics become deny all (except for the added users/groups).

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

The attached revised patches contain the requested changes, including adding code to allow access by default if no gpo_guids are found in the cache. In the online case, we were already allowing access by default if no applicable GPOs were found.

Regards,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-AD-GPO-ad_gpo-changes-for-offline-gpo-support.patch
Type: text/x-patch
Size: 8826 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140810/61722bd4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-GPO-sysdb_gpo-changes-for-offline-gpo-support.patch
Type: text/x-patch
Size: 3612 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140810/61722bd4/attachment-0001.bin>


More information about the sssd-devel mailing list