On Sun, Jul 20, 2014 at 05:48:45PM -0400, Yassir Elley wrote:
> Hi,
>
> The attached patch adds support for gpo version checking and for
> conditional downloading of policy files. In order to get that to work,
> this patch also replaces the deprecated smb functions (e.g. smbc_open,
> smbc_read, etc) I had been using (not knowing they were deprecated) with
> the newer smb functions, such as "smbc_getFunctionOpen(smbc_ctx)(smbc_ctx,
> smb_uri, O_RDONLY, 0755);".
>
> Before this patch, the policy files were being unconditionally downloaded
> every time. After this patch, the backend sends the cached_gpt_version
> (from a previous transaction, if any) to the gpo_child. The gpo_child
> downloads the per-GPO GPT.INI file, from which it extracts the
> sysvol_gpt_version. Only if the sysvol_gpt_version is greater than the
> cached_gpt_version (or if there is no cached_gpt_version) does the
> gpo_child download the policy files. The gpo_child returns the
> sysvol_gpt_version to the backend, which stores it in the cache.
>
> Note that, while a GPO has both a SYSVOL-stored Group Policy Template (gpt)
> version, as well as an LDAP-stored Group Policy Container (gpc) version
> (which should be equal), it is best practice to use the gpt version for
> version checking b/c SYSVOL replication is typically much faster than AD
> replication. In other words, the gpt version is quickly up-to-date on all
> DCs for a given domain, while the gpc version may be stale for some time
> after a change is made.
>
> In order to keep the gpo_child as simple as possible, the gpo_child does
> not interact with the sysdb cache at all (only the backend does). However,
> the gpo_child does need to parse the GPT.INI file in order to determine
> whether to download the policy files. As such, the gpo_child includes
> GPT.INI file parsing functionality (which is per-GPO, and not
> CSE-specific).
>
> Note that this patch does not add support for offline mode, which will be
> implemented in a subsequent patch.
>
> Regards,
> Yassir.
> From cabea44761b61e75fbd7355ab9ed8a346815e57d Mon Sep 17 00:00:00 2001
> From: Yassir Elley <yelley(a)redhat.com>
> Date: Mon, 7 Jul 2014 08:32:25 -0400
> Subject: [PATCH] AD-GPO: Cache gpo version; only download policy files if
> version changes.
In order to speed up the review process, here are my notes based on
reading the code. I haven't done any testing yet. See my comments
inline:
>
> ---
> Makefile.am | 1 +
> src/db/sysdb.h | 29 +++
> src/db/sysdb_gpo.c | 270 +++++++++++++++++++++++++++
I think it would be better to have two patches -- one with the sysdb API
and one with the AD changes.
> src/providers/ad/ad_gpo.c | 307 +++++++++++++++++++-----------
> src/providers/ad/ad_gpo_child.c | 404
> ++++++++++++++++++++++++++++++++--------
> 5 files changed, 818 insertions(+), 193 deletions(-)
> create mode 100644 src/db/sysdb_gpo.c
>
> diff --git a/Makefile.am b/Makefile.am
> index
> e3592868ce29b569a71f6ad74bc24cc617301b34..d80da00a47fc118b19aee4ff6b40de5fdd0ce792
> 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2297,6 +2297,7 @@ libsss_ad_la_SOURCES = \
> src/providers/ad/ad_gpo.c \
> src/providers/ad/ad_gpo.h \
> src/providers/ad/ad_gpo_ndr.c \
> + src/db/sysdb_gpo.c \
I would prefer if sysdb_gpo was part of libsss_util, similar to how the
other sysdb modules are handled. If you only want to have this code as part
of the AD provider to now grow libsss_util any more, we can alternatively
rename the file to src/providers/ad/ad_gpo_util.c or similar, but I think
a separate module is OK and more reusable in the future.
> src/providers/ad/ad_opts.h \
> src/providers/ad/ad_srv.c \
> src/providers/ad/ad_subdomains.c \
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index
> 17cd5110c9bdafd8d7b18e621188523ed41e5c8a..1bc0ec48e06954892f96f0b513e6225eb65dd483
> 100644
[....]
> diff --git a/src/db/sysdb_gpo.c b/src/db/sysdb_gpo.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..86907733a0908ec85e7a9069d37864a41b1b63cd
> --- /dev/null
> +++ b/src/db/sysdb_gpo.c
> @@ -0,0 +1,270 @@
> +/*
> + SSSD
> +
> + Authors:
> + Yassir Elley <yelley(a)redhat.com>
> +
> + Copyright (C) 2014 Red Hat
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <
http://www.gnu.org/licenses/>.
> +*/
> +
> +
> +#include "db/sysdb.h"
> +#include "db/sysdb_private.h"
> +
> +static struct ldb_dn *
> +sysdb_gpo_dn(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain,
> + const char *gpo_guid)
> +{
> + errno_t ret;
> + char *clean_gpo_guid;
> + struct ldb_dn *dn;
> +
> + ret = sysdb_dn_sanitize(NULL, gpo_guid, &clean_gpo_guid);
> + if (ret != EOK) {
> + return NULL;
> + }
> +
> + DEBUG(SSSDBG_TRACE_FUNC, SYSDB_TMPL_GPO"\n", clean_gpo_guid,
> domain->name);
> +
> + dn = ldb_dn_new_fmt(mem_ctx, domain->sysdb->ldb, SYSDB_TMPL_GPO,
> + clean_gpo_guid, domain->name);
> + talloc_free(clean_gpo_guid);
> +
> + return dn;
> +}
> +
> +errno_t
> +sysdb_gpo_store_gpo(struct sss_domain_info *domain,
> + const char *dom_name,
Why do you have a separate dom_name parameter alongside domain? Can you
use domain->name in the function instead?
> + const char *gpo_guid,
> + int gpo_version)
> +{
> + errno_t ret, sret;
> + int lret;
> + struct ldb_message *update_msg;
> + struct ldb_message **msgs;
> + struct ldb_dn *dn;
> + static const char *attrs[] = SYSDB_GPO_ATTRS;
> + size_t count;
> + bool in_transaction = false;
> + TALLOC_CTX *tmp_ctx;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (!tmp_ctx) return ENOMEM;
Please put a blank line here for better readability.
> + dn = sysdb_gpo_dn(tmp_ctx, domain, gpo_guid);
> + if (!dn) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + update_msg = ldb_msg_new(tmp_ctx);
> + if (!update_msg) {
> + ret = ENOMEM;
> + goto done;
> + }
> + update_msg->dn = dn;
I think the memory hierarchy would be cleaner if you did:
update_msg = ldb_msg_new(tmp_ctx);
update_msg->dn = sysdb_gpo_dn(update_msg, domain, gpo_guid);
This was update_msg->dn is not only part of update_msg structure
logically, but also as far as talloc hierarchy goes. It's not really
important in this function, but it's a good practice.
> +
> + ret = sysdb_transaction_start(domain->sysdb);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Failed to start transaction\n");
> + goto done;
> + }
> +
> + in_transaction = true;
> +
> + dn = update_msg->dn;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (!tmp_ctx) return ENOMEM;
You overwrite both dn and tmp_ctx above.
> +
> + /* Check for an existing gpo_guid entry */
> + ret = sysdb_search_entry(tmp_ctx, domain->sysdb, dn, LDB_SCOPE_BASE,
> + NULL, attrs, &count, &msgs);
> + if (ret != EOK && ret != ENOENT) goto done;
I realize that here the error code can be either EOK or ENOENT and
nothing else, but to me it's more readable to write:
if (ret == ENOENT) {
/* new gpo */
} else if (ret == EOK) {
/* update existing gpo */
} else {
/* error! */
}
But this is more of a suggestion, feel free to keep the code the way it
is if you disagree.
> +
> + if (ret == EOK && count != 1) {
> + /* More than one reply for a base search? */
> + ret = EIO;
> + goto done;
> + } else if (ret == ENOENT) {
> + /* Create new GPO */
> + DEBUG(SSSDBG_TRACE_FUNC,
> + "Adding new GPO [gpo_guid:%s][gpo_version:%d]\n",
> + gpo_guid, gpo_version);
> +
> + /* Add the objectClass */
> + lret = ldb_msg_add_empty(update_msg, SYSDB_OBJECTCLASS,
> + LDB_FLAG_MOD_ADD,
> + NULL);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + lret = ldb_msg_add_string(update_msg, SYSDB_OBJECTCLASS,
> + SYSDB_GPO_OC);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + /* Add the GPO GUID */
> + lret = ldb_msg_add_empty(update_msg, SYSDB_GPO_GUID_ATTR,
> + LDB_FLAG_MOD_ADD,
> + NULL);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + lret = ldb_msg_add_string(update_msg, SYSDB_GPO_GUID_ATTR,
> gpo_guid);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + /* Add the Version */
> + lret = ldb_msg_add_empty(update_msg, SYSDB_GPO_VERSION_ATTR,
> + LDB_FLAG_MOD_ADD,
> + NULL);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + lret = ldb_msg_add_fmt(update_msg, SYSDB_GPO_VERSION_ATTR,
> + "%d", gpo_version);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + lret = ldb_add(domain->sysdb->ldb, update_msg);
> + if (lret != LDB_SUCCESS) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "Failed to add GPO: [%s]\n",
> + ldb_strerror(lret));
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> + } else {
> + /* Update the existing GPO */
> +
> + DEBUG(SSSDBG_TRACE_FUNC,
> + "Updating new GPO [%s][%s]\n", dom_name, gpo_guid);
> +
> + /* Add the Version */
> + lret = ldb_msg_add_empty(update_msg, SYSDB_GPO_VERSION_ATTR,
> + LDB_FLAG_MOD_ADD,
Are you sure you want to add another version string and make it multivalued
as opposed to replacing a version string? Please note this is a genuine
question, but if you do intend to make the attribute multivalued maybe
it's worth adding acomment.
> + NULL);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + lret = ldb_msg_add_fmt(update_msg, SYSDB_GPO_VERSION_ATTR,
> + "%d", gpo_version);
> + if (lret != LDB_SUCCESS) {
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + lret = sss_ldb_modify_permissive(domain->sysdb->ldb, update_msg);
> + if (lret != LDB_SUCCESS) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "Failed to modify GPO: [%s]\n", ldb_strerror(lret));
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> + }
> +
> + ret = sysdb_transaction_commit(domain->sysdb);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "Could not commit transaction: [%s]\n", strerror(ret));
> + goto done;
> + }
> + in_transaction = false;
> +
> +done:
> + if (in_transaction) {
> + sret = sysdb_transaction_cancel(domain->sysdb);
> + if (sret != EOK) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Could not cancel
transaction\n");
> + }
> + }
> + talloc_free(tmp_ctx);
> + return ret;
> +}
> +
> +errno_t
> +sysdb_gpo_get_gpo(TALLOC_CTX *mem_ctx,
> + struct sss_domain_info *domain,
> + const char *gpo_guid,
> + struct ldb_result **_result)
> +{
> + errno_t ret;
> + int lret;
> + struct ldb_dn *base_dn;
> + TALLOC_CTX *tmp_ctx;
> + struct ldb_result *res;
> +
> + const char *attrs[] = SYSDB_GPO_ATTRS;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (!tmp_ctx) return ENOMEM;
> +
> + DEBUG(SSSDBG_TRACE_FUNC, SYSDB_TMPL_GPO_BASE"\n", domain->name);
> +
> + base_dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
> + SYSDB_TMPL_GPO_BASE,
> + domain->name);
> + if (!base_dn) {
> + ret = ENOMEM;
> + goto done;
> + }
Nitpick - while you're changing the module anyway, please put a blank
line here, too.
> + lret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res, base_dn,
> + LDB_SCOPE_SUBTREE, attrs, SYSDB_GPO_FILTER,
> gpo_guid);
> + if (lret) {
> + DEBUG(SSSDBG_MINOR_FAILURE,
> + "Could not locate GPO: [%s]\n",
> + ldb_strerror(lret));
> + ret = sysdb_error_to_errno(lret);
> + goto done;
> + }
> +
> + if (res->count > 1) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Search for GUID [%s] returned more
> than " \
> + "one object.\n", gpo_guid);
> + ret = EINVAL;
> + goto done;
> + } else if (res->count == 0) {
> + ret = ENOENT;
> + goto done;
> + }
> + *_result = talloc_steal(mem_ctx, res);
> + ret = EOK;
> +done:
> +
> + if (ret == ENOENT) {
> + DEBUG(SSSDBG_TRACE_ALL, "No such entry.\n");
> + } else if (ret) {
> + DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret,
strerror(ret));
> + }
> +
> + talloc_free(tmp_ctx);
> + return ret;
> +}
> diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> index
> e33ea72e9fdfb053655a5ab7c63de735da0f6954..470a6fcfcc45c5c66186c6406eb2966a0a3fd8bd
> 100644
> --- a/src/providers/ad/ad_gpo.c
> +++ b/src/providers/ad/ad_gpo.c
> @@ -86,7 +86,7 @@
> #define DENY_LOGON_LOCALLY "SeDenyInteractiveLogonRight"
>
> #define GP_EXT_GUID_SECURITY "{827D319E-6EAC-11D2-A4EA-00C04F79F83A}"
> -#define GP_EXT_GUID_SECURITY_SUFFIX "/Microsoft/Windows
> NT/SecEdit/GptTmpl.inf"
> +#define GP_EXT_GUID_SECURITY_SUFFIX "/Machine/Microsoft/Windows
> NT/SecEdit/GptTmpl.inf"
>
> #ifndef SSSD_LIBEXEC_PATH
> #error "SSSD_LIBEXEC_PATH not defined"
> @@ -115,9 +115,10 @@ struct gp_gpo {
> const char *gpo_dn;
> const char *gpo_guid;
> const char *gpo_display_name;
> - const char *gpo_file_sys_path;
> - const char *gpo_unix_path;
> - uint32_t gpo_container_version;
> + const char *smb_server;
> + const char *smb_share;
> + const char *smb_path;
> + uint32_t gpc_version;
> const char **gpo_cse_guids;
> int num_gpo_cse_guids;
> int gpo_func_version;
> @@ -156,10 +157,15 @@ int ad_gpo_process_gpo_recv(struct tevent_req *req,
> int *num_candidate_gpos);
> struct tevent_req *ad_gpo_process_cse_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> - char *cse_smb_uri,
> - char *cse_unix_path);
> + struct sss_domain_info *domain,
> + const char *smb_server,
> + const char *smb_share,
> + const char *smb_path,
> + const char *smb_cse_suffix,
> + const char *gpo_guid);
> int ad_gpo_process_cse_recv(struct tevent_req *req,
> TALLOC_CTX *mem_ctx,
> + int *_sysvol_gpt_version,
> int *_allowed_size,
> char ***_allowed_sids,
> int *_denied_size,
> @@ -1298,8 +1304,6 @@ ad_gpo_cse_step(struct tevent_req *req)
> {
> struct tevent_req *subreq;
> struct ad_gpo_access_state *state;
> - char *cse_smb_uri;
> - char *cse_unix_path;
> int i = 0;
>
> state = tevent_req_data(req, struct ad_gpo_access_state);
> @@ -1319,15 +1323,19 @@ ad_gpo_cse_step(struct tevent_req *req)
> state->cse_gpo_index, i,
> cse_filtered_gpo->gpo_cse_guids[i]);
> }
>
> - cse_smb_uri = talloc_asprintf(state, "%s%s",
> - cse_filtered_gpo->gpo_file_sys_path,
> - GP_EXT_GUID_SECURITY_SUFFIX);
> + DEBUG(SSSDBG_TRACE_FUNC, "smb_server: %s\n",
> cse_filtered_gpo->smb_server);
> + DEBUG(SSSDBG_TRACE_FUNC, "smb_share: %s\n",
> cse_filtered_gpo->smb_share);
> + DEBUG(SSSDBG_TRACE_FUNC, "smb_path: %s\n",
> cse_filtered_gpo->smb_path);
> + DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n",
> cse_filtered_gpo->gpo_guid);
>
> - cse_unix_path = talloc_asprintf(state, "%s%s",
> - cse_filtered_gpo->gpo_unix_path,
> - GP_EXT_GUID_SECURITY_SUFFIX);
> -
> - subreq = ad_gpo_process_cse_send(state, state->ev, cse_smb_uri,
> cse_unix_path);
> + subreq = ad_gpo_process_cse_send(state,
> + state->ev,
> + state->domain,
> + cse_filtered_gpo->smb_server,
> + cse_filtered_gpo->smb_share,
> + cse_filtered_gpo->smb_path,
> + GP_EXT_GUID_SECURITY_SUFFIX,
> + cse_filtered_gpo->gpo_guid);
>
> tevent_req_set_callback(subreq, ad_gpo_cse_done, req);
> return EAGAIN;
> @@ -1348,6 +1356,7 @@ ad_gpo_cse_done(struct tevent_req *subreq)
> struct tevent_req *req;
> struct ad_gpo_access_state *state;
> int ret;
> + int sysvol_gpt_version;
> char **allowed_sids;
> int allowed_size;
> char **denied_sids;
> @@ -1356,11 +1365,24 @@ ad_gpo_cse_done(struct tevent_req *subreq)
> req = tevent_req_callback_data(subreq, struct tevent_req);
> state = tevent_req_data(req, struct ad_gpo_access_state);
>
> - ret = ad_gpo_process_cse_recv(subreq, state, &allowed_size,
> + struct gp_gpo *cse_filtered_gpo =
> + state->cse_filtered_gpos[state->cse_gpo_index];
> +
> + const char *gpo_guid = cse_filtered_gpo->gpo_guid;
> +
> + DEBUG(SSSDBG_TRACE_FUNC, "gpo_guid: %s\n", gpo_guid);
> +
> + ret = ad_gpo_process_cse_recv(subreq, state, &sysvol_gpt_version,
> &allowed_size,
> &allowed_sids, &denied_size,
> &denied_sids);
>
> talloc_zfree(subreq);
>
> +
> + DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n",
> sysvol_gpt_version);
> +
> + ret = sysdb_gpo_store_gpo(state->domain, state->domain->name,
> gpo_guid,
> + sysvol_gpt_version);
> +
> if (ret != EOK) {
> /* TBD: handle ret error */
> goto done;
> @@ -2274,48 +2296,46 @@ ad_gpo_populate_candidate_gpos(TALLOC_CTX *mem_ctx,
> }
>
> /*
> - * This function converts the input_path to an smb uri and a unix_path,
> which
> - * are used to populate the _smb_uri and _unix_path output parameters,
> - * respectively.
> + * This function parses the input_path into its components, replaces each
> + * forward slash ('\') with a back slash ('/'), and populates the
output
> params.
I think the forward slash ans backslash are the other way around :-)
I'll continue with the review tomorrow, sorry, but I'm running out of time
today, I just wanted to send out what I was able to review so far in case
you wanted to do some fixes in tree. Feel free to send updated patches
if you agree with some of the points I raised (or not if you prefer to
send the update in a single large batch).
Thank you for the patch!
To be continued :-)
Hi Jakub,
I agree with all of your comments so far and have attached revised patches to this email.
I split the original patch into two patches, as you suggested, and I addressed your review
comments in the revised patches. Thanks for the review! Keep the comments coming! :)
Regards,
Yassir.