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

Sumit Bose sbose at redhat.com
Tue Jan 20 16:12:06 UTC 2015


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 at 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 at 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 at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list