[PATCH] print password complexity hint when change password failed

[Pavel Březina]

Now the output looks like:

$ su test-user
Password:
Password expired. Change your password now.
Current Password:
New password:
Retype new password:
Password change failed. Please make sure the password meets the 
complexity constraints.
su: incorrect password

[Jakub Hrozek]

On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
>  Now the output looks like:
>  
>  $ su test-user
>  Password:
>  Password expired. Change your password now.
>  Current Password:
>  New password:
>  Retype new password:
>  Password change failed. Please make sure the password meets the complexity
>  constraints.
>  su: incorrect password 
This works but I think the change is too broad, the hint would now be
printed for any password change failure without a specific message and
that might be misleading.

The reason why msg is zero-sized is that the user_error_message variable
in changepw_child() is overwritten with result_string which is "". So
one change might be to not overwrite user_error_message with empty
string.

The other change would be in changepw_child() -- we could
special-case when krb5_change_password() returns result_code 4
(Password change rejected) and then instead of sending
SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
(SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
check password complexity settings.

[Pavel Březina]

On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
>  On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
> > Now the output looks like:
> >
> > $ su test-user
> > Password:
> > Password expired. Change your password now.
> > Current Password:
> > New password:
> > Retype new password:
> > Password change failed. Please make sure the password meets the complexity
> > constraints.
> > su: incorrect password
>
>  This works but I think the change is too broad, the hint would now be
>  printed for any password change failure without a specific message and
>  that might be misleading.
>
>  The reason why msg is zero-sized is that the user_error_message variable
>  in changepw_child() is overwritten with result_string which is "". So
>  one change might be to not overwrite user_error_message with empty
>  string. 
For some reason Kerberos returns empty string but result_string.length = 30.

>  The other change would be in changepw_child() -- we could
>  special-case when krb5_change_password() returns result_code 4
>  (Password change rejected) and then instead of sending
>  SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
>  (SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
>  check password complexity settings. 
How about this? I avoided new error code and chose to fill server 
message in provider. The output now looks like:

Password change failed. Server message: Please make sure the password 
meets the complexity constraints.

[Jakub Hrozek]

On Tue, Jul 16, 2013 at 10:25:34AM +0200, Pavel Březina wrote:
>  On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
>  >On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
>  >>Now the output looks like:
>  >>
>  >>$ su test-user
>  >>Password:
>  >>Password expired. Change your password now.
>  >>Current Password:
>  >>New password:
>  >>Retype new password:
>  >>Password change failed. Please make sure the password meets the complexity
>  >>constraints.
>  >>su: incorrect password
>  >
>  >This works but I think the change is too broad, the hint would now be
>  >printed for any password change failure without a specific message and
>  >that might be misleading.
>  >
>  >The reason why msg is zero-sized is that the user_error_message variable
>  >in changepw_child() is overwritten with result_string which is "". So
>  >one change might be to not overwrite user_error_message with empty
>  >string.
>  
>  For some reason Kerberos returns empty string but result_string.length = 30.
>  
>  >The other change would be in changepw_child() -- we could
>  >special-case when krb5_change_password() returns result_code 4
>  >(Password change rejected) and then instead of sending
>  >SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
>  >(SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
>  >check password complexity settings.
>  
>  How about this? I avoided new error code and chose to fill server message in
>  provider. The output now looks like:
>  
>  Password change failed. Server message: Please make sure the password meets
>  the complexity constraints. 
This works for me, I tested a couple of usual scenarios to make sure we
don't print this message too frequently, but seems like everything is
working fine.

Ack

[Jakub Hrozek]

On Tue, Jul 16, 2013 at 08:03:04PM +0200, Jakub Hrozek wrote:
>  On Tue, Jul 16, 2013 at 10:25:34AM +0200, Pavel Březina wrote:
>  > On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
>  > >On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
>  > >>Now the output looks like:
>  > >>
>  > >>$ su test-user
>  > >>Password:
>  > >>Password expired. Change your password now.
>  > >>Current Password:
>  > >>New password:
>  > >>Retype new password:
>  > >>Password change failed. Please make sure the password meets the complexity
>  > >>constraints.
>  > >>su: incorrect password
>  > >
>  > >This works but I think the change is too broad, the hint would now be
>  > >printed for any password change failure without a specific message and
>  > >that might be misleading.
>  > >
>  > >The reason why msg is zero-sized is that the user_error_message variable
>  > >in changepw_child() is overwritten with result_string which is "". So
>  > >one change might be to not overwrite user_error_message with empty
>  > >string.
>  > 
>  > For some reason Kerberos returns empty string but result_string.length = 30.
>  > 
>  > >The other change would be in changepw_child() -- we could
>  > >special-case when krb5_change_password() returns result_code 4
>  > >(Password change rejected) and then instead of sending
>  > >SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
>  > >(SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
>  > >check password complexity settings.
>  > 
>  > How about this? I avoided new error code and chose to fill server message in
>  > provider. The output now looks like:
>  > 
>  > Password change failed. Server message: Please make sure the password meets
>  > the complexity constraints.
>  
>  This works for me, I tested a couple of usual scenarios to make sure we
>  don't print this message too frequently, but seems like everything is
>  working fine.
>  
>  Ack 
Pushed to master and sssd-1-10

[Jakub Hrozek]

On Wed, Jul 17, 2013 at 01:18:27PM +0200, Jakub Hrozek wrote:
>  On Tue, Jul 16, 2013 at 08:03:04PM +0200, Jakub Hrozek wrote:
>  > On Tue, Jul 16, 2013 at 10:25:34AM +0200, Pavel Březina wrote:
>  > > On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
>  > > >On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
>  > > >>Now the output looks like:
>  > > >>
>  > > >>$ su test-user
>  > > >>Password:
>  > > >>Password expired. Change your password now.
>  > > >>Current Password:
>  > > >>New password:
>  > > >>Retype new password:
>  > > >>Password change failed. Please make sure the password meets the
> complexity
>  > > >>constraints.
>  > > >>su: incorrect password
>  > > >
>  > > >This works but I think the change is too broad, the hint would now be
>  > > >printed for any password change failure without a specific message and
>  > > >that might be misleading.
>  > > >
>  > > >The reason why msg is zero-sized is that the user_error_message variable
>  > > >in changepw_child() is overwritten with result_string which is
> "". So
>  > > >one change might be to not overwrite user_error_message with empty
>  > > >string.
>  > > 
>  > > For some reason Kerberos returns empty string but result_string.length = 30.
>  > > 
>  > > >The other change would be in changepw_child() -- we could
>  > > >special-case when krb5_change_password() returns result_code 4
>  > > >(Password change rejected) and then instead of sending
>  > > >SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
>  > > >(SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
>  > > >check password complexity settings.
>  > > 
>  > > How about this? I avoided new error code and chose to fill server message in
>  > > provider. The output now looks like:
>  > > 
>  > > Password change failed. Server message: Please make sure the password meets
>  > > the complexity constraints.
>  > 
>  > This works for me, I tested a couple of usual scenarios to make sure we
>  > don't print this message too frequently, but seems like everything is
>  > working fine.
>  > 
>  > Ack
>  
>  Pushed to master and sssd-1-10 
FYI, attached is a backport of the patch on the 1.9 branch. I think the
backport was trivial enough so that I pushed it, but retroactive review
would be welcome, too.

Also, the patch was already verified by a customer.

[Jakub Hrozek]

On Sun, Aug 11, 2013 at 10:47:13PM +0200, Jakub Hrozek wrote:
>  On Wed, Jul 17, 2013 at 01:18:27PM +0200, Jakub Hrozek wrote:
>  > On Tue, Jul 16, 2013 at 08:03:04PM +0200, Jakub Hrozek wrote:
>  > > On Tue, Jul 16, 2013 at 10:25:34AM +0200, Pavel Březina wrote:
>  > > > On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
>  > > > >On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
>  > > > >>Now the output looks like:
>  > > > >>
>  > > > >>$ su test-user
>  > > > >>Password:
>  > > > >>Password expired. Change your password now.
>  > > > >>Current Password:
>  > > > >>New password:
>  > > > >>Retype new password:
>  > > > >>Password change failed. Please make sure the password meets the
> complexity
>  > > > >>constraints.
>  > > > >>su: incorrect password
>  > > > >
>  > > > >This works but I think the change is too broad, the hint would now be
>  > > > >printed for any password change failure without a specific message
> and
>  > > > >that might be misleading.
>  > > > >
>  > > > >The reason why msg is zero-sized is that the user_error_message
> variable
>  > > > >in changepw_child() is overwritten with result_string which is
> "". So
>  > > > >one change might be to not overwrite user_error_message with empty
>  > > > >string.
>  > > > 
>  > > > For some reason Kerberos returns empty string but result_string.length =
> 30.
>  > > > 
>  > > > >The other change would be in changepw_child() -- we could
>  > > > >special-case when krb5_change_password() returns result_code 4
>  > > > >(Password change rejected) and then instead of sending
>  > > > >SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
>  > > > >(SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
>  > > > >check password complexity settings.
>  > > > 
>  > > > How about this? I avoided new error code and chose to fill server message
> in
>  > > > provider. The output now looks like:
>  > > > 
>  > > > Password change failed. Server message: Please make sure the password
> meets
>  > > > the complexity constraints.
>  > > 
>  > > This works for me, I tested a couple of usual scenarios to make sure we
>  > > don't print this message too frequently, but seems like everything is
>  > > working fine.
>  > > 
>  > > Ack
>  > 
>  > Pushed to master and sssd-1-10
>  
>  FYI, attached is a backport of the patch on the 1.9 branch. I think the
>  backport was trivial enough so that I pushed it, but retroactive review
>  would be welcome, too.
>  
>  Also, the patch was already verified by a customer. 
Let's try it again, now with the patch attached.

[Michal Židek]

On 08/11/2013 10:49 PM, Jakub Hrozek wrote:
>  On Sun, Aug 11, 2013 at 10:47:13PM +0200, Jakub Hrozek wrote:
> > On Wed, Jul 17, 2013 at 01:18:27PM +0200, Jakub Hrozek wrote:
> >> On Tue, Jul 16, 2013 at 08:03:04PM +0200, Jakub Hrozek wrote:
> >>> On Tue, Jul 16, 2013 at 10:25:34AM +0200, Pavel Březina wrote:
> >>>> On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
> >>>>> On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
> >>>>>> Now the output looks like:
> >>>>>>
> >>>>>> $ su test-user
> >>>>>> Password:
> >>>>>> Password expired. Change your password now.
> >>>>>> Current Password:
> >>>>>> New password:
> >>>>>> Retype new password:
> >>>>>> Password change failed. Please make sure the password meets the
> complexity
> >>>>>> constraints.
> >>>>>> su: incorrect password
> >>>>>
> >>>>> This works but I think the change is too broad, the hint would now
> be
> >>>>> printed for any password change failure without a specific message
> and
> >>>>> that might be misleading.
> >>>>>
> >>>>> The reason why msg is zero-sized is that the user_error_message
> variable
> >>>>> in changepw_child() is overwritten with result_string which is
> "". So
> >>>>> one change might be to not overwrite user_error_message with empty
> >>>>> string.
> >>>>
> >>>> For some reason Kerberos returns empty string but result_string.length =
> 30.
> >>>>
> >>>>> The other change would be in changepw_child() -- we could
> >>>>> special-case when krb5_change_password() returns result_code 4
> >>>>> (Password change rejected) and then instead of sending
> >>>>> SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
> >>>>> (SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user to
> >>>>> check password complexity settings.
> >>>>
> >>>> How about this? I avoided new error code and chose to fill server message
> in
> >>>> provider. The output now looks like:
> >>>>
> >>>> Password change failed. Server message: Please make sure the password
> meets
> >>>> the complexity constraints.
> >>>
> >>> This works for me, I tested a couple of usual scenarios to make sure we
> >>> don't print this message too frequently, but seems like everything is
> >>> working fine.
> >>>
> >>> Ack
> >>
> >> Pushed to master and sssd-1-10
> >
> > FYI, attached is a backport of the patch on the 1.9 branch. I think the
> > backport was trivial enough so that I pushed it, but retroactive review
> > would be welcome, too.
> >
> > Also, the patch was already verified by a customer.
>
>  Let's try it again, now with the patch attached.
>  
Just tested on 1.9. The error message is printed properly. The code
looks good to me too.

Additional ACK from me.

Michal

[Michal Židek]

On 08/11/2013 11:31 PM, Michal Židek wrote:
>  On 08/11/2013 10:49 PM, Jakub Hrozek wrote:
> > On Sun, Aug 11, 2013 at 10:47:13PM +0200, Jakub Hrozek wrote:
> >> On Wed, Jul 17, 2013 at 01:18:27PM +0200, Jakub Hrozek wrote:
> >>> On Tue, Jul 16, 2013 at 08:03:04PM +0200, Jakub Hrozek wrote:
> >>>> On Tue, Jul 16, 2013 at 10:25:34AM +0200, Pavel Březina wrote:
> >>>>> On 07/10/2013 04:32 PM, Jakub Hrozek wrote:
> >>>>>> On Fri, Jun 21, 2013 at 02:49:51PM +0200, Pavel Březina wrote:
> >>>>>>> Now the output looks like:
> >>>>>>>
> >>>>>>> $ su test-user
> >>>>>>> Password:
> >>>>>>> Password expired. Change your password now.
> >>>>>>> Current Password:
> >>>>>>> New password:
> >>>>>>> Retype new password:
> >>>>>>> Password change failed. Please make sure the password meets
> the
> >>>>>>> complexity
> >>>>>>> constraints.
> >>>>>>> su: incorrect password
> >>>>>>
> >>>>>> This works but I think the change is too broad, the hint would
> >>>>>> now be
> >>>>>> printed for any password change failure without a specific
> >>>>>> message and
> >>>>>> that might be misleading.
> >>>>>>
> >>>>>> The reason why msg is zero-sized is that the user_error_message
> >>>>>> variable
> >>>>>> in changepw_child() is overwritten with result_string which is
> >>>>>> "". So
> >>>>>> one change might be to not overwrite user_error_message with
> empty
> >>>>>> string.
> >>>>>
> >>>>> For some reason Kerberos returns empty string but
> >>>>> result_string.length = 30.
> >>>>>
> >>>>>> The other change would be in changepw_child() -- we could
> >>>>>> special-case when krb5_change_password() returns result_code 4
> >>>>>> (Password change rejected) and then instead of sending
> >>>>>> SSS_PAM_USER_INFO_CHPASS_ERROR send a new code
> >>>>>> (SSS_PAM_USER_INFO_CHPASS_REJECTED?) that would instruct the user
> to
> >>>>>> check password complexity settings.
> >>>>>
> >>>>> How about this? I avoided new error code and chose to fill server
> >>>>> message in
> >>>>> provider. The output now looks like:
> >>>>>
> >>>>> Password change failed. Server message: Please make sure the
> >>>>> password meets
> >>>>> the complexity constraints.
> >>>>
> >>>> This works for me, I tested a couple of usual scenarios to make
> >>>> sure we
> >>>> don't print this message too frequently, but seems like everything
> is
> >>>> working fine.
> >>>>
> >>>> Ack
> >>>
> >>> Pushed to master and sssd-1-10
> >>
> >> FYI, attached is a backport of the patch on the 1.9 branch. I think the
> >> backport was trivial enough so that I pushed it, but retroactive review
> >> would be welcome, too.
> >>
> >> Also, the patch was already verified by a customer.
> >
> > Let's try it again, now with the patch attached.
> >
>
>  Just tested on 1.9. The error message is printed properly. The code 
               
           s/error message/hint
>  looks good to me too.
>
>  Additional ACK from me.
>
>  Michal