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

[Alexander Bokovoy]

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.

>
>-- 
>Martin^3 Babinsky

-- 
/ Alexander Bokovoy