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

Lukas Slebodnik lslebodn at redhat.com
Tue Nov 4 18:12:14 UTC 2014


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

>+    }
>+
>+    *_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) \


>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.

LS



More information about the sssd-devel mailing list