[PATCH] AD: support gpo processing in offline mode

[Yassir Elley]

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?
* 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?

Regards,
Yassir.

[Jakub Hrozek]

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

[Yassir Elley]

----- Original Message -----
> 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 we found no GPOs after running to completion, then it means that there are no *applicable* GPOs on the server side (taking into account the hostname, inheritance logic, dacl, supported-cse, etc). That's all that matters. There may well be other GPOs on the server side that were *not* applicable, but we don't care about those.
> If that's true, then I agree. I don't think there is any equivalent of
> IPA's allow_all, correct?

I think IPA's allow_all is simply the default HBAC rule that allows all access. For GPOs, in the absence of any Allow Logon Locally policy setting, the default is also to allow all access. In the case of IPA, as soon as the allow_all rule is removed, then the semantics become deny all (except for those allowed by additional rules). Similarly, as soon as a single user/group is added to the Allow Logon Locally policy setting, the semantics become deny all (except for the added users/groups).
> > 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 
> > 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.

The attached revised patches contain the requested changes, including adding code to allow access by default if no gpo_guids are found in the cache. In the online case, we were already allowing access by default if no applicable GPOs were found.

Regards,
Yassir.

[Jakub Hrozek]

On Sun, Aug 10, 2014 at 08:18:46PM -0400, Yassir Elley wrote:
> ----- Original Message -----
> > 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 we found no GPOs after running to completion, then it means that
> there are no *applicable* GPOs on the server side (taking into account the
> hostname, inheritance logic, dacl, supported-cse, etc). That's all that
> matters. There may well be other GPOs on the server side that were *not*
> applicable, but we don't care about those.

Right, then I agree with allowing access.
> > If that's true, then I agree. I don't think there is any equivalent of
> > IPA's allow_all, correct?
>
> I think IPA's allow_all is simply the default HBAC rule that allows
> all access. For GPOs, in the absence of any Allow Logon Locally policy
> setting, the default is also to allow all access. In the case of IPA,
> as soon as the allow_all rule is removed, then the semantics become deny
> all (except for those allowed by additional rules). Similarly, as soon as
> a single user/group is added to the Allow Logon Locally policy setting,
> the semantics become deny all (except for the added users/groups).

Yes, exactly.
> > > 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.
> > > >

Thanks for the patches, these work for me.

ACK.

[Jakub Hrozek]

On Wed, Aug 13, 2014 at 02:34:10PM +0200, Jakub Hrozek wrote:
> On Sun, Aug 10, 2014 at 08:18:46PM -0400, Yassir Elley wrote:
> > ----- Original Message -----
> > > 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 we found no GPOs after running to completion, then it means that
> > there are no *applicable* GPOs on the server side (taking into account the
> > hostname, inheritance logic, dacl, supported-cse, etc). That's all that
> > matters. There may well be other GPOs on the server side that were *not*
> > applicable, but we don't care about those.
>
> Right, then I agree with allowing access.
> > > If that's true, then I agree. I don't think there is any equivalent of
> > > IPA's allow_all, correct?
> >
> > I think IPA's allow_all is simply the default HBAC rule that allows
> > all access. For GPOs, in the absence of any Allow Logon Locally policy
> > setting, the default is also to allow all access. In the case of IPA,
> > as soon as the allow_all rule is removed, then the semantics become deny
> > all (except for those allowed by additional rules). Similarly, as soon as
> > a single user/group is added to the Allow Logon Locally policy setting,
> > the semantics become deny all (except for the added users/groups).
>
> Yes, exactly.
> > > > 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.
> > > > >
>
> Thanks for the patches, these work for me.
>
> ACK.

master:
    d3c6fca0f0d3b1c5d3dda3dcf3de0ae3ae4c0c38
    9bda5ab39fc3429191e2272a8be62e230677ecb1

Can you ack the sysdb patch in the thread "TESTS: Add unit tests for the
GPO interface" ? I've changed them to apply on top of these..