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