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(a)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.
+ 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.
- 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.
+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.
bye,
Sumit