[SSSD] [PATCHES] Sysdb interface for netgroups

Sumit Bose sbose at redhat.com
Mon Sep 20 10:46:16 UTC 2010


On Fri, Sep 17, 2010 at 11:21:40AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 09/17/2010 11:16 AM, Dmitri Pal wrote:
> > Stephen Gallagher wrote:
> >> On 09/16/2010 05:05 PM, Stephen Gallagher wrote:
> >>> I've rewritten these patches. Now, instead of searching for individual
> >>> netgroup entries, the code will take advantage of the memberOf plugin to
> >>> return all netgroup triples in a single call to sysdb_getnetgr()
> >>
> >>> This approach will make life easier with LDAP as well, as we will have
> >>> built-in loop prevention.
> >>
> >>> Please review these patches while my netgroup work is ongoing.
> >>
> >>> Patch 0001: New sysdb interfaces for netgroups. Add and delete support
> >>> for netgroups entries, tuples and nested netgroups.
> >>
> >>> Patch 0002: Unit tests for the above interfaces.
> >>
> >>
> >>
> >> Yet another change to these patches.
> >>
> >> I realized that I actually need to get back the three strings from the
> >> triple instead of the internal representation. It makes the most sense
> >> for this to be done in sysdb_getnetgr(), so attached are two new patches
> >> which changes the interface (again) to return a structure of three
> >> strings instead of a single string representing the tuple.
> >>
> >>
> > Stephen, I just glanced over. Did not have a thorough review.
> > There is a bug in sysdb_netgr_split_triple
> > The user, host and domain strings need to be initialized to NULL.
> > 
> > Have not looked at other places :)
> > 
> 
> Ah, you're correct. I accidentally initialized the wrong pointers (the
> p_* pointers). Thanks for that. New patch attached.
> 
> 
> This is what I get for not writing exhaustive unit tests, but I'm
> working on a deadline here :)
> 

The patches apply fine. There is a whitespace issue, 0001 adds an empty
line at the end of sysdb_search.c . All tests passed, please find some
comments below.

bye,
Sumit

> - -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAkyTh4QACgkQeiVVYja6o6N1DwCeItdwGQoqPStrHs76nbzmHAIc
> IyoAn3kLMRSzopQeJevNMD4RbrJpHmha
> =IvTL
> -----END PGP SIGNATURE-----

...

> +int sysdb_set_netgroup_attr(struct sysdb_ctx *ctx,
> +                            struct sss_domain_info *domain,
> +                            const char *name,
> +                            struct sysdb_attrs *attrs,
> +                            int mod_op)
> +{
> +    errno_t ret;
> +    struct ldb_dn *dn;
> +    TALLOC_CTX *tmp_ctx;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (!tmp_ctx) {
> +        return ENOMEM;
> +    }
> +
> +    dn = sysdb_netgroup_dn(ctx, tmp_ctx, domain->name, name);
> +    if (!dn) {

ret is undefined

> +        goto done;
> +    }
> +
> +    ret = sysdb_set_entry_attr(tmp_ctx, ctx, dn, attrs, mod_op);
> +
> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
>  

....

> +int sysdb_delete_netgroup(struct sysdb_ctx *sysdb,
> +                          struct sss_domain_info *domain,
> +                          const char *name)
> +{
> +    TALLOC_CTX *tmp_ctx;
> +    struct ldb_message *msg;
> +    int ret;
> +
> +    if (!name) return EINVAL;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (!tmp_ctx) {
> +        return ENOMEM;
> +    }
> +
> +    ret = sysdb_search_netgroup_by_name(tmp_ctx, sysdb,
> +                                        domain, name, NULL, &msg);
> +    if (ret) {
> +        DEBUG(6, ("sysdb_search_netgroup_by_name failed: %d (%s)\n",
> +                   ret, strerror(ret)));
> +        goto fail;
> +    }
> +
> +    ret = sysdb_delete_entry(sysdb, msg->dn, false);
> +    if (ret) {
> +        goto fail;
> +    }
> +
> +    talloc_free(tmp_ctx);
> +    return EOK;
> +
> +fail:
> +    DEBUG(6, ("Error: %d (%s)\n", ret, strerror(ret)));

I think it would better match with the other functions to have a 'done'
block which is also called if ret==EOK.


> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +

...

> +/* This function splits a three-tuple into three strings
> + * It assumes that any whitespace between the parentheses
> + * and commas are intentional and does not attempt to
> + * strip them out. Leading and trailing whitespace is
> + * ignored.
> + *
> + * This behavior is compatible with nss_ldap's
> + * implementation.
> + */
> +static errno_t sysdb_netgr_split_triple(TALLOC_CTX *mem_ctx,
> +                                        const char *triple,
> +                                        char **hostname,
> +                                        char **username,
> +                                        char **domainname)
> +{
> +    errno_t ret;
> +    TALLOC_CTX *tmp_ctx;
> +    const char *p = triple;
> +    const char *p_host;
> +    const char *p_user;
> +    const char *p_domain;
> +    size_t len;
> +
> +    char *host = NULL;
> +    char *user = NULL;
> +    char *domain = NULL;
> +
> +    /* Pre-set the values to NULL here so if they are not
> +     * copied, we don't return garbage below.
> +     */
> +    *hostname = NULL;
> +    *username = NULL;
> +    *domainname = NULL;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (!tmp_ctx) {
> +        return ENOMEM;
> +    }
> +
> +    /* Remove any leading whitespace */
> +    while (*p && isspace(*p)) p++;
> +
> +    if (*p != '(') {
> +        /* Triple must start and end with parentheses */
> +        ret = EINVAL;
> +        goto done;
> +    }
> +    p++;
> +    p_host = p;
> +
> +    /* Find the first comma */
> +    while (*p && *p != ',') p++;
> +
> +    if (!*p) {
> +        /* No comma was found: parse error */
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    len = p - p_host;
> +
> +    if (len > 0) {
> +        /* Copy the host string */
> +        host = talloc_array(tmp_ctx, char, len+1);
> +        if (!host) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        memcpy(host, p_host, len);
> +        host[len] = '\0';

talloc_strndup might be more convenient here

> +    }
> +    p++;
> +    p_user = p;
> +
> +    /* Find the second comma */
> +    while (*p && *p != ',') p++;
> +
> +    if (!*p) {
> +        /* No comma was found: parse error */
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    len = p - p_user;
> +
> +    if (len > 0) {
> +        /* Copy the user string */
> +        user = talloc_array(tmp_ctx, char, len+1);
> +        if (!user) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        memcpy(user, p_user, len);
> +        user[len] = '\0';
> +    }
> +    p++;
> +    p_domain = p;
> +
> +    /* Find the closing parenthesis */
> +    while (*p && *p != ')') p++;
> +

I think a missing closing parenthesis should be an error.

> +    len = p - p_domain;
> +
> +    if (len > 0) {
> +        /* Copy the domain string */
> +        domain = talloc_array(tmp_ctx, char, len+1);
> +        if (!domain) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        memcpy(domain, p_domain, len);
> +        domain[len] = '\0';
> +    }
> +    p++;
> +
> +    /* skip trailing whitespace */
> +    while (*p && isspace(*p)) p++;
> +
> +    if (*p) {
> +        /* Extra data after the closing parenthesis
> +         * is a parse error
> +         */
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    /* Return any non-NULL values */
> +    if (host) {
> +        *hostname = talloc_steal(mem_ctx, host);
> +    }
> +
> +    if (user) {
> +        *username = talloc_steal(mem_ctx, user);
> +    }
> +
> +    if (domain) {
> +        *domainname = talloc_steal(mem_ctx, domain);
> +    }
> +
> +    ret = EOK;
> +
> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}

...

> +
> +errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
> +                       struct sysdb_ctx *ctx,
> +                       struct sss_domain_info *domain,
> +                       const char *netgroup,
> +                       struct sysdb_netgroup_ctx ***triples)
> +{
> +    TALLOC_CTX *tmp_ctx;
> +    static const char *attrs[] = SYSDB_NETGR_ATTRS;
> +    struct ldb_dn *base_dn;
> +    struct ldb_result *result;
> +    char *netgroup_dn;
> +    char *triple_str;
> +    struct ldb_message_element *el;
> +    struct sysdb_netgroup_ctx **tmp_triples = NULL;
> +    int lret;
> +    int i, j;
> +    size_t size = 0;
> +    errno_t ret;
> +
> +    if (!domain) {
> +        return EINVAL;
> +    }
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (!tmp_ctx) {
> +        return ENOMEM;
> +    }
> +
> +    base_dn = ldb_dn_new_fmt(tmp_ctx, ctx->ldb,
> +                             SYSDB_TMPL_NETGROUP_BASE,
> +                             domain->name);
> +    if (!base_dn) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    netgroup_dn = talloc_asprintf(tmp_ctx, SYSDB_TMPL_NETGROUP,
> +                                  netgroup, domain->name);
> +    if (!netgroup_dn) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    lret = ldb_search(ctx->ldb, tmp_ctx, &result, base_dn,
> +                     LDB_SCOPE_SUBTREE, attrs,
> +                     SYSDB_NETGR_TRIPLES_FILTER,
> +                     netgroup, netgroup_dn);
> +    ret = sysdb_error_to_errno(lret);
> +    if (ret != EOK) {
> +        goto done;
> +    }
> +
> +    if(result->count == 0) {
> +        ret = ENOENT;
> +        goto done;
> +    }
> +
> +    for (i=0; i < result->count; i++) {
> +        el = ldb_msg_find_element(result->msgs[i], SYSDB_NETGROUP_TRIPLE);
> +        if (!el) {
> +            /* No triples in this netgroup. It might be a nesting
> +             * container only.
> +             * Skip it and continue on.
> +             */
> +            continue;
> +        }
> +
> +        /* Enlarge the array by the value count
> +         * Always keep one extra entry for the NULL terminator
> +         */
> +        tmp_triples = talloc_realloc(tmp_ctx, tmp_triples,
> +                                     struct sysdb_netgroup_ctx *,
> +                                     size+el->num_values+1);
> +        if (!tmp_triples) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +
> +        /* Copy in all of the triples */
> +        for(j = 0; j < el->num_values; j++) {
> +            triple_str = talloc_strdup(tmp_ctx,
> +                                       (const char *)el->values[j].data);

I think it would be safer to use talloc_strndup with el->values[j].length


> +            if (!triple_str) {
> +                ret = ENOMEM;
> +                goto done;
> +            }
> +
> +            tmp_triples[size] = talloc_zero(tmp_triples,
> +                                            struct sysdb_netgroup_ctx);
> +            if (!tmp_triples[size]) {
> +                ret = ENOMEM;
> +                goto done;
> +            }
> +
> +            ret = sysdb_netgr_split_triple(tmp_triples[size],
> +                                           triple_str,
> +                                           &tmp_triples[size]->hostname,
> +                                           &tmp_triples[size]->username,
> +                                           &tmp_triples[size]->domainname);
> +            if (ret != EOK) {
> +                goto done;
> +            }
> +
> +            size++;
> +        }
> +    }
> +
> +    if (!tmp_triples) {
> +        /* No entries were found
> +         * Create a dummy reply
> +         */
> +        tmp_triples = talloc_array(tmp_ctx, struct sysdb_netgroup_ctx *, 1);
> +        if (!tmp_triples) {
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +    }
> +    /* Add NULL terminator */
> +    tmp_triples[size] = NULL;
> +
> +    *triples = talloc_steal(mem_ctx, tmp_triples);
> +    ret = EOK;
> +
> +done:
> +    talloc_zfree(tmp_ctx);
> +    return ret;
> +}




More information about the sssd-devel mailing list