On Mon, 2012-07-02 at 12:02 -0400, Simo Sorce wrote:
0001 ACK
0002 ACK
0003 ACK
0004 ACK
0005 ACK
0006 NACK
1.
You should never allow to set a domain that differs from the realm name
in the AD provider, it is always assumed realm = domain in AD.
In AD both the realm and the domain are case insensitive however MIT
libs needs to use the Realm all upper case for compat reasons.
I think the best thing for now is to simply ignore KRB5_REALM (do not
even define AD_KRB5_REALM) and just always user the upper cased domain
for the realm variable unconditionally.
I left AD_KRB5_REALM defined for simplicity (I can set it in
ad_get_common_options() and use it everywhere else).
2.
This is not really a nack, but I noticed you are using only the basic
SRV records to resolve the DC. Please open a ticket to perform full DC
discovery, using CLDAP pings and handling 'sites'.
https://fedorahosted.org/sssd/ticket/1032
3.
I think we should also probably ignore SDAP_SASL_AUTHID and
SDAP_SASL_REALM, they are redundant in an AD environment I think, as you
cannot have arbitrary values in there.
There's no real reason to do so. No one is likely to set these, so I'm
just going to leave them alone.
4.
Do we really support sudo with AD ?
We support SUDO with the LDAP provider. AD consumes the LDAP provider.
So if someone wants to set up SUDO entries in their AD server, we'll
honor it.
5.
We probably want to always set the canonicalize bit by default, also
should we turn off ldap_referrals by default ?
We cannot set the canonicalize bit by default. There's a bug in AD 2008
and AD 2008R2 that causes it to return bad data that libkrb5 chokes on
if we try to perform a password-change with canonicalization enabled.
Everything else seem very close to the IPA provider so the code in
general is ok and needs only the mentioned minor fixes.
Thanks
0007 NACK
1.
We should probably not support krb5_try_kdcip() in the AD provider.
2.
Remove AD_KRB5_REALM/KRB5_REALM and always use upper(domain)
The rest looks fine.
Fixed (as mentioned above, I tied everything to ad_get_common_options())
New patches since review:
Patch 0008 is the same as sent earlier, Patch 0009 adds the AD provider
into the contrib spec (forgotten earlier).