[SSSD] [PRELIMINARY][PATCH] ifp users and groups

Jakub Hrozek jhrozek at redhat.com
Mon May 11 15:58:04 UTC 2015


Thanks for the patches. They work quite well! 

One bug I found is that if you query a nonexistant object path with
GetAll, then all subsequent queries block. Maybe we don't finish some
request on error? This is what I tried:
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Users/redhat_2ecom/10327 org.freedesktop.DBus.Properties.GetAll string:org.freedesktop.sssd.infopipe.Users.User                                                                    
Error org.freedesktop.DBus.Error.NoReply: Message did not receive a reply (timeout by message bus)
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Users/linux_2etest/465400000 org.freedesktop.DBus.Properties.GetAll string:org.freedesktop.sssd.infopipe.Users.User
Error org.freedesktop.DBus.Error.TimedOut: Activation of org.freedesktop.sssd.infopipe timed out

The first objectpath is a copy-n-paste error from a different machine,
the second is legitimate. All works well unless I use the nonexistant path.

Except for the last patch, I only have some nitpicks, see inline. Hopefully,
we'll push the patches soon.

> From 0608ed28fd0499255d566a74d2b600b4bbbad081 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Wed, 21 Jan 2015 13:38:06 +0100
> Subject: [PATCH 1/8] sbus: provide custom error names

ACK

> From be552bf54a91e27c5d04a1e56bc1c3d3ded62d30 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Thu, 29 Jan 2015 15:29:26 +0100
> Subject: [PATCH 2/8] sbus: add sbus_opath_decompose[_exact]

ACK

> From 1b50ad9839b4b77003933d41aee8f0f30b0a557a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Wed, 18 Mar 2015 13:01:07 +0100
> Subject: [PATCH 3/8] sbus: add a{sas} get invoker

Lukas nacked this one, we need the CI tests to pass. Also when I ran the
patches through Coverity, it saw a warning:

Error: UNINIT (CWE-457): [#def1]
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c:319: var_decl: Declaring variable "ret" without initializer.
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c:413: uninit_use: Using uninitialized value "ret".
#  411|   done:
#  412|       talloc_free(table_iter);
#  413|->     return ret;
#  414|   }
#  415|   

Error: COMPILER_WARNING: [#def2]
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c: scope_hint: In function 'sbus_invoke_get_aDOsasDE'
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c:413:5: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     return ret;
#     ^
#  411|   done:
#  412|       talloc_free(table_iter);
#  413|->     return ret;
#  414|   }
#  415|   

I guess neither me or you saw the warning locally because we compile with
-O0? But in general the code looks well to me and has a unit test, I'll
ACK it if you fix up the initialization.

> From f91333767425810d47b6299816ed21547b303de2 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Thu, 15 Jan 2015 13:52:31 +0100
> Subject: [PATCH 4/8] IFP: add org.freedesktop.sssd.infopipe.Users

ACK to the code, but can you add a couple of examples using dbus-send to
the commit message?

I tested that fetching an existing user returns an object path that can
be used later and fetching a non-existing user returns:
    Error org.freedesktop.sssd.Error.NotFound: User not found
I also tested a subdomain user and a user with an overriden username. Both
work since you're correctly using getpwuid_with_views();

Not sure if the bug I found earlier is related to this patch or not..

> From 6fd468596486a894a9a23d9c2d59468e67844dc5 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Mon, 9 Feb 2015 12:02:33 +0100
> Subject: [PATCH 5/8] IFP: add org.freedesktop.sssd.infopipe.Users.User
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2150

Can you also add some examples to the commit message?

> +static void ifp_users_get_as_string(struct sbus_request *sbus_req,
> +                                    void *data,
> +                                    const char *attr,
> +                                    const char **_out)
> +{
> +    struct ifp_ctx *ifp_ctx;
> +    struct ldb_message *msg;
> +    errno_t ret;
> +
> +    *_out = NULL;
> +
> +    ifp_ctx = talloc_get_type(data, struct ifp_ctx);
> +    if (ifp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
> +        return;
> +    }
> +
> +    if (!ifp_is_user_attr_allowed(ifp_ctx, attr)) {
> +        DEBUG(SSSDBG_TRACE_ALL, "Attribute %s is not allowed\n", attr);
> +        return;
> +    }
> +
> +    ret = ifp_users_user_get(sbus_req, ifp_ctx, NULL, NULL, &msg);
> +    if (ret != EOK) {
> +        return;
> +    }
> +
> +    *_out = ldb_msg_find_attr_as_string(msg, attr, NULL);

I think you need to use sss_view_ldb_msg_find_attr_as_string() here. Anyway,
I'm not seeing data from ID views:

[jhrozek at client] sssd $ [(review)] getent passwd psuser at ad.example.com
clientview at AD.EXAMPLE.COM:*:198601106:198601106:ps user:/home/AD.EXAMPLE.COM/psuser:/bin/sh
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Users/AD_2eEXAMPLE_2eCOM/198601106 org.freedesktop.DBus.Properties.Get string:org.freedesktop.sssd.infopipe.Users.User string:name                                                                                                                                                            
method return sender=:1.53 -> dest=:1.56 reply_serial=2
   variant       string "psuser at AD.EXAMPLE.COM"

> +
> +    return;
> +}
> +
> +static void ifp_users_get_as_uint32(struct sbus_request *sbus_req,
> +                                    void *data,
> +                                    const char *attr,
> +                                    uint32_t *_out)

Maybe later we'll need to come up with some smarter caching to avoid
calling a ldb search per attribute, but let's not optimize before we
need to..

> +{
> +    struct ifp_ctx *ifp_ctx;
> +    struct ldb_message *msg;
> +    errno_t ret;
> +
> +    *_out = 0;
> +
> +    ifp_ctx = talloc_get_type(data, struct ifp_ctx);
> +    if (ifp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
> +        return;
> +    }
> +
> +    if (!ifp_is_user_attr_allowed(ifp_ctx, attr)) {
> +        DEBUG(SSSDBG_TRACE_ALL, "Attribute %s is not allowed\n", attr);
> +        return;
> +    }
> +
> +    ret = ifp_users_user_get(sbus_req, ifp_ctx, NULL, NULL, &msg);
> +    if (ret != EOK) {
> +        return;
> +    }
> +
> +    *_out = ldb_msg_find_attr_as_uint(msg, attr, 0);

I think you should use sss_view_ldb_msg_find_attr_as_uint64() here,
unsigned int is a bit platform specific, plus the overrides don't work
(same as with string..)

> +
> +    return;
> +}
> +

Updating groups works fine..

> +void ifp_users_user_get_groups(struct sbus_request *sbus_req,
> +                               void *data,
> +                               const char ***_out,
> +                               int *_size)

But retrieving groups seems to also include non-POSIX groups with GID 0..

> +{
> +    struct ifp_ctx *ifp_ctx;
> +    struct sss_domain_info *domain;
> +    const char *username;
> +    struct ldb_message *user;
> +    struct ldb_result *res;
> +    const char **out;
> +    errno_t ret;
> +    int i;
> +
> +    *_out = NULL;
> +    *_size = 0;
> +
> +    ifp_ctx = talloc_get_type(data, struct ifp_ctx);
> +    if (ifp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
> +        return;
> +    }
> +
> +    if (!ifp_is_user_attr_allowed(ifp_ctx, "groups")) {
> +        DEBUG(SSSDBG_TRACE_ALL, "Attribute %s is not allowed\n",
> +              SYSDB_MEMBEROF);
> +        return;
> +    }
> +
> +    ret = ifp_users_user_get(sbus_req, ifp_ctx, NULL, &domain, &user);
> +    if (ret != EOK) {
> +        return;
> +    }
> +
> +    username = ldb_msg_find_attr_as_string(user, SYSDB_NAME, NULL);
> +    if (username == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "User name is empty!\n");
> +        return;
> +    }
> +
> +    /* Run initgroups. */
> +    ret = sysdb_initgroups_with_views(sbus_req, domain, username, &res);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get groups for %s@%s [%d]: %s\n",
> +              username, domain->name, ret, sss_strerror(ret));
> +        return;
> +    }
> +
> +    out = talloc_zero_array(sbus_req, const char *, res->count);
> +    if (out == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> +        return;
> +    }
> +
> +    for (i = 0; i < res->count; i++) {
> +        out[i] = ifp_groups_build_path_from_msg(out, domain, res->msgs[i]);

I think groups with UID 0 should be filtered here. You can test with
ipausers on an IPA client..

> +        if (out[i] == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "ifp_groups_build_path() failed\n");
> +            return;
> +        }
> +    }
> +
> +    *_out = out;
> +    *_size = res->count;
> +}

The rest looks OK to me..

> From 507f8c5d3be63c5b35fa016a00a00f253ad3194b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Mon, 9 Feb 2015 12:17:37 +0100
> Subject: [PATCH 6/8] IFP: add org.freedesktop.sssd.infopipe.Groups

Same as with others, please add some examples with dbus-send.

Otherwise ACK. I tested with a group that has an override placed on it
and all combinations seemed to work:
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups org.freedesktop.sssd.infopipe.Groups.FindByName string:gr1          
method return sender=:1.88 -> dest=:1.90 reply_serial=2
   object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups org.freedesktop.sssd.infopipe.Groups.FindByName string:clientgr1
method return sender=:1.88 -> dest=:1.91 reply_serial=2
   object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups org.freedesktop.sssd.infopipe.Groups.FindByID uint32:12345
method return sender=:1.88 -> dest=:1.95 reply_serial=2
   object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
[jhrozek at client] sssd $ [(review)] dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups org.freedesktop.sssd.infopipe.Groups.FindByID uint32:465400007
method return sender=:1.88 -> dest=:1.96 reply_serial=2
   object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"

> From a4c1f316f7c434e0e20f457287dda01a96a888c0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Tue, 10 Feb 2015 12:19:19 +0100
> Subject: [PATCH 7/8] IFP: add org.freedesktop.sssd.infopipe.Groups.Group

Similar comment about overrides as with the users patch.
Also please add some examples to the commit message of this patch.

[...]

> +static errno_t
> +ifp_groups_group_get_members(TALLOC_CTX *mem_ctx,
> +                             struct sbus_request *sbus_req,
> +                             void *data,
> +                             const char ***_users,
> +                             int *_users_count,
> +                             const char ***_groups,
> +                             int *_groups_count)
> +{
> +    TALLOC_CTX *tmp_ctx;
> +    struct ldb_message *msg;
> +    struct ldb_message_element *el;
> +    struct sss_domain_info *domain;
> +    const char *class;
> +    const char **users;
> +    int users_count;
> +    const char **groups;
> +    int groups_count;
> +    const char *dn;
> +    struct ldb_dn *ldb_dn;
> +    size_t count;
> +    struct ldb_message **entry;
> +    errno_t ret;
> +    int i;
> +    const char *attrs[] = {SYSDB_OBJECTCLASS, SYSDB_UIDNUM, SYSDB_GIDNUM, NULL};
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        return ENOMEM;
> +    }
> +
> +    ret = ifp_groups_group_get(sbus_req, data, NULL, &domain, &msg);
> +    if (ret != EOK) {
> +        goto done;
> +    }
> +
> +    el = ldb_msg_find_element(msg, SYSDB_MEMBER);
> +    if (el == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get member attribute!\n");
> +        ret = ERR_INTERNAL;
> +        goto done;
> +    }
> +
> +    if (el->num_values == 0) {
> +        *_users = NULL;
> +        *_groups = NULL;
> +        *_users_count = 0;
> +        *_groups_count = 0;
> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    users = talloc_zero_array(tmp_ctx, const char *, el->num_values + 1);
> +    if (users == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    groups = talloc_zero_array(tmp_ctx, const char *, el->num_values + 1);
> +    if (groups == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> +        ret = ENOMEM;
> +        goto done;
> +    }

Here I guess you could optimize and run an ASQ search instead of N base searches.

> From 201615b00948719836d7ae23b36918f664260f47 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Mon, 4 May 2015 14:45:50 +0200
> Subject: [PATCH 8/8] IFP: remove GetUserAttr and GetUserGroup
> 
> Those methods are replaced by proper user and group implementation.
> ---
>  src/responder/ifp/ifp_iface.c   |   3 -
>  src/responder/ifp/ifp_iface.xml |  12 -
>  src/responder/ifp/ifp_private.h |   5 -
>  src/responder/ifp/ifpsrv_cmd.c  | 566 ----------------------------------------
>  4 files changed, 586 deletions(-)

As discussed on IRC, we can't just remove API that is widely used. Feel
free to prepare a different patch that sends a deprecation warning, though,
but we must not remove API without deprecating it first and making sure
all downstreams have been converted to the new API.



More information about the sssd-devel mailing list