On Mon, 2012-02-06 at 12:30 +0100, Jan Zelený wrote:
On Sun, 2012-02-05 at 21:18 +0100, Jan Zeleny wrote:
Stephen Gallagher sgallagh@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_l dap_ opt:test_check_num_opts:0: Failure 'IPA_OPTS_BASIC_TEST != SDAP_OPTS_BASIC' occured
Done
Nack. Just updating the IPA_OPTS_BASIC_TEST value isn't sufficient. You also need to add the equivalent line in ipa_common.c for ipa_def_ldap_opts. I made this same mistake in a patch last week, don't copy my bad behaviors :)
Good catch, fixed
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
You didn't change sdap_ldap_passwd_change_done(). But on further reflection, this should be named differently anyway (since we're not changing the password with this function, just the lastChanged time). Maybe call it sdap_lastchange_done() or similar?
Yes, I meant to rename that as well. I guess Sunday evening is not the best time to do patches for me ;-) Fixed
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.
Yeah, I'm beginning to get the sense that the original behavior was actually the customer using the password hash directly through the shadow map (and verified using pam_unix.so, not pam_ldap.so). I don't think we want to support that at all.
Our reasonable nod to this atrocity will be to modify the lastChange time (which you're doing correctly, assuming that the LDAP server ACIs are sufficiently stupid to allow this).
Frankly, the more I was diving into this, the less I liked it. I am truly curious if this change will do any good for the original bug reporter. I still had no luck reproducing it the way it was described in the bug.
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
ldap_parse_result() doesn't return an errno_t. It's an unspecified 'int'. (Yes, errno_t happens to be internally defined as an int, but let's play it safe here.)
My bad. Fixed
The logic of the functions looks good.
Thanks for the review Jan
One more pass, then we should be good to go.
Hopefully this is it. Just a note that I intentionally left out description in man page, I don't want to encourage this setup by any means.
Jan
Ack (with one minor modification) and pushed to master.
I changed the DEBUG log level of the sdap_modify_shadow_lastchange_done() if the ldap_parse_result() returned LDAP_SUCCESS. s/SSSDBG_OP_FAILURE/SSSDBG_TRACE_LIBS/