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

[Martin Babinsky]

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