[SSSD] [PATCHES] support local overrides

Jakub Hrozek jhrozek at redhat.com
Mon Jul 27 16:27:59 UTC 2015


On Mon, Jul 27, 2015 at 05:27:29PM +0200, Pavel Březina wrote:
> On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> >On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
> >>On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
> >>>On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
> >>>>https://fedorahosted.org/sssd/ticket/2584
> >>>btw (unrelated to these patches) it seems strange to require the user of
> >>>the sysdb database to connect to the db and then separately initialize
> >>>subdomains...I would vote for merging that code..
> >>
> >>+1
> >
> >I'll file a ticket for michal.
> >
> >>
> >>>>+
> >>>>+#include <talloc.h>
> >>>>+#include <popt.h>
> >>>>+
> >>>>+#include "confdb/confdb.h"
> >>>>+
> >>>>+struct sss_tool_ctx {
> >>>>+    struct confdb_ctx *confdb;
> >>>>+    char *confdb_path;
> >>>>+
> >>>>+    char *default_domain;
> >>>>+    struct sss_domain_info *domains;
> >>>
> >>>I wonder if the tool_ctx must be open for consumers? Currently only
> >>>domains is used, maybe we could get away with a getter and an opaque
> >>>structure? We can always make the structure public in future, but if
> >>>it's public from the start, then it's easier to add cruft..
> >>
> >>I think it is not worth the effort with such simple structure. I am
> >>all for opaque structures but here is just no gain in code safety. It would
> >>only make code lines bigger.
> >
> >Fine.
> >
> >>
> >>>>+enum sss_tool_domain {
> >>>>+    SSS_TOOL_DOMAIN_REQUIRED,
> >>>>+    SSS_TOOL_DOMAIN_OPTIONAL
> >>>>+};
> >>>>+
> >>>>+int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
> >>>>+                        struct sss_tool_ctx *tool_ctx,
> >>>>+                        const char *input,
> >>>>+                        enum sss_tool_domain require_domain,
> >>>
> >>>Do we need require_domain now that you added the getpwnam?
> >>
> >>No, removed.
> >
> >Thanks
> >
> >[...]
> >
> >>>
> >>>Hmm looks like you should have a check for res validity here, for cases
> >>>no domain match..
> >>
> >>Added a dom validity.
> >
> >Thanks.
> >
> >>
> >>>btw it might be nice to split this large function into smaller ones.
> >>>Maybe the do-while check could be a separate function returning res.
> >>
> >>There is no way to split it that would be actually beneficial. There are
> >>three blocks:
> >>a) beginning - getpwnam
> >>b) middle - iteration
> >>c) end - linearized dn
> >>
> >>If you take away the iteration you can directly return dn from the new
> >>function since you don't need res at all so that would also took away the
> >>end. And the beginning belongs to the function were iteration is IMHO.
> >
> >OK
> >
> >
> >> From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Fri, 24 Jul 2015 09:55:28 +0200
> >>Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
> >>
> >>Objects doesn't have to have overrideDN specified when using LOCAL view.
> >>Since the view is not stored on the server we do not want to contact
> >>LDAP therefore we special case LOCAL view saying that it is OK that
> >>this attribute is missing.
> >>
> >>Preparation for:
> >>https://fedorahosted.org/sssd/ticket/2584
> >>---
> >>  src/db/sysdb.h                      |  3 +-
> >>  src/db/sysdb_views.c                |  7 +++++
> >>  src/tests/cmocka/test_sysdb_views.c | 62 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 71 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> >>index 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22 100644
> >>--- a/src/db/sysdb.h
> >>+++ b/src/db/sysdb.h
> >>@@ -157,9 +157,10 @@
> >>  #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
> >>  #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
> >>
> >>+#define SYSDB_DEFAULT_VIEW_NAME "default"
> >>+#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side overrides */
> >>  #define SYSDB_VIEW_CLASS "view"
> >>  #define SYSDB_VIEW_NAME "viewName"
> >>-#define SYSDB_DEFAULT_VIEW_NAME "default"
> >>  #define SYSDB_OVERRIDE_CLASS "overrride"
> >>  #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
> >>  #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
> >>diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
> >>index aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec 100644
> >>--- a/src/db/sysdb_views.c
> >>+++ b/src/db/sysdb_views.c
> >>@@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain,
> >>          override_dn_str = ldb_msg_find_attr_as_string(obj,
> >>                                                        SYSDB_OVERRIDE_DN, NULL);
> >>          if (override_dn_str == NULL) {
> >>+            if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) == 0) {
> >>+                /* LOCAL view doesn't have to have overrideDN specified. */
> >>+                ret = EOK;
> >>+                goto done;
> >>+            }
> >>+
> >>              DEBUG(SSSDBG_CRIT_FAILURE,
> >>                    "Missing override DN for objext [%s].\n",
> >>                    ldb_dn_get_linearized(obj->dn));
> >>+
> >>              ret = ENOENT;
> >>              goto done;
> >>          }
> >>diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c
> >>index 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871 100644
> >>--- a/src/tests/cmocka/test_sysdb_views.c
> >>+++ b/src/tests/cmocka/test_sysdb_views.c
> >>@@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void **state)
> >>      assert_int_equal(ldb_val_string_cmp(&el->values[1], "OVERRIDEKEY2"), 0);
> >>  }
> >>
> >>+void test_sysdb_add_overrides_to_object_local(void **state)
> >>+{
> >>+    int ret;
> >>+    struct ldb_message *orig;
> >>+    struct ldb_message_element *el;
> >>+    char *tmp_str;
> >>+    struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
> >>+                                                         struct sysdb_test_ctx);
> >>+
> >>+    orig = ldb_msg_new(test_ctx);
> >>+    assert_non_null(orig);
> >>+
> >>+    tmp_str = talloc_strdup(orig,  "ORIGNAME");
> >
> >Please put assert_non_null here and to other assignments to tmp_str in
> >these two tests, otherwise ACK
> >
> >>+    ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
> >>+    assert_int_equal(ret, EOK);
> >>+
> >>+    tmp_str = talloc_strdup(orig,  "ORIGGECOS");
> >>+    ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
> >>+    assert_int_equal(ret, EOK);
> >>+
> >>+    test_ctx->domain->has_views = true;
> >>+    test_ctx->domain->view_name = "LOCAL";
> >>+
> >>+    ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
> >>+    assert_int_equal(ret, EOK);
> >>+}
> >
> >> From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Wed, 22 Jul 2015 10:02:02 +0200
> >>Subject: [PATCH 2/3] TOOLS: add common command framework
> >
> >ACK
> >
> >
> >> From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Fri, 24 Jul 2015 09:58:11 +0200
> >>Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2584
> >
> >There was one more Coverity warning:
> >Error: UNINIT (CWE-457): [#def1]
> >sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable "ret" without initializer.
> >sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using uninitialized value "ret".
> >#  250|
> >#  251|   done:
> >#  252|->     if (ret != EOK) {
> >#  253|           talloc_free(attrs);
> >#  254|           return NULL;
> >
> >Otherwise looks good.
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> 
> Thanks, new patches are attached. In addition to your comments:
> - one new patch that adds null check in original unit test
> - man page and tool change that says that zero ids are not supported

I am still waiting for Coverity and CI results.

> From db9ba08b8c359eec318fa3dfc28e038618ea35f7 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Mon, 27 Jul 2015 17:24:45 +0200
> Subject: [PATCH 1/4] VIEWS TEST: add null-check

ACK


> From 9b67c5777f51cedd1b6669d0f103c232e80e9ad6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Fri, 24 Jul 2015 09:55:28 +0200
> Subject: [PATCH 2/4] SYSDB: prepare for LOCAL view

ACK

> From 39b1bf17cb52e000c8ef74862fb88a4762176730 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Wed, 22 Jul 2015 10:02:02 +0200
> Subject: [PATCH 3/4] TOOLS: add common command framework

ACK

> From 99c97b41a6feeb0edd5be11d27b8e832d4694843 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> Date: Fri, 24 Jul 2015 09:58:11 +0200
> Subject: [PATCH 4/4] TOOLS: add sss_override for local overrides

there is still an issue with user getpwnam finds but sysdb_getpwnam not:

(gdb) set args user-add root -n toor
(gdb) r
Starting program: /usr/sbin/sss_override user-add root -n toor
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.21-5.fc22.x86_64
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header

Program received signal SIGSEGV, Segmentation fault.
override_user_add (cmdline=0x7fffffffe390, tool_ctx=0x6110c0, pvt=0x0)
    at /sssd/src/tools/sss_override.c:567
567             fprintf(stderr, _("Unable to find user %s@%s.\n"),
Missing separate debuginfos, use: dnf debuginfo-install bzip2-libs-1.0.6-14.fc22.x86_64 cyrus-sasl-lib-2.1.26-22.fc22.x86_64 dbus-libs-1.8.18-1.fc22.x86_64 elfutils-libelf-0.161-6.fc22.x86_64 elfutils-libs-0.161-6.fc22.x86_64 glib2-2.44.1-1.fc22.x86_64 libattr-2.4.47-9.fc22.x86_64 libbasicobjects-0.1.1-24.fc22.x86_64 libcap-2.24-7.fc22.x86_64 libcollection-0.6.2-24.fc22.x86_64 libdhash-0.4.3-24.fc22.x86_64 libgcc-5.1.1-1.fc22.x86_64 libgcrypt-1.6.3-4.fc22.x86_64 libgpg-error-1.17-2.fc22.x86_64 libini_config-1.1.0-24.fc22.x86_64 libldb-1.1.20-1.fc22.x86_64 libpath_utils-0.2.1-24.fc22.x86_64 libref_array-0.1.4-24.fc22.x86_64 libselinux-2.3-10.fc22.x86_64 libtalloc-2.1.2-1.fc22.x86_64 libtdb-1.3.4-1.fc22.x86_64 libtevent-0.9.24-1.fc22.x86_64 nspr-4.10.8-1.fc22.x86_64 nss-3.19.2-1.0.fc22.x86_64 nss-softokn-freebl-3.19.2-1.0.fc22.x86_64 nss-util-3.19.2-1.0.fc22.x86_64 openldap-2.4.40-12.fc22.x86_64 openssl-libs-1.0.1k-11.fc22.x86_64 pcre-8.37-2.fc22.x86_64 popt-1.16-5.fc22.x86_64 systemd-libs-219-19.fc22.x86_64 xz-libs-5.2.0-2.fc22.x86_64 zlib-1.2.8-7.fc22.x86_64
(gdb) list 567
562             return EXIT_FAILURE;
563         }
564
565         dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user);
566         if (dn == NULL) {
567             fprintf(stderr, _("Unable to find user %s@%s.\n"),
568                     user.orig_name, user.domain->name);
569             return EXIT_FAILURE;
570         }
571
(gdb) p user
$1 = {input_name = 0x7fffffffe7b5 "root", orig_name = 0x611a30 "root", domain = 0x0, 
  name = 0x619c00 "toor", uid = 0, gid = 0, home = 0x0, shell = 0x0, gecos = 0x0}

Same issue is there with groups btw.



More information about the sssd-devel mailing list