On Thu, Jul 31, 2014 at 02:40:02PM -0400, Yassir Elley wrote:
Hi,
The attached patch adds support for gpo processing in offline mode. While the code for
online mode uses LDAP to determine which gpo-guids are applicable (and then uses SMB to
retrieve policy files), the code in offline mode simply retrieves all gpo-guids from the
cache (and then retrieves locally cached per-gpo-guid policy files). Note that neither
version checking nor the ad_gpo_cache_timeout option are relevant when in offline mode.
Unresolved issues
* if there are no gpo-guids in the cache, the code currently denies
access; i suspect we should be allowing access instead; agree?
If all the request ran into completion /and/ yet we found no GPOs, then
it means there are no GPOs on the server side, right?
If that's true, then I agree. I don't think there is any equivalent of
IPA's allow_all, correct?
* i don't think offline callbacks are needed, but i'm unclear
about whether
online callbacks are needed; i suspect they are not needed for the access
provider (b/c I don't see them being used by the ad_access_filter code);
should we trigger a fresh round of gpo processing when transitioning from
offline to online?
This is something I'll test, but I suspect that we don't need additional
callbacks. Usually there would be authentication attempt before the
access control that would flip the offline state to online.
>
Regards,
Yassir.
From 99f1f669b20d9f101916ebcf70bd463343624fa1 Mon Sep 17 00:00:00
2001
From: Yassir Elley <yelley(a)redhat.com>
Date: Mon, 21 Jul 2014 15:56:56 -0400
Subject: [PATCH] AD-GPO: Support offline mode
---
src/db/sysdb.h | 7 ++-
src/db/sysdb_gpo.c | 58 +++++++++++++++++++-
It would be nice if you could split the sysdb changes into a separate
patch. ACK to the sysdb changes themselves, though.
src/providers/ad/ad_gpo.c | 134
++++++++++++++++++++++++++++++++++++++--------
3 files changed, 176 insertions(+), 23 deletions(-)
[...]
index
c26011d672886faadaf2bb60c4537c2de8bf6532..ab92c9a9e859c4fd8077118467948c50cc524f67 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -989,16 +989,67 @@ immediately:
return req;
}
+static errno_t
+process_offline_gpo(TALLOC_CTX *mem_ctx,
+ const char *user,
+ enum gpo_access_control_mode gpo_mode,
+ struct sss_domain_info *domain,
+ struct ldb_message *gpo_cache_entry)
+{
+ const char *policy_filename = NULL;
+ const char *cached_gpo_guid;
+ char **allowed_sids;
+ int allowed_size;
+ char **denied_sids;
+ int denied_size;
+ int ret;
+
+ cached_gpo_guid = ldb_msg_find_attr_as_string(gpo_cache_entry,
+ SYSDB_GPO_GUID_ATTR, NULL);
I think you should check the cached_gpo_guid for validity here and bail
out if cached_gpo_guid == NULL.
+
+ policy_filename = talloc_asprintf(mem_ctx,
+ GPO_CACHE_PATH"/%s/Policies/%s%s",
+ domain->name,
+ cached_gpo_guid,
+ GP_EXT_GUID_SECURITY_SUFFIX);
There is a missing NULL check here as well.
+
+ DEBUG(SSSDBG_TRACE_FUNC, "policy_filename:%s\n", policy_filename);
+
+ ret = ad_gpo_parse_policy_file(mem_ctx,
+ policy_filename,
+ &allowed_sids,
+ &allowed_size,
+ &denied_sids,
+ &denied_size);
+
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Cannot parse policy file: [%s][%d][%s]\n",
+ policy_filename, ret, strerror(ret));
+ goto done;
+ }
+
+ ret = ad_gpo_access_check
+ (mem_ctx, gpo_mode, user, domain,
+ allowed_sids, allowed_size, denied_sids, denied_size);
+
+ done:
+ return ret;
+}
+
static void
ad_gpo_connect_done(struct tevent_req *subreq)
{
struct tevent_req *req;
struct ad_gpo_access_state *state;
- char* filter;
+ char *filter;
char *sam_account_name;
char *domain_dn;
int dp_error;
errno_t ret;
+ struct ldb_result *res;
+ struct ldb_message *gpo_cache_entry;
+ int i;
const char *attrs[] = {AD_AT_DN, AD_AT_UAC, NULL};
@@ -1009,20 +1060,60 @@ ad_gpo_connect_done(struct tevent_req *subreq)
talloc_zfree(subreq);
if (ret != EOK) {
- /* TBD: handle (dp_error == DP_ERR_OFFLINE) case */
+ if (dp_error != DP_ERR_OFFLINE) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Failed to connect to AD server: [%d](%s)\n",
+ ret, strerror(ret));
+ goto done;
+ } else {
Could you move the whole "else" block into its own function?
+ DEBUG(SSSDBG_TRACE_FUNC, "Preparing for offline
operation.\n");
- DEBUG(SSSDBG_OP_FAILURE,
- "Failed to connect to AD server: [%d](%s)\n",
- ret, sss_strerror(ret));
+ ret = sysdb_gpo_get_gpos(state, state->domain, &res);
The rest looks good to me! Given I already had some formatting comments,
I also tested the patch and it works fine.