Re: [Freeipa-devel] [PATCH] ipatests: port of p11helper test from github
On 03/16/2015 12:03 PM, Milan Kubik wrote:
On 03/13/2015 02:59 PM, Milan Kubik wrote:
Hi,
this is a patch with port of [1] to pytest.
[1]:
https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py
Cheers,
Milan
Added few more asserts in methods where the test could fail and cause
other errors.
New version of the patch after brief discussion with Martin Basti.
Removed unnecessary variable assignments and separated a new test case.
From d231c1fa215f0eb7ca43750d5b85c9c745b078d0 Mon Sep 17 00:00:00 2001
From: Milan Kubik mku...@redhat.com
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github
Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.
[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py
https://fedorahosted.org/freeipa/ticket/4829
---
ipatests/test_ipapython/test_ipap11helper.py | 220 +++
make-lint| 2 +-
2 files changed, 221 insertions(+), 1 deletion(-)
create mode 100644 ipatests/test_ipapython/test_ipap11helper.py
diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index ..3f55156dc2570f005d4744463cae76f9a420bd01
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,220 @@
+# -*- coding: utf-8 -*-
+# Authors:
+# Milan Kubik mku...@redhat.com
+#
+# Copyright (C) 2015 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/.
+
+Test the `ipapython/ipap11helper/p11helper.c` module.
+
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import sys
+import subprocess
+import tempfile
+
+import pytest
+
+import _ipap11helper
+
+config_data =
+# SoftHSM v2 configuration file
+directories.tokendir = %s/tokens
+objectstore.backend = file
+
+
+libsofthsm = /usr/lib64/pkcs11/libsofthsm2.so
+
+logging.basicConfig(level=logging.INFO)
+log = logging.getLogger('t')
+
+@pytest.fixture(scope=module)
+def p11(request):
+token_path = tempfile.mkdtemp(prefix='pytest_', suffix='_pkcs11')
+os.chdir(token_path)
+os.mkdir('tokens')
+
+with open('softhsm2.conf', 'w') as cfg:
+cfg.write(config_data % token_path)
+
+os.environ['SOFTHSM2_CONF'] = os.path.join(token_path, 'softhsm2.conf')
+
+subprocess.check_call(['softhsm2-util', '--init-token', '--slot', '0',
+ '--label', 'test', '--pin', '1234', '--so-pin', '1234'])
+
+try:
+p11 = _ipap11helper.P11_Helper(0, 1234, libsofthsm)
+except _ipap11helper.Error:
+pytest.fail('Failed to initialize the helper object.', pytrace=False)
+
+def fin():
+try:
+p11.finalize()
+except _ipap11helper.Error:
+pytest.fail('Failed to finalize the helper object.', pytrace=False)
+finally:
+del os.environ['SOFTHSM2_CONF']
+
+request.addfinalizer(fin)
+
+return p11
+
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)
+
+class test_p11helper(object):
+def test_generate_master_key(self, p11):
+assert p11.generate_master_key(užžž-aest, m, key_length=16, cka_wrap=True,
+ cka_unwrap=True)
+
+# replica 1
+def test_generate_replica_key_pair(self, p11):
+assert p11.generate_replica_key_pair(ureplica1, id1, pub_cka_wrap=True,
+ priv_cka_unwrap=True)
+
+def test_find_key(self, p11):
+rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY, label=ureplica1, cka_wrap=True)
+assert len(rep1_pub) == 1, replica key pair has to contain 1 pub key instead of %s % len(rep1_pub)
+
+rep1_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY, label=ureplica1, cka_unwrap=True)
+assert len(rep1_priv) == 1, replica key pair has to contain 1 private key instead of %s % len(rep1_priv)
+
+def test_find_key_by_uri(self, p11):
+rep1_pub = p11.find_keys(uri=pkcs11:object=replica1;objecttype=public)
+assert len(rep1_pub) == 1, replica key pair has to contain 1 pub key instead of %s % len(rep1_pub)
+
+def test_get_attribute_from_object(self, p11):
+
Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code
On 03/13/2015 05:37 PM, Martin Babinsky wrote: Attaching the next iteration of patches. I have tried my best to reword the ipa-client-install man page bit about the new option. Any suggestions to further improve it are welcome. I have also slightly modified the 'kinit_keytab' function so that in Kerberos errors are reported for each attempt and the text of the last error is retained when finally raising exception. The approach looks very good. I think that my only concern with this patch is this part: +ccache.init_creds_keytab(keytab=ktab, principal=princ) ... +except krbV.Krb5Error as e: +last_exc = str(e) +root_logger.debug(Attempt %d/%d: failed: %s + % (attempt, attempts, last_exc)) +time.sleep(1) + +root_logger.debug(Maximum number of attempts (%d) reached + % attempts) +raise StandardError(Error initializing principal %s: %s +% (principal, last_exc)) The problem here is that this function will raise the super-generic StandardError instead of the proper with all the context and information about the error that the caller can then process. I think that except krbV.Krb5Error as e: if attempt == max_attempts: log something raise would be better. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0018-0019] ipa-dns-install: Use LDAPI for all DS connections
On 12/03/15 17:15, Martin Babinsky wrote: On 03/12/2015 03:59 PM, Martin Babinsky wrote: On 03/11/2015 03:13 PM, Martin Basti wrote: On 11/03/15 13:00, Martin Babinsky wrote: These patches solve https://fedorahosted.org/freeipa/ticket/4933. They are to be applied to master branch. I will rebase them for ipa-4-1 after the review. Thank you for the patches. I have a few comments: IPA-4-1 Replace simple bind with LDAPI is too big change for 4-1, we should start TLS if possible to avoid MINSSF0 error. The LDAPI patches should go only into IPA master branch. You can do something like this: --- a/ipaserver/install/service.py +++ b/ipaserver/install/service.py @@ -107,6 +107,10 @@ class Service(object): if not self.realm: raise errors.NotFound(reason=realm is missing for %s % (self)) conn = ipaldap.IPAdmin(ldapi=self.ldapi, realm=self.realm) +elif self.dm_password is not None: +conn = ipaldap.IPAdmin(self.fqdn, port=389, + cacert=paths.IPA_CA_CRT, + start_tls=True) else: conn = ipaldap.IPAdmin(self.fqdn, port=389) PATCH 0018: 1) please add there more chatty commit message about using LDAPI 2) I do not like much idea of adding 'realm' kwarg into __init__ method of OpenDNSSECInstance IIUC, it is because get_masters() method, which requires realm to use LDAPI. You can just add ods.realm=realm, before call get_master() in ipa-dns-install if options.dnssec_master: +ods.realm=api.env.realm dnssec_masters = ods.get_masters() (Honza will change it anyway during refactoring) PATCH 0019: 1) commit message deserves to be more chatty, can you explain there why you removed kerberos cache? Martin^2 Attaching updated patches. Patch 0018 should go to both 4.1 and master branches. Patch 0019 should go only to master. One more update. Patch 0018 is for both 4.1 and master. Patch 0019 is for master only. Thank for patches Patch 0018: 1) Works for me but needs rebase on master Patch 0019: 1) Please rename the patch/commit message, the patch changes only ipa-dns-install connections not all DS operations 2) I have some troubles with applying patch, it needs rebase due 0018 -- Martin Basti -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH] 0041 Always reload StateFile before getting or modifying the, stored values.
https://fedorahosted.org/freeipa/ticket/4901
--
David Kupka
From f7c47015b40141e0d1c5d93add4b7293a95cd830 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Mon, 16 Mar 2015 08:05:59 -0400
Subject: [PATCH] Always reload StateFile before getting or modifying the
stored values.
This change does not solve using multiple instances of StateFile concurently
because there is no use for it in near future. Instead this solves an issue of loosing
records when more instances of StateFile are interleaved in sequential code.
https://fedorahosted.org/freeipa/ticket/4901
---
ipapython/sysrestore.py | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/ipapython/sysrestore.py b/ipapython/sysrestore.py
index 6db33a7efe944baca5838264040d71cd06e6129c..580df9a4fd6d0fae35602dad1f81d498fa8f0173 100644
--- a/ipapython/sysrestore.py
+++ b/ipapython/sysrestore.py
@@ -283,8 +283,11 @@ class FileStore:
class StateFile:
A metadata file for recording system state which can
-be backed up and later restored. The format is something
-like:
+be backed up and later restored.
+StateFile gets reloaded every time to prevent loss of information
+recorded by child processes. But we do not solve concurrency
+because there is no need for it right now.
+The format is something like:
[httpd]
running=True
@@ -363,6 +366,8 @@ class StateFile:
if not isinstance(value, (str, bool, unicode)):
raise ValueError(Only strings, booleans or unicode strings are supported)
+self._load()
+
if not self.modules.has_key(module):
self.modules[module] = {}
@@ -378,6 +383,8 @@ class StateFile:
If the item doesn't exist, #None will be returned, otherwise
the original string or boolean value is returned.
+self._load()
+
if not self.modules.has_key(module):
return None
@@ -389,6 +396,8 @@ class StateFile:
If the item doesn't exist, no change is done.
+self._load()
+
try:
del self.modules[module][key]
except KeyError:
--
2.1.0
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0018-0020] ipa-dns-install: Use LDAPI for all DS connections
On 03/16/2015 01:44 PM, Martin Basti wrote: On 12/03/15 17:15, Martin Babinsky wrote: On 03/12/2015 03:59 PM, Martin Babinsky wrote: On 03/11/2015 03:13 PM, Martin Basti wrote: On 11/03/15 13:00, Martin Babinsky wrote: These patches solve https://fedorahosted.org/freeipa/ticket/4933. They are to be applied to master branch. I will rebase them for ipa-4-1 after the review. Thank you for the patches. I have a few comments: IPA-4-1 Replace simple bind with LDAPI is too big change for 4-1, we should start TLS if possible to avoid MINSSF0 error. The LDAPI patches should go only into IPA master branch. You can do something like this: --- a/ipaserver/install/service.py +++ b/ipaserver/install/service.py @@ -107,6 +107,10 @@ class Service(object): if not self.realm: raise errors.NotFound(reason=realm is missing for %s % (self)) conn = ipaldap.IPAdmin(ldapi=self.ldapi, realm=self.realm) +elif self.dm_password is not None: +conn = ipaldap.IPAdmin(self.fqdn, port=389, + cacert=paths.IPA_CA_CRT, + start_tls=True) else: conn = ipaldap.IPAdmin(self.fqdn, port=389) PATCH 0018: 1) please add there more chatty commit message about using LDAPI 2) I do not like much idea of adding 'realm' kwarg into __init__ method of OpenDNSSECInstance IIUC, it is because get_masters() method, which requires realm to use LDAPI. You can just add ods.realm=realm, before call get_master() in ipa-dns-install if options.dnssec_master: +ods.realm=api.env.realm dnssec_masters = ods.get_masters() (Honza will change it anyway during refactoring) PATCH 0019: 1) commit message deserves to be more chatty, can you explain there why you removed kerberos cache? Martin^2 Attaching updated patches. Patch 0018 should go to both 4.1 and master branches. Patch 0019 should go only to master. One more update. Patch 0018 is for both 4.1 and master. Patch 0019 is for master only. Thank for patches Patch 0018: 1) Works for me but needs rebase on master Patch 0019: 1) Please rename the patch/commit message, the patch changes only ipa-dns-install connections not all DS operations 2) I have some troubles with applying patch, it needs rebase due 0018 -- Martin Basti Attaching updated patches: Patch 0018 is for ipa-4-1 branch. Patches 0019 and 0020 are for master branch. I hope they will apply cleanly this time (they did for me). -- Martin^3 Babinsky From b8fa778811cdde75da7faa5a2bc37a20655db372 Mon Sep 17 00:00:00 2001 From: Martin Babinsky mbabi...@redhat.com Date: Thu, 12 Mar 2015 16:14:22 +0100 Subject: [PATCH] ipa-dns-install: use STARTTLS to connect to DS BindInstance et al. now use STARTTLS to set up secure connection to DS during ipa-dns-install. This fixes https://fedorahosted.org/freeipa/ticket/4933 --- install/tools/ipa-dns-install| 12 ipaserver/install/bindinstance.py| 10 ++ ipaserver/install/dnskeysyncinstance.py | 7 --- ipaserver/install/odsexporterinstance.py | 5 +++-- ipaserver/install/opendnssecinstance.py | 5 +++-- ipaserver/install/service.py | 10 -- 6 files changed, 32 insertions(+), 17 deletions(-) diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install index 11f79f0f9be226aaee8c95deb31e7e21f8a18dbb..2b6ad02abee9428870b0a554f5b4088e77b0e852 100755 --- a/install/tools/ipa-dns-install +++ b/install/tools/ipa-dns-install @@ -151,7 +151,7 @@ def main(): confirm=False, validate=False) if dm_password is None: sys.exit(Directory Manager password required) -bind = bindinstance.BindInstance(fstore, dm_password) +bind = bindinstance.BindInstance(fstore, dm_password, start_tls=True) # try the connection try: @@ -160,7 +160,8 @@ def main(): except errors.ACIError: sys.exit(Password is not valid!) -ods = opendnssecinstance.OpenDNSSECInstance(fstore, dm_password) +ods = opendnssecinstance.OpenDNSSECInstance(fstore, dm_password, +start_tls=True) if options.dnssec_master: dnssec_masters = ods.get_masters() # we can reinstall current server if it is dnssec master @@ -214,10 +215,13 @@ def main(): bind.create_instance() # on dnssec master this must be installed last -dnskeysyncd = dnskeysyncinstance.DNSKeySyncInstance(fstore, dm_password) +dnskeysyncd = dnskeysyncinstance.DNSKeySyncInstance(fstore, dm_password, +start_tls=True) dnskeysyncd.create_instance(api.env.host, api.env.realm) if options.dnssec_master: -ods_exporter = odsexporterinstance.ODSExporterInstance(fstore, dm_password) +ods_exporter = odsexporterinstance.ODSExporterInstance(fstore, +
Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code
On 03/16/2015 12:15 PM, Martin Kosek wrote: On 03/13/2015 05:37 PM, Martin Babinsky wrote: Attaching the next iteration of patches. I have tried my best to reword the ipa-client-install man page bit about the new option. Any suggestions to further improve it are welcome. I have also slightly modified the 'kinit_keytab' function so that in Kerberos errors are reported for each attempt and the text of the last error is retained when finally raising exception. The approach looks very good. I think that my only concern with this patch is this part: +ccache.init_creds_keytab(keytab=ktab, principal=princ) ... +except krbV.Krb5Error as e: +last_exc = str(e) +root_logger.debug(Attempt %d/%d: failed: %s + % (attempt, attempts, last_exc)) +time.sleep(1) + +root_logger.debug(Maximum number of attempts (%d) reached + % attempts) +raise StandardError(Error initializing principal %s: %s +% (principal, last_exc)) The problem here is that this function will raise the super-generic StandardError instead of the proper with all the context and information about the error that the caller can then process. I think that except krbV.Krb5Error as e: if attempt == max_attempts: log something raise would be better. Yes that seems reasonable. I'm just thinking whether we should re-raise Krb5Error or raise ipalib.errors.KerberosError? the latter options makes more sense to me as we would not have to additionally import Krb5Error everywhere and it would also make the resulting errors more consistent. I am thinking about someting like this: except krbV.Krb5Error as e: if attempt == attempts: # log that we have reaches maximum number of attempts raise KerberosError(minor=str(e)) What do you think? -- Martin^3 Babinsky -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
On 03/06/2015 07:30 PM, thierry bordaz wrote: On 02/19/2015 04:19 PM, Martin Basti wrote: On 19/02/15 13:01, thierry bordaz wrote: On 02/04/2015 05:14 PM, Jan Cholasta wrote: Hi, Dne 4.2.2015 v 15:25 David Kupka napsal(a): On 02/03/2015 11:50 AM, thierry bordaz wrote: On 09/17/2014 12:32 PM, thierry bordaz wrote: On 09/01/2014 01:08 PM, Petr Viktorin wrote: On 08/08/2014 03:54 PM, thierry bordaz wrote: Hi, The attached patch is related to 'User Life Cycle' (https://fedorahosted.org/freeipa/ticket/3813) It creates a stageuser plugin with a first function stageuser-add. Stage user entries are provisioned under 'cn=staged users,cn=accounts,cn=provisioning,SUFFIX'. Thanks thierry Avoid `from ipalib.plugins.baseldap import *` in new code; instead import the module itself and use e.g. `baseldap.LDAPObject`. The stageuser help (docstring) is copied from the user plugin, and discusses things like account lockout and disabling users. It should rather explain what stageuser itself does. (And I don't very much like the Note about the interface being badly designed...) Also decide if the docs should call it staged user or stage user or stageuser. A lot of the code is copied and pasted over from the users plugin. Don't do that. Either import things (e.g. validate_nsaccountlock) from the users plugin, or move the reused code into a shared module. For the `user` object, since so much is the same, it might be best to create a common base class for user and stageuser; and similarly for the Command plugins. The default permissions need different names, and you don't need another copy of the 'non_object' ones. Also, run the makeaci script. Hello, This modified patch is mainly moving common base class into a new plugin: accounts.py. user/stageuser plugin inherits from accounts. It also creates a better description of what are stage user, how to add a new stage user, updates ACI.txt and separate active/stage user managed permissions. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Thanks David for the reviews. Here the last patches ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel The freeipa-tbordaz-0002 patch had trailing whitespaces on few lines so I'm attaching fixed version (and unchanged patch freeipa-tbordaz-0003-3 to keep them together). The ULC feature is still WIP but these patches look good to me and don't break anything as far as I tested. We should push them now to avoid further rebases. Thierry can then prepare other patches delivering the rest of ULC functionality. Few comments from just reading the patches: 1) I would name the base class baseuser, account does not necessarily mean user account. 2) This is very wrong: -class user_add(LDAPCreate): +class user_add(user, LDAPCreate): You are creating a plugin which is both an object and an command. 3) This is purely subjective, but I don't like the name deleteuser, as it has a verb in it. We usually don't do that and IMHO we shouldn't do that. Honza Thank you for the review. I am attaching the updates patches ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, I'm getting errors during make rpms: if [ != yes ]; then \ ./makeapi --validate; \ ./makeaci --validate; \ fi /root/freeipa/ipalib/plugins/baseuser.py:641 command baseuser_add doc is not internationalized /root/freeipa/ipalib/plugins/baseuser.py:653 command baseuser_find doc is not internationalized /root/freeipa/ipalib/plugins/baseuser.py:647 command baseuser_mod doc is not internationalized 0 commands without doc, 3 commands whose doc is not i18n Command baseuser_add in ipalib, not in API Command baseuser_find in ipalib, not in API Command baseuser_mod in ipalib, not in API There are one or more new commands defined. Update API.txt and increment the minor version in VERSION. There are one or more documentation problems. You must fix these before preceeding Issues probably caused by this: 1) You should not use the register decorator, if this class is just for inheritance @register() class baseuser_add(LDAPCreate): @register() class baseuser_mod(LDAPUpdate): @register() class baseuser_find(LDAPSearch): see dns.py plugin and DNSZoneBase and dnszone classes 2) there might be an issue with @register() class baseuser(LDAPObject): the register decorator should not be there, I was warned by Petr^3 to not use permission in parent class. The same permission should be specified only in one place (for example user class), (otherwise they will be generated twice??) I don't know more details about it. -- Martin Basti Hello Martin, Jan, Thanks for your review. I changed the patch so that it
Re: [Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes
On 03/13/2015 10:13 AM, Martin Kosek wrote: On 03/12/2015 09:43 PM, Nathan Kinder wrote: On 03/04/2015 11:25 AM, Nathan Kinder wrote: On 03/04/2015 10:58 AM, Martin Basti wrote: On 04/03/15 19:56, Nathan Kinder wrote: On 03/04/2015 10:41 AM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/28/2015 01:13 PM, Nathan Kinder wrote: On 02/28/2015 01:07 PM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/27/2015 01:18 PM, Nathan Kinder wrote: On 02/27/2015 01:08 PM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/27/2015 12:20 PM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/26/2015 12:55 AM, Martin Kosek wrote: On 02/26/2015 03:28 AM, Nathan Kinder wrote: Hi, The two attached patches address some issues that affect ipa-client-install when syncing time from the NTP server. Now that we use ntpd to perform the time sync, the client install can end up hanging forever when the server is not reachable (firewall issues, etc.). These patches address the issues in two different ways: 1 - Don't attempt to sync time when --no-ntp is specified. 2 - Implement a timeout capability that is used when we run ntpd to perform the time sync to prevent indefinite hanging. The one potentially contentious issue is that this introduces a new dependency on python-subprocess32 to allow us to have timeout support when using Python 2.x. This is packaged for Fedora, but I don't see it on RHEL or CentOS currently. It would need to be packaged there. https://fedorahosted.org/freeipa/ticket/4842 Thanks, -NGK Thanks for Patches. For the second patch, I would really prefer to avoid new dependency, especially if it's not packaged in RHEL/CentOS. Maybe we could use some workaround instead, as in: http://stackoverflow.com/questions/3733270/python-subprocess-timeout I don't like having to add an additional dependency either, but the alternative seems more risky. Utilizing the subprocess32 module (which is really just a backport of the normal subprocess module from Python 3.x) is not invasive for our code in ipautil.run(). Adding some sort of a thread that has to kill the spawned subprocess seems more risky (see the discussion about a race condition in the stackoverflow thread above). That said, I'm sure the thread/poll method can be made to work if you and others feel strongly that this is a better approach than adding a new dependency. Why not use /usr/bin/timeout from coreutils? That sounds like a perfectly good idea. I wasn't aware of it's existence (or it's possible that I forgot about it). Thanks for the suggestion! I'll test out a reworked version of the patch. Do you think that there is value in leaving the timeout capability centrally in ipautil.run()? We only need it for the call to 'ntpd' right now, but there might be a need for using a timeout for other commands in the future. The alternative is to just modify synconce_ntp() to use /usr/bin/timeout and leave ipautil.run() alone. I think it would require a lot of research. One of the programs spawned by this is pkicreate which could take quite some time, and spawning a clone in particular. It is definitely an interesting idea but I think it is safest for now to limit it to just NTP for now. What I meant was that we would have an optional keyword timeout parameter to ipautil.run() that defaults to None, just like my subprocess32 approach. If a timeout is not passed in, we would use subprocess.Popen() to run the specified command just like we do today. We would only actually pass the timeout parameter to ipautil.run() in synconce_ntp() for now, so no other commands would have a timeout in effect. The capability would be available for other commands this way though. Let me propose a patch with this implementation, and if you don't like it, we can leave ipautil.run() alone and restrict the changes to synconce_ntp(). An updated patch 0002 is attached that uses the approach mentioned above. Looks good. Not to nitpick to death but... Can you add timeout to ipaplatform/base/paths.py as BIN_TIMEOUT = /usr/bin/timeout and reference that instead? It's for portability. Sure. I was wondering if we should do something around a full path. And a question. I'm impatient. Should there be a notice that it will timeout after n seconds somewhere so people like me don't ^C after 2 seconds? Or is that just overkill and I need to learn patience? Probably both. :) There's always going to be someone out there who will do ctrl-C, so I think printing out a notice is a good idea. I'll add this. Stylistically, should we prefer p.returncode is 15 or p.returncode == 15? After some reading, it seems that '==' should be used. Small integers work with 'is', but '==' is the consistent way that equality of integers should be checked. I'll modify this. Another updated patch 0002 is attached that addresses Rob's review comments. Thanks, -NGK LGTM. Does someone else have time to test this? I also don't know if there is a policy on placement
Re: [Freeipa-devel] [PATCHES 0018-0020] ipa-dns-install: Use LDAPI for all DS connections
On 16/03/15 14:26, Martin Babinsky wrote: On 03/16/2015 01:44 PM, Martin Basti wrote: On 12/03/15 17:15, Martin Babinsky wrote: On 03/12/2015 03:59 PM, Martin Babinsky wrote: On 03/11/2015 03:13 PM, Martin Basti wrote: On 11/03/15 13:00, Martin Babinsky wrote: These patches solve https://fedorahosted.org/freeipa/ticket/4933. They are to be applied to master branch. I will rebase them for ipa-4-1 after the review. Thank you for the patches. I have a few comments: IPA-4-1 Replace simple bind with LDAPI is too big change for 4-1, we should start TLS if possible to avoid MINSSF0 error. The LDAPI patches should go only into IPA master branch. You can do something like this: --- a/ipaserver/install/service.py +++ b/ipaserver/install/service.py @@ -107,6 +107,10 @@ class Service(object): if not self.realm: raise errors.NotFound(reason=realm is missing for %s % (self)) conn = ipaldap.IPAdmin(ldapi=self.ldapi, realm=self.realm) +elif self.dm_password is not None: +conn = ipaldap.IPAdmin(self.fqdn, port=389, + cacert=paths.IPA_CA_CRT, + start_tls=True) else: conn = ipaldap.IPAdmin(self.fqdn, port=389) PATCH 0018: 1) please add there more chatty commit message about using LDAPI 2) I do not like much idea of adding 'realm' kwarg into __init__ method of OpenDNSSECInstance IIUC, it is because get_masters() method, which requires realm to use LDAPI. You can just add ods.realm=realm, before call get_master() in ipa-dns-install if options.dnssec_master: +ods.realm=api.env.realm dnssec_masters = ods.get_masters() (Honza will change it anyway during refactoring) PATCH 0019: 1) commit message deserves to be more chatty, can you explain there why you removed kerberos cache? Martin^2 Attaching updated patches. Patch 0018 should go to both 4.1 and master branches. Patch 0019 should go only to master. One more update. Patch 0018 is for both 4.1 and master. Patch 0019 is for master only. Thank for patches Patch 0018: 1) Works for me but needs rebase on master Patch 0019: 1) Please rename the patch/commit message, the patch changes only ipa-dns-install connections not all DS operations 2) I have some troubles with applying patch, it needs rebase due 0018 -- Martin Basti Attaching updated patches: Patch 0018 is for ipa-4-1 branch. Patches 0019 and 0020 are for master branch. I hope they will apply cleanly this time (they did for me). ACK -- Martin Basti -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] ipatests: port of p11helper test from github
On 16/03/15 15:32, Milan Kubik wrote:
On 03/16/2015 12:03 PM, Milan Kubik wrote:
On 03/13/2015 02:59 PM, Milan Kubik wrote:
Hi,
this is a patch with port of [1] to pytest.
[1]:
https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py
Cheers,
Milan
Added few more asserts in methods where the test could fail and cause
other errors.
New version of the patch after brief discussion with Martin Basti.
Removed unnecessary variable assignments and separated a new test case.
Hello,
thank you for the patch.
I have a few nitpicks:
1)
You can remove this and use just hexlify(s)
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)
2)
+ def test_find_secret_key(self, p11):
+ assert p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
label=užžž-aest)
In tests before you tested the exact number of expected IDs returned by
find_keys method, why not here?
Martin^2
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes
On 03/16/2015 01:56 PM, Martin Babinsky wrote: On 03/13/2015 10:13 AM, Martin Kosek wrote: On 03/12/2015 09:43 PM, Nathan Kinder wrote: On 03/04/2015 11:25 AM, Nathan Kinder wrote: On 03/04/2015 10:58 AM, Martin Basti wrote: On 04/03/15 19:56, Nathan Kinder wrote: On 03/04/2015 10:41 AM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/28/2015 01:13 PM, Nathan Kinder wrote: On 02/28/2015 01:07 PM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/27/2015 01:18 PM, Nathan Kinder wrote: On 02/27/2015 01:08 PM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/27/2015 12:20 PM, Rob Crittenden wrote: Nathan Kinder wrote: On 02/26/2015 12:55 AM, Martin Kosek wrote: On 02/26/2015 03:28 AM, Nathan Kinder wrote: Hi, The two attached patches address some issues that affect ipa-client-install when syncing time from the NTP server. Now that we use ntpd to perform the time sync, the client install can end up hanging forever when the server is not reachable (firewall issues, etc.). These patches address the issues in two different ways: 1 - Don't attempt to sync time when --no-ntp is specified. 2 - Implement a timeout capability that is used when we run ntpd to perform the time sync to prevent indefinite hanging. The one potentially contentious issue is that this introduces a new dependency on python-subprocess32 to allow us to have timeout support when using Python 2.x. This is packaged for Fedora, but I don't see it on RHEL or CentOS currently. It would need to be packaged there. https://fedorahosted.org/freeipa/ticket/4842 Thanks, -NGK Thanks for Patches. For the second patch, I would really prefer to avoid new dependency, especially if it's not packaged in RHEL/CentOS. Maybe we could use some workaround instead, as in: http://stackoverflow.com/questions/3733270/python-subprocess-timeout I don't like having to add an additional dependency either, but the alternative seems more risky. Utilizing the subprocess32 module (which is really just a backport of the normal subprocess module from Python 3.x) is not invasive for our code in ipautil.run(). Adding some sort of a thread that has to kill the spawned subprocess seems more risky (see the discussion about a race condition in the stackoverflow thread above). That said, I'm sure the thread/poll method can be made to work if you and others feel strongly that this is a better approach than adding a new dependency. Why not use /usr/bin/timeout from coreutils? That sounds like a perfectly good idea. I wasn't aware of it's existence (or it's possible that I forgot about it). Thanks for the suggestion! I'll test out a reworked version of the patch. Do you think that there is value in leaving the timeout capability centrally in ipautil.run()? We only need it for the call to 'ntpd' right now, but there might be a need for using a timeout for other commands in the future. The alternative is to just modify synconce_ntp() to use /usr/bin/timeout and leave ipautil.run() alone. I think it would require a lot of research. One of the programs spawned by this is pkicreate which could take quite some time, and spawning a clone in particular. It is definitely an interesting idea but I think it is safest for now to limit it to just NTP for now. What I meant was that we would have an optional keyword timeout parameter to ipautil.run() that defaults to None, just like my subprocess32 approach. If a timeout is not passed in, we would use subprocess.Popen() to run the specified command just like we do today. We would only actually pass the timeout parameter to ipautil.run() in synconce_ntp() for now, so no other commands would have a timeout in effect. The capability would be available for other commands this way though. Let me propose a patch with this implementation, and if you don't like it, we can leave ipautil.run() alone and restrict the changes to synconce_ntp(). An updated patch 0002 is attached that uses the approach mentioned above. Looks good. Not to nitpick to death but... Can you add timeout to ipaplatform/base/paths.py as BIN_TIMEOUT = /usr/bin/timeout and reference that instead? It's for portability. Sure. I was wondering if we should do something around a full path. And a question. I'm impatient. Should there be a notice that it will timeout after n seconds somewhere so people like me don't ^C after 2 seconds? Or is that just overkill and I need to learn patience? Probably both. :) There's always going to be someone out there who will do ctrl-C, so I think printing out a notice is a good idea. I'll add this. Stylistically, should we prefer p.returncode is 15 or p.returncode == 15? After some reading, it seems that '==' should be used. Small integers work with 'is', but '==' is the consistent way that equality of integers should be checked. I'll modify this. Another updated patch 0002 is attached that
[Freeipa-devel] [PATCH 0021] show the exception message thrown by dogtag._parse_ca_status during install
https://fedorahosted.org/freeipa/ticket/4885
--
Martin^3 Babinsky
From 7e0f8b4d65f6c3f8c7d14f154aa5ef80bb064c4c Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Mon, 16 Mar 2015 12:36:25 +0100
Subject: [PATCH] show the exception message thrown by dogtag._parse_ca_status
during install
https://fedorahosted.org/freeipa/ticket/4885
---
ipaplatform/redhat/services.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 8759cab76c7d72a3abbf935e7f15f7a32a0b6987..c9994e409a8a005012c0467c016608b8f689eef1 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -212,8 +212,8 @@ class RedHatCAService(RedHatService):
status = dogtag._parse_ca_status(stdout)
# end of workaround
-except Exception:
-status = 'check interrupted'
+except Exception as e:
+status = 'check interrupted due to error: %s' % e
root_logger.debug('The CA status is: %s' % status)
if status == 'running':
break
--
2.1.0
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0021] how the exception message raised by dogtag._parse_ca_status during install
https://fedorahosted.org/freeipa/ticket/4885
--
Martin^3 Babinsky
From 7e0f8b4d65f6c3f8c7d14f154aa5ef80bb064c4c Mon Sep 17 00:00:00 2001
From: Martin Babinsky mbabi...@redhat.com
Date: Mon, 16 Mar 2015 12:36:25 +0100
Subject: [PATCH] show the exception message thrown by dogtag._parse_ca_status
during install
https://fedorahosted.org/freeipa/ticket/4885
---
ipaplatform/redhat/services.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 8759cab76c7d72a3abbf935e7f15f7a32a0b6987..c9994e409a8a005012c0467c016608b8f689eef1 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -212,8 +212,8 @@ class RedHatCAService(RedHatService):
status = dogtag._parse_ca_status(stdout)
# end of workaround
-except Exception:
-status = 'check interrupted'
+except Exception as e:
+status = 'check interrupted due to error: %s' % e
root_logger.debug('The CA status is: %s' % status)
if status == 'running':
break
--
2.1.0
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] Password vault
On 3/13/2015 2:27 AM, Endi Sukma Dewata wrote:
On 3/11/2015 9:12 PM, Endi Sukma Dewata wrote:
Thanks for the review. New patch attached to be applied on top of all
previous patches. Please see comments below.
New patch #362-1 attached replacing #362. It fixed some issues in
handle_not_found().
New patch #363 attached. It adds supports for vault vaultcontainer ID
parameter.
--
Endi S. Dewata
From e1d2a3a62e6d16c1c9b19f4cb19b900427ea5e1f Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Thu, 12 Mar 2015 09:21:02 -0400
Subject: [PATCH] Vault ID improvements.
The vault plugin has been modified to accept a single vault ID
in addition to separate name and parent ID attributes. The vault
container has also been modified in the same way. New test cases
have been added to verify this functionality.
https://fedorahosted.org/freeipa/ticket/3872
---
ipalib/plugins/vault.py| 143 +--
ipalib/plugins/vaultcontainer.py | 137 +--
ipalib/plugins/vaultsecret.py | 10 +-
ipatests/test_xmlrpc/test_vault_plugin.py | 151 +
ipatests/test_xmlrpc/test_vaultcontainer_plugin.py | 90 ++--
ipatests/test_xmlrpc/test_vaultsecret_plugin.py| 115
6 files changed, 565 insertions(+), 81 deletions(-)
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index
d47067758186601365e5924f5d13c7ab51ba66e5..38693d0710e000695cae21fb4db5dfb4c85b5c74
100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -61,7 +61,7 @@ EXAMPLES:
ipa vault-find
) + _(
List shared vaults:
- ipa vault-find --container-id /shared
+ ipa vault-find /shared
) + _(
Add a standard vault:
ipa vault-add MyVault
@@ -171,8 +171,8 @@ class vault(LDAPObject):
cli_name='vault_name',
label=_('Vault name'),
primary_key=True,
-pattern='^[a-zA-Z0-9_.-]+$',
-pattern_errmsg='may only include letters, numbers, _, ., and -',
+pattern='^[a-zA-Z0-9_.-/]+$',
+pattern_errmsg='may only include letters, numbers, _, ., -, and /',
maxlength=255,
),
Str('vault_id?',
@@ -217,7 +217,7 @@ class vault(LDAPObject):
# get vault ID from parameters
name = keys[-1]
-container_id =
self.api.Object.vaultcontainer.normalize_id(options.get('container_id'))
+container_id =
self.api.Object.vaultcontainer.absolute_id(options.get('container_id'))
vault_id = container_id + name
dn = self.base_dn
@@ -250,6 +250,81 @@ class vault(LDAPObject):
return id
+def split_id(self, id):
+
+Splits a vault ID into (vault name, container ID) tuple.
+
+
+if not id:
+return (None, None)
+
+# split ID into container ID and vault name
+parts = id.rsplit(u'/', 1)
+
+if len(parts) == 2:
+vault_name = parts[1]
+container_id = u'%s/' % parts[0]
+
+else:
+vault_name = parts[0]
+container_id = None
+
+if not vault_name:
+vault_name = None
+
+return (vault_name, container_id)
+
+def merge_id(self, vault_name, container_id):
+
+Merges a vault name and a container ID into a vault ID.
+
+
+if not vault_name:
+id = container_id
+
+elif vault_name.startswith('/') or not container_id:
+id = vault_name
+
+else:
+id = container_id + vault_name
+
+return id
+
+def normalize_params(self, *args, **options):
+
+Normalizes the vault ID in the parameters.
+
+
+vault_id = self.parse_params(*args, **options)
+(vault_name, container_id) = self.split_id(vault_id)
+return self.update_params(vault_name, container_id, *args, **options)
+
+def parse_params(self, *args, **options):
+
+Extracts the vault name and container ID in the parameters.
+
+
+# get vault name and container ID from parameters
+vault_name = args[0]
+if type(vault_name) is tuple:
+vault_name = vault_name[0]
+container_id =
self.api.Object.vaultcontainer.normalize_id(options.get('container_id'))
+
+return self.merge_id(vault_name, container_id)
+
+def update_params(self, new_vault_name, new_container_id, *args,
**options):
+
+Stores vault name and container ID back into the parameters.
+
+
+args_list = list(args)
+args_list[0] = new_vault_name
+args = tuple(args_list)
+
+options['container_id'] = new_container_id
+
+return (args, options)
+
def get_kra_id(self, id):
Generates a client key ID to store/retrieve data in KRA.
@@ -363,10 +438,14 @@ class vault_add(LDAPCreate):
msg_summary = _('Added
Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code
Dne 16.3.2015 v 13:30 Martin Babinsky napsal(a): On 03/16/2015 12:15 PM, Martin Kosek wrote: On 03/13/2015 05:37 PM, Martin Babinsky wrote: Attaching the next iteration of patches. I have tried my best to reword the ipa-client-install man page bit about the new option. Any suggestions to further improve it are welcome. I have also slightly modified the 'kinit_keytab' function so that in Kerberos errors are reported for each attempt and the text of the last error is retained when finally raising exception. The approach looks very good. I think that my only concern with this patch is this part: +ccache.init_creds_keytab(keytab=ktab, principal=princ) ... +except krbV.Krb5Error as e: +last_exc = str(e) +root_logger.debug(Attempt %d/%d: failed: %s + % (attempt, attempts, last_exc)) +time.sleep(1) + +root_logger.debug(Maximum number of attempts (%d) reached + % attempts) +raise StandardError(Error initializing principal %s: %s +% (principal, last_exc)) The problem here is that this function will raise the super-generic StandardError instead of the proper with all the context and information about the error that the caller can then process. I think that except krbV.Krb5Error as e: if attempt == max_attempts: log something raise would be better. Yes that seems reasonable. I'm just thinking whether we should re-raise Krb5Error or raise ipalib.errors.KerberosError? the latter options makes more sense to me as we would not have to additionally import Krb5Error everywhere and it would also make the resulting errors more consistent. I am thinking about someting like this: except krbV.Krb5Error as e: if attempt == attempts: # log that we have reaches maximum number of attempts raise KerberosError(minor=str(e)) What do you think? NACK, don't use ipalib from ipapython in new code, we are trying to get rid of this circular dependency. Krb5Error is OK in this case. -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
