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

[Drew Erny]

On 06/08/2015 02:42 AM, 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.

I think 0 or -1 are both good sentinel values to indicate no password 
life, and it's probably unlikely that anyone is relying on the 
functionality of 0=90 days (especially if that functionality is 
undocumented, but I'm not sure if it is), so it might be safe to change 
when we roll over to 4.2.

Then, whichever flag we settle on, in the C code for password changes 
that sets the new expiration time, we just set that expiration time to 
the maximum value of time_t instead of adding the max lifetime.