Re: [Freeipa-devel] [PATCH] 0001 ipa_kdb add krbPrincipalAuthInd handling

[Matt Rogers]

On 04/27, Matt Rogers wrote:
> On 04/27, Sumit Bose wrote:
> > On Tue, Apr 26, 2016 at 02:02:04PM -0400, Matt Rogers wrote:
> > > On 04/26, Sumit Bose wrote:
> > > > On Thu, Apr 14, 2016 at 12:59:55PM -0400, Matt Rogers wrote:
> > > > > 
> > > > > 
> > > > > - Original Message -
> > > > > > From: "Nathaniel McCallum" 
> > > > > > To: "Matt Rogers" , freeipa-devel@redhat.com
> > > > > > Sent: Thursday, April 14, 2016 10:32:15 AM
> > > > > > Subject: Re: [Freeipa-devel] [PATCH] 0001 ipa_kdb add 
> > > > > > krbPrincipalAuthInd handling
> > > > > > 
> > > > > > On Mon, 2016-04-11 at 10:41 -0400, Matt Rogers wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > The attached patch is a part of the authentication indicator
> > > > > > > enhancements,
> > > > > > > adding indicator value storage and retrieval for the KDB driver.
> > > > > > > 
> > > > > > > https://fedorahosted.org/freeipa/ticket/5782
> > > > > > 
> > > > > > Can you add some whitespace in next_attr()? The density of the code
> > > > > > there hurts readability.
> > > > > > 
> > > > > Sure, I've attached the revised patch.
> > > > 
> > > > Hi Matt,
> > > > 
> > > > thank you for the patch. Currently I have the following question.
> > > > 
> > > > You call krb5_dbe_set_string to remove the 'require_auth' data before
> > > > calling ipadb_get_ldap_mod_extra_data()
> > > > 
> > > 
> > > > > +/* Delete authinds from tl_data so it is not included in 
> > > > > krbExtraData. */
> > > > > +kerr = krb5_dbe_set_string(kcontext, entry, "require_auth", 
> > > > > NULL);
> > > > > +if (kerr) {
> > > > > +goto done;
> > > > > +}
> > > > > +
> > > > >  kerr = ipadb_get_ldap_mod_extra_data(imods,
> > > > >   entry->tl_data,
> > > > >   mod_op);
> > > > > 
> > > > 
> > > > Why it is needed to filter this data again in
> > > > ipadb_get_ldap_mod_extra_data()?
> > > > 
> > > 
> > > Hi Sumit, thanks for looking. The above krb5_dbe_set_string() call I
> > > believe I left there in error - We decided to operate on a filtered copy
> > > of the tl_data (which filter_authind_str_attrs() handles) rather than
> > > removing it completely with krb5_dbe_set_string(). I had tested with the
> > > above code commented out but forgot to remove it with the submitted patch.
> > 
> > ok, makes sense.
> > 
> > Nevertheless, comments in kdb.h like:
> > 
> > /* String attributes (currently stored inside tl-data) map C string keys to
> >  * values.  They can be set via kadmin and consumed by KDC plugins. */
> > 
> > and
> > 
> > /* String attributes may not always be represented in tl-data.  kadmin 
> > clients
> >  * must use the get_strings and set_string RPCs. */
> > 
> > make me wonder if it is a good idea to operate on the tl-data of type
> > KRB5_TL_STRING_ATTRS directly? I know we do this in other places as well
> > so I'm not insisting to change it, I'm just wondering about the reasons.
> > 
> > Would something like (error checks are missing)
> > 
> > kerr = krb5_dbe_get_string(kcontext, entry, "require_auth",
> >&require_auth_str);
> > 
> > if (require_auth_str != NULL) {
> > kerr = krb5_dbe_set_string(kcontext, entry, "require_auth", NULL);
> > }
> > 
> > kerr = ipadb_get_ldap_mod_extra_data(imods,
> >  entry->tl_data,
> >  mod_op);
> > 
> > if (require_auth_str != NULL) {
> > kerr = krb5_dbe_set_string(kcontext, entry, "require_auth",
> >require_auth_str);
> > }
> > 
> > have the same effect with only using the recommended API (and making the
> > patch smaller)?
> > 
> 
> I believe that would be the same effect, just less efficient. But
> sticking to the API is probably safer in the long run. I am okay
> with changing it if you would prefer. 
> 

Here's the updated patch. Thanks again for the review!

-- 
Matt Rogers
Red Hat, Inc
>From c08bb73685b2f7a17a22e5ea066ad9b60b7c4163 Mon Sep 17 00:00:00 2001
From: Matt Rogers 
Date: Fri, 25 Mar 2016 17:01:40 -0400
Subject: [PATCH] ipa_kdb: add krbPrincipalAuthInd handling

Store and retrieve the authentication indicator "require_auth" string in
the krbPrincipalAuthInd attribute. Skip storing auth indicators to
krbExtraData.
---
 daemons/ipa-kdb/ipa_kdb_principals.c | 171 +++
 install/share/60kerberos.ldif|   6 +-
 2 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index 
629f8193223c924267f6d5f39d258cfbc51c7f63..837b4ad37f25aebbe61623e156544cf01d1c0c9e
 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -54,6 +54,7 @@ static char *std_principal_attrs[] = {
 "krbLastSuccessfulAuth",
 "krbLastFailedAuth",
 "krbLoginFail

Re: [Freeipa-devel] [PATCH 0088-0095] Add --forward-policy option into installers

[Petr Vobornik]

On 04/28/2016 06:29 PM, Martin Basti wrote:
> 
> 
> On 28.04.2016 17:27, Petr Spacek wrote:
>> On 28.4.2016 16:14, Martin Basti wrote:
>>>
>>> On 22.04.2016 14:35, Petr Spacek wrote:
 On 12.4.2016 17:26, Martin Basti wrote:
> On 04.04.2016 17:37, Petr Spacek wrote:
>> On 31.3.2016 13:45, Martin Basti wrote:
>>> On 21.03.2016 16:51, Petr Spacek wrote:
 On 10.3.2016 22:17, Lukas Slebodnik wrote:
> On (10/03/16 22:14), Petr Spacek wrote:
>> Hello,
>>
>> I forgot to send a patches before I leave, so here it is:
>>
>> Auto-detect default value for --forward-policy option in
>> installers
>>
>> See
>> https://fedorahosted.org/freeipa/ticket/5710
>> commit messages, and design page
>> https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones
>>
>>
>>
>>
>>
>>
>> I did not have time to test it thoroughly but it LGTM :-D
>>
>> Please note that this is first part, it does not solve upgrade
>> (yet) and
>> warnings in forwardzone-* interface.
>>
>> This can be solved in another patch set, this can be pushed if
>> it passes
>> review.
>>
> ENOPATH
 LOL, here it is.



>>> * Remove function ipapython.ipautil.host_exists() *
>>> ACK
>>>
>>>
>>> * Extend installers with --forward-policy option *
>>> 1)
>>> There is no --forward-policy option in ipa-dns-install
>>>
>>>
>>> * Move automatic empty zone list into ipapython.dnsutil and make it
>>> reusable *
>>> ACK
>>>
>>>
>>> * Add assert_absolute_dnsname() helper to ipapython.dnsutil *
>>> ACK
>>>
>>>
>>> * Move function is_auto_empty_zone() into ipapython.dnsutil *
>>> ACK
>>>
>>>
>>> * Use shared sanity check and tests
>>> ipapython.dnsutil.is_auto_empty_zone() *
>>> ACK
>>>
>>> * Add function ipapython.dnsutil.inside_auto_empty_zone() *
>>> ACK
>>>
>>> * Auto-detect default value for --forward-policy option in
>>> installers *
>>> LGTM, but ipa-dns-install is missing option --forward-policy
>>>
>>> # ipa-dns-install
>>> ...
>>> Unexpected error - see /var/log/ipaserver-install.log for details:
>>> AttributeError: Values instance has no attribute 'forward_policy'
>>>
>>>
>>> Summary: 6 ACKs, 1 LGTM, 1 NACK => NACK
>> Thank you very much for review.
>>
>> Here is my second attempt :-)
>>
> Hello,
> code works as expected, but it is quite inconsistent with current
> behavior
>
> ipa-server-install --forward-policy should raise error without
> --setup-dns
> option
>
> Like here:
> [root@vm-058-134 ~]# ipa-server-install --forwarder=10.2.3.4
> Usage: ipa-server-install [options]
>
> ipa-server-install: error: You cannot specify a --forwarder option
> without the
> --setup-dns option
> ipa.ipapython.install.cli.install_tool(Server): ERRORThe
> ipa-server-install command failed. See
> /var/log/ipaserver-install.log for more
> information
>
> Martin
 Fixed patches are attached. Thank you for your time!

>>> almost ack but patches need rebase on  ipa-4-3
>> Here you go.
>>
> 
> ACK (somebody have to push it, because I'm banned from fedora as spambot
> :) and it will take time until system unblock me due to sync)
> 

pushed to

master:
* 9ee6d379c496cd1648d58a6ecca72ed41638a3f1 Remove function
ipapython.ipautil.host_exists()
* 89974548891baa6dbbab401913359e398a2cbc57 Extend installers with
--forward-policy option
* bd32b48eb0180b73c3bd769b7ea2b369a095c000 Move automatic empty zone
list into ipapython.dnsutil and make it reusable
* 41464b74f43ab0a7f9ad650bdaac19308fc7ff5c Add assert_absolute_dnsname()
helper to ipapython.dnsutil
* 6752d6404af13a933105e53ea5875adfb933e293 Move function
is_auto_empty_zone() into ipapython.dnsutil
* 1df30b4646d0738c5d218acf9f65b6617a970c98 Use shared sanity check and
tests ipapython.dnsutil.is_auto_empty_zone()
* c7ee765c4de086ac92922519d7065fc6b6796f10 Add function
ipapython.dnsutil.inside_auto_empty_zone()
* 51907d5bb8fce9e5358fed50c0ec7074ef7f0c69 Auto-detect default value for
--forward-policy option in installers

ipa-4-3:
* 6dbc4ccbe42835780f7716d96ec45a9b94ed8b64 Remove function
ipapython.ipautil.host_exists()
* 54e26799540cbadafa08d1e975e46f261c521c81 Extend installers with
--forward-policy option
* 5c53cf2cd687fdd77a62dc12ed359953f547b86a Move automatic empty zone
list into ipapython.dnsutil and make it reusable
* 4a8dcc1cac0d96bf12fb608d3b12e81bbdf2503b Add assert_absolute_dnsname()
helper to ipapython.dnsutil
* ea1bf611f8b027f6a7140c532d1940eada2c6071 Move function
is_auto_empty_zone() into ipapython.dnsutil
* f2cf30d38cb28fb6f73a2df6a436658e548d2b10 Use shared sanit

Re: [Freeipa-devel] [PATCH 0088-0095] Add --forward-policy option into installers

[Martin Basti]

On 28.04.2016 17:27, Petr Spacek wrote:

On 28.4.2016 16:14, Martin Basti wrote:


On 22.04.2016 14:35, Petr Spacek wrote:

On 12.4.2016 17:26, Martin Basti wrote:

On 04.04.2016 17:37, Petr Spacek wrote:

On 31.3.2016 13:45, Martin Basti wrote:

On 21.03.2016 16:51, Petr Spacek wrote:

On 10.3.2016 22:17, Lukas Slebodnik wrote:

On (10/03/16 22:14), Petr Spacek wrote:

Hello,

I forgot to send a patches before I leave, so here it is:

Auto-detect default value for --forward-policy option in installers

See
https://fedorahosted.org/freeipa/ticket/5710
commit messages, and design page
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones





I did not have time to test it thoroughly but it LGTM :-D

Please note that this is first part, it does not solve upgrade (yet) and
warnings in forwardzone-* interface.

This can be solved in another patch set, this can be pushed if it passes
review.


ENOPATH

LOL, here it is.




* Remove function ipapython.ipautil.host_exists() *
ACK


* Extend installers with --forward-policy option *
1)
There is no --forward-policy option in ipa-dns-install


* Move automatic empty zone list into ipapython.dnsutil and make it
reusable *
ACK


* Add assert_absolute_dnsname() helper to ipapython.dnsutil *
ACK


* Move function is_auto_empty_zone() into ipapython.dnsutil *
ACK


* Use shared sanity check and tests ipapython.dnsutil.is_auto_empty_zone() *
ACK

* Add function ipapython.dnsutil.inside_auto_empty_zone() *
ACK

* Auto-detect default value for --forward-policy option in installers *
LGTM, but ipa-dns-install is missing option --forward-policy

# ipa-dns-install
...
Unexpected error - see /var/log/ipaserver-install.log for details:
AttributeError: Values instance has no attribute 'forward_policy'


Summary: 6 ACKs, 1 LGTM, 1 NACK => NACK

Thank you very much for review.

Here is my second attempt :-)


Hello,
code works as expected, but it is quite inconsistent with current behavior

ipa-server-install --forward-policy should raise error without --setup-dns
option

Like here:
[root@vm-058-134 ~]# ipa-server-install --forwarder=10.2.3.4
Usage: ipa-server-install [options]

ipa-server-install: error: You cannot specify a --forwarder option without the
--setup-dns option
ipa.ipapython.install.cli.install_tool(Server): ERRORThe
ipa-server-install command failed. See /var/log/ipaserver-install.log for more
information

Martin

Fixed patches are attached. Thank you for your time!


almost ack but patches need rebase on  ipa-4-3

Here you go.



ACK (somebody have to push it, because I'm banned from fedora as spambot 
:) and it will take time until system unblock me due to sync)


--
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 0024] ipa-replica-manage: added --suffix option for certain commands

[Martin Basti]

On 25.04.2016 10:39, Stanislav Laznicka wrote:

On 04/22/2016 05:15 PM, Stanislav Laznicka wrote:

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



On 15.04.2016 14:30, Stanislav Laznicka wrote:

On 04/13/2016 01:40 PM, Martin Basti wrote:



On 06.04.2016 14:04, Stanislav Laznicka wrote:

On 03/30/2016 04:52 PM, Martin Basti wrote:

On 24.03.2016 19:10, Stanislav Laznicka wrote:

On 03/23/2016 08:13 PM, Martin Basti wrote:

[...]
Can you please update design
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 
(mainly
the --suffix option)? Also there are missing clean-ruv and 
list-ruv

commands in design, and fix usage at the bottom.

1)
I don't understand this expression
+if dirman_passwd is None or (
+   not dirman_passwd and args[0] in 
cs_enabled_commands):


You already tested if subcommand belongs to 
cs_enabled_commands few
lines above, IMO the 'dirman_password is None' expression is 
enough.
If I understand it well, when empty password is entered, the 
program continues and uses Kerberos credentials for some 
operations. E.g. for list-ruv, if empty password is entered, 
the command would only display RUVs for domain tree but not for 
the CA tree no matter if CA is set up or not (in the current 
state of the patch, after get_ruv refactoring). This here is 
one possible way around this, although the check for non-empty 
password might probably just as well be in get_ruv_both_suffixes.


ok

2)
+# tuple of commands that work with ca tree and need Directory 
Manager

password
+cs_enabled_commands = ("list-ruv", "clean-ruv", 
"abort-clean-ruv")


this variable is used only toi detect if dirman passwd is 
needed, I
suggest to rename it to commands_req_dirman_passwd, or 
something better.


3)
Q: Do we need is_cs_set() function?
A: Yes!

I wanted to give you ultimate NACK, but then I checked how 
get_ruv code

works and I changed my mind.

Please write a comment where is_cs_set function is used, why 
we need
extra function instead of catching an exception, possibly you 
can open a

refactoring ticket.
After the refactoring changes, is_cs_set should not be needed 
anymore, removed it.


Shame:
1)
+if not test_connection(realm, host, options.nolookup) 
or\

Please use parentheses instead of backslash

2)
+   args[0] in cs_enabled_commands:

+   not dirman_passwd and args[0] in 
cs_enabled_commands):


Indentation is not multiplication of 4

Shame on me indeed, fixed it.


Nitpicks (I don't insist on fixing these):
1)
+if servers.get('ca', None):

None is default

2)
+for (netloc, rid) in servers['ca']:
parentheses are not needed

3)
+ print("\t%s: %s" % (netloc, rid))
Would be nice to use .format() instead of %

Martin^2



I changed my mind, ultimate NACK.
Please fix get_ruv function, is_cs_set will not help. In case 
there are no RUVs but CA is installed, sys.exit there prevents 
us from removing RUVs (or any else operation) on domain 
suffix, and vice versa.
I propose to move ticket to 4.4 milestone because I would like 
to avoid breaking something in 4.3, as this will hit many 
places in ipa-replica-manage.


Please provide the refactoring of get_ruv as separate patch a 
put these patches on top of it.


Martin2
Did the refactoring. There also seemed to be duplicit code in 
abort_clean_ruv for some reason, removed it (I hope it does not 
break anything, but it seemed rather useless). Also had to 
change the numbers of the patches so that they would apply.


NACK

* ipa-replica-manage refactoring *

1)
Instead of:
-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug("Failed to connect to server {host}: 
{err}"

+  .format(host=host, err=e))
+raise RuntimeError(e)

I expected

-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug(traceback.format_exc())
+raise RuntimeError("Failed to connect to server {host}: 
{err}"

+ .format(host=host, err=e)))

Should be fixed now.


2)
-print("No RUV records found.")
-sys.exit(0)

Here is exit state 0, so we should not raise error.

I think that we should create new nonfatal exception.
Made a new nonfatal error exception, then. This step was a bit 
controversial when it comes to get_ruv_both_suffixes because it 
needs to catch both this new exception and a RuntimeError 
exception for both trees. As the main idea here was not to stop 
if either tree is missing (resulting in RuntimeError in get_ruv) 
or contains no records (NonFatalError), it is only printed that 
something bad may have happened (or it's debug-logged in case of 
nonfatal errors). In the end, only NonFatalError exception is 
raised if no records were found for whatever reason resulting in 
the script always returning 0.


3)
-print("unable to decode: %s" % ruv)
+root_logger.debug("unable to decode: %s" % ruv)
This may break

Re: [Freeipa-devel] [PATCH 0088-0095] Add --forward-policy option into installers

[Petr Spacek]

On 28.4.2016 16:14, Martin Basti wrote:
> 
> 
> On 22.04.2016 14:35, Petr Spacek wrote:
>> On 12.4.2016 17:26, Martin Basti wrote:
>>>
>>> On 04.04.2016 17:37, Petr Spacek wrote:
 On 31.3.2016 13:45, Martin Basti wrote:
> On 21.03.2016 16:51, Petr Spacek wrote:
>> On 10.3.2016 22:17, Lukas Slebodnik wrote:
>>> On (10/03/16 22:14), Petr Spacek wrote:
 Hello,

 I forgot to send a patches before I leave, so here it is:

 Auto-detect default value for --forward-policy option in installers

 See
 https://fedorahosted.org/freeipa/ticket/5710
 commit messages, and design page
 https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones





 I did not have time to test it thoroughly but it LGTM :-D

 Please note that this is first part, it does not solve upgrade (yet) 
 and
 warnings in forwardzone-* interface.

 This can be solved in another patch set, this can be pushed if it 
 passes
 review.

>>> ENOPATH
>> LOL, here it is.
>>
>>
>>
>* Remove function ipapython.ipautil.host_exists() *
> ACK
>
>
> * Extend installers with --forward-policy option *
> 1)
> There is no --forward-policy option in ipa-dns-install
>
>
> * Move automatic empty zone list into ipapython.dnsutil and make it
> reusable *
> ACK
>
>
> * Add assert_absolute_dnsname() helper to ipapython.dnsutil *
> ACK
>
>
> * Move function is_auto_empty_zone() into ipapython.dnsutil *
> ACK
>
>
> * Use shared sanity check and tests 
> ipapython.dnsutil.is_auto_empty_zone() *
> ACK
>
> * Add function ipapython.dnsutil.inside_auto_empty_zone() *
> ACK
>
> * Auto-detect default value for --forward-policy option in installers *
> LGTM, but ipa-dns-install is missing option --forward-policy
>
> # ipa-dns-install
> ...
> Unexpected error - see /var/log/ipaserver-install.log for details:
> AttributeError: Values instance has no attribute 'forward_policy'
>
>
> Summary: 6 ACKs, 1 LGTM, 1 NACK => NACK
 Thank you very much for review.

 Here is my second attempt :-)

>>> Hello,
>>> code works as expected, but it is quite inconsistent with current behavior
>>>
>>> ipa-server-install --forward-policy should raise error without --setup-dns
>>> option
>>>
>>> Like here:
>>> [root@vm-058-134 ~]# ipa-server-install --forwarder=10.2.3.4
>>> Usage: ipa-server-install [options]
>>>
>>> ipa-server-install: error: You cannot specify a --forwarder option without 
>>> the
>>> --setup-dns option
>>> ipa.ipapython.install.cli.install_tool(Server): ERRORThe
>>> ipa-server-install command failed. See /var/log/ipaserver-install.log for 
>>> more
>>> information
>>>
>>> Martin
>> Fixed patches are attached. Thank you for your time!
>>
> almost ack but patches need rebase on  ipa-4-3

Here you go.

-- 
Petr^2 Spacek
From b23ec012a7adbc2a81775677eda6c901bfa21384 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Mon, 7 Mar 2016 10:52:35 +0100
Subject: [PATCH] Remove function ipapython.ipautil.host_exists()

The function duplicated ipalib.util.verify_host_resolvable() in slightly
incompatible way because it used NSS while rest of IPA is using only DNS.
---
 install/tools/ipa-replica-manage | 15 +--
 ipapython/ipautil.py | 14 --
 2 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index 8414fe9aeed50a20f142c7a29b6c7973adb62408..83fb9ab2ffbea195fc7f9b6549915dad99a092d3 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -38,9 +38,8 @@ from ipaserver.install import opendnssecinstance, dnskeysyncinstance
 from ipapython import version, ipaldap
 from ipalib import api, errors, util
 from ipalib.constants import CACERT
-from ipalib.util import (create_topology_graph,
-get_topology_connection_errors, has_managed_topology)
-from ipapython.ipa_log_manager import *
+from ipalib.util import has_managed_topology, verify_host_resolvable
+from ipapython.ipa_log_manager import root_logger, standard_logging_setup
 from ipapython.dn import DN
 from ipapython.config import IPAOptionParser
 from ipaclient import ipadiscovery
@@ -744,10 +743,14 @@ def check_last_link(delrepl, realm, dirman_passwd, force):
 
 
 def enforce_host_existence(host, message=None):
-if host is not None and not ipautil.host_exists(host):
+if host is None:
+return
+
+try:
+verify_host_resolvable(host, root_logger)
+except errors.DNSNotARecordError as ex:
 if message is None:
-message = "Unknown host %s" % host
-
+message = "Unknown host %s: %s" % (host, ex)
 sys.exit(message)
 
 def e

Re: [Freeipa-devel] [PATCH 0099] ipa-nis-manage: add status option

[Petr Spacek]

On 28.4.2016 14:52, Abhijeet Kasurde wrote:
> Hi Petr,
> 
> On 04/25/2016 08:28 PM, Petr Spacek wrote:
>> Hello,
>>
>> ipa-nis-manage: add status option
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1329275
>>
>>
>>
> Can you reword the error message here as well ?
> 
>  if len(args) != 1:
>  sys.exit("You must specify one action, either enable or disable")
> 
> Thanks,
> Abhijeet Kasurde

Good catch!

-- 
Petr^2 Spacek
From 839f50f980c2976cd68e6036f59221c2d60769e1 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Mon, 25 Apr 2016 16:56:08 +0200
Subject: [PATCH] ipa-nis-manage: add status option

https://bugzilla.redhat.com/show_bug.cgi?id=1329275
---
 install/tools/ipa-nis-manage   | 22 ++
 install/tools/man/ipa-nis-manage.1 |  8 ++--
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/install/tools/ipa-nis-manage b/install/tools/ipa-nis-manage
index 3aa1507b205eaa679edebda2a3705b494369abc3..1c3e0c1c5abe52a56bb98aed64a283e5e7e0bca1 100755
--- a/install/tools/ipa-nis-manage
+++ b/install/tools/ipa-nis-manage
@@ -47,7 +47,7 @@ nis_config_dn = DN(('cn', 'NIS Server'), ('cn', 'plugins'), ('cn', 'config'))
 compat_dn = DN(('cn', 'Schema Compatibility'), ('cn', 'plugins'), ('cn', 'config'))
 
 def parse_options():
-usage = "%prog [options] \n"
+usage = "%prog [options] \n"
 usage += "%prog [options]\n"
 parser = OptionParser(usage=usage, formatter=config.IPAFormatter())
 
@@ -96,8 +96,8 @@ def main():
 options, args = parse_options()
 
 if len(args) != 1:
-sys.exit("You must specify one action, either enable or disable")
-elif args[0] != "enable" and args[0] != "disable":
+sys.exit("You must specify one action: enable | disable | status")
+elif args[0] != "enable" and args[0] != "disable" and args[0] != "status":
 sys.exit("Unrecognized action [" + args[0] + "]")
 
 standard_logging_setup(None, debug=options.debug)
@@ -187,11 +187,25 @@ def main():
 print(lde)
 retval = 1
 
+elif args[0] == "status":
+nis_entry = get_entry(nis_config_dn, conn)
+enabled = (nis_entry and
+   nis_entry.get(
+   'nsslapd-pluginenabled', '')[0].lower() == "on")
+if enabled:
+print("Plugin is enabled")
+retval = 0
+else:
+print("Plugin is not enabled")
+retval = 4
+
 else:
 retval = 1
 
 if retval == 0:
-print("This setting will not take effect until you restart Directory Server.")
+if args[0] in {"enable", "disable"}:
+print("This setting will not take effect until you restart "
+  "Directory Server.")
 
 if args[0] == "enable":
 print("The %s service may need to be started." % servicemsg)
diff --git a/install/tools/man/ipa-nis-manage.1 b/install/tools/man/ipa-nis-manage.1
index d60fda788fb7b97d6fa97544f63f346eb13e956a..93278487c1adb50f4ae49804d0214841e2bef30c 100644
--- a/install/tools/man/ipa-nis-manage.1
+++ b/install/tools/man/ipa-nis-manage.1
@@ -20,13 +20,15 @@
 .SH "NAME"
 ipa\-nis\-manage \- Enables or disables the NIS listener plugin
 .SH "SYNOPSIS"
-ipa\-nis\-manage [options] 
+ipa\-nis\-manage [options] 
 .SH "DESCRIPTION"
 Run the command with the \fBenable\fR option to enable the NIS plugin.
 
 Run the command with the \fBdisable\fR option to disable the NIS plugin.
 
-In both cases the user will be prompted to provide the Directory Manager's password unless option \fB\-y\fR is used.
+Run the command with the \fBstatus\fR option to read status of the NIS plugin. Return code 0 indicates enabled plugin, return code 4 indicates disabled plugin.
+
+In all cases the user will be prompted to provide the Directory Manager's password unless option \fB\-y\fR is used.
 
 Directory Server will need to be restarted after the NIS listener plugin has been enabled.
 
@@ -45,3 +47,5 @@ File containing the Directory Manager password
 2 if the plugin is already in the required status (enabled or disabled)
 
 3 if RPC services cannot be enabled.
+
+4 if status command detected plugin in disabled state.
-- 
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] Added fix for notifying user about locked user account in WebUI

[Martin Basti]

On 28.04.2016 16:16, Pavel Vomacka wrote:




On 04/20/2016 07:52 AM, Abhijeet Kasurde wrote:

Hi All,

Please review this patch.

Fixes : https://fedorahosted.org/freeipa/ticket/5076

Thanks,
Abhijeet Kasurde




Hi,

Javascript part LGTM.

--
Pavel^3 Vomacka



ACK

Pushed to master: 3d07c889ce21ffe1d8baec3fd0c13bc67aa1d725

-- 
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] More Python 3 fixes

[Martin Basti]

On 26.04.2016 15:31, Petr Viktorin wrote:

Hello,
Here are two patches for problems with using IPA with Python 3.





ACK

Pushed
master:
* 28b0bfaefea02fe188b57b602396f5ef62863726 dns plugin: Fix zone 
normalization under Python 3
* 05cb4ba4e97d8cbffaf1c16451c488db4a90a878 sysrestore: Iterate over a 
list of dict keys

ipa-4-3:
* 1e228f27e9d6314b5cdc58c772c350b0592ebe86 dns plugin: Fix zone 
normalization under Python 3
* c8bcf4287f9aa881a6517f26adce001b3dbcd1c2 sysrestore: Iterate over a 
list of dict keys


-- 
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] Added fix for notifying user about locked user account in WebUI

[Pavel Vomacka]

On 04/20/2016 07:52 AM, Abhijeet Kasurde wrote:

Hi All,

Please review this patch.

Fixes : https://fedorahosted.org/freeipa/ticket/5076

Thanks,
Abhijeet Kasurde




Hi,

Javascript part LGTM.

--
Pavel^3 Vomacka
-- 
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 0088-0095] Add --forward-policy option into installers

[Martin Basti]

On 22.04.2016 14:35, Petr Spacek wrote:

On 12.4.2016 17:26, Martin Basti wrote:


On 04.04.2016 17:37, Petr Spacek wrote:

On 31.3.2016 13:45, Martin Basti wrote:

On 21.03.2016 16:51, Petr Spacek wrote:

On 10.3.2016 22:17, Lukas Slebodnik wrote:

On (10/03/16 22:14), Petr Spacek wrote:

Hello,

I forgot to send a patches before I leave, so here it is:

Auto-detect default value for --forward-policy option in installers

See
https://fedorahosted.org/freeipa/ticket/5710
commit messages, and design page
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/AutomaticEmptyZones




I did not have time to test it thoroughly but it LGTM :-D

Please note that this is first part, it does not solve upgrade (yet) and
warnings in forwardzone-* interface.

This can be solved in another patch set, this can be pushed if it passes
review.


ENOPATH

LOL, here it is.




   * Remove function ipapython.ipautil.host_exists() *
ACK


* Extend installers with --forward-policy option *
1)
There is no --forward-policy option in ipa-dns-install


* Move automatic empty zone list into ipapython.dnsutil and make it reusable *
ACK


* Add assert_absolute_dnsname() helper to ipapython.dnsutil *
ACK


* Move function is_auto_empty_zone() into ipapython.dnsutil *
ACK


* Use shared sanity check and tests ipapython.dnsutil.is_auto_empty_zone() *
ACK

* Add function ipapython.dnsutil.inside_auto_empty_zone() *
ACK

* Auto-detect default value for --forward-policy option in installers *
LGTM, but ipa-dns-install is missing option --forward-policy

# ipa-dns-install
...
Unexpected error - see /var/log/ipaserver-install.log for details:
AttributeError: Values instance has no attribute 'forward_policy'


Summary: 6 ACKs, 1 LGTM, 1 NACK => NACK

Thank you very much for review.

Here is my second attempt :-)


Hello,
code works as expected, but it is quite inconsistent with current behavior

ipa-server-install --forward-policy should raise error without --setup-dns 
option

Like here:
[root@vm-058-134 ~]# ipa-server-install --forwarder=10.2.3.4
Usage: ipa-server-install [options]

ipa-server-install: error: You cannot specify a --forwarder option without the
--setup-dns option
ipa.ipapython.install.cli.install_tool(Server): ERRORThe
ipa-server-install command failed. See /var/log/ipaserver-install.log for more
information

Martin

Fixed patches are attached. Thank you for your time!


almost ack but patches need rebase on  ipa-4-3

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

Re: [Freeipa-devel] [TESTS][PATCH] Ping module tests in a non-declarative way

[Martin Basti]

On 08.04.2016 10:32, Peter Lacko wrote:





Hello,

I have a few comments:

1)
Please set up your git name and email correctly (consistently for all 
patches)


this is not right From: root

2)
-# Copyright (C) 2012  Red Hat
+# Copyright (C) 2016  Red Hat

leave there both years please
+# Copyright (C) 2012, 2016  Red Hat

3)
Please put the patch number to the email subject, it is easier to find correct 
patch for us

Otherwise LGTM and works for me.

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

Re: [Freeipa-devel] [PATCH 0099] ipa-nis-manage: add status option

[Abhijeet Kasurde]

Hi Petr,

On 04/25/2016 08:28 PM, Petr Spacek wrote:

Hello,

ipa-nis-manage: add status option

https://bugzilla.redhat.com/show_bug.cgi?id=1329275




Can you reword the error message here as well ?

 if len(args) != 1:
 sys.exit("You must specify one action, either enable or disable")

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

Re: [Freeipa-devel] [PATCH] 0003 webui: Issue New Certificate dialogs validates data

[Martin Basti]

On 28.04.2016 14:42, Martin Basti wrote:



On 28.01.2016 17:55, Pavel Vomacka wrote:

Hello,

On 01/28/2016 12:20 PM, Pavel Vomacka wrote:
'Issue new certificate' dialog now validates whether user fills 
'principal' and 'csr' field.
In case that one of these fields is empty then it does not allow to 
submit the dialog.


https://fedorahosted.org/freeipa/ticket/5432
I'm sending edited patch. One useless section is removed. The 
textarea for CSR is moved to another section which has the same 
layout. The method for creating content was useless, too. So it is 
removed in this patch.


--
Pavel Vomacka



Bump for review



OK, this thread is duplicated, patch has been pushed.
-- 
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] [WIP] Thin client

[Jan Cholasta]

Hi,

I have pushed my thin client WIP branch to GitHub: 
.


All commits up to "ipalib: use relative imports for cross-plugin 
imports" should be good for review. The rest is subject to change 
(WARNING: I will force push into this branch).


Honza

--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH] 0003 webui: Issue New Certificate dialogs validates data

[Martin Basti]

On 28.01.2016 17:55, Pavel Vomacka wrote:

Hello,

On 01/28/2016 12:20 PM, Pavel Vomacka wrote:
'Issue new certificate' dialog now validates whether user fills 
'principal' and 'csr' field.
In case that one of these fields is empty then it does not allow to 
submit the dialog.


https://fedorahosted.org/freeipa/ticket/5432
I'm sending edited patch. One useless section is removed. The textarea 
for CSR is moved to another section which has the same layout. The 
method for creating content was useless, too. So it is removed in this 
patch.


--
Pavel Vomacka



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

Re: [Freeipa-devel] [PATCH 0097-0098] Makefile: replace perl with sed

[Martin Basti]

On 25.04.2016 09:39, Lukas Slebodnik wrote:

On (22/04/16 13:29), Petr Spacek wrote:

Hello,

Makefile: add sed to BuildRequires

It was requried since forever but we did not explicitly mention it.

Makefile: replace perl with sed

Perl was missing in BuildRequires anyway and it is used only on one place,
all other places are using sed.

--
Petr^2 Spacek

>From 3c7a3c87d62358407d856119e384c70040acec1e Mon Sep 17 00:00:00 2001

From: Petr Spacek 
Date: Fri, 22 Apr 2016 10:40:11 +0200
Subject: [PATCH] Makefile: replace perl with sed

Perl was missing in BuildRequires anyway and it is used only on one place,
all other places are using sed.
---
Makefile | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)


ACK

>From 2deaef91ab71c0e78b2142c2102cfe65f0e4ed96 Mon Sep 17 00:00:00 2001

From: Petr Spacek 
Date: Fri, 22 Apr 2016 10:40:37 +0200
Subject: [PATCH] Makefile: add sed to BuildRequires

It was requried since forever but we did not explicitly mention it.
---
freeipa.spec.in | 1 +
1 file changed, 1 insertion(+)


NACK

As it was mentioned elsewhere in thread this patch is not needed.

LS


pushed to master:
* 8689e6be515aaf5b3d6c891e9e450b99cd8af8d4 Makefile: replace perl with sed

--
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 0099] ipa-nis-manage: add status option

[Petr Vobornik]

On 04/25/2016 04:58 PM, Petr Spacek wrote:
> Hello,
> 
> ipa-nis-manage: add status option
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1329275
> 

Trac ticket https://fedorahosted.org/freeipa/ticket/5856
-- 
Petr Vobornik

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

Re: [Freeipa-devel] [PATCH] Added warning to user for Internet Explorer

[Martin Basti]

On 27.04.2016 19:05, Rob Crittenden wrote:

Abhijeet Kasurde wrote:

Updated patch attached.

On 04/26/2016 09:07 PM, Rob Crittenden wrote:

Internet Explorer is no longer a supported browser.




ACK


pushed to master:
* f61910084d737f66b9914f3f945e9830a393c8af Added warning to user for 
Internet Explorer


--
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 0405] idviews: Add user certificate attribute to user ID overrides

[Tomas Babej]

On 04/19/2016 08:20 AM, Jan Cholasta wrote:
> On 13.4.2016 14:13, Tomas Babej wrote:
>> On 04/13/2016 09:55 AM, Tomas Babej wrote:
>>> On 04/07/2016 01:53 PM, Sumit Bose wrote:
 On Mon, Apr 04, 2016 at 04:27:02PM +0200, Jan Cholasta wrote:
> Hi,
>
> On 1.4.2016 16:53, Tomas Babej wrote:
>> Hi,
>>
>> this extends the user ID overrides with capability to store the user
>> certificate.
>>
>> https://fedorahosted.org/freeipa/ticket/4955
>
> The preferred way of managing certificates nowadays is using
> $OBJ-add-cert
> and $OBJ-remove-cert commands, you should add them here as well.
>
> I would even go as far as not allowing to modify certificates using
> idoverrideuser-mod - in user-mod and host-mod, it's there just for
> backward
> compatibility, which is not the case here. But I don't have a
> strong opinion
> on that.
>
> For consistency with user-find and host-find, the full certificate
> blob
> should not be shown in idoverrideuser-find. You can do that by setting
> search_display_attributes attribute on the idoverrideuser class
> appropriately.

 I tested the current patch with my related patches for SSSD and all is
 working as expected.

 bye,
 Sumit

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

>>>
>>> Thanks for the reviews,
>>>
>>> attaching a updated patch that addresses Honza's comments.
>>>
>>> Tomas
>>>
>>
>> Sending an improved version addressing a couple of additional issues.
> 
> 1) This bit in idoverrideuser_add.pre_callback() is redundant, as the
> certificate will always be DER here already:
> 
> # Normalize the certificate to DER format
> certs = options.get('usercertificate', [])
> certs_der = [x509.normalize_certificate(c) for c in certs]
> entry_attrs['usercertificate'] = certs_der
> 
> 
> 2) You need to call convert_usercertificate_pre() in
> idoverrideuser_mod.pre_callback() and convert_usercertificate_post() in
> idoverrideuser_{mod,find,show}.post_callback() as well.
> 
> Honza
> 

Updated patch attached, mentioned issues should be fixed, I also removed
one redundant import which escaped my careful eye.

Tomas
From ecfb6dbfb39120fa1c2caf83fd0d6c22471c212d Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 3 Mar 2016 15:14:10 +0100
Subject: [PATCH] idviews: Add user certificate attribute to user ID overrides

---
 ACI.txt  |  2 +-
 API.txt  | 30 +++--
 VERSION  |  4 +--
 install/share/71idviews.ldif |  2 +-
 ipalib/plugins/idviews.py| 79 ++--
 5 files changed, 109 insertions(+), 8 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 24cb332ce6e10c82a5bfab76d084fb6c0277800d..ae00cf7a1b8e2ea0e33798993bb24dc5f06127e3 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -149,7 +149,7 @@ aci: (targetfilter = "(objectclass=ipahostgroup)")(version 3.0;acl "permission:S
 dn: cn=views,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || description || entryusn || gidnumber || ipaanchoruuid || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaGroupOverride)")(version 3.0;acl "permission:System: Read Group ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=views,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "createtimestamp || description || entryusn || gecos || gidnumber || homedirectory || ipaanchoruuid || ipaoriginaluid || ipasshpubkey || loginshell || modifytimestamp || objectclass || uid || uidnumber")(targetfilter = "(objectclass=ipaUserOverride)")(version 3.0;acl "permission:System: Read User ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
+aci: (targetattr = "createtimestamp || description || entryusn || gecos || gidnumber || homedirectory || ipaanchoruuid || ipaoriginaluid || ipasshpubkey || loginshell || modifytimestamp || objectclass || uid || uidnumber || usercertificate")(targetfilter = "(objectclass=ipaUserOverride)")(version 3.0;acl "permission:System: Read User ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ranges,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || ipabaseid || ipabaserid || ipaidrangesize || ipanttrusteddomainsid || iparangetype || ipasecondarybaserid || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaidrange)")(version 3.0;acl "permission:System: Read ID Ranges";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=views,cn=accounts,dc=ipa,dc=example
diff --git a/API.txt b/API.txt
index 3598b08198cae536754259f7463669052efa3f86..b2aec7313b6b9496179beddb68e4a0f5a09608bf 100644
--- a/API.txt
+++ b/API.