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