[SSSD] [PATCH] IFP: Per-attribute ACL for users

Jakub Hrozek jhrozek at redhat.com
Fri May 2 16:34:24 UTC 2014


This patch depends on all the GetUserAttr patches.

Introduces a new option called user_attributes that allows to specify
which user attributes are allowed to be queried from the IFP responder.

By default only the default POSIX set is allowed, this option allows to
either add other attributes (+attrname) or remove them from the default
set (-attrname).
-------------- next part --------------
>From 55ecc17a541d35bb3542ab3b8d98c3e44132313a Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 2 Jan 2014 17:23:08 +0100
Subject: [PATCH] IFP: Per-attribute ACL for users

Introduces a new option called user_attributes that allows to specify
which user attributes are allowed to be queried from the IFP responder.

By default only the default POSIX set is allowed, this option allows to
either add other attributes (+attrname) or remove them from the default
set (-attrname).
---
 Makefile.am                          |   2 +-
 src/confdb/confdb.h                  |   1 +
 src/config/SSSDConfig/__init__.py.in |   1 +
 src/config/etc/sssd.api.conf         |   1 +
 src/man/sssd-ifp.5.xml               |  62 +++++++++++++++
 src/responder/ifp/ifp_private.h      |   3 +
 src/responder/ifp/ifpsrv.c           |  17 ++++
 src/responder/ifp/ifpsrv_cmd.c       |  30 ++++++-
 src/responder/ifp/ifpsrv_util.c      | 146 +++++++++++++++++++++++++++++++++++
 src/tests/cmocka/test_ifp.c          | 101 ++++++++++++++++++++++++
 10 files changed, 361 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 785e8479c99a7b48b26b12b62010467d8926c06e..e581fb645a20ebb27e696ee2f0508573b3c8addd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1695,7 +1695,7 @@ ifp_tests_LDADD = \
     $(SSSD_LIBS) \
     $(SSSD_INTERNAL_LTLIBS) \
     libsss_test_common.la
-endif
+endif # BUILD_IFP
 
 endif # HAVE_CMOCKA
 
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index 8498adb2f11a732e22be64b2e227d98af5d05cbc..7994a67576b219cb476ea7ca19d63521664e491c 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -132,6 +132,7 @@
 
 /* InfoPipe */
 #define CONFDB_IFP_CONF_ENTRY "config/ifp"
+#define CONFDB_IFP_USER_ATTR_LIST "user_attributes"
 
 /* Domains */
 #define CONFDB_DOMAIN_PATH_TMPL "config/domain/%s"
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index 3f17a923b62d57e3354913b4d391bc919afc61fa..fe76f059d010343872381a551c7ff48e5dacbdea 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -97,6 +97,7 @@ option_strings = {
 
     # [ifp]
     'allowed_uids': _('List of UIDs or user names allowed to access the InfoPipe responder'),
+    'user_attributes': _('List of user attributes the InfoPipe is allowed to publish'),
 
     # [provider]
     'id_provider' : _('Identity provider'),
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
index a5cb62f453a2a273bf1f93bbfe1b4493815aa6bb..c7c1232c378d7e641da678d853247ddfd31bb736 100644
--- a/src/config/etc/sssd.api.conf
+++ b/src/config/etc/sssd.api.conf
@@ -70,6 +70,7 @@ ssh_known_hosts_timeout = int, None, false
 [pac]
 # PAC responder
 allowed_uids = str, None, false
+user_attributes = str, None, false
 
 [ifp]
 # InfoPipe responder
diff --git a/src/man/sssd-ifp.5.xml b/src/man/sssd-ifp.5.xml
index 7e2ea7cfc868e56b7434f0275eeea5b792f31a5f..867c117edccc3c000f7d9e8456298b72ebcdf693 100644
--- a/src/man/sssd-ifp.5.xml
+++ b/src/man/sssd-ifp.5.xml
@@ -69,6 +69,68 @@
                         </para>
                     </listitem>
                 </varlistentry>
+
+                <varlistentry>
+                    <term>user_attributes (string)</term>
+                    <listitem>
+                        <para>
+                            Specifies the comma-separated list of white
+                            or blacklisted attributes.
+                        </para>
+                        <para>
+                            By default, the InfoPipe responder only
+                            allows the default set of POSIX attributes to
+                            be requested. This set is the same as returned by
+                            <citerefentry>
+                                <refentrytitle>getpwnam</refentrytitle>
+                                <manvolnum>3</manvolnum>
+                            </citerefentry>
+                            and includes:
+                            <variablelist>
+                                <varlistentry>
+                                    <term>name</term>
+                                    <listitem><para>user's login name</para></listitem>
+                                </varlistentry>
+                                <varlistentry>
+                                    <term>uidNumber</term>
+                                    <listitem><para>user ID</para></listitem>
+                                </varlistentry>
+                                <varlistentry>
+                                    <term>gidNumber</term>
+                                    <listitem><para>primary group ID</para></listitem>
+                                </varlistentry>
+                                <varlistentry>
+                                    <term>gecos</term>
+                                    <listitem><para>user information, typically full name</para></listitem>
+                                </varlistentry>
+                                <varlistentry>
+                                    <term>homeDirectory</term>
+                                    <listitem><para>home directory</para></listitem>
+                                </varlistentry>
+                                <varlistentry>
+                                    <term>loginShell</term>
+                                    <listitem><para>user shell</para></listitem>
+                                </varlistentry>
+                            </variablelist>
+                        </para>
+                        <para>
+                            It is possible to add another attribute to
+                            this set by using <quote>+attr_name</quote>
+                            or explicitly remove an attribute using
+                            <quote>-attr_name</quote>. For example, to
+                            allow <quote>telephoneNumber</quote> but deny
+                            <quote>loginShell</quote>, you would use the
+                            following configuration:
+                        <programlisting>
+user_attributes = +telephoneNumber, -loginShell
+                        </programlisting>
+                        </para>
+                        <para>
+                            Default: not set. Only the default set of
+                            POSIX attributes is allowed.
+                        </para>
+                    </listitem>
+                </varlistentry>
             </variablelist>
     </refsect1>
 
diff --git a/src/responder/ifp/ifp_private.h b/src/responder/ifp/ifp_private.h
index 52c480bb4e3bce525d74ac6d6ba9fb68cbdf3f07..c03cf6ccc8767de4ffeb469bfea84f06a4da3441 100644
--- a/src/responder/ifp/ifp_private.h
+++ b/src/responder/ifp/ifp_private.h
@@ -43,6 +43,7 @@ struct ifp_ctx {
     int neg_timeout;
 
     struct sysbus_ctx *sysbus;
+    const char **user_whitelist;
 };
 
 /* This is a throwaway method to ease the review of the patch.
@@ -68,4 +69,6 @@ const char *ifp_path_strip_prefix(const char *path, const char *prefix);
 errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
                                struct ldb_message_element *el);
 
+const char **ifp_parse_attr_list(TALLOC_CTX *mem_ctx, const char *conf_str);
+bool ifp_attr_allowed(const char *whitelist[], const char *attr);
 #endif /* _IFPSRV_PRIVATE_H_ */
diff --git a/src/responder/ifp/ifpsrv.c b/src/responder/ifp/ifpsrv.c
index 978f3614a2485a2160fa5a797df16dd143662bb2..e02b5f08a1c9ef5ecec518020c015df7005d4612 100644
--- a/src/responder/ifp/ifpsrv.c
+++ b/src/responder/ifp/ifpsrv.c
@@ -205,6 +205,7 @@ int ifp_process_init(TALLOC_CTX *mem_ctx,
     int ret;
     int max_retries;
     char *uid_str;
+    char *attr_list_str;
 
     ifp_cmds = get_ifp_cmds();
     ret = sss_process_init(mem_ctx, ev, cdb,
@@ -271,6 +272,22 @@ int ifp_process_init(TALLOC_CTX *mem_ctx,
         goto fail;
     }
 
+    ret = confdb_get_string(ifp_ctx->rctx->cdb, ifp_ctx->rctx,
+                            CONFDB_IFP_CONF_ENTRY, CONFDB_IFP_USER_ATTR_LIST,
+                            NULL, &attr_list_str);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_FATAL_FAILURE, ("Failed to get allowed UIDs.\n"));
+        goto fail;
+    }
+
+    ifp_ctx->user_whitelist = ifp_parse_attr_list(ifp_ctx, attr_list_str);
+    talloc_free(attr_list_str);
+    if (ifp_ctx->user_whitelist == NULL) {
+        DEBUG(SSSDBG_FATAL_FAILURE,
+              ("Failed to parse the allowed attribute list\n"));
+        goto fail;
+    }
+
     /* Enable automatic reconnection to the Data Provider */
     ret = confdb_get_int(ifp_ctx->rctx->cdb,
                          CONFDB_IFP_CONF_ENTRY,
diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
index 908d46d382490c442654abc4efd1838db33d949f..34e78f109aeee5cb332709a3a196f5227a537089 100644
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -96,17 +96,43 @@ static errno_t
 ifp_user_get_attr_unpack_msg(struct ifp_attr_req *attr_req)
 {
     bool parsed;
+    char **attrs;
+    int nattrs;
+    int i, ai;
+    const char **whitelist = attr_req->ireq->ifp_ctx->user_whitelist;
 
     parsed = sbus_request_parse_or_finish(attr_req->ireq->dbus_req,
                                           DBUS_TYPE_STRING, &attr_req->name,
                                           DBUS_TYPE_ARRAY, DBUS_TYPE_STRING,
-                                          &attr_req->attrs,
-                                          &attr_req->nattrs,
+                                          &attrs, &nattrs,
                                           DBUS_TYPE_INVALID);
     if (parsed == false) {
+        DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
         return EOK; /* handled */
     }
 
+    /* Copy the attributes to maintain memory hierarchy with talloc */
+    attr_req->attrs = talloc_zero_array(attr_req, const char *, nattrs+1);
+    if (attr_req->attrs == NULL) {
+        return ENOMEM;
+    }
+
+    ai = 0;
+    for (i = 0; i < nattrs; i++) {
+        if (ifp_attr_allowed(whitelist, attrs[i]) == false) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  "Attribute %s not present in the whitelist, skipping\n",
+                  attrs[i]);
+            continue;
+        }
+
+        attr_req->attrs[ai] = talloc_strdup(attr_req->attrs, attrs[i]);
+        if (attr_req->attrs[ai] == NULL) {
+            return ENOMEM;
+        }
+        ai++;
+    }
+
     return EOK;
 }
 
diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c
index abff136e06de25df3c9b26d2ce853d5e9b5fc2c3..b022ee4a544dda771f41b2a40eaed57a3a9dd634 100644
--- a/src/responder/ifp/ifpsrv_util.c
+++ b/src/responder/ifp/ifpsrv_util.c
@@ -24,6 +24,11 @@
 #include "db/sysdb.h"
 #include "responder/ifp/ifp_private.h"
 
+#define IFP_DEFAULT_ATTRS {SYSDB_NAME, SYSDB_UIDNUM,   \
+                           SYSDB_GIDNUM, SYSDB_GECOS,  \
+                           SYSDB_HOMEDIR, SYSDB_SHELL, \
+                           NULL}
+
 errno_t ifp_req_create(struct sbus_request *dbus_req,
                        struct ifp_ctx *ifp_ctx,
                        struct ifp_req **_ifp_req)
@@ -169,3 +174,144 @@ errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
 
     return EOK;
 }
+
+static inline bool
+attr_in_list(const char **list, size_t nlist, const char *str)
+{
+    size_t i;
+
+    for (i = 0; i < nlist; i++) {
+        if (strcasecmp(list[i], str) == 0) {
+            break;
+        }
+    }
+
+    return (i < nlist) ? true : false;
+}
+
+const char **
+ifp_parse_attr_list(TALLOC_CTX *mem_ctx, const char *conf_str)
+{
+    TALLOC_CTX *tmp_ctx;
+    errno_t ret;
+    const char **list = NULL;
+    const char **res = NULL;
+    int list_size;
+    char **conf_list = NULL;
+    int conf_list_size = 0;
+    const char **allow = NULL;
+    const char **deny = NULL;
+    int ai = 0, di = 0, li = 0;
+    int i;
+    const char *defaults[] = IFP_DEFAULT_ATTRS;
+
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        return NULL;
+    }
+
+    if (conf_str) {
+        ret = split_on_separator(tmp_ctx, conf_str, ',', true, true,
+                                 &conf_list, &conf_list_size);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE,
+                  "Cannot parse attribute ACL list  %s: %d\n", conf_str, ret);
+            goto done;
+        }
+
+        allow = talloc_zero_array(tmp_ctx, const char *, conf_list_size);
+        deny = talloc_zero_array(tmp_ctx, const char *, conf_list_size);
+        if (allow == NULL || deny == NULL) {
+            goto done;
+        }
+    }
+
+    for (i = 0; i < conf_list_size; i++) {
+        switch (conf_list[i][0]) {
+            case '+':
+                allow[ai] = conf_list[i] + 1;
+                ai++;
+                continue;
+            case '-':
+                deny[di] = conf_list[i] + 1;
+                di++;
+                continue;
+            default:
+                DEBUG(SSSDBG_CRIT_FAILURE, "ACL values must start with "
+                      "either '+' (allow) or '-' (deny), got '%s'\n",
+                      conf_list[i]);
+                goto done;
+        }
+    }
+
+    /* Assume the output will have to hold defauls and all the configured,
+     * values, resize later
+     */
+    list_size = 0;
+    while (defaults[list_size]) {
+        list_size++;
+    }
+    list_size += conf_list_size;
+
+    list = talloc_zero_array(tmp_ctx, const char *, list_size + 1);
+    if (list == NULL) {
+        goto done;
+    }
+
+    /* Start by copying explicitly allowed attributes */
+    for (i = 0; i < ai; i++) {
+        /* if the attribute is explicitly denied, skip it */
+        if (attr_in_list(deny, di, allow[i])) {
+            continue;
+        }
+
+        list[li] = talloc_strdup(list, allow[i]);
+        if (list[li] == NULL) {
+            goto done;
+        }
+        li++;
+
+        DEBUG(SSSDBG_TRACE_INTERNAL,
+              "Added allowed attr %s to whitelist\n", allow[i]);
+    }
+
+    /* Add defaults */
+    for (i = 0; defaults[i]; i++) {
+        /* if the attribute is explicitly denied, skip it */
+        if (attr_in_list(deny, di, defaults[i])) {
+            continue;
+        }
+
+        list[li] = talloc_strdup(list, defaults[i]);
+        if (list[li] == NULL) {
+            goto done;
+        }
+        li++;
+
+        DEBUG(SSSDBG_TRACE_INTERNAL,
+              "Added default attr %s to whitelist\n", defaults[i]);
+    }
+
+    res = talloc_steal(mem_ctx, list);
+done:
+    talloc_free(tmp_ctx);
+    return res;
+}
+
+bool
+ifp_attr_allowed(const char *whitelist[], const char *attr)
+{
+    size_t i;
+
+    if (whitelist == NULL) {
+        return false;
+    }
+
+    for (i = 0; whitelist[i]; i++) {
+        if (strcasecmp(whitelist[i], attr) == 0) {
+            break;
+        }
+    }
+
+    return (whitelist[i]) ? true : false;
+}
diff --git a/src/tests/cmocka/test_ifp.c b/src/tests/cmocka/test_ifp.c
index 188508bcb80e696dfd74322796f90300cf2c1560..70ebf942dcb1e366516bed92620dece6dedd0a0d 100644
--- a/src/tests/cmocka/test_ifp.c
+++ b/src/tests/cmocka/test_ifp.c
@@ -206,6 +206,105 @@ void test_el_to_dict(void **state)
     assert_false(dbus_message_iter_next(&iter_dict));
 }
 
+static void assert_string_list_equal(const char **s1,
+                                     const char **s2)
+{
+    int i;
+
+    for (i=0; s1[i]; i++) {
+        assert_non_null(s2[i]);
+        assert_string_equal(s1[i], s2[i]);
+    }
+
+    assert_null(s2[i]);
+}
+
+static void attr_parse_test(const char *expected[], const char *input)
+{
+    const char **res;
+    TALLOC_CTX *test_ctx;
+
+    test_ctx = talloc_new(NULL);
+    assert_non_null(test_ctx);
+
+    res = ifp_parse_attr_list(test_ctx, input);
+
+    if (expected) {
+        /* Positive test */
+        assert_non_null(res);
+        assert_string_list_equal(res, expected);
+    } else {
+        /* Negative test */
+        assert_null(res);
+    }
+
+    talloc_free(test_ctx);
+}
+
+void test_attr_acl(void **state)
+{
+    /* Test defaults */
+    const char *exp_defaults[] = { SYSDB_NAME, SYSDB_UIDNUM,
+                                   SYSDB_GIDNUM, SYSDB_GECOS,
+                                   SYSDB_HOMEDIR, SYSDB_SHELL,
+                                   NULL };
+    attr_parse_test(exp_defaults, NULL);
+
+    /* Test adding some attributes to the defaults */
+    const char *exp_add[] = { "telephoneNumber", "streetAddress",
+                              SYSDB_NAME, SYSDB_UIDNUM,
+                              SYSDB_GIDNUM, SYSDB_GECOS,
+                              SYSDB_HOMEDIR, SYSDB_SHELL,
+                              NULL };
+    attr_parse_test(exp_add, "+telephoneNumber, +streetAddress");
+
+    /* Test removing some attributes to the defaults */
+    const char *exp_rm[] = { SYSDB_NAME,
+                             SYSDB_GIDNUM, SYSDB_GECOS,
+                             SYSDB_HOMEDIR,
+                             NULL };
+    attr_parse_test(exp_rm, "-"SYSDB_SHELL ",-"SYSDB_UIDNUM);
+
+    /* Test both add and remove */
+    const char *exp_add_rm[] = { "telephoneNumber",
+                                 SYSDB_NAME, SYSDB_UIDNUM,
+                                 SYSDB_GIDNUM, SYSDB_GECOS,
+                                 SYSDB_HOMEDIR,
+                                 NULL };
+    attr_parse_test(exp_add_rm, "+telephoneNumber, -"SYSDB_SHELL);
+
+    /* Test rm trumps add */
+    const char *exp_add_rm_override[] = { SYSDB_NAME, SYSDB_UIDNUM,
+                                          SYSDB_GIDNUM, SYSDB_GECOS,
+                                          SYSDB_HOMEDIR, SYSDB_SHELL,
+                                          NULL };
+    attr_parse_test(exp_add_rm_override,
+                    "+telephoneNumber, -telephoneNumber, +telephoneNumber");
+
+    /* Remove all */
+    const char *rm_all[] = { NULL };
+    attr_parse_test(rm_all,  "-"SYSDB_NAME ", -"SYSDB_UIDNUM
+                             ", -"SYSDB_GIDNUM ", -"SYSDB_GECOS
+                             ", -"SYSDB_HOMEDIR ", -"SYSDB_SHELL);
+
+    /* Malformed list */
+    attr_parse_test(NULL,  "missing_plus_or_minus");
+}
+
+void test_attr_allowed(void **state)
+{
+    const char *whitelist[] = { "name", "gecos", NULL };
+    const char *emptylist[] = { NULL };
+
+    assert_true(ifp_attr_allowed(whitelist, "name"));
+    assert_true(ifp_attr_allowed(whitelist, "gecos"));
+
+    assert_false(ifp_attr_allowed(whitelist, "password"));
+
+    assert_false(ifp_attr_allowed(emptylist, "name"));
+    assert_false(ifp_attr_allowed(NULL, "name"));
+}
+
 int main(int argc, const char *argv[])
 {
     poptContext pc;
@@ -221,6 +320,8 @@ int main(int argc, const char *argv[])
         unit_test(ifp_test_req_wrong_uid),
         unit_test(test_path_prefix),
         unit_test(test_el_to_dict),
+        unit_test(test_attr_acl),
+        unit_test(test_attr_allowed),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
1.9.0



More information about the sssd-devel mailing list