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

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

Reply via email to