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

Yassir Elley yelley at redhat.com
Wed Jul 23 07:29:04 UTC 2014



----- Original Message -----
> On Tue, Jul 22, 2014 at 07:32:17PM +0200, Lukas Slebodnik wrote:
> > On (22/07/14 03:13), Yassir Elley wrote:
> > >> 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.
> > >> 
> > >
> > >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.
> > 
> > >From f3f00a78102ad5e389266b28c43d0396caf57592 Mon Sep 17 00:00:00 2001
> > >From: Yassir Elley <yelley at redhat.com>
> > >Date: Tue, 22 Jul 2014 03:03:03 -0400
> > >Subject: [PATCH 2/2] AD-GPO: only download policy files if gpo version
> > >changes
> > >
> > >---
> > > src/providers/ad/ad_gpo.c       | 306 +++++++++++++++++++-----------
> > > src/providers/ad/ad_gpo_child.c | 404
> > > ++++++++++++++++++++++++++++++++--------
> > > 2 files changed, 517 insertions(+), 193 deletions(-)
> > >
> > 
> > I  have few comments to the ad_gpo_child
> 
> Thank you for the second pair of eyes. I agree with your suggestions,
> see some more comments inline.
> 
> > 
> 
> [...]
> 
> > >+copy_smb_file_to_gpo_cache(SMBCCTX *smbc_ctx,
> > >+                           const char *smb_server,
> > >+                           const char *smb_share,
> > >+                           const char *smb_path,
> > >+                           const char *smb_cse_suffix)
> > > {
> > >-    SMBCCTX *context;
> > >+    char *smb_uri = NULL;
> > >+    SMBCFILE *file;
> > >     int ret = 0;
> > >     uint8_t *buf = NULL;
> > >     int buflen = 0;
> > >+
> > >     TALLOC_CTX *tmp_ctx = NULL;
> > > 
> > >     tmp_ctx = talloc_new(NULL);
> > >@@ -325,59 +489,131 @@ process_security_settings_cse(TALLOC_CTX *mem_ctx,
> > >         return ENOMEM;
> > >     }
> > > 
> > >-    DEBUG(SSSDBG_TRACE_ALL, "%s\n", smb_uri);
> > >-    DEBUG(SSSDBG_TRACE_ALL, "%s\n", unix_path);
> > >+    smb_uri = talloc_asprintf(tmp_ctx, "%s%s%s%s", smb_server,
> > >+                              smb_share, smb_path, smb_cse_suffix);
> 
> Unchecked allocation.

OK

> 
> > >+    DEBUG(SSSDBG_TRACE_FUNC, "smb_uri: %s\n", smb_uri);
> > > 
> > >-    context = smbc_new_context();
> > >-    if (context == NULL) {
> > >+    errno = 0;
> > >+    file = smbc_getFunctionOpen(smbc_ctx)(smbc_ctx, smb_uri, O_RDONLY,
> > >0755);
> > >+    if (file == NULL) {
> > >+        ret = errno;
> > >+        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_getFunctionOpen failed
> > >[%d][%s]\n",
> > >+              ret, strerror(ret));
> > >+        goto done;
> > >+    }
> > >+
> > >+    buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
> 
> Another unchecked allocation.

OK

> 
> > >+
> > >+    errno = 0;
> > >+    buflen = smbc_getFunctionRead(smbc_ctx)(smbc_ctx, file, buf,
> > >SMB_BUFFER_SIZE);
> > >+    if(buflen < 0) {
> 
> While you're changing the code, can you also put a space after if here?
> (I know, total nitpick..)

OK

> 
> > >+        ret = errno;
> > >+        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_getFunctionRead failed
> > >[%d][%s]\n",
> > >+              ret, strerror(ret));
> > >+        goto done;
> > >+    }
> > >+
> > >+    DEBUG(SSSDBG_TRACE_ALL, "smb_buflen: %d\n", buflen);
> > >+
> > >+    ret = gpo_cache_store_file(smb_path, smb_cse_suffix, buf, buflen);
> 
> General question here -- what happens if two users whose GPO policies
> reference the same files log in simultaneously? Do we clobber the files?

With the current patch, the answer is "yes" (the files are over-written). However, in the next patch, I am including a gpo_cache_expire_timeout option, in which case the answer will be "no". Specifically, if the policy files related to a gpo-guid have been cached at a certain time, and if the same gpo-guid is needed again within the expire_timeout time, whether by the same user or by a different user, then the gpo code will re-use the existing files in the GPO_CACHE dir (and will not interact with the SMB server at all). Does that make sense?

> 
> > >+    if (ret != EOK) {
> > >+        DEBUG(SSSDBG_CRIT_FAILURE,
> > >+              "gpo_cache_store_file failed [%d][%s]\n",
> > >+              ret, strerror(ret));
> > >+        goto done;
> > >+    }
> > >+
> > >+ done:
> > >+    talloc_free(tmp_ctx);
> > >+    return ret;
> > >+}
> > >+
> > >+
> > >+/*
> > >+ * Using its smb_uri components and cached_gpt_version inputs, this
> > >function
> > >+ * does several things:
> > >+ * - it downloads the GPT_INI file to GPO_CACHE
> > >+ * - it parses the sysvol_gpt_version field from the GPT_INI file
> > >+ * - if the sysvol_gpt_version is greater than the cached_gpt_version
> > >+ *   - it downloads the policy file to GPO_CACHE
> > >+ * - else
> > >+ *   - it doesn't retrieve the policy file
> > >+ *   - in this case, the backend will use the existing policy file in
> > >GPO_CACHE
> > >+ * - it returns the sysvol_gpt_version in the _sysvol_gpt_version output
> > >param
> > >+ *
> > >+ * Note that if the cached_gpt_version sent by the backend is -1 (to
> > >indicate
> > >+ * that no gpt_version has been set in the cache for the corresponding
> > >gpo_guid),
> > >+ * then the parsed sysvol_gpt_version (which must be at least 0) will be
> > >greater
> > >+ * than the cached_gpt_version, thereby triggering a fresh download.
> > >+ *
> > >+ * Note that the backend will later do the following:
> > >+ * - backend will save the the sysvol_gpt_version to sysdb cache
> > >+ * - backend will read the policy file from the GPO_CACHE
> > >+ */
> > >+static errno_t
> > >+perform_smb_operations(int cached_gpt_version,
> > >+                       const char *smb_server,
> > >+                       const char *smb_share,
> > >+                       const char *smb_path,
> > >+                       const char *smb_cse_suffix,
> > >+                       int *_sysvol_gpt_version)
> > >+{
> > >+    SMBCCTX *smbc_ctx;
> > >+    int ret = 0;
> >       I don't think we need to initialise ret to zero. From defensive point
> >       of
> >       view it would be better to init with EINVAL. I think zero can be
> >       removed.
> 
> I think it's better to set ret to zero (or EOK) prior to entering the
> done label from the main control flow, like this:
> 
>     static int fn(ctx)
>     {
>         ret = do_stuff(ctx);
>         if (ret != EOK) {
>             goto done;
>         }
> 
>         ret = EOK;
>     done:
>         return ret;
>     }

Why? In your example, if do_stuff() returns EOK, won't ret already be set to EOK (by the time it reaches the "ret = EOK;" statement)?

> 
> > >+    int sysvol_gpt_version;
> > >+
> > >+    smbc_ctx = smbc_new_context();
> > >+    if (smbc_ctx == NULL) {
> > >         DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate new smbc
> > >         context\n");
> > >         ret = ENOMEM;
> > >         goto done;
> > >     }
> > > 
> > >-    smbc_setFunctionAuthData(context, sssd_krb_get_auth_data_fn);
> > >-    smbc_setOptionUseKerberos(context, 1);
> > >+    smbc_setFunctionAuthData(smbc_ctx, sssd_krb_get_auth_data_fn);
> > >+    smbc_setOptionUseKerberos(smbc_ctx, 1);
> > > 
> > >     /* Initialize the context using the previously specified options */
> > >-    if (smbc_init_context(context) == NULL) {
> > >+    if (smbc_init_context(smbc_ctx) == NULL) {
> > >         DEBUG(SSSDBG_CRIT_FAILURE, "Could not initialize smbc
> > >         context\n");
> > >         ret = ENOMEM;
> > >         goto done;
> > >     }
> > > 
> > >-    /* Tell the compatibility layer to use this context */
> > >-    smbc_set_context(context);
> > >-
> > >-    int remotehandle = smbc_open(smb_uri, O_RDONLY, 0755);
> > >-    if (remotehandle < 0) {
> > >-        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_open failed\n");
> > >-        ret = EPIPE;
> > >-        goto done;
> > >-    }
> > >-
> > >-    buf = talloc_array(tmp_ctx, uint8_t, SMB_BUFFER_SIZE);
> > >-    buflen = smbc_read(remotehandle, buf, SMB_BUFFER_SIZE);
> > >-    if(buflen < 0) {
> > >-        DEBUG(SSSDBG_CRIT_FAILURE, "smbc_read failed\n");
> > >-        ret = EPIPE;
> > >-        goto done;
> > >-    }
> > >-
> > >-    DEBUG(SSSDBG_CRIT_FAILURE, "buflen: %d\n", buflen);
> > >-
> > >-    smbc_close(remotehandle);
> > >-
> > >-    ret = store_bytes_in_gpo_cache(unix_path, buf, buflen);
> > >+    /* download ini file */
> > >+    ret = copy_smb_file_to_gpo_cache(smbc_ctx, smb_server, smb_share,
> > >smb_path,
> > >+                                     GPT_INI);
> > >     if (ret != EOK) {
> > >         DEBUG(SSSDBG_CRIT_FAILURE,
> > >-              "store_bytes_in_gpo_cache failed [%d][%s]\n",
> > >+              "copy_smb_file_to_gpo_cache failed [%d][%s]\n",
> > >               ret, strerror(ret));
> > >         goto done;
> > >     }
> > > 
> > >+    ret = ad_gpo_parse_ini_file(smb_path, &sysvol_gpt_version);
> > >+    if (ret != EOK) {
> > >+        DEBUG(SSSDBG_CRIT_FAILURE,
> > >+              "Cannot parse ini file: [%d][%s]\n", ret, strerror(ret));
> > >+        goto done;
> > >+    }
> > >+
> > >+    DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n",
> > >sysvol_gpt_version);
> > >+
> > >+    if (sysvol_gpt_version > cached_gpt_version) {
> > >+        /* download policy file */
> > >+        ret = copy_smb_file_to_gpo_cache(smbc_ctx, smb_server, smb_share,
> > >+                                         smb_path, smb_cse_suffix);
> > >+        if (ret != EOK) {
> > >+            DEBUG(SSSDBG_CRIT_FAILURE,
> > >+                  "copy_smb_file_to_gpo_cache failed [%d][%s]\n",
> > >+                  ret, strerror(ret));
> > >+            goto done;
> > >+        }
> > >+    }
> > >+
> > >+    *_sysvol_gpt_version = sysvol_gpt_version;
> > >+
> > >  done:
> > >-    smbc_free_context(context, 0);
> > >-    talloc_free(tmp_ctx);
> > >+    smbc_free_context(smbc_ctx, 0);
> > >     return ret;
> > > }
> > > 
> > >@@ -388,6 +624,7 @@ main(int argc, const char *argv[])
> > >     poptContext pc;
> > >     int debug_fd = -1;
> > >     errno_t ret;
> > >+    int sysvol_gpt_version;
> > >     int result;
> > >     TALLOC_CTX *main_ctx = NULL;
> > >     uint8_t *buf = NULL;
> > >@@ -484,18 +721,21 @@ main(int argc, const char *argv[])
> > >         goto fail;
> > >     }
> > > 
> > >-    DEBUG(SSSDBG_TRACE_FUNC, "processing security settings\n");
> > >+    DEBUG(SSSDBG_TRACE_FUNC, "performing smb operations\n");
> > > 
> > >-    result = process_security_settings_cse(main_ctx,
> > >-                                           ibuf->smb_uri,
> > >-                                           ibuf->unix_path);
> > >+    result = perform_smb_operations(ibuf->cached_gpt_version,
> > >+                                    ibuf->smb_server,
> > >+                                    ibuf->smb_share,
> > >+                                    ibuf->smb_path,
> > >+                                    ibuf->smb_cse_suffix,
> > >+                                    &sysvol_gpt_version);
> > >     if (result != EOK) {
> > >         DEBUG(SSSDBG_CRIT_FAILURE,
> > >-              "process_security_settings_cse failed.[%d][%s].\n",
> > >+              "perform_smb_operations failed.[%d][%s].\n",
> > >               result, strerror(result));
> >       missing goto fail.
> > This is a reason of 1st warning.
> > src/providers/ad/ad_gpo_child.c:142:34: error: ‘sysvol_gpt_version’ may be
> > used uninitialized in this function [-Werror=maybe-uninitialized]
> >      SAFEALIGN_SET_UINT32(&r->buf[p], sysvol_gpt_version, &p);
> >                                   ^
> > src/providers/ad/ad_gpo_child.c:627:9: note: ‘sysvol_gpt_version’ was
> > declared here
> >      int sysvol_gpt_version;
> > 
> > >     }
> > 
> > 
> > There is also another gcc warning. It is only visible with -O1 or higher.
> > The same warning is with ferao 20/rawhide and rhel6
> 
> Nice catch, I only tested with clang and -O0.
> 
> > 
> >   src/providers/ad/ad_gpo_child.c: In function ‘main’:
> >   src/providers/ad/ad_gpo_child.c:142:34: error: ‘gpt_version’ may be used
> >   uninitialized in this function [-Werror=maybe-uninitialized]
> >        SAFEALIGN_SET_UINT32(&r->buf[p], sysvol_gpt_version, &p);
> >                                         ^
> >   src/providers/ad/ad_gpo_child.c:413:9: note: ‘gpt_version’ was declared
> >   here
> >        int gpt_version;
> >            ^
> >   cc1: all warnings being treated as errors
> > 
> > There are two possibel solutions:
> > a) explicit initialisation to unexpected value.
> >   --- a/src/providers/ad/ad_gpo_child.c
> >   +++ b/src/providers/ad/ad_gpo_child.c
> >   @@ -410,7 +410,7 @@ ad_gpo_parse_ini_file(const char *smb_path,
> >        struct ini_cfgobj *ini_config = NULL;
> >        const char *ini_filename;
> >        int ret;
> >   -    int gpt_version;
> >   +    int gpt_version = -1;
> >        TALLOC_CTX *tmp_ctx = NULL;
> > 
> > b) disable inlining of parse_ini_file_with_libini.
> >   --- a/src/providers/ad/ad_gpo_child.c
> >   +++ b/src/providers/ad/ad_gpo_child.c
> >   @@ -359,7 +359,7 @@ static errno_t gpo_cache_store_file(const char
> >   *smb_path,
> >        return ret;
> >    }
> > 
> >   -static errno_t
> >   +errno_t
> >    parse_ini_file_with_libini(struct ini_cfgobj *ini_config,
> >                               int *_gpt_version)
> >    {
> > 
> > LS
> 
> I personally prefer a) here.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 

Revised patch attached, which addresses the last three code review emails for this patch (Jakub-1, Lukas, Jakub-2).

Thanks,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-GPO-only-download-policy-files-if-gpo-version-cha.patch
Type: text/x-patch
Size: 45680 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140723/f9f6c64e/attachment.bin>


More information about the sssd-devel mailing list