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