[SSSD] [PATCHES] Views: apply user SSH public key override

Jakub Hrozek jhrozek at redhat.com
Wed Nov 5 10:28:29 UTC 2014


On Tue, Nov 04, 2014 at 08:55:28PM +0100, Sumit Bose wrote:
> On Tue, Nov 04, 2014 at 07:12:14PM +0100, Lukas Slebodnik wrote:
> > On (04/11/14 14:29), Sumit Bose wrote:
> > >On Tue, Nov 04, 2014 at 10:07:12AM +0100, Jakub Hrozek wrote:
> > >> On Tue, Oct 28, 2014 at 04:41:44PM +0100, Sumit Bose wrote:
> > >> > Hi,
> > >> > 
> > >> > this series of patches add support for SSH public key overrides. The
> > >> > main part is in patch 004. The others mainly enhance the exiting
> > >> > functions with respect to multi-values and variable attribute lists.
> > >> > 
> > >> > In contrast to other overrides the SSH public keys from the override
> > >> > attributes do not override any existing ones but are added to the
> > >> > existing ones. Please tell me if you think this is not a good idea so
> > >> > that I can change it.
> > >> > 
> > >> > bye,
> > >> > Sumit
> > >> 
> > >> I'm still running some tests and because I ran into issues updating my
> > >> devel server, I'm sending code-wise review now to speed up the process.
> > >> 
> > >> btw I discussed the old format of ssh keys with Honza and even though
> > >> neither of us tested them, the code is going to handle them fine,
> > >> because the conversion is happening as late as the client libraries
> > >> (sss_ssh_*).
> > >> 
> > >> > From c4fda98f9df7dd665e28f3c71097125bbf2ba07c Mon Sep 17 00:00:00 2001
> > >> > From: Sumit Bose <sbose at redhat.com>
> > >> > Date: Mon, 27 Oct 2014 16:53:44 +0100
> > >> > Subject: [PATCH 1/5] Add add_strings_lists() utilty function
> > >> > 
> > >> > ---
> > >> >  src/tests/cmocka/test_utils.c | 111 ++++++++++++++++++++++++++++++++++++++++++
> > >> 
> > >> Thanks for the unit tests!
> > >
> > >Thank you for the review.
> > >
> > >> 
> > >> >  src/util/util.c               |  60 +++++++++++++++++++++++
> > >> >  src/util/util.h               |  18 +++++++
> > >> >  3 files changed, 189 insertions(+)
> > >> > 
> > >> 
> > >> [...]
> > >> 
> > >> > +/**
> > >> > + * @brief Add two list of strings
> > >> > + *
> > >> > + * Create a new NULL-termintated list of strings by adding two lists together.
> > >> > + *
> > >> > + * @param[in] mem_ctx      Talloc memory context for the new list.
> > >> > + * @param[in] l1           First NULL-termintated list of strings.
> > >> > + * @param[in] l2           Second NULL-termintated list of strings.
> > >> > + * @param[in] copy_strings If set to 'true' the list items will be copied
> > >> > + *                         otherwise only the pointers to the items are
> > >> > + *                         copied.
> > >> > + * @param[out] new_list    New NULL-terminated list of strings. Must be freed
> > >> > + *                         with talloc_free() by the caller. If copy_strings
> > >> > + *                         is 'true' the new elements will be freed as well.
> > >> > + */
> > >> > +errno_t add_strings_lists(TALLOC_CTX *mem_ctx, const char **l1, const char **l2,
> > >> > +                          bool copy_strings, char ***new_list);
> > >> > +
> > >> 
> > >> Can you prefix the output variable with a leading underscore? I know in
> > >> this case it's clear from the name it's an output variable, but the
> > >> prefix is more or less a convention we follow.
> > >
> > >done
> > >
> > >> 
> > >> There is also a typo in the commit message (utilty).
> > >
> > >done
> > >
> > >> 
> > >> Otherwise ACK.
> > >> 
> > >> > From 93b96813267feb55c9d12bdddefd58fc4e4f1f2b Mon Sep 17 00:00:00 2001
> > >> > From: Sumit Bose <sbose at redhat.com>
> > >> > Date: Mon, 27 Oct 2014 13:33:08 +0100
> > >> > Subject: [PATCH 2/5] sysdb_get_user_attr_with_views: add mandatory override
> > >> >  attributes
> > >> 
> > >> ACK. I don't see an easy way around the discard_const either 
> > >> 
> > >> > From 3cebae5b5569dc559896945e244e0ae9575b04d1 Mon Sep 17 00:00:00 2001
> > >> > From: Sumit Bose <sbose at redhat.com>
> > >> > Date: Mon, 27 Oct 2014 15:11:08 +0100
> > >> > Subject: [PATCH 3/5] sysdb_add_overrides_to_object: add new parameter and
> > >> >  multi-value support
> > >> 
> > >> Can you add the new parameter to the doxygen description of
> > >> sysdb_add_overrides_to_object() ?
> > >
> > >done
> > >
> > >> 
> > >> > 
> > >
> > >...
> > >
> > >> > +++ b/src/db/sysdb_search.c
> > >> > @@ -124,7 +124,8 @@ errno_t sysdb_getpwnam_with_views(TALLOC_CTX *mem_ctx,
> > >> >       * the original object. */
> > >> >      if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
> > >> >          ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
> > >> > -                          override_obj == NULL ? NULL : override_obj ->msgs[0]);
> > >> > +                          override_obj == NULL ? NULL : override_obj ->msgs[0],
> > >> 
> > >> Is the space after override_obj intentional? It looks a bit odd ...
> > >
> > >fixed
> > >
> > >> 
> > >> > +                          NULL);
> > >> >          if (ret != EOK && ret != ENOENT) {
> > >> >              DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");
> > >> >              goto done;
> > >> 
> > >> The rest of the patch looks good to me.
> > >> 
> > >> > From 2d14490b0369e98afb17c1e340e2555d79187faa Mon Sep 17 00:00:00 2001
> > >> > From: Sumit Bose <sbose at redhat.com>
> > >> > Date: Thu, 16 Oct 2014 13:17:37 +0200
> > >
> > >...
> > >
> > >> > -                if (ret != EOK) {
> > >> > -                    DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_add_val failed.\n");
> > >> > -                    goto done;
> > >> > +                num_values = el->num_values;
> > >> > +                /* Only SYSDB_SSH_PUBKEY is allowed to have multiple values. */
> > >> > +                if (strcmp(allowed_attrs[c], SYSDB_SSH_PUBKEY) != 0
> > >> > +                        && num_values != 1) {
> > >> > +                    DEBUG(SSSDBG_MINOR_FAILURE,
> > >> > +                          "Override attribute for [%s] has more [%d] " \
> > >> > +                          "than one value, using onlye the first.\n",
> > >>                                                     ^^^^^
> > >>                                                     Typo..
> > >
> > >fixed
> > >
> > >> 
> > >> Also adds a compilation warning:
> > >> src/db/sysdb_views.c: In function 'sysdb_apply_default_override':
> > >> src/db/sysdb_views.c:616:21: warning: format '%d' expects argument of type 'int', but argument 7 has type 'size_t' [-Wformat=]
> > >
> > >fixed
> > >
> > >> 
> > >> > +                          allowed_attrs[c], num_values);
> > >> > +                    num_values = 1;
> > >> > +                }
> > >> > +
> > >
> > >The new version contains a 6th patch which adds the public key to the
> > >attribute list of the getorigbyname request so that the extdom plugin can
> > >send the SSH key from the default view to the client.
> > >
> > >bye,
> > >Sumit
> > 
> > >From af409207bf299c28d14093ee92e8824c894c4a11 Mon Sep 17 00:00:00 2001
> > >From: Sumit Bose <sbose at redhat.com>
> > >Date: Mon, 27 Oct 2014 16:53:44 +0100
> > >Subject: [PATCH 1/6] Add add_strings_lists() utility function
> > >
> > >---
> > > src/tests/cmocka/test_utils.c | 111 ++++++++++++++++++++++++++++++++++++++++++
> > > src/util/util.c               |  60 +++++++++++++++++++++++
> > > src/util/util.h               |  18 +++++++
> > > 3 files changed, 189 insertions(+)
> > >
> > //snip
> > 
> > >+errno_t add_strings_lists(TALLOC_CTX *mem_ctx, const char **l1, const char **l2,
> > >+                          bool copy_strings, char ***_new_list)
> > >+{
> > >+    size_t c;
> > >+    size_t l1_count = 0;
> > >+    size_t l2_count = 0;
> > >+    size_t new_count = 0;
> > >+    char **new;
> > >+    int ret;
> > >+
> > >+    if (l1 != NULL) {
> > >+        for (l1_count = 0; l1[l1_count] != NULL; l1_count++);
> > >+    }
> > >+
> > >+    if (l2 != NULL) {
> > >+        for (l2_count = 0; l2[l2_count] != NULL; l2_count++);
> > >+    }
> > Variables l1 and l2 can be NULL and function will continue.
> > >+
> > >+    new_count = l1_count + l2_count;
> > >+
> > >+    new = talloc_array(mem_ctx, char *, new_count + 1);
> > >+    if (new == NULL) {
> > >+        DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> > >+        return ENOMEM;
> > >+    }
> > >+    new [new_count] = NULL;
> > >+
> > >+    if (copy_strings) {
> > >+        for(c = 0; c < l1_count; c++) {
> > >+            new[c] = talloc_strdup(new, l1[c]);
> > >+            if (new[c] == NULL) {
> > >+                DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> > >+                ret = ENOMEM;
> > >+                goto done;
> > >+            }
> > >+        }
> > >+        for(c = 0; c < l2_count; c++) {
> > >+            new[l1_count + c] = talloc_strdup(new, l2[c]);
> > >+            if (new[l1_count + c] == NULL) {
> > >+                DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n");
> > >+                ret = ENOMEM;
> > >+                goto done;
> > >+            }
> > >+        }
> > >+    } else {
> > >+        memcpy(new, l1, sizeof(char *) * l1_count);
> >                       ^^
> >            warning: Null pointer passed as an argument to a 'nonnull' parameter
> > >+        memcpy(&new[l1_count], l2, sizeof(char *) * l2_count);
> >                                  ^^
> >            warning: Null pointer passed as an argument to a 'nonnull' parameter
> > 
> 
> Thank you Lukas for catching this. There is even a test for this case
> which passes. I guess memcpy() is basically a no-op if the last argument
> is 0. Nevertheless I added a check to be on the safe side.
> 
> > >+    }
> > >+
> > >+    *_new_list = new;
> > >+    ret = EOK;
> > >+
> > >+done:
> > >+    if (ret != EOK) {
> > >+        talloc_free(new);
> > >+    }
> > >+
> > >+    return ret;
> > >+}
> > 
> > >From d21b192e1ad2b6a0bf4621aed9437164bc1ea525 Mon Sep 17 00:00:00 2001
> > >From: Sumit Bose <sbose at redhat.com>
> > >Date: Tue, 28 Oct 2014 15:53:16 +0100
> > >Subject: [PATCH 5/6] Add test for sysdb_add_overrides_to_object()
> > >
> > >---
> > > Makefile.am                         |  15 +++
> > > src/tests/cmocka/test_sysdb_views.c | 235 ++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 250 insertions(+)
> > > create mode 100644 src/tests/cmocka/test_sysdb_views.c
> > >
> > >diff --git a/Makefile.am b/Makefile.am
> > >index 3c81295..6842e23 100644
> > >--- a/Makefile.am
> > >+++ b/Makefile.am
> > >@@ -215,6 +215,7 @@ if HAVE_CMOCKA
> > >         sss_sifp-tests \
> > >         test_search_bases \
> > >         sdap-tests \
> > >+        test_sysdb_views \
> > >         $(NULL)
> > > 
> > > if BUILD_IFP
> > >@@ -2058,6 +2059,20 @@ sss_sifp_tests_LDADD = \
> > >     $(SSSD_INTERNAL_LTLIBS)
> > > endif # BUILD_IFP
> > > 
> > >+test_sysdb_views_SOURCES = \
> > >+    src/tests/cmocka/test_sysdb_views.c \
> > >+    $(NULL)
> > >+test_sysdb_views_CFLAGS = \
> > >+    $(AM_CFLAGS) \
> > >+    $(NULL)
> > >+test_sysdb_views_LDADD = \
> > >+    $(CMOCKA_LIBS) \
> > >+    $(POPT_LIBS) \
> > >+    $(TALLOC_LIBS) \
> > >+    $(SSSD_INTERNAL_LTLIBS) \
> > >+    libsss_test_common.la \
> > >+    $(NULL)
> > >+
> > > endif # HAVE_CMOCKA
> > Compilation failed on platforms with disabled link all deplibs.
> > 
> >   CCLD     test_sysdb_views
> > /usr/bin/ld: src/tests/cmocka/test_sysdb_views-test_sysdb_views.o: undefined reference to symbol 'ldb_msg_new@@LDB_0.9.10'
> > /lib64/libldb.so.1: error adding symbols: DSO missing from command line
> > collect2: error: ld returned 1 exit status
> > 
> > Solution is simple:
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -2066,6 +2066,7 @@ test_sysdb_views_CFLAGS = \
> >      $(NULL)
> >  test_sysdb_views_LDADD = \
> >      $(CMOCKA_LIBS) \
> > +    $(LDB_LIBS) \
> >      $(POPT_LIBS) \
> >      $(TALLOC_LIBS) \
> >      $(SSSD_INTERNAL_LTLIBS) \
> 
> added
> 
> > 
> > 
> > >From fc81e59f58a568af1bc6a21aced45f6b8c26db24 Mon Sep 17 00:00:00 2001
> > >From: Sumit Bose <sbose at redhat.com>
> > >Date: Tue, 4 Nov 2014 13:58:39 +0100
> > >Subject: [PATCH 6/6] Add ssh pubkey to origbyname request
> > >
> > >Since the IPA clients expects that the extdom plugin delivers the
> > >default view data for a given user this patch adds the public SSH key to
> > >the list of returned attributes of the getorigbyname request so that it
> > >can be send back to the clients.
> > >---
> > > src/responder/nss/nsssrv_cmd.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > I was not able to apply this patch on current master.
> 
> As Jakub already said this set depends on the user_attrs patch set. I
> move the add_strings_lists() patch to the other set to get the order
> right.
> 
> bye,
> Sumit
> 
> > 
> > LS
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

OK, now also the default view works!

ACK



More information about the sssd-devel mailing list