[SSSD] [PATCH] AD: Add initial gpo-smb implementation.

Yassir Elley yelley at redhat.com
Thu Jun 12 20:37:25 UTC 2014



----- Original Message -----
> On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
> > 
> > 
> > ----- Original Message -----
> > > On (29/05/14 09:06), Yassir Elley wrote:
> > > >----- Original Message -----
> > > >> On Thu, May 29, 2014 at 04:09:21AM -0400, Yassir Elley wrote:
> > > >> > Also note that some (but not all) of the changes that were pushed to
> > > >> > master
> > > >> > as part of my patch titled "add libsmbclient to makefiles" seem to
> > > >> > have
> > > >> > been
> > > >> > deleted by subsequent pushes to master. As such, this patch
> > > >> > reintroduces
> > > >> > those changes (which only affect configure.ac and libsmbclient.m4).
> > > >> 
> > > >> That would be worrisome, what changes in particular? I'm not aware of
> > > >> pushing anything else than acked patches from the list..
> > > >> 
> > > >> Are you talking about the autoconf refactor Lukas did? IIRC that one
> > > >> only merged some code duplicates into a single file..
> > > >> _______________________________________________
> > > >> sssd-devel mailing list
> > > >> sssd-devel at lists.fedorahosted.org
> > > >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >> 
> > > >
> > > >Hi Jakub,
> > > >
> > > >You are right. I overlooked the fact that the autoconf refactor had
> > > >moved
> > > >things around. My mistake.
> > > >
> > > >Revised patch attached.
> > > >
> > > >Regards,
> > > >Yassir.
> > > 
> > > >From abc4ac3e664ccbeeb58d131dc6c37cc314f5ba3d Mon Sep 17 00:00:00 2001
> > > >From: Yassir Elley <yelley at redhat.com>
> > > >Date: Wed, 21 May 2014 15:11:36 -0400
> > > >Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
> > > >
> > > >---
> > > > src/providers/ad/ad_gpo.c | 680
> > > > +++++++++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 676 insertions(+), 4 deletions(-)
> > > >
> > > >diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> > > >index
> > > >168ed2d7e8657799a7c773ec8b24897cdd303ec4..519c0dbec4d85e7f6e41db672ae0f2c2f720d81b
> > > >100644
> > > >--- a/src/providers/ad/ad_gpo.c
> > > >+++ b/src/providers/ad/ad_gpo.c
> > > >@@ -31,6 +31,9 @@
> > > >  */
> > > > 
> > > > #include <security/pam_modules.h>
> > > >+#include <libsmbclient.h>
> > > >+#include <ini_configobj.h>
> > > >+#include <ini_config.h>
> > >             ^^^^^^^^^^^^
> > > This is the old ini_config interface. You can remove this line.
> > > 
> > > > #include "util/util.h"
> > > > #include "util/strtonum.h"
> > > > #include "providers/data_provider.h"
> > > 
> > > // snip
> > > 
> > > >+    int ret;
> > > >+    char **allow_sids = NULL; char **deny_sids = NULL;
> > > >+    int allow_size = 0; int deny_size = 0;
> > > >+
> > > >+    ret = ini_config_create(&ini_config);
> > > >+    if (ret) goto done;
> > > >+    ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
> > >             ^^^^^^^^^^^^^^^^^^^^^^^^
> > > This function is available in libini_config-1.1
> > > Please, update buildrequires in spec file.
> > > 
> > > diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
> > > index b5e7903..10704ae 100644
> > > --- a/contrib/sssd.spec.in
> > > +++ b/contrib/sssd.spec.in
> > > @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel
> > >  BuildRequires: libldb-devel
> > >  BuildRequires: libdhash-devel >= 0.4.2
> > >  BuildRequires: libcollection-devel
> > > -BuildRequires: libini_config-devel
> > > +BuildRequires: libini_config-devel >= 1.1
> > >  BuildRequires: dbus-devel
> > >  BuildRequires: dbus-libs
> > >  %if 0%{?rhel5_minor} >= 7
> > > 
> > > LS
> > 
> > Revised patch attached.
> > 
> > Also, I successfully tested the code after installing the ding-libs
> > packages from lslebodn's repo. Previously, I was using rpms that I had
> > manually compiled (for the ini_config_file_from_mem functionality).
> > 
> > Thanks,
> > Yassir.
> > 
> 
> 
> I didn't had a chance to do functional testings yet, I'll send my
> results tomorrow. For a start please find my comments on the code
> inline.

Please let me know if you have additional comments after functional testing.

> 
> bye,
> Sumit
> 
> 
> > From 24625d6cd02faeb39e8522769d470b3a8beed513 Mon Sep 17 00:00:00 2001
> > From: Yassir Elley <yelley at redhat.com>
> > Date: Wed, 21 May 2014 15:11:36 -0400
> > Subject: [PATCH] AD-GPO: Add initial gpo-smb implementation
> > 
> > ---
> >  contrib/sssd.spec.in      |   2 +-
> >  src/providers/ad/ad_gpo.c | 679
> >  +++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 676 insertions(+), 5 deletions(-)
> > 
> > diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
> > index
> > b5e7903713ccc6944fbed4f84c2f0413a257b760..10704ae48cf5636de2a5cbbb42fc00b99052144f
> > 100644
> > --- a/contrib/sssd.spec.in
> > +++ b/contrib/sssd.spec.in
> > @@ -97,7 +97,7 @@ BuildRequires: libtdb-devel
> >  BuildRequires: libldb-devel
> >  BuildRequires: libdhash-devel >= 0.4.2
> >  BuildRequires: libcollection-devel
> > -BuildRequires: libini_config-devel
> > +BuildRequires: libini_config-devel >= 1.1
> 
> Please add this new minimal version to the configure check as well.

I'm not exactly sure what you mean here? Can you please provide a diff?

> 
> >  BuildRequires: dbus-devel
> >  BuildRequires: dbus-libs
> >  %if 0%{?rhel5_minor} >= 7
> > diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> > index
> > bb6896e70ae96036b1a9e58a6c8e25d3d8f1543f..9a0f09f9d2735158d970bb19d631645eb8ae9cfa
> > 100644
> > --- a/src/providers/ad/ad_gpo.c
> > +++ b/src/providers/ad/ad_gpo.c
> 
> ...
> 
> > @@ -543,6 +571,235 @@ ad_gpo_filter_gpos_by_dacl(TALLOC_CTX *mem_ctx,
> >      return ret;
> >  }
> >  
> > +/*
> > + * This function determines whether the input cse_guid matches any of the
> > input
> > + * gpo_cse_guids. The boolean result is assigned to the _included output
> > param.
> > + */
> > +static errno_t
> > +ad_gpo_includes_cse_guid(const char *cse_guid,
> > +                         const char **gpo_cse_guids,
> > +                         int num_gpo_cse_guids,
> > +                         bool *_included)
> 
> In general I prefer that a function returns an error code and results
> are returned by reference. But in this case no error is returned only
> EOK and I think it would be easier to just return the bool value
> directly. If you prefer to keep the error code, e.g. because you plan to
> add some NULL checks for cse_guid and gpo_cse_guids and return EINVAL in
> this case, please check the return value when calling
> ad_gpo_includes_cse_guid() below.

I will change it to return a bool value, since the NULL checks are not needed.

> 
> > +{
> > +    int i = 0;
> > +    const char *gpo_cse_guid = NULL;
> > +
> > +    for (i = 0; i < num_gpo_cse_guids; i++) {
> > +        gpo_cse_guid = gpo_cse_guids[i];
> > +        if (strcmp(gpo_cse_guid, cse_guid) == 0) {
> > +            *_included = true;
> > +            return EOK;
> > +        }
> > +    }
> > +
> > +    *_included = false;
> > +    return EOK;
> > +}
> > +
> > +/*
> > + * This function takes an input dacl_filtered_gpos list, filters out any
> > gpo
> > + * that does not contain the input cse_guid, and assigns the result to the
> > + * _cse_filtered_gpos output parameter.
> > + */
> > +static errno_t
> > +ad_gpo_filter_gpos_by_cse_guid(TALLOC_CTX *mem_ctx,
> > +                               const char *cse_guid,
> > +                               struct gp_gpo **dacl_filtered_gpos,
> > +                               int num_dacl_filtered_gpos,
> > +                               struct gp_gpo ***_cse_filtered_gpos,
> > +                               int *_num_cse_filtered_gpos)
> > +{
> > +    TALLOC_CTX *tmp_ctx = NULL;
> > +    int i = 0;
> > +    int ret = 0;
> > +    struct gp_gpo *dacl_filtered_gpo = NULL;
> > +    int gpo_dn_idx = 0;
> > +    struct gp_gpo **cse_filtered_gpos = NULL;
> > +    bool included;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (tmp_ctx == NULL) {
> > +        ret = ENOMEM;
> > +        goto done;
> > +    }
> > +
> > +    cse_filtered_gpos = talloc_array(tmp_ctx,
> > +                                     struct gp_gpo *,
> > +                                     num_dacl_filtered_gpos + 1);
> > +    if (cse_filtered_gpos == NULL) {
> > +        ret = ENOMEM;
> > +        goto done;
> > +    }
> > +
> > +    for (i = 0; i < num_dacl_filtered_gpos; i++) {
> > +
> > +        dacl_filtered_gpo = dacl_filtered_gpos[i];
> > +
> > +        DEBUG(SSSDBG_TRACE_ALL, "examining cse candidate_gpo_guid: %s\n",
> > +              dacl_filtered_gpo->gpo_guid);
> > +
> > +        ret = ad_gpo_includes_cse_guid(cse_guid,
> > +                                       dacl_filtered_gpo->gpo_cse_guids,
> > +
> > dacl_filtered_gpo->num_gpo_cse_guids,
> > +                                       &included);
> 
> see above
> 
> > +        if (included) {
> > +            DEBUG(SSSDBG_TRACE_ALL,
> > +                  "GPO applicable to target per cse_guid filtering\n");
> > +            cse_filtered_gpos[gpo_dn_idx] =
> > talloc_steal(cse_filtered_gpos,
> > +
> > dacl_filtered_gpo);
> 
> Do you need the item still in dacl_filtered_gpos? If not I would prefer
> to set dacl_filtered_gpos[i] to NULL or call talloc_move on it. If it is
> still needed in dacl_filtered_gpos I wonder if it would be safer to copy
> the item to avoid that someone iterates over the dacl_filtered_gpos
> elements and frees them individually?

The item in dacl_filtered_gpos is no longer necessary. I will set dacl_filtered_gpos[i] to NULL.

> 
> > +            gpo_dn_idx++;
> > +        } else {
> > +            DEBUG(SSSDBG_TRACE_ALL,
> > +                  "GPO not applicable to target per cse_guid
> > filtering\n");
> > +            continue;
> > +        }
> > +    }
> > +
> > +    cse_filtered_gpos[gpo_dn_idx] = NULL;
> > +
> > +    *_cse_filtered_gpos = talloc_steal(mem_ctx, cse_filtered_gpos);
> > +    *_num_cse_filtered_gpos = gpo_dn_idx;
> > +
> > +    ret = EOK;
> > +
> > + done:
> > +    talloc_free(tmp_ctx);
> > +    return ret;
> > +}
> > +
> > +/*
> > + * This cse-specific function (GP_EXT_GUID_SECURITY) populates the output
> > + * parameter (found) based on whether the input user_sid or any of the
> > input
> > + * group_sids appear in the input list of privilege_sids.
> > + */
> > +static void
> > +check_rights(char **privilege_sids,
> > +             int privilege_size,
> > +             const char *user_sid,
> > +             const char **group_sids,
> > +             int group_size,
> > +             int *found)
> 
> I would suggest to return 'found' directly here as well and maybe
> convert it to a bool or is there a reason for having an int here?

I will return a bool directly here as well.

> 
> > +{
> > +  int match = 0;
> > +  int i, j;
> > +
> > +  for (i = 0; i < privilege_size; i++) {
> > +    if (strcmp(user_sid, privilege_sids[i]) == 0) {
> > +      match = 1;
> > +      break;
> > +    }
> > +    for (j = 0; j < group_size; j++) {
> > +      if (strcmp(group_sids[j], privilege_sids[i]) == 0) {
> > +	match = 1;
> > +	break;
> 
>    ^ Tab indent
> 
> > +      }
> > +    }
> > +  }
> > +
> > +  *found = match;
> > +}
> > +
> > +/*
> > + * This cse-specific function (GP_EXT_GUID_SECURITY) performs HBAC policy
> > + * application and determines whether logon access is granted or denied
> > for
> > + * the {user,domain} tuple specified in the inputs. This function returns
> > EOK
> > + * to indicate that access is granted. Any other return value indicates
> > that
> > + * access is denied.
> > + *
> > + * The access control algorithm first determines whether the
> > "principal_sids"
> > + * (i.e. user_sid or group_sids) appear in allowed_sids and denied_sids.
> > + *
> > + * For access to be granted, both the "allowed_sids_condition" *and* the
> > + * "denied_sids_condition" must be met (in all other cases, access is
> > denied).
> > + * 1) The "allowed_sids_condition" is satisfied if any of the
> > principal_sids
> > + *    appears in allowed_sids OR if the allowed_sids list is empty
> > + * 2) The "denied_sids_condition" is satisfied if none of the
> > principal_sids
> > + *    appear in denied_sids
> > + *
> > + * Note that a deployment that is unaware of GPO-based access-control
> > policy
> > + * settings is unaffected by them (b/c the absence of allowed_sids grants
> > access).
> > + *
> > + * Note that if a principal_sid appears in both allowed_sids and
> > denied_sids,
> > + * the "allowed_sids_condition" is met, but the "denied_sids_condition" is
> > not.
> > + * In other words, Deny takes precedence over Allow.
> 
> I have two comments here. First, is there a way to avoid having deny
> rules? The reason is that we had some troubles in the past here, because
> when checking deny rules all group memberships of a user must be
> available to properly deny access. If some are missing access might be
> granted although it should be rejected. I guess it is not possible to
> get rid of them because they will be set on the AD side and we have to
> respect them.

I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?

In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side. 

> 
> Second, you are only checking SeInteractiveLogonRight and
> SeDenyInteractiveLogonRight. What about
> Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you
> planning to integrate them as well and relate them to different PAM
> services?

The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.

> 
> > + */
> > +static errno_t
> > +ad_gpo_access_check(TALLOC_CTX *mem_ctx,
> > +                    const char *user,
> > +                    struct sss_domain_info *domain,
> > +                    char **allowed_sids,
> > +                    int allowed_size,
> > +                    char **denied_sids,
> > +                    int denied_size)
> > +{
> > +    const char *user_sid;
> > +    const char **group_sids;
> > +    int group_size = 0;
> > +    int access_granted = 0;
> > +    int access_denied = 0;
> 
> Is there a reason to use int instead of bool?

No. I have made the change.

> 
> > +    int ret;
> > +    int j;
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC, "POLICY FILE:\n");
> > +    DEBUG(SSSDBG_TRACE_FUNC, "allowed_size = %d\n", allowed_size);
> > +    for (j= 0; j < allowed_size; j++) {
> > +        DEBUG(SSSDBG_TRACE_FUNC, "allowed_sids[%d] = %s\n", j,
> > +              allowed_sids[j]);
> > +    }
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC, "denied_size = %d\n", denied_size);
> > +    for (j= 0; j < denied_size; j++) {
> > +        DEBUG(SSSDBG_TRACE_FUNC, " denied_sids[%d] = %s\n", j,
> > +              denied_sids[j]);
> > +    }
> > +
> > +    ret = ad_gpo_get_sids(mem_ctx, user, domain, &user_sid,
> > +                          &group_sids, &group_size);
> > +    if (ret != EOK) {
> > +        ret = ERR_NO_SIDS;
> > +        DEBUG(SSSDBG_OP_FAILURE,
> > +              "Unable to retrieve SIDs: [%d](%s)\n", ret,
> > sss_strerror(ret));
> > +        goto done;
> > +    }
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC, "CURRENT USER:\n");
> > +    DEBUG(SSSDBG_TRACE_FUNC, "       user_sid = %s\n", user_sid);
> > +
> > +    for (j= 0; j < group_size; j++) {
> > +        DEBUG(SSSDBG_TRACE_FUNC, "  group_sids[%d] = %s\n", j,
> > +              group_sids[j]);
> > +    }
> > +
> > +    /* If AllowLogonLocally is not defined, all users are allowed */
> > +    if (allowed_size == 0) {
> > +      access_granted = 1;
> > +    }  else {
> > +      check_rights(allowed_sids, allowed_size, user_sid,
> > +                   group_sids, group_size, &access_granted);
> > +    }
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC, " access_granted = %d\n", access_granted);
> > +
> > +    check_rights(denied_sids, denied_size, user_sid,
> > +                 group_sids, group_size, &access_denied);
> > +    DEBUG(SSSDBG_TRACE_FUNC, "  access_denied = %d\n", access_denied);
> > +
> > +    if (access_granted && !access_denied) {
> > +        return EOK;
> > +    } else {
> > +        return EACCES;
> > +    }
> > +
> > + done:
> > +
> > +    if (ret) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +
> >  /* == ad_gpo_access_send/recv implementation
> >  ================================*/
> >  
> >  struct ad_gpo_access_state {
> > @@ -558,6 +815,9 @@ struct ad_gpo_access_state {
> >      const char *target_dn;
> >      struct gp_gpo **dacl_filtered_gpos;
> >      int num_dacl_filtered_gpos;
> > +    struct gp_gpo **cse_filtered_gpos;
> > +    int num_cse_filtered_gpos;
> > +    int cse_gpo_index;
> >  };
> >  
> >  static void ad_gpo_connect_done(struct tevent_req *subreq);
> > @@ -565,6 +825,9 @@ static void ad_gpo_target_dn_retrieval_done(struct
> > tevent_req *subreq);
> >  static void ad_gpo_process_som_done(struct tevent_req *subreq);
> >  static void ad_gpo_process_gpo_done(struct tevent_req *subreq);
> >  
> > +static errno_t ad_gpo_cse_step(struct tevent_req *req);
> > +static void ad_gpo_cse_done(struct tevent_req *subreq);
> > +
> >  struct tevent_req *
> >  ad_gpo_access_send(TALLOC_CTX *mem_ctx,
> >                     struct tevent_context *ev,
> > @@ -586,6 +849,9 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
> >      state->domain = domain;
> >      state->dacl_filtered_gpos = NULL;
> >      state->num_dacl_filtered_gpos = 0;
> > +    state->cse_filtered_gpos = NULL;
> > +    state->num_cse_filtered_gpos = 0;
> > +    state->cse_gpo_index = -1;
> >      state->ev = ev;
> >      state->user = user;
> >      state->ldb_ctx = sysdb_ctx_get_ldb(domain->sysdb);
> > @@ -840,6 +1106,13 @@ ad_gpo_process_som_done(struct tevent_req *subreq)
> >  /*
> >   * This function retrieves a list of candidate_gpos and potentially
> >   reduces it
> >   * to a list of dacl_filtered_gpos, based on each GPO's DACL.
> > + *
> > + * This function then takes the list of dacl_filtered_gpos and potentially
> > + * reduces it to a list of cse_filtered_gpos, based on whether each GPO's
> > list
> > + * of cse_guids includes the "SecuritySettings" CSE GUID (used for HBAC).
> > + *
> > + * This function then sends each cse_filtered_gpo to the CSE processing
> > engine
> > + * for policy application, which currently consists of HBAC functionality.
> >   */
> >  static void
> >  ad_gpo_process_gpo_done(struct tevent_req *subreq)
> > @@ -878,7 +1151,7 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq)
> >                                       &state->num_dacl_filtered_gpos);
> >      if (ret != EOK) {
> >          DEBUG(SSSDBG_OP_FAILURE,
> > -              "Unable to filter GPO list: [%d](%s)\n",
> > +              "Unable to filter GPO list by DACKL: [%d](%s)\n",
> 
> I guess this                                      ^ is a typo to make the
> German reviewer smile (https://de.wikipedia.org/wiki/Dackel :-).
> 

Just for you, Sumit :)

> >                ret, sss_strerror(ret));
> >          goto done;
> >      }
> > @@ -896,10 +1169,133 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq)
> >                state->dacl_filtered_gpos[i]->gpo_guid);
> >      }
> >  
> > -    /* TBD: initiate SMB retrieval */
> > -    DEBUG(SSSDBG_TRACE_FUNC, "time for SMB retrieval\n");
> > +    ret = ad_gpo_filter_gpos_by_cse_guid(state,
> > +                                         GP_EXT_GUID_SECURITY,
> > +                                         state->dacl_filtered_gpos,
> > +                                         state->num_dacl_filtered_gpos,
> > +                                         &state->cse_filtered_gpos,
> > +                                         &state->num_cse_filtered_gpos);
> >  
> > -    ret = EOK;
> > +    if (ret != EOK) {
> > +        DEBUG(SSSDBG_OP_FAILURE,
> > +              "Unable to filter GPO list by CSE_GUID: [%d](%s)\n",
> > +               ret, strerror(ret));
> > +        goto done;
> > +    }
> > +
> > +    if (state->cse_filtered_gpos[0] == NULL) {
> > +        /* no gpos contain "SecuritySettings" cse_guid, nothing to enforce
> > */
> > +        DEBUG(SSSDBG_TRACE_FUNC,
> > +              "no applicable gpos found after cse_guid filtering\n");
> > +        ret = EOK;
> > +        goto done;
> > +    }
> > +
> > +    for (i = 0; i < state->num_cse_filtered_gpos; i++) {
> > +        DEBUG(SSSDBG_TRACE_FUNC, "cse_filtered_gpos[%d]->gpo_guid is
> > %s\n", i,
> > +                                  state->cse_filtered_gpos[i]->gpo_guid);
> > +    }
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC, "num_cse_filtered_gpos: %d\n",
> > +          state->num_cse_filtered_gpos);
> > +
> > +    ret = ad_gpo_cse_step(req);
> > +
> > + done:
> > +
> > +    if (ret == EOK) {
> > +        tevent_req_done(req);
> > +    } else if (ret != EAGAIN) {
> > +        tevent_req_error(req, ret);
> > +    }
> > +}
> > +
> > +static errno_t
> > +ad_gpo_cse_step(struct tevent_req *req)
> > +{
> > +    struct tevent_req *subreq;
> > +    struct ad_gpo_access_state *state;
> > +    char *smb_uri;
> > +    int i = 0;
> > +
> > +    state = tevent_req_data(req, struct ad_gpo_access_state);
> > +
> > +    state->cse_gpo_index++;
> > +    struct gp_gpo *cse_filtered_gpo =
> > +        state->cse_filtered_gpos[state->cse_gpo_index];
> > +
> > +    /* cse_filtered_gpo is NULL only after all GPOs have been processed */
> > +    if (cse_filtered_gpo == NULL) return EOK;
> > +
> > +    DEBUG(SSSDBG_TRACE_FUNC, "cse filtered_gpos[%d]->gpo_guid is %s\n",
> > +          state->cse_gpo_index, cse_filtered_gpo->gpo_guid);
> > +    DEBUG(SSSDBG_TRACE_FUNC, "cse filtered_gpos[%d]->file_sys_path is
> > %s\n",
> > +          state->cse_gpo_index, cse_filtered_gpo->gpo_file_sys_path);
> > +    for (i = 0; i < cse_filtered_gpo->num_gpo_cse_guids; i++) {
> > +        DEBUG(SSSDBG_TRACE_ALL,
> > +              "cse_filtered_gpos[%d]->gpo_cse_guids[%d]->gpo_guid is
> > %s\n",
> > +              state->cse_gpo_index, i,
> > cse_filtered_gpo->gpo_cse_guids[i]);
> > +    }
> > +
> > +    smb_uri = talloc_asprintf(state, "%s%s",
> > +                              cse_filtered_gpo->gpo_file_sys_path,
> > +                              GP_EXT_GUID_SECURITY_SUFFIX);
> > +
> > +    subreq = ad_gpo_process_cse_send(state, state->ev, smb_uri,
> > cse_filtered_gpo);
> > +
> > +    tevent_req_set_callback(subreq, ad_gpo_cse_done, req);
> > +    return EAGAIN;
> > +}
> > +
> > +/*
> > + * This cse-specific function (GP_EXT_GUID_SECURITY) retrieves a list of
> > + * allowed_sids and denied_sids, and uses them to determine whether logon
> > + * access is granted or denied for the state's {user, domain} tuple.
> > + *
> > + * If it is determined that the current cse_filtered_gpo grants access,
> > then
> > + * we process the next cse_filtered_gpo in the list. At any time, if
> > access is
> > + * denied, we return immediately with an error.
> > + */
> > +static void
> > +ad_gpo_cse_done(struct tevent_req *subreq)
> > +{
> > +    struct tevent_req *req;
> > +    struct ad_gpo_access_state *state;
> > +    int ret;
> > +    char **allowed_sids;
> > +    int allowed_size;
> > +    char **denied_sids;
> > +    int denied_size;
> > +
> > +    req = tevent_req_callback_data(subreq, struct tevent_req);
> > +    state = tevent_req_data(req, struct ad_gpo_access_state);
> > +
> > +    ret = ad_gpo_process_cse_recv(subreq, state, &allowed_sids,
> > &allowed_size,
> > +                                  &denied_sids, &denied_size);
> > +
> > +    talloc_zfree(subreq);
> > +
> > +    if (ret != EOK) {
> > +        /* TBD: handle ret error  */
> > +        goto done;
> > +    }
> > +
> > +    /* TBD: allowed/denied_sids/size, should be retrieved from cache */
> > +    ret = ad_gpo_access_check
> > +        (state, state->user, state->domain,
> > +         allowed_sids, allowed_size, denied_sids, denied_size);
> > +
> > +    if (ret == EOK) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "ret is EOK\n");
> 
> I think this should better be a SSSDBG_TRACE_ALL if needed at all.
> 
> > +    } else {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "ret is not EOK\n");
> 
> Please make the message a bit more verbose.
> 

These two debug messages were left over from my initial troubleshooting during development. I have removed them both.

> > +        DEBUG(SSSDBG_OP_FAILURE,
> > +              "GPO check failed: [%d](%s)\n",
> > +              ret, strerror(ret));
> > +        goto done;
> > +    }
> > +
> > +    ret = ad_gpo_cse_step(req);
> >  
> >   done:
> >  
> > @@ -2307,3 +2703,278 @@ ad_gpo_process_gpo_recv(struct tevent_req *req,
> >      *num_candidate_gpos = state->num_candidate_gpos;
> >      return EOK;
> >  }
> > +
> > +/* == ad_gpo_process_cse_send/recv helpers
> > ================================= */
> > +
> > +/*
> > + * This function uses the input ini_config object to parse the logon right
> > value
> > + * associated with the input name. This value is a list of sids, and is
> > used
> > + * to populate the output parameters. The input name can be either
> > + * ALLOW_LOGON_LOCALLY or DENY_LOGON_LOCALLY.
> > + */
> > +static errno_t
> > +parse_logon_right_with_libini(struct ini_cfgobj *ini_config,
> > +                              const char *name,
> > +                              int *_size,
> > +                              char ***_sids)
> > +{
> > +  int ret = 0;
> > +  struct value_obj *vobj = NULL;
> > +  char **sids = NULL;
> > +  int num_sids = 0;
> > +  int i;
> > +
> > +  ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
> > +                                INI_GET_FIRST_VALUE, &vobj);
> > +  if (vobj == NULL) {
> > +      DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
> > +            RIGHTS_SECTION, name);
> > +      return EOK;
> > +  }
> > +  sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
> > +
> > +  if (ret) {
> 
> Please do explicit checks, in this case (ret != 0) because according to
> the docs ini_get_string_config_array() returns 0 on success.

Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?

> 
> > +      DEBUG(SSSDBG_CRIT_FAILURE,
> > +            "ini_get_string_config_array failed [%d][%s]\n", ret,
> > strerror(ret));
> > +      return ret;
> > +  }
> > +
> > +  for (i = 0; i <num_sids; i++) {
> > +    if (sids[i][0] == '*') {
> > +      sids[i]++;
> > +    }
> > +  }
> 
> What is this loop good for?
> 
 
The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).

> > +
> > +  *_size = num_sids;
> > +  *_sids = sids;
> > +
> > +  return EOK;
> > +}
> > +
> > +/*
> > + * This function parses the cse-specific (GP_EXT_GUID_SECURITY) input
> > data_buf,
> > + * and uses the results to populate the output parameters with the list of
> > + * allowed_sids and denied_sids
> > + */
> > +static errno_t
> > +ad_gpo_parse_security_cse_buffer(uint8_t *data_buf,
> > +                                 int data_len,
> > +                                 char ***allowed_sids,
> > +                                 int *allowed_size,
> > +                                 char ***denied_sids,
> > +                                 int *denied_size)
> > +{
> > +    struct ini_cfgfile *file_ctx = NULL;
> > +    struct ini_cfgobj *ini_config = NULL;
> > +    int ret;
> > +    char **allow_sids = NULL; char **deny_sids = NULL;
> > +    int allow_size = 0; int deny_size = 0;
> > +
> > +    ret = ini_config_create(&ini_config);
> > +    if (ret) goto done;
> 
> see above (ret != 0)
> 
> > +    ret = ini_config_file_from_mem(data_buf, data_len, &file_ctx);
> > +    if (ret) goto done;
> 
> see above
> 
> > +    ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
> > +    if (ret) goto done;
> 
> see above
> 
> > +
> > +    ret = parse_logon_right_with_libini(ini_config,
> > +                                        ALLOW_LOGON_LOCALLY,
> > +                                        &allow_size,
> > +                                        &allow_sids);
> > +    if (ret) {goto done;}
> 
> see above, here (ret != EOK) would be good as well because
> parse_logon_right_with_libini() returns EOK on success.
> 
> > +
> > +    ret = parse_logon_right_with_libini(ini_config,
> > +                                        DENY_LOGON_LOCALLY,
> > +                                        &deny_size,
> > +                                        &deny_sids);
> > +    if (ret) {goto done;}
> 
> see above
> 
> > +
> > +    *allowed_sids = allow_sids;
> > +    *allowed_size = allow_size;
> > +    *denied_sids = deny_sids;
> > +    *denied_size = deny_size;
> > +
> > + done:
> > +
> > +    if (ret) {
> 
> see above
> 
> > +      DEBUG(SSSDBG_CRIT_FAILURE, "Error encountered: %d.\n", ret);
> > +    }
> > +
> > +    ini_config_file_close(file_ctx);
> > +    return ret;
> > +}
> > +
> > +static void
> > +sssd_krb_get_auth_data_fn(const char * pServer,
> > +                     const char * pShare,
> > +                     char * pWorkgroup,
> > +                     int maxLenWorkgroup,
> > +                     char * pUsername,
> > +                     int maxLenUsername,
> > +                     char * pPassword,
> > +                     int maxLenPassword)
> > +{
> > +    /* since we are using kerberos for authentication, we simply return */
> > +    return;
> > +}
> > +
> > +
> > +/*
> > + * This cse-specific function (GP_EXT_GUID_SECURITY) opens an SMB
> > connection,
> > + * retrieves the data referenced by the input smb_uri, and then closes the
> > SMB
> > + * connection. The data is then parsed and the results are used to
> > populate the
> > + * output parameters with the list of allowed_sids and denied_sids
> > + */
> > +static errno_t
> > +process_security_settings_cse(TALLOC_CTX *mem_ctx,
> > +                              const char *smb_uri,
> > +                              char ***_allowed_sids,
> > +                              int *_allowed_size,
> > +                              char ***_denied_sids,
> > +                              int *_denied_size)
> > +{
> > +    SMBCCTX *context;
> > +    int ret = 0;
> > +    uint8_t *buf = NULL;
> > +    int bytesread = 0;
> > +
> > +    char **allowed_sids;
> > +    char **denied_sids;
> > +    int allowed_size = 0;
> > +    int denied_size = 0;
> > +
> > +    DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
> > +
> > +    context = smbc_new_context();
> > +    if (!context) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc
> > context\n");
> > +    }
> > +
> > +    smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
> > +    smbc_setOptionUseKerberos(context, 1);
> > +
> > +    /* Initialize the context using the previously specified options */
> > +    if (!smbc_init_context(context)) {
> > +        smbc_free_context(context, 0);
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc context\n");
> > +    }
> > +
> > +    /* Tell the compatibility layer to use this context */
> > +    smbc_set_context(context);
> > +
> > +    int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
> > +    if (remotehandle < 0) DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open
> > failed\n");
> > +
> > +    buf = talloc_array(mem_ctx, uint8_t, SMB_BUFFER_SIZE);
> > +    bytesread = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
> > +    if(bytesread < 0) DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
> > +
> > +    DEBUG(SSSDBG_CRIT_FAILURE, "bytesread: %d\n", bytesread);
> > +
> > +    smbc_close(remotehandle);
> > +
> > +    ret = ad_gpo_parse_security_cse_buffer(buf,
> > +                                           bytesread,
> > +                                           &allowed_sids,
> > +                                           &allowed_size,
> > +                                           &denied_sids,
> > +                                           &denied_size);
> > +
> > +    /* TBD: allowed/denied_sids/size should be stored in cache */
> > +
> > +    *_allowed_sids = allowed_sids;
> > +    *_allowed_size = allowed_size;
> > +    *_denied_sids = denied_sids;
> > +    *_denied_size = denied_size;
> > +
> > +    return ret;
> > +}
> > +
> > +/* == ad_gpo_process_cse_send/recv implementation
> > ========================== */
> > +
> > +struct ad_gpo_process_cse_state {
> > +    struct tevent_context *ev;
> > +    char **allowed_sids;
> > +    int allowed_size;
> > +    char **denied_sids;
> > +    int denied_size;
> > +    struct gp_gpo *cse_filtered_gpo;
> > +};
> > +
> > +/*
> > + * This cse-specific function (GP_EXT_GUID_SECURITY) retrieves the data
> > + * referenced by the input smb_uri, and uses the parsed results to
> > populate the
> > + * state's list of allowed_sids and denied_sids.
> > + */
> > +struct tevent_req *
> > +ad_gpo_process_cse_send(TALLOC_CTX *mem_ctx,
> > +                        struct tevent_context *ev,
> > +                        char *smb_uri,
> > +                        struct gp_gpo *cse_filtered_gpo)
> > +{
> > +    struct tevent_req *req;
> > +    struct ad_gpo_process_cse_state *state;
> > +    errno_t ret;
> > +
> > +    char **allowed_sids;
> > +    int allowed_size;
> > +    char **denied_sids;
> > +    int denied_size;
> > +
> > +    req = tevent_req_create(mem_ctx, &state, struct
> > ad_gpo_process_cse_state);
> > +    if (req == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n");
> > +        return NULL;
> > +    }
> > +
> > +    state->ev = ev;
> > +    state->cse_filtered_gpo = cse_filtered_gpo;
> > +
> > +    ret = process_security_settings_cse
> > +        (state, smb_uri, &allowed_sids, &allowed_size,
> > +         &denied_sids, &denied_size);
> > +
> > +   if (ret != EOK) {
> > +       DEBUG(SSSDBG_CRIT_FAILURE, "process_security_settings_cse
> > FAILED\n");
> > +       goto done;
> > +    }
> > +
> > +   state->allowed_sids = allowed_sids;
> > +   state->allowed_size = allowed_size;
> > +   state->denied_sids = denied_sids;
> > +   state->denied_size = denied_size;
> > +
> > +   ret = EOK;
> > +
> > + done:
> > +
> > +    if (ret == EOK) {
> > +        tevent_req_done(req);
> > +        tevent_req_post(req, ev);
> > +    } else {
> > +       tevent_req_error(req, ret);
> > +       tevent_req_post(req, ev);
> > +   }
> 
> You can extract tevent_req_post() and run it unconditionally after the
> if block.
> 
> > +
> > +   return req;
> > +}
> > +
> > +int ad_gpo_process_cse_recv(struct tevent_req *req,
> > +                            TALLOC_CTX *mem_ctx,
> > +                            char ***allowed_sids,
> > +                            int *allowed_size,
> > +                            char ***denied_sids,
> > +                            int *denied_size)
> > +{
> > +    struct ad_gpo_process_cse_state *state =
> > +        tevent_req_data(req, struct ad_gpo_process_cse_state);
> > +
> > +    TEVENT_REQ_RETURN_ON_ERROR(req);
> > +
> > +    *allowed_sids = state->allowed_sids;
> > +    *allowed_size = state->allowed_size;
> 
> I guess talloc_steal is missing here.

Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation. 

> 

Regards,
Yassir.



More information about the sssd-devel mailing list