[Freeipa-devel] [PATCH] pwpolicy: Do not expire passwords when maxlife is set to 0 (infinity).

[thierry bordaz]

On 07/01/2016 11:31 AM, David Kupka wrote:
> On 01/07/16 11:22, thierry bordaz wrote:
>>
>>
>> On 07/01/2016 10:46 AM, David Kupka wrote:
>>> Hello Thierry!
>>>
>>> Thanks for looking into it. I will try to answer your questions and
>>> comments inline.
>>>
>>> On 01/07/16 10:26, thierry bordaz wrote:
>>>> Hi David,
>>>>
>>>> The patch looks good but being not familiar with that code, my 
>>>> comments
>>>> may be absolutely wrong
>>>>
>>>> In ipadb_get_pwd_expiration, if it is not 'self' we set
>>>> '*export=mod_time'.
>>>> If for some reason 'mod_time==0', it has now a specific meaning 'not
>>>> expiring' . Does it match the comment '* not 'self', so reset */'
>>>
>>> mod_time should be timestamp of the modification operation. So
>>> mod_time == 0 should happen only when the change was performed in the
>>> very beginning of 70's.
>>
>> Right. My fault I did not understand that code.
>>>
>>>>
>>>> In ipadb_entry_to_mods, it deletes krbPasswordExpiration. But just
>>>> before it adds in the mods krbPasswordExpiration=0 or
>>>> krbPasswordExpiration=entry->pw_expiration
>>>> Could we skip those mods if entry->pw_expiration==0 or 
>>>> expire_time==0 ?
>>>
>>> That is exactly what I thought. But in that part of code I have no
>>> chance to check if the attribute is present in the entry or not. Also
>>> I can't catch and ignore the resulting error when deleting nonexistent
>>> attribute. Here only LDAPMod structures are filled but the execution
>>> is done in an other part of code.
>>> So I choose the easy path and always set the attribute and delete it
>>> right after if necessary.
>>
>> I think there is something a bit strange here.
>> To be able to delete the attribute we first need to set it to a specific
>> value then deleting the value we manage to delete the attribute.
>> I did not find a routine like ipa_get_ldap_mod_delattr. With such
>> routine I wonder if we could to something like:
>>
>>         if (entry->pw_expiration == 0) {
>>             kerr = ipadb_get_ldap_mod_delattr(imods,
>> "krbPasswordExpiration");
>>         } else {
>>
>>            kerr = ipadb_get_ldap_mod_time(imods,
>> "krbPasswordExpiration",
>> entry->pw_expiration,
>>                                        mod_op);
>>         }
>>
>>
>> Instead of
>>
>>         kerr = ipadb_get_ldap_mod_time(imods,
>>                                        "krbPasswordExpiration",
>>                                        entry->pw_expiration,
>>                                        mod_op);
>>         if (entry->pw_expiration == 0) {
>>             kerr = ipadb_get_ldap_mod_time(imods,
>> "krbPasswordExpiration",
>> entry->pw_expiration, LDAP_MOD_DELETE);
>>         }
>>
>>
>>
>
> At some point I have added such routine and tried same approach as you 
> do. But when the krbPasswordExpiration is not present in the entry 
> (user already has non-expiring password) there is an error later when 
> the mods are applied (err=16 ~ no such attribute).
>
> When I found this I decided to always LDAP_MOD_REPLACE the attribute 
> (replace operation does not fail when the attribute is missing). And 
> then remove it when it shouldn't be there. After that I decided to 
> remove my _del_attr routine because I didn't want to add new 
> unnecessary code.

Yes I agree and the code is working fine !
My concern is that ipadb_mods_new is trying to find empty mod slot. 
Hopefully there is no such empty slot currently.
But in theory the mods can have a random order and I think it is dangerous.
Adding a value to be able to delete the attribute afterward can fail if 
the ops are done in the opposite order.

>
>>>
>>>>
>>>> In ipapwd_SetPassword, ipapwd_post_modadd, same remark as above.
>>>>
>>>> Something that I am not sure is what is the expected relation between
>>>> passwordexpirationtime and krbPasswordExpiration
>>>
>>> I'm not sure either. I think we don't use passwordexpirationtime
>>> attribute.
>> I think they should be somehow linked, in fact it is looking it is what
>> happen in ipapwd_write_krb_keys.
>> But it looks it happens only when the krb keys are created.
>
> Probablby, I don't recall seeing passwordexpirationtime attribute used 
> anywhere.
>
>>>
>>>>
>>>> thanks
>>>> thierry
>>>>
>>>> On 06/30/2016 09:34 PM, David Kupka wrote:
>>>>> On 04/05/16 17:22, Pavel Vomacka wrote:
>>>>>>
>>>>>>
>>>>>> On 05/04/2016 04:36 PM, Simo Sorce wrote:
>>>>>>> On Wed, 2016-05-04 at 15:39 +0200, Martin Kosek wrote:
>>>>>>>> On 05/02/2016 02:28 PM, David Kupka wrote:
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2795
>>>>>>>> That patch looks suspiciously short given the struggles I saw in
>>>>>>>> http://www.redhat.com/archives/freeipa-devel/2015-June/msg00198.html 
>>>>>>>>
>>>>>>>> :-)
>>>>>>>>
>>>>>>>> Instead of setting to IPAPWD_END_OF_TIME, should we instead avoid
>>>>>>>> filling
>>>>>>>> "krbPasswordExpiration" attribute at all, i.e. have password
>>>>>>>> *without*
>>>>>>>> expiration? Or is krbPasswordExpiration mandatory?
>>>>>>> So I looked at the MIT code, and it seem like they are coping just
>>>>>>> fine
>>>>>>> with a missing (ie value = 0 internally) pw_expiration attribute.
>>>>>>>
>>>>>>> So if we make our code cope with omitting any expiration if the
>>>>>>> attribute is missing then yes, we can mark no expiration with 
>>>>>>> simply
>>>>>>> removing (or not setting) the krbPasswordExpiration attribute.
>>>>>>> The attribute itself is optional and can be omitted.
>>>>>>>
>>>>>>> I think this is a good idea, and is definitely better than 
>>>>>>> inventing
>>>>>>> a a
>>>>>>> magic value.
>>>>>>>
>>>>>>> Simo.
>>>>>>>
>>>>>> Just a note: I tested David's patch and it actually doesn't work 
>>>>>> when
>>>>>> the new password policy for ipausers group is created (priority = 0,
>>>>>> which should be the highest priority). The maxlife and minlife 
>>>>>> values
>>>>>> are empty. Even if I set the new password policy maxlife and
>>>>>> minlife to
>>>>>> 0 the result was that password will expire in 90 days. The patch
>>>>>> worked
>>>>>> correctly when I changed value of maxlife and minlife to 0 in
>>>>>> 'global_policy'. Then the password expiration was set to 2038-01-01.
>>>>>>
>>>>>
>>>>> Hello!
>>>>>
>>>>> I hope I've finally find all the places in ipa-kdb and ipa-pwd-extop
>>>>> plugins to tickle in order to have password that don't expire. 
>>>>> Updated
>>>>> patch attached.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/2795
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>