----- 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@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.