[Freeipa-devel] [PATCH] 784 limit what attributes may be modified
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 freeipa-rcrit-784-krbtpolicy.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 784 limit what attributes may be modified
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. 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. Otherways it's good, it correctly raised an exception when I tried to misuse --setattr option. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 784 limit what attributes may be modified
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. Otherways it's good, it correctly raised an exception when I tried to misuse --setattr option. Martin >From 3be1c53c3eed62660ff102330675620931d195c4 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Mon, 16 May 2011 17:39:23 -0400 Subject: [PATCH] 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 --- ipalib/plugins/baseldap.py| 36 + ipalib/plugins/krbtpolicy.py |1 + ipalib/plugins/user.py|2 + tests/test_xmlrpc/test_krbtpolicy.py | 138 + tests/test_xmlrpc/test_user_plugin.py | 20 + 5 files changed, 197 insertions(+), 0 deletions(-) create mode 100644 tests/test_xmlrpc/test_krbtpolicy.py diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 43533c8..f07fb27 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -253,6 +253,8 @@ class LDAPObject(Object): # If an objectclass is possible but not default in an entry. Needed for # collecting attributes for ACI UI. possible_objectclasses = [] +limit_object_classes = [] # Only attributes in these are allowed +disallow_object_classes = [] # Disallow attributes in these search_attributes = [] search_attributes_config = None default_attributes = [] @@ -438,6 +440,36 @@ def _check_empty_attrs(params, entry_attrs): raise errors.RequirementError(name=a) +def _check_limit_object_class(attributes, attrs, allow_only): +""" +If the set of objectclasses is limited enforce that only those +are updated in entry_attrs (plus dn) + +allow_only tells us what mode to check in: + +If True then we enforce that the attributes must be in the list of +allowed. + +If False then those attributes are not allowed. +""" +if len(attributes[0]) == 0 and len(attributes[1]) == 0: +return +limitattrs = deepcopy(attrs) +# Go through the MUST first +for (oid, attr) in attributes[0].iteritems(): +if attr.names[0].lower() in limitattrs: +if not allow_only: +raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=attr.names[0].lower())) +limitattrs.remove(attr.names[0].lower()) +# And now the MAY +for (oid, attr) in attributes[1].iteritems(): +if attr.names[0].lower() in limitattrs: +if not allow_only: +raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=attr.names[0].lower())) +limitattrs.remove(attr.names[0].lower()) +if len(limitattrs) > 0 and allow_only: +raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not all
Re: [Freeipa-devel] [PATCH] 784 limit what attributes may be modified
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 784 limit what attributes may be modified
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 pushed to master and ipa-2-0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 784 limit what attributes may be modified
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
