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

[David Kupka]

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.

>>
>>>
>>> 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
>>>>
>>>>
>>>
>>>
>>
>


-- 
David Kupka