Re: [Freeipa-devel] [PATCH 154] ipa-kdb: map_groups() consider all results

2016-02-02 Thread Alexander Bokovoy

On Mon, 01 Feb 2016, Jakub Hrozek wrote:

On Tue, Jan 05, 2016 at 07:55:33PM +0100, Sumit Bose wrote:

Hi,

to find out to which local group a external user is mapped we do a
dereference search over the external groups with the SIDs related to the
external user. If a SID is mapped to more than one external group we
currently consider only the first returned match. With this patch all
results are taken into account. This makes sure all expected local group
memberships are added to the PAC which resolves
https://fedorahosted.org/freeipa/ticket/5573.


I tested with an AD user who was a member of several IPA external groups. All
groups were displayed.  We also have positive feedback from several users
who applied this patch.

The code looks good to me as well, Sumit explained some parts I didn't
understand on IRC.

ACK from me..

... and ACK from me too.
--
/ Alexander Bokovoy

--
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] CONFIGURE: Replace obsolete macros

2016-02-02 Thread Lukas Slebodnik
ehlo,

The AC_PROG_LIBTOOL macro is obsoleted by since libtool-2.0
which is already in rhel6+

https://fedorahosted.org/FedoraReview/wiki/AutoTools

simple patch is attached

LS
>From 079fbfa32cb7c70d76828d96f1db3ed05e7e10c0 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 2 Feb 2016 09:09:03 +0100
Subject: [PATCH] CONFIGURE: Replace obsolete macros

The AC_PROG_LIBTOOL macro is obsoleted by since libtool-2.0
which is already in rhel6+

https://fedorahosted.org/FedoraReview/wiki/AutoTools
---
 asn1/configure.ac| 2 +-
 client/configure.ac  | 3 +--
 daemons/configure.ac | 2 +-
 install/configure.ac | 1 -
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/asn1/configure.ac b/asn1/configure.ac
index 
c3e398ea20d4112ed8308ba5eb822ab22d256436..c0209142821482b1e76c8eac3d9179224847ff38
 100644
--- a/asn1/configure.ac
+++ b/asn1/configure.ac
@@ -6,7 +6,7 @@ AC_INIT([ipa-server],
 
 AC_CONFIG_HEADERS([config.h])
 AC_PROG_CC_C99
-AC_PROG_LIBTOOL
+LT_INIT
 
 AM_INIT_AUTOMAKE([foreign])
 
diff --git a/client/configure.ac b/client/configure.ac
index 
8e3a71f7b4c08f4c608606bd8f3ef846daeb9a4c..58f23afa71e9d377afd52c26b6e4c1dfb7b404a6
 100644
--- a/client/configure.ac
+++ b/client/configure.ac
@@ -3,8 +3,7 @@ m4_include(version.m4)
 AC_INIT([ipa-client],
 IPA_VERSION,
 [https://hosted.fedoraproject.org/projects/freeipa/newticket])
-LT_INIT()
-AC_PROG_LIBTOOL
+LT_INIT
 
 AC_CONFIG_HEADERS([config.h])
 AC_CONFIG_SUBDIRS([../asn1])
diff --git a/daemons/configure.ac b/daemons/configure.ac
index 
f2eebee51a0b5fd0648c835fd1f44667c5e49068..2a1f6aa8a24188c9b5f2d12fc25dbab079c2247c
 100644
--- a/daemons/configure.ac
+++ b/daemons/configure.ac
@@ -14,7 +14,7 @@ AM_MAINTAINER_MODE
 AC_PROG_CC_C99
 AC_STDC_HEADERS
 AC_DISABLE_STATIC
-AC_PROG_LIBTOOL
+LT_INIT
 
 AC_HEADER_STDC
 
diff --git a/install/configure.ac b/install/configure.ac
index 
cf19758a1e24c0682db954bf910120dbd31a05fa..b5f77bf8c737a437fe78ec5bfdc95599fce99760
 100644
--- a/install/configure.ac
+++ b/install/configure.ac
@@ -13,7 +13,6 @@ AM_MAINTAINER_MODE
 #AC_PROG_CC
 #AC_STDC_HEADERS
 #AC_DISABLE_STATIC
-#AC_PROG_LIBTOOL
 
 #AC_HEADER_STDC
 
-- 
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 0132] always start certmonger during IPA server configuration upgrade

2016-02-02 Thread Jan Cholasta

On 1.2.2016 14:54, Martin Basti wrote:



On 01.02.2016 13:55, Martin Babinsky wrote:

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





LGTM, works for me, tested on both ca-less server and CA-full server.

Because patch is touching certmonger I would like to get final ACK from
Honza.


This is suspicious:

-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+http.configure_certmonger_renewal_guard()

Why is it necessary?

--
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 0133] re-add missing fixture import to the CA ACL plugin test

2016-02-02 Thread Milan KubĂ­k

On 02/02/2016 03:36 PM, Martin Babinsky wrote:

This was causing some of the CA ACL plugin tests to fail/error.

Possible NACK. This is not an officially supported way how to reuse 
fixtures. [1]
I was working on a fix, that would contain all of the needed fixtures in 
the module that requires them.
See patch attached. However, this way kind of breaks DRY principle. The 
official conftest.py way is not practical for us, though.



[1]: 
http://pytest.org/latest/fixture.html#using-fixtures-from-classes-modules-or-projects


--
Milan Kubik

From 73dc9e91605c9299e48cdf62ddc0eb4927471a57 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Tue, 2 Feb 2016 11:34:26 +0100
Subject: [PATCH] ipatests: Add missing certificate profile fixture

---
 ipatests/test_xmlrpc/test_caacl_plugin.py | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/ipatests/test_xmlrpc/test_caacl_plugin.py b/ipatests/test_xmlrpc/test_caacl_plugin.py
index 85c7072a0bc483822b9b5c201d61932a423dd3d8..f20b02b295024313008be7b75bdcae2ade70b4ff 100644
--- a/ipatests/test_xmlrpc/test_caacl_plugin.py
+++ b/ipatests/test_xmlrpc/test_caacl_plugin.py
@@ -11,12 +11,21 @@ import pytest
 from ipalib import errors
 from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test
 
-# reuse the fixture
+from ipatests.test_xmlrpc.tracker.certprofile_plugin import CertprofileTracker
 from ipatests.test_xmlrpc.tracker.caacl_plugin import CAACLTracker
 from ipatests.test_xmlrpc.tracker.stageuser_plugin import StageUserTracker
 
 
 @pytest.fixture(scope='class')
+def default_profile(request):
+name = 'caIPAserviceCert'
+desc = u'Standard profile for network services'
+tracker = CertprofileTracker(name, store=True, desc=desc)
+tracker.track_create()
+return tracker
+
+
+@pytest.fixture(scope='class')
 def default_acl(request):
 name = u'hosts_services_caIPAserviceCert'
 tracker = CAACLTracker(name, service_category=u'all', host_category=u'all')
-- 
2.7.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] typo in service

2016-02-02 Thread Martin Basti
I acked patch attached to ticket: 
https://fedorahosted.org/freeipa/ticket/5659


Pushed to master: d85d70947361abd3822b8f42fa3de16e26e87d57

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


[Freeipa-devel] [PATCH 0133] re-add missing fixture import to the CA ACL plugin test

2016-02-02 Thread Martin Babinsky

This was causing some of the CA ACL plugin tests to fail/error.

--
Martin^3 Babinsky
From dd0f3a8523f3092034a863c2a525aef03672e5dd Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Tue, 2 Feb 2016 15:30:27 +0100
Subject: [PATCH] re-add missing fixture import to the CA ACL plugin test

---
 ipatests/test_xmlrpc/test_caacl_plugin.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ipatests/test_xmlrpc/test_caacl_plugin.py b/ipatests/test_xmlrpc/test_caacl_plugin.py
index 85c7072a0bc483822b9b5c201d61932a423dd3d8..33d2d6b2294f90f047ee60330ede9b750f8245ce 100644
--- a/ipatests/test_xmlrpc/test_caacl_plugin.py
+++ b/ipatests/test_xmlrpc/test_caacl_plugin.py
@@ -12,6 +12,9 @@ from ipalib import errors
 from ipatests.test_xmlrpc.xmlrpc_test import XMLRPC_test
 
 # reuse the fixture
+# pylint: disable=unused-import
+from ipatests.test_xmlrpc.test_certprofile_plugin import default_profile
+# pylint: enable=unused-import
 from ipatests.test_xmlrpc.tracker.caacl_plugin import CAACLTracker
 from ipatests.test_xmlrpc.tracker.stageuser_plugin import StageUserTracker
 
-- 
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 0132] always start certmonger during IPA server configuration upgrade

2016-02-02 Thread Martin Babinsky

On 02/02/2016 09:33 AM, Jan Cholasta wrote:

On 1.2.2016 14:54, Martin Basti wrote:



On 01.02.2016 13:55, Martin Babinsky wrote:

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





LGTM, works for me, tested on both ca-less server and CA-full server.

Because patch is touching certmonger I would like to get final ACK from
Honza.


This is suspicious:

-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+http.configure_certmonger_renewal_guard()

Why is it necessary?



I don't know, you tell me:
b9ae7690489368ead9f4983d386fa210dc265dfa

--
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 0132] always start certmonger during IPA server configuration upgrade

2016-02-02 Thread Jan Cholasta

On 2.2.2016 11:41, Martin Babinsky wrote:

On 02/02/2016 09:33 AM, Jan Cholasta wrote:

On 1.2.2016 14:54, Martin Basti wrote:



On 01.02.2016 13:55, Martin Babinsky wrote:

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





LGTM, works for me, tested on both ca-less server and CA-full server.

Because patch is touching certmonger I would like to get final ACK from
Honza.


This is suspicious:

-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+http.configure_certmonger_renewal_guard()

Why is it necessary?



I don't know, you tell me:
b9ae7690489368ead9f4983d386fa210dc265dfa


What I meant is why is the change necessary, not why is the original 
code necessary.


--
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 0085] Fix ipa-adtrust-install to always generate SRV records with FQDN

2016-02-02 Thread Petr Spacek
Hello,

Fix ipa-adtrust-install to always generate SRV records with FQDNs

Previous code failed in following setup:
* IPA domain = ipa.example.com
* IPA master = vm1.example.com
* IPA replica = vm2.example.com

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

-- 
Petr^2 Spacek
From d4d13e003b9fb7153e27691d67246b0dfa4b51ac Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 2 Feb 2016 17:20:21 +0100
Subject: [PATCH] Fix ipa-adtrust-install to always generate SRV records with
 FQDNs

Previous code failed in following setup:
* IPA domain = ipa.example.com
* IPA master = vm1.example.com
* IPA replica = vm2.example.com

https://fedorahosted.org/freeipa/ticket/5663
---
 ipaserver/install/adtrustinstance.py | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index 118f848cf33c3126d42ccda555a8308f52d2c390..9e7e001f7c505d09d5a61164399e9ed256ae9865 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -571,12 +571,7 @@ class ADTRUSTInstance(service.Service):
 """
 
 zone = self.domain_name
-host, host_domain = self.fqdn.split(".", 1)
-
-if normalize_zone(zone) == normalize_zone(host_domain):
-host_in_rr = host
-else:
-host_in_rr = normalize_zone(self.fqdn)
+host_in_rr = normalize_zone(self.fqdn)
 
 priority = 0
 
@@ -707,7 +702,7 @@ class ADTRUSTInstance(service.Service):
 # this is CIFS service of a different host in our
 # REALM, we need to remember it to announce via
 # SRV records for _msdcs
-self.cifs_hosts.append(fqdn.split(".")[0])
+self.cifs_hosts.append(normalize_zone(fqdn))
 
 except Exception as e:
 root_logger.critical("Checking replicas for cifs principals failed with error '%s'" % e)
-- 
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] IPA-SAM: Fix build with samba 4.4

2016-02-02 Thread Alexander Bokovoy

On Tue, 02 Feb 2016, Lukas Slebodnik wrote:

On (29/01/16 12:12), Lukas Slebodnik wrote:

ehlo,

attached patch shoudl fix build on fedora-24.
It blocks static analysis scan.

Even though it unblock build on fedora-24
the solution is not ideal. It's possible that some changes
need to be done in samba side as well.
(missing prototypes for trim_string, smb_xstrdup

LS



From f9057ca98557094a4db84ac072ee9efd02a4ff79 Mon Sep 17 00:00:00 2001

From: Lukas Slebodnik 
Date: Fri, 29 Jan 2016 10:40:18 +0100
Subject: [PATCH 1/3] IPA-SAM: Fix build with samba 4.4

samba_util.h is not shipped with samba-4.4
and it was indirectly included by "ndr.h"

Some functions have prototypes in different header file
"util/talloc_stack.h" and other does not have declarations
in other header file. But they are still part of libsamba-util.so

sh$ objdump -T /usr/lib64/libsamba-util.so.0.0.1 | grep -E "trim_s|xstrdup"
00022200 gDF .text  001f  SAMBA_UTIL_0.0.1 smb_xstrdup
000223b0 gDF .text  019d  SAMBA_UTIL_0.0.1 trim_string

ipa_sam.c: In function 'ldapsam_uid_to_sid':
ipa_sam.c:836:24: warning: implicit declaration of function 'talloc_stackframe'
 [-Wimplicit-function-declaration]
 TALLOC_CTX *tmp_ctx = talloc_stackframe();
   ^
ipa_sam.c: In function 'pdb_init_ipasam':
ipa_sam.c:4493:2: warning: implicit declaration of function 'trim_string'
 [-Wimplicit-function-declaration]
 trim_string( uri, "\"", "\"" );
 ^
ipa_sam.c:4580:26: warning: implicit declaration of function 'smb_xstrdup'
  [-Wimplicit-function-declaration]
 ldap_state->domain_dn = smb_xstrdup(dn);
 ^
---
daemons/ipa-sam/ipa_sam.c | 6 ++
1 file changed, 6 insertions(+)

diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index 
7274d600b532f101e8a614a47eea7632ed70..871775b0a19e9c273652ff7a0b497d86bb866aa6
 100644
--- a/daemons/ipa-sam/ipa_sam.c
+++ b/daemons/ipa-sam/ipa_sam.c
@@ -19,6 +19,12 @@
#include 
#include 
#include 
+#include 
+
+#ifndef _SAMBA_UTIL_H_
+bool trim_string(char *s, const char *front, const char *back);
+char *smb_xstrdup(const char *s);
+#endif

Bump,
it would be good to unblock static analysis scans.

Yes, ACK.
--
/ Alexander Bokovoy

--
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 0085] Fix ipa-adtrust-install to always generate SRV records with FQDN

2016-02-02 Thread Alexander Bokovoy

On Tue, 02 Feb 2016, Petr Spacek wrote:

Hello,

Fix ipa-adtrust-install to always generate SRV records with FQDNs

Previous code failed in following setup:
* IPA domain = ipa.example.com
* IPA master = vm1.example.com
* IPA replica = vm2.example.com

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

--
Petr^2 Spacek



From d4d13e003b9fb7153e27691d67246b0dfa4b51ac Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 2 Feb 2016 17:20:21 +0100
Subject: [PATCH] Fix ipa-adtrust-install to always generate SRV records with
FQDNs

Previous code failed in following setup:
* IPA domain = ipa.example.com
* IPA master = vm1.example.com
* IPA replica = vm2.example.com

https://fedorahosted.org/freeipa/ticket/5663
---
ipaserver/install/adtrustinstance.py | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/adtrustinstance.py 
b/ipaserver/install/adtrustinstance.py
index 
118f848cf33c3126d42ccda555a8308f52d2c390..9e7e001f7c505d09d5a61164399e9ed256ae9865
 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -571,12 +571,7 @@ class ADTRUSTInstance(service.Service):
"""

zone = self.domain_name
-host, host_domain = self.fqdn.split(".", 1)
-
-if normalize_zone(zone) == normalize_zone(host_domain):
-host_in_rr = host
-else:
-host_in_rr = normalize_zone(self.fqdn)
+host_in_rr = normalize_zone(self.fqdn)

priority = 0

@@ -707,7 +702,7 @@ class ADTRUSTInstance(service.Service):
# this is CIFS service of a different host in our
# REALM, we need to remember it to announce via
# SRV records for _msdcs
-self.cifs_hosts.append(fqdn.split(".")[0])
+self.cifs_hosts.append(normalize_zone(fqdn))

except Exception as e:
root_logger.critical("Checking replicas for cifs principals failed with 
error '%s'" % e)
--
2.5.0

ACK. 


--
/ Alexander Bokovoy

--
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] IPA-SAM: Fix build with samba 4.4

2016-02-02 Thread Lukas Slebodnik
On (29/01/16 12:12), Lukas Slebodnik wrote:
>ehlo,
>
>attached patch shoudl fix build on fedora-24.
>It blocks static analysis scan.
>
>Even though it unblock build on fedora-24
>the solution is not ideal. It's possible that some changes
>need to be done in samba side as well.
>(missing prototypes for trim_string, smb_xstrdup
>
>LS

>>From f9057ca98557094a4db84ac072ee9efd02a4ff79 Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik 
>Date: Fri, 29 Jan 2016 10:40:18 +0100
>Subject: [PATCH 1/3] IPA-SAM: Fix build with samba 4.4
>
>samba_util.h is not shipped with samba-4.4
>and it was indirectly included by "ndr.h"
>
>Some functions have prototypes in different header file
>"util/talloc_stack.h" and other does not have declarations
>in other header file. But they are still part of libsamba-util.so
>
>sh$ objdump -T /usr/lib64/libsamba-util.so.0.0.1 | grep -E "trim_s|xstrdup"
>00022200 gDF .text  001f  SAMBA_UTIL_0.0.1 smb_xstrdup
>000223b0 gDF .text  019d  SAMBA_UTIL_0.0.1 trim_string
>
>ipa_sam.c: In function 'ldapsam_uid_to_sid':
>ipa_sam.c:836:24: warning: implicit declaration of function 'talloc_stackframe'
>  [-Wimplicit-function-declaration]
>  TALLOC_CTX *tmp_ctx = talloc_stackframe();
>^
>ipa_sam.c: In function 'pdb_init_ipasam':
>ipa_sam.c:4493:2: warning: implicit declaration of function 'trim_string'
>  [-Wimplicit-function-declaration]
>  trim_string( uri, "\"", "\"" );
>  ^
>ipa_sam.c:4580:26: warning: implicit declaration of function 'smb_xstrdup'
>   [-Wimplicit-function-declaration]
>  ldap_state->domain_dn = smb_xstrdup(dn);
>  ^
>---
> daemons/ipa-sam/ipa_sam.c | 6 ++
> 1 file changed, 6 insertions(+)
>
>diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
>index 
>7274d600b532f101e8a614a47eea7632ed70..871775b0a19e9c273652ff7a0b497d86bb866aa6
> 100644
>--- a/daemons/ipa-sam/ipa_sam.c
>+++ b/daemons/ipa-sam/ipa_sam.c
>@@ -19,6 +19,12 @@
> #include 
> #include 
> #include 
>+#include 
>+
>+#ifndef _SAMBA_UTIL_H_
>+bool trim_string(char *s, const char *front, const char *back);
>+char *smb_xstrdup(const char *s);
>+#endif
Bump,
it would be good to unblock static analysis scans.

LS

-- 
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 0011-0012][RFE] ipa-replica-manage: automatically clean dangling RUVs

2016-02-02 Thread Martin Basti



On 29.01.2016 08:42, Stanislav Laznicka wrote:

On 01/26/2016 06:56 PM, Martin Basti wrote:



On 25.01.2016 16:41, Stanislav Laznicka wrote:

Hi,

Worked those comments into the code. Also added a bit different info 
message in clean_ruv with ca=True (ipa-replica-manage:430).


Also adding stepst to reproduce:
1. Create a master and some replica (3 replicas is a good solution - 
1 with CA, 1 without, 1 to be dangling (with CA))

2. Change domain level to 0 and ipactl restart
3. Remove the "dangling-to-be" replica from masters.ipa.etc and from 
both ipaca and domain subtrees in mapping tree.config

4. Try to remove the dangling ruvs with the command

Cheers,
Standa


On 01/22/2016 01:22 PM, Martin Basti wrote:

Hello,

I have a few comments

PATCH Automatically detect and remove dangling RUVs

1)
+# get the Directory Manager password
+if options.dirman_passwd:
+dirman_passwd = options.dirman_passwd
+else:
+dirman_passwd = installutils.read_password('Directory 
Manager',

+confirm=False, validate=False, retry=False)
+if dirman_passwd is None:
+sys.exit('Directory Manager password is required')
+
+options.dirman_passwd = dirman_passwd

IMO you need only else branch here

if not options.dirman_password:
dirman_passwd = installutils.read_password('Directory Manager',
confirm=False, validate=False, retry=False)
if dirman_passwd is None:
sys.exit('Directory Manager password is required')
   options.dirman_passwd = dirman_passwd


2)
We should use new formatting in new code (more times in code)

+sys.exit(
+"Failed to get data from '%s' while trying to list 
replicas: %s" %

+(host, e)
+)

sys.exit(
"Failed to get data from '{host}' while trying to list 
replicas: {e}".format(

  host=host, e=e
)
)

3)
+# get all masters
+masters_dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 
'etc'),

+ipautil.realm_to_suffix(realm))

IMO you should use constants:
 masters_dn = DN(api.env.container_masters, api.env.basedn)

4)
+# Get realm string for config tree
+s = realm.split('.')
+s = ['dc={dc},'.format(dc=x.lower()) for x in s]
+realm_config = DN(('cn', ''.join(s)[0:-1]))

Can be api.env.basedn used instead of this block of code?

5)
+masters = [x.single_value['cn'] for x in masters]

+for master in masters:

is there any reason why not iterate over the keys in info dict?

for master_name, master_data/values/whatever in info.items():
   master_data['online'] = True

Looks better than: info[master]['online'] = True

6)
I asked python gurus, for empty lists and dicts, please use [] and 
{} instead of list() and dict()

It is preferred and faster.

7)
+if(info[master]['ca']):
+entry = conn.get_entry(csreplica_dn)
+csruv = (master, 
entry.single_value.get('nsDS5ReplicaID'))

+if csruv not in csruvs:
+csruvs.append(csruv)

I dont like too much adding tuples into list and then doing search 
there, but it is as designed


However can you use set() instead of list when the purpose of 
variable is only testing existence?


related to:
csruvs
ruvs
offlines
clean_list
cleaned

8)
conn in finally block may be undefined

9)
unused local variables

clean_list
entry on line 570

10)
optional, comment what keys means in info structure




Hello,

1)
I accept your silence as the following code cannot use nothing from 
api.env

+# Get realm string for config tree
+s = realm.split('.')
+s = ['dc={dc},'.format(dc=x.lower()) for x in s]
+realm_config = DN(('cn', ''.join(s)[0:-1]))

but then please use:
s = ['dc={dc}'.format(dc=x.lower()) for x in s]
realm_config = DN(('cn', ','.join(s)))

But I still think that api.env.basedn can be used, because it 
contains the same format as you need

realm_config = DN(('cn', api.env.basedn))

Sorry, forgot to mention that in the last email. However, turns out 
you are right. I didn't think DN works like this but it does, so this 
is now in it.

2) nitpick
ca_dn = DN(('cn', 'ca'), DN(master.dn))

AFAIK can be just

ca_dn = DN(('cn', 'ca'), master.dn)


My bad, fixed.

3) uber nitpick
This is PEP8 valid, but somehow inconsistent with the rest of code 
and it hit my eyes


print('\t\tid: {id}, hostname: {host}'
.format(id=csruv[1], host=csruv[0])
)

we use in code

print(
   something1,
   something2
)

or

print(something1,
something2)


And that shouldn't be there anymore, too :)

Otherwise LGTM




ACK

Pushed to:
ipa-4-3: 44018142747e933f7d680c8d7bacfbd883ffddf6
master: c8eabaff9ef49d3f51b02d613c25c09bf155bb3c

-- 
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 0132] always start certmonger during IPA server configuration upgrade

2016-02-02 Thread Martin Basti



On 02.02.2016 12:18, Jan Cholasta wrote:

On 2.2.2016 11:46, Jan Cholasta wrote:

On 2.2.2016 11:41, Martin Babinsky wrote:

On 02/02/2016 09:33 AM, Jan Cholasta wrote:

On 1.2.2016 14:54, Martin Basti wrote:



On 01.02.2016 13:55, Martin Babinsky wrote:

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





LGTM, works for me, tested on both ca-less server and CA-full server.

Because patch is touching certmonger I would like to get final ACK 
from

Honza.


This is suspicious:

-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+http.configure_certmonger_renewal_guard()

Why is it necessary?



I don't know, you tell me:
b9ae7690489368ead9f4983d386fa210dc265dfa


What I meant is why is the change necessary, not why is the original
code necessary.


Ah, the original code didn't have the condition. LGTM then.


 LGTM + LGTM = ACK :)

Pushed to:
master: 612f4aa9003658f9a494ec327d50ec5a0592f7b4
ipa-4-3: d99552a8a9f855a7c5e00c4b0736061e05d6ed31
ipa-4-2: 3664efa31edf0dff6dd3410e2eccd12c9cd25782



--
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 0132] always start certmonger during IPA server configuration upgrade

2016-02-02 Thread Jan Cholasta

On 2.2.2016 11:46, Jan Cholasta wrote:

On 2.2.2016 11:41, Martin Babinsky wrote:

On 02/02/2016 09:33 AM, Jan Cholasta wrote:

On 1.2.2016 14:54, Martin Basti wrote:



On 01.02.2016 13:55, Martin Babinsky wrote:

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





LGTM, works for me, tested on both ca-less server and CA-full server.

Because patch is touching certmonger I would like to get final ACK from
Honza.


This is suspicious:

-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+http.configure_certmonger_renewal_guard()

Why is it necessary?



I don't know, you tell me:
b9ae7690489368ead9f4983d386fa210dc265dfa


What I meant is why is the change necessary, not why is the original
code necessary.


Ah, the original code didn't have the condition. LGTM then.

--
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 0085] Fix ipa-adtrust-install to always generate SRV records with FQDN

2016-02-02 Thread Petr Vobornik

On 02/02/2016 05:55 PM, Alexander Bokovoy wrote:

On Tue, 02 Feb 2016, Petr Spacek wrote:

Hello,

Fix ipa-adtrust-install to always generate SRV records with FQDNs

Previous code failed in following setup:
* IPA domain = ipa.example.com
* IPA master = vm1.example.com
* IPA replica = vm2.example.com

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

--
Petr^2 Spacek



From d4d13e003b9fb7153e27691d67246b0dfa4b51ac Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 2 Feb 2016 17:20:21 +0100
Subject: [PATCH] Fix ipa-adtrust-install to always generate SRV
records with
FQDNs

Previous code failed in following setup:
* IPA domain = ipa.example.com
* IPA master = vm1.example.com
* IPA replica = vm2.example.com

https://fedorahosted.org/freeipa/ticket/5663
---
ipaserver/install/adtrustinstance.py | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/adtrustinstance.py
b/ipaserver/install/adtrustinstance.py
index
118f848cf33c3126d42ccda555a8308f52d2c390..9e7e001f7c505d09d5a61164399e9ed256ae9865
100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -571,12 +571,7 @@ class ADTRUSTInstance(service.Service):
"""

zone = self.domain_name
-host, host_domain = self.fqdn.split(".", 1)
-
-if normalize_zone(zone) == normalize_zone(host_domain):
-host_in_rr = host
-else:
-host_in_rr = normalize_zone(self.fqdn)
+host_in_rr = normalize_zone(self.fqdn)

priority = 0

@@ -707,7 +702,7 @@ class ADTRUSTInstance(service.Service):
# this is CIFS service of a different host
in our
# REALM, we need to remember it to
announce via
# SRV records for _msdcs
-self.cifs_hosts.append(fqdn.split(".")[0])
+self.cifs_hosts.append(normalize_zone(fqdn))

except Exception as e:
root_logger.critical("Checking replicas for cifs
principals failed with error '%s'" % e)
--
2.5.0


ACK.


Pushed to:
master: 72e4a360fc0a31dd53d257350b3f490cc3ed07e4
ipa-4-3: 0256f6be1d241645cc974b0bfb5767de8aa2a6fc
--
Petr Vobornik

--
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 154] ipa-kdb: map_groups() consider all results

2016-02-02 Thread Petr Vobornik

On 02/02/2016 09:21 AM, Alexander Bokovoy wrote:

On Mon, 01 Feb 2016, Jakub Hrozek wrote:

On Tue, Jan 05, 2016 at 07:55:33PM +0100, Sumit Bose wrote:

Hi,

to find out to which local group a external user is mapped we do a
dereference search over the external groups with the SIDs related to the
external user. If a SID is mapped to more than one external group we
currently consider only the first returned match. With this patch all
results are taken into account. This makes sure all expected local group
memberships are added to the PAC which resolves
https://fedorahosted.org/freeipa/ticket/5573.


I tested with an AD user who was a member of several IPA external
groups. All
groups were displayed.  We also have positive feedback from several users
who applied this patch.

The code looks good to me as well, Sumit explained some parts I didn't
understand on IRC.

ACK from me..

... and ACK from me too.


Pushed to:
master: 348c400484cafe4969c3fa0c9f0c6f6e150df821
ipa-4-3: d6e81749c3d5904cfae59921802895b6cff528ff
ipa-4-2: d70c86f71a405984db24947a8e1c0caebdc499c8
--
Petr Vobornik

--
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] IPA-SAM: Fix build with samba 4.4

2016-02-02 Thread Petr Vobornik

On 02/02/2016 05:54 PM, Alexander Bokovoy wrote:

On Tue, 02 Feb 2016, Lukas Slebodnik wrote:

On (29/01/16 12:12), Lukas Slebodnik wrote:

ehlo,

attached patch shoudl fix build on fedora-24.
It blocks static analysis scan.

Even though it unblock build on fedora-24
the solution is not ideal. It's possible that some changes
need to be done in samba side as well.
(missing prototypes for trim_string, smb_xstrdup

LS



From f9057ca98557094a4db84ac072ee9efd02a4ff79 Mon Sep 17 00:00:00 2001

From: Lukas Slebodnik 
Date: Fri, 29 Jan 2016 10:40:18 +0100
Subject: [PATCH 1/3] IPA-SAM: Fix build with samba 4.4

samba_util.h is not shipped with samba-4.4
and it was indirectly included by "ndr.h"

Some functions have prototypes in different header file
"util/talloc_stack.h" and other does not have declarations
in other header file. But they are still part of libsamba-util.so

sh$ objdump -T /usr/lib64/libsamba-util.so.0.0.1 | grep -E
"trim_s|xstrdup"
00022200 gDF .text  001f  SAMBA_UTIL_0.0.1
smb_xstrdup
000223b0 gDF .text  019d  SAMBA_UTIL_0.0.1
trim_string

ipa_sam.c: In function 'ldapsam_uid_to_sid':
ipa_sam.c:836:24: warning: implicit declaration of function
'talloc_stackframe'
 [-Wimplicit-function-declaration]
 TALLOC_CTX *tmp_ctx = talloc_stackframe();
   ^
ipa_sam.c: In function 'pdb_init_ipasam':
ipa_sam.c:4493:2: warning: implicit declaration of function
'trim_string'
 [-Wimplicit-function-declaration]
 trim_string( uri, "\"", "\"" );
 ^
ipa_sam.c:4580:26: warning: implicit declaration of function
'smb_xstrdup'
  [-Wimplicit-function-declaration]
 ldap_state->domain_dn = smb_xstrdup(dn);
 ^
---
daemons/ipa-sam/ipa_sam.c | 6 ++
1 file changed, 6 insertions(+)

diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index
7274d600b532f101e8a614a47eea7632ed70..871775b0a19e9c273652ff7a0b497d86bb866aa6
100644
--- a/daemons/ipa-sam/ipa_sam.c
+++ b/daemons/ipa-sam/ipa_sam.c
@@ -19,6 +19,12 @@
#include 
#include 
#include 
+#include 
+
+#ifndef _SAMBA_UTIL_H_
+bool trim_string(char *s, const char *front, const char *back);
+char *smb_xstrdup(const char *s);
+#endif

Bump,
it would be good to unblock static analysis scans.

Yes, ACK.


Pushed to:
master: 017b343e13bbfdd0d9951417be8dac4614cb1416
ipa-4-3: 69cc457504306ec1f90d844efa1e4b422564f06d


--
Petr Vobornik

--
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 webui: Issue New Certificate dialogs validates data

2016-02-02 Thread Petr Vobornik

On 01/29/2016 10:58 AM, Pavel Vomacka wrote:



On 01/28/2016 07:28 PM, Petr Vobornik wrote:

Hi, there are few issues, NACK.

1. this patch uses tabs instead of spaces, previous was correct

Fixed.


2. code which focuses first invalid field could be replaced by:
  widget_mod.focus_invalid(that);

The old loop is replaced by this method.


3. there is a convention that field and widget names uses the same
name as the param, therefore 'textarea_cert' should be 'csr'. There is
no convention for messages in html widget but it might be better to
use a name reflecting purpose and not implementation. Instead of
'message_html_widget' use 'instructions' or just 'message' -
consistent with spec option.


Fixed. I chose 'message' instead of 'message_html_widget'.

Also I've filed: https://fedorahosted.org/freeipa/ticket/5652

Pavel Vomacka




ACK

Pushed to master: fb3b7f7d93060d42e9cc79768f72e0b2d4b0481f
--
Petr Vobornik

--
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 0022-23] Coverity patches

2016-02-02 Thread Stanislav Laznicka

On 02/01/2016 02:24 PM, Jan Cholasta wrote:

On 1.2.2016 12:11, Petr Spacek wrote:

On 1.2.2016 09:03, Jan Cholasta wrote:

Hi,

On 29.1.2016 15:49, Martin Basti wrote:



On 29.01.2016 15:49, Stanislav Laznicka wrote:
Reworded the commits so that they better reflect what's going on 
in those.


On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:

Hello,

I made some patches based on the Coverity report from 18.1.2016.

Cheers,
Standa




NACK, see my previous email


I don't think this deserves 9 patches, 1 would be sufficient enough.


I would rather have it is separate patches as these fixes are largely 
not

related. It will make bisecting easier.


Most of the fixes are cosmetic, which makes them related, and the rest 
are small isolated changes, so in reality it would hardly make 
bisecting easier and only increase the overhead. In the past we 
usually had put such fixes into a single patch and AFAIK nobody 
complained so far.


Squeezed the changes into two new patches, then. One for the very 
cosmetic changes, one for possible bugs.




Patch 0013:

1) I think this unreachable return is intentional, as indicated by 
the comment:


-#we shouldn't get here
-return [UNKNOWN_ERROR]


I would use
assert False, "we shouldn't get here"
neither we nor Coverity are confused when we hit the code path one day.

UNKNOWN_ERROR would pop up somewhere else and it will be harder to 
find out
why the hell the code behaves as mad. Traceback will clearly indicate 
that

there is a problem with the 'switch'.


Sure, my point is that returning None is no better than returning 
UNKNOWN_ERROR.


Added assert as suggested. There should still be no way of getting to 
it, though.


Petr^2 Spacek


2) How is this dead code?

-if options.mode == 'validate_pot' or options.mode == 
'validate_po':

+if options.mode in ('validate_pot', 'validate_po'):

-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):



Patch 0014-0015: LGTM


Actually scratch that, patch 0015 breaks correct code.

The dead code appears in the 'else' branch as the latter of these two 
conditions always evaluates to True. The first condition change is just 
a cosmetic one so that both of the conditions look the same.


Also removed the changes made in patch 0015.



Patch 0016: The original code is in fact correct.


Point taken, removed the change.


Patch 0017: This will break Python 3. The two branches are 
performing the same

action, but with different data types.

This might undergo further investigation in the future as there is no 
way how "bytes" instance could become an argument of this function (as 
suggested). Not even the newest Python 3 patches from pviktori mention 
this code.


Patch 0018: LGTM


Patch 0019: IMO the original code is better (None has a __class__ 
too, you know).



Made it more "Coverity friendly" yet nice enough modification.


Patch 0020: LGTM

It seems that there actually is a check that checks whether the input is 
correct. It is called ad-hoc but that might be the test feature. 
Therefore just added an assert so that Coverity does not complain.


Patch 0021: Please use the original error messages (there are no 
requests

being added to D-Bus, but to certmonger).


Honza





Added error messages that reflect the situation better, then.
From f7a4ba996960aa53af432a64489a8caf6baa1ee7 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 2 Feb 2016 12:50:26 +0100
Subject: [PATCH 1/2] Cosmetic changes to the code

ipadiscovery.py:  added assert should universe break
plugins/dns.py:   removed dead code
dnssec/ldapkeydb.py:  attribute assert in the proper object
test_automount_plugin.py: fixed possible close() on None
xmlrpc_test.py:   Coverity does not like accessing None.__class__

https://fedorahosted.org/freeipa/ticket/5661
---
 ipaclient/ipadiscovery.py | 2 +-
 ipalib/plugins/dns.py | 3 ---
 ipapython/dnssec/ldapkeydb.py | 2 +-
 ipatests/test_xmlrpc/test_automount_plugin.py | 2 ++
 ipatests/test_xmlrpc/xmlrpc_test.py   | 2 +-
 5 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/ipaclient/ipadiscovery.py b/ipaclient/ipadiscovery.py
index 45a71e190e56d33d51d37f16ae61a7b4c28df521..772add43a282838539b9b18dc60d3bba041bed1b 100644
--- a/ipaclient/ipadiscovery.py
+++ b/ipaclient/ipadiscovery.py
@@ -402,7 +402,7 @@ class IPADiscovery(object):
 return [0, thost, lrealms[0]]
 
 #we shouldn't get here
-return [UNKNOWN_ERROR]
+assert False, "Unknown error in ipadiscovery"
 
 except errors.DatabaseTimeout:
 root_logger.debug("LDAP Error: timeout")
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 0a9fa181347514f0cc11c3ab4a3e19864df4eec0..1892d29ec9f87d3b3f955ff8df6b154961fbce36 100644
--- a/ipalib/plugins/dns.py
+++