On 06/07/2016 07:35 PM, Alexander Bokovoy wrote:
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.

1.) there is one unused import that makes pylint choke:

"""
************* Module com.redhat.idm
install/oddjob/com.redhat.idm.trust-fetch-domains:6: [W0611(unused-import), ] Unused errors imported from ipalib)
Makefile:137: recipe for target 'lint' failed
make: *** [lint] Error 1
"""

2.) The UPN suffixes are added during trust establishment and I can also kinit as the enterprise principal using one of UPNs, but only when using two-way trust. Is this expected? I was not able to find any clue in the design page but maybe I was just not looking hard enough.

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

Reply via email to