[Freeipa-devel] [PATCH] 0207, 0218-0219 Solving trust conflicts and external trust topology fixes

[Alexander Bokovoy]

On Wed, 17 Aug 2016, Petr Spacek wrote:
>On 17.8.2016 12:41, Alexander Bokovoy wrote:
>> On Wed, 17 Aug 2016, Martin Babinsky wrote:
>>> On 08/15/2016 06:06 PM, Alexander Bokovoy wrote:
>>>> On Mon, 15 Aug 2016, Alexander Bokovoy wrote:
>>>>> Hi!
>>>>>
>>>>> Attached are trust-related patches.
>>>>>
>>>>> 0207 is a pre-requisite. I did send it before, it is re-formatting of
>>>>> the ipaserver/dcerpc.py to be close to PEP8 requirements.
>>>>>
>>>>> 0218 is an automated trust topology conflict resolver for DNS namespace
>>>>> conflicts. Read the commit message for details, and also comments in the
>>>>> code. With this patch FreeIPA becomes more smart than Windows Server
>>>>> which doesn't have automated trust topology conflict resolution. ;)
>>>>>
>>>>> 0219 fixes issue of topology details leaking through external trust. The
>>>>> problem is only in presentation as we store more data than needed -- it
>>>>> is impossible to cross external trust boundary anyway as it is handled
>>>>> by AD DC side, but one important consequence is that we need to store
>>>>> UPN suffixes before we start storing information about child domains.
>>>>> Again, read the commit message.
>>>>>
>>>>> These patches also are on top of my previously sent patches 0215-0216.
>>>> Patches attached.
>>>>
>>>>
>>>>
>>>
>>> Hi Alexander,
>>>
>>> patch 207: LGTM, but I have a feeling that the patch should be linked to
>>> both #6021 and #6076 so that it is not lost during backports.
>>>
>>> patch 218:
>>>
>>> ipalib/errors.py:
>>>
>>> 1.)
>>> I'm not sure if TrustTopologyConflictError should inherit from
>>> InvocationError. The semantics of InvocationError implies that something was
>>> wrong when trying to invoke the command (a param failed to validate/convert,
>>> incorrect number of args, etc.), while this is more of an exception during
>>> command execution (no. and type of params was correct, command started to
>>> execute but encountered an error condition). Thus I think it should inherit
>>> from ExecutionError. CC'ing Jan for more thoughts on this.
>>>
>>> 2.)
>>>
>>> Why is TrustTopologyConflictSolved listed amogn public errors? Since it is
>>> used only in dcerpc.py to restart trust establishment after resolving
>>> conflicts, it should be a private exception in dcerpc.py for this purpose.
>>>
>>> 3.)
>>> Also please split the exception format string like this so that the line is
>>> not too long (there is not much we can do about doctest so leave that line
>>> as it is):
>>>
>>> @@ -882,7 +882,8 @@ class TrustTopologyConflictError(InvocationError):
>>>     """
>>>
>>>     errno = 3017
>>> -    format = _("Forest '%(forest)s' has existing trust to forest(s)
>>> %(domains)s which prevents a trust to '%(conflict)s'")
>>> +    format = _("Forest '%(forest)s' has existing trust to forest(s) "
>>> +               "%(domains)s which prevents a trust to '%(conflict)s'")
>>>
>>> Do not worry about gettext, it can handle it just fine, there are plenty of
>>> examples in server plugins, for example.
>>>
>>>
>>> ipaserver/dcerpc.py:
>>>
>>> 1.)
>>>
>>> I think that instead of returning result and raising
>>> TrustTopologyConflictError based on that, the 'clear_ftinfo_conflict' can
>>> raise this exception directly. You can have an empty list defined at the
>>> beginning instead of 'result list', append unresolvable conflicts to it and
>>> then at the end of the method check if it is non-empty and raise the exception.
>>>
>>> 2.)
>>>
>>> +        # In the code below:
>>> +        # self -- the forest we establish trust to
>>> +        # another_domain -- a forest that establishes trust to 'self'
>>> +        # cinfo -- lsa_ForestTrustCollisionInfo structure that contain
>>> +        #          set of of lsa_ForestTrustCollisionRecord structures
>>> I would add this directly into the method docstring:
>>>
>>> """
>>> ...
>>>
>>> :param self: the forest we establish trust to
>>> :param another_domain: a forest that establishes trust to 'self'
>>> :param cinfo: lsa_ForestTrustCollisionInfo structure that contain
>>>           set of of lsa_ForestTrustCollisionRecord structures
>>> """
>>>
>>> Additionally, the behavior specifed in previous comment can be added using
>>> :raises: stanza:
>>>
>>> """
>>> :raises: errors.TrustTopologyConflictError if there are unresolvable
>>>    namespace conflicts between trusted domains
>>> """
>>>
>>> 3.)
>>>
>>> rewriting 'clear_ftinfo_conflict' according to point 1.) will allow to
>>> simplify code in 'update_ftinfo' like this:
>>>
>>> """
>>> -                res = self.clear_ftinfo_conflict(another_domain, cinfo)
>>> -                if len(res[1]) != 0:
>>> -                    domains = [x.name.string for x in res[1]]
>>> -                    raise errors.TrustTopologyConflictError(
>>> -                              target=self.info['dns_domain'],
>>> -                              conflict=another_domain.info['dns_domain'],
>>> -                              domains=domains)
>>> -                else:
>>> -                    raise errors.TrustTopologyConflictSolved(
>>> -                              target=self.info['dns_domain'],
>>> -                              conflict=another_domain.info['dns_domain'])
>>> +                self.clear_ftinfo_conflict(another_domain, cinfo)
>>> +                raise errors.TrustTopologyConflictSolved(
>>> +                    target=self.info['dns_domain'],
>>> +                    conflict=another_domain.info['dns_domain'])
>>> """
>>>
>>> Patch 218:
>>>
>>> 1.)
>>> typo in the commit message:
>>>
>>> """
>>> ...
>>> suffixes are forest-wide, there *are could be* user accounts in the
>>> ...
>>> """
>>>
>>> 2.)
>>> A stupid question I know, but why do we need to replace empty string with
>>> None here?
>>>
>>> """
>>>         # with the forest, thus we have to use
>>> netr_DsRGetForestTrustInformation
>>> -        domains =
>>> netr_pipe.netr_DsRGetForestTrustInformation(td.info['dc'], '', 0)
>>> +        domains =
>>> netr_pipe.netr_DsRGetForestTrustInformation(td.info['dc'], None, 0)
>>> """
>> I'm answering this one while working on the other fixes.
>>
>> netr_DsRGetForestTrustInformation() uses NULL to say a parameter is
>> missing, not empty string. It actually causes an issue if we don't do
>> that when asking a domain controller from a domain that is not a root
>> domain of the forest where we get currently undocumented error
>> NERR_ACFNotLoaded (0x000008b3). If non-NULL name is specified, AD domain
>> controller will perform proper operation only if it is a domain
>> controller from the forest root domain and the domain specified is
>> known. Obviously, a domain named '' cannot be known. This breaks external trust.
>>
>> Perhaps, this could be moved into a separate patch on its own.
>+1
>
>You already wrote commit message for it to the e-mail so it is almost for free
>:-) This really should be documented somewhere.
Uhm, no. The separate patch is there -- that change of '' to None is
part of the proper patch 0219. When I wrote the message above I was
thinking it is actually a part of 0218. Silly me.

So no changes, I did add a small sentence to the commit message in new
version of 0219 which refers to the MS-NRPC spec.
-- 
/ Alexander Bokovoy