[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