[Freeipa-devel] [PATCH] 0001 Fix ipa-client-install --uninstall crash
https://fedorahosted.org/freeipa/ticket/4273 David Kupka From b9a2b18accf3dd41304d244b00aeeb4887d72784 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 4 Jul 2014 08:26:23 +0200 Subject: [PATCH] Fix ipa-client-install --uninstall crash Fix ipa-client-install crash when chronyd service fails to start. https://fedorahosted.org/freeipa/ticket/4273 --- ipa-client/ipa-install/ipa-client-install | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index bfa43b1468887dcd408cd8f2941f9fd961f372ce..de55b1d2bf230c6819d09656f8905735a1ff8ad7 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -692,7 +692,10 @@ def uninstall(options, env): if restored: services.knownservices.ntpd.restart() -ipaclient.ntpconf.restore_forced_ntpd(statestore) +try: +ipaclient.ntpconf.restore_forced_ntpd(statestore) +except CalledProcessError, e: +root_logger.error('Failed to start chronyd: %s' % str(e)) if was_sshd_configured and services.knownservices.sshd.is_running(): services.knownservices.sshd.restart() -- 1.9.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0001 Fix ipa-client-install --uninstall crash
On 07/07/2014 09:15 PM, Petr Viktorin wrote: On 07/04/2014 08:47 AM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4273 David Kupka Hi, This works fine. Just two nitpicks in the log message: - %s means convert to string, so the str() is redundant - the logger methods take items to interpolate as arguments, instead of one string built with %. So you'll want: root_logger.error('Failed to start chronyd: %s', e) Hi, thanks for comments. I've fixed the patch accordingly. -- David Kupka From 8f51cc43f11485568814ea6937c941a07d5a9e37 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 8 Jul 2014 07:36:48 +0200 Subject: [PATCH] Fix ipa-client-install --uninstall crash Fix ipa-client-install crash when chronyd service fails to start. https://fedorahosted.org/freeipa/ticket/4273 --- ipa-client/ipa-install/ipa-client-install | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index bfa43b1468887dcd408cd8f2941f9fd961f372ce..617db26f499106fa10665af3ca9d9f2736ba9b00 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -692,7 +692,10 @@ def uninstall(options, env): if restored: services.knownservices.ntpd.restart() -ipaclient.ntpconf.restore_forced_ntpd(statestore) +try: +ipaclient.ntpconf.restore_forced_ntpd(statestore) +except CalledProcessError, e: +root_logger.error('Failed to start chronyd: %s', e) if was_sshd_configured and services.knownservices.sshd.is_running(): services.knownservices.sshd.restart() -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0002 Improve password validity check
https://fedorahosted.org/freeipa/ticket/2796 -- David Kupka From c0fb9fe49a8b7eb190414571df211c87ba9c3166 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 18 Jul 2014 10:06:55 +0200 Subject: [PATCH] Improve password validity check. Allow use of characters that no longer cause troubles. Check for leading and trailing characters in case of 389 Direcory Manager password. https://fedorahosted.org/freeipa/ticket/2796 --- install/tools/ipa-server-install | 34 -- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 671a226d625ab9e8168c569a6d83c35dfae52115..5b107c3ff3b61f87c30561a1aeed5ab65cf0bf27 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -121,7 +121,37 @@ def validate_dm_password(password): raise ValueError(Password must only contain ASCII characters) # Disallow characters that pkisilent doesn't process properly: -bad_characters = ' \\%' +bad_characters = '\\' +if any(c in bad_characters for c in password): +raise ValueError('Password must not contain these characters: %s' % +', '.join('%s' % c for c in bad_characters)) + +# TODO: Check https://fedorahosted.org/389/ticket/47849 +# Actual behavior of setup-ds.pl is that it does not accept white +# space characters in password when called interactively but does when +# provided such password in INF file. But it ignores leading and trailing +# white spaces in INF file. + +# Disallow leading spaces (other white spaces are checked before) +bad_prefix = ' ' +if password.startswith(bad_prefix): +raise ValueError('Password must not start with %s.' % bad_prefix) + +# Disallow trailing spaces (other white spaces are checked before) +bad_suffix = ' ' +if password.endswith(bad_suffix): +raise ValueError('Password must not end with %s.' % bad_prefix) + +def validate_admin_password(password): +if len(password) 8: +raise ValueError(Password must be at least 8 characters long) +if any(ord(c) 0x20 for c in password): +raise ValueError(Password must not contain control characters) +if any(ord(c) = 0x7F for c in password): +raise ValueError(Password must only contain ASCII characters) + +# Disallow characters that pkisilent doesn't process properly: +bad_characters = '\\' if any(c in bad_characters for c in password): raise ValueError('Password must not contain these characters: %s' % ', '.join('%s' % c for c in bad_characters)) @@ -450,7 +480,7 @@ def read_admin_password(): print This user is a regular system account used for IPA server administration. print #TODO: provide the option of generating a random password -admin_password = read_password(IPA admin) +admin_password = read_password(IPA admin, validator=validate_admin_password) return admin_password def check_dirsrv(unattended): -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0003 Test generated passwords composed of all allowed charactes
Test verifying that IPA server is able to install using passwords composed of all but forbidden characters. Related to https://fedorahosted.org/freeipa/ticket/2796 -- David Kupka From e4d1c384288f4b5c5d08f9f3abd9393b3b868c80 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 18 Jul 2014 10:15:05 +0200 Subject: [PATCH] Test generated passwords composed of all allowed charactes. Test ipaserver installation with generated passwords composed of all allowed characters. --- ipatests/test_password/test_password.py | 93 + 1 file changed, 93 insertions(+) create mode 100644 ipatests/test_password/test_password.py diff --git a/ipatests/test_password/test_password.py b/ipatests/test_password/test_password.py new file mode 100644 index ..eae7cb54b3e49dd90928100288c6af7ba8869087 --- /dev/null +++ b/ipatests/test_password/test_password.py @@ -0,0 +1,93 @@ +# Authors: +# David Kupka dku...@redhat.com +# +# Copyright (C) 2014 Red Hat +# see file 'COPYING' for use and warranty information +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +import random +import itertools +from ipatests.test_integration.base import IntegrationTest +from ipatests.test_integration import tasks + + +class PasswordGenerator(object): + +Generate passwords as a permutation of all allowed (ASCII0x20 \ banned) +characters that succeds in validator. + +__test__ = False + +def __init__(self, banned='', validator=lambda p: True): +self.validator = validator +# use ASCII characters starting with ' ' +# exclude underlying-tool-specific disallowed characters +self.allowed = list(set([chr(c) for c in range(0x20, 0x7F)]) - set(banned)) + +def __call__(self): +pw = self.allowed + +while True: +random.shuffle(pw) +if self.validator(''.join(pw)): +return ''.join(pw) + +class TestPassword(IntegrationTest): + +Test to install ipa-server using passwords composed from all +allowed characters. + +__test__ = True + +banned_admin = '\\' +banned_dirman = '\\' + +@staticmethod +def validator_dirman(pw): +if pw.startswith(' ') or pw.endswith(' '): +return False +else: +return True + +def test_password(self): +# run 10 tests by default +tests = 10 +# password generator for admin +pg_admin = PasswordGenerator(self.banned_admin) +# password generator for dirman +pg_dirman = PasswordGenerator(self.banned_dirman, self.validator_dirman) + +# backup password +pw_old_admin = self.master.config.admin_password +pw_old_dirman = self.master.config.dirman_password + +for t in range(0, tests): +# generate passwords +pw_admin = pg_admin() +pw_dirman = pg_dirman() +# set tested password +self.master.config.admin_password = pw_admin +self.master.config.dirman_password = pw_dirman + +try: +# install master on configured server +tasks.install_master(self.master) +except Exception, e: +self.log.error('Failed to install ipa with -a=\'%s\', -p=\'%s\': %s' % (pw_admin, pw_dirman, e)) +# uninstall master +tasks.uninstall_master(self.master) + +#restore password +self.master.config.admin_password = pw_old_admin +self.master.config.dirman_password = pw_old_dirman -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0002 Improve password validity check
On 07/18/2014 12:52 PM, Martin Kosek wrote: On 07/18/2014 12:33 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/2796 1) Would it be easier/more convenient to just implement following simple check instead of bad_prefix/bad_suffix? if password.strip() != password: raise ValueError('Password must not start or end with whitespace') Yes it would. Edited patch attached. 2) The main goal of the ticket 2796 was not fixed yet. It sometimes happen that when installation crashes somewhere right after pkicreate, it does not record and and does not uninstall the PKI component during ipa-server-install --uninstall. You may artificially invoke some crash in cainstance.py after pkicreate to test it. When fixing it, check how is_configured() in Service object works an how self.backup_state is called in other service modules (like dsinstance.py) where the detection works correctly. You're completely right, Martin. I was unable to reproduce the bug (to force pkicreate/pkispawn to fail) so I thought that it was fixed by the password restriction. Then I discovered that most of the banned characters for password are no longer causing troubles a focused on this. But it's yet another issue. Martin -- David Kupka From e9985196820757e61b07eb6470b6dec66502f497 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 21 Jul 2014 15:53:07 +0200 Subject: [PATCH] Improve password validity check. Allow use of characters that no longer cause troubles. Check for leading and trailing characters in case of 389 Direcory Manager password. --- install/tools/ipa-server-install | 28 ++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 671a226d625ab9e8168c569a6d83c35dfae52115..e05b5fce7b77059cac2ad2318827c1df3ee5706b 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -121,7 +121,31 @@ def validate_dm_password(password): raise ValueError(Password must only contain ASCII characters) # Disallow characters that pkisilent doesn't process properly: -bad_characters = ' \\%' +bad_characters = '\\' +if any(c in bad_characters for c in password): +raise ValueError('Password must not contain these characters: %s' % +', '.join('%s' % c for c in bad_characters)) + +# TODO: Check https://fedorahosted.org/389/ticket/47849 +# Actual behavior of setup-ds.pl is that it does not accept white +# space characters in password when called interactively but does when +# provided such password in INF file. But it ignores leading and trailing +# white spaces in INF file. + +# Disallow leading/trailing whaitespaces +if password.strip() != password: +raise ValueError('Password must not start or end with whitespace.') + +def validate_admin_password(password): +if len(password) 8: +raise ValueError(Password must be at least 8 characters long) +if any(ord(c) 0x20 for c in password): +raise ValueError(Password must not contain control characters) +if any(ord(c) = 0x7F for c in password): +raise ValueError(Password must only contain ASCII characters) + +# Disallow characters that pkisilent doesn't process properly: +bad_characters = '\\' if any(c in bad_characters for c in password): raise ValueError('Password must not contain these characters: %s' % ', '.join('%s' % c for c in bad_characters)) @@ -450,7 +474,7 @@ def read_admin_password(): print This user is a regular system account used for IPA server administration. print #TODO: provide the option of generating a random password -admin_password = read_password(IPA admin) +admin_password = read_password(IPA admin, validator=validate_admin_password) return admin_password def check_dirsrv(unattended): -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] Always record that pkicreate has been executed
https://fedorahosted.org/freeipa/ticket/2796 -- David Kupka From 5d1e323d87aa4bf2b21ed11b062e68e56fe9d887 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 21 Jul 2014 15:57:18 +0200 Subject: [PATCH] Always record that pkicreate has been executed. Record that pkicreate/pkispawn has been executed to allow cleanup even if the installation did not finish correctly. https://fedorahosted.org/freeipa/ticket/2796 --- ipaserver/install/cainstance.py | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index b13a77d5811343175288c1191991f1ee6e6b721a..03aec95710d19b0f6cdc8eb6185ab0e832b28031 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -602,6 +602,7 @@ class CAInstance(service.Service): 'Contents of pkispawn configuration file (%s):\n%s' % (cfg_file, ipautil.nolog_replace(f.read(), nolog))) +self.backup_state('installed', True) try: ipautil.run(args, nolog=nolog) except ipautil.CalledProcessError, e: @@ -646,6 +647,7 @@ class CAInstance(service.Service): '-redirect', 'logs=/var/log/pki-ca', '-enable_proxy' ] +self.backup_state('installed', True) ipautil.run(args, env={'PKI_HOSTNAME':self.fqdn}) def __enable(self): @@ -1320,6 +1322,8 @@ class CAInstance(service.Service): if not enabled is None and not enabled: self.disable() +# Just eat this state if it exists +installed = self.restore_state(installed) try: if self.dogtag_constants.DOGTAG_VERSION = 10: ipautil.run([paths.PKIDESTROY, -i, @@ -1355,9 +1359,12 @@ class CAInstance(service.Service): # remove CRL files root_logger.info(Remove old CRL files) -for f in get_crl_files(): -root_logger.debug(Remove %s, f) -installutils.remove_file(f) +try: +for f in get_crl_files(): +root_logger.debug(Remove %s, f) +installutils.remove_file(f) +except OSError, e: +root_logger.warning(Error while removing old CRL files: %s % e) # remove CRL directory root_logger.info(Remove CRL directory) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Reasons for not using certmonger DBus API
While solving ticket #4280 I noticed that we are messing with certmonger's files right under its hands. That can lead to some unpleasant race condition issues. Is there any reason why not to call certmonger via DBus and ask it to stop tracking the requests? -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid
https://fedorahosted.org/freeipa/ticket/4244 -- David Kupka From 513fd9b6cf7502ed08e31318dd9425bc12392720 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 23 Jul 2014 15:32:18 +0200 Subject: [PATCH] Verify otptoken timespan is valid When creating or modifying otptoken check that token validity start is not after validity end. https://fedorahosted.org/freeipa/ticket/4244 --- ipalib/plugins/otptoken.py | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py index 2880ee660d5dcdb18c504f50d7b72f5b8fb43d48..7dc01caafdf73e3f54bb4fbdb2ee5e8540e09e74 100644 --- a/ipalib/plugins/otptoken.py +++ b/ipalib/plugins/otptoken.py @@ -21,7 +21,7 @@ from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMem from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext from ipalib.plugable import Registry -from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound +from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound, ValidationError from ipalib.request import context from ipalib.frontend import Local @@ -103,6 +103,17 @@ def _normalize_owner(userobj, entry_attrs): if owner is not None: entry_attrs['ipatokenowner'] = userobj.get_dn(owner) +def _check_interval(not_before, not_after): + +if not_before and not_after: +if type(not_before) is str: +not_before = DateTime('not_before')._convert_scalar(not_before) +if type(not_after) is str: +not_after = DateTime('not_after')._convert_scalar(not_after) + +if not_before not_after: +return False +return True @register() class otptoken(LDAPObject): @@ -254,6 +265,11 @@ class otptoken_add(LDAPCreate): entry_attrs['ipatokenuniqueid'] = str(uuid.uuid4()) dn = DN(ipatokenuniqueid=%s % entry_attrs['ipatokenuniqueid'], dn) +if not _check_interval(entry_attrs.get('ipatokennotbefore', None), + entry_attrs.get('ipatokennotafter', None)): +raise ValidationError(name='not_after', +error='is before not_before!') + # Set the object class and defaults for specific token types entry_attrs['objectclass'] = otptoken.object_class + ['ipatoken' + options['type']] for ttype, tattrs in TOKEN_TYPES.items(): @@ -336,6 +352,26 @@ class otptoken_mod(LDAPUpdate): msg_summary = _('Modified OTP token %(value)s') def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options): +notafter_set = True +notbefore = entry_attrs.get('ipatokennotbefore', None) +notafter = entry_attrs.get('ipatokennotafter', None) +# notbefore xor notafter, exactly one of them is not None +if bool(notbefore) ^ bool(notafter): +result = self.api.Command.otptoken_find(ipatokenuniqueid= +entry_attrs.get('ipatokenuniqueid', None))['result'] +if result: +if notbefore is None: +notbefore = result[0]['ipatokennotbefore'][0] +if notafter is None: +notafter_set = False +notafter = result[0]['ipatokennotafter'][0] +if not _check_interval(notbefore, notafter): +if notafter_set: +raise ValidationError(name='not_after', +error='is before not_before!') +else: +raise ValidationError(name='not_before', +error='is after not_after!') _normalize_owner(self.api.Object.user, entry_attrs) return dn -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0006 Fix group-remove-member crash when group is removed from a protected group
https://fedorahosted.org/freeipa/ticket/4448 -- David Kupka From 306fd94ae35f153bd7eabf80217219ec25b2189b Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 23 Jul 2014 16:02:17 +0200 Subject: [PATCH] Fix group-remove-member crash when group is removed from a protected group https://fedorahosted.org/freeipa/ticket/4448 --- ipalib/plugins/group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py index af5d4b6bf5217fcda912a92453d15cd0974c1c53..4890bab111c2882ed34cfe28e7384982b9815ac4 100644 --- a/ipalib/plugins/group.py +++ b/ipalib/plugins/group.py @@ -526,7 +526,7 @@ class group_remove_member(LDAPRemoveMember): protected_group_name = keys[0] result = api.Command.group_show(protected_group_name) users_left = set(result['result'].get('member_user', [])) -users_deleted = set(options['user']) +users_deleted = set(options.get('user',[])) if users_left.issubset(users_deleted): raise errors.LastMemberError(key=sorted(users_deleted)[0], label=_(u'group'), container=protected_group_name) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0002 Improve password validity check
On 07/22/2014 08:55 AM, Martin Kosek wrote: On 07/21/2014 04:08 PM, David Kupka wrote: On 07/18/2014 12:52 PM, Martin Kosek wrote: On 07/18/2014 12:33 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/2796 1) Would it be easier/more convenient to just implement following simple check instead of bad_prefix/bad_suffix? if password.strip() != password: raise ValueError('Password must not start or end with whitespace') Yes it would. Edited patch attached. 2) The main goal of the ticket 2796 was not fixed yet. It sometimes happen that when installation crashes somewhere right after pkicreate, it does not record and and does not uninstall the PKI component during ipa-server-install --uninstall. You may artificially invoke some crash in cainstance.py after pkicreate to test it. When fixing it, check how is_configured() in Service object works an how self.backup_state is called in other service modules (like dsinstance.py) where the detection works correctly. You're completely right, Martin. I was unable to reproduce the bug (to force pkicreate/pkispawn to fail) so I thought that it was fixed by the password restriction. Then I discovered that most of the banned characters for password are no longer causing troubles a focused on this. But it's yet another issue. 1) Whitespace error: $ git am /tmp/freeipa-dkupka-0002-2-Improve-password-validity-check.patch Applying: Improve password validity check. /home/mkosek/freeipa/.git/rebase-apply/patch:25: trailing whitespace. # Disallow leading/trailing whaitespaces warning: 1 line adds whitespace errors. Git is highlighting these errors I was probably temporary blind. 2) The new admin validator is not applied to -a command line option and you can pass any garbage to it. You need to replace this section: if options.admin_password is not None and len(options.admin_password) 8: parser.error(Admin user password must be at least 8 characters long) ... with the new validator just like we validate DM password. Added. Martin -- David Kupka From c11f52766646a2ee597009bb14f15c3633c18591 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Thu, 24 Jul 2014 13:32:37 +0200 Subject: [PATCH] Improve password validity check. Allow use of characters that no longer cause troubles. Check for leading and trailing characters in case of 389 Direcory Manager password. --- install/tools/ipa-server-install | 35 +++ 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 671a226d625ab9e8168c569a6d83c35dfae52115..fc9cef06076110a969a3db6640f0288e3de2ce45 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -121,7 +121,31 @@ def validate_dm_password(password): raise ValueError(Password must only contain ASCII characters) # Disallow characters that pkisilent doesn't process properly: -bad_characters = ' \\%' +bad_characters = '\\' +if any(c in bad_characters for c in password): +raise ValueError('Password must not contain these characters: %s' % +', '.join('%s' % c for c in bad_characters)) + +# TODO: Check https://fedorahosted.org/389/ticket/47849 +# Actual behavior of setup-ds.pl is that it does not accept white +# space characters in password when called interactively but does when +# provided such password in INF file. But it ignores leading and trailing +# white spaces in INF file. + +# Disallow leading/trailing whaitespaces +if password.strip() != password: +raise ValueError('Password must not start or end with whitespace.') + +def validate_admin_password(password): +if len(password) 8: +raise ValueError(Password must be at least 8 characters long) +if any(ord(c) 0x20 for c in password): +raise ValueError(Password must not contain control characters) +if any(ord(c) = 0x7F for c in password): +raise ValueError(Password must only contain ASCII characters) + +# Disallow characters that pkisilent doesn't process properly: +bad_characters = '\\' if any(c in bad_characters for c in password): raise ValueError('Password must not contain these characters: %s' % ', '.join('%s' % c for c in bad_characters)) @@ -239,8 +263,11 @@ def parse_options(): validate_dm_password(options.dm_password) except ValueError, e: parser.error(DS admin password: + str(e)) -if options.admin_password is not None and len(options.admin_password) 8: -parser.error(Admin user password must be at least 8 characters long) +if options.admin_password is not None: +try: +validate_admin_password(options.admin_password) +except ValueError, e: +parser.error(Admin user password: + str(e)) if options.domain_name is not None: try: @@ -450,7 +477,7
[Freeipa-devel] [PATCH] 0007 test group: remove group from protected group
Simple test scenario from ticket #4448. Last test will fail until patch freeipa-dkupka-0006 gets accepted. -- David Kupka From 240f48865ebb93a9a4d71250f3bcef1c48c453bb Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Thu, 24 Jul 2014 14:45:50 +0200 Subject: [PATCH] test group: remove group from protected group. Related to https://fedorahosted.org/freeipa/ticket/4448 --- ipatests/test_xmlrpc/test_group_plugin.py | 63 +++ 1 file changed, 63 insertions(+) diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py index 71172893beb97d3373b0b9299f2bfa7bb642dba6..cc526277cfeca1a5122a704d72afb88b7d6250d7 100644 --- a/ipatests/test_xmlrpc/test_group_plugin.py +++ b/ipatests/test_xmlrpc/test_group_plugin.py @@ -31,6 +31,7 @@ from ipatests.test_xmlrpc.test_user_plugin import get_user_result group1 = u'testgroup1' group2 = u'testgroup2' group3 = u'testgroup3' +group4 = u'testgroup4' renamedgroup1 = u'testgroup' user1 = u'tuser1' @@ -50,6 +51,7 @@ class test_group(Declarative): ('group_del', [group1], {}), ('group_del', [group2], {}), ('group_del', [group3], {}), +('group_del', [group4], {}), ('group_del', [renamedgroup1], {}), ('user_del', [user1], {}), ] @@ -1010,4 +1012,65 @@ class test_group(Declarative): ), ), +# Test scenario from ticket #4448 +# https://fedorahosted.org/freeipa/ticket/4448 +dict( +desc='Add group %s' % group4, +command=('group_add', [group4], dict(description=u'Test desc 4')), +expected=dict( +value=group4, +summary=u'Added group %s' % group4, +result=dict( +cn=[group4], +description=[u'Test desc 4'], +objectclass=objectclasses.posixgroup, +gidnumber=[fuzzy_digits], +ipauniqueid=[fuzzy_uuid], +dn=get_group_dn(group4), +), +), +), + +dict( +desc='Add %s group to admins group' % group4, +command=('group_add_member', [u'admins'], dict(group=group4)), +expected=dict( +completed=1, +failed=dict( +member=dict( +group=tuple(), +user=tuple(), +), +), +result=dict( +dn=get_group_dn('admins'), +member_user=[u'admin'], +member_group=[group4], +gidnumber=[fuzzy_digits], +cn=[u'admins'], +description=[u'Account administrators group'], +), +), +), + +dict( +desc='Remove %s group from admins group' % group4, +command=('group_remove_member', [u'admins'], dict(group=group4)), +expected=dict( +completed=1, +failed=dict( +member=dict( +group=tuple(), +user=tuple(), +), +), +result=dict( +dn=get_group_dn(u'admins'), +cn=[u'admins'], +gidnumber=[fuzzy_digits], +member_user=[u'admin'], +description=[u'Account administrators group'], +), +), +), ] -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0007 test group: remove group from protected group
On 07/28/2014 06:41 PM, Petr Viktorin wrote: On 07/24/2014 03:11 PM, David Kupka wrote: Simple test scenario from ticket #4448. Last test will fail until patch freeipa-dkupka-0006 gets accepted. Thanks! These look fine, but since the new tests don't require that the rest of `test_group` is run first, I encourage you to put them in a separate class. Put to separate class, as suggested. Looks better now. This would ensure we don't add new inderdependencies between old and new tests in the future, making future test refactoring more straightforward. Also, you can select to run just a single test class from a module, so testing a targeted fix is faster. (And you can reuse group1, since the other test cleans it up) See test_permission_plugin for an example. The test still fails on current master but works fine with patch freeipa-dkupka-0006-2. -- David Kupka From 9e3c1bc8a83fc466a6e160842ab37c8e65e66229 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 29 Jul 2014 08:40:36 +0200 Subject: [PATCH] test group: remove group from protected group. Related to https://fedorahosted.org/freeipa/ticket/4448 --- ipatests/test_xmlrpc/test_group_plugin.py | 67 +++ 1 file changed, 67 insertions(+) diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py index 71172893beb97d3373b0b9299f2bfa7bb642dba6..26d71c4fae1a312709f6f27e0397ef3315cba33f 100644 --- a/ipatests/test_xmlrpc/test_group_plugin.py +++ b/ipatests/test_xmlrpc/test_group_plugin.py @@ -1009,5 +1009,72 @@ class test_group(Declarative): value=[user1], ), ), +] + +class test_group_remove_group_from_protected_group(Declarative): +cleanup_commands = [ +('group_del', [group1], {}), +] +tests = [ +# Test scenario from ticket #4448 +# https://fedorahosted.org/freeipa/ticket/4448 +dict( +desc='Add group %s' % group1, +command=('group_add', [group1], dict(description=u'Test desc 1')), +expected=dict( +value=group1, +summary=u'Added group %s' % group1, +result=dict( +cn=[group1], +description=[u'Test desc 1'], +objectclass=objectclasses.posixgroup, +gidnumber=[fuzzy_digits], +ipauniqueid=[fuzzy_uuid], +dn=get_group_dn(group1), +), +), +), + +dict( +desc='Add %s group to admins group' % group1, +command=('group_add_member', [u'admins'], dict(group=group1)), +expected=dict( +completed=1, +failed=dict( +member=dict( +group=tuple(), +user=tuple(), +), +), +result=dict( +dn=get_group_dn('admins'), +member_user=[u'admin'], +member_group=[group1], +gidnumber=[fuzzy_digits], +cn=[u'admins'], +description=[u'Account administrators group'], +), +), +), +dict( +desc='Remove %s group from admins group' % group1, +command=('group_remove_member', [u'admins'], dict(group=group1)), +expected=dict( +completed=1, +failed=dict( +member=dict( +group=tuple(), +user=tuple(), +), +), +result=dict( +dn=get_group_dn(u'admins'), +cn=[u'admins'], +gidnumber=[fuzzy_digits], +member_user=[u'admin'], +description=[u'Account administrators group'], +), +), +), ] -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid
On 07/29/2014 01:21 PM, Jan Cholasta wrote: Dne 24.7.2014 v 10:00 David Kupka napsal(a): On 07/23/2014 05:07 PM, Jan Cholasta wrote: Hi, On 23.7.2014 15:46, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4244 1) Use isinstance(X, Y) instead of type(X) is Y. Thanks for advice, will try to remember. 2) When is type(not_before) is str or type(not_after) is str true? The values coming from command options or LDAP should always be datetime, never str. Actually, it is never true. I don't know why I thought that there is such option. 3) There are some misindentations: +raise ValidationError(name='not_after', +error='is before not_before!') +raise ValidationError(name='not_after', +error='is before not_before!') +raise ValidationError(name='not_before', +error='is after not_after!') 4) We don't do exclamation marks in errors messages. You re right, it's probably better not to shout at customer :) 5) Generally, when you want to validate command options, you should look into options, not entry_attrs. 6) This is not right: +result = self.api.Command.otptoken_find(ipatokenuniqueid= +entry_attrs.get('ipatokenuniqueid', None))['result'] This is: +result = self.api.Command.otptoken_show(keys[-1])['result'] Both works, but Martin explained me why is otptoken_show better and how it actually works. Honza Few more nitpicks: 1) You can make _check_interval a little bit shorter by removing a redundant if statement: +def _check_interval(not_before, not_after): +if not_before and not_after: +return not_before = not_after +return True Nice :) 2) Please keep the 2 line space between the last global function and first class in the module. It's good to know that there is one less rule to discover. 3) Error messages are for users' eyes, please don't use internal identifiers in them. Done. 4) You don't need to check if the result of otptoken_show is empty, it never is. Removed. Although, needless check can't hurt. -- David Kupka From 64aff33276a2e929806fac296830b6bdf5b6f621 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 29 Jul 2014 13:58:33 +0200 Subject: [PATCH] Verify otptoken timespan is valid When creating or modifying otptoken check that token validity start is not after validity end. https://fedorahosted.org/freeipa/ticket/4244 --- ipalib/plugins/otptoken.py | 31 ++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py index 2880ee660d5dcdb18c504f50d7b72f5b8fb43d48..aea9891c7e2c3662aa22eb32cc2fadbc78687faa 100644 --- a/ipalib/plugins/otptoken.py +++ b/ipalib/plugins/otptoken.py @@ -21,7 +21,7 @@ from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMem from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext from ipalib.plugable import Registry -from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound +from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound, ValidationError from ipalib.request import context from ipalib.frontend import Local @@ -103,6 +103,11 @@ def _normalize_owner(userobj, entry_attrs): if owner is not None: entry_attrs['ipatokenowner'] = userobj.get_dn(owner) +def _check_interval(not_before, not_after): +if not_before and not_after: +return not_before = not_after +return True + @register() class otptoken(LDAPObject): @@ -254,6 +259,11 @@ class otptoken_add(LDAPCreate): entry_attrs['ipatokenuniqueid'] = str(uuid.uuid4()) dn = DN(ipatokenuniqueid=%s % entry_attrs['ipatokenuniqueid'], dn) +if not _check_interval(options.get('ipatokennotbefore', None), + options.get('ipatokennotafter', None)): +raise ValidationError(name='not_after', + error='is before the validity start') + # Set the object class and defaults for specific token types entry_attrs['objectclass'] = otptoken.object_class + ['ipatoken' + options['type']] for ttype, tattrs in TOKEN_TYPES.items(): @@ -336,6 +346,25 @@ class otptoken_mod(LDAPUpdate): msg_summary = _('Modified OTP token %(value)s') def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options): +notafter_set = True +notbefore = options.get('ipatokennotbefore', None) +notafter = options.get('ipatokennotafter', None) +# notbefore xor notafter, exactly one of them is not None +if bool(notbefore) ^ bool(notafter): +result
Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid
On 07/29/2014 03:28 PM, Jan Cholasta wrote: Dne 29.7.2014 v 14:11 David Kupka napsal(a): On 07/29/2014 01:21 PM, Jan Cholasta wrote: Dne 24.7.2014 v 10:00 David Kupka napsal(a): On 07/23/2014 05:07 PM, Jan Cholasta wrote: Hi, On 23.7.2014 15:46, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4244 1) Use isinstance(X, Y) instead of type(X) is Y. Thanks for advice, will try to remember. 2) When is type(not_before) is str or type(not_after) is str true? The values coming from command options or LDAP should always be datetime, never str. Actually, it is never true. I don't know why I thought that there is such option. 3) There are some misindentations: +raise ValidationError(name='not_after', +error='is before not_before!') +raise ValidationError(name='not_after', +error='is before not_before!') +raise ValidationError(name='not_before', +error='is after not_after!') 4) We don't do exclamation marks in errors messages. You re right, it's probably better not to shout at customer :) 5) Generally, when you want to validate command options, you should look into options, not entry_attrs. 6) This is not right: +result = self.api.Command.otptoken_find(ipatokenuniqueid= +entry_attrs.get('ipatokenuniqueid', None))['result'] This is: +result = self.api.Command.otptoken_show(keys[-1])['result'] Both works, but Martin explained me why is otptoken_show better and how it actually works. Honza Few more nitpicks: 1) You can make _check_interval a little bit shorter by removing a redundant if statement: +def _check_interval(not_before, not_after): +if not_before and not_after: +return not_before = not_after +return True Nice :) 2) Please keep the 2 line space between the last global function and first class in the module. It's good to know that there is one less rule to discover. 3) Error messages are for users' eyes, please don't use internal identifiers in them. Done. 4) You don't need to check if the result of otptoken_show is empty, it never is. Removed. Although, needless check can't hurt. One more thing, sorry I didn't notice it earlier: You must check if 'ipatokennotbefore' and 'ipatokennotafter' keys are in opttoken-show result before using them, otherwise you might end up with: ipa: ERROR: non-public: KeyError: 'ipatokennotbefore' Traceback (most recent call last): File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py, line 348, in wsgi_execute result = self.Command[name](*args, **options) File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 439, in __call__ ret = self.run(*args, **options) File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 754, in run return self.execute(*args, **options) File /usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py, line 1384, in execute *keys, **options) File /usr/lib/python2.7/site-packages/ipalib/plugins/otptoken.py, line 356, in pre_callback notbefore = result['ipatokennotbefore'][0] KeyError: 'ipatokennotbefore' Sorry for sending fifth wersion of the patch. I must test my patches more carefully. -- David Kupka From 0458522dff5be7cb8b7718d379d5cfb3e32c7b33 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 29 Jul 2014 15:45:21 +0200 Subject: [PATCH] Verify otptoken timespan is valid When creating or modifying otptoken check that token validity start is not after validity end. https://fedorahosted.org/freeipa/ticket/4244 --- ipalib/plugins/otptoken.py | 31 ++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py index 2880ee660d5dcdb18c504f50d7b72f5b8fb43d48..dfd010e7f8663b424d9c536907fcc93229181fa3 100644 --- a/ipalib/plugins/otptoken.py +++ b/ipalib/plugins/otptoken.py @@ -21,7 +21,7 @@ from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMem from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext from ipalib.plugable import Registry -from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound +from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound, ValidationError from ipalib.request import context from ipalib.frontend import Local @@ -103,6 +103,11 @@ def _normalize_owner(userobj, entry_attrs): if owner is not None: entry_attrs['ipatokenowner'] = userobj.get_dn(owner) +def _check_interval(not_before, not_after): +if not_before and not_after: +return not_before = not_after +return True
Re: [Freeipa-devel] Reasons for not using certmonger DBus API
On 07/23/2014 03:45 PM, Nalin Dahyabhai wrote: On Wed, Jul 23, 2014 at 10:12:39AM +0200, Martin Kosek wrote: Certmonger API looked complete enough to pull this off: https://git.fedorahosted.org/cgit/certmonger.git/tree/doc/api.txt If I am wrong, please tell me. No, it's meant to be complete -- the getcert command only uses the APIs to talk to the daemon, so they provide at least what it needs. Two words of caution: * That file's manually maintained, so it might not completely reflect what's available. The introspection data's generated at runtime, so if you poke the service with an introspection request, or using d-feet, which does so under the covers, you might spot discrepancies. It probably goes without saying, but please report any that you find. * The majority of properties are currently marked read-only, and you currently have to use the 'modify' API request to change them. Mostly this is a result of 'getcert' not having needed anything more than that, and properties having been added after the initial versions, so it's not set in stone. HTH, Nalin In fact it is almost enough complete for us. The only operation I can't find is 'write ca_external_helper'. add_principal_to_cas and remove_principal_from_cas are modifying this entry in ca file. Certmonger provide 'get_location' DBus method that returns value of this entry but I can't find any 'set_location' method, writable property or other way to modify it over DBus. Am I searching wrong? If not I looked in certmonger code and think that I will be able to add the missing functionality. But I'm unsure what is the preferred way, I can think of two: 1. set_location method 2. read-write location/ca_external_helper property -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 -- David Kupka From dbc53150b6fb5c6b436249994147c2204c35eba4 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 15 Aug 2014 16:11:16 +0200 Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- install/tools/ipa-upgradeconfig| 12 +- ipa-client/ipa-install/ipa-client-install | 2 +- ipa-client/ipaclient/ipa_certupdate.py | 5 +- ipapython/certmonger.py| 518 - ipaserver/install/cainstance.py| 10 +- ipaserver/install/certs.py | 28 +- ipaserver/install/ipa_cacert_manage.py | 4 +- ipaserver/install/plugins/ca_renewal_master.py | 4 +- 8 files changed, 271 insertions(+), 312 deletions(-) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index adf6c8d847b931744cbfc4fcd14b6fbea55c26f5..56e3984783d0940f0e4bbfe55c6c1483f9350522 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -693,23 +693,23 @@ def certificate_renewal_update(ca): for request in requests: nss_dir, nickname, ca_name, pre_command, post_command, profile = request criteria = ( -('cert_storage_location', nss_dir, certmonger.NPATH), -('cert_nickname', nickname, None), -('ca_name', ca_name, None), -('template_profile', profile, None), +('cert_storage_location', nss_dir), +('cert_nickname', nickname), +('ca_name', ca_name), +('template_profile', profile), ) request_id = certmonger.get_request_id(criteria) if request_id is None: break -val = certmonger.get_request_value(request_id, 'pre_certsave_command') +val = certmonger.get_request_value(request_id, 'cert-presave-command') if val is not None: val = val.split(' ', 1)[0] val = os.path.basename(val) if pre_command != val: break -val = certmonger.get_request_value(request_id, 'post_certsave_command') +val = certmonger.get_request_value(request_id, 'post-certsave-command') if val is not None: val = val.split(' ', 1)[0] val = os.path.basename(val) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 08fefc86d31392e9abf66ee4f8fff54a88179795..4cbfae077decca079f24385035428ddf0be38926 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -537,7 +537,7 @@ def uninstall(options, env): log_service_error(cmonger.service_name, 'start', e) try: -certmonger.stop_tracking(paths.NSS_DB_DIR, nickname=client_nss_nickname) +certmonger.stop_tracking(paths.NSS_DB_DIR, cert_nickname=client_nss_nickname) except (CalledProcessError, RuntimeError), e: root_logger.error(%s failed to stop tracking certificate: %s, cmonger.service_name, str(e)) diff --git a/ipa-client/ipaclient/ipa_certupdate.py b/ipa-client/ipaclient/ipa_certupdate.py index 4199a293b2652863a2e7835256efd9cb12c8c33a..012551e3c7e5e788aaccacd9294dbb3546d97ccc 100644 --- a/ipa-client/ipaclient/ipa_certupdate.py +++ b/ipa-client/ipaclient/ipa_certupdate.py @@ -123,9 +123,8 @@ class CertUpdate(admintool.AdminTool): dogtag_constants = dogtag.configured_constants() nickname = 'caSigningCert cert-pki-ca' criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) if request_id is not None: diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py index 67370738427998497770561dee55be2de66ec479..035433e59dc7aab95a108f4cdc609a6a661c943d 100644 --- a/ipapython/certmonger.py +++ b/ipapython/certmonger.py @@ -25,72 +25,153 @@ import os import sys import re import time +import dbus from ipapython import ipautil from ipapython import dogtag from ipapython.ipa_log_manager import * from ipaplatform.paths import paths +from ipapython.ipa_log_manager import root_logger +from dbus import DBusException REQUEST_DIR=paths.CERTMONGER_REQUESTS_DIR CA_DIR=paths.CERTMONGER_CAS_DIR -# Normalizer types for critera in get_request_id() -NPATH = 1 - -def find_request_value(filename, directive): - -Return a value from a certmonger request file for the requested directive - -It tries to do this a number of times because sometimes
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 08/19/2014 09:58 AM, Martin Kosek wrote: On 08/19/2014 09:05 AM, David Kupka wrote: FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 Thanks for this effort, the updated certmonger module looks much better! This will help us get rid of the non-standard communication with certmonger. Just couple initial comments from me by reading the code: 1) Testing needs fixed version of certmonger, right? This needs to be spelled out right with the patch. Yes, certmonger 0.75.13 and above should be fine according ticket https://fedorahosted.org/certmonger/ticket/36. Added to patch description. 2) Description text in patches is cheap, do not be afraid to use it and describe what you did and why. Link to the ticket is missing in the description as well: Ok, increased verbosity a bit :-) Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- 3) get_request_id API: criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) Do we want to continue using the criteria object or should we rather switch to normal function options? I.e. rather using request_id = certmonger.get_request_id(cert_nickname=nickname, cert_storage_location=dogtag_constants.ALIAS_DIR) ? It would look more consistent with other calls. I am just asking, not insisting. I've no preference here. It seems to be a very small change. Has anyone a reason to do it one way and not the other? 3) Starting function: +try: +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True) +except Exception, e: +root_logger.error('Failed to start certmonger: %s' % e) +raise e I see 2 issues related to this code: a) Do not call SYSTEMCTL directly. To be platform independent, rather use services.knownservices.messagebus.start() that is overridable by someone else porting to non-systemd platforms. Is there anything that can't be done using ipalib/ipapython/ipaplatform? b) In this case, do not use raise e, but just raise to keep the exception stack trace intact for better debugging. Every day there's something new to learn about python or FreeIPA. Both a) and b) should be fixed in other occasions and places. I found only one occurence of a) issue. Is there some hidden or are you talking about the whole FreeIPA project? 4) Feel free to add yourself to Authors section of this module. You refactored it greatly to earn it :-) Done. Martin -- David Kupka From cd026268d557904bd8fb3ae9642d88eb5c43ac45 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 19 Aug 2014 16:34:59 +0200 Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. FreeIPA certmonger module changed to use D-Bus to communicate with certmonger. Using the D-Bus API should be more stable and supported way of using cermonger than tampering with its files. =certmonger-0.75.13 is needed for this to work. https://fedorahosted.org/freeipa/ticket/4280 --- install/tools/ipa-upgradeconfig| 15 +- ipa-client/ipa-install/ipa-client-install | 2 +- ipa-client/ipaclient/ipa_certupdate.py | 5 +- ipapython/certmonger.py| 522 - ipaserver/install/cainstance.py| 10 +- ipaserver/install/certs.py | 28 +- ipaserver/install/ipa_cacert_manage.py | 4 +- ipaserver/install/plugins/ca_renewal_master.py | 4 +- 8 files changed, 274 insertions(+), 316 deletions(-) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index adf6c8d847b931744cbfc4fcd14b6fbea55c26f5..e39523afe384c93a2f0915c36e577fcb7d4a448a 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -692,24 +692,23 @@ def certificate_renewal_update(ca): # State not set, lets see if we are already configured for request in requests: nss_dir, nickname, ca_name, pre_command, post_command, profile = request -criteria = ( -('cert_storage_location', nss_dir, certmonger.NPATH), -('cert_nickname', nickname, None), -('ca_name', ca_name, None), -('template_profile', profile, None), +criteria = dict( +('cert-database', nss_dir), +('cert-nickname', nickname), +('template-profile', profile), ) -request_id = certmonger.get_request_id(criteria) +request_id = certmonger.get_request_id(**criteria) if request_id is None
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 08/19/2014 05:44 PM, Rob Crittenden wrote: David Kupka wrote: On 08/19/2014 09:58 AM, Martin Kosek wrote: On 08/19/2014 09:05 AM, David Kupka wrote: FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 Thanks for this effort, the updated certmonger module looks much better! This will help us get rid of the non-standard communication with certmonger. Just couple initial comments from me by reading the code: 1) Testing needs fixed version of certmonger, right? This needs to be spelled out right with the patch. Yes, certmonger 0.75.13 and above should be fine according ticket https://fedorahosted.org/certmonger/ticket/36. Added to patch description. You should update the spec to set the minimum version as well. Sure, thanks. 2) Description text in patches is cheap, do not be afraid to use it and describe what you did and why. Link to the ticket is missing in the description as well: Ok, increased verbosity a bit :-) Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- 3) get_request_id API: criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) Do we want to continue using the criteria object or should we rather switch to normal function options? I.e. rather using request_id = certmonger.get_request_id(cert_nickname=nickname, cert_storage_location=dogtag_constants.ALIAS_DIR) ? It would look more consistent with other calls. I am just asking, not insisting. I've no preference here. It seems to be a very small change. Has anyone a reason to do it one way and not the other? I think I used this criteria thing to avoid having a bazillion optional parameters and for future-proofing. I think at this point the list is probably pretty stable, so I'd base it on whether you care about having a whole ton of optional parameters or not (it has the advantage of self-documenting itself). The list is probably stable but also really excessive. I don't think it would help to have more than dozen optional parameters. So I prefer to leave as-is and change it in future if it is wanted. 3) Starting function: +try: +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True) +except Exception, e: +root_logger.error('Failed to start certmonger: %s' % e) +raise e I see 2 issues related to this code: a) Do not call SYSTEMCTL directly. To be platform independent, rather use services.knownservices.messagebus.start() that is overridable by someone else porting to non-systemd platforms. Is there anything that can't be done using ipalib/ipapython/ipaplatform? It can't make coffee (yet). b) In this case, do not use raise e, but just raise to keep the exception stack trace intact for better debugging. Every day there's something new to learn about python or FreeIPA. Both a) and b) should be fixed in other occasions and places. I found only one occurence of a) issue. Is there some hidden or are you talking about the whole FreeIPA project? 4) Feel free to add yourself to Authors section of this module. You refactored it greatly to earn it :-) Done. You already import dbus, why also separately import DBusException? Removed, thanks for noticing. rob -- David Kupka From b81786e68fba8efd4bb0c3e86a4702084137e30c Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 20 Aug 2014 13:58:50 +0200 Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. FreeIPA certmonger module changed to use D-Bus to communicate with certmonger. Using the D-Bus API should be more stable and supported way of using cermonger than tampering with its files. =certmonger-0.75.13 is needed for this to work. https://fedorahosted.org/freeipa/ticket/4280 --- freeipa.spec.in| 2 +- install/tools/ipa-upgradeconfig| 13 +- ipa-client/ipa-install/ipa-client-install | 2 +- ipa-client/ipaclient/ipa_certupdate.py | 5 +- ipapython/certmonger.py| 521 - ipaserver/install/cainstance.py| 10 +- ipaserver/install/certs.py | 28 +- ipaserver/install/ipa_cacert_manage.py | 4 +- ipaserver/install/plugins/ca_renewal_master.py | 4 +- 9 files changed, 273 insertions(+), 316 deletions(-) diff --git a/freeipa.spec.in b/freeipa.spec.in index 6f22bc92f76f2e4bd732a995e392d6845dab27b7..15aab5b45c5688f11e0125299c2842f23d986749 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in
[Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
https://fedorahosted.org/freeipa/ticket/3575 Also should fix https://bugzilla.redhat.com/show_bug.cgi?id=1128380 as installation is no longer interrupted when multiple IPs are resolved. But it does not add the option to change the IP address during second run. -- David Kupka From 5785d3ed205141aee0989751cab573fae84c53b3 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 25 Aug 2014 16:30:49 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-server-install | 61 --- ipaserver/install/bindinstance.py | 46 +++-- ipaserver/install/installutils.py | 86 +++ 3 files changed, 113 insertions(+), 80 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 6e77b434a018faec36a2808626c99a54bd493908..5c05a0c6e49a3471b6547a9a627324ba78080211 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -38,6 +38,7 @@ import nss.error import base64 import pwd import textwrap +import re from optparse import OptionGroup, OptionValueError try: @@ -176,7 +177,8 @@ def parse_options(): on their first login) basic_group.add_option(--hostname, dest=host_name, help=fully qualified name of server) basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, +# TODO: remove this workaround (type=ip) when #4506 is done + type=str, help=Master Server IP Address) basic_group.add_option(-N, --no-ntp, dest=conf_ntp, action=store_false, help=do not configure ntp, default=True) @@ -348,6 +350,25 @@ def parse_options(): parser.error(idmax (%u) cannot be smaller than idstart (%u) % (options.idmax, options.idstart)) +# TODO: remove this workaround (parsing reverse zones) when #4506 is done +if options.reverse_zone: +reverse_zone = [] +for rz in re.split(r'[\s,]+', options.reverse_zone): +if rz and rz not in reverse_zone: +reverse_zone.append(rz) +options.reverse_zone = reverse_zone + +# TODO: remove this workaround (parsing IPs) when #4506 is done +if options.ip_address: +ip_address = [] +for ip in re.split(r'[\s,]+', options.ip_address): +if ip and ip not in ip_address: +try: +ip_address.append(CheckedIPAddress(ip, match_local=True)) +except ValueError, e: +parser.error(Invalid IP address: %s % e) +options.ip_address = ip_address + #Automatically disable pkinit w/ dogtag until that is supported options.setup_pkinit = False @@ -832,11 +853,11 @@ def main(): realm_name = host_name = domain_name = -ip_address = +ip_address = [] master_password = dm_password = admin_password = -reverse_zone = None +reverse_zone = [] if not options.setup_dns and not options.unattended: if ipautil.user_input(Do you want to configure integrated DNS (BIND)?, False): @@ -895,11 +916,14 @@ def main(): domain_name = domain_name.lower() -ip = get_server_ip_address(host_name, fstore, options.unattended, options) -ip_address = str(ip) +ip_address = get_server_ip_address(host_name, fstore, options.unattended, options) -if options.reverse_zone and not bindinstance.verify_reverse_zone(options.reverse_zone, ip): -sys.exit(1) +for ip in map(str, ip_address): +for rev_zone in reverse_zone: +if bindinstance.verify_reverse_zone(rev_zone, ip): +break +else: +sys.exit(1) if not options.realm_name: realm_name = read_realm_name(domain_name, options.unattended) @@ -972,16 +996,23 @@ def main(): dns_forwarders = read_dns_forwarders() if options.reverse_zone: -reverse_zone = bindinstance.normalize_zone(options.reverse_zone) +for rz in options.reverse_zone: +reverse_zone.append(bindinstance.normalize_zone(rz)) elif not options.no_reverse: if options.unattended: -reverse_zone = util.get_reverse_zone_default(ip) +for ip in map(str, ip_address): +rz = util.get_reverse_zone_default(ip) +if not rz in reverse_zone: +reverse_zone.append(rz) elif bindinstance.create_reverse
[Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
https://fedorahosted.org/freeipa/ticket/4481 -- David Kupka From 0bb344026c4b46d726c6b5f3f52ffb0390295feb Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 26 Aug 2014 12:40:13 +0200 Subject: [PATCH] Add 'host' setting into default.conf configuration file. The 'host' setting is preferred over deprecated 'server' setting. Even that 'server' setting is deprecated keeping it should not hurt and may help to maintain backward compatibility. https://fedorahosted.org/freeipa/ticket/4481 --- ipa-client/ipa-install/ipa-client-install | 1 + 1 file changed, 1 insertion(+) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 08fefc86d31392e9abf66ee4f8fff54a88179795..acdebef6c305ca857422cd0fc1be67ff3f9c4e94 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -818,6 +818,7 @@ def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server): {'name':'realm', 'type':'option', 'value':cli_realm}, {'name':'domain', 'type':'option', 'value':cli_domain}, {'name':'server', 'type':'option', 'value':cli_server[0]}, + {'name':'host', 'type':'option', 'value':cli_server[0]}, {'name':'xmlrpc_uri', 'type':'option', 'value':'https://%s/ipa/xml' % ipautil.format_netloc(cli_server[0])}, {'name':'enable_ra', 'type':'option', 'value':'True'}] -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
On 08/26/2014 03:08 PM, Jan Cholasta wrote: Hi, Dne 26.8.2014 v 13:01 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4481 Doing this will break ipa-client-automount and ipa-certupdate, because they assume that api.env.host contains the hostname of the local system (which is the default value). It looked suspiciously simple so I could expect that there is some catch. There is obviously some confusion about what the option should represent (documentation says server hostname, code does client hostname), IMO we should resolve that first. Ok, are there any suggestions? What is the desired state? Honza -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0011 Allow user to force Kerberos realm during installation
Usually it isn't wise to allow something like this. But in environment with broken DNS (described in ticket) there is probably not many alternatives. https://fedorahosted.org/freeipa/ticket/ -- David Kupka From 6cfa293bffc03610bfc0391a96f0b95021f34c4e Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 12:31:09 +0200 Subject: [PATCH] Allow user to force Kerberos realm during installation. User can set realm not matching one resolved from DNS. This is useful especially when DNS is missconfigured. https://fedorahosted.org/freeipa/ticket/ --- ipa-client/ipa-install/ipa-client-install | 2 +- ipa-client/ipaclient/ipadiscovery.py | 42 --- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 08fefc86d31392e9abf66ee4f8fff54a88179795..4eb3b3b8dcf5e31f08e9895b33ca0419eaf2195a 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -2126,7 +2126,7 @@ def install(options, env, fstore, statestore): # Create the discovery instance ds = ipadiscovery.IPADiscovery() -ret = ds.search(domain=options.domain, servers=options.server, hostname=hostname, ca_cert_path=get_cert_path(options.ca_cert_file)) +ret = ds.search(domain=options.domain, servers=options.server, realm=options.realm_name, hostname=hostname, ca_cert_path=get_cert_path(options.ca_cert_file)) if options.server and ret != 0: # There is no point to continue with installation as server list was diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py index 0532f618e81d215c4416f62f81af2add48c7dc8e..589ca7ca856c288f68e2152489db2d43e075afd9 100644 --- a/ipa-client/ipaclient/ipadiscovery.py +++ b/ipa-client/ipaclient/ipadiscovery.py @@ -139,7 +139,7 @@ class IPADiscovery(object): domain = domain[p+1:] return (None, None) -def search(self, domain = , servers = , hostname=None, ca_cert_path=None): +def search(self, domain = , servers = , realm=None, hostname=None, ca_cert_path=None): Use DNS discovery to identify valid IPA servers. @@ -148,6 +148,7 @@ class IPADiscovery(object): Returns a constant representing the overall search result. +root_logger.debug(realm provided: %s % realm) root_logger.debug([IPA Discovery]) root_logger.debug( 'Starting IPA discovery with domain=%s, servers=%s, hostname=%s', @@ -218,13 +219,22 @@ class IPADiscovery(object): #search for kerberos root_logger.debug([Kerberos realm search]) -krb_realm, kdc = self.ipadnssearchkrb(self.domain) -if not servers and not krb_realm: +root_logger.debug(realm provided: %s % realm) +if realm: +root_logger.debug(Kerberos realm forced) +self.realm = realm +self.realm_source = 'Forced' +else: +realm = self.ipadnssearchkrbrealm() +self.realm = realm +self.realm_source = ( +'Discovered Kerberos DNS records from %s' % self.domain) + +if not servers and not realm: return REALM_NOT_FOUND -self.realm = krb_realm -self.kdc = kdc -self.realm_source = self.kdc_source = ( +self.kdc = self.ipadnssearchkrbkdc() +self.kdc_source = ( 'Discovered Kerberos DNS records from %s' % self.domain) # We may have received multiple servers corresponding to the domain @@ -452,11 +462,12 @@ class IPADiscovery(object): return servers -def ipadnssearchkrb(self, tdomain): +def ipadnssearchkrbrealm(self, domain=None): realm = None -kdc = None +if not domain: +domain = self.domain # now, check for a Kerberos realm the local host or domain is in -qname = _kerberos. + tdomain +qname = _kerberos. + domain root_logger.debug(Search DNS for TXT record of %s, qname) @@ -472,10 +483,13 @@ class IPADiscovery(object): realm = answer.strings[0] if realm: break +return realm -if realm: -# now fetch server information for the realm -domain = realm.lower() +def ipadnssearchkrbkdc(self, domain=None): +kdc = None + +if not domain: +domain = self.domain kdc = self.ipadns_search_srv(domain, '_kerberos._udp', 88, break_on_first=False) @@ -483,7 +497,7 @@ class IPADiscovery(object): if kdc: kdc = ','.join(kdc) else: -root_logger.debug(SRV record for KDC not found! Realm: %s, SRV record: %s % (realm, qname)) +root_logger.debug(SRV record for KDC not found! Domain: %s % domain
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
Patch modified according to jcholast's personally-delivered feedback: 1) use action='append' instead of that ugly parsing 2) do not use map(), FreeIPA doesn't like it On 08/25/2014 05:04 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/3575 Also should fix https://bugzilla.redhat.com/show_bug.cgi?id=1128380 as installation is no longer interrupted when multiple IPs are resolved. But it does not add the option to change the IP address during second run. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka From 13ae855492261afd3e7e8545be2cc06d17431b17 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 13:50:21 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-server-install | 43 ipaserver/install/bindinstance.py | 46 +++-- ipaserver/install/installutils.py | 86 +++ 3 files changed, 94 insertions(+), 81 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 6e77b434a018faec36a2808626c99a54bd493908..dde7731e5d991f3329efe8232fcd1bce434e280d 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -176,7 +176,7 @@ def parse_options(): on their first login) basic_group.add_option(--hostname, dest=host_name, help=fully qualified name of server) basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, + type=ip, ip_local=True, action=append, default=[], help=Master Server IP Address) basic_group.add_option(-N, --no-ntp, dest=conf_ntp, action=store_false, help=do not configure ntp, default=True) @@ -236,7 +236,8 @@ def parse_options(): type=ip, help=Add a DNS forwarder) dns_group.add_option(--no-forwarders, dest=no_forwarders, action=store_true, default=False, help=Do not add any DNS forwarders, use root servers instead) -dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use) +dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use, + action=append, default=[]) dns_group.add_option(--no-reverse, dest=no_reverse, action=store_true, default=False, help=Do not create reverse DNS zone) dns_group.add_option(--zonemgr, action=callback, callback=bindinstance.zonemgr_callback, @@ -832,11 +833,11 @@ def main(): realm_name = host_name = domain_name = -ip_address = +ip_address = [] master_password = dm_password = admin_password = -reverse_zone = None +reverse_zone = [] if not options.setup_dns and not options.unattended: if ipautil.user_input(Do you want to configure integrated DNS (BIND)?, False): @@ -895,11 +896,14 @@ def main(): domain_name = domain_name.lower() -ip = get_server_ip_address(host_name, fstore, options.unattended, options) -ip_address = str(ip) +ip_address = get_server_ip_address(host_name, fstore, options.unattended, options) -if options.reverse_zone and not bindinstance.verify_reverse_zone(options.reverse_zone, ip): -sys.exit(1) +for ip in ip_address: +for rev_zone in reverse_zone: +if bindinstance.verify_reverse_zone(rev_zone, str(ip)): +break +else: +sys.exit(1) if not options.realm_name: realm_name = read_realm_name(domain_name, options.unattended) @@ -972,16 +976,23 @@ def main(): dns_forwarders = read_dns_forwarders() if options.reverse_zone: -reverse_zone = bindinstance.normalize_zone(options.reverse_zone) +for rz in options.reverse_zone: +reverse_zone.append(bindinstance.normalize_zone(rz)) elif not options.no_reverse: if options.unattended: -reverse_zone = util.get_reverse_zone_default(ip) +for ip in ip_address: +rz = util.get_reverse_zone_default(str(ip)) +if not rz in reverse_zone: +reverse_zone.append(rz) elif bindinstance.create_reverse(): -reverse_zone = util.get_reverse_zone_default(ip) -reverse_zone = bindinstance.read_reverse_zone(reverse_zone, ip
Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
On 08/27/2014 11:22 AM, Jan Cholasta wrote: Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a): David Kupka wrote: On 08/26/2014 03:08 PM, Jan Cholasta wrote: Hi, Dne 26.8.2014 v 13:01 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4481 Doing this will break ipa-client-automount and ipa-certupdate, because they assume that api.env.host contains the hostname of the local system (which is the default value). It looked suspiciously simple so I could expect that there is some catch. There is obviously some confusion about what the option should represent (documentation says server hostname, code does client hostname), IMO we should resolve that first. Ok, are there any suggestions? What is the desired state? AIUI the server option is deprecated because it wasn't being used, not that it needed to be replaced. I believe that in most cases the server name is pulled from the xmlrpc_uri. Yes, that's what the ticket says: https://fedorahosted.org/freeipa/ticket/3071. Ok, adding 'host' entry with local host name. host has always meant the local host name. I think the man page is wrong. +1 Fixing the line in man page. rob -- David Kupka From 5c5dd23ad6ef32ead2505783ba8b12bd3e6b0366 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 16:02:35 +0200 Subject: [PATCH] Add 'host' setting into default.conf configuration file on client. Fix description in man page. 'host' setting specifies local hostname not the hostname of IPA server. https://fedorahosted.org/freeipa/ticket/4481 --- ipa-client/ipa-install/ipa-client-install | 5 +++-- ipa-client/man/default.conf.5 | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 08fefc86d31392e9abf66ee4f8fff54a88179795..13afc3e8bd72b42021f40b19e2b236952d9bf382 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -805,7 +805,7 @@ def uninstall(options, env): return rv -def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server): +def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname): ipaconf = ipaclient.ipachangeconf.IPAChangeConf(IPA Installer) ipaconf.setOptionAssignment( = ) ipaconf.setSectionNameDelimiters(([,])) @@ -818,6 +818,7 @@ def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server): {'name':'realm', 'type':'option', 'value':cli_realm}, {'name':'domain', 'type':'option', 'value':cli_domain}, {'name':'server', 'type':'option', 'value':cli_server[0]}, + {'name':'host', 'type':'option', 'value':hostname}, {'name':'xmlrpc_uri', 'type':'option', 'value':'https://%s/ipa/xml' % ipautil.format_netloc(cli_server[0])}, {'name':'enable_ra', 'type':'option', 'value':'True'}] @@ -2529,7 +2530,7 @@ def install(options, env, fstore, statestore): # Configure ipa.conf if not options.on_master: -configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server) +configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname) root_logger.info(Created /etc/ipa/default.conf) api.bootstrap(context='cli_installer', debug=options.debug) diff --git a/ipa-client/man/default.conf.5 b/ipa-client/man/default.conf.5 index c1ccf109e874907885fc3b51b63507c2b46b64ab..f3f62434f3c38384d0f93245592b4b91d4cc1815 100644 --- a/ipa-client/man/default.conf.5 +++ b/ipa-client/man/default.conf.5 @@ -96,7 +96,7 @@ Specifies whether the CA is acting as an RA agent, such as when dogtag is being Specifies whether an IPA client should attempt to fall back and try other services if the first connection fails. .TP .B host hostname -Specifies the hostname of the IPA server. This value is used to construct URL values on the client and server. +Specifies the local system hostname. .TP .B in_server boolean Specifies whether requests should be forwarded to an IPA server or handled locally. This is used internally by IPA in a similar way as context. The same IPA framework is used by the ipa command\-line tool and the server. This setting tells the framework whether it should execute the command as if on the server or forward it via XML\-RPC to a remote server. -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Hope, I've addressed all the issues (except 9 and 11, inline). Let's go for another round :-) On 08/27/2014 11:05 AM, Jan Cholasta wrote: Hi, Dne 25.8.2014 v 15:39 David Kupka napsal(a): On 08/19/2014 05:44 PM, Rob Crittenden wrote: David Kupka wrote: On 08/19/2014 09:58 AM, Martin Kosek wrote: On 08/19/2014 09:05 AM, David Kupka wrote: FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 Thanks for this effort, the updated certmonger module looks much better! This will help us get rid of the non-standard communication with certmonger. Just couple initial comments from me by reading the code: 1) Testing needs fixed version of certmonger, right? This needs to be spelled out right with the patch. Yes, certmonger 0.75.13 and above should be fine according ticket https://fedorahosted.org/certmonger/ticket/36. Added to patch description. You should update the spec to set the minimum version as well. Sure, thanks. 2) Description text in patches is cheap, do not be afraid to use it and describe what you did and why. Link to the ticket is missing in the description as well: Ok, increased verbosity a bit :-) Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- 3) get_request_id API: criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) Do we want to continue using the criteria object or should we rather switch to normal function options? I.e. rather using request_id = certmonger.get_request_id(cert_nickname=nickname, cert_storage_location=dogtag_constants.ALIAS_DIR) ? It would look more consistent with other calls. I am just asking, not insisting. I've no preference here. It seems to be a very small change. Has anyone a reason to do it one way and not the other? I think I used this criteria thing to avoid having a bazillion optional parameters and for future-proofing. I think at this point the list is probably pretty stable, so I'd base it on whether you care about having a whole ton of optional parameters or not (it has the advantage of self-documenting itself). The list is probably stable but also really excessive. I don't think it would help to have more than dozen optional parameters. So I prefer to leave as-is and change it in future if it is wanted. 3) Starting function: +try: +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True) +except Exception, e: +root_logger.error('Failed to start certmonger: %s' % e) +raise e I see 2 issues related to this code: a) Do not call SYSTEMCTL directly. To be platform independent, rather use services.knownservices.messagebus.start() that is overridable by someone else porting to non-systemd platforms. Is there anything that can't be done using ipalib/ipapython/ipaplatform? It can't make coffee (yet). b) In this case, do not use raise e, but just raise to keep the exception stack trace intact for better debugging. Every day there's something new to learn about python or FreeIPA. Both a) and b) should be fixed in other occasions and places. I found only one occurence of a) issue. Is there some hidden or are you talking about the whole FreeIPA project? 4) Feel free to add yourself to Authors section of this module. You refactored it greatly to earn it :-) Done. You already import dbus, why also separately import DBusException? Removed, thanks for noticing. rob 1) The patch needs to be rebased. 2) Please try to follow PEP8, at least in certmonger.py. 3) In certificate_renewal_update() in ipa-upgradeconfig you removed ca_name from criteria. 4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and stop_tracking() is unnecessary. We can keep calling request nicknames request_id and certificate nicknames nickname in our APIs. 5) I think it would be better to add a docstring to _cm_dbus_object.__init__() instead of doing this: # object is accesible over this DBus bus instance bus = None # DBus object path path = None # the actual DBus object obj = None # object interface name obj_dbus_if = None # object parent interface name parent_dbus_if = None # object inteface obj_if = None # property interface prop_if = None 6) In _start_certmonger(), please check if certmonger is already running before starting it, what applies to systemd might not apply to other init systems. 7) Do we ever need to connect to certmonger on the session bus? If not, there is no need to support
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
Forget to add str() conversion to some places when removing map(). Now it should be working again. On 08/27/2014 02:24 PM, David Kupka wrote: Patch modified according to jcholast's personally-delivered feedback: 1) use action='append' instead of that ugly parsing 2) do not use map(), FreeIPA doesn't like it On 08/25/2014 05:04 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/3575 Also should fix https://bugzilla.redhat.com/show_bug.cgi?id=1128380 as installation is no longer interrupted when multiple IPs are resolved. But it does not add the option to change the IP address during second run. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka From 8eaea5ada941ac813e22efa076b6989d2dbf6be6 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 13:50:21 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-server-install | 43 ipaserver/install/bindinstance.py | 46 +++-- ipaserver/install/installutils.py | 86 +++ 3 files changed, 94 insertions(+), 81 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 6e77b434a018faec36a2808626c99a54bd493908..dde7731e5d991f3329efe8232fcd1bce434e280d 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -176,7 +176,7 @@ def parse_options(): on their first login) basic_group.add_option(--hostname, dest=host_name, help=fully qualified name of server) basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, + type=ip, ip_local=True, action=append, default=[], help=Master Server IP Address) basic_group.add_option(-N, --no-ntp, dest=conf_ntp, action=store_false, help=do not configure ntp, default=True) @@ -236,7 +236,8 @@ def parse_options(): type=ip, help=Add a DNS forwarder) dns_group.add_option(--no-forwarders, dest=no_forwarders, action=store_true, default=False, help=Do not add any DNS forwarders, use root servers instead) -dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use) +dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use, + action=append, default=[]) dns_group.add_option(--no-reverse, dest=no_reverse, action=store_true, default=False, help=Do not create reverse DNS zone) dns_group.add_option(--zonemgr, action=callback, callback=bindinstance.zonemgr_callback, @@ -832,11 +833,11 @@ def main(): realm_name = host_name = domain_name = -ip_address = +ip_address = [] master_password = dm_password = admin_password = -reverse_zone = None +reverse_zone = [] if not options.setup_dns and not options.unattended: if ipautil.user_input(Do you want to configure integrated DNS (BIND)?, False): @@ -895,11 +896,14 @@ def main(): domain_name = domain_name.lower() -ip = get_server_ip_address(host_name, fstore, options.unattended, options) -ip_address = str(ip) +ip_address = get_server_ip_address(host_name, fstore, options.unattended, options) -if options.reverse_zone and not bindinstance.verify_reverse_zone(options.reverse_zone, ip): -sys.exit(1) +for ip in ip_address: +for rev_zone in reverse_zone: +if bindinstance.verify_reverse_zone(rev_zone, str(ip)): +break +else: +sys.exit(1) if not options.realm_name: realm_name = read_realm_name(domain_name, options.unattended) @@ -972,16 +976,23 @@ def main(): dns_forwarders = read_dns_forwarders() if options.reverse_zone: -reverse_zone = bindinstance.normalize_zone(options.reverse_zone) +for rz in options.reverse_zone: +reverse_zone.append(bindinstance.normalize_zone(rz)) elif not options.no_reverse: if options.unattended: -reverse_zone = util.get_reverse_zone_default(ip) +for ip in ip_address: +rz
[Freeipa-devel] [PATCH] 0012 Add record(s) to /etc/host when IPA is configured as DNS server.
This patch depends on freeipa-dkupka-0009 as it modifies the same part of code. https://fedorahosted.org/freeipa/ticket/4220 -- David Kupka From 549e682809d9e0ccc6debe6676f22b3f9d1755f4 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 2 Sep 2014 10:49:26 +0200 Subject: [PATCH] Add record(s) to /etc/host when IPA is configured as DNS server. This is to avoid chicken-egg problem when directory server fails to start without resolvable hostname and named fails to provide hostname without directory server. https://fedorahosted.org/freeipa/ticket/4220 --- ipaserver/install/installutils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py index 5661de6b6b918a61a8de3ed16c25d4b7debd212d..293caffb5dc6e9219a90f4ec33abd3e13086e09f 100644 --- a/ipaserver/install/installutils.py +++ b/ipaserver/install/installutils.py @@ -487,7 +487,7 @@ def get_server_ip_address(host_name, fstore, unattended, options): hosts_record = record_in_hosts(str(ip_address)) if hosts_record is None: -if ip_add_to_hosts: +if ip_add_to_hosts or options.setup_dns: print Adding [+str(ip_address)+ +host_name+] to your /etc/hosts file fstore.backup_file(paths.HOSTS) add_record_to_hosts(str(ip_address), host_name) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0012 Add record(s) to /etc/host when IPA is configured as DNS server.
Ok, the patch no longer depends on 0009. The reason is that 0012 is going to ipa-4.0 and 0009 to ipa-4.1. On 09/02/2014 12:13 PM, David Kupka wrote: This patch depends on freeipa-dkupka-0009 as it modifies the same part of code. https://fedorahosted.org/freeipa/ticket/4220 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka From 7dd0c42caa61378f43d69fa8f996ae2561a3005c Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 2 Sep 2014 16:32:30 +0200 Subject: [PATCH] Add record(s) to /etc/host when IPA is configured as DNS server. This is to avoid chicken-egg problem when directory server fails to start without resolvable hostname and named fails to provide hostname without directory server. https://fedorahosted.org/freeipa/ticket/4220 --- ipaserver/install/installutils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py index dc98d7a51aa743c87b2d7667246b6f029b8a648b..3b9138fef6ca0d907a8dc11d70d7732bc84836e6 100644 --- a/ipaserver/install/installutils.py +++ b/ipaserver/install/installutils.py @@ -489,7 +489,7 @@ def get_server_ip_address(host_name, fstore, unattended, options): hosts_record = record_in_hosts(ip_address) if hosts_record is None: -if ip_add_to_hosts: +if ip_add_to_hosts or options.setup_dns: print Adding [+ip_address+ +host_name+] to your /etc/hosts file fstore.backup_file(paths.HOSTS) add_record_to_hosts(ip_address, host_name) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
The patch now depends on freeipa-dkupka-0012 as both modifies the same part of code. On 09/02/2014 10:29 AM, David Kupka wrote: Forget to add str() conversion to some places when removing map(). Now it should be working again. On 08/27/2014 02:24 PM, David Kupka wrote: Patch modified according to jcholast's personally-delivered feedback: 1) use action='append' instead of that ugly parsing 2) do not use map(), FreeIPA doesn't like it On 08/25/2014 05:04 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/3575 Also should fix https://bugzilla.redhat.com/show_bug.cgi?id=1128380 as installation is no longer interrupted when multiple IPs are resolved. But it does not add the option to change the IP address during second run. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka From 8eaea5ada941ac813e22efa076b6989d2dbf6be6 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 13:50:21 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-server-install | 43 ipaserver/install/bindinstance.py | 46 +++-- ipaserver/install/installutils.py | 86 +++ 3 files changed, 94 insertions(+), 81 deletions(-) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 6e77b434a018faec36a2808626c99a54bd493908..dde7731e5d991f3329efe8232fcd1bce434e280d 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -176,7 +176,7 @@ def parse_options(): on their first login) basic_group.add_option(--hostname, dest=host_name, help=fully qualified name of server) basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, + type=ip, ip_local=True, action=append, default=[], help=Master Server IP Address) basic_group.add_option(-N, --no-ntp, dest=conf_ntp, action=store_false, help=do not configure ntp, default=True) @@ -236,7 +236,8 @@ def parse_options(): type=ip, help=Add a DNS forwarder) dns_group.add_option(--no-forwarders, dest=no_forwarders, action=store_true, default=False, help=Do not add any DNS forwarders, use root servers instead) -dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use) +dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use, + action=append, default=[]) dns_group.add_option(--no-reverse, dest=no_reverse, action=store_true, default=False, help=Do not create reverse DNS zone) dns_group.add_option(--zonemgr, action=callback, callback=bindinstance.zonemgr_callback, @@ -832,11 +833,11 @@ def main(): realm_name = host_name = domain_name = -ip_address = +ip_address = [] master_password = dm_password = admin_password = -reverse_zone = None +reverse_zone = [] if not options.setup_dns and not options.unattended: if ipautil.user_input(Do you want to configure integrated DNS (BIND)?, False): @@ -895,11 +896,14 @@ def main(): domain_name = domain_name.lower() -ip = get_server_ip_address(host_name, fstore, options.unattended, options) -ip_address = str(ip) +ip_address = get_server_ip_address(host_name, fstore, options.unattended, options) -if options.reverse_zone and not bindinstance.verify_reverse_zone(options.reverse_zone, ip): -sys.exit(1) +for ip in ip_address: +for rev_zone in reverse_zone: +if bindinstance.verify_reverse_zone(rev_zone, str(ip)): +break +else: +sys.exit(1) if not options.realm_name: realm_name = read_realm_name(domain_name, options.unattended) @@ -972,16 +976,23 @@ def main(): dns_forwarders = read_dns_forwarders() if options.reverse_zone: -reverse_zone = bindinstance.normalize_zone(options.reverse_zone) +for rz in options.reverse_zone
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/02/2014 01:56 PM, Jan Cholasta wrote: Dne 29.8.2014 v 14:34 David Kupka napsal(a): Hope, I've addressed all the issues (except 9 and 11, inline). Let's go for another round :-) On 08/27/2014 11:05 AM, Jan Cholasta wrote: Hi, Dne 25.8.2014 v 15:39 David Kupka napsal(a): On 08/19/2014 05:44 PM, Rob Crittenden wrote: David Kupka wrote: On 08/19/2014 09:58 AM, Martin Kosek wrote: On 08/19/2014 09:05 AM, David Kupka wrote: FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 Thanks for this effort, the updated certmonger module looks much better! This will help us get rid of the non-standard communication with certmonger. Just couple initial comments from me by reading the code: 1) Testing needs fixed version of certmonger, right? This needs to be spelled out right with the patch. Yes, certmonger 0.75.13 and above should be fine according ticket https://fedorahosted.org/certmonger/ticket/36. Added to patch description. You should update the spec to set the minimum version as well. Sure, thanks. 2) Description text in patches is cheap, do not be afraid to use it and describe what you did and why. Link to the ticket is missing in the description as well: Ok, increased verbosity a bit :-) Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- 3) get_request_id API: criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) Do we want to continue using the criteria object or should we rather switch to normal function options? I.e. rather using request_id = certmonger.get_request_id(cert_nickname=nickname, cert_storage_location=dogtag_constants.ALIAS_DIR) ? It would look more consistent with other calls. I am just asking, not insisting. I've no preference here. It seems to be a very small change. Has anyone a reason to do it one way and not the other? I think I used this criteria thing to avoid having a bazillion optional parameters and for future-proofing. I think at this point the list is probably pretty stable, so I'd base it on whether you care about having a whole ton of optional parameters or not (it has the advantage of self-documenting itself). The list is probably stable but also really excessive. I don't think it would help to have more than dozen optional parameters. So I prefer to leave as-is and change it in future if it is wanted. 3) Starting function: +try: +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True) +except Exception, e: +root_logger.error('Failed to start certmonger: %s' % e) +raise e I see 2 issues related to this code: a) Do not call SYSTEMCTL directly. To be platform independent, rather use services.knownservices.messagebus.start() that is overridable by someone else porting to non-systemd platforms. Is there anything that can't be done using ipalib/ipapython/ipaplatform? It can't make coffee (yet). b) In this case, do not use raise e, but just raise to keep the exception stack trace intact for better debugging. Every day there's something new to learn about python or FreeIPA. Both a) and b) should be fixed in other occasions and places. I found only one occurence of a) issue. Is there some hidden or are you talking about the whole FreeIPA project? 4) Feel free to add yourself to Authors section of this module. You refactored it greatly to earn it :-) Done. You already import dbus, why also separately import DBusException? Removed, thanks for noticing. rob 1) The patch needs to be rebased. I didn't notice the patch is targeted for 4.0. Can you please provide patches for both ipa-4-0 and ipa-4-1/master? Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0. 2) Please try to follow PEP8, at least in certmonger.py. 3) In certificate_renewal_update() in ipa-upgradeconfig you removed ca_name from criteria. 4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and stop_tracking() is unnecessary. We can keep calling request nicknames request_id and certificate nicknames nickname in our APIs. 5) I think it would be better to add a docstring to _cm_dbus_object.__init__() instead of doing this: # object is accesible over this DBus bus instance bus = None # DBus object path path = None # the actual DBus object obj = None # object interface name obj_dbus_if = None # object parent interface name parent_dbus_if = None # object inteface obj_if = None # property
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/03/2014 04:05 PM, Jan Cholasta wrote: Dne 3.9.2014 v 12:37 David Kupka napsal(a): On 09/02/2014 01:56 PM, Jan Cholasta wrote: Dne 29.8.2014 v 14:34 David Kupka napsal(a): Hope, I've addressed all the issues (except 9 and 11, inline). Let's go for another round :-) On 08/27/2014 11:05 AM, Jan Cholasta wrote: Hi, Dne 25.8.2014 v 15:39 David Kupka napsal(a): On 08/19/2014 05:44 PM, Rob Crittenden wrote: David Kupka wrote: On 08/19/2014 09:58 AM, Martin Kosek wrote: On 08/19/2014 09:05 AM, David Kupka wrote: FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 Thanks for this effort, the updated certmonger module looks much better! This will help us get rid of the non-standard communication with certmonger. Just couple initial comments from me by reading the code: 1) Testing needs fixed version of certmonger, right? This needs to be spelled out right with the patch. Yes, certmonger 0.75.13 and above should be fine according ticket https://fedorahosted.org/certmonger/ticket/36. Added to patch description. You should update the spec to set the minimum version as well. Sure, thanks. 2) Description text in patches is cheap, do not be afraid to use it and describe what you did and why. Link to the ticket is missing in the description as well: Ok, increased verbosity a bit :-) Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- 3) get_request_id API: criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) Do we want to continue using the criteria object or should we rather switch to normal function options? I.e. rather using request_id = certmonger.get_request_id(cert_nickname=nickname, cert_storage_location=dogtag_constants.ALIAS_DIR) ? It would look more consistent with other calls. I am just asking, not insisting. I've no preference here. It seems to be a very small change. Has anyone a reason to do it one way and not the other? I think I used this criteria thing to avoid having a bazillion optional parameters and for future-proofing. I think at this point the list is probably pretty stable, so I'd base it on whether you care about having a whole ton of optional parameters or not (it has the advantage of self-documenting itself). The list is probably stable but also really excessive. I don't think it would help to have more than dozen optional parameters. So I prefer to leave as-is and change it in future if it is wanted. 3) Starting function: +try: +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True) +except Exception, e: +root_logger.error('Failed to start certmonger: %s' % e) +raise e I see 2 issues related to this code: a) Do not call SYSTEMCTL directly. To be platform independent, rather use services.knownservices.messagebus.start() that is overridable by someone else porting to non-systemd platforms. Is there anything that can't be done using ipalib/ipapython/ipaplatform? It can't make coffee (yet). b) In this case, do not use raise e, but just raise to keep the exception stack trace intact for better debugging. Every day there's something new to learn about python or FreeIPA. Both a) and b) should be fixed in other occasions and places. I found only one occurence of a) issue. Is there some hidden or are you talking about the whole FreeIPA project? 4) Feel free to add yourself to Authors section of this module. You refactored it greatly to earn it :-) Done. You already import dbus, why also separately import DBusException? Removed, thanks for noticing. rob 1) The patch needs to be rebased. I didn't notice the patch is targeted for 4.0. Can you please provide patches for both ipa-4-0 and ipa-4-1/master? Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0. There is a little bug in ipa-upgradeconfig in the 4.0 version of the patch. This is wrong: for request in requests: nss_dir, nickname, ca_name, pre_command, post_command, profile = request criteria = { 'cert-database': nss_dir, 'cert-nickname': nickname, 'ca-name': ca_name, 'template-profile': profile, } It should be: for nss_dir, nickname, ca_name, pre_command, post_command in requests: criteria = { 'cert-database': nss_dir, 'cert-nickname': nickname, 'ca-name': ca_name, } 2) Please try to follow PEP8, at least in certmonger.py. 3
Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
On 09/03/2014 04:45 PM, Jan Cholasta wrote: Dne 3.9.2014 v 16:25 David Kupka napsal(a): On 09/03/2014 04:05 PM, Jan Cholasta wrote: Dne 3.9.2014 v 12:37 David Kupka napsal(a): On 09/02/2014 01:56 PM, Jan Cholasta wrote: Dne 29.8.2014 v 14:34 David Kupka napsal(a): Hope, I've addressed all the issues (except 9 and 11, inline). Let's go for another round :-) On 08/27/2014 11:05 AM, Jan Cholasta wrote: Hi, Dne 25.8.2014 v 15:39 David Kupka napsal(a): On 08/19/2014 05:44 PM, Rob Crittenden wrote: David Kupka wrote: On 08/19/2014 09:58 AM, Martin Kosek wrote: On 08/19/2014 09:05 AM, David Kupka wrote: FreeIPA will use certmonger D-Bus API as discussed in this thread https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html This change should prevent hard-to-reproduce bugs like https://fedorahosted.org/freeipa/ticket/4280 Thanks for this effort, the updated certmonger module looks much better! This will help us get rid of the non-standard communication with certmonger. Just couple initial comments from me by reading the code: 1) Testing needs fixed version of certmonger, right? This needs to be spelled out right with the patch. Yes, certmonger 0.75.13 and above should be fine according ticket https://fedorahosted.org/certmonger/ticket/36. Added to patch description. You should update the spec to set the minimum version as well. Sure, thanks. 2) Description text in patches is cheap, do not be afraid to use it and describe what you did and why. Link to the ticket is missing in the description as well: Ok, increased verbosity a bit :-) Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files. --- 3) get_request_id API: criteria = ( -('cert_storage_location', dogtag_constants.ALIAS_DIR, - certmonger.NPATH), -('cert_nickname', nickname, None), +('cert_storage_location', dogtag_constants.ALIAS_DIR), +('cert_nickname', nickname), ) request_id = certmonger.get_request_id(criteria) Do we want to continue using the criteria object or should we rather switch to normal function options? I.e. rather using request_id = certmonger.get_request_id(cert_nickname=nickname, cert_storage_location=dogtag_constants.ALIAS_DIR) ? It would look more consistent with other calls. I am just asking, not insisting. I've no preference here. It seems to be a very small change. Has anyone a reason to do it one way and not the other? I think I used this criteria thing to avoid having a bazillion optional parameters and for future-proofing. I think at this point the list is probably pretty stable, so I'd base it on whether you care about having a whole ton of optional parameters or not (it has the advantage of self-documenting itself). The list is probably stable but also really excessive. I don't think it would help to have more than dozen optional parameters. So I prefer to leave as-is and change it in future if it is wanted. 3) Starting function: +try: +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True) +except Exception, e: +root_logger.error('Failed to start certmonger: %s' % e) +raise e I see 2 issues related to this code: a) Do not call SYSTEMCTL directly. To be platform independent, rather use services.knownservices.messagebus.start() that is overridable by someone else porting to non-systemd platforms. Is there anything that can't be done using ipalib/ipapython/ipaplatform? It can't make coffee (yet). b) In this case, do not use raise e, but just raise to keep the exception stack trace intact for better debugging. Every day there's something new to learn about python or FreeIPA. Both a) and b) should be fixed in other occasions and places. I found only one occurence of a) issue. Is there some hidden or are you talking about the whole FreeIPA project? 4) Feel free to add yourself to Authors section of this module. You refactored it greatly to earn it :-) Done. You already import dbus, why also separately import DBusException? Removed, thanks for noticing. rob 1) The patch needs to be rebased. I didn't notice the patch is targeted for 4.0. Can you please provide patches for both ipa-4-0 and ipa-4-1/master? Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0. There is a little bug in ipa-upgradeconfig in the 4.0 version of the patch. This is wrong: for request in requests: nss_dir, nickname, ca_name, pre_command, post_command, profile = request criteria = { 'cert-database': nss_dir, 'cert-nickname': nickname, 'ca-name': ca_name, 'template-profile': profile, } It should be: for nss_dir, nickname, ca_name, pre_command, post_command in requests: criteria = { 'cert-database': nss_dir, 'cert-nickname': nickname, 'ca-name
Re: [Freeipa-devel] [PATCH] 0011 Allow user to force Kerberos realm during installation
On 09/03/2014 05:09 PM, Jan Cholasta wrote: Hi, Dne 27.8.2014 v 13:56 David Kupka napsal(a): Usually it isn't wise to allow something like this. But in environment with broken DNS (described in ticket) there is probably not many alternatives. https://fedorahosted.org/freeipa/ticket/ 1) I think you can log realm in search() as part of the Starting IPA discovery ... message instead of a separate message. 2) Also, no need to log the realm twice in search(). I forget to remove some redundant debug prints. 3) It looks like you forgot to un-indent some code in ipadnssearchkrbkdc(). Fixed, thanks. Honza -- David Kupka From 0f86ce45975933311f327a29d8d26dc60b4b4d73 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 12:31:09 +0200 Subject: [PATCH] Allow user to force Kerberos realm during installation. User can set realm not matching one resolved from DNS. This is useful especially when DNS is missconfigured. https://fedorahosted.org/freeipa/ticket/ --- ipa-client/ipa-install/ipa-client-install | 2 +- ipa-client/ipaclient/ipadiscovery.py | 42 --- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 08fefc86d31392e9abf66ee4f8fff54a88179795..4eb3b3b8dcf5e31f08e9895b33ca0419eaf2195a 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -2126,7 +2126,7 @@ def install(options, env, fstore, statestore): # Create the discovery instance ds = ipadiscovery.IPADiscovery() -ret = ds.search(domain=options.domain, servers=options.server, hostname=hostname, ca_cert_path=get_cert_path(options.ca_cert_file)) +ret = ds.search(domain=options.domain, servers=options.server, realm=options.realm_name, hostname=hostname, ca_cert_path=get_cert_path(options.ca_cert_file)) if options.server and ret != 0: # There is no point to continue with installation as server list was diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py index 0532f618e81d215c4416f62f81af2add48c7dc8e..919b26695c13ad9b216c27f293f1207bf94bdff1 100644 --- a/ipa-client/ipaclient/ipadiscovery.py +++ b/ipa-client/ipaclient/ipadiscovery.py @@ -139,7 +139,7 @@ class IPADiscovery(object): domain = domain[p+1:] return (None, None) -def search(self, domain = , servers = , hostname=None, ca_cert_path=None): +def search(self, domain=, servers=, realm=None, hostname=None, ca_cert_path=None): Use DNS discovery to identify valid IPA servers. @@ -218,13 +218,21 @@ class IPADiscovery(object): #search for kerberos root_logger.debug([Kerberos realm search]) -krb_realm, kdc = self.ipadnssearchkrb(self.domain) -if not servers and not krb_realm: +if realm: +root_logger.debug(Kerberos realm forced) +self.realm = realm +self.realm_source = 'Forced' +else: +realm = self.ipadnssearchkrbrealm() +self.realm = realm +self.realm_source = ( +'Discovered Kerberos DNS records from %s' % self.domain) + +if not servers and not realm: return REALM_NOT_FOUND -self.realm = krb_realm -self.kdc = kdc -self.realm_source = self.kdc_source = ( +self.kdc = self.ipadnssearchkrbkdc() +self.kdc_source = ( 'Discovered Kerberos DNS records from %s' % self.domain) # We may have received multiple servers corresponding to the domain @@ -452,11 +460,12 @@ class IPADiscovery(object): return servers -def ipadnssearchkrb(self, tdomain): +def ipadnssearchkrbrealm(self, domain=None): realm = None -kdc = None +if not domain: +domain = self.domain # now, check for a Kerberos realm the local host or domain is in -qname = _kerberos. + tdomain +qname = _kerberos. + domain root_logger.debug(Search DNS for TXT record of %s, qname) @@ -472,18 +481,21 @@ class IPADiscovery(object): realm = answer.strings[0] if realm: break +return realm -if realm: -# now fetch server information for the realm -domain = realm.lower() +def ipadnssearchkrbkdc(self, domain=None): +kdc = None + +if not domain: +domain = self.domain kdc = self.ipadns_search_srv(domain, '_kerberos._udp', 88, -break_on_first=False) + break_on_first=False) if kdc: kdc = ','.join(kdc) else: -root_logger.debug(SRV record for KDC not found! Realm: %s, SRV record: %s % (realm, qname)) +root_logger.debug(SRV record
Re: [Freeipa-devel] [PATCH] 0011 Allow user to force Kerberos realm during installation
On 09/04/2014 01:22 PM, Jan Cholasta wrote: Dne 4.9.2014 v 12:42 David Kupka napsal(a): On 09/03/2014 05:09 PM, Jan Cholasta wrote: Hi, Dne 27.8.2014 v 13:56 David Kupka napsal(a): Usually it isn't wise to allow something like this. But in environment with broken DNS (described in ticket) there is probably not many alternatives. https://fedorahosted.org/freeipa/ticket/ 1) I think you can log realm in search() as part of the Starting IPA discovery ... message instead of a separate message. 2) Also, no need to log the realm twice in search(). I forget to remove some redundant debug prints. 3) It looks like you forgot to un-indent some code in ipadnssearchkrbkdc(). Fixed, thanks. What I meant is that this: def ipadnssearchkrbkdc(self, domain=None): kdc = None if not domain: domain = self.domain kdc = self.ipadns_search_srv(domain, '_kerberos._udp', 88, break_on_first=False) if kdc: kdc = ','.join(kdc) else: root_logger.debug(SRV record for KDC not found! Domain: %s % domain) kdc = None return kdc should be this: def ipadnssearchkrbkdc(self, domain=None): if not domain: domain = self.domain kdc = self.ipadns_search_srv(domain, '_kerberos._udp', 88, break_on_first=False) if kdc: kdc = ','.join(kdc) else: root_logger.debug(SRV record for KDC not found! Domain: %s % domain) kdc = None return kdc Isn't that right? Oh, you're right, again :) Thanks. Honza -- David Kupka From e3dfea228328da6d520180515426095ce0985c47 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 12:31:09 +0200 Subject: [PATCH] Allow user to force Kerberos realm during installation. User can set realm not matching one resolved from DNS. This is useful especially when DNS is missconfigured. https://fedorahosted.org/freeipa/ticket/ --- ipa-client/ipa-install/ipa-client-install | 2 +- ipa-client/ipaclient/ipadiscovery.py | 52 +++ 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 08fefc86d31392e9abf66ee4f8fff54a88179795..4eb3b3b8dcf5e31f08e9895b33ca0419eaf2195a 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -2126,7 +2126,7 @@ def install(options, env, fstore, statestore): # Create the discovery instance ds = ipadiscovery.IPADiscovery() -ret = ds.search(domain=options.domain, servers=options.server, hostname=hostname, ca_cert_path=get_cert_path(options.ca_cert_file)) +ret = ds.search(domain=options.domain, servers=options.server, realm=options.realm_name, hostname=hostname, ca_cert_path=get_cert_path(options.ca_cert_file)) if options.server and ret != 0: # There is no point to continue with installation as server list was diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py index 0532f618e81d215c4416f62f81af2add48c7dc8e..0d574825aa493a8d565afe30077b74aec03924a3 100644 --- a/ipa-client/ipaclient/ipadiscovery.py +++ b/ipa-client/ipaclient/ipadiscovery.py @@ -139,7 +139,7 @@ class IPADiscovery(object): domain = domain[p+1:] return (None, None) -def search(self, domain = , servers = , hostname=None, ca_cert_path=None): +def search(self, domain=, servers=, realm=None, hostname=None, ca_cert_path=None): Use DNS discovery to identify valid IPA servers. @@ -218,13 +218,21 @@ class IPADiscovery(object): #search for kerberos root_logger.debug([Kerberos realm search]) -krb_realm, kdc = self.ipadnssearchkrb(self.domain) -if not servers and not krb_realm: +if realm: +root_logger.debug(Kerberos realm forced) +self.realm = realm +self.realm_source = 'Forced' +else: +realm = self.ipadnssearchkrbrealm() +self.realm = realm +self.realm_source = ( +'Discovered Kerberos DNS records from %s' % self.domain) + +if not servers and not realm: return REALM_NOT_FOUND -self.realm = krb_realm -self.kdc = kdc -self.realm_source = self.kdc_source = ( +self.kdc = self.ipadnssearchkrbkdc() +self.kdc_source = ( 'Discovered Kerberos DNS records from %s' % self.domain) # We may have received multiple servers corresponding to the domain @@ -452,11 +460,12 @@ class IPADiscovery(object): return servers -def ipadnssearchkrb(self, tdomain): +def ipadnssearchkrbrealm(self, domain=None): realm = None -kdc = None
[Freeipa-devel] [PATCH] Do not restart apache server when not necessary.
https://fedorahosted.org/freeipa/ticket/4352 -- David Kupka From 9f081c8f1cab3f0d7cb0d55054ae7ad8f1ed8a10 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 5 Sep 2014 09:55:23 +0200 Subject: [PATCH] Do not restart apache server when not necessary. https://fedorahosted.org/freeipa/ticket/4352 --- install/tools/ipa-replica-install | 1 - 1 file changed, 1 deletion(-) diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install index 5bfd61ee69d4682823a57f4b99a0d9a054a56d22..621127558a525a75a36fbbd3d97bc9084642869e 100755 --- a/install/tools/ipa-replica-install +++ b/install/tools/ipa-replica-install @@ -699,7 +699,6 @@ def main(): CA.configure_certmonger_renewal() CA.import_ra_cert(dir + /ra.p12) CA.fix_ra_perms() -services.knownservices.httpd.restart() # The DS instance is created before the keytab, add the SSL cert we # generated -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0014 Fix typo causing ipa-upgradeconfig to fail
https://fedorahosted.org/freeipa/ticket/4529 -- David Kupka From cd0201fb906bcd7cd67c230a642e245f1b3f60a9 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 10 Sep 2014 11:57:46 +0200 Subject: [PATCH] Fix typo causing ipa-upgradeconfig to fail. Replace 'post-certsave-command' by 'cert-postsave-command'. https://fedorahosted.org/freeipa/ticket/4529 --- install/tools/ipa-upgradeconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index 5dbf3087b24c5420741e92fd7364d2165ae77bad..3914eb59066b515d33bebc19ca5afb4f50548bb2 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -713,7 +713,7 @@ def certificate_renewal_update(ca): if pre_command != val: break -val = certmonger.get_request_value(request_id, 'post-certsave-command') +val = certmonger.get_request_value(request_id, 'cert-postsave-command') if val is not None: val = val.split(' ', 1)[0] val = os.path.basename(val) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/08/2014 05:56 PM, Martin Basti wrote: On 02/09/14 16:55, David Kupka wrote: The patch now depends on freeipa-dkupka-0012 as both modifies the same part of code. freeipa-dkupka-0012 is now accepted and merged upstream so there is no need to take this dependency into account. On 09/02/2014 10:29 AM, David Kupka wrote: Forget to add str() conversion to some places when removing map(). Now it should be working again. On 08/27/2014 02:24 PM, David Kupka wrote: Patch modified according to jcholast's personally-delivered feedback: 1) use action='append' instead of that ugly parsing 2) do not use map(), FreeIPA doesn't like it On 08/25/2014 05:04 PM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/3575 Also should fix https://bugzilla.redhat.com/show_bug.cgi?id=1128380 as installation is no longer interrupted when multiple IPs are resolved. But it does not add the option to change the IP address during second run. I haven't tested it yet, I only take a look because there may be conflict with 'dns root zone support' refactoring 1) +for ns_ip_address in nameserver_ip_address: +add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, +ns_hostname=api.env.host, ns_ip_address=ns_ip_address, +force=True) Are you sure this will work? Domain name is the same, so no new zone will be created (DuplicateEntry exception is handled inside add_zone function). IMO you should call add_zone only once. Fixed, thanks. BTW: I will change the add_zone function in refactoring , ns_hostname wil be remove, and ns_ip_address will take an p+ipv6 address 2) +resolv_txt = '' +for ip_address in self.ip_address: +resolv_txt += search +self.domain+\nnameserver +str(ip_address)+\n There is multiple search statements. search example.com nameserver 192.168.1.1 search example.com nameserver 2001:db8::1 ... and also there si a limit of namesevers which can be in resolv.conf, but I dont know if we care, statements over limit should be just ignored. http://linux.die.net/man/5/resolv.conf Since now, only localhost addresses (::1, 127.0.0.1) are placed inside this file when DNS server is part of the installation. 3) self.ip_address is confusing for me, I'm expecting only one address. Could it be ip_addresses or ip_address_list? Ask the framework gurus :-) Changed to plural as there are other variables named this way. -- David Kupka From 7c5bc742b08403a1a6d878b21dc725a2f71f48da Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 13:50:21 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-replica-install| 69 ++- install/tools/ipa-server-install | 63 - ipaserver/install/bindinstance.py| 74 +++-- ipaserver/install/installutils.py| 94 ipaserver/install/ipa_replica_prepare.py | 66 -- 5 files changed, 207 insertions(+), 159 deletions(-) diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install index 7c9e27e2b9d248653681c85236d3541ec9ab0ec7..6ca78a9c60d61d811ab09eb898966fb55bb3daf6 100755 --- a/install/tools/ipa-replica-install +++ b/install/tools/ipa-replica-install @@ -67,8 +67,8 @@ def parse_options(): default=False, help=configure a dogtag CA) basic_group.add_option(--setup-kra, dest=setup_kra, action=store_true, default=False, help=configure a dogtag KRA) -basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, +basic_group.add_option(--ip-address, dest=ip_addresses, + type=ip, ip_local=True, action=append, default=[], help=Replica server IP Address) basic_group.add_option(-p, --password, dest=password, sensitive=True, help=Directory Manager (existing master) password) @@ -112,7 +112,8 @@ def parse_options(): type=ip, help=Add a DNS forwarder) dns_group.add_option(--no-forwarders, dest=no_forwarders, action=store_true, default=False, help=Do not add any DNS forwarders, use root servers instead) -dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use) +dns_group.add_option(--reverse-zone, dest=reverse_zones, default=[], + action=append, help=The reverse DNS zone to use) dns_group.add_option(--no-reverse, dest=no_reverse, action
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
that is different +print sys.stderr, from the one provided on the command line. Please fix your DNS +print sys.stderr, or /etc/hosts file and restart the installation. +sys.exit(1) Could you write those extra addresses to output? We need to improve usability of our error messages UX is the king :) -- Martin Basti -- David Kupka From 824fa9aea5660c5e2e048409723c02d796a55383 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 13:50:21 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-replica-install| 86 +++- install/tools/ipa-server-install | 85 ++-- ipaserver/install/bindinstance.py| 73 +--- ipaserver/install/installutils.py| 96 ipaserver/install/ipa_replica_prepare.py | 66 -- 5 files changed, 243 insertions(+), 163 deletions(-) diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install index e3b65b0963d92e4d102a1166aa001fe8cb164562..2c711cbf07ea0cb302ce23365e494fd991ef00fc 100755 --- a/install/tools/ipa-replica-install +++ b/install/tools/ipa-replica-install @@ -67,8 +67,8 @@ def parse_options(): default=False, help=configure a dogtag CA) basic_group.add_option(--setup-kra, dest=setup_kra, action=store_true, default=False, help=configure a dogtag KRA) -basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, +basic_group.add_option(--ip-address, dest=ip_addresses, + type=ip, ip_local=True, action=append, default=[], help=Replica server IP Address) basic_group.add_option(-p, --password, dest=password, sensitive=True, help=Directory Manager (existing master) password) @@ -112,7 +112,8 @@ def parse_options(): type=ip, help=Add a DNS forwarder) dns_group.add_option(--no-forwarders, dest=no_forwarders, action=store_true, default=False, help=Do not add any DNS forwarders, use root servers instead) -dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use) +dns_group.add_option(--reverse-zone, dest=reverse_zones, default=[], + action=append, help=The reverse DNS zone to use) dns_group.add_option(--no-reverse, dest=no_reverse, action=store_true, default=False, help=Do not create new reverse DNS zone) dns_group.add_option(--no-host-dns, dest=no_host_dns, action=store_true, @@ -133,7 +134,7 @@ def parse_options(): parser.error(You cannot specify a --forwarder option without the --setup-dns option) if options.no_forwarders: parser.error(You cannot specify a --no-forwarders option without the --setup-dns option) -if options.reverse_zone: +if options.reverse_zones: parser.error(You cannot specify a --reverse-zone option without the --setup-dns option) if options.no_reverse: parser.error(You cannot specify a --no-reverse option without the --setup-dns option) @@ -141,7 +142,7 @@ def parse_options(): parser.error(You cannot specify a --forwarder option together with --no-forwarders) elif not options.forwarders and not options.no_forwarders: parser.error(You must specify at least one --forwarder option or --no-forwarders option) -elif options.reverse_zone and options.no_reverse: +elif options.reverse_zones and options.no_reverse: parser.error(You cannot specify a --reverse-zone option together with --no-reverse) return safe_options, options, args[0] @@ -264,22 +265,53 @@ def install_bind(config, options): forwarders = () bind = bindinstance.BindInstance(dm_password=config.dirman_password) -if options.reverse_zone: -if not bindinstance.verify_reverse_zone(options.reverse_zone, config.ip): -sys.exit(1) -reverse_zone = bindinstance.normalize_zone(options.reverse_zone) -else: -reverse_zone = bindinstance.find_reverse_zone(config.ip) -if reverse_zone is None and not options.no_reverse: -reverse_zone = util.get_reverse_zone_default(config.ip) -if not options.unattended and bindinstance.create_reverse(): -reverse_zone = bindinstance.read_reverse_zone(reverse_zone, config.ip) +reverse_zones = [] -if reverse_zone is not None
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/16/2014 06:09 PM, Martin Basti wrote: On 16/09/14 15:59, David Kupka wrote: On 09/12/2014 07:24 PM, Martin Basti wrote: snip / Be careful, reviewed on friday! :-) 1) whitespace error + pep8 error patch:76: trailing whitespace. # there is reverse zone for every ip address warning: 1 line adds whitespace errors. ./ipaserver/install/bindinstance.py:640:9: E265 block comment should start with '# ' Stupid mistake, sorry. 2) (server install) +for ip in ip_addresses: +for rev_zone in reverse_zones: +if bindinstance.verify_reverse_zone(rev_zone, str(ip)): +break +else: +sys.exit(1) Please add there error message instead 1 to exit function You are right, it's better to say what's wrong. 3) (server install) Code above, bindinstance.verify_reverse_zone(rev_zone, str(ip)): IMHO this will cause errors (not tested) try: rev-zones: [10.10.10.in-addr.arpa., 16.172.in-addr.arpa.] ip_addreses: [10.10.10.1, 172.16.0.1] it should be any() of zone can handle ip I indented the else branch more that I wanted. 4) (replica-install) I'm not sure about behavior of combination ip addresses, and reverse zones, In original code, if user specify reverse zone, the ip address must match. In patched version, you just add new zones for ip-addresses which doesn't math the user zones. IMO we should check if all addresses belongs to user specified zones, otherwise raise an error. But I'm open to suggestions. The behavior now basically is: Check if IP address matches any specified zone a. if yes we are good b. else search if there is existing zone that can be used ba. if yes we are good bb. else ask user for zone or create default if --unattended 5) Code for ipa-replica-install, and ipa-server-install, differs in parts where we expecting same behavior - ip addresses and reverse zones The behavoir now should be almost the same. The only difference is that when installing replica we need to search for existing reverse zones as they could be added during on another server. 6) https://engineering.redhat.com/irc-services.txt+# there is at least one ip address in every zone +if options.reverse_zones: +for reverse_zone in options.reverse_zones: +for ip in config.ips: -- A +if bindinstance.verify_reverse_zone(reverse_zone, ip): + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) +break +else: +sys.exit(1) * +# there is reverse zone for every ip address +for ip in config.ips: +for reverse_zone in options.reverse_zones: --- B +if bindinstance.verify_reverse_zone(reverse_zone, ip): +if reverse_zone not in reverse_zones: + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) +break +else: C +reverse_zone = bindinstance.find_reverse_zone(ip) +if reverse_zone is None and not options.no_reverse: +reverse_zone = util.get_reverse_zone_default(ip) - D1 +if not options.unattended and bindinstance.create_reverse(): - D +reverse_zone = bindinstance.read_reverse_zone(reverse_zone, ip) + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) - D2 You added all possible zones in step A, B step is not needed. If user doesn't specify reverse_zones you can just jump to C step *add error message C: elif not options.no_reverse D: you asked user if wants to create zone, but you don't care about answer, and in step D2 append zone from D1 note: --no-reverse and --reverse-zones cant be used together, you can use it in new code, test it before cycle Rewritten. 7) # Always use force=True as named is not set up yet add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, -ns_hostname=api.env.host, ns_ip_address=nameserver_ip_address, -force=True) + ns_hostname=api.env.host, ns_ip_address=None, force=True) +#for ns_ip_address in nameserver_ip_address: +#add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, +#ns_hostname=api.env.host, ns_ip_address=ns_ip_address, +#force=True) Please remove commented section I forget to clean the trash, thanks. You can remove 'ns_ip_address=None' from function 8) +if set(options.ip_addresses) = set(ips): +ips = options.ip_addresses +else
[Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
https://fedorahosted.org/freeipa/ticket/4421 -- David Kupka From 77faaa3c7887550b493f86f90f654da8e1f42eee Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 2 Sep 2014 16:11:55 +0200 Subject: [PATCH] Allow multiple krbprincipalnames. Allow user to specify multiple krbprincipalnames and krbcanonicalname. User must have IT specialist role or Host Administrators privilege assigned. https://fedorahosted.org/freeipa/ticket/4421 --- ACI.txt| 4 +++- API.txt| 2 +- ipalib/plugins/host.py | 21 + 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/ACI.txt b/ACI.txt index 1e6bec0ece554fb2457fae0462c0c673a9b24e41..2a83af6ffba230f2d6ba5ec521652dc2312ce6d0 100644 --- a/ACI.txt +++ b/ACI.txt @@ -85,7 +85,9 @@ aci: (targetattr = businesscategory || cn || createtimestamp || description || dn: cn=computers,cn=accounts,dc=ipa,dc=example aci: (targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add Hosts;allow (add) groupdn = ldap:///cn=System: Add Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example;) dn: cn=computers,cn=accounts,dc=ipa,dc=example -aci: (targetattr = krbprincipalname)(targetfilter = ((!(krbprincipalname=*))(objectclass=ipahost)))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) +aci: (targetattr = krbcanonicalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbCanonicalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbCanonicalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) +dn: cn=computers,cn=accounts,dc=ipa,dc=example +aci: (targetattr = krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) dn: cn=computers,cn=accounts,dc=ipa,dc=example aci: (targetattr = enrolledby || objectclass)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Enroll a Host;allow (write) groupdn = ldap:///cn=System: Enroll a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) dn: cn=computers,cn=accounts,dc=ipa,dc=example diff --git a/API.txt b/API.txt index bbd0f507b2faeec0239920cdcff28fe25d618e02..ef1f70397e7685161821a98a92ea575aa6eff532 100644 --- a/API.txt +++ b/API.txt @@ -1884,7 +1884,7 @@ option: Str('description', attribute=True, autofill=False, cli_name='desc', mult option: Bool('ipakrbokasdelegate', attribute=False, autofill=False, cli_name='ok_as_delegate', multivalue=False, required=False) option: Bool('ipakrbrequirespreauth', attribute=False, autofill=False, cli_name='requires_pre_auth', multivalue=False, required=False) option: Str('ipasshpubkey', attribute=True, autofill=False, cli_name='sshpubkey', csv=True, multivalue=True, required=False) -option: Str('krbprincipalname?', attribute=True, cli_name='principalname') +option: Str('krbprincipalname?', attribute=True, cli_name='principalname', multivalue=True) option: Str('l', attribute=True, autofill=False, cli_name='locality', multivalue=False, required=False) option: Str('macaddress', attribute=True, autofill=False, cli_name='macaddress', csv=True, multivalue=True, pattern='^([a-fA-F0-9]{2}[:|\\-]?){5}[a-fA-F0-9]{2}$', required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py index 570bbe56aa0a315a031051f9a895702ba7c35076..53c2b95b44063049d64e8ad96bd5e52f1e1cab48 100644 --- a/ipalib/plugins/host.py +++ b/ipalib/plugins/host.py @@ -303,16 +303,17 @@ class host(LDAPObject): # This will let it be added if the client ends up enrolling with # an administrator instead. 'ipapermright': {'write'}, -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], 'ipapermdefaultattr': {'krbprincipalname'}, 'replaces': [ '(target = ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX;)(targetfilter = (!(krbprincipalname=*)))(targetattr = krbprincipalname)(version 3.0;acl permission:Add krbPrincipalName to a host; allow (write) groupdn = ldap:///cn=Add krbPrincipalName to a host,cn=permissions,cn=pbac,$SUFFIX;)', ], 'default_privileges': {'Host Administrators', 'Host Enrollment'}, }, +'System: Add krbCanonicalName to a Host': { +'ipapermright': {'write'}, +'ipapermdefaultattr': {'krbcanonicalname'}, +'default_privileges': {'Host Administrators'}, +}, 'System: Enroll a Host': { 'ipapermright': {'write'}, 'ipapermdefaultattr': {'objectclass', 'enrolledby'}, @@ -761,6 +762,7 @@ class host_mod(LDAPUpdate
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/17/2014 07:25 AM, David Kupka wrote: On 09/16/2014 06:09 PM, Martin Basti wrote: On 16/09/14 15:59, David Kupka wrote: On 09/12/2014 07:24 PM, Martin Basti wrote: snip / Be careful, reviewed on friday! :-) 1) whitespace error + pep8 error patch:76: trailing whitespace. # there is reverse zone for every ip address warning: 1 line adds whitespace errors. ./ipaserver/install/bindinstance.py:640:9: E265 block comment should start with '# ' Stupid mistake, sorry. 2) (server install) +for ip in ip_addresses: +for rev_zone in reverse_zones: +if bindinstance.verify_reverse_zone(rev_zone, str(ip)): +break +else: +sys.exit(1) Please add there error message instead 1 to exit function You are right, it's better to say what's wrong. 3) (server install) Code above, bindinstance.verify_reverse_zone(rev_zone, str(ip)): IMHO this will cause errors (not tested) try: rev-zones: [10.10.10.in-addr.arpa., 16.172.in-addr.arpa.] ip_addreses: [10.10.10.1, 172.16.0.1] it should be any() of zone can handle ip I indented the else branch more that I wanted. 4) (replica-install) I'm not sure about behavior of combination ip addresses, and reverse zones, In original code, if user specify reverse zone, the ip address must match. In patched version, you just add new zones for ip-addresses which doesn't math the user zones. IMO we should check if all addresses belongs to user specified zones, otherwise raise an error. But I'm open to suggestions. The behavior now basically is: Check if IP address matches any specified zone a. if yes we are good b. else search if there is existing zone that can be used ba. if yes we are good bb. else ask user for zone or create default if --unattended 5) Code for ipa-replica-install, and ipa-server-install, differs in parts where we expecting same behavior - ip addresses and reverse zones The behavoir now should be almost the same. The only difference is that when installing replica we need to search for existing reverse zones as they could be added during on another server. 6) https://engineering.redhat.com/irc-services.txt+# there is at least one ip address in every zone +if options.reverse_zones: +for reverse_zone in options.reverse_zones: +for ip in config.ips: -- A +if bindinstance.verify_reverse_zone(reverse_zone, ip): + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) +break +else: +sys.exit(1) * +# there is reverse zone for every ip address +for ip in config.ips: +for reverse_zone in options.reverse_zones: --- B +if bindinstance.verify_reverse_zone(reverse_zone, ip): +if reverse_zone not in reverse_zones: + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) +break +else: C +reverse_zone = bindinstance.find_reverse_zone(ip) +if reverse_zone is None and not options.no_reverse: +reverse_zone = util.get_reverse_zone_default(ip) - D1 +if not options.unattended and bindinstance.create_reverse(): - D +reverse_zone = bindinstance.read_reverse_zone(reverse_zone, ip) + reverse_zones.append(bindinstance.normalize_zone(reverse_zone)) - D2 You added all possible zones in step A, B step is not needed. If user doesn't specify reverse_zones you can just jump to C step *add error message C: elif not options.no_reverse D: you asked user if wants to create zone, but you don't care about answer, and in step D2 append zone from D1 note: --no-reverse and --reverse-zones cant be used together, you can use it in new code, test it before cycle Rewritten. 7) # Always use force=True as named is not set up yet add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, -ns_hostname=api.env.host, ns_ip_address=nameserver_ip_address, -force=True) + ns_hostname=api.env.host, ns_ip_address=None, force=True) +#for ns_ip_address in nameserver_ip_address: +#add_zone(self.domain, self.zonemgr, dns_backup=self.dns_backup, +#ns_hostname=api.env.host, ns_ip_address=ns_ip_address, +#force=True) Please remove commented section I forget to clean the trash, thanks. You can remove 'ns_ip_address=None' from function 8) +if set(options.ip_addresses) = set(ips): +ips
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 04:28 PM, Martin Kosek wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? To allow additional krbPrincipalNames. This behavior is requested by the ticket. Martin -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 04:40 PM, Simo Sorce wrote: On Thu, 18 Sep 2014 16:28:19 +0200 Martin Kosek mko...@redhat.com wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? I think also both the code and the tests are missing to ensure that the krbPrincipalName *also* *always* lists the krbCanonicalName. I think with the current code you can end up in a situation where you can have a value in KrbCanonicalName and completely different values in KrbPrincipalName. I didn't realize that there is such requirement although it's logical. I will fix it, thanks. Simo. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0015-16 Allow multiple krbprincipalnames + test
On 09/18/2014 09:42 PM, Martin Kosek wrote: On 09/18/2014 09:11 PM, Simo Sorce wrote: On Thu, 18 Sep 2014 14:57:45 -0400 Rob Crittenden rcrit...@redhat.com wrote: Martin Kosek wrote: On 09/18/2014 04:06 PM, David Kupka wrote: On 09/18/2014 03:44 PM, Rob Crittenden wrote: David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4421 You are removing an ACI in this patch. It is always possible it is no longer needed. Did you test all the client enrollment scenarios? rob As far as I'm aware I'm not removing any ACI. I'm modifying ACI so it is possible to add krbPrincipalName to host even when there is already one (or more). And adding one ACI to allow writing krbCanonicalName to host. But I'm still not really familiar with ACI so please correct me if I'm wrong. What refers to is probably the update in ACI.txt - the ACI alternative to API.txt. David updated an ACI, not removed it. On that note, what is the reason for this permission change: -'ipapermtargetfilter': [ -'(objectclass=ipahost)', -'(!(krbprincipalname=*))', -], ? Yes, this is exactly the change I was referring to. Permission changes within a plugin now translate automatically to ACI changes. Sorry I wasn't clearer. This ACI gets replaced with a much simpler one and I'm not 100% sure it will work with all enrollments: -aci: (targetattr = krbprincipalname)(targetfilter = ((!(krbprincipalname=*))(objectclass=ipahost)))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) +aci: (targetattr = krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Add krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Add krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) The first one restricts writing the attribute only if it isn't already set. The second lets it be changed unconditionally. Yeah this is wrong indeed, the point of the ACI is to allow setting the principal only when it is not already set, which is the OTP enrollment case. But if krbprincipal is set then this specific permission should not grant rights to change it. At least this was my understanding. Simo. Right. It seems to me we should add keep this permission intact and add a new permission allowing adding krbPrincipalName aliases. This would allow writing both krbPrincipalName and krbCanonicalName. Martin Thank you all for explanation and help. I rewrote the patch so it should work as requested now. Also I added tests to reassure the behavior is correct. -- David Kupka From 3e4a4ce3d951bd83ec046025f5acbfc7a0823f6e Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 2 Sep 2014 16:11:55 +0200 Subject: [PATCH] Allow multiple krbprincipalnames. Allow user to specify multiple krbprincipalnames and krbcanonicalname. User must have IT specialist role or Host Administrators privilege assigned. https://fedorahosted.org/freeipa/ticket/4421 --- ACI.txt| 2 ++ API.txt| 2 +- ipalib/plugins/host.py | 50 -- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/ACI.txt b/ACI.txt index 1e6bec0ece554fb2457fae0462c0c673a9b24e41..1ac48c938c86a42082e4eb9a2cbbc42d9eb6fe8d 100644 --- a/ACI.txt +++ b/ACI.txt @@ -98,6 +98,8 @@ dn: cn=computers,cn=accounts,dc=ipa,dc=example aci: (targetattr = ipasshpubkey)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Manage Host SSH Public Keys;allow (write) groupdn = ldap:///cn=System: Manage Host SSH Public Keys,cn=permissions,cn=pbac,dc=ipa,dc=example;) dn: cn=computers,cn=accounts,dc=ipa,dc=example aci: (targetattr = description || l || macaddress || nshardwareplatform || nshostlocation || nsosversion || userclass)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Modify Hosts;allow (write) groupdn = ldap:///cn=System: Modify Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example;) +dn: cn=computers,cn=accounts,dc=ipa,dc=example +aci: (targetattr = krbcanonicalname || krbprincipalname)(targetfilter = (objectclass=ipahost))(version 3.0;acl permission:System: Modify krbCanonicalName and krbPrincipalName to a Host;allow (write) groupdn = ldap:///cn=System: Modify krbCanonicalName and krbPrincipalName to a Host,cn=permissions,cn=pbac,dc=ipa,dc=example;) dn: dc=ipa,dc=example aci: (targetattr = cn || createtimestamp || entryusn || macaddress || modifytimestamp || objectclass)(target = ldap:///cn=computers,cn=compat,dc=ipa,dc=example;)(version 3.0;acl permission:System: Read Host Compat Tree;allow (compare,read,search) userdn = ldap:///anyone;;) dn: cn=computers,cn=accounts,dc=ipa,dc=example diff --git a/API.txt b/API.txt index bbd0f507b2faeec0239920cdcff28fe25d618e02..ef1f70397e7685161821a98a92ea575aa6eff532 100644 --- a/API.txt
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. -- David Kupka From cea7c3e8eb41798d7f2bba916a95a78c034ee052 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 13:50:21 +0200 Subject: [PATCH] Detect and configure all usable IP addresses. Find, verify and configure all IP addresses that can be used to reach the server FreeIPA is being installed on. Ignore some IP address only if user specifies subset of detected addresses using --ip-address option. This change simplyfies FreeIPA installation on multihomed and dual-stacked servers. https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-replica-install| 48 ++--- install/tools/ipa-server-install | 50 ++ ipaserver/install/bindinstance.py| 114 ++- ipaserver/install/installutils.py| 96 +- ipaserver/install/ipa_replica_prepare.py | 65 ++ 5 files changed, 207 insertions(+), 166 deletions(-) diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install index e3b65b0963d92e4d102a1166aa001fe8cb164562..bdc1a86fd339718c91a67b6c32daa69905025e90 100755 --- a/install/tools/ipa-replica-install +++ b/install/tools/ipa-replica-install @@ -67,8 +67,8 @@ def parse_options(): default=False, help=configure a dogtag CA) basic_group.add_option(--setup-kra, dest=setup_kra, action=store_true, default=False, help=configure a dogtag KRA) -basic_group.add_option(--ip-address, dest=ip_address, - type=ip, ip_local=True, +basic_group.add_option(--ip-address, dest=ip_addresses, + type=ip, ip_local=True, action=append, default=[], help=Replica server IP Address) basic_group.add_option(-p, --password, dest=password, sensitive=True, help=Directory Manager (existing master) password) @@ -112,7 +112,8 @@ def parse_options(): type=ip, help=Add a DNS forwarder) dns_group.add_option(--no-forwarders, dest=no_forwarders, action=store_true, default=False, help=Do not add any DNS forwarders, use root servers instead) -dns_group.add_option(--reverse-zone, dest=reverse_zone, help=The reverse DNS zone to use) +dns_group.add_option(--reverse-zone, dest=reverse_zones, default
Re: [Freeipa-devel] [PATCH] 323 Fix certmonger code causing the ca_renewal_master update plugin to fail
On 09/17/2014 03:57 PM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4547. Honza Works for me, thanks for patch. ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse zone(s) Fixed. 5) ipa-server-install --ip-address=10.16.78.105 --reverse-zone=10.in-addr.arpa. --reverse-zone=16.10.in-addr.arpa. --setup-dns Creates both reverse zones, but 10.in-addr.arpa. is empty. I'm
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse zone(s) Fixed. 5) ipa-server-install --ip-address=10.16.78.105 --reverse-zone=10.in-addr.arpa. --reverse-zone=16.10.in-addr.arpa
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse zone(s) Fixed. 5) ipa-server-install --ip-address=10.16.78.105 --reverse-zone=10
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse
Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file
On 09/26/2014 09:56 AM, Martin Kosek wrote: On 09/02/2014 10:18 AM, Jan Cholasta wrote: Dne 27.8.2014 v 16:49 David Kupka napsal(a): On 08/27/2014 11:22 AM, Jan Cholasta wrote: Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a): David Kupka wrote: On 08/26/2014 03:08 PM, Jan Cholasta wrote: Hi, Dne 26.8.2014 v 13:01 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4481 Doing this will break ipa-client-automount and ipa-certupdate, because they assume that api.env.host contains the hostname of the local system (which is the default value). It looked suspiciously simple so I could expect that there is some catch. There is obviously some confusion about what the option should represent (documentation says server hostname, code does client hostname), IMO we should resolve that first. Ok, are there any suggestions? What is the desired state? AIUI the server option is deprecated because it wasn't being used, not that it needed to be replaced. I believe that in most cases the server name is pulled from the xmlrpc_uri. Yes, that's what the ticket says: https://fedorahosted.org/freeipa/ticket/3071. Ok, adding 'host' entry with local host name. host has always meant the local host name. I think the man page is wrong. +1 Fixing the line in man page. rob ACK as long as this works for Nalin. I see Nalin is OK with the patch, I am not so OK. What should we do with the server option then? It is still being referred to as Deprecated in the man page. Should we then un-deprecate it as Honza suggested down the thread? Martin Ok, changed man page. It no longer refer server as deprecated. -- David Kupka From 273d68f91dede5fccb70944b2a360865082bf276 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 27 Aug 2014 16:02:35 +0200 Subject: [PATCH] Add 'host' setting into default.conf configuration file on client. Fix description in man page. 'host' setting specifies local hostname not the hostname of IPA server. https://fedorahosted.org/freeipa/ticket/4481 --- ipa-client/ipa-install/ipa-client-install | 5 +++-- ipa-client/man/default.conf.5 | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index b3da28df19654a2bf676fd7499057828394c9618..45e802207d06a64cc53c581445c9e897c5295c88 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -812,7 +812,7 @@ def uninstall(options, env): return rv -def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server): +def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname): ipaconf = ipaclient.ipachangeconf.IPAChangeConf(IPA Installer) ipaconf.setOptionAssignment( = ) ipaconf.setSectionNameDelimiters(([,])) @@ -825,6 +825,7 @@ def configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server): {'name':'realm', 'type':'option', 'value':cli_realm}, {'name':'domain', 'type':'option', 'value':cli_domain}, {'name':'server', 'type':'option', 'value':cli_server[0]}, + {'name':'host', 'type':'option', 'value':hostname}, {'name':'xmlrpc_uri', 'type':'option', 'value':'https://%s/ipa/xml' % ipautil.format_netloc(cli_server[0])}, {'name':'enable_ra', 'type':'option', 'value':'True'}] @@ -2473,7 +2474,7 @@ def install(options, env, fstore, statestore): # Configure ipa.conf if not options.on_master: -configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server) +configure_ipa_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server, hostname) root_logger.info(Created /etc/ipa/default.conf) api.bootstrap(context='cli_installer', debug=options.debug) diff --git a/ipa-client/man/default.conf.5 b/ipa-client/man/default.conf.5 index c1ccf109e874907885fc3b51b63507c2b46b64ab..dbc8a5b4647439de4de7c01152d098eb0561e236 100644 --- a/ipa-client/man/default.conf.5 +++ b/ipa-client/man/default.conf.5 @@ -96,7 +96,7 @@ Specifies whether the CA is acting as an RA agent, such as when dogtag is being Specifies whether an IPA client should attempt to fall back and try other services if the first connection fails. .TP .B host hostname -Specifies the hostname of the IPA server. This value is used to construct URL values on the client and server. +Specifies the local system hostname. .TP .B in_server boolean Specifies whether requests should be forwarded to an IPA server or handled locally. This is used internally by IPA in a similar way as context. The same IPA framework is used by the ipa command\-line tool and the server. This setting tells the framework whether it should execute the command as if on the server or forward it via XML\-RPC to a remote server. @@ -164,7 +164,7 @@ Specifies the length of time authentication credentials cached
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/26/2014 02:47 PM, David Kupka wrote: On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/26/2014 05:50 PM, Martin Basti wrote: On 26/09/14 14:47, David Kupka wrote: On 09/26/2014 10:30 AM, David Kupka wrote: On 09/26/2014 09:34 AM, Jan Cholasta wrote: Dne 26.9.2014 v 08:28 David Kupka napsal(a): On 09/25/2014 04:17 PM, David Kupka wrote: On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) + ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) + ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0
[Freeipa-devel] [PATCH] 0017 Do not require description in UI.
https://fedorahosted.org/freeipa/ticket/4387 -- David Kupka From 8a0ac7417e904c21946e08bbdd759550bffab5ad Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 26 Sep 2014 02:54:28 -0400 Subject: [PATCH] Do not require description in UI. Description attribute is not required in LDAP schema so there is no reason to require it in UI. Modified tests to reflect this change. https://fedorahosted.org/freeipa/ticket/4387 --- API.txt | 14 +++--- ipalib/plugins/group.py | 2 +- ipalib/plugins/hbacsvcgroup.py| 2 +- ipalib/plugins/hostgroup.py | 2 +- ipalib/plugins/netgroup.py| 2 +- ipalib/plugins/privilege.py | 2 +- ipalib/plugins/role.py| 2 +- ipalib/plugins/sudocmdgroup.py| 2 +- ipatests/test_cmdline/test_cli.py | 1 - ipatests/test_xmlrpc/test_batch_plugin.py | 9 + 10 files changed, 15 insertions(+), 23 deletions(-) diff --git a/API.txt b/API.txt index bbd0f507b2faeec0239920cdcff28fe25d618e02..d8078b98a7ea7f2e51e4eec7f04b7321781318c7 100644 --- a/API.txt +++ b/API.txt @@ -1296,7 +1296,7 @@ args: 1,10,3 arg: Str('cn', attribute=True, cli_name='group_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('external', autofill=True, cli_name='external', default=False) option: Int('gidnumber', attribute=True, cli_name='gid', minvalue=1, multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') @@ -1681,7 +1681,7 @@ args: 1,7,3 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('setattr*', cli_name='setattr', exclude='webui') @@ -1931,7 +1931,7 @@ args: 1,7,3 arg: Str('cn', attribute=True, cli_name='hostgroup_name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('setattr*', cli_name='setattr', exclude='webui') @@ -2180,7 +2180,7 @@ args: 1,11,3 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Str('externalhost', attribute=True, cli_name='externalhost', multivalue=True, required=False) option: StrEnum('hostcategory', attribute=True, cli_name='hostcat', multivalue=False, required=False, values=(u'all',)) option: Str('nisdomainname', attribute=True, cli_name='nisdomain', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', required=False) @@ -2608,7 +2608,7 @@ args: 1,7,3 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('setattr*', cli_name='setattr', exclude='webui
Re: [Freeipa-devel] [PATCH] 0017 Do not require description in UI.
On 09/29/2014 10:22 AM, Martin Kosek wrote: On 09/29/2014 10:09 AM, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4387 The changes look OK so far, except the test fix. The test_batch_plugin.py test is apparently testing that batch command behaves well in RequirementError for options. Thus, we should not remove it, we should just pick different option. Like filling uid+first options with user-add, but missing the --last option. Martin Ok, but the test is bit redundant as there is already test for missing givenname that should behave the same way. -- David Kupka From 1e661dc28aaec2d9e1cffb873c193da7221eefd9 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 26 Sep 2014 02:54:28 -0400 Subject: [PATCH] Do not require description in UI. Description attribute is not required in LDAP schema so there is no reason to require it in UI. Modified tests to reflect this change. https://fedorahosted.org/freeipa/ticket/4387 --- API.txt | 14 +++--- ipalib/plugins/group.py | 2 +- ipalib/plugins/hbacsvcgroup.py| 2 +- ipalib/plugins/hostgroup.py | 2 +- ipalib/plugins/netgroup.py| 2 +- ipalib/plugins/privilege.py | 2 +- ipalib/plugins/role.py| 2 +- ipalib/plugins/sudocmdgroup.py| 2 +- ipatests/test_cmdline/test_cli.py | 1 - ipatests/test_xmlrpc/test_batch_plugin.py | 5 +++-- 10 files changed, 17 insertions(+), 17 deletions(-) diff --git a/API.txt b/API.txt index bbd0f507b2faeec0239920cdcff28fe25d618e02..d8078b98a7ea7f2e51e4eec7f04b7321781318c7 100644 --- a/API.txt +++ b/API.txt @@ -1296,7 +1296,7 @@ args: 1,10,3 arg: Str('cn', attribute=True, cli_name='group_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('external', autofill=True, cli_name='external', default=False) option: Int('gidnumber', attribute=True, cli_name='gid', minvalue=1, multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') @@ -1681,7 +1681,7 @@ args: 1,7,3 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('setattr*', cli_name='setattr', exclude='webui') @@ -1931,7 +1931,7 @@ args: 1,7,3 arg: Str('cn', attribute=True, cli_name='hostgroup_name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Flag('no_members', autofill=True, default=False, exclude='webui') option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui') option: Str('setattr*', cli_name='setattr', exclude='webui') @@ -2180,7 +2180,7 @@ args: 1,11,3 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', primary_key=True, required=True) option: Str('addattr*', cli_name='addattr', exclude='webui') option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui') -option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=True) +option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False) option: Str('externalhost', attribute=True, cli_name='externalhost', multivalue=True, required=False) option: StrEnum('hostcategory', attribute=True, cli_name='hostcat', multivalue=False, required=False, values=(u'all',)) option: Str('nisdomainname', attribute=True, cli_name='nisdomain', multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*$', required=False) @@ -2608,7 +2608,7 @@ args: 1,7,3 arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True) option: Str('addattr*', cli_name
[Freeipa-devel] [PATCH] 0018 Check that port 8443 is available when installing PKI.
https://fedorahosted.org/freeipa/ticket/4564 -- David Kupka From d5748822b8fac3cde01670507f80bfa9c4c04ede Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 29 Sep 2014 04:27:30 -0400 Subject: [PATCH] Check that port 8443 is available when installing PKI. https://fedorahosted.org/freeipa/ticket/4564 --- install/tools/ipa-ca-install | 9 + install/tools/ipa-server-install | 5 + ipaserver/install/cainstance.py | 8 3 files changed, 22 insertions(+) diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install index fc89412486a288568de85761742dbf32e8a63c65..8ace1cd5d5600f1406f1fdd4ddf37e784ed27270 100755 --- a/install/tools/ipa-ca-install +++ b/install/tools/ipa-ca-install @@ -98,6 +98,11 @@ def parse_options(): def get_dirman_password(): return installutils.read_password(Directory Manager (existing master), confirm=False, validate=False) +def check_ca(): +if not cainstance.check_port(): +print IPA requires port 8443 for PKI but it is currently in use. +sys.exit(1) + def install_dns_records(config, options): if not bindinstance.dns_container_exists(config.master_host_name, @@ -198,6 +203,8 @@ def install_replica(safe_options, options, filename): else: cainstance.replica_ca_install_check(config) +check_ca() + # Configure the CA if necessary CA = cainstance.install_replica_ca(config, postinstall=True) @@ -291,6 +298,8 @@ def install_master(safe_options, options): domain_name = api.env.domain host_name = api.env.host +check_ca() + dirname = dsinstance.config_dirname( dsinstance.realm_to_serverid(realm_name)) cadb = certs.CertDB(realm_name, subject_base=subject_base) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index 7d60d27bcfae9a89ad7c5d811d3f9d8a9fda60cb..520cfda43cc0bf2a85fab3db0c23aa60cc2d7372 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -795,6 +795,11 @@ def main(): # Make sure the 389-ds ports are available check_dirsrv(options.unattended) +if setup_ca: +if not cainstance.check_port(): +print IPA requires port 8443 for PKI but it is currently in use. +sys.exit(Aborting installation) + if options.conf_ntp: try: ipaclient.ntpconf.check_timedate_services() diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index 04968d411fc5bc1073e86fab42743fc65f7b828a..164d67b69a489af077ef4929958fb4027761e96a 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -105,6 +105,14 @@ def check_inst(): return True +def check_port(): + +Check that dogtag port (8443) is available. + +Returns True when the port is free, False if it's taken. + +return not ipautil.host_port_open(None, 8443) + def get_preop_pin(instance_root, instance_name): # Only used for Dogtag 9 preop_pin = None -- 1.9.3 From a4c20f41f289bfb7b338790637089608bd80f2cd Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Mon, 29 Sep 2014 04:27:30 -0400 Subject: [PATCH] Check that port 8443 is available when installing PKI. https://fedorahosted.org/freeipa/ticket/4564 --- install/tools/ipa-ca-install | 9 + install/tools/ipa-server-install | 5 + ipaserver/install/cainstance.py | 8 3 files changed, 22 insertions(+) diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install index 475794bb6186725ad5ab079adfb98849c589e67e..96950efd7c68a4646deb7e90e0394d8c3dde2e77 100755 --- a/install/tools/ipa-ca-install +++ b/install/tools/ipa-ca-install @@ -98,6 +98,11 @@ def get_dirman_password(): Directory Manager (existing master), confirm=False, validate=False) +def check_ca(): +if not cainstance.check_port(): +print IPA requires port 8443 for PKI but it is currently in use. +sys.exit(1) + def install_dns_records(config, options): if not bindinstance.dns_container_exists(config.master_host_name, @@ -175,6 +180,8 @@ def install_replica(safe_options, options, filename): else: cainstance.replica_ca_install_check(config) +check_ca() + # Configure the CA if necessary CA = cainstance.install_replica_ca(config, postinstall=True) @@ -269,6 +276,8 @@ def install_master(safe_options, options): domain_name = api.env.domain host_name = api.env.host +check_ca() + dirname = dsinstance.config_dirname( dsinstance.realm_to_serverid(realm_name)) cadb = certs.CertDB(realm_name, subject_base=subject_base) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index e73a098df3c34794639f75460baac70b4b49480a..5321e2694992815e1bc93fe49772f11b82256f22 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -816,6 +816,11 @@ def main(): # Make sure the 389
[Freeipa-devel] [PATCH] 0020 Set IPA CA for freeipa certificates
https://fedorahosted.org/freeipa/ticket/4618 -- David Kupka From ab15f67ee35d29cd30b6b6d703a000c3cfe3188b Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 7 Oct 2014 10:19:09 -0400 Subject: [PATCH] Set IPA CA for freeipa certificates. In previous versions (before moving certmonger.py to DBus) it was set and some tools and modules depends on it. For example: ipa-getcert uses this to filter freeipa certificates. https://fedorahosted.org/freeipa/ticket/4618 --- ipapython/certmonger.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py index b46d65b2fb0149aceee0864774e2ab76623e7730..0291d01b42aa6701b24dcb6905dcffab68a9ba63 100644 --- a/ipapython/certmonger.py +++ b/ipapython/certmonger.py @@ -289,6 +289,9 @@ def start_tracking(nickname, secdir, password_file=None, command=None): params['key-nickname'] = nickname params['key-database'] = os.path.abspath(secdir) params['key-storage'] = 'NSSDB' +ca_path = cm.obj_if.find_ca_by_nickname('IPA') +if ca_path: +params['ca'] = ca_path if command: params['cert-postsave-command'] = command if password_file: -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0021 Fix example usage in ipa man page.
https://fedorahosted.org/freeipa/ticket/4587 -- David Kupka From 883e90237fbde1075d00990568cde18773e80611 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 8 Oct 2014 01:43:47 -0400 Subject: [PATCH] Fix example usage in ipa man page. https://fedorahosted.org/freeipa/ticket/4587 --- ipa.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipa.1 b/ipa.1 index fc39fceaae5aa4c614ccaaa7e608f2326d926755..6acfcfd9f7ab580c9b086fa249903cdf10544cdb 100644 --- a/ipa.1 +++ b/ipa.1 @@ -149,7 +149,7 @@ Create a new user with username foo, first name foo and last name bar. \fBipa group\-add bar \-\-desc this is an example group Create a new group with name bar and description this is an example group. .TP -\fBipa group\-add\-member bar \-\-users=admin,foo\fR +\fBipa group\-add\-member bar \-\-users={admin,foo}\fR Add users admin and foo to the group bar. .TP \fBipa user\-show foo \-\-raw\fR -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0021 Fix example usage in ipa man page.
On 10/08/2014 08:02 AM, Alexander Bokovoy wrote: On Wed, 08 Oct 2014, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4587 -- David Kupka From 883e90237fbde1075d00990568cde18773e80611 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 8 Oct 2014 01:43:47 -0400 Subject: [PATCH] Fix example usage in ipa man page. https://fedorahosted.org/freeipa/ticket/4587 --- ipa.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipa.1 b/ipa.1 index fc39fceaae5aa4c614ccaaa7e608f2326d926755..6acfcfd9f7ab580c9b086fa249903cdf10544cdb 100644 --- a/ipa.1 +++ b/ipa.1 @@ -149,7 +149,7 @@ Create a new user with username foo, first name foo and last name bar. \fBipa group\-add bar \-\-desc this is an example group Create a new group with name bar and description this is an example group. .TP -\fBipa group\-add\-member bar \-\-users=admin,foo\fR +\fBipa group\-add\-member bar \-\-users={admin,foo}\fR Add users admin and foo to the group bar. .TP \fBipa user\-show foo \-\-raw\fR I would like to see a stance about shell expansion use here. May be add a phrase about that right after Add users ... to the group ...? It might not be entirely obvious to other people that we rely on a shell expansion features here. At first, I wanted to remove one of users mentioned there but '--users=foo' looks confusing to me (using plural and specifying just one value). Personally I would prefer to change all plural parameters to singular form but it is a large change considering the benefit. What about two examples? One '--users=foo' and other using shell expansion. -- David Kupka From 883e90237fbde1075d00990568cde18773e80611 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 8 Oct 2014 01:43:47 -0400 Subject: [PATCH] Fix example usage in ipa man page. https://fedorahosted.org/freeipa/ticket/4587 --- ipa.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipa.1 b/ipa.1 index fc39fceaae5aa4c614ccaaa7e608f2326d926755..6acfcfd9f7ab580c9b086fa249903cdf10544cdb 100644 --- a/ipa.1 +++ b/ipa.1 @@ -149,7 +149,7 @@ Create a new user with username foo, first name foo and last name bar. \fBipa group\-add bar \-\-desc this is an example group Create a new group with name bar and description this is an example group. .TP -\fBipa group\-add\-member bar \-\-users=admin,foo\fR +\fBipa group\-add\-member bar \-\-users={admin,foo}\fR Add users admin and foo to the group bar. .TP \fBipa user\-show foo \-\-raw\fR -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0021 Fix example usage in ipa man page.
On 10/08/2014 08:19 AM, David Kupka wrote: On 10/08/2014 08:02 AM, Alexander Bokovoy wrote: On Wed, 08 Oct 2014, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4587 -- David Kupka From 883e90237fbde1075d00990568cde18773e80611 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 8 Oct 2014 01:43:47 -0400 Subject: [PATCH] Fix example usage in ipa man page. https://fedorahosted.org/freeipa/ticket/4587 --- ipa.1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipa.1 b/ipa.1 index fc39fceaae5aa4c614ccaaa7e608f2326d926755..6acfcfd9f7ab580c9b086fa249903cdf10544cdb 100644 --- a/ipa.1 +++ b/ipa.1 @@ -149,7 +149,7 @@ Create a new user with username foo, first name foo and last name bar. \fBipa group\-add bar \-\-desc this is an example group Create a new group with name bar and description this is an example group. .TP -\fBipa group\-add\-member bar \-\-users=admin,foo\fR +\fBipa group\-add\-member bar \-\-users={admin,foo}\fR Add users admin and foo to the group bar. .TP \fBipa user\-show foo \-\-raw\fR I would like to see a stance about shell expansion use here. May be add a phrase about that right after Add users ... to the group ...? It might not be entirely obvious to other people that we rely on a shell expansion features here. At first, I wanted to remove one of users mentioned there but '--users=foo' looks confusing to me (using plural and specifying just one value). Personally I would prefer to change all plural parameters to singular form but it is a large change considering the benefit. What about two examples? One '--users=foo' and other using shell expansion. I forget to update the patch, sorry. -- David Kupka From 554d9b0f806f6eb7ad8ffc99fbd7ac6cb20c5c4c Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 8 Oct 2014 01:43:47 -0400 Subject: [PATCH] Fix example usage in ipa man page. https://fedorahosted.org/freeipa/ticket/4587 --- ipa.1 | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ipa.1 b/ipa.1 index fc39fceaae5aa4c614ccaaa7e608f2326d926755..fe2a1aa7bafadd70596b5d95bca49a3f583a3c3d 100644 --- a/ipa.1 +++ b/ipa.1 @@ -149,8 +149,11 @@ Create a new user with username foo, first name foo and last name bar. \fBipa group\-add bar \-\-desc this is an example group Create a new group with name bar and description this is an example group. .TP -\fBipa group\-add\-member bar \-\-users=admin,foo\fR -Add users admin and foo to the group bar. +\fBipa group\-add\-member bar \-\-users=foo\fR +Add user foo to the group bar. +.TP +\fBipa group\-add\-member bar \-\-users={admin,foo}\fR +Add users admin and foo to the group bar. This approach depends on shell expansion feature. .TP \fBipa user\-show foo \-\-raw\fR Display user foo as (s)he is stored on the server. -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0019 Stop dogtag when updating its configuration in, ipa-upgradeconfig
https://fedorahosted.org/freeipa/ticket/4569 -- David Kupka From a1363fa49a35115cfa15d51d7ae5c298828efc37 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 30 Sep 2014 08:41:49 -0400 Subject: [PATCH] Stop dogtag when updating its configuration in ipa-upgradeconfig. Modifying CS.cfg when dogtag is running may (and does) result in corrupting this file. https://fedorahosted.org/freeipa/ticket/4569 --- install/restart_scripts/renew_ca_cert | 31 +- install/tools/ipa-upgradeconfig | 15 +++-- ipaserver/install/cainstance.py | 108 ++ 3 files changed, 84 insertions(+), 70 deletions(-) diff --git a/install/restart_scripts/renew_ca_cert b/install/restart_scripts/renew_ca_cert index 2ad2038703a74fe3549708549091633b35695907..e14e699bf57c631238a342ba19a3a1d483574bbb 100644 --- a/install/restart_scripts/renew_ca_cert +++ b/install/restart_scripts/renew_ca_cert @@ -104,20 +104,23 @@ def main(): cfg_path, 'subsystem.select', '=') if config == 'New': syslog.syslog(syslog.LOG_NOTICE, Updating CS.cfg) -if x509.is_self_signed(cert, x509.DER): -installutils.set_directive( -cfg_path, 'hierarchy.select', 'Root', -quotes=False, separator='=') -installutils.set_directive( -cfg_path, 'subsystem.count', '1', -quotes=False, separator='=') -else: -installutils.set_directive( -cfg_path, 'hierarchy.select', 'Subordinate', -quotes=False, separator='=') -installutils.set_directive( -cfg_path, 'subsystem.count', '0', -quotes=False, separator='=') +with installutils.stopped_service( +configured_constants.SERVICE_NAME, +configured_constants.PKI_INSTANCE_NAME): +if x509.is_self_signed(cert, x509.DER): +installutils.set_directive( +cfg_path, 'hierarchy.select', 'Root', +quotes=False, separator='=') +installutils.set_directive( +cfg_path, 'subsystem.count', '1', +quotes=False, separator='=') +else: +installutils.set_directive( +cfg_path, 'hierarchy.select', 'Subordinate', +quotes=False, separator='=') +installutils.set_directive( +cfg_path, 'subsystem.count', '0', +quotes=False, separator='=') else: syslog.syslog(syslog.LOG_NOTICE, Not updating CS.cfg) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index ba4ac93998fa203719e058fdfe557f4f2a67a865..08ff9a224d92245ff2c5845e6c9df22a700df562 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -233,7 +233,12 @@ def upgrade_pki(ca, fstore): if not installutils.get_directive(configured_constants.CS_CFG_PATH, 'proxy.securePort', '=') and \ os.path.exists(paths.PKI_SETUP_PROXY): -ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib' +# update proxy configuration with stopped dogtag to prevent corruption +# of CS.cfg +with installutils.stopped_service( +configured_constants.SERVICE_NAME, +configured_constants.PKI_INSTANCE_NAME): +ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib' ,'-pki_instance_name=pki-ca','-subsystem_type=ca']) root_logger.debug('Proxy configuration updated') else: @@ -821,9 +826,11 @@ def migrate_crl_publish_dir(ca): root_logger.error('Cannot move CRL file to new directory: %s', e) try: -installutils.set_directive(caconfig.CS_CFG_PATH, -'ca.publish.publisher.instance.FileBaseCRLPublisher.directory', -publishdir, quotes=False, separator='=') +with installutils.stopped_service(caconfig.SERVICE_NAME, +caconfig.PKI_INSTANCE_NAME): +installutils.set_directive(caconfig.CS_CFG_PATH, +'ca.publish.publisher.instance.FileBaseCRLPublisher.directory', +publishdir, quotes=False, separator='=') except OSError, e: root_logger.error('Cannot update CA configuration file %s: %s', caconfig.CS_CFG_PATH, e) diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index 521f25d96693efe64b5859901bb3da9da79ee0ec..2793b407a88f0b5b6592f79a7b6279d2fa41a787 100644 --- a/ipaserver/install/cainstance.py +++ b
Re: [Freeipa-devel] [PATCH] 0019 Stop dogtag when updating its configuration in, ipa-upgradeconfig
On 10/08/2014 09:29 AM, Jan Cholasta wrote: Hi, Dne 8.10.2014 v 09:09 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4569 In renew_ca_cert and cainstance.py, dogtag should already be stopped in the places you modified, so why the change? I didn't noticed that it is already stopped, fixed. Also I don't think it's a good idea to backup CS.cfg when dogtag is still running (in cainstance.py). If the file is being modified by dogtag at the time it is backed up, the backup may be corrupted. Fixed, thanks. Honza -- David Kupka From 104dca26a87255be2b67652dd0f4c60b71e92e90 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 30 Sep 2014 08:41:49 -0400 Subject: [PATCH] Stop dogtag when updating its configuration in ipa-upgradeconfig. Modifying CS.cfg when dogtag is running may (and does) result in corrupting this file. https://fedorahosted.org/freeipa/ticket/4569 --- install/tools/ipa-upgradeconfig | 15 +++ ipaserver/install/cainstance.py | 6 -- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index ba4ac93998fa203719e058fdfe557f4f2a67a865..08ff9a224d92245ff2c5845e6c9df22a700df562 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -233,7 +233,12 @@ def upgrade_pki(ca, fstore): if not installutils.get_directive(configured_constants.CS_CFG_PATH, 'proxy.securePort', '=') and \ os.path.exists(paths.PKI_SETUP_PROXY): -ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib' +# update proxy configuration with stopped dogtag to prevent corruption +# of CS.cfg +with installutils.stopped_service( +configured_constants.SERVICE_NAME, +configured_constants.PKI_INSTANCE_NAME): +ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib' ,'-pki_instance_name=pki-ca','-subsystem_type=ca']) root_logger.debug('Proxy configuration updated') else: @@ -821,9 +826,11 @@ def migrate_crl_publish_dir(ca): root_logger.error('Cannot move CRL file to new directory: %s', e) try: -installutils.set_directive(caconfig.CS_CFG_PATH, -'ca.publish.publisher.instance.FileBaseCRLPublisher.directory', -publishdir, quotes=False, separator='=') +with installutils.stopped_service(caconfig.SERVICE_NAME, +caconfig.PKI_INSTANCE_NAME): +installutils.set_directive(caconfig.CS_CFG_PATH, +'ca.publish.publisher.instance.FileBaseCRLPublisher.directory', +publishdir, quotes=False, separator='=') except OSError, e: root_logger.error('Cannot update CA configuration file %s: %s', caconfig.CS_CFG_PATH, e) diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py index 521f25d96693efe64b5859901bb3da9da79ee0ec..ac6dd828aa38e14c16e7bb7c7d1c397793222852 100644 --- a/ipaserver/install/cainstance.py +++ b/ipaserver/install/cainstance.py @@ -1841,8 +1841,10 @@ def backup_config(dogtag_constants=None): if dogtag_constants is None: dogtag_constants = dogtag.configured_constants() -shutil.copy(dogtag_constants.CS_CFG_PATH, -dogtag_constants.CS_CFG_PATH + '.ipabkp') +with stopped_service(dogtag_constants.SERVICE_NAME, + instance_name=dogtag_constants.PKI_INSTANCE_NAME): +shutil.copy(dogtag_constants.CS_CFG_PATH, +dogtag_constants.CS_CFG_PATH + '.ipabkp') def update_cert_config(nickname, cert, dogtag_constants=None): -- 1.9.3 From f322136e5fd0bc1df5edf712c931c328dc5bdb5d Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 30 Sep 2014 08:41:49 -0400 Subject: [PATCH] Stop dogtag when updating its configuration in ipa-upgradeconfig. Modifying CS.cfg when dogtag is running may (and does) result in corrupting this file. https://fedorahosted.org/freeipa/ticket/4569 --- install/tools/ipa-upgradeconfig | 15 +++ ipaserver/install/cainstance.py | 6 -- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index 3914eb59066b515d33bebc19ca5afb4f50548bb2..abe3c077ccbaebf7317591eca19be99b686ae37d 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -233,7 +233,12 @@ def upgrade_pki(ca, fstore): if not installutils.get_directive(configured_constants.CS_CFG_PATH, 'proxy.securePort', '=') and \ os.path.exists(paths.PKI_SETUP_PROXY): -ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib' +# update proxy configuration with stopped dogtag to prevent corruption +# of CS.cfg +with installutils.stopped_service
Re: [Freeipa-devel] [PATCH 130] Missing DNS tests
On 10/01/2014 01:42 PM, Martin Basti wrote: This patch adds 2 missing DNS tests: * removing non-existent permission * removing idnssoamname value using dnszone-mod --name-server= Patch attached, belongs to ipa-4-1 and master branches. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0133] Fix ipactl service ordering
On 10/09/2014 12:44 PM, Martin Basti wrote: On 08/10/14 20:08, Martin Basti wrote: On 08/10/14 16:59, Martin Basti wrote: IPA sorts service order alphabetically, this patch modify ipactl to use integers. How to reproduce: set attribute ipaConfigString: startOrder 150 DN: cn=HTTP,cn=ipa.example.com,cn=masters,cn=ipa,cn=etc,dc=example,dc=com then run #ipactl restart httpd service should start as last service, but it starts almost first. Patch attached. selfNACK NACKing my SelfNACK, I accidentally installed wrong RPM and I though I haven't fixed it correctly. Patch is OK. Works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 352 Fix certmonger configuration in installer code
On 10/08/2014 01:23 PM, Jan Cholasta wrote: Dne 8.10.2014 v 12:27 Jan Cholasta napsal(a): Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4619. Honza Forgot to delete a line in dogtaginstance.py (thanks to David for noticing). Updated patch attached. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0131-0132] Add missing attributes to named.conf
On 10/03/2014 12:45 PM, Martin Basti wrote: Hello! Patch 131: https://fedorahosted.org/freeipa/ticket/3801#comment:31 Patch 132: I modified named.conf in 131, so I change the rest of paths to be ipaplatform specified. Patches attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hi! The upgrade processes looks fine to me. And I didn't find any surprise in the code. So it's A and C/2 from me. For the rest go to Petr^2. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 352 Fix certmonger configuration in installer code
On 10/10/2014 08:50 AM, Martin Kosek wrote: On 10/09/2014 03:56 PM, David Kupka wrote: On 10/08/2014 01:23 PM, Jan Cholasta wrote: Dne 8.10.2014 v 12:27 Jan Cholasta napsal(a): Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4619. Honza Forgot to delete a line in dogtaginstance.py (thanks to David for noticing). Updated patch attached. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK. Thanks, pushed to master. Just to double check - no parts of the fixes should be applied to 4.1 or 4.0 branches, is that correct? Martin I've never seen or been able to reproduce this bug other than on master branch. AFAIK, the issue was caused by KRA patches that are only in master. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0020 Set IPA CA for freeipa certificates
On 10/14/2014 09:32 AM, Jan Cholasta wrote: Dne 14.10.2014 v 08:55 David Kupka napsal(a): On 10/10/2014 04:04 PM, Jan Cholasta wrote: Hi, Dne 7.10.2014 v 16:56 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4618 This works, but I would prefer if the code did not silently ignore when the CA is not found. Honza Ok, modified patch attached. Nitpick: no periods at the end of exception messages please. Otherwise ACK. Removed. -- David Kupka From 17c0c34cd50c3e2493cef717be6287183d68 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 14 Oct 2014 03:40:43 -0400 Subject: [PATCH] Set IPA CA for freeipa certificates. In previous versions (before moving certmonger.py to DBus) it was set and some tools and modules depends on it. For example: ipa-getcert uses this to filter freeipa certificates. https://fedorahosted.org/freeipa/ticket/4618 --- ipapython/certmonger.py | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py index b46d65b2fb0149aceee0864774e2ab76623e7730..84f04a62e5e94716310efcd7847bc7f89460b73c 100644 --- a/ipapython/certmonger.py +++ b/ipapython/certmonger.py @@ -279,7 +279,7 @@ def start_tracking(nickname, secdir, password_file=None, command=None): certmonger to run when it renews a certificate. This command must reside in /usr/lib/ipa/certmonger to work with SELinux. -Returns True or False +Returns certificate nickname. cm = _connect_to_certmonger() params = {'TRACK': True} @@ -289,6 +289,10 @@ def start_tracking(nickname, secdir, password_file=None, command=None): params['key-nickname'] = nickname params['key-database'] = os.path.abspath(secdir) params['key-storage'] = 'NSSDB' +ca_path = cm.obj_if.find_ca_by_nickname('IPA') +if not ca_path: +raise RuntimeError('IPA CA not found') +params['ca'] = ca_path if command: params['cert-postsave-command'] = command if password_file: -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0019 Stop dogtag when updating its configuration in, ipa-upgradeconfig
On 10/10/2014 03:24 PM, Jan Cholasta wrote: Dne 8.10.2014 v 12:36 David Kupka napsal(a): On 10/08/2014 09:29 AM, Jan Cholasta wrote: Hi, Dne 8.10.2014 v 09:09 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4569 In renew_ca_cert and cainstance.py, dogtag should already be stopped in the places you modified, so why the change? I didn't noticed that it is already stopped, fixed. Also I don't think it's a good idea to backup CS.cfg when dogtag is still running (in cainstance.py). If the file is being modified by dogtag at the time it is backed up, the backup may be corrupted. Fixed, thanks. CAInstance.backup_config should be called only when Dogtag is stopped as well, you don't need to change it. backup_config is callable from outside of cainstance.py so it's safer to check that dogtag is stopped and stop it if necessary. When dogtag is already stopped it won't do anything. Honza It would be better to stop and start dogtag only once in ipa-upgradeconfig, not every time there is a modification to CS.cfg. OK. -- David Kupka From 2332a404f9e53549ccadb925e8c3f267b4034175 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 30 Sep 2014 08:41:49 -0400 Subject: [PATCH] Stop dogtag when updating its configuration in ipa-upgradeconfig. Modifying CS.cfg when dogtag is running may (and does) result in corrupting this file. https://fedorahosted.org/freeipa/ticket/4569 --- install/tools/ipa-upgradeconfig | 46 ++--- ipaserver/install/cainstance.py | 6 -- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index 82e7857d5dec8955935b948df34aab08bfa7f914..e064f38fc963d94c7775f2282402eaaddb682af4 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -233,8 +233,10 @@ def upgrade_pki(ca, fstore): if not installutils.get_directive(configured_constants.CS_CFG_PATH, 'proxy.securePort', '=') and \ os.path.exists(paths.PKI_SETUP_PROXY): -ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib' - ,'-pki_instance_name=pki-ca','-subsystem_type=ca']) +# update proxy configuration with stopped dogtag to prevent corruption +# of CS.cfg +ipautil.run([paths.PKI_SETUP_PROXY, '-pki_instance_root=/var/lib', + '-pki_instance_name=pki-ca','-subsystem_type=ca']) root_logger.debug('Proxy configuration updated') else: root_logger.debug('Proxy configuration up-to-date') @@ -1082,28 +1084,30 @@ def main(): ca = cainstance.CAInstance(api.env.realm, certs.NSS_DIR) ca.backup_config() -# migrate CRL publish dir before the location in ipa.conf is updated -ca_restart = migrate_crl_publish_dir(ca) +with installutils.stopped_service(configured_constants.SERVICE_NAME, +configured_constants.PKI_INSTANCE_NAME): +# migrate CRL publish dir before the location in ipa.conf is updated +ca_restart = migrate_crl_publish_dir(ca) -if ca.is_configured(): -crl = installutils.get_directive(configured_constants.CS_CFG_PATH, - 'ca.crl.MasterCRL.enableCRLUpdates', - '=') -sub_dict['CLONE']='#' if crl.lower() == 'true' else '' +if ca.is_configured(): +crl = installutils.get_directive(configured_constants.CS_CFG_PATH, +'ca.crl.MasterCRL.enableCRLUpdates', '=') +sub_dict['CLONE']='#' if crl.lower() == 'true' else '' -certmap_dir = dsinstance.config_dirname( -dsinstance.realm_to_serverid(api.env.realm)) +certmap_dir = dsinstance.config_dirname( +dsinstance.realm_to_serverid(api.env.realm)) + +upgrade(sub_dict, paths.HTTPD_IPA_CONF, ipautil.SHARE_DIR + ipa.conf) +upgrade(sub_dict, paths.HTTPD_IPA_REWRITE_CONF, ipautil.SHARE_DIR + ipa-rewrite.conf) +upgrade(sub_dict, paths.HTTPD_IPA_PKI_PROXY_CONF, ipautil.SHARE_DIR + ipa-pki-proxy.conf, add=True) +if subject_base: +upgrade( +sub_dict, +os.path.join(certmap_dir, certmap.conf), +os.path.join(ipautil.SHARE_DIR, certmap.conf.template) +) +upgrade_pki(ca, fstore) -upgrade(sub_dict, paths.HTTPD_IPA_CONF, ipautil.SHARE_DIR + ipa.conf) -upgrade(sub_dict, paths.HTTPD_IPA_REWRITE_CONF, ipautil.SHARE_DIR + ipa-rewrite.conf) -upgrade(sub_dict, paths.HTTPD_IPA_PKI_PROXY_CONF, ipautil.SHARE_DIR + ipa-pki-proxy.conf, add=True) -if subject_base: -upgrade( -sub_dict, -os.path.join(certmap_dir, certmap.conf), -os.path.join(ipautil.SHARE_DIR, certmap.conf.template) -) -upgrade_pki(ca, fstore) update_dbmodules(api.env.realm
[Freeipa-devel] [PATCH] 0023 Fix typo causing certmonger is provided with wrong path to, ipa-submit.
https://fedorahosted.org/freeipa/ticket/4624 -- David Kupka From c2808f958c9ee99374aadf808ca01bf7047de509 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 14 Oct 2014 06:54:00 -0400 Subject: [PATCH] Fix typo causing certmonger is provided with wrong path to ipa-submit. Using strip() instead split() caused that only first character of path was specified. Also using shlex for more robust parsing. https://fedorahosted.org/freeipa/ticket/4624 --- ipapython/certmonger.py | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py index ca8b373924428343fe108c88dfa0904237dfccce..dc6cff966539a288273172b3f3633646c96dd5ab 100644 --- a/ipapython/certmonger.py +++ b/ipapython/certmonger.py @@ -26,6 +26,7 @@ import os import sys import time import dbus +import shlex from ipapython import ipautil from ipapython import dogtag from ipapython.ipa_log_manager import * @@ -371,7 +372,7 @@ def add_principal_to_cas(principal): ca = _find_IPA_ca() if ca: ext_helper = ca.prop_if.Get(DBUS_CM_CA_IF, 'external-helper') -if ext_helper and ext_helper.find('-k') == -1: +if ext_helper and '-k' not in shlex.split(ext_helper): ext_helper = '%s -k %s' % (ext_helper.strip(), principal) ca.prop_if.Set(DBUS_CM_CA_IF, 'external-helper', ext_helper) @@ -383,8 +384,8 @@ def remove_principal_from_cas(): ca = _find_IPA_ca() if ca: ext_helper = ca.prop_if.Get(DBUS_CM_CA_IF, 'external-helper') -if ext_helper and ext_helper.find('-k'): -ext_helper = ext_helper.strip()[0] +if ext_helper and '-k' in shlex.split(ext_helper): +ext_helper = shlex.split(ext_helper)[0] ca.prop_if.Set(DBUS_CM_CA_IF, 'external-helper', ext_helper) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Fix printing of reverse zones in ipa-dns-install.
Submitting the patch again. I sent it from my gmail account accidentally. On 10/15/2014 03:58 PM, Martin Basti wrote: New contributor :-) ACK Thank you! -- David Kupka From 4d094e99ff82f69ad08b0df408d847350e900c7b Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 15 Oct 2014 09:24:20 -0400 Subject: [PATCH] Fix printing of reverse zones in ipa-dns-install. This was forgotten in patch for ticket https://fedorahosted.org/freeipa/ticket/3575 --- install/tools/ipa-dns-install | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install index ae60f211ab65fb2fddba437d4352f7fbd5ab6f8b..bf9d9908092a2cf4cd2f0252149bce426a8f2bb4 100755 --- a/install/tools/ipa-dns-install +++ b/install/tools/ipa-dns-install @@ -154,8 +154,8 @@ def main(): reverse_zones = bindinstance.check_reverse_zones(ip_addresses, options.reverse_zones, options, options.unattended, True) -if reverse_zones is not None: -print Using reverse zone %s % ', '.join(reverse_zones) +if reverse_zones: +print Using reverse zone(s) %s % ', '.join(reverse_zones) conf_ntp = ntpinstance.NTPInstance(fstore).is_enabled() -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 354-356 Check LDAP instead of local configuration to see if IPA CA is enabled
On 10/17/2014 10:15 AM, Jan Cholasta wrote: Dne 13.10.2014 v 14:48 Jan Cholasta napsal(a): Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/4621. Honza Rebased on top of current ipa-4-1, patches attached. Works for me, ACK. It would be nice to also start tracking certificates when IPA is CA-ful. But it can be done later, ticket: https://fedorahosted.org/freeipa/ticket/4644 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0117, 0135-0149] DNSSEC support
On 10/21/2014 10:57 AM, Martin Basti wrote: snip IPA-4-1 patches attached jcholast-357 patch should be applied last If there is no objections I will rebase to master branch after ack Works good for me but I didn't read the code. ACK for functionality. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 358 Do not check if port 8443 is available in step 2 of external CA install
On 10/22/2014 11:28 AM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4660. Honza ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0025 Respect UID and GID soft static allocation.
https://fedorahosted.org/freeipa/ticket/4585 -- David Kupka From 1cb3a44bcf5cba3dd741ac7222720d87f983e38d Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 22 Oct 2014 09:07:44 -0400 Subject: [PATCH] Respect UID and GID soft static allocation. https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation https://fedorahosted.org/freeipa/ticket/4585 --- ipaplatform/base/tasks.py | 43 + ipaplatform/redhat/tasks.py | 50 +++ ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/installutils.py | 42 5 files changed, 95 insertions(+), 44 deletions(-) diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py index 408447e43cd36d0cdf11a1877b3bc9880c4785de..1ea8db6a2a820f4efdb1522cfd87578e547aae99 100644 --- a/ipaplatform/base/tasks.py +++ b/ipaplatform/base/tasks.py @@ -22,7 +22,13 @@ This module contains default platform-specific implementations of system tasks. ''' +import pwd +import grp from ipaplatform.paths import paths +from ipapython.ipa_log_manager import log_mgr +from ipapython import ipautil + +log = log_mgr.get_logger(__name__) class BaseTaskNamespace(object): @@ -150,5 +156,42 @@ class BaseTaskNamespace(object): return +def create_system_user(self, name, group, homedir, shell): +Create a system user with a corresponding group +try: +grp.getgrnam(group) +except KeyError: +log.debug('Adding group %s', group) +args = [paths.GROUPADD, '-r', group] +try: +ipautil.run(args) +log.debug('Done adding group') +except ipautil.CalledProcessError as e: +log.critical('Failed to add group: %s', e) +raise +else: +log.debug('group %s exists', group) + +try: +pwd.getpwnam(name) +except KeyError: +log.debug('Adding user %s', name) +args = [ +paths.USERADD, +'-g', group, +'-c', 'DS System User', +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] +try: +ipautil.run(args) +log.debug('Done adding user') +except ipautil.CalledProcessError as e: +log.critical('Failed to add user: %s', e) +raise +else: +log.debug('user %s exists', name) + task_namespace = BaseTaskNamespace() diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py index 16d90a6d1a7d3d9aced5de82a5c1efe6b8c2..eaea806a533fd5f40856161de9bfc89fc3b7eb23 100644 --- a/ipaplatform/redhat/tasks.py +++ b/ipaplatform/redhat/tasks.py @@ -30,6 +30,8 @@ import socket import sys import urllib import base64 +import pwd +import grp from subprocess import CalledProcessError from nss.error import NSPRError @@ -393,5 +395,53 @@ class RedHatTaskNamespace(BaseTaskNamespace): return True +def create_system_user(self, name, group, homedir, shell): + +Create a system user with a corresponding group + +According to https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation +some system users should have fixed UID, GID and other parameters set. +This values should be constant and may be hardcoded. +Add other values for other users when needed. + +try: +grp.getgrnam(group) +except KeyError: +log.debug('Adding group %s', group) +args = [paths.GROUPADD, '-r', group] +if name == 'pkiuser': +args += ['-g', '17'] +try: +ipautil.run(args) +log.debug('Done adding group') +except ipautil.CalledProcessError as e: +log.critical('Failed to add group: %s', e) +raise +else: +log.debug('group %s exists', group) + +try: +pwd.getpwnam(name) +except KeyError: +log.debug('Adding user %s', name) +args = [ +paths.USERADD, +'-g', group, +'-c', 'DS System User', +'-M', '-r', name, +] +if name == 'pkiuser': +args += ['-u', '17', '-d', '/var/lib/pki', '-s', paths.NOLOGIN] +else: +args += ['-d', homedir, '-s', shell] +try: +ipautil.run(args) +log.debug('Done adding user') +except ipautil.CalledProcessError as e: +log.critical('Failed to add user: %s', e) +raise +else: +log.debug('user %s exists
Re: [Freeipa-devel] [PATCH] 0025 Respect UID and GID soft static allocation.
On 10/24/2014 10:43 AM, Martin Basti wrote: On 24/10/14 09:51, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4585 NACK 1) Why is there line with 'DS System User?' The comment should depend on service. +args = [ +paths.USERADD, +'-g', group, +'-c', 'DS System User', +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] This was part of the original code and I didn't notice it. Nice catch, thanks. 2) code create_system_user is duplicated between base and redhat tasks with platform dependent changes. IMO it would be better to have one method to create user, with keyword arguments. And then platform dependent method which will call method to create user with appropriate arguments (or with default arguments) You're right it was ugly. -- David Kupka From 104a196f619d549e3c53fa50df4199535d86fe32 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 22 Oct 2014 09:07:44 -0400 Subject: [PATCH] Respect UID and GID soft static allocation. https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation https://fedorahosted.org/freeipa/ticket/4585 --- ipaplatform/base/tasks.py | 48 +++ ipaplatform/redhat/tasks.py | 22 ++ ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/installutils.py | 42 -- 5 files changed, 72 insertions(+), 44 deletions(-) diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py index 408447e43cd36d0cdf11a1877b3bc9880c4785de..f2ba81f44bb991b218232aad84d7810cdae839ef 100644 --- a/ipaplatform/base/tasks.py +++ b/ipaplatform/base/tasks.py @@ -22,7 +22,13 @@ This module contains default platform-specific implementations of system tasks. ''' +import pwd +import grp from ipaplatform.paths import paths +from ipapython.ipa_log_manager import log_mgr +from ipapython import ipautil + +log = log_mgr.get_logger(__name__) class BaseTaskNamespace(object): @@ -150,5 +156,47 @@ class BaseTaskNamespace(object): return +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): +Create a system user with a corresponding group +try: +grp.getgrnam(group) +except KeyError: +log.debug('Adding group %s', group) +args = [paths.GROUPADD, '-r', group] +if gid: +args += ['-g', str(gid)] +try: +ipautil.run(args) +log.debug('Done adding group') +except ipautil.CalledProcessError as e: +log.critical('Failed to add group: %s', e) +raise +else: +log.debug('group %s exists', group) + +try: +pwd.getpwnam(name) +except KeyError: +log.debug('Adding user %s', name) +args = [ +paths.USERADD, +'-g', group, +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] +if uid: +args += ['-u', str(uid)] +if comment: +args += ['-c', comment] +try: +ipautil.run(args) +log.debug('Done adding user') +except ipautil.CalledProcessError as e: +log.critical('Failed to add user: %s', e) +raise +else: +log.debug('user %s exists', name) + task_namespace = BaseTaskNamespace() diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py index 16d90a6d1a7d3d9aced5de82a5c1efe6b8c2..c37f6e56853382186092d645538e635d49306f87 100644 --- a/ipaplatform/redhat/tasks.py +++ b/ipaplatform/redhat/tasks.py @@ -393,5 +393,27 @@ class RedHatTaskNamespace(BaseTaskNamespace): return True +def create_system_user(self, name, group, homedir, shell): + +Create a system user with a corresponding group + +According to https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation +some system users should have fixed UID, GID and other parameters set. +This values should be constant and may be hardcoded. +Add other values for other users when needed. + +uid = gid = comment = None +if name == 'pkiuser': +uid = 17 +gid = 17 +homedir = paths.VAR_LIB_PKI_DIR +shell = paths.NOLOGIN +comment = 'CA System User' +if name == 'dirsrv': +comment = 'DS System User' + +BaseTaskNamespace.create_system_user(self, name, group, homedir, +shell, uid, gid, comment) + tasks = RedHatTaskNamespace() diff --git a/ipaserver
Re: [Freeipa-devel] [PATCH 0153] fix regression: DNS zonemgr validation raises assertion error
Works for me, ACK. On 10/24/2014 01:27 PM, Martin Basti wrote: https://fedorahosted.org/freeipa/ticket/4663 Patch attached. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0025 Respect UID and GID soft static allocation.
On 10/24/2014 01:06 PM, David Kupka wrote: On 10/24/2014 10:43 AM, Martin Basti wrote: On 24/10/14 09:51, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4585 NACK 1) Why is there line with 'DS System User?' The comment should depend on service. +args = [ +paths.USERADD, +'-g', group, +'-c', 'DS System User', +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] This was part of the original code and I didn't notice it. Nice catch, thanks. 2) code create_system_user is duplicated between base and redhat tasks with platform dependent changes. IMO it would be better to have one method to create user, with keyword arguments. And then platform dependent method which will call method to create user with appropriate arguments (or with default arguments) You're right it was ugly. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I shouldn't break SOLID principles. -- David Kupka From d91e9fedd61780793981f347f529280b86fcca97 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 22 Oct 2014 09:07:44 -0400 Subject: [PATCH] Respect UID and GID soft static allocation. https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation https://fedorahosted.org/freeipa/ticket/4585 --- ipaplatform/base/tasks.py | 48 +++ ipaplatform/redhat/tasks.py | 21 + ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/installutils.py | 42 -- 5 files changed, 71 insertions(+), 44 deletions(-) diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py index 408447e43cd36d0cdf11a1877b3bc9880c4785de..f2ba81f44bb991b218232aad84d7810cdae839ef 100644 --- a/ipaplatform/base/tasks.py +++ b/ipaplatform/base/tasks.py @@ -22,7 +22,13 @@ This module contains default platform-specific implementations of system tasks. ''' +import pwd +import grp from ipaplatform.paths import paths +from ipapython.ipa_log_manager import log_mgr +from ipapython import ipautil + +log = log_mgr.get_logger(__name__) class BaseTaskNamespace(object): @@ -150,5 +156,47 @@ class BaseTaskNamespace(object): return +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): +Create a system user with a corresponding group +try: +grp.getgrnam(group) +except KeyError: +log.debug('Adding group %s', group) +args = [paths.GROUPADD, '-r', group] +if gid: +args += ['-g', str(gid)] +try: +ipautil.run(args) +log.debug('Done adding group') +except ipautil.CalledProcessError as e: +log.critical('Failed to add group: %s', e) +raise +else: +log.debug('group %s exists', group) + +try: +pwd.getpwnam(name) +except KeyError: +log.debug('Adding user %s', name) +args = [ +paths.USERADD, +'-g', group, +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] +if uid: +args += ['-u', str(uid)] +if comment: +args += ['-c', comment] +try: +ipautil.run(args) +log.debug('Done adding user') +except ipautil.CalledProcessError as e: +log.critical('Failed to add user: %s', e) +raise +else: +log.debug('user %s exists', name) + task_namespace = BaseTaskNamespace() diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py index 16d90a6d1a7d3d9aced5de82a5c1efe6b8c2..2c7cafae358f74dc45424f90fc702eef844ac0df 100644 --- a/ipaplatform/redhat/tasks.py +++ b/ipaplatform/redhat/tasks.py @@ -393,5 +393,26 @@ class RedHatTaskNamespace(BaseTaskNamespace): return True +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): + +Create a system user with a corresponding group + +According to https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation +some system users should have fixed UID, GID and other parameters set. +This values should be constant and may be hardcoded. +Add other values for other users when needed. + +if name == 'pkiuser': +uid = 17 +gid = 17 +homedir = paths.VAR_LIB_PKI_DIR +shell = paths.NOLOGIN +comment = 'CA System User
Re: [Freeipa-devel] [PATCH] 333 Handle profile changes in dogtag-ipa-ca-renew-agent
On 10/14/2014 10:52 AM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4627. (The original patch was posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00454.html.) How to test: 1. install server 2. run ipa-certupdate 3. run getcert list -d /etc/pki/pki-tomcat/alias -n 'caSigningCert cert-pki-ca', the request should be in CA_WORKING state 4. run ipa-cacert-manage renew 5. run getcert list -d /etc/pki/pki-tomcat/alias -n 'caSigningCert cert-pki-ca', the request should be in MONITORING state, there should be no ca-error Honza ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0025 Respect UID and GID soft static allocation.
On 10/24/2014 03:05 PM, David Kupka wrote: On 10/24/2014 01:06 PM, David Kupka wrote: On 10/24/2014 10:43 AM, Martin Basti wrote: On 24/10/14 09:51, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4585 NACK 1) Why is there line with 'DS System User?' The comment should depend on service. +args = [ +paths.USERADD, +'-g', group, +'-c', 'DS System User', +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] This was part of the original code and I didn't notice it. Nice catch, thanks. 2) code create_system_user is duplicated between base and redhat tasks with platform dependent changes. IMO it would be better to have one method to create user, with keyword arguments. And then platform dependent method which will call method to create user with appropriate arguments (or with default arguments) You're right it was ugly. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I shouldn't break SOLID principles. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Using super is probably better that explicit naming of parent class. Let user (developer) override UID/GID and hope that he knows why ... -- David Kupka From da0e375ac81d297a78649de7be98e8610aa83dcc Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 22 Oct 2014 09:07:44 -0400 Subject: [PATCH] Respect UID and GID soft static allocation. https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation https://fedorahosted.org/freeipa/ticket/4585 --- ipaplatform/base/tasks.py | 48 +++ ipaplatform/redhat/tasks.py | 21 + ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/installutils.py | 42 -- 5 files changed, 71 insertions(+), 44 deletions(-) diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py index 408447e43cd36d0cdf11a1877b3bc9880c4785de..f2ba81f44bb991b218232aad84d7810cdae839ef 100644 --- a/ipaplatform/base/tasks.py +++ b/ipaplatform/base/tasks.py @@ -22,7 +22,13 @@ This module contains default platform-specific implementations of system tasks. ''' +import pwd +import grp from ipaplatform.paths import paths +from ipapython.ipa_log_manager import log_mgr +from ipapython import ipautil + +log = log_mgr.get_logger(__name__) class BaseTaskNamespace(object): @@ -150,5 +156,47 @@ class BaseTaskNamespace(object): return +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): +Create a system user with a corresponding group +try: +grp.getgrnam(group) +except KeyError: +log.debug('Adding group %s', group) +args = [paths.GROUPADD, '-r', group] +if gid: +args += ['-g', str(gid)] +try: +ipautil.run(args) +log.debug('Done adding group') +except ipautil.CalledProcessError as e: +log.critical('Failed to add group: %s', e) +raise +else: +log.debug('group %s exists', group) + +try: +pwd.getpwnam(name) +except KeyError: +log.debug('Adding user %s', name) +args = [ +paths.USERADD, +'-g', group, +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] +if uid: +args += ['-u', str(uid)] +if comment: +args += ['-c', comment] +try: +ipautil.run(args) +log.debug('Done adding user') +except ipautil.CalledProcessError as e: +log.critical('Failed to add user: %s', e) +raise +else: +log.debug('user %s exists', name) + task_namespace = BaseTaskNamespace() diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py index 16d90a6d1a7d3d9aced5de82a5c1efe6b8c2..7c03fe7cebaa7a5c5ac8715fe23991be7570ea75 100644 --- a/ipaplatform/redhat/tasks.py +++ b/ipaplatform/redhat/tasks.py @@ -393,5 +393,26 @@ class RedHatTaskNamespace(BaseTaskNamespace): return True +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): + +Create a system user with a corresponding group + +According to https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation +some system users should have fixed UID, GID and other
Re: [Freeipa-devel] [PATCH] 0025 Respect UID and GID soft static allocation.
On 10/29/2014 02:34 PM, David Kupka wrote: On 10/24/2014 03:05 PM, David Kupka wrote: On 10/24/2014 01:06 PM, David Kupka wrote: On 10/24/2014 10:43 AM, Martin Basti wrote: On 24/10/14 09:51, David Kupka wrote: https://fedorahosted.org/freeipa/ticket/4585 NACK 1) Why is there line with 'DS System User?' The comment should depend on service. +args = [ +paths.USERADD, +'-g', group, +'-c', 'DS System User', +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] This was part of the original code and I didn't notice it. Nice catch, thanks. 2) code create_system_user is duplicated between base and redhat tasks with platform dependent changes. IMO it would be better to have one method to create user, with keyword arguments. And then platform dependent method which will call method to create user with appropriate arguments (or with default arguments) You're right it was ugly. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I shouldn't break SOLID principles. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Using super is probably better that explicit naming of parent class. Let user (developer) override UID/GID and hope that he knows why ... ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- David Kupka From ec277c8d8d9fc66f31236f306e038d39aa7417fb Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 22 Oct 2014 09:07:44 -0400 Subject: [PATCH] Respect UID and GID soft static allocation. https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation https://fedorahosted.org/freeipa/ticket/4585 --- ipaplatform/base/tasks.py | 48 +++ ipaplatform/redhat/tasks.py | 23 +++ ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/installutils.py | 42 -- 5 files changed, 73 insertions(+), 44 deletions(-) diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py index 408447e43cd36d0cdf11a1877b3bc9880c4785de..f2ba81f44bb991b218232aad84d7810cdae839ef 100644 --- a/ipaplatform/base/tasks.py +++ b/ipaplatform/base/tasks.py @@ -22,7 +22,13 @@ This module contains default platform-specific implementations of system tasks. ''' +import pwd +import grp from ipaplatform.paths import paths +from ipapython.ipa_log_manager import log_mgr +from ipapython import ipautil + +log = log_mgr.get_logger(__name__) class BaseTaskNamespace(object): @@ -150,5 +156,47 @@ class BaseTaskNamespace(object): return +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): +Create a system user with a corresponding group +try: +grp.getgrnam(group) +except KeyError: +log.debug('Adding group %s', group) +args = [paths.GROUPADD, '-r', group] +if gid: +args += ['-g', str(gid)] +try: +ipautil.run(args) +log.debug('Done adding group') +except ipautil.CalledProcessError as e: +log.critical('Failed to add group: %s', e) +raise +else: +log.debug('group %s exists', group) + +try: +pwd.getpwnam(name) +except KeyError: +log.debug('Adding user %s', name) +args = [ +paths.USERADD, +'-g', group, +'-d', homedir, +'-s', shell, +'-M', '-r', name, +] +if uid: +args += ['-u', str(uid)] +if comment: +args += ['-c', comment] +try: +ipautil.run(args) +log.debug('Done adding user') +except ipautil.CalledProcessError as e: +log.critical('Failed to add user: %s', e) +raise +else: +log.debug('user %s exists', name) + task_namespace = BaseTaskNamespace() diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py index 16d90a6d1a7d3d9aced5de82a5c1efe6b8c2..3f5fc90b4b988d44cf0dcaa5a8d569a3c5f453ee 100644 --- a/ipaplatform/redhat/tasks.py +++ b/ipaplatform/redhat/tasks.py @@ -393,5 +393,28 @@ class RedHatTaskNamespace(BaseTaskNamespace): return True +def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None): + +Create a system user with a corresponding
Re: [Freeipa-devel] [PATCH] 335 Fail if certmonger can't see new CA certificate in LDAP in ipa-cacert-manage
On 10/15/2014 04:43 PM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4629. It depends on my patches 333 and 334, which are also attached. (The original patch was posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00454.html.) How to test: 1. install server 2. kinit as admin 3. run ipa-cacert-manage renew --external-ca, it will produce a CSR 4. sign the CSR with some external CA to get new IPA CA certificate 5. run while true; do ldapdelete -H ldap://$HOSTNAME -Y GSSAPI 'cn=caSigningCert cert-pki-ca,cn=ca_renewal,cn=ipa,cn=etc,suffix'; done in background 6. run ipa-cacert-manage renew --external-cert-file=path to new IPA CA certificate --external-cert-file=path to external CA certificate chain 7. stop the loop from step 5 8. run getcert list -d /etc/pki/pki-tomcat/alias -n 'caSigningCert cert-pki-ca', the request should be in MONITORING state, there should be no ca-error Honza ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK. Please push only the patch freeipa-jcholast-335. Patches freeipa-jcholast-333 and freeipa-jcholast-334 was pushed earlier. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0026 Stop dirsrv last in ipactl stop.
https://fedorahosted.org/freeipa/ticket/4632 -- David Kupka From 79a716af3a82e7cb419376c727fd655af070904e Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Tue, 4 Nov 2014 03:22:59 -0500 Subject: [PATCH] Stop dirsrv last in ipactl stop. Other services may depend on directory server. https://fedorahosted.org/freeipa/ticket/4632 --- install/tools/ipactl | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/install/tools/ipactl b/install/tools/ipactl index 7a1e41b01a80eeea85c417399dcf4666f70d4b26..b1b0b6e26fa97cdc953c86eee22e160782b57379 100755 --- a/install/tools/ipactl +++ b/install/tools/ipactl @@ -291,12 +291,6 @@ def ipa_stop(options): finally: raise IpactlError() -try: -print Stopping Directory Service -dirsrv.stop(capture_output=False) -except: -raise IpactlError(Failed to stop Directory Service) - for svc in reversed(svc_list): svchandle = services.service(svc) try: @@ -305,6 +299,12 @@ def ipa_stop(options): except: emit_err(Failed to stop %s Service % svc) +try: +print Stopping Directory Service +dirsrv.stop(capture_output=False) +except: +raise IpactlError(Failed to stop Directory Service) + # remove file with list of started services try: os.unlink(paths.SVC_LIST_FILE) -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Fix installer adds invalid zonemgr email
On 11/07/2014 01:33 PM, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4707 Patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel I think that it would be better to use relative value (e.g.: hostmaster instead of hostmaster.my.example.zone.). But this works, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0027 Produce better error in group-add command.
https://fedorahosted.org/freeipa/ticket/4611 -- David Kupka From a3e735c0309c740186d14f2430bdcf84c7d752b4 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 5 Nov 2014 02:40:10 -0500 Subject: [PATCH] Produce better error in group-add command. https://fedorahosted.org/freeipa/ticket/4611 --- ipalib/plugins/group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py index 03e6893e3c7604268b503b28ea39ed3f610aec47..9f3f1ba16b3db0e04897736979f08ba7a3bdfecd 100644 --- a/ipalib/plugins/group.py +++ b/ipalib/plugins/group.py @@ -287,7 +287,7 @@ class group_add(LDAPCreate): if options['external']: entry_attrs['objectclass'].append('ipaexternalgroup') if 'gidnumber' in options: -raise errors.RequirementError(name='gid') +raise errors.InvocationError(message=_('Invalid combination of parameters: \'gid\', \'external\'')) elif not options['nonposix']: entry_attrs['objectclass'].append('posixgroup') if not 'gidnumber' in options: -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0028 Remove unneeded internal methods. Move code to public, methods.
-- David Kupka From 0269a920231a992b67da713d40e29a28fdd32430 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Thu, 6 Nov 2014 17:57:26 -0500 Subject: [PATCH] Remove unneeded internal methods. Move code to public methods. --- ipaplatform/base/services.py | 36 +--- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py index 961c368e6b4d81d337cf0a8601075f052352ecbf..01d95b39cc7b845bdc612d40b3eea29d6de2961a 100644 --- a/ipaplatform/base/services.py +++ b/ipaplatform/base/services.py @@ -436,7 +436,11 @@ class SystemdService(PlatformService): except: pass else: -self.__disable(instance_name) +try: +ipautil.run([paths.SYSTEMCTL, disable, + self.service_instance(instance_name)]) +except ipautil.CalledProcessError: +pass def mask(self, instance_name=): if instance_name != : @@ -444,40 +448,26 @@ class SystemdService(PlatformService): # remove instance file or link before masking if os.path.islink(srv_tgt): os.unlink(srv_tgt) - -self.__mask(instance_name) - -def unmask(self, instance_name=): -self.__unmask(instance_name) - -def __enable(self, instance_name=): -try: -ipautil.run([paths.SYSTEMCTL, enable, - self.service_instance(instance_name)]) -except ipautil.CalledProcessError: -pass - -def __disable(self, instance_name=): -try: -ipautil.run([paths.SYSTEMCTL, disable, - self.service_instance(instance_name)]) -except ipautil.CalledProcessError: -pass - -def __mask(self, instance_name=): try: ipautil.run([paths.SYSTEMCTL, mask, self.service_instance(instance_name)]) except ipautil.CalledProcessError: pass -def __unmask(self, instance_name=): +def unmask(self, instance_name=): try: ipautil.run([paths.SYSTEMCTL, unmask, self.service_instance(instance_name)]) except ipautil.CalledProcessError: pass +def __enable(self, instance_name=): +try: +ipautil.run([paths.SYSTEMCTL, enable, + self.service_instance(instance_name)]) +except ipautil.CalledProcessError: +pass + def install(self): self.enable() -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0029 Remove service file even if it isn't link.
Depends on freeipa-dkupka-0028. https://fedorahosted.org/freeipa/ticket/4658 -- David Kupka From 247ab543ed26c8eafa471b8b1d38309dacec9dbb Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Thu, 6 Nov 2014 18:08:58 -0500 Subject: [PATCH] Remove service file even if it isn't link. (Link to) service file from /etc/systemd/system/ must be removed before masking systemd service. https://fedorahosted.org/freeipa/ticket/4658 --- ipaplatform/base/services.py | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py index 01d95b39cc7b845bdc612d40b3eea29d6de2961a..c3cd04bd61602bf3eccf5dd1276ff7371a90a768 100644 --- a/ipaplatform/base/services.py +++ b/ipaplatform/base/services.py @@ -443,11 +443,9 @@ class SystemdService(PlatformService): pass def mask(self, instance_name=): -if instance_name != : -srv_tgt = os.path.join(paths.ETC_SYSTEMD_SYSTEM_DIR, instance_name) -# remove instance file or link before masking -if os.path.islink(srv_tgt): -os.unlink(srv_tgt) +srv_tgt = os.path.join(paths.ETC_SYSTEMD_SYSTEM_DIR, self.service_instance(instance_name)) +if os.path.exists(srv_tgt): +os.unlink(srv_tgt) try: ipautil.run([paths.SYSTEMCTL, mask, self.service_instance(instance_name)]) -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0027 Produce better error in group-add command.
On 11/10/2014 08:20 AM, Jan Cholasta wrote: Hi, Dne 7.11.2014 v 15:26 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4611 I think you should use MutuallyExclusiveError. Honza Thanks, that's the error class I was searching for. Unfortunately, I didn't know this one so I used more abstract error class. Fixed patch attached. -- David Kupka From bd7ed8ca7a43a423603d744a6081a49450fdc9af Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 5 Nov 2014 02:40:10 -0500 Subject: [PATCH] Produce better error in group-add command. https://fedorahosted.org/freeipa/ticket/4611 --- ipalib/plugins/group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py index 03e6893e3c7604268b503b28ea39ed3f610aec47..9ac9c7cd3595709d476687ee2efbe342a8ae0f6b 100644 --- a/ipalib/plugins/group.py +++ b/ipalib/plugins/group.py @@ -287,7 +287,7 @@ class group_add(LDAPCreate): if options['external']: entry_attrs['objectclass'].append('ipaexternalgroup') if 'gidnumber' in options: -raise errors.RequirementError(name='gid') +raise errors.MutuallyExclusiveError(message=_('Paramters can not be used together: \'gid\', \'external\'')) elif not options['nonposix']: entry_attrs['objectclass'].append('posixgroup') if not 'gidnumber' in options: -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0158] FIX: upgrade refential integrity plugin configuration
On 11/07/2014 03:22 PM, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4622 Patch attached. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel LGTM, please rebase to ipa-4-0. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0158] FIX: upgrade refential integrity plugin configuration
On 11/13/2014 10:18 AM, Martin Basti wrote: On 12/11/14 16:55, David Kupka wrote: On 11/07/2014 03:22 PM, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4622 Patch attached. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel LGTM, please rebase to ipa-4-0. Thanks. Rebased patch for 4.0 attached. Original patch for 4.1 and master attached. Thanks, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0160] Fix objectclass violation during upgrade
On 11/10/2014 03:21 PM, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4680 Entry dn: cn=adtrust agents,cn=sysaccounts,cn=etc,$SUFFIX must be updated before dn: cn=ADTrust Agents,cn=privileges,cn=pbac,$SUFFIX Patch attached Martin^2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, ACK, thanks. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0027 Produce better error in group-add command.
On 11/13/2014 11:24 AM, Jan Cholasta wrote: Dne 10.11.2014 v 13:24 David Kupka napsal(a): On 11/10/2014 08:20 AM, Jan Cholasta wrote: Hi, Dne 7.11.2014 v 15:26 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4611 I think you should use MutuallyExclusiveError. Honza Thanks, that's the error class I was searching for. Unfortunately, I didn't know this one so I used more abstract error class. Fixed patch attached. Given the messages that are used for MutuallyExclusiveError everywhere else in the framework, I think a better message would be gid cannot be set for external group. Fixed patch attached. -- David Kupka From dddadcc5c3ed5aff3880b7794d6d7ba7aae66d88 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 5 Nov 2014 02:40:10 -0500 Subject: [PATCH] Produce better error in group-add command. https://fedorahosted.org/freeipa/ticket/4611 --- ipalib/plugins/group.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py index 03e6893e3c7604268b503b28ea39ed3f610aec47..d25ed9a1958119a5872db85e958323fdb8205366 100644 --- a/ipalib/plugins/group.py +++ b/ipalib/plugins/group.py @@ -287,7 +287,7 @@ class group_add(LDAPCreate): if options['external']: entry_attrs['objectclass'].append('ipaexternalgroup') if 'gidnumber' in options: -raise errors.RequirementError(name='gid') +raise errors.MutuallyExclusiveError(reason=_('gid cannot be set for external group')) elif not options['nonposix']: entry_attrs['objectclass'].append('posixgroup') if not 'gidnumber' in options: -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0165] --zonemgr options must be unicode
On 11/18/2014 12:07 PM, Martin Basti wrote: On 13/11/14 18:28, Martin Basti wrote: To allow IDNA zonemgr email, value must be unicode not ASCII Ticket: https://fedorahosted.org/freeipa/ticket/4724 Patch attached. Patch for ipa-4.0 added. Thanks, works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 374 Fix wrong expiration date on renewed IPA CA certificates
On 11/19/2014 08:32 AM, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4717. Honza ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, thanks, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0030 Fix --{user, group}-ignore-attribute in migration plugin.
https://fedorahosted.org/freeipa/ticket/4620 -- David Kupka From b6aba1531af03ca3511690548de109d585828486 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 19 Nov 2014 09:57:59 -0500 Subject: [PATCH] Fix --{user,group}-ignore-attribute in migration plugin. Ignore case in attribute names. https://fedorahosted.org/freeipa/ticket/4620 --- ipalib/plugins/migration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py index 6b630a464f0be163e82de95afe3a74b22889574b..57545b594f4ec6c53521abeab339399099d8125e 100644 --- a/ipalib/plugins/migration.py +++ b/ipalib/plugins/migration.py @@ -197,7 +197,7 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs # do not migrate all attributes for attr in entry_attrs.keys(): -if attr in attr_blacklist: +if attr.lower() in attr_blacklist: del entry_attrs[attr] # do not migrate all object classes @@ -394,7 +394,7 @@ def _pre_migrate_group(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwarg # do not migrate all attributes for attr in entry_attrs.keys(): -if attr in attr_blacklist: +if attr.lower() in attr_blacklist: del entry_attrs[attr] # do not migrate all object classes -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0030 Fix --{user, group}-ignore-attribute in migration plugin.
On 11/20/2014 10:03 AM, Jan Cholasta wrote: Dne 20.11.2014 v 09:51 David Kupka napsal(a): https://fedorahosted.org/freeipa/ticket/4620 IMO changing the loop to: for attr in attr_blacklist: entry_attrs.pop(attr, None) would be better, because LDAPEntry already handles case insensitivity in attribute names. This seems better, thanks. -- David Kupka From 94293d14e51507819c4296c52d5d8ce4def9a4c8 Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Wed, 19 Nov 2014 09:57:59 -0500 Subject: [PATCH] Fix --{user,group}-ignore-attribute in migration plugin. Ignore case in attribute names. https://fedorahosted.org/freeipa/ticket/4620 --- ipalib/plugins/migration.py | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py index 6b630a464f0be163e82de95afe3a74b22889574b..fa3d512bf1434c7d349713f78c292b481021303a 100644 --- a/ipalib/plugins/migration.py +++ b/ipalib/plugins/migration.py @@ -196,9 +196,8 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs entry_attrs.setdefault('loginshell', default_shell) # do not migrate all attributes -for attr in entry_attrs.keys(): -if attr in attr_blacklist: -del entry_attrs[attr] +for attr in attr_blacklist: +entry_attrs.pop(attr, None) # do not migrate all object classes if 'objectclass' in entry_attrs: @@ -393,9 +392,8 @@ def _pre_migrate_group(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwarg raise ValueError('Schema %s not supported' % schema) # do not migrate all attributes -for attr in entry_attrs.keys(): -if attr in attr_blacklist: -del entry_attrs[attr] +for attr in attr_blacklist: +entry_attrs.pop(attr, None) # do not migrate all object classes if 'objectclass' in entry_attrs: -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0163] Fix compiler warning for pk11helper module
On 11/13/2014 09:59 AM, Martin Basti wrote: On 12/11/14 15:55, Martin Basti wrote: Part of ticket: https://fedorahosted.org/freeipa/ticket/4657 And here is the patch, sorry :-) ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Works for me, thanks, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0031 ipa-restore: Check if directory is provided + better errors.
https://fedorahosted.org/freeipa/ticket/4683 -- David Kupka From f5f5ae55999b2057549306331b07a3c41c0cabeb Mon Sep 17 00:00:00 2001 From: David Kupka dku...@redhat.com Date: Fri, 21 Nov 2014 06:30:17 -0500 Subject: [PATCH] ipa-restore: Check if directory is provided + better errors. https://fedorahosted.org/freeipa/ticket/4683 --- ipaserver/install/ipa_restore.py | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py index 93f176d302a49319940555a0be3037620143e1f3..c634588b5bc5a87896244166a9f8f7a571b5911f 100644 --- a/ipaserver/install/ipa_restore.py +++ b/ipaserver/install/ipa_restore.py @@ -152,6 +152,9 @@ class Restore(admintool.AdminTool): else: self.backup_dir = dirname +if not os.path.isdir(dirname): +raise self.option_parser.error(must provide path to backup directory) + if options.gpg_keyring: if (not os.path.exists(options.gpg_keyring + '.pub') or not os.path.exists(options.gpg_keyring + '.sec')): @@ -213,7 +216,10 @@ class Restore(admintool.AdminTool): try: dirsrv = services.knownservices.dirsrv -self.read_header() +try: +self.read_header() +except: +raise admintool.ScriptError('Cannot read backup metadata') # These two checks would normally be in the validate method but # we need to know the type of backup we're dealing with. if (self.backup_type != 'FULL' and not options.data_only and -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel