[SSSD] [PATCH] AD: Remove GPO dependency on libsamba-security

Yassir Elley yelley at redhat.com
Fri May 23 00:09:26 UTC 2014



----- Original Message -----
> On Wed, May 14, 2014 at 05:03:20PM -0400, Yassir Elley wrote:
> > 
> > 
> > ----- Original Message -----
> > > Hi,
> > > 
> > > The attached patch addresses a previous discussion about incorporating
> > > private samba functionality into the gpo code. The consensus was:
> > > * Don't link to private samba libraries!
> > > * If possible, use equivalent sssd functions
> > > * Else, re-implement (i.e. essentially copy) the samba functions in the
> > > sssd
> > > codebase
> > > 
> > > There were three functions that the GPO code was using from
> > > libsamba-security, and this patch replaces them with sssd equivalents or
> > > samba re-implementations as follows:
> > > * samba's string_to_sid() replaced with sss_idmap_sid_to_smb_sid()
> > > * samba's dom_sid_equal() reimplemented in 35 lines
> > > * samba's ndr_pull_security_descriptor() reimplemented in 445 lines
> > > (placed
> > > in separate ad_gpo_ndr.c file b/c of size)
> > > 
> > > This patch depends on a previously submitted patch ("AD: LDAP component
> > > of
> > > GPO-based access control"), which has not yet been pushed to master, but
> > > is
> > > expected to do so in the near future.
> > > 
> > > Regards,
> > > Yassir.
> > > 
> > 
> > I have updated this patch as a diff with respect to master, in light of the
> > fact that the previously submitted patch ("AD: LDAP component of GPO-based
> > access control") on which this patch depends, has now been pushed to
> > master.
> > 
> > Regards,
> > Yassir.
> 
> > From 213387e530796fdf3ceff8afa5c87e870cf9daf1 Mon Sep 17 00:00:00 2001
> > From: Yassir Elley <yelley at redhat.com>
> > Date: Tue, 6 May 2014 12:28:04 -0400
> > Subject: [PATCH] AD-GPO: Remove dependency on libsamba-security
> > 
> > ---
> >  
> > +static bool
> > +ad_gpo_dom_sid_equal(const struct dom_sid *sid1, const struct dom_sid
> > *sid2)
> > +{
> > +    int i;
> > +
> > +    if (sid1 == sid2) {
> > +        return true;
> > +    }
> > +
> > +    if (!sid1 || !sid2) {
> > +        return false;
> > +    }
> > +
> > +    if (sid1->sid_rev_num != sid2->sid_rev_num) {
> > +        return false;
> > +    }
> > +
> > +    for (i = 0; i < 6; i++) {
> > +        if (sid1->id_auth[i] != sid2->id_auth[i]) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    if (sid1->num_auths != sid2->num_auths) {
> > +        return false;
> > +    }
> > +
> > +    for (i = 0; i < sid1->num_auths-1; i++) {
> 
> you have a fix for this in the tread with the unit tests, please add it
> here and not with the unit tests.

Done.

> 
> > +        if (sid1->sub_auths[i] != sid2->sub_auths[i]) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +
> >  /*
> >   * This function retrieves the SIDs corresponding to the input user and
> >   returns
> >   * the user_sid, group_sids, and group_size in their respective output
> >   params.
> > @@ -207,10 +244,6 @@ ad_gpo_get_sids(TALLOC_CTX *mem_ctx,
> >      return ret;
> >  }
> >  
> > -bool string_to_sid(struct dom_sid *sidout, const char *sidstr);
> > -int dom_sid_string_buf(const struct dom_sid *sid, char *buf, int buflen);
> > -bool dom_sid_equal(const struct dom_sid *sid1, const struct dom_sid
> > *sid2);
> > -
> >  /*
> >   * This function determines whether the input ACE includes any of the
> >   * client's SIDs. The boolean result is assigned to the _included output
> >   param.
> > @@ -223,38 +256,62 @@ ad_gpo_ace_includes_client_sid(const char *user_sid,
> >                                 bool *_included)
> >  {
> >      int i = 0;
> > +    int ret = 0;
> >      struct dom_sid ace_dom_sid;
> > -    struct dom_sid user_dom_sid;
> > -    struct dom_sid group_dom_sid;
> > -    char buf[SID_MAX_LEN + 1];
> > +    struct dom_sid *user_dom_sid;
> > +    struct dom_sid *group_dom_sid;
> > +    TALLOC_CTX *tmp_ctx;
> > +    struct sss_idmap_ctx *idmap_ctx;
> > +    enum idmap_error_code err;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (tmp_ctx == NULL) {
> > +        ret = ENOMEM;
> > +        goto done;
> > +    }
> >  
> >      ace_dom_sid = ace->trustee;
> >  
> > -    dom_sid_string_buf(&ace_dom_sid, buf, SID_MAX_LEN);
> > +    err = sss_idmap_init(sss_idmap_talloc, tmp_ctx, sss_idmap_talloc_free,
> > +                         &idmap_ctx);
> > +    if (err != IDMAP_SUCCESS) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to initialize idmap
> > context.\n");
> > +        ret = EFAULT;
> > +        goto done;
> > +    }
> 
> I would suggest to pass the idmap_ctx down here. It is e.g. available in
> the sdap_options in the state in ad_gpo_process_gpo_done(). It's quite a
> way to pass it down but it safes some unneeded memory allocations. If
> you do this, please do not forget to call sss_idmap_free_smb_sid() on
> the allocated objects here.

Done. I also removed tmp_ctx, since I was no longer using it.

> 
> >  
> > -    if (!string_to_sid(&user_dom_sid, user_sid)) {
> > -        DEBUG(SSSDBG_OP_FAILURE, "string_to_sid failed\n");
> > -        return EINVAL;
> > +    err = sss_idmap_sid_to_smb_sid(idmap_ctx, user_sid, &user_dom_sid);
> > +    if (err != IDMAP_SUCCESS) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to initialize idmap
> > context.\n");
> > +        ret = EFAULT;
> > +        goto done;
> >      }
> >  
> > -    if (dom_sid_equal(&ace_dom_sid, &user_dom_sid)) {
> > +    if (ad_gpo_dom_sid_equal(&ace_dom_sid, user_dom_sid)) {
> >          *_included = true;
> > -        return EOK;
> > +        ret = EOK;
> > +        goto done;
> >      }
> >  
> >      for (i = 0; i < group_size; i++) {
> > -        if (!string_to_sid(&group_dom_sid, group_sids[i])) {
> > -            DEBUG(SSSDBG_OP_FAILURE, "string_to_sid failed\n");
> > -            return EINVAL;
> > +        err = sss_idmap_sid_to_smb_sid(idmap_ctx, group_sids[i],
> > &group_dom_sid);
> > +        if (err != IDMAP_SUCCESS) {
> > +            DEBUG(SSSDBG_CRIT_FAILURE, "Failed to initialize idmap
> > context.\n");
> > +            ret = EFAULT;
> > +            goto done;
> >          }
> > -        if (dom_sid_equal(&ace_dom_sid, &group_dom_sid)) {
> > +        if (ad_gpo_dom_sid_equal(&ace_dom_sid, group_dom_sid)) {
> >              *_included = true;
> > -            return EOK;
> > +            ret = EOK;
> > +            goto done;
> >          }
> >      }
> >  
> >      *_included = false;
> > -    return EOK;
> > +
> > + done:
> > +    talloc_free(tmp_ctx);
> > +    return ret;
> >  }
> >  
> >  /*
> > @@ -707,7 +764,6 @@ ad_gpo_target_dn_retrieval_done(struct tevent_req
> > *subreq)
> >                 ret, sss_strerror(ret));
> >          goto done;
> >      }
> > -
> >      state->target_dn = talloc_steal(state, target_dn);
> >      if (state->target_dn == NULL) {
> >          ret = ENOMEM;
> > @@ -1878,8 +1934,8 @@ ad_gpo_parse_machine_ext_names(TALLOC_CTX *mem_ctx,
> >  }
> >  
> >  enum ndr_err_code
> > -ndr_pull_security_descriptor(struct ndr_pull *ndr, int ndr_flags,
> > -                             struct security_descriptor *r);
> 
> I will talk with Andreas an Günther is ndr_pull_security_descriptor()
> can be made available in a public ndr library.

That would be excellent!! :)

> 
> > +ad_gpo_ndr_pull_security_descriptor(struct ndr_pull *ndr, int ndr_flags,
> > +                                    struct security_descriptor *r);
> >  
> >  /*
> >   * This function parses the input data blob and assigns the resulting
> > @@ -1894,6 +1950,7 @@ static errno_t ad_gpo_parse_sd(TALLOC_CTX *mem_ctx,
> >      struct ndr_pull *ndr_pull = NULL;
> >      struct security_descriptor sd;
> >      DATA_BLOB blob;
> > +    enum ndr_err_code ndr_err;
> >  
> >      blob.data = data;
> >      blob.length = length;
> > @@ -1904,7 +1961,14 @@ static errno_t ad_gpo_parse_sd(TALLOC_CTX *mem_ctx,
> >          return EINVAL;
> >      }
> >  
> > -    ndr_pull_security_descriptor(ndr_pull, NDR_SCALARS|NDR_BUFFERS, &sd);
> > +    ndr_err = ad_gpo_ndr_pull_security_descriptor(ndr_pull,
> > +                                                  NDR_SCALARS|NDR_BUFFERS,
> > +                                                  &sd);
> > +
> > +    if (ndr_err != NDR_ERR_SUCCESS) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to pull security
> > descriptor\n");
> > +        return EINVAL;
> > +    }
> >  
> >      *_gpo_sd = talloc_memdup(mem_ctx, &sd, sizeof(struct
> >      security_descriptor));
> >  
> > diff --git a/src/providers/ad/ad_gpo.h b/src/providers/ad/ad_gpo.h
> > index
> > 856013e43688bb8098f900184616ac33acb72438..851195d11e56df08ce8bc2619ee0e05dda390b30
> > 100644
> > --- a/src/providers/ad/ad_gpo.h
> > +++ b/src/providers/ad/ad_gpo.h
> > @@ -50,6 +50,4 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
> >  
> >  errno_t ad_gpo_access_recv(struct tevent_req *req);
> >  
> > -struct security_descriptor;
> > -
> >  #endif /* AD_GPO_H_ */
> > diff --git a/src/providers/ad/ad_gpo_ndr.c b/src/providers/ad/ad_gpo_ndr.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..0ee28f0972ce9d8e0c651fc2a03b2c0641f7ad58
> > --- /dev/null
> > +++ b/src/providers/ad/ad_gpo_ndr.c
> 
> I assume this file contains a copy of the generated ndr code from the
> samba tree, so I didn't looked closer here.

I am using code from samba's ndr_security.c, but I only took the functions I need. In other words, ad_gpo_ndr.c is a subset of ndr_security.c.

I have attached a revised patch.

Regards,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-GPO-Remove-dependency-on-libsamba-security.patch
Type: text/x-patch
Size: 29168 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140522/e40f858e/attachment.bin>


More information about the sssd-devel mailing list