Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
On 04/10/2012 08:06 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 04/10/2012 03:46 PM, Rob Crittenden wrote: Petr Viktorin wrote: ... pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. Done: https://fedorahosted.org/freeipa/ticket/2619 I made a minor change. VERSION shoudl just update the minor version number. I changed this, ACK, pushed to master and ipa-2-2 rob I followed the comment in the VERSION file, which says: # The version of the IPA API. This controls which # # client versions can use the XML-RPC and json APIs# # # # A change to existing API requires a MAJOR version# # update. The addition of new API bumps the MINOR # # version. # I see the actual policy is different. How does this really work? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
Petr Viktorin wrote: On 04/10/2012 08:06 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 04/10/2012 03:46 PM, Rob Crittenden wrote: Petr Viktorin wrote: ... pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. Done: https://fedorahosted.org/freeipa/ticket/2619 I made a minor change. VERSION shoudl just update the minor version number. I changed this, ACK, pushed to master and ipa-2-2 rob I followed the comment in the VERSION file, which says: # The version of the IPA API. This controls which # # client versions can use the XML-RPC and json APIs # # # # A change to existing API requires a MAJOR version # # update. The addition of new API bumps the MINOR # # version. # I see the actual policy is different. How does this really work? Yes, I guess that isn't too clear. You can ADD new API without causing backwards compatibility issues. You can modify the validation without causing backwards compat issues (other than the data itself may no longer be allowed, but that is unrelated to the API). In these cases just bump the minor version. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
On 04/09/2012 03:55 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob Right, that makes more sense. It changes API.txt though. Do I need to bump VERSION in this case? Also, is there a reason pattern_errmsg is included in API.txt? -- Petr³ From ece4a28d2b6dc3c35e7fbc6fc3ef80a063312846 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 6 Apr 2012 04:56:46 -0400 Subject: [PATCH] Limit permission and selfservice names to alphanumerics, -, _, space The DN and ACI code doesn't always escape special characters properly. Rather than trying to fix it, this patch takes the easy way out and enforces that the names are safe. https://fedorahosted.org/freeipa/ticket/2585 --- API.txt | 26 +- ipalib/plugins/permission.py |4 ipalib/plugins/selfservice.py|4 tests/test_xmlrpc/test_permission_plugin.py | 11 +++ tests/test_xmlrpc/test_selfservice_plugin.py | 13 + 5 files changed, 45 insertions(+), 13 deletions(-) diff --git a/API.txt b/API.txt index c1b2ba05c40576abf6328f834eadb11e2d030b49..a49376817daf06b504e8148783e50091a62b6363 100644 --- a/API.txt +++ b/API.txt @@ -2039,7 +2039,7 @@ output: Output('result', type 'bool', None) output: Output('value', type 'unicode', None) command: permission_add args: 1,12,3 -arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True) +arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=True) option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True) option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False) option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord')) @@ -2057,25 +2057,25 @@ output: Entry('result', type 'dict', Gettext('A dictionary representing an LDA output: Output('value', type 'unicode', None) command: permission_add_member args: 1,4,3 -arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True) +arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True) option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('version?', exclude='webui') option: Str('privilege*', alwaysask=True, cli_name='privileges', csv=True) output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None)) output: Output('failed', type 'dict', None) output: Output('completed', type 'int', None) command: permission_del args: 1,1,3 -arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True) +arg: Str('cn', attribute=True, cli_name='name', multivalue=True, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True) option: Flag('continue', autofill=True, cli_name='continue', default=False) output: Output('summary', (type 'unicode', type 'NoneType'), None) output: Output('result', type 'dict', None) output: Output('value', type 'unicode', None) command: permission_find args: 1,14,4 arg: Str('criteria?', noextrawhitespace=False) -option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, primary_key=True, query=True, required=False) +option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=False) option: Str('permissions',
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
Petr Viktorin wrote: On 04/09/2012 03:55 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob Right, that makes more sense. It changes API.txt though. Do I need to bump VERSION in this case? Also, is there a reason pattern_errmsg is included in API.txt? Yes, please bump VERSION. pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
On 04/10/2012 03:46 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 04/09/2012 03:55 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob Right, that makes more sense. It changes API.txt though. Do I need to bump VERSION in this case? Also, is there a reason pattern_errmsg is included in API.txt? Yes, please bump VERSION. Attaching updated patch. pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. Done: https://fedorahosted.org/freeipa/ticket/2619 -- Petr³ From 0454790c2c037f70ad268e6bdd4a25ca8927ce6a Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 6 Apr 2012 04:56:46 -0400 Subject: [PATCH] Limit permission and selfservice names to alphanumerics, -, _, space The DN and ACI code doesn't always escape special characters properly. Rather than trying to fix it, this patch takes the easy way out and enforces that the names are safe. https://fedorahosted.org/freeipa/ticket/2585 --- API.txt | 26 +- VERSION |4 ++-- ipalib/plugins/permission.py |4 ipalib/plugins/selfservice.py|4 tests/test_xmlrpc/test_permission_plugin.py | 11 +++ tests/test_xmlrpc/test_selfservice_plugin.py | 13 + 6 files changed, 47 insertions(+), 15 deletions(-) diff --git a/API.txt b/API.txt index c1b2ba05c40576abf6328f834eadb11e2d030b49..a49376817daf06b504e8148783e50091a62b6363 100644 --- a/API.txt +++ b/API.txt @@ -2039,7 +2039,7 @@ output: Output('result', type 'bool', None) output: Output('value', type 'unicode', None) command: permission_add args: 1,12,3 -arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True) +arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=True) option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True) option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False) option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord')) @@ -2057,25 +2057,25 @@ output: Entry('result', type 'dict', Gettext('A dictionary representing an LDA output: Output('value', type 'unicode', None) command: permission_add_member args: 1,4,3 -arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True) +arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True) option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('version?', exclude='webui') option: Str('privilege*', alwaysask=True, cli_name='privileges', csv=True) output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None)) output: Output('failed', type 'dict', None) output: Output('completed', type 'int', None) command: permission_del args: 1,1,3 -arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True) +arg: Str('cn', attribute=True, cli_name='name', multivalue=True, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True) option: Flag('continue', autofill=True, cli_name='continue', default=False) output: Output('summary', (type 'unicode', type 'NoneType'), None) output: Output('result', type 'dict', None) output: Output('value', type 'unicode', None) command: permission_find args: 1,14,4 arg:
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
Petr Viktorin wrote: On 04/10/2012 03:46 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 04/09/2012 03:55 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob Right, that makes more sense. It changes API.txt though. Do I need to bump VERSION in this case? Also, is there a reason pattern_errmsg is included in API.txt? Yes, please bump VERSION. Attaching updated patch. pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. Done: https://fedorahosted.org/freeipa/ticket/2619 I made a minor change. VERSION shoudl just update the minor version number. I changed this, ACK, pushed to master and ipa-2-2 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. -- Petr³ From 20a04656131dfeb36ee8b8f8313e6d712411c1e0 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 6 Apr 2012 04:56:46 -0400 Subject: [PATCH] Limit permission and selfservice names to alphanumerics, -, _, space The DN and ACI code doesn't always escape special characters properly. Rather than trying to fix it, this patch takes the easy way out and enforces that the names are safe. https://fedorahosted.org/freeipa/ticket/2585 --- ipalib/plugins/permission.py | 10 +- ipalib/plugins/selfservice.py| 10 +- tests/test_xmlrpc/test_permission_plugin.py | 11 +++ tests/test_xmlrpc/test_selfservice_plugin.py | 13 + 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py index ce2536d9921ede73d2c26468f5d99609552e1881..01f081c3829c9cd314fcd41c7578bf9f74ed82c3 100644 --- a/ipalib/plugins/permission.py +++ b/ipalib/plugins/permission.py @@ -18,6 +18,8 @@ # along with this program. If not, see http://www.gnu.org/licenses/. import copy +import re + from ipalib.plugins.baseldap import * from ipalib import api, _, ngettext from ipalib import Flag, Str, StrEnum @@ -92,6 +94,12 @@ dn_ipaconfig = str(DN('cn=ipaconfig,cn=etc,%s' % api.env.basedn)) + +def validate_name(ugettext, name): +if re.search('[^-a-zA-Z0-9 _]', name): +return _('May only contain alphanumerics or -, _, space') + + def check_attrs(attrs, type): # Trying to delete attributes - no need for validation if attrs is None: @@ -150,7 +158,7 @@ class permission(LDAPObject): label_singular = _('Permission') takes_params = ( -Str('cn', +Str('cn', validate_name, cli_name='name', label=_('Permission name'), primary_key=True, diff --git a/ipalib/plugins/selfservice.py b/ipalib/plugins/selfservice.py index 6f843d469be2e5c29fb7587fcef98069b839eec5..45306707893babbddf1a838e9bf5ff29454af6bc 100644 --- a/ipalib/plugins/selfservice.py +++ b/ipalib/plugins/selfservice.py @@ -18,6 +18,8 @@ # along with this program. If not, see http://www.gnu.org/licenses/. import copy +import re + from ipalib import api, _, ngettext from ipalib import Flag, Str from ipalib.request import context @@ -60,6 +62,12 @@ ), ) + +def validate_name(ugettext, name): +if re.search('[^-a-zA-Z0-9 _]', name): +return _('May only contain alphanumerics or -, _, space') + + class selfservice(Object): Selfservice object. @@ -72,7 +80,7 @@ class selfservice(Object): label_singular = _('Self Service Permission') takes_params = ( -Str('aciname', +Str('aciname', validate_name, cli_name='name', label=_('Self-service name'), doc=_('Self-service name'), diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py index 51546732ca3ca4b3eea777024d0e9a21b75b83cf..b9c3e9bbdb8dde652cc935f5a3c775d32b2372db 100644 --- a/tests/test_xmlrpc/test_permission_plugin.py +++ b/tests/test_xmlrpc/test_permission_plugin.py @@ -45,6 +45,8 @@ privilege1_dn = DN(('cn',privilege1), api.env.container_privilege,api.env.basedn) +invalid_permission1 = u'bad;perm' + class test_permission(Declarative): @@ -712,5 +714,14 @@ class test_permission(Declarative): ), ), +dict( +desc='Try to create invalid %r' % invalid_permission1, +command=('permission_add', [invalid_permission1], dict( + type=u'user', + permissions=u'write', +)), +expected=errors.ValidationError(name='name', +error='May only contain alphanumerics or -, _, space'), +), ] diff --git a/tests/test_xmlrpc/test_selfservice_plugin.py b/tests/test_xmlrpc/test_selfservice_plugin.py index d457627c5c5f8318af8c89c606b6ca037e7559c6..8d043494411382a399daf5dcb2a4ca1a9018c8f5 100644 --- a/tests/test_xmlrpc/test_selfservice_plugin.py +++ b/tests/test_xmlrpc/test_selfservice_plugin.py @@ -26,6 +26,7 @@ from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid selfservice1 = u'testself' +invalid_selfservice1 = u'bad+name' class test_selfservice(Declarative): @@ -270,4 +271,16 @@ class