[SSSD] [PATCH] AD: LDAP component of GPO-based access control

Sumit Bose sbose at redhat.com
Mon Apr 14 08:22:59 UTC 2014


On Fri, Apr 11, 2014 at 06:32:05PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > On Thu, Apr 03, 2014 at 12:30:17PM -0400, Yassir Elley wrote:
> > > 
> > > 
> > > ----- Original Message -----
> > 
> > Hi,
> > 
> > I've read the patches and did some basic tests. You can find my review
> > comments in-line below. I had to add some NULL-checks to make my tests
> > work, see details below as well.
> 
> Thanks for the thorough review. Much appreciated!
> 
> > 
> > In general I think you should add an option to disable the GPO checks to
> > be able to resolve regressions in existing setups.
> 
> I'm not sure I understand this comment. What do you mean by "existing setups"? If you mean GPO-unaware setups, then the GPO code should not break any existing regressions. If it did, we would want to know about it, right?

There might be setup where the GPO would in theory apply to a Linux
client running SSSD but since we didn't support it before access was
always granted. If now the GPO is evaluated a user might be rejected
although he was able to log in before. In this case it should be
possible to disable the GPO check to return the previous behaviour
without completely disabling access control.

> 
> > 
> > I don't know if you already have some unit tests in your tree, but there
> > are several functions in your patch where unit tests would be nice.
> 
> If I create some unit tests, should I include them in the next patch? Or should the next patch to the list be limited to addressing your review comments? I just don't know the process.

If I add a single new function I typically add the unit tests in the
same patch. In your case I think it would be more useful to create a
separate patch only with the unit tests.

> 
> > 
> > If to comments are addressed we should push the patch to master to get
> > more testing, coverity checks etc.
> 
> Sounds good. Below, I have included a few responses/questions related to your comments. If I have not responded to a comment, that means I agree with it and will make the necessary change. Is that OK? Or is it preferrable to give positive acknowledgment of each comment?

No, comments are only necessary where you disagree.

> 
> Thanks,
> Yassir.
> 
> > 
> > bye,
> > Sumit
> > 
> > 
> > > From a508e36e67aa881665d0387ff418162e5f1825cd Mon Sep 17 00:00:00 2001
> > > From: Yassir Elley <yelley at redhat.com>
> > > Date: Mon, 20 Jan 2014 11:17:06 -0500
> > > Subject: [PATCH] Implemented LDAP component of GPO-based access control
> > > 
> > > ---
> > >  Makefile.am                       |    3 +
> > >  src/providers/ad/ad_access.c      |  125 ++-
> > >  src/providers/ad/ad_domain_info.c |   34 +-
> > >  src/providers/ad/ad_domain_info.h |    1 +
> > >  src/providers/ad/ad_gpo.c         | 2199
> > >  +++++++++++++++++++++++++++++++++++++
> > >  src/providers/ad/ad_gpo.h         |   55 +
> > >  src/providers/ad/ad_id.c          |    3 +-
> > >  src/providers/ad/ad_subdomains.c  |    3 +-
> > >  src/providers/ldap/sdap_async.c   |  176 +++
> > >  src/providers/ldap/sdap_async.h   |   14 +
> > >  src/util/sss_ldap.h               |    9 +
> > >  11 files changed, 2571 insertions(+), 51 deletions(-)
> > >  create mode 100644 src/providers/ad/ad_gpo.c
> > >  create mode 100644 src/providers/ad/ad_gpo.h
> > > 
> > > diff --git a/Makefile.am b/Makefile.am
> > > index f319624..2935d15 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -1608,6 +1608,7 @@ ad_access_filter_tests_LDADD = \
> > >      libsss_ldap_common.la \
> > >      libsss_idmap.la \
> > >      libsss_krb5_common.la \
> > > +    libsss_ad.la \
> > >      libsss_test_common.la
> > 
> > I see
> > 
> >   CCLD     ad_access_filter_tests
> > 
> > *** Warning: Linking the executable ad_access_filter_tests against the
> > loadable module
> > *** libsss_ad.so is not portable!
> > 
> > when running make check, I think you have to replace libsss_ad.la by the list
> > of source files here
> 
> I didn't understand this comment. Are you suggesting inserting ${libsss_ad_la_SOURCES} in the LDADD line? I don't think the linker deals with source files.

yes, I meant to add the list of source files to the _SOURCES variable of
ad_access_filter_tests.

> 
> Also, since we have full control of libsss_ad.so, and since the unit tests will always be built together with the current version of libsss_ad.so, why does it matter that linking against libsss_ad.so is not portable. I thought that this warning would only be relevant if we were linking against a library that we had no control over.

I think this warning indicates that libtool cannot link the symbol of a
shared library into a binary (the symbols are resolved at build time
here and not at runtime which would be typical for a loadable module) on
all platforms. Maybe this is even dependant on the libtool version and
might fail on Linux platforms with older libtool?

As an alternative to add the source files you might want to create a
libsss_ad_common.la similar to libsss_ldap_common.la or
libsss_krb5_common.la?

> 
> > 
> > ...
> > 
> > > -    state->cindex++;
> > > -    if (state->clist[state->cindex] == NULL) {
> > > -        DEBUG(SSSDBG_OP_FAILURE,
> > > -            "Error retrieving access check result: %s\n",
> > > -            sss_strerror(ret));
> > > -        tevent_req_error(req, ret);
> > > -        return;
> > > +        /* If possible, retry with LDAP */
> > > +        state->cindex++;
> > > +        if (state->clist[state->cindex] == NULL) {
> > > +            DEBUG(SSSDBG_OP_FAILURE,
> > > +                  "Error retrieving access check result: %s\n",
> > > +                  sss_strerror(ret));
> > > +            tevent_req_error(req, ret);
> > > +            return;
> > > +        }
> > > +
> > > +        ret = ad_sdap_access_step(req, state->clist[state->cindex]);
> > > +        if (ret != EOK) {
> > > +            tevent_req_error(req, ret);
> > > +            return;
> > > +        }
> > > +
> > > +        /* Another check in progress */
> > 
> > You have to call return here because ad_sdap_access_step() will create a new
> > request which will return to ad_sdap_access_done().
> > 
> > >      }
> > >  
> > > -    ret = ad_access_step(req, state->clist[state->cindex]);
> > > -    if (ret != EOK) {
> > > -        tevent_req_error(req, ret);
> > > +    subreq = ad_gpo_access_send(state,
> > > +                                state->be_ctx->ev,
> > > +                                state->domain,
> > > +                                state->ctx,
> > > +                                state->pd->user);
> > > +
> > > +    if (!subreq) {
> > > +        tevent_req_error(req, ENOMEM);
> > >          return;
> > >      }
> > >  
> > > -    /* Another check in progress */
> > > +    tevent_req_set_callback(subreq, ad_gpo_access_done, req);
> > >  }
> > 
> > ...
> > 
> > >  
> > >  
> > >  static void
> > > -ad_access_check_done(struct tevent_req *req)
> > > +ad_access_done(struct tevent_req *req)
> > >  {
> > >      errno_t ret;
> > >      struct be_req *breq =
> > >              tevent_req_callback_data(req, struct be_req);
> > >      struct pam_data *pd =
> > >                      talloc_get_type(be_req_get_data(breq), struct
> > >                      pam_data);
> > > -
> > 
> > Please do not remove this empty line, it separates declarations from the
> > code.
> > There is no formal requirement in the coding-style but I think it helps
> > reading
> > the code nonetheless.
> 
> Agreed.
> 
> I noticed that some other functions in this file also do not separate declarations from the code with an empty line. Should I fix those as well in this patch (even though those functions are not my code)? Or is that a separate patch? In general, is there a policy of limiting the next patch to only addressing code review comments (and not inserting additional fixes like the ones here).

There is no need to change code which you do not touch. If you find
typos, coding style violations or other issues in the code you can send
them in a new patch in a new email thread.

About addressing review comments, in general that changes are not send
in a separate patch but squashed into the original patches. It's a bit
more burden on the reviewer but I think patches especially for new
features should stand on their own. Because it should be easy for a
person who looks at the patch later to understand it. As with source
code I think patches are more often read then reviewed.

Additionally I think it's not the task of the person who commits the
patches to squash the changes together before committing the. With patch
series it might not be obvious which change belongs to which patch,
there might be conflicts  which are hard for the committer to resolve
because he does not know the code.

> 
> > 
> > >      ret = ad_access_recv(req);
> > >      talloc_zfree(req);
> > >      switch (ret) {
> > > +

...

> > > +    num_enabled = 0;
> > > +    ptr = raw_gplink_value;
> > > +    for (i = 0; i < gplink_count; i++) {
> > > +        first = ptr + 1;
> > > +        last = strchr(first, delim);
> > > +        if (last == NULL) {
> > > +            break;
> > > +        }
> > > +        *last = '\0';
> > > +        last++;
> > > +        dn = first;
> > > +        if ( strncasecmp(dn, "LDAP://", 7)== 0 ) {
> > > +            dn = dn + 7;
> > > +        }
> > 
> > are you sure this can only be LDAP:// ? I wonder if it can be LDAPS:// is the
> > original request is using LDAPS? But since AD does not activate LDAPS by
> > default it might be safe.
> 
> The [MS-GPOL] spec indicates that the "LDAP://" prefix should be stripped from each GPO DN before being used in an LDAP search.

Ok, looks like we are safe then.

> 
> > 
> > > +        gplink_options = strchr(first, ';');
> > > +        if (gplink_options == NULL) {
> > > +            break;
> > > +        }
> > > +        *gplink_options = '\0';
> > > +        gplink_options++;
> > > +
> > > +        gplink_number = atoi(gplink_options);
> > 

...

> > > +    enforced_gpo_dns[num_enforced] = NULL;
> > > +    unenforced_gpo_dns[num_unenforced] = NULL;
> > > +
> > > +    candidate_gpos = talloc_array(tmp_ctx,
> > > +                                  struct gp_gpo *,
> > > +                                  num_candidate_gpos + 1);
> > > +
> > > +    if (candidate_gpos == NULL) {
> > > +        ret = ENOMEM;
> > > +        goto done;
> > > +    }
> > > +
> > > +    gpo_dn_idx = 0;
> > > +    for (i = num_unenforced - 1; i >= 0; i--) {
> > > +        candidate_gpos[gpo_dn_idx] = talloc_zero(candidate_gpos, struct
> > > gp_gpo);
> > > +        if (candidate_gpos[gpo_dn_idx] == NULL) {
> > > +            ret = ENOMEM;
> > > +            goto done;
> > > +        }
> > > +        candidate_gpos[gpo_dn_idx]->gpo_dn =
> > > +            talloc_strdup(candidate_gpos[gpo_dn_idx],
> > > unenforced_gpo_dns[i]);
> > > +
> > 
> > Is there a reason for adding the unenforced_gpo_dns in reverse order compared
> > to enforced_gpo_dns?
> 
> The [MS-GPOL] spec indicates that unenforced gpo_dns must be *prepended*, while enforced_gpo_dns must be *appended*. I implemented prepending of unenforced_gpo_dns by adding them in reverse order. 

Makes sense, thank you for the explanation.

bye,
Sumit



More information about the sssd-devel mailing list