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

Yassir Elley yelley at redhat.com
Tue May 6 16:42:45 UTC 2014



----- Original Message -----
> 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);
> # 

I have fixed the DEADCODE conditions detected by coverity and attached a revised cumulative patch. With regard to the COMPILER_WARNING, Lukas had already commented on these previously, and I had already fixed them in the most recent patch.
                  
> 
> 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?

This is expected behavior because I have not implemented "permissive" yet. For now, "permissive" and "enforcing" are identical. I was not planning on implementing "permissive" until after the code is able to retrieve policy over smb and make an enforcement decision. I did implement "disabled", since it allows users to simply turn off gpo checking altogether. 

The current code in ad_gpo_access.c simply does the following:

    switch (state->ctx->gpo_access_control_mode) {
    case GPO_ACCESS_CONTROL_DISABLED:
        /* do not evaluate gpos; mark request done */
        tevent_req_done(req);
        return;
    case GPO_ACCESS_CONTROL_PERMISSIVE:
    case GPO_ACCESS_CONTROL_ENFORCING:
        /* continue on to evaluate gpos */
        break;
    default:
        tevent_req_error(req, EINVAL);
        return;
    }

Regards,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implemented-LDAP-component-of-GPO-based-access-contr.patch
Type: text/x-patch
Size: 107255 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140506/409e3471/attachment.bin>


More information about the sssd-devel mailing list