----- 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(a)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(a)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.