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

Jakub Hrozek jhrozek at redhat.com
Tue Jul 22 17:13:02 UTC 2014


On Tue, Jul 22, 2014 at 03:13:58AM -0400, Yassir Elley wrote:

See some comments about ad_gpo.c inline, I'm still reading
ad_gpo_child.c

> 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

[...]

> @@ -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);

It's not really visible from the diff, but ad_gpo_cse_step increments
the index as the first thing it does and only then assigns to
cse_filtered_gpo. This reads to me as if you were skipping the first
entry...shouldn't the code read the other way around, first assign the
GPO and then increment the index?

> @@ -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,23 @@ 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);

Here you read the return code of ad_gpo_process_cse_recv() but don't
check it.

>  
>      talloc_zfree(subreq);
>  
> +
> +    DEBUG(SSSDBG_TRACE_FUNC, "sysvol_gpt_version: %d\n", sysvol_gpt_version);
> +
> +    ret = sysdb_gpo_store_gpo(state->domain, gpo_guid, sysvol_gpt_version);
> +
>      if (ret != EOK) {
>          /* TBD: handle ret error  */
>          goto done;
> @@ -2274,48 +2295,46 @@ ad_gpo_populate_candidate_gpos(TALLOC_CTX *mem_ctx,

[...]

> @@ -2709,32 +2734,23 @@ ad_gpo_get_gpo_attrs_done(struct tevent_req *subreq)
>      }
>  
>      file_sys_path = talloc_strdup(gp_gpo, raw_file_sys_path);
> -    ad_gpo_convert_to_smb_uri(state, state->server_hostname, file_sys_path,
> -                              &smb_uri, &unix_path);
>  
> -    gp_gpo->gpo_file_sys_path = talloc_asprintf(gp_gpo, "%s/Machine",
> -                                                smb_uri);
> -    if (gp_gpo->gpo_file_sys_path == NULL) {
> -        ret = ENOMEM;
> -        goto done;
> -    }
> +    ad_gpo_extract_smb_components(state, state->server_hostname,
> +                                  file_sys_path, &smb_server,
> +                                  &smb_share, &smb_path);

There is no return code check here.

>  
> -    DEBUG(SSSDBG_TRACE_FUNC, "gpo_file_sys_path: %s\n",
> -          gp_gpo->gpo_file_sys_path);
> +    gp_gpo->smb_server = talloc_strdup(gp_gpo, smb_server);
> +    DEBUG(SSSDBG_TRACE_ALL, "smb_server: %s\n", gp_gpo->smb_server);
>  
> -    gp_gpo->gpo_unix_path = talloc_asprintf(gp_gpo, "%s/Machine",
> -                                            unix_path);
> -    if (gp_gpo->gpo_unix_path == NULL) {
> -        ret = ENOMEM;
> -        goto done;
> -    }
> +    gp_gpo->smb_share = talloc_strdup(gp_gpo, smb_share);
> +    DEBUG(SSSDBG_TRACE_ALL, "smb_share: %s\n", gp_gpo->smb_share);
>  
> -    DEBUG(SSSDBG_TRACE_FUNC, "gpo_unix_path: %s\n",
> -          gp_gpo->gpo_unix_path);
> +    gp_gpo->smb_path = talloc_strdup(gp_gpo, smb_path);
> +    DEBUG(SSSDBG_TRACE_ALL, "smb_path: %s\n", gp_gpo->smb_path);

I wonder if you could call either talloc_steal to move the variables
like smb_path, smb_share and smb_server to the gp_gpo context or assign
to members of gp_gpo right away when calling
ad_gpo_extract_smb_components() instead of allocating the result on
state and assigning to local variables. Not a big deal though.

>  
>      /* retrieve AD_AT_VERSION_NUMBER */
>      ret = sysdb_attrs_get_uint32_t(results[0], AD_AT_VERSION_NUMBER,
> -                                   &gp_gpo->gpo_container_version);
> +                                   &gp_gpo->gpc_version);
>      if (ret != EOK) {
>          DEBUG(SSSDBG_OP_FAILURE,
>                "sysdb_attrs_get_uint32_t failed: [%d](%s)\n",
> @@ -2742,8 +2758,7 @@ ad_gpo_get_gpo_attrs_done(struct tevent_req *subreq)
>          goto done;
>      }
>  
> -    DEBUG(SSSDBG_TRACE_ALL, "gpo_container_version: %d\n",
> -                            gp_gpo->gpo_container_version);
> +    DEBUG(SSSDBG_TRACE_ALL, "gpc_version: %d\n", gp_gpo->gpc_version);
>  
>      /* retrieve AD_AT_MACHINE_EXT_NAMES */
>      ret = sysdb_attrs_get_el(results[0], AD_AT_MACHINE_EXT_NAMES, &el);
> @@ -2843,20 +2858,26 @@ ad_gpo_process_gpo_recv(struct tevent_req *req,
>  }
>  
>  /* == ad_gpo_process_cse_send/recv helpers ================================= */
> -
>  static errno_t
>  create_cse_send_buffer(TALLOC_CTX *mem_ctx,
> -                       char *cse_smb_uri,
> -                       char *cse_unix_path,
> +                       const char *smb_server,
> +                       const char *smb_share,
> +                       const char *smb_path,
> +                       const char *smb_cse_suffix,
> +                       int cached_gpt_version,
>                         struct io_buffer **io_buf)
>  {
>      struct io_buffer *buf;
>      size_t rp;
> -    int cse_smb_uri_length;
> -    int cse_unix_path_length;
> +    int smb_server_length;
> +    int smb_share_length;
> +    int smb_path_length;
> +    int smb_cse_suffix_length;
>  
> -    cse_smb_uri_length = strlen(cse_smb_uri);
> -    cse_unix_path_length = strlen(cse_unix_path);
> +    smb_server_length = strlen(smb_server);
> +    smb_share_length = strlen(smb_share);
> +    smb_path_length = strlen(smb_path);
> +    smb_cse_suffix_length = strlen(smb_cse_suffix);
>  
>      buf = talloc(mem_ctx, struct io_buffer);
>      if (buf == NULL) {
> @@ -2864,8 +2885,9 @@ create_cse_send_buffer(TALLOC_CTX *mem_ctx,
>          return ENOMEM;
>      }
>  
> -    buf->size = 2 * sizeof(uint32_t);
> -    buf->size += cse_smb_uri_length + cse_unix_path_length;
> +    buf->size = 5 * sizeof(uint32_t);
> +    buf->size += smb_server_length + smb_share_length + smb_path_length +
> +        smb_cse_suffix_length;
>  
>      DEBUG(SSSDBG_TRACE_ALL, "buffer size: %zu\n", buf->size);
>  
> @@ -2877,13 +2899,24 @@ create_cse_send_buffer(TALLOC_CTX *mem_ctx,
>      }
>  
>      rp = 0;
> -    /* cse_smb_uri */
> -    SAFEALIGN_SET_UINT32(&buf->data[rp], cse_smb_uri_length, &rp);
> -    safealign_memcpy(&buf->data[rp], cse_smb_uri, cse_smb_uri_length, &rp);
> +    /* cached_gpt_version */
> +    SAFEALIGN_SET_UINT32(&buf->data[rp], cached_gpt_version, &rp);
>  
> -    /* cse_unix_path */
> -    SAFEALIGN_SET_UINT32(&buf->data[rp], cse_unix_path_length, &rp);
> -    safealign_memcpy(&buf->data[rp], cse_unix_path, cse_unix_path_length, &rp);
> +    /* smb_server */
> +    SAFEALIGN_SET_UINT32(&buf->data[rp], smb_server_length, &rp);
> +    safealign_memcpy(&buf->data[rp], smb_server, smb_server_length, &rp);
> +
> +    /* smb_share */
> +    SAFEALIGN_SET_UINT32(&buf->data[rp], smb_share_length, &rp);
> +    safealign_memcpy(&buf->data[rp], smb_share, smb_share_length, &rp);
> +
> +    /* smb_path */
> +    SAFEALIGN_SET_UINT32(&buf->data[rp], smb_path_length, &rp);
> +    safealign_memcpy(&buf->data[rp], smb_path, smb_path_length, &rp);
> +
> +    /* smb_cse_suffix */
> +    SAFEALIGN_SET_UINT32(&buf->data[rp], smb_cse_suffix_length, &rp);
> +    safealign_memcpy(&buf->data[rp], smb_cse_suffix, smb_cse_suffix_length, &rp);
>  
>      *io_buf = buf;
>      return EOK;
> @@ -2934,7 +2967,8 @@ parse_logon_right_with_libini(TALLOC_CTX *mem_ctx,
>  
>      if (ret != 0) {
>          DEBUG(SSSDBG_CRIT_FAILURE,
> -              "ini_get_string_config_array failed [%d][%s]\n", ret, strerror(ret));
> +              "ini_get_string_config_array failed [%d][%s]\n",
> +              ret, strerror(ret));
>          goto done;
>      }
>  
> @@ -2977,12 +3011,12 @@ parse_logon_right_with_libini(TALLOC_CTX *mem_ctx,
>   * allowed_sids and denied_sids
>   */
>  static errno_t
> -ad_gpo_parse_security_cse_buffer(TALLOC_CTX *mem_ctx,
> -                                 const char *filename,
> -                                 char ***allowed_sids,
> -                                 int *allowed_size,
> -                                 char ***denied_sids,
> -                                 int *denied_size)
> +ad_gpo_parse_policy_file(TALLOC_CTX *mem_ctx,
> +                         const char *filename,
> +                         char ***allowed_sids,
> +                         int *allowed_size,
> +                         char ***denied_sids,
> +                         int *denied_size)
>  {
>      struct ini_cfgfile *file_ctx = NULL;
>      struct ini_cfgobj *ini_config = NULL;
> @@ -3066,17 +3100,26 @@ ad_gpo_parse_security_cse_buffer(TALLOC_CTX *mem_ctx,
>  
>  static errno_t
>  ad_gpo_parse_gpo_child_response(TALLOC_CTX *mem_ctx,
> -                         uint8_t *buf, ssize_t size, uint32_t *_result)
> +                                uint8_t *buf,
> +                                ssize_t size,
> +                                uint32_t *_sysvol_gpt_version,
> +                                uint32_t *_result)
>  {
>  
>      int ret;
>      size_t p = 0;
> +    uint32_t sysvol_gpt_version;
>      uint32_t result;
>  
> +    /* sysvol_gpt_version */
> +    SAFEALIGN_COPY_UINT32_CHECK(&sysvol_gpt_version, buf + p, size, &p);
> +
>      /* operation result code */
>      SAFEALIGN_COPY_UINT32_CHECK(&result, buf + p, size, &p);
>  
> +    *_sysvol_gpt_version = sysvol_gpt_version;
>      *_result = result;
> +
>      ret = EOK;
>      return ret;
>  }
> @@ -3085,7 +3128,8 @@ ad_gpo_parse_gpo_child_response(TALLOC_CTX *mem_ctx,
>  
>  struct ad_gpo_process_cse_state {
>      struct tevent_context *ev;
> -    const char *cse_unix_path;
> +    const char *smb_path;
> +    const char *smb_cse_suffix;
>      pid_t child_pid;
>      uint8_t *buf;
>      ssize_t len;
> @@ -3141,13 +3185,19 @@ static void gpo_cse_done(struct tevent_req *subreq);
>  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)
>  {
>      struct tevent_req *req;
>      struct tevent_req *subreq;
>      struct ad_gpo_process_cse_state *state;
> +    struct ldb_result *res;
>      struct io_buffer *buf = NULL;
> +    int cached_gpt_version = 0;
>      errno_t ret;
>  
>      req = tevent_req_create(mem_ctx, &state, struct ad_gpo_process_cse_state);
> @@ -3157,10 +3207,10 @@ ad_gpo_process_cse_send(TALLOC_CTX *mem_ctx,
>      }
>  
>      state->ev = ev;
> -    state->cse_unix_path = cse_unix_path;
>      state->buf = NULL;
>      state->len = 0;
> -
> +    state->smb_path = smb_path;
> +    state->smb_cse_suffix = smb_cse_suffix;
>      state->io = talloc(state, struct io);
>      if (state->io == NULL) {
>          DEBUG(SSSDBG_CRIT_FAILURE, "talloc failed.\n");
> @@ -3172,8 +3222,33 @@ ad_gpo_process_cse_send(TALLOC_CTX *mem_ctx,
>      state->io->read_from_child_fd = -1;
>      talloc_set_destructor((void *) state->io, gpo_child_io_destructor);
>  
> +    /* retrieve cached gpt version (or set it to -1 if unavailable) */
> +    DEBUG(SSSDBG_TRACE_FUNC, "retrieving GPO from cache [%s]\n", gpo_guid);
> +    ret = sysdb_gpo_get_gpo(state, domain, gpo_guid, &res);
> +    if (ret != EOK) {
> +        switch (ret) {
> +        case ENOENT:
> +            DEBUG(SSSDBG_TRACE_FUNC, "ENOENT\n");
> +            cached_gpt_version = -1;
> +            break;
> +        default:
> +            DEBUG(SSSDBG_FATAL_FAILURE, "Could not read GPO from cache: [%s]\n",
> +                  strerror(ret));
> +            goto fail;

This is more of a question, but would it make sense to read the GPOs from
the server and attempt a recovery instead of failing?

The rest of the changes in this file look good to me, now on to
gpo_child.c...



More information about the sssd-devel mailing list