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

Wed Aug 17 10:45:39 UTC 2016

On Wed, 17 Aug 2016, Martin Babinsky wrote:
>On 08/17/2016 12:13 PM, 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)
>>"""
>>
>
>Also I just noticed some pylint errors when doing build:
>
>Pylint is running, please wait ...
>************* Module ipaserver.dcerpc
>ipaserver/dcerpc.py:58: [E0401(import-error), ] Unable to import 
>'pysss_nss_idmap')
>ipaserver/dcerpc.py:59: [E0401(import-error), ] Unable to import 'pysss')
>ipaserver/dcerpc.py:1202: [E1305(too-many-format-args), 
>TrustDomainInstance.clear_ftinfo_conflict] Too many arguments for 
>format string)
>ipaserver/dcerpc.py:1207: [E0602(undefined-variable), 
>TrustDomainInstance.clear_ftinfo_conflict] Undefined variable 
>'conflict_type')
>Makefile:137: recipe for target 'pylint' failed
>
>You can probably ignore the first two since they may be caused by my 
>broken build environment.
Right, conflict_type[rec.type] is a left-over I had originally to choose
the type but since then decided to make two different error logging for
in-forest and external to forest cases.

Thanks.
-- 
/ Alexander Bokovoy