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(a)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(a)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(a)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(a)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(a)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(a)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@(a)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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel