[Freeipa-devel] [PATCH] 26 Fix '--random' param behaviour for host plugin
Petr Viktorin
pviktori at redhat.com
Mon Jun 25 14:59:47 UTC 2012
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.
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
>
--
Petr³
More information about the Freeipa-devel
mailing list