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

[Drew Erny]

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?