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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel