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_ldap_ > > 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 :) > > 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? > > > 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). > > 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.) > > > The logic of the functions looks good. > > Thanks for the review > Jan One more pass, then we should be good to go.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
