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.
--
Regards,
Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada
From 03f4568e021b1cca1d402c41ecc50573bb87ce55 Mon Sep 17 00:00:00 2001
From: Ondrej Hamada <oham...@redhat.com>
Date: Tue, 26 Jun 2012 15:23:55 +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, 80 insertions(+), 6 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 69ef82e20dafdfed38669ec36c05a5055754b06c..019152586cf129e501875437f97cb358545bd9b7 100644
--- a/tests/test_xmlrpc/test_host_plugin.py
+++ b/tests/test_xmlrpc/test_host_plugin.py
@@ -22,11 +22,15 @@
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 fuzzy_hash, fuzzy_date, fuzzy_issuer
-from tests.test_xmlrpc.xmlrpc_test import fuzzy_hex
+from nose.tools import raises, assert_raises
+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
import base64
@@ -740,3 +744,68 @@ 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 host_joined(self):
+ ret = api.Command['host_show'](self.fqdn1, all=True)
+ assert (ret['result']['has_keytab'] == True)
+ assert (ret['result']['has_password'] == False)
+
+ def test_a_join_host(self):
+ """
+ Create a test host and join him into IPA.
+ """
+ # create a test host with bulk enrollment password
+ random_pass = api.Command['host_add'](self.fqdn1, random=True, force=True)['result']['randompassword']
+
+ # joint the host with the bulk password
+ 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)
+ except ipautil.CalledProcessError, e:
+ pass
+ finally:
+ self.host_joined()
+
+ @raises(errors.ValidationError)
+ def test_b_try_password(self):
+ """
+ Try to change the password of enrolled host with specified password
+ """
+ api.Command['host_mod'](self.fqdn1, userpassword=self.new_pass)
+
+ @raises(errors.ValidationError)
+ def test_c_try_random(self):
+ """
+ Try to change the password of enrolled host with random password
+ """
+ api.Command['host_mod'](self.fqdn1, random=True)
+
+ def test_d_cleanup(self):
+ """
+ Clean up test data
+ """
+ os.unlink(self.keytabname)
+ api.Command['host_del'](self.fqdn1)
+ # verify that it's gone
+ with assert_raises(errors.NotFound):
+ api.Command['host_show'](self.fqdn1)
--
1.7.6.5
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
