[SSSD] [PATCH] AD: LDAP component of GPO-based access control

Sumit Bose sbose at redhat.com
Fri Apr 11 10:29:33 UTC 2014


On Thu, Apr 03, 2014 at 12:30:17PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > 
> > 
> > ----- Original Message -----
> > > 
> > > 
> > > ----- Original Message -----
> > > > On (12/03/14 18:20), Yassir Elley wrote:
> > > > >Hi,
> > > > >
> > > > >The attached patch implements the LDAP component of the GPO-based access
> > > > >control project. For more details on the project, see
> > > > >https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryGPOIntegration.
> > > > >The project is not yet complete, since the SMB component still needs to
> > > > >be
> > > > >implemented. However, this seems like a useful milestone to allow others
> > > > >to
> > > > >review the code. This is my first patch submission to the list, so any
> > > > >feedback is appreciated.
> > > > >
> > > > >In order to exercise the code in your AD environment, you will need to
> > > > >link
> > > > >a GPO to the site, domain, or OUs, associated with the policy target
> > > > >(i.e.
> > > > >domain member computer). Since we are not yet retrieving any policy
> > > > >files
> > > > >(over SMB), there is no access control taking place yet, meaning that
> > > > >any
> > > > >policy settings can be used in the GPOs. The only way to determine what
> > > > >is
> > > > >going on is by examining the logs.
> > > > >
> > > > >In summary, the code currently does the following:
> > > > >1. Determines the DN of the policy target (e.g.
> > > > >"cn=f20-laptop,OU=West,OU=Sales,DC=foo,DC=com")
> > > > >2. Calculates the list of SOM DNs (Site, Domain and OUs) associated with
> > > > >the
> > > > >policy target DN
> > > > >3. Retrieves various LDAP attributes from each SOM DN in order to
> > > > >populate
> > > > >a
> > > > >list of GPLinks (i.e. GPO DN plus GPO Options)
> > > > >4. Creates a prioritized list of GPO DNs based on SOM Priority, Link
> > > > >Order,
> > > > >and GPO Options
> > > > >5. Retrieves various LDAP attributes from each GPO DN in order to
> > > > >populate
> > > > >a
> > > > >list of candidate GPOs
> > > > >6. Converts the candidate list to a filtered list, by removing any GPO
> > > > >that
> > > > >has a DACL which denies the policy target permission to apply the GPO
> > > > >7. It is this filtered list that will be sent to the Short-Lived GPO
> > > > >Child
> > > > >process, which will make the necessary blocking SMB calls
> > > > >
> > > > >Regards,
> > > > >Yassir.
> > > > >
> > > > >
> > > > 
> > > > We changed DEBUG messages recently and your patch cannot be compiled on
> > > > current
> > > > master. Probably, it happened when you rebased patches.
> > > > 
> > > > I am attaching which fixes these problems. And some (probably not all)
> > > > code
> > > > style issues: trailing whitespaces, tabs instead of spaces ...
> > > > 
> > > > Feel free to squash this patch to yours.
> > > > 
> > > > LS
> > > > 
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > 
> > > 
> > > Hmmm... I did notice the recent DEBUG message changes and I went through
> > > and
> > > fixed each one so it compiled. I must have made a mistake somewhere when
> > > merging from my local gpo branch to my local master branch. Thanks for the
> > > patch!
> > > 
> > > Regards,
> > > Yassir.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > 
> > I have attached an updated patch to address Lukas's initial comments.
> > 
> > Regards,
> > Yassir.
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > 
> 
> I have re-organized the code (and added more comments) and attached an updated patch for code review. I hope this will make it easier to review.
> 
> Regards,
> Yassir.

Hi,

I've read the patches and did some basic tests. You can find my review
comments in-line below. I had to add some NULL-checks to make my tests
work, see details below as well.

In general I think you should add an option to disable the GPO checks to
be able to resolve regressions in existing setups.

I don't know if you already have some unit tests in your tree, but there
are several functions in your patch where unit tests would be nice.

If to comments are addressed we should push the patch to master to get
more testing, coverity checks etc.

bye,
Sumit


> From a508e36e67aa881665d0387ff418162e5f1825cd Mon Sep 17 00:00:00 2001
> From: Yassir Elley <yelley at redhat.com>
> Date: Mon, 20 Jan 2014 11:17:06 -0500
> Subject: [PATCH] Implemented LDAP component of GPO-based access control
> 
> ---
>  Makefile.am                       |    3 +
>  src/providers/ad/ad_access.c      |  125 ++-
>  src/providers/ad/ad_domain_info.c |   34 +-
>  src/providers/ad/ad_domain_info.h |    1 +
>  src/providers/ad/ad_gpo.c         | 2199 +++++++++++++++++++++++++++++++++++++
>  src/providers/ad/ad_gpo.h         |   55 +
>  src/providers/ad/ad_id.c          |    3 +-
>  src/providers/ad/ad_subdomains.c  |    3 +-
>  src/providers/ldap/sdap_async.c   |  176 +++
>  src/providers/ldap/sdap_async.h   |   14 +
>  src/util/sss_ldap.h               |    9 +
>  11 files changed, 2571 insertions(+), 51 deletions(-)
>  create mode 100644 src/providers/ad/ad_gpo.c
>  create mode 100644 src/providers/ad/ad_gpo.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index f319624..2935d15 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1608,6 +1608,7 @@ ad_access_filter_tests_LDADD = \
>      libsss_ldap_common.la \
>      libsss_idmap.la \
>      libsss_krb5_common.la \
> +    libsss_ad.la \
>      libsss_test_common.la

I see

  CCLD     ad_access_filter_tests

*** Warning: Linking the executable ad_access_filter_tests against the loadable module
*** libsss_ad.so is not portable!

when running make check, I think you have to replace libsss_ad.la by the list
of source files here

...

> -    state->cindex++;
> -    if (state->clist[state->cindex] == NULL) {
> -        DEBUG(SSSDBG_OP_FAILURE,
> -            "Error retrieving access check result: %s\n",
> -            sss_strerror(ret));
> -        tevent_req_error(req, ret);
> -        return;
> +        /* If possible, retry with LDAP */
> +        state->cindex++;
> +        if (state->clist[state->cindex] == NULL) {
> +            DEBUG(SSSDBG_OP_FAILURE,
> +                  "Error retrieving access check result: %s\n",
> +                  sss_strerror(ret));
> +            tevent_req_error(req, ret);
> +            return;
> +        }
> +
> +        ret = ad_sdap_access_step(req, state->clist[state->cindex]);
> +        if (ret != EOK) {
> +            tevent_req_error(req, ret);
> +            return;
> +        }
> +
> +        /* Another check in progress */

You have to call return here because ad_sdap_access_step() will create a new
request which will return to ad_sdap_access_done().

>      }
>  
> -    ret = ad_access_step(req, state->clist[state->cindex]);
> -    if (ret != EOK) {
> -        tevent_req_error(req, ret);
> +    subreq = ad_gpo_access_send(state,
> +                                state->be_ctx->ev,
> +                                state->domain,
> +                                state->ctx,
> +                                state->pd->user);
> +
> +    if (!subreq) {
> +        tevent_req_error(req, ENOMEM);
>          return;
>      }
>  
> -    /* Another check in progress */
> +    tevent_req_set_callback(subreq, ad_gpo_access_done, req);
>  }

...

>  
>  
>  static void
> -ad_access_check_done(struct tevent_req *req)
> +ad_access_done(struct tevent_req *req)
>  {
>      errno_t ret;
>      struct be_req *breq =
>              tevent_req_callback_data(req, struct be_req);
>      struct pam_data *pd =
>                      talloc_get_type(be_req_get_data(breq), struct pam_data);
> -

Please do not remove this empty line, it separates declarations from the code.
There is no formal requirement in the coding-style but I think it helps reading
the code nonetheless.

>      ret = ad_access_recv(req);
>      talloc_zfree(req);
>      switch (ret) {
> diff --git a/src/providers/ad/ad_domain_info.c b/src/providers/ad/ad_domain_info.c
> index 9594484..d18a9c1 100644
> --- a/src/providers/ad/ad_domain_info.c
> +++ b/src/providers/ad/ad_domain_info.c
> @@ -44,7 +44,9 @@
>  static errno_t
>  netlogon_get_domain_info(TALLOC_CTX *mem_ctx,
>                           struct sysdb_attrs *reply,
> -                         char **_flat_name, char **_forest)
> +                         char **_flat_name,
> +                         char **_site,
> +                         char **_forest)
>  {
>      errno_t ret;
>      struct ldb_message_element *el;
> @@ -53,6 +55,7 @@ netlogon_get_domain_info(TALLOC_CTX *mem_ctx,
>      enum ndr_err_code ndr_err;
>      struct netlogon_samlogon_response response;
>      const char *flat_name;
> +    const char *site;
>      const char *forest;
>  
>      ret = sysdb_attrs_get_el(reply, AD_AT_NETLOGON, &el);
> @@ -129,6 +132,25 @@ netlogon_get_domain_info(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
>  
> +    /* get site name */
> +    if (response.data.nt5_ex.client_site != NULL
> +        && response.data.nt5_ex.client_site[0] != '\0') {
> +        site = response.data.nt5_ex.client_site;
> +    } else if (response.data.nt5_ex.next_closest_site != NULL
> +               && response.data.nt5_ex.next_closest_site[0] != '\0') {
> +        site = response.data.nt5_ex.next_closest_site;

I think you don't want to use the next_closest_site here. In general for a
domain member always belongs to a site, if it cannot be determined it is an
error. The next_closest_site might have completely different policy settings.
We use next_closest_site in the DNS lookup code because e.g. for request to a
trusted forest there will only be a next_closest_site.

> +    } else {
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    *_site = talloc_strdup(mem_ctx, site);
> +    if (*_site == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
>      ret = EOK;
>  done:
>      talloc_free(ndr_pull);

...

> diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> new file mode 100644
> index 0000000..5a44b24
> --- /dev/null
> +++ b/src/providers/ad/ad_gpo.c
> @@ -0,0 +1,2199 @@
> +/*
> +    SSSD
> +
> +    ad_gpo.c
> +
> +    Authors:
> +    Yassir Elley <yelley at redhat.com>
> +
> +    Copyright (C) 2013 Red Hat
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +/*
> + * This file implements the following pair of *public* functions (see header):
> + *   ad_gpo_access_send/recv: provides client-side GPO processing
> + *
> + * This file also implements the following pairs of *private* functions (which
> + * are used by the public functions):
> + *   ad_gpo_process_som_send/recv: populate list of gp_som objects
> + *   ad_gpo_process_gpo_send/recv: populate list of gp_gpo objects
> + */
> +
> +#include <security/pam_modules.h>
> +#include "util/util.h"
> +#include "providers/data_provider.h"
> +#include "providers/dp_backend.h"
> +#include "providers/ad/ad_access.h"
> +#include "providers/ad/ad_common.h"
> +#include "providers/ad/ad_domain_info.h"
> +#include "providers/ad/ad_gpo.h"
> +#include "providers/ldap/sdap_access.h"
> +#include "providers/ldap/sdap_async.h"
> +#include "providers/ldap/sdap.h"
> +#include <ndr.h>
> +#include <gen_ndr/security.h>
> +
> +#define AD_AT_DN "distinguishedName"
> +#define AD_AT_UAC "userAccountControl"
> +#define AD_AT_CONFIG_NC "configurationNamingContext"
> +#define AD_AT_GPLINK "gPLink"
> +#define AD_AT_GPOPTIONS "gpOptions"
> +#define AD_AT_NT_SEC_DESC "nTSecurityDescriptor"
> +#define AD_AT_CN "cn"
> +#define AD_AT_DISPLAY_NAME "displayName"
> +#define AD_AT_FILE_SYS_PATH "gPCFileSysPath"
> +#define AD_AT_VERSION_NUMBER "versionNumber"
> +#define AD_AT_MACHINE_EXT_NAMES "gPCMachineExtensionNames"
> +#define AD_AT_USER_EXT_NAMES "gPCUserExtensionNames"
> +#define AD_AT_FUNC_VERSION "gPCFunctionalityVersion"
> +#define AD_AT_FLAGS "flags"
> +
> +#define UAC_WORKSTATION_TRUST_ACCOUNT 0x00001000
> +#define AD_AGP_GUID "edacfd8f-ffb3-11d1-b41d-00a0c968f939"
> +#define AD_AUTHENTICATED_USERS_SID "S-1-5-11"
> +#define SID_MAX_LEN 1024
> +
> +/* == common data structures and declarations ============================= */
> +
> +struct gp_som {
> +    char *som_dn;
> +    struct gp_gplink **gplink_list;
> +    int num_gplinks;
> +};
> +
> +struct gp_gplink {
> +    char *gpo_dn;
> +    bool enforced;
> +};
> +
> +struct gp_gpo {
> +    struct security_descriptor *gpo_sd;
> +    char *gpo_dn;
> +    char *gpo_guid;
> +    char *gpo_display_name;
> +    char *gpo_file_sys_path;
> +    uint32_t gpo_container_version;
> +    char **gpo_cse_guids;
> +    int num_gpo_cse_guids;
> +    int gpo_func_version;
> +    int gpo_flags;
> +};
> +
> +enum ace_eval_status {
> +    AD_GPO_ACE_DENIED,
> +    AD_GPO_ACE_ALLOWED,
> +    AD_GPO_ACE_NEUTRAL
> +};
> +
> +struct tevent_req *ad_gpo_process_som_send(TALLOC_CTX *mem_ctx,
> +                                           struct tevent_context *ev,
> +                                           struct sdap_id_conn_ctx *conn,
> +                                           struct sdap_id_op *sdap_op,
> +                                           struct sdap_options *opts,
> +                                           int timeout,
> +                                           char *target_dn,
> +                                           char *domain_name);
> +int ad_gpo_process_som_recv(struct tevent_req *req,
> +                            TALLOC_CTX *mem_ctx,
> +                            struct gp_som ***som_list);
> +
> +struct tevent_req *ad_gpo_process_gpo_send(TALLOC_CTX *mem_ctx,
> +                                           struct tevent_context *ev,
> +                                           struct sdap_id_op *sdap_op,
> +                                           struct sdap_options *opts,
> +                                           int timeout,
> +                                           struct gp_som **som_list);
> +int ad_gpo_process_gpo_recv(struct tevent_req *req,
> +                            TALLOC_CTX *mem_ctx,
> +                            struct gp_gpo ***candidate_gpos,
> +                            int *num_candidate_gpos);
> +
> +/* == ad_gpo_access_send/recv helpers =======================================*/
> +
> +/*
> + * This function retrieves the SIDs corresponding to the input user and returns
> + * the user_sid, group_sids, and group_size in their respective output params.
> + *
> + * Note: since authentication must complete successfully before the
> + * gpo access checks are called, we can safely assume that the user/computer
> + * has been authenticated. As such, this function always adds the
> + * AD_AUTHENTICATED_USERS_SID to the group_sids.
> + */
> +static errno_t
> +ad_gpo_get_sids(TALLOC_CTX *mem_ctx,
> +                char *user,
> +                struct sss_domain_info *domain,
> +                const char **_user_sid,
> +                const char ***_group_sids,
> +                int *_group_size)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    struct ldb_result *res;
> +    int ret = 0;
> +    int i = 0;
> +    int num_group_sids = 0;
> +    const char *user_sid = NULL;
> +    const char *group_sid = NULL;
> +    const char **group_sids = NULL;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    /* first result from sysdb_initgroups is user_sid; rest are group_sids */
> +    ret = sysdb_initgroups(tmp_ctx, domain, user, &res);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_initgroups failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        return ret;
> +    }
> +
> +    user_sid = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_SID_STR, NULL);
> +    num_group_sids = (res->count) - 1;
> +
> +    /* include space for AD_AUTHENTICATED_USERS_SID and NULL */
> +    group_sids = talloc_array(tmp_ctx, const char *, num_group_sids + 1 + 1);
> +    if (group_sids == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    for (i = 0; i < num_group_sids; i++) {
> +        group_sid = ldb_msg_find_attr_as_string(res->msgs[i+1],
> +                                                SYSDB_SID_STR, NULL);
> +        if (group_sid == NULL) {
> +            continue;
> +        }
> +
> +        group_sids[i] = talloc_strdup(group_sids, group_sid);
> +        if (group_sids[i] == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +    }
> +    group_sids[i++] = AD_AUTHENTICATED_USERS_SID;

I think this is risky. In general nobody should get the idea to free the list
entries manually. But if someone does he would fail here. I think it would be
better if list members always have the same storage class so please use
talloc_strdup() here as well.

> +    group_sids[i] = NULL;
> +
> +    *_group_size = num_group_sids + 1;
> +    *_group_sids = talloc_steal(mem_ctx, group_sids);
> +    *_user_sid = talloc_strdup(mem_ctx, user_sid);
> +    return EOK;
> +
> + done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +
> +bool string_to_sid(struct dom_sid *sidout, const char *sidstr);
> +int dom_sid_string_buf(const struct dom_sid *sid, char *buf, int buflen);
> +bool dom_sid_equal(const struct dom_sid *sid1, const struct dom_sid *sid2);
> +

...

> +static void
> +ad_gpo_target_dn_retrieval_done(struct tevent_req *subreq)
> +{
> +    struct tevent_req *req;
> +    struct ad_gpo_access_state *state;
> +    int ret;
> +    int dp_error;
> +    size_t reply_count;
> +    struct sysdb_attrs **reply;
> +    const char *target_dn = NULL;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct ad_gpo_access_state);
> +    ret = sdap_get_generic_recv(subreq, state,
> +                                &reply_count, &reply);
> +    talloc_zfree(subreq);
> +    if (ret != EOK) {
> +        ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> +        /* TBD: handle (dp_error == DP_ERR_OFFLINE) case */
> +
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to get policy target's DN: [%d](%s)\n",
> +               ret, strerror(ret));
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    /* make sure there is only one non-NULL reply returned */
> +
> +    if (reply_count < 1) {
> +        DEBUG(SSSDBG_OP_FAILURE, "No DN retrieved for policy target.\n");
> +        ret = ENOENT;
> +        goto done;
> +    } else if (reply_count > 1) {
> +        DEBUG(SSSDBG_OP_FAILURE, "Multiple replies for policy target\n");
> +        ret = ERR_INTERNAL;
> +        goto done;
> +    } else if (reply == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, "reply_count is 1, but reply is NULL\n");
> +        ret = ERR_INTERNAL;
> +        goto done;
> +    }
> +
> +    /* reply[0] holds requested attributes of single reply */
> +    ret = sysdb_attrs_get_string(reply[0], AD_AT_DN, &target_dn);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_string failed: [%d](%s)\n",
> +               ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    state->target_dn = talloc_strdup(state, target_dn);
> +    if (state->target_dn == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    uint32_t uac;

Please declare variables at the beginning of the function.

> +    ret = sysdb_attrs_get_uint32_t(reply[0], AD_AT_UAC, &uac);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_uint32_t failed: [%d](%s)\n",
> +               ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    /* we only support computer policy targets, not users */
> +    if (!(uac & UAC_WORKSTATION_TRUST_ACCOUNT)) {
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    state->target_dn = talloc_strdup(state, target_dn);
> +    if (state->target_dn == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    subreq = ad_gpo_process_som_send(state,
> +                                     state->ev,
> +                                     state->conn,
> +                                     state->sdap_op,
> +                                     state->opts,
> +                                     state->timeout,
> +                                     state->target_dn,
> +                                     state->domain->name);
> +    if (subreq == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    tevent_req_set_callback(subreq, ad_gpo_process_som_done, req);
> +
> + done:
> +
> +    if (ret != EOK) {
> +        tevent_req_error(req, ret);
> +    }
> +}
> +
> +static void
> +ad_gpo_process_som_done(struct tevent_req *subreq)
> +{
> +    struct tevent_req *req;
> +    struct ad_gpo_access_state *state;
> +    int ret;
> +    struct gp_som **som_list;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct ad_gpo_access_state);
> +    ret = ad_gpo_process_som_recv(subreq, state, &som_list);
> +    talloc_zfree(subreq);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to get som list: [%d](%s)\n",
> +               ret, strerror(ret));
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    subreq = ad_gpo_process_gpo_send(state,
> +                                     state->ev,
> +                                     state->sdap_op,
> +                                     state->opts,
> +                                     state->timeout,
> +                                     som_list);
> +    if (subreq == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    tevent_req_set_callback(subreq, ad_gpo_process_gpo_done, req);
> +

Currently it is obvious that ret==EOK if we reached this point. But if some
code is added in future it might be not so obvious anymore and I would suggest
to explicitly set ret=EOK before entering the done: block or call return here.
I would expect that a compiler will optimize out the line if it is clear that
ret can only be EOK when reaching this point.

> + done:
> +
> +    if (ret != EOK) {
> +        tevent_req_error(req, ret);
> +    }
> +}
> +
> +/*
> + * 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.
> + */
> +static void
> +ad_gpo_process_gpo_done(struct tevent_req *subreq)
> +{
> +    struct tevent_req *req;
> +    struct ad_gpo_access_state *state;
> +    int ret;
> +    int dp_error;
> +    struct gp_gpo **candidate_gpos = NULL;
> +    int num_candidate_gpos = 0;
> +    int i = 0;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct ad_gpo_access_state);
> +    ret = ad_gpo_process_gpo_recv(subreq, state, &candidate_gpos,
> +                                  &num_candidate_gpos);
> +
> +    talloc_zfree(subreq);
> +
> +    ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> +
> +    if (ret != EOK) {
> +        /* TBD: handle (dp_error == DP_ERR_OFFLINE) case */
> +
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to get GPO list: [%d](%s)\n",
> +              ret, strerror(ret));
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    ret = ad_gpo_filter_gpos_by_dacl(state, state->user, state->domain,
> +                                     candidate_gpos, num_candidate_gpos,
> +                                     &state->dacl_filtered_gpos,
> +                                     &state->num_dacl_filtered_gpos);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to filter GPO list: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    if (state->dacl_filtered_gpos[0] == NULL) {
> +        DEBUG(SSSDBG_TRACE_FUNC, "dacl_filtered_gpos is empty\n");
> +    }
> +
> +    for (i = 0; i < state->num_dacl_filtered_gpos; i++) {
> +        DEBUG(SSSDBG_TRACE_FUNC, "dacl_filtered_gpos[%d]->gpo_guid is %s\n", i,
> +              state->dacl_filtered_gpos[i]->gpo_guid);
> +    }
> +
> +    /* TBD: initiate SMB retrieval */
> +    DEBUG(SSSDBG_TRACE_FUNC, "time for SMB retrieval\n");
> +

see above

> + done:
> +
> +    if (ret == EOK) {
> +        tevent_req_done(req);
> +    } else if (ret != EAGAIN) {
> +        tevent_req_error(req, ret);
> +    }
> +}
> +
> +errno_t
> +ad_gpo_access_recv(struct tevent_req *req)
> +{
> +    TEVENT_REQ_RETURN_ON_ERROR(req);
> +
> +    return EOK;
> +}
> +
> +/* == ad_gpo_process_som_send/recv helpers ================================= */
> +
> +/*
> + * This function returns the parent of an LDAP DN
> + */
> +static char *ad_gpo_parent_dn(const char *dn)
> +{
> +    char *p;
> +
> +    if (dn == NULL) {
> +        return NULL;
> +    }
> +
> +    p = strchr(dn, ',');
> +    if (p == NULL) {
> +        return NULL;
> +    }

I would suggest the use calls from OpenLDAP or ldb (there is
ldb_dn_get_parent() maybe to already does the right thing) to properly split
the DN in components and created the parent DN. Your way would fail e.g. if the
RDN contains an escaped comma "\,".

> +
> +    return p+1;
> +}
> +
> +/*
> + * This function populates the _som_list output parameter by parsing the input
> + * DN into a list of gp_som objects. This function essentially repeatedly
> + * appends the input DN's parent to the SOM List (if the parent starts with
> + * "OU=" or "DC="), until the first "DC=" component is reached.
> + * Example: if input DN is "CN=MyComputer,CN=Computers,OU=Sales,DC=FOO,DC=COM",
> + * then SOM List has 2 SOM entries: {[OU=Sales,DC=FOO,DC=COM], [DC=FOO, DC=COM]}
> + */
> +
> +static errno_t
> +ad_gpo_populate_som_list(TALLOC_CTX *mem_ctx,
> +                         char *target_dn,
> +                         int *_num_soms,
> +                         struct gp_som ***_som_list)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    int ret;
> +    int rdn_count = 0;
> +    int som_idx = 0;
> +    struct gp_som **som_list;
> +    char *parent_dn = NULL;
> +    char *tmp_dn = NULL;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    tmp_dn = target_dn;
> +    while ((parent_dn = ad_gpo_parent_dn(tmp_dn))) {
> +        rdn_count++;
> +        tmp_dn = parent_dn;
> +    }

There is ldb_dn_get_comp_num() which should return the number of components in
a save way.

> +
> +    if (rdn_count == 0) {
> +        *_som_list = NULL;
> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    /* assume the worst-case, in which every parent is a SOM */
> +    /* include space for Site SOM and NULL: rdn_count + 1 + 1 */
> +    som_list = talloc_array(tmp_ctx, struct gp_som *, rdn_count + 1 + 1);
> +    if (som_list == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    /* first, populate the OU and Domain SOMs */
> +    tmp_dn = target_dn;
> +    while ((parent_dn = ad_gpo_parent_dn(tmp_dn))) {
> +
> +        if ((strncasecmp(parent_dn, "OU=", strlen("OU=")) == 0) ||
> +            (strncasecmp(parent_dn, "DC=", strlen("DC=")) == 0)) {
> +
> +            som_list[som_idx] = talloc_zero(som_list, struct gp_som);
> +            if (som_list[som_idx] == NULL) {
> +                ret = ENOMEM;
> +                goto done;
> +            }
> +            som_list[som_idx]->som_dn = talloc_strdup(som_list[som_idx],
> +                                                      parent_dn);
> +            if (som_list[som_idx]->som_dn == NULL) {
> +                ret = ENOMEM;
> +                goto done;
> +            }
> +            som_idx++;
> +        }
> +
> +        if (strncasecmp(parent_dn, "DC=", strlen("DC=")) == 0) {
> +            break;
> +        }
> +        tmp_dn = parent_dn;
> +    }
> +
> +    som_list[som_idx] = NULL;
> +
> +    *_num_soms = som_idx;
> +    *_som_list = talloc_steal(mem_ctx, som_list);
> +
> +    ret = EOK;
> +
> + done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +
> +/*
> + * This function populates the _gplink_list output parameter by parsing the
> + * input raw_gplink_value into an array of gp_gplink objects, each consisting of
> + * a GPO DN and bool enforced field.
> + *
> + * The raw_gplink_value is single string consisting of multiple gplink strings.
> + * The raw_gplink_value is in the following format:
> + *  "[GPO_DN_1;GPLinkOptions_1]...[GPO_DN_n;GPLinkOptions_n]"
> + *
> + * Each gplink string consists of a GPO DN and a GPLinkOptions field (which
> + * indicates whether its associated GPO DN is ignored, unenforced, or enforced).
> + * If a GPO DN is flagged as ignored, it is discarded and will not be added to
> + * the _gplink_list. If the allow_enforced_only input is true, AND a GPO DN is
> + * flagged as unenforced, it will also be discarded.
> + *
> + * Example: if raw_gplink_value="[OU=Sales,DC=FOO,DC=COM;0][DC=FOO,DC=COM;2]"
> + *   and allow_enforced_only=FALSE, then the output would consist of following:
> + *    _gplink_list[0]: {GPO DN: "OU=Sales,DC=FOO,DC=COM", enforced: FALSE}
> + *    _gplink_list[1]: {GPO DN: "DC=FOO,DC=COM",          enforced: TRUE}
> + */
> +static errno_t
> +ad_gpo_populate_gplink_list(TALLOC_CTX *mem_ctx,
> +                            char *som_dn,
> +                            char *raw_gplink_value,
> +                            struct gp_gplink ***_gplink_list,
> +                            bool allow_enforced_only)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    char *ptr;
> +    char *first;
> +    char *last;
> +    char *dn;
> +    char *gplink_options;
> +    const char delim = ']';
> +    struct gp_gplink **gplink_list;
> +    int i;
> +    int ret;
> +    int gplink_number;
> +    int gplink_count = 0;
> +    int num_enabled = 0;
> +
> +    if (raw_gplink_value == NULL ||
> +        *raw_gplink_value == '\0' ||
> +        _gplink_list == NULL) {
> +        return EINVAL;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "som_dn: %s\n", som_dn);
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ptr = raw_gplink_value;
> +
> +    while ((ptr = strchr(ptr, delim))) {
> +        if (ptr == NULL) break;
> +        ptr++;
> +        gplink_count++;
> +    }
> +
> +    if (gplink_count == 0) {
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    gplink_list = talloc_array(tmp_ctx, struct gp_gplink *, gplink_count + 1);
> +    if (gplink_list == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    num_enabled = 0;
> +    ptr = raw_gplink_value;
> +    for (i = 0; i < gplink_count; i++) {
> +        first = ptr + 1;
> +        last = strchr(first, delim);
> +        if (last == NULL) {
> +            break;
> +        }
> +        *last = '\0';
> +        last++;
> +        dn = first;
> +        if ( strncasecmp(dn, "LDAP://", 7)== 0 ) {
> +            dn = dn + 7;
> +        }

are you sure this can only be LDAP:// ? I wonder if it can be LDAPS:// is the
original request is using LDAPS? But since AD does not activate LDAPS by
default it might be safe.

> +        gplink_options = strchr(first, ';');
> +        if (gplink_options == NULL) {
> +            break;
> +        }
> +        *gplink_options = '\0';
> +        gplink_options++;
> +
> +        gplink_number = atoi(gplink_options);

In general we prefer strto* for better error checking. I assume that
gplink_options is expected to be a 32bit value. If this is true you might want
to use strtouint32(). In this case it might be better to declare gplink_options
as uint32t instead of int?

> +        DEBUG(SSSDBG_TRACE_ALL,
> +              "gplink_list[%d]: [%s; %d]\n", num_enabled, dn, gplink_number);
> +
> +        if ((gplink_number == 1) || (gplink_number ==3)) {
> +            /* ignore flag is set */
> +            DEBUG(SSSDBG_TRACE_ALL, "ignored gpo skipped\n");
> +            ptr = last;
> +            continue;
> +        }
> +
> +        if (allow_enforced_only && (gplink_number == 0)) {
> +            /* unenforced flag is set; only enforced gpos allowed */
> +            DEBUG(SSSDBG_TRACE_ALL, "unenforced gpo skipped\n");
> +            ptr = last;
> +            continue;
> +        }
> +
> +        gplink_list[num_enabled] = talloc_zero(gplink_list, struct gp_gplink);
> +        if (gplink_list[num_enabled] == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +        gplink_list[num_enabled]->gpo_dn =
> +            talloc_strdup(gplink_list[num_enabled], dn);
> +
> +        if (gplink_list[num_enabled]->gpo_dn == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        if (gplink_number == 0) {
> +            gplink_list[num_enabled]->enforced = 0;
> +            num_enabled++;
> +        } else if (gplink_number == 2) {
> +            gplink_list[num_enabled]->enforced = 1;
> +            num_enabled++;
> +        } else {
> +            ret = EINVAL;
> +            goto done;
> +        }
> +
> +        ptr = last;
> +    }
> +    gplink_list[num_enabled] = NULL;
> +
> +    *_gplink_list = talloc_steal(mem_ctx, gplink_list);
> +    ret = EOK;
> +
> + done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +

...

> +static void
> +ad_gpo_get_som_attrs_done(struct tevent_req *subreq)
> +{
> +    struct tevent_req *req;
> +    struct ad_gpo_process_som_state *state;
> +    int ret;
> +    int dp_error;
> +    size_t num_results;
> +    struct sysdb_attrs **results;
> +    struct ldb_message_element *el = NULL;
> +    uint8_t *raw_gplink_value;
> +    uint8_t *raw_gpoptions_value;
> +    int allow_enforced_only = 0;
> +    struct gp_som *gp_som;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct ad_gpo_process_som_state);
> +    ret = sdap_get_generic_recv(subreq, state,
> +                                &num_results, &results);
> +    talloc_zfree(subreq);
> +
> +    if (ret != EOK) {
> +        ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> +        /* TBD: handle (dp_error == DP_ERR_OFFLINE) case */
> +
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to get SOM attributes: [%d](%s)\n",
> +              ret, strerror(ret));
> +        ret = ENOENT;
> +        goto done;
> +    }
> +    if ((num_results < 1) || (results == NULL)) {
> +        DEBUG(SSSDBG_OP_FAILURE, "no attrs found for SOM; try next SOM.\n");
> +        ret = ad_gpo_get_som_attrs_step(req);
> +        goto done;
> +    } else if (num_results > 1) {
> +        DEBUG(SSSDBG_OP_FAILURE, "Received multiple replies\n");
> +        ret = ERR_INTERNAL;
> +        goto done;
> +    }
> +
> +    /* Get the gplink value, if available */
> +    ret = sysdb_attrs_get_el(results[0], AD_AT_GPLINK, &el);
> +
> +    if (ret != EOK && ret != ENOENT) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_el() failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    if ((ret == ENOENT) || (el->num_values == 0)) {
> +        DEBUG(SSSDBG_OP_FAILURE, "no attrs found for SOM; try next SOM\n");
> +        ret = ad_gpo_get_som_attrs_step(req);
> +        goto done;
> +    }
> +
> +    raw_gplink_value = el[0].values[0].data;
> +
> +    ret = sysdb_attrs_get_el(results[0], AD_AT_GPOPTIONS, &el);
> +
> +    if (ret != EOK && ret != ENOENT) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_el() failed\n");
> +        goto done;
> +    }
> +
> +    if ((ret == ENOENT) || (el->num_values == 0)) {
> +        DEBUG(SSSDBG_TRACE_ALL,
> +              "gpoptions attr not found or has no value; defaults to 0\n");
> +        allow_enforced_only = 0;
> +    }  else {
> +        raw_gpoptions_value = el[0].values[0].data;
> +        allow_enforced_only = atoi((char *)raw_gpoptions_value);

see above about atoi();

> +    }
> +
> +    gp_som = state->som_list[state->som_index];
> +    ret = ad_gpo_populate_gplink_list(gp_som,
> +                                      gp_som->som_dn,
> +                                      (char *)raw_gplink_value,
> +                                      &gp_som->gplink_list,
> +                                      state->allow_enforced_only);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "ad_gpo_populate_gplink_list() failed\n");
> +        goto done;
> +    }
> +
> +    if (allow_enforced_only) {
> +        state->allow_enforced_only = 1;
> +    }
> +
> +    ret = ad_gpo_get_som_attrs_step(req);
> +
> + done:
> +
> +    if (ret == EOK) {
> +        tevent_req_done(req);
> +    } else if (ret != EAGAIN) {
> +        tevent_req_error(req, ret);
> +    }
> +}
> +
> +int
> +ad_gpo_process_som_recv(struct tevent_req *req,
> +                        TALLOC_CTX *mem_ctx,
> +                        struct gp_som ***som_list)
> +{
> +
> +    struct ad_gpo_process_som_state *state =
> +        tevent_req_data(req, struct ad_gpo_process_som_state);
> +
> +    TEVENT_REQ_RETURN_ON_ERROR(req);
> +    *som_list = talloc_steal(mem_ctx, state->som_list);
> +    return EOK;
> +}
> +
> +/* == ad_gpo_process_gpo_send/recv helpers ================================= */
> +
> +/*
> + * This function examines the gp_gplink objects in each gp_som object specified
> + * in the input som_list, and populates the _candidate_gpos output parameter's
> + * gpo_dn fields with prioritized list of GPO DNs. Prioritization ensures that:
> + * - GPOs linked to an OU will be applied after GPOs linked to a Domain,
> + *   which will be applied after GPOs linked to a Site.
> + * - multiple GPOs linked to a single SOM are applied in their link order
> + *   (i.e. 1st GPO linked to SOM is applied after 2nd GPO linked to SOM, etc).
> + * - enforced GPOs are applied after unenforced GPOs.
> + *
> + * As such, the _candidate_gpos output's dn fields looks like (in link order):
> + * [unenforced {Site, Domain, OU}; enforced {Site, Domain, OU}]
> + *
> + * Note that in the case of conflicting policy settings, GPOs appearing later
> + * in the list will trump GPOs appearing earlier in the list.
> + */
> +static errno_t
> +ad_gpo_populate_candidate_gpos(TALLOC_CTX *mem_ctx,
> +                               struct gp_som **som_list,
> +                               struct gp_gpo ***_candidate_gpos,
> +                               int *_num_candidate_gpos)
> +{
> +
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    struct gp_som *gp_som = NULL;
> +    struct gp_gplink *gp_gplink = NULL;
> +    struct gp_gpo **candidate_gpos = NULL;
> +    int num_candidate_gpos = 0;
> +    char **enforced_gpo_dns = NULL;
> +    char **unenforced_gpo_dns = NULL;
> +    int gpo_dn_idx = 0;
> +    int num_enforced = 0;
> +    int enforced_idx = 0;
> +    int num_unenforced = 0;
> +    int unenforced_idx = 0;
> +    int i = 0;
> +    int j = 0;
> +    int ret;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    while (som_list[i]) {
> +        gp_som = som_list[i];
> +        j = 0;
> +        while (gp_som->gplink_list[j]) {

I had to change this to

           while (gp_som && gp_som->gplink_list && gp_som->gplink_list[j]) {

to make my basic tests work. Maybe it would be good to add such a NULL check to
the som_list while loop as well?

> +            gp_gplink = gp_som->gplink_list[j];
> +            if (gp_gplink == NULL) {
> +                DEBUG(SSSDBG_OP_FAILURE, "unexpected null gp_gplink\n");
> +                ret = EINVAL;
> +                goto done;
> +            }
> +            if (gp_gplink->enforced) {
> +                num_enforced++;
> +            } else {
> +                num_unenforced++;
> +            }
> +            j++;
> +        }
> +        i++;
> +    }
> +
> +    num_candidate_gpos = num_enforced + num_unenforced;
> +
> +    if (num_candidate_gpos == 0) {
> +        *_candidate_gpos = NULL;
> +        *_num_candidate_gpos = 0;
> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    enforced_gpo_dns = talloc_array(tmp_ctx, char *, num_enforced + 1);
> +    if (enforced_gpo_dns == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    unenforced_gpo_dns = talloc_array(tmp_ctx, char *, num_unenforced + 1);
> +    if (unenforced_gpo_dns == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    i = 0;
> +    while (som_list[i]) {
> +        gp_som = som_list[i];
> +        j = 0;
> +        while (gp_som->gplink_list[j]) {

see above

> +            gp_gplink = gp_som->gplink_list[j];
> +            if (gp_gplink == NULL) {
> +                DEBUG(SSSDBG_OP_FAILURE, "unexpected null gp_gplink\n");
> +                ret = EINVAL;
> +                goto done;
> +            }
> +
> +            if (gp_gplink->enforced) {
> +                enforced_gpo_dns[enforced_idx] =
> +                    talloc_strdup(enforced_gpo_dns, gp_gplink->gpo_dn);
> +                if (enforced_gpo_dns[enforced_idx] == NULL) {
> +                    ret = ENOMEM;
> +                    goto done;
> +                }
> +                enforced_idx++;
> +            } else {
> +
> +                unenforced_gpo_dns[unenforced_idx] =
> +                    talloc_strdup(unenforced_gpo_dns, gp_gplink->gpo_dn);
> +
> +                if (unenforced_gpo_dns[unenforced_idx] == NULL) {
> +                    ret = ENOMEM;
> +                    goto done;
> +                }
> +                unenforced_idx++;
> +            }
> +            j++;
> +        }
> +        i++;
> +    }
> +    enforced_gpo_dns[num_enforced] = NULL;
> +    unenforced_gpo_dns[num_unenforced] = NULL;
> +
> +    candidate_gpos = talloc_array(tmp_ctx,
> +                                  struct gp_gpo *,
> +                                  num_candidate_gpos + 1);
> +
> +    if (candidate_gpos == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    gpo_dn_idx = 0;
> +    for (i = num_unenforced - 1; i >= 0; i--) {
> +        candidate_gpos[gpo_dn_idx] = talloc_zero(candidate_gpos, struct gp_gpo);
> +        if (candidate_gpos[gpo_dn_idx] == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +        candidate_gpos[gpo_dn_idx]->gpo_dn =
> +            talloc_strdup(candidate_gpos[gpo_dn_idx], unenforced_gpo_dns[i]);
> +

Is there a reason for adding the unenforced_gpo_dns in reverse order compared
to enforced_gpo_dns?

> +        if (candidate_gpos[gpo_dn_idx]->gpo_dn == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              "candidate_gpos[%d]->gpo_dn: %s\n",
> +              gpo_dn_idx, candidate_gpos[gpo_dn_idx]->gpo_dn);
> +        gpo_dn_idx++;
> +    }
> +
> +    for (i = 0; i < num_enforced; i++) {
> +
> +        candidate_gpos[gpo_dn_idx] = talloc_zero(candidate_gpos, struct gp_gpo);
> +        if (candidate_gpos[gpo_dn_idx] == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        candidate_gpos[gpo_dn_idx]->gpo_dn =
> +            talloc_strdup(candidate_gpos[gpo_dn_idx], enforced_gpo_dns[i]);
> +        if (candidate_gpos[gpo_dn_idx]->gpo_dn == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              "candidate_gpos[%d]->gpo_dn: %s\n",
> +              gpo_dn_idx, candidate_gpos[gpo_dn_idx]->gpo_dn);
> +        gpo_dn_idx++;
> +    }
> +
> +    candidate_gpos[gpo_dn_idx] = NULL;
> +
> +    *_candidate_gpos = talloc_steal(mem_ctx, candidate_gpos);
> +    *_num_candidate_gpos = num_candidate_gpos;
> +
> +    ret = EOK;
> +
> + done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +

...

> +/*
> + * This function uses the input som_list to populate a prioritized list of
> + * gp_gpo objects, prioritized based on SOM type, link order, and whether the
> + * GPO is "enforced". This list represents the initial set of candidate GPOs
> + * that might be applicable to the target. This list can not be expanded, but
> + * it might be reduced based on subsequent filtering steps. The GPO object DNs
> + * are used to retrieve certain LDAP attributes of each GPO object, that are
> + * parsed into the various fields of the gp_gpo object.
> + */
> +struct tevent_req *
> +ad_gpo_process_gpo_send(TALLOC_CTX *mem_ctx,
> +                        struct tevent_context *ev,
> +                        struct sdap_id_op *sdap_op,
> +                        struct sdap_options *opts,
> +                        int timeout,
> +                        struct gp_som **som_list)
> +{
> +    struct tevent_req *req;
> +    struct ad_gpo_process_gpo_state *state;
> +    errno_t ret;
> +
> +    req = tevent_req_create(mem_ctx, &state, struct ad_gpo_process_gpo_state);
> +    if (req == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n");
> +        return NULL;
> +    }
> +
> +    state->ev = ev;
> +    state->sdap_op = sdap_op;
> +    state->opts = opts;
> +    state->timeout = timeout;
> +    state->gpo_index = -1;
> +    state->candidate_gpos = NULL;
> +    state->num_candidate_gpos = 0;
> +
> +    ret = ad_gpo_populate_candidate_gpos(state,
> +                                         som_list,
> +                                         &state->candidate_gpos,
> +                                         &state->num_candidate_gpos);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to retrieve GPO List: [%d](%s)\n",
> +              ret, strerror(ret));
> +        ret = ENOENT;
> +        goto immediately;
> +    }
> +
> +    if (state->candidate_gpos == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, "no gpos found\n");
> +        goto immediately;

I think you should set ret here, otherwise it would be EOK which might mot be
what you want.

> +    }
> +
> +    ret = ad_gpo_get_gpo_attrs_step(req);
> +
> +immediately:
> +
> +    if (ret == EOK) {
> +        tevent_req_done(req);
> +        tevent_req_post(req, ev);
> +    } else if (ret != EAGAIN) {
> +        tevent_req_error(req, ret);
> +        tevent_req_post(req, ev);
> +    }
> +
> +    return req;
> +}
> +
> +static errno_t
> +ad_gpo_get_gpo_attrs_step(struct tevent_req *req)
> +{
> +    const char *attrs[] = {AD_AT_NT_SEC_DESC, AD_AT_CN, AD_AT_DISPLAY_NAME,
> +                           AD_AT_FILE_SYS_PATH, AD_AT_VERSION_NUMBER,
> +                           AD_AT_MACHINE_EXT_NAMES, AD_AT_FUNC_VERSION,
> +                           AD_AT_FLAGS, NULL};
> +    struct tevent_req *subreq;
> +    struct ad_gpo_process_gpo_state *state;
> +
> +    state = tevent_req_data(req, struct ad_gpo_process_gpo_state);
> +
> +    state->gpo_index++;
> +    struct gp_gpo *gp_gpo = state->candidate_gpos[state->gpo_index];
> +
> +    /* gp_gpo is NULL only after all GPOs have been processed */
> +    if (gp_gpo == NULL) return EOK;
> +
> +    char *gpo_dn = gp_gpo->gpo_dn;
> +
> +    subreq = sdap_sd_search_send(state, state->ev,
> +                                 state->opts, sdap_id_op_handle(state->sdap_op),
> +                                 gpo_dn, SECINFO_DACL, attrs, state->timeout);
> +
> +    if (subreq == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sdap_get_generic_send failed.\n");
> +        return ENOMEM;
> +    }
> +
> +    tevent_req_set_callback(subreq, ad_gpo_get_gpo_attrs_done, req);
> +    return EAGAIN;
> +}
> +
> +static void
> +ad_gpo_get_gpo_attrs_done(struct tevent_req *subreq)
> +{
> +    struct tevent_req *req;
> +    struct ad_gpo_process_gpo_state *state;
> +    int ret;
> +    int dp_error;
> +    size_t num_results;
> +    struct sysdb_attrs **results;
> +    struct ldb_message_element *el = NULL;
> +    const char *gpo_guid = NULL;
> +    char *smb_uri = NULL;
> +
> +    req = tevent_req_callback_data(subreq, struct tevent_req);
> +    state = tevent_req_data(req, struct ad_gpo_process_gpo_state);
> +
> +    ret = sdap_sd_search_recv(subreq, state, &num_results, &results);
> +    talloc_zfree(subreq);
> +
> +    if (ret != EOK) {
> +        ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> +        /* TBD: handle (dp_error == DP_ERR_OFFLINE) case */
> +
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "Unable to get GPO attributes: [%d](%s)\n",
> +              ret, strerror(ret));
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    if ((num_results < 1) || (results == NULL)) {
> +        DEBUG(SSSDBG_OP_FAILURE, "no attrs found for GPO; try next GPO.\n");
> +        ret = ad_gpo_get_gpo_attrs_step(req);
> +        goto done;
> +    }
> +    else if (num_results > 1) {
> +        DEBUG(SSSDBG_OP_FAILURE, "Received multiple replies\n");
> +        ret = ERR_INTERNAL;
> +        goto done;
> +    }
> +
> +    struct gp_gpo *gp_gpo = state->candidate_gpos[state->gpo_index];
> +
> +    /* retrieve AD_AT_CN */
> +    ret = sysdb_attrs_get_string(results[0], AD_AT_CN, &gpo_guid);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_string failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    gp_gpo->gpo_guid = talloc_strdup(gp_gpo, gpo_guid);
> +    if (gp_gpo->gpo_guid == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "populating attrs for gpo_guid: %s\n",
> +          gp_gpo->gpo_guid);
> +
> +    /* retrieve AD_AT_DISPLAY_NAME */
> +    const char *gpo_display_name = NULL;

Please move the declaration to the beginning of the function.

> +    ret = sysdb_attrs_get_string(results[0], AD_AT_DISPLAY_NAME,
> +                                 &gpo_display_name);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_string failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    gp_gpo->gpo_display_name = talloc_strdup(gp_gpo, gpo_display_name);
> +    if (gp_gpo->gpo_display_name == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "gpo_display_name: %s\n",
> +                            gp_gpo->gpo_display_name);
> +
> +    /* retrieve AD_AT_FILE_SYS_PATH */
> +    const char *raw_file_sys_path = NULL;

see above

> +    ret = sysdb_attrs_get_string(results[0],
> +                                 AD_AT_FILE_SYS_PATH,
> +                                 &raw_file_sys_path);
> +
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_string failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    char *file_sys_path = talloc_strdup(gp_gpo, raw_file_sys_path);

see above

> +    ad_gpo_convert_to_smb_uri(state, file_sys_path, &smb_uri);
> +
> +    gp_gpo->gpo_file_sys_path = talloc_asprintf(gp_gpo, "%s/Machine",
> +                                                smb_uri);
> +    if (gp_gpo->gpo_file_sys_path == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "gpo_file_sys_path: %s\n",
> +          gp_gpo->gpo_file_sys_path);
> +
> +    /* retrieve AD_AT_VERSION_NUMBER */
> +    ret = sysdb_attrs_get_uint32_t(results[0], AD_AT_VERSION_NUMBER,
> +                                   &gp_gpo->gpo_container_version);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_uint32_t failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "gpo_container_version: %d\n",
> +                            gp_gpo->gpo_container_version);
> +
> +    /* retrieve AD_AT_MACHINE_EXT_NAMES */
> +    ret = sysdb_attrs_get_el(results[0], AD_AT_MACHINE_EXT_NAMES, &el);
> +    if (ret != EOK && ret != ENOENT) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_el() failed\n");
> +        goto done;
> +    }
> +    if ((ret == ENOENT) || (el->num_values == 0)) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "machine_ext_names not found or has no value\n");
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    uint8_t *raw_machine_ext_names = el[0].values[0].data;

see above

> +
> +    ret = ad_gpo_parse_machine_ext_names(gp_gpo,
> +                                         (char *)raw_machine_ext_names,
> +                                         &gp_gpo->gpo_cse_guids,
> +                                         &gp_gpo->num_gpo_cse_guids);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "ad_gpo_parse_machine_ext_names() failed\n");
> +        goto done;
> +    }
> +
> +    /* retrieve AD_AT_FUNC_VERSION */
> +    ret = sysdb_attrs_get_int32_t(results[0], AD_AT_FUNC_VERSION,
> +                                  &gp_gpo->gpo_func_version);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_int32_t failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "gpo_func_version: %d\n",
> +                            gp_gpo->gpo_func_version);
> +
> +    /* retrieve AD_AT_FLAGS */
> +    ret = sysdb_attrs_get_int32_t(results[0], AD_AT_FLAGS,
> +                                  &gp_gpo->gpo_flags);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "sysdb_attrs_get_int32_t failed: [%d](%s)\n",
> +              ret, strerror(ret));
> +        goto done;
> +    }
> +
> +    DEBUG(SSSDBG_TRACE_ALL, "gpo_flags: %d\n", gp_gpo->gpo_flags);
> +
> +    /* retrieve AD_AT_NT_SEC_DESC */
> +    ret = sysdb_attrs_get_el(results[0], AD_AT_NT_SEC_DESC, &el);
> +    if (ret != EOK && ret != ENOENT) {
> +        DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_el() failed\n");
> +        goto done;
> +    }
> +    if ((ret == ENOENT) || (el->num_values == 0)) {
> +        DEBUG(SSSDBG_OP_FAILURE,
> +              "nt_sec_desc attribute not found or has no value\n");
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    ret = ad_gpo_parse_sd(gp_gpo, el[0].values[0].data, el[0].values[0].length,
> +                          &gp_gpo->gpo_sd);
> +
> +    ret = ad_gpo_get_gpo_attrs_step(req);
> +
> + done:
> +
> +    if (ret == EOK) {
> +        tevent_req_done(req);
> +    } else if (ret != EAGAIN) {
> +        tevent_req_error(req, ret);
> +    }
> +}
> +
> +int
> +ad_gpo_process_gpo_recv(struct tevent_req *req,
> +                        TALLOC_CTX *mem_ctx,
> +                        struct gp_gpo ***candidate_gpos,
> +                        int *num_candidate_gpos)
> +{
> +    struct ad_gpo_process_gpo_state *state =
> +        tevent_req_data(req, struct ad_gpo_process_gpo_state);
> +
> +    TEVENT_REQ_RETURN_ON_ERROR(req);
> +
> +    *candidate_gpos = talloc_steal(mem_ctx, state->candidate_gpos);
> +    *num_candidate_gpos = state->num_candidate_gpos;
> +    return EOK;
> +}
> diff --git a/src/providers/ad/ad_gpo.h b/src/providers/ad/ad_gpo.h
> new file mode 100644
> index 0000000..99baf3c
> --- /dev/null
> +++ b/src/providers/ad/ad_gpo.h
> @@ -0,0 +1,55 @@
> +/*
> +    SSSD
> +
> +    Authors:
> +        Stephen Gallagher <sgallagh at redhat.com>
> +
> +    Copyright (C) 2012 Red Hat

I always knew that Stephen is ahead of time :-)

> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifndef AD_GPO_H_
> +#define AD_GPO_H_
> +
> +/*
> + * This pair of functions provides client-side GPO processing.
> + *
> + * While a GPO can target both user and computer objects, this
> + * implementation only supports targetting of computer objects.
> + *
> + * A GPO overview is at https://fedorahosted.org/sssd/wiki/GpoOverview
> + *
> + * In summary, client-side processing involves:
> + * - determining the target's DN
> + * - extracting the SOM object DNs (i.e. OUs and Domain) from target's DN
> + * - including the target's Site as another SOM object
> + * - determining which GPOs apply to the target's SOMs
> + * - prioritizing GPOs based on SOM, link order, and whether GPO is "enforced"
> + * - retrieving the corresponding GPO objects
> + * - sending the GPO DNs to the CSE processing engine for policy application
> + * - policy application currently consists of HBAC-like functionality
> + */
> +struct tevent_req *
> +ad_gpo_access_send(TALLOC_CTX *mem_ctx,
> +                       struct tevent_context *ev,
> +                       struct sss_domain_info *domain,
> +                       struct ad_access_ctx *ctx,
> +                       char *user);
> +
> +errno_t ad_gpo_access_recv(struct tevent_req *req);
> +
> +struct security_descriptor;
> +
> +#endif /* AD_GPO_H_ */
> diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
> index 3425b35..6af073d 100644
> --- a/src/providers/ad/ad_id.c
> +++ b/src/providers/ad/ad_id.c
> @@ -555,10 +555,11 @@ ad_enumeration_master_done(struct tevent_req *subreq)
>                                                  struct ad_enumeration_state);
>      char *flat_name;
>      char *master_sid;
> +    char *site;
>      char *forest;
>  
>      ret = ad_master_domain_recv(subreq, state,
> -                                &flat_name, &master_sid, &forest);
> +                                &flat_name, &master_sid, &site, &forest);

I just realize that I could have made this comment earlier but maybe it would
be better to allow NULL arguments in ad_master_domain_recv() for values which
are not needed by the caller instead of creating useless variables?

>      talloc_zfree(subreq);
>      if (ret != EOK) {
>          DEBUG(SSSDBG_OP_FAILURE, "Cannot retrieve master domain info\n");
> diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c
> index b191f73..cda6012 100644
> --- a/src/providers/ad/ad_subdomains.c
> +++ b/src/providers/ad/ad_subdomains.c
> @@ -92,6 +92,7 @@ struct ad_subdomains_req_ctx {
>  
>      char *master_sid;
>      char *flat_name;
> +    char *site;
>      char *forest;
>  };
>  
> @@ -572,7 +573,7 @@ static void ad_subdomains_master_dom_done(struct tevent_req *req)
>  
>      ret = ad_master_domain_recv(req, ctx,
>                                  &ctx->flat_name, &ctx->master_sid,
> -                                &ctx->forest);
> +                                &ctx->site, &ctx->forest);
>      talloc_zfree(req);
>      if (ret != EOK) {
>          DEBUG(SSSDBG_OP_FAILURE, "Cannot retrieve master domain info\n");
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index a3da4ac..5058f8b 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -19,6 +19,7 @@
>      along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  */
>  
> +
>  #include <ctype.h>
>  #include "util/util.h"
>  #include "util/strtonum.h"
> @@ -1829,6 +1830,181 @@ sdap_x_deref_search_recv(struct tevent_req *req,
>      return EOK;
>  }
>  
> +/* ==Security Descriptor (ACL) search=================================== */
> +struct sdap_sd_search_state {
> +  LDAPControl **ctrls;
> +  struct sdap_options *opts;
> +  size_t reply_count;
> +  struct sysdb_attrs **reply;
> +  struct sdap_reply sreply;
> +};
> +
> +static int sdap_sd_search_create_control(struct sdap_handle *sh,
> +					 int val,
> +					 LDAPControl **ctrl);
> +static int sdap_sd_search_ctrls_destructor(void *ptr);
> +static errno_t sdap_sd_search_parse_entry(struct sdap_handle *sh,
> +                                          struct sdap_msg *msg,
> +                                          void *pvt);
> +static void sdap_sd_search_done(struct tevent_req *subreq);
> +
> +struct tevent_req *
> +sdap_sd_search_send(TALLOC_CTX *memctx, struct tevent_context *ev,
> +                     struct sdap_options *opts, struct sdap_handle *sh,
> +                     const char *base_dn, int sd_flags,
> +                     const char **attrs, int timeout)
> +{
> +    struct tevent_req *req = NULL;
> +    struct tevent_req *subreq = NULL;
> +    struct sdap_sd_search_state *state;
> +    int ret;
> +
> +    req = tevent_req_create(memctx, &state, struct sdap_sd_search_state);
> +    if (!req) return NULL;
> +
> +    state->ctrls = talloc_zero_array(state, LDAPControl *, 2);
> +    state->opts = opts;
> +    if (state->ctrls == NULL) {
> +        talloc_zfree(req);
> +        return NULL;

Please use the convention used in other *_send() calls as well. Only return
NULL if tevent_req_create() failed. For all errors call tevent_req_error() and
tevent_req_post() and return req to the caller.

> +    }
> +    talloc_set_destructor((TALLOC_CTX *) state->ctrls,
> +                          sdap_sd_search_ctrls_destructor);
> +
> +    ret = sdap_sd_search_create_control(sh, sd_flags, &state->ctrls[0]);
> +    if (ret != EOK) {
> +        talloc_zfree(req);
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create SD control\n");
> +        return NULL;

see above

> +    }
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "Searching entry [%s] using SD\n", base_dn);
> +    subreq = sdap_get_generic_ext_send(state, ev, opts, sh, base_dn,
> +                                       LDAP_SCOPE_BASE, "(objectclass=*)", attrs,
> +                                       false, state->ctrls, NULL, 0, timeout,
> +                                       true, sdap_sd_search_parse_entry,
> +                                       state);
> +    if (!subreq) {
> +        talloc_zfree(req);
> +        return NULL;

see above

> +    }
> +    tevent_req_set_callback(subreq, sdap_sd_search_done, req);
> +
> +    return req;
> +}
> +



More information about the sssd-devel mailing list