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. > >> >> 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. > >> >> 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. > >> >> thanks >> thierry
I don't see a strict NACK here. Given pvomacka's functional ACK, I have pushed it. We can fix corner cases later. Pushed to master: d2cb9ed327ee4003598d5e45d80ab7918b89eeed >> >> 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 >>> >> -- Petr Vobornik -- 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