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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code