Re: [Freeipa-devel] Sudorules user validation help
On Thu, 28 May 2015, Martin Kosek wrote: On 05/28/2015 04:27 PM, Drew Erny wrote: In the ticket, however, it's stated that if the user wants to use any combination of weird characters, they should be able to. Would it be better to just define a function like def validate_username(username, ignore_pattern=False): and have it ignore all username validation? Tough question. FreeIPA in general tries to sanitize user input and does not allow everything user wants and try to advise the best practices, as we see it. In your case, if we allow only the 2 mentioned user login formats, it should work for AD use case just fine and add some level of sanity check of the user name. BTW, given we run an LDAP search later on this user name, isn't there a possibility of LDAP injection if we choose to allow all characters, including ( and )? :-) In any case, if we choose to ignore the pattern, we do not need the extra validator function at all. We would just skip validation in the pre callback if a user is being added. I still think we should run the validator exactly for the reasons outlined above. There are few limiting factors for Active Directory and Linux environments -- while user and group objects names are specified in 'cn' attirbute in Active Directory, in POSIX environment we get the real name from sAMAccountName attribute for users: * Certain characters in the Relative Distinguished Names of objects must be escaped using the backslash, \, escape character. The characters that must be escaped are: , \ # + ; = In addition, any leading or trailing spaces in the RDN must be escaped. * The following characters are not allowed in sAMAccountName values: [ ] : ; | = + * ? / \ , * In Windows Server 2003 domains and above, if you do not assign a value for sAMAccountName, the system will create a semi-random value for your. This value will be similar to: $KJK000-H4GJL6AQOV1I * The schema allows 256 characters in sAMAccountName values. However, the system limits sAMAccountName to 20 characters for user and group objects and 16 characters for computer objects. As you can see, group names may have ( and ), also ! and few more characters which you have to escape properly before making them part of the LDAP filter. Additionally, we actually have to allow UTF-8 characters, not just ASCII as syntax for DirectoryString (OID 1.3.6.1.4.1.1466.115.121.1.15) requires that. On 05/28/2015 09:40 AM, Drew Erny wrote: OK, I see now what you mean by that. That is a simpler solution. I'll do it that way. On 05/28/2015 04:44 AM, Martin Kosek wrote: On 05/27/2015 08:41 PM, Drew Erny wrote: Hey, Freeipa-devel, I'm working on ticket #3226 (https://fedorahosted.org/freeipa/ticket/3226) I've identified the problem. The sudorules add user command adds the user validations at the end of it's pre-callback using add_external_pre_callback. However, the user plugin pattern-matches a string for the uid param, because it only allows certain characters. I've been picking through the codebase and I think I have enough understanding to ask this: What if we changed the user uid validation to a standalone rule function (you can do that, right? pass in a function as a validation rule?) that would normally just assert that the pattern matches, but could have that pattern matching validation overridden in some cases. I think that's the easiest, cleanest way to change user so that sudorules and other plugins can ignore this validation if necessary (I'm trying to figure out exactly how to implement this without changing any APIs). Am I understanding the plugin params API correctly, and can I do this? Is this the best way to do this? The only other solution I see is to write sudorules-specific code in some plugin-related (either user.py or baseldap.py module, which would create unwanted coupling. Most specifically, this would be a change to the object instantiated at ipalib/plugins/user.py line 467 Thoughts and suggestions? I think it would make sense to follow the example with validate_hostname and prepare a function validate_username(username, upn=False, netbios_name=False) [1]. where upn would allow using @. on top of current validator (i.e. u...@domain.test) and netbios_name would allow the DOMAIN\user style. I would just suggest making sure the standard user validation error message is still the same to avoid unnecessary QE fail positives. In add_external_pre_callback you could then just simply call validate_username(user, True, True) just like it is already done with hostname. My 2 cents. Martin [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380525(v=vs.85).aspx -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code -- / Alexander Bokovoy -- Manage your subscription for the Freeipa-devel mailing list:
Re: [Freeipa-devel] Sudorules user validation help
Ok, so should I write a regex that matches that broader pattern, and only allow sudorules users to be added that follow those broader restrictions? On 05/28/2015 02:09 PM, Alexander Bokovoy wrote: On Thu, 28 May 2015, Martin Kosek wrote: On 05/28/2015 04:27 PM, Drew Erny wrote: In the ticket, however, it's stated that if the user wants to use any combination of weird characters, they should be able to. Would it be better to just define a function like def validate_username(username, ignore_pattern=False): and have it ignore all username validation? Tough question. FreeIPA in general tries to sanitize user input and does not allow everything user wants and try to advise the best practices, as we see it. In your case, if we allow only the 2 mentioned user login formats, it should work for AD use case just fine and add some level of sanity check of the user name. BTW, given we run an LDAP search later on this user name, isn't there a possibility of LDAP injection if we choose to allow all characters, including ( and )? :-) In any case, if we choose to ignore the pattern, we do not need the extra validator function at all. We would just skip validation in the pre callback if a user is being added. I still think we should run the validator exactly for the reasons outlined above. There are few limiting factors for Active Directory and Linux environments -- while user and group objects names are specified in 'cn' attirbute in Active Directory, in POSIX environment we get the real name from sAMAccountName attribute for users: * Certain characters in the Relative Distinguished Names of objects must be escaped using the backslash, \, escape character. The characters that must be escaped are: , \ # + ; = In addition, any leading or trailing spaces in the RDN must be escaped. * The following characters are not allowed in sAMAccountName values: [ ] : ; | = + * ? / \ , * In Windows Server 2003 domains and above, if you do not assign a value for sAMAccountName, the system will create a semi-random value for your. This value will be similar to: $KJK000-H4GJL6AQOV1I * The schema allows 256 characters in sAMAccountName values. However, the system limits sAMAccountName to 20 characters for user and group objects and 16 characters for computer objects. As you can see, group names may have ( and ), also ! and few more characters which you have to escape properly before making them part of the LDAP filter. Additionally, we actually have to allow UTF-8 characters, not just ASCII as syntax for DirectoryString (OID 1.3.6.1.4.1.1466.115.121.1.15) requires that. On 05/28/2015 09:40 AM, Drew Erny wrote: OK, I see now what you mean by that. That is a simpler solution. I'll do it that way. On 05/28/2015 04:44 AM, Martin Kosek wrote: On 05/27/2015 08:41 PM, Drew Erny wrote: Hey, Freeipa-devel, I'm working on ticket #3226 (https://fedorahosted.org/freeipa/ticket/3226) I've identified the problem. The sudorules add user command adds the user validations at the end of it's pre-callback using add_external_pre_callback. However, the user plugin pattern-matches a string for the uid param, because it only allows certain characters. I've been picking through the codebase and I think I have enough understanding to ask this: What if we changed the user uid validation to a standalone rule function (you can do that, right? pass in a function as a validation rule?) that would normally just assert that the pattern matches, but could have that pattern matching validation overridden in some cases. I think that's the easiest, cleanest way to change user so that sudorules and other plugins can ignore this validation if necessary (I'm trying to figure out exactly how to implement this without changing any APIs). Am I understanding the plugin params API correctly, and can I do this? Is this the best way to do this? The only other solution I see is to write sudorules-specific code in some plugin-related (either user.py or baseldap.py module, which would create unwanted coupling. Most specifically, this would be a change to the object instantiated at ipalib/plugins/user.py line 467 Thoughts and suggestions? I think it would make sense to follow the example with validate_hostname and prepare a function validate_username(username, upn=False, netbios_name=False) [1]. where upn would allow using @. on top of current validator (i.e. u...@domain.test) and netbios_name would allow the DOMAIN\user style. I would just suggest making sure the standard user validation error message is still the same to avoid unnecessary QE fail positives. In add_external_pre_callback you could then just simply call validate_username(user, True, True) just like it is already done with hostname. My 2 cents. Martin [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380525(v=vs.85).aspx -- Manage your subscription for the Freeipa-devel mailing list:
Re: [Freeipa-devel] Sudorules user validation help
OK, I see now what you mean by that. That is a simpler solution. I'll do it that way. On 05/28/2015 04:44 AM, Martin Kosek wrote: On 05/27/2015 08:41 PM, Drew Erny wrote: Hey, Freeipa-devel, I'm working on ticket #3226 (https://fedorahosted.org/freeipa/ticket/3226) I've identified the problem. The sudorules add user command adds the user validations at the end of it's pre-callback using add_external_pre_callback. However, the user plugin pattern-matches a string for the uid param, because it only allows certain characters. I've been picking through the codebase and I think I have enough understanding to ask this: What if we changed the user uid validation to a standalone rule function (you can do that, right? pass in a function as a validation rule?) that would normally just assert that the pattern matches, but could have that pattern matching validation overridden in some cases. I think that's the easiest, cleanest way to change user so that sudorules and other plugins can ignore this validation if necessary (I'm trying to figure out exactly how to implement this without changing any APIs). Am I understanding the plugin params API correctly, and can I do this? Is this the best way to do this? The only other solution I see is to write sudorules-specific code in some plugin-related (either user.py or baseldap.py module, which would create unwanted coupling. Most specifically, this would be a change to the object instantiated at ipalib/plugins/user.py line 467 Thoughts and suggestions? I think it would make sense to follow the example with validate_hostname and prepare a function validate_username(username, upn=False, netbios_name=False) [1]. where upn would allow using @. on top of current validator (i.e. u...@domain.test) and netbios_name would allow the DOMAIN\user style. I would just suggest making sure the standard user validation error message is still the same to avoid unnecessary QE fail positives. In add_external_pre_callback you could then just simply call validate_username(user, True, True) just like it is already done with hostname. My 2 cents. Martin [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380525(v=vs.85).aspx -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] Sudorules user validation help
On 05/28/2015 04:27 PM, Drew Erny wrote: In the ticket, however, it's stated that if the user wants to use any combination of weird characters, they should be able to. Would it be better to just define a function like def validate_username(username, ignore_pattern=False): and have it ignore all username validation? Tough question. FreeIPA in general tries to sanitize user input and does not allow everything user wants and try to advise the best practices, as we see it. In your case, if we allow only the 2 mentioned user login formats, it should work for AD use case just fine and add some level of sanity check of the user name. BTW, given we run an LDAP search later on this user name, isn't there a possibility of LDAP injection if we choose to allow all characters, including ( and )? :-) In any case, if we choose to ignore the pattern, we do not need the extra validator function at all. We would just skip validation in the pre callback if a user is being added. On 05/28/2015 09:40 AM, Drew Erny wrote: OK, I see now what you mean by that. That is a simpler solution. I'll do it that way. On 05/28/2015 04:44 AM, Martin Kosek wrote: On 05/27/2015 08:41 PM, Drew Erny wrote: Hey, Freeipa-devel, I'm working on ticket #3226 (https://fedorahosted.org/freeipa/ticket/3226) I've identified the problem. The sudorules add user command adds the user validations at the end of it's pre-callback using add_external_pre_callback. However, the user plugin pattern-matches a string for the uid param, because it only allows certain characters. I've been picking through the codebase and I think I have enough understanding to ask this: What if we changed the user uid validation to a standalone rule function (you can do that, right? pass in a function as a validation rule?) that would normally just assert that the pattern matches, but could have that pattern matching validation overridden in some cases. I think that's the easiest, cleanest way to change user so that sudorules and other plugins can ignore this validation if necessary (I'm trying to figure out exactly how to implement this without changing any APIs). Am I understanding the plugin params API correctly, and can I do this? Is this the best way to do this? The only other solution I see is to write sudorules-specific code in some plugin-related (either user.py or baseldap.py module, which would create unwanted coupling. Most specifically, this would be a change to the object instantiated at ipalib/plugins/user.py line 467 Thoughts and suggestions? I think it would make sense to follow the example with validate_hostname and prepare a function validate_username(username, upn=False, netbios_name=False) [1]. where upn would allow using @. on top of current validator (i.e. u...@domain.test) and netbios_name would allow the DOMAIN\user style. I would just suggest making sure the standard user validation error message is still the same to avoid unnecessary QE fail positives. In add_external_pre_callback you could then just simply call validate_username(user, True, True) just like it is already done with hostname. My 2 cents. Martin [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380525(v=vs.85).aspx -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] Sudorules user validation help
Hey, Freeipa-devel, I'm working on ticket #3226 (https://fedorahosted.org/freeipa/ticket/3226) I've identified the problem. The sudorules add user command adds the user validations at the end of it's pre-callback using add_external_pre_callback. However, the user plugin pattern-matches a string for the uid param, because it only allows certain characters. I've been picking through the codebase and I think I have enough understanding to ask this: What if we changed the user uid validation to a standalone rule function (you can do that, right? pass in a function as a validation rule?) that would normally just assert that the pattern matches, but could have that pattern matching validation overridden in some cases. I think that's the easiest, cleanest way to change user so that sudorules and other plugins can ignore this validation if necessary (I'm trying to figure out exactly how to implement this without changing any APIs). Am I understanding the plugin params API correctly, and can I do this? Is this the best way to do this? The only other solution I see is to write sudorules-specific code in some plugin-related (either user.py or baseldap.py module, which would create unwanted coupling. Most specifically, this would be a change to the object instantiated at ipalib/plugins/user.py line 467 Thoughts and suggestions? Thanks, Drew Erny de...@redhat.com -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
