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
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@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!
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.
There is also a typo in the commit message (utilty).
Otherwise ACK.
From 93b96813267feb55c9d12bdddefd58fc4e4f1f2b Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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@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() ?
With the new parameter an attribute list other than the default one can be used.
Override attributes with multiple values (e.g. SSH public keys) are noew supported as well.
src/db/sysdb.h | 3 ++- src/db/sysdb_search.c | 24 ++++++++++++++++-------- src/db/sysdb_views.c | 39 +++++++++++++++++++++++---------------- src/responder/nss/nsssrv_cmd.c | 2 +- 4 files changed, 42 insertions(+), 26 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index ebb1bbe..f582f6a 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -487,7 +487,8 @@ errno_t sysdb_search_group_override_by_gid(TALLOC_CTX *mem_ctx,
errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain, struct ldb_message *obj,
struct ldb_message *override_obj);
struct ldb_message *override_obj,const char **req_attrs);errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain, struct ldb_message *obj); diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index dacbd23..c039aa8 100644 --- a/src/db/sysdb_search.c +++ 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 ...
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@redhat.com Date: Thu, 16 Oct 2014 13:17:37 +0200 Subject: [PATCH 4/5] Views: apply user SSH public key override
With this patch the SSH public key override attribute is read from the FreeIPA server and saved in the cache with the other override data.
Since it is possible to have multiple public SSH keys this override value does not replace any other data but will be added to existing values.
Fixes https://fedorahosted.org/sssd/ticket/2454
src/db/sysdb_views.c | 38 +++++++++---- src/man/sssd-ipa.5.xml | 3 + src/providers/ipa/ipa_common.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/responder/ssh/sshsrv_cmd.c | 123 +++++++++++++++++++++++++++++++---------- 5 files changed, 126 insertions(+), 40 deletions(-)
diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index b8e90dd..d6cae9a 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -560,6 +560,8 @@ errno_t sysdb_apply_default_override(struct sss_domain_info *domain, TALLOC_CTX *tmp_ctx; struct sysdb_attrs *attrs; size_t c;
- size_t d;
- size_t num_values; struct ldb_message_element *el = NULL; const char *allowed_attrs[] = { SYSDB_UIDNUM, SYSDB_GIDNUM,
@@ -567,6 +569,7 @@ errno_t sysdb_apply_default_override(struct sss_domain_info *domain, SYSDB_HOMEDIR, SYSDB_SHELL, SYSDB_NAME,
SYSDB_SSH_PUBKEY, NULL };@@ -584,7 +587,6 @@ errno_t sysdb_apply_default_override(struct sss_domain_info *domain, }
for (c = 0; allowed_attrs[c] != NULL; c++) {
/* TODO: add nameAlias for case-insentitive searches */ ret = sysdb_attrs_get_el_ext(override_attrs, allowed_attrs[c], false, &el); if (ret == EOK) {@@ -607,17 +609,30 @@ errno_t sysdb_apply_default_override(struct sss_domain_info *domain, goto done; } } else {
ret = sysdb_attrs_add_val(attrs, allowed_attrs[c],&el->values[0]);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..
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=]
allowed_attrs[c], num_values);num_values = 1;}for (d = 0; d < num_values; d++) {ret = sysdb_attrs_add_val(attrs, allowed_attrs[c],&el->values[d]);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE,"sysdb_attrs_add_val failed.\n");goto done;}DEBUG(SSSDBG_TRACE_ALL,"Override [%s] with [%.*s] for [%s].\n",allowed_attrs[c], (int) el->values[d].length,el->values[d].data, ldb_dn_get_linearized(obj_dn)); }
DEBUG(SSSDBG_TRACE_ALL, "Override [%s] with [%.*s] for [%s].\n",allowed_attrs[c],(int) el->values[0].length,el->values[0].data,ldb_dn_get_linearized(obj_dn)); } } else if (ret != ENOENT) { DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_el_ext failed.\n");@@ -981,6 +996,7 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain, {SYSDB_HOMEDIR, OVERRIDE_PREFIX SYSDB_HOMEDIR}, {SYSDB_SHELL, OVERRIDE_PREFIX SYSDB_SHELL}, {SYSDB_NAME, OVERRIDE_PREFIX SYSDB_NAME},
{SYSDB_SSH_PUBKEY, OVERRIDE_PREFIX SYSDB_SSH_PUBKEY}, {NULL, NULL}diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml index 51f14f8..e8a716c 100644 --- a/src/man/sssd-ipa.5.xml +++ b/src/man/sssd-ipa.5.xml @@ -626,6 +626,9 @@ <listitem> <para>ldap_user_shell</para> </listitem>
<listitem><para>ldap_user_ssh_public_key</para></listitem> </itemizedlist> </para> <para>diff --git a/src/providers/ipa/ipa_common.h b/src/providers/ipa/ipa_common.h index 0e9324f..4952765 100644 --- a/src/providers/ipa/ipa_common.h +++ b/src/providers/ipa/ipa_common.h @@ -128,6 +128,7 @@ enum ipa_override_attrs { IPA_AT_OVERRIDE_SHELL, IPA_AT_OVERRIDE_GROUP_NAME, IPA_AT_OVERRIDE_GROUP_GID_NUMBER,
IPA_AT_OVERRIDE_USER_SSH_PUBLIC_KEY,
IPA_OPTS_OVERRIDE
}; diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h index 4785e01..32f5ebe 100644 --- a/src/providers/ipa/ipa_opts.h +++ b/src/providers/ipa/ipa_opts.h @@ -283,6 +283,7 @@ struct sdap_attr_map ipa_override_map[] = { { "ldap_user_shell", "loginShell", SYSDB_SHELL, NULL }, { "ldap_group_name", "cn", SYSDB_NAME, NULL }, { "ldap_group_gid_number", "gidNumber", SYSDB_GIDNUM, NULL },
- { "ldap_user_ssh_public_key", "ipaSshPubKey", SYSDB_SSH_PUBKEY, NULL }, SDAP_ATTR_MAP_TERMINATOR
};
diff --git a/src/responder/ssh/sshsrv_cmd.c b/src/responder/ssh/sshsrv_cmd.c index ad83163..5bed2e0 100644 --- a/src/responder/ssh/sshsrv_cmd.c +++ b/src/responder/ssh/sshsrv_cmd.c @@ -232,8 +232,8 @@ ssh_user_pubkeys_search_next(struct ssh_cmd_ctx *cmd_ctx) return EFAULT; }
- ret = sysdb_get_user_attr(cmd_ctx, cmd_ctx->domain,
cmd_ctx->name, attrs, &res);
- ret = sysdb_get_user_attr_with_views(cmd_ctx, cmd_ctx->domain,
cmd_ctx->name, attrs, &res);@@ -782,6 +782,65 @@ ssh_cmd_parse_request(struct ssh_cmd_ctx *cmd_ctx) return EOK; }
+static errno_t decode_and_add_base64_data(struct ssh_cmd_ctx *cmd_ctx,
struct ldb_message_element *el,size_t fqname_len,const char *fqname,size_t *c)+{
- struct cli_ctx *cctx = cmd_ctx->cctx;
- uint8_t *key;
- size_t key_len;
- uint8_t *body;
- size_t body_len;
- int ret;
- size_t d;
- TALLOC_CTX *tmp_ctx;
- if (el == NULL) {
DEBUG(SSSDBG_TRACE_ALL, "Mssing element, nothing to do.\n");return EOK;- }
- tmp_ctx = talloc_new(NULL);
- if (tmp_ctx == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");return ENOMEM;- }
- for (d = 0; d < el->num_values; d++) {
key = sss_base64_decode(tmp_ctx, (const char *) el->values[d].data,&key_len);if (key == NULL) {DEBUG(SSSDBG_OP_FAILURE, "sss_base64_decode failed.\n");ret = ENOMEM;goto done;}ret = sss_packet_grow(cctx->creq->out,3*sizeof(uint32_t) + key_len + fqname_len);if (ret != EOK) {DEBUG(SSSDBG_OP_FAILURE, "sss_packet_grow failed.\n");goto done;}sss_packet_get_body(cctx->creq->out, &body, &body_len);SAFEALIGN_SET_UINT32(body+(*c), 0, c);SAFEALIGN_SET_UINT32(body+(*c), fqname_len, c);safealign_memcpy(body+(*c), fqname, fqname_len, c);SAFEALIGN_SET_UINT32(body+(*c), key_len, c);safealign_memcpy(body+(*c), key, key_len, c);- }
- ret = EOK;
+done:
- talloc_free(tmp_ctx);
- return ret;
+}
static errno_t ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) { @@ -790,14 +849,13 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) uint8_t *body; size_t body_len; size_t c = 0;
- unsigned int i;
- struct ldb_message_element *el;
- struct ldb_message_element *el = NULL;
- struct ldb_message_element *el_override = NULL;
- struct ldb_message_element *el_orig = NULL; uint32_t count = 0; const char *name; char *fqname; uint32_t fqname_len;
uint8_t *key;
size_t key_len;
ret = sss_packet_new(cctx->creq, 0, sss_packet_get_cmd(cctx->creq->in),
@@ -811,6 +869,20 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) count = el->num_values; }
- el_orig = ldb_msg_find_element(cmd_ctx->result,
ORIGINALAD_PREFIX SYSDB_SSH_PUBKEY);- if (el_orig) {
count = el_orig->num_values;- }
- if (DOM_HAS_VIEWS(cmd_ctx->domain)) {
el_override = ldb_msg_find_element(cmd_ctx->result,OVERRIDE_PREFIX SYSDB_SSH_PUBKEY);if (el_override) {count += el_override->num_values;}- }
- ret = sss_packet_grow(cctx->creq->out, 2*sizeof(uint32_t)); if (ret != EOK) { return ret;
@@ -820,7 +892,7 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx) SAFEALIGN_SET_UINT32(body+c, count, &c); SAFEALIGN_SET_UINT32(body+c, 0, &c);
- if (!el) {
- if (count == 0) { return EOK; }
@@ -840,30 +912,23 @@ ssh_cmd_build_reply(struct ssh_cmd_ctx *cmd_ctx)
fqname_len = strlen(fqname)+1;
- for (i = 0; i < el->num_values; i++) {
key = sss_base64_decode(cmd_ctx,(const char *)el->values[i].data,&key_len);if (!key) {return ENOMEM;}ret = sss_packet_grow(cctx->creq->out,3*sizeof(uint32_t) + key_len + fqname_len);if (ret != EOK) {talloc_free(key);return ret;}sss_packet_get_body(cctx->creq->out, &body, &body_len);
- ret = decode_and_add_base64_data(cmd_ctx, el, fqname_len, fqname, &c);
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n");return ret;- }
SAFEALIGN_SET_UINT32(body+c, 0, &c);SAFEALIGN_SET_UINT32(body+c, fqname_len, &c);safealign_memcpy(body+c, fqname, fqname_len, &c);SAFEALIGN_SET_UINT32(body+c, key_len, &c);safealign_memcpy(body+c, key, key_len, &c);
- ret = decode_and_add_base64_data(cmd_ctx, el_orig, fqname_len, fqname, &c);
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n");return ret;- }
talloc_free(key);count++;
ret = decode_and_add_base64_data(cmd_ctx, el_override, fqname_len, fqname,
&c);if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "decode_and_add_base64_data failed.\n");return ret;}
return EOK;
-- 1.8.3.1
From 60fb14f60d9a301d6913bc1c29fc652225c3d21d Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@redhat.com Date: Tue, 28 Oct 2014 15:53:16 +0100 Subject: [PATCH 5/5] Add test for sysdb_add_overrides_to_object()
ACK
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@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@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@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@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
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@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@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@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@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@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@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@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
On Tue, Nov 04, 2014 at 07:12:14PM +0100, Lukas Slebodnik wrote:
From fc81e59f58a568af1bc6a21aced45f6b8c26db24 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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
This is the order of patches I used:
41604fc Add ssh pubkey to origbyname request 3a01245 Add test for sysdb_add_overrides_to_object() 4a9509c Views: apply user SSH public key override 0d1f3ad sysdb_add_overrides_to_object: add new parameter and multi-value support 6e334db sysdb_get_user_attr_with_views: add mandatory override attributes a083182 nss: return user_attributes in origbyname request d5d68db nss: parse user_attributes option 8f25558 Add parse_attr_list_ex() helper function 0894b61 IPA: inherit ldap_user_extra_attrs to AD subdomains 78e05ea Add add_strings_lists() utility function
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@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@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@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@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@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@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) \
added
From fc81e59f58a568af1bc6a21aced45f6b8c26db24 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Nov 04, 2014 at 08:55:28PM +0100, Sumit Bose wrote:
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@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@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@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@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@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' parameterThank 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@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) \
added
From fc81e59f58a568af1bc6a21aced45f6b8c26db24 Mon Sep 17 00:00:00 2001 From: Sumit Bose sbose@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
OK, now also the default view works!
ACK
On Wed, Nov 05, 2014 at 11:28:29AM +0100, Jakub Hrozek wrote:
OK, now also the default view works!
ACK
Pushed to master: * f9f513ee1dd4ca10ab980a180d0468ae5167d021 * a524965fbe0551f1b3a68f1e5c7a5689a652998f * ab355eced46b5f488ed62a79a7f2e5ac2b6a574c * 1a9f66352070d71a6b998c5afbc268ba6fddc51c * 16c37880f089431211290aa31bdcd3c9bc12aa77
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Sumit Bose