On Sun, 2012-02-05 at 21:18 +0100, Jan Zeleny wrote:
> 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_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