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

[Alexander Bokovoy]

Hi Martin!

On Thu, 12 Apr 2012, Martin Kosek wrote:
>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()
thanks.



>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'),
>+)
Ok, will fix.


>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)
ok, makes sense. We'll need to do it to both directions later (not now).

>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
>
>...
The idea was that this code is only executed on the server. We need to
differentiate between:
- running on client
- running on server, no samba4 python bindings
- running on server with samba4 python bindings

By making it executed all time you are affecting the client code as
well while with current approach it only affects server side.


>+    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)
Same here, it is only needed on server side.

Let us get consensus over 3) and 4) and I'll fix patches altogether (and
push).

-- 
/ Alexander Bokovoy