[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