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?
@@ -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...