[Freeipa-devel] [PATCH] 0118 add support for subdomains

[Jan Cholasta]

On 28.9.2013 22:01, Alexander Bokovoy wrote:
> On Fri, 27 Sep 2013, Sumit Bose wrote:
>> On Fri, Sep 27, 2013 at 03:53:08PM +0300, Alexander Bokovoy wrote:
>>> On Mon, 23 Sep 2013, Alexander Bokovoy wrote:
>>> >On Mon, 23 Sep 2013, Alexander Bokovoy wrote:
>>> >>On Mon, 23 Sep 2013, Alexander Bokovoy wrote:
>>> >>>On Mon, 23 Sep 2013, Martin Kosek wrote:
>>> >>>>>>However, we don't have trust type available so it needs to
>>> discovered
>>> >>>>>>every time. This doesn't play well with the framework, it is
>>> simply not
>>> >>>>>>expecting dynamic containers.
>>> >>>>>
>>> >>>>>This doesn't sound like a big obstacle to me. Right now the
>>> trust_type lookup
>>> >>>>>is done in trust_show.execute() for some reason, which is not
>>> the best place to
>>> >>>>>do it IMHO. Doing it in trust.get_dn() instead should simplify
>>> things enough to
>>> >>>>>make parent_object work.
>>> >>>>
>>> >>>>Yup, get_dn() is the method where object DN lookup should be
>>> done. See for
>>> >>>>example host.py plugin get_dn method, we also do a dynamic lookup
>>> for correct
>>> >>>>host name.
>>> >>>I'll see if that would work.
>>> >>>
>>> >>>>the best way to implement dynamic DN gathering is the get_dn()
>>> method. That
>>> >>>>way, it could be implemented in one place and all commands could
>>> take advantage
>>> >>>>of it instead of re-implementing it several times in pre_callback
>>> - this is
>>> >>>>just hackish.
>>> >>>I'd suggest you look into the code. The commands use pre_callback
>>> for a
>>> >>>different purpose than implementing dynamic DN gathering.
>>> >>>
>>> >>>>I think it would have been very useful to have a design page
>>> before sending a
>>> >>>>patch. It is then easier to make design decisions without having
>>> to dig into
>>> >>>>the patch.
>>> >>>The design page is there for long time:
>>> >>>http://www.freeipa.org/page/V3/Transitive_Trusts
>>> >>Ok, here is new version of the patch and updated version of my 0117
>>> >>patch as Sumit noticed I've sent wrong version.
>>> >Ok, here is updated 0118 which fixes API.txt change for trustdomain_add
>>> >-- I renamed trustdomain_create to trustdomain_add but forgot to rerun
>>> >makeapi.
>>> New edition attached for all subdomain-related patches:
>>
>> I did some tests and all is working as expected.
>>
>>>
>>> freeipa-abbra-0117-ipaserver-dcerpc.py-populate-forest-trust-informatio-3.patch
>>>
>>>   Use realmdomains to report name suffix routes at the time we
>>> establish trust
>>>
>>> freeipa-abbra-0118-trusts-support-subdomains-in-a-forest-3.patch
>>>   Introduce trustdomain-* commands to fetch list of domains associated
>>>   with a forest trust and allow filtering them off
>>
>> We talked on irc that ipaNTSupportedEncryptionTypes in the filter
>> for the trusted domains should be replace by a different attribute.
>> Because of an error in ipasam the ipaNTSupportedEncryptionTypes is only
>> set in recent versions and might not be present in the directory trees of
>> older versions.
> Fixed in the attached patch 0118 version 4.
>
> Also attached first attempt to implement transiting through trusted
> domains, as patch 0123. In this patch we grant transition only if all
> three realms (client, transited realm, and server realm) match any of
> our trusted domains and our domain. This is probably a bit wider but it
> worked for me bidirectionally, from a child domain to a service in IPA,
> and from IPA realm to a service in a child domain of a forest trust.
>

+        if not trust_type:
+            for ttype in self.trust_types:
+                dn=make_trust_dn(self.env, ttype, DN(*sdn))
+                try:
+                    object_exists = self.backend.get_entry(dn, [''])
+                    return object_exists.dn
+                except errors.NotFound:
+                    pass
+            # if no trust object of any type exist, default to 'ad'
+            # this is required for *_add calls.
+            trust_type = u'ad'

This could be optimized to do a single LDAP search, i.e. something like:

         if trust_type is None:
             ldap = self.backend
             filter = ldap.make_filter({'objectclass': 
['ipaNTTrustedDomain'], 'cn': [keys[-1]]})
             try:
                 result = ldap.get_entries(base_dn, ldap.SCOPE_SUBTREE, 
filter, [''])
             except errors.NotFound:
                 trust_type = u'ad'
             else:
                 if len(result) > 1:
                     raise errors.OnlyOneValueAllowed(attr='trust_type')
                 return result[0].dn


+        # First see if the trust is already in place
+        # Force retrieval of the trust object by not passing trust_type
+        dn = self.obj.get_dn(keys[-1], {})

The "{}" does not seem right to me (you are passing it as a second key 
to get_dn, which is probably not what you want).


              (dn, attrs) = entry
-
+
              # Translate ipanttrusttype to trusttype if --raw not used

No trailing whitespace please.


+    takes_options = LDAPCreate.takes_options + (_trust_type_option,
+        Str('ipanttrustpartner?',
+            label=_('Trusted domain partner'),
+        ),
+    )
+
+    def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, 
**options):
+        if 'ipanttrustpartner' in options:
+            entry_attrs['ipanttrustpartner'] = 
[options['ipanttrustpartner']]
+        return dn

Why is ipanttrustpartner a trustdomain_add option rather than 
trustdomain virtual attribute?


+class trustdomain_fetch(LDAPRetrieve):

I know I caused some confusion here, but I *do* think that this single 
command should be in the trust namespace, as it is actually method of 
the trust object (it takes a trust object name argument and creates 
trustdomain objects for all the subdomains in the trust, right?) I would 
suggest "trust_domain_fetch" or "trust_fetch_domains".


+class trustdomain_filter(LDAPRetrieve):

It seems this command toggles the blacklist state of a trustdomain 
object. AFAIK we don't have any toggle command in IPA and I am opposed 
to the idea of introducing them - it's better to be explicit than 
implicit. I think two commands would be better for this 
("trustdomain_enable" and "trustdomain_disable" perhaps?)


Honza

-- 
Jan Cholasta