date:20160301

Re: [Freeipa-devel] [PATCH 016, 024, 025] First part of the replica promotion tests + testplan

2016-03-01 Thread Oleg Fayans

Hi Martin,

On 03/01/2016 07:04 PM, Martin Basti wrote:
> 
> 
> On 01.03.2016 14:56, Martin Basti wrote:
>>
>>
>>
>> On 01.03.2016 12:37, Martin Basti wrote:
>>>
>>>
>>> On 01.03.2016 12:32, Martin Basti wrote:


 On 29.02.2016 13:16, Oleg Fayans wrote:
> Hi all,
>
> Finally the tests pass.
>
> The patch 0024 applies on top of patch 0022 (please, consider
> reviewing
> it also). Besides, the whole functionality depends on Martin's
> patch N 0421
>
> All patches pass pylint.
 hello,

 I cannot apply patches on master branch
 Martin^2
>>> My bad I applied wrong patch
>>>
>
>
> On 12/19/2015 11:56 PM, Martin Basti wrote:
>>
>> On 17.12.2015 10:04, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> I am sorry, in my previous email I attached the old version of patch
>>> 0016. The correct on is attached.
>>>
>>> On 12/16/2015 05:47 PM, Martin Basti wrote:
 On 16.12.2015 15:39, Martin Basti wrote:
> On 15.12.2015 10:29, Oleg Fayans wrote:
>> Hi Martin,
>>
>> The updated patches are attached. Patch 0017 includes all
>> changes from
>> patch 0018, so, if you approve this one, there would be no
>> need to
>> continue with the review of 0018. This one contains all changes
>> related
>> to you remarks from 0018 review. Please see my explanation on the
>> stdout+stderr part in the thread from patch 0018.
>> With these two patches applied one of the tests fails due this
>> bug:
>> https://fedorahosted.org/freeipa/ticket/5550
>>
>> On 12/09/2015 12:17 PM, Martin Basti wrote:
>>> On 09.12.2015 12:10, Martin Basti wrote:
 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)
>>
>>

Re: [Freeipa-devel] [PATCH 0428] SPEC: do not execute upgrade when ipa server is not installed

2016-03-01 Thread Rob Crittenden

Martin Basti wrote:
> 
> 
> On 01.03.2016 20:13, Rob Crittenden wrote:
>> Martin Basti wrote:
>>> https://fedorahosted.org/freeipa/ticket/5704
>>>
>>> Patch attached.
>>>
>>>
>> Would it be safer to integrate this into ipa-upgrade itself? You'd just
>> need to return 0 for the case where IPA isn't installed.
>>
>> rob
> How about the case when ipa-server-upgrade is called by user from CLI?
> It should fail because IPA is not installed, instead of returning
> success. That check is in specfile anyway due service restart.
> 
> Martin^2

Yeah, I was hoping you'd miss that :-)

It just seems to me, as you point out, that it should check when run by
anything, user or spec, so adding it only to the spec seems wrong. I'm
not a huge fan of option bloat but that would be one way around this,
--graceful-exit or something. Could make it a hidden option if you wanted.

rob

-- 
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 0428] SPEC: do not execute upgrade when ipa server is not installed

2016-03-01 Thread Rob Crittenden

Martin Basti wrote:
> https://fedorahosted.org/freeipa/ticket/5704
> 
> Patch attached.
> 
> 

Would it be safer to integrate this into ipa-upgrade itself? You'd just
need to return 0 for the case where IPA isn't installed.

rob

-- 
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 0428] SPEC: do not execute upgrade when ipa server is not installed

2016-03-01 Thread Martin Basti


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

Patch attached.
From 416ce2c0ab50f80d5d976dfa6a2b9a12975e15fb Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 1 Mar 2016 18:56:28 +0100
Subject: [PATCH] SPEC: do not run upgrade when ipa server is not installed

Running upgrade when IPA is not installed produces false positive errors

https://fedorahosted.org/freeipa/ticket/5704
---
 freeipa.spec.in | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 2ec7fab88d658111d210b0368d9076917e4a98bf..fbe50becb017e897ea230cc3f68515d39fba7e6f 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -875,15 +875,18 @@ fi
 
 
 %posttrans server
-# This must be run in posttrans so that updates from previous
-# execution that may no longer be shipped are not applied.
-/usr/sbin/ipa-server-upgrade --quiet >/dev/null || :
-
-# Restart IPA processes. This must be also run in postrans so that plugins
-# and software is in consistent state
+# don't execute upgrade and restart of IPA when server is not installed
 python2 -c "import sys; from ipaserver.install import installutils; sys.exit(0 if installutils.is_ipa_configured() else 1);" > /dev/null 2>&1
-# NOTE: systemd specific section
+
 if [  $? -eq 0 ]; then
+# This must be run in posttrans so that updates from previous
+# execution that may no longer be shipped are not applied.
+/usr/sbin/ipa-server-upgrade --quiet >/dev/null || :
+
+# Restart IPA processes. This must be also run in postrans so that plugins
+# and software is in consistent state
+# NOTE: systemd specific section
+
 /bin/systemctl is-enabled ipa.service >/dev/null 2>&1
 if [  $? -eq 0 ]; then
 /bin/systemctl restart ipa.service >/dev/null 2>&1 || :
-- 
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 016, 024, 025] First part of the replica promotion tests + testplan

2016-03-01 Thread Martin Basti




On 01.03.2016 14:56, Martin Basti wrote:




On 01.03.2016 12:37, Martin Basti wrote:



On 01.03.2016 12:32, Martin Basti wrote:



On 29.02.2016 13:16, Oleg Fayans wrote:

Hi all,

Finally the tests pass.

The patch 0024 applies on top of patch 0022 (please, consider 
reviewing
it also). Besides, the whole functionality depends on Martin's 
patch N 0421


All patches pass pylint.

hello,

I cannot apply patches on master branch
Martin^2

My bad I applied wrong patch




On 12/19/2015 11:56 PM, Martin Basti wrote:


On 17.12.2015 10:04, Oleg Fayans wrote:

Hi Martin,

I am sorry, in my previous email I attached the old version of patch
0016. The correct on is attached.

On 12/16/2015 05:47 PM, Martin Basti wrote:

On 16.12.2015 15:39, Martin Basti wrote:

On 15.12.2015 10:29, Oleg Fayans wrote:

Hi Martin,

The updated patches are attached. Patch 0017 includes all 
changes from
patch 0018, so, if you approve this one, there would be no 
need to

continue with the review of 0018. This one contains all changes
related
to you remarks from 0018 review. Please see my explanation on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this 
bug:

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

On 12/09/2015 12:17 PM, Martin Basti wrote:

On 09.12.2015 12:10, Martin Basti wrote:

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

Re: [Freeipa-devel] [PATCH 0427] fix broken configuration of sidgen and extdom plugins

2016-03-01 Thread Martin Basti




On 01.03.2016 17:14, Martin Basti wrote:



On 01.03.2016 17:13, Petr Vobornik wrote:

On 03/01/2016 05:02 PM, Martin Basti wrote:



On 01.03.2016 16:39, Petr Vobornik wrote:

On 02/23/2016 06:15 PM, Martin Basti wrote:



On 23.02.2016 17:31, Tomas Babej wrote:


On 02/23/2016 01:25 PM, Martin Basti wrote:


On 23.02.2016 13:02, Alexander Bokovoy wrote:

On Tue, 23 Feb 2016, Martin Basti wrote:
 From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 
00:00:00

2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket 
after DS

restart

Restarting DS executed by upgrade plugin causes that upgrade
frameworg
was waiting for not proper socket to be ready. This commit fix
issue.

Please fix the commit message typos.


Fixed. Updated patches attached.

ACK.

Tomas

Pushed to master: 0accf8ccb64963954dbe7c137d23f52e5901ac4f
Pushed to ipa-4-3: 4734012c8063460f93f3b819a5bbcca797f6059e
Pushed to ipa-4-2: 63d8caf0d105f02decc0b5d865fedf6ad063bc1a



Testing freeipa-4.2.4 build and it fails at
install/dsinstance.py:add_sidgen_plugin:936

adding self.ldap_connect() on line 937 fixed the issue.


Well I may rework PATCH 0416, and fix it in different way, or I can add
self.ldap_connect() to sidgen and extdom steps.

Which is better?



I would avoid reworking it in all 3 branches if in 4.3 and master it 
works and is actually correct. Doesn't make sense to change new code 
because of missing features in old branches. Adding connect to 
ipa-4-2 seems enough to me.


IMO it works accidentaly there, any additional patch or restart can 
break it in 4.3 and master too



Patch attached.

ipa-4-2 needs to have backported ca8f63624b204f0c4f2d1ac5f4ed93551f7e62df
From fbdf7938dfb150e618a13310bc71de396a0ba944 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Tue, 1 Mar 2016 17:36:55 +0100
Subject: [PATCH] Fix connections to DS during installation

Regression caused by commit 9818e463f5d0a91b300801ee7c8f31f25de402b2,
admin_conn should be connected in method if there is no connection.

https://fedorahosted.org/freeipa/ticket/5665
---
 ipaserver/install/dsinstance.py | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 93af0ac0a3f6a9a36fbc500f05f9795f9db0de2f..b7a480749f0edb0dca5af7229d023deefd45768a 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -1067,6 +1067,9 @@ class DsInstance(service.Service):
 """
 Add sidgen plugin configuration only if it does not already exist.
 """
+if not self.admin_conn:
+self.ldap_connect()
+
 dn = DN('cn=IPA SIDGEN,cn=plugins,cn=config')
 try:
 self.admin_conn.get_entry(dn)
@@ -1085,6 +1088,9 @@ class DsInstance(service.Service):
 """
 Add extdom configuration if it does not already exist.
 """
+if not self.admin_conn:
+self.ldap_connect()
+
 dn = DN('cn=ipa_extdom_extop,cn=plugins,cn=config')
 try:
 self.admin_conn.get_entry(dn)
-- 
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 0425] pylint: suppress false positive no-member errors

2016-03-01 Thread Martin Basti




On 25.02.2016 17:50, Martin Basti wrote:



On 25.02.2016 15:48, Martin Basti wrote:

The last pylint 1.5 patch, \o/

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



self-NACK too broad disables



Updated patches attached.
From 3e99288583f02661ed4875e90a10491948f42564 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 25 Feb 2016 13:46:33 +0100
Subject: [PATCH] pylint: supress false positive no-member errors

pylint 1.5 prints many false positive no-member errors which are
supressed by this commit.

https://fedorahosted.org/freeipa/ticket/5615
---
 install/tools/ipactl | 4 ++--
 ipalib/krb_utils.py  | 2 +-
 ipalib/plugins/batch.py  | 9 +++--
 ipalib/plugins/server.py | 5 +++--
 ipapython/ipaldap.py | 2 +-
 ipapython/ipautil.py | 2 +-
 ipapython/nsslib.py  | 7 +--
 ipaserver/install/installutils.py| 9 +++--
 ipaserver/install/ipa_otptoken_import.py | 5 +++--
 ipaserver/install/server/install.py  | 3 ++-
 ipaserver/install/service.py | 3 ++-
 ipatests/test_xmlrpc/xmlrpc_test.py  | 2 ++
 12 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/install/tools/ipactl b/install/tools/ipactl
index ff5ea5a50a291da35e895d5674bdc8ea76ed48d4..d27ada56556c58c42242cf0add11ef47d4440f56 100755
--- a/install/tools/ipactl
+++ b/install/tools/ipactl
@@ -290,7 +290,7 @@ def ipa_start(options):
 
 if isinstance(e, IpactlError):
 # do not display any other error message
-raise IpactlError(rval=e.rval)
+raise IpactlError(rval=e.rval)  # pylint: disable=no-member
 else:
 raise IpactlError()
 
@@ -387,7 +387,7 @@ def ipa_restart(options):
 pass
 if isinstance(e, IpactlError):
 # do not display any other error message
-raise IpactlError(rval=e.rval)
+raise IpactlError(rval=e.rval)  # pylint: disable=no-member
 else:
 raise IpactlError()
 
diff --git a/ipalib/krb_utils.py b/ipalib/krb_utils.py
index b33e4b7c82cf08c68220531ebacca309117ad770..e6e277c7a0926187ebcde7ba08e45ebb56ad865e 100644
--- a/ipalib/krb_utils.py
+++ b/ipalib/krb_utils.py
@@ -160,7 +160,7 @@ def get_credentials(name=None, ccache_name=None):
 try:
 return gssapi.Credentials(usage='initiate', name=name, store=store)
 except gssapi.exceptions.GSSError as e:
-if e.min_code == KRB5_FCC_NOFILE:
+if e.min_code == KRB5_FCC_NOFILE:  # pylint: disable=no-member
 raise ValueError('"%s", ccache="%s"' % (e.message, ccache_name))
 raise
 
diff --git a/ipalib/plugins/batch.py b/ipalib/plugins/batch.py
index 626ba2835bd5387df3d49d66293284f40e4b0d42..2da7b7ca811fc67b22c43655352ace539488ce0d 100644
--- a/ipalib/plugins/batch.py
+++ b/ipalib/plugins/batch.py
@@ -114,11 +114,16 @@ class batch(Command):
 if isinstance(e, errors.RequirementError) or \
 isinstance(e, errors.CommandError):
 self.info(
-'%s: batch: %s', context.principal, e.__class__.__name__
+'%s: batch: %s',
+context.principal,  # pylint: disable=no-member
+e.__class__.__name__
 )
 else:
 self.info(
-'%s: batch: %s(%s): %s', context.principal, name, ', '.join(api.Command[name]._repr_iter(**params)),  e.__class__.__name__
+'%s: batch: %s(%s): %s',
+context.principal, name,  # pylint: disable=no-member
+', '.join(api.Command[name]._repr_iter(**params)),
+e.__class__.__name__
 )
 if isinstance(e, errors.PublicError):
 reported_error = e
diff --git a/ipalib/plugins/server.py b/ipalib/plugins/server.py
index e31def77cc649e6392ea8527040e61a56a83ff2f..93ced8b73049b61fe274c15d84150a892cd34529 100644
--- a/ipalib/plugins/server.py
+++ b/ipalib/plugins/server.py
@@ -228,8 +228,9 @@ class server_conncheck(crud.PKQuery):
 privilege = u'Replication Administrators'
 privilege_dn = self.api.Object.privilege.get_dn(privilege)
 ldap = self.obj.backend
-filter = ldap.make_filter(
-{'krbprincipalname': context.principal, 'memberof': privilege_dn},
+filter = ldap.make_filter({
+'krbprincipalname': context.principal,  # pylint: disable=no-member
+'memberof': privilege_dn},
 rules=ldap.MATCH_ALL)
 try:
 ldap.find_entries(base_dn=self.api.env.basedn, filter=filter)
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 7522c504b5b8901002776521f05f4ebab8c35ec8..2965ba4a5509eb16589bc9dde9485147d9114032 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py

Re: [Freeipa-devel] [PATCH 0416-0419] fix broken configuration of sidgen and extdom plugins

2016-03-01 Thread Petr Vobornik


On 03/01/2016 05:02 PM, Martin Basti wrote:



On 01.03.2016 16:39, Petr Vobornik wrote:

On 02/23/2016 06:15 PM, Martin Basti wrote:



On 23.02.2016 17:31, Tomas Babej wrote:


On 02/23/2016 01:25 PM, Martin Basti wrote:


On 23.02.2016 13:02, Alexander Bokovoy wrote:

On Tue, 23 Feb 2016, Martin Basti wrote:

 From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00
2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS
restart

Restarting DS executed by upgrade plugin causes that upgrade
frameworg
was waiting for not proper socket to be ready. This commit fix
issue.

Please fix the commit message typos.


Fixed. Updated patches attached.

ACK.

Tomas

Pushed to master: 0accf8ccb64963954dbe7c137d23f52e5901ac4f
Pushed to ipa-4-3: 4734012c8063460f93f3b819a5bbcca797f6059e
Pushed to ipa-4-2: 63d8caf0d105f02decc0b5d865fedf6ad063bc1a



Testing freeipa-4.2.4 build and it fails at
install/dsinstance.py:add_sidgen_plugin:936

adding self.ldap_connect() on line 937 fixed the issue.


Well I may rework PATCH 0416, and fix it in different way, or I can add
self.ldap_connect() to sidgen and extdom steps.

Which is better?



I would avoid reworking it in all 3 branches if in 4.3 and master it 
works and is actually correct. Doesn't make sense to change new code 
because of missing features in old branches. Adding connect to ipa-4-2 
seems enough to me.

--
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] [python-pytest-multihost] Request to add support to specify username/password for each host

2016-03-01 Thread Petr Viktorin

On 02/29/2016 05:57 PM, Niranjan wrote:
> Greetings,
> 
> Attached wrong patch in my last mail. Request to review the patch 
> attached to this email
> 

Hello,
I've generalized the patch a bit to make it use existing host
attributes, and I added tests.
Could you check if it still works for you?


-- 
Petr Viktorin
From 7f8c83091d52d2aea509f90634ccb7f2b1a880d4 Mon Sep 17 00:00:00 2001
From: Niranjan MR 
Date: Fri, 26 Feb 2016 15:30:25 +0530
Subject: [PATCH] Add support to specify usernames/password per host

Additional generalizations by Petr Viktorin

https://fedorahosted.org/python-pytest-multihost/ticket/5

Signed-off-by: Niranjan MR 
Signed-off-by: Petr Viktorin 
---
 pytest_multihost/host.py   | 21 ---
 pytest_multihost/transport.py  |  9 +++--
 test_pytestmultihost/test_localhost.py | 65 ++
 3 files changed, 87 insertions(+), 8 deletions(-)

diff --git a/pytest_multihost/host.py b/pytest_multihost/host.py
index f3346f6..15fb43c 100644
--- a/pytest_multihost/host.py
+++ b/pytest_multihost/host.py
@@ -27,9 +27,19 @@ class BaseHost(object):
 transport_class = None
 
 def __init__(self, domain, hostname, role, ip=None,
- external_hostname=None):
+ external_hostname=None, username=None, password=None):
 self.domain = domain
 self.role = str(role)
+if username is None:
+self.ssh_username = self.config.ssh_username
+else:
+self.ssh_username = username
+if password is None:
+self.ssh_key_filename = self.config.ssh_key_filename
+self.ssh_password = self.config.ssh_password
+else:
+self.ssh_key_filename = None
+self.ssh_password = password
 
 shortname, dot, ext_domain = hostname.partition('.')
 self.shortname = shortname
@@ -65,9 +75,6 @@ class BaseHost(object):
 raise RuntimeError('Could not determine IP address of %s' %
self.external_hostname)
 
-self.ssh_password = self.config.ssh_password
-self.ssh_key_filename = self.config.ssh_key_filename
-self.ssh_username = self.config.ssh_username
 self.host_key = None
 self.ssh_port = 22
 
@@ -109,9 +116,13 @@ class BaseHost(object):
 ip = dct.pop('ip', None)
 external_hostname = dct.pop('external_hostname', None)
 
+username = dct.pop('username', None)
+password = dct.pop('password', None)
+
 check_config_dict_empty(dct, 'host %s' % hostname)
 
-return cls(domain, hostname, role, ip, external_hostname)
+return cls(domain, hostname, role, ip, external_hostname,
+   username, password)
 
 def to_dict(self):
 """Export info about this Host to a dict"""
diff --git a/pytest_multihost/transport.py b/pytest_multihost/transport.py
index eda71cd..8a36f02 100644
--- a/pytest_multihost/transport.py
+++ b/pytest_multihost/transport.py
@@ -178,13 +178,16 @@ class ParamikoTransport(Transport):
 self._transport = transport = paramiko.Transport(sock)
 transport.connect(hostkey=host.host_key)
 if host.ssh_key_filename:
-self.log.debug('Authenticating with private RSA key')
 filename = os.path.expanduser(host.ssh_key_filename)
 key = paramiko.RSAKey.from_private_key_file(filename)
+self.log.debug(
+'Authenticating with private RSA key using user %s' %
+host.ssh_username)
 transport.auth_publickey(username=host.ssh_username, key=key)
 elif host.ssh_password:
-self.log.debug('Authenticating with password')
-transport.auth_password(username='root',
+self.log.debug('Authenticating with password using user %s' %
+   host.ssh_username)
+transport.auth_password(username=host.ssh_username,
 password=host.ssh_password)
 else:
 self.log.critical('No SSH credentials configured')
diff --git a/test_pytestmultihost/test_localhost.py b/test_pytestmultihost/test_localhost.py
index 265c3b8..0ec8216 100644
--- a/test_pytestmultihost/test_localhost.py
+++ b/test_pytestmultihost/test_localhost.py
@@ -31,6 +31,21 @@ def get_conf_dict():
 'ip': '127.0.0.1',
 'role': 'local',
 },
+{
+'name': 'localhost',
+'external_hostname': 'localhost',
+'ip': '127.0.0.1',
+'username': '__nonexisting_test_username__',
+'role': 'badusername',
+},
+{
+'name': 'localhost',
+'external_hostname':

Re: [Freeipa-devel] [PATCH 0416-0419] fix broken configuration of sidgen and extdom plugins

2016-03-01 Thread Martin Basti




On 01.03.2016 16:39, Petr Vobornik wrote:

On 02/23/2016 06:15 PM, Martin Basti wrote:



On 23.02.2016 17:31, Tomas Babej wrote:


On 02/23/2016 01:25 PM, Martin Basti wrote:


On 23.02.2016 13:02, Alexander Bokovoy wrote:

On Tue, 23 Feb 2016, Martin Basti wrote:

 From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00
2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS
restart

Restarting DS executed by upgrade plugin causes that upgrade 
frameworg
was waiting for not proper socket to be ready. This commit fix 
issue.

Please fix the commit message typos.


Fixed. Updated patches attached.

ACK.

Tomas

Pushed to master: 0accf8ccb64963954dbe7c137d23f52e5901ac4f
Pushed to ipa-4-3: 4734012c8063460f93f3b819a5bbcca797f6059e
Pushed to ipa-4-2: 63d8caf0d105f02decc0b5d865fedf6ad063bc1a



Testing freeipa-4.2.4 build and it fails at 
install/dsinstance.py:add_sidgen_plugin:936


adding self.ldap_connect() on line 937 fixed the issue.


Well I may rework PATCH 0416, and fix it in different way, or I can add 
self.ldap_connect() to sidgen and extdom steps.


Which is better?


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

Re: [Freeipa-devel] [PATCH 0416-0419] fix broken configuration of sidgen and extdom plugins

2016-03-01 Thread Petr Vobornik


On 02/23/2016 06:15 PM, Martin Basti wrote:



On 23.02.2016 17:31, Tomas Babej wrote:


On 02/23/2016 01:25 PM, Martin Basti wrote:


On 23.02.2016 13:02, Alexander Bokovoy wrote:

On Tue, 23 Feb 2016, Martin Basti wrote:

 From f2ae1bd129a1741500d2f3dcb86a0da553604d15 Mon Sep 17 00:00:00
2001
From: Martin Basti 
Date: Tue, 23 Feb 2016 10:37:47 +0100
Subject: [PATCH 4/4] fix upgrade: wait for proper DS socket after DS
restart

Restarting DS executed by upgrade plugin causes that upgrade frameworg
was waiting for not proper socket to be ready. This commit fix issue.

Please fix the commit message typos.


Fixed. Updated patches attached.

ACK.

Tomas

Pushed to master: 0accf8ccb64963954dbe7c137d23f52e5901ac4f
Pushed to ipa-4-3: 4734012c8063460f93f3b819a5bbcca797f6059e
Pushed to ipa-4-2: 63d8caf0d105f02decc0b5d865fedf6ad063bc1a



Testing freeipa-4.2.4 build and it fails at 
install/dsinstance.py:add_sidgen_plugin:936


adding self.ldap_connect() on line 937 fixed the issue.
--
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

[Freeipa-devel] [PATCH 0087] Pylint: enable parallelism

2016-03-01 Thread Petr Spacek

Hello,

Pylint: enable parallelism

The config file specifies 8 cores but Pylint very quickly
ends up with 3 cores so do not worry about overwhelming your system.

-- 
Petr^2 Spacek
From cf0994b03016ca25ad1774f2b49266eb9abd5acb Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 1 Mar 2016 15:42:48 +0100
Subject: [PATCH] Pylint: enable parallelism

The config file specifies 8 cores but Pylint very quickly
ends up with 3 cores so do not worry about overwhelming your system.
---
 pylintrc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pylintrc b/pylintrc
index 1003e49217ec9761ea5e17d3e965376a0cca1a32..6717583690d2c05a5fdc61de003b1bc165538c82 100644
--- a/pylintrc
+++ b/pylintrc
@@ -7,7 +7,7 @@ persistent=no
 load-plugins=pylint_plugins
 
 # Use multiple processes to speed up Pylint.
-jobs=1
+jobs=8
 
 [MESSAGES CONTROL]
 
-- 
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 0086] Fix URL for reporting bugs in string

2016-03-01 Thread Tomas Babej



On 03/01/2016 03:23 PM, Petr Spacek wrote:
> Hello,
> 
> Fix URL for reporting bugs in strings.
> 

ACK, good catch.

-- 
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 0086] Fix URL for reporting bugs in string

2016-03-01 Thread Tomas Babej



On 03/01/2016 03:25 PM, Tomas Babej wrote:
> 
> 
> On 03/01/2016 03:23 PM, Petr Spacek wrote:
>> Hello,
>>
>> Fix URL for reporting bugs in strings.
>>
> 
> ACK, good catch.
> 

Pushed to master: e9922c36b15476f99426d0e85fde857887fb5c7d

-- 
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 0086] Fix URL for reporting bugs in string

2016-03-01 Thread Petr Spacek

Hello,

Fix URL for reporting bugs in strings.

-- 
Petr^2 Spacek
From e3eea4e08be25e92ba07fbe647e6d938a7c416f9 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 1 Mar 2016 15:09:01 +0100
Subject: [PATCH] Fix URL for reporting bugs in strings

---
 install/po/Makefile.in | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/install/po/Makefile.in b/install/po/Makefile.in
index 2459ecad130c7532706223a8a0b125809c2928b4..6ab26daccbbc768b9449f1dc3437a8cc033a13fc 100644
--- a/install/po/Makefile.in
+++ b/install/po/Makefile.in
@@ -23,7 +23,8 @@ MSGMERGE_UPDATE = $(MSGMERGE) --update
 
 COPYRIGHT_HOLDER = Red Hat
 PACKAGE_NAME = $(DOMAIN)
-PACKAGE_BUGREPORT = https://hosted.fedoraproject.org/projects/freeipa/newticket
+# An email address or URL where you can report bugs in the untranslated strings
+PACKAGE_BUGREPORT = https://fedorahosted.org/freeipa/newticket
 XGETTEXT_OPTIONS = \
 --add-comments="TRANSLATORS:" \
 --copyright-holder="$(COPYRIGHT_HOLDER)" \
-- 
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] [TEST][Patch 0022] small refactoring in integration tests due to BZ 1303095

2016-03-01 Thread Milan Kubík


On 02/19/2016 02:11 PM, Oleg Fayans wrote:

Hi Milan,

On 02/12/2016 04:03 PM, Milan Kubík wrote:



Agreed. The latest patch gets rid of all resolv.conf related
manipulations. The tests work (where not affected by
https://fedorahosted.org/bind-dyndb-ldap/ticket/160)



--
Milan Kubik


Works for me, tested on sudo test that requires autodiscovery. ACK

--
Milan Kubik

--
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 016, 024, 025] First part of the replica promotion tests + testplan

2016-03-01 Thread Martin Basti





On 01.03.2016 12:37, Martin Basti wrote:



On 01.03.2016 12:32, Martin Basti wrote:



On 29.02.2016 13:16, Oleg Fayans wrote:

Hi all,

Finally the tests pass.

The patch 0024 applies on top of patch 0022 (please, consider reviewing
it also). Besides, the whole functionality depends on Martin's patch 
N 0421


All patches pass pylint.

hello,

I cannot apply patches on master branch
Martin^2

My bad I applied wrong patch




On 12/19/2015 11:56 PM, Martin Basti wrote:


On 17.12.2015 10:04, Oleg Fayans wrote:

Hi Martin,

I am sorry, in my previous email I attached the old version of patch
0016. The correct on is attached.

On 12/16/2015 05:47 PM, Martin Basti wrote:

On 16.12.2015 15:39, Martin Basti wrote:

On 15.12.2015 10:29, Oleg Fayans wrote:

Hi Martin,

The updated patches are attached. Patch 0017 includes all 
changes from

patch 0018, so, if you approve this one, there would be no need to
continue with the review of 0018. This one contains all changes
related
to you remarks from 0018 review. Please see my explanation on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this 
bug:

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

On 12/09/2015 12:17 PM, Martin Basti wrote:

On 09.12.2015 12:10, Martin Basti wrote:

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.

Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI

2016-03-01 Thread Martin Babinsky


On 02/29/2016 05:37 PM, thierry bordaz wrote:

On 02/26/2016 05:48 PM, Martin Babinsky wrote:

On 02/26/2016 04:24 PM, thierry bordaz wrote:

On 02/25/2016 07:17 PM, thierry bordaz wrote:

On 02/25/2016 12:03 PM, Martin Babinsky wrote:

On 02/24/2016 04:30 PM, thierry bordaz wrote:

On 01/21/2016 05:04 PM, Martin Babinsky wrote:

On 01/21/2016 01:37 PM, thierry bordaz wrote:





Hi Thierry,

I have couple of comments to your patch:

1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.

See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.

2.)
+DNA_BIND_METHOD   = "dnaRemoteBindMethod"
+DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+dna_config_base   = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN

Uppercase names are usually reserved for module-level constants.
OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:

"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed
Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""

When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see
function/methods
in replication.py).

3.)

+for i in range(len(entries)) :
+
+mod = []
+if entries[i].single_value.get(DNA_BIND_METHOD) !=
method:
+mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD,
method))
+
+if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))


please use idiomatic Python when handling list of entries, e.g.:

"""
for entry in entries:
   mod = []
   if entry.single_value.get(DNA_BIND_METHOD) != method:
   ...
"""

4.) I think that this method should in DSInstance class since it
deals
with directory server configuration. Service is a parent object
of all
other service installers/configurators and should contain only
methods
common to more children.

5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a
part of
Directory server installation. Is there any reason why this is
not the
case?

6.)

+while attempt != MAX_WAIT:
+try:
+entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+break
+except errors.NotFound:
+root_logger.debug("So far enable not find DNA
shared
config entry for dnaHostname=%s under %s. Retry in 2sec" %
(self.fqdn,
sharedcfgdn))
+attempt = attempt + 1
+time.sleep(2)
+continue
+
+# safety checking
+# there is no return, if there are several entries, as a
workaround of #5510
+if len(entries) != 1:

I am quite afraid what would happen if the server does not return
any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly,
e. g.
raise an exception when a timeout is reached and no entries were
returned.

7.)

+if len(mod) > 0:

A Pythonic way to test for non-empty container is

"""
if mods:
   # do stuff
"""

since an empty list/dict/set evaluates to False and non-empty
containers are True.


8.)

+entry = conn.get_entry(entries[i].dn)
+if entry.single_value.get(DNA_BIND_METHOD) !=
method:
+root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))

rather than re-fetching the modified entry and testing what
happened,
you can just catch an exception raised by unsuccessfull mod and
log an
error like this:

"""
try:
   conn.modify_s(entry.dn, mod)
except Exception as e:
   root_logger.error("Failed to modify entry {}:
{}".format(entry, e))
"""

as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.

9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".


Hi Martin,

Finally tested... here is the updated patch. Thanks for you patience


thanks
thierry


Hi Thierry,

the patch works as expected. I have some more nitpicks though:

1.) Please fix the following pep8 errors:

http://paste.fedoraproject.org/329086/56397841/

you can

[Freeipa-devel] [PATCH 0391-0392] Add missing return value checks to pthread operations & replace strcmp(var, "") with strlen(var) to workaround Clang bug 20144

2016-03-01 Thread Petr Spacek

Hello,

Add missing return value checks to pthread operations.
Detected by clang 3.8 -O2 -Wunused-value.

Replace strcmp(var, "") with strlen(var) to workaround Clang bug 20144.
https://llvm.org/bugs/show_bug.cgi?id=20144

-- 
Petr^2 Spacek
From ccfcc54d15eee928ac9005f902bc5c79da8360e5 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 1 Mar 2016 13:37:59 +0100
Subject: [PATCH] Replace strcmp(var, "") with strlen(var) to workaround Clang
 bug 20144.

https://llvm.org/bugs/show_bug.cgi?id=20144
---
 src/fs.c  | 2 +-
 src/ldap_helper.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/fs.c b/src/fs.c
index 7cbee986ccb00bb1bb63be4d734cde06ca77ce4a..09b71d70ea4f15bc5122df1960933f47f0d44eda 100644
--- a/src/fs.c
+++ b/src/fs.c
@@ -85,7 +85,7 @@ fs_dirs_create(const char *path) {
 	 end != NULL;
 	 end = strchr(end + 1, '/')) {
 		*end = '\0';
-		if (strcmp(curr_path, "") != 0)
+		if (strlen(curr_path) > 0)
 			/* Absolute paths would have first component empty. */
 			CHECK(fs_dir_create(curr_path));
 		*end = '/';
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index 415786d31776d8780f44e75f48674c47a2f61b21..7c50348beef421571e52cf03243e70aa55c0cc88 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -366,7 +366,7 @@ validate_local_instance_settings(ldap_instance_t *inst, settings_set_t *set) {
 	/* Use instance name as default working directory */
 	CHECK(str_new(inst->mctx, ));
 	CHECK(setting_get_str("directory", inst->local_settings, _name));
-	dir_default = (strcmp(dir_name, "") == 0);
+	dir_default = (strlen(dir_name) == 0);
 	if (dir_default == ISC_TRUE) {
 		CHECK(str_cat_char(buff, "dyndb-ldap/"));
 		CHECK(str_cat_char(buff, inst->db_name));
-- 
2.5.0

From e8e0f3d814820972e21a11e2961467f4b770ea20 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 1 Mar 2016 14:32:45 +0100
Subject: [PATCH] Add missing return value checks to pthread operations.

Detected by clang 3.8 -O2 -Wunused-value.
---
 src/semaphore.c|  2 +-
 src/settings.c |  2 +-
 src/syncrepl.c | 13 +++--
 src/zone_manager.c | 10 ++
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/src/semaphore.c b/src/semaphore.c
index 9a502f11e07dc5c4e07549b923cfd05028251493..d86019db42beee6aededfd6a2a01aaa86d7981d9 100644
--- a/src/semaphore.c
+++ b/src/semaphore.c
@@ -46,7 +46,7 @@ semaphore_init(semaphore_t *sem, int value)
 
 	result = isc_condition_init(>cond);
 	if (result != ISC_R_SUCCESS)
-		isc_mutex_destroy(>mutex);
+		DESTROYLOCK(>mutex);
 
 	return result;
 }
diff --git a/src/settings.c b/src/settings.c
index 0f516861f7bef8626cfc90e7e4cabde0ab915cdb..5755f83c4c92125b19af00dd724d9927c4bdc89c 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -532,7 +532,7 @@ settings_set_free(settings_set_t **set) {
 		mctx = (*set)->mctx;
 
 		if ((*set)->lock != NULL) {
-			isc_mutex_destroy((*set)->lock);
+			DESTROYLOCK((*set)->lock);
 			SAFE_MEM_PUT_PTR(mctx, (*set)->lock);
 		}
 
diff --git a/src/syncrepl.c b/src/syncrepl.c
index 94e161997a5eaf19cc71816cfe8fc0fee17d31f4..5ee1eb1aeb75ac4dbece9d429dca117465bef782 100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -120,7 +120,7 @@ finish(isc_task_t *task, isc_event_t *event) {
 	log_debug(1, "sync_barrier_wait(): finish reached");
 	LOCK(>sctx->mutex);
 	sync_state_change(bev->sctx, sync_finished, ISC_FALSE);
-	isc_condition_broadcast(>sctx->cond);
+	BROADCAST(>sctx->cond);
 	UNLOCK(>sctx->mutex);
 	activate_zones(task, inst);
 
@@ -274,9 +274,10 @@ sync_ctx_init(isc_mem_t *mctx, ldap_instance_t *inst, sync_ctx_t **sctxp) {
 
 cleanup:
 	if (lock_ready == ISC_TRUE)
-		isc_mutex_destroy(>mutex);
+		DESTROYLOCK(>mutex);
 	if (cond_ready == ISC_TRUE)
-		isc_condition_init(>cond);
+		RUNTIME_CHECK(isc_condition_destroy(>cond)
+			  == ISC_R_SUCCESS);
 	if (refcount_ready == ISC_TRUE)
 		isc_refcount_destroy(>task_cnt);
 	MEM_PUT_AND_DETACH(sctx);
@@ -308,11 +309,11 @@ sync_ctx_free(sync_ctx_t **sctxp) {
 		isc_refcount_decrement(>task_cnt, NULL);
 		SAFE_MEM_PUT_PTR(sctx->mctx, taskel);
 	}
-	isc_condition_destroy(>cond);
+	RUNTIME_CHECK(isc_condition_destroy(>cond) == ISC_R_SUCCESS);
 	isc_refcount_destroy(>task_cnt);
 	UNLOCK(>mutex);
 
-	isc_mutex_destroy(&(*sctxp)->mutex);
+	DESTROYLOCK(&(*sctxp)->mutex);
 	MEM_PUT_AND_DETACH(*sctxp);
 }
 
@@ -438,7 +439,7 @@ sync_barrier_wait(sync_ctx_t *sctx, const char *inst_name) {
 
 	log_debug(1, "sync_barrier_wait(): wait until all events are processed");
 	while (sctx->state != sync_finished)
-		isc_condition_wait(>cond, >mutex);
+		WAIT(>cond, >mutex);
 	log_debug(1, "sync_barrier_wait(): all events were processed");
 
 cleanup:
diff --git a/src/zone_manager.c b/src/zone_manager.c
index 6f7c57dd05dcbfdffa6b823399bb5b0bd4a33129..93e3fe5675713876d02ce52d036aa2f8a90e288e 100644
--- a/src/zone_manager.c
+++ b/src/zone_manager.c
@@ -58,7 +58,8 @@ destroy_manager(void)
 	db_instance_t *db_inst;
 	db_instance_t *next;
 
-

Re: [Freeipa-devel] [PATCH 0390] Fix build with GCC 4.9+

2016-03-01 Thread Petr Spacek

On 1.3.2016 12:06, Lukas Slebodnik wrote:
> On (25/02/16 15:57), Petr Spacek wrote:
>> On 19.2.2016 13:55, Petr Spacek wrote:
>>> Hello,
>>>
>>> Fix build with GCC 4.9+.
>>>
>>> GCC 4.9+ is too aggressive when optimizing functions with nonnull
>>> attributes. This removes most of asserts() in the plugin.
>>> GCC 6 adds warnings for these cases.
>>>
>>> We are disabling the unwanted condition pruning by adding
>>> -fno-delete-null-pointer-checks argument.
>>> BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.
>>>
>>> Additionally we silence warnings to prevent build failures when -Werror
>>> is used.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1307346
>>
>> Updated version is attached. It contains less autotools magic because it
>> enables attribute nonnull only under Clang static analyzer and Coverity - as 
>> a
>> result we do not have to silence GCC warnings from -Wnonnull.
>>
>> Please review so I can fix build in Fedora 24.
>>
>> Thank you.
>>
>> -- 
>> Petr^2 Spacek
> 
>>From 4732fe9f4e525c44b46e7ed0734ccaec94fba49e Mon Sep 17 00:00:00 2001
>> From: Petr Spacek 
>> Date: Fri, 19 Feb 2016 13:39:27 +0100
>> Subject: [PATCH] Fix build with GCC 4.9+.
>>
>> GCC 4.9+ is too aggressive when optimizing functions with nonnull
>> attributes. This removes most of asserts() in the plugin.
>> GCC 6 adds warnings for these cases.
>>
>> We are disabling the unwanted condition pruning by adding
>> -fno-delete-null-pointer-checks argument.
>> BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.
>>
>> Additionally we enable nonnull attribute only when the build is running under
>> Clang static analyzer or Coverity.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1307346
>> ---
>> configure.ac | 13 +
>> src/util.h   |  8 ++--
>> 2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 
>> a06708b1a5ee64bb64c80272c10ed1a35670c8d0..a0123ac0a62b5acd5238f028d8c42e83af4060db
>>  100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -39,6 +39,19 @@ AC_TRY_COMPILE([
>> [CFLAGS="$SAVED_CFLAGS"
>>  AC_MSG_RESULT([no])])
>>
>> +# Check if build chain supports -fno-delete-null-pointer-checks
>> +# this flag avoids too agressive optimizations which would remove some 
>> asserts
>> +# BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a
>> +AC_MSG_CHECKING([for -fno-delete-null-pointer-checks compiler flag])
>> +SAVED_CFLAGS="$CFLAGS"
>> +CFLAGS="$CFLAGS -fno-delete-null-pointer-checks"
>> +AC_TRY_COMPILE([
>> +extern int fdef(void);
>> +],[],
>> +[AC_MSG_RESULT([yes])],
>> +[CFLAGS="$SAVED_CFLAGS"
>> + AC_MSG_RESULT([no])])
>> +
> NACK.
> 
> It failes with clang.
> 
> configure:12982: checking for -fno-delete-null-pointer-checks compiler flag
> configure:12999: clang -c -O2 -g -pipe -Wall -Werror=format-security 
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong 
> --param=ssp-buffer-size=4 -grecord-gcc-switches 
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic 
> -fvisibility=hidden -fno-delete-null-pointer-checks  conftest.c >&5
> clang-3.8: warning: optimization flag '-fno-delete-null-pointer-checks' is 
> not supported
> clang-3.8: warning: argument unused during compilation: 
> '-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1'
> configure:12999: $? = 0
> configure:13000: result: yes
> 
> Reproducer:
> autoreconf -if && CC=clang ./configure && make

Thanks! I was testing this only with Clang static analyzer ...

Here is updated patch.

-- 
Petr^2 Spacek
From 6b2ac51fe4ff75c9f59499cbaa4306f70db46425 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Fri, 19 Feb 2016 13:39:27 +0100
Subject: [PATCH] Fix build with GCC 4.9+.

GCC 4.9+ is too aggressive when optimizing functions with nonnull
attributes. This removes most of asserts() in the plugin.
GCC 6 adds warnings for these cases.

We are disabling the unwanted condition pruning by adding
-fno-delete-null-pointer-checks argument.
BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.

Additionally we enable nonnull attribute only when the build is running under
Clang static analyzer or Coverity.

https://bugzilla.redhat.com/show_bug.cgi?id=1307346
---
 configure.ac | 14 ++
 src/util.h   |  8 ++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index a06708b1a5ee64bb64c80272c10ed1a35670c8d0..48f5cb63c3bb5535fe1da56abe7583e15d4b5f92 100644
--- a/configure.ac
+++ b/configure.ac
@@ -39,6 +39,20 @@ AC_TRY_COMPILE([
 [CFLAGS="$SAVED_CFLAGS"
  AC_MSG_RESULT([no])])
 
+# Check if build chain supports -fno-delete-null-pointer-checks
+# this flag avoids too agressive optimizations which would remove some asserts
+# BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a
+AC_MSG_CHECKING([for -fno-delete-null-pointer-checks compiler flag])
+SAVED_CFLAGS="$CFLAGS"

Re: [Freeipa-devel] [PATCH] 952 cookie parser: do not fail on cookie with empty value

2016-03-01 Thread Martin Basti




On 01.03.2016 10:32, Petr Vobornik wrote:

Forgot to attach ticket number.

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



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] 0007 Refactor test_sudocmd_plugin

2016-03-01 Thread Tomas Babej



On 03/01/2016 01:27 PM, Aleš Mareček wrote:
> ACK.
> Thank you!
>  - alich -
> 
> - Original Message -
>> From: "Filip Skola" 
>> To: "Aleš Mareček" 
>> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
>> Sent: Wednesday, February 24, 2016 8:07:55 PM
>> Subject: Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
>>
>> Hi,
>>
>> these problems have been fixed.
>>
>> F.
>>
>> - Original Message -
>>> NACK.
>>> Some little changes still required:
>>>  * fixing the pep8 errors
>>>  * fixing the wrong comment
>>>
>>> [root@master2 freeipa]# pep8 ipatests/test_xmlrpc/test_sudocmd_plugin.py
>>> ipatests/test_xmlrpc/test_sudocmd_plugin.py:94:80: E501 line too long (87 >
>>> 79 characters)
>>> ipatests/test_xmlrpc/test_sudocmd_plugin.py:97:80: E501 line too long (87 >
>>> 79 characters)
>>> ipatests/test_xmlrpc/test_sudocmd_plugin.py:134:80: E501 line too long (80

>>> 79 characters)
>>>   
>>> [root@master2 freeipa]# pep8 ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
>>> ipatests/test_xmlrpc/tracker/sudocmd_plugin.py:14:80: E501 line too long
>>> (81
 79 characters)
>>>
>>> [root@master2 freeipa]# grep 'Class for'
>>> ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
>>> """ Class for host plugin like tests """
>>>
>>>
>>> - Original Message -
 From: "Filip Skola" 
 To: "Aleš Mareček" 
 Cc: freeipa-devel@redhat.com, "Milan Kubík" 
 Sent: Monday, February 22, 2016 1:59:43 PM
 Subject: Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin

 Hi,

 sudocmd tracker has been created.

 Filip

 - Original Message -
> NACK.
>
> "create_sudocmd" and "delete_sudocmd" should be placed in Tracker. So
> this
> patch should create the tracker as well.
>
> - Original Message -
>> From: "Filip Skola" 
>> To: freeipa-devel@redhat.com
>> Sent: Monday, January 25, 2016 3:57:25 PM
>> Subject: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
>>
>> Hello,
>>
>> attaching refactored sudocmd_plugin.
>>
>> Filip
>> --
>> 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
>

>>>
>>
> 

Pushed to master: 007c360f85151caab7d608cc0a4eb1916b18eba9

Note: Please keep to down-posting on freeipa-devel.

-- 
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] 0008 Refactor test_sudocmdgroup_plugin, create SudoCmdGroupTracker

2016-03-01 Thread Tomas Babej



On 03/01/2016 01:29 PM, Aleš Mareček wrote:
> ACK.
> Thank you!
> 
> Master push: Make sure it will go *after or together with* the previous patch 
> from Filip, #0007, thanks!
> 
>  - alich -
> 
> - Original Message -
>> From: "Filip Skola" 
>> To: "Aleš Mareček" 
>> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
>> Sent: Wednesday, February 24, 2016 8:13:03 PM
>> Subject: Re: [Freeipa-devel] [PATCH] 0008 Refactor test_sudocmdgroup_plugin, 
>> create SudoCmdGroupTracker
>>
>> Hi,
>>
>> fixed. To be honest, I left that +1char longer lines there on purpose. IMHO
>> it brings better readability and pep8 *.py | wc -l in test_xmlrpc dir
>> returns an overwhelming number anyway. But yeah, some of these weren't
>> really necessary...so I changed them all :)
>>
>> This patch is dependent on 0007-3 patch.
>>
>> Filip
>>
>> - Original Message -
>>> NACK.
>>>
>>>
>>> [root@master2 test_xmlrpc]# pep8 test_sudocmdgroup_plugin.py
>>> test_sudocmdgroup_plugin.py:26:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:70:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:76:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:84:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:90:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:98:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:104:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:166:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:180:80: E501 line too long (80 > 79 characters)
>>> test_sudocmdgroup_plugin.py:186:80: E501 line too long (84 > 79 characters)
>>> [root@master2 test_xmlrpc]# pep8 tracker/sudocmdgroup_plugin.py
>>> tracker/sudocmdgroup_plugin.py:36:80: E501 line too long (82 > 79
>>> characters)
>>> tracker/sudocmdgroup_plugin.py:42:80: E501 line too long (82 > 79
>>> characters)
>>> tracker/sudocmdgroup_plugin.py:46:80: E501 line too long (85 > 79
>>> characters)
>>> tracker/sudocmdgroup_plugin.py:55:80: E501 line too long (82 > 79
>>> characters)
>>> tracker/sudocmdgroup_plugin.py:64:80: E501 line too long (82 > 79
>>> characters)
>>>
>>>
>>>
>>> - Original Message -
 From: "Filip Skola" 
 To: "Aleš Mareček" 
 Cc: freeipa-devel@redhat.com, "Milan Kubík" 
 Sent: Monday, February 22, 2016 3:41:36 PM
 Subject: Re: [Freeipa-devel] [PATCH] 0008 Refactor
 test_sudocmdgroup_plugin, create SudoCmdGroupTracker

 Hi,

 the test has been updated so it now uses the SudoCmdTracker (from the
 previous patch).

 Filip

 - Original Message -
> NACK.
>
> "create_sudocmd" and "delete_sudocmd" should be imported from Tracker,
> not
> from the previous test (sudocmd_plugin).
>
>   - alich -
>
> - Original Message -
>> From: "Filip Skola" 
>> To: freeipa-devel@redhat.com
>> Sent: Thursday, January 28, 2016 12:49:17 PM
>> Subject: [Freeipa-devel] [PATCH] 0008 Refactor
>> test_sudocmdgroup_plugin,
>> create SudoCmdGroupTracker
>>
>> Hi,
>>
>> sending the next sudo patch. This one depends on the previous one
>> (sudocmd_plugin).
>>
>> Filip
>>
>> --
>> 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
>

>>>
>>
> 

Pushed to master: dd38602fa5ea3f0a51db5458e846f3756ab74e47

-- 
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 00136] use LDAPS during standalone CA/KRA subsystem deployment

2016-03-01 Thread Tomas Babej



On 02/26/2016 06:03 PM, Martin Babinsky wrote:
> This patch fixes https://fedorahosted.org/freeipa/ticket/5570 and also
> enables CA installation on CA-less master with hardened dirsrv
> configuration.
> 
> When testing I ran into the issue with Dogtag restart during KRA
> installation [1] which I will try to troubleshoot with Dogtag guys. You
> are welcome to troubleshoot it also during the review, maybe I did some
> misconfiguration on my part.
> 
> [1] https://www.redhat.com/archives/pki-devel/2016-February/msg00100.html
> 
> 

Works fine, ACK!

Pushed to:
master: 276d16775a4ce8af5d39ca8a7bf5bcd638df343f
ipa-4-3: 8de860cc081dd0e5e8b0ae3a97fbb89d6d1386c4
ipa-4-2: c7c126fb51c5b2c92622f493d1c7efbadb899e49

-- 
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] 0007 webui: Add a field for GID in the user add dialog

2016-03-01 Thread Pavel Vomacka


Hi,

The patch adds new field in user add dialog. This combo box lists GIDs 
of posix groups

so user can choose one. It is also possible to fill a GID number
which is not in the list.

Link to the ticket: https://fedorahosted.org/freeipa/ticket/5505

--
Pavel^3 Vomacka
>From 6f6dbaea8a964bd1af50831c482cad4b66ec95c8 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Tue, 1 Mar 2016 12:17:04 +0100
Subject: [PATCH] Add field for group id in user add dialog

Add new field in user add dialog. This combo box lists all posix groups
so user can choose one. It is also possible to fill a GID number
which is not in the list.

https://fedorahosted.org/freeipa/ticket/5505
---
 install/ui/src/freeipa/user.js | 10 ++
 install/ui/test/data/ipa_init.json |  1 +
 ipalib/plugins/internal.py |  1 +
 3 files changed, 12 insertions(+)

diff --git a/install/ui/src/freeipa/user.js b/install/ui/src/freeipa/user.js
index a920e088aacd02585cd131dce725272f47e4cf1c..7583215dbeb9ae7085a4cd6c48648b6705fd141d 100644
--- a/install/ui/src/freeipa/user.js
+++ b/install/ui/src/freeipa/user.js
@@ -447,6 +447,16 @@ return {
 name: 'noprivate',
 label: '@i18n:objects.user.noprivate',
 metadata: '@mc-opt:user_add:noprivate'
+},
+{
+$type: 'entity_select',
+name: 'gidnumber',
+label: '@i18n:objects.user.gidnumber',
+editable: true,
+searchable: true,
+other_entity: 'group',
+other_field: 'gidnumber',
+filter_options: {'posix': true}
 }
 ]
 },
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index b25fa9357d264ef5d82d24205cb6be9ec094bed7..26908a02667b0743aafbeec2f67ce79bca8a85f0 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -608,6 +608,7 @@
 "mode_delete": "delete",
 "mode_preserve": "preserve",
 "noprivate": "No private group",
+"gidnumber": "GID number",
 "status_confirmation": "Are you sure you want to ${action} the user?The change will take effect immediately.",
 "status_link": "Click to ${action}",
 "unlock": "Unlock",
diff --git a/ipalib/plugins/internal.py b/ipalib/plugins/internal.py
index 7156d4f47004dd702d3896ca736cc1f42227a321..b265507c33109f359a6e29610339c1a193b229bd 100644
--- a/ipalib/plugins/internal.py
+++ b/ipalib/plugins/internal.py
@@ -754,6 +754,7 @@ class i18n_messages(Command):
 "mode_delete": _("delete"),
 "mode_preserve": _("preserve"),
 "noprivate": _("No private group"),
+"gidnumber": _("GID number"),
 "status_confirmation": _("Are you sure you want to ${action} the user?The change will take effect immediately."),
 "status_link": _("Click to ${action}"),
 "unlock": _("Unlock"),
-- 
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] 0008 Refactor test_sudocmdgroup_plugin, create SudoCmdGroupTracker

2016-03-01 Thread Aleš Mareček

ACK.
Thank you!

Master push: Make sure it will go *after or together with* the previous patch 
from Filip, #0007, thanks!

 - alich -

- Original Message -
> From: "Filip Skola" 
> To: "Aleš Mareček" 
> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> Sent: Wednesday, February 24, 2016 8:13:03 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0008 Refactor test_sudocmdgroup_plugin, 
> create SudoCmdGroupTracker
> 
> Hi,
> 
> fixed. To be honest, I left that +1char longer lines there on purpose. IMHO
> it brings better readability and pep8 *.py | wc -l in test_xmlrpc dir
> returns an overwhelming number anyway. But yeah, some of these weren't
> really necessary...so I changed them all :)
> 
> This patch is dependent on 0007-3 patch.
> 
> Filip
> 
> - Original Message -
> > NACK.
> > 
> > 
> > [root@master2 test_xmlrpc]# pep8 test_sudocmdgroup_plugin.py
> > test_sudocmdgroup_plugin.py:26:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:70:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:76:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:84:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:90:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:98:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:104:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:166:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:180:80: E501 line too long (80 > 79 characters)
> > test_sudocmdgroup_plugin.py:186:80: E501 line too long (84 > 79 characters)
> > [root@master2 test_xmlrpc]# pep8 tracker/sudocmdgroup_plugin.py
> > tracker/sudocmdgroup_plugin.py:36:80: E501 line too long (82 > 79
> > characters)
> > tracker/sudocmdgroup_plugin.py:42:80: E501 line too long (82 > 79
> > characters)
> > tracker/sudocmdgroup_plugin.py:46:80: E501 line too long (85 > 79
> > characters)
> > tracker/sudocmdgroup_plugin.py:55:80: E501 line too long (82 > 79
> > characters)
> > tracker/sudocmdgroup_plugin.py:64:80: E501 line too long (82 > 79
> > characters)
> > 
> > 
> > 
> > - Original Message -
> > > From: "Filip Skola" 
> > > To: "Aleš Mareček" 
> > > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > > Sent: Monday, February 22, 2016 3:41:36 PM
> > > Subject: Re: [Freeipa-devel] [PATCH] 0008 Refactor
> > > test_sudocmdgroup_plugin, create SudoCmdGroupTracker
> > > 
> > > Hi,
> > > 
> > > the test has been updated so it now uses the SudoCmdTracker (from the
> > > previous patch).
> > > 
> > > Filip
> > > 
> > > - Original Message -
> > > > NACK.
> > > > 
> > > > "create_sudocmd" and "delete_sudocmd" should be imported from Tracker,
> > > > not
> > > > from the previous test (sudocmd_plugin).
> > > > 
> > > >   - alich -
> > > > 
> > > > - Original Message -
> > > > > From: "Filip Skola" 
> > > > > To: freeipa-devel@redhat.com
> > > > > Sent: Thursday, January 28, 2016 12:49:17 PM
> > > > > Subject: [Freeipa-devel] [PATCH] 0008 Refactor
> > > > > test_sudocmdgroup_plugin,
> > > > > create SudoCmdGroupTracker
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > sending the next sudo patch. This one depends on the previous one
> > > > > (sudocmd_plugin).
> > > > > 
> > > > > Filip
> > > > > 
> > > > > --
> > > > > 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

Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin

2016-03-01 Thread Aleš Mareček

ACK.
Thank you!
 - alich -

- Original Message -
> From: "Filip Skola" 
> To: "Aleš Mareček" 
> Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> Sent: Wednesday, February 24, 2016 8:07:55 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> 
> Hi,
> 
> these problems have been fixed.
> 
> F.
> 
> - Original Message -
> > NACK.
> > Some little changes still required:
> >  * fixing the pep8 errors
> >  * fixing the wrong comment
> > 
> > [root@master2 freeipa]# pep8 ipatests/test_xmlrpc/test_sudocmd_plugin.py
> > ipatests/test_xmlrpc/test_sudocmd_plugin.py:94:80: E501 line too long (87 >
> > 79 characters)
> > ipatests/test_xmlrpc/test_sudocmd_plugin.py:97:80: E501 line too long (87 >
> > 79 characters)
> > ipatests/test_xmlrpc/test_sudocmd_plugin.py:134:80: E501 line too long (80
> > >
> > 79 characters)
> >   
> > [root@master2 freeipa]# pep8 ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
> > ipatests/test_xmlrpc/tracker/sudocmd_plugin.py:14:80: E501 line too long
> > (81
> > > 79 characters)
> > 
> > [root@master2 freeipa]# grep 'Class for'
> > ipatests/test_xmlrpc/tracker/sudocmd_plugin.py
> > """ Class for host plugin like tests """
> > 
> > 
> > - Original Message -
> > > From: "Filip Skola" 
> > > To: "Aleš Mareček" 
> > > Cc: freeipa-devel@redhat.com, "Milan Kubík" 
> > > Sent: Monday, February 22, 2016 1:59:43 PM
> > > Subject: Re: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> > > 
> > > Hi,
> > > 
> > > sudocmd tracker has been created.
> > > 
> > > Filip
> > > 
> > > - Original Message -
> > > > NACK.
> > > > 
> > > > "create_sudocmd" and "delete_sudocmd" should be placed in Tracker. So
> > > > this
> > > > patch should create the tracker as well.
> > > > 
> > > > - Original Message -
> > > > > From: "Filip Skola" 
> > > > > To: freeipa-devel@redhat.com
> > > > > Sent: Monday, January 25, 2016 3:57:25 PM
> > > > > Subject: [Freeipa-devel] [PATCH] 0007 Refactor test_sudocmd_plugin
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > attaching refactored sudocmd_plugin.
> > > > > 
> > > > > Filip
> > > > > --
> > > > > 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

Re: [Freeipa-devel] [REVIEW] Intial stab towards Authentication Indicators

2016-03-01 Thread Martin Kosek


On 02/29/2016 11:35 PM, Nathaniel McCallum wrote:

On Fri, 2016-02-26 at 09:00 +0100, Martin Kosek wrote:

On 02/25/2016 10:51 PM, Simo Sorce wrote:


On Thu, 2016-02-25 at 16:13 -0500, Nathaniel McCallum wrote:


On Thu, 2016-02-25 at 12:19 -0500, Nathaniel McCallum wrote:


On Thu, 2016-02-25 at 10:49 -0500, Simo Sorce wrote:



On Thu, 2016-02-25 at 10:32 -0500, Nathaniel McCallum wrote:




On Wed, 2016-02-24 at 09:55 -0500, Nathaniel McCallum
wrote:




On Sun, 2016-02-21 at 20:50 -0500, Simo Sorce wrote:





On Sun, 2016-02-21 at 20:20 -0500, Nathaniel McCallum
wrote:






https://github.com/npmccallum/freeipa/pull/1

The above (pseudo) pull request contains four patches
against
FreeIPA
to enable the insertion of Authentication Indicators
into
Kerberos
tickets. The basic flow looks like this.

First, we patch ipa-pwd-extop to return a control
indicating
what
authentication method succeeded resulting in a
successful
bind.

Second, we patch ipa-otpd to check the returned
control to
ensure
that
the bind resulted from an otp validation.

Third, we patch ipa-kdb to enable the KDC to return
either
the
encrypted timestamp or encrypted challenge preauth
mechanism
when
the
user is configured for optional 2FA logins. Clients
can
then
decide
whether to do 1FA or 2FA login (for kinit, sane
behavior
already
exists).

Forth, we patch ipa-kdb again to insert hard-coded
authentication
indicators for either OTP or RADIUS.

Some explanation is required for the first two
patches.
Currently,
it
is possible to do a 1FA through the otp
preauthentication
mechanism
if
the user is configured for doing optional 2FA.
However,
because
we
want
to insert an authentication indicator in this code
path, we
need
to
guarantee that a request going through the otp
preauth
mechanism
actually validates an OTP. This is the purpose of the
control.

Items still on the TODO list:

   * Authentication Indicator enforcement
 - Upstream libkrb5 needs to grow funcs for
reading
indicators
 - Schema change to add indicators multi-value
attr to
services
 - ipa-kdb needs to implement check_policy_tgs()


   * SSSD needs to learn to handle optional 2FA

I will write up a project page for all of this
tomorrow.
But
this
small
code basically amounts to my brainstorming. It is not
ready
for
merge,
just basic review.


It looks mostly ok, however the LDAP control part needs
to be
done
as
a
request/response pair.
A client that wishes to know what kind of
authentication
happened
should
send a request control, and only in that case , the
server
will
send
the
associated reply control with the requested
information.

I just pushed a new version of the control (now merged
into a
single
patch): https://github.com/npmccallum/freeipa/commit/a781
91ee5d
31
e1de
39
f28eb637f66199da7e9225

In this version the client sends a critical control with
no
content
indicating that the server must validate an OTP. If the
LDAP
server
doesn't support the control (for whatever reason), bind
will
fail. If
the LDAP server doesn't validate an OTP (for whatever
reason),
bind
will fail.

This approach is simpler and doesn't require a
request/response
control
pair.

I need some design advice. My goal here is that we need a
way to
expose
the authentication indicators to services in the FreeIPA
UI/CLI.

Here is the good news: users can already set these values
in
FreeIPA
using kadmin. They do this by simply setting the
require_auth
string on
the target service principal. Our kdb plugin then encodes
these
with
the rest of the tl_data into the krbExtraData attribute.

I see two approaches here. First, we can try to manipulate
the
krbExtraData attribute directly. Second, we can create a
separate
attribute for the authentication indicator strings and then
synthesize
the tl_data internally in kdb. We would have to do this for
both
reads
and writes so as not to break existing kdb functionality.

The trade-off that I see is that the first method
complicates the
python framework side where the second method complicates
the kdb
plugin.

A third option, which I doubt is even possible, is to use
kadmin
to
manipulate this option rather than modifying LDAP directly.

Thoughts?

We should translate it, we need that to allow to delegate
access
only
to
the specific attribute via our standard means.

We already do this for other tl_data entries.

The krbExtraData access cannot always be delegated because it
would
be
open ended. also it is really obnoxious to have to manipulate
ASN.1
stuff in the framework.

kadmin could be used at some point, but we'd still want to
have
this
attribute extracted in order to be able to grant access
control
individually, as our ACL system and delegation system is more
fine
grained than what kadmin can offer.

After discussing this with MIT, Simo and Matt, it seems that
the best
option is to update the (MIT) upstream krbPrincipal objectClass
to
have
a new attribute. The reason for this is twofold. First, it has
upstream
value. Second, we don't have good

Re: [Freeipa-devel] [PATCH] 953 advise: configure TLS in redhat_nss_pam_ldapd and redhat_nss_ldap plugins

2016-03-01 Thread Tomas Babej



On 03/01/2016 10:36 AM, Petr Vobornik wrote:
> On 02/26/2016 03:29 PM, Petr Spacek wrote:
>> On 25.2.2016 18:01, Petr Vobornik wrote:
>>> I did not add --enableldapstarttls to config_redhat_nss_ldap because
>>> I'm not
>>> sure if it is present on el5 (IMO it is not).
>>>
>>> authconfig in:
>>> * config_redhat_nss_ldap got
>>>* --enableldaptls
>>>
>>> * config_redhat_nss_pam_ldapd got
>>>* --enableldaptls
>>>* --enableldapstarttls
>>> options
>>
>> Shouldn't it get only one of them?
>>
>> It seems weird to enable both at the same time.
>>
>> Petr^2 Spacek
>>
>>> https://fedorahosted.org/freeipa/ticket/5654
>>
> 
> Updated patch attached. It uses only --enableldaptls in both commands.
> 
> --enableldapstarttls is an alias for enableldaptls.
> 
> After testing and checking /etc/openldap/ldap.conf, I don't think that
> these options have any effect on el6. There is no 'ssl no' or 'ssl
> start_tls' in any combination or lack of the options. Maybe they have
> effect somewhere else. Anyway it shouldn't do any harm.
> 
> 

ACK.

Pushed to:
master: 02d3ea106214c7e170cb9bf051e4085ade440134
ipa-4-3: b2c5c32d78f099ecc0fb1f10fbf2acd9e36da3ae
ipa-4-2: 6111a30962db4f4bf095201854f3aaa3493adf7c

-- 
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 200] slapi-nis: update configuration to allow external members

2016-03-01 Thread Tomas Babej



On 02/29/2016 01:07 PM, Tomas Babej wrote:
> 
> 
> On 02/29/2016 07:19 AM, Jan Cholasta wrote:
>> On 26.2.2016 21:38, Lukas Slebodnik wrote:
>>> On (26/02/16 12:37), Tomas Babej wrote:


 On 02/26/2016 07:30 AM, Jan Cholasta wrote:
> On 22.2.2016 19:56, Tomas Babej wrote:
>>
>>
>> On 02/22/2016 06:14 PM, Alexander Bokovoy wrote:
>>> On Mon, 22 Feb 2016, Tomas Babej wrote:


 On 02/22/2016 11:48 AM, Alexander Bokovoy wrote:
> Hi,
>
> attached patch should update compat tree configuration if it
> exist to
> follow slapi-nis 0.55 which has support for external members of IPA
> groups.
>
> However, the real work is done in SSSD. These patches are not
> upstreamed
> yet. We'll need to bump SSSD dependency in future once they come to
> distros.
>
>
>

 This looks good.

 However, the new update file needs to be added to Makefile.am.
 Additionally, patch adds a whitespace error.
>>> Updated patch is attached.
>>>
>>
>> ACK.
>>
>> This should not be pushed until the dependency for SSSD can be bumped.
>
> https://bodhi.fedoraproject.org/updates/FEDORA-2016-d872920f74
>

 Attaching the required spec change.

 Tomas
>>>
 From dae8b8fd0b23bf25ccf75b275deaa5c599faa27b Mon Sep 17 00:00:00 2001
 From: Tomas Babej 
 Date: Fri, 26 Feb 2016 12:35:09 +0100
 Subject: [PATCH] spec: Bump required sssd version to 1.13.3-5

 Required as part of: https://fedorahosted.org/freeipa/ticket/4403
>>>   ^
>>> There isn't mentioned sssd related ticket in slapi-nis bug
>>> It would be good to add also sssd related ticket to commit message
>>> https://fedorahosted.org/sssd/ticket/2522
>>
>> +1, that's  in IPA.
>>
> 
> Attaching patch with updated commit message.
> 
> Tomas
> 

Rebased and pushed to:

ipa-4-2: dbea05e1578e2d6d80940f1d4289ecd98a0593ab
ipa-4-3: 5e2c6b0f630300e20c11595e67c61e7eb3982aae
master: 271086ebdd10b2229534220d830d1cbd5af6a352

-- 
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 016 - 017] First part of the replica promotion tests + testplan

2016-03-01 Thread Martin Basti




On 01.03.2016 12:32, Martin Basti wrote:



On 29.02.2016 13:16, Oleg Fayans wrote:

Hi all,

Finally the tests pass.

The patch 0024 applies on top of patch 0022 (please, consider reviewing
it also). Besides, the whole functionality depends on Martin's patch 
N 0421


All patches pass pylint.

hello,

I cannot apply patches on master branch
Martin^2

My bad I applied wrong patch




On 12/19/2015 11:56 PM, Martin Basti wrote:


On 17.12.2015 10:04, Oleg Fayans wrote:

Hi Martin,

I am sorry, in my previous email I attached the old version of patch
0016. The correct on is attached.

On 12/16/2015 05:47 PM, Martin Basti wrote:

On 16.12.2015 15:39, Martin Basti wrote:

On 15.12.2015 10:29, Oleg Fayans wrote:

Hi Martin,

The updated patches are attached. Patch 0017 includes all 
changes from

patch 0018, so, if you approve this one, there would be no need to
continue with the review of 0018. This one contains all changes
related
to you remarks from 0018 review. Please see my explanation on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this bug:
https://fedorahosted.org/freeipa/ticket/5550

On 12/09/2015 12:17 PM, Martin Basti wrote:

On 09.12.2015 12:10, Martin Basti wrote:

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

Re: [Freeipa-devel] [PATCH 016 - 017] First part of the replica promotion tests + testplan

2016-03-01 Thread Martin Basti




On 29.02.2016 13:16, Oleg Fayans wrote:

Hi all,

Finally the tests pass.

The patch 0024 applies on top of patch 0022 (please, consider reviewing
it also). Besides, the whole functionality depends on Martin's patch N 0421

All patches pass pylint.

hello,

I cannot apply patches on master branch
Martin^2



On 12/19/2015 11:56 PM, Martin Basti wrote:


On 17.12.2015 10:04, Oleg Fayans wrote:

Hi Martin,

I am sorry, in my previous email I attached the old version of patch
0016. The correct on is attached.

On 12/16/2015 05:47 PM, Martin Basti wrote:

On 16.12.2015 15:39, Martin Basti wrote:

On 15.12.2015 10:29, Oleg Fayans wrote:

Hi Martin,

The updated patches are attached. Patch 0017 includes all changes from
patch 0018, so, if you approve this one, there would be no need to
continue with the review of 0018. This one contains all changes
related
to you remarks from 0018 review. Please see my explanation on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this bug:
https://fedorahosted.org/freeipa/ticket/5550

On 12/09/2015 12:17 PM, Martin Basti wrote:

On 09.12.2015 12:10, Martin Basti wrote:

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

Re: [Freeipa-devel] Design: Automatic Empty Zone handling in bind-dyndb-ldap

2016-03-01 Thread Martin Basti




On 19.02.2016 09:11, Petr Spacek wrote:

On 12.1.2016 15:10, Martin Basti wrote:


On 12.01.2016 15:06, Petr Spacek wrote:

On 8.1.2016 18:14, Martin Basti wrote:

On 08.01.2016 16:57, Petr Spacek wrote:

Hello,

recent improvements in FreeIPA 4.3.0 (finally) prevent FreeIPA installer from
creating made-up DNS reverse zones, which already exist on some other DNS
server.

This change uncovered a well-hidden automatic empty zones in BIND 9.9+, which
is now causing problem to users.

It seems that this can be fixed by change to the code which handles forward
DNS zones. Short design document with necessary background is available on:
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones


Please be so kind and review it ASAP, so I can write the patch quickly and
make life of our QE guys easier.

Have a nice Friday.


Hello,

IIUC, the differences between default bind behaviour and bind-dyndb-ldap
behaviour are:

* disable automatic empty zone when policy is 'first' or 'only', instead of
just 'only'
I liked it more than default behaviour of named, but could be this somehow
unexpected by users, or they will be happy that it works better (?) than in
named?

I hope users will appreciate it :-)


* bind-dyndb-ldap will not recreate automate empty zone
IMO this should not harm at all

so design LGTM, I will thinking about it over this weekend

Did you find any problem?


Petr^2 Spacek

My mind did not pop out any issue during weekend.
It should work :)

I was discussing this further with BIND upstream and Mark Andrews do not like
it. IMHO we should respect his opinion and do that same what BIND 9.11 is
going to do.

For this reason I've updated design page
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones
with the new approach.

Please review it again. It contains new sections Configuration and Upgrade.

Thank you!


If bind wants to have it in this way, LGTM.

--
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 0390] Fix build with GCC 4.9+

2016-03-01 Thread Lukas Slebodnik

On (25/02/16 15:57), Petr Spacek wrote:
>On 19.2.2016 13:55, Petr Spacek wrote:
>> Hello,
>> 
>> Fix build with GCC 4.9+.
>> 
>> GCC 4.9+ is too aggressive when optimizing functions with nonnull
>> attributes. This removes most of asserts() in the plugin.
>> GCC 6 adds warnings for these cases.
>> 
>> We are disabling the unwanted condition pruning by adding
>> -fno-delete-null-pointer-checks argument.
>> BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.
>> 
>> Additionally we silence warnings to prevent build failures when -Werror
>> is used.
>> 
>> https://bugzilla.redhat.com/show_bug.cgi?id=1307346
>
>Updated version is attached. It contains less autotools magic because it
>enables attribute nonnull only under Clang static analyzer and Coverity - as a
>result we do not have to silence GCC warnings from -Wnonnull.
>
>Please review so I can fix build in Fedora 24.
>
>Thank you.
>
>-- 
>Petr^2 Spacek

>From 4732fe9f4e525c44b46e7ed0734ccaec94fba49e Mon Sep 17 00:00:00 2001
>From: Petr Spacek 
>Date: Fri, 19 Feb 2016 13:39:27 +0100
>Subject: [PATCH] Fix build with GCC 4.9+.
>
>GCC 4.9+ is too aggressive when optimizing functions with nonnull
>attributes. This removes most of asserts() in the plugin.
>GCC 6 adds warnings for these cases.
>
>We are disabling the unwanted condition pruning by adding
>-fno-delete-null-pointer-checks argument.
>BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a.
>
>Additionally we enable nonnull attribute only when the build is running under
>Clang static analyzer or Coverity.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=1307346
>---
> configure.ac | 13 +
> src/util.h   |  8 ++--
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
>diff --git a/configure.ac b/configure.ac
>index 
>a06708b1a5ee64bb64c80272c10ed1a35670c8d0..a0123ac0a62b5acd5238f028d8c42e83af4060db
> 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -39,6 +39,19 @@ AC_TRY_COMPILE([
> [CFLAGS="$SAVED_CFLAGS"
>  AC_MSG_RESULT([no])])
> 
>+# Check if build chain supports -fno-delete-null-pointer-checks
>+# this flag avoids too agressive optimizations which would remove some asserts
>+# BIND 9 did the same in its commit 603a78708343f063b44affb882ef93bb19a5142a
>+AC_MSG_CHECKING([for -fno-delete-null-pointer-checks compiler flag])
>+SAVED_CFLAGS="$CFLAGS"
>+CFLAGS="$CFLAGS -fno-delete-null-pointer-checks"
>+AC_TRY_COMPILE([
>+  extern int fdef(void);
>+],[],
>+[AC_MSG_RESULT([yes])],
>+[CFLAGS="$SAVED_CFLAGS"
>+ AC_MSG_RESULT([no])])
>+
NACK.

It failes with clang.

configure:12982: checking for -fno-delete-null-pointer-checks compiler flag
configure:12999: clang -c -O2 -g -pipe -Wall -Werror=format-security 
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong 
--param=ssp-buffer-size=4 -grecord-gcc-switches 
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic 
-fvisibility=hidden -fno-delete-null-pointer-checks  conftest.c >&5
clang-3.8: warning: optimization flag '-fno-delete-null-pointer-checks' is not 
supported
clang-3.8: warning: argument unused during compilation: 
'-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1'
configure:12999: $? = 0
configure:13000: result: yes

Reproducer:
autoreconf -if && CC=clang ./configure && make

LS

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

Re: [Freeipa-devel] [PATCH] 953 advise: configure TLS in redhat_nss_pam_ldapd and redhat_nss_ldap plugins

2016-03-01 Thread Petr Vobornik


On 02/26/2016 03:29 PM, Petr Spacek wrote:

On 25.2.2016 18:01, Petr Vobornik wrote:

I did not add --enableldapstarttls to config_redhat_nss_ldap because I'm not
sure if it is present on el5 (IMO it is not).

authconfig in:
* config_redhat_nss_ldap got
   * --enableldaptls

* config_redhat_nss_pam_ldapd got
   * --enableldaptls
   * --enableldapstarttls
options


Shouldn't it get only one of them?

It seems weird to enable both at the same time.

Petr^2 Spacek


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




Updated patch attached. It uses only --enableldaptls in both commands.

--enableldapstarttls is an alias for enableldaptls.

After testing and checking /etc/openldap/ldap.conf, I don't think that 
these options have any effect on el6. There is no 'ssl no' or 'ssl 
start_tls' in any combination or lack of the options. Maybe they have 
effect somewhere else. Anyway it shouldn't do any harm.

--
Petr Vobornik
From 8fdc69cb741ef67e8a1901ba5b8f1899ba8f3668 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Thu, 25 Feb 2016 15:25:12 +0100
Subject: [PATCH] advise: configure TLS in redhat_nss_pam_ldapd and
 redhat_nss_ldap plugins

authconfig in config_redhat_nss_ldap and config_redhat_nss_pam_ldapd got
new option --enableldaptls

It should have effect primarily on el5 systems.

https://fedorahosted.org/freeipa/ticket/5654
---
 ipaserver/advise/plugins/legacy_clients.py | 4 ++--
 ipatests/test_integration/test_advise.py   | 7 ---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/ipaserver/advise/plugins/legacy_clients.py b/ipaserver/advise/plugins/legacy_clients.py
index b6e1fc5a1549787fbe2805b0297d79211ae21d77..9e381f12a4f60e970a08de34b79a6ad5028de449 100644
--- a/ipaserver/advise/plugins/legacy_clients.py
+++ b/ipaserver/advise/plugins/legacy_clients.py
@@ -195,7 +195,7 @@ class config_redhat_nss_pam_ldapd(config_base_legacy_client):
 
 self.log.comment('Use the authconfig to configure nsswitch.conf '
  'and the PAM stack')
-self.log.command('authconfig --updateall --enableldap '
+self.log.command('authconfig --updateall --enableldap --enableldaptls '
  '--enableldapauth --ldapserver=%s --ldapbasedn=%s\n'
  % (uri, base))
 
@@ -363,7 +363,7 @@ class config_redhat_nss_ldap(config_base_legacy_client):
 
 self.log.comment('Use the authconfig to configure nsswitch.conf '
  'and the PAM stack')
-self.log.command('authconfig --updateall --enableldap '
+self.log.command('authconfig --updateall --enableldap --enableldaptls '
  '--enableldapauth --ldapserver=%s --ldapbasedn=%s\n'
  % (uri, base))
 
diff --git a/ipatests/test_integration/test_advise.py b/ipatests/test_integration/test_advise.py
index 613096f1caed3efb7db33076da5e57bea58cfa13..82d6d84cfd6e987dcbe5b8a9214a1c5eeeadf052 100644
--- a/ipatests/test_integration/test_advise.py
+++ b/ipatests/test_integration/test_advise.py
@@ -104,7 +104,8 @@ class TestAdvice(IntegrationTest):
 advice_regex = "\#\!\/bin\/sh.*" \
"yum[\s]+install[\s]+\-y[\s]+curl[\s]+openssl[\s]+nss_ldap" \
"[\s]+authconfig.*authconfig[\s]+\-\-updateall" \
-   "[\s]+\-\-enableldap[\s]+\-\-enableldapauth[\s]+" \
+   "[\s]+\-\-enableldap[\s]+\-\-enableldaptls"\
+   "[\s]+\-\-enableldapauth[\s]+" \
"\-\-ldapserver=.*[\s]+\-\-ldapbasedn=.*"
 raiseerr = True
 
@@ -116,8 +117,8 @@ class TestAdvice(IntegrationTest):
 advice_regex = "\#\!\/bin\/sh.*" \
"yum[\s]+install[\s]+\-y[\s]+curl[\s]+openssl[\s]+" \
"nss\-pam\-ldapd[\s]+pam_ldap[\s]+authconfig.*" \
-   "authconfig[\s]+\-\-updateall[\s]+" \
-   "\-\-enableldap[\s]+\-\-enableldapauth[\s]+" \
+   "authconfig[\s]+\-\-updateall[\s]+\-\-enableldap"\
+   "[\s]+\-\-enableldaptls[\s]+\-\-enableldapauth[\s]+" \
"\-\-ldapserver=.*[\s]+\-\-ldapbasedn=.*"
 raiseerr = True
 
-- 
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] 952 cookie parser: do not fail on cookie with empty value

2016-03-01 Thread Petr Vobornik


Forgot to attach ticket number.

https://fedorahosted.org/freeipa/ticket/5709
--
Petr Vobornik
From fc053d348a9414716af7794baa4300209548a4e3 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Tue, 2 Feb 2016 17:04:47 +0100
Subject: [PATCH] cookie parser: do not fail on cookie with empty value

https://fedorahosted.org/freeipa/ticket/5709
---
 ipapython/cookie.py|  3 ++-
 ipatests/test_ipapython/test_cookie.py | 16 
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/ipapython/cookie.py b/ipapython/cookie.py
index b44522157ffa6981b5415fb4db7717742c1f5717..89c3e3cd6c430b636cd1a4cae40b3671bec9e44c 100644
--- a/ipapython/cookie.py
+++ b/ipapython/cookie.py
@@ -272,8 +272,9 @@ class Cookie(object):
 if match:
 key = match.group(1)
 value = match.group(2)
+
 # Double quoted value?
-if value[0] == '"':
+if value and value[0] == '"':
 if value[-1] == '"':
 value = value[1:-1]
 else:
diff --git a/ipatests/test_ipapython/test_cookie.py b/ipatests/test_ipapython/test_cookie.py
index 97cb79a07b067e18f9195c14703e537595f2fb9d..6af447984d7a9a00b3c5636698d43b7cf1221486 100644
--- a/ipatests/test_ipapython/test_cookie.py
+++ b/ipatests/test_ipapython/test_cookie.py
@@ -50,6 +50,22 @@ class TestParse(unittest.TestCase):
 with self.assertRaises(ValueError):
 cookies = Cookie.parse(s)
 
+# 1 cookie with empty value
+s = 'color='
+cookies = Cookie.parse(s)
+self.assertEqual(len(cookies), 1)
+cookie = cookies[0]
+self.assertEqual(cookie.key, 'color')
+self.assertEqual(cookie.value, '')
+self.assertEqual(cookie.domain, None)
+self.assertEqual(cookie.path, None)
+self.assertEqual(cookie.max_age, None)
+self.assertEqual(cookie.expires, None)
+self.assertEqual(cookie.secure, None)
+self.assertEqual(cookie.httponly, None)
+self.assertEqual(str(cookie), "color=")
+self.assertEqual(cookie.http_cookie(), "color=;")
+
 # 1 cookie with name/value
 s = 'color=blue'
 cookies = Cookie.parse(s)
-- 
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

[Freeipa-devel] [PATCH] 952 cookie parser: do not fail on cookie with empty value

2016-03-01 Thread Petr Vobornik



--
Petr Vobornik
From 39735d51bcf5a35462e7c6b6c67cca02c6ea42ae Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Tue, 2 Feb 2016 17:04:47 +0100
Subject: [PATCH] cookie parser: do not fail on cookie with empty value

---
 ipapython/cookie.py|  3 ++-
 ipatests/test_ipapython/test_cookie.py | 16 
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/ipapython/cookie.py b/ipapython/cookie.py
index b44522157ffa6981b5415fb4db7717742c1f5717..89c3e3cd6c430b636cd1a4cae40b3671bec9e44c 100644
--- a/ipapython/cookie.py
+++ b/ipapython/cookie.py
@@ -272,8 +272,9 @@ class Cookie(object):
 if match:
 key = match.group(1)
 value = match.group(2)
+
 # Double quoted value?
-if value[0] == '"':
+if value and value[0] == '"':
 if value[-1] == '"':
 value = value[1:-1]
 else:
diff --git a/ipatests/test_ipapython/test_cookie.py b/ipatests/test_ipapython/test_cookie.py
index 97cb79a07b067e18f9195c14703e537595f2fb9d..6af447984d7a9a00b3c5636698d43b7cf1221486 100644
--- a/ipatests/test_ipapython/test_cookie.py
+++ b/ipatests/test_ipapython/test_cookie.py
@@ -50,6 +50,22 @@ class TestParse(unittest.TestCase):
 with self.assertRaises(ValueError):
 cookies = Cookie.parse(s)
 
+# 1 cookie with empty value
+s = 'color='
+cookies = Cookie.parse(s)
+self.assertEqual(len(cookies), 1)
+cookie = cookies[0]
+self.assertEqual(cookie.key, 'color')
+self.assertEqual(cookie.value, '')
+self.assertEqual(cookie.domain, None)
+self.assertEqual(cookie.path, None)
+self.assertEqual(cookie.max_age, None)
+self.assertEqual(cookie.expires, None)
+self.assertEqual(cookie.secure, None)
+self.assertEqual(cookie.httponly, None)
+self.assertEqual(str(cookie), "color=")
+self.assertEqual(cookie.http_cookie(), "color=;")
+
 # 1 cookie with name/value
 s = 'color=blue'
 cookies = Cookie.parse(s)
-- 
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

37 matches

Mail list logo