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`.
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.
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.
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']
"""
--
Martin^3 Babinsky
--
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
