[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password

[Jakub Hrozek]

This is a backport of a fix for
https://fedorahosted.org/sssd/ticket/1515 to the 1-8 branch.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Jakub Hrozek]

On Mon, Sep 10, 2012 at 05:36:50PM +0200, Jakub Hrozek wrote:
> This is a backport of a fix for
> https://fedorahosted.org/sssd/ticket/1515 to the 1-8 branch.

Joschi Brauschle nacked this patch off-list. I'm sending another version
of the patch attached, CC-ing Joschi this time.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Joschi Brauchle]

Hallo Jakub,

I have tested the latest patch successfully.

Please note that in the 2nd hunk,
1) the line with "KRB5_DEBUG(1, kerr);" should also be removed to avoid 
duplicate debug messages and
2) you could directly call "kerr_handle_error" instead of 
"kerr_to_status" (as kerr != 0).

See current version:
------------
@@ -777,9 +812,7 @@ static errno_t changepw_child(int fd, struct 
krb5_req *kr)
                                          kr->options);
      if (kerr != 0) {
          KRB5_DEBUG(1, kerr);
-        if (kerr == KRB5_KDC_UNREACH) {
-            pam_status = PAM_AUTHINFO_UNAVAIL;
-        }
+        pam_status = kerr_to_status(kerr);
          goto sendresponse;
      }
------------

But those are just minor cosmetic problems and do not affect the 
functionality of the patch.

Best regards,
Joschi Brauchle

On 09/12/2012 11:32 AM, Jakub Hrozek wrote:
> On Mon, Sep 10, 2012 at 05:36:50PM +0200, Jakub Hrozek wrote:
> > This is a backport of a fix for
> > https://fedorahosted.org/sssd/ticket/1515 to the 1-8 branch.
>
> Joschi Brauschle nacked this patch off-list. I'm sending another version
> of the patch attached, CC-ing Joschi this time.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Jakub Hrozek]

On Fri, Sep 14, 2012 at 05:34:17PM +0200, Joschi Brauchle wrote:
> Hallo Jakub,
>
> I have tested the latest patch successfully.
>
> Please note that in the 2nd hunk,
> the line with "KRB5_DEBUG(1, kerr);" should also be removed to
>
> avoid duplicate debug messages and
> 2) you could directly call "kerr_handle_error" instead of
> "kerr_to_status" (as kerr != 0).
> See current version:
>
> @@ -777,9 +812,7 @@ static errno_t changepw_child(int fd, struct
> krb5_req *kr)
>                                          kr->options);
>      if (kerr != 0) {
>          KRB5_DEBUG(1, kerr);
>    if (kerr == KRB5_KDC_UNREACH) {
>        pam_status = PAM_AUTHINFO_UNAVAIL;
>    }
>    pam_status = kerr_to_status(kerr);
>    goto sendresponse;
> }
>
> But those are just minor cosmetic problems and do not affect the
> functionality of the patch.
>
> Best regards,
> Joschi Brauchle

Thank you for the careful review, Joschi.

An updated patch that fixes the two items pointed out above is attached.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Pavel Březina]

On 09/17/2012 08:29 PM, Jakub Hrozek wrote:
> On Fri, Sep 14, 2012 at 05:34:17PM +0200, Joschi Brauchle wrote:
> > > Hallo Jakub,
> > >
> > > I have tested the latest patch successfully.
> > >
> > > Please note that in the 2nd hunk,
> > > the line with "KRB5_DEBUG(1, kerr);" should also be removed to
> > >
> > > avoid duplicate debug messages and
> > > 2) you could directly call "kerr_handle_error" instead of
> > > "kerr_to_status" (as kerr != 0).
> > > See current version:
> > >
> > > @@ -777,9 +812,7 @@ static errno_t changepw_child(int fd, struct
> > > krb5_req *kr)
> > >                                          kr->options);
> > >      if (kerr != 0) {
> > >          KRB5_DEBUG(1, kerr);
> > >    if (kerr == KRB5_KDC_UNREACH) {
> > >        pam_status = PAM_AUTHINFO_UNAVAIL;
> > >    }
> > >    pam_status = kerr_to_status(kerr);
> > >    goto sendresponse;
> > > }
> > >
> > > But those are just minor cosmetic problems and do not affect the
> > > functionality of the patch.
> > >
> > > Best regards,
> > > Joschi Brauchle
>
> Thank you for the careful review, Joschi.
>
> An updated patch that fixes the two items pointed out above is attached.

Nack (nitpick).
> 0001-KRB5-Return-PAM_AUTH_ERR-on-incorrect-password.patch
>
> +static int kerr_handle_error(krb5_error_code kerr)
> +{
> int pam_status;
> KRB5_DEBUG(1, kerr);

You should use SSSDBG macro here.
> switch (kerr) {
>    case KRB5_KDC_UNREACH:
>        pam_status = PAM_AUTHINFO_UNAVAIL;
>        break;
>    case KRB5KDC_ERR_KEY_EXP:
>        pam_status = PAM_NEW_AUTHTOK_REQD;
>        break;
>    case KRB5KRB_AP_ERR_BAD_INTEGRITY:
>        pam_status = PAM_AUTH_ERR;
>        break;
>    case KRB5KDC_ERR_PREAUTH_FAILED:
>        pam_status = PAM_CRED_ERR;
>        break;
>    default:
>        pam_status = PAM_SYSTEM_ERR;
>        break;
> }
> return pam_status;
>
> +}

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Jakub Hrozek]

On Thu, Sep 20, 2012 at 10:04:02AM +0200, Pavel Březina wrote:
> On 09/17/2012 08:29 PM, Jakub Hrozek wrote:
> > On Fri, Sep 14, 2012 at 05:34:17PM +0200, Joschi Brauchle wrote:
> > > > Hallo Jakub,
> > > >
> > > > I have tested the latest patch successfully.
> > > >
> > > > Please note that in the 2nd hunk,
> > > > the line with "KRB5_DEBUG(1, kerr);" should also be removed to
> > > >
> > > > avoid duplicate debug messages and
> > > > 2) you could directly call "kerr_handle_error" instead of
> > > > "kerr_to_status" (as kerr != 0).
> > > > See current version:
> > > >
> > > > @@ -777,9 +812,7 @@ static errno_t changepw_child(int fd, struct
> > > > krb5_req *kr)
> > > >                                          kr->options);
> > > >      if (kerr != 0) {
> > > >          KRB5_DEBUG(1, kerr);
> > > >    if (kerr == KRB5_KDC_UNREACH) {
> > > >        pam_status = PAM_AUTHINFO_UNAVAIL;
> > > >    }
> > > >    pam_status = kerr_to_status(kerr);
> > > >    goto sendresponse;
> > > > }
> > > >
> > > > But those are just minor cosmetic problems and do not affect the
> > > > functionality of the patch.
> > > >
> > > > Best regards,
> > > > Joschi Brauchle
> >
> > Thank you for the careful review, Joschi.
> >
> > An updated patch that fixes the two items pointed out above is attached.
>
> Nack (nitpick).
> > 0001-KRB5-Return-PAM_AUTH_ERR-on-incorrect-password.patch
> >
> > +static int kerr_handle_error(krb5_error_code kerr)
> > +{
> > int pam_status;
> > KRB5_DEBUG(1, kerr);
>
> You should use SSSDBG macro here.

Thanks, a new patch is attached.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Jakub Hrozek]

On Thu, Sep 20, 2012 at 10:30:24AM +0200, Jakub Hrozek wrote:
> Thanks, a new patch is attached.

Umm, now it is. Sorry.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Pavel Březina]

On 09/20/2012 10:32 AM, Jakub Hrozek wrote:
> On Thu, Sep 20, 2012 at 10:30:24AM +0200, Jakub Hrozek wrote:
> > Thanks, a new patch is attached.
>
> Umm, now it is. Sorry.

Ack.

[PATCH] KRB5: Return PAM_AUTH_ERR on incorrect password (1.8 backport)

[Jakub Hrozek]

On Thu, Sep 20, 2012 at 10:33:44AM +0200, Pavel Březina wrote:
> On 09/20/2012 10:32 AM, Jakub Hrozek wrote:
> > On Thu, Sep 20, 2012 at 10:30:24AM +0200, Jakub Hrozek wrote:
> > > Thanks, a new patch is attached.
> >
> > Umm, now it is. Sorry.
>
> Ack.

Pushed to sssd-1-8