On Thu, Jun 13, 2013 at 06:04:15PM +0200, Jakub Hrozek wrote: > On Thu, Jun 13, 2013 at 05:51:18PM +0200, Ondrej Kos wrote: > > On 06/13/2013 05:37 PM, Jakub Hrozek wrote: > > >On Thu, Jun 13, 2013 at 04:40:43PM +0200, Ondrej Kos wrote: > > >>On 06/12/2013 05:51 PM, Jakub Hrozek wrote: > > >>>On Fri, May 24, 2013 at 04:52:49PM +0200, Lukas Slebodnik wrote: > > >>>>ehlo, > > >>>> > > >>>>Commit c6872e79e8496fd075e20aec0343ade99cca725c caused that password > > >>>>migration > > >>>>doesn't work using sssd. > > >>>> > > >>>>If pre authentication failed then we should send message to backend, > > >>>>so password migration could be detected. > > >>>> > > >>>>https://fedorahosted.org/sssd/ticket/1873 > > >>>> > > >>>>Patch is attached. > > >>>> > > >>>>LS > > >>> > > >>>Hi, > > >>> > > >>>sorry the review took such a long time. However, this is not the correct > > >>>approach. If you take a look at ipa_auth.c and how the migration is > > >>>performed in src/providers/ipa/ipa_auth.c: > > >>> > > >>>263 if (state->pd->cmd == SSS_PAM_AUTHENTICATE && > > >>>264 state->pd->pam_status == PAM_CRED_ERR) { > > >>>265 > > >>>266 req = get_password_migration_flag_send(state, state->ev, > > >>> > > >>>The migration is attempted if the krb5_auth request returns PAM_CRED_ERR > > >>>as the PAM error. Now the PAM error is converted from an error code the > > >>>Kerberos child process sends to the krb5_auth request in > > >>>krb5_auth_done(). > > >>> > > >>>When a user is migrated from LDAP then the authentication would fail > > >>>with KRB5_PREAUTH_FAILED in the krb5_child process and the krb5_child > > >>>would return ERR_AUTH_FAILED (see src/providers/krb5/krb5_child.c:1201). > > >>> > > >>>ERR_AUTH_FAILED is then converted in krb5_auth_done() into PAM_AUTH_ERR. > > >>> > > >>>So I see two options here: > > >>> 1) change the condition in ipa_auth.c from detecting PAM_CRED_ERR to > > >>> detecting PAM_AUTH_ERR. I don't like this because any authentication > > >>> failure would then check the migration flag. > > >>> 2) Add a new ERR_AUTH code, maybe ERR_PREAUTH_FAILED to krb5_child, > > >>> return that instead of ERR_AUTH_FAILED from the krb5_child and > > >>> convert ERR_PREAUTH_FAILED into PAM_CRED_ERR in krb5_auth_done. > > >>> > > >>>BTW to prove this was indeed the root cause, I tested by implementing 1) > > >>>option above: > > >>> > > >>>diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c > > >>>index 651196a..03576eb 100644 > > >>>--- a/src/providers/ipa/ipa_auth.c > > >>>+++ b/src/providers/ipa/ipa_auth.c > > >>>@@ -261,7 +261,7 @@ static void ipa_auth_handler_done(struct tevent_req > > >>>*req) > > >>> } > > >>> > > >>> if (state->pd->cmd == SSS_PAM_AUTHENTICATE && > > >>>- state->pd->pam_status == PAM_CRED_ERR) { > > >>>+ state->pd->pam_status == PAM_AUTH_ERR) { > > >>> > > >>> req = get_password_migration_flag_send(state, state->ev, > > >>> > > >>> state->ipa_auth_ctx->sdap_id_ctx, > > >>> > > >>>And the migration worked like a charm. > > >>>_______________________________________________ > > >>>sssd-devel mailing list > > >>>sssd-devel@lists.fedorahosted.org > > >>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > >>> > > >> > > >>Hi, > > >> > > >>Attached is a patch witch fixes this issue correctly, following Jakub's > > >>second provided option. > > >> > > > > > >The patch works fine, thank you. Some comments about the error code > > >below: > > > > > >>--- a/src/providers/krb5/krb5_auth.c > > >>+++ b/src/providers/krb5/krb5_auth.c > > >>@@ -1032,8 +1032,8 @@ static void krb5_auth_done(struct tevent_req > > >>*subreq) > > >> ret = EOK; > > >> goto done; > > >> > > >>- case ERR_AUTH_FAILED: > > >>- state->pam_status = PAM_AUTH_ERR; > > >>+ case ERR_PREAUTH_FAILED: > > >>+ state->pam_status = PAM_CRED_ERR; > > >> state->dp_err = DP_ERR_OK; > > > > > >we want to keep both cases here, ERR_AUTH_FAILED is still returned if > > >authentication fails with wrong password. > > > > > >> ret = EOK; > > >> goto done; > > >>diff --git a/src/providers/krb5/krb5_child.c > > >>b/src/providers/krb5/krb5_child.c > > >>index > > >>8f746a8db561928349ffed8b7434db2a113a1f86..dc1f06dd1e86954ca1f4c90e97d9eb95022e07fd > > >> 100644 > > >>--- a/src/providers/krb5/krb5_child.c > > >>+++ b/src/providers/krb5/krb5_child.c > > >>@@ -1174,7 +1174,7 @@ static errno_t map_krb5_error(krb5_error_code kerr) > > >> case KRB5KRB_AP_ERR_BAD_INTEGRITY: > > > > > >BAD_INTEGRITY should still return AUTH_FAILED. > > > > > >> case KRB5_PREAUTH_FAILED: > > >> case KRB5KDC_ERR_PREAUTH_FAILED: > > >>- return ERR_AUTH_FAILED; > > >>+ return ERR_PREAUTH_FAILED; > > >> > > >> default: > > >> return ERR_INTERNAL; > > >>diff --git a/src/util/util_errors.c b/src/util/util_errors.c > > >>index > > >>b617f540691a245d1132469a1f019bcb0eb6e775..096ac532fc7b9c8deb025ef9e948bc04ed0e0959 > > >> 100644 > > >>--- a/src/util/util_errors.c > > >>+++ b/src/util/util_errors.c > > >>@@ -35,6 +35,7 @@ struct err_string error_to_str[] = { > > >> { "Cached credentials are expired" }, /* ERR_CACHED_CREDS_EXPIRED */ > > >> { "Authentication Denied" }, /* ERR_AUTH_DENIED */ > > >> { "Authentication Failed" }, /* ERR_AUTH_FAILED */ > > > > > >I know I suggested ERR_PREAUTH_FAILED but now I'm thinking it might not > > >be the best code, because it's too Kerberos specific. So what about > > >using more generic ERR_CRED_INVALID ? > > > > > >>+ { "Preauthentication Failed"}, /* ERR_PREAUTH_FAILED */ > > >> { "Password Change Denied" }, /* ERR_CHPASS_DENIED */ > > >> { "Password Change Failed" }, /* ERR_CHPASS_FAILED */ > > >> { "Network I/O Error" }, /* ERR_NETWORK_IO */ > > >>diff --git a/src/util/util_errors.h b/src/util/util_errors.h > > >>index > > >>a602a6ea92f72a51f5e21342940b2072bbe9296d..2143774c3a2113e5ad372aa97770220c4cf3f859 > > >> 100644 > > >>--- a/src/util/util_errors.h > > >>+++ b/src/util/util_errors.h > > >>@@ -57,6 +57,7 @@ enum sssd_errors { > > >> ERR_CACHED_CREDS_EXPIRED, > > >> ERR_AUTH_DENIED, > > >> ERR_AUTH_FAILED, > > >>+ ERR_PREAUTH_FAILED, > > > > > >The new error code would have to be updated here, too. > > > > > >> ERR_CHPASS_DENIED, > > >> ERR_CHPASS_FAILED, > > >> ERR_NETWORK_IO, > > >>-- > > >>1.8.1.4 > > >> > > >_______________________________________________ > > >sssd-devel mailing list > > >sssd-devel@lists.fedorahosted.org > > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > > Invalid password still returns "Authentication failure", invalid > password when migrating returns "Insufficient credentials to access > authentication data" and password migration works. > > Ack.
Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel