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

Wed Aug 17 11:22:41 UTC 2016

On Wed, 17 Aug 2016, Martin Babinsky wrote:
>On 08/17/2016 12:41 PM, 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 for separate patch, thank you for your explanation.
No need for separate patch -- see my explanation in the other email.
-- 
/ Alexander Bokovoy