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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky