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

Martin Kosek mkosek at redhat.com
Fri May 27 17:21:47 UTC 2011


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





More information about the Freeipa-devel mailing list