Re: [Freeipa-devel] [PATCH 0067] ipa-cacert-renew: Fix connection to ldap.

[Jan Cholasta]

On 19.11.2015 17:28, David Kupka wrote:

https://fedorahosted.org/freeipa/ticket/5468


ipa-cacert-manage is not the only code which uses ldap2 this way.

It would be better to find the root cause of this rather than working 
around it.


--
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

[Freeipa-devel] [PATCH 0098-0099] domain level 1 topology checks during IPA server uninstall

[Martin Babinsky]

These two patches fix the following tickets:

https://fedorahosted.org/freeipa/ticket/5377
https://fedorahosted.org/freeipa/ticket/5409

I have added a new option '--ignore-disconnected-topology' which forces 
IPA master uninstall despite reported errors in topology. I'm not quite 
sure if we want to flood ipa-server-install with uninstall-specific 
options, maybe it is better to skip the check in unattended mode and 
just print a warning about disconnected topology and what to do about it.


I would like to hear your opinions about this.

--
Martin^3 Babinsky
From 037b311d033454d11c971fcfe3875b758e1c1a8f Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 17:58:44 +0100
Subject: [PATCH 2/2] implement domain level 1 specific topology checks into
 IPA server uninstaller

When uninstalling domain level 1 master its removal from topology is checked
on remote masters. The uninstaller also checks whether the uninstallation
disconnects the topology and if yes aborts the procedure. The
'--ignore-disconnected-topology' options skips this check.

https://fedorahosted.org/freeipa/ticket/5377
https://fedorahosted.org/freeipa/ticket/5409
---
 install/tools/man/ipa-server-install.1 |   3 +
 ipaserver/install/server/install.py| 192 -
 2 files changed, 168 insertions(+), 27 deletions(-)

diff --git a/install/tools/man/ipa-server-install.1 b/install/tools/man/ipa-server-install.1
index 5c601b123385a30a1bd6962663f8f97b528e805e..7769d4f5116d262df05e7e7b0bbd0d2eb37f6266 100644
--- a/install/tools/man/ipa-server-install.1
+++ b/install/tools/man/ipa-server-install.1
@@ -61,6 +61,9 @@ The maximum user and group id number (default: idstart+19). If set to zero,
 \fB\-\-no_hbac_allow\fR
 Don't install allow_all HBAC rule. This rule lets any user from any host access any service on any other host. It is expected that users will remove this rule before moving to production.
 .TP
+\fB\-\-ignore-disconnected-topology\fR
+Ignore errors reported when uninstallation of IPA server would lead to disconnected domain level 1 topology.
+.TP
 \fB\-\-no\-ui\-redirect\fR
 Do not automatically redirect to the Web UI.
 .TP
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 6629e8ec12f50c83a691dcd60e2cbf1125598fcb..a7d7d56b621267bfd61d4eb0b1ad66d6a3005caa 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -26,7 +26,7 @@ from ipapython.ipautil import (
 from ipaplatform import services
 from ipaplatform.paths import paths
 from ipaplatform.tasks import tasks
-from ipalib import api, constants, errors, x509
+from ipalib import api, create_api, constants, errors, x509
 from ipalib.constants import CACERT
 from ipalib.util import validate_domain_name
 import ipaclient.ntpconf
@@ -290,6 +290,108 @@ def common_cleanup(func):
 return decorated
 
 
+def check_master_deleted(api, masters, interactive):
+try:
+host_princ = api.Command.host_show(api.env.host)['result']['krbprincipalname'][0]
+except Exception as e:
+raise RuntimeError(
+"Failed to get host principal name: {0}".format(e)
+)
+
+ccache_path = os.path.join('/', 'tmp', 'krb5cc_host')
+with ipautil.private_ccache(ccache_path):
+try:
+ipautil.kinit_keytab(host_princ, paths.KRB5_KEYTAB, ccache_path)
+except Exception as e:
+raise RuntimeError(
+"Kerberos authentication as '{0}' failed: {1}".format(
+host_princ, e
+)
+)
+
+last_server = True
+for master in masters:
+master_cn = master['cn'][0]
+if api.env.host == master_cn:
+continue
+
+last_server = False
+master_ldap_uri = u'ldap://{0}'.format(master_cn)
+
+# initialize remote api
+remote_api = create_api(mode=None)
+remote_api.bootstrap(ldap_uri=master_ldap_uri, in_server=True)
+remote_api.finalize()
+
+root_logger.debug("Connecting to '{0}'...".format(master_ldap_uri))
+try:
+remote_api.Backend.ldap2.connect(ccache=ccache_path)
+remote_api.Command.server_show(api.env.host)
+root_logger.debug(
+"Server entry '{0}' present on '{1}'".format(
+api.env.host, master_cn
+)
+)
+return False
+except (errors.NotFound, errors.ACIError):
+# this may occur because the node was already deleted from the
+# topology and the host principal doesn't exist
+root_logger.debug(
+"'{0}' was removed from topology".format(
+api.env.host
+)
+)
+return True
+except errors.NetworkError:
+# try the next m

Re: [Freeipa-devel] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Simo Sorce]

On Thu, 2015-11-19 at 15:43 +0100, Jan Cholasta wrote:
> Hi,
> 
> the attached patches fix  
> and .
> 
> I worked around the issue of checking if the user is privileged to 
> perform replica promotion by using host credentials instead. The host 
> must be a member of the IPA servers host group "ipaservers" in order to 
> be able to promote itself. Using host credentials will also allow 
> replica install using one-time password.
> 
> User credentials are still used for connection check and to 
> automatically add the host to ipaservers if the user is privileged to do 
> that.
> 
> Simo, is this approach OK? Could you check the new ACIs in patches 510 
> and 513?
> 
> I have a couple of questions:
> 
> 1) Why are custodia keys for the replica added to LDAP using connection 
> to the remote master instead of local ldapi connection? Is it to 
> eliminate race conditions caused by replication timeout from the replica 
> to the remote master?

Yes it is critical for that reason.

> If the code was changed to use ldapi and wait until the key appears in 
> custodia on the remote master, we could lose the "IPA server hosts can 
> create own Custodia secrets" and "IPA server hosts can manage own 
> Custodia secrets" ACIs from patch 510. Not sure if it's worth the change 
> though.

Not worth the change, as it has the same security properties.

> 2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?
> 
> If 'member' was used instead, we would gain referential integrity and 
> the ability to add ACIs based on the attribute (think 
> userattr="member#USERDN").

To avoid referential integrity and mixup with other group objects, it
was intentional.

> 3) Why is 'memberPrincipal' used in cn=custodia at all?
> 
> The hostname of the replica is already in 'cn', so instead of searching 
> cn=custodia for entries matching (memberPrincipal=host/$HOSTNAME), we 
> could get cn={enc,sig}/$HOSTNAME,cn=custodia directly.

They idea was to be flexible, in a future we might have some shared
keys, then the only way to properly resolve keys would be to use a
multivalued attribute.
Or we may want to have multiple keys sets for the same principal (it
doesn't have to be a host technically, it could be a service or even a
user in the future), and a naming based convention would make it harder
to deal with that.


On the patches
--
509:
- commit says only: "aci: add IPA servers host group 'ipaservers'"
but it does other things like changing how CA renewal certificate acis
are added, I think that must be separated in another patch.

- Why "Manage Host Keytab"  aci has been changed to exclude ipaservers ?

- Why adding the host to the ipaservers group is done in the
krbinstance ? I would expect LDAP ops to be mostly done in the
DSinstance.

- I do not see where the host is added to the ipaservers group on
upgrade.

510:
- We should probably tightenup the ACI to allos host X to only add
memberPrincipal = X and no other value, also the host should not be
allowed to change the memberPrincipal attribute only the keys.
If we can't express this in ACIs we can live with the ones you propose
though.

511:
ack, but I think you should catch potential exceptions from the
os.rmdir, as it will throw if there is some other file in the dir (for
whatever reason). The exception can be safely ignored, we just do not
want to blow up with a traceback here.

512:
- I do not think this is correct, it seem to me you are overriding the
local ccache (if any) with the host keytab. If I read this right, this
means that if you run kinit admin && ipa-replica-install then you will
kind your admin credentials gone and a host TGT instead. Am I reading
this change correctly ?

513:
Nack
- Should be folded into 510
- Should not allow all hosts in the domain but only ipaservers for aal
three changes

514:
- Should be folded in 512, they are completely interdependent changes.
- I do not understand how this works. promote_check() runs before
promote() and will fail if the host is not already in the ipaservers
group, so you will never be able to actually add the host to the group ?
sounds like chicken-egg ... or what am I missing ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

-- 
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 0067] ipa-cacert-renew: Fix connection to ldap.

[David Kupka]

https://fedorahosted.org/freeipa/ticket/5468
--
David Kupka
From 818ec6b7729867ae9e22b24aad3b306046d2f37d Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Thu, 19 Nov 2015 16:13:38 +
Subject: [PATCH] ipa-cacert-renew: Fix connection to ldap.

https://fedorahosted.org/freeipa/ticket/5468
---
 ipaserver/install/ipa_cacert_manage.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaserver/install/ipa_cacert_manage.py b/ipaserver/install/ipa_cacert_manage.py
index 66cba891fad4b679ae51a4a11a094de341c24e88..e030b1efb74c47a261f9e2fa2330f8518cb3fa47 100644
--- a/ipaserver/install/ipa_cacert_manage.py
+++ b/ipaserver/install/ipa_cacert_manage.py
@@ -123,7 +123,7 @@ class CACertManage(admintool.AdminTool):
 return rc
 
 def ldap_connect(self):
-conn = ldap2(api)
+conn = api.Backend.ldap2
 
 password = self.options.password
 if not password:
-- 
2.4.3

-- 
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 0352] fix caching in get_ipa_config

[Jan Cholasta]

On 19.11.2015 13:29, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5463

Patch attached.


Thanks, ACK.

Pushed to:
master: 7f0d018c66da1fe2adedd45aa9f5a63c913e4527
ipa-4-2: 0ca4c1db3ee6d9366f447d0d704fa56d98e366b4

--
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

[Freeipa-devel] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Jan Cholasta]

Hi,

the attached patches fix  
and .


I worked around the issue of checking if the user is privileged to 
perform replica promotion by using host credentials instead. The host 
must be a member of the IPA servers host group "ipaservers" in order to 
be able to promote itself. Using host credentials will also allow 
replica install using one-time password.


User credentials are still used for connection check and to 
automatically add the host to ipaservers if the user is privileged to do 
that.


Simo, is this approach OK? Could you check the new ACIs in patches 510 
and 513?


I have a couple of questions:

1) Why are custodia keys for the replica added to LDAP using connection 
to the remote master instead of local ldapi connection? Is it to 
eliminate race conditions caused by replication timeout from the replica 
to the remote master?


If the code was changed to use ldapi and wait until the key appears in 
custodia on the remote master, we could lose the "IPA server hosts can 
create own Custodia secrets" and "IPA server hosts can manage own 
Custodia secrets" ACIs from patch 510. Not sure if it's worth the change 
though.


2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?

If 'member' was used instead, we would gain referential integrity and 
the ability to add ACIs based on the attribute (think 
userattr="member#USERDN").


3) Why is 'memberPrincipal' used in cn=custodia at all?

The hostname of the replica is already in 'cn', so instead of searching 
cn=custodia for entries matching (memberPrincipal=host/$HOSTNAME), we 
could get cn={enc,sig}/$HOSTNAME,cn=custodia directly.


Honza

--
Jan Cholasta
From fe5cf036513648a6b9398fb118ade33f4cd1b25a Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 11 Nov 2015 11:01:57 +0100
Subject: [PATCH] aci: add IPA servers host group 'ipaservers'

https://fedorahosted.org/freeipa/ticket/3416
---
 ACI.txt|   4 +-
 install/share/bootstrap-template.ldif  |  11 +++
 install/share/default-aci.ldif |  11 ---
 install/updates/20-ipaservers_hostgroup.update |  10 +++
 install/updates/40-delegation.update   |  18 ++--
 install/updates/Makefile.am|   1 +
 ipalib/plugins/host.py |   6 ++
 ipalib/plugins/hostgroup.py|  26 ++
 ipaserver/install/krbinstance.py   |   7 ++
 ipaserver/install/replication.py   | 111 -
 10 files changed, 75 insertions(+), 130 deletions(-)
 create mode 100644 install/updates/20-ipaservers_hostgroup.update

diff --git a/ACI.txt b/ACI.txt
index 40fa822..bbc2e66 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -119,7 +119,7 @@ aci: (targetattr = "usercertificate")(targetfilter = "(objectclass=ipahost)")(ve
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "userpassword")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Manage Host Enrollment Password";allow (write) groupdn = "ldap:///cn=System: Manage Host Enrollment Password,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "krblastpwdchange || krbprincipalkey")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Manage Host Keytab";allow (write) groupdn = "ldap:///cn=System: Manage Host Keytab,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+aci: (targetattr = "krblastpwdchange || krbprincipalkey")(targetfilter = "(&(!(memberOf=cn=ipaservers,cn=hostgroups,cn=accounts,dc=ipa,dc=example))(objectclass=ipahost))")(version 3.0;acl "permission:System: Manage Host Keytab";allow (write) groupdn = "ldap:///cn=System: Manage Host Keytab,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "createtimestamp || entryusn || ipaallowedtoperform;read_keys || ipaallowedtoperform;write_keys || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Manage Host Keytab Permissions";allow (compare,read,search,write) groupdn = "ldap:///cn=System: Manage Host Keytab Permissions,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
@@ -137,7 +137,7 @@ aci: (targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System
 dn: cn=hostgroups,cn=accounts,dc=ipa,dc=example
 aci: (targetfilter = "(objectclass=ipahostgroup)")(version 3.0;acl "permission:System: Add Hostgroups";allow (add) groupdn = "ldap:///cn=System: Add Hostgroups,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=hostgroups,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "member")(targetfilter = "(objectclass=ipahostgroup)")(version 3.0;acl "permission:System: Modify Hostgroup Membership";allow (write) groupdn = "ldap:///cn=System: Modify Hostgroup Membership,cn=permissions,cn=pbac,dc

Re: [Freeipa-devel] [PATCH 0097] fix critical error messages when adding KRA container that already exists

[Jan Cholasta]

On 19.11.2015 14:47, Martin Babinsky wrote:

On 11/19/2015 02:31 PM, Jan Cholasta wrote:

On 19.11.2015 11:23, Martin Babinsky wrote:

On 11/19/2015 10:50 AM, Martin Babinsky wrote:

https://fedorahosted.org/freeipa/ticket/5346




Attaching updated patches.


1) It seems the self._ldap_disconnect() was actually necessary:

 cannot connect to
'ldapi://%2fvar%2frun%2fslapd-ABC-IDM-LAB-ENG-BRQ-REDHAT-COM.socket':
 The ipa-kra-install command failed. See
/var/log/ipaserver-kra-install.log for more information

After re-adding it, ipa-kra-install works again.


2) I don't want to see any messages when there's nothing wrong:

   [7/8]: add vault container
 Vault container already exists

Please lower the log level of this message from info to debug.


OK, attaching updated patches.



Thanks, ACK.

Pushed to:
master: 4d59a711af2b5b5e3441116f6d18d54ec9eccfb8
ipa-4-2: f2a7a3ee011263ae95982c875f3e9d9c5091438c

--
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

Re: [Freeipa-devel] [PATCH 0331, 0337] User plugin: allow multiple managers per user - CLI part

[David Kupka]

On 19/11/15 12:54, Martin Basti wrote:



On 18.11.2015 16:10, Martin Basti wrote:



On 12.11.2015 12:39, Martin Basti wrote:



On 27.10.2015 14:59, Martin Basti wrote:



On 20.10.2015 18:46, Martin Basti wrote:



On 20.10.2015 16:07, Martin Basti wrote:



On 20.10.2015 15:57, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5344

Patch attached.

Test are failing, a fix in UserTracker has to be done (partially
in my patch 329)



SelfNACK, I forgot to add stageuser tests



Updated patch attached.

I extracted tests to the separate patch, tests do not work, I had
issues with user and stageuser trackers.




Patch to fix issues with --addattr and managers added and attached.




The new one patch 0331 attached, patch 0337 is not needed anymore.

This patch also fixes https://fedorahosted.org/freeipa/ticket/5387



updated patch attached.



updated patch attached



Works for me, ACK.

--
David Kupka

--
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 0332] fix user post_callback

[David Kupka]

On 12/11/15 12:36, Martin Basti wrote:



On 21.10.2015 11:14, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5387

Patch attached.




Fix for this ticket has been implemented in patch mbasti-0331-2

Attached patch contains only common postcallback code to from user and
stageuser to parent class.




Works for me, ACK.

--
David Kupka

--
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 0097] fix critical error messages when adding KRA container that already exists

[Martin Babinsky]

On 11/19/2015 02:31 PM, Jan Cholasta wrote:

On 19.11.2015 11:23, Martin Babinsky wrote:

On 11/19/2015 10:50 AM, Martin Babinsky wrote:

https://fedorahosted.org/freeipa/ticket/5346




Attaching updated patches.


1) It seems the self._ldap_disconnect() was actually necessary:

 cannot connect to
'ldapi://%2fvar%2frun%2fslapd-ABC-IDM-LAB-ENG-BRQ-REDHAT-COM.socket':
 The ipa-kra-install command failed. See
/var/log/ipaserver-kra-install.log for more information

After re-adding it, ipa-kra-install works again.


2) I don't want to see any messages when there's nothing wrong:

   [7/8]: add vault container
 Vault container already exists

Please lower the log level of this message from info to debug.


OK, attaching updated patches.

--
Martin^3 Babinsky
From 86f9e71729ff3fe6ab9387e7ec057070eb249d95 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 14:33:49 +0100
Subject: [PATCH] suppress errors arising from adding existing LDAP entries
 during KRA install

https://fedorahosted.org/freeipa/ticket/5346
---
 ipaserver/install/krainstance.py | 16 ++--
 ipaserver/install/service.py |  4 +++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index ed47be7374ff89e787661dc1447b9388ba0f6334..be62226ada2878a4a826570e6ac17d7800cd5938 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -53,6 +53,8 @@ ADMIN_GROUPS = [
 'Security Domain Administrators'
 ]
 
+LDAPMOD_ERR_ALREADY_EXISTS = 68
+
 class KRAInstance(DogtagInstance):
 """
 We assume that the CA has already been installed, and we use the
@@ -312,8 +314,18 @@ class KRAInstance(DogtagInstance):
 conn.disconnect()
 
 def __add_vault_container(self):
-self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix})
-self.ldap_disconnect()
+try:
+self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix},
+   raise_on_err=True)
+except ipautil.CalledProcessError as e:
+if e.returncode == LDAPMOD_ERR_ALREADY_EXISTS:
+self.log.debug("Vault container already exists")
+else:
+self.log.error("Failed to add vault container: {0}".format(e))
+finally:
+# we need to disconnect from LDAP, because _ldap_mod() makes the
+# connection without actually using it
+self.ldap_disconnect()
 
 def __apply_updates(self):
 sub_dict = {
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index b9e68121dda6ea0b52c9ad923fcd5c72a22598a4..c856cccd03a5d7f166240ff87d9c49ef45f2a64d 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -184,7 +184,7 @@ class Service(object):
 self.admin_conn.unbind()
 self.admin_conn = None
 
-def _ldap_mod(self, ldif, sub_dict=None):
+def _ldap_mod(self, ldif, sub_dict=None, raise_on_err=False):
 pw_name = None
 fd = None
 path = ipautil.SHARE_DIR + ldif
@@ -228,6 +228,8 @@ class Service(object):
 try:
 ipautil.run(args, nolog=nologlist)
 except ipautil.CalledProcessError as e:
+if raise_on_err:
+raise
 root_logger.critical("Failed to load %s: %s" % (ldif, str(e)))
 finally:
 if pw_name:
-- 
2.4.3

From 8b9d337e3fee98a343006c01c3c9bdd54ce1f040 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 14:33:49 +0100
Subject: [PATCH] suppress errors arising from adding existing LDAP entries
 during KRA install

https://fedorahosted.org/freeipa/ticket/5346
---
 ipaserver/install/krainstance.py | 16 ++--
 ipaserver/install/service.py |  4 +++-
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index 192745b6d7f9f402267e435f7223f1bf8849..a2514debae600bdc46afb92e426a5f616529fde2 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -47,6 +47,8 @@ from ipapython.ipa_log_manager import log_mgr
 IPA_KRA_RECORD = "ipa-kra"
 
 
+LDAPMOD_ERR_ALREADY_EXISTS = 68
+
 class KRAInstance(DogtagInstance):
 """
 We assume that the CA has already been installed, and we use the
@@ -308,8 +310,18 @@ class KRAInstance(DogtagInstance):
 conn.disconnect()
 
 def __add_vault_container(self):
-self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix})
-self.ldap_disconnect()
+try:
+self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix},
+   raise_on_err=True)
+except ipautil.CalledProcessError as e:
+if e.returncode == LDAPMOD_ERR_ALREADY_EXISTS:
+self.log.debug("Vault container already exists")
+else:
+self.log.error("Failed to add vault container: {0

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Babinsky]

On 11/19/2015 01:08 PM, Martin Basti wrote:



On 18.11.2015 14:26, Martin Basti wrote:



On 18.11.2015 14:24, Martin Kosek wrote:

On 11/18/2015 02:18 PM, Martin Basti wrote:


On 18.11.2015 13:55, Martin Kosek wrote:

On 11/18/2015 01:30 PM, Martin Basti wrote:

+DEFAULT_TIME_LIMIT = -1.0
+DEFAULT_SIZE_LIMIT = 0

...

   if time_limit is None or time_limit == 0:
-time_limit = -1.0
+if self.time_limit is not None and self.time_limit != 0:
+time_limit = self.time_limit
+else:
+time_limit = self.DEFAULT_TIME_LIMIT
+

I wonder why is the -1 default time limit a float number, I would
expect that
some trouble may emerge out of it. Maybe Rob knows?


Python LDAP allows to have smaller granularity than seconds and it
internally
convert time limit values to float.

Hm, ok. I was just curious since the value we expose in API is Int
and the
default does not use the smaller granularity, so I was thinking "Why
bother?".
If it is fixed in your patch, good. If not, good also, no need to
bikeshed here
I suppose.

I just keep the original values there, I do not want to touch it with
this patch


Updated patch attached, the original one has wrong default limit for ldap2



ACK

--
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 0097] fix critical error messages when adding KRA container that already exists

[Jan Cholasta]

On 19.11.2015 11:23, Martin Babinsky wrote:

On 11/19/2015 10:50 AM, Martin Babinsky wrote:

https://fedorahosted.org/freeipa/ticket/5346




Attaching updated patches.


1) It seems the self._ldap_disconnect() was actually necessary:

cannot connect to 
'ldapi://%2fvar%2frun%2fslapd-ABC-IDM-LAB-ENG-BRQ-REDHAT-COM.socket':
The ipa-kra-install command failed. See 
/var/log/ipaserver-kra-install.log for more information


After re-adding it, ipa-kra-install works again.


2) I don't want to see any messages when there's nothing wrong:

  [7/8]: add vault container
Vault container already exists

Please lower the log level of this message from info to debug.

--
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

Re: [Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Jan Cholasta]

On 19.11.2015 14:09, Martin Babinsky wrote:

On 11/19/2015 01:08 PM, Martin Basti wrote:



On 18.11.2015 14:26, Martin Basti wrote:



On 18.11.2015 14:24, Martin Kosek wrote:

On 11/18/2015 02:18 PM, Martin Basti wrote:


On 18.11.2015 13:55, Martin Kosek wrote:

On 11/18/2015 01:30 PM, Martin Basti wrote:

+DEFAULT_TIME_LIMIT = -1.0
+DEFAULT_SIZE_LIMIT = 0

...

   if time_limit is None or time_limit == 0:
-time_limit = -1.0
+if self.time_limit is not None and self.time_limit
!= 0:
+time_limit = self.time_limit
+else:
+time_limit = self.DEFAULT_TIME_LIMIT
+

I wonder why is the -1 default time limit a float number, I would
expect that
some trouble may emerge out of it. Maybe Rob knows?


Python LDAP allows to have smaller granularity than seconds and it
internally
convert time limit values to float.

Hm, ok. I was just curious since the value we expose in API is Int
and the
default does not use the smaller granularity, so I was thinking "Why
bother?".
If it is fixed in your patch, good. If not, good also, no need to
bikeshed here
I suppose.

I just keep the original values there, I do not want to touch it with
this patch


Updated patch attached, the original one has wrong default limit for
ldap2



ACK


Hold your horses.

--
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

[Freeipa-devel] [PATCH 0352] fix caching in get_ipa_config

[Martin Basti]

https://fedorahosted.org/freeipa/ticket/5463

Patch attached.
From 6f8bbe74a667586b4a3474c88d05b3ea26e22146 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 19 Nov 2015 13:25:49 +0100
Subject: [PATCH] fix caching in get_ipa_config

Different opbject types were compared thus always result of comparation
was False and caching does not work.

https://fedorahosted.org/freeipa/ticket/5463
---
 ipaserver/plugins/ldap2.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index 7614a3a5585c8469ab95098446331f6185226e82..7b492b2a4a1e8222a8eef9dc12dc5d5ba6e209fc 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -222,7 +222,7 @@ class ldap2(CrudBackend, LDAPClient):
 
 try:
 config_entry = getattr(context, 'config_entry')
-if config_entry.conn is self.conn:
+if config_entry.conn.conn is self.conn:
 return config_entry
 except AttributeError:
 # Not in our context yet
-- 
2.5.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 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

On 18.11.2015 14:26, Martin Basti wrote:



On 18.11.2015 14:24, Martin Kosek wrote:

On 11/18/2015 02:18 PM, Martin Basti wrote:


On 18.11.2015 13:55, Martin Kosek wrote:

On 11/18/2015 01:30 PM, Martin Basti wrote:

+DEFAULT_TIME_LIMIT = -1.0
+DEFAULT_SIZE_LIMIT = 0

...

   if time_limit is None or time_limit == 0:
-time_limit = -1.0
+if self.time_limit is not None and self.time_limit != 0:
+time_limit = self.time_limit
+else:
+time_limit = self.DEFAULT_TIME_LIMIT
+
I wonder why is the -1 default time limit a float number, I would 
expect that

some trouble may emerge out of it. Maybe Rob knows?

Python LDAP allows to have smaller granularity than seconds and it 
internally

convert time limit values to float.
Hm, ok. I was just curious since the value we expose in API is Int 
and the
default does not use the smaller granularity, so I was thinking "Why 
bother?".
If it is fixed in your patch, good. If not, good also, no need to 
bikeshed here

I suppose.
I just keep the original values there, I do not want to touch it with 
this patch



Updated patch attached, the original one has wrong default limit for ldap2
From 770e1bec7a95fa63f910213a5611ba9bccf18547 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 18 Nov 2015 10:31:05 +0100
Subject: [PATCH] Upgrade: increase time limit for upgrades

Default ldap search limit is now 30s by default during upgrade.

This value is configurable via default.conf file, option
'upgrade_ldapsearch_limit'.

Limits must be changed for the whole ldap2 connection, because this
connection is used inside update plugins and commands called from
upgrade.

Together with increasing the time limit, also size limit should be
unlimited during upgrade. With sizelimit=None we may get the
TimeExceeded exception from getting default value of the sizelimit from LDAP.

https://fedorahosted.org/freeipa/ticket/5267
---
 ipalib/constants.py |  3 +++
 ipapython/ipaldap.py| 35 ++-
 ipaserver/install/ldapupdate.py |  7 +++
 ipaserver/plugins/ldap2.py  | 34 +-
 4 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/ipalib/constants.py b/ipalib/constants.py
index fc0560ba4fe746f11e8ff3175508ace2e50c3187..15be3ff865cdb0e6a01570144c9fdcc4c6909dac 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -189,6 +189,9 @@ DEFAULT_CONFIG = (
 # behavior when newer clients talk to older servers. Use with caution.
 ('skip_version_check', False),
 
+# Upgrade
+('upgrade_ldapsearch_limit', 30),  # limit in seconds
+
 # 
 #  The remaining keys are never set from the values here!
 # 
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7e5bc04fea1dc89cc141051025d00a57d74b2b41..7b70894b27b5c32ed05251cf3bc6d1824c88010e 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -689,8 +689,12 @@ class LDAPClient(object):
 'nsslapd-minssf-exclude-rootdse': True,
 })
 
+DEFAULT_TIME_LIMIT = -1.0
+DEFAULT_SIZE_LIMIT = 0
+
 def __init__(self, ldap_uri, start_tls=False, force_schema_updates=False,
- no_schema=False, decode_attrs=True):
+ no_schema=False, decode_attrs=True, time_limit=None,
+ size_limit=None):
 """Create LDAPClient object.
 
 :param ldap_uri: The LDAP URI to connect to
@@ -706,12 +710,18 @@ class LDAPClient(object):
 :param decode_attrs:
 If true, attributes are decoded to Python types according to their
 syntax.
+:param time_limit: time limit for ldap search that is used for whole
+connection
+:param size_limit: size limit for ldap seach that is used for whole
+connection
 """
 self.ldap_uri = ldap_uri
 self._start_tls = start_tls
 self._force_schema_updates = force_schema_updates
 self._no_schema = no_schema
 self._decode_attrs = decode_attrs
+self.time_limit = time_limit
+self.size_limit = size_limit
 
 self.log = log_mgr.get_logger(self)
 self._has_schema = False
@@ -1283,6 +1293,11 @@ class LDAPClient(object):
 (default skips these entries)
 paged_search -- search using paged results control
 
+size_limit and time_limit priority:
+1. use locally specified limits (if specified)
+2. use limits specified in __init__ (if specified)
+3. use default limits
+
 :raises: errors.NotFound if result set is empty
  or base_dn doesn't exist
 """
@@ -1295,9 +1310,17 @@ class LDAPClient(object):
 truncated = False
 
 if time_limit is None or time_limit == 0:
-time_limit = -1.0
+

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[Jan Cholasta]

On 19.11.2015 13:01, David Kupka wrote:

On 18/11/15 14:10, Jan Cholasta wrote:

On 10.11.2015 19:19, Rob Crittenden wrote:

Jan Cholasta wrote:

On 9.11.2015 16:51, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

the attached patch fixes
.

Honza





There be a note in renew_ra_cert that the lock is obtained in
advance by
renew_ra_cert_pre.


Added comment.



It looks like it will silently fail if the lock cannot be acquired. Is
that desired?


All unhandled exceptions are logged to syslog in both renew_ra_cert_pre
and renew_ra_cert:

 try:
 main()
 except Exception:
 syslog.syslog(syslog.LOG_ERR, traceback.format_exc())

Updated patch attached.



My confusion was with the auto-expiration. I guess this is ok. When
debugging this sort of thing via logs the more the merrier, so I guess
I'd have added a syslog to say "obtaining lock" or "locked" and then
something when the renewal actually starts, so one can try to piece
together what happened after the fact if something goes wrong.

I guess certmonger already logs when a pre/post command is executed so
that may already be available.


Yes. The ticket is not related to logging anyway.

Is the last patch OK, then?



Thanks for the patch. Works for me, ACK.


Pushed to:
master: f3076c6ab37e081ba9b0ec9f0502379f60dfbd10
ipa-4-2: f831cb6a3da0c5f2a3e71004ae327273b25723fa

--
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

Re: [Freeipa-devel] [PATCH 506] cert renewal: make renewal of ipaCert atomic

[David Kupka]

On 18/11/15 14:10, Jan Cholasta wrote:

On 10.11.2015 19:19, Rob Crittenden wrote:

Jan Cholasta wrote:

On 9.11.2015 16:51, Rob Crittenden wrote:

Jan Cholasta wrote:

Hi,

the attached patch fixes
.

Honza





There be a note in renew_ra_cert that the lock is obtained in
advance by
renew_ra_cert_pre.


Added comment.



It looks like it will silently fail if the lock cannot be acquired. Is
that desired?


All unhandled exceptions are logged to syslog in both renew_ra_cert_pre
and renew_ra_cert:

 try:
 main()
 except Exception:
 syslog.syslog(syslog.LOG_ERR, traceback.format_exc())

Updated patch attached.



My confusion was with the auto-expiration. I guess this is ok. When
debugging this sort of thing via logs the more the merrier, so I guess
I'd have added a syslog to say "obtaining lock" or "locked" and then
something when the renewal actually starts, so one can try to piece
together what happened after the fact if something goes wrong.

I guess certmonger already logs when a pre/post command is executed so
that may already be available.


Yes. The ticket is not related to logging anyway.

Is the last patch OK, then?



Thanks for the patch. Works for me, ACK.

--
David Kupka

--
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 0331, 0337] User plugin: allow multiple managers per user - CLI part

[Martin Basti]

On 18.11.2015 16:10, Martin Basti wrote:



On 12.11.2015 12:39, Martin Basti wrote:



On 27.10.2015 14:59, Martin Basti wrote:



On 20.10.2015 18:46, Martin Basti wrote:



On 20.10.2015 16:07, Martin Basti wrote:



On 20.10.2015 15:57, Martin Basti wrote:

https://fedorahosted.org/freeipa/ticket/5344

Patch attached.

Test are failing, a fix in UserTracker has to be done (partially 
in my patch 329)




SelfNACK, I forgot to add stageuser tests



Updated patch attached.

I extracted tests to the separate patch, tests do not work, I had 
issues with user and stageuser trackers.





Patch to fix issues with --addattr and managers added and attached.




The new one patch 0331 attached, patch 0337 is not needed anymore.

This patch also fixes https://fedorahosted.org/freeipa/ticket/5387



updated patch attached.



updated patch attached
From 05e586484da12e136f86f7b5e50cb4703ea38333 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 5 Nov 2015 17:11:23 +0100
Subject: [PATCH] Allow multiple managers per user - CLI part

Added commands:
* user-add-manager
* user-remove-manager
* stageuser-add-manager
* stageuser-remove-manager

Commit contains override of convert_attribute_members method in baseuser
class that ensures the managers will be returned in 'manager' attribute
due to backward compatibility instead of 'manager_user' as would be
expected.

https://fedorahosted.org/freeipa/ticket/5344

This patch also fixes: https://fedorahosted.org/freeipa/ticket/5387
---
 API.txt | 44 +++
 VERSION |  4 ++--
 ipalib/plugins/baseuser.py  | 50 ++---
 ipalib/plugins/stageuser.py | 22 ++--
 ipalib/plugins/user.py  | 24 +++---
 5 files changed, 113 insertions(+), 31 deletions(-)

diff --git a/API.txt b/API.txt
index 873c6d54221a0c1657b5457bd9dceedb4adf06b3..0976c97213775d79da43ee382a0badbe029b7960 100644
--- a/API.txt
+++ b/API.txt
@@ -4248,6 +4248,17 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
+command: stageuser_add_manager
+args: 1,5,3
+arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required=True)
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Flag('no_members', autofill=True, default=False, exclude='webui')
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('user*', alwaysask=True, cli_name='users', csv=True)
+option: Str('version?', exclude='webui')
+output: Output('completed', , None)
+output: Output('failed', , None)
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 command: stageuser_del
 args: 1,2,3
 arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=True, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required=True)
@@ -4367,6 +4378,17 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
+command: stageuser_remove_manager
+args: 1,5,3
+arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required=True)
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Flag('no_members', autofill=True, default=False, exclude='webui')
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('user*', alwaysask=True, cli_name='users', csv=True)
+option: Str('version?', exclude='webui')
+output: Output('completed', , None)
+output: Output('failed', , None)
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 command: stageuser_show
 args: 1,5,3
 arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required=True)
@@ -5208,6 +5230,17 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
+command: user_add_manager
+args: 1,5,3
+arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required

Re: [Freeipa-devel] [PATCH 0097] fix critical error messages when adding KRA container that already exists

[Martin Babinsky]

On 11/19/2015 10:50 AM, Martin Babinsky wrote:

https://fedorahosted.org/freeipa/ticket/5346




Attaching updated patches.

--
Martin^3 Babinsky
From fa37e2514259d78d1b54c33f18fb95ec8b4a37cf Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 10:24:40 +0100
Subject: [PATCH] suppress errors arising from adding existing LDAP entries
 during KRA install

https://fedorahosted.org/freeipa/ticket/5346
---
 ipaserver/install/krainstance.py | 12 ++--
 ipaserver/install/service.py |  4 +++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index ed47be7374ff89e787661dc1447b9388ba0f6334..380c6ae3b18882650a2a0c1db026cfa7d299b15c 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -53,6 +53,8 @@ ADMIN_GROUPS = [
 'Security Domain Administrators'
 ]
 
+LDAPMOD_ERR_ALREADY_EXISTS = 68
+
 class KRAInstance(DogtagInstance):
 """
 We assume that the CA has already been installed, and we use the
@@ -312,8 +314,14 @@ class KRAInstance(DogtagInstance):
 conn.disconnect()
 
 def __add_vault_container(self):
-self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix})
-self.ldap_disconnect()
+try:
+self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix},
+   raise_on_err=True)
+except ipautil.CalledProcessError as e:
+if e.returncode == LDAPMOD_ERR_ALREADY_EXISTS:
+self.log.info("Vault container already exists")
+else:
+self.log.error("Failed to add vault container: {0}".format(e))
 
 def __apply_updates(self):
 sub_dict = {
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index b9e68121dda6ea0b52c9ad923fcd5c72a22598a4..c856cccd03a5d7f166240ff87d9c49ef45f2a64d 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -184,7 +184,7 @@ class Service(object):
 self.admin_conn.unbind()
 self.admin_conn = None
 
-def _ldap_mod(self, ldif, sub_dict=None):
+def _ldap_mod(self, ldif, sub_dict=None, raise_on_err=False):
 pw_name = None
 fd = None
 path = ipautil.SHARE_DIR + ldif
@@ -228,6 +228,8 @@ class Service(object):
 try:
 ipautil.run(args, nolog=nologlist)
 except ipautil.CalledProcessError as e:
+if raise_on_err:
+raise
 root_logger.critical("Failed to load %s: %s" % (ldif, str(e)))
 finally:
 if pw_name:
-- 
2.4.3

From 66236204154f968fb94615cf3c955cf88c182ff8 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 10:24:40 +0100
Subject: [PATCH] suppress errors arising from adding existing LDAP entries
 during KRA install

https://fedorahosted.org/freeipa/ticket/5346
---
 ipaserver/install/krainstance.py | 12 ++--
 ipaserver/install/service.py |  4 +++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index 69fe636732e6d3a8c1e0c460b641f061e519df92..e49be9f787d9d9f0e27eda0f278a23e39bef4c9b 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -47,6 +47,8 @@ from ipapython.ipa_log_manager import log_mgr
 IPA_KRA_RECORD = "ipa-kra"
 
 
+LDAPMOD_ERR_ALREADY_EXISTS = 68
+
 class KRAInstance(DogtagInstance):
 """
 We assume that the CA has already been installed, and we use the
@@ -306,8 +308,14 @@ class KRAInstance(DogtagInstance):
 conn.disconnect()
 
 def __add_vault_container(self):
-self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix})
-self.ldap_disconnect()
+try:
+self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix},
+   raise_on_err=True)
+except ipautil.CalledProcessError as e:
+if e.returncode == LDAPMOD_ERR_ALREADY_EXISTS:
+self.log.info("Vault container already exists")
+else:
+self.log.error("Failed to add vault container: {0}".format(e))
 
 def __apply_updates(self):
 sub_dict = {
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index f0eaee2c99d2949ca77659bf163a22f6785d9bc5..e59e82c9fbd0c15dd97c1814a91a78612a151230 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -155,7 +155,7 @@ class Service(object):
 self.admin_conn.unbind()
 self.admin_conn = None
 
-def _ldap_mod(self, ldif, sub_dict=None):
+def _ldap_mod(self, ldif, sub_dict=None, raise_on_err=False):
 pw_name = None
 fd = None
 path = ipautil.SHARE_DIR + ldif
@@ -199,6 +199,8 @@ class Service(object):
 try:
 ipautil.run(args, nolog=nologlist)
 except ipautil.CalledProcessError, e:
+if raise_on_err:
+

Re: [Freeipa-devel] [PATCH 508] install: export KRA agent PEM file in ipa-kra-install

[Jan Cholasta]

On 19.11.2015 11:01, Martin Babinsky wrote:

On 11/19/2015 09:07 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




ACK


Thanks.

Pushed to:
master: 164fb7b1d19ef316d2ec55a8f85876ccf310544f
ipa-4-2: 9d4f383a94b28d415396e1529c747c5e5bbdbc0f

--
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

[Freeipa-devel] [PATCH 0351] call directly is_host_resolvable function to verify addresses in NS records

[Martin Basti]

Testing if address is resolvable can be done by directly call of 
is_host_resovable, instead of call the dns-resolve command which is 
doing the same (works as proxy).


Patch attached.
From 19914a0196da6522d75a74ca7f8a196ed2616d7a Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 18 Nov 2015 19:25:04 +0100
Subject: [PATCH] Call directly function is_host_resolvable instead do call via
 framework

---
 ipalib/plugins/dns.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 901afbb7ac619ee6f25d38808b4c9a7b6cdef112..919592b8faf627774326b3c756794b588a5706b5 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1563,9 +1563,7 @@ def check_ns_rec_resolvable(zone, name):
 elif not name.is_absolute():
 # this is a DNS name relative to the zone
 name = name.derelativize(zone.make_absolute())
-try:
-return api.Command['dns_resolve'](unicode(name))
-except errors.NotFound:
+if not is_host_resolvable(name):
 raise errors.NotFound(
 reason=_('Nameserver \'%(host)s\' does not have a corresponding '
  'A/ record') % {'host': name}
-- 
2.5.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 0344] Use absolute domain name in detection of A/AAAA records

[Martin Basti]

On 18.11.2015 18:33, Petr Spacek wrote:

On 12.11.2015 13:58, Martin Basti wrote:


On 09.11.2015 08:47, Petr Spacek wrote:

On 4.11.2015 16:16, Martin Basti wrote:

Patch attached.

https://fedorahosted.org/freeipa/ticket/5421

I'm not entirely sure how this patch will interact with magic included in
ipalib/plugins/dns.py:class dns_resolve(Command).

I would like to delete the 'normalization' from at least one of these places.

Also, as you know, DNS names are not strings and should be manipulated using
python-dns so all crazy things in DNS names do not break in weird corner cases.


Updated patch attached.

Hmm, you bravely ignored my comment about class dns_resolve(Command) above,
sooo: NACK.

As far as I can tell ipalib/plugins/dns.py:class dns_resolve(Command) behaves
in the same brain-dead way as original is_host_resolvable() function. Please
fix both, not just one.

If you are sure that the behavior of the dns-resolve is bad, then 
updated patch that removes the code which appending the api.env.domain 
to query.


Patch attached.
From 43a8522a2a0d61858e49e9a1a870e04a8f6bcbb8 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 4 Nov 2015 16:09:21 +0100
Subject: [PATCH] Use absolute domain in  detection of A/ records

Python dns resolver append configured domain to queries which may lead
to false positive answer.

Exmaple: resolving "ipa.example.com" may return records for
"ipa.example.com.example.com" if domain is configured as "example.com"

https://fedorahosted.org/freeipa/ticket/5421
---
 ipalib/plugins/dns.py | 6 +-
 ipapython/ipautil.py  | 5 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 686eb758521ee0af2c91e16a599387f740bdb347..901afbb7ac619ee6f25d38808b4c9a7b6cdef112 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -4186,16 +4186,12 @@ class dns_resolve(Command):
 
 takes_args = (
 Str('hostname',
-label=_('Hostname'),
+label=_('Hostname (FQDN)'),
 ),
 )
 
 def execute(self, *args, **options):
 query=args[0]
-if query.find(api.env.domain) == -1 and query.find('.') == -1:
-query = '%s.%s.' % (query, api.env.domain)
-if query[-1] != '.':
-query = query + '.'
 
 if not is_host_resolvable(query):
 raise errors.NotFound(
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..2e306013bf64f56917688da7aec3d9678ec627bc 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -49,6 +49,7 @@ from ipapython import ipavalidate
 from ipapython import config
 from ipaplatform.paths import paths
 from ipapython.dn import DN
+from ipapython.dnsutil import DNSName
 
 SHARE_DIR = paths.USR_SHARE_IPA_DIR
 PLUGINS_SHARE_DIR = paths.IPA_PLUGINS
@@ -911,9 +912,11 @@ def bind_port_responder(port, socket_type=socket.SOCK_STREAM, socket_timeout=Non
 raise last_socket_error # pylint: disable=E0702
 
 def is_host_resolvable(fqdn):
+if not isinstance(fqdn, DNSName):
+fqdn = DNSName(fqdn)
 for rdtype in (rdatatype.A, rdatatype.):
 try:
-resolver.query(fqdn, rdtype)
+resolver.query(fqdn.make_absolute(), rdtype)
 except DNSException:
 continue
 else:
-- 
2.5.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 508] install: export KRA agent PEM file in ipa-kra-install

[Martin Babinsky]

On 11/19/2015 09:07 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




ACK

--
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

[Freeipa-devel] [PATCH 0097] fix critical error messages when adding KRA container that already exists

[Martin Babinsky]

https://fedorahosted.org/freeipa/ticket/5346

--
Martin^3 Babinsky
From cf880b128ca4a4b53b8d70d1dce7d7aadab130c8 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 10:24:40 +0100
Subject: [PATCH] suppress errors arising from adding existing LDAP entries
 during KRA install

https://fedorahosted.org/freeipa/ticket/5346
---
 ipaserver/install/krainstance.py | 14 --
 ipaserver/install/service.py |  9 +++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index e75860d2802bc54cdf7fc47fdcaee60842e1a350..417d4dda03acf50ced2f83eb78f517c347a40890 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -53,6 +53,8 @@ ADMIN_GROUPS = [
 'Security Domain Administrators'
 ]
 
+LDAPMOD_ERR_ALREADY_EXISTS = 68
+
 class KRAInstance(DogtagInstance):
 """
 We assume that the CA has already been installed, and we use the
@@ -310,8 +312,16 @@ class KRAInstance(DogtagInstance):
 conn.disconnect()
 
 def __add_vault_container(self):
-self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix})
-self.ldap_disconnect()
+try:
+self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix},
+   raise_on_err=True)
+except ipautil.CalledProcessError as e:
+if e.returncode == LDAPMOD_ERR_ALREADY_EXISTS:
+self.log.info("Vault container already exists")
+else:
+self.log.error("Failed to add vault container: {0}".format(e))
+finally:
+self.ldap_disconnect()
 
 def __apply_updates(self):
 sub_dict = {
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index b9e68121dda6ea0b52c9ad923fcd5c72a22598a4..c9f9ed7e0659bf5151a3ec84957c57927d971734 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -184,7 +184,7 @@ class Service(object):
 self.admin_conn.unbind()
 self.admin_conn = None
 
-def _ldap_mod(self, ldif, sub_dict=None):
+def _ldap_mod(self, ldif, sub_dict=None, raise_on_err=False):
 pw_name = None
 fd = None
 path = ipautil.SHARE_DIR + ldif
@@ -228,7 +228,12 @@ class Service(object):
 try:
 ipautil.run(args, nolog=nologlist)
 except ipautil.CalledProcessError as e:
-root_logger.critical("Failed to load %s: %s" % (ldif, str(e)))
+if raise_on_err:
+raise
+else:
+root_logger.critical(
+"Failed to load %s: %s" % (ldif, str(e)))
+
 finally:
 if pw_name:
 os.remove(pw_name)
-- 
2.4.3

From 7dc22c784a12cdd511c7760f2c9b0d37f1e2fe2a Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 Nov 2015 10:24:40 +0100
Subject: [PATCH] suppress errors arising from adding existing LDAP entries
 during KRA install

https://fedorahosted.org/freeipa/ticket/5346
---
 ipaserver/install/krainstance.py | 14 --
 ipaserver/install/service.py | 10 +++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index 69fe636732e6d3a8c1e0c460b641f061e519df92..67a510cafda29b5ced6a0cce6eb3115bbdf70f55 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -47,6 +47,8 @@ from ipapython.ipa_log_manager import log_mgr
 IPA_KRA_RECORD = "ipa-kra"
 
 
+LDAPMOD_ERR_ALREADY_EXISTS = 68
+
 class KRAInstance(DogtagInstance):
 """
 We assume that the CA has already been installed, and we use the
@@ -306,8 +308,16 @@ class KRAInstance(DogtagInstance):
 conn.disconnect()
 
 def __add_vault_container(self):
-self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix})
-self.ldap_disconnect()
+try:
+self._ldap_mod('vault.ldif', {'SUFFIX': self.suffix},
+   raise_on_err=True)
+except ipautil.CalledProcessError as e:
+if e.returncode == LDAPMOD_ERR_ALREADY_EXISTS:
+self.log.info("Vault container already exists")
+else:
+self.log.error("Failed to add vault container: {0}".format(e))
+finally:
+self.ldap_disconnect()
 
 def __apply_updates(self):
 sub_dict = {
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index f0eaee2c99d2949ca77659bf163a22f6785d9bc5..98e73c4dbbb70e0b7467cefcc853730ee95dad48 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -155,7 +155,7 @@ class Service(object):
 self.admin_conn.unbind()
 self.admin_conn = None
 
-def _ldap_mod(self, ldif, sub_dict=None):
+def _ldap_mod(self, ldif, sub_dict=None, raise_on_err=False):
 pw_name = None
 fd = None
 path = ipautil.SHARE_DIR + 

Re: [Freeipa-devel] [TESTS][PATCH 0006] Add comments to stageuser plugin tests

[Petr Viktorin]

On 11/19/2015 09:30 AM, Lenka Doudova wrote:
> On 11/18/2015 04:51 PM, Martin Babinsky wrote:
>> On 11/18/2015 02:16 PM, Lenka Doudova wrote:
>>> Hi,
>>>
>>> here's a patch that adds a few comments to stageuser tests in order to
>>> allow easier determining of a problem when tests fail.
>>>
>>> Lenka
>>>
>>>
>>
>> Hi Lenka,
>>
>> Firstly a technical detail: Python indexes lists from 0, so the
>> comments in 'options_ok' do not correctly map to the test names anyway.
>>
>> I am also not sure if this patch is worth reviewing and pushing as it
>> IMHO doesn't help in the identification of failed tests at all.
>>
>> This should be solved at more fundamental level.
>>
> Ouch, good point, I didn't realize. Sorry.
> 
> Anyway, the issue is that even if tests are run in verbose mode, you get
> output like this:
> 
> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser27]
> PASSED
> 
> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser28]
> PASSED
> 
> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser29]
> PASSED
> 
> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser210]
> PASSED
> 
> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser211]
> PASSED
> 
> ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser212]
> PASSED
> 
> 
> If some test fails, you can't really tell which command was the one
> responsible for the fail. This should be solved by
> https://fedorahosted.org/freeipa/ticket/5449. Until that's done, though,
> the only way to find out which command failed is looking at the code and
> finding which parameters were put into the command, which was not much
> possible without better commenting the test case (as I realized last
> week when David Kupka asked me to help him find the reason for failed
> tests).
> Obviously I can rewrite the tests so there's 27 separate test cases, one
> for each parameter, instead of one method that 'unwraps' into 27 test
> cases, which would entirely eliminate the confusion. So far I don't know
> of a way to put 27 similar test cases in one method which would allow
> easy recognition of the test cases.
> I'll wait with fixing the patch until further discussion.

Hello,
Pytest wants you to be a bit more explicit about how to name the
parameters. (It "hides" dicts by default, because large dicts would make
the output even more confusing than the numbers.)

Please try the attached patch.
Docs are at https://pytest.org/latest/fixture.html#parametrizing-a-fixture

-- 
Petr Viktorin
From 1d36f052102942dcf3dab24bf5ede504ec94ca2f Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Thu, 19 Nov 2015 10:27:47 +0100
Subject: [PATCH] stageuser tests: Use "str" to display fixture parameter names

---
 ipatests/test_xmlrpc/test_stageuser_plugin.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipatests/test_xmlrpc/test_stageuser_plugin.py b/ipatests/test_xmlrpc/test_stageuser_plugin.py
index 43c59b7c7ec2902064fc363c66e98cc98e8b9e17..21bfe5f7fa9a39e3af1cdb14c48e821901765d39 100644
--- a/ipatests/test_xmlrpc/test_stageuser_plugin.py
+++ b/ipatests/test_xmlrpc/test_stageuser_plugin.py
@@ -329,7 +329,7 @@ def stageduser(request):
 return tracker.make_fixture(request)
 
 
-@pytest.fixture(scope='class', params=options_ok)
+@pytest.fixture(scope='class', params=options_ok, ids=str)
 def stageduser2(request):
 tracker = StageUserTracker(u'suser2', u'staged', u'user', **request.param)
 return tracker.make_fixture_activate(request)
-- 
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] [TESTS][PATCH 0006] Add comments to stageuser plugin tests

[Lenka Doudova]

On 11/18/2015 04:51 PM, Martin Babinsky wrote:

On 11/18/2015 02:16 PM, Lenka Doudova wrote:

Hi,

here's a patch that adds a few comments to stageuser tests in order to
allow easier determining of a problem when tests fail.

Lenka




Hi Lenka,

Firstly a technical detail: Python indexes lists from 0, so the 
comments in 'options_ok' do not correctly map to the test names anyway.


I am also not sure if this patch is worth reviewing and pushing as it 
IMHO doesn't help in the identification of failed tests at all.


This should be solved at more fundamental level.


Ouch, good point, I didn't realize. Sorry.

Anyway, the issue is that even if tests are run in verbose mode, you get 
output like this:


ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser27] 
PASSED


ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser28] 
PASSED


ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser29] 
PASSED


ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser210] 
PASSED


ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser211] 
PASSED


ipatests/test_xmlrpc/test_stageuser_plugin.py::TestStagedUser::test_create_attr[stageduser212] 
PASSED



If some test fails, you can't really tell which command was the one 
responsible for the fail. This should be solved by 
https://fedorahosted.org/freeipa/ticket/5449. Until that's done, though, 
the only way to find out which command failed is looking at the code and 
finding which parameters were put into the command, which was not much 
possible without better commenting the test case (as I realized last 
week when David Kupka asked me to help him find the reason for failed 
tests).
Obviously I can rewrite the tests so there's 27 separate test cases, one 
for each parameter, instead of one method that 'unwraps' into 27 test 
cases, which would entirely eliminate the confusion. So far I don't know 
of a way to put 27 similar test cases in one method which would allow 
easy recognition of the test cases.

I'll wait with fixing the patch until further discussion.

Lenka
-- 
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 508] install: export KRA agent PEM file in ipa-kra-install

[Jan Cholasta]

Hi,

the attached patch fixes .

Honza

--
Jan Cholasta
From d371aa19b35441a408a68034327e302237b71f9e Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Thu, 19 Nov 2015 08:50:05 +0100
Subject: [PATCH] install: export KRA agent PEM file in ipa-kra-install

https://fedorahosted.org/freeipa/ticket/5462
---
 ipaserver/install/krainstance.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ipaserver/install/krainstance.py b/ipaserver/install/krainstance.py
index e75860d..ed47be7 100644
--- a/ipaserver/install/krainstance.py
+++ b/ipaserver/install/krainstance.py
@@ -266,6 +266,8 @@ class KRAInstance(DogtagInstance):
 
 shutil.move(paths.KRA_BACKUP_KEYS_P12, paths.KRACERT_P12)
 
+export_kra_agent_pem()
+
 self.log.debug("completed creating KRA instance")
 
 def __create_kra_agent(self):
-- 
2.4.3

-- 
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