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 <[email protected]> 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/
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
