[Freeipa-devel] [PATCH] 26 Fix '--random' param behaviour for host plugin
Ondrej Hamada
ohamada at redhat.com
Tue Jun 26 14:27:07 UTC 2012
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.
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: ohama at jabbim.cz
IRC: ohamada
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ohamada-26-3-Change-random-passwords-behaviour.patch
Type: text/x-patch
Size: 6376 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120626/32aeac18/attachment.bin>
More information about the Freeipa-devel
mailing list