> 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
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/