[Freeipa-devel] Password Maxlife 0 causes expiration of 90 days

[Simo Sorce]

On Mon, 2015-06-08 at 08:42 +0200, Martin Kosek wrote:
> On 06/05/2015 05:07 PM, Simo Sorce wrote:
> > On Fri, 2015-06-05 at 10:37 -0400, Drew Erny wrote:
> >> On 06/04/2015 05:41 PM, Alexander Bokovoy wrote:
> >>> On Thu, 04 Jun 2015, Drew Erny wrote:
> >>>> https://fedorahosted.org/freeipa/ticket/2795
> >>>>
> >>>> I've tracked down the source of this bug; it's nutty C stuff.
> >>>>
> >>>> So, in daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c, when you 
> >>>> update password, the expiration time appears to be set in the 
> >>>> function ipapwd_CheckPolicy on line 631, which looks like
> >>>>
> >>>>     data->expireTime = data->timeNow + pol.max_pwd_life;
> >>>>
> >>>> So the bug has to be in how pol.max_pwd_life gets is value. So I 
> >>>> check around, pol is initialized like this:
> >>>>
> >>>>     struct ipapwd_policy pol = {0};
> >>>>     ...
> >>>>     pol.max_pwd_life = IPAPWD_DEFAULT_PWDLIFE;
> >>>>
> >>>> And IPAPWD_DEFAULT_PWDLIFE is a constant 90 days.
> >>>>
> >>>> But then the actual value of max_pwd_life is obtained by passing pol 
> >>>> into the function ipapwd_getPolicy on line 577 or 590, depending on 
> >>>> the password change type.
> >>>>
> >>>> Inside of ipapwd_getPolicy, there's a couple of lines starting at 
> >>>> line 393
> >>>>
> >>>>     tmpint = slapi_entry_attr_get_int(pe, "krbMaxPwdLife");
> >>>>     if (tmpint != 0) {
> >>>>         policy->max_pwd_life = tmpint;
> >>>>     }:
> >>>>
> >>>> Which sets the max password life to the returned value, unless this 
> >>>> function returns 0. However, the documentation from 
> >>>> /usr/include/dirsrv/slapi-plugin.h says that that function, 
> >>>> slapi_entry_attr_get_int, returns 0 if the entry does not contain 
> >>>> that attribute. So, since the value 0 is returned, an error is 
> >>>> assumed to have occurred that member of the struct is left 
> >>>> untouched... which means it's still set to the value it was set to 
> >>>> when it was initialized, 90 days.
> >>>>
> >>>> So, when the expireTime is set at line 631, it's set to 90 days 
> >>>> because the value returned by slapi_entry_attr_get_int is 0.
> >>>>
> >>>> I've checked to see if we can get some error context out of the pe 
> >>>> variable passed in, but it appears to be an opaque struct that the 
> >>>> user isn't meant to see the internals of.
> >>>>
> >>>> I'm not really sure what to do with this knowledge. The only thing I 
> >>>> can think would be to use another sentinel value, like -1, to 
> >>>> indicate that the password does not expire; or, otherwise, to 
> >>>> document that there is no way to have non-expiring passwords, and 
> >>>> administrators can only set value to some far-future date, and then 
> >>>> close this bug. Or, we could just set the default expiration date to 
> >>>> be somewhere far in the future. I'm not really qualified to make a 
> >>>> call on how to proceed with this, but I'm capable of making the 
> >>>> change if someone more senior decides.
> >>>>
> >>>> I can also totally see this issue with the interface of slapi-plugin 
> >>>> being the possible cause of many bugs.
> >>> You can use slapi_entry_attr_exists() to check if attribute does exist
> >>> and then treat result of slapi_entry_attr_get_int() as actual value.
> >>>
> >>> Otherwise, that's a great investigation!
> >>
> >> Using slapi_entry_attr_exists() clears us of having to worry about 
> >> getting an error condition back, but I'm still not confident how to 
> >> handle the 0 maximum. Should I just put in a far-future date?
> > 
> > The current behavior is completely intentional, not a side effect of the
> > code, the code was written that way intentionally.
> > 
> > However me may consider an RFE that requests different behavior, we
> > would have to devise a special value for krbMaxPwdLife that means
> > "infinite".
> 
> Maybe. If we do this, we should also ban "0" as krbMaxPwdLife as it confuses
> people.
> 
> Let us say, that the user sets krbMaxPwdLife to "infinite", what is the wished
> effect on the user entry? Should krbPasswordExpiration be simply removed/not
> added when password is being set? As we cannot put any special word there, it
> is GeneralizedTime syntax. I assume that our LDAP BIND/kinit code would need to
> be checked that it reacts properly to missing value in that case.

We should probably set a special value (MAX int or something) and then
instruct the code to skip checks if that value is found. I do not think
we should omit the expiration time, no exp time gives you the default
policy as that may be the result of a partial import.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York