[SSSD] [PATCH] warn user if password is about to expire

Sumit Bose sbose at redhat.com
Wed Nov 28 16:39:38 UTC 2012


On Wed, Nov 28, 2012 at 04:54:45PM +0100, Pavel Březina wrote:
> On 11/28/2012 11:15 AM, Sumit Bose wrote:
> >On Tue, Nov 20, 2012 at 11:18:22AM +0100, Pavel Březina wrote:
> >>On 11/19/2012 04:59 PM, Pavel Březina wrote:
> >>>https://fedorahosted.org/sssd/ticket/1638
> >>
> >>Self nack.
> >>
> >>>-            if (pwd_exp_warning == 0 ||
> >>>-                difftime(now + pwd_exp_warning, ppolicy->expire) > 0.0) {
> >>>+            if (pwd_exp_warning != 0 &&
> >>>+                difftime(pwd_exp_warning, ppolicy->expire) > 0.0) {
> >>>                 goto done;
> >>>             }
> >>
> >>We don't need to test if pwd_exp_warning != 0, because if it is 0 then
> >>difftime returns value <= 0 and the condition fails.
> >>
> >>New patch is attached.
> >>
> >
> >> From 3bb970535dd1d12016ed8ba431039b843be9da7c Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> >>Date: Mon, 19 Nov 2012 16:52:36 +0100
> >>Subject: [PATCH] warn user if password is about to expire
> >>
> >>https://fedorahosted.org/sssd/ticket/1638
> >>
> >>If pwd_exp_warning == 0, expiry warning should be printed if it is
> >>returned by server.
> >>
> >>If pwd_exp_warning > 0, expiry warning should be printed only if
> >>the password will expire in time <= pwd_exp_warning.
> >>
> >>ppolicy->expiry contains period in seconds after which the password
> >>expires. Not the exact timestamp. Thus we should not add 'now' to
> >>pwd_exp_warning.
> >>---
> >>  src/providers/ldap/ldap_auth.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >>diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
> >>index 32a2e04ea959a3cc81b88f5b2b19575c813e8adf..04fd24ffa4708dce53fb937e92efbbc102bef97c 100644
> >>--- a/src/providers/ldap/ldap_auth.c
> >>+++ b/src/providers/ldap/ldap_auth.c
> >>@@ -212,7 +212,6 @@ static errno_t check_pwexpire_ldap(struct pam_data *pd,
> >>      if (ppolicy->grace > 0 || ppolicy->expire > 0) {
> >>          uint32_t *data;
> >>          uint32_t *ptr;
> >>-        time_t now = time(NULL);
> >>          int ret;
> >>
> >>          if (pwd_exp_warning < 0) {
> >>@@ -231,8 +230,7 @@ static errno_t check_pwexpire_ldap(struct pam_data *pd,
> >>              ptr++;
> >>              *ptr = ppolicy->grace;
> >>          } else if (ppolicy->expire > 0) {
> >>-            if (pwd_exp_warning == 0 ||
> >>-                difftime(now + pwd_exp_warning, ppolicy->expire) > 0.0) {
> >>+            if (difftime(pwd_exp_warning, ppolicy->expire) > 0.0) {
> >>                  goto done;
> >>              }
> >>              *ptr = SSS_PAM_USER_INFO_EXPIRE_WARN;
> >
> >I haven't tested the patch yet, but I would recommend to drop difftime()
> >because pwd_exp_warning and ppolicy->expire are integers and just using
> >'<' or '>' would be easier and better to read.
> >
> >That said I think the current logic is flawed. As you said of
> >pwd_exp_warning is 0 difftime returns value <= 0 and the expire time is
> >added to the response. But if pwd_exp_warning is positive
> >ppolicy->expire must be larger than pwd_exp_warning to be added to the
> >response. This means the warning is only printed if it is still longer
> >than the time given in pwd_expiration_warning.
> >
> >bye,
> >Sumit
> 
> 
> You are correct sir. Thank you. Looks like I forgot to test the option.
> 
> I'm sending a new patch. I don't have time to test it myself right now,
> but I don't want to delay 1.9.3 if with any luck it is going to be
> released today.
> 
> I'll try it tomorrow if it's still pending, though I'm quite
> confident it will work.
> 
> 

Patch looks good, applies to master and compiles without warning. I
didn't had a chance to test it either so I'll give a tentative ACK to
not delay 1.9.3. I'll try to do some testing later today.

bye,
Sumit



More information about the sssd-devel mailing list