[PATCHES] nss: Add original DN and memberOf to origbyname request

[Sumit Bose]

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

[Jakub Hrozek]

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 
> 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..
>            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.
>            }
>            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 
> 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 
> Date: Tue, 20 Jan 2015 12:51:57 +0100
> Subject: [PATCH 3/3] nss: Add original DN and memberOf to origbyname request

ACK

[Jakub Hrozek]

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:

btw the tests went fine (sans some problems we have with RHEL-6 test
machine), so I'm inclined to give my ACK.

[Jakub Hrozek]

On Tue, Jan 20, 2015 at 05:01:07PM +0100, Jakub Hrozek wrote:
> 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:
>
> btw the tests went fine (sans some problems we have with RHEL-6 test
> machine), so I'm inclined to give my ACK.

Pushed to master:
    * 7543052f562f157f7b17fdc46a6777d80c0cb3bd
    * a4d64002b5ca763622bde240d27797d361ba0388
    * 5f4d896ec8e06476f4282b562b1044de14c48ecf
sssd-1-12:
    * dcc99fc87bc7ec44fdc8ec897218384cc274d4dd
    * 2eb78055d7a344c0ef58adbaa84dac86df13174e
    * 70ec6df14be2ddc26147095e260b4f9c7e606a6b

[Sumit Bose]

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 
> > 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 
> > 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 
> > 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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Tue, Jan 20, 2015 at 05:12:06PM +0100, Sumit Bose wrote:
> > >    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.

It's fine since it's a tmp_ctx.

ACK to the first patch as well.