date:20151213


Hello,

patch 0374 fixes the ticket, but I found more issues with directory 
permission, I fixed them in 0375


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

Patches attached.
From ae0bcea3f6173bd6466d26a7d0cb2886029a10f6 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 9 Dec 2015 12:12:22 +0100
Subject: [PATCH] DNS: fix file permissions

With non default umask named-pkcs11 cannot access the softhsm token storage

https://fedorahosted.org/freeipa/ticket/5520
---
 ipaserver/install/dnskeysyncinstance.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipaserver/install/dnskeysyncinstance.py b/ipaserver/install/dnskeysyncinstance.py
index b2ccc027469a352c815963abfd0c0a61dd37297f..f2a976eecd2c4f6de1e12c46969c6d5addd79e41 100644
--- a/ipaserver/install/dnskeysyncinstance.py
+++ b/ipaserver/install/dnskeysyncinstance.py
@@ -201,7 +201,8 @@ class DNSKeySyncInstance(service.Service):
 # create dnssec directory
 if not os.path.exists(paths.IPA_DNSSEC_DIR):
 self.logger.debug("Creating %s directory", paths.IPA_DNSSEC_DIR)
-os.mkdir(paths.IPA_DNSSEC_DIR, 0o770)
+os.mkdir(paths.IPA_DNSSEC_DIR)
+os.chmod(paths.IPA_DNSSEC_DIR, 0o770)
 # chown ods:named
 os.chown(paths.IPA_DNSSEC_DIR, self.ods_uid, self.named_gid)
 
@@ -218,6 +219,7 @@ class DNSKeySyncInstance(service.Service):
 named_fd.truncate(0)
 named_fd.write(softhsm_conf_txt)
 named_fd.close()
+os.chmod(paths.DNSSEC_SOFTHSM2_CONF, 0o644)
 
 # setting up named to use softhsm2
 if not self.fstore.has_file(paths.SYSCONFIG_NAMED):
-- 
2.5.0

From 57f7841185e6e25d12ca83a537d2cb7184854a23 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 9 Dec 2015 13:40:04 +0100
Subject: [PATCH] Explicitly call chmod on newly created directories

Without calling os.chmod(), umask is effective and may cause that
directory is created with permission that causes failure.

This can be related to https://fedorahosted.org/freeipa/ticket/5520
---
 ipaplatform/base/services.py |  1 +
 ipaserver/install/cainstance.py  |  1 +
 ipaserver/install/ipa_backup.py  |  7 ---
 ipaserver/install/ipa_replica_prepare.py |  3 ++-
 ipaserver/install/ipa_restore.py | 10 ++
 5 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index da2f1011e34431664cd5c730668ae483b7bd0a1d..e6a0403b6edfb62a1d7f807fef93121718ba59f5 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -421,6 +421,7 @@ class SystemdService(PlatformService):
 try:
 if not ipautil.dir_exists(srv_tgt):
 os.mkdir(srv_tgt)
+os.mkdir(srv_tgt, 0o755)
 if os.path.exists(srv_lnk):
 # Remove old link
 os.unlink(srv_lnk)
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 2ca718a7b6799b7daf825918517a54852746a84f..56ec3fe74e8d4adfe17f46a62f705021f6a81f75 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -794,6 +794,7 @@ class CAInstance(DogtagInstance):
 
 if not ipautil.dir_exists(self.ra_agent_db):
 os.mkdir(self.ra_agent_db)
+os.chmod(self.ra_agent_db, 0o755)
 
 # Create the password file for this db
 hex_str = binascii.hexlify(os.urandom(10))
diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py
index 6d97ef13b383b9917fa70426a99463f8c14955e8..523cb9180f36a32f3d18547c9b5db86e913985d9 100644
--- a/ipaserver/install/ipa_backup.py
+++ b/ipaserver/install/ipa_backup.py
@@ -272,8 +272,8 @@ class Backup(admintool.AdminTool):
 os.chown(self.top_dir, pent.pw_uid, pent.pw_gid)
 os.chmod(self.top_dir, 0o750)
 self.dir = os.path.join(self.top_dir, "ipa")
-os.mkdir(self.dir, 0o750)
-
+os.mkdir(self.dir)
+os.chmod(self.dir, 0o750)
 os.chown(self.dir, pent.pw_uid, pent.pw_gid)
 
 self.header = os.path.join(self.top_dir, 'header')
@@ -585,7 +585,8 @@ class Backup(admintool.AdminTool):
 backup_dir = os.path.join(paths.IPA_BACKUP_DIR, time.strftime('ipa-full-%Y-%m-%d-%H-%M-%S'))
 filename = os.path.join(backup_dir, "ipa-full.tar")
 
-os.mkdir(backup_dir, 0o700)
+os.mkdir(backup_dir)
+os.chmod(backup_dir, 0o700)
 
 cwd = os.getcwd()
 os.chdir(self.dir)
diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index c1bce693b37d26944339f0797b5c15b3da847215..cef0228ea87b8e0bc2c01cfe4b1589811c631c79 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -361,7 +361,8 @@ class ReplicaPrepare(admintool.AdminTool):
 
 self.top_dir =

Re: [Freeipa-devel] [PATCH 0388] tests: Add hostmask detection for sudo rules validating



On 12/10/2015 09:05 AM, Lukas Slebodnik wrote:
> On (08/12/15 14:47), Tomas Babej wrote:
>>
>>
>> On 12/03/2015 04:33 PM, Tomas Babej wrote:
>>>
>>>
>>> On 12/03/2015 04:26 PM, Aleš Mareček wrote:
 Hello,

 ACK for code
 NACK for the placing "get_client_ip_with_hostmask" function to 
 test_sudo.py (this function should be in some more general file)

>>>
>>> What place would you propose? The task.py is not a good place, as this
>>> is not really a task.
>>>
>>> Nevertheless, I'd rather have it moved when an use case outside
>>> test_sudo.py actually arises. Right now it would lead to unnecessary
>>> cluttering.
>>>
>>> Tomas
>>>
>>
>> I re-discovered ipatests.test_integration.util (two years after I
>> created it :D) - which seemed ideal for this function.
>>
>> Updated patch attached.
>>
>> Tomas
> 
>>From 33552d6078d75ee99f9ec19ae143df5a61ba84a4 Mon Sep 17 00:00:00 2001
>> From: Tomas Babej 
>> Date: Wed, 2 Dec 2015 15:25:49 +0100
>> Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
>> hostmask
>>
>> IPA sudo tests worked under the assumption that the clients
>> that are executing the sudo commands have their IPs assigned
>> within 255.255.255.0 hostmask.
>>
>> Removes this (invalid) assumption and adds a
>> dynamic detection of the hostmask of the IPA client.
>>
>> https://fedorahosted.org/freeipa/ticket/5501
>> ---
>> ipatests/test_integration/test_sudo.py | 33 +++--
>> ipatests/test_integration/util.py  | 16 
>> 2 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/ipatests/test_integration/util.py 
>> b/ipatests/test_integration/util.py
>> index 
>> 1a1bb3fcc923c9f2721f0a4c1cb7a1ba2ccc2dd8..187f39e80e84af0eb4938fb19ac3d3c7c2280ed9
>>  100644
>> --- a/ipatests/test_integration/util.py
>> +++ b/ipatests/test_integration/util.py
>> @@ -58,3 +58,19 @@ def run_repeatedly(host, command, assert_zero_rc=True, 
>> test=None,
>>  .format(cmd=' '.join(command),
>>  times=timeout / time_step,
>>  timeout=timeout))
>> +
>> +
>> +def get_host_ip_with_hostmask(host):
>> +"""
>> +Detects the IP of the host including the hostmask.
>> +
>> +Returns None if the IP could not be detected.
>> +"""
>> +
>> +ip = host.ip
>> +result = host.run_command(['ip', 'addr'])
>> +full_ip_regex = r'(?P%s/\d{1,2}) ' % re.escape(ip)
>> +match = re.search(full_ip_regex, result.stdout_text)
> ./make-lint 
> * Module ipatests.test_integration.util
> ipatests/test_integration/util.py:72: [E0602(undefined-variable), 
> get_host_ip_with_hostmask] Undefined variable 're')
> ipatests/test_integration/util.py:73: [E0602(undefined-variable), 
> get_host_ip_with_hostmask] Undefined variable 're')
> ===
> Errors were found during the static code check.
> If you are certain that any of the reported errors are false positives, please
> mark them in the source code according to the pylint documentation.
> ===
> Makefile:124: recipe for target 'lint' failed
> 
> LS
> 

Nothing can break when moving a function, right? Missing import fixed.

Tomas
From c176ff1ab9ea1c56dc0c5d44bc490d925fad1497 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 2 Dec 2015 15:25:49 +0100
Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
 hostmask

IPA sudo tests worked under the assumption that the clients
that are executing the sudo commands have their IPs assigned
within 255.255.255.0 hostmask.

Removes this (invalid) assumption and adds a
dynamic detection of the hostmask of the IPA client.

https://fedorahosted.org/freeipa/ticket/5501
---
 ipatests/test_integration/test_sudo.py | 32 ++--
 ipatests/test_integration/util.py  | 17 +
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/ipatests/test_integration/test_sudo.py b/ipatests/test_integration/test_sudo.py
index 1dd4c5d73c9fa4288af4fc2708aa3abd51407217..b1f31556a96180c3b30b2fcc03dd35b5cd994ff5 100644
--- a/ipatests/test_integration/test_sudo.py
+++ b/ipatests/test_integration/test_sudo.py
@@ -17,8 +17,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
 
+import pytest
+
 from ipatests.test_integration.base import IntegrationTest
 from ipatests.test_integration.tasks import clear_sssd_cache
+from ipatests.test_integration import util
 
 
 class TestSudo(IntegrationTest):
@@ -269,13 +272,25 @@ class TestSudo(IntegrationTest):
  '--hostgroups', 'testhostgroup'])
 
 def test_sudo_rule_restricted_to_one_hostmask_setup(self):
-# Add the client's /24 hostmask to the

Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin

2015-12-13 Thread Aleš Mareček

Ah, sorry, haven't realized there had been devel list attached.
Ok, there is some problem with \r\n in the patch.
Filip, please take a look at it...
Thanks...
 - alich -

- Original Message -
> From: "Filip Škola" 
> To: "Aleš Mareček" 
> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> Sent: Thursday, December 10, 2015 11:29:52 AM
> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> 
> Hi,
> 
> this if fixed. Also issues with test_stageuser_plugin caused by
> UserTracker changes should be fixed here.
> 
> Filip
> 
> 
> On Mon, 7 Dec 2015 09:29:31 -0500 (EST)
> Aleš Mareček  wrote:
> 
> > NACK.
> > 
> > $ ./make-lint
> > * Module ipatests.test_xmlrpc.test_user_plugin
> > ipatests/test_xmlrpc/test_user_plugin.py:42:
> > [E0611(no-name-in-module), ] No name 'ldaptracker' in module
> > 'ipatests.test_xmlrpc')
> > 
> > $ grep ldaptracker ipatests/test_xmlrpc/test_user_plugin.py
> > from ipatests.test_xmlrpc.ldaptracker import Tracker
> > $ ls ipatests/test_xmlrpc/ldaptracker*
> > ls: cannot access ipatests/test_xmlrpc/ldaptracker*: No such file or
> > directory
> > 
> > 
> > - Original Message -
> > > From: "Filip Škola" 
> > > To: "Milan Kubík" 
> > > Cc: freeipa-devel@redhat.com
> > > Sent: Thursday, December 3, 2015 5:38:43 PM
> > > Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > > 
> > > Hi,
> > > 
> > > sending corrected version.
> > > 
> > > F.
> > > 
> > > On Thu, 12 Nov 2015 14:03:19 +0100
> > > Milan Kubík  wrote:
> > > 
> > > > On 11/10/2015 12:13 PM, Filip Škola wrote:
> > > > > Hi,
> > > > >
> > > > > fixed.
> > > > >
> > > > > F.
> > > > >
> > > > > On Tue, 10 Nov 2015 10:52:45 +0100
> > > > > Milan Kubík  wrote:
> > > > >
> > > > >> On 11/09/2015 04:35 PM, Filip Škola wrote:
> > > > >>> Another patch was applied in the meantime.
> > > > >>>
> > > > >>> Attaching an updated version.
> > > > >>>
> > > > >>> F.
> > > > >>>
> > > > >>> On Mon, 9 Nov 2015 13:35:02 +0100
> > > > >>> Milan Kubík  wrote:
> > > > >>>
> > > >  On 11/06/2015 11:32 AM, Filip Škola wrote:
> > > >  Hi,
> > > >  the patch doesn't apply.
> > > > 
> > > > >> Please fix this.
> > > > >>
> > > > >>   ipatests/test_xmlrpc/test_user_plugin.py:1419:
> > > > >> [E0602(undefined-variable),
> > > > >> TestDeniedBindWithExpiredPrincipal.teardown_class] Undefined
> > > > >> variable 'user1')
> > > > >>
> > > > >> Also, use the version numbers for your changed patches.
> > > > >>
> > > > >
> > > > >
> > > > Thanks for the patch. Several issues:
> > > > 
> > > > 1. Use dict.items instead of dict.iteritems, for python3
> > > > compatibility
> > > > 
> > > > 2. What is the purpose of TestPrepare class? The 'purge' methods
> > > > do not call any ipa commands.
> > > > Tracker.make_fixture should be used to make the Tracked resources
> > > > clean themselves up when they're out of scope.
> > > > 
> > > > 3. Why reference the resources by hardcoded name if they have a
> > > > fixture representation?
> > > > 
> > > > 4. Rewrite {create,delete}_test_group to a fixture. You may want
> > > > to use different scope (or not).
> > > > 
> > > > 5. In `def atest_rename_to_invalid_login(self, user):` - use
> > > > pytest.skipif decorator and provide a reason if you must,
> > > > do not obfuscate method name in order not to run it.
> > > > 
> > > > 
> > > 
> > > 
> > > --
> > > 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
> 
> 

-- 
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 0116] CI tests: remove '-p' option from ipa-dns-install calls

2015-12-13 Thread Martin Babinsky


See commit message.

--
Martin^3 Babinsky
From 108ce5787620227ef657c8e655dd3427655ebd06 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 10 Dec 2015 16:32:35 +0100
Subject: [PATCH] CI tests: remove '-p' option from ipa-dns-install calls

fix for https://fedorahosted.org/freeipa/ticket/4933 made ipa-dns-install to
use LDAPI and deprecated -p option for directory manager password. This patche
remove the option from calls to ipa-dns-install in CI tests so that
deprecation warning does not clutter the logs.
---
 ipatests/test_integration/tasks.py   | 3 +--
 ipatests/test_integration/test_backup_and_restore.py | 1 -
 ipatests/test_integration/test_dnssec.py | 5 -
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index c3681fca952807ac6ebcca56ce961df2d3f33f0c..eb1378ef4ba8b5ec27034c8a0c429fd6114cd0a0 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -935,7 +935,7 @@ def resolve_record(nameserver, query, rtype="SOA", retry=True, timeout=100):
 def install_kra(host, domain_level=None, first_instance=False, raiseonerr=True):
 if not domain_level:
domain_level = domainlevel(host)
-command = ["ipa-kra-install", "-U", "-p", host.config.dirman_password]
+command = ["ipa-kra-install", "-U"]
 if domain_level == DOMAIN_LEVEL_0 and not first_instance:
 replica_file = get_replica_filename(host)
 command.append(replica_file)
@@ -957,7 +957,6 @@ def install_dns(host, raiseonerr=True):
 args = [
 "ipa-dns-install",
 "--forwarder", host.config.dns_forwarder,
-"-p", host.config.dirman_password,
 "-U",
 ]
 return host.run_command(args, raiseonerr=raiseonerr)
diff --git a/ipatests/test_integration/test_backup_and_restore.py b/ipatests/test_integration/test_backup_and_restore.py
index 8b9cd2dc4af146b2c00e6d2224625c4288854be8..b8abb343b027a9b61c6c2d8660ac2e926c5e70bf 100644
--- a/ipatests/test_integration/test_backup_and_restore.py
+++ b/ipatests/test_integration/test_backup_and_restore.py
@@ -285,7 +285,6 @@ class BaseBackupAndRestoreWithDNSSEC(IntegrationTest):
 "ipa-dns-install",
 "--dnssec-master",
 "--forwarder", cls.master.config.dns_forwarder,
-"-p", cls.master.config.dirman_password,
 "-U",
 ]
 cls.master.run_command(args)
diff --git a/ipatests/test_integration/test_dnssec.py b/ipatests/test_integration/test_dnssec.py
index 5d6acb7cc0d843929fd23e5a07f37803cbfdb792..a4387d353d0a72c107978bb292561dde312a9721 100644
--- a/ipatests/test_integration/test_dnssec.py
+++ b/ipatests/test_integration/test_dnssec.py
@@ -91,7 +91,6 @@ class TestInstallDNSSECLast(IntegrationTest):
 "ipa-dns-install",
 "--dnssec-master",
 "--forwarder", self.master.config.dns_forwarder,
-"-p", self.master.config.dirman_password,
 "-U",
 ]
 self.master.run_command(args)
@@ -249,7 +248,6 @@ class TestInstallDNSSECFirst(IntegrationTest):
 "ipa-dns-install",
 "--dnssec-master",
 "--forwarder", cls.master.config.dns_forwarder,
-"-p", cls.master.config.dirman_password,
 "-U",
 ]
 cls.master.run_command(args)
@@ -423,7 +421,6 @@ class TestMigrateDNSSECMaster(IntegrationTest):
 "ipa-dns-install",
 "--dnssec-master",
 "--forwarder", cls.master.config.dns_forwarder,
-"-p", cls.master.config.dirman_password,
 "-U",
 ]
 cls.master.run_command(args)
@@ -458,7 +455,6 @@ class TestMigrateDNSSECMaster(IntegrationTest):
 "ipa-dns-install",
 "--disable-dnssec-master",
 "--forwarder", self.master.config.dns_forwarder,
-"-p", self.master.config.dirman_password,
 "--force",
 "-U",
 ]
@@ -474,7 +470,6 @@ class TestMigrateDNSSECMaster(IntegrationTest):
 "--dnssec-master",
 "--kasp-db", replica_backup_filename,
 "--forwarder", self.master.config.dns_forwarder,
-"-p", self.master.config.dirman_password,
 "-U",
 ]
 self.replicas[0].run_command(args)
-- 
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] 0001 Refactor test_user_plugin

2015-12-13 Thread Filip Skola

Hi,

I don't know what is causing the \r\n issue. I use vim and than send each email 
with claws-mail. Didn't spot this issue when trying emailing the patch to my 
other address. I'm trying to send it from zimbra now, let me know if that 
helped pls.

Fix for the stageuser plugin issues caused by this patch should have been 
included in the last update; I think the remaining issue is not caused by 
UserTracker changes. Please correct me, if I'm wrong.

> There is some issue with "test_rename_to_too_long_login" test. It fails but
> actually this is false positive because it is possible to create login upto
> 255 characters. I don't know why test mentions 32 characters without any
> other modified setup.
> NACK for now.
>  - alich -

This has been changed. This test still fails, though.

Filip

> 
> 
> - Original Message -
> > From: "Aleš Mareček" 
> > To: "Filip Škola" 
> > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > Sent: Thursday, December 10, 2015 4:11:47 PM
> > Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > 
> > Ah, sorry, haven't realized there had been devel list attached.
> > Ok, there is some problem with \r\n in the patch.
> > Filip, please take a look at it...
> > Thanks...
> >  - alich -
> > 
> > - Original Message -
> > > From: "Filip Škola" 
> > > To: "Aleš Mareček" 
> > > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > > Sent: Thursday, December 10, 2015 11:29:52 AM
> > > Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > > 
> > > Hi,
> > > 
> > > this if fixed. Also issues with test_stageuser_plugin caused by
> > > UserTracker changes should be fixed here.
> > > 
> > > Filip
> > > 
> > > 
> > > On Mon, 7 Dec 2015 09:29:31 -0500 (EST)
> > > Aleš Mareček  wrote:
> > > 
> > > > NACK.
> > > > 
> > > > $ ./make-lint
> > > > * Module ipatests.test_xmlrpc.test_user_plugin
> > > > ipatests/test_xmlrpc/test_user_plugin.py:42:
> > > > [E0611(no-name-in-module), ] No name 'ldaptracker' in module
> > > > 'ipatests.test_xmlrpc')
> > > > 
> > > > $ grep ldaptracker ipatests/test_xmlrpc/test_user_plugin.py
> > > > from ipatests.test_xmlrpc.ldaptracker import Tracker
> > > > $ ls ipatests/test_xmlrpc/ldaptracker*
> > > > ls: cannot access ipatests/test_xmlrpc/ldaptracker*: No such file or
> > > > directory
> > > > 
> > > > 
> > > > - Original Message -
> > > > > From: "Filip Škola" 
> > > > > To: "Milan Kubík" 
> > > > > Cc: freeipa-devel@redhat.com
> > > > > Sent: Thursday, December 3, 2015 5:38:43 PM
> > > > > Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > sending corrected version.
> > > > > 
> > > > > F.
> > > > > 
> > > > > On Thu, 12 Nov 2015 14:03:19 +0100
> > > > > Milan Kubík  wrote:
> > > > > 
> > > > > > On 11/10/2015 12:13 PM, Filip Škola wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > fixed.
> > > > > > >
> > > > > > > F.
> > > > > > >
> > > > > > > On Tue, 10 Nov 2015 10:52:45 +0100
> > > > > > > Milan Kubík  wrote:
> > > > > > >
> > > > > > >> On 11/09/2015 04:35 PM, Filip Škola wrote:
> > > > > > >>> Another patch was applied in the meantime.
> > > > > > >>>
> > > > > > >>> Attaching an updated version.
> > > > > > >>>
> > > > > > >>> F.
> > > > > > >>>
> > > > > > >>> On Mon, 9 Nov 2015 13:35:02 +0100
> > > > > > >>> Milan Kubík  wrote:
> > > > > > >>>
> > > > > >  On 11/06/2015 11:32 AM, Filip Škola wrote:
> > > > > >  Hi,
> > > > > >  the patch doesn't apply.
> > > > > > 
> > > > > > >> Please fix this.
> > > > > > >>
> > > > > > >>   ipatests/test_xmlrpc/test_user_plugin.py:1419:
> > > > > > >> [E0602(undefined-variable),
> > > > > > >> TestDeniedBindWithExpiredPrincipal.teardown_class] Undefined
> > > > > > >> variable 'user1')
> > > > > > >>
> > > > > > >> Also, use the version numbers for your changed patches.
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > > Thanks for the patch. Several issues:
> > > > > > 
> > > > > > 1. Use dict.items instead of dict.iteritems, for python3
> > > > > > compatibility
> > > > > > 
> > > > > > 2. What is the purpose of TestPrepare class? The 'purge' methods
> > > > > > do not call any ipa commands.
> > > > > > Tracker.make_fixture should be used to make the Tracked resources
> > > > > > clean themselves up when they're out of scope.
> > > > > > 
> > > > > > 3. Why reference the resources by hardcoded name if they have a
> > > > > > fixture representation?
> > > > > > 
> > > > > > 4. Rewrite {create,delete}_test_group to a fixture. You may want
> > > > > > to use different scope (or not).
> > > > > > 
> > > > > > 5. In `def atest_rename_to_invalid_login(self, user):` - use
> > > > > > pytest.skipif decorator and provide a reason if you must,

Re: [Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

2015-12-13 Thread Filip Škola

On Mon, 7 Dec 2015 17:49:18 +0100
Milan Kubík  wrote:

> On 12/03/2015 08:15 PM, Filip Škola wrote:
> > On Mon, 30 Nov 2015 17:18:30 +0100
> > Milan Kubík  wrote:
> >
> >> On 11/23/2015 04:42 PM, Filip Škola wrote:
> >>> Sending updated patch.
> >>>
> >>> F.
> >>>
> >>> On Mon, 23 Nov 2015 14:59:34 +0100
> >>> Filip Škola  wrote:
> >>>
>  Found couple of issues (broke some dependencies).
> 
>  NACK
> 
>  F.
> 
>  On Fri, 20 Nov 2015 13:56:36 +0100
>  Filip Škola  wrote:
> 
> > Another one.
> >
> > F.
> >>>
> >> Hi, the tests look good. Few remarks, though.
> >>
> >> 1. Please, use the shortes copyright notice in new modules.
> >>
> >>   #
> >>   # Copyright (C) 2015  FreeIPA Contributors see COPYING for
> >> license #
> >>
> >> 2. The tests `test_group_remove_group_from_protected_group` and
> >> `test_group_full_set_of_objectclass_not_available_post_detach`
> >> were not ported. Please, include them in the patch.
> >>
> >> Also, for less hassle, please rebase your patches on top of
> >> freeipa-mkubik-0025-3-Separated-Tracker-implementations-into-standalone-pa.patch
> >> Which changes the location of tracker implementations and prevents
> >> circular imports.
> >>
> >> Thanks.
> >>
> >
> >
> > Hi,
> >
> > these cases are there, in corresponding classes. They are marked
> > with the original comments. (However I can move them to separate
> > class if desirable.)
> >
> > The copyright notice is changed. Also included a few changes in the
> > test with user without private group.
> >
> > Filip
> NACK
> 
> linter:
> * Module tracker.group_plugin
> ipatests/test_xmlrpc/tracker/group_plugin.py:257: 
> [E0102(function-redefined), GroupTracker.check_remove_member] method 
> already defined line 253)
> 
> Probably a leftover after the rebase made on top of my patch. Please
> fix it. You can check youch changes by make-lint script before
> sending them.
> 
> Thanks
> 


Hi,

I learned to use make-lint!

Thanks,
F.
>From 2e231e285215818bbe1e06aeba573d43c86fab8b Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Mon, 9 Nov 2015 16:48:55 +0100
Subject: [PATCH] Refactor test_group_plugin, use GroupTracker for tests

---
 ipatests/test_xmlrpc/test_group_plugin.py | 1728 +
 ipatests/test_xmlrpc/test_stageuser_plugin.py |4 +-
 ipatests/test_xmlrpc/tracker/group_plugin.py  |  149 ++-
 3 files changed, 736 insertions(+), 1145 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py
index f2bd0f4b9c8d517500b63cf49a8a7bc7c29aab6e..1ac278c7e224b8dd8793dc56c299dcae88aa78a3 100644
--- a/ipatests/test_xmlrpc/test_group_plugin.py
+++ b/ipatests/test_xmlrpc/test_group_plugin.py
@@ -1,6 +1,7 @@
 # Authors:
 #   Rob Crittenden 
 #   Pavel Zuna 
+#   Filip Skola 
 #
 # Copyright (C) 2008  Red Hat
 # see file 'COPYING' for use and warranty information
@@ -26,1137 +27,648 @@ import pytest
 
 from ipalib import api, errors
 from ipatests.test_xmlrpc import objectclasses
-from ipatests.test_xmlrpc.xmlrpc_test import (Declarative, fuzzy_digits, fuzzy_uuid, fuzzy_set_ci,
- add_sid, add_oc, XMLRPC_test, raises_exact)
+from ipatests.test_xmlrpc.xmlrpc_test import (
+fuzzy_digits, fuzzy_uuid, fuzzy_set_ci, add_sid, add_oc,
+XMLRPC_test, raises_exact
+)
 from ipapython.dn import DN
-from ipatests.test_xmlrpc.test_user_plugin import get_user_result
 
+from ipatests.test_xmlrpc.tracker.group_plugin import GroupTracker
 from ipatests.test_xmlrpc.tracker.user_plugin import UserTracker
-from ipatests.util import assert_deepequal
+from ipatests.test_xmlrpc.test_user_plugin import user, user_npg2
+from ipatests.util import assert_deepequal, get_group_dn
 
 
-group1 = u'testgroup1'
-group2 = u'testgroup2'
-group3 = u'testgroup3'
-renamedgroup1 = u'testgroup'
-user1 = u'tuser1'
+notagroup = u'notagroup'
+renamedgroup1 = u'renamedgroup'
+invalidgroup1 = u'+tgroup1'
+external_sid1 = u'S-1-1-123456-789-1'
 
-invalidgroup1=u'+tgroup1'
 
-# When adding external SID member to a group we can't test
-# it fully due to possibly missing Samba 4 python bindings
-# and/or not configured AD trusts. Thus, we'll use incorrect
-# SID value to merely test that proper exceptions are raised
-external_sid1=u'S-1-1-123456-789-1'
+@pytest.fixture(scope='class')
+def group(request):
+tracker = GroupTracker(name=u'testgroup1', description=u'Test desc1')
+return tracker.make_fixture(request)
 
-def get_group_dn(cn):
-return DN(('cn', cn), api.env.container_group, api.env.basedn)
+
+@pytest.fixture(scope='class')
+def group2(request):
+tracker = GroupTracker(name=u'testgroup2', description=u'Test desc2')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def managed_group(request, user):
+

Re: [Freeipa-devel] [PATCH 0058, 0064] dns: do not add (forward)zone if it is already resolvable.

2015-12-13 Thread Petr Spacek

On 9.12.2015 13:37, David Kupka wrote:
> On 08/12/15 15:24, Petr Spacek wrote:
>> On 8.12.2015 12:19, David Kupka wrote:
>>> On 08/12/15 08:56, Petr Spacek wrote:
 On 7.12.2015 14:41, David Kupka wrote:
> +def is_host_resolvable(fqdn):
> +if not isinstance(fqdn, DNSName):
> +fqdn = DNSName(fqdn)
> +for rdtype in (rdatatype.A, rdatatype.):
> +try:
> +resolver.query(fqdn.make_absolute(), rdtype)
> +except DNSException:
> +continue
> +else:
> +return True
> +
> +return False
>

 NACK, you are re-introducing duplicate function which was removed in
 498471e4aed1367b72cd74d15811d0584a6ee268.

 Please amend the patch ASAP to use new verify_host_resolvable() function 
 so I
 can test it and get it into 4.3.

>>> Updated patches attached.
>>
>> Hmm, my review script screams:
>>
>> Detected base branch:   origin/master
>> Checks will be made against following base commit: 848912a add missing
>> /ipaplatform/constants.py to .gitignore
>> Writing API to API.txt
>> ./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual
>> indentation
>> ./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for
>> visual indent
>> ./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual
>> indentation
>> ./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for
>> visual indent
>> ./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
>> visual indent
>> ./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
>> ./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
>> ./ipaserver/install/bindinstance.py:49:9: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
>> characters)
>> ./ipaserver/install/bindinstance.py:438:19: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/server/common.py:271:63: E261 at least two spaces before
>> inline comment
>> ./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
>> characters)
>> ERROR: PEP8 --diff failed, continuing ...
>> ERROR: API.txt was changed without a change in VERSION, continuing ...
>> Summary of detected errors:
>>   ERROR: PEP8 --diff failed
>>   ERROR: API.txt was changed without a change in VERSION
>>
>> Please fix it :-)
>>
>> Make make this more pleasant for you, you can use (and review) my attached
>> patch. It adds 'review' target to Makefile, it will do the same checks as I 
>> do.
>>
> 
> Unfortunately your tool does not work for me (output w/ -o xtrace attached).
> Anyway I have incremented API version and fixed most of the pep8 errors except
> for E124 twice in ipalib/plugins/dns.py as it seems to be convention in the
> file and E501 twice in ipapython/ipautil.py because IMO breaking string
> constant into multiple lines does not help readability.
> 
> Updated patches also attached.

We are almost there, but NACK for now.

1) Following attempt to install IPA explodes. Please note that
dom-245.idm.lab.eng.brq.redhat.com DNS domain is delegated to the IPA server
before installation is started, so it gives 'timeout' or possibly SERVFAIL.

# ipa-server-install --ds-password=root4lab --admin-password=root4lab
--setup-dns --forwarder=10.38.5.26 --no-reverse --allow-zone-overlap
--domain=dom-245.idm.lab.eng.brq.redhat.com
--realm=DOM-245.IDM.LAB.ENG.BRQ.REDHAT.COM
[...]

Configuring DNS (named)
  [1/11]: generating rndc key file
  [2/11]: adding DNS container
  [3/11]: setting up our zone
  [error] InvocationError: DNS check for domain
dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
30.000453949 seconds. Please make sure that the domain is properly delegated
to this IPA server.
ipa.ipapython.install.cli.install_tool(Server): ERRORDNS check for domain
dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
30.000453949 seconds. Please make sure that the domain is properly delegated
to this IPA server.
ipa.ipapython.install.cli.install_tool(Server): ERRORThe
ipa-server-install command failed. See /var/log/ipaserver-install.log for more
information

2015-12-09T17:15:37Z DEBUG   File
"/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 171, in execute
return_value = self.run()
  File "/usr/lib/python2.7/site-packages/ipapython/install/cli.py", line 318,
in run
cfgr.run()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 310,
in run
self.execute()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 332,
in execute
for nothing in self._executor():
  File

Re: [Freeipa-devel] [PATCHES 516-517] spec file: put Python modules into standalone packages

2015-12-13 Thread Petr Vobornik


On 12/10/2015 01:51 PM, Jan Cholasta wrote:

On 10.12.2015 11:32, Jan Cholasta wrote:

On 9.12.2015 20:51, Petr Vobornik wrote:

On 12/07/2015 04:21 PM, Jan Cholasta wrote:

Hi,

the attached patches partially fix
. This is done to allow
the addition of Python 3 packages, see
.



See commit messages for more information.

In order to test:
1. make rpms
2.





3. Test with both dnf and yum-deprecated.

Beware that when you run "yum-deprecated clean all", it does not remove
cache for the on-disk repository created in step 2, you have to remove
the /var/cache/yum/$basearch/$releasever/$reponame directory manually.

Honza



Shouldn't freeipa-server-dns and freeipa-server-trust-add depend on
freeipa-server? They do not in this patch. IMO they should.


Fixed.



following updates work (all on f23, update from 4.2.3):
   dnf update
   dnf update freeipa-*
   yum-depracated update freeipa-*

for both client or server with all packages.

but when I tried to install only client "dnf install freeipa-client" and
then following failed:
   dnf update freeipa-client

The difference was:
 Installing:
  freeipa-client-common  noarch
  freeipa-common noarch
  python2-ipaclient  noarch
  python2-ipalib x86_64
 Upgrading:
  freeipa-client

Works:
  Installing:
  freeipa-client-common  noarch
  freeipa-common noarch
  freeipa-python-compat  noarch
  replacing  freeipa-python.x86_64 4.2.3-1.1.fc23
  python2-ipaclient  noarch
  python2-ipalib x86_64
 Upgrading:
  freeipa-client


not sure if it is a problem, otherwise the patch looks OK.


Hmm, Fedora packaging guidelines are silent about this.

When you run "dnf update freeipa-client freeipa-python" to force
freeipa-python update it works fine.

I removed Provides added Conflicts on freeipa-python which seem to have
fixed it.


Actually I think it would be better to keep the Provides and do
Conflicts < 4.2.91 - fixed.

Also updated %alt_name dependencies to be in sync with %name
dependencies and added arch-specific python-ipalib Provides to
python2-ipalib.

Updated patches attached.



ACK from me.

What surprised me a bit is that I don't see any other responses here.

If somebody didn't ready the mail, here is the new package list:

freeipa-admintools
freeipa-client
freeipa-client-common
freeipa-common
freeipa-debuginfo
freeipa-python-compat
freeipa-server
freeipa-server-common
freeipa-server-dns
freeipa-server-trust-ad
python2-ipaclient
python2-ipalib
python2-ipaserver
python2-ipatests
--
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] [TESTS][PATCH 0006] Add comments to stageuser plugin tests

On 09.12.2015 12:40, Martin Babinsky wrote:

On 12/09/2015 11:29 AM, Lenka Doudova wrote:

On 12/09/2015 10:13 AM, Martin Basti wrote:

On 09.12.2015 09:41, Lenka Doudova wrote:

Hi,

attaching fixed patches for master and ipa-4-2 branch.
Also fixes test accordingly to
https://fedorahosted.org/freeipa/ticket/5387.

Lenka

On 11/20/2015 12:13 PM, Martin Babinsky wrote:

On 11/19/2015 10:34 AM, Petr Viktorin wrote:

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

This looks like a better approach IMHO, you can then see which
attribute/value was being checked.

I would very much favor more descriptive test/fixture names in the
beginning.

Hello,

we use usually bottom posting on freeipa-devel please try to keep
reply in this way.

Patch:

I do not like the idea of separated lists, IMO it is hard to manage
and is easy to create mistakes.

How about this (untested, just from top of my head):
http://fpaste.org/298994/65184014/

Martin

Great idea, thanks. Fixed patches attached.

Lenka

Tests pass and code looks good, ACK.

Pushed to master: a66a2c5160dbc23cdeec55d17422812028939e16
Pushed to ipa-4-2: 75675fc71a148dcc17b025a80123aa01644f5297

--
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 0377] CI: fix test_vault installation on domain level 1


Patch attached.
From 028049d455a12b4a5e97845e967aaf0b6103c0fc Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 9 Dec 2015 09:35:59 +0100
Subject: [PATCH] CI: fix ipa-kra-install on domain level 1

---
 ipatests/test_integration/test_vault.py | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/ipatests/test_integration/test_vault.py b/ipatests/test_integration/test_vault.py
index ba472af7152b508fb8a6fd92ebf18879518d2246..74b554eb283278940e6ed0a93596ed194eadadcb 100644
--- a/ipatests/test_integration/test_vault.py
+++ b/ipatests/test_integration/test_vault.py
@@ -92,12 +92,7 @@ class TestInstallKRA(IntegrationTest):
 def test_create_and_retrieve_vault_replica_with_kra(self):
 
 # install KRA on replica
-self.replicas[0].run_command([
-"ipa-kra-install",
-tasks.get_replica_filename(self.replicas[0]),
-"-p", self.replicas[0].config.dirman_password,
-"-U",
-])
+tasks.install_kra(self.replicas[0], first_instance=False)
 
 # create vault
 self.replicas[0].run_command([
-- 
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] [PATCHES 529-530] ca install: use host credentials in domain level 1


On 10.12.2015 09:51, Jan Cholasta wrote:

Hi,

the attached patches fix .

My patches 523-525 are required for this:
.


Honza


Rebased patches attached.

--
Jan Cholasta
From a60dfc311007ead160a98f23f87618e7c5ce0fb1 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 9 Dec 2015 10:31:18 +0100
Subject: [PATCH 1/2] aci: merge domain and CA suffix replication agreement
 ACIs

https://fedorahosted.org/freeipa/ticket/5399
---
 install/share/ca-topology.uldif |  6 --
 install/share/replica-acis.ldif |  6 +++---
 install/updates/20-aci.update   | 10 ++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/install/share/ca-topology.uldif b/install/share/ca-topology.uldif
index 7ce3cb1..fea591b 100644
--- a/install/share/ca-topology.uldif
+++ b/install/share/ca-topology.uldif
@@ -10,11 +10,5 @@ default: objectclass: iparepltopoconf
 default: ipaReplTopoConfRoot: o=ipaca
 default: cn: ca
 
-# Update CA replication settings
-dn: cn=o\3Dipaca,cn=mapping tree,cn=config
-add: aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-add: aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-add: aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "ldap:///cn=Remove Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-
 dn: cn=replica,cn=o\3Dipaca,cn=mapping tree,cn=config
 onlyifexist: nsds5replicabinddngroup: cn=replication managers,cn=sysaccounts,cn=etc,$SUFFIX
diff --git a/install/share/replica-acis.ldif b/install/share/replica-acis.ldif
index 8c0bc8e..6735130 100644
--- a/install/share/replica-acis.ldif
+++ b/install/share/replica-acis.ldif
@@ -1,16 +1,16 @@
 # Replica administration
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "ldap:///cn=Remove Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
diff --git a/install/updates/20-aci.update b/install/updates/20-aci.update
index ca4c0df..b06f569 100644
--- a/install/updates/20-aci.update
+++ b/install/updates/20-aci.update
@@ -66,6 +66,16 @@ add:aci:(targetattr="*")(version 3.0; acl "Admin can read all tasks"; allow (rea
 dn: cn=mapping tree,cn=config
 add:aci: (target = "ldap:///cn=meTo($$dn),cn=*,cn=mapping tree,cn=config")(targetattr = "objectclass || cn")(version 3.0; acl "Allow hosts to read their replication agreements"; allow(read, search, compare) userdn = "ldap:///fqdn=($$dn),cn=computers,cn=accounts,$SUFFIX";)
 
+dn: cn="$SUFFIX",cn=mapping tree,cn=config
+remove:aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
+remove:aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
+remove:aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "
+
+dn: cn=o\3Dipaca,cn=mapping tree,cn=config
+remove:aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow

Re: [Freeipa-devel] [PATCH 0058, 0064] dns: do not add (forward)zone if it is already resolvable.

2015-12-13 Thread David Kupka


On 08/12/15 15:24, Petr Spacek wrote:

On 8.12.2015 12:19, David Kupka wrote:

On 08/12/15 08:56, Petr Spacek wrote:

On 7.12.2015 14:41, David Kupka wrote:

+def is_host_resolvable(fqdn):
+if not isinstance(fqdn, DNSName):
+fqdn = DNSName(fqdn)
+for rdtype in (rdatatype.A, rdatatype.):
+try:
+resolver.query(fqdn.make_absolute(), rdtype)
+except DNSException:
+continue
+else:
+return True
+
+return False



NACK, you are re-introducing duplicate function which was removed in
498471e4aed1367b72cd74d15811d0584a6ee268.

Please amend the patch ASAP to use new verify_host_resolvable() function so I
can test it and get it into 4.3.


Updated patches attached.


Hmm, my review script screams:

Detected base branch:   origin/master
Checks will be made against following base commit: 848912a add missing
/ipaplatform/constants.py to .gitignore
Writing API to API.txt
./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual
indentation
./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for
visual indent
./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual
indentation
./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
./ipaserver/install/bindinstance.py:49:9: E128 continuation line
under-indented for visual indent
./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
characters)
./ipaserver/install/bindinstance.py:438:19: E128 continuation line
under-indented for visual indent
./ipaserver/install/server/common.py:271:63: E261 at least two spaces before
inline comment
./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
characters)
ERROR: PEP8 --diff failed, continuing ...
ERROR: API.txt was changed without a change in VERSION, continuing ...
Summary of detected errors:
  ERROR: PEP8 --diff failed
  ERROR: API.txt was changed without a change in VERSION

Please fix it :-)

Make make this more pleasant for you, you can use (and review) my attached
patch. It adds 'review' target to Makefile, it will do the same checks as I do.



Unfortunately your tool does not work for me (output w/ -o xtrace attached).
Anyway I have incremented API version and fixed most of the pep8 errors 
except for E124 twice in ipalib/plugins/dns.py as it seems to be 
convention in the file and E501 twice in ipapython/ipautil.py because 
IMO breaking string constant into multiple lines does not help readability.


Updated patches also attached.
--
David Kupka
From 4fb72e2b197d365b99bb426fa4b4919c60efe44b Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Wed, 2 Dec 2015 13:17:13 +
Subject: [PATCH] dns: do not add (forward)zone if it is already resolvable.

Check if the zone user wants to add is already resolvable and refuse to
create it if yes. --skip-overlap-check and --force options suppress this check.

https://fedorahosted.org/freeipa/ticket/5087
---
 API.txt   |  7 ++--
 VERSION   |  2 +-
 ipalib/plugins/dns.py | 32 +++---
 ipapython/ipautil.py  | 89 +--
 4 files changed, 120 insertions(+), 10 deletions(-)

diff --git a/API.txt b/API.txt
index 60c98c31aa85d6c8879cd145f3d84188d4fea5b7..3a9fb65a386a2a6529b8cd241642446c135471f2 100644
--- a/API.txt
+++ b/API.txt
@@ -959,7 +959,7 @@ output: Entry('result', , Gettext('A dictionary representing an LDA
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
 command: dnsforwardzone_add
-args: 1,8,3
+args: 1,9,3
 arg: DNSNameParam('idnsname', attribute=True, cli_name='name', multivalue=False, only_absolute=True, primary_key=True, required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
@@ -968,6 +968,7 @@ option: StrEnum('idnsforwardpolicy', attribute=True, cli_name='forward_policy',
 option: Str('name_from_ip', attribute=False, cli_name='name_from_ip', multivalue=False, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
 option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Flag('skip_overlap_check', autofill=True, default=False)
 option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
@@ -1366,7 +1367,7 @@ output: Entry('result', , Gettext('A dictionary representing an LDA
 output: Output('summary', (,

Re: [Freeipa-devel] [PATCH 0115] fix error message assertion in negative forced client reenrollment tests




On 08.12.2015 18:02, Martin Babinsky wrote:
This patch fixes the assertions in negative test cases of 
'test_forced_client_reenrollment' CI test suite.


On ipa-4-2 it fixes https://fedorahosted.org/freeipa/ticket/5511 and 
makes all 8 tests in this suite green, shiny and happy again.


It also fixes negative test cases on master branch, but the positive 
cases are broken there due to 
https://fedorahosted.org/freeipa/ticket/5528




I haven't received ACK email, but according this 
https://www.redhat.com/archives/freeipa-devel/2015-December/msg00367.html, 
ACK is already there


Pushed to:
master: 7c4ce9a09863d5364b4634fac03e83a4e9caae42
ipa-4-2: 742ffcde9920379da28c8b51060064c1bff03dc1

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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


On 10.12.2015 15:01, Martin Babinsky wrote:

On 12/10/2015 07:57 AM, Jan Cholasta wrote:

On 9.12.2015 16:39, Jan Cholasta wrote:

On 7.12.2015 08:14, Jan Cholasta wrote:

On 6.12.2015 21:32, Martin Basti wrote:



On 04.12.2015 16:58, Simo Sorce wrote:

On Fri, 2015-12-04 at 15:39 +0100, Jan Cholasta wrote:

On 4.12.2015 15:16, Jan Cholasta wrote:

On 4.12.2015 15:12, Jan Cholasta wrote:

On 4.12.2015 11:15, Petr Vobornik wrote:

On 12/03/2015 03:11 PM, Martin Basti wrote:


On 01.12.2015 12:19, Jan Cholasta wrote:

On 23.11.2015 15:47, Simo Sorce wrote:

On Mon, 2015-11-23 at 15:37 +0100, Jan Cholasta wrote:

Ad alternative is to add the host to ipaservers before the
checks
are
done and remove it again if any of them fail.

Too error prone, I am ok with the current way in your patches
until/unless I can think of a fail safe way. :-)

Updated patches attached. Note that 520 should be applied
between 509
and 510.




Functional ACK


Simo, do you want to review the ACIs or other things it the
patches? Or
can the patches be pushed?

There were no changes in the ACIs since last time.

Actually, memberPrincipal was removed from the "IPA server hosts
can
manage own Custodia secrets" ACI, as per Simo's request.


Rebased patches attached.

Note that 520 should still be applied between 509 and 510.


LGTM


ACK


Thanks.

Pushed to master: 01ddf51df76f3298499973355c5461727e46ab5b


Martin Babinsky found out that ipaservers is not created early enough
when installing a replica of a 4.2 or older server which causes a crash.

The attached patch fixes that.


Actually I don't like how I fixed that, here's an updated patch.

Also, I noticed that replica promotion fails too late in domain level 0.
Fixed as well.





ACK to both patches.


Thanks.

Pushed to master: b4a78db4e7d06237a715f088d1b914b47eccf32f

--
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 0069] Add 'review' target for make

2015-12-13 Thread Lukas Slebodnik

On (10/12/15 18:04), Petr Spacek wrote:
>On 9.12.2015 15:30, Petr Spacek wrote:
>> Hello,
>> 
>> this patch automates some of sanity checks proposed by Petr Vobornik.
>> 
>> 'make review' should be used in root of clean Git tree which has patches 
>> under
>> review applied.
>>
+1 for automatisation
-1 for name.

Your script does not review[1] anything. Code review(peer review) is a job
for humans. What do you think about alternative
names: "review-helper", "sanity-check" ...

>> Magic in review.sh attempts to detect nearest remote branch which can be used
>> as diff base for review. Please see review.sh for further details.
>
>And here is the patch! :-)
>
>-- 
>Petr^2 Spacek

[1] https://en.wikipedia.org/wiki/Code_review

>From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
>From: Petr Spacek 
>Date: Tue, 8 Dec 2015 12:06:33 +0100
>Subject: [PATCH] Add 'review' target for make. It automates following tasks:
>
>- check if ACI.txt and API.txt are up-to-date
>- check if VERSION was changed if API was changed
>- pep8 --diff does not produce new errors when ran on diff from origin/branch
>- make lint does not produce errors
>---
> Makefile  |  4 
> review.sh | 64 +++
> 2 files changed, 68 insertions(+)
> create mode 100755 review.sh
>
>diff --git a/Makefile b/Makefile
>index 
>d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..b85b3a5d6362150dcebf16745414c3d416c9547f
> 100644
>--- a/Makefile
>+++ b/Makefile
>@@ -79,6 +79,10 @@ check: bootstrap-autogen server tests
>   (cd $$subdir && $(MAKE) check) || exit 1; \
>   done
> 
>+# works only in Git tree
>+review: version-update
>+  ./review.sh
>+
> bootstrap-autogen: version-update client-autogen
>   @echo "Building IPA $(IPA_VERSION)"
>   cd asn1; if [ ! -e Makefile ]; then ../autogen.sh --prefix=/usr 
> --sysconfdir=/etc --localstatedir=/var --libdir=$(LIBDIR); fi
>diff --git a/review.sh b/review.sh
>new file mode 100755
>index 
>..3a5114fbe88d3aba8e926e049500f5ac5f41a83e
>--- /dev/null
>+++ b/review.sh
>@@ -0,0 +1,64 @@
>+#!/bin/bash
>+is_tree_clean() {
>+  test "$(git status --porcelain "$@")" == ""
>+  return $?
>+}
>+
>+log_error() {
>+  echo "ERROR: ${1}, continuing ..."
>+  errs=(${errs[@]} "ERROR: ${1}\n")
>+}
>+
>+set -o errexit -o nounset #-o xtrace
>+
>+LOGFILE="$(mktemp --suff=.log)"
>+exec > >(tee -i ${LOGFILE})
>+exec 2>&1
>+
>+echo -n "make lint is running ... "
>+make --silent lint || log_error "make lint failed"
>+
>+# Go backwards in history until you find a remote branch from which current
>+# branch was created. This will be used as base for git diff.
>+PATCHCNT=0
>+BASEBRANCH=""
If base branch means the remote branch which was used for cloning the cerrent
git HEAD than you can simplify it a littl bit.
git rev-parse --symbolic-full-name @{upstream}

>+CURRBRANCH="$(git branch --remote --contains)"
>+while [ "${BASEBRANCH}" == "" ]
>+do
>+  BASEBRANCH="$(git branch --remote --contains "HEAD~${PATCHCNT}" | grep 
>-v "^. ${CURRBRANCH}$" || :)"
>+  PATCHCNT="$(expr "${PATCHCNT}" + 1)"
Then for patch count you can use
PATCHCNT=$(git log --oneline $BASEBRANCH..HEAD | wc -l)

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

[Freeipa-devel] [PATCH 0070] Makefile: disable parallel build

2015-12-13 Thread Petr Spacek

Hello,

Makefile: disable parallel build

IPA build system cannot cope with parallel build anyway, so this patch
disables parallel build explicitly so it does not blow up when user
has -j specified in default MAKEOPTS.

-- 
Petr^2 Spacek
From 6fe4d68d1357d3e7716ed3925b4d4a89f87f1a72 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Thu, 10 Dec 2015 18:35:20 +0100
Subject: [PATCH] Makefile: disable parallel build

IPA build system cannot cope with parallel build anyway, so this patch
disables parallel build explicitly so it does not blow up when user
has -j specified in default MAKEOPTS.
---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index 236fe472b8ea0a1492bc176b23da4083e14f5f28..751959a97e93e1b1ea4e3f4243a0a44b946628b8 100644
--- a/Makefile
+++ b/Makefile
@@ -1,3 +1,6 @@
+# IPA build system cannot cope with parallel build; disable parallel build
+.NOTPARALLEL:
+
 include VERSION
 
 SUBDIRS=asn1 daemons install ipapython ipa-client
-- 
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] First part of the replica promotion tests + testplan




On 09.12.2015 11:14, Oleg Fayans wrote:

Hi Martin

On 12/09/2015 10:30 AM, Martin Basti wrote:


On 08.12.2015 23:48, Oleg Fayans wrote:

Substituted a hardcoded suffix name with a constant DOMAIN_SUFFIX_NAME

On 12/08/2015 02:33 PM, Oleg Fayans wrote:

Hi all,


The patches are rebased against the current master.

On 12/02/2015 05:10 PM, Martin Basti wrote:

On 02.12.2015 16:18, Oleg Fayans wrote:

Hi Martin,

On 12/01/2015 04:08 PM, Martin Basti wrote:

On 27.11.2015 16:26, Oleg Fayans wrote:

And patch N 16 passes lint too:

On 11/27/2015 04:03 PM, Oleg Fayans wrote:

Hi,

On 11/27/2015 03:26 PM, Martin Basti wrote:

On 27.11.2015 15:04, Oleg Fayans wrote:

Hi Martin,

All your suggestions were taken into account. Both patches are
updated. Thank you for your help!

On 11/26/2015 10:50 AM, Martin Basti wrote:

On 26.11.2015 10:04, Oleg Fayans wrote:

Hi Martin,

I agree to all your points but one. please, see my comment
below

On 11/25/2015 07:42 PM, Martin Basti wrote:

Hi,

0) Note
Please be aware of
https://fedorahosted.org/freeipa/ticket/5469
during
KRA testing

1)
Please do not use MIN and MAX_DOMAIN_LEVEL constants, this may
change
over time, use DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1 for domain
level 0
and 1

2)
Why uninstall KRA then server, is not enough just uninstall
server
which
covers KRA uninstall?

+def teardown_method(self, method):
+for host in self.replicas:
+host.run_command(self.kra_uninstall,
raiseonerr=False)
+tasks.uninstall_master(host)


3)
Can be this function more generic? It should allow specify
host
where
KRA should be installed not just master

+def test_kra_install_master(self):
+self.master.run_command(self.kra_install)


4)

TestLevel0(Dummy):
Can be the test name more specific, something like
TestReplicaPromotionLevel0


5)
please remove this, the patch is on review and it will be
pushed
sooner
than tests
+@pytest.mark.xfail  # Ticket N 5455

and as I mentioned in ticket #5455, I cannot reproduce it with
ipa-kra-install, so please provide steps to reproduce if you
insist
that
this still does not work as expected with KRA.

6) This is completely wrong, it removes everything that we
tried to
achieve with previous patches with domain level in CI

Actually, being able to configure domain level per class is WAY
more
convenient, than to always have to think which domain level is
appropriate for which particular test during jenkins job
configuration. In fact, I should have thought about it from the
very
beginning. For example, in test_replica_promotion.py we have on
class,
which intiates with domain level = 1, while others - with
domain
level
0. With config-based approach, we would have to implement a
separate
step that raises domain level. Overall, I am against the
approach,
when you have to remember to set certain domain level in config
for
any particular test. The tests themselves should be aware of
the
domain level they need.

I do not say that we should not have something that overrides
settings
in from config in a particular test case, I say your patch is
doing it
wrong.

I agree it is useful to have param domain_level in
install_master,
and
intall_topo methods,  but is cannot be MAX_DOMAIN_LEVEL by
default,
because with your current patch the domain_level in config is
not
used
at all, it will be always MAX_DOMAIN_LEVEL

For example I want to achieve this goal:
test_vault.py, this test suite can run on domain level1 and on
domain
level0, so with one test we can test 2 domain levels just with
putting
domain level into config file.

I agree that with extraordinary test like replica promotion test
is, we
need something that allows override the config file.

As I said bellow, domain_level default value should be None in
install_master and install_topo plugin. If domain level was
specified
use the specified one, if not (value is None) use the domain
level
from
config file.

Agreed :)


Martin

[PATCH] Enabled setting domain_level per class derived from
TestIntegration

When I configure domain level 0 in yaml config, how is this
supposed to
get into install methods when you removed that code?

-"--domain-level=%i" % host.config.domain_level
+"--domain-level=%i" % domain_level


You always use MAX_DOMAIN_LEVEL in this case or whatever is
specified in
domain_level option.
I suggest to use domain_level=None, and when it is None use
'host.config.domain_level', if it is not None, use
'domain_level'

With this we can specify domain level in config file for test
that
can
be used for both domain levels and you can manually specify
domain
level
for test that requires specific domain level.

Also this should go away

@classmethod
def install(cls, mh):
+if hasattr(cls, "domain_level") and cls.master:
+cls.master.config.domain_level = cls.domain_level
if cls.topology is None:
return

I do not see reason why test should override configuration in
config in
this

Re: [Freeipa-devel] [PATCH 0071] replica: Fix ipa-replica-install with replica file (domain, level 0).



On 12/09/2015 08:31 AM, David Kupka wrote:
> On 08/12/15 16:33, Tomas Babej wrote:
>>
>>
>> On 12/08/2015 04:20 PM, Oleg Fayans wrote:
>>> ACK. The initial issue is fixed.
>>>
>>> On 12/08/2015 03:03 PM, David Kupka wrote:
 https://fedorahosted.org/freeipa/ticket/5531


>>>
>>
>> Can we get some more love for the patch and provide at least a sentence
>> worth of commit message before pushing?
>>
>> It's not obvious from the title what the patch does, other than it fixes
>> things.
>>
>> Tomas
>>
> I believe it's pretty obvious from linked ticket and attached patch
> changing just 5 lines.
> But you're right verbosity in commit message could save time later.
> Patch with changed commit message attached.
> 

Yes, I'd rather avoid the need to go to the ticket when perusing the git
log.

ACK, thanks.

Tomas

-- 
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 0395] replicainstall: Make sure the enrollment state is preserved

On 12/11/2015 06:05 PM, Martin Basti wrote:
> 
> 
> On 11.12.2015 14:03, Tomas Babej wrote:
>>
>> On 12/10/2015 02:22 PM, Tomas Babej wrote:
>>>
>>> On 12/10/2015 02:18 PM, Tomas Babej wrote:
 Hi,

 During the promote_check phase, the subsequent checks after the machine
 is enrolled may cause the installation to abort, hence leaving it
 enrolled even though it might not have been prior to the execution of
 the ipa-replica-install command.

 Make sure that ipa-client-install --uninstall is called on the machine
 that has not been enrolled before in case of failure during the
 promote_check phase.

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

>>> Self-NACK, installer is redundant for uninstall_client.
>>>
>>> Updated patch attached.
>>>
>>> Tomas
>>>
>>>
>>>
>> Attaching rebased version.
>>
>>
> ACK

Pushed to master: 90f7fa074ac10f907c7a300305e17e6de17bd29a

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 523-525] replica install: add remote connection check over API



On 12/11/2015 05:37 PM, Martin Basti wrote:
> 
> 
> On 11.12.2015 15:40, Jan Cholasta wrote:
>> On 11.12.2015 08:03, Jan Cholasta wrote:
>>> On 11.12.2015 07:08, Jan Cholasta wrote:
 On 10.12.2015 15:56, Martin Babinsky wrote:
> On 12/10/2015 09:48 AM, Jan Cholasta wrote:
>> On 9.12.2015 16:38, Jan Cholasta wrote:
>>> On 9.12.2015 14:52, Jan Cholasta wrote:
 On 9.12.2015 10:02, Jan Cholasta wrote:
> Hi,
>
> the attached patches fix
> .

 Note that this needs selinux-policy fix to work, so put SELinux
 into
 permissive mode for testing:
 .
>>>
>>> Updated patches attached.
>>
>> I screwed up a change in patch 524 and accidentally included a
>> chunk of
>> code in patch 525 that doesn't belong in it.
>>
>> Updated patches attached.
>>
>>
>>
>
> Patches work as expected and I was not able to find any functional
> problem.
>
> I have a question about the naming of the oddjob helper script: the
> one
> related to trusts is named 'com.redhat.idm.trust-fetch-domains',
> and the
> conncheck runner is named 'org.freeipa.server.conncheck'. I don't want
> to start another bikeshedding conversation but shouldn't we named them
> in a consistent fashion (either rename the first one in separate patch
> or rename the new helper to com.redhat.idm.server.conncheck)?
>
> I understand that as an upstream, we should go with the
> 'org.freeipa.*'
> convention, but having two helpers with different prefixes makes me
> sad.

 If you look at the larger picture, org.freeipa is the consistent name.
 It makes me sad as well, but mistakes should be corrected. This is
 similar to how we use PEP8 in new code, but do not fix it in old code
 just for the sake of fixing it.

>
> That is a nitpick though, it does not affect the overall functionality
> of the patches so ACK.

 Thanks for the review. The current patch 523 breaks the trusts oddjob
 with SELinux in enforcing mode, I will send an update which corrects
 that, until bug 1289930 is fixed.
>>>
>>> Updated patches attached.
>>
>> Rebased on top of current master.
>>
>>
>>
> Just question, should be any kinited user allowed to run conncheck via rpc?
> 
> Martin^2

I guess there's is little harm, any kinited user that was allowed to
access the machine could perform the conncheck even without these patches:

# ipa-replica-conncheck --master master.ipa.test -p ran...@ipa.test -w
ratarata -a -r IPA.TEST
Check connection from replica to remote master 'master.ipa.test':
   Directory Service: Unsecure port (389): OK
   Directory Service: Secure port (636): OK
   Kerberos KDC: TCP (88): OK
   Kerberos Kpasswd: TCP (464): OK
   HTTP Server: Unsecure port (80): OK
   HTTP Server: Secure port (443): OK

The following list of ports use UDP protocol and would need to be
checked manually:
   Kerberos KDC: UDP (88): SKIPPED
   Kerberos Kpasswd: UDP (464): SKIPPED

Connection from replica to master is OK.
Start listening on required ports for remote master check
Get credentials to log in to remote master
Check SSH connection to remote master
Execute check on remote master
Check connection from master to remote replica 'replica.ipa.test':
   Directory Service: Unsecure port (389): OK
   Directory Service: Secure port (636): OK
   Kerberos KDC: TCP (88): OK
   Kerberos KDC: UDP (88): OK
   Kerberos Kpasswd: TCP (464): OK
   Kerberos Kpasswd: UDP (464): OK
   HTTP Server: Unsecure port (80): OK
   HTTP Server: Secure port (443): OK

Connection from master to replica is OK.

-- 
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 0111] prevent crashes of server uninstall check caused by failed, 5 LDAP connections




On 07.12.2015 16:05, Martin Babinsky wrote:

On 12/04/2015 08:49 PM, Rob Crittenden wrote:

Martin Babinsky wrote:

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


Should it also warn about the potential loss of the DNSSEC master?

rob



Probably, but that warrants a separate ticket IMHO.

IIRC these checks are a part of replica deletion code at domain level 
1 in ipa-replica-manage, so it should be possible to transplant them 
on server uninstaller.



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

ACK
Rebased.
Pushed to master: 4cc206b0f82dd68d615f0aebba5b03acf127f53a

--
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 0395] replicainstall: Make sure the enrollment state is preserved




On 11.12.2015 14:03, Tomas Babej wrote:


On 12/10/2015 02:22 PM, Tomas Babej wrote:


On 12/10/2015 02:18 PM, Tomas Babej wrote:

Hi,

During the promote_check phase, the subsequent checks after the machine
is enrolled may cause the installation to abort, hence leaving it
enrolled even though it might not have been prior to the execution of
the ipa-replica-install command.

Make sure that ipa-client-install --uninstall is called on the machine
that has not been enrolled before in case of failure during the
promote_check phase.

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


Self-NACK, installer is redundant for uninstall_client.

Updated patch attached.

Tomas




Attaching rebased version.



ACK
-- 
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 0018] Fixed install_ca and install_kra failures at domain level 0




On 11.12.2015 17:28, Oleg Fayans wrote:

+myre = re.compile(".*Backed up to (?P.*?)\n.*")


IMO this regexp is not good.

1)
please name it better than "myre"

2)
initial '.*' is not needed because regexp does not start with '^' and 
you use search() later


3)

trailing '.*' is not needed as well, because it does not end with '$'

4)
You can use re.MULTILINE that will parse string per lines

path_re = re.compile("^Backed up to (?P.*)$", re.MULTILINE)

5)
+matched = myre.search(result.stdout_text + result.stderr_text)
Why do you need search in both stderr and stdout?

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] [PATCHES 529-530] ca install: use host credentials in domain level 1


Hi,

the attached patches fix .

My patches 523-525 are required for this: 
.


Honza

--
Jan Cholasta
From 4bcb399365501265ee062020ff9ef80fd6235a66 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 9 Dec 2015 10:31:18 +0100
Subject: [PATCH 1/2] aci: merge domain and CA suffix replication agreement
 ACIs

https://fedorahosted.org/freeipa/ticket/5399
---
 install/share/ca-topology.uldif |  6 --
 install/share/replica-acis.ldif |  6 +++---
 install/updates/20-aci.update   | 10 ++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/install/share/ca-topology.uldif b/install/share/ca-topology.uldif
index 7ce3cb1..fea591b 100644
--- a/install/share/ca-topology.uldif
+++ b/install/share/ca-topology.uldif
@@ -10,11 +10,5 @@ default: objectclass: iparepltopoconf
 default: ipaReplTopoConfRoot: o=ipaca
 default: cn: ca
 
-# Update CA replication settings
-dn: cn=o\3Dipaca,cn=mapping tree,cn=config
-add: aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-add: aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-add: aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "ldap:///cn=Remove Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-
 dn: cn=replica,cn=o\3Dipaca,cn=mapping tree,cn=config
 onlyifexist: nsds5replicabinddngroup: cn=replication managers,cn=sysaccounts,cn=etc,$SUFFIX
diff --git a/install/share/replica-acis.ldif b/install/share/replica-acis.ldif
index 8c0bc8e..6735130 100644
--- a/install/share/replica-acis.ldif
+++ b/install/share/replica-acis.ldif
@@ -1,16 +1,16 @@
 # Replica administration
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "ldap:///cn=Remove Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
diff --git a/install/updates/20-aci.update b/install/updates/20-aci.update
index ca4c0df..b06f569 100644
--- a/install/updates/20-aci.update
+++ b/install/updates/20-aci.update
@@ -66,6 +66,16 @@ add:aci:(targetattr="*")(version 3.0; acl "Admin can read all tasks"; allow (rea
 dn: cn=mapping tree,cn=config
 add:aci: (target = "ldap:///cn=meTo($$dn),cn=*,cn=mapping tree,cn=config")(targetattr = "objectclass || cn")(version 3.0; acl "Allow hosts to read their replication agreements"; allow(read, search, compare) userdn = "ldap:///fqdn=($$dn),cn=computers,cn=accounts,$SUFFIX";)
 
+dn: cn="$SUFFIX",cn=mapping tree,cn=config
+remove:aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
+remove:aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
+remove:aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "
+
+dn: cn=o\3Dipaca,cn=mapping tree,cn=config
+remove:aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication

Re: [Freeipa-devel] [PATCH 0373] Upgrade: Fix IPA version comparison

2015-12-13 Thread Lukas Slebodnik

On (09/12/15 19:22), Martin Basti wrote:
>https://fedorahosted.org/freeipa/ticket/5535
>
>Patch attached.

>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00 2001
>From: Martin Basti 
>Date: Wed, 9 Dec 2015 18:53:35 +0100
>Subject: [PATCH] Fix version comparison
>
>Use RPM library to compare vendor versions of IPA for redhat platform
>
>https://fedorahosted.org/freeipa/ticket/5535
>---
> freeipa.spec.in |  2 ++
> ipaplatform/redhat/tasks.py | 19 +++
> 2 files changed, 21 insertions(+)
>
>diff --git a/freeipa.spec.in b/freeipa.spec.in
>index 
>9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9
> 100644
>--- a/freeipa.spec.in
>+++ b/freeipa.spec.in
>@@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
> Requires: gzip
> Requires: python-gssapi >= 1.1.0
> Requires: custodia
>+Requires: rpm-python
>+Requires: rpmdevtools
Could you explain why do you need the 2nd package?
It does not contains any python modules
and I cannot see usage of any binary in this patch

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

[Freeipa-devel] [PATCH] Add 'review' target for make

2015-12-13 Thread Petr Spacek

Hello,

this patch automates some of sanity checks proposed by Petr Vobornik.

'make review' should be used in root of clean Git tree which has patches under
review applied.

Magic in review.sh attempts to detect nearest remote branch which can be used
as diff base for review. Please see review.sh for further details.

-- 
Petr^2 Spacek

-- 
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 0115] fix error message assertion in negative forced client reenrollment tests

2015-12-13 Thread Lukas Slebodnik

On (08/12/15 18:02), Martin Babinsky wrote:
>This patch fixes the assertions in negative test cases of
>'test_forced_client_reenrollment' CI test suite.
>
>On ipa-4-2 it fixes https://fedorahosted.org/freeipa/ticket/5511 and makes
>all 8 tests in this suite green, shiny and happy again.
>
>It also fixes negative test cases on master branch, but the positive cases
>are broken there due to https://fedorahosted.org/freeipa/ticket/5528
>
>-- 
>Martin^3 Babinsky

>From eb152f6996a8b653d8676ade826e806898fdf556 Mon Sep 17 00:00:00 2001
>From: Martin Babinsky 
>Date: Tue, 8 Dec 2015 17:00:11 +0100
>Subject: [PATCH] fix error message assertion in negative forced client
> reenrollment tests
>
>https://fedorahosted.org/freeipa/ticket/5511
>
>---
> ipatests/test_integration/test_forced_client_reenrollment.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/ipatests/test_integration/test_forced_client_reenrollment.py 
>b/ipatests/test_integration/test_forced_client_reenrollment.py
>index 
>e1edff9b7f2d535a341d1e8ca55917012943818e..df0e90526c5c8dd78d3af9d7ddb7c9bdbf5a2268
> 100644
>--- a/ipatests/test_integration/test_forced_client_reenrollment.py
>+++ b/ipatests/test_integration/test_forced_client_reenrollment.py
>@@ -198,7 +198,7 @@ class TestForcedClientReenrollment(IntegrationTest):
> assert 'IPA Server: %s' % server.hostname in result.stderr_text
> 
> if expect_fail:
>-err_msg = 'Kerberos authentication failed using keytab'
>+err_msg = "Kerberos authentication failed: "
Test passed for freeipa-4.2:
* fedora 23 with krb5-1.14
* rhel7.2 with krb5-1.13

ACK

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

[Freeipa-devel] [PATCH] 0047 dogtaginstance: remove unused function 'check_inst'

2015-12-13 Thread Fraser Tweedale

Just some drive-by cleanup of an unused function.

Cheers,
Fraser
From 6eb963aac30376a1d86bbdc4b9ce299cbec5220a Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 14 Dec 2015 16:52:40 +1100
Subject: [PATCH] dogtaginstance: remove unused function 'check_inst'

---
 ipaplatform/base/paths.py   |  1 -
 ipaserver/install/dogtaginstance.py | 17 -
 2 files changed, 18 deletions(-)

diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index 
762a38136e6c612767705389ee667b6f2ddab397..5513d77c831f8f93ba5878fd7b1d0f22a3640651
 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -236,7 +236,6 @@ class BasePathNamespace(object):
 SCHEMA_COMPAT_ULDIF = "/usr/share/ipa/schema_compat.uldif"
 IPA_JS_PLUGINS_DIR = "/usr/share/ipa/ui/js/plugins"
 UPDATES_DIR = "/usr/share/ipa/updates/"
-PKI_CONF_SERVER_XML_TEMPLATE = "/usr/share/pki/%s/conf/server.xml"
 CACHE_IPA_SESSIONS = "/var/cache/ipa/sessions"
 VAR_KERBEROS_KRB5KDC_DIR = "/var/kerberos/krb5kdc/"
 VAR_KRB5KDC_K5_REALM = "/var/kerberos/krb5kdc/.k5."
diff --git a/ipaserver/install/dogtaginstance.py 
b/ipaserver/install/dogtaginstance.py
index 
aad6fbbe5b00aa9d352d87b66ee3e7f91bf1a64e..193423d7e09cec17a82d4f5da2ed6c43accf1c0c
 100644
--- a/ipaserver/install/dogtaginstance.py
+++ b/ipaserver/install/dogtaginstance.py
@@ -47,23 +47,6 @@ from ipapython.ipa_log_manager import log_mgr
 PKI_USER = "pkiuser"
 
 
-def check_inst(subsystem):
-"""
-Validate that the appropriate dogtag/RHCS packages have been installed.
-"""
-
-# Check for a couple of binaries we need
-if not os.path.exists(paths.PKISPAWN):
-return False
-if not os.path.exists(paths.PKIDESTROY):
-return False
-
-if not os.path.exists(paths.PKI_CONF_SERVER_XML_TEMPLATE % subsystem):
-return False
-
-return True
-
-
 def get_security_domain():
 """
 Get the security domain from the REST interface on the local Dogtag CA
-- 
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 0373] Upgrade: Fix IPA version comparison


On 11.12.2015 19:01, Tomas Babej wrote:



On 12/11/2015 09:36 AM, Martin Kosek wrote:

On 12/10/2015 05:09 PM, Martin Basti wrote:



On 10.12.2015 15:49, Tomas Babej wrote:


On 12/10/2015 11:23 AM, Martin Basti wrote:


On 10.12.2015 09:13, Lukas Slebodnik wrote:

On (09/12/15 19:22), Martin Basti wrote:

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

Patch attached.

>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00
2001

From: Martin Basti 
Date: Wed, 9 Dec 2015 18:53:35 +0100
Subject: [PATCH] Fix version comparison

Use RPM library to compare vendor versions of IPA for redhat platform

https://fedorahosted.org/freeipa/ticket/5535
---
freeipa.spec.in |  2 ++
ipaplatform/redhat/tasks.py | 19 +++
2 files changed, 21 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index
9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9


100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
Requires: gzip
Requires: python-gssapi >= 1.1.0
Requires: custodia
+Requires: rpm-python
+Requires: rpmdevtools

Could you explain why do you need the 2nd package?
It does not contains any python modules
and I cannot see usage of any binary in this patch

LS

Thanks for this catch, it is actually located in yum package, I rather
copy stringToVersion function from there to IPA, to avoid dependency
hell

Updated patch attached.



Looking good. The __cmp__ function, however, is not available in Python
3. As we will eventually support python3 in RHEL as well, maybe we
should make sure even platform-dependent parts are python3 compatible?
For the future's sake.

Tomas


Thanks,

python 3 compatible patch attached.


I wonder why we have to add so much code here and reimplement RPM
version checking if it is already provided by rpmdevtools:

~~~
$ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
WARNING: hyphen in release1: 4.2.0-15.el7

rpmdev-vercmp  
rpmdev-vercmp  
rpmdev-vercmp # with no arguments, prompt

Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and 12 if
EVR2
is newer.  Other exit statuses indicate problems.

ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
12
~~~

which could be directly called from __eq__ or __lt__, since we are in
platform specific code anyway already.

Martin


Imho we should generally prefer reaching out to a Python library rather
launching a subprocess to compare the versions, it's unnecessary overhead.

That said, the main argument for the usage of rpm-python over
rpmdevtools I see is that rpm-python is very likely to be present on the
system (i.e. it is yum's own dependency), while rpmdevtools will not be
present by default.

 From the standpoint of trying to minimize the size of freeipa
installation (i.e recent wget -> curl migration), we should keep the
spirit here and do not introduce a dependency for a collection of
developer tools.

/2 cents


+1, also a single function is hardly "much code".

--
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] [PATCHES 523-525] replica install: add remote connection check over API


On 11.12.2015 18:49, Tomas Babej wrote:



On 12/11/2015 05:37 PM, Martin Basti wrote:



On 11.12.2015 15:40, Jan Cholasta wrote:

On 11.12.2015 08:03, Jan Cholasta wrote:

On 11.12.2015 07:08, Jan Cholasta wrote:

On 10.12.2015 15:56, Martin Babinsky wrote:

On 12/10/2015 09:48 AM, Jan Cholasta wrote:

On 9.12.2015 16:38, Jan Cholasta wrote:

On 9.12.2015 14:52, Jan Cholasta wrote:

On 9.12.2015 10:02, Jan Cholasta wrote:

Hi,

the attached patches fix
.


Note that this needs selinux-policy fix to work, so put SELinux
into
permissive mode for testing:
.


Updated patches attached.


I screwed up a change in patch 524 and accidentally included a
chunk of
code in patch 525 that doesn't belong in it.

Updated patches attached.





Patches work as expected and I was not able to find any functional
problem.

I have a question about the naming of the oddjob helper script: the
one
related to trusts is named 'com.redhat.idm.trust-fetch-domains',
and the
conncheck runner is named 'org.freeipa.server.conncheck'. I don't want
to start another bikeshedding conversation but shouldn't we named them
in a consistent fashion (either rename the first one in separate patch
or rename the new helper to com.redhat.idm.server.conncheck)?

I understand that as an upstream, we should go with the
'org.freeipa.*'
convention, but having two helpers with different prefixes makes me
sad.


If you look at the larger picture, org.freeipa is the consistent name.
It makes me sad as well, but mistakes should be corrected. This is
similar to how we use PEP8 in new code, but do not fix it in old code
just for the sake of fixing it.



That is a nitpick though, it does not affect the overall functionality
of the patches so ACK.


Thanks for the review. The current patch 523 breaks the trusts oddjob
with SELinux in enforcing mode, I will send an update which corrects
that, until bug 1289930 is fixed.


Updated patches attached.


Rebased on top of current master.




Just question, should be any kinited user allowed to run conncheck via rpc?

Martin^2


I guess there's is little harm, any kinited user that was allowed to
access the machine could perform the conncheck even without these patches:


In the RPC check, the user must have the Replication Administrators 
privilege, which by default only admins have.




# ipa-replica-conncheck --master master.ipa.test -p ran...@ipa.test -w
ratarata -a -r IPA.TEST
Check connection from replica to remote master 'master.ipa.test':
Directory Service: Unsecure port (389): OK
Directory Service: Secure port (636): OK
Kerberos KDC: TCP (88): OK
Kerberos Kpasswd: TCP (464): OK
HTTP Server: Unsecure port (80): OK
HTTP Server: Secure port (443): OK

The following list of ports use UDP protocol and would need to be
checked manually:
Kerberos KDC: UDP (88): SKIPPED
Kerberos Kpasswd: UDP (464): SKIPPED

Connection from replica to master is OK.
Start listening on required ports for remote master check
Get credentials to log in to remote master
Check SSH connection to remote master
Execute check on remote master
Check connection from master to remote replica 'replica.ipa.test':
Directory Service: Unsecure port (389): OK
Directory Service: Secure port (636): OK
Kerberos KDC: TCP (88): OK
Kerberos KDC: UDP (88): OK
Kerberos Kpasswd: TCP (464): OK
Kerberos Kpasswd: UDP (464): OK
HTTP Server: Unsecure port (80): OK
HTTP Server: Secure port (443): OK

Connection from master to replica is OK.




--
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] [PATCHES 529-530] ca install: use host credentials in domain level 1


On 11.12.2015 17:24, Martin Basti wrote:



On 11.12.2015 15:00, Jan Cholasta wrote:

On 10.12.2015 09:51, Jan Cholasta wrote:

Hi,

the attached patches fix .

My patches 523-525 are required for this:
.



Honza


Rebased patches attached.


Patch works for me, but can you provide explanations (and update commit
message) why the ACI change is needed:

* why it is moved three ACIs from 'cn="$SUFFIX",cn=mapping
tree,cn=config' to 'cn=mapping tree,cn=config'


So that they apply to all replication agreements.


* why you removed completely 'dn: cn=o\3Dipaca,cn=mapping tree,cn=config'


I didn't, they were moved to cn=mapping tree,cn=config as well.

Updated patches attached.

--
Jan Cholasta
From 730b9c2f5693020272a7458b9540366bca56b430 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 9 Dec 2015 10:31:18 +0100
Subject: [PATCH 1/2] aci: merge domain and CA suffix replication agreement
 ACIs

Merge the two identical sets of replication agreement permission ACIs for
the domain and CA suffixes into a single set suitable for replication
agreements for both suffixes. This makes the replication agreement
permissions behave correctly during CA replica install, so that any
non-admin user with the proper permissions (such as members of the
ipaservers host group) can set up replication for the CA suffix.

https://fedorahosted.org/freeipa/ticket/5399
---
 install/share/ca-topology.uldif |  6 --
 install/share/replica-acis.ldif |  6 +++---
 install/updates/20-aci.update   | 10 ++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/install/share/ca-topology.uldif b/install/share/ca-topology.uldif
index 7ce3cb1..fea591b 100644
--- a/install/share/ca-topology.uldif
+++ b/install/share/ca-topology.uldif
@@ -10,11 +10,5 @@ default: objectclass: iparepltopoconf
 default: ipaReplTopoConfRoot: o=ipaca
 default: cn: ca
 
-# Update CA replication settings
-dn: cn=o\3Dipaca,cn=mapping tree,cn=config
-add: aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-add: aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-add: aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "ldap:///cn=Remove Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
-
 dn: cn=replica,cn=o\3Dipaca,cn=mapping tree,cn=config
 onlyifexist: nsds5replicabinddngroup: cn=replication managers,cn=sysaccounts,cn=etc,$SUFFIX
diff --git a/install/share/replica-acis.ldif b/install/share/replica-acis.ldif
index 8c0bc8e..6735130 100644
--- a/install/share/replica-acis.ldif
+++ b/install/share/replica-acis.ldif
@@ -1,16 +1,16 @@
 # Replica administration
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(version 3.0;acl "permission:Add Replication Agreements";allow (add) groupdn = "ldap:///cn=Add Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5Replica)(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement)(objectClass=nsMappingTree))")(version 3.0; acl "permission:Modify Replication Agreements"; allow (read, write, search) groupdn = "ldap:///cn=Modify Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
 
-dn: cn="$SUFFIX",cn=mapping tree,cn=config
+dn: cn=mapping tree,cn=config
 changetype: modify
 add: aci
 aci: (targetattr=*)(targetfilter="(|(objectclass=nsds5replicationagreement)(objectclass=nsDSWindowsReplicationAgreement))")(version 3.0;acl "permission:Remove Replication Agreements";allow (delete) groupdn = "ldap:///cn=Remove Replication Agreements,cn=permissions,cn=pbac,$SUFFIX";)
diff --git a/install/updates/20-aci.update b/install/updates/20-aci.update
index 5b9741d..cef842b 100644
--- a/install/updates/20-aci.update
+++ b/install/updates/20-aci.update
@@ -66,6 +66,16 @@ add:aci:(targetattr="*")(version 3.0; acl "Admin can read all tasks"; allow (rea
 dn: cn=mapping tree,cn=config
 add:aci: (target = "ldap:///cn=meTo($$dn),cn=*,cn=mapping tree,cn=config")(targetattr = "objectclass || cn")(version 3.0; acl "Allow hosts to read their replication agreements"; allow(read, search, compare) userdn =

Re: [Freeipa-devel] [PATCH] 0751 spec: Split out python-ipap11helper and, python-default_encoding_utf8