On Tue, 07 Jun 2016, Martin Babinsky wrote:
On 06/06/2016 12:33 PM, Alexander Bokovoy wrote:
Hi,

this patch adds support for external trust to Active Directory.

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743



Hi Alexander,

I have a few comments:

1.) there are numerous pep8 errors in the code:

"""
./ipaserver/dcerpc.py:1044:80: E501 line too long (115 > 79 characters)
./ipaserver/dcerpc.py:1044:106: E251 unexpected spaces around keyword / parameter equals ./ipaserver/dcerpc.py:1044:108: E251 unexpected spaces around keyword / parameter equals
./ipaserver/dcerpc.py:1107:80: E501 line too long (110 > 79 characters)
./ipaserver/dcerpc.py:1108:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (114 > 79 characters)
./ipaserver/dcerpc.py:1111:80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1400:80: E501 line too long (143 > 79 characters)
./ipaserver/dcerpc.py:1401:80: E501 line too long (88 > 79 characters)
./ipaserver/plugins/trust.py:66:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:174:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:175:80: E501 line too long (106 > 79 characters)
./ipaserver/plugins/trust.py:201:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:209:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:690:9: E124 closing bracket does not match visual indentation ./ipaserver/plugins/trust.py:694:80: E501 line too long (127 > 79 characters)
"""

Please fix them. You can check whether your commit does not break pep8 by running `git show HEAD -U0 | pep8 --diff`.
Thanks for the review.

The lines will have to be long. Sorry, we are not in the age of
80-column terminals anymore. Try to look at the ipaserver/dcerpc.py from
a bigger perspective -- there are currently 224 errors reported by pep8
in this file. Fixing some of them makes little sense -- for example,
pylint hints have to be where they are and that makes lines long, error
messages have to be what they are and changing them to be smaller is
counterproductive.

I'm updated the patch to have most of the lines within 90 columns.


2.)

I have difficulty understanding the following code:

"""
+        self.__allow_behavior = 0

        domain_validator = DomainValidator(api)
        self.configured = domain_validator.is_configured()

        if self.configured:
            self.local_flatname = domain_validator.flatname
            self.local_dn = domain_validator.dn
            self.__populate_local_domain()

+    def allow_behavior(self, behavior_set):
+        if type(behavior_set) is int:
+           behavior_set = [behavior_set]
+        if TRUST_JOIN_EXTERNAL in behavior_set:
+           self.__allow_behavior |= TRUST_JOIN_EXTERNAL
+
"""
I assume this is made like this to accommodate setting some other behavior flags which can pop up in the future (otherwise a simple Boolean to indicate external trust should be enough), int hat case I would propose to rewrite it in this form and document it:


"""
def allow_behavior(self, *flags):
   """
   Set the expected trust join behavior
   :param flags: trust flags to set
   """
   for flag in flags:
       self.__allow_behavior |= flag
"""

Also, I am not a big fan of doubly underscored methods/variables since they do not 'hide' anything (Python's name mangling can be bypassed with ease anyway). If you want to indicate that the 'allow_behavior' attribute belongs to the internal implementation of the class prefix it with single underscore.
You cannot have a property and a method named the same in the same
class. I changed the code to follow your other suggestion. I kept the
__allow_behavior as it is, though, because there is no difference
between __ and _ semantically to a person who would be reading the code
in question but I keep __ memorized. ;)


3.)

"""
if self.remote_domain.info['dns_domain'] != self.remote_domain.info['dns_forest']: - raise errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], domain=self.remote_domain.info['dns_domain'])
+            if not (self.__allow_behavior & TRUST_JOIN_EXTERNAL):
+ raise errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], domain=self.remote_domain.info['dns_domain'])

        if not self.remote_domain.read_only:
            trustdom_pass = samba.generate_random_password(128, 128)
            self.get_realmdomains()
- self.remote_domain.establish_trust(self.local_domain, trustdom_pass, trust_type) - self.local_domain.establish_trust(self.remote_domain, trustdom_pass, trust_type) + self.remote_domain.establish_trust(self.local_domain, trustdom_pass, trust_type, bool(self.__allow_behavior & TRUST_JOIN_EXTERNAL)) + self.local_domain.establish_trust(self.remote_domain, trustdom_pass, trust_type, bool(self.__allow_behavior & TRUST_JOIN_EXTERNAL)) # if trust is inbound, we don't need to verify it because AD DC will respond # with WERR_NO_SUCH_DOMAIN -- in only does verification for outbound trusts.
            ...

"""

Please use a separate variable to store the result of bitwise AND you pass to the methods, e.g.:

"""
join_as_external = bool(self.__allow_behavior & TRUST_JOIN_EXTERNAL)
if self.remote_domain.info['dns_domain'] != self.remote_domain.info['dns_forest']:
   ...

"""

There is enough copy-pasta we have to deal with when fixing bugs, better not add any more of that.
Thanks, changed this part.

4.)

In trust-find post-callback you can use `attrs.single_value.get('ipanttrustattributes', 0)` to get trust attributes from the LDAP entry (they haven't been converted to dicts yet). It makes code bit more readable. The same can be said for trust-show post-callback.

"""
  def post_callback(self, ldap, entries, truncated, *args, **options):
       if options.get('pkey_only', False):
            return truncated

       for attrs in entries:
            # Translate ipanttrusttype to trusttype if --raw not used
            trust_type = attrs.get('ipanttrusttype', [None])[0]
+            attributes = attrs.get('ipanttrustattributes', [0])[0]
            if not options.get('raw', False) and trust_type is not None:
- attrs['trusttype'] = trust_type_string(attrs['ipanttrusttype'][0]) + attrs['trusttype'] = [trust_type_string(trust_type, attributes)]
                del attrs['ipanttrusttype']
+                if attributes:
+                    del attrs['ipanttrustattributes']
"""
Updated patch is attached.

--
/ Alexander Bokovoy
From e9e8b4af5bf834aaceb9a7835733bd3641d5f93c Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@redhat.com>
Date: Mon, 6 Jun 2016 08:55:44 +0300
Subject: [PATCH 1/5] trusts: Add support for an external trust to Active 
 Directory domain

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743
---
 API.txt                    |  3 ++-
 ipaserver/dcerpc.py        | 50 +++++++++++++++++++++++++++----------
 ipaserver/plugins/trust.py | 61 +++++++++++++++++++++++++++++++++++-----------
 3 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/API.txt b/API.txt
index f170930..d5fbc27 100644
--- a/API.txt
+++ b/API.txt
@@ -5208,12 +5208,13 @@ arg: Str('cn', cli_name='name')
 option: Str('version?')
 output: Output('result')
 command: trust_add
-args: 1,14,3
+args: 1,15,3
 arg: Str('cn', cli_name='realm')
 option: Str('addattr*', cli_name='addattr')
 option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Int('base_id?', cli_name='base_id')
 option: Bool('bidirectional?', cli_name='two_way', default=False)
+option: Bool('external?', cli_name='external', default=False)
 option: Int('range_size?', cli_name='range_size')
 option: StrEnum('range_type?', cli_name='range_type', 
values=[u'ipa-ad-trust-posix', u'ipa-ad-trust'])
 option: Flag('raw', autofill=True, cli_name='raw', default=False)
diff --git a/ipaserver/dcerpc.py b/ipaserver/dcerpc.py
index 25ffbfa..5f56643 100644
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -77,6 +77,11 @@ and Samba4 python bindings.
 TRUST_ONEWAY        = 1
 TRUST_BIDIRECTIONAL = 3
 
+# Trust join behavior
+# External trust -- allow creating trust to a non-root domain in the forest
+TRUST_JOIN_EXTERNAL = 1
+
+
 def is_sid_valid(sid):
     try:
         security.dom_sid(sid)
@@ -1037,7 +1042,7 @@ class TrustDomainInstance(object):
             # We can ignore the error here -- setting up name suffix routes 
may fail
             pass
 
-    def establish_trust(self, another_domain, trustdom_secret, 
trust_type='bidirectional'):
+    def establish_trust(self, another_domain, trustdom_secret, 
trust_type='bidirectional', trust_external=False):
         """
         Establishes trust between our and another domain
         Input: another_domain -- instance of TrustDomainInstance, initialized 
with #retrieve call
@@ -1060,6 +1065,8 @@ class TrustDomainInstance(object):
             info.trust_direction |= lsa.LSA_TRUST_DIRECTION_OUTBOUND
         info.trust_type = lsa.LSA_TRUST_TYPE_UPLEVEL
         info.trust_attributes = 0
+        if trust_external:
+            info.trust_attributes |= lsa.LSA_TRUST_ATTRIBUTE_NON_TRANSITIVE
 
         try:
             dname = lsa.String()
@@ -1096,14 +1103,17 @@ class TrustDomainInstance(object):
             # server as that one doesn't support AES encryption types
             pass
 
-        try:
-            info = self._pipe.QueryTrustedDomainInfo(trustdom_handle, 
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX)
-            info.trust_attributes |= lsa.LSA_TRUST_ATTRIBUTE_FOREST_TRANSITIVE
-            self._pipe.SetInformationTrustedDomain(trustdom_handle, 
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX, info)
-        except RuntimeError as e:
-            root_logger.error('unable to set trust to transitive: %s' % 
(str(e)))
+        if not trust_external:
+            try:
+                info = self._pipe.QueryTrustedDomainInfo(trustdom_handle,
+                                                         
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX)
+                info.trust_attributes |= 
lsa.LSA_TRUST_ATTRIBUTE_FOREST_TRANSITIVE
+                self._pipe.SetInformationTrustedDomain(trustdom_handle,
+                                                       
lsa.LSA_TRUSTED_DOMAIN_INFO_INFO_EX, info)
+            except RuntimeError as e:
+                root_logger.error('unable to set trust transitivity status: 
%s' % (str(e)))
 
-        if self.info['is_pdc']:
+        if self.info['is_pdc'] or trust_external:
             self.update_ftinfo(another_domain)
 
     def verify_trust(self, another_domain):
@@ -1262,6 +1272,7 @@ class TrustDomainJoins(object):
         self.api = api
         self.local_domain = None
         self.remote_domain = None
+        self.__allow_behavior = 0
 
         domain_validator = DomainValidator(api)
         self.configured = domain_validator.is_configured()
@@ -1271,6 +1282,10 @@ class TrustDomainJoins(object):
             self.local_dn = domain_validator.dn
             self.__populate_local_domain()
 
+    def allow_behavior(self, *flags):
+        for f in flags:
+           self.__allow_behavior |= int(f)
+
     def __populate_local_domain(self):
         # Initialize local domain info using kerberos only
         ld = TrustDomainInstance(self.local_flatname)
@@ -1358,14 +1373,19 @@ class TrustDomainJoins(object):
                 realm_passwd
             )
 
+        trust_external = bool(self.__allow_behavior & TRUST_JOIN_EXTERNAL)
         if self.remote_domain.info['dns_domain'] != 
self.remote_domain.info['dns_forest']:
-            raise 
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], 
domain=self.remote_domain.info['dns_domain'])
+            if not trust_external:
+                raise 
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'],
+                                                 
domain=self.remote_domain.info['dns_domain'])
 
         if not self.remote_domain.read_only:
             trustdom_pass = samba.generate_random_password(128, 128)
             self.get_realmdomains()
-            self.remote_domain.establish_trust(self.local_domain, 
trustdom_pass, trust_type)
-            self.local_domain.establish_trust(self.remote_domain, 
trustdom_pass, trust_type)
+            self.remote_domain.establish_trust(self.local_domain,
+                                               trustdom_pass, trust_type, 
trust_external)
+            self.local_domain.establish_trust(self.remote_domain,
+                                              trustdom_pass, trust_type, 
trust_external)
             # if trust is inbound, we don't need to verify it because AD DC 
will respond
             # with WERR_NO_SUCH_DOMAIN -- in only does verification for 
outbound trusts.
             result = True
@@ -1381,8 +1401,12 @@ class TrustDomainJoins(object):
         if not(isinstance(self.remote_domain, TrustDomainInstance)):
             self.populate_remote_domain(realm, realm_server, realm_passwd=None)
 
+        trust_external = bool(self.__allow_behavior & TRUST_JOIN_EXTERNAL)
         if self.remote_domain.info['dns_domain'] != 
self.remote_domain.info['dns_forest']:
-            raise 
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'], 
domain=self.remote_domain.info['dns_domain'])
+            if not trust_external:
+                raise 
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'],
+                                                 
domain=self.remote_domain.info['dns_domain'])
 
-        self.local_domain.establish_trust(self.remote_domain, trustdom_passwd, 
trust_type)
+        self.local_domain.establish_trust(self.remote_domain,
+                                          trustdom_passwd, trust_type, 
trust_external)
         return dict(local=self.local_domain, remote=self.remote_domain, 
verified=False)
diff --git a/ipaserver/plugins/trust.py b/ipaserver/plugins/trust.py
index ee0ab5d..744be93 100644
--- a/ipaserver/plugins/trust.py
+++ b/ipaserver/plugins/trust.py
@@ -62,8 +62,10 @@ except Exception as e:
 
 if api.env.in_server and api.env.context in ['lite', 'server']:
     try:
-        import ipaserver.dcerpc #pylint: disable=F0401
-        from ipaserver.dcerpc import TRUST_ONEWAY, TRUST_BIDIRECTIONAL
+        import ipaserver.dcerpc  # pylint: disable=F0401
+        from ipaserver.dcerpc import (TRUST_ONEWAY,
+                                      TRUST_BIDIRECTIONAL,
+                                      TRUST_JOIN_EXTERNAL)
         import dbus
         import dbus.mainloop.glib
         _bindings_installed = True
@@ -162,11 +164,18 @@ trust_output_params = (
         label=_('Trust type')),
     Str('truststatus',
         label=_('Trust status')),
+    Str('ipantadditionalsuffixes*',
+        label=_('UPN suffixes')),
 )
 
+# Trust type is a combination of ipanttrusttype and ipanttrustattributes
+# We shift trust attributes by 3 bits to left so bit 0 becomes bit 3 and
+# 2+(1 << 3) becomes 10.
 _trust_type_dict = {1 : _('Non-Active Directory domain'),
                     2 : _('Active Directory domain'),
-                    3 : _('RFC4120-compliant Kerberos realm')}
+                    3 : _('RFC4120-compliant Kerberos realm'),
+                    10: _('Non-transitive external trust to a domain in 
another Active Directory forest')}
+
 _trust_direction_dict = {1 : _('Trusting forest'),
                          2 : _('Trusted forest'),
                          3 : _('Two-way trust')}
@@ -189,14 +198,17 @@ DBUS_IFACE_TRUST = 'com.redhat.idm.trust'
 CRED_STYLE_SAMBA = 1
 CRED_STYLE_KERBEROS = 2
 
-def trust_type_string(level):
+LSA_TRUST_ATTRIBUTE_NON_TRANSITIVE = 0x00000001
+
+def trust_type_string(level, attrs):
     """
     Returns a string representing a type of the trust. The original field is 
an enum:
       LSA_TRUST_TYPE_DOWNLEVEL  = 0x00000001,
       LSA_TRUST_TYPE_UPLEVEL    = 0x00000002,
       LSA_TRUST_TYPE_MIT        = 0x00000003
     """
-    string = _trust_type_dict.get(int(level), _trust_type_dict_unknown)
+    transitive = int(attrs) & LSA_TRUST_ATTRIBUTE_NON_TRANSITIVE
+    string = _trust_type_dict.get(int(level) | (transitive << 3), 
_trust_type_dict_unknown)
     return unicode(string)
 
 def trust_direction_string(level):
@@ -677,6 +689,12 @@ sides.
              doc=(_('Establish bi-directional trust. By default trust is 
inbound one-way only.')),
              default=False,
         ),
+        Bool('external?',
+             label=_('External trust'),
+             cli_name='external',
+             doc=(_('Establish external trust to a domain in another forest. 
The trust is not transitive beyond the domain.')),
+             default=False,
+        ),
     )
 
     msg_summary = _('Added Active Directory trust for realm "%(value)s"')
@@ -735,12 +753,15 @@ sides.
                 fetch_trusted_domains_over_dbus(self.api, self.log, 
result['value'])
 
         # Format the output into human-readable values
+        attributes = int(result['result'].get('ipanttrustattributes', [0])[0])
         result['result']['trusttype'] = [trust_type_string(
-            result['result']['ipanttrusttype'][0])]
+            result['result']['ipanttrusttype'][0], attributes)]
         result['result']['trustdirection'] = [trust_direction_string(
             result['result']['ipanttrustdirection'][0])]
         result['result']['truststatus'] = [trust_status_string(
             result['verified'])]
+        if attributes:
+            result['result'].pop('ipanttrustattributes', None)
 
         del result['verified']
         result['result'].pop('ipanttrustauthoutgoing', None)
@@ -929,6 +950,11 @@ sides.
         trust_type = TRUST_ONEWAY
         if options.get('bidirectional', False):
             trust_type = TRUST_BIDIRECTIONAL
+
+        # If we are forced to establish external trust, allow it
+        if options.get('external', False):
+            self.trustinstance.allow_behavior(TRUST_JOIN_EXTERNAL)
+
         # 1. Full access to the remote domain. Use admin credentials and
         # generate random trustdom password to do work on both sides
         if full_join:
@@ -1033,7 +1059,7 @@ class trust_mod(LDAPUpdate):
 class trust_find(LDAPSearch):
     __doc__ = _('Search for trusts.')
     has_output_params = LDAPSearch.has_output_params + trust_output_params +\
-                        (Str('ipanttrusttype'),)
+                        (Str('ipanttrusttype'), Str('ipanttrustattributes'))
 
     msg_summary = ngettext(
         '%(count)d trust matched', '%(count)d trusts matched', 0
@@ -1043,7 +1069,7 @@ class trust_find(LDAPSearch):
     # search needs to be done on a sub-tree scope
     def pre_callback(self, ldap, filters, attrs_list, base_dn, scope, *args, 
**options):
         # list only trust, not trust domains
-        trust_filter = '(ipaNTTrustPartner=*)'
+        trust_filter = 
'(&(ipaNTTrustPartner=*)(&(objectclass=ipaIDObject)(objectclass=ipaNTTrustedDomain)))'
         filter = ldap.combine_filters((filters, trust_filter), 
rules=ldap.MATCH_ALL)
         return (filter, base_dn, ldap.SCOPE_SUBTREE)
 
@@ -1060,10 +1086,13 @@ class trust_find(LDAPSearch):
 
         for attrs in entries:
             # Translate ipanttrusttype to trusttype if --raw not used
-            trust_type = attrs.get('ipanttrusttype', [None])[0]
+            trust_type = attrs.single_value.get('ipanttrusttype', None)
+            attributes = attrs.single_value.get('ipanttrustattributes', 0)
             if not options.get('raw', False) and trust_type is not None:
-                attrs['trusttype'] = 
trust_type_string(attrs['ipanttrusttype'][0])
+                attrs['trusttype'] = [trust_type_string(trust_type, 
attributes)]
                 del attrs['ipanttrusttype']
+                if attributes:
+                    del attrs['ipanttrustattributes']
 
         return truncated
 
@@ -1071,7 +1100,7 @@ class trust_find(LDAPSearch):
 class trust_show(LDAPRetrieve):
     __doc__ = _('Display information about a trust.')
     has_output_params = LDAPRetrieve.has_output_params + trust_output_params +\
-                        (Str('ipanttrusttype'), Str('ipanttrustdirection'))
+                        (Str('ipanttrusttype'), Str('ipanttrustdirection'), 
Str('ipanttrustattributes'))
 
     def execute(self, *keys, **options):
         result = super(trust_show, self).execute(*keys, **options)
@@ -1088,16 +1117,20 @@ class trust_show(LDAPRetrieve):
         # if --raw not used
 
         if not options.get('raw', False):
-            trust_type = entry_attrs.get('ipanttrusttype', [None])[0]
+            trust_type = entry_attrs.single_value.get('ipanttrusttype', None)
+            attributes = entry_attrs.single_value.get('ipanttrustattributes', 
0)
             if trust_type is not None:
-                entry_attrs['trusttype'] = trust_type_string(trust_type)
+                entry_attrs['trusttype'] = [trust_type_string(trust_type, 
attributes)]
                 del entry_attrs['ipanttrusttype']
 
-            dir_str = entry_attrs.get('ipanttrustdirection', [None])[0]
+            dir_str = entry_attrs.single_value.get('ipanttrustdirection', None)
             if dir_str is not None:
                 entry_attrs['trustdirection'] = 
[trust_direction_string(dir_str)]
                 del entry_attrs['ipanttrustdirection']
 
+            if attributes:
+                del entry_attrs['ipanttrustattributes']
+
         return dn
 
 
-- 
2.7.4

-- 
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

Reply via email to