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.
> >+ 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.
>+
>+ 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..)
>+ 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?
>+ 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;
}
> >+ 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.