Stephen Gallagher <sgallagh(a)redhat.com> wrote:
On Fri, 2012-02-03 at 15:45 +0100, Jan Zelený wrote:
> Please note that I haven't fully tested this yet, the LDAP server
> configuration needed for this is a little bit twisted ;-) I will perform
> more testing during the weekend. Consider this patch being preliminary
> and don't push it until it's tested.
Nack.
You need to update the IPA LDAP options as well:
Running suite(s): ipa_ldap_opt
50%: Checks: 2, Failures: 1, Errors: 0
/home/sgallagh/workspace/sssd/src/tests/ipa_ldap_opt-tests.c:79:F:ipa_ldap_
opt:test_check_num_opts:0: Failure 'IPA_OPTS_BASIC_TEST != SDAP_OPTS_BASIC'
occured
Done
Just for aesthetic reasons, please don't use the prefix
sdap_ldap_*.
"sdap" is already shorthand for SSSD LDAP. sdap_ldap_result() is a
special case, since it's specifically a handler for "ldap_result"
structures. In general, I'd prefer that you call it something like
sdap_modify_shadow_lastchange_send().
Done
Why are you changing the user's password? That's not only out
of scope,
but you're setting their password in plaintext on the server, if I'm
reading this correctly (you're modifying the entry with the raw text of
the new password). We ONLY support password modification via
password-change extended operation.
The scope of
https://fedorahosted.org/sssd/ticket/1019 is ONLY to update
the last change time. We absolutely don't want people using shadow
passwords; they're extremely insecure (highly vulnerable to offline
dictionary attack).
As I wrote before, I'd like to consult this with you because I have no idea
how to test this and all my attempts were unsuccessful - I didn't even manage
to reproduce the original behavior.
Please modify the manpage for ldap_pwd_policy in sssd-ldap(5) as
"Note
that the current version of sssd cannot update this attribute during a
password change." is no longer accurate.
Done
Please don't conflate LDAP errors and errno errors in the
"ret" variable
in sdap_ldap_modify_passwd_done(). If for some reason openldap changed
the value of LDAP_SUCCESS, this would break under our noses.
Done
The logic of the functions looks good.
Thanks for the review
Jan