[Freeipa-devel] [PATCH] 0042-0048 AD trusts support (master)

[Martin Kosek]

On Tue, 2012-04-03 at 16:57 +0200, Sumit Bose wrote:
> On Tue, Apr 03, 2012 at 01:41:35PM +0300, Alexander Bokovoy wrote:
> > Hi!
> > 
> > Attached are the current patches for adding support for Active Directory
> > trusts for FreeIPA v3 (master).
> > 
> > These are tested and working with samba4 build available in ipa-devel@
> > repo. You have to use --delegate until we'll get all the parts of the
> > Heimdal puzzle untangled and solved, and Simo patch 490 (s4u2proxy fix)
> > is committed as well.
> > 
> > Sumit asked me to send patches for review and commit to master so that
> > he can proceed with his changes (removal of kadmin.local use, SID
> > population task for 389-ds, etc). Without kadmin.local use fix these
> > patches are not working with SELinux enabled.
> > 
> > Patches have [../9] mark because they were generated out of my adwork
> > tree. I have merged two patches together for obvious change reason and
> > have left out Simo's s4u2proxy patch out, thus there are seven patches
> > proposed for commit.
> 
> I have tested the patches and they worked fine for me. They currently
> only work in F17, because it relies on the version of python-ldap
> shipped with F17. So it is an
> 
> ACK
> 
> form my side. It would be nice if someone more can have a look at the
> python parts if they are in agreement with the IPA standards (I expect
> they are :-).
> 
> bye,
> Sumit
> 
> > 
> > -- 
> > / Alexander Bokovoy
> 

Hello Alexander,

I read the patches with focus on Python parts, please check my
comments.

freeipa-abbra-0042-ticket-2192.patch:
1) s4u2proxy records that you add for new replicas should also be
removed
during replica uninstall. Otherwise you will get a warning when the
replica is being re-installed.

You can find the clean up code in replication.py in ipaserver/install in
function replica_cleanup()


freeipa-abbra-0044-ticket-1821.patch:
1) Missing i18n:

+trust_output_params = (
+    Str('ipantflatname',
+        label='Domain NetBIOS name'),
+    Str('ipantsecurityidentifier',
+        label='Domain Security Identifier'),
+    Str('trustdirection',
+        label='Trust direction'),
+    Str('trusttype',
+        label='Trust type'),
+)

2) This does not look nice (and returns False (i.e. not str) when level
is out of bounds):
+def trust_type_string(level):
+    return int(level) in (1,2,3) and (u'Forest',u'Cross-Forest',u'MIT')[int(level)-1]
+
+def trust_direction_string(level):
+    return int(level) in (1,2,3) and (u'Downlevel',u'Uplevel',u'Both directions')[int(level)-1]

Maybe something like this would be better (and i18n-ed):
_trust_type_dict = {1 : _('Forest'),
                    2 : _('Cross-Forest'),
                    3 : _('MIT')}
_trust_type_dict_unknown = _('Unknown')
def trust_type_string(level):
    string = _trust_type_dict.get(int(level), _trust_type_dict_unknown)
    return unicode(string)


3) I would not try to import ipaserver.dcerpc every time the command is
executed:
+        try:
+            import ipaserver.dcerpc
+        except Exception, e:
+            raise errors.NotFound(name=_('AD Trust setup'),
+                  reason=_('Cannot perform join operation without Samba
4 python bindings installed'))

I would rather do it once in the beginning and set a flag:

try:
    import ipaserver.dcerpc
     _bindings_installed = True
except Exception:
    _bindings_installed = False

...

+    def execute(self, *keys, **options):
+        # Join domain using full credentials and with random trustdom
+        # secret (will be generated by the join method)
+        trustinstance = None
+        if not _bindings_installed:
+            raise errors.NotFound(name=_('AD Trust setup'),
+                  reason=_('Cannot perform join operation without Samba
4 python bindings installed'))


4) Another import inside a function:
+        def arcfour_encrypt(key, data):
+            from Crypto.Cipher import ARC4
+            c = ARC4.new(key)
+            return c.encrypt(data)


HTH,
Martin