Re: [Freeipa-devel] [PATCH] 0059..0064 Lightweight sub-CAs

[Fraser Tweedale]

On Tue, Jun 07, 2016 at 03:42:22PM +1000, Fraser Tweedale wrote:
> On Wed, Jun 01, 2016 at 02:51:04PM +1000, Fraser Tweedale wrote:
> > Hi team,
> > 
> > This patchset implements the 'ca' plugin for creating and managing
> > lightweight sub-CAs, and updates the 'caacl' plugin and
> > 'cert-request' command to support multiple CAs.
> > 
> > A brief overview of the patches:
> > 
> > 0059
> >   'ca' plugin, associated schema changes and container objects,
> >   Dogtag REST API wrapper
> > 0060
> >   Add CA entry for the IPA CA on install/upgrade
> > 0061
> >   Update 'caacl' plugin with CA support (including enforcement)
> > 0062
> >   Update ra.request_certificate() to support specifying target CA
> > 0063
> >   Add '--ca' option to 'cert-request' command
> > 0064
> >   Add '--issuer' option to 'cert-find' command
> > 
> > These patches depend on other pending patches:
> > 
> > 0051, 0052, 0053, 0054, 0055, 0056
> > 
> > Signing key replication depends on unmerged Dogtag patches.  Builds
> > of Dogtag with the required patches, and of FreeIPA with all
> > completed sub-CAs work, should be available from my COPR soon:
> > https://copr.fedorainfracloud.org/coprs/ftweedal/freeipa/
> > 
> > Some parts of the design are not implemented in the current
> > patchset, including:
> > 
> > - local parent CA (ipaca object) references
> > - sub-CA certificate renewal
> > - 'cert-show' command '--ca=NAME' option
> > - certmonger support for specifying CA
> > - revocation of deleted CAs
> > 
> > I look forward to your reviews!
> > 
> > Thanks,
> > Fraser
> >
> Rebased and updated patches attached.
> 
> Substantive changes:
> 
> - add required attributes for issuer DN and subject DN
> - prevent rename of IPA CA
> - when adding IPA CA entry, contact Dogtag to learn authority id,
>   issuer DN and subject DN
> - add 'read_ca' method to Dogtag interface
> - tighten ACIs to prevent modification of ipacaid attribute
> 
Updated patch 0064-3; adds --issuer option to cert-show and --ca
option to cert-show and cert-find.
From 7367f8efb5e8d8f1edeaaa74b3f35b79fc5a9a46 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Tue, 10 May 2016 13:56:40 +1000
Subject: [PATCH] Add issuer options to 'cert-show' and 'cert-find'

Add options to cert-show and cert-find for specifying the issuer as
a DN, or a CA name.

Also add the issuer DN to the output of cert-find.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 API.txt |   8 +++-
 VERSION |   4 +-
 ipaserver/plugins/cert.py   | 106 +++-
 ipaserver/plugins/dogtag.py |   9 
 4 files changed, 93 insertions(+), 34 deletions(-)

diff --git a/API.txt b/API.txt
index 
571c646c7b973b3107e9abf9f60caa4ded297c2f..f6853991659bfcaaae12d68fe8b4b8edefd6855f
 100644
--- a/API.txt
+++ b/API.txt
@@ -730,11 +730,13 @@ output: Entry('result')
 output: Output('summary', type=[, ])
 output: PrimaryKey('value')
 command: cert_find
-args: 0,17,4
+args: 0,19,4
 option: Flag('all', autofill=True, cli_name='all', default=False)
+option: Str('ca?', autofill=False)
 option: Flag('exactly?', autofill=True, default=False)
 option: Str('issuedon_from?', autofill=False)
 option: Str('issuedon_to?', autofill=False)
+option: Str('issuer?', autofill=False)
 option: Int('max_serial_number?', autofill=False)
 option: Int('min_serial_number?', autofill=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
@@ -774,8 +776,10 @@ option: Int('revocation_reason', autofill=True, default=0)
 option: Str('version?')
 output: Output('result')
 command: cert_show
-args: 1,2,1
+args: 1,4,1
 arg: Str('serial_number')
+option: Str('ca?', autofill=False)
+option: Str('issuer?', autofill=False)
 option: Str('out?')
 option: Str('version?')
 output: Output('result')
diff --git a/VERSION b/VERSION
index 
6577c80904f99ab6b30217ddc82625ccda928df0..600ab8182332de68bc356100ae0e232c68e30d90
 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=179
-# Last change: ftweedal - add --ca option to cert-request
+IPA_API_VERSION_MINOR=180
+# Last change: ftweedal - add issuer options to cert-show and cert-find
diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 
f09947337efc3d5b5872937df4a82663dfb6eff9..ce7d042d4f872c9f8176e887fd2bbfe07c1fe1a9
 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -583,37 +583,17 @@ class cert_show(VirtualCommand):
 
 takes_args = _serial_number
 
-has_output_params = (
-Str('certificate',
-label=_('Certificate'),
-),
-Str('subject',
-label=_('Subject'),
-),
-Str('issuer',
-label=_('Issuer'),
-),
-Str('valid_not_before',
-label=_('Not Before'),

Re: [Freeipa-devel] [PATCH] 0201 Add support for an external trust to Active Directory domain

[Alexander Bokovoy]

On Tue, 07 Jun 2016, Alexander Bokovoy wrote:

> del attrs['ipanttrusttype']
> +if attributes:
> +del attrs['ipanttrustattributes']
> """
Updated patch is attached.

Another update, forgot one space in the allow_behavior().

I also spent some time and did pep8 fixes for dcerpc.py. I reduced
reported errors down to 22 from 260+. These 22 are for lines longer than
79 characters and I don't want to reduce them further because they are
smaller than 84 characters already.


--
/ Alexander Bokovoy
From e9e8b4af5bf834aaceb9a7835733bd3641d5f93c Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy 
Date: Mon, 6 Jun 2016 08:55:44 +0300
Subject: [PATCH 1/5] trusts: Add support for an external trust to Active 
 Directory domain

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743
---
 API.txt|  3 ++-
 ipaserver/dcerpc.py| 50 +++--
 ipaserver/plugins/trust.py | 61 +++---
 3 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/API.txt b/API.txt
index f170930..d5fbc27 100644
--- a/API.txt
+++ b/API.txt
@@ -5208,12 +5208,13 @@ arg: Str('cn', cli_name='name')
 option: Str('version?')
 output: Output('result')
 command: trust_add
-args: 1,14,3
+args: 1,15,3
 arg: Str('cn', cli_name='realm')
 option: Str('addattr*', cli_name='addattr')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Int('base_id?', cli_name='base_id')
 option: Bool('bidirectional?', cli_name='two_way', default=False)
+option: Bool('external?', cli_name='external', default=False)
 option: Int('range_size?', cli_name='range_size')
 option: StrEnum('range_type?', cli_name='range_type', 
values=[u'ipa-ad-trust-posix', u'ipa-ad-trust'])
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
diff --git a/ipaserver/dcerpc.py b/ipaserver/dcerpc.py
index 25ffbfa..5f56643 100644
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -77,6 +77,11 @@ and Samba4 python bindings.
 TRUST_ONEWAY= 1
 TRUST_BIDIRECTIONAL = 3
 
+# Trust join behavior
+# External trust -- allow creating trust to a non-root domain in the forest
+TRUST_JOIN_EXTERNAL = 1
+
+
 def is_sid_valid(sid):
 try:
 security.dom_sid(sid)
@@ -1037,7 +1042,7 @@ class TrustDomainInstance(object):
 # We can ignore the error here -- setting up name suffix routes 
may fail
 pass
 
-def establish_trust(self, another_domain, trustdom_secret, 
trust_type='bidirectional'):
+def establish_trust(self, another_domain, trustdom_secret, 
trust_type='bidirectional', trust_external=False):
 """
 Establishes trust between our and another domain
 Input: another_domain -- instance of TrustDomainInstance, initialized 
with #retrieve call
@@ -1060,6 +1065,8 @@ class TrustDomainInstance(object):
 info.trust_direction |= lsa.LSA_TRUST_DIRECTION_OUTBOUND
 info.trust_type = lsa.LSA_TRUST_TYPE_UPLEVEL
 info.trust_attributes = 0
+if trust_external:
+info.trust_attributes |= lsa.LSA_TRUST_ATTRIBUTE_NON_TRANSITIVE
 
 try:
 dname = lsa.String()
@@ -1096,14 +1103,17 @@ class TrustDomainInstance(object):
 # server as that one doesn't support AES encryption types
 pass
 
-try:
-info = self._pipe.QueryTrustedDomainInfo(trustdom_handle, 
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX)
-info.trust_attributes |= lsa.LSA_TRUST_ATTRIBUTE_FOREST_TRANSITIVE
-self._pipe.SetInformationTrustedDomain(trustdom_handle, 
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX, info)
-except RuntimeError as e:
-root_logger.error('unable to set trust to transitive: %s' % 
(str(e)))
+if not trust_external:
+try:
+info = self._pipe.QueryTrustedDomainInfo(trustdom_handle,
+ 
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX)
+info.trust_attributes |= 
lsa.LSA_TRUST_ATTRIBUTE_FOREST_TRANSITIVE
+self._pipe.SetInformationTrustedDomain(trustdom_handle,
+   
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX, info)
+except RuntimeError as e:
+root_logger.error('unable to set trust transitivity status: 
%s' % (str(e)))
 
-if self.info['is_pdc']:
+if self.info['is_pdc'] or trust_external:
 

Re: [Freeipa-devel] [PATCH] 0202 support UPNs for trusted domain users

[Alexander Bokovoy]

On Tue, 07 Jun 2016, Martin Babinsky wrote:

On 06/07/2016 06:38 PM, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, Martin Babinsky wrote:

On 06/06/2016 12:34 PM, Alexander Bokovoy wrote:

Hi,

Add support for additional user name principal suffixes from
trusted Active Directory forests. UPN suffixes are property
of the forest and as such are associated with the forest root
domain.

FreeIPA stores UPN suffixes as ipaNTAdditionalSuffixes multi-valued
attribute of ipaNTTrustedDomain object class.

In order to look up UPN suffixes, netr_DsRGetForestTrustInformation
LSA RPC call is used instead of netr_DsrEnumerateDomainTrusts.

For more details on UPN and naming in Active Directory see
https://technet.microsoft.com/en-us/library/cc739093%28v=ws.10%29.aspx

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





Hi Alexander,

a few comments:

1.)

there are some PEP8 violations in the patch. Not all of them need to
be fixed, though (line overhangs < 5 characters are OK by me).

"""
./ipaserver/dcerpc.py:1199:80: E501 line too long (80 > 79 characters)
./ipaserver/dcerpc.py:1200:80: E501 line too long (83 > 79 characters)
./ipaserver/dcerpc.py:1258:40: E203 whitespace before ':'
./ipaserver/dcerpc.py:1263:80: E501 line too long (81 > 79 characters)
./ipaserver/dcerpc.py:1271:80: E501 line too long (90 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/plugins/trust.py:490:9: E128 continuation line
under-indented for visual indent
./ipaserver/plugins/trust.py:490:80: E501 line too long (93 > 79
characters)
./ipaserver/plugins/trust.py:490:92: E202 whitespace before ']'
./ipaserver/plugins/trust.py:493:80: E501 line too long (83 > 79
characters)
./ipaserver/plugins/trust.py:1461:80: E501 line too long (80 > 79
characters)
./ipaserver/plugins/trust.py:1462:59: E202 whitespace before ']'
./ipaserver/plugins/trust.py:1544:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:1638:1: E302 expected 2 blank lines, found 1
"""

I've fixed trust.py part, the dcerpc.py fixes in 0201 should be enough
now -- breaking following line is not giving any reasonable benefit:

  res['ipantflatname'] =
unicode(t.forest_trust_data.netbios_domain_name.string)



Looking at the code, it would be IMHO more readable to directly append 
dict literals to the result like so:


"""
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -1269,18 +1269,20 @@ def fetch_domains(api, mydomain, trustdomain, 
creds=None, server=None):

tname = unicode(t.forest_trust_data.dns_domain_name.string)
if tname == trustdomain:
continue
-res = dict()
-res['cn'] = tname
-res['ipantflatname'] = 
unicode(t.forest_trust_data.netbios_domain_name.string)
-res['ipanttrusteddomainsid'] = 
unicode(t.forest_trust_data.domain_sid)

-result['domains'][tname] = res
+
+result['domains'][tname] = {
+'cn': tname,
+'ipantflatname': unicode(
+t.forest_trust_data.netbios_domain_name.string),
+'ipanttrusteddomainsid': unicode(
+t.forest_trust_data.domain_sid)
+}
elif t.type == lsa.LSA_FOREST_TRUST_TOP_LEVEL_NAME:
tname = unicode(t.forest_trust_data.string)
if tname == trustdomain:
continue
-res = dict()
-res['cn'] = tname
-result['suffixes'][tname] = res
+
+result['suffixes'][tname] = {'cn': tname}
return result
"""

Makes sense. Fixed.



Also there is a whitespace before colon here:
"""
+result = {'domains': {}, 'suffixes' : {}}
   ^
"""
Please fix that and I will be a happy engineer.

Fixed.




2.)

Should the ipaNTAdditionalSuffixes attribute be case insensitive? It
makes sense but I'm just asking so that we don't end changing the
schema later.

ipaNTAdditionalSuffixes is defined as

attributeTypes: ( 2.16.840.1.113730.3.8.11.75 NAME
'ipaNTAdditionalSuffixes' DESC 'Suffix for the user principal name
associated with the domain' EQUALITY caseIgnoreMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.15)

There should be no problem with case sensitivity already.

OK I presume that the UPN suffixes on AD are also case-insensitive so 
everything should be alright.

Yes, as everything realm-related on AD side, they are case-insensitive.

Updated patch attached.
--
/ Alexander Bokovoy
From d12f4a85392bcef8f2280f4fbbae79ed6c66f42f Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy 
Date: Mon, 6 Jun 2016 11:41:46 +0300
Subject: [PATCH 2/6] adtrust: support UPNs for trusted domain users

Add support for additional user name principal suffixes from
trusted Active Directory forests. UPN suffixes are property
of the forest and as such are associated with the forest root
domain.

FreeIPA stores UPN suffixes as ipaNTAdditionalSuffixes multi-valued
attribute of 

Re: [Freeipa-devel] [PATCH] 0034: webui: Authentication indicators

[Petr Vobornik]

On 06/06/2016 08:33 PM, Pavel Vomacka wrote:
> 
> 
> On 06/06/2016 07:03 PM, Petr Vobornik wrote:
>> On 06/06/2016 12:27 PM, Pavel Vomacka wrote:
>>>
>>> On 06/02/2016 06:22 PM, Petr Vobornik wrote:
 On 06/01/2016 10:41 AM, Pavel Vomacka wrote:
> On 05/27/2016 05:58 PM, Pavel Vomacka wrote:
>> On 05/27/2016 05:44 PM, Nathaniel McCallum wrote:
>>> On Fri, 2016-05-27 at 17:43 +0200, Pavel Vomacka wrote:
 On 05/12/2016 11:13 PM, Nathaniel McCallum wrote:
> On Wed, 2016-05-11 at 13:08 +0200, Pavel Vomacka wrote:
>> Hi,
>>
>> the patch adds webui part for authentication indicators.
>>
>> Ticket: https://fedorahosted.org/freeipa/ticket/5872
> The otp option displays as: OTP.
> The radius option displays as: Radius.
>
> However, both are acronyms. The capitalization should be
> consistent.
 I'm sorry for late answer. They are displayed this way: 'OTP' and
 'Radius'. So it should not require any change.
>>> That is incorrect. It should be: OTP and RADIUS.
>> I'm sorry, I didn't understand correctly. Fixed patch attached.
>>
>>
>>
> The last patch changes the 'Radius' to 'RADIUS' in whole WebUI.
>
 After some though, we should really support the arbitrary values
 also in
 Web UI.

 I'd reuse aci.attributes_widget - move the widget elsewhere, make it a
 general widget and a base class for the original aci.attributes_widget.

 The only difference should be populate method and some strings.

 The base widget can populate from options - same as normal checkboxes
 widget so that service page doesn't need to inherit from the base
 class.

>>> Ok, thank you for advice. The edited patch is attached.
>>
>> 1. The new base class should have different name, e.g.
>> custom_checkboxes_widget . It is not related to (aci) attributes.
>>
>> Then it can be registered as such widget and the attributes widget can
>> be registered in ACI plugin. It would then also remove the ugly usage of
>> both $type and $factory.
> Fixed.
>>
>> 2. Why not implement empty placeholder populate function in the base
>> class? Then
>>if (that.populate && typeof that.populate === 'function')
>> that.populate();
>>
>> could be changed into:
>>that.populate();
>>
>> Child class should simply override.
> I just did not like a empty method there. But it's true that it is more
> readable with empty method and the method is documented.
>>
>> 3. If I add a custom indicator to one server and then switch to another.
>> It will offer the custom option as well, but unchecked. The added
>> options are not cleared on page reset. I'm not sure if it should be
>> called a feature or a bug. It is remnant of the original implementation.
> From my point of view it is a feature. I.e. When I add custom
> authentication indicator to one service there is a big chance that I
> would like to add it to another one. And in this case the custom auth.
> ind. is already present.
> 
> Fixed patch attached.
> 
> -- 
> Pavel^3 Vomacka

I've fixed trailing whitespace error on line:

"* Additional options "

And also a typo("custom_checkobox_widget") in commit message is fixed.

ACK

master:
* afededacb92ce1903885b265c7adca87b634c21a Auth Indicators WebUI part
-- 
Petr Vobornik

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

Re: [Freeipa-devel] [PATCH] 0202 support UPNs for trusted domain users

[Martin Babinsky]

On 06/07/2016 06:38 PM, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, Martin Babinsky wrote:

On 06/06/2016 12:34 PM, Alexander Bokovoy wrote:

Hi,

Add support for additional user name principal suffixes from
trusted Active Directory forests. UPN suffixes are property
of the forest and as such are associated with the forest root
domain.

FreeIPA stores UPN suffixes as ipaNTAdditionalSuffixes multi-valued
attribute of ipaNTTrustedDomain object class.

In order to look up UPN suffixes, netr_DsRGetForestTrustInformation
LSA RPC call is used instead of netr_DsrEnumerateDomainTrusts.

For more details on UPN and naming in Active Directory see
https://technet.microsoft.com/en-us/library/cc739093%28v=ws.10%29.aspx

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





Hi Alexander,

a few comments:

1.)

there are some PEP8 violations in the patch. Not all of them need to
be fixed, though (line overhangs < 5 characters are OK by me).

"""
./ipaserver/dcerpc.py:1199:80: E501 line too long (80 > 79 characters)
./ipaserver/dcerpc.py:1200:80: E501 line too long (83 > 79 characters)
./ipaserver/dcerpc.py:1258:40: E203 whitespace before ':'
./ipaserver/dcerpc.py:1263:80: E501 line too long (81 > 79 characters)
./ipaserver/dcerpc.py:1271:80: E501 line too long (90 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/plugins/trust.py:490:9: E128 continuation line
under-indented for visual indent
./ipaserver/plugins/trust.py:490:80: E501 line too long (93 > 79
characters)
./ipaserver/plugins/trust.py:490:92: E202 whitespace before ']'
./ipaserver/plugins/trust.py:493:80: E501 line too long (83 > 79
characters)
./ipaserver/plugins/trust.py:1461:80: E501 line too long (80 > 79
characters)
./ipaserver/plugins/trust.py:1462:59: E202 whitespace before ']'
./ipaserver/plugins/trust.py:1544:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:1638:1: E302 expected 2 blank lines, found 1
"""

I've fixed trust.py part, the dcerpc.py fixes in 0201 should be enough
now -- breaking following line is not giving any reasonable benefit:

   res['ipantflatname'] =
unicode(t.forest_trust_data.netbios_domain_name.string)



Looking at the code, it would be IMHO more readable to directly append 
dict literals to the result like so:


"""
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -1269,18 +1269,20 @@ def fetch_domains(api, mydomain, trustdomain, 
creds=None, server=None):

 tname = unicode(t.forest_trust_data.dns_domain_name.string)
 if tname == trustdomain:
 continue
-res = dict()
-res['cn'] = tname
-res['ipantflatname'] = 
unicode(t.forest_trust_data.netbios_domain_name.string)
-res['ipanttrusteddomainsid'] = 
unicode(t.forest_trust_data.domain_sid)

-result['domains'][tname] = res
+
+result['domains'][tname] = {
+'cn': tname,
+'ipantflatname': unicode(
+t.forest_trust_data.netbios_domain_name.string),
+'ipanttrusteddomainsid': unicode(
+t.forest_trust_data.domain_sid)
+}
 elif t.type == lsa.LSA_FOREST_TRUST_TOP_LEVEL_NAME:
 tname = unicode(t.forest_trust_data.string)
 if tname == trustdomain:
 continue
-res = dict()
-res['cn'] = tname
-result['suffixes'][tname] = res
+
+result['suffixes'][tname] = {'cn': tname}
 return result
"""

Also there is a whitespace before colon here:
"""
+result = {'domains': {}, 'suffixes' : {}}
^
"""
Please fix that and I will be a happy engineer.


2.)

Should the ipaNTAdditionalSuffixes attribute be case insensitive? It
makes sense but I'm just asking so that we don't end changing the
schema later.

ipaNTAdditionalSuffixes is defined as

attributeTypes: ( 2.16.840.1.113730.3.8.11.75 NAME
'ipaNTAdditionalSuffixes' DESC 'Suffix for the user principal name
associated with the domain' EQUALITY caseIgnoreMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.15)

There should be no problem with case sensitivity already.

OK I presume that the UPN suffixes on AD are also case-insensitive so 
everything should be alright.



3.)

"""
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
return truncated
trust_dn = self.obj.get_dn(args[0], trust_type=u'ad')
trust_entry = ldap.get_entry(trust_dn)
+blacklist = trust_entry.get('ipantsidblacklistincoming')
for entry in entries:
-sid = entry['ipanttrusteddomainsid'][0]
-
-blacklist = trust_entry.get('ipantsidblacklistincoming')
-if blacklist is None:
+sid = entry.get('ipanttrusteddomainsid', [None])[0]
+if sid is None:
continue

if sid in blacklist:
   

Re: [Freeipa-devel] [PATCHES 0146-0152] Server Roles v2

[Pavel Vomacka]

On 06/07/2016 12:07 PM, Martin Babinsky wrote:

On 06/03/2016 05:25 PM, Martin Babinsky wrote:

I am sending rebased patches implementing
http://www.freeipa.org/page/V4/Server_Roles

I hope the patches work since I have had a lot of fun rebasing them on
top of thin client and DNS locations effort.

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





Sending updated patches according to Jan's interactive review.

Since the name of attributes returned by API commands and signature of 
`server-role-find` have changed, a small update in WebUI patches is 
required.




NACK, why did you remove sizelimit from server_role_find command's? Is 
it possible to return it back? It breaks WebUI.
-- 
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] 0201 Add support for an external trust to Active Directory domain

[Alexander Bokovoy]

On Tue, 07 Jun 2016, Martin Babinsky wrote:
Again, we only require contributors to follow PEP8 when adding new 
code/directly touching old one. Please note that there are more 
serious transgressions than a couple of long lines that should 
_definitely_ be fixed (indentation errors, whitespace around operators 
etc.).


We as a contributors to the project _all_ have to conform to the style 
guide[1] to at least keep some uniform style in the codebase we touch. 
I fail to see why your contributions should be exempt from the rules 
we all strive to adhere to.

Sure we do. It is futile to shuffle dcerpc.py into pep8 style, though.
Look into an example I provided to mbasti, for example:

   res['ipantflatname'] = 
unicode(t.forest_trust_data.netbios_domain_name.string)

This line is 90+ characters long and there are not that many methods to
split it without making thing ugly. I'm not against formatting changes
but they should be taken in the context of the code.


(also you can sandwich # pylint: disable/ # pylint: enable directives 
around the offending long live to keep pep8 satisfied, just saying)


[1] http://www.freeipa.org/page/Python_Coding_Style



2.)

I have difficulty understanding the following code:

"""
+self.__allow_behavior = 0

   domain_validator = DomainValidator(api)
   self.configured = domain_validator.is_configured()

   if self.configured:
   self.local_flatname = domain_validator.flatname
   self.local_dn = domain_validator.dn
   self.__populate_local_domain()

+def allow_behavior(self, behavior_set):
+if type(behavior_set) is int:
+   behavior_set = [behavior_set]
+if TRUST_JOIN_EXTERNAL in behavior_set:
+   self.__allow_behavior |= TRUST_JOIN_EXTERNAL
+
"""
I assume this is made like this to accommodate setting some other
behavior flags which can pop up in the future (otherwise a simple
Boolean to indicate external trust should be enough), int hat case I
would propose to rewrite it in this form and document it:


"""
def allow_behavior(self, *flags):
  """
  Set the expected trust join behavior
  :param flags: trust flags to set
  """
  for flag in flags:
  self.__allow_behavior |= flag
"""

Also, I am not a big fan of doubly underscored methods/variables since
they do not 'hide' anything (Python's name mangling can be bypassed
with ease anyway). If you want to indicate that the 'allow_behavior'
attribute belongs to the internal implementation of the class prefix
it with single underscore.

You cannot have a property and a method named the same in the same
class. I changed the code to follow your other suggestion. I kept the
__allow_behavior as it is, though, because there is no difference
between __ and _ semantically to a person who would be reading the code
in question but I keep __ memorized. ;)



Fair enough, although you could have more meaningful method and 
attribute names, like `self.__behavior_flags` and `def 
set_behavior_flags()`.

Nah, this is not worth it, really.


Updated patch is attached.



Also when sending revised patches please try to conform to the common 
patch naming convention: 
http://www.freeipa.org/page/Contribute/Patch_Format#Naming


Surprisingly I have found out that my own patches actually do not 
follow it. Shame on me!

Frankly speaking, this particular part -- revision number after the
patch number versus at the end of the patch file name -- always created
issues to me. You can tell git to use specific suffix (.1.patch, for
example) to avoid doing full renames with the latter rather than
shoehorning the number into the middle.

--
/ Alexander Bokovoy

--
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] 0206 adtrust optimize forest root LDAP filter

[Alexander Bokovoy]

Hi,

`ipa trust-find' command should only show trusted forest root domains

The child domains should be visible via

  ipa trustdomain-find forest.root

The difference between forest root (or external domain) and child
domains is that root domain gets ipaIDObject class to allow assigning a
POSIX ID to the object. This POSIX ID is used by Samba when an Active
Directory domain controller connects as forest trusted domain object.

Child domains can only talk to IPA via forest root domain, thus they
don't need POSIX ID for their TDOs. This allows us a way to
differentiate objects for the purpose of 'trust-find' /
'trustdomain-find' commands.

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


--
/ Alexander Bokovoy
From 672f62bfe736d28ac1cbd4535f3a841ff9abd52e Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy 
Date: Tue, 7 Jun 2016 18:54:36 +0300
Subject: [PATCH 6/6] adtrust: optimize forest root LDAP filter

`ipa trust-find' command should only show trusted forest root domains

The child domains should be visible via

   ipa trustdomain-find forest.root

The difference between forest root (or external domain) and child
domains is that root domain gets ipaIDObject class to allow assigning a
POSIX ID to the object. This POSIX ID is used by Samba when an Active
Directory domain controller connects as forest trusted domain object.

Child domains can only talk to IPA via forest root domain, thus they
don't need POSIX ID for their TDOs. This allows us a way to
differentiate objects for the purpose of 'trust-find' /
'trustdomain-find' commands.

Fixes https://fedorahosted.org/freeipa/ticket/5942
---
 ipaserver/plugins/trust.py | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/ipaserver/plugins/trust.py b/ipaserver/plugins/trust.py
index f9b48f3..0f4f71a 100644
--- a/ipaserver/plugins/trust.py
+++ b/ipaserver/plugins/trust.py
@@ -485,7 +485,7 @@ class trust(LDAPObject):
 container_dn = api.env.container_trusts
 object_name = _('trust')
 object_name_plural = _('trusts')
-object_class = ['ipaNTTrustedDomain']
+object_class = ['ipaNTTrustedDomain', 'ipaIDObject']
 default_attributes = ['cn', 'ipantflatname', 'ipanttrusteddomainsid',
   'ipanttrusttype', 'ipanttrustattributes',
   'ipanttrustdirection', 'ipanttrustpartner',
@@ -577,7 +577,7 @@ class trust(LDAPObject):
 if trust_type is None:
 ldap = self.backend
 trustfilter = ldap.make_filter({
-'objectclass': ['ipaNTTrustedDomain'],
+'objectclass': ['ipaNTTrustedDomain', 'ipaIDObject'],
 'cn': [keys[-1]]},
 rules=ldap.MATCH_ALL
 )
@@ -1074,9 +1074,7 @@ class trust_find(LDAPSearch):
 # search needs to be done on a sub-tree scope
 def pre_callback(self, ldap, filters, attrs_list, base_dn, scope, *args, 
**options):
 # list only trust, not trust domains
-trust_filter = 
'(&(ipaNTTrustPartner=*)(&(objectclass=ipaIDObject)(objectclass=ipaNTTrustedDomain)))'
-filter = ldap.combine_filters((filters, trust_filter), 
rules=ldap.MATCH_ALL)
-return (filter, base_dn, ldap.SCOPE_SUBTREE)
+return (filters, base_dn, ldap.SCOPE_SUBTREE)
 
 def execute(self, *args, **options):
 result = super(trust_find, self).execute(*args, **options)
-- 
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] 0202 support UPNs for trusted domain users

[Alexander Bokovoy]

On Tue, 07 Jun 2016, Martin Babinsky wrote:

On 06/06/2016 12:34 PM, Alexander Bokovoy wrote:

Hi,

Add support for additional user name principal suffixes from
trusted Active Directory forests. UPN suffixes are property
of the forest and as such are associated with the forest root
domain.

FreeIPA stores UPN suffixes as ipaNTAdditionalSuffixes multi-valued
attribute of ipaNTTrustedDomain object class.

In order to look up UPN suffixes, netr_DsRGetForestTrustInformation
LSA RPC call is used instead of netr_DsrEnumerateDomainTrusts.

For more details on UPN and naming in Active Directory see
https://technet.microsoft.com/en-us/library/cc739093%28v=ws.10%29.aspx

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





Hi Alexander,

a few comments:

1.)

there are some PEP8 violations in the patch. Not all of them need to 
be fixed, though (line overhangs < 5 characters are OK by me).


"""
./ipaserver/dcerpc.py:1199:80: E501 line too long (80 > 79 characters)
./ipaserver/dcerpc.py:1200:80: E501 line too long (83 > 79 characters)
./ipaserver/dcerpc.py:1258:40: E203 whitespace before ':'
./ipaserver/dcerpc.py:1263:80: E501 line too long (81 > 79 characters)
./ipaserver/dcerpc.py:1271:80: E501 line too long (90 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/plugins/trust.py:490:9: E128 continuation line 
under-indented for visual indent

./ipaserver/plugins/trust.py:490:80: E501 line too long (93 > 79 characters)
./ipaserver/plugins/trust.py:490:92: E202 whitespace before ']'
./ipaserver/plugins/trust.py:493:80: E501 line too long (83 > 79 characters)
./ipaserver/plugins/trust.py:1461:80: E501 line too long (80 > 79 
characters)

./ipaserver/plugins/trust.py:1462:59: E202 whitespace before ']'
./ipaserver/plugins/trust.py:1544:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:1638:1: E302 expected 2 blank lines, found 1
"""

I've fixed trust.py part, the dcerpc.py fixes in 0201 should be enough
now -- breaking following line is not giving any reasonable benefit:

   res['ipantflatname'] = 
unicode(t.forest_trust_data.netbios_domain_name.string)


2.)

Should the ipaNTAdditionalSuffixes attribute be case insensitive? It 
makes sense but I'm just asking so that we don't end changing the 
schema later.

ipaNTAdditionalSuffixes is defined as

attributeTypes: ( 2.16.840.1.113730.3.8.11.75 NAME
'ipaNTAdditionalSuffixes' DESC 'Suffix for the user principal name
associated with the domain' EQUALITY caseIgnoreMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.15)

There should be no problem with case sensitivity already.


3.)

"""
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.get('pkey_only', False):
return truncated
trust_dn = self.obj.get_dn(args[0], trust_type=u'ad')
trust_entry = ldap.get_entry(trust_dn)
+blacklist = trust_entry.get('ipantsidblacklistincoming')
for entry in entries:
-sid = entry['ipanttrusteddomainsid'][0]
-
-blacklist = trust_entry.get('ipantsidblacklistincoming')
-if blacklist is None:
+sid = entry.get('ipanttrusteddomainsid', [None])[0]
+if sid is None:
continue

if sid in blacklist:
entry['domain_enabled'] = [False]
else:
entry['domain_enabled'] = [True]
return truncated
"""

Again, you can use `entry.single_value.get('ipanttrusteddomainsid', 
None)` to test/get the attribute value

done.



4.)


"""
def add_new_domains_from_trust(myapi, trustinstance, trust_entry, 
domains, **options):

result = []
if not domains:
return result

trust_name = trust_entry['cn'][0]
# trust range must exist by the time add_new_domains_from_trust 
is called

range_name = trust_name.upper() + '_id_range'
old_range = myapi.Command.idrange_show(range_name, raw=True)['result']
idrange_type = old_range['iparangetype'][0]

-for dom in domains:
+suffixes = list()
+suffixes.extend(y['cn'] for x,y in 
domains['suffixes'].iteritems() if x not in domains['domains'])

+
+for dom in domains['domains'].itervalues():
dom['trust_type'] = u'ad'

"""

iterkeys/itervalues are not Python 3 compatible, please use 
`keys()/values()` and do not worry about performance implications in 
Python 2.

I switched to six.iteritems().



5.)

"""
result.append(res['result'])

-if idrange_type != u'ipa-ad-trust-posix':
+if idrange_type != u'ipa-ad-trust-posix' and 
dom.get('ipanttrusteddomainsid', False):

range_name = name.upper() + '_id_range'

"""

Just a nitpick, I am confused by the 'False' default when getting the 
value of 'ipanttrusteddomainsid' (which is a string I presume). You 
could put an empty string/list there since empty sequences evaluate to 
False.

the attribute may not be in the object so I need to have something to
evaluate 

Re: [Freeipa-devel] [PATCH] 0201 Add support for an external trust to Active Directory domain

[Martin Babinsky]

On 06/07/2016 06:00 PM, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, Martin Babinsky wrote:

On 06/06/2016 12:33 PM, Alexander Bokovoy wrote:

Hi,

this patch adds support for external trust to Active Directory.

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743




Hi Alexander,

I have a few comments:

1.) there are numerous pep8 errors in the code:

"""
./ipaserver/dcerpc.py:1044:80: E501 line too long (115 > 79 characters)
./ipaserver/dcerpc.py:1044:106: E251 unexpected spaces around keyword
/ parameter equals
./ipaserver/dcerpc.py:1044:108: E251 unexpected spaces around keyword
/ parameter equals
./ipaserver/dcerpc.py:1107:80: E501 line too long (110 > 79 characters)
./ipaserver/dcerpc.py:1108:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (114 > 79 characters)
./ipaserver/dcerpc.py::80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1400:80: E501 line too long (143 > 79 characters)
./ipaserver/dcerpc.py:1401:80: E501 line too long (88 > 79 characters)
./ipaserver/plugins/trust.py:66:80: E501 line too long (91 > 79
characters)
./ipaserver/plugins/trust.py:174:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:175:80: E501 line too long (106 > 79
characters)
./ipaserver/plugins/trust.py:201:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:209:80: E501 line too long (91 > 79
characters)
./ipaserver/plugins/trust.py:690:9: E124 closing bracket does not
match visual indentation
./ipaserver/plugins/trust.py:694:80: E501 line too long (127 > 79
characters)
"""

Please fix them. You can check whether your commit does not break pep8
by running `git show HEAD -U0 | pep8 --diff`.

Thanks for the review.

The lines will have to be long. Sorry, we are not in the age of
80-column terminals anymore. Try to look at the ipaserver/dcerpc.py from
a bigger perspective -- there are currently 224 errors reported by pep8
in this file. Fixing some of them makes little sense -- for example,
pylint hints have to be where they are and that makes lines long, error
messages have to be what they are and changing them to be smaller is
counterproductive.

I'm updated the patch to have most of the lines within 90 columns.



This is not about 80 column terminals, this is about a) not moving your 
neck through its full range of motion when reading the code, b) 
conforming to the rules set up by some project regarding contributions 
to its codebase [1].


These are all the pep8 violations introduced _only by the code the patch 
is touching/adding_, not in the whole dcerpc module:



"""
./ipaserver/dcerpc.py:1045:80: E501 line too long (113 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (93 > 79 characters)
./ipaserver/dcerpc.py:1110:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1112:80: E501 line too long (97 > 79 characters)
./ipaserver/dcerpc.py:1114:80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1287:12: E111 indentation is not a multiple of four
./ipaserver/dcerpc.py:1379:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1380:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1386:80: E501 line too long (89 > 79 characters)
./ipaserver/dcerpc.py:1388:80: E501 line too long (88 > 79 characters)
./ipaserver/dcerpc.py:1407:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1408:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1411:80: E501 line too long (86 > 79 characters)
./ipaserver/plugins/trust.py:176:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:177:80: E501 line too long (106 > 79 
characters)

./ipaserver/plugins/trust.py:203:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:211:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:695:80: E501 line too long (127 > 79 
characters)
./ipaserver/plugins/trust.py:697:9: E124 closing bracket does not match 
visual indentation
./ipaserver/plugins/trust.py:1062:25: E127 continuation line 
over-indented for visual indent
./ipaserver/plugins/trust.py:1072:80: E501 line too long (109 > 79 
characters)
./ipaserver/plugins/trust.py:1092:80: E501 line too long (80 > 79 
characters)
./ipaserver/plugins/trust.py:1103:25: E127 continuation line 
over-indented for visual indent
./ipaserver/plugins/trust.py:1103:80: E501 line too long (104 > 79 
characters)
./ipaserver/plugins/trust.py:1121:80: 

Re: [Freeipa-devel] [PATCH] 0005 Always qualify requests for admin in ipa-replica-conncheck

[Martin Basti]

On 07.06.2016 17:25, Florence Blanc-Renaud wrote:

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




On 02.06.2016 14:58, Florence Blanc-Renaud wrote:


Hi,

this patch modifies ipa-replica-conncheck when it performs the SSH 
connection to the master, so that the username is always fully 
qualified.


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




LGTM, but because current issues with replica install in master 
branch, I couldn't test it and I would like to be sure that 
ipa-replica-install using NTP will work too



Just little nitpick, for better readibility, 'command' should be on 
new line

-'%s@%s' % (self.user, self.addr), command
+'-o User=%s' % self.user,
+'%s' % self.addr, command

Martin^2


Hi Martin,

thanks for the review. I am attaching a new patch with your 
suggestion. Just for my record, what would be the command-line options 
to test the scenario you're referring to?


Flo.



Hello,

scenario is:
1. install server
2. create host entry with OTP (ipa host-add replica.hostname 
--password=OTPpasswd)
3. add host to ipaservers group (ipa hostgroup-add-member ipaservers 
--hosts=replica.hostname)
4. install replica (ipa-replica-install --server  
--domain  --password=OTPpasswd)


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

Re: [Freeipa-devel] [PATCH] 0201 Add support for an external trust to Active Directory domain

[Alexander Bokovoy]

On Tue, 07 Jun 2016, Martin Babinsky wrote:

On 06/06/2016 12:33 PM, Alexander Bokovoy wrote:

Hi,

this patch adds support for external trust to Active Directory.

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743




Hi Alexander,

I have a few comments:

1.) there are numerous pep8 errors in the code:

"""
./ipaserver/dcerpc.py:1044:80: E501 line too long (115 > 79 characters)
./ipaserver/dcerpc.py:1044:106: E251 unexpected spaces around keyword 
/ parameter equals
./ipaserver/dcerpc.py:1044:108: E251 unexpected spaces around keyword 
/ parameter equals

./ipaserver/dcerpc.py:1107:80: E501 line too long (110 > 79 characters)
./ipaserver/dcerpc.py:1108:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (114 > 79 characters)
./ipaserver/dcerpc.py::80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1400:80: E501 line too long (143 > 79 characters)
./ipaserver/dcerpc.py:1401:80: E501 line too long (88 > 79 characters)
./ipaserver/plugins/trust.py:66:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:174:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:175:80: E501 line too long (106 > 79 
characters)

./ipaserver/plugins/trust.py:201:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:209:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:690:9: E124 closing bracket does not 
match visual indentation
./ipaserver/plugins/trust.py:694:80: E501 line too long (127 > 79 
characters)

"""

Please fix them. You can check whether your commit does not break pep8 
by running `git show HEAD -U0 | pep8 --diff`.

Thanks for the review.

The lines will have to be long. Sorry, we are not in the age of
80-column terminals anymore. Try to look at the ipaserver/dcerpc.py from
a bigger perspective -- there are currently 224 errors reported by pep8
in this file. Fixing some of them makes little sense -- for example,
pylint hints have to be where they are and that makes lines long, error
messages have to be what they are and changing them to be smaller is
counterproductive.

I'm updated the patch to have most of the lines within 90 columns.



2.)

I have difficulty understanding the following code:

"""
+self.__allow_behavior = 0

domain_validator = DomainValidator(api)
self.configured = domain_validator.is_configured()

if self.configured:
self.local_flatname = domain_validator.flatname
self.local_dn = domain_validator.dn
self.__populate_local_domain()

+def allow_behavior(self, behavior_set):
+if type(behavior_set) is int:
+   behavior_set = [behavior_set]
+if TRUST_JOIN_EXTERNAL in behavior_set:
+   self.__allow_behavior |= TRUST_JOIN_EXTERNAL
+
"""
I assume this is made like this to accommodate setting some other 
behavior flags which can pop up in the future (otherwise a simple 
Boolean to indicate external trust should be enough), int hat case I 
would propose to rewrite it in this form and document it:



"""
def allow_behavior(self, *flags):
   """
   Set the expected trust join behavior
   :param flags: trust flags to set
   """
   for flag in flags:
   self.__allow_behavior |= flag
"""

Also, I am not a big fan of doubly underscored methods/variables since 
they do not 'hide' anything (Python's name mangling can be bypassed 
with ease anyway). If you want to indicate that the 'allow_behavior' 
attribute belongs to the internal implementation of the class prefix 
it with single underscore.

You cannot have a property and a method named the same in the same
class. I changed the code to follow your other suggestion. I kept the
__allow_behavior as it is, though, because there is no difference
between __ and _ semantically to a person who would be reading the code
in question but I keep __ memorized. ;)



3.)

"""
if self.remote_domain.info['dns_domain'] != 
self.remote_domain.info['dns_forest']:
-raise errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], 
domain=self.remote_domain.info['dns_domain'])

+if not (self.__allow_behavior & TRUST_JOIN_EXTERNAL):
+raise errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], 
domain=self.remote_domain.info['dns_domain'])


if not self.remote_domain.read_only:
trustdom_pass = 

Re: [Freeipa-devel] [PATCH] 0202 support UPNs for trusted domain users

[Martin Babinsky]

On 06/06/2016 12:34 PM, Alexander Bokovoy wrote:

Hi,

Add support for additional user name principal suffixes from
trusted Active Directory forests. UPN suffixes are property
of the forest and as such are associated with the forest root
domain.

FreeIPA stores UPN suffixes as ipaNTAdditionalSuffixes multi-valued
attribute of ipaNTTrustedDomain object class.

In order to look up UPN suffixes, netr_DsRGetForestTrustInformation
LSA RPC call is used instead of netr_DsrEnumerateDomainTrusts.

For more details on UPN and naming in Active Directory see
https://technet.microsoft.com/en-us/library/cc739093%28v=ws.10%29.aspx

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





Hi Alexander,

a few comments:

1.)

there are some PEP8 violations in the patch. Not all of them need to be 
fixed, though (line overhangs < 5 characters are OK by me).


"""
./ipaserver/dcerpc.py:1199:80: E501 line too long (80 > 79 characters)
./ipaserver/dcerpc.py:1200:80: E501 line too long (83 > 79 characters)
./ipaserver/dcerpc.py:1258:40: E203 whitespace before ':'
./ipaserver/dcerpc.py:1263:80: E501 line too long (81 > 79 characters)
./ipaserver/dcerpc.py:1271:80: E501 line too long (90 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/plugins/trust.py:490:9: E128 continuation line 
under-indented for visual indent

./ipaserver/plugins/trust.py:490:80: E501 line too long (93 > 79 characters)
./ipaserver/plugins/trust.py:490:92: E202 whitespace before ']'
./ipaserver/plugins/trust.py:493:80: E501 line too long (83 > 79 characters)
./ipaserver/plugins/trust.py:1461:80: E501 line too long (80 > 79 
characters)

./ipaserver/plugins/trust.py:1462:59: E202 whitespace before ']'
./ipaserver/plugins/trust.py:1544:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:1638:1: E302 expected 2 blank lines, found 1
"""

2.)

Should the ipaNTAdditionalSuffixes attribute be case insensitive? It 
makes sense but I'm just asking so that we don't end changing the schema 
later.


3.)

"""
 def post_callback(self, ldap, entries, truncated, *args, **options):
 if options.get('pkey_only', False):
 return truncated
 trust_dn = self.obj.get_dn(args[0], trust_type=u'ad')
 trust_entry = ldap.get_entry(trust_dn)
+blacklist = trust_entry.get('ipantsidblacklistincoming')
 for entry in entries:
-sid = entry['ipanttrusteddomainsid'][0]
-
-blacklist = trust_entry.get('ipantsidblacklistincoming')
-if blacklist is None:
+sid = entry.get('ipanttrusteddomainsid', [None])[0]
+if sid is None:
 continue

 if sid in blacklist:
 entry['domain_enabled'] = [False]
 else:
 entry['domain_enabled'] = [True]
 return truncated
"""

Again, you can use `entry.single_value.get('ipanttrusteddomainsid', 
None)` to test/get the attribute value


4.)


"""
 def add_new_domains_from_trust(myapi, trustinstance, trust_entry, 
domains, **options):

 result = []
 if not domains:
 return result

 trust_name = trust_entry['cn'][0]
 # trust range must exist by the time add_new_domains_from_trust is 
called

 range_name = trust_name.upper() + '_id_range'
 old_range = myapi.Command.idrange_show(range_name, raw=True)['result']
 idrange_type = old_range['iparangetype'][0]

-for dom in domains:
+suffixes = list()
+suffixes.extend(y['cn'] for x,y in domains['suffixes'].iteritems() 
if x not in domains['domains'])

+
+for dom in domains['domains'].itervalues():
 dom['trust_type'] = u'ad'

"""

iterkeys/itervalues are not Python 3 compatible, please use 
`keys()/values()` and do not worry about performance implications in 
Python 2.


5.)

"""
 result.append(res['result'])

-if idrange_type != u'ipa-ad-trust-posix':
+if idrange_type != u'ipa-ad-trust-posix' and 
dom.get('ipanttrusteddomainsid', False):

 range_name = name.upper() + '_id_range'

"""

Just a nitpick, I am confused by the 'False' default when getting the 
value of 'ipanttrusteddomainsid' (which is a string I presume). You 
could put an empty string/list there since empty sequences evaluate to 
False.


6.)

"""
 pass
+
+try:
+dn = myapi.Object.trust.get_dn(trust_name, trust_type=u'ad')
+ldap = myapi.Backend.ldap2
+entry = ldap.get_entry(dn)
+tlns = list(entry.get('ipantadditionalsuffixes', []))
+tlns.extend(x for x in suffixes if x not in tlns)
+entry['ipantadditionalsuffixes'] = tlns
+ldap.update_entry(entry)
+except errors.EmptyModlist:
+pass
+

"""

On this line:

"""
+tlns = list(entry.get('ipantadditionalsuffixes', []))
"""

is the additional conversion to list really needed? IIRC for multivalued 
attributes you get a list by default.


7.)

"""
 dn = 

Re: [Freeipa-devel] [PATCH] 0005 Always qualify requests for admin in ipa-replica-conncheck

[Florence Blanc-Renaud]

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




On 02.06.2016 14:58, Florence Blanc-Renaud wrote:


Hi,

this patch modifies ipa-replica-conncheck when it performs the SSH 
connection to the master, so that the username is always fully qualified.


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




LGTM, but because current issues with replica install in master 
branch, I couldn't test it and I would like to be sure that 
ipa-replica-install using NTP will work too



Just little nitpick, for better readibility, 'command' should be on 
new line

-'%s@%s' % (self.user, self.addr), command
+'-o User=%s' % self.user,
+'%s' % self.addr, command

Martin^2


Hi Martin,

thanks for the review. I am attaching a new patch with your suggestion. 
Just for my record, what would be the command-line options to test the 
scenario you're referring to?


Flo.

From c044d89b789c91384ac0c648e1f2eee88cac4cf3 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud 
Date: Wed, 1 Jun 2016 17:42:48 +0200
Subject: [PATCH] Always qualify requests for admin in ipa-replica-conncheck

ipa-replica-conncheck connects to the master using an SSH command:
ssh -o StrictHostKeychecking=no -o UserKnownHostsFile= \
-o GSSAPIAuthentication=yes @ \
echo OK

The issue is that the principal name is not fully qualified (for instance
'admin' is used, even if ipa-replica-conncheck was called with
--principal ad...@example.com).
When the FreeIPA server is running with a /etc/sssd/sssd.conf containing
[sssd]
default_domain_suffix = ad.domain.com
this leads to the SSH connection failure because admin is not defined in
the default domain.

The fix uses the fully qualified principal name, and calls ssh with
ssh -o StrictHostKeychecking=no -o UserKnownHostsFile= \
-o GSSAPIAuthentication=yes -o User= \
 echo OK
to avoid syntax issues with admin@DOMAIN@master

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

diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index fdf08d63127614a9b26995026e3c25806003f5a0..991f4e429dd1df7036b4a1c0175ca5daaea521ad 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -66,7 +66,9 @@ class SshExec(object):
 '-o StrictHostKeychecking=no',
 '-o UserKnownHostsFile=%s' % tmpf.name,
 '-o GSSAPIAuthentication=yes',
-'%s@%s' % (self.user, self.addr), command
+'-o User=%s' % self.user,
+'%s' % self.addr,
+command
 ]
 if verbose:
 cmd.insert(1, '-v')
@@ -517,7 +519,8 @@ def main():
 except Exception:
 print_info("Retrying using SSH...")
 
-user = principal.partition('@')[0]
+# Ticket 5812 Always qualify requests for admin
+user = principal
 ssh = SshExec(user, options.master)
 
 print_info("Check SSH connection to remote master")
-- 
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] 0201 Add support for an external trust to Active Directory domain

[Martin Babinsky]

On 06/06/2016 12:33 PM, Alexander Bokovoy wrote:

Hi,

this patch adds support for external trust to Active Directory.

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743




Hi Alexander,

I have a few comments:

1.) there are numerous pep8 errors in the code:

"""
./ipaserver/dcerpc.py:1044:80: E501 line too long (115 > 79 characters)
./ipaserver/dcerpc.py:1044:106: E251 unexpected spaces around keyword / 
parameter equals
./ipaserver/dcerpc.py:1044:108: E251 unexpected spaces around keyword / 
parameter equals

./ipaserver/dcerpc.py:1107:80: E501 line too long (110 > 79 characters)
./ipaserver/dcerpc.py:1108:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (114 > 79 characters)
./ipaserver/dcerpc.py::80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1400:80: E501 line too long (143 > 79 characters)
./ipaserver/dcerpc.py:1401:80: E501 line too long (88 > 79 characters)
./ipaserver/plugins/trust.py:66:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:174:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:175:80: E501 line too long (106 > 79 
characters)

./ipaserver/plugins/trust.py:201:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:209:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:690:9: E124 closing bracket does not match 
visual indentation
./ipaserver/plugins/trust.py:694:80: E501 line too long (127 > 79 
characters)

"""

Please fix them. You can check whether your commit does not break pep8 
by running `git show HEAD -U0 | pep8 --diff`.


2.)

I have difficulty understanding the following code:

"""
+self.__allow_behavior = 0

 domain_validator = DomainValidator(api)
 self.configured = domain_validator.is_configured()

 if self.configured:
 self.local_flatname = domain_validator.flatname
 self.local_dn = domain_validator.dn
 self.__populate_local_domain()

+def allow_behavior(self, behavior_set):
+if type(behavior_set) is int:
+   behavior_set = [behavior_set]
+if TRUST_JOIN_EXTERNAL in behavior_set:
+   self.__allow_behavior |= TRUST_JOIN_EXTERNAL
+
"""
I assume this is made like this to accommodate setting some other 
behavior flags which can pop up in the future (otherwise a simple 
Boolean to indicate external trust should be enough), int hat case I 
would propose to rewrite it in this form and document it:



"""
def allow_behavior(self, *flags):
"""
Set the expected trust join behavior
:param flags: trust flags to set
"""
for flag in flags:
self.__allow_behavior |= flag
"""

Also, I am not a big fan of doubly underscored methods/variables since 
they do not 'hide' anything (Python's name mangling can be bypassed with 
ease anyway). If you want to indicate that the 'allow_behavior' 
attribute belongs to the internal implementation of the class prefix it 
with single underscore.


3.)

"""
 if self.remote_domain.info['dns_domain'] != 
self.remote_domain.info['dns_forest']:
-raise 
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], 
domain=self.remote_domain.info['dns_domain'])

+if not (self.__allow_behavior & TRUST_JOIN_EXTERNAL):
+raise 
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], 
domain=self.remote_domain.info['dns_domain'])


 if not self.remote_domain.read_only:
 trustdom_pass = samba.generate_random_password(128, 128)
 self.get_realmdomains()
-self.remote_domain.establish_trust(self.local_domain, 
trustdom_pass, trust_type)
-self.local_domain.establish_trust(self.remote_domain, 
trustdom_pass, trust_type)
+self.remote_domain.establish_trust(self.local_domain, 
trustdom_pass, trust_type, bool(self.__allow_behavior & 
TRUST_JOIN_EXTERNAL))
+self.local_domain.establish_trust(self.remote_domain, 
trustdom_pass, trust_type, bool(self.__allow_behavior & 
TRUST_JOIN_EXTERNAL))
 # if trust is inbound, we don't need to verify it because 
AD DC will respond
 # with WERR_NO_SUCH_DOMAIN -- in only does verification 
for outbound trusts.

 ...

"""

Please use a separate variable to store the result of bitwise AND you 
pass to the methods, e.g.:


"""
join_as_external = 

Re: [Freeipa-devel] ipapwd_extop vs password_extop

[thierry bordaz]

On 06/07/2016 03:47 PM, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, thierry bordaz wrote:
Well here we have IPA password extop that receives a 'compat' 
entry. This compat entry does not exist except in slapi-nis that 
can do the mapping to the real entry. What I was thinking of was 
some kind of call from IPA password extop to slapi-nis that for a 
given entry DN return the real entryDN. But the tranformation of 
the extop('compat') -> extop('real') would be done in IPA password 
extop
no, just look at slapi-nis code to see how we rewrite DN of the 
request.

You'd need to do a similar trick.



Thanks for the pointer. What differs is that slapi-nis is doing the 
mapping in an operation (here bind) preop.
But with extop there is no preop call. Mapping looks to be done in 
backend_locate, my understanding is that we need to find a way to 
call something like backend_locate from extop and it can not be done 
with an internal search because slapi-nis ignores them.

May be it is time to add pre/post operations for extop? Granted, they
are not going to be useful for most cases but this would solve our
problem, right?

Right but it creates a dependency on DS.
An other option would be to use the broker api (to allow a plugin to 
call others plugins callback), but it would require changes on slapi-nis 
and IPA pwd extop.


The nicer approach is a extop - preop/postop. I will start on this.
Thanks you for all you feedback

--
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] ipapwd_extop vs password_extop

[Alexander Bokovoy]

On Tue, 07 Jun 2016, thierry bordaz wrote:
Well here we have IPA password extop that receives a 'compat' 
entry. This compat entry does not exist except in slapi-nis that 
can do the mapping to the real entry. What I was thinking of was 
some kind of call from IPA password extop to slapi-nis that for a 
given entry DN return the real entryDN. But the tranformation of 
the extop('compat') -> extop('real') would be done in IPA password 
extop

no, just look at slapi-nis code to see how we rewrite DN of the request.
You'd need to do a similar trick.



Thanks for the pointer. What differs is that slapi-nis is doing the 
mapping in an operation (here bind) preop.
But with extop there is no preop call. Mapping looks to be done in 
backend_locate, my understanding is that we need to find a way to call 
something like backend_locate from extop and it can not be done with 
an internal search because slapi-nis ignores them.

May be it is time to add pre/post operations for extop? Granted, they
are not going to be useful for most cases but this would solve our
problem, right?
--
/ Alexander Bokovoy

--
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] ipapwd_extop vs password_extop

[thierry bordaz]

On 06/07/2016 01:20 PM, Alexander Bokovoy wrote:

On Tue, 07 Jun 2016, thierry bordaz wrote:



On 06/06/2016 07:12 PM, Alexander Bokovoy wrote:

On Mon, 06 Jun 2016, thierry bordaz wrote:



On 06/06/2016 11:07 AM, Alexander Bokovoy wrote:

On Mon, 06 Jun 2016, thierry bordaz wrote:

Hello,

In DS it is possible to register callbacks for extended op.
For https://www.ietf.org/rfc/rfc3062.txt (password modify extop),
there is a default callback that is implemented in DS core server.
Freeipa enables a plugin 'cn=ipa_pwd_extop,cn=plugins,cn=config'
that also register a callback ipapwd_extop/ipapwd_chpwop, for that
same extop.

The server calls the callbacks until it can find one that can handle
the OID. But the order is not guaranty.
So the extop can be handled either by password_extop or either by
ipapwd_chpwop.

Those two functions are not doing the same checking/updates.

I would like to confirm if in Freeipa context, if only
ipapwd_extop/ipapwd_chpwop should be called

Correct. I think we have also added plugin priority to allow handling
this type of conflicts gracefully.




The order of the plugins, is based on nsslapd-pluginprecedence.
(The lower precedence is called first)

The problem is that ipapw_extop and password_extop have the same 
precedence: 50. So the order they are selected basically rely on 
order they were registered.
"Password Modify extended operation plugin" (core server) is 
registered before "IPA Password Extended Operation plugin".
I think we need  to add a precedence of "IPA Password Extended 
Operation plugin" to be sure it will be selected over "Password 
Modify extended operation plugin"


In addition https://fedorahosted.org/389/ticket/48770 changed the 
way plugins are selected so even setting a precedence is currently 
not a solution (I opened https://fedorahosted.org/389/ticket/48870)


In short we need to fix https://fedorahosted.org/389/ticket/48870


Regarding item 5.4, replacing a compat entry with its related real 
entry.
I think there is something missing.  In fact the extop is doing an 
internal MOD but before calling the MOD it checks the entry exists. 
And if called against a 'compat' entry the extop fails before 
calling the MOD.
But even if it was successfull it may be not be a good idea to make 
'Schema Compat' preop-mod doing the mapping 'compat' -> real for 
all MODS.
given that schema compat is read-only (always), you can do remapping 
all

the time. We have this mapping already for password checks, how is this
different?
It should be similar to mapping done for password check. When is it 
triggered or where is the code for this mapping ?

See slapi-nis/src/back-sch.c:backend_bind_cb() for details.

A option is to change "IPA Password Extended Operation plugin" to 
do this mapping 'compat' -> 'real' but it is looking like a hack.

Yes, it is unacceptable.

Well here we have IPA password extop that receives a 'compat' entry. 
This compat entry does not exist except in slapi-nis that can do the 
mapping to the real entry. What I was thinking of was some kind of 
call from IPA password extop to slapi-nis that for a given entry DN 
return the real entryDN. But the tranformation of the extop('compat') 
-> extop('real') would be done in IPA password extop

no, just look at slapi-nis code to see how we rewrite DN of the request.
You'd need to do a similar trick.



Thanks for the pointer. What differs is that slapi-nis is doing the 
mapping in an operation (here bind) preop.
But with extop there is no preop call. Mapping looks to be done in 
backend_locate, my understanding is that we need to find a way to call 
something like backend_locate from extop and it can not be done with an 
internal search because slapi-nis ignores them.




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

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

[Stanislav Laznicka]

Hello,

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


Also, I have a few comments to the patch:

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


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


Standa


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


Hi all,

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


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




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

Re: [Freeipa-devel] [PATCH 0499] Pylint: exclude some files/dirs from check

[Martin Basti]

On 07.06.2016 12:58, Pavel Vomacka wrote:




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

See commit message, yacctab.py causes lint errors and must be excluded


Patch attached.




Works well, ACK.

--
Pavel^3 Vomacka



Pushed to master: 1d9425dab7b16a0c518dadc5ba42c027045c4529

-- 
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] ipapwd_extop vs password_extop

[Alexander Bokovoy]

On Tue, 07 Jun 2016, thierry bordaz wrote:



On 06/06/2016 07:12 PM, Alexander Bokovoy wrote:

On Mon, 06 Jun 2016, thierry bordaz wrote:



On 06/06/2016 11:07 AM, Alexander Bokovoy wrote:

On Mon, 06 Jun 2016, thierry bordaz wrote:

Hello,

In DS it is possible to register callbacks for extended op.
For https://www.ietf.org/rfc/rfc3062.txt (password modify extop),
there is a default callback that is implemented in DS core server.
Freeipa enables a plugin 'cn=ipa_pwd_extop,cn=plugins,cn=config'
that also register a callback ipapwd_extop/ipapwd_chpwop, for that
same extop.

The server calls the callbacks until it can find one that can handle
the OID. But the order is not guaranty.
So the extop can be handled either by password_extop or either by
ipapwd_chpwop.

Those two functions are not doing the same checking/updates.

I would like to confirm if in Freeipa context, if only
ipapwd_extop/ipapwd_chpwop should be called

Correct. I think we have also added plugin priority to allow handling
this type of conflicts gracefully.




The order of the plugins, is based on nsslapd-pluginprecedence.
(The lower precedence is called first)

The problem is that ipapw_extop and password_extop have the same 
precedence: 50. So the order they are selected basically rely on 
order they were registered.
"Password Modify extended operation plugin" (core server) is 
registered before "IPA Password Extended Operation plugin".
I think we need  to add a precedence of "IPA Password Extended 
Operation plugin" to be sure it will be selected over "Password 
Modify extended operation plugin"


In addition https://fedorahosted.org/389/ticket/48770 changed the 
way plugins are selected so even setting a precedence is currently 
not a solution (I opened 
https://fedorahosted.org/389/ticket/48870)


In short we need to fix https://fedorahosted.org/389/ticket/48870


Regarding item 5.4, replacing a compat entry with its related real 
entry.
I think there is something missing.  In fact the extop is doing an 
internal MOD but before calling the MOD it checks the entry 
exists. And if called against a 'compat' entry the extop fails 
before calling the MOD.
But even if it was successfull it may be not be a good idea to 
make 'Schema Compat' preop-mod doing the mapping 'compat' -> real 
for all MODS.

given that schema compat is read-only (always), you can do remapping all
the time. We have this mapping already for password checks, how is this
different?
It should be similar to mapping done for password check. When is it 
triggered or where is the code for this mapping ?

See slapi-nis/src/back-sch.c:backend_bind_cb() for details.

A option is to change "IPA Password Extended Operation plugin" to 
do this mapping 'compat' -> 'real' but it is looking like a hack.

Yes, it is unacceptable.

Well here we have IPA password extop that receives a 'compat' entry. 
This compat entry does not exist except in slapi-nis that can do the 
mapping to the real entry. What I was thinking of was some kind of 
call from IPA password extop to slapi-nis that for a given entry DN 
return the real entryDN. But the tranformation of the extop('compat') 
-> extop('real') would be done in IPA password extop

no, just look at slapi-nis code to see how we rewrite DN of the request.
You'd need to do a similar trick.

--
/ Alexander Bokovoy

--
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] ipapwd_extop vs password_extop

[thierry bordaz]

On 06/06/2016 07:12 PM, Alexander Bokovoy wrote:

On Mon, 06 Jun 2016, thierry bordaz wrote:



On 06/06/2016 11:07 AM, Alexander Bokovoy wrote:

On Mon, 06 Jun 2016, thierry bordaz wrote:

Hello,

 In DS it is possible to register callbacks for extended op.
 For https://www.ietf.org/rfc/rfc3062.txt (password modify extop),
 there is a default callback that is implemented in DS core server.
 Freeipa enables a plugin 'cn=ipa_pwd_extop,cn=plugins,cn=config'
 that also register a callback ipapwd_extop/ipapwd_chpwop, for that
 same extop.

 The server calls the callbacks until it can find one that can handle
 the OID. But the order is not guaranty.
 So the extop can be handled either by password_extop or either by
 ipapwd_chpwop.

 Those two functions are not doing the same checking/updates.

 I would like to confirm if in Freeipa context, if only
 ipapwd_extop/ipapwd_chpwop should be called

Correct. I think we have also added plugin priority to allow handling
this type of conflicts gracefully.




The order of the plugins, is based on nsslapd-pluginprecedence.
(The lower precedence is called first)

The problem is that ipapw_extop and password_extop have the same 
precedence: 50. So the order they are selected basically rely on 
order they were registered.
"Password Modify extended operation plugin" (core server) is 
registered before "IPA Password Extended Operation plugin".
I think we need  to add a precedence of "IPA Password Extended 
Operation plugin" to be sure it will be selected over "Password 
Modify extended operation plugin"


In addition https://fedorahosted.org/389/ticket/48770 changed the way 
plugins are selected so even setting a precedence is currently not a 
solution (I opened https://fedorahosted.org/389/ticket/48870)


In short we need to fix https://fedorahosted.org/389/ticket/48870


Regarding item 5.4, replacing a compat entry with its related real 
entry.
I think there is something missing.  In fact the extop is doing an 
internal MOD but before calling the MOD it checks the entry exists. 
And if called against a 'compat' entry the extop fails before calling 
the MOD.
But even if it was successfull it may be not be a good idea to make 
'Schema Compat' preop-mod doing the mapping 'compat' -> real for all 
MODS.

given that schema compat is read-only (always), you can do remapping all
the time. We have this mapping already for password checks, how is this
different?
It should be similar to mapping done for password check. When is it 
triggered or where is the code for this mapping ?


A option is to change "IPA Password Extended Operation plugin" to do 
this mapping 'compat' -> 'real' but it is looking like a hack.

Yes, it is unacceptable.

Well here we have IPA password extop that receives a 'compat' entry. 
This compat entry does not exist except in slapi-nis that can do the 
mapping to the real entry. What I was thinking of was some kind of call 
from IPA password extop to slapi-nis that for a given entry DN return 
the real entryDN. But the tranformation of the extop('compat') -> 
extop('real') would be done in IPA password extop




--
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 0499] Pylint: exclude some files/dirs from check

[Pavel Vomacka]

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

See commit message, yacctab.py causes lint errors and must be excluded


Patch attached.




Works well, ACK.

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

Re: [Freeipa-devel] [PATCH 0492] Translations: update ipa-4-3 translations

[Martin Babinsky]

On 06/01/2016 05:10 PM, Martin Basti wrote:

Patch attached.


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

[Freeipa-devel] [PATCH 0403-0407] Preparation work for per-server config in LDAP

[Petr Spacek]

Hello,

this patch set is preparation work for per-server config in LDAP, which is
required for DNS location in IPA.

This patch set should not cause any user-visible changes.

https://fedorahosted.org/bind-dyndb-ldap/ticket/162

-- 
Petr^2 Spacek
From 5a4e0b7026dc4f7f786d1d59a3a9ad33bfe89e30 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Wed, 4 May 2016 16:20:44 +0200
Subject: [PATCH] Fix in destroy_ldap_instance() caused by uninitialized
 MetaLDAP.

This happened only if an new_ldap_instance() failed with an error before
initializing MetaLDAP.
---
 src/mldap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mldap.c b/src/mldap.c
index 8cffe8a1fbf8eaa20aae79c28ad8d7a305494f19..143abce757f3d65e68356a2c0e660c475ed0ab58 100644
--- a/src/mldap.c
+++ b/src/mldap.c
@@ -79,7 +79,7 @@ void
 mldap_destroy(mldapdb_t **mldapp) {
 	mldapdb_t *mldap;
 
-	REQUIRE(mldapp != NULL && *mldapp != NULL);
+	REQUIRE(mldapp != NULL);
 
 	mldap = *mldapp;
 	if (mldap == NULL)
-- 
2.5.5

From 6c4cabd8be5d90502786a5f4356dbb4742ec7a70 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Thu, 2 Jun 2016 15:30:19 +0200
Subject: [PATCH] Separate BIND config utilities from ACL parsing.

In future we will use the code from multiple modules.
There should be no user-visible changes caused by this split.

https://fedorahosted.org/bind-dyndb-ldap/ticket/162
---
 src/Makefile.am|   2 +
 src/acl.c  | 118 +++--
 src/bindcfg.c  | 116 
 src/bindcfg.h  |  24 +++
 src/zone_manager.c |   3 ++
 5 files changed, 151 insertions(+), 112 deletions(-)
 create mode 100644 src/bindcfg.c
 create mode 100644 src/bindcfg.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 58f73ec9d37471471036fb532ac239523512eb80..595fb4d95577f025e97dbce0770325a82a053ad8 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -3,6 +3,7 @@ bindplugindir=$(libdir)/bind
 
 HDRS =\
 	acl.h			\
+	bindcfg.h		\
 	compat.h		\
 	empty_zones.h		\
 	fs.h			\
@@ -31,6 +32,7 @@ HDRS =\
 ldap_la_SOURCES =		\
 	$(HDRS)			\
 	acl.c			\
+	bindcfg.c		\
 	empty_zones.c		\
 	fwd_register.c		\
 	fs.c			\
diff --git a/src/acl.c b/src/acl.c
index e4b3f524876ac93a177613c7f325883398367af8..d03a34d8c4ba5016ae9b248dc86781952d92c8d4 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -6,13 +6,11 @@
 
 #include 
 #include 
-#include 
 #include 
 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -30,17 +28,12 @@
 #include 
 
 #include "acl.h"
+#include "bindcfg.h"
 #include "str.h"
 #include "util.h"
 #include "log.h"
 #include "types.h"
 
-static isc_once_t once = ISC_ONCE_INIT;
-static cfg_type_t *update_policy;
-static cfg_type_t *allow_query;
-static cfg_type_t *allow_transfer;
-static cfg_type_t *forwarders;
-
 /* Following definitions are necessary for context ("map" configuration object)
  * required during ACL parsing. */
 static cfg_clausedef_t * empty_map_clausesets[] = {
@@ -60,105 +53,6 @@ const enum_txt_assoc_t acl_type_txts[] = {
 	{ -1,			NULL		} /* end marker */
 };
 
-static cfg_type_t * ATTR_NONNULLS ATTR_CHECKRESULT
-get_type_from_tuplefield(const cfg_type_t *cfg_type, const char *name)
-{
-	cfg_type_t *ret = NULL;
-	const cfg_tuplefielddef_t *field;
-
-	REQUIRE(cfg_type != NULL && cfg_type->of != NULL);
-	REQUIRE(name != NULL);
-
-	field = (cfg_tuplefielddef_t *)cfg_type->of;
-	for (int i = 0; field[i].name != NULL; i++) {
-		if (!strcmp(field[i].name, name)) {
-			ret = field[i].type;
-			break;
-		}
-	}
-
-	return ret;
-}
-
-static cfg_type_t * ATTR_NONNULLS ATTR_CHECKRESULT
-get_type_from_clause(const cfg_clausedef_t *clause, const char *name)
-{
-	cfg_type_t *ret = NULL;
-
-	REQUIRE(clause != NULL);
-	REQUIRE(name != NULL);
-
-	for (int i = 0; clause[i].name != NULL; i++) {
-		if (!strcmp(clause[i].name, name)) {
-			ret = clause[i].type;
-			break;
-		}
-	}
-
-	return ret;
-}
-
-static cfg_type_t * ATTR_NONNULLS ATTR_CHECKRESULT
-get_type_from_clause_array(const cfg_type_t *cfg_type, const char *name)
-{
-	cfg_type_t *ret = NULL;
-	const cfg_clausedef_t **clauses;
-
-	REQUIRE(cfg_type != NULL && cfg_type->of != NULL);
-	REQUIRE(name != NULL);
-
-	clauses = (const cfg_clausedef_t **)cfg_type->of;
-	for (int i = 0; clauses[i] != NULL; i++) {
-		ret = get_type_from_clause(clauses[i], name);
-		if (ret != NULL)
-			break;
-	}
-
-	return ret;
-}
-
-static void
-init_cfgtypes(void)
-{
-	cfg_type_t *zoneopts;
-
-	zoneopts = _type_namedconf;
-	zoneopts = get_type_from_clause_array(zoneopts, "zone");
-	zoneopts = get_type_from_tuplefield(zoneopts, "options");
-
-	update_policy = get_type_from_clause_array(zoneopts, "update-policy");
-	allow_query = get_type_from_clause_array(zoneopts, "allow-query");
-	allow_transfer = get_type_from_clause_array(zoneopts, "allow-transfer");
-	forwarders = get_type_from_clause_array(zoneopts, "forwarders");
-}
-
-static isc_result_t 

Re: [Freeipa-devel] [PATCHES 0146-0152] Server Roles v2

[Martin Babinsky]

On 06/03/2016 05:25 PM, Martin Babinsky wrote:

I am sending rebased patches implementing
http://www.freeipa.org/page/V4/Server_Roles

I hope the patches work since I have had a lot of fun rebasing them on
top of thin client and DNS locations effort.

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





Sending updated patches according to Jan's interactive review.

Since the name of attributes returned by API commands and signature of 
`server-role-find` have changed, a small update in WebUI patches is 
required.


--
Martin^3 Babinsky
From e9c3d6887551782ef5e02d1c57ddb321540b6d10 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 26 May 2016 19:24:22 +0200
Subject: [PATCH 1/7] Server Roles: definitions of server roles and attributes

This patch introduces classes which define the properties of server roles and
attributes and their relationship to LDAP attributes representing the
role/attribute.

A brief documentation about defining and using roles is given at the beginning
of the module.

http://www.freeipa.org/page/V4/Server_Roles
https://fedorahosted.org/freeipa/ticket/5181
---
 ipaserver/servroles.py | 581 +
 1 file changed, 581 insertions(+)
 create mode 100644 ipaserver/servroles.py

diff --git a/ipaserver/servroles.py b/ipaserver/servroles.py
new file mode 100644
index ..2972e54310f9128c055526fbc6b933118039198f
--- /dev/null
+++ b/ipaserver/servroles.py
@@ -0,0 +1,581 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license
+#
+
+
+"""
+This module contains the set of classes which abstract various bits and pieces
+of information present in the LDAP tree about functionalities such as DNS
+server, Active Directory trust controller etc. These properties come in two
+distinct groups:
+
+server roles
+this group represents a genral functionality provided by one or more
+IPA servers, such as DNS server, certificate authority and such. In
+this case there is a many-to-many mapping between the roles and the
+masters which provide them.
+
+server attributes
+these represent a functionality associated with the whole topology,
+such as CA renewal master or DNSSec key master.
+
+See the corresponding design page (http://www.freeipa.org/page/V4/Server_Roles)
+for more info.
+
+Both of these groups use `LDAPBasedProperty` class as a base.
+
+Server Roles
+
+
+Server role objects are usually consuming information from the master's service
+container (cn=FQDN,cn=masters,cn=ipa,cn=etc,$SUFFIX) are represented by
+`ServiceBasedRole`class. To create an instance of such role, you only need to
+specify role name and individual services comprising the role (more systemd
+services may be enabled to provide some function):
+
+>>> example_role = ServiceBasedRole(
+... "Example Role",
+... component_services = ['SERVICE1', 'SERVICE2'])
+>>> example_role.name
+'Example Role'
+
+The role object can then be queried for the status of the role in the whole
+topology or on a single master by using its `status` method. This method
+returns a list of dictionaries akin to LDAP entries comprised from server name,
+role name and role status (enabled if role is enabled, configured if the
+service entries are present but not marked as enabled by 'enabledService'
+config string, absent if the service entries are not present).
+
+Note that 'AD trust agent' role is based on membership of the master in the
+'adtrust agents' sysaccount group and is thus an instance of different class
+(`ADTrustBasedRole`). This role also does not have 'configured' status, since
+the master is either member of the group ('enabled') or not ('absent')
+
+Server Attributes
+=
+
+Server attributes are implemented as instances of `ServerAttribute` class. The
+attribute is defined by some flag set on 'ipaConfigString' attribute of some
+service entry. To create your own server attribute, see the following example:
+
+>>> example_attribute = ServerAttribute("Example Attribute", example_role,
+... "SERVICE1", "roleMaster")
+>>> example_attribute.name
+'Example Attribute'
+
+The FQDN of master with the attribute set can be requested using `get()`
+method. The attribute master can be changed by the `set()` method
+which accepts FQDN of a new master hosting the attribute.
+
+The available role/attribute instances are stored in
+`role_instances`/`attribute_instances` dictionaries keyed by instance name.
+"""
+
+import abc
+from collections import namedtuple, defaultdict
+
+from ldap import SCOPE_ONELEVEL
+import six
+
+from ipalib import _, errors
+from ipapython.dn import DN
+
+
+if six.PY3:
+unicode = str
+
+
+ENABLED = u'enabled'
+CONFIGURED = u'configured'
+ABSENT = u'absent'
+
+
+@six.add_metaclass(abc.ABCMeta)
+class LDAPBasedProperty(object):
+"""
+base class for all master properties defined by 

Re: [Freeipa-devel] [PATCH 0042] Removed dead code from LDAPRemoveReverseMember

[Martin Basti]

On 07.06.2016 10:43, Jan Cholasta wrote:

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

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


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


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


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

Should I revert this commit then and postpone the ticket?


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




Ok

--
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] thin client regressions: otptoken

[Alexander Bokovoy]

On Tue, 07 Jun 2016, Jan Cholasta wrote:

On 7.6.2016 10:17, Alexander Bokovoy wrote:

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
  sys.exit(api.Backend.cli.run(argv))
File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
  rv = cmd.output_for_cli(self.api.Backend.textui, result, *args,
**options)
File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 103, in output_for_cli
  qr = self._get_qrcode(output, uri, options['version'])
File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 61, in _get_qrcode
  qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an
internal error has occurred


Looks rather like a py3 regression to me.


Fix attached, made a ticket https://fedorahosted.org/freeipa/ticket/5938
--
/ Alexander Bokovoy
From 398d3c0decb6b39207a2f9dcd8d27291800de1dd Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy 
Date: Tue, 7 Jun 2016 11:37:41 +0300
Subject: [PATCH 5/5] otptoken: only decode the token output for Python 2

When IPA client is using Python 3, there is no str.decode() method
anymore.

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
   sys.exit(api.Backend.cli.run(argv))
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
   rv = cmd.output_for_cli(self.api.Backend.textui, result, *args, **options)
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py", line 
103, in output_for_cli
   qr = self._get_qrcode(output, uri, options['version'])
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py", line 
61, in _get_qrcode
   qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an internal 
error has occurred

Fixes https://fedorahosted.org/freeipa/ticket/5938
---
 ipaclient/plugins/otptoken.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipaclient/plugins/otptoken.py b/ipaclient/plugins/otptoken.py
index 5d3f1f8..968b01e 100644
--- a/ipaclient/plugins/otptoken.py
+++ b/ipaclient/plugins/otptoken.py
@@ -58,7 +58,9 @@ class otptoken_add(MethodOverride):
 encoding = locale.getpreferredencoding(False)
 
 try:
-qr_code = qr_output.getvalue().decode(encoding)
+qr_code = qr_output.getvalue()
+if not six.PY3:
+qr_code = qr_code.decode(encoding)
 except UnicodeError:
 add_message(
 version,
-- 
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 0042] Removed dead code from LDAPRemoveReverseMember

[Jan Cholasta]

On 7.6.2016 10:22, Martin Basti wrote:



On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

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


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


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


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

Should I revert this commit then and postpone the ticket?


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


--
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 0497] Py3: fix unicode/str error in LDAP*ReverseMember

[Martin Basti]

On 07.06.2016 10:35, Jan Cholasta wrote:

On 7.6.2016 10:29, Martin Basti wrote:



On 07.06.2016 09:08, Jan Cholasta wrote:

On 6.6.2016 14:33, Martin Basti wrote:

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

Patch attached.


Could we drop the error message parsing and do something sane instead?



Not now, we can do it later and push this patch just as workaround


What's the point of that?

Point is that it will work as before, and I don't have to time to fix it 
nicely now.


--
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 0497] Py3: fix unicode/str error in LDAP*ReverseMember

[Jan Cholasta]

On 7.6.2016 10:29, Martin Basti wrote:



On 07.06.2016 09:08, Jan Cholasta wrote:

On 6.6.2016 14:33, Martin Basti wrote:

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

Patch attached.


Could we drop the error message parsing and do something sane instead?



Not now, we can do it later and push this patch just as workaround


What's the point of that?

--
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 0497] Py3: fix unicode/str error in LDAP*ReverseMember

[Martin Basti]

On 07.06.2016 09:08, Jan Cholasta wrote:

On 6.6.2016 14:33, Martin Basti wrote:

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

Patch attached.


Could we drop the error message parsing and do something sane instead?



Not now, we can do it later and push this patch just as workaround

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

Re: [Freeipa-devel] [PATCH] 0042: Fix bad searching of reverse DNS zone

[Pavel Vomacka]

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

Hi,

the commit message does not say what was wrong and why and what works now.
Please improve the commit message before pushing this.

Commit message improved.


Petr^2 Spacek

On 6.6.2016 19:03, Pavel Vomacka wrote:

Fix bad searching of reverse DNS zone

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

--

Pavel^3 Vomacka


From 612129e8c6d84481937afe40ef578454214fc6bc Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 6 Jun 2016 18:56:03 +0200
Subject: [PATCH] Fix bad searching of reverse DNS zone

There was a problem with finding correct DNS zone. It found a first substring match.
Therefore when there was location 0.10.10.in-addr.arpa. and 110.10.10.in-addr.arpa
the location for IP address 10.10.110.1 was the first one, which is incorrect. Now
it finds the second one, because it finds the longest match.

https://fedorahosted.org/freeipa/ticket/5796
---
 install/ui/src/freeipa/dns.js | 27 +--
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/install/ui/src/freeipa/dns.js b/install/ui/src/freeipa/dns.js
index 8bfc23a1ac7fdd47c917091126f6298e323f1dcf..3d384184aa0f4349ae5fdce2673b0803baa3777a 100644
--- a/install/ui/src/freeipa/dns.js
+++ b/install/ui/src/freeipa/dns.js
@@ -2263,28 +2263,27 @@ IPA.dns.ptr_redirection_dialog = function(spec) {
 //2nd step: find target zone
 that.find_zone = function(data) {
 var zones = data.result.result;
-var target_zone = null;
+var target_zone = {
+index: 100,
+target_zone: ''
+};
 
 for (var i=0; i -1) {
-var msg = text.get('@i18n:objects.dnsrecord.ptr_redir_zone');
-msg = msg.replace('${zone}', zone_name);
-that.append_status(msg);
+var index = that.reverse_address.indexOf(zone_name);
 
-if (!target_zone ||
-(target_zone && zone_name.length > target_zone.length)) {
-
-target_zone = zone_name;
-}
-
-break;
+if (index > -1 && target_zone.index > index) {
+target_zone.target_zone = zone_name;
 }
 }
 
-if (target_zone) {
-that.zone = target_zone;
+if (target_zone.target_zone !== '') {
+
+that.zone = target_zone.target_zone;
+var msg = text.get('@i18n:objects.dnsrecord.ptr_redir_zone');
+msg = msg.replace('${zone}', that.zone);
+that.append_status(msg);
 that.check_record();
 } else {
 that.append_status(text.get('@i18n:objects.dnsrecord.ptr_redir_zone_err'));
-- 
2.5.5

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

Re: [Freeipa-devel] [PATCH 0042] Removed dead code from LDAPRemoveReverseMember

[Martin Basti]

On 07.06.2016 09:07, Jan Cholasta wrote:

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

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


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


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


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


Should I revert this commit then and postpone the ticket?

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] thin client regressions: otptoken

[Jan Cholasta]

On 7.6.2016 10:17, Alexander Bokovoy wrote:

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
   sys.exit(api.Backend.cli.run(argv))
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
   rv = cmd.output_for_cli(self.api.Backend.textui, result, *args,
**options)
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 103, in output_for_cli
   qr = self._get_qrcode(output, uri, options['version'])
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py",
line 61, in _get_qrcode
   qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an
internal error has occurred


Looks rather like a py3 regression to me.

--
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] thin client regressions: otptoken

[Alexander Bokovoy]

ipa: ERROR: AttributeError: 'str' object has no attribute 'decode'
Traceback (most recent call last):
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1345, in run
   sys.exit(api.Backend.cli.run(argv))
 File "/usr/lib/python3.5/site-packages/ipalib/cli.py", line 1110, in run
   rv = cmd.output_for_cli(self.api.Backend.textui, result, *args, **options)
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py", line 
103, in output_for_cli
   qr = self._get_qrcode(output, uri, options['version'])
 File "/usr/lib/python3.5/site-packages/ipaclient/plugins/otptoken.py", line 
61, in _get_qrcode
   qr_code = qr_output.getvalue().decode(encoding)
AttributeError: 'str' object has no attribute 'decode' ipa: ERROR: an internal 
error has occurred



--
/ Alexander Bokovoy

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

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

[Stanislav Laznicka]

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

From 8ba87072d8e998ccb8743390eb541e74f6b1aa96 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 10:08:45 +0200
Subject: [PATCH] Uninstaller won't fail if service can't be started

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

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index afcb6b0c10e54b1aae975fd1e81f37144e6b97ed..928957594098563c89ed905946933110e92b0bb5 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -1269,7 +1269,11 @@ class BindInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 self.named_regular.unmask()
 if named_regular_enabled:
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index 00f890175ae583f485797da6f913a7f83b302df3..3dc115c225316362957e78ce4a6b12e59844edf9 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -549,7 +549,11 @@ class HTTPInstance(service.Service):
 self.print_msg('WARNING: ' + str(e))
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 # disabled by default, by ldap_enable()
 if enabled:
diff --git a/ipaserver/install/krbinstance.py b/ipaserver/install/krbinstance.py
index f560a6ec4c2e4ce931cc1552976db5900a3fa5cd..b3c742ece6974931c8c918fb2742ec7c518a490b 100644
--- a/ipaserver/install/krbinstance.py
+++ b/ipaserver/install/krbinstance.py
@@ -405,7 +405,11 @@ class KrbInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 self.kpasswd = KpasswdInstance()
 self.kpasswd.uninstall()
diff --git a/ipaserver/install/ntpinstance.py b/ipaserver/install/ntpinstance.py
index 8b0f0e5395dae3c058fc31bd8914741e4d158830..9bac7b75130dbf592598e1e2a4a64c62ff5858f0 100644
--- a/ipaserver/install/ntpinstance.py
+++ b/ipaserver/install/ntpinstance.py
@@ -183,4 +183,8 @@ class NTPInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
diff --git a/ipaserver/install/odsexporterinstance.py b/ipaserver/install/odsexporterinstance.py
index e9f7bf853d98237aa19aace384b8ff7021c3a85a..089372b13414a74b8b90566223662fa56d28ec7f 100644
--- a/ipaserver/install/odsexporterinstance.py
+++ b/ipaserver/install/odsexporterinstance.py
@@ -193,7 +193,11 @@ class ODSExporterInstance(service.Service):
 signerd_service.enable()
 
 if signerd_running:
-signerd_service.start()
+try:
+signerd_service.start()
+except Exception as e:
+root_logger.error("Unable to start {sname}: {err}"
+  .format(sname=self.service_name, err=e))
 
 installutils.remove_keytab(paths.IPA_ODS_EXPORTER_KEYTAB)
 installutils.remove_ccache(ccache_path=paths.IPA_ODS_EXPORTER_CCACHE)
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index f0c512ba04129d08b5874f58c7a25620f7435b2a..4c2c4145240f030ce8946205faea336a6f462f9f 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -386,4 +386,8 @@ class OpenDNSSECInstance(service.Service):
 self.enable()
 
 if running:
-self.restart()
+try:
+self.restart()
+except Exception as e:
+root_logger.error("Unable to restart {sname}: {err}"
+  .format(sname=self.service_name, err=e))
diff 

Re: [Freeipa-devel] [PATCH] 0039-40: DNS Location: WebUI

[Pavel Vomacka]

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




On 05.06.2016 18:34, Pavel Vomacka wrote:

Hello,

please review attached patches which add WebUI part of DNS Locations 
feature.


--
Pavel^3 Vomacka




NACK

1)
When I edit location description and click on revert button, then that 
nice location table just disappear :)
It's the same situation as with using 'Save' button - reported here: 
https://fedorahosted.org/freeipa/ticket/5776 . I'll write there a 
comment that revert button hides values in association tables.


2)
Can we put a placeholder "100" (gray font or something) to Location 
weight in server detail view? Because when weight is not specified 
then default is 100

Placeholder added.

--
Pavel^3



Martin^2


From e23d35737161ddf0cb2c5f4caec69efe826f3924 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Sun, 5 Jun 2016 18:03:17 +0200
Subject: [PATCH 1/2] Add adapter attribute for choosing record

The new attribute of the adapter contains the name of record which will be
extracted from API call result.

Part of: https://fedorahosted.org/freeipa/ticket/5905
---
 install/ui/src/freeipa/field.js | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/install/ui/src/freeipa/field.js b/install/ui/src/freeipa/field.js
index fdf925e3d7adc62622e0c9e5e1440eefbdac807b..12ef7f45585802c4099e469c9fb5188b3a3587ab 100644
--- a/install/ui/src/freeipa/field.js
+++ b/install/ui/src/freeipa/field.js
@@ -802,6 +802,13 @@ field.Adapter = declare(null, {
 result_index: 0,
 
 /**
+ * Name of the record which we want to extract from the result.
+ * Used in dnslocations.
+ * @type {String}
+ */
+result_name: 'result',
+
+/**
  * Extract record from RPC call response
  *
  * Tries to detect if supplied data is RPC call response if so, it
@@ -821,10 +828,10 @@ field.Adapter = declare(null, {
 var dr = data.result;
 var record = null;
 if (dr) {
-if (IPA.defined(dr.result)) record = dr.result;
+if (IPA.defined(dr[this.result_name])) record = dr[this.result_name];
 else if (dr.results) {
 var result = dr.results[this.result_index];
-if (result) record = result.result;
+if (result) record = result[this.result_name];
 }
 }
 return record;
-- 
2.5.5

From 4bc33370ec97a8f4a149f7aef05a6a89a58680a4 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Sun, 5 Jun 2016 18:07:29 +0200
Subject: [PATCH 2/2] DNS Locations: WebUI part

WebUI part of DNS Location feature.

https://fedorahosted.org/freeipa/ticket/5905
---
 install/ui/src/freeipa/navigation/menu_spec.js |   5 +
 install/ui/src/freeipa/topology.js | 244 -
 install/ui/test/data/ipa_init.json |   3 +-
 ipaserver/plugins/internal.py  |   1 +
 4 files changed, 249 insertions(+), 4 deletions(-)

diff --git a/install/ui/src/freeipa/navigation/menu_spec.js b/install/ui/src/freeipa/navigation/menu_spec.js
index fb64ccaea56cabdb6addb28543d3f968a746018b..0afc7daceb725cee5677982c68c09d1666d42885 100644
--- a/install/ui/src/freeipa/navigation/menu_spec.js
+++ b/install/ui/src/freeipa/navigation/menu_spec.js
@@ -226,6 +226,11 @@ var nav = {};
 hidden: true
 },
 {
+entity: 'location',
+facet: 'search',
+hidden: true
+},
+{
 facet: 'topology-graph',
 hidden: true
 }
diff --git a/install/ui/src/freeipa/topology.js b/install/ui/src/freeipa/topology.js
index c26dc685328904ed893c960b6b4b0981cd9d28ce..cea164cb457d60a0b2f2f0d0976ba16b06b19a88 100644
--- a/install/ui/src/freeipa/topology.js
+++ b/install/ui/src/freeipa/topology.js
@@ -51,7 +51,8 @@ var topology = IPA.topology = {
 suffix_search: 'topologysuffix_search',
 server_search: 'server_search',
 domainlevel: 'domainlevel_details',
-topologygraph: 'topology-graph'
+topologygraph: 'topology-graph',
+location_search: 'location_search'
 }
 }
 };
@@ -193,6 +194,14 @@ return {
 var make_server_spec = function() {
 return {
 name: 'server',
+policies: [
+{
+$factory: IPA.facet_update_policy,
+source_facet: 'details',
+dest_entity: 'location',
+dest_facet: 'details'
+}
+],
 facets: [
{
 $type: 'search',
@@ -212,7 +221,6 @@ return {
 },
 {
 $type: 'details',
-no_update: true,
 disable_facet_tabs: true,
 sections: [
 {
@@ -221,7 +229,18 @@ return {
 { name: 'cn', 

Re: [Freeipa-devel] [PATCH] 0042: Fix bad searching of reverse DNS zone

[Petr Spacek]

Hi,

the commit message does not say what was wrong and why and what works now.
Please improve the commit message before pushing this.

Petr^2 Spacek

On 6.6.2016 19:03, Pavel Vomacka wrote:
> Fix bad searching of reverse DNS zone
> 
> https://fedorahosted.org/freeipa/ticket/5796
> 
> -- 
> 
> Pavel^3 Vomacka

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

Re: [Freeipa-devel] [PATCH 0497] Py3: fix unicode/str error in LDAP*ReverseMember

[Jan Cholasta]

On 6.6.2016 14:33, Martin Basti wrote:

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

Patch attached.


Could we drop the error message parsing and do something sane instead?

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

[Jan Cholasta]

On 6.6.2016 18:29, Martin Basti wrote:



On 03.06.2016 14:28, Stanislav Laznicka wrote:

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


On 03.06.2016 14:13, Stanislav Laznicka wrote:

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



NACK

please remove it from LDAPAddReverseMember too, it contains the same
code

Martin^2


Please see the modified patch.

Standa


ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc


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


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

[thierry bordaz]

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




On 03.06.2016 13:38, Stanislav Laznicka wrote:

Hello,

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


Standa




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

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


Martin^2

Hello,

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


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


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