Re: [Freeipa-devel] [PATCH 0150] replica-install: Fix --domain

[Jan Cholasta]

On 28.7.2016 16:53, Petr Spacek wrote:

On 28.7.2016 16:39, Jan Cholasta wrote:

On 25.7.2016 16:36, Petr Spacek wrote:

On 25.7.2016 16:16, Jan Cholasta wrote:

On 25.7.2016 15:55, Petr Spacek wrote:

Hello,

replica-install: Fix --domain

Replica installation must not check existence of --domain - the domain
must (logically) exist.

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


Note that Server.domain_name is already defined on line 1204 in
ipaserver/install/server/install.py.


My bad, here is updated patch.


Please use the original domain_name definition in Server, the order of knobs
in the class matters.


Ah, the order in class is re-used for ordering in --help.

Given that --domain option depends on setup_dns which is defined before, I had
to move realm option down so realm & domain are next to each other.


Right. Works for me, ACK.

Pushed to master: 6eb9eb730350937692a442b66e4d79d3ebd4eb01

--
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] 0096 caacl: fix regression in rule instantiation

[Fraser Tweedale]

On Thu, Jul 28, 2016 at 09:56:30AM +0200, Martin Babinsky wrote:
> On 07/28/2016 03:31 AM, Fraser Tweedale wrote:
> > The attached patch fixes a kerberos.Principal-related regression.
> > 
> > Thanks,
> > Fraser
> > 
> Hi Fraser,
> 
> The ticket you linked in the commit message points to a closed milestone.
> You have to open a new ticket which needs to be triaged. Sorry, those are
> the processes.
> 
Filed ticket: https://fedorahosted.org/freeipa/ticket/6146
Updated patch attached (rebase and update commit message only).

Thanks,
Fraser
From ef74c727e31a08af679eeeca027dd6a6bf526f0e Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Thu, 28 Jul 2016 10:55:45 +1000
Subject: [PATCH] caacl: fix regression in rule instantiation

The Principal refactor causes service collections
('memberservice_service' attribute) to return Principal objects
where previously it returned strings, but the HBAC machinery used
for CA ACL enforcement only handles strings.  Update the code to
stringify service Principal objects when adding them to HBAC rules.

Fixes: https://fedorahosted.org/freeipa/ticket/6146
---
 ipaserver/plugins/caacl.py | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ipaserver/plugins/caacl.py b/ipaserver/plugins/caacl.py
index 
d316cc7c48cf2997d6be6b052dc1efa6d6fcdb6a..a7817c4cf64f070c74557f52e9f26c9013a4963c
 100644
--- a/ipaserver/plugins/caacl.py
+++ b/ipaserver/plugins/caacl.py
@@ -132,16 +132,21 @@ def _acl_make_rule(principal_type, obj):
 rule.services.names = obj.get(attr, [])
 
 # add principals and principal's groups
-m = {'user': 'group', 'host': 'hostgroup', 'service': None}
 category_attr = '{}category'.format(principal_type)
 if category_attr in obj and obj[category_attr][0].lower() == 'all':
 rule.users.category = {pyhbac.HBAC_CATEGORY_ALL}
 else:
-principal_attr = 'member{}_{}'.format(principal_type, principal_type)
-rule.users.names = obj.get(principal_attr, [])
-if m[principal_type] is not None:
-group_attr = 'member{}_{}'.format(principal_type, 
m[principal_type])
-rule.users.groups = obj.get(group_attr, [])
+if principal_type == 'user':
+rule.users.names = obj.get('memberuser_user', [])
+rule.users.groups = obj.get('memberuser_group', [])
+elif principal_type == 'host':
+rule.users.names = obj.get('memberhost_host', [])
+rule.users.groups = obj.get('memberhost_hostgroup', [])
+elif principal_type == 'service':
+rule.users.names = [
+unicode(principal)
+for principal in obj.get('memberservice_service', [])
+]
 
 return rule
 
-- 
2.5.5

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

Re: [Freeipa-devel] [PATCH] 0001 six.u function instead of the decode

[Ariel Barria]

2016-07-28 7:10 GMT-05:00 Petr Spacek :
> On 27.7.2016 18:26, Ariel Barria wrote:
>> 2016-07-26 9:39 GMT-05:00 Petr Spacek :
>>> On 26.7.2016 16:28, Jan Cholasta wrote:
 Hi,

 On 26.7.2016 16:09, Martin Basti wrote:
>
>
> On 22.07.2016 00:14, Ariel Barria wrote:
>> Hello everyone.
>>
>> I send patch for review.

 NACK, six.u() is supposed to be used on string literals *only* [1].

 A proper fix would be something like:

 value = self.to_text()
 if not isinstance(value, unicode):
 value = value.decode('ascii')
 return value
>>
>> thanks for the guidance and comments
>>
>>>
>>> Most importantly, we should fix/provide this method in python-dns and 
>>> inherit
>>> this method from there.
>>
>> Well, I made a pr, but apparently travis ci test failed with other
>> versions of python, so it is possible that they do not approve, I will
>> be performing other tests to see what the downside.
>>
>> https://github.com/rthalley/dnspython/pull/195
>
> Looking at the PR, there are functions
> dns.name.to_text()
> dns.name.to_unicode()
>
> What is missing in them?
>
> Petr^2 Spacek
>
>
> I will look on this, for some reason we received your e-mail just today
> (2016-07-26)
>>
>> welcome.
>> it was my mistake, I sent the patch to the list before being signed to the 
>> list
>>

 Honza

 [1] 
> --
> Petr^2 Spacek

Hi.
I send the requested changes
thanks for review.
From 4da94aee34d56b7baf0888ca5ef83aaecbc9544b Mon Sep 17 00:00:00 2001
From: "Ariel O. Barria" 
Date: Thu, 21 Jul 2016 17:06:05 -0500
Subject: [PATCH] freeipa arielb 0001-0002 six.u function instead of the decode
 function

to avoid DNSName.ToASCII broken with python3
replace the decode () function to use the inherited
functions python-dns:
 - from_unicode()
 - to_unicode()
 - to_text()

https://fedorahosted.org/freeipa/ticket/5935
---
 ipapython/dnsutil.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ipapython/dnsutil.py b/ipapython/dnsutil.py
index 16549c8f6a4a0782191526856e918d7ac6b94dcc..effdc0a8d198c6a30766fddb7a1865658913b176 100644
--- a/ipapython/dnsutil.py
+++ b/ipapython/dnsutil.py
@@ -71,7 +71,11 @@ class DNSName(dns.name.Name):
 
 def ToASCII(self):
 #method named by RFC 3490 and python standard library
-return self.to_text().decode('ascii')  # must be unicode string
+value = self.to_text()
+if not isinstance(value, unicode):
+n = dns.name.from_unicode(self.to_unicode())
+value = n.to_text(True)
+return value  # must be unicode string
 
 def canonicalize(self):
 return DNSName(super(DNSName, self).canonicalize())
-- 
2.7.4

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

Re: [Freeipa-devel] [PATCH 42-44, 48-51][tests] RFE: Allow users to authenticate with alternative names

[Martin Babinsky]

On 07/28/2016 01:44 PM, Milan Kubík wrote:

On 07/28/2016 12:51 PM, Martin Babinsky wrote:

On 07/27/2016 11:54 AM, Milan Kubík wrote:







Hi Milan,

the tests seem to work as expected except
`test_enterprise_principal_UPN_overlap_without_additional_suffix`
which crashes on #6099. I have a few comments, however:

This is a test that hits a known bug. I have added an expected fail
marker for it.


Patch 42:

1.)
+class KerberosAliasMixin:

make sure the class inherits from 'object' so that it behaves like
new-style class in Python 2.

Also, I think that 'check_for_tracker' method is slightly redundant.
If somebody would use the mixin directly, then he will still get
"NotImplementedError" exceptions and see that he probably did
something wrong.

If you really really want to treat to prevent the instantiation of the
class, then use ABC module to turn it into proper abstract class.

But in this case it should IMHO be enough that you explained the class
usage in the module docstring.

Ok. Fixed the inheritance and removed the check for Tracker. The check
for krbprincipalname attribute has been kept.


2.)
+def _make_add_alias_cmd(self):
+raise RuntimeError("The _make_add_alias_cmd method "
+   "is not implemented.")
+
+def _make_remove_alias_cmd(self):
+raise RuntimeError("The _make_remove_alias_cmd method "
+   "is not implemented."

Abstract methods should raise "NotImplementedError" exception.


Fixed.

3.)
is the 'options=None' kwarg in {add,remove}_principals methods
necessary? I didn't see it anywhere in the later commits so I guess
you can safely remove it. Better yet, you can just replace it with
'**options' since all it does is to pass options to the
`*-add/remove-principal` commands.


Fixed to **options.

4.)
a nitpick but IIRC the mixin class should be listed before the actual
hierarchy base so that MRO works as expected.

So instead

+class UserTracker(Tracker, KerberosAliasMixin):

It should say
+class UserTracker(KerberosAliasMixin, Tracker):

Fixed in all three classes.


PATCH 43: LGTM

PATCH 44:

Please do not create any more utility modules. Either put it to
existing 'ipatests/util.py' or better yet, rename the module to
something like 'mock_trust' since the scope of it is to provide mocked
trust entries.

Moved the functions from test_range_plugin.py module to a new mock_trust
module. The fix for the range plugin test introduced a new commit here.


PATCH 45:

It would be nice if you could add '-E' option in the same way to
indicate enterprise principal name resolution.

Done.


PATCH 46: LGTM
PATCH 47:

1.)
+from ipatests.test_xmlrpc.test_range_plugin import (
+get_trust_dn, get_trusted_dom_dict, encode_mockldap_value)

Since you already introduced a module grouping all trust-mocking
machinery in patch 44, you could extract these functions and put it
there to improve code reuse.

Fixed.


2.)
I am not a big fan of duplicate code in 'trusted_domain' and
'trusted_domain_with_suffix' fixtures. Use some module-level mapping
for common attributes and add more specific attributes per fixture.

Fixed


3.)
I would like to expand the test cases for AD realm namespace overlap
checks. We should reject enterprise principals with suffixes
coinciding with realm names, NETBIOS names, and UPN suffixes and I
would like to test all three cases to get a full coverage.


Extended with explicit checks fot rhe AD realm and NETBIOS name by
constructing the enterprise principal from corresponding LDAP
attributes.

The fixed and rebased version of the commits is in my repo [1].

[1]: https://github.com/apophys/freeipa/tree/krb5-principal-aliases-test

Regards



Tests works and code is ok, however you will need to create a separate
ticket to your patches, since it is not allowed to add fixes to
tickets in closed milestones. Sorry that I didn't notice it earlier.

cond-ACK if you create ticket and remove the xfail from
`test_enterprise_principal_overlap_with_AD_realm` test case as the fix
for #6099 was pushed to master.



Hi,

thanks for the review. The xfail marker was removed. The commit messages
now reffer to new ticket [1].
Since the changes during review introduced new commit in the sequence, I
have abandoned the patches 45 to 47 and renumbered them (with the new
one) from 48 onwards.

[1]: https://fedorahosted.org/freeipa/ticket/6142

Regards



Thanks, 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 0027][Tests] Fix failing automember tests in 4.3

[Ganna Kaihorodova]

Greetings! 

ACK

Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer


- Original Message -
From: "Lenka Doudova" 
To: "freeipa-devel" 
Sent: Wednesday, July 13, 2016 3:21:25 PM
Subject: [Freeipa-devel] [PATCH 0027][Tests] Fix failing automember tests in
4.3

Hi,

providing patch to fix two failing automember tests in 4.3 branch. The 
reason of the failure was the output normalization (specifically manager 
attribute for user).

The patch is intended for ipa-4-3 branch only.


Lenka


-- 
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 0151-0152] install: Call hostnamectl set-hostname only if --hostname option is use server-install: Fix --hostname option to always override api.env value

[Petr Spacek]

On 28.7.2016 16:44, Jan Cholasta wrote:
> On 28.7.2016 16:37, Petr Spacek wrote:
>> On 28.7.2016 16:35, Jan Cholasta wrote:
>>> On 28.7.2016 16:20, Petr Spacek wrote:
 Hello,

 install: Call hostnamectl set-hostname only if --hostname option is used

 This commit also splits hostname backup and configuration into two separate
 functions. This allows us to backup hostname without setting it at the
 same time.

 https://fedorahosted.org/freeipa/ticket/6071
>>>
>>> Note that you can set ca_host in cfg unconditionally, the value is only 
>>> valid
>>> during install and is not written anywhere.
>>
>> I prefer not to set it so the code blows up when we attempt to touch 
>> variables
>> we should reference in particular setups. I.e. Take this as a first step
>> towards api.env without invalid values :-)
> 
> OK. Use the proper condition then ("if setup_ca:").

Oh, I'm probably blind. Here is revised version.

Petr^2 Spacek

> 
>>
>> (In my stash I have a patch which removes nonsense defaults from
>> ipalib.constants. To be pushed when we a new git branch for 4.4...)
>>
>> Petr^2 Spacek
>>
 server-install: Fix --hostname option to always override api.env values

 Attempts to compare local hostname with user-provided values are error
 prone as we found out in #5794. This patch removes comparison and makes
 the env values deterministic.

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


 Jan, this patch set should fix problems you have seen in containers.
From d61ab05a3d9d978146a50f66c1c757144c78b700 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 12 Jul 2016 17:42:40 +0200
Subject: [PATCH] server-install: Fix --hostname option to always override
 api.env values

Attempts to compare local hostname with user-provided values are error
prone as we found out in #5794. This patch removes comparison and makes
the env values deterministic.

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

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index c0c676b870b481696ae75742c7bf88074b0ecf9c..7e1c53b0575950302f90f9b7c9b8721df11f9b9e 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -559,7 +559,12 @@ def install_check(installer):
 cfg = dict(
 context='installer',
 in_server=True,
+# make sure host name specified by user is used instead of default
+host=host_name,
 )
+if not setup_ca:
+# we have an IPA-integrated CA
+cfg['ca_host'] = host_name
 
 # Create the management framework config file and finalize api
 target_fname = paths.IPA_DEFAULT_CONF
@@ -585,14 +590,6 @@ def install_check(installer):
 # Must be readable for everyone
 os.chmod(target_fname, 0o644)
 
-system_hostname = get_fqdn()
-if host_name != system_hostname:
-root_logger.debug("Chosen hostname (%s) differs from system hostname "
-  "(%s) - change it" % (host_name, system_hostname))
-# update `api.env.ca_host` to correct hostname
-# https://fedorahosted.org/freeipa/ticket/4936
-api.env.ca_host = host_name
-
 api.bootstrap(**cfg)
 api.finalize()
 
-- 
2.7.4

From acb5bb64bf23705a0d0efcf03d5f4015d9170f60 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Thu, 28 Jul 2016 16:13:55 +0200
Subject: [PATCH] install: Call hostnamectl set-hostname only if --hostname
 option is used

This commit also splits hostname backup and configuration into two separate
functions. This allows us to backup hostname without setting it at the
same time.

https://fedorahosted.org/freeipa/ticket/6071
---
 client/ipa-client-install   |  3 ++-
 doc/guide/guide.org | 10 +-
 ipaplatform/base/tasks.py   |  7 ++-
 ipaplatform/redhat/tasks.py | 13 ++---
 ipaserver/install/server/install.py | 10 +-
 5 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 3c323173cc6f0a02aba1595f78637bbaac5a71ef..a4f15736bb20d83b305c6c14e7a51c469cd0e337 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -2518,7 +2518,8 @@ def install(options, env, fstore, statestore):
 if options.hostname and not options.on_master:
 # skip this step when run by ipa-server-install as it always configures
 # hostname
-tasks.backup_and_replace_hostname(fstore, statestore, options.hostname)
+tasks.backup_hostname(fstore, statestore)
+tasks.set_hostname(options.hostname)
 
 ntp_srv_servers = []
 if not options.on_master and options.conf_ntp:
diff --git a/doc/guide/guide.org b/doc/guide/guide.org
index 6d181559f0af90e7be7089aa94ab4900fa4e90b5..2e852a964991781ef5dd7b93ac481891897e1ed0 

Re: [Freeipa-devel] [PATCH 0151-0152] install: Call hostnamectl set-hostname only if --hostname option is use server-install: Fix --hostname option to always override api.env value

[Jan Cholasta]

On 28.7.2016 16:37, Petr Spacek wrote:

On 28.7.2016 16:35, Jan Cholasta wrote:

On 28.7.2016 16:20, Petr Spacek wrote:

Hello,

install: Call hostnamectl set-hostname only if --hostname option is used

This commit also splits hostname backup and configuration into two separate
functions. This allows us to backup hostname without setting it at the
same time.

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


Note that you can set ca_host in cfg unconditionally, the value is only valid
during install and is not written anywhere.


I prefer not to set it so the code blows up when we attempt to touch variables
we should reference in particular setups. I.e. Take this as a first step
towards api.env without invalid values :-)


OK. Use the proper condition then ("if setup_ca:").



(In my stash I have a patch which removes nonsense defaults from
ipalib.constants. To be pushed when we a new git branch for 4.4...)

Petr^2 Spacek


server-install: Fix --hostname option to always override api.env values

Attempts to compare local hostname with user-provided values are error
prone as we found out in #5794. This patch removes comparison and makes
the env values deterministic.

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


Jan, this patch set should fix problems you have seen in containers.



--
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 0151-0152] install: Call hostnamectl set-hostname only if --hostname option is use server-install: Fix --hostname option to always override api.env value

[Petr Spacek]

On 28.7.2016 16:35, Jan Cholasta wrote:
> On 28.7.2016 16:20, Petr Spacek wrote:
>> Hello,
>>
>> install: Call hostnamectl set-hostname only if --hostname option is used
>>
>> This commit also splits hostname backup and configuration into two separate
>> functions. This allows us to backup hostname without setting it at the
>> same time.
>>
>> https://fedorahosted.org/freeipa/ticket/6071
> 
> Note that you can set ca_host in cfg unconditionally, the value is only valid
> during install and is not written anywhere.

I prefer not to set it so the code blows up when we attempt to touch variables
we should reference in particular setups. I.e. Take this as a first step
towards api.env without invalid values :-)

(In my stash I have a patch which removes nonsense defaults from
ipalib.constants. To be pushed when we a new git branch for 4.4...)

Petr^2 Spacek

>> server-install: Fix --hostname option to always override api.env values
>>
>> Attempts to compare local hostname with user-provided values are error
>> prone as we found out in #5794. This patch removes comparison and makes
>> the env values deterministic.
>>
>> https://fedorahosted.org/freeipa/ticket/6071
>>
>>
>> Jan, this patch set should fix problems you have seen in containers.
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
Petr^2 Spacek
-- 
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 0150] replica-install: Fix --domain

[Jan Cholasta]

On 25.7.2016 16:36, Petr Spacek wrote:

On 25.7.2016 16:16, Jan Cholasta wrote:

On 25.7.2016 15:55, Petr Spacek wrote:

Hello,

replica-install: Fix --domain

Replica installation must not check existence of --domain - the domain
must (logically) exist.

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


Note that Server.domain_name is already defined on line 1204 in
ipaserver/install/server/install.py.


My bad, here is updated patch.


Please use the original domain_name definition in Server, the order of 
knobs in the class matters.


--
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 0151-0152] install: Call hostnamectl set-hostname only if --hostname option is use server-install: Fix --hostname option to always override api.env value

[Petr Spacek]

Hello,

install: Call hostnamectl set-hostname only if --hostname option is used

This commit also splits hostname backup and configuration into two separate
functions. This allows us to backup hostname without setting it at the
same time.

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

server-install: Fix --hostname option to always override api.env values

Attempts to compare local hostname with user-provided values are error
prone as we found out in #5794. This patch removes comparison and makes
the env values deterministic.

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


Jan, this patch set should fix problems you have seen in containers.

-- 
Petr^2 Spacek
From 39a514f6818811d45e495da9bff7411df199a3fb Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 12 Jul 2016 17:42:40 +0200
Subject: [PATCH] server-install: Fix --hostname option to always override
 api.env values

Attempts to compare local hostname with user-provided values are error
prone as we found out in #5794. This patch removes comparison and makes
the env values deterministic.

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

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index c0c676b870b481696ae75742c7bf88074b0ecf9c..5d24a1358e7a5aeda1b23f92e7146efd6f629bbb 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -559,7 +559,12 @@ def install_check(installer):
 cfg = dict(
 context='installer',
 in_server=True,
+# make sure host name specified by user is used instead of default
+host=host_name,
 )
+if not (options.http_cert_files or options.dirsrv_cert_files):
+# we have an IPA-integrated CA
+cfg['ca_host'] = host_name
 
 # Create the management framework config file and finalize api
 target_fname = paths.IPA_DEFAULT_CONF
@@ -585,14 +590,6 @@ def install_check(installer):
 # Must be readable for everyone
 os.chmod(target_fname, 0o644)
 
-system_hostname = get_fqdn()
-if host_name != system_hostname:
-root_logger.debug("Chosen hostname (%s) differs from system hostname "
-  "(%s) - change it" % (host_name, system_hostname))
-# update `api.env.ca_host` to correct hostname
-# https://fedorahosted.org/freeipa/ticket/4936
-api.env.ca_host = host_name
-
 api.bootstrap(**cfg)
 api.finalize()
 
-- 
2.7.4

From fab9a97c62da4a6d08908e4e243af9a319a17e68 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Thu, 28 Jul 2016 16:13:55 +0200
Subject: [PATCH] install: Call hostnamectl set-hostname only if --hostname
 option is used

This commit also splits hostname backup and configuration into two separate
functions. This allows us to backup hostname without setting it at the
same time.

https://fedorahosted.org/freeipa/ticket/6071
---
 client/ipa-client-install   |  3 ++-
 doc/guide/guide.org | 10 +-
 ipaplatform/base/tasks.py   |  7 ++-
 ipaplatform/redhat/tasks.py | 13 ++---
 ipaserver/install/server/install.py | 10 +-
 5 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 3c323173cc6f0a02aba1595f78637bbaac5a71ef..a4f15736bb20d83b305c6c14e7a51c469cd0e337 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -2518,7 +2518,8 @@ def install(options, env, fstore, statestore):
 if options.hostname and not options.on_master:
 # skip this step when run by ipa-server-install as it always configures
 # hostname
-tasks.backup_and_replace_hostname(fstore, statestore, options.hostname)
+tasks.backup_hostname(fstore, statestore)
+tasks.set_hostname(options.hostname)
 
 ntp_srv_servers = []
 if not options.on_master and options.conf_ntp:
diff --git a/doc/guide/guide.org b/doc/guide/guide.org
index 6d181559f0af90e7be7089aa94ab4900fa4e90b5..2e852a964991781ef5dd7b93ac481891897e1ed0 100644
--- a/doc/guide/guide.org
+++ b/doc/guide/guide.org
@@ -1039,14 +1039,14 @@ def restore_context_default(filepath):
 # version in platform services
 restore_context = restore_context_default
 
-# Default implementation of backup and replace hostname that does nothing
-def backup_and_replace_hostname_default(fstore, statestore, hostname):
+# Default implementation of backup hostname that does nothing
+def backup_hostname_default(fstore, statestore):
 return
 
-# Backup and replace system's hostname
-# Since many platforms have their own way how to store system's hostname, this method must be
+# Backup system's hostname
+# Since many platforms have their own way of handling system's hostname, this method must be
 # implemented in platform services
-backup_and_replace_hostname = 

Re: [Freeipa-devel] [PATCH 680] compat: fix ping call

[Jan Cholasta]

On 28.7.2016 10:11, Jan Cholasta wrote:

Hi,

the attached patch fixes .


Pushed to master under the one-liner rule: 
b8b7b9bf8e8a23d652c99c335219abf9de1a6fb7




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] 0001 six.u function instead of the decode

[Petr Spacek]

On 27.7.2016 18:26, Ariel Barria wrote:
> 2016-07-26 9:39 GMT-05:00 Petr Spacek :
>> On 26.7.2016 16:28, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 26.7.2016 16:09, Martin Basti wrote:


 On 22.07.2016 00:14, Ariel Barria wrote:
> Hello everyone.
>
> I send patch for review.
>>>
>>> NACK, six.u() is supposed to be used on string literals *only* [1].
>>>
>>> A proper fix would be something like:
>>>
>>> value = self.to_text()
>>> if not isinstance(value, unicode):
>>> value = value.decode('ascii')
>>> return value
> 
> thanks for the guidance and comments
> 
>>
>> Most importantly, we should fix/provide this method in python-dns and inherit
>> this method from there.
> 
> Well, I made a pr, but apparently travis ci test failed with other
> versions of python, so it is possible that they do not approve, I will
> be performing other tests to see what the downside.
> 
> https://github.com/rthalley/dnspython/pull/195

Looking at the PR, there are functions
dns.name.to_text()
dns.name.to_unicode()

What is missing in them?

Petr^2 Spacek


 I will look on this, for some reason we received your e-mail just today
 (2016-07-26)
> 
> welcome.
> it was my mistake, I sent the patch to the list before being signed to the 
> list
> 
>>>
>>> Honza
>>>
>>> [1] 
-- 
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 42-44, 48-51][tests] RFE: Allow users to authenticate with alternative names

[Milan Kubík]

On 07/28/2016 12:51 PM, Martin Babinsky wrote:

On 07/27/2016 11:54 AM, Milan Kubík wrote:







Hi Milan,

the tests seem to work as expected except
`test_enterprise_principal_UPN_overlap_without_additional_suffix`
which crashes on #6099. I have a few comments, however:

This is a test that hits a known bug. I have added an expected fail
marker for it.


Patch 42:

1.)
+class KerberosAliasMixin:

make sure the class inherits from 'object' so that it behaves like
new-style class in Python 2.

Also, I think that 'check_for_tracker' method is slightly redundant.
If somebody would use the mixin directly, then he will still get
"NotImplementedError" exceptions and see that he probably did
something wrong.

If you really really want to treat to prevent the instantiation of the
class, then use ABC module to turn it into proper abstract class.

But in this case it should IMHO be enough that you explained the class
usage in the module docstring.

Ok. Fixed the inheritance and removed the check for Tracker. The check
for krbprincipalname attribute has been kept.


2.)
+def _make_add_alias_cmd(self):
+raise RuntimeError("The _make_add_alias_cmd method "
+   "is not implemented.")
+
+def _make_remove_alias_cmd(self):
+raise RuntimeError("The _make_remove_alias_cmd method "
+   "is not implemented."

Abstract methods should raise "NotImplementedError" exception.


Fixed.

3.)
is the 'options=None' kwarg in {add,remove}_principals methods
necessary? I didn't see it anywhere in the later commits so I guess
you can safely remove it. Better yet, you can just replace it with
'**options' since all it does is to pass options to the
`*-add/remove-principal` commands.


Fixed to **options.

4.)
a nitpick but IIRC the mixin class should be listed before the actual
hierarchy base so that MRO works as expected.

So instead

+class UserTracker(Tracker, KerberosAliasMixin):

It should say
+class UserTracker(KerberosAliasMixin, Tracker):

Fixed in all three classes.


PATCH 43: LGTM

PATCH 44:

Please do not create any more utility modules. Either put it to
existing 'ipatests/util.py' or better yet, rename the module to
something like 'mock_trust' since the scope of it is to provide mocked
trust entries.

Moved the functions from test_range_plugin.py module to a new mock_trust
module. The fix for the range plugin test introduced a new commit here.


PATCH 45:

It would be nice if you could add '-E' option in the same way to
indicate enterprise principal name resolution.

Done.


PATCH 46: LGTM
PATCH 47:

1.)
+from ipatests.test_xmlrpc.test_range_plugin import (
+get_trust_dn, get_trusted_dom_dict, encode_mockldap_value)

Since you already introduced a module grouping all trust-mocking
machinery in patch 44, you could extract these functions and put it
there to improve code reuse.

Fixed.


2.)
I am not a big fan of duplicate code in 'trusted_domain' and
'trusted_domain_with_suffix' fixtures. Use some module-level mapping
for common attributes and add more specific attributes per fixture.

Fixed


3.)
I would like to expand the test cases for AD realm namespace overlap
checks. We should reject enterprise principals with suffixes
coinciding with realm names, NETBIOS names, and UPN suffixes and I
would like to test all three cases to get a full coverage.


Extended with explicit checks fot rhe AD realm and NETBIOS name by
constructing the enterprise principal from corresponding LDAP 
attributes.


The fixed and rebased version of the commits is in my repo [1].

[1]: https://github.com/apophys/freeipa/tree/krb5-principal-aliases-test

Regards



Tests works and code is ok, however you will need to create a separate 
ticket to your patches, since it is not allowed to add fixes to 
tickets in closed milestones. Sorry that I didn't notice it earlier.


cond-ACK if you create ticket and remove the xfail from 
`test_enterprise_principal_overlap_with_AD_realm` test case as the fix 
for #6099 was pushed to master.




Hi,

thanks for the review. The xfail marker was removed. The commit messages 
now reffer to new ticket [1].
Since the changes during review introduced new commit in the sequence, I 
have abandoned the patches 45 to 47 and renumbered them (with the new 
one) from 48 onwards.


[1]: https://fedorahosted.org/freeipa/ticket/6142

Regards

--
Milan Kubik

From 0a56dc99aa1243e525e8c0fdd8ef217077d23ab1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= 
Date: Fri, 22 Jul 2016 17:07:56 +0200
Subject: [PATCH 1/7] ipatests: Add tracker class for kerberos principal
 aliases

The commit implements a mixin class providing capability
to track and modify kerberos principal aliases on supported
types of entries.

The class using the mixin must inherit from the Tracker class
and must provide the implementation of two methods:

* _make_add_alias_cmd
* _make_remove_alias_cmd

These are used to get the type specific command for the 

Re: [Freeipa-devel] [PATCH 0003] Test validity of URIs in certificate

[Lenka Doudova]

Hi,

I cannot find any attached patch :)

Lenka


On 07/28/2016 01:30 PM, Peter Lacko wrote:

Attached you can find a patch adding test for URIs in generated certificate
ipatests/test_xmlrpc/test_cert_plugin.py
Since I'm leaving Red Hat in end of July, I won't be able to modify this patch 
anymore.

Regards,

Peter



--
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 0003] Test validity of URIs in certificate

[Peter Lacko]

Attached you can find a patch adding test for URIs in generated certificate 
ipatests/test_xmlrpc/test_cert_plugin.py
Since I'm leaving Red Hat in end of July, I won't be able to modify this patch 
anymore.

Regards,

Peter

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

Re: [Freeipa-devel] [PATCH 0018] Minor fix in ipa-replica-manage MAN page

[Florence Blanc-Renaud]

On 07/27/2016 05:47 AM, Abhijeet Kasurde wrote:

Ping.


On 07/13/2016 09:24 AM, Abhijeet Kasurde wrote:


Hi All,

Please review patch.

Fixes: https://fedorahosted.org/freeipa/ticket/6058
--
Thanks,
Abhijeet Kasurde

IRC: akasurde
http://akasurde.github.io





--
Thanks,
Abhijeet Kasurde

IRC: akasurde
http://akasurde.github.io




Hi,

thanks for your patch! The man page now correctly reflects the output of 
ipa-replica-manage list [SERVER].

ACK

Flo.

--
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 42-47][tests] RFE: Allow users to authenticate with alternative names

[Martin Babinsky]

On 07/27/2016 11:54 AM, Milan Kubík wrote:







Hi Milan,

the tests seem to work as expected except
`test_enterprise_principal_UPN_overlap_without_additional_suffix`
which crashes on #6099. I have a few comments, however:

This is a test that hits a known bug. I have added an expected fail
marker for it.


Patch 42:

1.)
+class KerberosAliasMixin:

make sure the class inherits from 'object' so that it behaves like
new-style class in Python 2.

Also, I think that 'check_for_tracker' method is slightly redundant.
If somebody would use the mixin directly, then he will still get
"NotImplementedError" exceptions and see that he probably did
something wrong.

If you really really want to treat to prevent the instantiation of the
class, then use ABC module to turn it into proper abstract class.

But in this case it should IMHO be enough that you explained the class
usage in the module docstring.

Ok. Fixed the inheritance and removed the check for Tracker. The check
for krbprincipalname attribute has been kept.


2.)
+def _make_add_alias_cmd(self):
+raise RuntimeError("The _make_add_alias_cmd method "
+   "is not implemented.")
+
+def _make_remove_alias_cmd(self):
+raise RuntimeError("The _make_remove_alias_cmd method "
+   "is not implemented."

Abstract methods should raise "NotImplementedError" exception.


Fixed.

3.)
is the 'options=None' kwarg in {add,remove}_principals methods
necessary? I didn't see it anywhere in the later commits so I guess
you can safely remove it. Better yet, you can just replace it with
'**options' since all it does is to pass options to the
`*-add/remove-principal` commands.


Fixed to **options.

4.)
a nitpick but IIRC the mixin class should be listed before the actual
hierarchy base so that MRO works as expected.

So instead

+class UserTracker(Tracker, KerberosAliasMixin):

It should say
+class UserTracker(KerberosAliasMixin, Tracker):

Fixed in all three classes.


PATCH 43: LGTM

PATCH 44:

Please do not create any more utility modules. Either put it to
existing 'ipatests/util.py' or better yet, rename the module to
something like 'mock_trust' since the scope of it is to provide mocked
trust entries.

Moved the functions from test_range_plugin.py module to a new mock_trust
module. The fix for the range plugin test introduced a new commit here.


PATCH 45:

It would be nice if you could add '-E' option in the same way to
indicate enterprise principal name resolution.

Done.


PATCH 46: LGTM
PATCH 47:

1.)
+from ipatests.test_xmlrpc.test_range_plugin import (
+get_trust_dn, get_trusted_dom_dict, encode_mockldap_value)

Since you already introduced a module grouping all trust-mocking
machinery in patch 44, you could extract these functions and put it
there to improve code reuse.

Fixed.


2.)
I am not a big fan of duplicate code in 'trusted_domain' and
'trusted_domain_with_suffix' fixtures. Use some module-level mapping
for common attributes and add more specific attributes per fixture.

Fixed


3.)
I would like to expand the test cases for AD realm namespace overlap
checks. We should reject enterprise principals with suffixes
coinciding with realm names, NETBIOS names, and UPN suffixes and I
would like to test all three cases to get a full coverage.


Extended with explicit checks fot rhe AD realm and NETBIOS name by
constructing the enterprise principal from corresponding LDAP attributes.

The fixed and rebased version of the commits is in my repo [1].

[1]: https://github.com/apophys/freeipa/tree/krb5-principal-aliases-test

Regards



Tests works and code is ok, however you will need to create a separate 
ticket to your patches, since it is not allowed to add fixes to tickets 
in closed milestones. Sorry that I didn't notice it earlier.


cond-ACK if you create ticket and remove the xfail from 
`test_enterprise_principal_overlap_with_AD_realm` test case as the fix 
for #6099 was pushed to master.


--
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] [PATCH 0197] re-set canonical principal name on migrated users

[Martin Babinsky]

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

I have also noticed that the principal aliases are not preserved during 
migration from FreeIPA 4.4.


That, however, requires more powerful runes to transform the realm of 
all values and warrants a separate ticket if we even want to support 
migration of user aliases.


--
Martin^3 Babinsky
From 208ee38df29cf05436b6a1dd6c3556f70bbbd62d Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 28 Jul 2016 10:42:58 +0200
Subject: [PATCH] re-set canonical principal name on migrated users

The migration procedure has been updated to re-set `krbcanonicalname`
attribute on migrated users as well as `krbprincipalname` so that migration
from FreeIPA versions supporting principal aliases does not break subsequent
authentication of migrated users.

https://fedorahosted.org/freeipa/ticket/6101
---
 ipaserver/plugins/migration.py | 41 -
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/ipaserver/plugins/migration.py b/ipaserver/plugins/migration.py
index 7f634a7ccf8c49a4c8e0cc3fe2b2dce84b5cadff..404c4aeb08ff2ee018799af3a9224bec93c26f82 100644
--- a/ipaserver/plugins/migration.py
+++ b/ipaserver/plugins/migration.py
@@ -36,6 +36,7 @@ if api.env.in_server and api.env.context in ['lite', 'server']:
 from ipalib import _
 from ipapython.dn import DN
 from ipapython.ipautil import write_tmp_file
+from ipapython.kerberos import Principal
 import datetime
 from ipaplatform.paths import paths
 
@@ -152,6 +153,32 @@ _supported_scopes = {u'base': SCOPE_BASE, u'onelevel': SCOPE_ONELEVEL, u'subtree
 _default_scope = u'onelevel'
 
 
+def _create_kerberos_principals(ldap, pkey, entry_attrs, failed):
+"""
+Create 'krbprincipalname' and 'krbcanonicalname' attributes for incoming
+user entry or skip it if there already is a user with such principal name.
+The code does not search for `krbcanonicalname` since we assume that the
+canonical principal name is always contained among values of
+`krbprincipalname` attribute.Both `krbprincipalname` and `krbcanonicalname`
+are set to default value generated from uid and realm.
+
+Note: the migration does not currently preserve principal aliases
+"""
+principal = Principal((pkey,), realm=api.env.realm)
+try:
+ldap.find_entry_by_attr(
+'krbprincipalname', principal, 'krbprincipalaux', [''],
+DN(api.env.container_user, api.env.basedn)
+)
+except errors.NotFound:
+entry_attrs['krbprincipalname'] = principal
+entry_attrs['krbcanonicalname'] = principal
+except errors.LimitsExceeded:
+failed[pkey] = unicode(_krb_failed_msg % unicode(principal))
+else:
+failed[pkey] = unicode(_krb_err_msg % unicode(principal))
+
+
 def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs):
 assert isinstance(dn, DN)
 attr_blacklist = ['krbprincipalkey','memberofindirect','memberindirect']
@@ -217,19 +244,7 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
 except ValueError:  # object class not present
 pass
 
-# generate a principal name and check if it isn't already taken
-principal = u'%s@%s' % (pkey, api.env.realm)
-try:
-ldap.find_entry_by_attr(
-'krbprincipalname', principal, 'krbprincipalaux', [''],
-DN(api.env.container_user, api.env.basedn)
-)
-except errors.NotFound:
-entry_attrs['krbprincipalname'] = principal
-except errors.LimitsExceeded:
-failed[pkey] = unicode(_krb_failed_msg % principal)
-else:
-failed[pkey] = unicode(_krb_err_msg % principal)
+_create_kerberos_principals(ldap, pkey, entry_attrs, failed)
 
 # Fix any attributes with DN syntax that point to entries in the old
 # tree
-- 
2.7.4

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

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

[Martin Basti]

On 17.06.2016 13:56, Stanislav Laznicka wrote:

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

On 17.6.2016 12:12, Stanislav Laznicka wrote:

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

On 17.6.2016 09:24, Stanislav Laznicka wrote:

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

On 17.6.2016 08:43, Stanislav Laznicka wrote:

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

On 16.6.2016 17:33, Stanislav Laznicka wrote:

Hello,

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

other
exception, too...


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

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

I'm just thinking out loud.

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

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

etc.

Again, just wondering out loud.


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

if anything. However, thinking in advance may save us much trouble in
the future, of course.


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


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

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

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

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


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

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


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

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






Hello,

suprisingly, patch needs rebase :)

1)
Is the script error the right Exception?

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

3)
I really hate to print errors to STDOUT from modules and then just call 
exit(1) (duplicated evil), could you replace print('xxx') with raise 
AnException('xxx')


Martin

-- 
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] 0002 Add client install option to set ipa_backup_server

[Petr Spacek]

On 27.7.2016 20:03, Martin Basti wrote:
> 
> 
> On 26.07.2016 17:01, Ariel Barria wrote:
>> Hello everyone.
>>
>> I send patch for review.
>>
>> Regards,
>>
>>
> Hello, thank you for the patch, but I have a few comments:
> 
> 1)
> can you please use option --backup-server instead of --ipa-backup-server to be
> consistent with --server (as we don't have option --ipa-server)
> 
> 2)
> values passed by --server option are validated if it is IPA server or not,
> this should happen for backup server(s) too.
> 
> But looking to current ipa-client-install, it may be challenging to achieve
> this goal. I'm afraid that you might rather wait until we refactor the client
> code (next release hopefully). But in case you are brave enough, I can provide
> advises, but it will be hell.
> 
> 3)
> There is a question, if the backup server should be used also for krb5.conf or
> other configs where multiple servers can be specified. Probably not. But at
> least this should be mentioned in manpage that this option is used only for
> SSSD (probably there should be check to prevent using --backup-server together
> with --no-sssd option)

I would use backup_servers even in krb5.conf. Quick testing indicates that
krb5 lib respects order of KDCs in krb5.conf so simply list backup servers at
the end of the list.

That would remove mutual exclusivity of --no-sssd and --backup-server options.

Petr^2 Spacek

> 
> 4)
> 'man ipa-client-install' should be updated with the new option
> 
> 5)
> ipa_backup_server allows to specify multiple servers, so the new option should
> be multivalued (and then joined to coma separated list into SSSD config)
> 
> regards,
> Martin

-- 
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 0017] Added fix for correct IPA backup file name

[Martin Basti]

On 07.07.2016 15:06, Rob Crittenden wrote:

Abhijeet Kasurde wrote:

Hi Florence,


On 07/07/2016 03:30 PM, Florence Blanc-Renaud wrote:

On 07/07/2016 10:58 AM, Abhijeet Kasurde wrote:

Hi All,

Please review the patch.

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

--
Thanks,
Abhijeet Kasurde

IRC: akasurde
http://akasurde.github.io




Hi Abhijeet,

thanks for your patch. I have a comment though: if the filename is
modified in ipa-backup, then it should also be changed in ipa-restore,
to make sure that the backup can be restored. It may be a good idea to
define the file name as a constant and use this constant everywhere.


I will change ipa-restore as well.

As far as I can see, the tool ipa-restore checks that the backup
version and ipa-restore version are consistent, meaning that both
tools should use the same filename and that it will not break backward
compatibility, but other team members can confirm.


I will wait for other team members to comment on this.


ipa-restore will probably need to look for both the ipa-full.tar and 
ipa-full.tar.gz because the version check is optional.


rob



I'm curious if this even worth to fix this.

We are saying that after each upgrade users need to recreate backups 
(because with high probability old backup will stop working with new 
packages) but I'm sure that users are not doing that, so we have to 
check both in restore.


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 678-679] client: fix hiding of commands which lack server support

[Jan Cholasta]

On 28.7.2016 10:16, Florence Blanc-Renaud wrote:

On 07/25/2016 02:47 PM, Jan Cholasta wrote:

Hi,

the attached patches fix .

Honza




Hi,
Tested with freeipa server 4.1.4-4.fc22 and patched client, correctly
hides the commands.
Tested with server 4.4 + patched client, correctly shows the commands.

Ack,
Flo.


Thanks.

Pushed to master: f563d982f2331456feadb5f961b7883d28348211

--
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 0027][Tests] Fix failing automember tests in 4.3

[Martin Basti]

On 13.07.2016 15:21, Lenka Doudova wrote:

Hi,

providing patch to fix two failing automember tests in 4.3 branch. The 
reason of the failure was the output normalization (specifically 
manager attribute for user).


The patch is intended for ipa-4-3 branch only.


Lenka




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

Re: [Freeipa-devel] [PATCH] 0083: webui: remove full name column from user to user group adder dialog

[Martin Basti]

On 25.07.2016 11:17, Alexander Bokovoy wrote:

On Thu, 21 Jul 2016, Pavel Vomacka wrote:

Remove full name from adding user to user group dialog

As the 'cn' is not in the response of user-show there is empty column 
in adder dialog.

Therefore the column was removed.

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

ACK.


Pushed to master: ffea8218c7d6db3abe4667a4d05817249a0e83bd

--
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 678-679] client: fix hiding of commands which lack server support

[Florence Blanc-Renaud]

On 07/25/2016 02:47 PM, Jan Cholasta wrote:

Hi,

the attached patches fix .

Honza




Hi,
Tested with freeipa server 4.1.4-4.fc22 and patched client, correctly 
hides the commands.

Tested with server 4.4 + patched client, correctly shows the commands.

Ack,
Flo.

--
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 680] compat: fix ping call

[Jan Cholasta]

Hi,

the attached patch fixes .

Honza

--
Jan Cholasta
From 9e6fe0c5d86e624925db20c59c4092b1c54f4654 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 25 Jul 2016 15:58:20 +0200
Subject: [PATCH] compat: fix ping call

Copy & paste accident caused the ping command to be called with an unwanted
argument, which results in an exception.

Remove the argument to fix it.

https://fedorahosted.org/freeipa/ticket/6129
---
 ipaclient/remote_plugins/compat.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipaclient/remote_plugins/compat.py b/ipaclient/remote_plugins/compat.py
index 40521af..aef5718 100644
--- a/ipaclient/remote_plugins/compat.py
+++ b/ipaclient/remote_plugins/compat.py
@@ -39,7 +39,7 @@ def get_package(api, client):
 try:
 server_version = env['result']['api_version']
 except KeyError:
-ping = client.forward(u'ping', u'api_version', version=u'2.0')
+ping = client.forward(u'ping', version=u'2.0')
 try:
 match = re.search(u'API version (2\.[0-9]+)', ping['summary'])
 except KeyError:
-- 
2.7.4

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

Re: [Freeipa-devel] [PATCH] 0096 caacl: fix regression in rule instantiation

[Martin Babinsky]

On 07/28/2016 03:31 AM, Fraser Tweedale wrote:

The attached patch fixes a kerberos.Principal-related regression.

Thanks,
Fraser


Hi Fraser,

The ticket you linked in the commit message points to a closed 
milestone. You have to open a new ticket which needs to be triaged. 
Sorry, those are the processes.


--
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 0194] harden the check for trust namespace overlap in new principals

[Martin Babinsky]

On 07/27/2016 03:30 PM, David Kupka wrote:

On 26/07/16 13:18, Martin Babinsky wrote:

On 07/21/2016 12:56 PM, Martin Babinsky wrote:

'*-add-principal' would crash with error if the trusted domains did not
have any UPN suffixes or NETBIOS name associated with them. This patch
fixes that.

Big thanks to Milan who found and reported the issue during writing
tests for the feature.

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




Bump for review.



Works for me, ACK.



Pushed to master: da2305ddb99ab982c757ab723acc95cda3d2f025

--
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] 0078-82: webui tests: tests for new certificate widget

[Lenka Doudova]

On 07/20/2016 04:51 PM, Pavel Vomacka wrote:
Please review attached patches, which add tests for new certificate 
widget in WebUI.


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




Hi,
thanks for patches.
Functionally ok, but you have lots of PEP8 errors in patches 78, 80, 81 
and 82 -> NACK.
Also in patch 82, method test_arbitrary_certificate, comment says user 
needs to have "arbitrary_cert" configured, but the property in config 
file is correctly "arbitrary_cert_path", so it's a bit misleading.


Patch 79 is OK, ACK.

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