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

Sumit Bose sbose at redhat.com
Wed Jun 11 20:24:28 UTC 2014


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.

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.

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

> +{
> +    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?

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

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

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?

> + */
> +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?

> +    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 :-).

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

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

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

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

> +    *denied_sids = state->denied_sids;
> +    *denied_size = state->denied_size;
> +
> +    return EOK;
> +}
> -- 
> 1.8.5.3
> 

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list