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.
Petr^2 Spacek
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
