The Samba-Bugzilla – Attachment 13625 Details for
Bug 13046
libgpo doesn't sort the GPOs in the correct order.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Updated git-am patch for master with loop termination fix.
google-gpo-fix1 (text/plain), 14.07 KB, created by
Jeremy Allison
on 2017-09-22 17:37:57 UTC
(
hide
)
Description:
Updated git-am patch for master with loop termination fix.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2017-09-22 17:37:57 UTC
Size:
14.07 KB
patch
obsolete
>From cd8c2c5a87ff7609f50a05a54895fdd8d78ac7de Mon Sep 17 00:00:00 2001 >From: Lutz Justen <ljusten@google.com> >Date: Thu, 21 Sep 2017 10:01:58 -0700 >Subject: [PATCH 1/3] lib: gpo: Changes order to match GPO application order. > >The order of GPOs in a gpo_list generated by ads_get_gpo_list >did not match the order of application. Since GPOs are pushed >to the FRONT of gpo_list, GPOs have to be pushed in the opposite >order of application. (Pushing to front is useful to get >inheritance blocking right). > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13046 > >Signed-off-by: Lutz Justen <ljusten@google.com> >Reviewed-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Guenther Deschner <gd@samba.org> >--- > libgpo/gpo_ldap.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > >diff --git a/libgpo/gpo_ldap.c b/libgpo/gpo_ldap.c >index 4533d61a1e3..7ede12c3c21 100644 >--- a/libgpo/gpo_ldap.c >+++ b/libgpo/gpo_ldap.c >@@ -812,12 +812,6 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > dump_gplink(&gp_link); > } > >- /* block inheritance from now on */ >- if (gp_link.gp_opts & >- GPOPTIONS_BLOCK_INHERITANCE) { >- add_only_forced_gpos = true; >- } >- > status = add_gplink_to_gpo_list(ads, > mem_ctx, > gpo_list, >@@ -829,6 +823,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > if (!ADS_ERR_OK(status)) { > return status; > } >+ >+ /* block inheritance from now on */ >+ if (gp_link.gp_opts & >+ GPOPTIONS_BLOCK_INHERITANCE) { >+ add_only_forced_gpos = true; >+ } > } > } > >@@ -858,12 +858,6 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > dump_gplink(&gp_link); > } > >- /* block inheritance from now on */ >- if (gp_link.gp_opts & >- GPOPTIONS_BLOCK_INHERITANCE) { >- add_only_forced_gpos = true; >- } >- > status = add_gplink_to_gpo_list(ads, > mem_ctx, > gpo_list, >@@ -875,6 +869,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > if (!ADS_ERR_OK(status)) { > return status; > } >+ >+ /* block inheritance from now on */ >+ if (gp_link.gp_opts & >+ GPOPTIONS_BLOCK_INHERITANCE) { >+ add_only_forced_gpos = true; >+ } > } > } > >-- >2.11.0 > > >From b5301a00533a95f64d18d1444f621545e2f45bb8 Mon Sep 17 00:00:00 2001 >From: Lutz Justen <ljusten@google.com> >Date: Thu, 21 Sep 2017 10:11:15 -0700 >Subject: [PATCH 2/3] lib: gpo: Fixes issue with GPOPTIONS_BLOCK_INHERITANCE. > >GP links with the GPOPTIONS_BLOCK_INHERITANCE option set >were blocking GPOs from the same link (i.e. an OU with >the flag set would block its own GPOs). This patch makes >sure the GPOs from the link are added to the list. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13046 > >Signed-off-by: Lutz Justen <ljusten@google.com> >Reviewed-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Guenther Deschner <gd@samba.org> >--- > libgpo/gpo_ldap.c | 140 ++++++++++++++++++++++++++++++------------------------ > 1 file changed, 77 insertions(+), 63 deletions(-) > >diff --git a/libgpo/gpo_ldap.c b/libgpo/gpo_ldap.c >index 7ede12c3c21..8fb5fdc40c1 100644 >--- a/libgpo/gpo_ldap.c >+++ b/libgpo/gpo_ldap.c >@@ -561,10 +561,16 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, > const struct security_token *token) > { > ADS_STATUS status; >- int i; >- >- for (i = 0; i < gp_link->num_links; i++) { >- >+ uint32_t count; >+ >+ /* >+ * Note: DLIST_ADD pushes to the front, >+ * so loop from last to first to get the >+ * order right. >+ */ >+ for (count = gp_link->num_links; count > 0; count--) { >+ /* NB. Index into arrays is one less than counter. */ >+ uint32_t i = count - 1; > struct GROUP_POLICY_OBJECT *new_gpo = NULL; > > if (gp_link->link_opts[i] & GPO_LINK_OPT_DISABLED) { >@@ -726,7 +732,15 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > const struct security_token *token, > struct GROUP_POLICY_OBJECT **gpo_list) > { >- /* (L)ocal (S)ite (D)omain (O)rganizational(U)nit */ >+ /* >+ * Push GPOs to gpo_list so that the traversal order of the list matches >+ * the order of application: >+ * (L)ocal (S)ite (D)omain (O)rganizational(U)nit >+ * Within domains and OUs: parent-to-child. >+ * Since GPOs are pushed to the front of gpo_list, GPOs have to be >+ * pushed in the opposite order of application (OUs first, local last, >+ * child-to-parent). >+ */ > > ADS_STATUS status; > struct GP_LINK gp_link; >@@ -745,63 +759,18 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > > DEBUG(10,("ads_get_gpo_list: getting GPO list for [%s]\n", dn)); > >- /* (L)ocal */ >- status = add_local_policy_to_gpo_list(mem_ctx, gpo_list, >- GP_LINK_LOCAL); >- if (!ADS_ERR_OK(status)) { >- return status; >- } >- >- /* (S)ite */ >- >- /* are site GPOs valid for users as well ??? */ >- if (flags & GPO_LIST_FLAG_MACHINE) { >- >- status = ads_site_dn_for_machine(ads, mem_ctx, >- ads->config.ldap_server_name, >- &site_dn); >- if (!ADS_ERR_OK(status)) { >- return status; >- } >- >- DEBUG(10,("ads_get_gpo_list: query SITE: [%s] for GPOs\n", >- site_dn)); >- >- status = ads_get_gpo_link(ads, mem_ctx, site_dn, &gp_link); >- if (ADS_ERR_OK(status)) { >- >- if (DEBUGLEVEL >= 100) { >- dump_gplink(&gp_link); >- } >- >- status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, >- site_dn, &gp_link, >- GP_LINK_SITE, >- add_only_forced_gpos, >- token); >- if (!ADS_ERR_OK(status)) { >- return status; >- } >- >- if (flags & GPO_LIST_FLAG_SITEONLY) { >- return ADS_ERROR(LDAP_SUCCESS); >- } >- >- /* inheritance can't be blocked at the site level */ >- } >- } >- > tmp_dn = dn; > > while ((parent_dn = ads_parent_dn(tmp_dn)) && > (!strequal(parent_dn, ads_parent_dn(ads->config.bind_path)))) { > >- /* (D)omain */ > >- /* An account can just be a member of one domain */ >- if (strncmp(parent_dn, "DC=", strlen("DC=")) == 0) { >+ /* (O)rganizational(U)nit */ > >- DEBUG(10,("ads_get_gpo_list: query DC: [%s] for GPOs\n", >+ /* An account can be a member of more OUs */ >+ if (strncmp(parent_dn, "OU=", strlen("OU=")) == 0) { >+ >+ DEBUG(10,("ads_get_gpo_list: query OU: [%s] for GPOs\n", > parent_dn)); > > status = ads_get_gpo_link(ads, mem_ctx, parent_dn, >@@ -817,7 +786,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > gpo_list, > parent_dn, > &gp_link, >- GP_LINK_DOMAIN, >+ GP_LINK_OU, > add_only_forced_gpos, > token); > if (!ADS_ERR_OK(status)) { >@@ -833,6 +802,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > } > > tmp_dn = parent_dn; >+ > } > > /* reset dn again */ >@@ -841,13 +811,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > while ((parent_dn = ads_parent_dn(tmp_dn)) && > (!strequal(parent_dn, ads_parent_dn(ads->config.bind_path)))) { > >+ /* (D)omain */ > >- /* (O)rganizational(U)nit */ >- >- /* An account can be a member of more OUs */ >- if (strncmp(parent_dn, "OU=", strlen("OU=")) == 0) { >+ /* An account can just be a member of one domain */ >+ if (strncmp(parent_dn, "DC=", strlen("DC=")) == 0) { > >- DEBUG(10,("ads_get_gpo_list: query OU: [%s] for GPOs\n", >+ DEBUG(10,("ads_get_gpo_list: query DC: [%s] for GPOs\n", > parent_dn)); > > status = ads_get_gpo_link(ads, mem_ctx, parent_dn, >@@ -863,7 +832,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > gpo_list, > parent_dn, > &gp_link, >- GP_LINK_OU, >+ GP_LINK_DOMAIN, > add_only_forced_gpos, > token); > if (!ADS_ERR_OK(status)) { >@@ -879,8 +848,53 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > } > > tmp_dn = parent_dn; >+ } >+ >+ /* (S)ite */ >+ >+ /* are site GPOs valid for users as well ??? */ >+ if (flags & GPO_LIST_FLAG_MACHINE) { > >- }; >+ status = ads_site_dn_for_machine(ads, mem_ctx, >+ ads->config.ldap_server_name, >+ &site_dn); >+ if (!ADS_ERR_OK(status)) { >+ return status; >+ } >+ >+ DEBUG(10,("ads_get_gpo_list: query SITE: [%s] for GPOs\n", >+ site_dn)); >+ >+ status = ads_get_gpo_link(ads, mem_ctx, site_dn, &gp_link); >+ if (ADS_ERR_OK(status)) { >+ >+ if (DEBUGLEVEL >= 100) { >+ dump_gplink(&gp_link); >+ } >+ >+ status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, >+ site_dn, &gp_link, >+ GP_LINK_SITE, >+ add_only_forced_gpos, >+ token); >+ if (!ADS_ERR_OK(status)) { >+ return status; >+ } >+ >+ if (flags & GPO_LIST_FLAG_SITEONLY) { >+ return ADS_ERROR(LDAP_SUCCESS); >+ } >+ >+ /* inheritance can't be blocked at the site level */ >+ } >+ } >+ >+ /* (L)ocal */ >+ status = add_local_policy_to_gpo_list(mem_ctx, gpo_list, >+ GP_LINK_LOCAL); >+ if (!ADS_ERR_OK(status)) { >+ return status; >+ } > > return ADS_ERROR(LDAP_SUCCESS); > } >-- >2.11.0 > > >From 1b6a6803cee00a00bcbdbb5488ff212a1e746b78 Mon Sep 17 00:00:00 2001 >From: Lutz Justen <ljusten@google.com> >Date: Thu, 21 Sep 2017 10:32:05 -0700 >Subject: [PATCH 3/3] lib: gpo: Put enforced GPOs at the end of the list. > >Enforced GPOs should be applied on top of all non-enforced GPOs, >so that they override policies set in non-enforced GPOs. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13046 > >Signed-off-by: Lutz Justen <ljusten@google.com> >Reviewed-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Guenther Deschner <gd@samba.org> >--- > libgpo/gpo_ldap.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 54 insertions(+), 8 deletions(-) > >diff --git a/libgpo/gpo_ldap.c b/libgpo/gpo_ldap.c >index 8fb5fdc40c1..80628786753 100644 >--- a/libgpo/gpo_ldap.c >+++ b/libgpo/gpo_ldap.c >@@ -554,6 +554,7 @@ ADS_STATUS ads_get_gpo(ADS_STRUCT *ads, > static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, > TALLOC_CTX *mem_ctx, > struct GROUP_POLICY_OBJECT **gpo_list, >+ struct GROUP_POLICY_OBJECT **forced_gpo_list, > const char *link_dn, > struct GP_LINK *gp_link, > enum GPO_LINK_TYPE link_type, >@@ -571,7 +572,10 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, > for (count = gp_link->num_links; count > 0; count--) { > /* NB. Index into arrays is one less than counter. */ > uint32_t i = count - 1; >+ struct GROUP_POLICY_OBJECT **target_list = NULL; > struct GROUP_POLICY_OBJECT *new_gpo = NULL; >+ bool is_forced = >+ (gp_link->link_opts[i] & GPO_LINK_OPT_ENFORCED) != 0; > > if (gp_link->link_opts[i] & GPO_LINK_OPT_DISABLED) { > DEBUG(10,("skipping disabled GPO\n")); >@@ -580,7 +584,7 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, > > if (only_add_forced_gpos) { > >- if (!(gp_link->link_opts[i] & GPO_LINK_OPT_ENFORCED)) { >+ if (!is_forced) { > DEBUG(10,("skipping nonenforced GPO link " > "because GPOPTIONS_BLOCK_INHERITANCE " > "has been set\n")); >@@ -623,7 +627,8 @@ static ADS_STATUS add_gplink_to_gpo_list(ADS_STRUCT *ads, > new_gpo->link = link_dn; > new_gpo->link_type = link_type; > >- DLIST_ADD(*gpo_list, new_gpo); >+ target_list = is_forced ? forced_gpo_list : gpo_list; >+ DLIST_ADD(*target_list, new_gpo); > > DEBUG(10,("add_gplink_to_gplist: added GPLINK #%d %s " > "to GPO list\n", i, gp_link->link_names[i])); >@@ -722,24 +727,27 @@ static ADS_STATUS add_local_policy_to_gpo_list(TALLOC_CTX *mem_ctx, > } > > /**************************************************************** >- get the full list of GROUP_POLICY_OBJECTs for a given dn >+ Get the full list of GROUP_POLICY_OBJECTs for a given dn. > ****************************************************************/ > >-ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, >+static ADS_STATUS ads_get_gpo_list_internal(ADS_STRUCT *ads, > TALLOC_CTX *mem_ctx, > const char *dn, > uint32_t flags, > const struct security_token *token, >- struct GROUP_POLICY_OBJECT **gpo_list) >+ struct GROUP_POLICY_OBJECT **gpo_list, >+ struct GROUP_POLICY_OBJECT **forced_gpo_list) > { > /* > * Push GPOs to gpo_list so that the traversal order of the list matches > * the order of application: > * (L)ocal (S)ite (D)omain (O)rganizational(U)nit >- * Within domains and OUs: parent-to-child. >+ * For different domains and OUs: parent-to-child. >+ * Within same level of domains and OUs: Link order. > * Since GPOs are pushed to the front of gpo_list, GPOs have to be > * pushed in the opposite order of application (OUs first, local last, > * child-to-parent). >+ * Forced GPOs are appended in the end since they override all others. > */ > > ADS_STATUS status; >@@ -748,6 +756,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > bool add_only_forced_gpos = false; > > ZERO_STRUCTP(gpo_list); >+ ZERO_STRUCTP(forced_gpo_list); > > if (!dn) { > return ADS_ERROR_NT(NT_STATUS_INVALID_PARAMETER); >@@ -784,6 +793,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > status = add_gplink_to_gpo_list(ads, > mem_ctx, > gpo_list, >+ forced_gpo_list, > parent_dn, > &gp_link, > GP_LINK_OU, >@@ -830,6 +840,7 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > status = add_gplink_to_gpo_list(ads, > mem_ctx, > gpo_list, >+ forced_gpo_list, > parent_dn, > &gp_link, > GP_LINK_DOMAIN, >@@ -872,8 +883,12 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > dump_gplink(&gp_link); > } > >- status = add_gplink_to_gpo_list(ads, mem_ctx, gpo_list, >- site_dn, &gp_link, >+ status = add_gplink_to_gpo_list(ads, >+ mem_ctx, >+ gpo_list, >+ forced_gpo_list, >+ site_dn, >+ &gp_link, > GP_LINK_SITE, > add_only_forced_gpos, > token); >@@ -899,4 +914,35 @@ ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, > return ADS_ERROR(LDAP_SUCCESS); > } > >+/**************************************************************** >+ Get the full list of GROUP_POLICY_OBJECTs for a given dn, wrapper >+ around ads_get_gpo_list_internal() that ensures correct ordering. >+****************************************************************/ >+ >+ADS_STATUS ads_get_gpo_list(ADS_STRUCT *ads, >+ TALLOC_CTX *mem_ctx, >+ const char *dn, >+ uint32_t flags, >+ const struct security_token *token, >+ struct GROUP_POLICY_OBJECT **gpo_list) >+{ >+ struct GROUP_POLICY_OBJECT *forced_gpo_list = NULL; >+ ADS_STATUS status = ads_get_gpo_list_internal(ads, >+ mem_ctx, >+ dn, >+ flags, >+ token, >+ gpo_list, >+ &forced_gpo_list); >+ if (!ADS_ERR_OK(status)) { >+ return status; >+ } >+ /* >+ * Append |forced_gpo_list| at the end of |gpo_list|, >+ * so that forced GPOs are applied on top of non enforced GPOs. >+ */ >+ DLIST_CONCATENATE(*gpo_list, forced_gpo_list); >+ return status; >+} >+ > #endif /* HAVE_LDAP */ >-- >2.11.0 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=13625">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Flags:
gd
:
review+
Actions:
View
Attachments on
bug 13046
:
13621
| 13625 |
13647