from:"Stanislav Laznicka"


On 08/17/2016 03:50 PM, Pavel Vomacka wrote:




On 08/17/2016 02:42 PM, Pavel Vomacka wrote:



On 08/11/2016 07:49 PM, Petr Vobornik wrote:

On 08/11/2016 07:21 PM, Martin Basti wrote:


On 11.08.2016 18:57, Pavel Vomacka wrote:


On 08/11/2016 02:00 PM, Petr Vobornik wrote:

On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:

On Thu, 11 Aug 2016, Jan Cholasta wrote:

On 4.8.2016 17:27, Jan Pazdziora wrote:
On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander Bokovoy 
wrote:

Got it. One thing I would correct, though, -- don't use
kadmin.local, we
do support setting ok_as_delegate on the service principals 
via IPA

CLI:
$ ipa service-mod --help |grep -A1 ok-as-delegate
--ok-as-delegate=BOOL
Client credentials may be delegated 
to the

service

I've tried

  ipa service-mod --ok-as-delegate=True HTTP/$(hostname)

but that does not seem to have the same effect as

  modprinc +ok_to_auth_as_delegate HTTP/ipa.example.test

-- obtaining the delegated certificated fails.
That's because ok_as_delegate and ok_to_auth_as_delegate are 
different

flags.
Right. The following patch adds ok_to_auth_as_delegate to the 
service

principal.

I haven't added any tickets to it yet.



This might deserve also nice Web UI checkbox similar to "Trusted for
delegation". CCing Pavel.

Here is patch with new checkbox. It is without ticket in commit 
message so
once we will have the ticket I will send another patch witch 
updated commit

message.

https://fedorahosted.org/freeipa/newticket

;-)
It's prerequisite for https://fedorahosted.org/freeipa/ticket/5764 
so we

might use that.

Thank you, patch with updated commit message attached.




Attached patch adds checkbox also to host page.


Thank you, works as expected. 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] 0001 Added new authentication method


On 08/17/2016 03:58 PM, Alexander Bokovoy wrote:

On Thu, 11 Aug 2016, Petr Vobornik wrote:

On 08/11/2016 07:21 PM, Martin Basti wrote:



On 11.08.2016 18:57, Pavel Vomacka wrote:



On 08/11/2016 02:00 PM, Petr Vobornik wrote:

On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:

On Thu, 11 Aug 2016, Jan Cholasta wrote:

On 4.8.2016 17:27, Jan Pazdziora wrote:

On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander Bokovoy wrote:

Got it. One thing I would correct, though, -- don't use
kadmin.local, we
do support setting ok_as_delegate on the service principals 
via IPA

CLI:
$ ipa service-mod --help |grep -A1 ok-as-delegate
--ok-as-delegate=BOOL
   Client credentials may be delegated to the
service

I've tried

 ipa service-mod --ok-as-delegate=True HTTP/$(hostname)

but that does not seem to have the same effect as

 modprinc +ok_to_auth_as_delegate HTTP/ipa.example.test

-- obtaining the delegated certificated fails.
That's because ok_as_delegate and ok_to_auth_as_delegate are 
different

flags.
Right. The following patch adds ok_to_auth_as_delegate to the 
service

principal.

I haven't added any tickets to it yet.



This might deserve also nice Web UI checkbox similar to "Trusted for
delegation". CCing Pavel.

Here is patch with new checkbox. It is without ticket in commit 
message so
once we will have the ticket I will send another patch witch 
updated commit

message.


https://fedorahosted.org/freeipa/newticket

;-)


It's prerequisite for https://fedorahosted.org/freeipa/ticket/5764 so we
might use that.

Sounds good. Patch with updated commit message is attached.



Thank you for the updated patch, works as expected so 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] 0001 Added new authentication method

On 08/17/2016 04:11 PM, Tibor Dudlak wrote:

On Wed, Aug 17, 2016 at 3:36 PM, Stanislav Laznicka 
<slazn...@redhat.com <mailto:slazn...@redhat.com>> wrote:

On 08/16/2016 03:16 PM, Tibor Dudlak wrote:

Hi,

I have edited this patch after review. It should be okay now.

Thank you.

On Thu, Aug 11, 2016 at 7:49 PM, Petr Vobornik
<pvobo...@redhat.com <mailto:pvobo...@redhat.com>> wrote:

On 08/11/2016 07:21 PM, Martin Basti wrote:
>
>
> On 11.08.2016 18:57, Pavel Vomacka wrote:
>>
>>
>> On 08/11/2016 02:00 PM, Petr Vobornik wrote:
>>> On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:
>>>> On Thu, 11 Aug 2016, Jan Cholasta wrote:
>>>>> On 4.8.2016 17:27, Jan Pazdziora wrote:
>>>>>> On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander
Bokovoy wrote:
>>>>>>> Got it. One thing I would correct, though, -- don't use
>>>>>>> kadmin.local, we
>>>>>>> do support setting ok_as_delegate on the service
principals via IPA
>>>>>>> CLI:
>>>>>>> $ ipa service-mod --help |grep -A1 ok-as-delegate
>>>>>>> --ok-as-delegate=BOOL
>>>>>>> Client credentials may be delegated to the
>>>>>>> service
>>>>>> I've tried
>>>>>>
>>>>>>  ipa service-mod --ok-as-delegate=True
HTTP/$(hostname)
>>>>>>
>>>>>> but that does not seem to have the same effect as
>>>>>>
>>>>>>  modprinc +ok_to_auth_as_delegate
HTTP/ipa.example.test
>>>>>>
>>>>>> -- obtaining the delegated certificated fails.
>>>>> That's because ok_as_delegate and
ok_to_auth_as_delegate are different
>>>>> flags.
>>>> Right. The following patch adds ok_to_auth_as_delegate
to the service
>>>> principal.
>>>>
>>>> I haven't added any tickets to it yet.
>>>>
>>>>
>>> This might deserve also nice Web UI checkbox similar to
"Trusted for
>>> delegation". CCing Pavel.
>>>
>> Here is patch with new checkbox. It is without ticket in
commit message so
>> once we will have the ticket I will send another patch
witch updated commit
>> message.
>
> https://fedorahosted.org/freeipa/newticket
<https://fedorahosted.org/freeipa/newticket>
>
> ;-)

It's prerequisite for
https://fedorahosted.org/freeipa/ticket/5764
<https://fedorahosted.org/freeipa/ticket/5764> so we
might use that.

Please, add your answers at the end of the previous mail in the
future.

Also, your patch raises pep8 errors:
./ipaserver/plugins/xmlserver.py:31:80: E501 line too long (189 >
79 characters)
./ipaserver/rpcserver.py:885:5: E113 unexpected indentation

Could you please fix them?

Hi,

thanks for review Stanislav. I understand 
./ipaserver/rpcserver.py:885:5: E113 unexpected indentation, that is 
my fault but really do not understand first one. Is there policy that 
you decided not to patch existing files, even if there was obviously 
longer line before patch until it is not necessary?

Anyway I hope it should be ok now.

Thank you.

There's a policy to try to be pep8 compliant as much as we can with any 
new patches. Your new patch would still raise some pep8 errors, please 
see the attached patch that should be ok. If it's ok with you then ACK, 
it seems to be working.

From e8f7cffe8fa24d2e02285ab2907e95463aad4311 Mon Sep 17 00:00:00 2001
From: Tiboris <tibor.dud...@gmail.com>
Date: Tue, 16 Aug 2016 14:13:29 +0200
Subject: [PATCH] Added new authentication method

Addressing ticket https://fedorahosted.org/freeipa/ticket/5764
---
 ipaserver/plugins/xmlserver.py |  6 +-
 ipaserver/rpcserver.py | 17 +
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/ipaserver/plugins/xmlserver.py b/ipaserver/plugins/xmlserver.py
index d8fe24e0cb407603e9898e934229c9373f3c8b62..08c7456ed6dbfcc59f532314894031fba584e20a 100644
--- a/ipaserver/plugins/xmlserver.py
+++ b/ipaserver/plugins/xmlserver.py
@@ -28,12 +28,16 @@ register = Registry()

 if api.env.context in ('server', 'lite'):
-from ipaserver.rpcserver import wsgi_dispatch, x

Re: [Freeipa-devel] [PATCH] 0001 Added new authentication method


On 08/16/2016 03:16 PM, Tibor Dudlak wrote:

Hi,

I have edited this patch after review. It should be okay now.

Thank you.

On Thu, Aug 11, 2016 at 7:49 PM, Petr Vobornik > wrote:


On 08/11/2016 07:21 PM, Martin Basti wrote:
>
>
> On 11.08.2016 18:57, Pavel Vomacka wrote:
>>
>>
>> On 08/11/2016 02:00 PM, Petr Vobornik wrote:
>>> On 08/11/2016 10:54 AM, Alexander Bokovoy wrote:
 On Thu, 11 Aug 2016, Jan Cholasta wrote:
> On 4.8.2016 17:27, Jan Pazdziora wrote:
>> On Wed, Aug 03, 2016 at 10:29:52AM +0300, Alexander Bokovoy
wrote:
>>> Got it. One thing I would correct, though, -- don't use
>>> kadmin.local, we
>>> do support setting ok_as_delegate on the service
principals via IPA
>>> CLI:
>>> $ ipa service-mod --help |grep -A1 ok-as-delegate
>>> --ok-as-delegate=BOOL
>>> Client credentials may be delegated to the
>>> service
>> I've tried
>>
>>  ipa service-mod --ok-as-delegate=True HTTP/$(hostname)
>>
>> but that does not seem to have the same effect as
>>
>>  modprinc +ok_to_auth_as_delegate HTTP/ipa.example.test
>>
>> -- obtaining the delegated certificated fails.
> That's because ok_as_delegate and ok_to_auth_as_delegate are
different
> flags.
 Right. The following patch adds ok_to_auth_as_delegate to the
service
 principal.

 I haven't added any tickets to it yet.


>>> This might deserve also nice Web UI checkbox similar to
"Trusted for
>>> delegation". CCing Pavel.
>>>
>> Here is patch with new checkbox. It is without ticket in commit
message so
>> once we will have the ticket I will send another patch witch
updated commit
>> message.
>
> https://fedorahosted.org/freeipa/newticket

>
> ;-)

It's prerequisite for https://fedorahosted.org/freeipa/ticket/5764
 so we
might use that.



Please, add your answers at the end of the previous mail in the future.

Also, your patch raises pep8 errors:
./ipaserver/plugins/xmlserver.py:31:80: E501 line too long (189 > 79 
characters)

./ipaserver/rpcserver.py:885:5: E113 unexpected indentation

Could you please fix them?
-- 
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 0063] Raise error on topology disconnect/last-role-host removal during server uninstall


On 08/17/2016 02:17 PM, Martin Babinsky wrote:

On 08/16/2016 03:47 PM, Stanislav Laznicka wrote:

On 08/15/2016 02:20 PM, Martin Babinsky wrote:

On 08/15/2016 02:13 PM, Martin Babinsky wrote:

On 08/12/2016 12:08 PM, Stanislav Laznicka wrote:

Hello,

topology disconnect/last-role-host removal errors would just be 
logged

during server uninstall even if ignore options are not present. The
host
would still appear in the topology even after "successful" uninstall.

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





The patch seems to be ok, however shouldn't we use sys.exit() when
handling ServerRemovalError? Yes raising SystemExit from within a
function is a horrible practice, but it is already done on several 
other

places instead of letting the exception bubble up to the main handler.

CC'ing Jan for his thoughts on this since I may be wrong.


Hmm, you will definitely need sys.exit() here since otherwise
ipa-server-install reports 0 exit code even if there was an exception
thrown:

"""
[root@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server
will leave your installation without a DNS..
[root@master1 ~]# echo $?
0
"""


Because of #5750 (remove sys.exit() calls from installer modules) I
rather raise ScriptError in this case. See the modified patch. It
depends on either of my 48-4 or 48-5 patches (pick one).



Make sure you raise it using str(e) because ScriptError does not 
coerce the argument to string/unicode:


@@ -303,7 +303,7 @@ def 
remove_master_from_managed_topology(api_instance, options):

 replication.run_server_del_as_cli(
 api_instance, api_instance.env.host, **server_del_options)
 except errors.ServerRemovalError as e:
-raise ScriptError(e)
+raise ScriptError(str(e))
 except Exception as e:
 # if the master was already deleted we will just get a warning
 root_logger.warning("Failed to delete master: {}".format(e))


Oops, sorry, attached is the fixed patch.
Regardless of this fix, I still get exit code 0 after exception is 
raised:


"""
[root@client1 ~]# ipa-server-install --uninstall -U
This server is active DNSSEC key master. Uninstall could break your 
DNS system.
ipa : ERRORServer removal aborted: Replica is active 
DNSSEC key master. Uninstall could break your DNS system. Please 
disable or replace DNSSEC key master first..

[root@client1 ~]# echo $?
0
"""

I guess there is nothing we can do about this now as the fix for this 
seems to be beyond the scope of this patch ( or am I mistaken?).


Dandy. The actual relevant ticket to zero return value no matter what is 
already there - https://fedorahosted.org/freeipa/ticket/5725.


From e91f2018016f4f2e3acac492bea2199f699fe6e7 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 12 Aug 2016 11:59:41 +0200
Subject: [PATCH] Fail on topology disconnect/last role removal

Disconnecting topology/removing last-role-host during server
uninstallation should raise error rather than just being logged
if the appropriate ignore settings are not present.

https://fedorahosted.org/freeipa/ticket/6168
---
 ipaserver/install/server/install.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 8dc7a6820d214f15fb80fef29c7296fa237bc2a9..48e0b9b7d6ae460c02db3c50b4ce8d401079bd41 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -294,7 +294,6 @@ def common_cleanup(func):
 def remove_master_from_managed_topology(api_instance, options):
 try:
 # we may force the removal
-# if the master was already deleted we will just get a warning
 server_del_options = dict(
 force=True,
 ignore_topology_disconnect=options.ignore_topology_disconnect,
@@ -303,8 +302,10 @@ def remove_master_from_managed_topology(api_instance, options):
 
 replication.run_server_del_as_cli(
 api_instance, api_instance.env.host, **server_del_options)
-
+except errors.ServerRemovalError as e:
+raise ScriptError(str(e))
 except Exception as e:
+# if the master was already deleted we will just get a warning
 root_logger.warning("Failed to delete master: {}".format(e))
 
 
-- 
2.7.4

-- 
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 0058] Make get_entries not ignore its size_limit argument


On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:

On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question 
in my
head whether ipaldap.get_entries is used properly throughout the 
system
- does it always assume that it gets ALL the requested entries or 
just a
few of those as configured by the 'ipaSearchRecordsLimit' 
attribute of

ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin (which is
usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get 
everything

anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit 
is not

set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause problems
should ipaSearchRecordsLimit be set to a low value as in the ticket.


I see. This is *not* intentional, the **kwargs of get_entries() 
should be passed to find_entries(). This definitely needs to be fixed.




Anyway, this ticket is not really easily fixable without more profound
changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you pass the
same size limit to all the searches? Do you subtract the result size
from the size limit after each search? Do you do something else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we have to
do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is not
perfect, it would probably be better to subtract the number of 
currently

loaded records from the sizelimit, although in the end the number of
returned values will not be higher than the given size_limit. However,
it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than ignoring
it. Then, any use of get_entries could be fixed accordingly if someone
sees fit.



Right. Anyway, this is a different issue than above, so please put 
this into a separate commit.



Please see the attached patches, then.

Self-NACK, with Honza's help I found there was a mistake in the code. I 
also found an off-by-one bug which I hope I could stick to the other two 
patches (attaching only the modified and new patches).
From a0b900321fd0bcb9d93f9e558f48af6854e853df Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 11 Aug 2016 14:09:22 +0200
Subject: [PATCH 1/2] fix permission_find fail on low search size limit

permission_find() method would have failed if size_limit in config is too
small caused by a search in post_callback. This search should also
respect the passed sizelimit or the sizelimit from ipa config if no
sizelimit is passed.

https://fedorahosted.org/freeipa/ticket/5640
---
 ipaserver/plugins/permission.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py
index 830773ae7a09f0197da702e4ec31b0b58f1214dd..f11f32fe75abe79afd975f20a17d7074d7fdaf76 100644
--- a/ipaserver/plugins/permission.py
+++ b/ipaserver/plugins/permission.py
@@ -1305,10 +1305,10 @@ class permission_find(baseldap.LDAPSearch):
 if options.get('all'):
 attrs_list.append('*')
 try:
-legacy_entries = ldap.get_entries(
+(legacy_entries, truncated) = ldap.find_entries(
 base_dn=DN(self.obj.container_dn, self.api.env.basedn),
 filter=ldap.combine_filters(filters, rules=ldap.MATCH_ALL),
-attrs_list=attrs_list)
+attrs_list=attrs_list, size_limit=max_entries)
 # Retrieve th

Re: [Freeipa-devel] [PATCH 0063] Raise error on topology disconnect/last-role-host removal during server uninstall


On 08/15/2016 02:20 PM, Martin Babinsky wrote:

On 08/15/2016 02:13 PM, Martin Babinsky wrote:

On 08/12/2016 12:08 PM, Stanislav Laznicka wrote:

Hello,

topology disconnect/last-role-host removal errors would just be logged
during server uninstall even if ignore options are not present. The 
host

would still appear in the topology even after "successful" uninstall.

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





The patch seems to be ok, however shouldn't we use sys.exit() when
handling ServerRemovalError? Yes raising SystemExit from within a
function is a horrible practice, but it is already done on several other
places instead of letting the exception bubble up to the main handler.

CC'ing Jan for his thoughts on this since I may be wrong.

Hmm, you will definitely need sys.exit() here since otherwise 
ipa-server-install reports 0 exit code even if there was an exception 
thrown:


"""
[root@master1 ~]# ipa-server-install --uninstall -U
ipa : ERRORServer removal aborted: Deleting this server 
will leave your installation without a DNS..

[root@master1 ~]# echo $?
0
"""

Because of #5750 (remove sys.exit() calls from installer modules) I 
rather raise ScriptError in this case. See the modified patch. It 
depends on either of my 48-4 or 48-5 patches (pick one).


From 6f7683fe0b9da6640a740bedf113bb3a9dc99bd1 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 12 Aug 2016 11:59:41 +0200
Subject: [PATCH] Fail on topology disconnect/last role removal

Disconnecting topology/removing last-role-host during server
uninstallation should raise error rather than just being logged
if the appropriate ignore settings are not present.

https://fedorahosted.org/freeipa/ticket/6168
---
 ipaserver/install/server/install.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 8dc7a6820d214f15fb80fef29c7296fa237bc2a9..7a21126ac017dfad89cda9774fbec31c8818f745 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -294,7 +294,6 @@ def common_cleanup(func):
 def remove_master_from_managed_topology(api_instance, options):
 try:
 # we may force the removal
-# if the master was already deleted we will just get a warning
 server_del_options = dict(
 force=True,
 ignore_topology_disconnect=options.ignore_topology_disconnect,
@@ -303,8 +302,10 @@ def remove_master_from_managed_topology(api_instance, options):
 
 replication.run_server_del_as_cli(
 api_instance, api_instance.env.host, **server_del_options)
-
+except errors.ServerRemovalError as e:
+raise ScriptError(e)
 except Exception as e:
+# if the master was already deleted we will just get a warning
 root_logger.warning("Failed to delete master: {}".format(e))
 
 
-- 
2.7.4

-- 
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 0048] Remove sys.exit() from installer modules


On 08/16/2016 01:03 PM, Martin Basti wrote:



On 16.08.2016 12:06, Stanislav Laznicka wrote:

On 08/16/2016 08:36 AM, Stanislav Laznicka wrote:

On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
I chose ScriptError because it's able to change the return value of 
the script, which is necessary sometimes. RuntimeError, which may 
seem more suitable, would not be able to do that. However, I am 
open for ideas on which exception type to use.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
I really hate to print errors to STDOUT from modules and then just 
call exit(1) (duplicated evil), could you replace print('xxx') 
with raise AnException('xxx')
Did that, only kept those prints printing directly to stderr. Not 
sure if those should be changed as well.



Bumping for review/opinions.

Also a self-NACK, there were some sys imports that should not have 
been there anymore (thanks, mbasti). Attaching a fixed version.




You use RuntimeError in bindinstance.py, in other files you use 
ScriptError, is this RuntimeError on purpose there?


Martin^2


I used it there as there was one other function in the module that would 
raise it, too. Adding a patch that raises ScriptError in 
bindinstance.check_reverse_zones() as well if you think raising 
ScriptError here would be more appropriate.


From 844b22add517477df49162a73c70b0358e08d0eb Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |   3 +-
 ipaserver/install/ca.py|  42 +-
 ipaserver/install/cainstance.py|   5 +-
 ipaserver/install/dns.py   |   5 +-
 ipaserver/install/dsinstance.py|   4 +-
 ipaserver/install/installutils.py  |  20 ++---
 ipaserver/install/ipa_ldap_updater.py  |   4 +-
 ipaserver/install/krainstance.py   |   4 +-
 ipaserver/install/replication.py   |  10 ++-
 ipaserver/install/server/install.py|  75 -
 ipaserver/install/server/replicainstall.py | 129 ++---
 11 files changed, 149 insertions(+), 152 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 7538e145cbe37dfc21963d97dea0e835e3bd5072..5c3bdac1a48d9d7e149babf0193d57b522aa9988 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -44,6 +44,7 @@ from ipapython import dnsutil
 from ipapython.dnsutil import DNSName
 from ipapython.ipa_log_manager import root_logger
 from ipapython.dn import DN
+from ipapython.admintool import ScriptError
 import ipalib
 from ipalib import api, errors
 from ipalib.constants import IPA_CA_RECORD
@@ -473,7 +474,7 @@ def check_reverse_zones(ip_addresses, reverse_zones, options, unattended,
 except ValueError as e:
 msg = "Reverse zone %s will not be used: %s" % (rz, e)
 if unattended:
-sys.exit(msg)
+raise ScriptError(msg)
 else:
 root_logger.warning(msg)
 continue
diff --git a/ipaserver/install/ca.py b/ipaserver/install/ca.py
index bce804ac1c4e3eaf8dd08bed894ad45ea2d73ae1..00e0b038ca03320fd7b8268fb3eb96c5bc50a3ac 100644
--- a/ipaserver/install/ca.py
+++ b/ipaserver/install/ca.py
@@ -4,10 +4,9 @@
 
 from __future__ import print_function
 
-import sys
-
 from ipaserver.install import cainstance, dsinstance, bindinstance
 from ipapython import ipautil, certdb
+from ipapython.admintool import ScriptError
 from ipaplatform import services
 from ipaplatform.paths import paths
 from ipaserver.install import installutils, certs
@@ -30,12 +29,11 @@ def install_check(standalone, replica_config, options):
 
 if replica_config is not None:
 if standalone and api.env.ra_plugin == 'selfsign':
-sys.exit('A selfsign CA can not be added')
+raise ScriptError('A selfsign CA can not be added')
 
 if ((not options.promote
  and not ipautil.file_exists(replica_config.dir + "/cacert.p12"))):
-print('CA cannot be installed in CA-less setup.')
-sys.exit(1)
+raise ScriptError('CA cannot be installed in CA-less setup.')
 
 if standalone and not options.skip_conncheck:
 principal = options.principal
@@ -53,7 +51,7 @@ def install_check(standalone, replica_config, options):
 
 if standalone:
 if api.Command.ca_is_enabled()['result']:
-sys.exit(
+

Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules


On 08/16/2016 08:36 AM, Stanislav Laznicka wrote:

On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
I chose ScriptError because it's able to change the return value of 
the script, which is necessary sometimes. RuntimeError, which may 
seem more suitable, would not be able to do that. However, I am open 
for ideas on which exception type to use.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
I really hate to print errors to STDOUT from modules and then just 
call exit(1) (duplicated evil), could you replace print('xxx') with 
raise AnException('xxx')
Did that, only kept those prints printing directly to stderr. Not 
sure if those should be changed as well.



Bumping for review/opinions.

Also a self-NACK, there were some sys imports that should not have been 
there anymore (thanks, mbasti). Attaching a fixed version.


From 4617dbff9f8c358f4e528e1a8961ab7eb63cc2bd Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |   2 +-
 ipaserver/install/ca.py|  42 +-
 ipaserver/install/cainstance.py|   5 +-
 ipaserver/install/dns.py   |   5 +-
 ipaserver/install/dsinstance.py|   4 +-
 ipaserver/install/installutils.py  |  20 ++---
 ipaserver/install/ipa_ldap_updater.py  |   4 +-
 ipaserver/install/krainstance.py   |   4 +-
 ipaserver/install/replication.py   |  10 ++-
 ipaserver/install/server/install.py|  75 -
 ipaserver/install/server/replicainstall.py | 129 ++---
 11 files changed, 148 insertions(+), 152 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 7538e145cbe37dfc21963d97dea0e835e3bd5072..c0a4647d0544a8aab1fa6701722d4b0ffe6163d5 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -473,7 +473,7 @@ def check_reverse_zones(ip_addresses, reverse_zones, options, unattended,
 except ValueError as e:
 msg = "Reverse zone %s will not be used: %s" % (rz, e)
 if unattended:
-sys.exit(msg)
+raise RuntimeError(msg)
 else:
 root_logger.warning(msg)
 continue
diff --git a/ipaserver/install/ca.py b/ipaserver/install/ca.py
index bce804ac1c4e3eaf8dd08bed894ad45ea2d73ae1..00e0b038ca03320fd7b8268fb3eb96c5bc50a3ac 100644
--- a/ipaserver/install/ca.py
+++ b/ipaserver/install/ca.py
@@ -4,10 +4,9 @@
 
 from __future__ import print_function
 
-import sys
-
 from ipaserver.install import cainstance, dsinstance, bindinstance
 from ipapython import ipautil, certdb
+from ipapython.admintool import ScriptError
 from ipaplatform import services
 from ipaplatform.paths import paths
 from ipaserver.install import installutils, certs
@@ -30,12 +29,11 @@ def install_check(standalone, replica_config, options):
 
 if replica_config is not None:
 if standalone and api.env.ra_plugin == 'selfsign':
-sys.exit('A selfsign CA can not be added')
+raise ScriptError('A selfsign CA can not be added')
 
 if ((not options.promote
  and not ipautil.file_exists(replica_config.dir + "/cacert.p12"))):
-print('CA cannot be installed in CA-less setup.')
-sys.exit(1)
+raise ScriptError('CA cannot be installed in CA-less setup.')
 
 if standalone and not options.skip_conncheck:
 principal = options.principal
@@ -53,7 +51,7 @@ def install_check(standalone, replica_config, options):
 
 if standalone:
 if api.Command.ca_is_enabled()['result']:
-sys.exit(
+raise ScriptError(
 "One or more CA masters are already present in IPA realm "
 "'%s'.\nIf you wish to replicate CA to this host, please "
 "re-run 'ipa-ca-install'\nwith a replica file generated on "
@@ -64,28 +62,28 @@ def install_check(standalone, replica_config, options):
 if not cainstance.is_step_one_done():
 # This can happen if someone passes external_ca_file without
 # already having done the first stage of the CA install.
-print("CA is not installed yet. To install with an external CA "
+raise ScriptError(
+  "CA is not installed yet. To install with an external CA

Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules


On 08/02/2016 01:08 PM, Stanislav Laznicka wrote:

On 07/28/2016 10:57 AM, Martin Basti wrote:

Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
I chose ScriptError because it's able to change the return value of 
the script, which is necessary sometimes. RuntimeError, which may seem 
more suitable, would not be able to do that. However, I am open for 
ideas on which exception type to use.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
I really hate to print errors to STDOUT from modules and then just 
call exit(1) (duplicated evil), could you replace print('xxx') with 
raise AnException('xxx')
Did that, only kept those prints printing directly to stderr. Not sure 
if those should be changed as well.



Bumping for review/opinions.

--
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] [DESIGN][UPDATE] Time-Based HBAC Policies


On 08/12/2016 06:48 PM, Petr Spacek wrote:

On 11.8.2016 12:34, Stanislav Laznicka wrote:

Hello,

I updated the design of the Time-Based HBAC Policies according to the
discussion we led here earlier. Please check the design page
http://www.freeipa.org/page/V4/Time-Based_Account_Policies. The biggest
changes are in the Implementation and Feature Management sections. I also
added a short How to Use section.

Thank you for the review! I will add some comments inline.

Nice page!

On the high level it all makes sense.

ad LDAP schema
==
1) Why accessTime attribute is MAY in ipaTimeRule object class?
Does it make sense to have the object without accessTime? I do not think so.
My idea was that we allow users prepare a few time rule objects before 
filling them with the actual times.

Also, it could be good to add description attribute to the object class and
incorporate it into commands (including find).


Definitely a good idea, I will work that in.

2) Besides all this, I spent few minutes in dark history of IPA. The
accessTime attribute was introduced back in 2009 in commit
"55ba300c7cb59cf05b16cc01281f51d93eb25acf" aka "Incorporate new schema for 
IPAv2".

The commit does not contain any reasoning for the change but I can see that
the attribute is already used as MAY in old object classes ipaHBACRule and
ipaSELinuxUserMap.

Is any of these a problem?
I believe that the accessTime attribute was originally brought to IPA 
when there was an implementation of time policies for HBAC objects and 
it's been rotting there ever since those capabilities were removed. We 
may eventually use a new attribute for storage of the time strings as 
accessTime by definition is multi-valued which is not what's currently 
desired (although we may end up with it some day in the future). 
However, I don't think any other use of accessTime should be a problem 
as it's been obsoleted for a long time.

Why is it even in ipaSELinuxUserMap object class?
I'm sorry to say I have no idea. I used it for what it originally was - 
a means for storing time strings at HBAC rules.

Commit
55512dc938eb4a9a6655e473beab587e340af55c does not mention any reason for doing 
so.

I cannot see any other problem so the low-level stuff is good and can be
implemented.


ad User interface
=
We need to polish the user interface so it really usable.

At least the web interface should contain some shortcuts. E.g. when I'm adding
a new HBAC rule, the "time" section should contain also "something" to
immediately add new time rule so I do not need to go to time rules first and
then go back to HBAC page.

Similarly, dialog for rule modification should allow to easily change all the
values, warn if time rules is shared, and also have an easy way to
'disconnect' the time rule, i.e. make a copy of it and edit only the new copy
(instead of the shared original).

All these are user interface things not affecting the low-level stuff.


Maybe you should sat down with some UX designer, talk about these cases and
draw some hand-made pictures.

I will add Pavel V. to CC, we may want to discuss this.

I do not believe that this will require any changes in schema so you can
polish SSSD and framework implementation in meantime.

On the link below is a PROTOTYPE-patched FreeIPA that covers most of the CLI
functionality (except for the creation of iCalendar strings from options) for
better illustration of the design.

https://github.com/stlaz/freeipa/tree/timerules_2
Honestly I did not look at the code today :-)

Overall, I'm glad to see current proposal. After so many iteration, we reached
something which does not have any glaring problem :-)
It definitely felt better to be writing it with all the previous 
knowledge. Thank you! :-)


--
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 0063] Raise error on topology disconnect/last-role-host removal during server uninstall

2016-08-12 Thread Stanislav Laznicka


Hello,

topology disconnect/last-role-host removal errors would just be logged 
during server uninstall even if ignore options are not present. The host 
would still appear in the topology even after "successful" uninstall.


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

From b8099217336af8ed191bed67303d1e52505c2a86 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 12 Aug 2016 11:59:41 +0200
Subject: [PATCH] Fail on topology disconnect/last role removal

Disconnecting topology/removing last-role-host during server
uninstallation should raise error rather than just being logged
if the appropriate ignore settings are not present.

https://fedorahosted.org/freeipa/ticket/6168
---
 ipaserver/install/server/install.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 94698898934844350488d5fc52d6e1e567624502..7aa2590f65fd0dda7f8faf4056bf7cce7032e1b4 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -294,7 +294,6 @@ def common_cleanup(func):
 def remove_master_from_managed_topology(api_instance, options):
 try:
 # we may force the removal
-# if the master was already deleted we will just get a warning
 server_del_options = dict(
 force=True,
 ignore_topology_disconnect=options.ignore_topology_disconnect,
@@ -303,8 +302,10 @@ def remove_master_from_managed_topology(api_instance, options):
 
 replication.run_server_del_as_cli(
 api_instance, api_instance.env.host, **server_del_options)
-
+except errors.ServerRemovalError:
+raise
 except Exception as e:
+# if the master was already deleted we will just get a warning
 root_logger.warning("Failed to delete master: {}".format(e))
 
 
-- 
2.7.4

-- 
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 0058] Make get_entries not ignore its size_limit argument

2016-08-11 Thread Stanislav Laznicka


On 08/11/2016 07:49 AM, Jan Cholasta wrote:

On 2.8.2016 13:47, Stanislav Laznicka wrote:

On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question 
in my
head whether ipaldap.get_entries is used properly throughout the 
system
- does it always assume that it gets ALL the requested entries or 
just a

few of those as configured by the 'ipaSearchRecordsLimit' attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin (which is
usually the case in the framework), then ipaSearchRecordsLimit is
used. If you call it on some arbitrary LDAPClient instance, the
hardcoded default (= unlimited) is used.



One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit 
is not

set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?

AFAIU it is desired that the search is unlimited. However, due to the
fact that neither size_limit nor paged_search are passed from
ldap2.get_entries() to ldap2.find_entries() (methods inherited from
LDAPClient), only the number of records specified by
ipaSearchRecordsLimit is returned. That could eventually cause problems
should ipaSearchRecordsLimit be set to a low value as in the ticket.


I see. This is *not* intentional, the **kwargs of get_entries() should 
be passed to find_entries(). This definitely needs to be fixed.




Anyway, this ticket is not really easily fixable without more profound
changes. Often, multiple LDAP searches are done during command
execution. What do you do with the size limit then? Do you pass the
same size limit to all the searches? Do you subtract the result size
from the size limit after each search? Do you do something else with
it? ... The answer is that it depends on the purpose of each
individual LDAP search (like in get_memberindirect() above, we have to
do unlimited search, otherwise the resulting entry would be
incomplete), and fixing this accross the whole framework is a
non-trivial task.


I do realize that the proposed fix for the permission plugin is not
perfect, it would probably be better to subtract the number of currently
loaded records from the sizelimit, although in the end the number of
returned values will not be higher than the given size_limit. However,
it seems reasonable that if get_entries is passed a size limit, it
should apply it over current ipaSearchRecordsLimit rather than ignoring
it. Then, any use of get_entries could be fixed accordingly if someone
sees fit.



Right. Anyway, this is a different issue than above, so please put 
this into a separate commit.



Please see the attached patches, then.

From 75d8cf9c3708b68c9b3a9ba999b3d034b1ddc33a Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 11 Aug 2016 14:08:33 +0200
Subject: [PATCH 1/2] Make get_entries() not ignore its limit arguments

get_entries() wouldn't pass some arguments deeper to find_entries()
function it wraps. This would cause unexpected behavior in some
cases throughout the framework where specific (non-)limitations
are expected.

https://fedorahosted.org/freeipa/ticket/5640
---
 ipapython/ipaldap.py | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 704e71a9471c27430328a8c7c6a319aa72a9d482..a3f0a5668616f66ba744c336832064269882eb8b 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1298,7 +1298,8 @@ class LDAPClient(object):
 for their description.
 """
 entries, truncated = self.find_entries(
-base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list)
+base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list,
+**kwargs)
 try:
 self.handle_truncated_result(truncated)
 except errors.LimitsExceeded as e:
@@ -1313,7 +1314,8 @@ class LDAPClient(object):
 
 def find_entries(self, filter=None, attrs_list=None, base_dn=None,
  scope=ldap.SCOPE_SUBTREE, time_limit=None,
- size_limit=None, search_refs=False, paged_search=False):
+ size_limit=None, search_refs=False, paged_search=False,
+ **kwargs):
 """
 Return a list of entries and in

[Freeipa-devel] [DESIGN][UPDATE] Time-Based HBAC Policies

2016-08-11 Thread Stanislav Laznicka


Hello,

I updated the design of the Time-Based HBAC Policies according to the 
discussion we led here earlier. Please check the design page 
http://www.freeipa.org/page/V4/Time-Based_Account_Policies. The biggest 
changes are in the Implementation and Feature Management sections. I 
also added a short How to Use section.


On the link below is a PROTOTYPE-patched FreeIPA that covers most of the 
CLI functionality (except for the creation of iCalendar strings from 
options) for better illustration of the design.


https://github.com/stlaz/freeipa/tree/timerules_2

I will add FreeIPA people that recently had some say about this to CC so 
that we can get the discussion flowing.


Thanks,
Standa

--
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 0061] Removing objectclass expectation for LDAP*ReverseMember based commands

2016-08-10 Thread Stanislav Laznicka


Hello,

I missed some tests in patch to 
https://fedorahosted.org/freeipa/ticket/5892. This patch should fix the 
rest of the additional objectclass expectations mentioned in 
https://fedorahosted.org/freeipa/ticket/6198.
From 7b39ff355ab70a63e96dd8d9bcc6da3a6930a3ba Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 10 Aug 2016 10:53:00 +0200
Subject: [PATCH] Removed objectclass from LDAP*ReverseMember based tests

Some tests were broken because of the recent changes in baseldap (#5892)
as they were wrongly expecting an objectclass attribute.

https://fedorahosted.org/freeipa/ticket/6198
---
 ipatests/test_xmlrpc/test_old_permission_plugin.py | 1 -
 ipatests/test_xmlrpc/test_privilege_plugin.py  | 7 ---
 2 files changed, 8 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_old_permission_plugin.py b/ipatests/test_xmlrpc/test_old_permission_plugin.py
index 2328c6742d6996c3c3780f4578a8fe02ba371c11..6d1117b6b33b51d575b3ee40356d4fda8df2533f 100644
--- a/ipatests/test_xmlrpc/test_old_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_old_permission_plugin.py
@@ -214,7 +214,6 @@ class test_old_permission(Declarative):
 'cn': [privilege1],
 'description': [u'privilege desc. 1'],
 'memberof_permission': [permission1],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
diff --git a/ipatests/test_xmlrpc/test_privilege_plugin.py b/ipatests/test_xmlrpc/test_privilege_plugin.py
index c375f907f6d379e48934a5375eebaf51e1d27c65..49f6c80b91478047de7c710c24cea55706ccf499 100644
--- a/ipatests/test_xmlrpc/test_privilege_plugin.py
+++ b/ipatests/test_xmlrpc/test_privilege_plugin.py
@@ -149,7 +149,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'privilege desc. 1'],
 'memberof_permission': [permission1],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
@@ -287,7 +286,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'privilege desc. 1'],
 'memberof_permission': [permission1, permission2],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
@@ -310,7 +308,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'privilege desc. 1'],
 'memberof_permission': [permission1, permission2],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
@@ -387,7 +384,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'New desc 1'],
 'memberof_permission': [permission2],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
@@ -410,7 +406,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'New desc 1'],
 'memberof_permission': [permission2],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
@@ -433,7 +428,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'New desc 1'],
 'memberof_permission': [permission2],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
@@ -456,7 +450,6 @@ class test_privilege(Declarative):
 'cn': [privilege1],
 'description': [u'New desc 1'],
 'memberof_permission': [permission2],
-'objectclass': objectclasses.privilege,
 }
 ),
 ),
-- 
2.7.4

-- 
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 0060] Add --force-join option to ipa-replica-install

2016-08-09 Thread Stanislav Laznicka


On 08/10/2016 07:31 AM, Jan Cholasta wrote:

On 9.8.2016 18:52, Petr Vobornik wrote:

On 08/09/2016 04:18 PM, Martin Basti wrote:



On 09.08.2016 16:07, Stanislav Laznicka wrote:

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




Didn't we agreed that --force-join should be always used (without extra
replica-install option)


+1


Did we?

IMO the default behavior should be the same as in domain level 0 when 
trying to install replica on an already enrolled host.

That was my impression as well.

--
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 0057] Don't show part of warning containing --force-ntpd in replica install

2016-08-09 Thread Stanislav Laznicka


On 08/04/2016 07:34 AM, Jan Cholasta wrote:

On 3.8.2016 19:39, Martin Basti wrote:



On 03.08.2016 18:10, Petr Vobornik wrote:

On 07/13/2016 12:36 PM, Stanislav Laznicka wrote:

On 07/13/2016 09:51 AM, Petr Vobornik wrote:

On 07/13/2016 08:26 AM, Stanislav Laznicka wrote:

On 07/12/2016 08:44 AM, Stanislav Laznicka wrote:

On 07/11/2016 04:27 PM, Petr Vobornik wrote:

On 07/11/2016 01:23 PM, Stanislav Laznicka wrote:

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




Isn't the bug about something else?

The issue was that ipa-replica-install doesn't have --force-ntpd
option.
It is an option of ipa-client-install which is run from replica
installer.

The unattended mode is unrelated.

My understanding is that the bug says that '--force-ntpd' option
should not be shown when ipa-client-install is run during replica
installation.

During replica installation, the ipa-client-install script is run
with
the '--unattended' flag in the 'ensure_enrolled()' function. 
Being a

separate script, there's not many options on how to pass the
information not to show the message to ipa-client-install. Using 
the

already used flag to get rid of the message seemed easiest to me.
Introducing a new 'hidden' flag (like '--from-replica'), on the 
other

hand, seems a bit harsh.


Just to throw it out there - it's possible that the '--force-join'
client option would also appear as a hint from the client install
script
(during replica installation). Should this also be muted somehow?
To me,
it seems reasonable to rather add it as an argument to
ipa-replica-install to pass it to the client install script.

IMO client installation initiated from replica needs to have a 
special

option(hidden in help) similar to --on-server (or what's its name).
E.g.
the name can be --replica-install. Maybe --on-server can be used 
but it
may have other implication which might not be valid for this use 
case.


Anything else are just workarounds. Imagine that admin runs
ipa-client-install with --unattended or --force-join. He would 
then not

get the message as now.

Reviving thread to get other opinion.


The --on-master option won't do here as it seems that the client would
require some IPA pre-configuration for successful install. A new 
option

will have to be created, then.

I'm for new "hidden" option.


I'm against any hidden options, this should be made correctly by
modularization/fixing of client install, to be able call it from python
not as external process


+1, but this is non-trivial and definitely not material for 4.4.1. For 
4.4.1 the hidden option should be OK.




Just from top of my head, can we just use option --no-ntp with client
install in replica installer? Server NTP should not depend on client ntp
config.
I'm just afraid that we may get kerberos time issue during client
install if client time does not match server time.

Or second approach, always call client install from replica with
--force-ntpd, unless there is --no-ntp used for replica, then call
ipa-client-install with --no-ntp

But it needs investigation.


CCing David as he knows everything NTP-related.



Martin^2



As I was trying to point out, the situation about --force-join is a 
bit

different. The option again would be shown and is not available in
ipa-replica-install. I think it should be available to allow direct
replica installation even when previous installation failed/left some
mess on the master (ofc the user could run `ipa-replica-manage del
 --cleanup` on the master instead).


That could work but imho is out of scope of this ticket.





Please see the attached patch that always adds the --no-ntp option to 
ipa-client-install.


From f563794f3f5f6f9d92ffc257489e92147b398ccf Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 9 Aug 2016 15:22:33 +0200
Subject: [PATCH] Don't show --force-ntpd option in replica install

Always run the client installation script with --no-ntp
option so that it does not show the message about --force-ntpd
option that does not exist in ipa-replica-install. The time
synchronization is done elsewhere anyway.

https://fedorahosted.org/freeipa/ticket/6046
---
 ipaserver/install/server/replicainstall.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 9d05a0be5a2679d825b4ee6bc2ea55ed358e8ff9..f54ff7da06c57b9c8251429cbdacc5c300805f84 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -881,7 +881,7 @@ def install(installer):
 try:
 args = [paths.IPA_CLIENT_INSTALL, "--on-master", "--unattended",
 "--domain", config.domain_name, "--server", config.host_name,
-"--realm", config.realm_name]
+"--realm", config.realm_name, "--no-ntp"]
 if options.no_dns_sshfp:
 ar

Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

2016-08-02 Thread Stanislav Laznicka


On 07/19/2016 09:20 AM, Jan Cholasta wrote:

Hi,

On 14.7.2016 14:36, Stanislav Laznicka wrote:

Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question in my
head whether ipaldap.get_entries is used properly throughout the system
- does it always assume that it gets ALL the requested entries or just a
few of those as configured by the 'ipaSearchRecordsLimit' attribute of
ipaConfig.etc which it actually gets?


That depends. If you call get_entries() on the ldap2 plugin (which is 
usually the case in the framework), then ipaSearchRecordsLimit is 
used. If you call it on some arbitrary LDAPClient instance, the 
hardcoded default (= unlimited) is used.




One spot that I know the get_entries method was definitely not used
properly before this patch is in the
baseldap.LDAPObject.get_memberindirect() method:

 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get everything
anyway
 697 paged_search=True)

which to me seems kind of important if the environment size_limit is not
set properly :) The patch does not fix the non-propagation of the
paged_search, though.


Why do you think size_limit is not used properly here?
AFAIU it is desired that the search is unlimited. However, due to the 
fact that neither size_limit nor paged_search are passed from 
ldap2.get_entries() to ldap2.find_entries() (methods inherited from 
LDAPClient), only the number of records specified by 
ipaSearchRecordsLimit is returned. That could eventually cause problems 
should ipaSearchRecordsLimit be set to a low value as in the ticket.


Anyway, this ticket is not really easily fixable without more profound 
changes. Often, multiple LDAP searches are done during command 
execution. What do you do with the size limit then? Do you pass the 
same size limit to all the searches? Do you subtract the result size 
from the size limit after each search? Do you do something else with 
it? ... The answer is that it depends on the purpose of each 
individual LDAP search (like in get_memberindirect() above, we have to 
do unlimited search, otherwise the resulting entry would be 
incomplete), and fixing this accross the whole framework is a 
non-trivial task.


I do realize that the proposed fix for the permission plugin is not 
perfect, it would probably be better to subtract the number of currently 
loaded records from the sizelimit, although in the end the number of 
returned values will not be higher than the given size_limit.  However, 
it seems reasonable that if get_entries is passed a size limit, it 
should apply it over current ipaSearchRecordsLimit rather than ignoring 
it. Then, any use of get_entries could be fixed accordingly if someone 
sees fit.


--
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 0048] Remove sys.exit() from installer modules

2016-08-02 Thread Stanislav Laznicka


On 07/28/2016 10:57 AM, Martin Basti wrote:

On 17.06.2016 13:56, Stanislav Laznicka wrote:

On 06/17/2016 01:01 PM, Petr Vobornik wrote:

On 17.6.2016 12:12, Stanislav Laznicka wrote:

On 06/17/2016 09:51 AM, Petr Vobornik wrote:

On 17.6.2016 09:24, Stanislav Laznicka wrote:

On 06/17/2016 08:48 AM, Petr Spacek wrote:

On 17.6.2016 08:43, Stanislav Laznicka wrote:

On 06/17/2016 07:45 AM, Petr Spacek wrote:

On 16.6.2016 17:33, Stanislav Laznicka wrote:

Hello,

This patch removes most sys.exits() from installer modules and
scripts and
replaces them with ScriptError. I only left sys.exits at places
where the user
decides yes/no on continuation of the script.
I wonder if yes/no should be replaced with KeyboardInterrupt 
or some

other
exception, too...


I'm not sure, it seems more clear to just really exit if the user
desires it
and it's what we say we'll do (with possible cleanup 
beforehand). Do

you think
we could benefit somehow by raising an exception here?

I'm just thinking out loud.

It seemed to me that it is easier to share cleanup on one except 
block

instead
of having if (interrupt): cleanup; if (interrupt2): same_cleanup;

etc.

Again, just wondering out loud.

If the cleanup is the same, or similar it might be more 
beneficial to
have it in a function where you could pass what was set up 
already and
therefore needs cleanup. But that's just an opinion coming from 
thinking
out loud as well. I went through the code to see if there's much 
cleanup
after these user actions and it seems that usually there's 
nothing much
if anything. However, thinking in advance may save us much 
trouble in

the future, of course.

Btw the original scope of the ticket is to replace sys.exit calls 
ONLY
in installer modules. Please don't waste time with debugging other 
use

cases before 4.4 is out.


I might have gotten carried away a bit. Would you suggest keeping the
sys.exits replaced only in ipaserver/install/server/replicainstall.py,
ipaserver/install/server/install.py modules which are the installer
modules managed by AdminTool? I considered the modules in
ipaserver/install/ to also be installer modules as they are heavily 
used

during installation and the sys.exits there mainly occur in functions
called from install()/install_check() methods. The *-install scripts
were perhaps really obviously over the scope.

Yes, modules:
  ipaserver/install/bindinstance.py  |  2 +-
  ipaserver/install/ca.py| 19 +++---
  ipaserver/install/cainstance.py|  5 +-
  ipaserver/install/dns.py   |  5 +-
  ipaserver/install/dsinstance.py|  3 +-
  ipaserver/install/installutils.py  | 16 +++---
  ipaserver/install/ipa_ldap_updater.py  |  2 +-
  ipaserver/install/krainstance.py   |  3 +-
  ipaserver/install/replication.py   | 10 ++--
  ipaserver/install/server/install.py| 64 +++--
  ipaserver/install/server/replicainstall.py | 92

not modules:
install/tools/ipa-adtrust-install | 17 +++---
install/tools/ipa-ca-install | 23 
install/tools/ipa-compat-manage | 11 ++--
install/tools/ipa-dns-install | 3 +-

I'll keep the sys.exit replaces that won't make it here on the 
side, we

may want to do them later.

I'm a bit worried that the patch might change some behavior. Maybe I'm
wrong.


Attached is the patch with correct modules with sys.exits replaced.

I double-checked the changes and I believe the behavior shouldn't 
really change.






Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?
I chose ScriptError because it's able to change the return value of the 
script, which is necessary sometimes. RuntimeError, which may seem more 
suitable, would not be able to do that. However, I am open for ideas on 
which exception type to use.


2)
Can you use rather raise Exception(), instead of raise Exception


Sure, please see the modified attached patch.

3)
I really hate to print errors to STDOUT from modules and then just 
call exit(1) (duplicated evil), could you replace print('xxx') with 
raise AnException('xxx')
Did that, only kept those prints printing directly to stderr. Not sure 
if those should be changed as well.
From d4e820b4e59da1f2ecc6810cbb3353d6f9d53053 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |   2 +-
 ipaserver/install/ca.py|  40 -
 ipaserver/install/cainstance.py|   5 +-
 ipaserver/install/dns.py   |   5 +-
 ipaserver/install/dsinstance.py|   3 +-
 ipaserver/install/installutils.py  |  20 ++---
 ipaserver/install/ipa_ldap_updater.py  |   3 +-
 ipa

Re: [Freeipa-devel] [PATCH 0059] Fix to ipa-cacert-manage man and help differences

2016-08-02 Thread Stanislav Laznicka


On 07/19/2016 10:25 AM, Florence Blanc-Renaud wrote:

On 07/15/2016 02:09 PM, Stanislav Laznicka wrote:

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




Hi Stanislav,

thanks for your patch. As CERTFILE is added in the arguments for 
install, I would suggest to mention it in the command description. For 
instance:


install
  - Install a CA certificate
  This command can be used to install the certificate contained in 
CERTFILE as a new CA certificate to IPA.


Flo.


Hi,

Thanks for the notice, I agree that it'd be better to be more verbose 
about the CERTFILE argument. Please see the modified patch.


From 6cfe281647a489909085875b3011486ca276f044 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 15 Jul 2016 14:04:59 +0200
Subject: [PATCH] Improvements for the ipa-cacert-manage man and help

The man page for ipa-cacert-manage didn't mention that some
options are only applicable to the install some to the renew
subcommand.

Also fixed a few missing articles.

https://fedorahosted.org/freeipa/ticket/6013
---
 install/tools/man/ipa-cacert-manage.1  | 38 ++
 ipaserver/install/ipa_cacert_manage.py |  2 +-
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/install/tools/man/ipa-cacert-manage.1 b/install/tools/man/ipa-cacert-manage.1
index 1f37788336048e412eee71757f236c9944860514..f0a1033ab372c2f923a883b385c0e3304b98f56f 100644
--- a/install/tools/man/ipa-cacert-manage.1
+++ b/install/tools/man/ipa-cacert-manage.1
@@ -20,7 +20,9 @@
 .SH "NAME"
 ipa\-cacert\-manage \- Manage CA certificates in IPA
 .SH "SYNOPSIS"
-\fBipa\-cacert\-manage\fR [\fIOPTIONS\fR...] \fICOMMAND\fR
+\fBipa\-cacert\-manage\fR [\fIOPTIONS\fR...] renew
+.RE
+\fBipa\-cacert\-manage\fR [\fIOPTIONS\fR...] install \fICERTFILE\fR
 .SH "DESCRIPTION"
 \fBipa\-cacert\-manage\fR can be used to manage CA certificates in IPA.
 .SH "COMMANDS"
@@ -29,7 +31,7 @@ ipa\-cacert\-manage \- Manage CA certificates in IPA
 \- Renew the IPA CA certificate
 .sp
 .RS
-This command can be used to manually renew CA certificate of the IPA CA.
+This command can be used to manually renew the CA certificate of the IPA CA.
 .sp
 When the IPA CA is the root CA (the default), it is not usually necessary to manually renew the CA certificate, as it will be renewed automatically when it is about to expire, but you can do so if you wish.
 .sp
@@ -42,13 +44,30 @@ When the IPA CA is not configured, this command is not available.
 \- Install a CA certificate
 .sp
 .RS
-This command can be used to install new CA certificate to IPA.
+This command can be used to install the certificate contained in \fICERTFILE\fR as a new CA certificate to IPA.
 .RE
-.SH "OPTIONS"
+.SH "COMMON OPTIONS"
+.TP
+\fB\-\-version\fR
+Show the program's version and exit.
+.TP
+\fB\-h\fR, \fB\-\-help\fR
+Show the help for this program.
 .TP
 \fB\-p\fR \fIDM_PASSWORD\fR, \fB\-\-password\fR=\fIDM_PASSWORD\fR
 The Directory Manager password to use for authentication.
 .TP
+\fB\-v\fR, \fB\-\-verbose\fR
+Print debugging information.
+.TP
+\fB\-q\fR, \fB\-\-quiet\fR
+Output only errors.
+.TP
+\fB\-\-log\-file\fR=\fIFILE\fR
+Log to the given file.
+.RE
+.SH "RENEW OPTIONS"
+.TP
 \fB\-\-self\-signed\fR
 Sign the renewed certificate by itself.
 .TP
@@ -57,6 +76,8 @@ Sign the renewed certificate by external CA.
 .TP
 \fB\-\-external\-cert\-file\fR=\fIFILE\fR
 File containing the IPA CA certificate and the external CA certificate chain. The file is accepted in PEM and DER certificate and PKCS#7 certificate chain formats. This option may be used multiple times.
+.RE
+.SH "INSTALL OPTIONS"
 .TP
 \fB\-n\fR \fINICKNAME\fR, \fB\-\-nickname\fR=\fINICKNAME\fR
 Nickname for the certificate.
@@ -73,15 +94,6 @@ T \- CA trusted to issue client certificates
 .IP
 p \- not trusted
 .RE
-.TP
-\fB\-v\fR, \fB\-\-verbose\fR
-Print debugging information.
-.TP
-\fB\-q\fR, \fB\-\-quiet\fR
-Output only errors.
-.TP
-\fB\-\-log\-file\fR=\fIFILE\fR
-Log to the given file.
 .SH "EXIT STATUS"
 0 if the command was successful
 
diff --git a/ipaserver/install/ipa_cacert_manage.py b/ipaserver/install/ipa_cacert_manage.py
index de13ad39397ae5e9b924b0621521e5fc6016c8e6..32ef25c7aac3e57d27955b6a2608adb6a1626019 100644
--- a/ipaserver/install/ipa_cacert_manage.py
+++ b/ipaserver/install/ipa_cacert_manage.py
@@ -35,7 +35,7 @@ from ipaserver.install import certs, cainstance, installutils
 class CACertManage(admintool.AdminTool):
 command_name = 'ipa-cacert-manage'
 
-usage = "%prog {renew|install} [options]"
+usage = "%prog renew [options]\n%prog install [options] CERTFILE"
 
 description = "Manage CA certificates."
 
-- 
2.7.4

-- 
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] [DESIGN] Time-Based HBAC Policies

2016-07-15 Thread Stanislav Laznicka

On 07/15/2016 02:10 PM, Simo Sorce wrote:

On Wed, 2016-05-18 at 15:28 +0200, Stanislav Laznicka wrote:

On 05/18/2016 02:19 PM, Alexander Bokovoy wrote:

On Wed, 18 May 2016, Stanislav Laznicka wrote:

when removal succeeds but addition fails for some reason?
The
operation is not atomic anymore.

We offline-discussed this with Honza. There should be a new
command
`ipa hbacrule-replace-accesstime rule_name --orig-time=icalstr1
--new-time=icalstr2`. As it would be derived from LDAPQuery, the
atomicity is kept. This may not be very nice for CLI but should
work
well for WebUI. Both icalstr1 and icalstr2 need to be encoded as
newlines that appear so often in iCalendar strings would only
make a
mess here.

Example of use:

ipa hbacrule-replace-accesstime rule_name
--orig-time="'BEGIN:VCALENDAR\\r\\nPRODID:-//The Company//iCal4j
1.0//EN\\r\\nVERSION:2.0\\r\\nMETHOD:REQUEST\\r\\nBEGIN:VEVENT\\r
\\nUID:1...@company.org\\r\\nDTSTAMP:20160406T112129Z\\r\\nDTSTART:2
0101115T05Z\\r\\nDTEND:20101115T07Z\\r\\nRRULE:FREQ=MONTH
LY;INTERVAL=5;BYDAY=MO;BYHOUR=5,6\\r\\nEND:VEVENT\\r\\nEND:VCALEN
DAR\\r\\n'"
--new-time="'BEGIN:VCALENDAR\\r\\nPRODID:-//The Company//iCal4j
1.0//EN\\r\\nVERSION:2.0\\r\\nMETHOD:REQUEST\\r\\nBEGIN:VEVENT\\r
\\nUID:1...@company.org\\r\\nDTSTAMP:20160406T112129Z\\r\\nDTSTART:2
0101115T05Z\\r\\nDTEND:20101115T07Z\\r\\nRRULE:FREQ=MONTH
LY;INTERVAL=5;BYDAY=MO,TU;BYHOUR=5,6\\r\\nEND:VEVENT\\r\\nEND:VCA
LENDAR\\r\\n'"

to add Tuesdays to the timespan defined by the rule.

I would really like to see a file input support here. It would be
simpler to operate in CLI as you would anyway create vCal files --
no
sane person is going to deal with these strings directly on the
command
line.

That is correct and some basic file support is already in the patches
I
sent earlier, though replacing rules is not a part of it. However,
it
does not solve the problem as you would still need access to the
files
to work with the attributes and then change the files accordingly.

However, we've had yet another brainstorm with Petr^2, Martin^2 and
Honza. We really don't want the above so we came up with some ideas
that
I'm listing below. Note that we also do not want more than one
VEVENT
component in any of the time rules. So, the ideas:
1) Have the time rules as separate objects. This approach got
most
support here. Adding Simo and Jakub to CC should they have any input
against this.
2) Have the time rules stored as strings in the multi-valued
accesstime attribute at each rule. These would be referenced by
their
UID property of the VEVENT component of the iCalendar string (instead
of
that pure hell above). As each of the strings can only contain one
VEVENT which has to define a UID, the only problem would be to keep
the
uniqueness of UIDs consistent.

From my point of view, 1) seems rather better but your experience
might
be different. Don't hesitate to share your opinions, please.

Can you please give me an example ldif of a complete hbac rule
including time rules with the 2 different proposals ?

I do not really care a lot how the framework ends up managing the
objetcs, I care mostly about how the information is stored in LDAP and
how efficient the storage will be for SSSD retrieval.

That's my evaluation pov.
Keep in mind that rules are modified rarely but downloaded much more
frequently, so it is ok to have a slightly harder way to store them to
gain efficiency in retrieving and downloading them.

Simo.

Please find the ldif files attached, with some additional changes than
only to hbac rules. It's from my current implementations.

OT: We had an offline discussion with Honza that to keep the backward
compatibility, it might be good to introduce v2 of HBAC rules so that's
what you see there. Perhaps accessTime should be in that v2 rule as well
but that's even more off-topic here.

objectClasses: (2.16.840.1.113730.3.8.4.80 NAME 'ipaTimeRule' SUP top STRUCTURAL MUST ( cn ) MAY ( memberOf $ accessTime ) X-ORIGIN 'IPA v4.5')
attributeTypes: (2.16.840.1.113730.3.8.4.72 NAME 'memberTimeRule' DESC 'Reference to a time rule describing some period of time' SUP distinguishedName EQUALITY distinguishedNameMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.12 X-ORIGIN 'IPA v4.5' )
objectClasses: (2.16.840.1.113730.3.8.4.16 NAME 'ipaHBACRulev2' SUP ipaAssociation STRUCTURAL MUST accessRuleType MAY ( sourceHost $ sourceHostCategory $ serviceCategory $ memberService $ externalHost $ accessTime $ memberTimeRule ) X-ORIGIN 'IPA v4.5' )

dn: cn=timerules,$SUFFIX
changetype: add
objectClass: top
objectClass: nsContainer
cn: timerules

attributeTypes: (2.16.840.1.113730.3.8.11.72 NAME 'timeruleTemplate' DESC 'CNs of the timerule templates' EQUALITY caseIgnoreMatch ORDERING caseIgnoreOrderingMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'IPA v4.3' )
objectClasses: (2.16.840.1.113730.3.8.4.7 NAME 'ipaHBACRulev2' SUP ipaAssociation STRUCTURAL MUST accessRuleType MAY ( sourceHost

[Freeipa-devel] [PATCH 0059] Fix to ipa-cacert-manage man and help differences

2016-07-15 Thread Stanislav Laznicka


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

From 44e55c3523aa1bf9a7243b9d22fb52e50f7c9440 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 15 Jul 2016 14:04:59 +0200
Subject: [PATCH] Improvements for the ipa-cacert-manage man and help

The man page for ipa-cacert-manage didn't mention that some
options are only applicable to the install some to the renew
subcommand.

Also fixed a few missing articles.

https://fedorahosted.org/freeipa/ticket/6013
---
 install/tools/man/ipa-cacert-manage.1  | 38 ++
 ipaserver/install/ipa_cacert_manage.py |  2 +-
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/install/tools/man/ipa-cacert-manage.1 b/install/tools/man/ipa-cacert-manage.1
index 1f37788336048e412eee71757f236c9944860514..d12b4b23eb72922c914712a9d1f969e996a2e272 100644
--- a/install/tools/man/ipa-cacert-manage.1
+++ b/install/tools/man/ipa-cacert-manage.1
@@ -20,7 +20,9 @@
 .SH "NAME"
 ipa\-cacert\-manage \- Manage CA certificates in IPA
 .SH "SYNOPSIS"
-\fBipa\-cacert\-manage\fR [\fIOPTIONS\fR...] \fICOMMAND\fR
+\fBipa\-cacert\-manage\fR [\fIOPTIONS\fR...] renew
+.RE
+\fBipa\-cacert\-manage\fR [\fIOPTIONS\fR...] install \fICERTFILE\fR
 .SH "DESCRIPTION"
 \fBipa\-cacert\-manage\fR can be used to manage CA certificates in IPA.
 .SH "COMMANDS"
@@ -29,7 +31,7 @@ ipa\-cacert\-manage \- Manage CA certificates in IPA
 \- Renew the IPA CA certificate
 .sp
 .RS
-This command can be used to manually renew CA certificate of the IPA CA.
+This command can be used to manually renew the CA certificate of the IPA CA.
 .sp
 When the IPA CA is the root CA (the default), it is not usually necessary to manually renew the CA certificate, as it will be renewed automatically when it is about to expire, but you can do so if you wish.
 .sp
@@ -42,13 +44,30 @@ When the IPA CA is not configured, this command is not available.
 \- Install a CA certificate
 .sp
 .RS
-This command can be used to install new CA certificate to IPA.
+This command can be used to install a new CA certificate to IPA.
 .RE
-.SH "OPTIONS"
+.SH "COMMON OPTIONS"
+.TP
+\fB\-\-version\fR
+Show the program's version and exit.
+.TP
+\fB\-h\fR, \fB\-\-help\fR
+Show the help for this program.
 .TP
 \fB\-p\fR \fIDM_PASSWORD\fR, \fB\-\-password\fR=\fIDM_PASSWORD\fR
 The Directory Manager password to use for authentication.
 .TP
+\fB\-v\fR, \fB\-\-verbose\fR
+Print debugging information.
+.TP
+\fB\-q\fR, \fB\-\-quiet\fR
+Output only errors.
+.TP
+\fB\-\-log\-file\fR=\fIFILE\fR
+Log to the given file.
+.RE
+.SH "RENEW OPTIONS"
+.TP
 \fB\-\-self\-signed\fR
 Sign the renewed certificate by itself.
 .TP
@@ -57,6 +76,8 @@ Sign the renewed certificate by external CA.
 .TP
 \fB\-\-external\-cert\-file\fR=\fIFILE\fR
 File containing the IPA CA certificate and the external CA certificate chain. The file is accepted in PEM and DER certificate and PKCS#7 certificate chain formats. This option may be used multiple times.
+.RE
+.SH "INSTALL OPTIONS"
 .TP
 \fB\-n\fR \fINICKNAME\fR, \fB\-\-nickname\fR=\fINICKNAME\fR
 Nickname for the certificate.
@@ -73,15 +94,6 @@ T \- CA trusted to issue client certificates
 .IP
 p \- not trusted
 .RE
-.TP
-\fB\-v\fR, \fB\-\-verbose\fR
-Print debugging information.
-.TP
-\fB\-q\fR, \fB\-\-quiet\fR
-Output only errors.
-.TP
-\fB\-\-log\-file\fR=\fIFILE\fR
-Log to the given file.
 .SH "EXIT STATUS"
 0 if the command was successful
 
diff --git a/ipaserver/install/ipa_cacert_manage.py b/ipaserver/install/ipa_cacert_manage.py
index de13ad39397ae5e9b924b0621521e5fc6016c8e6..32ef25c7aac3e57d27955b6a2608adb6a1626019 100644
--- a/ipaserver/install/ipa_cacert_manage.py
+++ b/ipaserver/install/ipa_cacert_manage.py
@@ -35,7 +35,7 @@ from ipaserver.install import certs, cainstance, installutils
 class CACertManage(admintool.AdminTool):
 command_name = 'ipa-cacert-manage'
 
-usage = "%prog {renew|install} [options]"
+usage = "%prog renew [options]\n%prog install [options] CERTFILE"
 
 description = "Manage CA certificates."
 
-- 
2.7.4

-- 
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 0058] Make get_entries not ignore its size_limit argument

2016-07-14 Thread Stanislav Laznicka


Hello,

This patch fixes https://fedorahosted.org/freeipa/ticket/5640.

With not so much experience with the framework, it raises question in my 
head whether ipaldap.get_entries is used properly throughout the system 
- does it always assume that it gets ALL the requested entries or just a 
few of those as configured by the 'ipaSearchRecordsLimit' attribute of 
ipaConfig.etc which it actually gets?


One spot that I know the get_entries method was definitely not used 
properly before this patch is in the 
baseldap.LDAPObject.get_memberindirect() method:


 692 result = self.backend.get_entries(
 693 self.api.env.basedn,
 694 filter=filter,
 695 attrs_list=['member'],
 696 size_limit=-1, # paged search will get everything 
anyway

 697 paged_search=True)

which to me seems kind of important if the environment size_limit is not 
set properly :) The patch does not fix the non-propagation of the 
paged_search, though.


Cheers,
Standa
From f76d301b418219b61a571e12ad7404eaf91a5046 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 14 Jul 2016 13:53:56 +0200
Subject: [PATCH] Make get_entries() not ignore size_limit argument

The permission_find command would in some cases ignore
the sizelimit parameter passed to it. This was caused
by the ipaldap.get_entries() method not passing one of its
parameters further down.

https://fedorahosted.org/freeipa/ticket/5640
---
 ipapython/ipaldap.py| 5 +++--
 ipaserver/plugins/permission.py | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 704e71a9471c27430328a8c7c6a319aa72a9d482..74d985e8546ad2553bdac5a61da7df8acb6a0923 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -1283,7 +1283,7 @@ class LDAPClient(object):
 return cls.combine_filters(flts, rules)
 
 def get_entries(self, base_dn, scope=ldap.SCOPE_SUBTREE, filter=None,
-attrs_list=None, **kwargs):
+attrs_list=None, size_limit=None, **kwargs):
 """Return a list of matching entries.
 
 :raises: errors.LimitsExceeded if the list is truncated by the server
@@ -1298,7 +1298,8 @@ class LDAPClient(object):
 for their description.
 """
 entries, truncated = self.find_entries(
-base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list)
+base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list,
+size_limit=size_limit)
 try:
 self.handle_truncated_result(truncated)
 except errors.LimitsExceeded as e:
diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py
index 830773ae7a09f0197da702e4ec31b0b58f1214dd..e05fc0030c62583e0bcd495c22045274cae14660 100644
--- a/ipaserver/plugins/permission.py
+++ b/ipaserver/plugins/permission.py
@@ -1308,7 +1308,7 @@ class permission_find(baseldap.LDAPSearch):
 legacy_entries = ldap.get_entries(
 base_dn=DN(self.obj.container_dn, self.api.env.basedn),
 filter=ldap.combine_filters(filters, rules=ldap.MATCH_ALL),
-attrs_list=attrs_list)
+attrs_list=attrs_list, size_limit=max_entries)
 # Retrieve the root entry (with all legacy ACIs) at once
 root_entry = ldap.get_entry(DN(api.env.basedn), ['aci'])
 except errors.NotFound:
-- 
2.7.4

-- 
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 0057] Don't show part of warning containing --force-ntpd in replica install

2016-07-13 Thread Stanislav Laznicka


On 07/13/2016 09:51 AM, Petr Vobornik wrote:

On 07/13/2016 08:26 AM, Stanislav Laznicka wrote:

On 07/12/2016 08:44 AM, Stanislav Laznicka wrote:

On 07/11/2016 04:27 PM, Petr Vobornik wrote:

On 07/11/2016 01:23 PM, Stanislav Laznicka wrote:

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




Isn't the bug about something else?

The issue was that ipa-replica-install doesn't have --force-ntpd option.
It is an option of ipa-client-install which is run from replica
installer.

The unattended mode is unrelated.

My understanding is that the bug says that '--force-ntpd' option
should not be shown when ipa-client-install is run during replica
installation.

During replica installation, the ipa-client-install script is run with
the '--unattended' flag in the 'ensure_enrolled()' function. Being a
separate script, there's not many options on how to pass the
information not to show the message to ipa-client-install. Using the
already used flag to get rid of the message seemed easiest to me.
Introducing a new 'hidden' flag (like '--from-replica'), on the other
hand, seems a bit harsh.


Just to throw it out there - it's possible that the '--force-join'
client option would also appear as a hint from the client install script
(during replica installation). Should this also be muted somehow? To me,
it seems reasonable to rather add it as an argument to
ipa-replica-install to pass it to the client install script.


IMO client installation initiated from replica needs to have a special
option(hidden in help) similar to --on-server (or what's its name). E.g.
the name can be --replica-install. Maybe --on-server can be used but it
may have other implication which might not be valid for this use case.

Anything else are just workarounds. Imagine that admin runs
ipa-client-install with --unattended or --force-join. He would then not
get the message as now.


The --on-master option won't do here as it seems that the client would 
require some IPA pre-configuration for successful install. A new option 
will have to be created, then.


As I was trying to point out, the situation about --force-join is a bit 
different. The option again would be shown and is not available in 
ipa-replica-install. I think it should be available to allow direct 
replica installation even when previous installation failed/left some 
mess on the master (ofc the user could run `ipa-replica-manage del 
 --cleanup` on the master instead).


--
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 0057] Don't show part of warning containing --force-ntpd in replica install

2016-07-13 Thread Stanislav Laznicka


On 07/12/2016 08:44 AM, Stanislav Laznicka wrote:

On 07/11/2016 04:27 PM, Petr Vobornik wrote:

On 07/11/2016 01:23 PM, Stanislav Laznicka wrote:

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




Isn't the bug about something else?

The issue was that ipa-replica-install doesn't have --force-ntpd option.
It is an option of ipa-client-install which is run from replica 
installer.


The unattended mode is unrelated.


My understanding is that the bug says that '--force-ntpd' option 
should not be shown when ipa-client-install is run during replica 
installation.


During replica installation, the ipa-client-install script is run with 
the '--unattended' flag in the 'ensure_enrolled()' function. Being a 
separate script, there's not many options on how to pass the 
information not to show the message to ipa-client-install. Using the 
already used flag to get rid of the message seemed easiest to me. 
Introducing a new 'hidden' flag (like '--from-replica'), on the other 
hand, seems a bit harsh.


Just to throw it out there - it's possible that the '--force-join' 
client option would also appear as a hint from the client install script 
(during replica installation). Should this also be muted somehow? To me, 
it seems reasonable to rather add it as an argument to 
ipa-replica-install to pass it to the client install script.


--
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 0184] vault-add: set the default vault type on the client side if none was given


On 07/12/2016 02:10 PM, Martin Babinsky wrote:

Quick fix for https://fedorahosted.org/freeipa/ticket/6047

Note that it depends on mbasti's patch 552 (already acked) otherwise 
client-side vault commands would not be even visible in CLI.




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 0550] host-find: do not show SSH keys by default


On 07/08/2016 01:52 PM, Martin Basti wrote:

Reproducible only with 2+ hosts, patch attached.

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



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 0182] ipa-compat-manage: use server API to retrieve plugin statu


On 07/12/2016 10:02 AM, Stanislav Laznicka wrote:

On 07/11/2016 10:50 AM, Martin Babinsky wrote:
Fixes regression reported in 
https://fedorahosted.org/freeipa/ticket/6033



Hello,

The ticket is rather cryptic as it has ipa-compat-manage in header but 
describes error in ipa-nis-manage. Both scripts suffer with the very 
same error, could you fix the latter as well?


Thanks,
Standa


Never mind, found the ipa-nis-manage patch posted earlier in the mailing 
list ACKed although it probably hasn't been pushed yet.


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 0182] ipa-compat-manage: use server API to retrieve plugin statu


On 07/11/2016 10:50 AM, Martin Babinsky wrote:

Fixes regression reported in https://fedorahosted.org/freeipa/ticket/6033




Hello,

The ticket is rather cryptic as it has ipa-compat-manage in header but 
describes error in ipa-nis-manage. Both scripts suffer with the very 
same error, could you fix the latter as well?


Thanks,
Standa
-- 
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 0057] Don't show part of warning containing --force-ntpd in replica install


On 07/11/2016 04:27 PM, Petr Vobornik wrote:

On 07/11/2016 01:23 PM, Stanislav Laznicka wrote:

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




Isn't the bug about something else?

The issue was that ipa-replica-install doesn't have --force-ntpd option.
It is an option of ipa-client-install which is run from replica installer.

The unattended mode is unrelated.


My understanding is that the bug says that '--force-ntpd' option should 
not be shown when ipa-client-install is run during replica installation.


During replica installation, the ipa-client-install script is run with 
the '--unattended' flag in the 'ensure_enrolled()' function. Being a 
separate script, there's not many options on how to pass the information 
not to show the message to ipa-client-install. Using the already used 
flag to get rid of the message seemed easiest to me. Introducing a new 
'hidden' flag (like '--from-replica'), on the other hand, seems a bit harsh.


--
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 0183] ipa-advise: correct handling of plugin namespace iteration

2016-07-11 Thread Stanislav Laznicka


On 07/11/2016 02:18 PM, Martin Babinsky wrote:

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




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

[Freeipa-devel] [PATCH 0057] Don't show part of warning containing --force-ntpd in replica install

2016-07-11 Thread Stanislav Laznicka


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

From b35b2154984df77bdaf08bc82c227ea9f801ed00 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Mon, 11 Jul 2016 13:17:32 +0200
Subject: [PATCH] Don't show --force-ntpd option in replica install

Don't show the warning message part about --force-ntpd when
ipa-client-install is run from replica installation (the client install
is run in unattended mode).

https://fedorahosted.org/freeipa/ticket/6046
---
 client/ipa-client-install | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 8546ff8b0dbea0f28fba12a00c2ee1868ec7c3c6..7ef8c0767a2a7700470b726def90ed438f53 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -2241,11 +2241,13 @@ def install(options, env, fstore, statestore):
 try:
 ipaclient.ntpconf.check_timedate_services()
 except ipaclient.ntpconf.NTPConflictingService as e:
-print("WARNING: ntpd time synchronization service will not" \
-  " be configured as")
-print("conflicting service (%s) is enabled" % e.conflicting_service)
-print("Use --force-ntpd option to disable it and force configuration" \
-  " of ntpd")
+print(
+"WARNING: ntpd time synchronization service will not"
+" be configured as conflicting service (%s) is enabled"
+% e.conflicting_service)
+if not options.unattended:
+print("Use --force-ntpd option to disable it and force "
+  "configuration of ntpd")
 print("")
 
 # configuration of ntpd is disabled in this case
-- 
2.7.4

-- 
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 0056] removed unused parameter from migrate-ds

2016-07-11 Thread Stanislav Laznicka


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

From 1bd10577189ee3037244a51edc1af82bde05806e Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Mon, 11 Jul 2016 12:31:39 +0200
Subject: [PATCH] Removed unused method parameter from migrate-ds

An extra parameter on client side command override of migrate-ds output
was causing errors.

https://fedorahosted.org/freeipa/ticket/6034
---
 ipaclient/plugins/migration.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaclient/plugins/migration.py b/ipaclient/plugins/migration.py
index 8ac5f66bf1440b245c1268cd97d5a3e0dc2e6226..cf8d461bfa144f1287ef36a231f553fd9cd102b3 100644
--- a/ipaclient/plugins/migration.py
+++ b/ipaclient/plugins/migration.py
@@ -50,7 +50,7 @@ can use their Kerberos accounts.''')
 option = option.clone_retype(option.name, File)
 yield option
 
-def output_for_cli(self, textui, result, ldapuri, bindpw, **options):
+def output_for_cli(self, textui, result, ldapuri, **options):
 textui.print_name(self.name)
 if not result['enabled']:
 textui.print_plain(self.migration_disabled_msg)
-- 
2.7.4

-- 
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] 0070..0071 Fix replica installation from IPA v4.2

2016-07-01 Thread Stanislav Laznicka


On 06/17/2016 08:59 AM, Fraser Tweedale wrote:

The attached patches fix
https://fedorahosted.org/freeipa/ticket/5963

Thanks Milan for reporting.

Cheers,
Fraser

Tried this patch on 4.4 with domain level set to 0 and it does fix the 
issue for me so ACK for 4.4.


Not sure if this is going to be backported but if so, both patches will 
need modifications for 4.2 and the latter patch will require 
modifications for 4.3 for them to apply.


--
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 0144] Fix `Conflicts` with ipa-python

2016-06-30 Thread Stanislav Laznicka


On 06/29/2016 02:36 PM, Petr Spacek wrote:

Hello,

Fix `Conflicts` with ipa-python

The conflicts should have constant version in it because it is related
to package split.

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


I've tested the same change in RHEL 7.2->7.3 upgrade and it worked just fine.
Upgrade from IPA 4.3.1 to master on Fedora 24 worked just fine, too.


ACK, worked for me as well.

--
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 0545] cert.py: split doctring to multiple ugettext strings

2016-06-30 Thread Stanislav Laznicka


On 06/30/2016 01:08 PM, Martin Basti wrote:



On 30.06.2016 10:13, Stanislav Laznicka wrote:

On 06/30/2016 09:18 AM, Martin Basti wrote:
Make life of translators easier, there was recent change in cert.py 
docstring, so they have to translate the whole docstring again, so 
I'm splitting it to multiple parts.



Patch attached

I'm not sure whether the "See RFC 5280 for more details" should be 
split from the link.

It doesn't matter
Also, you need to add "\n" at the end of each reason string (you're 
not using multiline strings there).

Fixed, patch attached


Looks good to me, 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 0545] cert.py: split doctring to multiple ugettext strings

2016-06-30 Thread Stanislav Laznicka


On 06/30/2016 09:18 AM, Martin Basti wrote:
Make life of translators easier, there was recent change in cert.py 
docstring, so they have to translate the whole docstring again, so I'm 
splitting it to multiple parts.



Patch attached

I'm not sure whether the "See RFC 5280 for more details" should be split 
from the link. Also, you need to add "\n" at the end of each reason 
string (you're not using multiline strings there).


--
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 0096] Add authentication indicators support to Host objects


On 06/29/2016 04:02 PM, Stanislav Laznicka wrote:

On 06/29/2016 03:53 PM, Martin Basti wrote:



On 29.06.2016 15:52, Stanislav Laznicka wrote:

On 06/24/2016 03:14 PM, Martin Basti wrote:



On 24.06.2016 15:11, Sumit Bose wrote:

On Tue, Jun 21, 2016 at 02:25:49PM -0400, Nathaniel McCallum wrote:

https://fedorahosted.org/freeipa/ticket/433
The patch works for me as expected, but the API.txt update is 
missing in

the patch.

bye,
Sumit


There are no updated managed permissions for krbprincipalauthind 
attribute in hosts.py, is this omitted on purpose?

Martin^2


The attached patch adds them should these be required.




Then we also needs patch for services.py, because there are missing 
ACIs too


Martin^2


It was already included but let me separate it in two patches, then.



Good catch from Petr Vobornik - the rebuilt ACI.txt should also be included.

From 9a80066123e8e97fb9c9daed4f339a5d5368faf3 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 29 Jun 2016 15:56:55 +0200
Subject: [PATCH 1/2] host: Added permissions for auth. indicators read/modify

Added permissions for Kerberos authentication indicators reading and
modifying to host objects.

https://fedorahosted.org/freeipa/ticket/433
---
 ACI.txt   | 4 ++--
 ipaserver/plugins/host.py | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 98566de..86955c5 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -137,13 +137,13 @@ aci: (targetattr = "createtimestamp || entryusn || ipaallowedtoperform;read_keys
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "ipasshpubkey")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Manage Host SSH Public Keys";allow (write) groupdn = "ldap:///cn=System: Manage Host SSH Public Keys,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "description || ipaassignedidview || l || macaddress || nshardwareplatform || nshostlocation || nsosversion || userclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Modify Hosts";allow (write) groupdn = "ldap:///cn=System: Modify Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+aci: (targetattr = "description || ipaassignedidview || krbprincipalauthind || l || macaddress || nshardwareplatform || nshostlocation || nsosversion || userclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Modify Hosts";allow (write) groupdn = "ldap:///cn=System: Modify Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || macaddress || modifytimestamp || objectclass")(target = "ldap:///cn=computers,cn=compat,dc=ipa,dc=example;)(version 3.0;acl "permission:System: Read Host Compat Tree";allow (compare,read,search) userdn = "ldap:///anyone;;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "memberof")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Read Host Membership";allow (compare,read,search) userdn = "ldap:///all;;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "cn || createtimestamp || description || enrolledby || entryusn || fqdn || ipaassignedidview || ipaclientversion || ipakrbauthzdata || ipasshpubkey || ipauniqueid || krbcanonicalname || krblastpwdchange || krbpasswordexpiration || krbprincipalaliases || krbprincipalexpiration || krbprincipalname || l || macaddress || managedby || modifytimestamp || nshardwareplatform || nshostlocation || nsosversion || objectclass || serverhostname || usercertificate || userclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Read Hosts";allow (compare,read,search) userdn = "ldap:///all;;)
+aci: (targetattr = "cn || createtimestamp || description || enrolledby || entryusn || fqdn || ipaassignedidview || ipaclientversion || ipakrbauthzdata || ipasshpubkey || ipauniqueid || krbcanonicalname || krblastpwdchange || krbpasswordexpiration || krbprincipalaliases || krbprincipalauthind || krbprincipalexpiration || krbprincipalname || l || macaddress || managedby || modifytimestamp || nshardwareplatform || nshostlocation || nsosversion || objectclass || serverhostname || usercertificate || userclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Read Hosts";allow (compare,read,search) userdn = "ldap:///all;;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Remove Hosts";allow (delete) groupdn = "ldap:///cn=System: Remove Hosts,cn=p

Re: [Freeipa-devel] [PATCH 0096] Add authentication indicators support to Host objects


On 06/29/2016 03:53 PM, Martin Basti wrote:



On 29.06.2016 15:52, Stanislav Laznicka wrote:

On 06/24/2016 03:14 PM, Martin Basti wrote:



On 24.06.2016 15:11, Sumit Bose wrote:

On Tue, Jun 21, 2016 at 02:25:49PM -0400, Nathaniel McCallum wrote:

https://fedorahosted.org/freeipa/ticket/433
The patch works for me as expected, but the API.txt update is 
missing in

the patch.

bye,
Sumit


There are no updated managed permissions for krbprincipalauthind 
attribute in hosts.py, is this omitted on purpose?

Martin^2


The attached patch adds them should these be required.




Then we also needs patch for services.py, because there are missing 
ACIs too


Martin^2


It was already included but let me separate it in two patches, then.

From d05969e29aa190602ae9f90c6e6161e517b0ad0d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 29 Jun 2016 15:56:55 +0200
Subject: [PATCH 1/2] host: Added permissions for auth. indicators read/modify

Added permissions for Kerberos authentication indicators reading and
modifying to host objects.

https://fedorahosted.org/freeipa/ticket/433
---
 ipaserver/plugins/host.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 0072431de3f130d09066100f12d9fcb34e9fb96b..c54439e9b55de85d871241083ccb512cc1a88f29 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -333,7 +333,7 @@ class host(LDAPObject):
 'enrolledby', 'managedby', 'ipaassignedidview',
 'krbprincipalname', 'krbcanonicalname', 'krbprincipalaliases',
 'krbprincipalexpiration', 'krbpasswordexpiration',
-'krblastpwdchange',
+'krblastpwdchange', 'krbprincipalauthind',
 },
 },
 'System: Read Host Membership': {
@@ -411,6 +411,7 @@ class host(LDAPObject):
 'ipapermdefaultattr': {
 'description', 'l', 'nshardwareplatform', 'nshostlocation',
 'nsosversion', 'macaddress', 'userclass', 'ipaassignedidview',
+'krbprincipalauthind',
 },
 'replaces': [
 '(targetattr = "description || l || nshostlocation || nshardwareplatform || nsosversion")(target = "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX;)(version 3.0;acl "permission:Modify Hosts";allow (write) groupdn = "ldap:///cn=Modify Hosts,cn=permissions,cn=pbac,$SUFFIX";)',
-- 
2.5.5

From 3a503b91680b49afc5bc0ba39ec5451f5b0352a1 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 29 Jun 2016 15:58:07 +0200
Subject: [PATCH 2/2] service: Added permissions for auth. indicators
 read/modify

Added permissions for Kerberos authentication indicators reading and
modifying to service objects.
---
 ipaserver/plugins/service.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 701314f8d9f2ac14c2b92fea1b75c7bf1754dac3..bc5bf529b45568d63e2a5b99906a7755d4ac8d40 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -437,7 +437,7 @@ class service(LDAPObject):
 'krbprincipalname', 'krbcanonicalname', 'krbprincipalaliases',
 'krbprincipalexpiration', 'krbpasswordexpiration',
 'krblastpwdchange', 'ipakrbauthzdata', 'ipakrbprincipalalias',
-'krbobjectreferences',
+'krbobjectreferences', 'krbprincipalauthind',
 },
 },
 'System: Add Services': {
@@ -465,7 +465,7 @@ class service(LDAPObject):
 },
 'System: Modify Services': {
 'ipapermright': {'write'},
-'ipapermdefaultattr': {'usercertificate'},
+'ipapermdefaultattr': {'usercertificate', 'krbprincipalauthind'},
 'replaces': [
 '(targetattr = "usercertificate")(target = "ldap:///krbprincipalname=*,cn=services,cn=accounts,$SUFFIX;)(version 3.0;acl "permission:Modify Services";allow (write) groupdn = "ldap:///cn=Modify Services,cn=permissions,cn=pbac,$SUFFIX";)',
 ],
-- 
2.5.5

-- 
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 0096] Add authentication indicators support to Host objects


On 06/24/2016 03:14 PM, Martin Basti wrote:



On 24.06.2016 15:11, Sumit Bose wrote:

On Tue, Jun 21, 2016 at 02:25:49PM -0400, Nathaniel McCallum wrote:

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

The patch works for me as expected, but the API.txt update is missing in
the patch.

bye,
Sumit


There are no updated managed permissions for krbprincipalauthind 
attribute in hosts.py, is this omitted on purpose?

Martin^2


The attached patch adds them should these be required.


From becd1e2d284dcd98a2ba35fcd68e0f9354f0a365 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 29 Jun 2016 15:45:28 +0200
Subject: [PATCH] Added permissions for auth. indicators read/modify

Added permissions for Kerberos authentication indicators reading and
modifying to host and service objects.

https://fedorahosted.org/freeipa/ticket/433
---
 ipaserver/plugins/host.py| 3 ++-
 ipaserver/plugins/service.py | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 1091f85748d675c479285ad73465aa9541c61b45..be4a1711f3d6b7ee3bc12cbee1c705a9067f73b2 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -333,7 +333,7 @@ class host(LDAPObject):
 'enrolledby', 'managedby', 'ipaassignedidview',
 'krbprincipalname', 'krbcanonicalname', 'krbprincipalaliases',
 'krbprincipalexpiration', 'krbpasswordexpiration',
-'krblastpwdchange',
+'krblastpwdchange', 'krbprincipalauthind',
 },
 },
 'System: Read Host Membership': {
@@ -411,6 +411,7 @@ class host(LDAPObject):
 'ipapermdefaultattr': {
 'description', 'l', 'nshardwareplatform', 'nshostlocation',
 'nsosversion', 'macaddress', 'userclass', 'ipaassignedidview',
+'krbprincipalauthind',
 },
 'replaces': [
 '(targetattr = "description || l || nshostlocation || nshardwareplatform || nsosversion")(target = "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX;)(version 3.0;acl "permission:Modify Hosts";allow (write) groupdn = "ldap:///cn=Modify Hosts,cn=permissions,cn=pbac,$SUFFIX";)',
diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 701314f8d9f2ac14c2b92fea1b75c7bf1754dac3..bc5bf529b45568d63e2a5b99906a7755d4ac8d40 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -437,7 +437,7 @@ class service(LDAPObject):
 'krbprincipalname', 'krbcanonicalname', 'krbprincipalaliases',
 'krbprincipalexpiration', 'krbpasswordexpiration',
 'krblastpwdchange', 'ipakrbauthzdata', 'ipakrbprincipalalias',
-'krbobjectreferences',
+'krbobjectreferences', 'krbprincipalauthind',
 },
 },
 'System: Add Services': {
@@ -465,7 +465,7 @@ class service(LDAPObject):
 },
 'System: Modify Services': {
 'ipapermright': {'write'},
-'ipapermdefaultattr': {'usercertificate'},
+'ipapermdefaultattr': {'usercertificate', 'krbprincipalauthind'},
 'replaces': [
 '(targetattr = "usercertificate")(target = "ldap:///krbprincipalname=*,cn=services,cn=accounts,$SUFFIX;)(version 3.0;acl "permission:Modify Services";allow (write) groupdn = "ldap:///cn=Modify Services,cn=permissions,cn=pbac,$SUFFIX";)',
 ],
-- 
2.5.5

-- 
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 Fix ipa-server-certinstall with certs signed by 3rd-party CA


On 06/22/2016 09:29 PM, Florence Blanc-Renaud wrote:

Hi,

This patch fixes ipa-server-certinstall when used with 3rd-party certs.
The scenario is the following:
- install the server with an embedded CA
- use ipa-cacert-manage to install a 3rd party CA
- use ipa-certupdate to put the 3rd party CA cert in the relevant NSS 
databases (/etc/ipa/nssdb /etc/httpd/alias /etc/pki/pki-tomcat/alias 
and /etc/dirsrv/slapd-XXX)
- use ipa-server-certinstall to replace the Directory/Apache server 
certificates with a cert signed by the 3rd party CA.


Note that I had to run ipa-certupdate after putting selinux mode to 
permissive (otherwise the cert does not get into 
/etc/pki/pki-tomcat/alias) and a bz has been opened against 
selinux-policy to solve this issue.


https://fedorahosted.org/freeipa/ticket/4785
https://fedorahosted.org/freeipa/ticket/4786



Hello,

The patch works as expected with the selinux requirement you mentioned. 
I will just add Honza for code sanity check. Therefore conditional ACK 
if the code can take no further improvements.


Standa

-- 
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 0042] Removed dead code from LDAPRemoveReverseMember


On 06/28/2016 10:34 AM, Stanislav Laznicka wrote:

On 06/17/2016 09:14 AM, Stanislav Laznicka wrote:

On 06/14/2016 04:40 PM, Jan Cholasta wrote:

On 14.6.2016 16:35, Martin Basti wrote:

On 14.06.2016 16:37, Jan Cholasta wrote:

On 14.6.2016 16:29, Martin Basti wrote:

On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:

On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:

On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:

On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:

On 03.06.2016 14:13, Stanislav Laznicka wrote:

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


NACK

please remove it from LDAPAddReverseMember too, it 
contains the

same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that
every
reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and 
I don't

feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no 
point in
postponing it. I see no reason to be really afraid, I'm pretty 
sure
that removing the objectclass attribute (which is invisible in 
the
CLI anyway) from the output of all the 4 commands that use 
this code

won't break anything.



Ok

It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options 
should make

it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code 
that

was removed, and as you can see Standa had add extra objectclass
attribute there


So the assumption here is that if it's in git history, it's not a 
bug?


The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)

Right. I added the objectclass so that it behaves similar to what the 
other commands do and so that it does not break tests but it can be 
removed and tests could be fixed. The question here is - do we want 
it in 4.4, then? If so can we be sure it won't break anything 
(although we know that it's broken as it is, and it's been like that 
for the past 4 years)?


Currently, the LDAP*ReverseMember are used in adding/removing 
permissions/privileges to privileges/roles so I think we don't want 
it to go bad - but could it?


After discussion with Honza we agreed that the patch should not break 
anything while trying to fix the bug. Attached is the new patch with 
fixed tests.



Hopefully last modification - attrs_list should not be updated by 
received options as these don't reflect the entry's actual attributes.
From 29c4db5e6696ae1885a458914fad3f251b0a4e8a Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.
Also had to fix some tests as objectClass will not be returned by
default now.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py  | 4 ++--
 ipatests/test_xmlrpc/test_permission_plugin.py | 4 
 ipatests/test_xmlrpc/test_role_plugin.py   | 5 -
 3 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..c35f660c790290aa0449ea2441a4fd0f5baed880 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2159,7 +2159,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list)
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
@@ -2258,7 +2258,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list)
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
diff

[Freeipa-devel] [PATCH 0053] Fix wrong imports in copy-schema-to-ca

2016-06-28 Thread Stanislav Laznicka


Hello,

The attached patch fixes wrong imports in copy-schema-to-ca.py script.

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

From 8c72d5257f0643ca486f6c7a2649123a72e5ceb2 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 28 Jun 2016 14:37:34 +0200
Subject: [PATCH] Fix wrong imports in copy-schema-to-ca.py

Some imports were not possible in old versions of IPA. This caused
import exceptions on the script start.

https://fedorahosted.org/freeipa/ticket/6003
---
 install/share/copy-schema-to-ca.py | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/install/share/copy-schema-to-ca.py b/install/share/copy-schema-to-ca.py
index c2f070aa29b7abf1cb32c46020ae80450cfd5080..6cbe3ddec42ce89e6a8ddecaf0a0f561cb0175d0 100755
--- a/install/share/copy-schema-to-ca.py
+++ b/install/share/copy-schema-to-ca.py
@@ -21,7 +21,17 @@ from ipapython import ipautil
 from ipapython.ipa_log_manager import root_logger, standard_logging_setup
 from ipaserver.install.dsinstance import schema_dirname
 from ipalib import api
-from ipaplatform.constants import constants
+
+try:
+# BE CAREFUL when using the constants module - you need to define all
+# the constants separately because of old IPA installations
+from ipaplatform.constants import constants
+PKI_USER = constants.PKI_USER
+DS_USER = constants.DS_USER
+except ImportError:
+# oh dear, this is an old IPA (3.0+)
+from ipaserver.install.dsinstance import DS_USER   #pylint: disable=E0611
+from ipaserver.install.cainstance import PKI_USER  #pylint: disable=E0611
 
 try:
 from ipaplatform import services
@@ -52,8 +62,8 @@ def _sha1_file(filename):
 def add_ca_schema():
 """Copy IPA schema files into the CA DS instance
 """
-pki_pent = pwd.getpwnam(constants.PKI_USER)
-ds_pent = pwd.getpwnam(constants.DS_USER)
+pki_pent = pwd.getpwnam(PKI_USER)
+ds_pent = pwd.getpwnam(DS_USER)
 for schema_fname in SCHEMA_FILENAMES:
 source_fname = os.path.join(ipautil.SHARE_DIR, schema_fname)
 target_fname = os.path.join(schema_dirname(SERVERID), schema_fname)
-- 
2.5.5

-- 
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 0042] Removed dead code from LDAPRemoveReverseMember

2016-06-28 Thread Stanislav Laznicka


On 06/17/2016 09:14 AM, Stanislav Laznicka wrote:

On 06/14/2016 04:40 PM, Jan Cholasta wrote:

On 14.6.2016 16:35, Martin Basti wrote:

On 14.06.2016 16:37, Jan Cholasta wrote:

On 14.6.2016 16:29, Martin Basti wrote:

On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:

On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:

On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:

On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:

On 03.06.2016 14:13, Stanislav Laznicka wrote:

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


NACK

please remove it from LDAPAddReverseMember too, it 
contains the

same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that
every
reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and I 
don't

feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no 
point in
postponing it. I see no reason to be really afraid, I'm pretty 
sure

that removing the objectclass attribute (which is invisible in the
CLI anyway) from the output of all the 4 commands that use this 
code

won't break anything.



Ok

It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options should 
make

it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code that
was removed, and as you can see Standa had add extra objectclass
attribute there


So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)

Right. I added the objectclass so that it behaves similar to what the 
other commands do and so that it does not break tests but it can be 
removed and tests could be fixed. The question here is - do we want it 
in 4.4, then? If so can we be sure it won't break anything (although 
we know that it's broken as it is, and it's been like that for the 
past 4 years)?


Currently, the LDAP*ReverseMember are used in adding/removing 
permissions/privileges to privileges/roles so I think we don't want it 
to go bad - but could it?


After discussion with Honza we agreed that the patch should not break 
anything while trying to fix the bug. Attached is the new patch with 
fixed tests.


From 821960d8e84f2f4e075af93321398ea62af2e206 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.
Also had to fix some tests as objectClass will not be returned by
default now.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py  | 8 ++--
 ipatests/test_xmlrpc/test_permission_plugin.py | 4 
 ipatests/test_xmlrpc/test_role_plugin.py   | 5 -
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..6fc56e0f8218a2ace30bb858b91b365490b5d051 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2126,6 +2126,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 result = self.api.Command[self.show_command](keys[-1])['result']
 dn = result['dn']
 assert isinstance(dn, DN)
+entry_attrs = ldap.make_entry(dn, self.args_options_2_entry(**options))
 
 for callback in self.get_callbacks('pre'):
 dn = callback(self, ldap, dn, *keys, **options)
@@ -2135,6 +2136,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 attrs_list = ['*'] + self.obj.default_attributes
 else:
 attrs_list = set(self.obj.default_attributes)
+attrs_list.update(entry_attrs.keys())
 if options.get('no_members', False):
 attrs_list.difference_update(self.obj.attribute_members)
 attrs_list = list(attrs_list)
@@ -2159,7 +2161,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started


On 06/24/2016 04:04 PM, Martin Basti wrote:

On 24.06.2016 15:50, Stanislav Laznicka wrote:

On 06/21/2016 04:39 PM, Martin Basti wrote:



On 14.06.2016 17:26, Stanislav Laznicka wrote:

-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': 
{err}"

+ .format(svcname=signerd_service.service_name,
+  err=e))


why is signerd so special?

Martin^2


From ODSExporterInstance.uninstall():

signerd_service = services.knownservices.ods_signerd

This means that signerd_service here is not an instance of the 
service.Service class or of its child class but is rather an instance 
of the RedHatService class, a child class of the 
services.SystemdService class. Thus it has to be treated with special 
care.




Well then I prefer to put this option to systemdservice, and only pass 
it from service.restart() to systemdService.restart()


You forgot to handle regular_named service, which is the same as 
ods_signerd


The passing of the option is not as easy as it may seem. The 
service.restart() method calls self.service.restart(). However, 
self.service is not directly of SystemdService class but rather of one 
of RedHat*Service classes, some of which override the 
SystemdService.restart() method (and call it somewhere in the process of 
their overridden method).


Would you then suggest:
1) Adding the option to all the methods on the way from Service to 
SystemdService classes just for the sake of passing => exceptions still 
may occur and suppress_errors option therefore does not do what you'd 
expect it to do
2) Adding the option to all the methods and treat all the possible 
exceptions on the way with a try: .. except: .. blocks ===> veeery ugly 
copy-pasta.
3) Leaving it as is => some .restart() calls offer suppress_errors 
option, some don't.


--
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 0043] Stop uninstaller from failing if a service can't be started


On 06/21/2016 04:39 PM, Martin Basti wrote:



On 14.06.2016 17:26, Stanislav Laznicka wrote:

-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': {err}"
+ .format(svcname=signerd_service.service_name,
+  err=e))


why is signerd so special?

Martin^2


From ODSExporterInstance.uninstall():

signerd_service = services.knownservices.ods_signerd

This means that signerd_service here is not an instance of the 
service.Service class or of its child class but is rather an instance of 
the RedHatService class, a child class of the services.SystemdService 
class. Thus it has to be treated with special care.


--
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 0052] Added missing nsSystemIndex attributes to .update file


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

From e177f6377a84691ba1cdb45ff39488d5d8f8f34d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 24 Jun 2016 14:21:01 +0200
Subject: [PATCH] Add missing nsSystemIndex attributes

https://fedorahosted.org/freeipa/ticket/5947
---
 install/updates/20-indices.update | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/install/updates/20-indices.update b/install/updates/20-indices.update
index b05dc6ff8d98469e9cf0025679755c6679225516..445eda5ab6939f21654335ea4dd50d7b2cab008f 100644
--- a/install/updates/20-indices.update
+++ b/install/updates/20-indices.update
@@ -222,6 +222,7 @@ dn: cn=ntUniqueId,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
 default:cn: ntUniqueId
 default:ObjectClass: top
 default:ObjectClass: nsIndex
+default:nsSystemIndex: false
 only:nsIndexType: eq
 only:nsIndexType: pres
 
@@ -229,6 +230,7 @@ dn: cn=ntUserDomainId,cn=index,cn=userRoot,cn=ldbm database,cn=plugins,cn=config
 default:cn: ntUserDomainId
 default:ObjectClass: top
 default:ObjectClass: nsIndex
+default:nsSystemIndex: false
 only:nsIndexType: eq
 only:nsIndexType: pres
 
-- 
2.5.5

-- 
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 0050-0051] Topology fixes for CA suffix


On 06/24/2016 11:52 AM, Martin Babinsky wrote:

On 06/24/2016 11:30 AM, Petr Vobornik wrote:

On 06/23/2016 05:30 PM, Stanislav Laznicka wrote:

On 06/23/2016 04:38 PM, Petr Vobornik wrote:

On 06/23/2016 04:20 PM, Stanislav Laznicka wrote:

Hello,

attached are patches fixing the logic mentioned in
https://fedorahosted.org/freeipa/ticket/5967.



If server supports the suffix can be verified in validate_nodes call
where masters are already fetched.


Thank you for the suggestion, modified patch 50 attached.



Maybe it's just me, but the code is hard to ready. Check the attached
version - speeding up review process.

I've also change the first commit message line it was too generic.





If you intend to use that internal function in other modules, please 
remove the leading underscore from its name. Otherwise pylint/IDEs may 
complain about import of private module member.


Thank you for the review/update. You went the other way around it which 
indeed does seem much more readable. Not sure if you meant to change the 
order of the patches which in order 50 -> 51 would make 50 alone not 
work because of missing import but I did change the order and fixed that.


I also included the objection from Martin in the patches and removed the 
leading underscore from the imported function.


From e0a21a40cd0c8871ebb19fdce4ce1882ad674bed Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 23 Jun 2016 16:07:18 +0200
Subject: [PATCH 1/2] Fix topologysuffix-verify failing connections

topologysuffix-verify would have checked connectivity even between hosts that
are not managed by the given suffix.

https://fedorahosted.org/freeipa/ticket/5967
---
 ipaserver/plugins/topology.py | 4 +++-
 ipaserver/topology.py | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ipaserver/plugins/topology.py b/ipaserver/plugins/topology.py
index c1848f0cc699f84b40be3623e956780d65de8619..0d0b3c0841558c5adce05996fecb297b998bce66 100644
--- a/ipaserver/plugins/topology.py
+++ b/ipaserver/plugins/topology.py
@@ -14,7 +14,8 @@ from ipalib import _, ngettext
 from ipalib import output
 from ipalib.constants import DOMAIN_LEVEL_1
 from ipaserver.topology import (
-create_topology_graph, get_topology_connection_errors)
+create_topology_graph, get_topology_connection_errors,
+map_masters_to_suffixes)
 from ipapython.dn import DN
 
 if six.PY3:
@@ -476,6 +477,7 @@ Checks done:
 
 masters = self.api.Command.server_find(
 '', sizelimit=0, no_members=False)['result']
+masters = map_masters_to_suffixes(masters).get(keys[0], [])
 segments = self.api.Command.topologysegment_find(
 keys[0], sizelimit=0)['result']
 graph = create_topology_graph(masters, segments)
diff --git a/ipaserver/topology.py b/ipaserver/topology.py
index 27c3b29a4d3c2fe477e6e519b4006b3d96f0eeae..385da29a66fb7276c55e9aac5c8c266b897721a7 100644
--- a/ipaserver/topology.py
+++ b/ipaserver/topology.py
@@ -70,7 +70,7 @@ def get_topology_connection_errors(graph):
 return connect_errors
 
 
-def _map_masters_to_suffixes(masters):
+def map_masters_to_suffixes(masters):
 masters_to_suffix = {}
 
 for master in masters:
@@ -97,7 +97,7 @@ def _create_topology_graphs(api_instance):
 masters = api_instance.Command.server_find(
 u'', sizelimit=0, no_members=False)['result']
 
-suffix_to_masters = _map_masters_to_suffixes(masters)
+suffix_to_masters = map_masters_to_suffixes(masters)
 
 topology_graphs = {}
 
-- 
2.5.5

From 9cd466fcc32399907b231250e78523fa305cb68a Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 23 Jun 2016 16:04:04 +0200
Subject: [PATCH 2/2] topo segment-add: validate that both masters support
 target suffix

This patch removes the ability to add segment between hosts where
either does not support the requested suffix.

https://fedorahosted.org/freeipa/ticket/5967
---
 ipaserver/plugins/topology.py | 30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/ipaserver/plugins/topology.py b/ipaserver/plugins/topology.py
index 0d0b3c0841558c5adce05996fecb297b998bce66..4676c48891c7a6b97ec1f97ad9a7c51400daf06c 100644
--- a/ipaserver/plugins/topology.py
+++ b/ipaserver/plugins/topology.py
@@ -43,9 +43,6 @@ B to A. Creation of unidirectional segments is not allowed.
 """) + _("""
 To verify that no server is disconnected in the topology of the given suffix,
 use:
-  ipa topologysuffix-verify $suffix
-""") + _("""
-
 Examples:
   Find all IPA servers:
 ipa server-find
@@ -204,7 +201,7 @@ class topologysegment(LDAPObject):
 ),
 )
 
-def validate_nodes(self, ldap, dn, entry_attrs):
+def validate_nodes(self, ldap, dn, entry_attrs, suffix):
 leftnode = entry_attrs.get('iparepltoposegmentleftnode')
 rightnode = entry_attrs.get('iparepltop

[Freeipa-devel] [PATCH 0050-0051] Topology fixes for CA suffix

2016-06-23 Thread Stanislav Laznicka


Hello,

attached are patches fixing the logic mentioned in 
https://fedorahosted.org/freeipa/ticket/5967.
From 7d833bf0018f4b3e85bae88cbe383568f6d9c3f4 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 23 Jun 2016 16:04:04 +0200
Subject: [PATCH 1/2] Raise exception on incorrect segment addition

This patch removes the ability to add CA segment between hosts where
either is not yet managed by the CA suffix.

https://fedorahosted.org/freeipa/ticket/5967
---
 ipaserver/plugins/topology.py | 37 +
 1 file changed, 37 insertions(+)

diff --git a/ipaserver/plugins/topology.py b/ipaserver/plugins/topology.py
index c1848f0cc699f84b40be3623e956780d65de8619..1c54d187342bfdc830fd650f24bf5720a5135ea1 100644
--- a/ipaserver/plugins/topology.py
+++ b/ipaserver/plugins/topology.py
@@ -89,6 +89,37 @@ def validate_domain_level(api):
 )
 
 
+def validate_ca_connectivity(api, left_node, right_node):
+"""
+Validate whether two nodes may create a CA replication agreement
+"""
+ca_hosts = api.Command.topologysegment_find(u'ca', sizelimit=0)['result']
+left_found = False
+right_found = False
+for ca_host in ca_hosts:
+if (not left_found and left_node in
+(ca_host['iparepltoposegmentleftnode'][0],
+ ca_host['iparepltoposegmentrightnode'][0])):
+left_found = True
+if (not right_found and right_node in
+(ca_host['iparepltoposegmentleftnode'][0],
+ ca_host['iparepltoposegmentrightnode'][0])):
+right_found = True
+
+if left_found and right_found:
+return
+
+invalid_nodes = {}
+if not left_found:
+invalid_nodes['leftnode'] = left_node
+if not right_found:
+invalid_nodes['rightnode'] = right_node
+raise errors.ValidationError(
+name=', '.join(invalid_nodes.keys()),
+error=_("The following nodes are not yet in the 'ca' suffix: {hosts}"
+.format(hosts=', '.join(invalid_nodes.values()
+
+
 @register()
 class topologysegment(LDAPObject):
 """
@@ -266,6 +297,12 @@ class topologysegment_add(LDAPCreate):
 assert isinstance(dn, DN)
 validate_domain_level(self.api)
 self.obj.validate_nodes(ldap, dn, entry_attrs)
+if keys[0] == 'ca':
+validate_ca_connectivity(
+self.api,
+entry_attrs['iparepltoposegmentleftnode'],
+entry_attrs['iparepltoposegmentrightnode']
+)
 return dn
 
 
-- 
2.5.5

From b146c6bec3e99b06ce86d931a22ce038b1807a02 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 23 Jun 2016 16:07:18 +0200
Subject: [PATCH 2/2] Fix topologysuffix-verify failing connections

topologysuffix-verify would have checked connectivity even between hosts that
are not managed by the given suffix.

https://fedorahosted.org/freeipa/ticket/5967
---
 ipaserver/plugins/topology.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipaserver/plugins/topology.py b/ipaserver/plugins/topology.py
index 1c54d187342bfdc830fd650f24bf5720a5135ea1..5cf2927c6da36dbb93b241450c05a0bb87d759aa 100644
--- a/ipaserver/plugins/topology.py
+++ b/ipaserver/plugins/topology.py
@@ -14,7 +14,8 @@ from ipalib import _, ngettext
 from ipalib import output
 from ipalib.constants import DOMAIN_LEVEL_1
 from ipaserver.topology import (
-create_topology_graph, get_topology_connection_errors)
+create_topology_graph, get_topology_connection_errors,
+_map_masters_to_suffixes)
 from ipapython.dn import DN
 
 if six.PY3:
@@ -513,6 +514,7 @@ Checks done:
 
 masters = self.api.Command.server_find(
 '', sizelimit=0, no_members=False)['result']
+masters = _map_masters_to_suffixes(masters).get(keys[0], [])
 segments = self.api.Command.topologysegment_find(
 keys[0], sizelimit=0)['result']
 graph = create_topology_graph(masters, segments)
-- 
2.5.5

-- 
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 0049] Fix host principal password required in ipa-ca-install

2016-06-23 Thread Stanislav Laznicka


On 06/23/2016 08:09 AM, Jan Cholasta wrote:

On 22.6.2016 16:22, Stanislav Laznicka wrote:

Hello,

Please see the patch attached that fixes the issue from
https://fedorahosted.org/freeipa/ticket/5965. The patch took me quite a
while to create as I thought something was wrong with the SshExec class
which actually was where the password was required.


"The nss_db variable didn't go through the proper initialization"

You are going to have to be more specific, because the variable is 
properly initialized right here:


with certdb.NSSDatabase(nss_dir) as nss_db:

And the nss_db.secdir attribute used in the api.bootstrap() call is 
properly initialized in NSSDatabase():


def __init__(self, nssdir=None):
if nssdir is None:
self.secdir = tempfile.mkdtemp()
self._is_temporary = True
else:
self.secdir = nssdir
self._is_temporary = False

You're right, the commit message was rather generic. Hopefully this new 
one will be better.


From 010b809a0e940dc25af9f531b60b5b72d1a48b79 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 22 Jun 2016 16:08:49 +0200
Subject: [PATCH] Fix to ipa-ca-install asking for host principal password

With a ca_cert_file specified in options, the nss_db was used before the
certificates from the file were added to it, which caused an exception
that led to fallback to ssh which is broken.

https://fedorahosted.org/freeipa/ticket/5965
---
 install/tools/ipa-replica-conncheck | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index 991f4e429dd1df7036b4a1c0175ca5daaea521ad..e308b118f20306107bc62eba2a60187fbc52f4fc 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -462,10 +462,6 @@ def main():
 nss_dir = paths.IPA_NSSDB_DIR
 
 with certdb.NSSDatabase(nss_dir) as nss_db:
-api.bootstrap(context='client', xmlrpc_uri=xmlrpc_uri,
-  nss_dir=nss_db.secdir)
-api.finalize()
-
 if options.ca_cert_file:
 nss_dir = nss_db.secdir
 
@@ -483,6 +479,9 @@ def main():
 else:
 nss_dir = None
 
+api.bootstrap(context='client', xmlrpc_uri=xmlrpc_uri,
+  nss_dir=nss_db.secdir)
+api.finalize()
 try:
 api.Backend.rpcclient.connect()
 api.Command.ping()
-- 
2.5.5

-- 
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 0049] Fix host principal password required in ipa-ca-install

2016-06-22 Thread Stanislav Laznicka


Hello,

Please see the patch attached that fixes the issue from 
https://fedorahosted.org/freeipa/ticket/5965. The patch took me quite a 
while to create as I thought something was wrong with the SshExec class 
which actually was where the password was required.


The fact is that should rpcclient connection fail for some other reason 
and the control would fall back to SSH, this will still be broken and 
needs fixing. I will create a ticket for that.


Standa

From 66e49904f7901fbfebcbd1a8b9f397667e89c60b Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 22 Jun 2016 16:08:49 +0200
Subject: [PATCH] Fix to ipa-ca-install asking for host principal password

The nss_db variable didn't go through the proper initialization

https://fedorahosted.org/freeipa/ticket/5965
---
 install/tools/ipa-replica-conncheck | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index 991f4e429dd1df7036b4a1c0175ca5daaea521ad..e308b118f20306107bc62eba2a60187fbc52f4fc 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -462,10 +462,6 @@ def main():
 nss_dir = paths.IPA_NSSDB_DIR
 
 with certdb.NSSDatabase(nss_dir) as nss_db:
-api.bootstrap(context='client', xmlrpc_uri=xmlrpc_uri,
-  nss_dir=nss_db.secdir)
-api.finalize()
-
 if options.ca_cert_file:
 nss_dir = nss_db.secdir
 
@@ -483,6 +479,9 @@ def main():
 else:
 nss_dir = None
 
+api.bootstrap(context='client', xmlrpc_uri=xmlrpc_uri,
+  nss_dir=nss_db.secdir)
+api.finalize()
 try:
 api.Backend.rpcclient.connect()
 api.Command.ping()
-- 
2.5.5

-- 
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 0047] Fix uninitialized variables in replicainstall

2016-06-21 Thread Stanislav Laznicka


On 06/16/2016 10:16 AM, Stanislav Laznicka wrote:

Hello,

There was a possible use of uninitialized variables in replicainstall.



Discard the patch, Martin sent the same patch yesterday but Honza seems 
to have already taken care of it.


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

Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules


On 06/17/2016 01:01 PM, Petr Vobornik wrote:

On 17.6.2016 12:12, Stanislav Laznicka wrote:

On 06/17/2016 09:51 AM, Petr Vobornik wrote:

On 17.6.2016 09:24, Stanislav Laznicka wrote:

On 06/17/2016 08:48 AM, Petr Spacek wrote:

On 17.6.2016 08:43, Stanislav Laznicka wrote:

On 06/17/2016 07:45 AM, Petr Spacek wrote:

On 16.6.2016 17:33, Stanislav Laznicka wrote:

Hello,

This patch removes most sys.exits() from installer modules and
scripts and
replaces them with ScriptError. I only left sys.exits at places
where the user
decides yes/no on continuation of the script.

I wonder if yes/no should be replaced with KeyboardInterrupt or some
other
exception, too...


I'm not sure, it seems more clear to just really exit if the user
desires it
and it's what we say we'll do (with possible cleanup beforehand). Do
you think
we could benefit somehow by raising an exception here?

I'm just thinking out loud.

It seemed to me that it is easier to share cleanup on one except block
instead
of having if (interrupt): cleanup; if (interrupt2): same_cleanup;

etc.

Again, just wondering out loud.


If the cleanup is the same, or similar it might be more beneficial to
have it in a function where you could pass what was set up already and
therefore needs cleanup. But that's just an opinion coming from thinking
out loud as well. I went through the code to see if there's much cleanup
after these user actions and it seems that usually there's nothing much
if anything. However, thinking in advance may save us much trouble in
the future, of course.


Btw the original scope of the ticket is to replace sys.exit calls ONLY
in installer modules. Please don't waste time with debugging other use
cases before 4.4 is out.


I might have gotten carried away a bit. Would you suggest keeping the
sys.exits replaced only in ipaserver/install/server/replicainstall.py,
ipaserver/install/server/install.py modules which are the installer
modules managed by AdminTool? I considered the modules in
ipaserver/install/ to also be installer modules as they are heavily used
during installation and the sys.exits there mainly occur in functions
called from install()/install_check() methods. The *-install scripts
were perhaps really obviously over the scope.

Yes, modules:
  ipaserver/install/bindinstance.py  |  2 +-
  ipaserver/install/ca.py| 19 +++---
  ipaserver/install/cainstance.py|  5 +-
  ipaserver/install/dns.py   |  5 +-
  ipaserver/install/dsinstance.py|  3 +-
  ipaserver/install/installutils.py  | 16 +++---
  ipaserver/install/ipa_ldap_updater.py  |  2 +-
  ipaserver/install/krainstance.py   |  3 +-
  ipaserver/install/replication.py   | 10 ++--
  ipaserver/install/server/install.py| 64 +++--
  ipaserver/install/server/replicainstall.py | 92

not modules:
install/tools/ipa-adtrust-install | 17 +++---
install/tools/ipa-ca-install | 23 
install/tools/ipa-compat-manage | 11 ++--
install/tools/ipa-dns-install | 3 +-


I'll keep the sys.exit replaces that won't make it here on the side, we
may want to do them later.

I'm a bit worried that the patch might change some behavior. Maybe I'm
wrong.


Attached is the patch with correct modules with sys.exits replaced.

I double-checked the changes and I believe the behavior shouldn't really 
change.


From 08648ce78aef1e0868d6fbaffd23ed643fd49486 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 17 Jun 2016 13:14:49 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 ipaserver/install/bindinstance.py  |  2 +-
 ipaserver/install/ca.py| 19 +++---
 ipaserver/install/cainstance.py|  5 +-
 ipaserver/install/dns.py   |  5 +-
 ipaserver/install/dsinstance.py|  3 +-
 ipaserver/install/installutils.py  | 16 +++---
 ipaserver/install/ipa_ldap_updater.py  |  2 +-
 ipaserver/install/krainstance.py   |  3 +-
 ipaserver/install/replication.py   | 10 ++--
 ipaserver/install/server/install.py| 64 +++--
 ipaserver/install/server/replicainstall.py | 92 --
 11 files changed, 118 insertions(+), 103 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 78e75359266bbefe7954242b98922272fb0c9194..c9e528856414371ce2e5e54df3642907c89d2856 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -466,7 +466,7 @@ def check_reverse_zones(ip_addresses, reverse_zones, options, unattended,
 except ValueError as e:
 msg = "Reverse zone %s will not be used: %s" % (rz, e)
 if unattended:
-

Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules


On 06/17/2016 09:51 AM, Petr Vobornik wrote:

On 17.6.2016 09:24, Stanislav Laznicka wrote:

On 06/17/2016 08:48 AM, Petr Spacek wrote:

On 17.6.2016 08:43, Stanislav Laznicka wrote:

On 06/17/2016 07:45 AM, Petr Spacek wrote:

On 16.6.2016 17:33, Stanislav Laznicka wrote:

Hello,

This patch removes most sys.exits() from installer modules and
scripts and
replaces them with ScriptError. I only left sys.exits at places
where the user
decides yes/no on continuation of the script.

I wonder if yes/no should be replaced with KeyboardInterrupt or some
other
exception, too...


I'm not sure, it seems more clear to just really exit if the user
desires it
and it's what we say we'll do (with possible cleanup beforehand). Do
you think
we could benefit somehow by raising an exception here?

I'm just thinking out loud.

It seemed to me that it is easier to share cleanup on one except block
instead
of having if (interrupt): cleanup; if (interrupt2): same_cleanup;

etc.

Again, just wondering out loud.


If the cleanup is the same, or similar it might be more beneficial to
have it in a function where you could pass what was set up already and
therefore needs cleanup. But that's just an opinion coming from thinking
out loud as well. I went through the code to see if there's much cleanup
after these user actions and it seems that usually there's nothing much
if anything. However, thinking in advance may save us much trouble in
the future, of course.


Btw the original scope of the ticket is to replace sys.exit calls ONLY
in installer modules. Please don't waste time with debugging other use
cases before 4.4 is out.

I might have gotten carried away a bit. Would you suggest keeping the 
sys.exits replaced only in ipaserver/install/server/replicainstall.py, 
ipaserver/install/server/install.py modules which are the installer 
modules managed by AdminTool? I considered the modules in 
ipaserver/install/ to also be installer modules as they are heavily used 
during installation and the sys.exits there mainly occur in functions 
called from install()/install_check() methods. The *-install scripts 
were perhaps really obviously over the scope.


I'll keep the sys.exit replaces that won't make it here on the side, we 
may want to do them later.


--
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 0048] Remove sys.exit() from installer modules


On 06/17/2016 08:48 AM, Petr Spacek wrote:

On 17.6.2016 08:43, Stanislav Laznicka wrote:

On 06/17/2016 07:45 AM, Petr Spacek wrote:

On 16.6.2016 17:33, Stanislav Laznicka wrote:

Hello,

This patch removes most sys.exits() from installer modules and scripts and
replaces them with ScriptError. I only left sys.exits at places where the user
decides yes/no on continuation of the script.

I wonder if yes/no should be replaced with KeyboardInterrupt or some other
exception, too...


I'm not sure, it seems more clear to just really exit if the user desires it
and it's what we say we'll do (with possible cleanup beforehand). Do you think
we could benefit somehow by raising an exception here?

I'm just thinking out loud.

It seemed to me that it is easier to share cleanup on one except block instead
of having if (interrupt): cleanup; if (interrupt2): same_cleanup;

etc.

Again, just wondering out loud.

If the cleanup is the same, or similar it might be more beneficial to 
have it in a function where you could pass what was set up already and 
therefore needs cleanup. But that's just an opinion coming from thinking 
out loud as well. I went through the code to see if there's much cleanup 
after these user actions and it seems that usually there's nothing much 
if anything. However, thinking in advance may save us much trouble in 
the future, of course.


--
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 0042] Removed dead code from LDAPRemoveReverseMember


On 06/14/2016 04:40 PM, Jan Cholasta wrote:

On 14.6.2016 16:35, Martin Basti wrote:

On 14.06.2016 16:37, Jan Cholasta wrote:

On 14.6.2016 16:29, Martin Basti wrote:

On 08.06.2016 14:17, Stanislav Laznicka wrote:

On 06/07/2016 10:42 AM, Martin Basti wrote:

On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:

On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:

On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:

On 03.06.2016 14:13, Stanislav Laznicka wrote:

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


NACK

please remove it from LDAPAddReverseMember too, it contains 
the

same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that
every
reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and I 
don't

feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no 
point in

postponing it. I see no reason to be really afraid, I'm pretty sure
that removing the objectclass attribute (which is invisible in the
CLI anyway) from the output of all the 4 commands that use this 
code

won't break anything.



Ok

It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options should 
make

it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?


I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code that
was removed, and as you can see Standa had add extra objectclass
attribute there


So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)

Right. I added the objectclass so that it behaves similar to what the 
other commands do and so that it does not break tests but it can be 
removed and tests could be fixed. The question here is - do we want it 
in 4.4, then? If so can we be sure it won't break anything (although we 
know that it's broken as it is, and it's been like that for the past 4 
years)?


Currently, the LDAP*ReverseMember are used in adding/removing 
permissions/privileges to privileges/roles so I think we don't want it 
to go bad - but could it?


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

Re: [Freeipa-devel] [PATCH 0048] Remove sys.exit() from installer modules


On 06/17/2016 07:45 AM, Petr Spacek wrote:

On 16.6.2016 17:33, Stanislav Laznicka wrote:

Hello,

This patch removes most sys.exits() from installer modules and scripts and
replaces them with ScriptError. I only left sys.exits at places where the user
decides yes/no on continuation of the script.

I wonder if yes/no should be replaced with KeyboardInterrupt or some other
exception, too...

I'm not sure, it seems more clear to just really exit if the user 
desires it and it's what we say we'll do (with possible cleanup 
beforehand). Do you think we could benefit somehow by raising an 
exception here?


--
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 0048] Remove sys.exit() from installer modules

2016-06-16 Thread Stanislav Laznicka


Hello,

This patch removes most sys.exits() from installer modules and scripts 
and replaces them with ScriptError. I only left sys.exits at places 
where the user decides yes/no on continuation of the script.


From 7968f068141e53f7bf111221b38c40cac432 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 16 Jun 2016 17:12:24 +0200
Subject: [PATCH] Remove sys.exit from install modules and scripts

sys.exit() calls sometimes make it hard to find bugs and mask code that
does not always work properly.

https://fedorahosted.org/freeipa/ticket/5750
---
 install/tools/ipa-adtrust-install  | 17 +++---
 install/tools/ipa-ca-install   | 23 
 install/tools/ipa-compat-manage| 11 ++--
 install/tools/ipa-dns-install  |  3 +-
 ipaserver/install/bindinstance.py  |  2 +-
 ipaserver/install/ca.py| 19 +++---
 ipaserver/install/cainstance.py|  5 +-
 ipaserver/install/dns.py   |  5 +-
 ipaserver/install/dsinstance.py|  3 +-
 ipaserver/install/installutils.py  | 16 +++---
 ipaserver/install/ipa_ldap_updater.py  |  2 +-
 ipaserver/install/krainstance.py   |  3 +-
 ipaserver/install/replication.py   | 10 ++--
 ipaserver/install/server/install.py| 64 +++--
 ipaserver/install/server/replicainstall.py | 92 --
 15 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 36caa5c2d429c6cf107df03e82aa80e15d8efe01..da635cb02c3c8affb234515dd64f2cb06e9ea872 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -37,6 +37,7 @@ from ipaserver.install.installutils import (
 from ipaserver.install import service
 from ipapython import version
 from ipapython import ipautil, sysrestore, ipaldap
+from ipapython.admintool import ScriptError
 from ipalib import api, errors, krb_utils
 from ipapython.config import IPAOptionParser
 from ipaplatform.paths import paths
@@ -192,7 +193,7 @@ def set_and_check_netbios_name(netbios_name, unattended):
 if not adtrustinstance.check_netbios_name(netbios_name):
 if unattended and not gen_netbios_name:
 netbios_name_error(netbios_name)
-sys.exit("Aborting installation.")
+raise ScriptError("Aborting installation.")
 else:
 if netbios_name:
 netbios_name_error(netbios_name)
@@ -227,7 +228,7 @@ def main():
 safe_options, options = parse_options()
 
 if os.getegid() != 0:
-sys.exit("Must be root to setup AD trusts on server")
+raise ScriptError("Must be root to setup AD trusts on server")
 
 standard_logging_setup(log_file_name, debug=options.debug, filemode='a')
 print("\nThe log file for this installation can be found in %s" % log_file_name)
@@ -255,7 +256,7 @@ def main():
 
 # Check if samba packages are installed
 if not adtrustinstance.check_inst():
-sys.exit("Aborting installation.")
+raise ScriptError("Aborting installation.")
 
 # Initialize the ipalib api
 cfg = dict(
@@ -318,14 +319,14 @@ def main():
 try:
 principal = krb_utils.get_principal()
 except errors.CCacheError as e:
-sys.exit("Must have Kerberos credentials to setup AD trusts on server: %s" % e.message)
+raise ScriptError("Must have Kerberos credentials to setup AD trusts on server: %s" % e.message)
 
 try:
 api.Backend.ldap2.connect()
 except errors.ACIError as e:
-sys.exit("Outdated Kerberos credentials. Use kdestroy and kinit to update your ticket")
+raise ScriptError("Outdated Kerberos credentials. Use kdestroy and kinit to update your ticket")
 except errors.DatabaseError as e:
-sys.exit("Cannot connect to the LDAP database. Please check if IPA is running")
+raise ScriptError("Cannot connect to the LDAP database. Please check if IPA is running")
 
 try:
 user = api.Command.user_show(principal.partition('@')[0].partition('/')[0])['result']
@@ -334,9 +335,9 @@ def main():
 group['cn'][0] in user['memberof_group']):
 raise errors.RequirementError(name='admins group membership')
 except errors.RequirementError as e:
-sys.exit("Must have administrative privileges to setup AD trusts on server")
+raise ScriptError("Must have administrative privileges to setup AD trusts on server")
 except Exception as e:
-sys.exit("Unrecognized error during check of admin rights: %s" % (str(e)))
+raise ScriptError("Unrecognized error during check of admin rights: %s" % (str(e)))
 
 (netbios_name, reset_ne

[Freeipa-devel] [PATCH 0047] Fix uninitialized variables in replicainstall

2016-06-16 Thread Stanislav Laznicka


Hello,

There was a possible use of uninitialized variables in replicainstall.

From 1b26d42e00506b007e087c74cafc0327090aec40 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 16 Jun 2016 10:05:34 +0200
Subject: [PATCH] Fix unitialized variables in replicainstall

ipaconf and target_fname variables would have been used uninitialized in finally block
should an exception occur in the try block before their initialization.
---
 ipaserver/install/server/replicainstall.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index f597880471eb3710ebc7163f771d4e6dc9f1e3d6..8864944d814239eec82c8c5d862f0f9ca04b23d2 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -1304,10 +1304,12 @@ def promote(installer):
 http_pkcs12_info = installer._http_pkcs12_info
 pkinit_pkcs12_file = installer._pkinit_pkcs12_file
 pkinit_pkcs12_info = installer._pkinit_pkcs12_info
+target_fname = paths.IPA_DEFAULT_CONF
 
 ccache = os.environ['KRB5CCNAME']
 remote_api = installer._remote_api
 conn = remote_api.Backend.ldap2
+ipaconf = ipaclient.ipachangeconf.IPAChangeConf("IPA Replica Promote")
 try:
 conn.connect(ccache=installer._ccache)
 
@@ -1318,9 +1320,7 @@ def promote(installer):
 )
 
 # Save client file and merge in server directives
-target_fname = paths.IPA_DEFAULT_CONF
 fstore.backup_file(target_fname)
-ipaconf = ipaclient.ipachangeconf.IPAChangeConf("IPA Replica Promote")
 ipaconf.setOptionAssignment(" = ")
 ipaconf.setSectionNameDelimiters(("[", "]"))
 
-- 
2.5.5

-- 
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 0043] Stop uninstaller from failing if a service can't be started

2016-06-14 Thread Stanislav Laznicka


On 06/14/2016 09:25 AM, Stanislav Laznicka wrote:

On 06/13/2016 02:51 PM, Martin Babinsky wrote:

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

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



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception 
raising and log an error instead of copy-pasting try: ... except: 
blocks?


A good point. SystemdService start()/restart() methods will need 
augmenting as well (signerd service) but I suppose that's about it for 
this issue. I'll send a patch updated accordingly.


Actually, adding an argument to SystemdService's start()/restart() 
methods turned out to be very, very bad idea for this purpose (it would 
end up with way worse copy-paste than the original patch). Attached is 
probably the most minimal solution.
From 33f9b79a68431b54e3a047f3f4a67dec5554803c Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 14 Jun 2016 17:17:37 +0200
Subject: [PATCH] Uninstaller won't fail if service can't be started

https://fedorahosted.org/freeipa/ticket/5775
---
 ipaserver/install/bindinstance.py|  2 +-
 ipaserver/install/httpinstance.py|  2 +-
 ipaserver/install/krbinstance.py |  2 +-
 ipaserver/install/ntpinstance.py |  2 +-
 ipaserver/install/odsexporterinstance.py |  7 ++-
 ipaserver/install/opendnssecinstance.py  |  2 +-
 ipaserver/install/service.py | 28 +++-
 7 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 78e75359266bbefe7954242b98922272fb0c9194..b9b9a7d7f3bef5fb461182e23a4b45d39a3f0405 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -1273,7 +1273,7 @@ class BindInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 self.named_regular.unmask()
 if named_regular_enabled:
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index 00f890175ae583f485797da6f913a7f83b302df3..cd81bbed3830e239b6cf50f765f176547ee788fa 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -549,7 +549,7 @@ class HTTPInstance(service.Service):
 self.print_msg('WARNING: ' + str(e))
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 # disabled by default, by ldap_enable()
 if enabled:
diff --git a/ipaserver/install/krbinstance.py b/ipaserver/install/krbinstance.py
index f560a6ec4c2e4ce931cc1552976db5900a3fa5cd..61d02f9fd346d7b78e5d8682038d9b1a1a149c74 100644
--- a/ipaserver/install/krbinstance.py
+++ b/ipaserver/install/krbinstance.py
@@ -405,7 +405,7 @@ class KrbInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
 
 self.kpasswd = KpasswdInstance()
 self.kpasswd.uninstall()
diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 8b0f0e5395dae3c058fc31bd8914741e4d158830..8b2b1c1ec5102355eb6461bcfa5b9985ba02bc12 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -183,4 +183,4 @@ class NTPInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
diff --git a/ipaserver/install/odsexporterinstance.py b/ipaserver/install/odsexporterinstance.py
index e9f7bf853d98237aa19aace384b8ff7021c3a85a..4625df81988ba654ac844f93812fc6802b48dee8 100644
--- a/ipaserver/install/odsexporterinstance.py
+++ b/ipaserver/install/odsexporterinstance.py
@@ -193,7 +193,12 @@ class ODSExporterInstance(service.Service):
 signerd_service.enable()
 
 if signerd_running:
-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start '{svcname}': {err}"
+  .format(svcname=signerd_service.service_name,
+  err=e))
 
 installutils.remove_keytab(paths.IPA_ODS_EXPORTER_KEYTAB)
 installutils.remove_ccache(ccache_path=paths.IPA_ODS_EXPORTER_CCACHE)
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index f0c512ba04129d08b5874f58c7a25620f7435b2a..c4de0eb4614c235981840db0041a62f4db42ccef 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -386,4 +386,4 @@ class OpenDNSSECInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+self.restart(suppress_errors=True)
diff --git a/ipaserver/install/servic

Re: [Freeipa-devel] [PATCH 0043] Stop uninstaller from failing if a service can't be started

2016-06-14 Thread Stanislav Laznicka


On 06/13/2016 02:51 PM, Martin Babinsky wrote:

On 06/07/2016 10:14 AM, Stanislav Laznicka wrote:

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



Umm, wouldn't it be better to augment the `Service.start()/restart()` 
methods themselves with parameters that will suppress exception 
raising and log an error instead of copy-pasting try: ... except: blocks?


A good point. SystemdService start()/restart() methods will need 
augmenting as well (signerd service) but I suppose that's about it for 
this issue. I'll send a patch updated accordingly.


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

Re: [Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

2016-06-14 Thread Stanislav Laznicka


On 06/13/2016 10:15 AM, Petr Vobornik wrote:

On 06/10/2016 06:31 PM, Stanislav Laznicka wrote:

On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote:

On 06/08/2016 10:07 AM, Petr Spacek wrote:

On 7.6.2016 15:11, Stanislav Laznicka wrote:

Hello,

Thank you for your patch. As the thin-client patches were pushed in the
meantime, the patch won't apply. Could you please send a rebased version?

Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief conclusion to the
changes made in the commit. This could help for faster orientation in the
changes that were made to a certain part of code should you be searching for a
bug introduced by a commit. Should some more info be required, it can be added
to the ticket. Could you therefore shorten the commit message?

(My personal opinion, no golden standard.)

Honestly I disagree with Standa. Yes, the commit message seems to be a bit
long but *tickets* are not the best place to put *technical* information into.

Tickets are planning tool but keep in mind that Trac may/will vanish one day
and all we will have will be (Git?) repo.

I would recommend putting the comment about expected input format into code
comments somewhere around batch command definition.

This would reduce commit message (roughly, the text needs to be adapted) to
part starting 'The code did not check the format of ' ... which is perfectly
reasonable description of the change.

IMHO.
Petr^2 Spacek



2) Please do not add the tickets to comments in the code. You can use git
blame -L or git log -L to see in which commits were the changes introduced to
a certain part of a file, these commits should include the ticket number if
more info is needed.

Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:

Hi all,

the following patch checks the format of parameters passed to a method
called through the batch command. I picked the ConversionError for invalid
parameters format but this choice can be discussed if you have better
suggestions...

Fixes:https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat





Hi,

please find an updated patch version with a less verbose commit msg. I also
removed the reference to ticket # in the code.

Flo.



Hello,

Thank you for your updated patch. I have just one small issue that maybe exceeds
the scope of this ticket. If the check for dictionary instance in list:

+if not isinstance(arg, dict):
+raise errors.ConversionError(
+name='methods',
+error=_(u'must contain dict objects'))

fails at a further member of the list, by raising an exception, you will lose
information about execution of all the previous commands but these were already
executed. This hasn't seem to be an issue until now so I wonder if it is a
problem or not.

Right, this is something that we will need to address but not in scope
of this ticket and probably not 4.4 release.


So far, batch commands have only been utilized in the WebUI so I am adding Petr
for opinions on how to handle this properly so that WebUI could react to it
should it ever happen (although AFAIK this should never happen for batch
commands called from the UI).

Web UI doesn't care because it sends it correctly :) The bug is trying
to use batch command while talking directly to API - e.g. because of
performance reasons.


Thank you for the explanation. ACK, then.

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

Re: [Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

2016-06-10 Thread Stanislav Laznicka


On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote:

On 06/08/2016 10:07 AM, Petr Spacek wrote:

On 7.6.2016 15:11, Stanislav Laznicka wrote:

Hello,

Thank you for your patch. As the thin-client patches were pushed in the
meantime, the patch won't apply. Could you please send a rebased version?

Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief conclusion to the
changes made in the commit. This could help for faster orientation in the
changes that were made to a certain part of code should you be searching for a
bug introduced by a commit. Should some more info be required, it can be added
to the ticket. Could you therefore shorten the commit message?

(My personal opinion, no golden standard.)

Honestly I disagree with Standa. Yes, the commit message seems to be a bit
long but *tickets* are not the best place to put *technical* information into.

Tickets are planning tool but keep in mind that Trac may/will vanish one day
and all we will have will be (Git?) repo.

I would recommend putting the comment about expected input format into code
comments somewhere around batch command definition.

This would reduce commit message (roughly, the text needs to be adapted) to
part starting 'The code did not check the format of ' ... which is perfectly
reasonable description of the change.

IMHO.
Petr^2 Spacek



2) Please do not add the tickets to comments in the code. You can use git
blame -L or git log -L to see in which commits were the changes introduced to
a certain part of a file, these commits should include the ticket number if
more info is needed.

Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:

Hi all,

the following patch checks the format of parameters passed to a method
called through the batch command. I picked the ConversionError for invalid
parameters format but this choice can be discussed if you have better
suggestions...

Fixes:https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat






Hi,

please find an updated patch version with a less verbose commit msg. I 
also removed the reference to ticket # in the code.


Flo.



Hello,

Thank you for your updated patch. I have just one small issue that maybe 
exceeds the scope of this ticket. If the check for dictionary instance 
in list:


+if not isinstance(arg, dict):
+raise errors.ConversionError(
+name='methods',
+error=_(u'must contain dict objects'))

fails at a further member of the list, by raising an exception, you will 
lose information about execution of all the previous commands but these 
were already executed. This hasn't seem to be an issue until now so I 
wonder if it is a problem or not.


So far, batch commands have only been utilized in the WebUI so I am 
adding Petr for opinions on how to handle this properly so that WebUI 
could react to it should it ever happen (although AFAIK this should 
never happen for batch commands called from the UI).


Standa
-- 
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 0046] Don't fail in find/show methods if userCertificate is invalid

2016-06-10 Thread Stanislav Laznicka


On 06/09/2016 04:32 PM, Rob Crittenden wrote:

Fraser Tweedale wrote:

On Thu, Jun 09, 2016 at 03:07:34PM +0200, Martin Basti wrote:

On 09.06.2016 15:03, Martin Basti wrote:

On 09.06.2016 15:02, Stanislav Laznicka wrote:

On 06/09/2016 02:51 PM, Rob Crittenden wrote:

Stanislav Laznicka wrote:

Hello,

Please see the attached patch of
https://fedorahosted.org/freeipa/ticket/5797.

Standa


Just wondering out loud but should usercertificate be excluded
from the output if it is unparsable? Is there any value in
showing that a bogus value is in there?

rob

I think it is a good pointer that something has gone wrong with the
certificate. Another way would be to print 'Invalid certificate'
instead of it similar to what Apache LDAP Browser does.

We can return a warning message that something with certificates is
broken.

Martin^2


And you should log it at error log level, because it is error


Is the data from LDAP actually invalid?  It should not be possible
to store data that is not a syntactically valid X.509 cert in the
userCertificate attribute (if it is, we should file a ticket against
389).

Is there a full traceback for the original error of #5797?  What is
the datum that is the immediate cause of the error and what happens
to it between the database and the function that throws?

Could it be a python3 bytes/str problem originating in
x509.normalize_certificate?

Cheers,
Fraser



A cert can get in several different ways. IPA sure tries hard not to 
allow bad certs but I guess they can happen:


$ ldapmodify -Y GSSAPI
SASL/GSSAPI authentication started
SASL username: ad...@greyoak.com
SASL SSF: 56
SASL data security layer installed.
dn: 
krbprincipalname=cert/slithy.greyoak@greyoak.com,cn=services,cn=accounts,dc=greyoak,dc=com

changetype: modify
add: usercertificate
usercertificate: foo

modifying entry 
"krbprincipalname=cert/slithy.greyoak@greyoak.com,cn=services,cn=accounts,dc=greyoak,dc=com"

< .. snip .. >
That is exactly how I was reproducing this issue.

Added is the patch that adds error message and logs properly.
From 6913cb98da07b235b571d0772889df9a2caff1e8 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 9 Jun 2016 13:13:24 +0200
Subject: [PATCH] host/service-show/find shouldn't fail on invalid certificate

host/service-show/find methods would have failed if the first
certificate they had in userCertificate attribute were invalid.
Expected behavior is that they just show the rest of the reqested
attributes.

https://fedorahosted.org/freeipa/ticket/5797
---
 ipalib/messages.py   | 10 ++
 ipaserver/plugins/host.py| 31 +--
 ipaserver/plugins/service.py | 34 +++---
 3 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/ipalib/messages.py b/ipalib/messages.py
index e863bdd495b55921c9e487794f5c9573a6166038..2c02f8b912f5e8253048f63c0b60a5483b29772b 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -395,6 +395,16 @@ class DNSForwardPolicyConflictWithEmptyZone(PublicMessage):
 )
 
 
+class CertificateInvalid(PublicMessage):
+"""
+***13022 Failed to parse a certificate
+"""
+errno = 13022
+type = "error"
+format = _("%(subject)s: Invalid certificate. "
+   "%(reason)s")
+
+
 def iter_messages(variables, base):
 """Return a tuple with all subclasses
 """
diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index e59e0fa93c9fc0b9c6fccc36421d3489678a0eb2..befa4d8bbb602594cfaa388cf9615d1f62b6a028 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -1023,7 +1023,21 @@ class host_find(LDAPSearch):
 if options.get('pkey_only', False):
 return truncated
 for entry_attrs in entries:
-set_certificate_attrs(entry_attrs)
+hostname = entry_attrs['fqdn']
+if isinstance(hostname, (tuple, list)):
+hostname = hostname[0]
+try:
+set_certificate_attrs(entry_attrs)
+except errors.CertificateFormatError as e:
+self.add_message(
+messages.CertificateInvalid(
+subject=hostname,
+reason=e,
+)
+)
+self.log.error("Invalid certificate: {err}".format(err=e))
+del(entry_attrs['usercertificate'])
+
 set_kerberos_attrs(entry_attrs, options)
 rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
 self.obj.suppress_netgroup_memberof(ldap, entry_attrs)
@@ -1066,7 +1080,20 @@ class host_show(LDAPRetrieve):
 # fetched anywhere.
 entry_attrs['has_keytab'] = False
 
-set_certificate_attrs(entry_attrs)
+hostname = entry_attrs['fqdn']
+if

Re: [Freeipa-devel] [PATCH 0046] Don't fail in find/show methods if userCertificate is invalid


On 06/09/2016 02:51 PM, Rob Crittenden wrote:

Stanislav Laznicka wrote:

Hello,

Please see the attached patch of
https://fedorahosted.org/freeipa/ticket/5797.

Standa





Just wondering out loud but should usercertificate be excluded from 
the output if it is unparsable? Is there any value in showing that a 
bogus value is in there?


rob
I think it is a good pointer that something has gone wrong with the 
certificate. Another way would be to print 'Invalid certificate' instead 
of it similar to what Apache LDAP Browser does.


--
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] 0006 add context to exception on LdapEntry decode error


On 06/09/2016 11:58 AM, Florence Blanc-Renaud wrote:

On 06/08/2016 01:14 PM, Stanislav Laznicka wrote:

On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

On 06/06/2016 02:47 PM, Florence Blanc-Renaud wrote:


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


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




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except 
block should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure in 
which case this part of the code was called. Do you happen to know 
if it possible for an invalid value to be in the raw_sync list but 
not in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what happened.

Forgot to include devel-list, sorry.


Hi,

updated patch attached.

Flo.


Thank you for the updated patch. It seems to work as expected so 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

[Freeipa-devel] [PATCH 0046] Don't fail in find/show methods if userCertificate is invalid


Hello,

Please see the attached patch of 
https://fedorahosted.org/freeipa/ticket/5797.


Standa

From 5f59311092d7f2205287d8c2945325d1017c866a Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 9 Jun 2016 13:13:24 +0200
Subject: [PATCH] host/service-show/find shouldn't fail on invalid certificate

host/service-show/find methods would have failed if the first
certificate they had in userCertificate attribute were invalid.
Expected behavior is that they just show the rest of the reqested
attributes.

https://fedorahosted.org/freeipa/ticket/5797
---
 ipaserver/plugins/host.py| 13 +++--
 ipaserver/plugins/service.py | 14 --
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index e59e0fa93c9fc0b9c6fccc36421d3489678a0eb2..83393e3952dc043dd7ce5c1659cfc753f481b17b 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -1023,7 +1023,12 @@ class host_find(LDAPSearch):
 if options.get('pkey_only', False):
 return truncated
 for entry_attrs in entries:
-set_certificate_attrs(entry_attrs)
+try:
+set_certificate_attrs(entry_attrs)
+except errors.CertificateFormatError as e:
+self.log.debug(e)
+del(entry_attrs['usercertificate'])
+
 set_kerberos_attrs(entry_attrs, options)
 rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
 self.obj.suppress_netgroup_memberof(ldap, entry_attrs)
@@ -1065,8 +1070,12 @@ class host_show(LDAPRetrieve):
 # If an OTP is set there is no keytab, at least not one
 # fetched anywhere.
 entry_attrs['has_keytab'] = False
+try:
+set_certificate_attrs(entry_attrs)
+except errors.CertificateFormatError as e:
+self.log.debug(e)
+del(entry_attrs['usercertificate'])
 
-set_certificate_attrs(entry_attrs)
 set_kerberos_attrs(entry_attrs, options)
 rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
 
diff --git a/ipaserver/plugins/service.py b/ipaserver/plugins/service.py
index 7b8f2a7aa8711bc8bf6f2e42c5794c8cf358f252..2b82b8fd4a65766f6e0ebe1a6c6516a622e67c28 100644
--- a/ipaserver/plugins/service.py
+++ b/ipaserver/plugins/service.py
@@ -698,7 +698,12 @@ class service_find(LDAPSearch):
 return truncated
 for entry_attrs in entries:
 self.obj.get_password_attributes(ldap, entry_attrs.dn, entry_attrs)
-set_certificate_attrs(entry_attrs)
+try:
+set_certificate_attrs(entry_attrs)
+except errors.CertificateFormatError as e:
+self.log.debug(e)
+del(entry_attrs['usercertificate'])
+
 set_kerberos_attrs(entry_attrs, options)
 rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
 return truncated
@@ -721,7 +726,12 @@ class service_show(LDAPRetrieve):
 assert isinstance(dn, DN)
 self.obj.get_password_attributes(ldap, dn, entry_attrs)
 
-set_certificate_attrs(entry_attrs)
+try:
+set_certificate_attrs(entry_attrs)
+except errors.CertificateFormatError as e:
+self.log.debug(e)
+del(entry_attrs['usercertificate'])
+
 set_kerberos_attrs(entry_attrs, options)
 rename_ipaallowedtoperform_from_ldap(entry_attrs, options)
 
-- 
2.5.5

-- 
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 0041] Increase nsslapd-db-locks


On 06/07/2016 08:56 AM, thierry bordaz wrote:



On 06/06/2016 07:23 PM, Martin Basti wrote:




On 03.06.2016 13:38, Stanislav Laznicka wrote:

Hello,

The attached patch implements solution to 
https://fedorahosted.org/freeipa/ticket/5914. The patch is rather 
hacky as nsslapd-db-locks requires to be modified when DS is not 
running although I accept proposals for better solution.


Standa


LGTM and works for me, but I want ack from thierry because of the value

* value set by this patch is 100K, it is okay or should be rather 50K 
as is mentioned in the ticket
* should be upgrade for this change, or should be this done only for 
new installations? (patch solves new installations only and I don't 
think that we should change it by upgrade, users may have already 
tuned DS)


Martin^2

Hello,

This value also depends of the data. For example adding groups with 
10K members spikes the dblock consumption up to 40K because of 
membership computation during the update.
The size of the entries can also contribute if for example we have 
several entries per page or they are too large and fit on several pages.


IMHO we should support out of the box groups with 10K, so 
'nsslapd-db-locks: 5' looks good to me.
However it could be documented why it can consum so much lock and how 
to tune it.


thanks
thierry

Thank you, Thierry. Reduced the 100k locks to 50k locks in the patch 
(attached).
From 1d58f8ac6dc426092a353b79f64d63db27b4b82b Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 3 Jun 2016 13:27:04 +0200
Subject: [PATCH] Increase nsslapd-db-locks to 5

Sometimes the lock table would run out of available locks. This should
improve the lock table default configuration.

https://fedorahosted.org/freeipa/ticket/5914
---
 ipaserver/install/dsinstance.py | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 00ef5f3a9370c2782213e5b269267eaa1ba4ae40..2bd0340ef4e1a1b72de17ac7ae9cdf56ca102fa8 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -250,8 +250,7 @@ class DsInstance(service.Service):
 
 self.step("creating directory server user", create_ds_user)
 self.step("creating directory server instance", self.__create_instance)
-if self.config_ldif:
-self.step("updating configuration in dse.ldif", self.__update_dse_ldif)
+self.step("updating configuration in dse.ldif", self.__update_dse_ldif)
 self.step("restarting directory server", self.__restart_instance)
 self.step("adding default schema", self.__add_default_schemas)
 self.step("enabling memberof plugin", self.__add_memberof_module)
@@ -571,9 +570,15 @@ class DsInstance(service.Service):
 temp_filename = new_dse_ldif.name
 with open(dse_filename, "r") as input_file:
 parser = installutils.ModifyLDIF(input_file, new_dse_ldif)
-# parse modification from config ldif
-with open(self.config_ldif, "r") as config_ldif:
-parser.modifications_from_ldif(config_ldif)
+parser.replace_value(
+'cn=config,cn=ldbm database,cn=plugins,cn=config',
+'nsslapd-db-locks',
+['5']
+)
+if self.config_ldif:
+# parse modifications from ldif file supplied by the admin
+with open(self.config_ldif, "r") as config_ldif:
+parser.modifications_from_ldif(config_ldif)
 parser.parse()
 new_dse_ldif.flush()
 shutil.copy2(temp_filename, dse_filename)
-- 
2.5.5

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

Re: [Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

2016-06-08 Thread Stanislav Laznicka


On 06/08/2016 02:09 PM, Petr Vobornik wrote:

On 06/08/2016 10:07 AM, Petr Spacek wrote:

On 7.6.2016 15:11, Stanislav Laznicka wrote:

Hello,

Thank you for your patch. As the thin-client patches were pushed in the
meantime, the patch won't apply. Could you please send a rebased version?

Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief conclusion to the
changes made in the commit. This could help for faster orientation in the
changes that were made to a certain part of code should you be searching for a
bug introduced by a commit. Should some more info be required, it can be added
to the ticket. Could you therefore shorten the commit message?

(My personal opinion, no golden standard.)

Honestly I disagree with Standa. Yes, the commit message seems to be a bit
long but *tickets* are not the best place to put *technical* information into.

Tickets are planning tool but keep in mind that Trac may/will vanish one day
and all we will have will be (Git?) repo.

+1

The commit message is very good and honestly I'd like to see more of
such commit messages.


I would recommend putting the comment about expected input format into code
comments somewhere around batch command definition.

This would reduce commit message (roughly, the text needs to be adapted) to
part starting 'The code did not check the format of ' ... which is perfectly
reasonable description of the change.

Batch command deserves a doc string, so that it would be visible in API
browser but that is not subject of this ticket. Btw, expected format is
already in the code.

It is, although it may be not be as clear it is what you're looking for.

As for the commit message, I would like see the description of batch 
commands (which I think is really good) in the doc string rather than in 
the commit message to get the information about the change I need. 
However, that would require proper documentation (such as that in the 
commit message) in the code (where it belongs). As it is not there, or 
it is not that clear, I guess that the commit message is the right spot 
to have it and I was wrong here.

IMHO.
Petr^2 Spacek



2) Please do not add the tickets to comments in the code. You can use git
blame -L or git log -L to see in which commits were the changes introduced to
a certain part of a file, these commits should include the ticket number if
more info is needed.

+1


Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:

Hi all,

the following patch checks the format of parameters passed to a method
called through the batch command. I picked the ConversionError for invalid
parameters format but this choice can be discussed if you have better
suggestions...

Fixes: https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat












--
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 0042] Removed dead code from LDAPRemoveReverseMember

2016-06-08 Thread Stanislav Laznicka


On 06/07/2016 10:42 AM, Martin Basti wrote:



On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

On 06/03/2016 02:19 PM, Martin Basti wrote:


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the 
same

code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that every
reverse member command always acts like --all was specified.


I'm really afraid, what can happen if we put attr_list into get_entry()
instead of '*', because this code were there for 4 years and I don't
feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?


It's a bug and should be fixed. The fix is easy so I see no point in 
postponing it. I see no reason to be really afraid, I'm pretty sure 
that removing the objectclass attribute (which is invisible in the 
CLI anyway) from the output of all the 4 commands that use this code 
won't break anything.




Ok
It seems that tests expect objectClass to be always returned in derived 
methods. Is that expected behavior? If so, please see the attached 
patch. I wonder if the keys of the passed options should make it to 
attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?
From d554d96135160c8adcd3c798876de1e5ae83b4a0 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 7 Jun 2016 12:11:39 +0200
Subject: [PATCH 1/2] Revert "Removed dead code from
 LDAP{Remove,Add}ReverseMember"

While the code was really dead, it should serve a purpose elsewhere.
This reverts commit c56d65b064e1e0410c03cf1206816cad4d8d86cc.
---
 ipaserver/plugins/baseldap.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 7367c87988bfa6f1db5c38c6dd633e541f59d27e..62b726da1a0baef9fc9fa4bb386a2101ca1f10b2 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2131,6 +2131,14 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
+if options.get('all', False):
+attrs_list = ['*'] + self.obj.default_attributes
+else:
+attrs_list = set(self.obj.default_attributes)
+if options.get('no_members', False):
+attrs_list.difference_update(self.obj.attribute_members)
+attrs_list = list(attrs_list)
+
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
@@ -,6 +2230,14 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
+if options.get('all', False):
+attrs_list = ['*'] + self.obj.default_attributes
+else:
+attrs_list = set(self.obj.default_attributes)
+if options.get('no_members', False):
+attrs_list.difference_update(self.obj.attribute_members)
+attrs_list = list(attrs_list)
+
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

From 5a3c58f27efe73bcf478051d502df246ed10d0d2 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH 2/2] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..22eea887c4425bf890e0dc0da9e8779812fe5769 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2159,7 +2159,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list + ['objectclass'])
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
@@ -2258,7 +2258,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].a

Re: [Freeipa-devel] [PATCH] 0006 add context to exception on LdapEntry decode error

2016-06-08 Thread Stanislav Laznicka


On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

On 06/06/2016 02:47 PM, Florence Blanc-Renaud wrote:


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


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




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except block 
should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure in 
which case this part of the code was called. Do you happen to know if 
it possible for an invalid value to be in the raw_sync list but not 
in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what happened.

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

Re: [Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

2016-06-07 Thread Stanislav Laznicka


Hello,

Thank you for your patch. As the thin-client patches were pushed in the 
meantime, the patch won't apply. Could you please send a rebased version?


Also, I have a few comments to the patch:

1) I think that the commit message should be rather a brief conclusion 
to the changes made in the commit. This could help for faster 
orientation in the changes that were made to a certain part of code 
should you be searching for a bug introduced by a commit. Should some 
more info be required, it can be added to the ticket. Could you 
therefore shorten the commit message?


2) Please do not add the tickets to comments in the code. You can use 
git blame -L or git log -L to see in which commits were the changes 
introduced to a certain part of a file, these commits should include the 
ticket number if more info is needed.


Standa


On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:


Hi all,

the following patch checks the format of parameters passed to a method 
called through the batch command. I picked the ConversionError for 
invalid parameters format but this choice can be discussed if you have 
better suggestions...


Fixes: https://fedorahosted.org/freeipa/ticket/5810
--
Florence Blanc-Renaud
Identity Management Team, Red Hat




-- 
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 0043] Stop uninstaller from failing if a service can't be started

2016-06-07 Thread Stanislav Laznicka


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

From 8ba87072d8e998ccb8743390eb541e74f6b1aa96 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 7 Jun 2016 10:08:45 +0200
Subject: [PATCH] Uninstaller won't fail if service can't be started

https://fedorahosted.org/freeipa/ticket/5775
---
 ipaserver/install/bindinstance.py| 6 +-
 ipaserver/install/httpinstance.py| 6 +-
 ipaserver/install/krbinstance.py | 6 +-
 ipaserver/install/ntpinstance.py | 6 +-
 ipaserver/install/odsexporterinstance.py | 6 +-
 ipaserver/install/opendnssecinstance.py  | 6 +-
 ipaserver/install/service.py | 6 +-
 7 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index afcb6b0c10e54b1aae975fd1e81f37144e6b97ed..928957594098563c89ed905946933110e92b0bb5 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -1269,7 +1269,11 @@ class BindInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 self.named_regular.unmask()
 if named_regular_enabled:
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index 00f890175ae583f485797da6f913a7f83b302df3..3dc115c225316362957e78ce4a6b12e59844edf9 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -549,7 +549,11 @@ class HTTPInstance(service.Service):
 self.print_msg('WARNING: ' + str(e))
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 # disabled by default, by ldap_enable()
 if enabled:
diff --git a/ipaserver/install/krbinstance.py b/ipaserver/install/krbinstance.py
index f560a6ec4c2e4ce931cc1552976db5900a3fa5cd..b3c742ece6974931c8c918fb2742ec7c518a490b 100644
--- a/ipaserver/install/krbinstance.py
+++ b/ipaserver/install/krbinstance.py
@@ -405,7 +405,11 @@ class KrbInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 self.kpasswd = KpasswdInstance()
 self.kpasswd.uninstall()
diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 8b0f0e5395dae3c058fc31bd8914741e4d158830..9bac7b75130dbf592598e1e2a4a64c62ff5858f0 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -183,4 +183,8 @@ class NTPInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
diff --git a/ipaserver/install/odsexporterinstance.py b/ipaserver/install/odsexporterinstance.py
index e9f7bf853d98237aa19aace384b8ff7021c3a85a..089372b13414a74b8b90566223662fa56d28ec7f 100644
--- a/ipaserver/install/odsexporterinstance.py
+++ b/ipaserver/install/odsexporterinstance.py
@@ -193,7 +193,11 @@ class ODSExporterInstance(service.Service):
 signerd_service.enable()
 
 if signerd_running:
-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 installutils.remove_keytab(paths.IPA_ODS_EXPORTER_KEYTAB)
 installutils.remove_ccache(ccache_path=paths.IPA_ODS_EXPORTER_CCACHE)
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index f0c512ba04129d08b5874f58c7a25620f7435b2a..4c2c4145240f030ce8946205faea336a6f462f9f 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -386,4 +386,8 @@ class OpenDNSSECInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+

Re: [Freeipa-devel] [PATCH 0042] Removed dead code from LDAPRemoveReverseMember


On 06/03/2016 02:19 PM, Martin Basti wrote:


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the same code

Martin^2


Please see the modified patch.

Standa

From 6bd0ea9cb7749cceba40e5df03aeca2e9ee3b70d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 3 Jun 2016 14:08:59 +0200
Subject: [PATCH] Removed dead code from LDAP{Remove,Add}ReverseMember

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 16 
 1 file changed, 16 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index bbd8ba146ead81857bbc4c2aee550b855b846be5..3f0e68b143f2dcb50be6bc49a752b19da02a248d 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2129,14 +2129,6 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
-if options.get('all', False):
-attrs_list = ['*'] + self.obj.default_attributes
-else:
-attrs_list = set(self.obj.default_attributes)
-if options.get('no_members', False):
-attrs_list.difference_update(self.obj.attribute_members)
-attrs_list = list(attrs_list)
-
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
@@ -2228,14 +2220,6 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
-if options.get('all', False):
-attrs_list = ['*'] + self.obj.default_attributes
-else:
-attrs_list = set(self.obj.default_attributes)
-if options.get('no_members', False):
-attrs_list.difference_update(self.obj.attribute_members)
-attrs_list = list(attrs_list)
-
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

-- 
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 0042] Removed dead code from LDAPRemoveReverseMember


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

From 350cfa89f34a6f9beddc85a195963966e1aa561d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 3 Jun 2016 14:08:59 +0200
Subject: [PATCH] Removed dead code from LDAPRemoveReverseMember

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 8 
 1 file changed, 8 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index bbd8ba146ead81857bbc4c2aee550b855b846be5..f60fb505961ed3b6ce97b505623af153532dd1c9 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2228,14 +2228,6 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
-if options.get('all', False):
-attrs_list = ['*'] + self.obj.default_attributes
-else:
-attrs_list = set(self.obj.default_attributes)
-if options.get('no_members', False):
-attrs_list.difference_update(self.obj.attribute_members)
-attrs_list = list(attrs_list)
-
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

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

[Freeipa-devel] [PATCH 0041] Increase nsslapd-db-locks


Hello,

The attached patch implements solution to 
https://fedorahosted.org/freeipa/ticket/5914. The patch is rather hacky 
as nsslapd-db-locks requires to be modified when DS is not running 
although I accept proposals for better solution.


Standa

From f46164b12228708eac9656c85ff82657fa2b2a1f Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 3 Jun 2016 13:27:04 +0200
Subject: [PATCH] Increase nsslapd-db-locks to 10

https://fedorahosted.org/freeipa/ticket/5914
---
 ipaserver/install/dsinstance.py | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 00ef5f3a9370c2782213e5b269267eaa1ba4ae40..5e461fcdc61c0f27977c06ab4e8671952160c05d 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -250,8 +250,7 @@ class DsInstance(service.Service):
 
 self.step("creating directory server user", create_ds_user)
 self.step("creating directory server instance", self.__create_instance)
-if self.config_ldif:
-self.step("updating configuration in dse.ldif", self.__update_dse_ldif)
+self.step("updating configuration in dse.ldif", self.__update_dse_ldif)
 self.step("restarting directory server", self.__restart_instance)
 self.step("adding default schema", self.__add_default_schemas)
 self.step("enabling memberof plugin", self.__add_memberof_module)
@@ -571,9 +570,15 @@ class DsInstance(service.Service):
 temp_filename = new_dse_ldif.name
 with open(dse_filename, "r") as input_file:
 parser = installutils.ModifyLDIF(input_file, new_dse_ldif)
-# parse modification from config ldif
-with open(self.config_ldif, "r") as config_ldif:
-parser.modifications_from_ldif(config_ldif)
+parser.replace_value(
+'cn=config,cn=ldbm database,cn=plugins,cn=config',
+'nsslapd-db-locks',
+['10']
+)
+if self.config_ldif:
+# parse modifications from ldif file supplied by the admin
+with open(self.config_ldif, "r") as config_ldif:
+parser.modifications_from_ldif(config_ldif)
 parser.parse()
 new_dse_ldif.flush()
 shutil.copy2(temp_filename, dse_filename)
-- 
2.5.5

-- 
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 0040] Always add hostname=IPAREALM to krb5.conf


On 06/02/2016 08:11 PM, Martin Basti wrote:


On 02.06.2016 16:02, Stanislav Laznicka wrote:

Hello,

In this patch I am adding the mapping = to 
krb5.conf as requested in https://fedorahosted.org/freeipa/ticket/5903.



ACK

I have just one question, where is install/share/krb5.conf.template 
file used in code?


Anyway, for the god of consistency I'm pushing this and that template 
can be updated and removed in another patch, if unused.


Pushed to master: f0160a2ed28325e46be2921ac44d71ee88d1b3b1

It's rather cryptically hidden in the code (took me a while to find it 
again, I used it in some other patch before). It's used in creation of 
the krbinstance. See ipaserver/install/krbinstance.py:293 for the 
funzies and profit.


--
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 0040] Always add hostname=IPAREALM to krb5.conf

2016-06-02 Thread Stanislav Laznicka


Hello,

In this patch I am adding the mapping = to 
krb5.conf as requested in https://fedorahosted.org/freeipa/ticket/5903.


From a1547a654d60562d7cdad259a06a4072d51f5a4f Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 2 Jun 2016 15:40:03 +0200
Subject: [PATCH] Added = mapping to krb5.conf

https://fedorahosted.org/freeipa/ticket/5903
---
 client/ipa-client-install| 11 +++
 install/share/krb5.conf.template |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 538b6adee8a22839c5f16d46a242228ca3ca43ee..1606d1d2f8de0bcea13a8dce98d63d585edb8e58 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -1049,7 +1049,7 @@ def hardcode_ldap_server(cli_server):
 return
 
 def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok,
-options, filename, client_domain):
+options, filename, client_domain, client_hostname):
 
 krbconf = ipaclient.ipachangeconf.IPAChangeConf("IPA Installer")
 krbconf.setOptionAssignment((" = ", " "))
@@ -1108,7 +1108,8 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok,
 
 #[domain_realm]
 dropts = [{'name':'.'+cli_domain, 'type':'option', 'value':cli_realm},
-  {'name':cli_domain, 'type':'option', 'value':cli_realm}]
+  {'name':cli_domain, 'type':'option', 'value':cli_realm},
+  {'name':client_hostname, 'type':'option', 'value':cli_realm}]
 
 #add client domain mapping if different from server domain
 if cli_domain != client_domain:
@@ -2560,7 +2561,8 @@ def install(options, env, fstore, statestore):
 dnsok=False,
 options=options,
 filename=krb_name,
-client_domain=client_domain):
+client_domain=client_domain,
+client_hostname=hostname):
 root_logger.error("Test kerberos configuration failed")
 return CLIENT_INSTALL_ERROR
 env['KRB5_CONFIG'] = krb_name
@@ -2761,7 +2763,8 @@ def install(options, env, fstore, statestore):
 dnsok=dnsok,
 options=options,
 filename=paths.KRB5_CONF,
-client_domain=client_domain):
+client_domain=client_domain,
+client_hostname=hostname):
 return CLIENT_INSTALL_ERROR
 
 root_logger.info(
diff --git a/install/share/krb5.conf.template b/install/share/krb5.conf.template
index 92431d3fde6afecd0e74803e18724379e8746f9b..63136ca3e213c87ef51d02182569305f1690 100644
--- a/install/share/krb5.conf.template
+++ b/install/share/krb5.conf.template
@@ -26,6 +26,7 @@ $OTHER_LIBDEFAULTS
 [domain_realm]
  .$DOMAIN = $REALM
  $DOMAIN = $REALM
+ $FQDN = $REALM
 $OTHER_DOMAIN_REALM_MAPS
 [dbmodules]
   $REALM = {
-- 
2.5.5

-- 
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 0039] Deprecate --domain-level option from ipa-server-install

2016-06-02 Thread Stanislav Laznicka

I had a different solution prepared but it seems that the deprecated 
flag in KnobBase does the trick. Although if the only thing it does is 
to remove the option from help, it may need to be renamed (help_hidden, 
maybe?).


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

From 0ee60e059d33d3f55fab2e41e2198853d9116f32 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 2 Jun 2016 14:08:32 +0200
Subject: [PATCH] Deprecated the domain-level option in ipa-server-install

https://fedorahosted.org/freeipa/ticket/5907
---
 ipaserver/install/server/install.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 2d71b3837214683566f37644c41a680953a64207..113c2b27733eebe8aa22029c2961ee6eb3e248b2 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -1367,6 +1367,7 @@ class Server(BaseServer):
 int, constants.MAX_DOMAIN_LEVEL,
 description="IPA domain level",
 cli_name='domain-level',
+deprecated=True,
 )
 
 @domainlevel.validator
-- 
2.5.5

-- 
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 0038] Reduced time for IO blocking of DS


Hello,

This is a fix to https://fedorahosted.org/freeipa/ticket/5383. From the 
comments I am not sure if nsslapd-idletimeout should be reduced as well. 
If so, could you please propose a value that you find reasonable?


Thanks,
Standa
From 812566cc687fedc1df2f00950440e9e7abd67d99 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 31 May 2016 17:01:29 +0200
Subject: [PATCH] Decreased timeout for IO blocking for DS

Should fix the DS from going unresponsive in some cases

https://fedorahosted.org/freeipa/ticket/5383
---
 install/updates/10-config.update | 4 
 1 file changed, 4 insertions(+)

diff --git a/install/updates/10-config.update b/install/updates/10-config.update
index 0914fb93880942cdd1f0bb142461256e9b960167..f725bbfdcdb8171f8954ed19fc31ac5f6b4242eb 100644
--- a/install/updates/10-config.update
+++ b/install/updates/10-config.update
@@ -68,3 +68,7 @@ only:nsslapd-sasl-max-buffer-size:2097152
 # setting, password migration fails
 dn: cn=config
 only:nsslapd-allow-hashed-passwords:on
+
+# Decrease default value for IO blocking to prevent server unresponsiveness
+dn: cn=config
+only:nsslapd-ioblocktimeout:1
-- 
2.5.5

-- 
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 0033] Fix CA being presented as running even if it weren't


On 05/31/2016 11:40 AM, Stanislav Laznicka wrote:

On 05/31/2016 10:22 AM, Stanislav Laznicka wrote:

On 05/30/2016 12:54 PM, Jan Cholasta wrote:

On 30.5.2016 12:36, Martin Basti wrote:



On 26.05.2016 19:31, Stanislav Laznicka wrote:


Self NACK. I should not post patches when tired, sorry. Minor fix is
attached.


On 05/26/2016 07:21 PM, Stanislav Laznicka wrote:

Hello,

Please, see the attached patch. Fixes
https://fedorahosted.org/freeipa/ticket/5898

Standa





LGTM, if nobody is against this, I will push it in 2 days


NACK, please add `wait` argument and call self.wait_until_running(), 
same as in start() and restart().



A pretty good point, please see the modified patch.
Self.NACK - can't add 'wait' agrument to service.Service.is_running 
this easy.



Should be fixed now.
From 7de58955f4876a3c9cfbfb22b38931b9dd95062e Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 26 May 2016 15:24:15 +0200
Subject: [PATCH] Fixes CA always being presented as running

Even after manually stopping the pki-tomcatd service instance the
service's is_running() method would still return True.

https://fedorahosted.org/freeipa/ticket/5898
---
 ipaplatform/base/services.py   |  4 ++--
 ipaplatform/redhat/services.py | 17 +
 ipaserver/install/service.py   |  4 ++--
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index 641a654183c52c0330cb4ece2a54c6bd0a96394c..a36b2f4ff7f325f882bdb9974e59259656b9a10e 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -148,7 +148,7 @@ class PlatformService(object):
 def restart(self, instance_name="", capture_output=True, wait=True):
 return
 
-def is_running(self, instance_name=""):
+def is_running(self, instance_name="", wait=True):
 return False
 
 def is_installed(self):
@@ -303,7 +303,7 @@ class SystemdService(PlatformService):
 if wait and self.is_running(instance_name):
 self.wait_for_open_ports(self.service_instance(instance_name))
 
-def is_running(self, instance_name=""):
+def is_running(self, instance_name="", wait=True):
 instance = self.service_instance(instance_name, 'is-active')
 
 while True:
diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 92dae452a31a0b3680e9c407eccb120881cc9e25..849737059d54df5af47ae288ef97b933d9e869fe 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -222,6 +222,23 @@ class RedHatCAService(RedHatService):
 if wait:
 self.wait_until_running()
 
+def is_running(self, instance_name="", wait=True):
+if instance_name:
+return super(RedHatCAService, self).is_running(instance_name)
+try:
+status = dogtag.ca_status()
+if status == 'running':
+return True
+elif status == 'starting' and wait:
+# Exception is raised if status is 'starting' even after wait
+self.wait_until_running()
+return True
+except Exception as e:
+root_logger.debug(
+'Failed to check CA status: {err}'.format(err=e)
+)
+return False
+
 
 # Function that constructs proper Red Hat OS family-specific server classes for
 # services of specified name
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 40767acd57d5e1fa8126144ca64f6951848ce214..c09dc9013b091e6eebcf9b297fef8337671ce40e 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -346,8 +346,8 @@ class Service(object):
 def restart(self, instance_name="", capture_output=True, wait=True):
 self.service.restart(instance_name, capture_output=capture_output, wait=wait)
 
-def is_running(self):
-return self.service.is_running()
+def is_running(self, instance_name="", wait=True):
+return self.service.is_running(instance_name, wait)
 
 def install(self):
 self.service.install()
-- 
2.5.5

-- 
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 0033] Fix CA being presented as running even if it weren't


On 05/31/2016 10:22 AM, Stanislav Laznicka wrote:

On 05/30/2016 12:54 PM, Jan Cholasta wrote:

On 30.5.2016 12:36, Martin Basti wrote:



On 26.05.2016 19:31, Stanislav Laznicka wrote:


Self NACK. I should not post patches when tired, sorry. Minor fix is
attached.


On 05/26/2016 07:21 PM, Stanislav Laznicka wrote:

Hello,

Please, see the attached patch. Fixes
https://fedorahosted.org/freeipa/ticket/5898

Standa









LGTM, if nobody is against this, I will push it in 2 days


NACK, please add `wait` argument and call self.wait_until_running(), 
same as in start() and restart().



A pretty good point, please see the modified patch.


Self.NACK - can't add 'wait' agrument to service.Service.is_running this 
easy.
-- 
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 0033] Fix CA being presented as running even if it weren't


On 05/30/2016 12:54 PM, Jan Cholasta wrote:

On 30.5.2016 12:36, Martin Basti wrote:



On 26.05.2016 19:31, Stanislav Laznicka wrote:


Self NACK. I should not post patches when tired, sorry. Minor fix is
attached.


On 05/26/2016 07:21 PM, Stanislav Laznicka wrote:

Hello,

Please, see the attached patch. Fixes
https://fedorahosted.org/freeipa/ticket/5898

Standa









LGTM, if nobody is against this, I will push it in 2 days


NACK, please add `wait` argument and call self.wait_until_running(), 
same as in start() and restart().



A pretty good point, please see the modified patch.
From bce6e608e3b952a61ecb49542d43f576689a93aa Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 26 May 2016 15:24:15 +0200
Subject: [PATCH] Fixes CA always being presented as running

Even after manually stopping the pki-tomcatd service instance the
service's is_running() method would still return True.

https://fedorahosted.org/freeipa/ticket/5898
---
 ipaplatform/redhat/services.py | 17 +
 ipaserver/install/service.py   |  4 ++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 92dae452a31a0b3680e9c407eccb120881cc9e25..849737059d54df5af47ae288ef97b933d9e869fe 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -222,6 +222,23 @@ class RedHatCAService(RedHatService):
 if wait:
 self.wait_until_running()
 
+def is_running(self, instance_name="", wait=True):
+if instance_name:
+return super(RedHatCAService, self).is_running(instance_name)
+try:
+status = dogtag.ca_status()
+if status == 'running':
+return True
+elif status == 'starting' and wait:
+# Exception is raised if status is 'starting' even after wait
+self.wait_until_running()
+return True
+except Exception as e:
+root_logger.debug(
+'Failed to check CA status: {err}'.format(err=e)
+)
+return False
+
 
 # Function that constructs proper Red Hat OS family-specific server classes for
 # services of specified name
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 40767acd57d5e1fa8126144ca64f6951848ce214..c09dc9013b091e6eebcf9b297fef8337671ce40e 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -346,8 +346,8 @@ class Service(object):
 def restart(self, instance_name="", capture_output=True, wait=True):
 self.service.restart(instance_name, capture_output=capture_output, wait=wait)
 
-def is_running(self):
-return self.service.is_running()
+def is_running(self, instance_name="", wait=True):
+return self.service.is_running(instance_name, wait)
 
 def install(self):
 self.service.install()
-- 
2.5.5

-- 
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 0036] Increased mod_wsgi socket-timeout


On 05/30/2016 02:12 PM, Petr Spacek wrote:

On 28.5.2016 15:59, Martin Basti wrote:

On 27.05.2016 14:52, Stanislav Laznicka wrote:

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




Is possible to remove timeout completely as it used to be before?

Even if this timeout is exceeded, command continue in execution and it just
doesnt print result to user

I agree with Martin. The timeout is pointless, please remove it or set it to
2^31 or so.

The documentation does not clearly state what happens in the corner 
cases of this setting. However, by looking at the source code, I'm 
guessing that 0 is the default value which would eventually point to the 
Apache TimeOut and negative values seem just wrong for them here. They 
are converting it with atoi(), so I propose to set this to 2^31-1.
From 8fe005387d6952f112246a66298c521676a73224 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 27 May 2016 14:44:30 +0200
Subject: [PATCH] Increased mod_wsgi socket-timeout

Longer-running CLI commands sometimes fail with "gateway time out" although
the task still runs and finishes on server, not notifying the CLI back.
Increasing socket-timeout should solve this.

https://fedorahosted.org/freeipa/ticket/5833
---
 install/conf/ipa.conf | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/install/conf/ipa.conf b/install/conf/ipa.conf
index cf10fc815640bcfc56152d342ee70d7d363ba4e5..c6457485a19ccdd469b96d8d29c71039290ed9da 100644
--- a/install/conf/ipa.conf
+++ b/install/conf/ipa.conf
@@ -41,7 +41,8 @@ WSGISocketPrefix /run/httpd/wsgi
 
 
 # Configure mod_wsgi handler for /ipa
-WSGIDaemonProcess ipa processes=2 threads=1 maximum-requests=500 display-name=%{GROUP}
+WSGIDaemonProcess ipa processes=2 threads=1 maximum-requests=500 \
+ display-name=%{GROUP} socket-timeout=2147483647
 WSGIImportScript /usr/share/ipa/wsgi.py process-group=ipa application-group=ipa
 WSGIScriptAlias /ipa /usr/share/ipa/wsgi.py
 WSGIScriptReloading Off
-- 
2.5.5

-- 
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 0037] Added /etc/krb5.conf.d/ to krb5.conf


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

From 7a55f169181ab8647cd2d919f35c004b14d5bc7f Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 27 May 2016 16:12:31 +0200
Subject: [PATCH] Added krb5.conf.d/ to included dirs in krb5.conf

The include of /etc/krb5.conf.d/ is required for crypto-policies to work properly

https://fedorahosted.org/freeipa/ticket/5912
---
 client/ipa-client-install| 3 ++-
 install/share/krb5.conf.template | 1 +
 ipaplatform/base/paths.py| 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index cff3fbfcdee8690c9466ea339a362edfb151a11a..ddefdbc385b5ac4619debf96610e8a7cdb18fc2e 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -1058,7 +1058,8 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok,
 krbconf.setIndent(("","  ",""))
 
 opts = [{'name':'comment', 'type':'comment', 'value':'File modified by ipa-client-install'},
-{'name':'empty', 'type':'empty'}]
+{'name':'empty', 'type':'empty'},
+{'name':'includedir', 'type':'option', 'value':paths.COMMON_KRB5_CONF_DIR, 'delim':' '}]
 
 # SSSD include dir
 if options.sssd:
diff --git a/install/share/krb5.conf.template b/install/share/krb5.conf.template
index 92431d3fde6afecd0e74803e18724379e8746f9b..f8b256aee690def6c415004df948a34d485578b1 100644
--- a/install/share/krb5.conf.template
+++ b/install/share/krb5.conf.template
@@ -1,3 +1,4 @@
+includedir /etc/krb5.conf.d/
 includedir /var/lib/sss/pubconf/krb5.include.d/
 
 [logging]
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index ca7eb6cf47b4442fa538a47c74846e13c25e02e8..336839b71e446bfc459d3bd5065b4c029b312832 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -68,6 +68,7 @@ class BasePathNamespace(object):
 DNSSEC_SOFTHSM_PIN_SO = "/etc/ipa/dnssec/softhsm_pin_so"
 IPA_NSSDB_DIR = "/etc/ipa/nssdb"
 IPA_NSSDB_PWDFILE_TXT = "/etc/ipa/nssdb/pwdfile.txt"
+COMMON_KRB5_CONF_DIR = "/etc/krb5.conf.d/"
 KRB5_CONF = "/etc/krb5.conf"
 KRB5_KEYTAB = "/etc/krb5.keytab"
 LDAP_CONF = "/etc/ldap.conf"
-- 
2.5.5

-- 
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 0036] Increased mod_wsgi socket-timeout


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

From e69514ade7bae97bb2bb0e541c080c727ff7056c Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 27 May 2016 14:44:30 +0200
Subject: [PATCH] Increased mod_wsgi socket-timeout

Longer-running CLI commands sometimes fail with "gateway time out" although
the task still runs and finishes on server, not notifying the CLI back.
Increasing socket-timeout should solve this.

https://fedorahosted.org/freeipa/ticket/5833
---
 install/conf/ipa.conf | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/install/conf/ipa.conf b/install/conf/ipa.conf
index cf10fc815640bcfc56152d342ee70d7d363ba4e5..7686999406bc2be57c121006931289c17bee67ad 100644
--- a/install/conf/ipa.conf
+++ b/install/conf/ipa.conf
@@ -41,7 +41,8 @@ WSGISocketPrefix /run/httpd/wsgi
 
 
 # Configure mod_wsgi handler for /ipa
-WSGIDaemonProcess ipa processes=2 threads=1 maximum-requests=500 display-name=%{GROUP}
+WSGIDaemonProcess ipa processes=2 threads=1 maximum-requests=500 \
+ display-name=%{GROUP} socket-timeout=300
 WSGIImportScript /usr/share/ipa/wsgi.py process-group=ipa application-group=ipa
 WSGIScriptAlias /ipa /usr/share/ipa/wsgi.py
 WSGIScriptReloading Off
-- 
2.5.5

-- 
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 35] Added pyusb dependency


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

From 38eda0f2a08b6bff65217d6c4517fffc1e1b0f86 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 27 May 2016 13:45:57 +0200
Subject: [PATCH] Added pyusb as a dependency

https://fedorahosted.org/freeipa/ticket/5886
---
 freeipa.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 94bb1abee5782cc9f00fd83b916294f34deaccd6..c6499a5c6073ac4c2814d7ad14964927b88b1981 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -505,6 +505,7 @@ Requires: python-requests
 Requires: python-custodia
 Requires: python-dns >= 1.11.1
 Requires: python-netifaces >= 0.10.4
+Requires: pyusb
 
 Conflicts: %{alt_name}-python < %{version}
 
@@ -553,6 +554,7 @@ Requires: python3-custodia
 Requires: python3-requests
 Requires: python3-dns >= 1.11.1
 Requires: python3-netifaces >= 0.10.4
+Requires: python3-pyusb
 
 %description -n python3-ipalib
 IPA is an integrated solution to provide centrally managed Identity (users,
-- 
2.5.5

-- 
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 0034] Added some attributes to the Modify Users permission


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

From f10e9b782bd92b86eb05ebd947d9799093a14091 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Fri, 27 May 2016 13:27:03 +0200
Subject: [PATCH] Added some attributes to Modify Users permission

Added 'employeenumber', 'departmentnumber' and 'mail' to Modify Users
permission

https://fedorahosted.org/freeipa/ticket/5911#comment:2
---
 ACI.txt| 2 +-
 ipalib/plugins/user.py | 7 ---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index ae00cf7a1b8e2ea0e33798993bb24dc5f06127e3..01234e434a7dc2f2abe731c7d9cc5bc58030908b 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -313,7 +313,7 @@ aci: (targetattr = "usercertificate")(targetfilter = "(objectclass=posixaccount)
 dn: cn=users,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "ipasshpubkey")(targetfilter = "(objectclass=posixaccount)")(version 3.0;acl "permission:System: Manage User SSH Public Keys";allow (write) groupdn = "ldap:///cn=System: Manage User SSH Public Keys,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=users,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "businesscategory || carlicense || cn || description || displayname || employeetype || facsimiletelephonenumber || gecos || givenname || homephone || inetuserhttpurl || initials || l || labeleduri || loginshell || manager || mepmanagedentry || mobile || objectclass || ou || pager || postalcode || preferredlanguage || roomnumber || secretary || seealso || sn || st || street || telephonenumber || title || userclass")(targetfilter = "(objectclass=posixaccount)")(version 3.0;acl "permission:System: Modify Users";allow (write) groupdn = "ldap:///cn=System: Modify Users,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+aci: (targetattr = "businesscategory || carlicense || cn || departmentnumber || description || displayname || employeenumber || employeetype || facsimiletelephonenumber || gecos || givenname || homephone || inetuserhttpurl || initials || l || labeleduri || loginshell || mail || manager || mepmanagedentry || mobile || objectclass || ou || pager || postalcode || preferredlanguage || roomnumber || secretary || seealso || sn || st || street || telephonenumber || title || userclass")(targetfilter = "(objectclass=posixaccount)")(version 3.0;acl "permission:System: Modify Users";allow (write) groupdn = "ldap:///cn=System: Modify Users,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=UPG Definition,cn=Definitions,cn=Managed Entries,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "*")(target = "ldap:///cn=UPG Definition,cn=Definitions,cn=Managed Entries,cn=etc,dc=ipa,dc=example")(version 3.0;acl "permission:System: Read UPG Definition";allow (compare,read,search) groupdn = "ldap:///cn=System: Read UPG Definition,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=users,cn=accounts,dc=ipa,dc=example
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index f92d26b2e8e5096f68c5c7fe576a120b93aad7b5..d71305d082e1d1f0c7463d12b15569259f2e2edd 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -294,10 +294,11 @@ class user(baseuser):
 'System: Modify Users': {
 'ipapermright': {'write'},
 'ipapermdefaultattr': {
-'businesscategory', 'carlicense', 'cn', 'description',
-'displayname', 'employeetype', 'facsimiletelephonenumber',
+'businesscategory', 'carlicense', 'cn', 'departmentnumber',
+'description', 'displayname', 'employeetype',
+'employeenumber', 'facsimiletelephonenumber',
 'gecos', 'givenname', 'homephone', 'inetuserhttpurl',
-'initials', 'l', 'labeleduri', 'loginshell', 'manager',
+'initials', 'l', 'labeleduri', 'loginshell', 'manager', 'mail',
 'mepmanagedentry', 'mobile', 'objectclass', 'ou', 'pager',
 'postalcode', 'roomnumber', 'secretary', 'seealso', 'sn', 'st',
 'street', 'telephonenumber', 'title', 'userclass',
-- 
2.5.5

-- 
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 0033] Fix CA being presented as running even if it weren't

Self NACK. I should not post patches when tired, sorry. Minor fix is 
attached.



On 05/26/2016 07:21 PM, Stanislav Laznicka wrote:

Hello,

Please, see the attached patch. Fixes 
https://fedorahosted.org/freeipa/ticket/5898


Standa





From b42146384771d95761cbeaab516f559ee87b66cc Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 26 May 2016 15:24:15 +0200
Subject: [PATCH] Fixes CA always being presented as running

Even after manually stopping the pki-tomcatd service instance the
service the is_running() method would still return True.

https://fedorahosted.org/freeipa/ticket/5898
---
 ipaplatform/redhat/services.py | 18 ++
 ipaserver/install/service.py   |  4 ++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 92dae452a31a0b3680e9c407eccb120881cc9e25..6f8ac2cb41c8b716d8fa13869565d9dc047d2cc4 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -222,6 +222,24 @@ class RedHatCAService(RedHatService):
 if wait:
 self.wait_until_running()
 
+def is_running(self, instance_name=""):
+if instance_name:
+return super(RedHatCAService, self).is_running(instance_name)
+while True:
+try:
+status = dogtag.ca_status()
+except Exception as e:
+root_logger.debug(
+'Failed to check CA status: {err}'.format(err=e)
+)
+return False
+if status == 'running':
+return True
+elif status == 'starting':
+time.sleep(1)
+else:
+return False
+
 
 # Function that constructs proper Red Hat OS family-specific server classes for
 # services of specified name
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 40767acd57d5e1fa8126144ca64f6951848ce214..e1361c2fb339c73dd36028257ed862d02d9411e3 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -346,8 +346,8 @@ class Service(object):
 def restart(self, instance_name="", capture_output=True, wait=True):
 self.service.restart(instance_name, capture_output=capture_output, wait=wait)
 
-def is_running(self):
-return self.service.is_running()
+def is_running(self, instance_name=""):
+return self.service.is_running(instance_name)
 
 def install(self):
 self.service.install()
-- 
2.5.5

-- 
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 0033] Fix CA being presented as running even if it weren't


Hello,

Please, see the attached patch. Fixes 
https://fedorahosted.org/freeipa/ticket/5898


Standa

From ba7ecd6eed5cb2d70dbe684539d927c44e4c11b6 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Thu, 26 May 2016 15:24:15 +0200
Subject: [PATCH] Fixes CA always being presented as running

Even after manually stopping the pki-tomcatd service instance the
service the is_running() method would still return True.

https://fedorahosted.org/freeipa/ticket/5898
---
 ipaplatform/redhat/services.py | 18 ++
 ipaserver/install/service.py   |  4 ++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 92dae452a31a0b3680e9c407eccb120881cc9e25..7f54f76fa04bbcfafd57ef7cda48cce5c197cd4b 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -222,6 +222,24 @@ class RedHatCAService(RedHatService):
 if wait:
 self.wait_until_running()
 
+def is_running(self, instance_name=""):
+if instance_name:
+super(RedHatCAService, self).is_running(instance_name)
+while True:
+try:
+status = dogtag.ca_status()
+except Exception as e:
+root_logger.debug(
+'Failed to check CA status: {err}'.format(err=e)
+)
+return False
+if status == 'running':
+return True
+elif status == 'starting':
+time.sleep(1)
+else:
+return False
+
 
 # Function that constructs proper Red Hat OS family-specific server classes for
 # services of specified name
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 40767acd57d5e1fa8126144ca64f6951848ce214..e1361c2fb339c73dd36028257ed862d02d9411e3 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -346,8 +346,8 @@ class Service(object):
 def restart(self, instance_name="", capture_output=True, wait=True):
 self.service.restart(instance_name, capture_output=capture_output, wait=wait)
 
-def is_running(self):
-return self.service.is_running()
+def is_running(self, instance_name=""):
+return self.service.is_running(instance_name)
 
 def install(self):
 self.service.install()
-- 
2.5.5

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

Re: [Freeipa-devel] [PATCH] 0001 Add missing CA options to the manpage for ipa-replica-install


Hello,

Thank you for your first patch! It seems fine to me so ACK.

Standa

On 05/20/2016 12:52 PM, Florence Blanc-Renaud wrote:


Hi all,

this one will be my first patch submission, so apologies in advance if 
I make mistakes...


The man page for ipa-replica-install was missing some commands related 
to CA-less installation, as well as --allow-zone-overlap and 
--auto-reverse. I added them in the relevant sections (CERTIFICATE 
SYSTEM OPTIONS and DNS OPTIONS).


I also fixed a wrong short option for --realm (-r).

Fixes: https://fedorahosted.org/freeipa/ticket/5835




-- 
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 0484] remove unused code from automount plugin


On 05/25/2016 06:21 PM, Martin Basti wrote:


On 25.05.2016 09:11, Stanislav Laznicka wrote:


LGTM, could you please just add the ticket to the commit message?


On 05/20/2016 04:28 PM, Martin Basti wrote:




On 20.05.2016 15:03, Martin Basti wrote:

The removed code is unused for long time.

Patch attached.




https://fedorahosted.org/freeipa/attachment/ticket/5899/





updated patch attached.

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 0477] upgrade: always start CA

2016-05-25 Thread Stanislav Laznicka


On 05/20/2016 03:00 PM, Martin Basti wrote:


On 19.05.2016 13:34, Stanislav Laznicka wrote:


Also, I tried to upgrade from 4.2.4 to 4.3.1 and it seems that it 
might be necessary to start the service even earlier in the upgrade 
logic. Attached is the trace that occurred during the upgrade.


I sent the whole log earlier accidentally, hopefully it will not 
arrive here as well.


On 05/19/2016 11:10 AM, Stanislav Laznicka wrote:


NACK, see my comments below

+# following upgrade steps require running CA
This is a nitpicky nitpick but could you please change this comment 
for # the following ...

Took me a while to understand what you were trying to say here.
+if ca_running and not ca.is_running():
+ca.stop('pki-tomcat')
+elif not ca_running and ca.is_running():
+ca.start('pki-tomcat')
+
You should swap ca.stop and ca.start here, you're stopping the 
service when it's stopped and starting it when it's already running.

Shame, shame, shame on me.



On 05/12/2016 04:34 PM, Martin Basti wrote:

Patch attached.

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











I moved starting of CA to the earlier phase and swapped start/stop to 
correct order.


Patch attached.

Seems to work as expected now. 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 0484] remove unused code from automount plugin

2016-05-25 Thread Stanislav Laznicka


LGTM, could you please just add the ticket to the commit message?


On 05/20/2016 04:28 PM, Martin Basti wrote:




On 20.05.2016 15:03, Martin Basti wrote:

The removed code is unused for long time.

Patch attached.




https://fedorahosted.org/freeipa/attachment/ticket/5899/




-- 
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 0477] upgrade: always start CA

Also, I tried to upgrade from 4.2.4 to 4.3.1 and it seems that it might 
be necessary to start the service even earlier in the upgrade logic. 
Attached is the trace that occurred during the upgrade.


I sent the whole log earlier accidentally, hopefully it will not arrive 
here as well.


On 05/19/2016 11:10 AM, Stanislav Laznicka wrote:


NACK, see my comments below

+# following upgrade steps require running CA
This is a nitpicky nitpick but could you please change this comment 
for # the following ...

Took me a while to understand what you were trying to say here.
+if ca_running and not ca.is_running():
+ca.stop('pki-tomcat')
+elif not ca_running and ca.is_running():
+ca.start('pki-tomcat')
+
You should swap ca.stop and ca.start here, you're stopping the service 
when it's stopped and starting it when it's already running.


On 05/12/2016 04:34 PM, Martin Basti wrote:

Patch attached.

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









2016-05-19T09:28:31Z DEBUG stderr=
2016-05-19T09:29:12Z ERROR IPA server upgrade failed: Inspect /var/log/ipaupgrade.log and run command ipa-server-upgrade manually.
2016-05-19T09:30:11Z DEBUG   File "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 171, in execute
return_value = self.run()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/ipa_server_upgrade.py", line 48, in run
server.upgrade()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/upgrade.py", line 1719, in upgrade
upgrade_configuration()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/upgrade.py", line 1639, in upgrade_configuration
certificate_renewal_update(ca, ds, http),
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/upgrade.py", line 920, in certificate_renewal_update
ca.configure_renewal()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/dogtaginstance.py", line 321, in configure_renewal
profile=profile)
  File "/usr/lib/python2.7/site-packages/ipapython/certmonger.py", line 509, in dogtag_start_tracking
cm.obj_if.add_request(params)
  File "/usr/lib64/python2.7/site-packages/dbus/proxies.py", line 145, in __call__
**keywords)
  File "/usr/lib64/python2.7/site-packages/dbus/connection.py", line 651, in call_blocking
message, timeout)

2016-05-19T09:30:11Z DEBUG The ipa-server-upgrade command failed, exception: DBusException: org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.
2016-05-19T09:30:11Z ERROR Unexpected error - see /var/log/ipaupgrade.log for details:
DBusException: org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken. 
2016-05-19T09:30:11Z ERROR The ipa-server-upgrade command failed. See /var/log/ipaupgrade.log for more information
-- 
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 0477] upgrade: always start CA


NACK, see my comments below

+# following upgrade steps require running CA

This is a nitpicky nitpick but could you please change this comment for 
# the following ...

Took me a while to understand what you were trying to say here.

+if ca_running and not ca.is_running():
+ca.stop('pki-tomcat')
+elif not ca_running and ca.is_running():
+ca.start('pki-tomcat')
+

You should swap ca.stop and ca.start here, you're stopping the service 
when it's stopped and starting it when it's already running.


On 05/12/2016 04:34 PM, Martin Basti wrote:

Patch attached.

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





-- 
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 0483] fix referenced before assignment error in baseldap


ACK


On 05/18/2016 07:24 PM, Martin Basti wrote:

Patch attached





-- 
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 0032] Remove dangling RUVs even if replicas are offline


On 05/19/2016 08:52 AM, Ludwig Krispenz wrote:


On 05/19/2016 08:02 AM, Stanislav Laznicka wrote:

On 05/18/2016 04:44 PM, Petr Vobornik wrote:

On 05/18/2016 04:36 PM, Stanislav Laznicka wrote:

There's no ticket for this patch but as there was a fix to 389-ds
mentioned in https://fedorahosted.org/freeipa/ticket/5396, the TODO
section in clean_dangling_ruvs could be removed.


What about using
   'replica-force-cleaning':'yes',

every time?

Is there a drawback which we would like to avoid?


The DS website mentions two possible risks
- possible loss of changes on deleted replica should these have not 
been reflected to some other replicas
this is a theoretical concern that there might be changes from the 
replica to be removed which are not yet on all servers, but to me the 
problem that cleaning ruvs hangs because replicas cannot be reached is 
the worse scenario.
- if some offline replica comes back online, it may re-pollute the 
RUVs back


I'm not sure of the probability of the second scenario, in my rather 
simple environment the re-pollution did not happen.
there have been fixes in 389-ds to prevent the repollution, so it 
should no longer happen.


Thank you, Ludwig. It seems reasonable to have the option set to 'yes' 
all the time, then.
From f02fb50f5356642e82902cbce6753e1e61b1628f Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Wed, 18 May 2016 16:27:26 +0200
Subject: [PATCH] Remove dangling RUVs even if replicas are offline

Previously, an offline replica would mean the RUVs cannot
be removed otherwise the task would be hanging in the DS.
This is fixed in 389-ds 1.3.5.

https://fedorahosted.org/freeipa/ticket/5396
---
 freeipa.spec.in  | 6 +++---
 install/tools/ipa-replica-manage | 4 
 ipaserver/install/replication.py | 1 +
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 21426d2ef6e6a59e27cc9d46cce07cfd7409bf2b..b5c155bd6d2d90af4aecb4439c9a74e88be063bf 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -42,7 +42,7 @@ Source0:freeipa-%{version}.tar.gz
 BuildRoot:  %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 %if ! %{ONLY_CLIENT}
-BuildRequires:  389-ds-base-devel >= 1.3.4.4
+BuildRequires:  389-ds-base-devel >= 1.3.5
 BuildRequires:  svrcore-devel
 BuildRequires:  policycoreutils >= 2.1.12-5
 BuildRequires:  systemd-units
@@ -131,7 +131,7 @@ Requires: %{name}-client = %{version}-%{release}
 Requires: %{name}-admintools = %{version}-%{release}
 Requires: %{name}-common = %{version}-%{release}
 Requires: python2-ipaserver = %{version}-%{release}
-Requires: 389-ds-base >= 1.3.4.6
+Requires: 389-ds-base >= 1.3.5
 Requires: openldap-clients > 2.4.35-4
 Requires: nss >= 3.14.3-12.0
 Requires: nss-tools >= 3.14.3-12.0
@@ -163,7 +163,7 @@ Requires: zip
 Requires: policycoreutils >= 2.1.12-5
 Requires: tar
 Requires(pre): certmonger >= 0.78
-Requires(pre): 389-ds-base >= 1.3.4.6
+Requires(pre): 389-ds-base >= 1.3.5
 Requires: fontawesome-fonts
 Requires: open-sans-fonts
 Requires: openssl
diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index 14e768965601cef08f13792bb5cd086534199538..f6ec413a81cd7e311d64bdf89d87096da33bed50 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -753,10 +753,6 @@ def clean_dangling_ruvs(realm, host, options):
 print('\t\tid: {id}, hostname: {host}'
   .format(id=csruv[1], host=csruv[0]))
 
-# TODO: this can be removed when #5396 is fixed
-if offlines:
-sys.exit("ERROR: All replicas need to be online to proceed.")
-
 if not options.force and not ipautil.user_input("Proceed with cleaning?", False):
 sys.exit("Aborted")
 
diff --git a/ipaserver/install/replication.py b/ipaserver/install/replication.py
index dd9453ce4fdac5d1bc43335fca2d8a96da62ad61..e4cb26f888089e5b9cabffab93ee2aab02eb8c02 100644
--- a/ipaserver/install/replication.py
+++ b/ipaserver/install/replication.py
@@ -1353,6 +1353,7 @@ class ReplicationManager(object):
 'cn': ['clean %d' % replicaId],
 'replica-base-dn': [self.db_suffix],
 'replica-id': [replicaId],
+'replica-force-cleaning': ['yes'],
 }
 )
 try:
-- 
2.5.5

-- 
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 0032] Remove dangling RUVs even if replicas are offline