On 06/27/2012 12:45 PM, Petr Viktorin wrote: > On 06/26/2012 04:27 PM, Ondrej Hamada wrote: >> On 06/25/2012 04:59 PM, Petr Viktorin wrote: >>> On 06/20/2012 05:43 PM, Ondrej Hamada wrote: >>>> On 06/15/2012 07:36 AM, Martin Kosek wrote: >>>>> On Thu, 2012-06-14 at 16:35 -0400, Rob Crittenden wrote: >>>>>> Ondrej Hamada wrote: >>>>>>> Improved options checking so that host-mod operation is not changing >>>>>>> password for enrolled host when '--random' option is used. >>>>>>> >>>>>>> https://fedorahosted.org/freeipa/ticket/2799 >>>>>>> >>>>>>> Updated set of characters that is used for generating random >>>>>>> passwords >>>>>>> for ipa hosts. Following characters were removed from the set: >>>>>>> '"`\$<> >>>>>>> >>>>>>> https://fedorahosted.org/freeipa/ticket/2800 >>>>>> This works ok but it would be nice to have a test for both setting a >>>>>> password and random on an enrolled host to prevent regressions. We >>>>>> have >>>>>> some ipa-getkeytab tests already and these can be extended to test >>>>>> this >>>>>> I think. >>>>>> >>>>>> Might be nice to mention in the inline comment the set of characters >>>>>> excluded and why. >>>>>> >>>>>> rob >>>>>> >>>> I've added new test class into test_host_plugin.py that takes care of >>>> that. Just there is a problem that the ipa-join command always fails on >>>> 'adding key into keytab'. But the attributes necessary for testing are >>>> set correctly, so the testing can continue. >>>>> We already generate passwords for users with this character set: >>>>> user_pwdchars = string.digits + string.ascii_letters + '_,.@+-=' >>>>> >>>>> Why would we want to generate passwords for host enrolling with a >>>>> different set? Additionally, I think the set of characters you chose is >>>>> too wide, try entering a passwords with ' ', !, (, ), &, or ; without >>>>> careful escaping or quoting... >>>>> >>>>> Martin >>>>> >>>> Ok, I've used the same set of characters as for the user passwords. >>> >>> Should this set just be used for generated passwords by default? >>> Possibly with slightly longer passwords so they aren't suddenly weaker. >> >> I prefer to generate strong passwords by default and if anyone needs >> easier one, then he must adjust it. Especially in this case when we use >> one generator in different places. >>> >>> >>> >>> Anyway, the patch works great here. I just have a few style issues: >>> >>>> >>>> freeipa-ohamada-26-2-Change-random-passwords-behaviour.patch >>>> >>>> >>>> From bc19f44023643ff726e6e36634fbcbcbd0859583 Mon Sep 17 00:00:00 2001 >>>> From: Ondrej Hamada<oham...@redhat.com> >>>> Date: Mon, 18 Jun 2012 15:25:05 +0200 >>>> Subject: [PATCH] Change random passwords behaviour >>>> >>>> Improved options checking so that host-mod operation is not changing >>>> password for enrolled host when '--random' option is used. >>>> >>>> Unit tests added. >>>> >>>> https://fedorahosted.org/freeipa/ticket/2799 >>>> >>>> Updated set of characters that is used for generating random passwords >>>> for ipa hosts. All characters that might need escaping were removed. >>>> >>>> https://fedorahosted.org/freeipa/ticket/2800 >>>> --- >>>> ipalib/plugins/host.py | 11 ++++- >>>> tests/test_xmlrpc/test_host_plugin.py | 75 >>>> ++++++++++++++++++++++++++++++++- >>>> 2 files changed, 82 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py >>>> index >>>> 96b73cc5594335ad02dd43f87e7e011ab84157a1..9680d7c024ea8976f92a71bf576d6712c44a2bcf >>>> >>>> 100644 >>>> --- a/ipalib/plugins/host.py >>>> +++ b/ipalib/plugins/host.py >>>> @@ -24,6 +24,7 @@ import sys >>>> from nss.error import NSPRError >>>> import nss.nss as nss >>>> import netaddr >>>> +import string >>>> >>>> from ipalib import api, errors, util >>>> from ipalib import Str, Flag, Bytes >>>> @@ -99,6 +100,10 @@ EXAMPLES: >>>> ipa host-add-managedby --hosts=test2 test >>>> """) >>>> >>>> +# Characters to be used by random password generator >>>> +# The set was chosen to avoid the need for escaping the characters >>>> by user >>>> +host_pwd_chars=string.digits + string.ascii_letters + '_,.@+-=' >>>> + >>>> def remove_fwd_ptr(ipaddr, host, domain, recordtype): >>>> api.log.debug('deleting ipaddr %s' % ipaddr) >>>> try: >>>> @@ -404,7 +409,7 @@ class host_add(LDAPCreate): >>>> if 'krbprincipal' in entry_attrs['objectclass']: >>>> entry_attrs['objectclass'].remove('krbprincipal') >>>> if options.get('random'): >>>> - entry_attrs['userpassword'] = ipa_generate_password() >>>> + entry_attrs['userpassword'] = >>>> ipa_generate_password(characters=host_pwd_chars) >>>> # save the password so it can be displayed in >>>> post_callback >>>> setattr(context, 'randompassword', >>>> entry_attrs['userpassword']) >>>> cert = options.get('usercertificate') >>>> @@ -596,7 +601,7 @@ class host_mod(LDAPUpdate): >>>> def pre_callback(self, ldap, dn, entry_attrs, attrs_list, >>>> *keys, **options): >>>> # Allow an existing OTP to be reset but don't allow a OTP >>>> to be >>>> # added to an enrolled host. >>>> - if 'userpassword' in options: >>>> + if options.get('userpassword') or options.get('random'): >>>> entry = {} >>>> self.obj.get_password_attributes(ldap, dn, entry) >>>> if not entry['has_password'] and entry['has_keytab']: >>>> @@ -649,7 +654,7 @@ class host_mod(LDAPUpdate): >>>> entry_attrs['usercertificate'] = cert >>>> >>>> if options.get('random'): >>>> - entry_attrs['userpassword'] = ipa_generate_password() >>>> + entry_attrs['userpassword'] = >>>> ipa_generate_password(characters=host_pwd_chars) >>>> setattr(context, 'randompassword', >>>> entry_attrs['userpassword']) >>>> if 'macaddress' in entry_attrs: >>>> if 'objectclass' in entry_attrs: >>>> diff --git a/tests/test_xmlrpc/test_host_plugin.py >>>> b/tests/test_xmlrpc/test_host_plugin.py >>>> index >>>> 8798168afa71653b64870c77d11a7fa81ec4c952..fa1f2906f556af388499eac316c4b7c05c66ad85 >>>> >>>> 100644 >>>> --- a/tests/test_xmlrpc/test_host_plugin.py >>>> +++ b/tests/test_xmlrpc/test_host_plugin.py >>>> @@ -22,9 +22,13 @@ >>>> Test the `ipalib.plugins.host` module. >>>> """ >>>> >>>> +import os >>>> +import tempfile >>>> +from ipapython import ipautil >>>> from ipalib import api, errors, x509 >>>> from ipalib.dn import * >>>> -from tests.test_xmlrpc.xmlrpc_test import Declarative, fuzzy_uuid, >>>> fuzzy_digits >>>> +from tests.test_xmlrpc.xmlrpc_test import Declarative, XMLRPC_test >>>> +from tests.test_xmlrpc.xmlrpc_test import fuzzy_uuid, fuzzy_digits >>>> from tests.test_xmlrpc.xmlrpc_test import fuzzy_hash, fuzzy_date, >>>> fuzzy_issuer >>>> from tests.test_xmlrpc.xmlrpc_test import fuzzy_hex >>> >>> To avoid the repetition you can put the imported names in parentheses: >>> >>> from tests.test_xmlrpc.xmlrpc_test import (Declarative, XMLRPC_test, >>> fuzzy_uuid, fuzzy_digits, fuzzy_hash, fuzzy_date, fuzzy_issuer, >>> fuzzy_hex) >>> >>> >>>> from tests.test_xmlrpc import objectclasses >>>> @@ -740,3 +744,72 @@ class test_host(Declarative): >>>> ), >>>> >>>> ] >>>> + >>>> +class test_host_false_pwd_change(XMLRPC_test): >>>> + >>>> + fqdn1 = u'testhost1.%s' % api.env.domain >>>> + short1 = u'testhost1' >>>> + new_pass = u'pass_123' >>>> + >>>> + command = "ipa-client/ipa-join" >>>> + [keytabfd, keytabname] = tempfile.mkstemp() >>>> + os.close(keytabfd) >>>> + >>>> + # auxiliary function for checking whether the join operation has >>>> set >>>> + # correct attributes >>>> + def keytab_exists(self): >>>> + ret = api.Command['host_show'](self.fqdn1,all=True) >>>> + assert (ret['result']['has_keytab'] == True) >>>> + assert (ret['result']['has_password'] == False) >>> >>> The parentheses around assert's argument are unnecessary. >>> >>>> + def test_a_join_host(self): >>>> + """ >>>> + Create a test host and join him into IPA. >>>> + """ >>>> + try: >>>> + random_pass = api.Command['host_add'](self.fqdn1, >>>> random=True, force=True)['result']['randompassword'] >>>> + except: >>>> + # new host must be created with the random password >>>> + assert (False) >>> >>> I don't see why you used a try/except block here. It's not good to >>> hide the error that was raised. >>> >>>> + new_args = [self.command, >>>> + "-s", api.env.host, >>>> + "-h", self.fqdn1, >>>> + "-k", self.keytabname, >>>> + "-w", random_pass, >>>> + "-q", >>>> + ] >>>> + try: >>>> + # join operation may fail on 'adding key into keytab', but >>>> + # the keytab is not necessary for further tests >>>> + (out, err, rc) = ipautil.run(new_args, None) >>>> + self.keytab_exists() >>>> + except ipautil.CalledProcessError, e: >>>> + self.keytab_exists() >>>> + >>>> + def test_b_try_password(self): >>>> + """ >>>> + Try to change the password of enrolled host with specified >>>> password >>>> + """ >>>> + try: >>>> + api.Command['host_mod'](self.fqdn1,userpassword=self.new_pass) >>> >>> Add a space after the comma (here and below). >>> >>>> + assert (False) >>>> + except errors.ValidationError: >>>> + pass >>> >>> It's better to use nose's @raises decorator here. See for example >>> test_hbac_plugin.py. >>> >>>> + def test_c_try_random(self): >>>> + """ >>>> + Try to change the password of enrolled host with random >>>> password >>>> + """ >>>> + try: >>>> + api.Command['host_mod'](self.fqdn1,random=True) >>>> + assert (False) >>>> + except errors.ValidationError: >>>> + pass >>>> + >>>> + def test_d_cleanup(self): >>>> + """ >>>> + Clean up test data >>>> + """ >>>> + os.unlink(self.keytabname) >>>> + api.Command['host_del'](self.fqdn1) >>>> -- 1.7.6.5 >>>> >>>> >>>> >>>> _______________________________________________ >>>> Freeipa-devel mailing list >>>> Freeipa-devel@redhat.com >>>> https://www.redhat.com/mailman/listinfo/freeipa-devel >>>> >>> >>> >> Thanks for the coding style hints, it looks better now. Corrected patch >> attached. >> > > Looks good. ACK. >
Pushed to master. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel