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

Yassir Elley yelley at redhat.com
Fri Apr 11 22:32:05 UTC 2014



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

Thanks for the thorough review. Much appreciated!

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

I'm not sure I understand this comment. What do you mean by "existing setups"? If you mean GPO-unaware setups, then the GPO code should not break any existing regressions. If it did, we would want to know about it, right?

> 
> 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 I create some unit tests, should I include them in the next patch? Or should the next patch to the list be limited to addressing your review comments? I just don't know the process.

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

Sounds good. Below, I have included a few responses/questions related to your comments. If I have not responded to a comment, that means I agree with it and will make the necessary change. Is that OK? Or is it preferrable to give positive acknowledgment of each comment?

Thanks,
Yassir.

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

I didn't understand this comment. Are you suggesting inserting ${libsss_ad_la_SOURCES} in the LDADD line? I don't think the linker deals with source files.

Also, since we have full control of libsss_ad.so, and since the unit tests will always be built together with the current version of libsss_ad.so, why does it matter that linking against libsss_ad.so is not portable. I thought that this warning would only be relevant if we were linking against a library that we had no control over.

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

Agreed.

I noticed that some other functions in this file also do not separate declarations from the code with an empty line. Should I fix those as well in this patch (even though those functions are not my code)? Or is that a separate patch? In general, is there a policy of limiting the next patch to only addressing code review comments (and not inserting additional fixes like the ones here).

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

The [MS-GPOL] spec indicates that the "LDAP://" prefix should be stripped from each GPO DN before being used in an LDAP search.

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

The [MS-GPOL] spec indicates that unenforced gpo_dns must be *prepended*, while enforced_gpo_dns must be *appended*. I implemented prepending of unenforced_gpo_dns by adding them in reverse order. 

> 
> > +        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;
> > +}
> > +
> _______________________________________________
> 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