[SSSD] [PATCH] Send error message from krb5_child to backend.

Jakub Hrozek jhrozek at redhat.com
Thu Jun 13 16:04:15 UTC 2013


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



More information about the sssd-devel mailing list