[SSSD] [PATCH] AD: Cache gpo version; only download policy files if version changes

Jakub Hrozek jhrozek at redhat.com
Mon Jul 21 18:29:24 UTC 2014


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 at 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 at 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 :-)



More information about the sssd-devel mailing list