[Freeipa-devel] [PATCH] 26 Fix '--random' param behaviour for host plugin
Martin Kosek
mkosek at redhat.com
Wed Jun 27 11:00:38 UTC 2012
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 + '_,. at +-='
>>>>>
>>>>> 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<ohamada at 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 + '_,. at +-='
>>>> +
>>>> 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 at 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
More information about the Freeipa-devel
mailing list