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

Yassir Elley yelley at redhat.com
Fri Apr 18 16:56:27 UTC 2014



----- Original Message -----
> 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

Hi,

I responded to your comments (see below) and have attached a revised patch, which includes changes for:
* numbering the enum from 0
* using tmp_ctx in ad_gpo_populate_som_list
* rewriting ad_gpo_parent_dn so there is no leak

Thanks,
Yassir.

> 
> > +    /* 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.

I think I was pattern-matching off "enum schema_type", which starts with a 1. But I agree that there is no reason to use explicit values at all here. I have made the necessary change.

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

I agree with your comment. I have rewritten the ad_gpo_parent_dn function so that there are no memory leaks.

> 
> > +}
> > +
> > +/*
> > + * 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.

I agree. I have made the change.

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

I agree. I have made the change.

Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implemented-LDAP-component-of-GPO-based-access-contr.patch
Type: text/x-patch
Size: 104518 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140418/e3c07fce/attachment.bin>


More information about the sssd-devel mailing list