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

+}

On Fri, Apr 11, 2014 at 06:32:05PM -0400, Yassir Elley wrote:

----- Original Message -----

...

On Thu, Apr 03, 2014 at 12:30:17PM -0400, Yassir Elley wrote:

...

----- Original Message -----

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?

There might be setup where the GPO would in theory apply to a Linux client running SSSD but since we didn't support it before access was always granted. If now the GPO is evaluated a user might be rejected although he was able to log in before. In this case it should be possible to disable the GPO check to return the previous behaviour without completely disabling access control.

...

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 I add a single new function I typically add the unit tests in the same patch. In your case I think it would be more useful to create a separate patch only with the unit tests.

...

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?

No, comments are only necessary where you disagree.

Thanks, Yassir.

...

bye, Sumit

...

From a508e36e67aa881665d0387ff418162e5f1825cd Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@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.

yes, I meant to add the list of source files to the _SOURCES variable of ad_access_filter_tests.

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.

I think this warning indicates that libtool cannot link the symbol of a shared library into a binary (the symbols are resolved at build time here and not at runtime which would be typical for a loadable module) on all platforms. Maybe this is even dependant on the libtool version and might fail on Linux platforms with older libtool?

As an alternative to add the source files you might want to create a libsss_ad_common.la similar to libsss_ldap_common.la or libsss_krb5_common.la?

...

...

...

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

There is no need to change code which you do not touch. If you find typos, coding style violations or other issues in the code you can send them in a new patch in a new email thread.

About addressing review comments, in general that changes are not send in a separate patch but squashed into the original patches. It's a bit more burden on the reviewer but I think patches especially for new features should stand on their own. Because it should be easy for a person who looks at the patch later to understand it. As with source code I think patches are more often read then reviewed.

Additionally I think it's not the task of the person who commits the patches to squash the changes together before committing the. With patch series it might not be obvious which change belongs to which patch, there might be conflicts which are hard for the committer to resolve because he does not know the code.

...

...

 ret = ad_access_recv(req);
 talloc_zfree(req);
 switch (ret) {

...

...

...

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

Ok, looks like we are safe then.

...

...

•    gplink_options = strchr(first, ';');
  

•    if (gplink_options == NULL) {
  

•        break;
  

•    }
  

•    *gplink_options = '\0';
  

•    gplink_options++;
  

•    gplink_number = atoi(gplink_options);
  

...

...

...

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

Makes sense, thank you for the explanation.

bye, Sumit