[Freeipa-devel] [PATCH] 784 limit what attributes may be modified

[Martin Kosek]

On Fri, 2011-05-27 at 19:21 +0200, Martin Kosek wrote:
> On Fri, 2011-05-27 at 11:10 -0400, Rob Crittenden wrote:
> > Martin Kosek wrote:
> > > On Mon, 2011-05-16 at 17:46 -0400, Rob Crittenden wrote:
> > >> Add option to limit the attributes allowed in an entry.
> > >>
> > >> Kerberos ticket policy can update policy in a user entry. This allowed
> > >> set/addattr to be used to modify attributes outside of the ticket policy
> > >> perview, also bypassing all validation/normalization. Likewise the
> > >> ticket policy was updatable by the user plugin bypassing all validation.
> > >>
> > >> Add two new LDAPObject values to control this behavior:
> > >>
> > >> limit_object_classes: only attributes in these are allowed
> > >> disallow_object_classes: attributes in these are disallowed
> > >>
> > >> By default both of these lists are empty so are skipped.
> > >>
> > >> ticket 744
> > >>
> > >> rob
> > >
> > > NACK. I have some concerns with this patch. In function
> > > _check_limit_object_class:
> > >
> > > 1) You change input attribute 'attrs' by removing the items from it. If
> > > user passes the same list of attrs to be checked and the function is run
> > > twice, the 'attrs' parameter in second run is corrupt.
> > >
> > > You can try it by running e.g. `ipa krbtpolicy-mod --maxrenew=24044' and
> > > checking the value of this parameter in the function.
> > 
> > Good catch, updated patch attached.
> > 
> > >
> > > 2) The purpose of this statement is not clear to me:
> > > +    if len(attrs)>  0 and allow_only:
> > > +        raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=attrs[0]))
> > > Maybe just the exception text is misleading.
> > 
> > This function has 2 modes: "allow only the attributes in these 
> > objectclasses" or "specifically deny the attributes in these 
> > objectclasses". This enforces the first type. If when we've gone through 
> > all the attributes there are any left over they must not be allowed so 
> > raise an error. This is documented in the function header.
> 
> Thanks for explanation, now I get it. It all looks OK, ACK.
> 
> Martin
> 

Checked again as I had some second thoughts. But no problem found.

Pushed to master, ipa-2-0.

Martin