[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