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_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.