On Tue, Jan 20, 2015 at 04:05:08PM +0100, Jakub Hrozek wrote:
On Tue, Jan 20, 2015 at 01:59:48PM +0100, Sumit Bose wrote:
> Hi,
>
> The original values of memberOf and the original DN are used in the HBAC
> evaluation and should be send to the IPA client for AD users via the
> extdom plugin. The attached patches adds those attribute to the response
> of of the getorig request. No changes in the extdom plugin are needed.
>
> Since originalMemberOf is a multi-value attribute fill_orig() must be
> able to handle multi-value attributes. The first patch adds the needed
> changes while the second refactors fill_orig() a bit. To make reviewing
> easier I didn't squash them together. The last patch adds the new
> attributes to the getorig request.
>
> bye,
> Sumit
The patches work well in my testing. I've submitted them for automated
tests. See two questions inline:
> From 8ea260876027dbf312c447b615e740986a990f03 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Tue, 20 Jan 2015 12:48:19 +0100
> Subject: [PATCH 1/3] nss: make fill_orig() multi-value aware
>
> ---
> src/responder/nss/nsssrv_cmd.c | 86 ++++++++++++++++++++------
> src/tests/cmocka/test_nss_srv.c | 131 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 197 insertions(+), 20 deletions(-)
>
> diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> index
3c5d450714fb3f7655cd32aeef900b4f5e9782c7..6bb03d81b355fb2aa06508b4d9b756dd6e2391e6 100644
> --- a/src/responder/nss/nsssrv_cmd.c
> +++ b/src/responder/nss/nsssrv_cmd.c
> @@ -4612,13 +4612,14 @@ static errno_t fill_orig(struct sss_packet *packet,
> {
> int ret;
> TALLOC_CTX *tmp_ctx;
> - const char *tmp_str;
> uint8_t *body;
> size_t blen;
> size_t pctr = 0;
> size_t c;
> + size_t d;
> size_t sum;
> size_t found;
> + size_t array_size;
> size_t extra_attrs_count = 0;
> const char **extra_attrs_list = NULL;
> const char *orig_attr_list[] = {SYSDB_SID_STR,
> @@ -4637,6 +4638,8 @@ static errno_t fill_orig(struct sss_packet *packet,
> struct sized_string *keys;
> struct sized_string *vals;
> struct nss_ctx *nctx;
> + struct ldb_message_element *el;
> + struct ldb_val *val;
>
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
> @@ -4652,10 +4655,9 @@ static errno_t fill_orig(struct sss_packet *packet,
> extra_attrs_count++);
> }
>
> - keys = talloc_array(tmp_ctx, struct sized_string,
> - sizeof(orig_attr_list) + extra_attrs_count);
> - vals = talloc_array(tmp_ctx, struct sized_string,
> - sizeof(orig_attr_list) + extra_attrs_count);
> + array_size = sizeof(orig_attr_list) + extra_attrs_count;
> + keys = talloc_array(tmp_ctx, struct sized_string, array_size);
> + vals = talloc_array(tmp_ctx, struct sized_string, array_size);
> if (keys == NULL || vals == NULL) {
> DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> ret = ENOMEM;
> @@ -4665,26 +4667,72 @@ static errno_t fill_orig(struct sss_packet *packet,
> sum = 0;
> found = 0;
> for (c = 0; orig_attr_list[c] != NULL; c++) {
> - tmp_str = ldb_msg_find_attr_as_string(msg, orig_attr_list[c], NULL);
> - if (tmp_str != NULL) {
> - to_sized_string(&keys[found], orig_attr_list[c]);
> - sum += keys[found].len;
> - to_sized_string(&vals[found], tmp_str);
> - sum += vals[found].len;
> + el = ldb_msg_find_element(msg, orig_attr_list[c]);
> + if (el != NULL && el->num_values > 0) {
> + if (el->num_values > 1) {
> + array_size += el->num_values;
Aren't we allocating one more sized_string than required here? We
already allocated one item previously..
Not a big deal though, it's just tmp_ctx memory..
yes, you are right, but since I was already generous in the initial
allocation by using just the full number of possible attributes and not
checking if they are already set or not I thought I can skip doing the
-1 here as well. But if you prefer I can add it.
> + keys = talloc_realloc(tmp_ctx, keys, struct sized_string,
> + array_size);
> + vals = talloc_realloc(tmp_ctx, vals, struct sized_string,
> + array_size);
> + if (keys == NULL || vals == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> + ret = ENOMEM;
> + goto done;
> + }
> + }
> + for (d = 0; d < el->num_values; d++) {
> + to_sized_string(&keys[found], orig_attr_list[c]);
> + sum += keys[found].len;
> + val = &(el->values[d]);
> + if (val == NULL || val->data == NULL
> + || val->data[val->length] != '\0') {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "Unexpected attribute value found for
[%s].\n",
> + orig_attr_list[c]);
> + ret = EINVAL;
> + goto done;
> + }
> + to_sized_string(&vals[found], (const char *)val->data);
> + sum += vals[found].len;
>
> - found++;
> + found++;
> + }
> }
> }
>
> for (c = 0; c < extra_attrs_count; c++) {
> - tmp_str = ldb_msg_find_attr_as_string(msg, extra_attrs_list[c], NULL);
> - if (tmp_str != NULL) {
> - to_sized_string(&keys[found], extra_attrs_list[c]);
> - sum += keys[found].len;
> - to_sized_string(&vals[found], tmp_str);
> - sum += vals[found].len;
> + el = ldb_msg_find_element(msg, extra_attrs_list[c]);
> + if (el != NULL && el->num_values > 0) {
> + if (el->num_values > 1) {
> + array_size += el->num_values;
> + keys = talloc_realloc(tmp_ctx, keys, struct sized_string,
> + array_size);
> + vals = talloc_realloc(tmp_ctx, vals, struct sized_string,
> + array_size);
> + if (keys == NULL || vals == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> + ret = ENOMEM;
> + goto done;
> + }
> + }
> + for (d = 0; d < el->num_values; d++) {
> + to_sized_string(&keys[found], extra_attrs_list[c]);
> + sum += keys[found].len;
> + val = &(el->values[d]);
> + if (val == NULL || val->data == NULL
> + || val->data[val->length] != '\0') {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "Unexpected attribute value found for
[%s].\n",
> + orig_attr_list[c]);
> + ret = EINVAL;
> + goto done;
I was wondering for a bit if it makes any sense to skip the malformed
items. But IIRC ldb guarantees the data to be NULL-terminated, so I
guess it makes little sense to over-engineer for a condition that won't
happen.
I picked it from ldb_msg_find_attr_as_string() so at least doing the
test will not be a performance degradation and since it should never
happen I think we should report an error when it happens.
bye,
Sumit
> + }
> + to_sized_string(&vals[found], (const char *)val->data);
> + sum += vals[found].len;
>
> - found++;
> + found++;
> + }
> }
> }
>
> From e44218e547c894da90e02bebf38aedd27d2712c7 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Tue, 20 Jan 2015 13:50:16 +0100
> Subject: [PATCH 2/3] nss: refactor fill_orig()
ACK
> From 97e50fcfaf11056935a40c3df1b2b9b7b4b1aec7 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Tue, 20 Jan 2015 12:51:57 +0100
> Subject: [PATCH 3/3] nss: Add original DN and memberOf to origbyname request
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel