----- Original Message -----
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(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
[...]
> @@ -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?
I ran into problems when using an index initialized to 0. Although I don't remember
what those problems were anymore, I was able to solve them by using an index initialized
to -1, which is what I am using now, and which is why the current code works correctly
(and doesn't skip the first entry).
> @@ -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.
OK.
>
> 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.
OK.
>
> - 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.
If I store directly to the gp_gpo member variables, they are initially stored correctly.
However, later on (in ad_gpo_cse_step), the member variables have corrupted values. Not
sure why. As such, I have left this part of the code unchanged.
>
> /* 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?
I think it could make sense to attempt a recovery. However, I think it would be better to
fail quickly, so that the actual problem can be fixed, rather than attempting a recovery
which might mask the problem. For example, if the sysdb_gpo code was very buggy and failed
for some reason (every time!), but we kept reading the GPOs from the server anyway, then
the user would have a false sense of security that the cache was working and that the
gpo's would work fine in offline mode (which they wouldn't).
The rest of the changes in this file look good to me, now on to
gpo_child.c...
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I'll send a revised patch later.
Thanks,
Yassir.