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

Jakub Hrozek jhrozek at redhat.com
Mon Apr 28 11:28:00 UTC 2014


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

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?

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

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

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


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

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

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

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



More information about the sssd-devel mailing list