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

Jakub Hrozek jhrozek at redhat.com
Tue May 6 13:42:24 UTC 2014


On Tue, Apr 29, 2014 at 07:22:37PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > 
> > 
> > ----- Original Message -----
> > > On Fri, Apr 18, 2014 at 12:56:27PM -0400, Yassir Elley wrote:
> > > > I agree. I have made the change.
> > > > 
> > > > Yassir.
> > > Hi,
> > > 
> > > sorry I didn't get to continue the review sooner, but the patch is
> > > massive and I've been trying to get some dbus-related code to master
> > > last week. I hope you don't mind Yassir as you were on a vacation last
> > > week :)
> > 
> > No problem at all. Thanks for the review! I agree with your comments and will
> > address them in my next patch. I have included answers to specific questions
> > below.
> > 
> > > 
> > > In general the code is good, I only have some more suggestions about
> > > coding style or talloc hierarchy and one question about the default, see
> > > below.
> > > 
> > > > From 3617c7b171a4d7090aa6e4c2673769686b4a5919 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
> > > 
> > > [...]
> > > 
> > > > +                            There are three supported values for this
> > > > option:
> > > > +                            <itemizedlist>
> > > > +                                <listitem>
> > > > +                                    <para>
> > > > +                                        disabled: GPO-based access
> > > > control
> > > > rules
> > > > +                                        are neither evaluated nor
> > > > enforced.
> > > > +                                    </para>
> > > > +                                </listitem>
> > > > +                                <listitem>
> > > > +                                    <para>
> > > > +                                        enforcing: GPO-based access
> > > > control
> > > > +                                        rules are evaluated and
> > > > enforced.
> > > > +                                    </para>
> > > > +                                </listitem>
> > > > +                                <listitem>
> > > > +                                    <para>
> > > > +                                        permissive: GPO-based access
> > > > control
> > > > +                                        rules are evaluated, but not
> > > > enforced.
> > > > +                                        Instead, a syslog message will
> > > > be
> > > > +                                        emitted indicating that the user
> > > > would
> > > > +                                        have been denied access if this
> > > > option's
> > > > +                                        value were set to enforcing.
> > > > +                                    </para>
> > > > +                                </listitem>
> > > > +                            </itemizedlist>
> > > > +                        </para>
> > > > +                        <para>
> > > > +                            Default: disabled
> > > 
> > > Is there a reason the default is disabled rather than permissive? Don't
> > > we want to identify the cases where a user would be locked out soon?
> > 
> > I agree that the initial default should be "permissive". My mistake.
> > 
> > > > +                        </para>
> > > > +                    </listitem>
> > > > +                </varlistentry>
> > > > +
> > > > +                <varlistentry>
> > > >                      <term>dyndns_update (boolean)</term>
> > > >                      <listitem>
> > > >                          <para>
> > > > diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
> > > > index
> > > > cae075d4222bba3c05d2c09e26da9b19f4507cc1..cd51129f63bafb2d1d40a9a2880ea02c7617f0db
> > > > 100644
> > > > --- a/src/providers/ad/ad_access.c
> > > > +++ b/src/providers/ad/ad_access.c
> > > 
> > > [...]
> > > 
> > > >  static errno_t
> > > > @@ -385,7 +420,26 @@ ad_access_recv(struct tevent_req *req)
> > > >  }
> > > >  
> > > 
> > > In general with tevent requests, the _recv() function should be the last
> > > one in the code, so when you read the code and find the recv() function,
> > > you know you can stop reading and the rest is either another request or
> > > helper functions. Here you have _done after _recv.
> > > 
> > > >  static void
> > > > -ad_access_check_done(struct tevent_req *req);
> > > > +ad_gpo_access_done(struct tevent_req *subreq)
> > > > +{
> > > > +    struct tevent_req *req;
> > > > +    errno_t ret;
> > > > +
> > > > +    req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > +
> > > > +    ret = ad_gpo_access_recv(subreq);
> > > > +    talloc_zfree(subreq);
> > > > +
> > > 
> > > It would be nice to have a DEBUG message here, at least for failure.
> > > 
> > > > +    if (ret == EOK) {
> > > > +      tevent_req_done(req);
> > > > +    } else {
> > > > +      tevent_req_error(req, ret);
> > > > +    }
> > > > +}
> > > > diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> > > > new file mode 100644
> > > > index
> > > > 0000000000000000000000000000000000000000..935e1924905fb25461ebf57db2e42eb1e8348417
> > > > --- /dev/null
> > > > +++ b/src/providers/ad/ad_gpo.c
> > > > @@ -0,0 +1,2221 @@
> > > 
> > > [...]
> > > 
> > > > +/*
> > > > + * This function determines whether the input ACE includes any of the
> > > > + * client's SIDs. The boolean result is assigned to the _included output
> > > > param.
> > > > + */
> > > > +static errno_t
> > > > +ad_gpo_ace_includes_client_sid(const char *user_sid,
> > > > +                               const char **group_sids,
> > > > +                               int group_size,
> > > > +                               struct security_ace *ace,
> > > > +                               bool *_included)
> > > 
> > > I wonder if this function could simply return a boolean, the caller
> > > ignores the return code anyway.
> > > 
> > > > +{
> > > > +    int i = 0;
> > > > +    struct dom_sid ace_dom_sid;
> > > > +    struct dom_sid user_dom_sid;
> > > > +    struct dom_sid group_dom_sid;
> > > > +    char buf[SID_MAX_LEN + 1];
> > > > +
> > > > +    ace_dom_sid = ace->trustee;
> > > > +
> > > > +    dom_sid_string_buf(&ace_dom_sid, buf, SID_MAX_LEN);
> > > > +
> > > > +    if (!string_to_sid(&user_dom_sid, user_sid)) {
> > > > +        DEBUG(SSSDBG_OP_FAILURE, "string_to_sid failed\n");
> > > > +        return EINVAL;
> > > > +    }
> > > > +
> > > > +    if (dom_sid_equal(&ace_dom_sid, &user_dom_sid)) {
> > > > +        *_included = true;
> > > > +        return EOK;
> > > > +    }
> > > > +
> > > > +    for (i = 0; i < group_size; i++) {
> > > > +        if (!string_to_sid(&group_dom_sid, group_sids[i])) {
> > > > +            DEBUG(SSSDBG_OP_FAILURE, "string_to_sid failed\n");
> > > > +            return EINVAL;
> > > > +        }
> > > > +        if (dom_sid_equal(&ace_dom_sid, &group_dom_sid)) {
> > > > +            *_included = true;
> > > > +            return EOK;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    *_included = false;
> > > > +    return EOK;
> > > > +}
> > > > +
> > > > +/*
> > > > + * This function determines whether use of the extended right
> > > > + * named "ApplyGroupPolicy" (AGP) is allowed, by comparing the specified
> > > > + * user_sid and group_sids against the specified access control entry
> > > > (ACE).
> > > > + * This function returns ALLOWED, DENIED, or NEUTRAL depending on
> > > > whether
> > > > + * the ACE explictly allows, explicitly denies, or does neither.
> > > > + *
> > > > + * The ACE evaluation algorithm is specified in [MS-ADTS] 5.1.3.3.4:
> > > > + * - Deny access by default
> > > > + * - If the "Inherit Only" (IO) flag is set in the ACE, skip the ACE.
> > > > + * - If the SID in the ACE does not match any SID in the requester's
> > > > + *   security context, skip the ACE
> > > > + * - If the ACE type is "Object Access Allowed", the access right
> > > > + *   RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType
> > > 
> > > Thanks for the comments, they are really helpful. What does 'M' stands
> > > for here, though?
> > 
> > This is taken straight from the spec. The 'M' stands for "access mask", which
> > represents the set of access rights (e.g. RIGHT_GENERIC_READ,
> > RIGHT_DS_CREATE_CHILD, RIGHT_DS_CONTROL_ACCESS, etc) associated with an
> > individual ACE. The access right of interest to the GPO code is
> > RIGHT_DS_CONTROL_ACCESS, which serves as a container for all control access
> > rights. The specific access right is identified by a GUID in the ACE's
> > ObjectType. In our case, this is the GUID corresponding to ApplyGroupPolicy.
> > 
> > > 
> > > > + *   field in the ACE is either not present OR contains a GUID value
> > > > equal
> > > > + *   to AGP, then grant requested control access right. Stop access
> > > > checking.
> > > > + * - If the ACE type is "Object Access Denied", the access right
> > > > + *   RIGHT_DS_CONTROL_ACCESS (CR) is present in M, and the ObjectType
> > > > + *   field in the ACE is either not present OR contains a GUID value
> > > > equal
> > > > to
> > > > + *   AGP, then deny the requested control access right. Stop access
> > > > checking.
> > > > + */
> > > > +static enum ace_eval_status ad_gpo_evaluate_ace(struct security_ace
> > > > *ace,
> > > > +                                                const char *user_sid,
> > > > +                                                const char **group_sids,
> > > > +                                                int group_size)
> > > 
> > > [...]
> > > 
> > > > +static errno_t
> > > > +ad_gpo_filter_gpos_by_dacl(TALLOC_CTX *mem_ctx,
> > > > +                           char *user,
> > > > +                           struct sss_domain_info *domain,
> > > > +                           struct gp_gpo **candidate_gpos,
> > > > +                           int num_candidate_gpos,
> > > > +                           struct gp_gpo ***_dacl_filtered_gpos,
> > > > +                           int *_num_dacl_filtered_gpos)
> > > > +{
> > > > +    TALLOC_CTX *tmp_ctx = NULL;
> > > > +    int i = 0;
> > > > +    int ret = 0;
> > > > +    struct gp_gpo *candidate_gpo = NULL;
> > > > +    struct security_descriptor *sd = NULL;
> > > > +    struct security_acl *dacl = NULL;
> > > > +    const char *user_sid = NULL;
> > > > +    const char **group_sids = NULL;
> > > > +    int group_size = 0;
> > > > +    int gpo_dn_idx = 0;
> > > > +    bool access_allowed = false;
> > > > +    struct gp_gpo **dacl_filtered_gpos = NULL;
> > > > +
> > > > +    tmp_ctx = talloc_new(NULL);
> > > > +    if (tmp_ctx == NULL) {
> > > > +        ret = ENOMEM;
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    ret = ad_gpo_get_sids(tmp_ctx, user, domain, &user_sid,
> > > > +                          &group_sids, &group_size);
> > > > +    if (ret != EOK) {
> > > > +        DEBUG(SSSDBG_OP_FAILURE,
> > > > +              "Unable to retrieve SIDs: [%d](%s)\n", ret,
> > > > strerror(ret));
> > > > +        ret = ENOENT;
> > > 
> > > I wonder if ENOENT is the right error code, the information is not
> > > missing it just can't be processed, right? You can even create your own
> > > specific error codes if you find that the standard errno codes don't
> > > express the error well..
> > 
> > You are correct. I will address this in the next patch.
> > 
> > > 
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    dacl_filtered_gpos = talloc_array(tmp_ctx,
> > > > +                                 struct gp_gpo *,
> > > > +                                 num_candidate_gpos + 1);
> > > > +
> > > 
> > > [...]
> > > 
> > > > +    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;
> > > > +    }
> > > 
> > > The target_dn here is already allocated on state, it's just part of
> > > reply, (state->reply->target_dn), I think you can just assign to the
> > > state->target_dn pointer or if you'd like to free reply later on, steal
> > > target dn onto state instead of duplicating the string. This issue is
> > > repeated in the code couple of times.
> > > 
> > > > +
> > > > +    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;
> > > > +    }
> > > > +    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;
> > > > +        }
> > > > +        gplink_options = strchr(first, ';');
> > > > +        if (gplink_options == NULL) {
> > > > +            break;
> > > 
> > > Shouldn't we juts continue to the next item instead of breaking? Or does
> > > this condition mean that the end of the list was reached?
> > 
> > I think you are right. We should just continue to the next item.
> > 
> > > 
> > > 
> > > > +        }
> > > > +        *gplink_options = '\0';
> > > > +        gplink_options++;
> > > > +
> > > > +        gplink_number = strtouint32(gplink_options, NULL, 10);
> > > 
> > > I think you should check errno for failures after the strotouint32 call.
> > > 
> > > > +        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;
> > > > +    }
> > > 
> > > [...]
> > > 
> > > > +ad_gpo_site_dn_retrieval_done(struct tevent_req *subreq)
> > > > +{
> > > > +    struct tevent_req *req;
> > > > +    struct ad_gpo_process_som_state *state;
> > > > +    int ret;
> > > > +    int dp_error;
> > > > +    int i = 0;
> > > > +    size_t reply_count;
> > > > +    struct sysdb_attrs **reply;
> > > > +    const char *configNC;
> > > > +
> > > > +    req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > +    state = tevent_req_data(req, struct ad_gpo_process_som_state);
> > > > +
> > > 
> > > Can you add a comment that would mention that this attribute should be
> > > read from the rootdse in the future and maybe even link to
> > > https://fedorahosted.org/sssd/ticket/2276 ?
> > 
> > Yes.
> > 
> > > 
> > > > +    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 configNC: [%d](%s)\n", ret, strerror(ret));
> > > > +        ret = ENOENT;
> > > > +        goto done;
> > > > +    }
> > > 
> > > [...]
> > > 
> > > > +    /* note that space was allocated for site_dn when allocating
> > > > som_list
> > > > */
> > > > +    state->som_list[state->num_soms] =
> > > > +        talloc_zero(state->som_list, struct gp_som);
> > > > +    if (state->som_list[state->num_soms] == NULL) {
> > > > +        ret = ENOMEM;
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    state->som_list[state->num_soms]->som_dn =
> > > > +        talloc_strdup(state->som_list[state->num_soms], state->site_dn);
> > > 
> > > I think talloc_steal can be used safely here, no need to strdup.
> > > 
> > > > +
> > > > +    if (state->som_list[state->num_soms]->som_dn == NULL) {
> > > > +        ret = ENOMEM;
> > > > +        goto done;
> > > > +    }
> > > > +
> > > > +    state->num_soms++;
> > > > +    state->som_list[state->num_soms] = NULL;
> > > > +
> > > > +    i = 0;
> > > > +    while (state->som_list[i]) {
> > > > +        DEBUG(SSSDBG_TRACE_FUNC, "som_list[%d]->som_dn is %s\n", i,
> > > > +              state->som_list[i]->som_dn);
> > > > +        i++;
> > > > +    }
> > > > +
> > > > +    ret = ad_gpo_get_som_attrs_step(req);
> > > > +
> > > > + done:
> > > > +
> > > > +    if (ret == EOK) {
> > > > +        tevent_req_done(req);
> > > 
> > > Can we even have EOK here? Looks like the functions either return EAGAIN
> > > or outright failure..
> > 
> > ad_gpo_get_som_attrs_step() returns EOK after all SOMs have been processed.
> > There is a comment to that effect in the code.
> > 
> > > 
> > > > +    } else if (ret != EAGAIN) {
> > > > +        tevent_req_error(req, ret);
> > > > +    }
> > > > +
> > > > +}
> > > > +static errno_t
> > > > +ad_gpo_get_som_attrs_step(struct tevent_req *req)
> > > > +{
> > > > +    const char *attrs[] = {AD_AT_GPLINK, AD_AT_GPOPTIONS, NULL};
> > > > +    struct tevent_req *subreq;
> > > > +    struct ad_gpo_process_som_state *state;
> > > > +
> > > > +    state = tevent_req_data(req, struct ad_gpo_process_som_state);
> > > > +
> > > > +    state->som_index++;
> > > > +    struct gp_som *gp_som = state->som_list[state->som_index];
> > > > +
> > > > +    /* gp_som is NULL only after all SOMs have been processed */
> > > > +    if (gp_som == NULL) return EOK;
> > > > +
> > > > +    char *som_dn = gp_som->som_dn;
> > > > +    subreq = sdap_get_generic_send(state, state->ev,  state->opts,
> > > > +                                   sdap_id_op_handle(state->sdap_op),
> > > > +                                   som_dn, LDAP_SCOPE_BASE,
> > > > +                                   "(objectclass=*)", attrs, NULL, 0,
> > > > +                                   state->timeout,
> > > > +                                   false);
> > > > +
> > > > +    if (subreq == NULL) {
> > > > +        DEBUG(SSSDBG_OP_FAILURE, "sdap_get_generic_send failed.\n");
> > > > +        return ENOMEM;
> > > > +    }
> > > > +
> > > > +    tevent_req_set_callback(subreq, ad_gpo_get_som_attrs_done, req);
> > > > +    return EAGAIN;
> > > > +}
> > > 
> > > [...]
> > > 
> > > > diff --git a/src/util/sss_ldap.h b/src/util/sss_ldap.h
> > > > index
> > > > f298b2fbb30cf1532f8e94504ffb83ef73880b81..083c89141987f38313906d7d7a9a003a74c10cef
> > > > 100644
> > > > --- a/src/util/sss_ldap.h
> > > > +++ b/src/util/sss_ldap.h
> > > > @@ -55,6 +55,15 @@ int sss_ldap_get_diagnostic_msg(TALLOC_CTX *mem_ctx,
> > > >  #define LDAP_SERVER_ASQ_OID "1.2.840.113556.1.4.1504"
> > > >  #endif /* LDAP_SERVER_ASQ_OID */
> > > >  
> > > > +#ifndef LDAP_SERVER_SD_OID
> > > > +#define LDAP_SERVER_SD_OID "1.2.840.113556.1.4.801"
> > > > +#endif /* LDAP_SERVER_SD_OID */
> > > > +
> > > > +#define SECINFO_OWNER ( 0x00000001 )
> > > > +#define SECINFO_GROUP ( 0x00000002 )
> > > > +#define SECINFO_DACL ( 0x00000004 )
> > > > +#define SECINFO_SACL ( 0x00000008 )
> > > > +
> > > 
> > > Of all these constants, only SECINFO_DACL is used, is the rest included
> > > for posterity? Do these magic values come from some technet article or
> > > MSDN documentation? If so, it might be nice to add a link or a comment.
> > 
> > These four flags indicate which security descriptor parts to retrieve during
> > a search. They are defined at
> > http://msdn.microsoft.com/en-us/library/aa366987(v=vs.85).aspx. While we
> > only use a single flag (SECINFO_DACL), I thought I would include all four
> > flags for completeness. Should I remove the other three flags that are not
> > being used?
> > 
> > > 
> > > >  int sss_ldap_control_create(const char *oid, int iscritical,
> > > >                              struct berval *value, int dupval,
> > > >                              LDAPControl **ctrlp);
> > > > --
> > > > 1.8.5.3
> > > > 
> > > 
> > > In general, it's really nice work, thank you!
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > 
> > I will now work on a new patch to address the comments you have raised.
> > 
> > Regards,
> > Yassir.
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > 
> 
> Hi Jakub,
> 
> I have attached a revised cumulative patch that aims to address the review comments. 
> 
> Additionally, I made the following changes en masse:
> * changed talloc_strdup() to talloc_steal() when applicable (i.e. when dealing with a talloc context)
> * changed all occurrences of strerror() to sss_strerror() in order to use sssd-specific error strings
> 
> Thanks,
> Yassir.

Hi,

I have some more comments, sorry I haven't caught them earlier. I ran the
patch through Coverity and it uncovered several warnings, see below:

Error: COMPILER_WARNING:
sssd-1.11.90/src/providers/ad/ad_gpo.c: scope_hint: In function 'ad_gpo_target_dn_retrieval_done'
sssd-1.11.90/src/providers/ad/ad_gpo.c:711:22: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
#     state->target_dn = talloc_steal(state, target_dn);
#                      ^

Error: COMPILER_WARNING:
sssd-1.11.90/src/providers/ad/ad_gpo.c: scope_hint: In function 'ad_gpo_parent_dn'
sssd-1.11.90/src/providers/ad/ad_gpo.c:902:17: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
#     *_parent_dn = talloc_steal(mem_ctx, p);
#                 ^

Error: DEADCODE (CWE-561):
sssd-1.11.90/src/providers/ad/ad_gpo.c:1053: cond_notnull: Condition "ptr = __coverity_strchr(ptr, 93)", taking true branch. Now the value of "ptr" is not NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1054: notnull: At condition "ptr == NULL", the value of "ptr" cannot be NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1054: dead_error_condition: The condition "ptr == NULL" cannot be true.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1054: dead_error_line: Execution cannot reach this statement "break;".

Error: DEADCODE (CWE-561):
sssd-1.11.90/src/providers/ad/ad_gpo.c:1768: cond_notnull: Condition "ptr = __coverity_strchr(ptr, 92)", taking true branch. Now the value of "ptr" is not NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1769: notnull: At condition "ptr == NULL", the value of "ptr" cannot be NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1769: dead_error_condition: The condition "ptr == NULL" cannot be true.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1769: dead_error_line: Execution cannot reach this statement "break;".

Error: DEADCODE (CWE-561):
sssd-1.11.90/src/providers/ad/ad_gpo.c:1812: cond_notnull: Condition "raw_machine_ext_names_value == NULL", taking false branch. Now the value of "raw_machine_ext_names_value" is not NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1824: assignment: Assigning: "ptr" = "raw_machine_ext_names_value".
sssd-1.11.90/src/providers/ad/ad_gpo.c:1825: notnull: At condition "ptr == NULL", the value of "ptr" cannot be NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1825: dead_error_condition: The condition "ptr == NULL" cannot be true.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1826: dead_error_begin: Execution cannot reach this statement "ret = 12;".

Error: DEADCODE (CWE-561):
sssd-1.11.90/src/providers/ad/ad_gpo.c:1830: cond_notnull: Condition "ptr = __coverity_strchr(ptr, 93)", taking true branch. Now the value of "ptr" is not NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1831: notnull: At condition "ptr == NULL", the value of "ptr" cannot be NULL.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1831: dead_error_condition: The condition "ptr == NULL" cannot be true.
sssd-1.11.90/src/providers/ad/ad_gpo.c:1831: dead_error_line: Execution cannot reach this statement "break;".

Error: COMPILER_WARNING:
sssd-1.11.90/src/providers/ad/ad_gpo.c: scope_hint: In function 'ad_gpo_get_gpo_attrs_done'
sssd-1.11.90/src/providers/ad/ad_gpo.c:2091:22: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
#     gp_gpo->gpo_guid = talloc_steal(gp_gpo, gpo_guid);
#                      ^

Error: COMPILER_WARNING:
sssd-1.11.90/src/providers/ad/ad_gpo.c:2110:30: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
#     gp_gpo->gpo_display_name = talloc_steal(gp_gpo, gpo_display_name);
#                              ^

Error: COMPILER_WARNING:
sssd-1.11.90/src/providers/ad/ad_gpo.c:2131:19: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
#     file_sys_path = talloc_steal(gp_gpo, raw_file_sys_path);
#                   ^

Also, when I set ad_gpo_access_control=permissive and tried to log in as
Administrator, I was kicked out. I thought that permissive
setting would ignore the errors, but maybe I stumbled upon something
unexpected?

The relevant domain log messages are:
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [ad_gpo_connect_done] (0x0400): sam_account_name is adclient.win.example.com$
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with [(&(objectclass=user)(sAMAccountName=adclient.win.example.com$))][dc=win,dc=example,dc=com].
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [distinguishedName]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [userAccountControl]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid = 18
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_result] (0x2000): Trace: sh[0xfa31d0], connected[1], ops[0xf53520], ldap[0xf67d30]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_REFERENCE]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_result] (0x2000): Trace: sh[0xfa31d0], connected[1], ops[0xf53520], ldap[0xf67d30]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_REFERENCE]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_result] (0x2000): Trace: sh[0xfa31d0], connected[1], ops[0xf53520], ldap[0xf67d30]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_REFERENCE]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_result] (0x2000): Trace: sh[0xfa31d0], connected[1], ops[0xf53520], ldap[0xf67d30]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_REFERENCE]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_result] (0x2000): Trace: sh[0xfa31d0], connected[1], ops[0xf53520], ldap[0xf67d30]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_process_message] (0x4000): Message type: [LDAP_RES_SEARCH_RESULT]
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_get_generic_ext_done] (0x0400): Search result: Success(0), no errmsg set
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [ad_gpo_target_dn_retrieval_done] (0x0040): No DN retrieved for policy target.
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [sdap_id_op_destroy] (0x4000): releasing operation connection
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [ad_gpo_access_done] (0x0040): GPO-based access control failed.
(Tue May  6 14:56:15 2014) [sssd[be[WIN.EXAMPLE.COM]]] [be_pam_handler_callback] (0x0100): Backend returned: (3, 4, No such file or directory) [Internal Error (System error)]



More information about the sssd-devel mailing list