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