[SSSD] [PATCH] AD: LDAP component of GPO-based access control

Sumit Bose sbose at redhat.com
Fri Apr 18 12:08:44 UTC 2014


On Wed, Apr 16, 2014 at 08:47:17PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > 
> > 
> > ----- Original Message -----
> > > On Tue, Apr 15, 2014 at 07:05:10PM -0400, Yassir Elley wrote:
> > > > 
> > > > 
> > > > ----- Original Message -----
> > > > > On Tue, 2014-04-15 at 21:23 +0200, Jakub Hrozek wrote:
> > > > > > On Tue, Apr 15, 2014 at 03:03:55PM -0400, Simo Sorce wrote:
> > > > > > > > I was thinking 1.12.0 would be 'no' and 1.12.1 would be 'yes'.
> > > > > > > > 1.12.0
> > > > > > > > should be what F-21 ships with and 1.12.1 would be delivered as
> > > > > > > > an
> > > > > > > > update
> > > > > > > > to F-21. I'm not sure about RHEL 7.1, but I suppose we'd have
> > > > > > > > 1.12.1
> > > > > > > > or 1.12.2 in 7.1 (and I'm not making any prediction on RHEL the
> > > > > > > > product
> > > > > > > > now. I'm just a random dude who happens to have a .redhat.com
> > > > > > > > address).
> > > > > > > 
> > > > > > > This would change behavior midrelease, it is not nice,
> > > > > > > I would say
> > > > > > > 1.12.0 ships in permissive mode by default, and 1.13.0 switches to
> > > > > > > enforcing by default.
> > > > > > 
> > > > > > I realize changing a default in a minor release is not great, but
> > > > > > 1.13
> > > > > > will be at least 6 months from 1.12, realistically a year later..what
> > > > > > is
> > > > > > the point of a cool feature like GPO if nobody will use it?
> > > > > > 
> > > > > > > 
> > > > > > > Interested users can always manually switch to enforcing on their
> > > > > > > own,
> > > > > > > there is no need to rush this onto them.
> > > > > > 
> > > > > > I think most users will use whatever realmd sets up..I'm not sure,
> > > > > > but
> > > > > > is it still the simple access provider? Should we consider changing
> > > > > > the
> > > > > > default to 'ad' when we switch the GPO feature to 'enforcing' ?
> > > > > 
> > > > > If it is cool people will use it, also nobody will prohib Fedora/other
> > > > > distro's to change the distro default.
> > > > > 
> > > > > So if 1.13.0 is not out yet when F22 ship, F22 can still just turn it
> > > > > on.
> > > > > 
> > > > > > > > > * Since one of the goals of having an AD provider is to
> > > > > > > > > simplify
> > > > > > > > > configuration for AD environments (as compared to the LDAP
> > > > > > > > > provider),
> > > > > > > > > I think that using the 'order' option might undermine that goal
> > > > > > > > > (by
> > > > > > > > > introducing complexity).
> > > > > > > > 
> > > > > > > > I agree. The order was not a good idea, a boolean is much simpler
> > > > > > > > to
> > > > > > > > understand. If the ad_acess_filter was defined, it should have
> > > > > > > > been
> > > > > > > > used
> > > > > > > > without any additional 'order'..
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > * We should agree upon a name for the options field. How about
> > > > > > > > > "ad_enforce_gpo_hbac"= {True, False}?
> > > > > > > > 
> > > > > > > > Is HBAC a term that would be easily understood by AD admins? I
> > > > > > > > understand it, but I know what it means from working on IPA..
> > > > > > > > 
> > > > > > > > What about ad_access_enforce_gpo?
> > > > > > > 
> > > > > > > why the ad_ prefix ?
> > > > > > > 
> > > > > > > gpo_access_control = disabled|permissive|enforcing
> > > > > > 
> > > > > > This would sound like GPO is supported (or planned to be supported)
> > > > > > by
> > > > > > all backends. Compare entry_cache_timeout with ldap_search_timeout.
> > > > > 
> > > > > uhmm ok ad_gpo_access_control is ok as well, I think it is quite clear
> > > > > that GPO is an AD specific technology, but I may be biased from years
> > > > > of
> > > > > mental association :-)
> > > > >  
> > > > > > > > > * When we talk about "loudly complaining", is this simply a
> > > > > > > > > log message with high priority? If so, how high should it be
> > > > > > > > > (FATAL_FAILURE? CRIT_FAILURE? OP_FAILURE?)
> > > > > > > > 
> > > > > > > > I was thinking FATAL_FAILURE. I think a syslog message is
> > > > > > > > probably
> > > > > > > > an
> > > > > > > > overkill.
> > > > > > > 
> > > > > > > It is not a failure, let alone fatal.
> > > > > > > It should be something that gets logged in the security log
> > > > > > > however,
> > > > > > > so
> > > > > > > whatever ends up there by default is fine.
> > > > > > > 
> > > > > > > This is auditing, we shouldn't use *debug* messages for this.
> > > > > > 
> > > > > > Fine, whatever is better visible to the admin. I was just trying to
> > > > > > think
> > > > > > as the poor locked out admin "Oh sssd denied access? tail
> > > > > > /var/log/sssd/*.log".
> > > > > 
> > > > > Nothing prevent us from also emitting a debug message I guess, but
> > > > > normally sssd debug logs are not enabled, and the system log is a
> > > > > better
> > > > > place for these kind of messages.
> > > > > 
> > > > > Simo.
> > > > > 
> > > > > --
> > > > 
> > > > Hi Sumit,
> > > > 
> > > > As you know, the gpo-ldap patch you reviewed does not actually perform an
> > > > access control check (it only determines which GPOs are applicable to the
> > > > policy target). Enforcing the access control requires retrieval of files
> > > > via SMB, which will be in a subsequent gpo-smb patch. As such, I am
> > > > thinking that it would make more sense to postpone implementation of this
> > > > config option until the gpo-smb patch, at which time the config option
> > > > can
> > > > actually be tested. Do you agree?
> > > 
> > > Well, the config option can be tested by watching the log files even
> > > now.  But I think it is ok to add them with your next round of patches
> > > since the current one will "only" go to master. But please note that
> > > people still might have issues with it because failures in the GPO code
> > > will lead to access denied.
> > > 
> > > I you prefer you can send the patch for the new option in a separate
> > > patch as well.
> > > 
> > > bye,
> > > Sumit
> > > 
> > 
> > Actually, I agree that it may be useful to disable gpo settings. As such, I
> > will include the config option with this patch. "Disabled" mode will not
> > perform any gpo processing, while "permissive" mode and "enforcing" mode
> > *will* perform gpo processing (which doesn't currently include any
> > enforcement). Once the gpo-smb patch is complete (i.e. once gpo evaluation
> > and enforcement are implemented), the semantics for the
> > gpo_access_control_mode config option will be:
> > * "disabled":   no gpo evaluation or gpo enforcement
> > * "permissive": gpo evaluation, but no gpo enforcement (if access would be
> > denied, syslog message is output)
> > * "enforcing":  both gpo evaluation and gpo enforcement (if access is denied,
> > access is denied!)
> > 
> 
> I have attached a new patch, which squashes the changes made to address Sumit's code review comments with the original patch. Among other things, this new patch includes the ad_gpo_access_control config setting, and it also addresses the "libsss_ad.so is not portable!" warning (by introducing libsss_ad_common.la).
> 
> Regards,
> Yassir.

Thank you. Patch applies and is working as expected. I still have a few
minor comments and questions (see below), but in general it is an ACK
from my side.

Jakub, it would be nice if you can continue here. The only nice-to-have
fix is to use tmp_ctx (see below). But this can be fixed in the next
round as well.

bye,
Sumit

> +    /* supported GPO access control modes */
> +    enum gpo_access_control_mode {
> +        GPO_ACCESS_CONTROL_DISABLED = 1,
> +        GPO_ACCESS_CONTROL_PERMISSIVE = 2,
> +        GPO_ACCESS_CONTROL_ENFORCING = 3,
> +    } gpo_access_control_mode;
>  };

I prefer to include 0 when I used enums with explicit values because
then there is a know value if the enum is initialized with zeros. If you
think it is not needed I wonder why you assing explicit values at all?
Please note, there is no need to change anything here, I just would like
to know the reasoning.

 
> +static char *ad_gpo_parent_dn(TALLOC_CTX *mem_ctx,
> +                              struct ldb_context *ldb_ctx,
> +                              const char *dn)
> +{
> +    struct ldb_dn *ldb_dn;
> +    struct ldb_dn *parent_ldb_dn;
> +    const char *p;
> +    char *parent_dn;
> +
> +    ldb_dn = ldb_dn_new(mem_ctx, ldb_ctx, dn);
> +
> +    parent_ldb_dn = ldb_dn_get_parent(mem_ctx, ldb_dn);
> +
> +    p = ldb_dn_get_linearized(parent_ldb_dn);
> +
> +    parent_dn = talloc_strdup(mem_ctx, p);
> +
> +    return parent_dn;

just a comment as well. You are leaking ldb_dn here. This is ok with
talloc and your usage, because mem_ctx is the state of a request and
will go way soon. But you have to be careful when you call this function
with a long living memory context.

> +}
> +
> +/*
> + * This function populates the _som_list output parameter by parsing the input
> + * DN into a list of gp_som objects. This function essentially repeatedly
> + * appends the input DN's parent to the SOM List (if the parent starts with
> + * "OU=" or "DC="), until the first "DC=" component is reached.
> + * Example: if input DN is "CN=MyComputer,CN=Computers,OU=Sales,DC=FOO,DC=COM",
> + * then SOM List has 2 SOM entries: {[OU=Sales,DC=FOO,DC=COM], [DC=FOO, DC=COM]}
> + */
> +
> +static errno_t
> +ad_gpo_populate_som_list(TALLOC_CTX *mem_ctx,
> +                         struct ldb_context *ldb_ctx,
> +                         char *target_dn,
> +                         int *_num_soms,
> +                         struct gp_som ***_som_list)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    int ret;
> +    int rdn_count = 0;
> +    int som_idx = 0;
> +    struct gp_som **som_list;
> +    char *parent_dn = NULL;
> +    char *tmp_dn = NULL;
> +    struct ldb_dn *ldb_target_dn;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ldb_target_dn = ldb_dn_new(mem_ctx, ldb_ctx, target_dn);

Since you already have a tmp_ctx, I would suggest to use it.

> +    rdn_count = ldb_dn_get_comp_num(ldb_target_dn);
> +
> +    if (rdn_count == 0) {
> +        *_som_list = NULL;
> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    /* assume the worst-case, in which every parent is a SOM */
> +    /* include space for Site SOM and NULL: rdn_count + 1 + 1 */
> +    som_list = talloc_array(tmp_ctx, struct gp_som *, rdn_count + 1 + 1);
> +    if (som_list == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    /* first, populate the OU and Domain SOMs */
> +    tmp_dn = target_dn;;
> +    while ((parent_dn = ad_gpo_parent_dn(mem_ctx, ldb_ctx, tmp_dn))) {

I think you can even use tmp_ctx here as well, because you
talloc_strdup() the parent_dn later with another memory context.

> +
> +        if ((strncasecmp(parent_dn, "OU=", strlen("OU=")) == 0) ||
> +            (strncasecmp(parent_dn, "DC=", strlen("DC=")) == 0)) {
> +
> +            som_list[som_idx] = talloc_zero(som_list, struct gp_som);
> +            if (som_list[som_idx] == NULL) {
> +                ret = ENOMEM;
> +                goto done;
> +            }
> +            som_list[som_idx]->som_dn = talloc_strdup(som_list[som_idx],
> +                                                      parent_dn);
> +            if (som_list[som_idx]->som_dn == NULL) {
> +                ret = ENOMEM;
> +                goto done;
> +            }
> +            som_idx++;
> +        }
> +
> +        if (strncasecmp(parent_dn, "DC=", strlen("DC=")) == 0) {
> +            break;
> +        }
> +        tmp_dn = parent_dn;
> +    }
> +
> +    som_list[som_idx] = NULL;
> +
> +    *_num_soms = som_idx;
> +    *_som_list = talloc_steal(mem_ctx, som_list);
> +
> +    ret = EOK;
> +
> + done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +
> +/*



More information about the sssd-devel mailing list