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

[Jan Cholasta]

On 25.11.2015 18:46, Simo Sorce wrote:

On Wed, 2015-11-25 at 10:25 +0100, Jan Cholasta wrote:

On 20.11.2015 16:49, Jan Cholasta wrote:

On 19.11.2015 17:43, Simo Sorce wrote:

510:
- We should probably tightenup the ACI to allos host X to only add
memberPrincipal = X and no other value, also the host should not be
allowed to change the memberPrincipal attribute only the keys.
If we can't express this in ACIs we can live with the ones you propose
though.


I think this can be done.


Turns out this can be done only if member (or some other DN attribute)
is used instead of memberPrincipal.

So, to reiterate:


2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?

If 'member' was used instead, we would gain referential integrity and
the ability to add ACIs based on the attribute (think
userattr="member#USERDN").


To avoid referential integrity and mixup with other group objects, it
was intentional.


Why is referential integrity a problem?


Because it will remove the member if the object it references goes away,
and I do not want an "orphaned" entry for custodia.


But without referential integrity you get an orphaned entry too, except 
with an extra dangling reference. IMHO that's even worse than "plain" 
orhpaned entry, because you can't spot it just by looking at the 
attribute value.





Mixup with other group objects can be solved by using a different attribute.


There is also the fact in future we may want to use this with "external"
principals (like in IPA-IPA trusts or similar) so I didn't want to have
to come up with bogus DNs in that case.


IIRC Alexander was working on something like exposing external 
principals in LDAP using the compat plugin, in order to allow external 
users to run IPA commands.


Alternatively, it could do what groups do - use DN for internal 
references and string (be it principal or something else) for external 
references.


Anyway, either memberPrincipal is replaced with a member-like attribute, 
or the ACI stays as it is. I would prefer a member-like attribute, 
because I feel that's the way LDAP entries should reference each other, 
but I will leave the decision to you.


--
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] [pytest-multihost-devel][PATCH] Functions for handling various file and directory operations

[Abhijeet Kasurde]

Hi Petr,

On 11/25/2015 08:27 PM, Petr Viktorin wrote:

On 11/25/2015 10:08 AM, Abhijeet Kasurde wrote:

Hi All,

Please find the patch for pytest-multihost-plugin.

Fixes : https://fedorahosted.org/python-pytest-multihost/ticket/2

Thanks! These will be useful.

ACK, pushed as e7bf95b3ba4ca84b73abffda1abcf6187c5c8a67
I wrote some tests for the file operations as well:
5e76908430bbb56ca981b04a88f12359c73c7e1f


Thanks for adding testcases for these functions.

I plan to do a release (and update the Fedora package) after IPA starts
testing under Python 3. Let me know if an earlier release would help you.
Yes, indeed. It will be very helpful. Let me know if you need any help 
for my side.




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] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Jan Cholasta]

Works for me.

On 25.11.2015 21:35, Oleg Fayans wrote:

Hi,

Should I cover ticket N 3416 in the replica promotion test plan? It
should be tested, and IMO there is no sense in creating a separate test
plan for just that.

On 11/19/2015 03:43 PM, Jan Cholasta wrote:

Hi,

the attached patches fix 
and .

I worked around the issue of checking if the user is privileged to
perform replica promotion by using host credentials instead. The host
must be a member of the IPA servers host group "ipaservers" in order to
be able to promote itself. Using host credentials will also allow
replica install using one-time password.

User credentials are still used for connection check and to
automatically add the host to ipaservers if the user is privileged to do
that.

Simo, is this approach OK? Could you check the new ACIs in patches 510
and 513?

I have a couple of questions:

1) Why are custodia keys for the replica added to LDAP using connection
to the remote master instead of local ldapi connection? Is it to
eliminate race conditions caused by replication timeout from the replica
to the remote master?

If the code was changed to use ldapi and wait until the key appears in
custodia on the remote master, we could lose the "IPA server hosts can
create own Custodia secrets" and "IPA server hosts can manage own
Custodia secrets" ACIs from patch 510. Not sure if it's worth the change
though.

2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?

If 'member' was used instead, we would gain referential integrity and
the ability to add ACIs based on the attribute (think
userattr="member#USERDN").

3) Why is 'memberPrincipal' used in cn=custodia at all?

The hostname of the replica is already in 'cn', so instead of searching
cn=custodia for entries matching (memberPrincipal=host/$HOSTNAME), we
could get cn={enc,sig}/$HOSTNAME,cn=custodia directly.

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] Allow ipa-getkeytab to find server name from config file

[Fraser Tweedale]

On Wed, Nov 25, 2015 at 09:44:09AM -0500, Simo Sorce wrote:
> On Wed, 2015-11-25 at 14:34 +1000, Fraser Tweedale wrote:
> > On Tue, Nov 24, 2015 at 02:36:17PM -0500, Simo Sorce wrote:
> > > On Tue, 2015-11-24 at 17:34 +0100, Jan Cholasta wrote:
> > > > On 24.11.2015 17:30, Simo Sorce wrote:
> > > > > On Tue, 2015-11-24 at 09:14 +0100, Jan Cholasta wrote:
> > > > >> On 24.11.2015 09:06, Petr Spacek wrote:
> > > > >>> On 24.11.2015 07:32, Jan Cholasta wrote:
> > > >  On 23.11.2015 21:18, Simo Sorce wrote:
> > > > > Fixes #2203 by reading the server name from /etc/ipa/default.conf 
> > > > > if not
> > > > > provided on the command line.
> > > > >
> > > > > Simo.
> > > > 
> > > >  Just a thought: it would be nice if we had libipaconfig and used 
> > > >  it everywhere
> > > >  (the framework, ipa-getkeytab, certmonger, ...). I don't like that 
> > > >  there are
> > > >  at least 3 ipa config parser implementations now, each with its 
> > > >  own quirks.
> > > > >>>
> > > > >>> Yeah, define a Augeas grammar for IPA config and call it. There is 
> > > > >>> C and
> > > > >>> Python binding in RHEL 7.2 already.
> > > > >>
> > > > >> This won't be sufficient, as it should go beyond just parsing the 
> > > > >> config
> > > > >> file - there are dynamic defaults and backward compatiblity support
> > > > >> which should be the same everywhere too.
> > > > >
> > > > > This is a much bigger task, that someone keen on big refactoring may
> > > > > undertake, but I do not think we should delay this useful feature for
> > > > > that. It will be really easy to change this code to use whatever other
> > > > > ipa library when it materializes.
> > > > >
> > > > > For now this patch stands IMHO.
> > > > 
> > > > Sure, I'm not suggesting otherwise.
> > > 
> > > Rebased patch that includes proper spec file buildrequires.
> > > 
> > > Simo.
> > > 
> > Tested and works as expected (great usability win IMO).
> > 
> > Man page should be updated accordingly.
> > 
> > Cheers,
> > Fraser
> 
> Attached new patch that includes manpage changes.
> 
> Simo.
> 
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] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Oleg Fayans]

Hi,

Should I cover ticket N 3416 in the replica promotion test plan? It 
should be tested, and IMO there is no sense in creating a separate test 
plan for just that.


On 11/19/2015 03:43 PM, Jan Cholasta wrote:

Hi,

the attached patches fix 
and .

I worked around the issue of checking if the user is privileged to
perform replica promotion by using host credentials instead. The host
must be a member of the IPA servers host group "ipaservers" in order to
be able to promote itself. Using host credentials will also allow
replica install using one-time password.

User credentials are still used for connection check and to
automatically add the host to ipaservers if the user is privileged to do
that.

Simo, is this approach OK? Could you check the new ACIs in patches 510
and 513?

I have a couple of questions:

1) Why are custodia keys for the replica added to LDAP using connection
to the remote master instead of local ldapi connection? Is it to
eliminate race conditions caused by replication timeout from the replica
to the remote master?

If the code was changed to use ldapi and wait until the key appears in
custodia on the remote master, we could lose the "IPA server hosts can
create own Custodia secrets" and "IPA server hosts can manage own
Custodia secrets" ACIs from patch 510. Not sure if it's worth the change
though.

2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?

If 'member' was used instead, we would gain referential integrity and
the ability to add ACIs based on the attribute (think
userattr="member#USERDN").

3) Why is 'memberPrincipal' used in cn=custodia at all?

The hostname of the replica is already in 'cn', so instead of searching
cn=custodia for entries matching (memberPrincipal=host/$HOSTNAME), we
could get cn={enc,sig}/$HOSTNAME,cn=custodia directly.

Honza





--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

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

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

[Martin Basti]

Hi,

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


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


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


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


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


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


4)

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


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

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

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

[PATCH] Enabled setting domain_level per class derived from TestIntegration

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


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


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


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


Also this should go away

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

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


Martin

On 25.11.2015 16:44, Oleg Fayans wrote:

Hi,

Here is the updated version of the patch (more tests + fixed the 
issues of the first one) + patch 0017, that implements the necessary 
changes in the background code, i. e. patch 16 does not work without 
patch 17


On 11/18/2015 05:20 PM, Martin Basti wrote:



On 09.11.2015 15:09, Oleg Fayans wrote:

Hi guys,

Here are first two automated testcases from this (so far incomplete)
testplan: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan

Testplan review is highly appreciated





PATCH 16: NACK

1)
What is the reason to add an unused parameter to 'domain_level' to
install_topo()?
Also it is good practise to add new option as the last parameter.

2)
cab you in both tests specify a domain level with constant instead of
number literal?

3)
both test call install_topo with custom domain level, but it cannot work
because 1)  (did you run the test?)

4)
How the test "TestLevel1" is supposed to work?
Respectively why there is call of install_topo() that installs replica.
As this test just tests that ipa-replica-prepare is not working anymore,
is it worth to spend 20 minutes with installing replica and then just no
tot use it? IMO to install master in install step is enough.

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] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Simo Sorce]

On Wed, 2015-11-25 at 10:25 +0100, Jan Cholasta wrote:
> On 20.11.2015 16:49, Jan Cholasta wrote:
> > On 19.11.2015 17:43, Simo Sorce wrote:
> >> 510:
> >> - We should probably tightenup the ACI to allos host X to only add
> >> memberPrincipal = X and no other value, also the host should not be
> >> allowed to change the memberPrincipal attribute only the keys.
> >> If we can't express this in ACIs we can live with the ones you propose
> >> though.
> >
> > I think this can be done.
> 
> Turns out this can be done only if member (or some other DN attribute) 
> is used instead of memberPrincipal.
> 
> So, to reiterate:
> 
> >>> 2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?
> >>>
> >>> If 'member' was used instead, we would gain referential integrity and
> >>> the ability to add ACIs based on the attribute (think
> >>> userattr="member#USERDN").
> >>
> >> To avoid referential integrity and mixup with other group objects, it
> >> was intentional.
> 
> Why is referential integrity a problem?

Because it will remove the member if the object it references goes away,
and I do not want an "orphaned" entry for custodia.

> Mixup with other group objects can be solved by using a different attribute.

There is also the fact in future we may want to use this with "external"
principals (like in IPA-IPA trusts or similar) so I didn't want to have
to come up with bogus DNs in that case.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

-- 
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 560] Allow to set allowed krb authz data type per user

[Simo Sorce]

On Wed, 2015-11-25 at 08:09 +0100, Jan Cholasta wrote:
> On 25.11.2015 00:09, Simo Sorce wrote:
> > This patch is untested and mostly an RFC.
> >
> > I think it is all we need to allow to specify authz data types per user
> > and by setting the attribute to NONE preventing a user from getting
> > MS-PAC data in their ticket.
> >
> > Alexander you changed quite a bit the code around here so I'd like to
> > know if you think the change I made in the KDC will cause any issue with
> > the special PACs we generate for master's principals. As far as I can
> > tell it shouldn't.
> >
> > Any opinion is welcome.
> 
> Before your change, the server entry was checked for AS requests, now 
> only the client entry is checked for AS requests. I'm not very familiar 
> with ipa-kdb, but shouldn't the server entry still be checked as a 
> fallback when there is no authorization data in the client entry?

This is partly why I CCed Alexander, the way the get function works is
that it will get policy on the entry itself and if nothing is there it
will try with the global policy, so in both cases the global policy is
sourced as fallback.

For AS requests though you are generally asking for a TGT so the
"server" is the krbtgt entry that has no policy. It is through though
that a client *can* ask for a ticket directly via an AS request, that is
uncommon and it is unclear to me what we should do in that case if
client and server have incompatible options.

Well this is why it is a RFC after all :)

> The attribute is exposed in the service plugin, shouldn't it be exposed 
> in the user plugin as well?

I didn't do it on purpose yet but eventually we may want to expose it,
indeed. The reason I didn't is that we may want to use something like
CoS to populate the attribute based on group membership and I am not
sure we want to expose it per user, up top debate.

> Nitpick: don't remove the space character here: "( uid )".

noted.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

-- 
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] Removed duplicate domain name validation function

[Stanislav Laznicka]

There were two functions for the same purpose. Removed one.
From 15c192fdee0390ca8b6aa923691d66b1081ffae4 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Wed, 25 Nov 2015 16:38:00 +0100
Subject: [PATCH] Removed duplicate domain name validating function

---
 ipa-client/ipa-install/ipa-client-install |  9 +---
 ipalib/plugins/dns.py | 22 ---
 ipalib/plugins/host.py|  2 +-
 ipalib/plugins/service.py |  2 +-
 ipalib/util.py| 35 +++
 ipapython/ipautil.py  | 12 ---
 6 files changed, 39 insertions(+), 43 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 05a550b..837e8bd 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -54,6 +54,7 @@ try:
 from ipapython.config import IPAOptionParser
 from ipalib import api, errors
 from ipalib import x509, certstore
+from ipalib.util import is_host_resolvable
 from ipalib.constants import CACERT
 from ipapython.dn import DN
 from ipapython.ssh import SSHPublicKey
@@ -1761,11 +1762,13 @@ def get_server_connection_interface(server):
 
 def client_dns(server, hostname, options):
 
-dns_ok = ipautil.is_host_resolvable(hostname)
-
-if not dns_ok:
+try:
+is_host_resolvable(root_logger, hostname)
+dns_ok = True
+except errors.DNSNotARecordError:
 root_logger.warning("Hostname (%s) does not have A/ record.",
 hostname)
+dns_ok = False
 
 if (options.dns_updates or options.all_ip_addresses or options.ip_addresses
 or not dns_ok):
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 830a70f..44c6933 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -51,9 +51,10 @@ from ipalib.util import (normalize_zonemgr,
  DNSSECSignatureMissingError, UnresolvableRecordError,
  EDNS0UnsupportedError, DNSSECValidationError,
  validate_dnssec_zone_forwarder_step1,
- validate_dnssec_zone_forwarder_step2)
+ validate_dnssec_zone_forwarder_step2,
+ is_host_resolvable)
 
-from ipapython.ipautil import CheckedIPAddress, is_host_resolvable
+from ipapython.ipautil import CheckedIPAddress
 from ipapython.dnsutil import DNSName
 
 if six.PY3:
@@ -1554,7 +1555,7 @@ _dns_record_options = tuple(__dns_record_options_iter())
 _dns_supported_record_types = tuple(record.rrtype for record in _dns_records \
 if record.supported)
 
-def check_ns_rec_resolvable(zone, name):
+def check_ns_rec_resolvable(zone, name, log):
 assert isinstance(zone, DNSName)
 assert isinstance(name, DNSName)
 
@@ -1563,7 +1564,9 @@ def check_ns_rec_resolvable(zone, name):
 elif not name.is_absolute():
 # this is a DNS name relative to the zone
 name = name.derelativize(zone.make_absolute())
-if not is_host_resolvable(name):
+try:
+is_host_resolvable(log, name)
+except errors.DNSNotARecordError:
 raise errors.NotFound(
 reason=_('Nameserver \'%(host)s\' does not have a corresponding '
  'A/ record') % {'host': name}
@@ -2734,7 +2737,8 @@ class dnszone_add(DNSZoneBase_add):
 
 # verify if user specified server is resolvable
 if not options['force']:
-check_ns_rec_resolvable(keys[0], entry_attrs['idnssoamname'])
+check_ns_rec_resolvable(keys[0], entry_attrs['idnssoamname'],
+self.log)
 # show warning about --name-server option
 context.show_warning_nameserver_option = True
 else:
@@ -2833,7 +2837,7 @@ class dnszone_mod(DNSZoneBase_mod):
 nameserver = entry_attrs['idnssoamname']
 if nameserver:
 if not nameserver.is_empty() and not options['force']:
-check_ns_rec_resolvable(keys[0], nameserver)
+check_ns_rec_resolvable(keys[0], nameserver, self.log)
 context.show_warning_nameserver_option = True
 else:
 # empty value, this option is required by ldap
@@ -3004,7 +3008,7 @@ class dnsrecord(LDAPObject):
 if options.get('force', False) or nsrecords is None:
 return
 for nsrecord in nsrecords:
-check_ns_rec_resolvable(keys[0], DNSName(nsrecord))
+check_ns_rec_resolvable(keys[0], DNSName(nsrecord), self.log)
 
 def _idnsname_pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
 assert isinstance(dn, DN)
@@ -4196,7 +4200,9 @@ class dns_resolve(Command):
 def execute(self, *args, **options):
 query=args[0]
 
-if not

Re: [Freeipa-devel] [PATCH] 928-936 webui: topology visualization

[Martin Babinsky]

On 11/25/2015 03:28 PM, Petr Vobornik wrote:

On 11/24/2015 02:09 PM, Martin Babinsky wrote:

On 11/24/2015 12:17 PM, Petr Vobornik wrote:

On 11/24/2015 12:10 PM, Ludwig Krispenz wrote:

Hi Petr,

I'm testing these patches.Two observations so far:
- in Topology->IPA Servers I see a table of my servers and the managed
suffix column I see both suffixes, ipaca and the realm,
but if I select one of the servers I Only see the realm suffix, this
was
different in the demo video


implemented in patch 927 (separate thread)


- the graph layou is not stable. If I arrange it and the try some
actions on a segment, eg try to remove one (which is rejected), the
graph does change again-


Not sure if I understand it correctly. One can set static position to a
node by double clicking on it (this needs to be somehow communicated)
otherwise it is random - each refresh.

Maybe we should not refresh on failed operation because there is no
change.

+1, failed operations on segments should not trigger graph redraws.


Fixed in patch 936-1.


Confirmed.






Ludwig

On 11/20/2015 04:00 PM, Petr Vobornik wrote:

Patches 928-931 are prerequisites.

Patches 932-934 implements the visualization

Patches 935-936 adds 'add' and 'remove' segment functionality to the
visualization page.

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




I have tested the new feature a bit (I have looked also on the code, but
I do not speak JavaScript very well) and so far I have found these
issues:

1.) If you first setup topology w/ domain level 0 and try to render
topology graph, you are correctly informed that managed topology is
available on domain level 1 only. IF you raise your domainlevel and
click refresh, you get the same error message, probably because the
request to get domainlevel is sent only in the beginning of session. You
have to logout and then login back to see the graph.


Login/Logout should not be required. Refresh of UI(F5) should be
sufficient.

Anyway, fixed in new patch 937.


Confirmed works as advertised.



2.) If I add a new segment using Add dialog and clicking 'Add & edit'
button, I get the following error:

"""
IPA Error 3007: RequirementError

'topologysuffixcn' is required
"""

However the segment is created normally. I guess that there is some bug
when viewing the segment immediately after creation.


Fixed in patch 935-1.



Indeed.



3.) The key with suffix colors should be more descriptive. I would like
to propose the following format:

"""
Color coding of segments by suffix:
{suffix_name} ({suffix_dn})
e.g.
ipaca (o=ipaca)
realm (dc=ipa,dc=test)
"""

4.) It would be nice if the graph canvas could be dynamically resized
based on the container size, but I am not sure how difficult it is to
implement it.
(Graphical nitpick: I would change the color of nodes, green does not
fit very well with the bluish/grey  palette of other WebUI elements)


I would like this, #3 and #4 to go trough additional designer review and
therefore fixed in other patch.

I'm OK with that, it is a cosmetic change anyway so we shouldn't 
bikeshed over it.

Right now I'm not sure what is the correct behavior for the re-sizing.

Atm the canvas size is fixed: 960x500.

1. What should be the canvas size when size of the container is very
small, e.g. 330x90? With static positions, nodes would not be visible.
2. Similar issue when nodes are put into corners and then the window is
shrunk.

Probably there could be a minimum size and then enlarge the cavas when
nodes are dragged.

Minimum canvas size makes sense IMHO. I'm not sure about the behavior of 
the auto-resize during node manipulation. I would begin with a canvas 
that has some reasonable minimal size and expands to the size of the 
container/facet (not sure about the terminology here) during resize with 
some reasonable padding added. Then we can implement some 
zooming/panning feature on top.




Otherwise I think that it is a good start and additional features can be
built on top of this patchset.



All in all I think that all these points can be addressed in followup 
patches after discussion with UX people. If nobody else objects I would 
ACK these patches.


--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH] Add option to disable setkeytab extended operations

[Sumit Bose]

On Wed, Nov 25, 2015 at 09:54:13AM -0500, Simo Sorce wrote:
> On Wed, 2015-11-25 at 10:24 +0100, Sumit Bose wrote:
> > On Tue, Nov 24, 2015 at 02:42:32PM -0500, Simo Sorce wrote:
> > > Since some time we use the getkeytab operation to fetch keytabs on newer
> > > clients. According to bug #232 setkeytab can be used to circumvent
> > > password quality controls so it needs to be slowly retired.
> > 
> > ipasam uses this exop to create the cross-realm TGT principal objects,
> > krbtgt/DOM.A@DOM.B. What should be used instead to make sure that
> > setkeytab can safely be disabled?
> 
> It must use the new getkeytab extended operation.
> 
> Can you open a ticket to fix this and assign it to me ?

Here you are

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

bye,
Sumit

> 
> Simo.
> 
> > bye,
> > Sumit
> > 
> > > 
> > > The attached patches implement #5485 in 2 parts.
> > > 
> > > The first introduces the option DisableSetKeytab which globally disables
> > > the setkeytab extended operation. This is set to false by default for
> > > backwards compatibility.
> > > 
> > > The second introduces an option called DisableUserSetKeytab, which is
> > > active by default in new installs (but not in upgraded ones), and only
> > > disables the use of setkeytab for ipa suers, but not for hosts/services.
> > > This is because user's are the ones that may abuse the interface to
> > > escape password policies and users also normally do not acquire keytabs,
> > > so it is a safe bet to disable just them by default in new installs.
> > > 
> > > (Testing in progress)
> > > 
> > > Simo.
> > > 
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
> 
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 

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

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

[Oleg Fayans]

Hi,

Here is the updated version of the patch (more tests + fixed the issues 
of the first one) + patch 0017, that implements the necessary changes in 
the background code, i. e. patch 16 does not work without patch 17


On 11/18/2015 05:20 PM, Martin Basti wrote:



On 09.11.2015 15:09, Oleg Fayans wrote:

Hi guys,

Here are first two automated testcases from this (so far incomplete)
testplan: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan

Testplan review is highly appreciated





PATCH 16: NACK

1)
What is the reason to add an unused parameter to 'domain_level' to
install_topo()?
Also it is good practise to add new option as the last parameter.

2)
cab you in both tests specify a domain level with constant instead of
number literal?

3)
both test call install_topo with custom domain level, but it cannot work
because 1)  (did you run the test?)

4)
How the test "TestLevel1" is supposed to work?
Respectively why there is call of install_topo() that installs replica.
As this test just tests that ipa-replica-prepare is not working anymore,
is it worth to spend 20 minutes with installing replica and then just no
tot use it? IMO to install master in install step is enough.

Martin^2



--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From e40484bee08091ce1c058712a73040c9b0ce Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Wed, 25 Nov 2015 16:38:37 +0100
Subject: [PATCH] Prepared integration tests for replica promotion

---
 .../test_integration/test_replica_promotion.py | 157 +
 1 file changed, 157 insertions(+)
 create mode 100644 ipatests/test_integration/test_replica_promotion.py

diff --git a/ipatests/test_integration/test_replica_promotion.py b/ipatests/test_integration/test_replica_promotion.py
new file mode 100644
index ..64cf9bc72fdb7a70f3161139eec3b5d4081d5add
--- /dev/null
+++ b/ipatests/test_integration/test_replica_promotion.py
@@ -0,0 +1,157 @@
+from ipatests.test_integration.base import IntegrationTest
+from ipatests.test_integration import tasks
+from ipatests.test_integration.test_caless import assert_error
+from ipalib.constants import MIN_DOMAIN_LEVEL
+from ipalib.constants import MAX_DOMAIN_LEVEL
+from env_config import get_global_config
+import pytest
+
+
+class Dummy(IntegrationTest):
+config = get_global_config()
+kra_install = ["ipa-kra-install", "-U", "-p", config.dirman_password]
+kra_uninstall = ["ipa-kra-install", "--uninstall", "-U"]
+
+@classmethod
+def install(cls, mh):
+tasks.install_master(cls.master, domain_level=cls.domain_level)
+
+def teardown_method(self, method):
+for host in self.replicas:
+host.run_command(self.kra_uninstall, raiseonerr=False)
+tasks.uninstall_master(host)
+
+def test_kra_install_master(self):
+self.master.run_command(self.kra_install)
+tasks.kinit_admin(self.master)
+result = self.master.run_command(["ipa", "vault-find"],
+raiseonerr=False)
+assert(result.stdout_text.find("0 vaults matched") > 0), result.stdout_text
+
+
+class TestLevel0(Dummy):
+
+topology = 'line'
+num_replicas = 1
+domain_level = MIN_DOMAIN_LEVEL
+
+def test_promotion_disabled(self):
+"""
+Testcase http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#
+Test_case:_Make_sure_the_feature_is_unavailable_under_domain_level_0
+"""
+client = self.replicas[0]
+tasks.install_client(self.master, client)
+args = ['ipa-replica-install', '-U',
+'-p', self.master.config.dirman_password,
+'-w', self.master.config.admin_password,
+'--ip-address', client.ip]
+result = client.run_command(args, raiseonerr=False)
+assert_error(result,
+ 'You must provide a file generated by ipa-replica-prepare'
+ ' to create a replica when the domain is at level 0', 1)
+
+def test_backup_restore(self):
+"""
+TestCase: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+#Test_case:_ipa-restore_after_domainlevel_raise_restores_original_domain_level
+"""
+command = ["ipa", "topologysegment-find", "realm"]
+tasks.install_replica(self.master, self.replicas[0])
+backup_file = tasks.ipa_backup(self.master)
+self.master.run_command(["ipa", "domainlevel-set", "1"])
+result = self.master.run_command(command)
+assert(result.stdout_text.rfind("1 segment matched") > 0), result.stdout_text
+tasks.ipa_restore(self.master, backup_file)
+result2 = self.master.run_command(command, raiseonerr=False)
+assert(result2.stdout_text.rfind("0 segments matched") > 0), result2.stdout_text
+
+
+class TestKRAInstall(IntegrationTest):
+"""
+TestCase: http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan
+#Test_case:_

Re: [Freeipa-devel] [PATCH 0015] mod_auth_gssapi: Remove ntlmssp support and restrict, mechanism to krb5

[Gabe Alford]

Bump for push. May need a rebase.

On Wed, Jul 22, 2015 at 7:49 AM, Simo Sorce  wrote:

> - Original Message -
> > From: "Christian Heimes" 
> > To: "freeipa-devel" 
> > Sent: Wednesday, July 22, 2015 9:32:59 AM
> > Subject: [Freeipa-devel] [PATCH 0015] mod_auth_gssapi: Remove ntlmssp
> support and restrict, mechanism to krb5
> >
> > By default mod_auth_gssapi allows all locally available mechanisms. If
> > the gssntlmssp package is installed, it also offers ntlmssp.  This has
> > the annoying side effect that some browser will pop up a
> > username/password request dialog if no Krb5 credentials are available.
> >
> > The patch restricts the mechanism to krb5 and removes ntlmssp and
> > iakerb support from Apache's ipa.conf.
> >
> > The new feature was added to mod_auth_gssapi 1.3.0.
> >
> > https://fedorahosted.org/freeipa/ticket/5114
>
> LGTM
>
> Simo.
>
> --
> Simo Sorce * Red Hat, Inc. * New York
>
> --
> Manage your subscription for the Freeipa-devel mailing list:
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
>
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0384] ipa-client-automount: Leverage IPAChangeConf to configure the idmapd

[Gabe Alford]

Ack.

Gabe

On Wed, Nov 11, 2015 at 7:22 AM, Tomas Babej  wrote:

> Hi,
>
> Simple regexp substitution caused that the domain directive fell under
> an inapprorpiate section, if the domain directive was not present. Hence
> the idmapd.conf file was not properly parsed.
>
> Use IPAChangeConf to put the directive in its correct place even if it
> the domain directive is missing.
>
> https://fedorahosted.org/freeipa/ticket/5069
>
> --
> Manage your subscription for the Freeipa-devel mailing list:
> https://www.redhat.com/mailman/listinfo/freeipa-devel
> Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
>
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [pytest-multihost-devel][PATCH] Functions for handling various file and directory operations

[Petr Viktorin]

On 11/25/2015 10:08 AM, Abhijeet Kasurde wrote:
> Hi All,
> 
> Please find the patch for pytest-multihost-plugin.
> 
> Fixes : https://fedorahosted.org/python-pytest-multihost/ticket/2

Thanks! These will be useful.

ACK, pushed as e7bf95b3ba4ca84b73abffda1abcf6187c5c8a67
I wrote some tests for the file operations as well:
5e76908430bbb56ca981b04a88f12359c73c7e1f


I plan to do a release (and update the Fedora package) after IPA starts
testing under Python 3. Let me know if an earlier release would help you.



-- 
Petr Viktorin

-- 
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] Add option to disable setkeytab extended operations

[Simo Sorce]

On Wed, 2015-11-25 at 10:24 +0100, Sumit Bose wrote:
> On Tue, Nov 24, 2015 at 02:42:32PM -0500, Simo Sorce wrote:
> > Since some time we use the getkeytab operation to fetch keytabs on newer
> > clients. According to bug #232 setkeytab can be used to circumvent
> > password quality controls so it needs to be slowly retired.
> 
> ipasam uses this exop to create the cross-realm TGT principal objects,
> krbtgt/DOM.A@DOM.B. What should be used instead to make sure that
> setkeytab can safely be disabled?

It must use the new getkeytab extended operation.

Can you open a ticket to fix this and assign it to me ?

Simo.

> bye,
> Sumit
> 
> > 
> > The attached patches implement #5485 in 2 parts.
> > 
> > The first introduces the option DisableSetKeytab which globally disables
> > the setkeytab extended operation. This is set to false by default for
> > backwards compatibility.
> > 
> > The second introduces an option called DisableUserSetKeytab, which is
> > active by default in new installs (but not in upgraded ones), and only
> > disables the use of setkeytab for ipa suers, but not for hosts/services.
> > This is because user's are the ones that may abuse the interface to
> > escape password policies and users also normally do not acquire keytabs,
> > so it is a safe bet to disable just them by default in new installs.
> > 
> > (Testing in progress)
> > 
> > Simo.
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York


-- 
Simo Sorce * Red Hat, Inc * New York

-- 
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] 0748 Handle encoding for ipautil.run

[Petr Viktorin]

On 11/25/2015 11:04 AM, Jan Cholasta wrote:
> On 24.11.2015 17:21, Petr Viktorin wrote:
>> On 11/23/2015 10:50 AM, Jan Cholasta wrote:
>>> On 23.11.2015 07:43, Jan Cholasta wrote:
 On 19.11.2015 00:55, Petr Viktorin wrote:
> On 11/03/2015 02:39 PM, Petr Viktorin wrote:
>> Hello,
>> Python 3's strings are Unicode, so data coming to or leaving a Python
>> program needs to be decoded/encoded if it's to be handled as a
>> string.
>> One of the boundaries where encoding is necessary is external
>> programs,
>> specifically, ipautil.run.
>> Unfortunately there's no one set of options that would work for all
>> run() invocations, so I went through them all and specified the
>> stdin/stdout/stderr encoding where necessary. I've also updated the
>> call
>> sites to make it clearer if the return values are used or not.
>> If an encoding is not set, run() will accept/return bytes. (This is a
>> fail-safe setting, since it can't raise errors, and using bytes where
>> strings are expected generally fails loudly in py3.)
>>
>> Note that the changes are not effective under Python 2.
>
> ping,
> Could someone look at this patch?

 Looking.
>>>
>>> 1) I think a better approach would be to use str for stdin/stdout/stderr
>>> by default in both Python 2 and 3, i.e. do nothing in Python 2 and
>>> encode/decode using locale.getpreferredencoding() in Python 3. This is
>>> consistent with sys.std{in,out,err} in both Python 2 and 3. Using bytes
>>> or different encoding should be opt-in.
>>>
>>> Note that different encoding should be used only if the LC_ALL or LANG
>>> variables are overriden in the command's environment.
>>
>> That would assume the output of *everything* run via ipautil.run can be
>> decoded using the locale encoding. Any stray invalid byte would make IPA
>> crash, even in cases where we don't care about the output. IPA calls too
>> many weird tools to assume they all output text.
> 
> Such a stray invalid byte may bubble through our code and cause havoc
> somewhere else, which is much harder to troubleshoot than a crash in
> ipautil.run(), where you can see that it was a misbehaving command that
> caused the crash and exactly what command it was.

The invalid byte can't bubble through code, because if you don't specify
an encoding you get bytes back. You'll get a crash when you try using it
as a string.

locale.getpreferredencoding() is not used consistently in all software,
especially if it's not written in Python. For instance, wget won't
magically re-encode the data it fetches for us. It's better to
explicitly specify the encoding every time.

> If we don't care about output somewhere, we should not capture it there
> at all.

Then people need to remember to put "capture_output=True" everywhere
(except that also disables logging of the output, so we'd need an
additional option).

>>> 3) I think the "surrogateescape" error handler would be better for
>>> non-strict decoding, as the text can be encoded back to bytes without
>>> loss of information.
>>
>> Non-strict decoding is meant for logging, so we want backslash escapes
>> rather than invalid surrogates. If you need round-tripping then you
>> should use bytes.
> 
> I see. There is no guarantee that non-strict output will be used only
> for logging though.
> 
> I think it might be better to return raw output and let the caller use a
> decode-for-logging utility function instead. The logger already escapes
> invalid bytes itself, so the function would have to be used only when
> raising exceptions.

It's also used for logging at a higher level (info-critical).
But this makes sense; changed.

-- 
Petr Viktorin

From 778db1a57fe4bee8b111f7b5c0faee50560e9aa6 Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Tue, 27 Oct 2015 17:54:54 +0100
Subject: [PATCH] Handle encoding for ipautil.run

For Python 2, nothing is changed, and all data discussed below is
bytestrings (str).

Parts of the "argv" for run() may be strings or bytes, and may even be
mixed.

Stdin must be bytes (or None) by default.
If stdin_encoding is given, it may be a string, and is encoded appropriately.

Stdout and stderr are returned as bytes, unless specified otherwise.
This means there will be no decoding errors in the (very common) case
where these are ignored.
For the logs, these output streams are encoded with 'backslashreplace',
so errors can't be raised, and also binary garbage won't show up in logs.

If the stdout_encoding and/or stderr_encoding arguments are given,
the corresponding stream is decoded. This may raise an exception
if the data can't be encoded.
To suppress errors, pass {stdout,stderr}_strict=False. This will cause
invalid characters to be replaced with backslash sequences. It's appropriate
when the stream is copied to a log file, NOT when the data is further
manipulated.
---
 .../certmonger/dogtag-ipa-ca-renew-agent-submit|  9 ++-
 install/certmonger/ipa-server-guard   

Re: [Freeipa-devel] [PATCH] Add option to disable setkeytab extended operations

[Simo Sorce]

On Wed, 2015-11-25 at 09:02 -0500, Rob Crittenden wrote:
> Jan Cholasta wrote:
> > On 24.11.2015 22:17, Simo Sorce wrote:
> >> On Tue, 2015-11-24 at 14:57 -0500, Simo Sorce wrote:
> >>> On Tue, 2015-11-24 at 14:42 -0500, Simo Sorce wrote:
>  Since some time we use the getkeytab operation to fetch keytabs on
>  newer
>  clients. According to bug #232 setkeytab can be used to circumvent
>  password quality controls so it needs to be slowly retired.
> 
>  The attached patches implement #5485 in 2 parts.
> 
>  The first introduces the option DisableSetKeytab which globally
>  disables
>  the setkeytab extended operation. This is set to false by default for
>  backwards compatibility.
> 
>  The second introduces an option called DisableUserSetKeytab, which is
>  active by default in new installs (but not in upgraded ones), and only
>  disables the use of setkeytab for ipa suers, but not for
>  hosts/services.
>  This is because user's are the ones that may abuse the interface to
>  escape password policies and users also normally do not acquire
>  keytabs,
>  so it is a safe bet to disable just them by default in new installs.
> 
>  (Testing in progress)
> >>>
> >>> Tested and working as expected.
> >>
> >> I realized that adding options to ipaConfig require to add them in the
> >> UI as well, attached patches add options in API.txt and config.py
> >> Make now complain I should change API Major or Minor, but it is not
> >> clear to me why given this are additional values and no real change or
> >> new function is introduced. What's the recommendation ?
> > 
> > When does make complain? It is supposed to complain only when API.txt
> > does not match code.
> > 
> > Anyway, we usually bump minor version even for backward compatible
> > changes, see e.g. commit 9549a59.
> > 
> 
> The point of API.txt (and the heavy client) was to save a round-trip.
> Being able to pass in an invalid option would void that rule hence
> having to update the API when new values are added.
> 
> rob

Ok added version change to the second patch (so we bump it only once
given these are basically related changes.


-- 
Simo Sorce * Red Hat, Inc * New York
From 5b77aba72415ad40dfef02a25effa7fc09d34aa2 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Tue, 24 Nov 2015 14:02:01 -0500
Subject: [PATCH] Disable User's ability to use the setkeytab exop.

Users can still obtain a keytab for themselves using the getkeytab exop
which does not circumvent password policy checks.

Users are disallowed from using setkeytab by default in new installations
but not in existing installations (no forced upgrade).

Signed-off-by: Simo Sorce 

Ticket: https://fedorahosted.org/freeipa/ticket/5485
---
 API.txt|  2 +-
 VERSION|  2 +-
 daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c   |  4 
 .../ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c| 18 +-
 daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h   |  1 +
 install/share/bootstrap-template.ldif  |  1 +
 ipalib/plugins/config.py   |  2 +-
 7 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/API.txt b/API.txt
index 763ec12413fc1267b2cf7f519cf8c9bc14360c8c..9a7a23f0d6e3efcdaa355e15a37e8879d8f030d7 100644
--- a/API.txt
+++ b/API.txt
@@ -766,7 +766,7 @@ args: 0,25,3
 option: Str('addattr*', cli_name='addattr', exclude='webui')
 option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
 option: Str('delattr*', cli_name='delattr', exclude='webui')
-option: StrEnum('ipaconfigstring', attribute=True, autofill=False, cli_name='ipaconfigstring', csv=True, multivalue=True, required=False, values=(u'AllowNThash', u'DisableSetKeytab', u'KDC:Disable Last Success', u'KDC:Disable Lockout'))
+option: StrEnum('ipaconfigstring', attribute=True, autofill=False, cli_name='ipaconfigstring', csv=True, multivalue=True, required=False, values=(u'AllowNThash', u'DisableSetKeytab', u'DisableUserSetKeytab', u'KDC:Disable Last Success', u'KDC:Disable Lockout'))
 option: Str('ipadefaultemaildomain', attribute=True, autofill=False, cli_name='emaildomain', multivalue=False, required=False)
 option: Str('ipadefaultloginshell', attribute=True, autofill=False, cli_name='defaultshell', multivalue=False, required=False)
 option: Str('ipadefaultprimarygroup', attribute=True, autofill=False, cli_name='defaultgroup', multivalue=False, required=False)
diff --git a/VERSION b/VERSION
index 064e98cb46a084b9f87f4fb52c8cd6033be171a4..cbd1cb35fc303ad1175ee59e1702d47de3ecae30 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=158
+IPA_API_VERSION_MINOR=159
 # Last change: mba

Re: [Freeipa-devel] [PATCH] Allow ipa-getkeytab to find server name from config file

[Simo Sorce]

On Wed, 2015-11-25 at 14:34 +1000, Fraser Tweedale wrote:
> On Tue, Nov 24, 2015 at 02:36:17PM -0500, Simo Sorce wrote:
> > On Tue, 2015-11-24 at 17:34 +0100, Jan Cholasta wrote:
> > > On 24.11.2015 17:30, Simo Sorce wrote:
> > > > On Tue, 2015-11-24 at 09:14 +0100, Jan Cholasta wrote:
> > > >> On 24.11.2015 09:06, Petr Spacek wrote:
> > > >>> On 24.11.2015 07:32, Jan Cholasta wrote:
> > >  On 23.11.2015 21:18, Simo Sorce wrote:
> > > > Fixes #2203 by reading the server name from /etc/ipa/default.conf 
> > > > if not
> > > > provided on the command line.
> > > >
> > > > Simo.
> > > 
> > >  Just a thought: it would be nice if we had libipaconfig and used it 
> > >  everywhere
> > >  (the framework, ipa-getkeytab, certmonger, ...). I don't like that 
> > >  there are
> > >  at least 3 ipa config parser implementations now, each with its own 
> > >  quirks.
> > > >>>
> > > >>> Yeah, define a Augeas grammar for IPA config and call it. There is C 
> > > >>> and
> > > >>> Python binding in RHEL 7.2 already.
> > > >>
> > > >> This won't be sufficient, as it should go beyond just parsing the 
> > > >> config
> > > >> file - there are dynamic defaults and backward compatiblity support
> > > >> which should be the same everywhere too.
> > > >
> > > > This is a much bigger task, that someone keen on big refactoring may
> > > > undertake, but I do not think we should delay this useful feature for
> > > > that. It will be really easy to change this code to use whatever other
> > > > ipa library when it materializes.
> > > >
> > > > For now this patch stands IMHO.
> > > 
> > > Sure, I'm not suggesting otherwise.
> > 
> > Rebased patch that includes proper spec file buildrequires.
> > 
> > Simo.
> > 
> Tested and works as expected (great usability win IMO).
> 
> Man page should be updated accordingly.
> 
> Cheers,
> Fraser

Attached new patch that includes manpage changes.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 482b29614989a68d67e0eccf53f1fe8c5382527e Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Mon, 23 Nov 2015 14:50:04 -0500
Subject: [PATCH] Support sourcing the IPA server name from config

Use ding-libs to parse /etc/ipa/default.conf to find the IPA server
to contact by default.

Signed-off-by: Simo Sorce 

Ticket: https://fedorahosted.org/freeipa/ticket/2203
---
 freeipa.spec.in|  1 +
 ipa-client/Makefile.am |  4 ++
 ipa-client/configure.ac| 28 +
 ipa-client/ipa-getkeytab.c | 93 +-
 ipa-client/man/ipa-getkeytab.1 | 12 +++---
 5 files changed, 132 insertions(+), 6 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index d66b0ffe9eeb061c19642995aa19459673d61dec..5524576054ad77f7263e02642d1ae795aeb7077e 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -97,6 +97,7 @@ BuildRequires:  python-kdcproxy >= 0.3
 BuildRequires:  python-six
 BuildRequires:  python-jwcrypto
 BuildRequires:  custodia
+BuildRequires:  libini_config-devel >= 1.2.0
 
 # Build dependencies for unit tests
 BuildRequires:  libcmocka-devel
diff --git a/ipa-client/Makefile.am b/ipa-client/Makefile.am
index 0da351c6a28a4e2a216bec3f85d0500e4aef47ff..6c426779530f216b11dff0764132221ea2793289 100644
--- a/ipa-client/Makefile.am
+++ b/ipa-client/Makefile.am
@@ -15,6 +15,7 @@ export AM_CFLAGS
 KRB5_UTIL_DIR=../util
 KRB5_UTIL_SRCS=$(KRB5_UTIL_DIR)/ipa_krb5.c
 ASN1_UTIL_DIR=../asn1
+IPA_CONF_FILE=$(sysconfdir)/ipa/default.conf
 
 AM_CPPFLAGS =			\
 	-I.			\
@@ -27,11 +28,13 @@ AM_CPPFLAGS =			\
 	-DLIBEXECDIR=\""$(libexecdir)"\"			\
 	-DDATADIR=\""$(datadir)"\"\
 	-DLOCALEDIR=\""$(localedir)"\"\
+	-DIPACONFFILE=\""$(IPA_CONF_FILE)"\"			\
 	$(KRB5_CFLAGS)		\
 	$(OPENLDAP_CFLAGS)	\
 	$(SASL_CFLAGS)		\
 	$(POPT_CFLAGS)		\
 	$(WARN_CFLAGS)		\
+	$(INI_CFLAGS)		\
 	$(NULL)
 
 sbin_PROGRAMS =			\
@@ -53,6 +56,7 @@ ipa_getkeytab_LDADD = 		\
 	$(SASL_LIBS)		\
 	$(POPT_LIBS)		\
 	$(LIBINTL_LIBS) \
+	$(INI_LIBS)		\
 	$(NULL)
 
 ipa_rmkeytab_SOURCES =		\
diff --git a/ipa-client/configure.ac b/ipa-client/configure.ac
index 78da8e6e413b8becbd4c75422abffb670050f446..943c3f1b62f6a4947335178e9bcf1d45434a7e90 100644
--- a/ipa-client/configure.ac
+++ b/ipa-client/configure.ac
@@ -192,6 +192,34 @@ LIBS="$SAVELIBS"
 AC_SUBST(LIBINTL_LIBS)
 
 dnl ---
+dnl - Check for libini_config
+dnl ---
+PKG_CHECK_MODULES([LIBINI_CONFIG], [ini_config >= 1.2.0], [have_libini_config=1], [have_libini_config=])
+if test x$have_libini_config = x; then
+AC_MSG_WARN([Could not find LIBINI_CONFIG headers])
+else
+INI_CONFIG_CFLAGS="`$PKG_CONFIG --cflags ini_config`"
+INI_CONFIG_LIBS="`$PKG_CONFIG --libs ini_config`"
+AC_CHECK_LIB(ini_config, ini_config_file_open, [],
+ [AC_MSG_WARN([ini_config library must suppo

Re: [Freeipa-devel] [PATCH 559] Fix kadmin for new users

[Martin Kosek]

On 11/25/2015 03:32 PM, Simo Sorce wrote:
> On Wed, 2015-11-25 at 14:13 +0100, Tomas Babej wrote:
>>
>> On 11/25/2015 02:13 PM, Tomas Babej wrote:
>>>
>>>
>>> On 11/25/2015 02:00 PM, Martin Babinsky wrote:
 On 11/24/2015 11:32 PM, Simo Sorce wrote:
> Ticket #937 was reopened a while ago because one corner case, new users
> that have never been assigned a password cause kadmin/kadmin.local to
> throw a fit when they try to visualize information about those user's
> principals.
>
> This patch fakes up modification information when no krbExtraData is
> available for the principal so that kadmin is happy.
>
> Tested and working as designed.
>
> Simo.
>
>
>
 ACK

>>>
>>> Pushed to master: 0f52eddd1d2781ccc1941c191e9ab6e3ccf6919d
>>>
>>
>> On a related note, should we backport this to later branches?
> 
> It wouldn't hurt, it should apply straight to any 4.x and probably
> latest 3.x branches too.

I would not fix anything older than FreeIPA 4.1.x which is in F22, which is the
oldest supported Fedora (or rather fill be, one month after F23 GA).

-- 
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 0362] KRA: do not stop certmonger during standalone uninstall

[Martin Basti]

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

Patch attached.
From 256195119576afc4dc5761abbd3d12202ead19e7 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 25 Nov 2015 15:20:51 +0100
Subject: [PATCH] KRA: do not stop certmonger during standalone uninstall

https://fedorahosted.org/freeipa/ticket/5477
---
 ipaserver/install/kra.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaserver/install/kra.py b/ipaserver/install/kra.py
index 14568ec429e5f06e789087fc887596d8909af4c8..3f8227688a2704d239ae75fc896adf59465210c8 100644
--- a/ipaserver/install/kra.py
+++ b/ipaserver/install/kra.py
@@ -107,6 +107,6 @@ def uninstall(standalone):
 except errors.NotFound:
 pass
 
-kra.stop_tracking_certificates()
+kra.stop_tracking_certificates(stop_certmonger=not standalone)
 if kra.is_installed():
 kra.uninstall()
-- 
2.5.0

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

Re: [Freeipa-devel] [PATCH 559] Fix kadmin for new users

[Simo Sorce]

On Wed, 2015-11-25 at 14:13 +0100, Tomas Babej wrote:
> 
> On 11/25/2015 02:13 PM, Tomas Babej wrote:
> > 
> > 
> > On 11/25/2015 02:00 PM, Martin Babinsky wrote:
> >> On 11/24/2015 11:32 PM, Simo Sorce wrote:
> >>> Ticket #937 was reopened a while ago because one corner case, new users
> >>> that have never been assigned a password cause kadmin/kadmin.local to
> >>> throw a fit when they try to visualize information about those user's
> >>> principals.
> >>>
> >>> This patch fakes up modification information when no krbExtraData is
> >>> available for the principal so that kadmin is happy.
> >>>
> >>> Tested and working as designed.
> >>>
> >>> Simo.
> >>>
> >>>
> >>>
> >> ACK
> >>
> > 
> > Pushed to master: 0f52eddd1d2781ccc1941c191e9ab6e3ccf6919d
> > 
> 
> On a related note, should we backport this to later branches?

It wouldn't hurt, it should apply straight to any 4.x and probably
latest 3.x branches too.

Simo.

-- 
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] 928-936 webui: topology visualization

[Petr Vobornik]

On 11/24/2015 02:09 PM, Martin Babinsky wrote:

On 11/24/2015 12:17 PM, Petr Vobornik wrote:

On 11/24/2015 12:10 PM, Ludwig Krispenz wrote:

Hi Petr,

I'm testing these patches.Two observations so far:
- in Topology->IPA Servers I see a table of my servers and the managed
suffix column I see both suffixes, ipaca and the realm,
but if I select one of the servers I Only see the realm suffix, this was
different in the demo video


implemented in patch 927 (separate thread)


- the graph layou is not stable. If I arrange it and the try some
actions on a segment, eg try to remove one (which is rejected), the
graph does change again-


Not sure if I understand it correctly. One can set static position to a
node by double clicking on it (this needs to be somehow communicated)
otherwise it is random - each refresh.

Maybe we should not refresh on failed operation because there is no
change.

+1, failed operations on segments should not trigger graph redraws.


Fixed in patch 936-1.







Ludwig

On 11/20/2015 04:00 PM, Petr Vobornik wrote:

Patches 928-931 are prerequisites.

Patches 932-934 implements the visualization

Patches 935-936 adds 'add' and 'remove' segment functionality to the
visualization page.

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




I have tested the new feature a bit (I have looked also on the code, but
I do not speak JavaScript very well) and so far I have found these issues:

1.) If you first setup topology w/ domain level 0 and try to render
topology graph, you are correctly informed that managed topology is
available on domain level 1 only. IF you raise your domainlevel and
click refresh, you get the same error message, probably because the
request to get domainlevel is sent only in the beginning of session. You
have to logout and then login back to see the graph.


Login/Logout should not be required. Refresh of UI(F5) should be sufficient.

Anyway, fixed in new patch 937.



2.) If I add a new segment using Add dialog and clicking 'Add & edit'
button, I get the following error:

"""
IPA Error 3007: RequirementError

'topologysuffixcn' is required
"""

However the segment is created normally. I guess that there is some bug
when viewing the segment immediately after creation.


Fixed in patch 935-1.



3.) The key with suffix colors should be more descriptive. I would like
to propose the following format:

"""
Color coding of segments by suffix:
{suffix_name} ({suffix_dn})
e.g.
ipaca (o=ipaca)
realm (dc=ipa,dc=test)
"""

4.) It would be nice if the graph canvas could be dynamically resized
based on the container size, but I am not sure how difficult it is to
implement it.
(Graphical nitpick: I would change the color of nodes, green does not
fit very well with the bluish/grey  palette of other WebUI elements)


I would like this, #3 and #4 to go trough additional designer review and 
therefore fixed in other patch.


Right now I'm not sure what is the correct behavior for the re-sizing.

Atm the canvas size is fixed: 960x500.

1. What should be the canvas size when size of the container is very 
small, e.g. 330x90? With static positions, nodes would not be visible.
2. Similar issue when nodes are put into corners and then the window is 
shrunk.


Probably there could be a minimum size and then enlarge the cavas when 
nodes are dragged.




Otherwise I think that it is a good start and additional features can be
built on top of this patchset.


--
Petr Vobornik
From f760b12f9a852f76fadcd8a27b160442ccb8a0b0 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Tue, 24 Nov 2015 17:27:37 +0100
Subject: [PATCH] webui: update topology graph after raising domain level

When topology graph was shown with domain level == 0, a view describing
that domain level needs to be at least 1 was shown.

If domain level is raised, this view is then properly replaced by the
graph when shown again.

https://fedorahosted.org/freeipa/ticket/4286
---
 install/ui/src/freeipa/dialog.js   |  6 +++---
 install/ui/src/freeipa/topology.js | 34 +-
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/install/ui/src/freeipa/dialog.js b/install/ui/src/freeipa/dialog.js
index f8f1bba78bee44e6d5e78be9c6c906bd55b9c698..c732a72780d432b45a30cbec2c7ef93cc56a3b72 100644
--- a/install/ui/src/freeipa/dialog.js
+++ b/install/ui/src/freeipa/dialog.js
@@ -1720,10 +1720,10 @@ IPA.command_dialog = dialogs.command_dialog = function(spec) {
 args: that.args,
 options: options,
 on_success: function(data) {
-that.on_success();
+that.on_success(data);
 },
-on_error: function() {
-that.on_error();
+on_error: function(data) {
+that.on_error(data);
 }
 });
 return command;
diff --git a/install/ui/src/freeipa/topology.js b/install/ui/src/freeipa/topology.js
index 01e036aa41e60492c0f2238b34d00987177f096e..8dfa02a004235251a1ccd929896618a7

Re: [Freeipa-devel] [PATCH 0104] do not disconnect when using existing connection to check default CA ACLs

[Rob Crittenden]

Martin Babinsky wrote:
> On 11/25/2015 09:56 AM, Jan Cholasta wrote:
>> On 25.11.2015 09:28, Martin Babinsky wrote:
>>> On 11/25/2015 07:21 AM, Jan Cholasta wrote:
 On 25.11.2015 05:56, Fraser Tweedale wrote:
> On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote:
>> On 24.11.2015 17:17, Martin Babinsky wrote:
>>> On 11/24/2015 05:10 PM, Martin Babinsky wrote:
 On 11/24/2015 05:01 PM, Martin Babinsky wrote:
> On 11/24/2015 04:58 PM, Jan Cholasta wrote:
>> On 24.11.2015 16:48, Martin Babinsky wrote:
>>> On 11/24/2015 04:44 PM, Martin Babinsky wrote:
 https://fedorahosted.org/freeipa/ticket/5459

>>> forgot to attach the actual file *slaps himself*
>>
>> ipaserver/install/cainstance.py:1849: [E1101(no-member),
>> ensure_default_caacl] Instance of 'API' has no 'Backed' member)
>>
>
> Fixed
>
>
>
 Also attaching rebased patch for ipa-4-2



>>> Ignore the rebased patch, the original 104.1 applies on 4-2 just
>>> fine.
>>
>> Thanks, ACK.
>>
>> Pushed to:
>> master: ed830af693c596b286b30959eb3166b59cc030c6
>> ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a
>>
> Thanks for anaysing and fixing these connection issues which were
> presumably introduced by my patches.  I did not hit these in my
> testing.  Admittedly I was focused on the ipa-4-2 branch and my
> testing was mainly done there.
>
> Brittle LDAP connection logic in install and upgrade is an ongoing
> problem.  What shall we do to improve the situation?  Push
> connection details into the Backend and let it connect if/when
> needed, rather than managing connection state from the outside?
>
> I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491

 I don't think we need to be smart about it, everyone just needs to make
 sure that when an ad-hoc connection is opened somewhere, it is also
 closed in the same place. The same applies for any other resource.

>
> Thanks,
> Fraser
>


>>>
>>> Maybe it would be a good idea to implement some decorator/context
>>> manager to connect/disconnect from specified connection and wrap the
>>> CRUD code. Example usage:
>>>
>>> """
>>> decorator:
>>> @connected(api.Backend.ldap2, ldapi=True)
>>> def do_magic(api, *args, **kwargs):
>>> # do stuff
>>>
>>> context manager:
>>>
>>> with connected(api.Backend.ldap2, ccache=example_ccache):
>>>  do_some_other_magic()
>>> """
>>>
>>> I remember Jan having some objections against this so it would be nice
>>> to hear why this is not a good idea.
>>
>> a) For ad-hoc connection objects, LDAPClient should be used instead of
>> ldap2. It already supports the context manager protocol:
>>
>>  with LDAPClient(uri) as conn:
>>  conn.gssapi_bind()
>>  ...
>>
> I didn't know that you can and should use LDAPClient this way. We should
> document it somewhere and encourage other contributors to use this
> approach.
> 
>> b) The api.Backend.ldap2 object should be connected and disconnected
>> exactly once per api object per command. This is not done ATM and the
>> usual workaround is to connect it manually, which has lead to the mess
>> we have now.
>>
> Then we should find time to implement this behavior so that we can stop
> shooting ourselves in the foot when messing around with API commands.

I'm not sure I follow. Are we still talking about tools or did this
shift to requests? It used to be at a single connection was created for
each request and used everywhere internally.

There is probably nothing to prevent you from doing something similar in
the tools but given that restarts are common I hardly see the point.

>> Also, only GSSAPI authentication with valid IPA user credentials should
>> be used with ldap2, even for ldapi connections.

IIRC we kept the old IPAdmin class around in the old days for this very
reason.

rob

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

Re: [Freeipa-devel] [PATCH 0349] baseuser.py compare objectclasses as case insensitive

[Martin Basti]

On 25.11.2015 14:33, Martin Babinsky wrote:

On 11/17/2015 11:47 AM, Martin Basti wrote:

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

Patch attached.



ACK but please fix a typo in the commit message before pushing.


Fixed
Pushed to master: 6bbde3e0f7d661fa559ed5e3365381d2ba113dce

--
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] Add option to disable setkeytab extended operations

[Rob Crittenden]

Jan Cholasta wrote:
> On 24.11.2015 22:17, Simo Sorce wrote:
>> On Tue, 2015-11-24 at 14:57 -0500, Simo Sorce wrote:
>>> On Tue, 2015-11-24 at 14:42 -0500, Simo Sorce wrote:
 Since some time we use the getkeytab operation to fetch keytabs on
 newer
 clients. According to bug #232 setkeytab can be used to circumvent
 password quality controls so it needs to be slowly retired.

 The attached patches implement #5485 in 2 parts.

 The first introduces the option DisableSetKeytab which globally
 disables
 the setkeytab extended operation. This is set to false by default for
 backwards compatibility.

 The second introduces an option called DisableUserSetKeytab, which is
 active by default in new installs (but not in upgraded ones), and only
 disables the use of setkeytab for ipa suers, but not for
 hosts/services.
 This is because user's are the ones that may abuse the interface to
 escape password policies and users also normally do not acquire
 keytabs,
 so it is a safe bet to disable just them by default in new installs.

 (Testing in progress)
>>>
>>> Tested and working as expected.
>>
>> I realized that adding options to ipaConfig require to add them in the
>> UI as well, attached patches add options in API.txt and config.py
>> Make now complain I should change API Major or Minor, but it is not
>> clear to me why given this are additional values and no real change or
>> new function is introduced. What's the recommendation ?
> 
> When does make complain? It is supposed to complain only when API.txt
> does not match code.
> 
> Anyway, we usually bump minor version even for backward compatible
> changes, see e.g. commit 9549a59.
> 

The point of API.txt (and the heavy client) was to save a round-trip.
Being able to pass in an invalid option would void that rule hence
having to update the API when new values are added.

rob

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

Re: [Freeipa-devel] [PATCH 0381] admintool: Remove the option to override the log file

[Petr Spacek]

On 25.11.2015 14:15, Jan Cholasta wrote:
> On 25.11.2015 13:49, Tomas Babej wrote:
>>
>>
>> On 11/25/2015 01:29 PM, Jan Cholasta wrote:
>>> On 25.11.2015 13:24, Tomas Babej wrote:
 On 11/10/2015 02:22 PM, Tomas Babej wrote:
> Hi,
>
> This has been rarely used, and can be replaced by proper shell output
> redirection.
>
> https://fedorahosted.org/freeipa/ticket/5408
>

 Here's an updated version of the patch that gets rid of one missed
 occurrence of log_file usage.
>>>
>>> The ticket seems unrelated to the change.
>>>
>>> Shouldn't the option be kept in the respective commands for backward
>>> compatibility? See how the debug option is handled in AdminTool.
>>>
>>
>> Yeah, the correct ticket is: https://fedorahosted.org/freeipa/ticket/5385
>>
>> I'm curious, in what manner do you envision the backward compatibility?
>> The debug option is being replaced with verbose, but here we're removing
>> existing functionality since it does not work properly and is of little
>> use anyway.
>>
>> So there are two possiblities I see:
>>
>> 1.) We remove the functionality and keep the option, just to be able to
>> say that this option is deprecated and bail out.

This is such a minor thing that I would throw it away completely. It is not
worth the bytes :-)

User will get a message that the option does not exist which seems
functionally equivalent to the 'say that this option is deprecated and bail 
out'.

Petr^2 Spacek

>>
>> 2.) We keep the functionality, and keep the option, just issue a warning
>> when it's being used.
>>
>>  From my point of view: I did not do (1), but imho we can add it, albeit
>> it's a marginal usability improvement. As far as (2) goes, it does not
>> solve the underlying problem.
> 
> Add log_file_option=False argument to add_options(), if it's True, add the
> option to the parser. Set it to True in commands which currently have the 
> option.
> 
> In _setup_logging(), if the option value is not None, add a handler for the
> file to the log manager (untested):
> 
> user_file_handler = dict(
> name='user_file',
> filename=self.options.log_file,
> filemode=log_file_mode,
> permission=0600,
> level='debug',
> format=ipa_log_manager.LOGGING_FORMAT_STANDARD_FILE,
> )
> ipa_log_manager.log_mgr.create_log_handlers([user_file_handler], None, 
> None)
> 
> This way it will log into both the standard location and the user-specified 
> file.

-- 
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 0349] baseuser.py compare objectclasses as case insensitive

[Martin Babinsky]

On 11/17/2015 11:47 AM, Martin Basti wrote:

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

Patch attached.



ACK but please fix a typo in the commit message before pushing.

--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 0358] ipa-getkeytab: do not return error if translations cannot be loaded

[Tomas Babej]

On 11/25/2015 02:31 PM, Tomas Babej wrote:
> 
> 
> On 11/24/2015 05:38 PM, Martin Basti wrote:
>>
>>
>> On 24.11.2015 17:33, Jan Cholasta wrote:
>>> On 24.11.2015 16:52, Martin Basti wrote:
 https://fedorahosted.org/freeipa/ticket/5483

 Patch attached.
>>>
>>> Doesn't init_gettext() itself already print to stderr on failure?
>>>
>> Nope
>>
> 
> ACK, works fine.
> 
> Failed to load translations
> Keytab successfully retrieved and stored in: test.keytab
> 

Pushed to:
master: d43c3becbd406d145426d0409b8fe2a36ee6c63c
ipa-4-2: 34dbb42c51ce17a450e41f998e2b3f30682c

-- 
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 0358] ipa-getkeytab: do not return error if translations cannot be loaded

[Tomas Babej]

On 11/24/2015 05:38 PM, Martin Basti wrote:
> 
> 
> On 24.11.2015 17:33, Jan Cholasta wrote:
>> On 24.11.2015 16:52, Martin Basti wrote:
>>> https://fedorahosted.org/freeipa/ticket/5483
>>>
>>> Patch attached.
>>
>> Doesn't init_gettext() itself already print to stderr on failure?
>>
> Nope
> 

ACK, works fine.

Failed to load translations
Keytab successfully retrieved and stored in: test.keytab

-- 
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 0102] update idrange tests to reflect disabled modification of local ID ranges

[Tomas Babej]

On 11/23/2015 12:58 PM, Tomas Babej wrote:
> 
> 
> On 11/20/2015 06:41 PM, Milan Kubík wrote:
>> On 11/20/2015 04:06 PM, Martin Babinsky wrote:
>>> When I fixed https://fedorahosted.org/freeipa/ticket/4826 I forgot to
>>> fix the corresponding xmlrpc tests.
>>>
>>> This oversight bit me today when I ran in-tree tests on my VM.
>>>
>>> Here is the patch that makes idrange tests green and shiny again.
>>>
>> Tests are now passing, ACK
>>
> 
> Tests are now passing, however, the code paths that have been checked by
> these tests are no longer executed (as an modifications to the local
> ranges are prohibited now).
> 
> This essentially means that we are testing the same thing multiple times
> (i.e. local range cannot be dealt with), and the test names are
> misleading currently.
> 
> I think we should either reduce the set of (effectively) duplicate
> tests, or, better, introduce their replacements that are using allowed
> range types, if possible.
> 
> Tomas
> 

I created a ticket so that this does not get lost:

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

-- 
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 0344] Use absolute domain name in detection of A/AAAA records

[Martin Basti]

On 25.11.2015 13:36, Petr Spacek wrote:

On 19.11.2015 11:05, Martin Basti wrote:


On 18.11.2015 18:33, Petr Spacek wrote:

On 12.11.2015 13:58, Martin Basti wrote:

On 09.11.2015 08:47, Petr Spacek wrote:

On 4.11.2015 16:16, Martin Basti wrote:

Patch attached.

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

I'm not entirely sure how this patch will interact with magic included in
ipalib/plugins/dns.py:class dns_resolve(Command).

I would like to delete the 'normalization' from at least one of these places.

Also, as you know, DNS names are not strings and should be manipulated using
python-dns so all crazy things in DNS names do not break in weird corner
cases.


Updated patch attached.

Hmm, you bravely ignored my comment about class dns_resolve(Command) above,
sooo: NACK.

As far as I can tell ipalib/plugins/dns.py:class dns_resolve(Command) behaves
in the same brain-dead way as original is_host_resolvable() function. Please
fix both, not just one.


If you are sure that the behavior of the dns-resolve is bad, then updated
patch that removes the code which appending the api.env.domain to query.

Patch attached.

ACK


Pushed to master: 800c7023241fd6182da300cf120870072e6ca602

--
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 0381] admintool: Remove the option to override the log file

[Jan Cholasta]

On 25.11.2015 13:49, Tomas Babej wrote:



On 11/25/2015 01:29 PM, Jan Cholasta wrote:

On 25.11.2015 13:24, Tomas Babej wrote:

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

Hi,

This has been rarely used, and can be replaced by proper shell output
redirection.

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



Here's an updated version of the patch that gets rid of one missed
occurrence of log_file usage.


The ticket seems unrelated to the change.

Shouldn't the option be kept in the respective commands for backward
compatibility? See how the debug option is handled in AdminTool.



Yeah, the correct ticket is: https://fedorahosted.org/freeipa/ticket/5385

I'm curious, in what manner do you envision the backward compatibility?
The debug option is being replaced with verbose, but here we're removing
existing functionality since it does not work properly and is of little
use anyway.

So there are two possiblities I see:

1.) We remove the functionality and keep the option, just to be able to
say that this option is deprecated and bail out.

2.) We keep the functionality, and keep the option, just issue a warning
when it's being used.

 From my point of view: I did not do (1), but imho we can add it, albeit
it's a marginal usability improvement. As far as (2) goes, it does not
solve the underlying problem.


Add log_file_option=False argument to add_options(), if it's True, add 
the option to the parser. Set it to True in commands which currently 
have the option.


In _setup_logging(), if the option value is not None, add a handler for 
the file to the log manager (untested):


user_file_handler = dict(
name='user_file',
filename=self.options.log_file,
filemode=log_file_mode,
permission=0600,
level='debug',
format=ipa_log_manager.LOGGING_FORMAT_STANDARD_FILE,
)
ipa_log_manager.log_mgr.create_log_handlers([user_file_handler], 
None, None)


This way it will log into both the standard location and the 
user-specified file.


--
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 559] Fix kadmin for new users

[Tomas Babej]

On 11/25/2015 02:13 PM, Tomas Babej wrote:
> 
> 
> On 11/25/2015 02:00 PM, Martin Babinsky wrote:
>> On 11/24/2015 11:32 PM, Simo Sorce wrote:
>>> Ticket #937 was reopened a while ago because one corner case, new users
>>> that have never been assigned a password cause kadmin/kadmin.local to
>>> throw a fit when they try to visualize information about those user's
>>> principals.
>>>
>>> This patch fakes up modification information when no krbExtraData is
>>> available for the principal so that kadmin is happy.
>>>
>>> Tested and working as designed.
>>>
>>> Simo.
>>>
>>>
>>>
>> ACK
>>
> 
> Pushed to master: 0f52eddd1d2781ccc1941c191e9ab6e3ccf6919d
> 

On a related note, should we backport this to later branches?

-- 
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 559] Fix kadmin for new users

[Tomas Babej]

On 11/25/2015 02:00 PM, Martin Babinsky wrote:
> On 11/24/2015 11:32 PM, Simo Sorce wrote:
>> Ticket #937 was reopened a while ago because one corner case, new users
>> that have never been assigned a password cause kadmin/kadmin.local to
>> throw a fit when they try to visualize information about those user's
>> principals.
>>
>> This patch fakes up modification information when no krbExtraData is
>> available for the principal so that kadmin is happy.
>>
>> Tested and working as designed.
>>
>> Simo.
>>
>>
>>
> ACK
> 

Pushed to master: 0f52eddd1d2781ccc1941c191e9ab6e3ccf6919d

-- 
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 0351] call directly is_host_resolvable function to verify addresses in NS records

[Martin Basti]

On 25.11.2015 13:36, Petr Spacek wrote:

On 19.11.2015 11:11, Martin Basti wrote:

Testing if address is resolvable can be done by directly call of
is_host_resovable, instead of call the dns-resolve command which is doing the
same (works as proxy).

Patch attached.

ACK


Pushed to master: 7acfaee8abc7f4dc7f09e975147b96944231db5b

--
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 0360] Make dns-resolve command deprecated

[Martin Basti]

On 25.11.2015 13:36, Petr Spacek wrote:

On 25.11.2015 12:12, Martin Basti wrote:

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

Patch attached.

ACK


updated and pushed
Pushed to master: 749dfc3917cd5b3d0f222d144e8fc96e08308e10

--
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 559] Fix kadmin for new users

[Martin Babinsky]

On 11/24/2015 11:32 PM, Simo Sorce wrote:

Ticket #937 was reopened a while ago because one corner case, new users
that have never been assigned a password cause kadmin/kadmin.local to
throw a fit when they try to visualize information about those user's
principals.

This patch fakes up modification information when no krbExtraData is
available for the principal so that kadmin is happy.

Tested and working as designed.

Simo.




ACK

--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 0361] Remove invalid error message from topology upgrade

[Tomas Babej]

On 11/25/2015 01:58 PM, Tomas Babej wrote:
> 
> 
> On 11/25/2015 12:47 PM, Martin Basti wrote:
>> https://fedorahosted.org/freeipa/ticket/5482
>>
>> Patch attached.
>>
>>
> 
> ACK.
> 

Pushed to master: 801672cc6618947f5cc4607910871e695587fcbf

-- 
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 0361] Remove invalid error message from topology upgrade

[Tomas Babej]

On 11/25/2015 12:47 PM, Martin Basti wrote:
> https://fedorahosted.org/freeipa/ticket/5482
> 
> 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 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Tomas Babej]

On 11/25/2015 09:04 AM, Jan Cholasta wrote:
> On 23.11.2015 15:19, Rob Crittenden wrote:
>> Tomas Babej wrote:
>>>
>>>
>>> On 11/23/2015 01:50 PM, Jan Cholasta wrote:
 On 23.11.2015 13:40, Tomas Babej wrote:
>
>
> On 11/23/2015 01:31 PM, Jan Cholasta wrote:
>> On 23.11.2015 13:28, Tomas Babej wrote:
>>>
>>>
>>> On 11/23/2015 01:11 PM, Jan Cholasta wrote:
 On 23.11.2015 12:53, Tomas Babej wrote:
> Hi,
>
> If the code within the private_ccache contextmanager does not
> set/removes the KRB5CCNAME, the pop method will raise KeyError,
> which
> will cause unnecessary termination of the code flow.
>
> Make sure the KRB5CCNAME is popped out of os.environ only if
> present.
>
> Tomas

 NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.
>>>
>>> Yeah, well, both ways are equivalent, I found the if statement more
>>> explicit. We can go with the suggested version, if it's more
>>> pleasing
>>> though - patch is attached.
>>>

 Also I don't think the comment is necessary, it's quite obvious
 what the
 code does.

>>>
>>> I don't think an explanatory comment can hurt, ever. Worst thing
>>> that
>>> happens is that somebody is assured that he understands the code
>>> correctly.
>>
>> Actually the worst thing is that someone changes the code without
>> changing the comment. If the comment does not add any real value,
>> it's
>> only a maintenance burden.
>>
>
> Yep, that's a real issue in our code base (i.e. I recently removed
> such
> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
> sure the comments describe the implementation properly is on the
> author/reviewer though.
>
> What's "added value" highly depends on your skill set, and familiarity
> with the code base. Particularly the familiarity with the code base
> can
> diminish over time even for the author, and those are the times where
> comments can come to the rescue.

 Let's take a look at the code:

  if original_value is not None:
  os.environ['KRB5CCNAME'] = original_value
  else:
  os.environ.pop('KRB5CCNAME', None)

 Can you tell me what in there couldn't be obvious to a person with even
 the most basic skill set?

 IMHO a docstring for private_cccache would add some real value, but
 this
 comment alone does not.

>
> For a newcomer to the project, even a trivial comment (from the
> point of
> view of the experienced developer) can be valuable.

 Following this logic, there should be a comment for every line of our
 code, which is ridiculous.

>>>
>>> Here's the version of the patch with the comment removed.
>>
>> IMHO the comment should have been something like "ensure no KRB5CCNAME
>> otherwise it blows up in ..." If it took you 20 minutes to track down a
>> one-line change then a comment might save the next guy who changes the
>> behavior.
> 
> As I wrote earlier, I think it would make more sense to put this into
> the docstring of private_ccache().
> 
> ACK on the patch.
> 

Pushed to master: 1904d7cc3ab33046428dbdcb7c6f521f9e083287

-- 
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 0381] admintool: Remove the option to override the log file

[Tomas Babej]

On 11/25/2015 01:29 PM, Jan Cholasta wrote:
> On 25.11.2015 13:24, Tomas Babej wrote:
>> On 11/10/2015 02:22 PM, Tomas Babej wrote:
>>> Hi,
>>>
>>> This has been rarely used, and can be replaced by proper shell output
>>> redirection.
>>>
>>> https://fedorahosted.org/freeipa/ticket/5408
>>>
>>
>> Here's an updated version of the patch that gets rid of one missed
>> occurrence of log_file usage.
> 
> The ticket seems unrelated to the change.
> 
> Shouldn't the option be kept in the respective commands for backward
> compatibility? See how the debug option is handled in AdminTool.
> 

Yeah, the correct ticket is: https://fedorahosted.org/freeipa/ticket/5385

I'm curious, in what manner do you envision the backward compatibility?
The debug option is being replaced with verbose, but here we're removing
existing functionality since it does not work properly and is of little
use anyway.

So there are two possiblities I see:

1.) We remove the functionality and keep the option, just to be able to
say that this option is deprecated and bail out.

2.) We keep the functionality, and keep the option, just issue a warning
when it's being used.

>From my point of view: I did not do (1), but imho we can add it, albeit
it's a marginal usability improvement. As far as (2) goes, it does not
solve the underlying problem.

Tomas
From 3a1c4a54c0be5a8bb9802df3bd023d5afb82c9f1 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Tue, 10 Nov 2015 14:20:45 +0100
Subject: [PATCH] admintool: Remove the option to override the log file

This has been rarely used, and can be replaced by proper shell output
redirection.

https://fedorahosted.org/freeipa/ticket/5385
---
 install/tools/man/ipa-kra-install.1|  3 ---
 install/tools/man/ipa-server-upgrade.1 |  3 ---
 ipapython/admintool.py | 17 ++---
 ipapython/install/cli.py   |  7 +--
 4 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/install/tools/man/ipa-kra-install.1 b/install/tools/man/ipa-kra-install.1
index e3133eee188af0b613fca76b51d2f5b4f8d1ba7d..5bda4fe4b80947588ad7afde2c9f8fd81f320614 100644
--- a/install/tools/man/ipa-kra-install.1
+++ b/install/tools/man/ipa-kra-install.1
@@ -47,9 +47,6 @@ Enable debug output when more verbose output is needed
 .TP
 \fB\-q\fR, \fB\-\-quiet\fR
 Output only errors
-.TP
-\fB\-v\fR, \fB\-\-log-file\fR=\fFILE\fR
-Log to the given file
 .SH "EXIT STATUS"
 0 if the command was successful
 
diff --git a/install/tools/man/ipa-server-upgrade.1 b/install/tools/man/ipa-server-upgrade.1
index cbbdc590171bff0a88b67bcf1de961fd783ac35c..7f06e09fc4d181fa9a89772e7285d4a6567bc361 100644
--- a/install/tools/man/ipa-server-upgrade.1
+++ b/install/tools/man/ipa-server-upgrade.1
@@ -36,9 +36,6 @@ Print debugging information
 \fB\-q\fR, \fB\-\-quiet\fR
 Output only errors
 .TP
-\fB-\-log-file=FILE\fR
-Log to given file
-.TP
 
 .SH "EXIT STATUS"
 0 if the command was successful
diff --git a/ipapython/admintool.py b/ipapython/admintool.py
index e40f300b1318b7325eb2b39ec83970753d39c406..0c8a5d54676fac0704202cf183990115978cebed 100644
--- a/ipapython/admintool.py
+++ b/ipapython/admintool.py
@@ -113,8 +113,6 @@ class AdminTool(object):
 action="store_true", help="alias for --verbose (deprecated)")
 group.add_option("-q", "--quiet", dest="quiet", default=False,
 action="store_true", help="output only errors")
-group.add_option("--log-file", dest="log_file", default=None,
-metavar="FILE", help="log to the given file")
 parser.add_option_group(group)
 
 @classmethod
@@ -208,9 +206,8 @@ class AdminTool(object):
 :param _to_file: Setting this to false will disable logging to file.
 For internal use.
 
-If the --log-file option was given or if a filename is in
-self.log_file_name, the tool will log to that file. In this case,
-all messages are logged.
+If self.log_file_name is set, the tool will log to that file.
+In this case, all messages are logged.
 
 What is logged to the console depends on command-line options:
 the default is INFO; --quiet sets ERROR; --verbose sets DEBUG.
@@ -232,12 +229,8 @@ class AdminTool(object):
 self._setup_logging(log_file_mode=log_file_mode)
 
 def _setup_logging(self, log_file_mode='w', no_file=False):
-if no_file:
-log_file_name = None
-elif self.options.log_file:
-log_file_name = self.options.log_file
-else:
-log_file_name = self.log_file_name
+log_file_name = None if no_file else self.log_file_name
+
 if self.options.verbose:
 console_format = '%(name)s: %(levelname)s: %(message)s'
 verbose = True
@@ -249,10 +242,12 @@ class AdminTool(object):
 verbose = False
 else:
 verbose = True
+
 ipa_log_manager.standard_logging_setup(
 log_file_name, console_format=console_for

Re: [Freeipa-devel] [PATCH 0359] Fix forwardzone upgrade

[Jan Cholasta]

On 25.11.2015 13:36, Petr Spacek wrote:

On 25.11.2015 10:42, Martin Basti wrote:

When original zone belongs to realmdomains, upgrade will fail.

The attached patch solve the issue.

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


ACK


Pushed to:
master: 6eeb4e4f2a9fb6fe5cf83e6b84c737ad3e295de1
ipa-4-2: 8b0f60fbfb5192a8dde2b459d9d15ab11337cf66

--
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 0360] Make dns-resolve command deprecated

[Petr Spacek]

On 25.11.2015 12:12, Martin Basti wrote:
> https://fedorahosted.org/freeipa/ticket/5466
> 
> Patch attached.

ACK

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0344] Use absolute domain name in detection of A/AAAA records

[Petr Spacek]

On 19.11.2015 11:05, Martin Basti wrote:
> 
> 
> On 18.11.2015 18:33, Petr Spacek wrote:
>> On 12.11.2015 13:58, Martin Basti wrote:
>>>
>>> On 09.11.2015 08:47, Petr Spacek wrote:
 On 4.11.2015 16:16, Martin Basti wrote:
> Patch attached.
>
> https://fedorahosted.org/freeipa/ticket/5421
 I'm not entirely sure how this patch will interact with magic included in
 ipalib/plugins/dns.py:class dns_resolve(Command).

 I would like to delete the 'normalization' from at least one of these 
 places.

 Also, as you know, DNS names are not strings and should be manipulated 
 using
 python-dns so all crazy things in DNS names do not break in weird corner
 cases.

>>> Updated patch attached.
>> Hmm, you bravely ignored my comment about class dns_resolve(Command) above,
>> sooo: NACK.
>>
>> As far as I can tell ipalib/plugins/dns.py:class dns_resolve(Command) behaves
>> in the same brain-dead way as original is_host_resolvable() function. Please
>> fix both, not just one.
>>
> If you are sure that the behavior of the dns-resolve is bad, then updated
> patch that removes the code which appending the api.env.domain to query.
> 
> Patch attached.

ACK

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0351] call directly is_host_resolvable function to verify addresses in NS records

[Petr Spacek]

On 19.11.2015 11:11, Martin Basti wrote:
> Testing if address is resolvable can be done by directly call of
> is_host_resovable, instead of call the dns-resolve command which is doing the
> same (works as proxy).
> 
> Patch attached.

ACK

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0359] Fix forwardzone upgrade

[Petr Spacek]

On 25.11.2015 10:42, Martin Basti wrote:
> When original zone belongs to realmdomains, upgrade will fail.
> 
> The attached patch solve the issue.
> 
> https://fedorahosted.org/freeipa/ticket/5472

ACK

-- 
Petr^2 Spacek

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

Re: [Freeipa-devel] [PATCH 0381] admintool: Remove the option to override the log file

[Jan Cholasta]

On 25.11.2015 13:24, Tomas Babej wrote:

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

Hi,

This has been rarely used, and can be replaced by proper shell output
redirection.

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



Here's an updated version of the patch that gets rid of one missed
occurrence of log_file usage.


The ticket seems unrelated to the change.

Shouldn't the option be kept in the respective commands for backward 
compatibility? See how the debug option is handled in AdminTool.


--
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 0381] admintool: Remove the option to override the log file

[Tomas Babej]

On 11/10/2015 02:22 PM, Tomas Babej wrote:
> Hi,
> 
> This has been rarely used, and can be replaced by proper shell output
> redirection.
> 
> https://fedorahosted.org/freeipa/ticket/5408
> 

Here's an updated version of the patch that gets rid of one missed
occurrence of log_file usage.

Tomas
From 992bee01dd9f30e11fb04725cecdce84d0ecf7d0 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Tue, 10 Nov 2015 14:20:45 +0100
Subject: [PATCH] admintool: Remove the option to override the log file

This has been rarely used, and can be replaced by proper shell output
redirection.

https://fedorahosted.org/freeipa/ticket/5408
---
 install/tools/man/ipa-kra-install.1|  3 ---
 install/tools/man/ipa-server-upgrade.1 |  3 ---
 ipapython/admintool.py | 17 ++---
 ipapython/install/cli.py   |  7 +--
 4 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/install/tools/man/ipa-kra-install.1 b/install/tools/man/ipa-kra-install.1
index e3133eee188af0b613fca76b51d2f5b4f8d1ba7d..5bda4fe4b80947588ad7afde2c9f8fd81f320614 100644
--- a/install/tools/man/ipa-kra-install.1
+++ b/install/tools/man/ipa-kra-install.1
@@ -47,9 +47,6 @@ Enable debug output when more verbose output is needed
 .TP
 \fB\-q\fR, \fB\-\-quiet\fR
 Output only errors
-.TP
-\fB\-v\fR, \fB\-\-log-file\fR=\fFILE\fR
-Log to the given file
 .SH "EXIT STATUS"
 0 if the command was successful
 
diff --git a/install/tools/man/ipa-server-upgrade.1 b/install/tools/man/ipa-server-upgrade.1
index cbbdc590171bff0a88b67bcf1de961fd783ac35c..7f06e09fc4d181fa9a89772e7285d4a6567bc361 100644
--- a/install/tools/man/ipa-server-upgrade.1
+++ b/install/tools/man/ipa-server-upgrade.1
@@ -36,9 +36,6 @@ Print debugging information
 \fB\-q\fR, \fB\-\-quiet\fR
 Output only errors
 .TP
-\fB-\-log-file=FILE\fR
-Log to given file
-.TP
 
 .SH "EXIT STATUS"
 0 if the command was successful
diff --git a/ipapython/admintool.py b/ipapython/admintool.py
index e40f300b1318b7325eb2b39ec83970753d39c406..0c8a5d54676fac0704202cf183990115978cebed 100644
--- a/ipapython/admintool.py
+++ b/ipapython/admintool.py
@@ -113,8 +113,6 @@ class AdminTool(object):
 action="store_true", help="alias for --verbose (deprecated)")
 group.add_option("-q", "--quiet", dest="quiet", default=False,
 action="store_true", help="output only errors")
-group.add_option("--log-file", dest="log_file", default=None,
-metavar="FILE", help="log to the given file")
 parser.add_option_group(group)
 
 @classmethod
@@ -208,9 +206,8 @@ class AdminTool(object):
 :param _to_file: Setting this to false will disable logging to file.
 For internal use.
 
-If the --log-file option was given or if a filename is in
-self.log_file_name, the tool will log to that file. In this case,
-all messages are logged.
+If self.log_file_name is set, the tool will log to that file.
+In this case, all messages are logged.
 
 What is logged to the console depends on command-line options:
 the default is INFO; --quiet sets ERROR; --verbose sets DEBUG.
@@ -232,12 +229,8 @@ class AdminTool(object):
 self._setup_logging(log_file_mode=log_file_mode)
 
 def _setup_logging(self, log_file_mode='w', no_file=False):
-if no_file:
-log_file_name = None
-elif self.options.log_file:
-log_file_name = self.options.log_file
-else:
-log_file_name = self.log_file_name
+log_file_name = None if no_file else self.log_file_name
+
 if self.options.verbose:
 console_format = '%(name)s: %(levelname)s: %(message)s'
 verbose = True
@@ -249,10 +242,12 @@ class AdminTool(object):
 verbose = False
 else:
 verbose = True
+
 ipa_log_manager.standard_logging_setup(
 log_file_name, console_format=console_format,
 filemode=log_file_mode, debug=debug, verbose=verbose)
 self.log = ipa_log_manager.log_mgr.get_logger(self)
+
 if log_file_name:
 self.log.debug('Logging to %s' % log_file_name)
 elif not no_file:
diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py
index aed0bc9fe12e0c56987a4e2f78d73f476dcfc2c8..37ede148b0cbde2f4c4ef46bddf39d13cbda6ed9 100644
--- a/ipapython/install/cli.py
+++ b/ipapython/install/cli.py
@@ -265,12 +265,7 @@ class ConfigureTool(admintool.AdminTool):
 index += 1
 
 def _setup_logging(self, log_file_mode='w', no_file=False):
-if no_file:
-log_file_name = None
-elif self.options.log_file:
-log_file_name = self.options.log_file
-else:
-log_file_name = self.log_file_name
+log_file_name = None if no_file else self.log_file_name
 ipa_log_manager.standard_logging_setup(log_file_name,
debug=self.options.verbo

Re: [Freeipa-devel] [PATCH 0360] Make dns-resolve command deprecated

[Petr Spacek]

On 25.11.2015 12:12, Martin Basti wrote:
> https://fedorahosted.org/freeipa/ticket/5466
> 
> Patch attached.

Please fix missing space after ',' and typo in 'choosen'. Otherwise it works,
so cond-ACK.

-- 
Petr^2 Spacek

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

[Freeipa-devel] [PATCH 0361] Remove invalid error message from topology upgrade

[Martin Basti]

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

Patch attached.
From abceb5bea904984e9a50d0fcd454269d48c7b2cf Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 25 Nov 2015 12:42:02 +0100
Subject: [PATCH] Remove invalid error messages from topology upgrade

Return False does not mean that update failed, it mean that nothing has
been updated, respectively ldap is up to date.

https://fedorahosted.org/freeipa/ticket/5482
---
 ipaserver/install/cainstance.py | 4 +---
 ipaserver/install/plugins/update_ca_topology.py | 5 ++---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 90edb362f496b89f433532bf8786c29da3902de9..5237a5aee8fda13ca3e27205938a8927dfb5752b 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -809,9 +809,7 @@ class CAInstance(DogtagInstance):
 'SUFFIX': api.env.basedn,
 'FQDN': self.fqdn,
 })
-rv = ld.update([paths.CA_TOPOLOGY_ULDIF])
-if not rv:
-raise RuntimeError("Failed to update CA topology configuration")
+ld.update([paths.CA_TOPOLOGY_ULDIF])
 
 def __disable_nonce(self):
 # Turn off Nonces
diff --git a/ipaserver/install/plugins/update_ca_topology.py b/ipaserver/install/plugins/update_ca_topology.py
index ce4b5bc9aa04f70df32b9184a10728696ff091e3..311db3f0c423f7a9110766afd4d57fa3b24520be 100644
--- a/ipaserver/install/plugins/update_ca_topology.py
+++ b/ipaserver/install/plugins/update_ca_topology.py
@@ -25,9 +25,8 @@ class update_ca_topology(Updater):
 'SUFFIX': self.api.env.basedn,
 'FQDN': self.api.env.host,
 })
-rv = ld.update([paths.CA_TOPOLOGY_ULDIF])
-if not rv:
-self.log.error("Failed to update CA topology configuration")
+
+ld.update([paths.CA_TOPOLOGY_ULDIF])
 
 return False, []
 
-- 
2.5.0

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

[Freeipa-devel] [PATCH 0360] Make dns-resolve command deprecated

[Martin Basti]

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

Patch attached.
From 94bc97e4509d6942a5176561f17f3eb547469ef4 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 18 Nov 2015 19:44:08 +0100
Subject: [PATCH] Make command dns-resolve deprecated.

To debug DNS issues other commands should be used like 'dig', 'host',
'nslookup' instead of command 'ipa dns-resolve'.

This command is executed on server side, what may not be helpful with
debugging clients.

'ipa dns-resolve' command is worse copy of host command, users should use
'host' command instead.

dns-resolve is removed from CLI

https://fedorahosted.org/freeipa/ticket/5466
---
 ipalib/messages.py| 10 ++
 ipalib/plugins/dns.py | 17 ++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/ipalib/messages.py b/ipalib/messages.py
index 7b4aaf4d85f72df3a27a05f23ee66b01c1e062db..44fee6d1512a8a9fb9f5d90ef090e86d8a7e4b84 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -311,6 +311,16 @@ class DNSSuspiciousRelativeName(PublicMessage):
 )
 
 
+class CommandDeprecatedWarning(PublicMessage):
+"""
+**13015** Used when user uses a deprecated option
+"""
+
+errno = 13015
+type = "warning"
+format = _(u"'%(command)s' is deprecated. %(additional_info)s")
+
+
 def iter_messages(variables, base):
 """Return a tuple with all subclasses
 """
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 68897757e5dba4246689e8431b49880b1645a3ca..6896784b47740a4e73c32adedfd01e0ea3dfd023 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -4180,7 +4180,9 @@ class dnsrecord_find(LDAPSearch):
 
 @register()
 class dns_resolve(Command):
-__doc__ = _('Resolve a host name in DNS.')
+__doc__ = _('Resolve a host name in DNS. (Deprecated)')
+
+NO_CLI = True
 
 has_output = output.standard_value
 msg_summary = _('Found \'%(value)s\'')
@@ -4198,8 +4200,17 @@ class dns_resolve(Command):
 raise errors.NotFound(
 reason=_('Host \'%(host)s\' not found') % {'host': query}
 )
-
-return dict(result=True, value=query)
+result = dict(result=True, value=query)
+messages.add_message(
+options['version'], result,
+messages.CommandDeprecatedWarning(
+command='dns-resolve',
+additional_info='The command may return an unexpected result,'
+'the resolution of the DNS domain is done on '
+'a randomly choosen IPA server.'
+)
+)
+return result
 
 
 @register()
-- 
2.5.0

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

Re: [Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 24.11.2015 17:21, Petr Viktorin wrote:

On 11/23/2015 10:50 AM, Jan Cholasta wrote:

On 23.11.2015 07:43, Jan Cholasta wrote:

On 19.11.2015 00:55, Petr Viktorin wrote:

On 11/03/2015 02:39 PM, Petr Viktorin wrote:

Hello,
Python 3's strings are Unicode, so data coming to or leaving a Python
program needs to be decoded/encoded if it's to be handled as a string.
One of the boundaries where encoding is necessary is external programs,
specifically, ipautil.run.
Unfortunately there's no one set of options that would work for all
run() invocations, so I went through them all and specified the
stdin/stdout/stderr encoding where necessary. I've also updated the
call
sites to make it clearer if the return values are used or not.
If an encoding is not set, run() will accept/return bytes. (This is a
fail-safe setting, since it can't raise errors, and using bytes where
strings are expected generally fails loudly in py3.)

Note that the changes are not effective under Python 2.


ping,
Could someone look at this patch?


Looking.


1) I think a better approach would be to use str for stdin/stdout/stderr
by default in both Python 2 and 3, i.e. do nothing in Python 2 and
encode/decode using locale.getpreferredencoding() in Python 3. This is
consistent with sys.std{in,out,err} in both Python 2 and 3. Using bytes
or different encoding should be opt-in.

Note that different encoding should be used only if the LC_ALL or LANG
variables are overriden in the command's environment.


That would assume the output of *everything* run via ipautil.run can be
decoded using the locale encoding. Any stray invalid byte would make IPA
crash, even in cases where we don't care about the output. IPA calls too
many weird tools to assume they all output text.


Such a stray invalid byte may bubble through our code and cause havoc 
somewhere else, which is much harder to troubleshoot than a crash in 
ipautil.run(), where you can see that it was a misbehaving command that 
caused the crash and exactly what command it was.


If we don't care about output somewhere, we should not capture it there 
at all.



3) I think the "surrogateescape" error handler would be better for
non-strict decoding, as the text can be encoded back to bytes without
loss of information.


Non-strict decoding is meant for logging, so we want backslash escapes
rather than invalid surrogates. If you need round-tripping then you
should use bytes.


I see. There is no guarantee that non-strict output will be used only 
for logging though.


I think it might be better to return raw output and let the caller use a 
decode-for-logging utility function instead. The logger already escapes 
invalid bytes itself, so the function would have to be used only when 
raising exceptions.



I've changed the error strategy to 'replace' though, as I realized
'backslashreplace' doesn't work for decoding.


--
Jan Cholasta

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

[Freeipa-devel] [PATCH 0359] Fix forwardzone upgrade

[Martin Basti]

When original zone belongs to realmdomains, upgrade will fail.

The attached patch solve the issue.

https://fedorahosted.org/freeipa/ticket/5472
From 163a0e88cf4dabf53b4b1321f4202683d3fea06d Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 25 Nov 2015 09:57:07 +0100
Subject: [PATCH] Fix upgrade of forwardzones when zone is in realmdomains

https://fedorahosted.org/freeipa/ticket/5472
---
 ipalib/plugins/realmdomains.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipalib/plugins/realmdomains.py b/ipalib/plugins/realmdomains.py
index 92f5cab7f4b75f2826fda97ba23103e17331f779..daaced5a2d0ac8429ebbba6ae23325c0d8abd040 100644
--- a/ipalib/plugins/realmdomains.py
+++ b/ipalib/plugins/realmdomains.py
@@ -280,7 +280,7 @@ class realmdomains_mod(LDAPUpdate):
 continue
 
 try:
-api.Command['dnsrecord_add'](
+self.api.Command['dnsrecord_add'](
 unicode(domain),
 u'_kerberos',
 txtrecord=api.env.realm
@@ -309,7 +309,7 @@ class realmdomains_mod(LDAPUpdate):
 continue
 
 try:
-api.Command['dnsrecord_del'](
+self.api.Command['dnsrecord_del'](
 unicode(domain),
 u'_kerberos',
 txtrecord=api.env.realm
-- 
2.5.0

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

Re: [Freeipa-devel] [PATCH 0104] do not disconnect when using existing connection to check default CA ACLs

[Jan Cholasta]

On 25.11.2015 10:40, Fraser Tweedale wrote:

On Wed, Nov 25, 2015 at 09:28:27AM +0100, Martin Babinsky wrote:

On 11/25/2015 07:21 AM, Jan Cholasta wrote:

On 25.11.2015 05:56, Fraser Tweedale wrote:

On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote:

On 24.11.2015 17:17, Martin Babinsky wrote:

On 11/24/2015 05:10 PM, Martin Babinsky wrote:

On 11/24/2015 05:01 PM, Martin Babinsky wrote:

On 11/24/2015 04:58 PM, Jan Cholasta wrote:

On 24.11.2015 16:48, Martin Babinsky wrote:

On 11/24/2015 04:44 PM, Martin Babinsky wrote:

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


forgot to attach the actual file *slaps himself*


ipaserver/install/cainstance.py:1849: [E1101(no-member),
ensure_default_caacl] Instance of 'API' has no 'Backed' member)



Fixed




Also attaching rebased patch for ipa-4-2




Ignore the rebased patch, the original 104.1 applies on 4-2 just fine.


Thanks, ACK.

Pushed to:
master: ed830af693c596b286b30959eb3166b59cc030c6
ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a


Thanks for anaysing and fixing these connection issues which were
presumably introduced by my patches.  I did not hit these in my
testing.  Admittedly I was focused on the ipa-4-2 branch and my
testing was mainly done there.

Brittle LDAP connection logic in install and upgrade is an ongoing
problem.  What shall we do to improve the situation?  Push
connection details into the Backend and let it connect if/when
needed, rather than managing connection state from the outside?

I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491


I don't think we need to be smart about it, everyone just needs to make
sure that when an ad-hoc connection is opened somewhere, it is also
closed in the same place. The same applies for any other resource.



Thanks,
Fraser






Maybe it would be a good idea to implement some decorator/context manager to
connect/disconnect from specified connection and wrap the CRUD code. Example
usage:

"""
decorator:
@connected(api.Backend.ldap2, ldapi=True)
def do_magic(api, *args, **kwargs):
# do stuff

context manager:

with connected(api.Backend.ldap2, ccache=example_ccache):
 do_some_other_magic()
"""

I remember Jan having some objections against this so it would be nice to
hear why this is not a good idea.


This would not handle a connection loss (e.g. due to DS restart) but
would still a useful and relatively non-intrusive measure.
Furthermore it will make it easier for developer to do the Right
Thing and reviewer to verify (i.e. it's easy to see when patch does
LDAP things but is not using the context manager).

Jan, it is well and good to say "developer just has to remember to
open and close the connection" - and I accept that I have been the
main culprit recently - but let us make it as easy as possible to
get right, and hard for a patch that does not do the proper resource
management to slip through.


See the other thread.

--
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 0104] do not disconnect when using existing connection to check default CA ACLs

[Fraser Tweedale]

On Wed, Nov 25, 2015 at 09:28:27AM +0100, Martin Babinsky wrote:
> On 11/25/2015 07:21 AM, Jan Cholasta wrote:
> >On 25.11.2015 05:56, Fraser Tweedale wrote:
> >>On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote:
> >>>On 24.11.2015 17:17, Martin Babinsky wrote:
> On 11/24/2015 05:10 PM, Martin Babinsky wrote:
> >On 11/24/2015 05:01 PM, Martin Babinsky wrote:
> >>On 11/24/2015 04:58 PM, Jan Cholasta wrote:
> >>>On 24.11.2015 16:48, Martin Babinsky wrote:
> On 11/24/2015 04:44 PM, Martin Babinsky wrote:
> >https://fedorahosted.org/freeipa/ticket/5459
> >
> forgot to attach the actual file *slaps himself*
> >>>
> >>>ipaserver/install/cainstance.py:1849: [E1101(no-member),
> >>>ensure_default_caacl] Instance of 'API' has no 'Backed' member)
> >>>
> >>
> >>Fixed
> >>
> >>
> >>
> >Also attaching rebased patch for ipa-4-2
> >
> >
> >
> Ignore the rebased patch, the original 104.1 applies on 4-2 just fine.
> >>>
> >>>Thanks, ACK.
> >>>
> >>>Pushed to:
> >>>master: ed830af693c596b286b30959eb3166b59cc030c6
> >>>ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a
> >>>
> >>Thanks for anaysing and fixing these connection issues which were
> >>presumably introduced by my patches.  I did not hit these in my
> >>testing.  Admittedly I was focused on the ipa-4-2 branch and my
> >>testing was mainly done there.
> >>
> >>Brittle LDAP connection logic in install and upgrade is an ongoing
> >>problem.  What shall we do to improve the situation?  Push
> >>connection details into the Backend and let it connect if/when
> >>needed, rather than managing connection state from the outside?
> >>
> >>I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491
> >
> >I don't think we need to be smart about it, everyone just needs to make
> >sure that when an ad-hoc connection is opened somewhere, it is also
> >closed in the same place. The same applies for any other resource.
> >
> >>
> >>Thanks,
> >>Fraser
> >>
> >
> >
> 
> Maybe it would be a good idea to implement some decorator/context manager to
> connect/disconnect from specified connection and wrap the CRUD code. Example
> usage:
> 
> """
> decorator:
> @connected(api.Backend.ldap2, ldapi=True)
> def do_magic(api, *args, **kwargs):
># do stuff
> 
> context manager:
> 
> with connected(api.Backend.ldap2, ccache=example_ccache):
> do_some_other_magic()
> """
> 
> I remember Jan having some objections against this so it would be nice to
> hear why this is not a good idea.
> 
This would not handle a connection loss (e.g. due to DS restart) but
would still a useful and relatively non-intrusive measure.
Furthermore it will make it easier for developer to do the Right
Thing and reviewer to verify (i.e. it's easy to see when patch does
LDAP things but is not using the context manager).

Jan, it is well and good to say "developer just has to remember to
open and close the connection" - and I accept that I have been the
main culprit recently - but let us make it as easy as possible to
get right, and hard for a patch that does not do the proper resource
management to slip through.

Cheers,
Fraser

-- 
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 558] Allow disabling requireing preauth by default for Service Principal Names

[Martin Babinsky]

On 11/24/2015 10:20 PM, Simo Sorce wrote:

This addresses #3860, giving admins the option to not require preauth
for Hosts and services.

I did not add this option by default, although it does reduce the load
on the KDC as well as speed up TGT acquisition for service principal
accounts that acquire TGTs.

Tested and working as expected (SPNs are not returned PREAUTH_NEEDED
error while normal users are).

HTH,
Simo.




Hi Simo,

I was not able to apply the patch on current master branch:

"""
git am 
../review/ssorce/3860/freeipa-simo-558-1-Allow-admins-to-disable-preauth-for-SPNs.patch 
-3 


Applying: Allow admins to disable preauth for SPNs.
error: invalid object 100644 a6b4d4349a9ac6de453d9ad3c679ec32add4e43b 
for 'ipalib/plugins/config.py'

fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Allow admins to disable preauth for SPNs.
"""

It seems that I nedd to apply some of your other patches first (which one?)

--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [IPAQE][REVIEW-REQUEST][TEST PLAN] Replica promotion

[Martin Basti]

On 23.11.2015 18:51, Oleg Fayans wrote:

Hi all,

Here is a draft of the Replica Promotion test plan
http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan



Hello,

Test case: ipa-replica-manage is deprecated in domain level 1
I would be more specific in this test case, deprecated is only connect 
and disconnect subcommand


You have tests for replica manage, but as I wrote in different thread[1] 
on freeipa-devel, also ipa-csreplica-manage works different with domain 
level 1.


4)
ipa-csreplica-manage behaves differently with domain level 1
4a)
ipa-csreplica-manage connect should not work with domain level 1
4b)
ipa-csreplica-manage disconnect should not work with domain level 1
4c)
ipa-csreplica-manage del should not work with domain level 1

IMO we should have also tests for ipa-replica-manage and 
ipa-csreplica-manage with domain level 0, AFAIK we do not have any of 
them, and domain level 0 will be supported for very long time.


Test case: Replica created using old workflow is functional after domain 
upgrade
It is nice, but I would like to extend it to install another replica to 
existing topology after the domain level is raised.


Also we had IRC chat about ipa-ca-install. We found out that we do not 
have tests for standalone call of ipa-ca-install on replica.

So I miss these testcases, as I wrote before in original thread[1]:
1)
CA has been affected by replica promotion patches
1a)
test if ipa-ca-install works on replica with domain level 1
1b)
test if ipa-ca-install works on replica with domain level 0

Martin

[1] 
https://www.redhat.com/archives/freeipa-devel/2015-November/msg00106.html


--
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] Add option to disable setkeytab extended operations

[Sumit Bose]

On Tue, Nov 24, 2015 at 02:42:32PM -0500, Simo Sorce wrote:
> Since some time we use the getkeytab operation to fetch keytabs on newer
> clients. According to bug #232 setkeytab can be used to circumvent
> password quality controls so it needs to be slowly retired.

ipasam uses this exop to create the cross-realm TGT principal objects,
krbtgt/DOM.A@DOM.B. What should be used instead to make sure that
setkeytab can safely be disabled?

bye,
Sumit

> 
> The attached patches implement #5485 in 2 parts.
> 
> The first introduces the option DisableSetKeytab which globally disables
> the setkeytab extended operation. This is set to false by default for
> backwards compatibility.
> 
> The second introduces an option called DisableUserSetKeytab, which is
> active by default in new installs (but not in upgraded ones), and only
> disables the use of setkeytab for ipa suers, but not for hosts/services.
> This is because user's are the ones that may abuse the interface to
> escape password policies and users also normally do not acquire keytabs,
> so it is a safe bet to disable just them by default in new installs.
> 
> (Testing in progress)
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

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

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

[Jan Cholasta]

On 20.11.2015 16:49, Jan Cholasta wrote:

On 19.11.2015 17:43, Simo Sorce wrote:

510:
- We should probably tightenup the ACI to allos host X to only add
memberPrincipal = X and no other value, also the host should not be
allowed to change the memberPrincipal attribute only the keys.
If we can't express this in ACIs we can live with the ones you propose
though.


I think this can be done.


Turns out this can be done only if member (or some other DN attribute) 
is used instead of memberPrincipal.


So, to reiterate:


2) Why is 'memberPrincipal' used in cn=custodia instead of 'member'?

If 'member' was used instead, we would gain referential integrity and
the ability to add ACIs based on the attribute (think
userattr="member#USERDN").


To avoid referential integrity and mixup with other group objects, it
was intentional.


Why is referential integrity a problem?

Mixup with other group objects can be solved by using a different attribute.

--
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 0104] do not disconnect when using existing connection to check default CA ACLs

[Martin Babinsky]

On 11/25/2015 09:56 AM, Jan Cholasta wrote:

On 25.11.2015 09:28, Martin Babinsky wrote:

On 11/25/2015 07:21 AM, Jan Cholasta wrote:

On 25.11.2015 05:56, Fraser Tweedale wrote:

On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote:

On 24.11.2015 17:17, Martin Babinsky wrote:

On 11/24/2015 05:10 PM, Martin Babinsky wrote:

On 11/24/2015 05:01 PM, Martin Babinsky wrote:

On 11/24/2015 04:58 PM, Jan Cholasta wrote:

On 24.11.2015 16:48, Martin Babinsky wrote:

On 11/24/2015 04:44 PM, Martin Babinsky wrote:

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


forgot to attach the actual file *slaps himself*


ipaserver/install/cainstance.py:1849: [E1101(no-member),
ensure_default_caacl] Instance of 'API' has no 'Backed' member)



Fixed




Also attaching rebased patch for ipa-4-2




Ignore the rebased patch, the original 104.1 applies on 4-2 just
fine.


Thanks, ACK.

Pushed to:
master: ed830af693c596b286b30959eb3166b59cc030c6
ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a


Thanks for anaysing and fixing these connection issues which were
presumably introduced by my patches.  I did not hit these in my
testing.  Admittedly I was focused on the ipa-4-2 branch and my
testing was mainly done there.

Brittle LDAP connection logic in install and upgrade is an ongoing
problem.  What shall we do to improve the situation?  Push
connection details into the Backend and let it connect if/when
needed, rather than managing connection state from the outside?

I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491


I don't think we need to be smart about it, everyone just needs to make
sure that when an ad-hoc connection is opened somewhere, it is also
closed in the same place. The same applies for any other resource.



Thanks,
Fraser






Maybe it would be a good idea to implement some decorator/context
manager to connect/disconnect from specified connection and wrap the
CRUD code. Example usage:

"""
decorator:
@connected(api.Backend.ldap2, ldapi=True)
def do_magic(api, *args, **kwargs):
# do stuff

context manager:

with connected(api.Backend.ldap2, ccache=example_ccache):
 do_some_other_magic()
"""

I remember Jan having some objections against this so it would be nice
to hear why this is not a good idea.


a) For ad-hoc connection objects, LDAPClient should be used instead of
ldap2. It already supports the context manager protocol:

 with LDAPClient(uri) as conn:
 conn.gssapi_bind()
 ...

I didn't know that you can and should use LDAPClient this way. We should 
document it somewhere and encourage other contributors to use this approach.



b) The api.Backend.ldap2 object should be connected and disconnected
exactly once per api object per command. This is not done ATM and the
usual workaround is to connect it manually, which has lead to the mess
we have now.

Then we should find time to implement this behavior so that we can stop 
shooting ourselves in the foot when messing around with API commands.



Also, only GSSAPI authentication with valid IPA user credentials should
be used with ldap2, even for ldapi connections.




--
Martin^3 Babinsky

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

[Freeipa-devel] [pytest-multihost-devel][PATCH] Functions for handling various file and directory operations

[Abhijeet Kasurde]

Hi All,

Please find the patch for pytest-multihost-plugin.

Fixes : https://fedorahosted.org/python-pytest-multihost/ticket/2

Thanks,
Abhijeet Kasurde
From 72dfedf298ed6e27cc10f7c63fa1202a0942c88e Mon Sep 17 00:00:00 2001
From: Abhijeet Kasurde 
Date: Wed, 25 Nov 2015 14:30:35 +0530
Subject: [PATCH] Added functions for handling various file operations

Functions are added for removing directory, removing file,
renaming file.

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

Signed-off-by: Abhijeet Kasurde 
---
 pytest_multihost/transport.py | 46 ++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/pytest_multihost/transport.py b/pytest_multihost/transport.py
index 2b1ccbc32e8ed4b28e263d9bbe2df4bc2c8617de..f85099fd856e461ca68a342050fae61508c34d10 100644
--- a/pytest_multihost/transport.py
+++ b/pytest_multihost/transport.py
@@ -1,5 +1,5 @@
 #
-# Copyright (C) 2013  Red Hat
+# Copyright (C) 2015  Red Hat
 # Copyright (C) 2014  pytest-multihost contributors
 # See COPYING for license
 #
@@ -98,6 +98,18 @@ class Transport(object):
 self._command_index += 1
 return '%s.cmd%s' % (self.host.logger_name, self._command_index)
 
+def rmdir(self, path):
+"""Remove directory"""
+raise NotImplementedError('Transport.rmdir')
+
+def rename_file(self, oldpath, newpath):
+"""Rename file"""
+raise NotImplementedError('Transport.rename_file')
+
+def remove_file(self, filepath):
+"""Removes files"""
+raise NotImplementedError('Transport.remove_file')
+
 
 class Command(object):
 """A Popen-style object representing a remote command
@@ -246,6 +258,18 @@ class ParamikoTransport(Transport):
 self.log.info('PUT %s', remotepath)
 self.sftp.put(localpath, remotepath)
 
+def rmdir(self, path):
+self.log.info('RMDIR %s', path)
+self.sftp.rmdir(path)
+
+def remove_file(self, filepath):
+self.log.info('REMOVE FILE %s', filepath)
+self.sftp.remove(filepath)
+
+def rename_file(self, oldpath, newpath):
+self.log.info('RENAME %s to %s', oldpath, newpath)
+self.sftp.rename(oldpath, newpath)
+
 
 class OpenSSHTransport(Transport):
 """Transport that uses the `ssh` binary"""
@@ -341,6 +365,26 @@ class OpenSSHTransport(Transport):
 else:
 raise IOError('File %r could not be read' % filename)
 
+def rmdir(self, path):
+self.log.info('RMDIR %s', path)
+cmd = self._run(['rmdir', path])
+cmd.wait()
+
+def remove_file(self, filepath):
+self.log.info('REMOVE FILE %s', filepath)
+cmd = self._run(['rm', filepath])
+cmd.wait()
+if cmd.returncode != 0:
+raise IOError('File %r could not be deleted' % filepath)
+
+def rename_file(self, oldpath, newpath):
+self.log.info('RENAME %s TO %s', oldpath, newpath)
+cmd = self._run(['mv', oldpath, newpath])
+cmd.wait()
+if cmd.returncode != 0:
+raise IOError('File %r could not be renamed to %r '
+  % (oldpath, newpath))
+
 
 class SSHCallWrapper(object):
 """Adapts a /usr/bin/ssh call to the paramiko.Channel interface
-- 
2.4.3

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

Re: [Freeipa-devel] [PATCH 0104] do not disconnect when using existing connection to check default CA ACLs

[Jan Cholasta]

On 25.11.2015 09:28, Martin Babinsky wrote:

On 11/25/2015 07:21 AM, Jan Cholasta wrote:

On 25.11.2015 05:56, Fraser Tweedale wrote:

On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote:

On 24.11.2015 17:17, Martin Babinsky wrote:

On 11/24/2015 05:10 PM, Martin Babinsky wrote:

On 11/24/2015 05:01 PM, Martin Babinsky wrote:

On 11/24/2015 04:58 PM, Jan Cholasta wrote:

On 24.11.2015 16:48, Martin Babinsky wrote:

On 11/24/2015 04:44 PM, Martin Babinsky wrote:

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


forgot to attach the actual file *slaps himself*


ipaserver/install/cainstance.py:1849: [E1101(no-member),
ensure_default_caacl] Instance of 'API' has no 'Backed' member)



Fixed




Also attaching rebased patch for ipa-4-2




Ignore the rebased patch, the original 104.1 applies on 4-2 just fine.


Thanks, ACK.

Pushed to:
master: ed830af693c596b286b30959eb3166b59cc030c6
ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a


Thanks for anaysing and fixing these connection issues which were
presumably introduced by my patches.  I did not hit these in my
testing.  Admittedly I was focused on the ipa-4-2 branch and my
testing was mainly done there.

Brittle LDAP connection logic in install and upgrade is an ongoing
problem.  What shall we do to improve the situation?  Push
connection details into the Backend and let it connect if/when
needed, rather than managing connection state from the outside?

I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491


I don't think we need to be smart about it, everyone just needs to make
sure that when an ad-hoc connection is opened somewhere, it is also
closed in the same place. The same applies for any other resource.



Thanks,
Fraser






Maybe it would be a good idea to implement some decorator/context
manager to connect/disconnect from specified connection and wrap the
CRUD code. Example usage:

"""
decorator:
@connected(api.Backend.ldap2, ldapi=True)
def do_magic(api, *args, **kwargs):
# do stuff

context manager:

with connected(api.Backend.ldap2, ccache=example_ccache):
 do_some_other_magic()
"""

I remember Jan having some objections against this so it would be nice
to hear why this is not a good idea.


a) For ad-hoc connection objects, LDAPClient should be used instead of 
ldap2. It already supports the context manager protocol:


with LDAPClient(uri) as conn:
conn.gssapi_bind()
...

b) The api.Backend.ldap2 object should be connected and disconnected 
exactly once per api object per command. This is not done ATM and the 
usual workaround is to connect it manually, which has lead to the mess 
we have now.


Also, only GSSAPI authentication with valid IPA user credentials should 
be used with ldap2, even for ldapi connections.


--
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 0104] do not disconnect when using existing connection to check default CA ACLs

[Martin Babinsky]

On 11/25/2015 07:21 AM, Jan Cholasta wrote:

On 25.11.2015 05:56, Fraser Tweedale wrote:

On Tue, Nov 24, 2015 at 05:38:45PM +0100, Jan Cholasta wrote:

On 24.11.2015 17:17, Martin Babinsky wrote:

On 11/24/2015 05:10 PM, Martin Babinsky wrote:

On 11/24/2015 05:01 PM, Martin Babinsky wrote:

On 11/24/2015 04:58 PM, Jan Cholasta wrote:

On 24.11.2015 16:48, Martin Babinsky wrote:

On 11/24/2015 04:44 PM, Martin Babinsky wrote:

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


forgot to attach the actual file *slaps himself*


ipaserver/install/cainstance.py:1849: [E1101(no-member),
ensure_default_caacl] Instance of 'API' has no 'Backed' member)



Fixed




Also attaching rebased patch for ipa-4-2




Ignore the rebased patch, the original 104.1 applies on 4-2 just fine.


Thanks, ACK.

Pushed to:
master: ed830af693c596b286b30959eb3166b59cc030c6
ipa-4-2: c5faaede276f3052517ddf86e64cb228e95dca2a


Thanks for anaysing and fixing these connection issues which were
presumably introduced by my patches.  I did not hit these in my
testing.  Admittedly I was focused on the ipa-4-2 branch and my
testing was mainly done there.

Brittle LDAP connection logic in install and upgrade is an ongoing
problem.  What shall we do to improve the situation?  Push
connection details into the Backend and let it connect if/when
needed, rather than managing connection state from the outside?

I filed a ticket: https://fedorahosted.org/freeipa/ticket/5491


I don't think we need to be smart about it, everyone just needs to make
sure that when an ad-hoc connection is opened somewhere, it is also
closed in the same place. The same applies for any other resource.



Thanks,
Fraser






Maybe it would be a good idea to implement some decorator/context 
manager to connect/disconnect from specified connection and wrap the 
CRUD code. Example usage:


"""
decorator:
@connected(api.Backend.ldap2, ldapi=True)
def do_magic(api, *args, **kwargs):
   # do stuff

context manager:

with connected(api.Backend.ldap2, ccache=example_ccache):
do_some_other_magic()
"""

I remember Jan having some objections against this so it would be nice 
to hear why this is not a good idea.


--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCH 507] install: drop support for Dogtag 9

[Jan Cholasta]

On 25.11.2015 09:06, Jan Cholasta wrote:

On 24.11.2015 14:58, David Kupka wrote:

On 10/11/15 09:52, Jan Cholasta wrote:

On 10.11.2015 09:28, Jan Cholasta wrote:

Hi,

the attached patch fixes
.

Honza


Actually working patch attached.




Hi, thanks for the patch. It works for me but needs trivial rebase. ACK
when rebased.


Thanks. Rebased patch attached in the previous message.

Pushed to master: aeffe2da42734655cbaedb2c4d4f9e28bd2df1c0

--
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 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Jan Cholasta]

On 23.11.2015 15:19, Rob Crittenden wrote:

Tomas Babej wrote:



On 11/23/2015 01:50 PM, Jan Cholasta wrote:

On 23.11.2015 13:40, Tomas Babej wrote:



On 11/23/2015 01:31 PM, Jan Cholasta wrote:

On 23.11.2015 13:28, Tomas Babej wrote:



On 11/23/2015 01:11 PM, Jan Cholasta wrote:

On 23.11.2015 12:53, Tomas Babej wrote:

Hi,

If the code within the private_ccache contextmanager does not
set/removes the KRB5CCNAME, the pop method will raise KeyError, which
will cause unnecessary termination of the code flow.

Make sure the KRB5CCNAME is popped out of os.environ only if present.

Tomas


NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.


Yeah, well, both ways are equivalent, I found the if statement more
explicit. We can go with the suggested version, if it's more pleasing
though - patch is attached.



Also I don't think the comment is necessary, it's quite obvious
what the
code does.



I don't think an explanatory comment can hurt, ever. Worst thing that
happens is that somebody is assured that he understands the code
correctly.


Actually the worst thing is that someone changes the code without
changing the comment. If the comment does not add any real value, it's
only a maintenance burden.



Yep, that's a real issue in our code base (i.e. I recently removed such
a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
sure the comments describe the implementation properly is on the
author/reviewer though.

What's "added value" highly depends on your skill set, and familiarity
with the code base. Particularly the familiarity with the code base can
diminish over time even for the author, and those are the times where
comments can come to the rescue.


Let's take a look at the code:

 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
 os.environ.pop('KRB5CCNAME', None)

Can you tell me what in there couldn't be obvious to a person with even
the most basic skill set?

IMHO a docstring for private_cccache would add some real value, but this
comment alone does not.



For a newcomer to the project, even a trivial comment (from the point of
view of the experienced developer) can be valuable.


Following this logic, there should be a comment for every line of our
code, which is ridiculous.



Here's the version of the patch with the comment removed.


IMHO the comment should have been something like "ensure no KRB5CCNAME
otherwise it blows up in ..." If it took you 20 minutes to track down a
one-line change then a comment might save the next guy who changes the
behavior.


As I wrote earlier, I think it would make more sense to put this into 
the docstring of private_ccache().


ACK on the patch.

--
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 0385] replicainstall: Add possiblity to install client in one

[Jan Cholasta]

On 24.11.2015 15:56, Tomas Babej wrote:



On 11/23/2015 04:43 PM, Jan Cholasta wrote:

Hi,

On 23.11.2015 12:50, Tomas Babej wrote:

Hi,

this patch implements the single command replica promotion&enrollment
for #5310.

Tomas

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


1) ensure_enrolled() should be called from promote_check() after the
client check is done:

 client_fstore = sysrestore.FileStore(paths.IPA_CLIENT_SYSRESTORE)
 if not client_fstore.has_files():
 ensure_enrolled(installer)


Also it should no longer be decorated with @common_cleanup.




2)

+server_name = Knob(
+str, None,
+description="fully qualified name of IPA server to enrooll to",
+cli_name='server',
+)

Please use the same identifier ipa-client-install uses, i.e.
s/server_name/server/.

Also there is typo in the description: "enrooll".


Fixed.


You don't need to set cli_name anymore, as it's the same as the default.






3)

+host_name = Knob(
+str, None,
+description="fully qualified name of this host",
+cli_name='hostname',
+)

This knob is already defined in BaseServer, there's no need to redefine
it here (just remove the "host_name = None").

If you want to change the description, change it in BaseServer.


Yep, that was the reasoning here. Changed in BaseServer.




4)

+keytab = Knob(
+str, None,
+description="path to backed up keytab from previous enrollment",
+cli_name='keytab',
+)

ipa-client-install uses the short name -k for the keytab option, it
should be used here as well.



Added.


You don't need to set cli_name here as well.





5) The replica file argument conflicts with the --realm, --domain,
--server, --admin-password and --principal options. This should be
checked in Replica.__init__().

The --hostname option should either be conflicting as well or be
implemented properly for legacy replica install.



All of them now conflict --replica-file.


Replica file is not an option but rather an argument in ipa-replica-install.

I think the error message should use the same wording which is used for 
--forwarder vs. --no-forwarder and --reverse-zone vs. --no-reverse: "You 
cannot specify a --something option together with replica file".






6) I think --admin-password should be renamed to --password and the
description changed, since it now also allows OTP enrollment.



I changed the requirements to mandate --principal when --password is
used, as we discussed.


I was wrong here, sorry.

ipa-replica-install assumes "admin" for principal by default, so we just 
need to make sure ipa-client-install is called with --principal when 
password is used, whether it's the provided principal or the default 
"admin".




The name of the option cannot really be changed, as it is already taken
by the dm_password.


Right.

--
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