[SSSD] [PATCH] AD: support gpo processing in offline mode
Jakub Hrozek
jhrozek at redhat.com
Tue Aug 5 10:14:47 UTC 2014
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 at 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.
More information about the sssd-devel
mailing list