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

2016-07-01 Thread 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.


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

2016-07-01 Thread 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 

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

2016-07-01 Thread Petr Vobornik
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


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

2016-07-01 Thread thierry bordaz



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);
}







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.




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









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


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

2016-07-01 Thread David Kupka

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

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


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

2016-07-01 Thread Pavel Vomacka

Hi David,

I did a functional review, and everything works well, so functional-ACK. 
But I did not do the code review.


On 07/01/2016 10:26 AM, 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 */'


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 ?

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


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






--
Pavel^3 Vomacka

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

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

2016-07-01 Thread thierry bordaz

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 */'


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 ?

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


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




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

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

2016-07-01 Thread David Kupka

On 30/06/16 21:34, 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


Updated patch attached.

--
David Kupka
From af5e8516cf743544f529c2cd234af91e5251380e Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Thu, 30 Jun 2016 08:52:33 +0200
Subject: [PATCH] Allow unexpiring passwords

Treat maxlife=0 in password policy as "never expire". Delete
krbPasswordExpiration in user entry when password should never expire.

https://fedorahosted.org/freeipa/ticket/2795
---
 daemons/ipa-kdb/ipa_kdb_passwords.c   |  6 +-
 daemons/ipa-kdb/ipa_kdb_principals.c  | 11 +++
 daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c  | 22 --
 daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c |  4 
 ipaserver/plugins/pwpolicy.py |  2 +-
 5 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c
index ad57181d5049f36c69044bb2c9cfe183d7e4ea25..a3d4fe2436da60d081040754780d3e815acb1473 100644
--- a/daemons/ipa-kdb/ipa_kdb_passwords.c
+++ b/daemons/ipa-kdb/ipa_kdb_passwords.c
@@ -253,7 +253,11 @@ krb5_error_code ipadb_get_pwd_expiration(krb5_context context,
 
 if (truexp) {
 if (ied->pol) {
-*expire_time = mod_time + ied->pol->max_pwd_life;
+if (ied->pol->max_pwd_life) {
+*expire_time = mod_time + ied->pol->max_pwd_life;
+} else {
+*expire_time = 0;
+}
 } else {
 *expire_time = mod_time + IPAPWD_DEFAULT_PWDLIFE;
 }
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index f1d3e9e89c2016b8a9ebad9c0c6fd46487a33a4b..6cdfa909452a4b55912b2a5a74648abd2053482a 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1850,6 +1850,11 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
"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);
+}
 if (kerr) {
 goto done;
 }
@@ -2105,6 +2110,12 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
 kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
expire_time, mod_op);
+if (expire_time == 0) {
+kerr = ipadb_get_ldap_mod_time(imods,
+   "krbPasswordExpiration",
+   expire_time, LDAP_MOD_DELETE);
+}
+
 if (kerr) {
 goto done;
 }
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
index 5dc606d22305cf63a16feca30aab2728bb20b80d..0bb50fc319e2b2520d36534d369ad42f95c80c8e 100644
--- 

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

2016-06-30 Thread David Kupka

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
From dc2250869ca4f527b9b353913ed5a750a6f0f6e5 Mon Sep 17 00:00:00 2001
From: David Kupka 
Date: Thu, 30 Jun 2016 08:52:33 +0200
Subject: [PATCH] Allow unexpiring passwords

Treat maxlife=0 in password policy as "never expire". Delete
krbPasswordExpiration in user entry when password should never expire.

https://fedorahosted.org/freeipa/ticket/2795
---
 daemons/ipa-kdb/ipa_kdb_passwords.c   |  6 +-
 daemons/ipa-kdb/ipa_kdb_principals.c  | 11 +++
 daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c  | 11 ++-
 daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c |  4 
 ipaserver/plugins/pwpolicy.py |  2 +-
 5 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_passwords.c b/daemons/ipa-kdb/ipa_kdb_passwords.c
index ad57181d5049f36c69044bb2c9cfe183d7e4ea25..a3d4fe2436da60d081040754780d3e815acb1473 100644
--- a/daemons/ipa-kdb/ipa_kdb_passwords.c
+++ b/daemons/ipa-kdb/ipa_kdb_passwords.c
@@ -253,7 +253,11 @@ krb5_error_code ipadb_get_pwd_expiration(krb5_context context,
 
 if (truexp) {
 if (ied->pol) {
-*expire_time = mod_time + ied->pol->max_pwd_life;
+if (ied->pol->max_pwd_life) {
+*expire_time = mod_time + ied->pol->max_pwd_life;
+} else {
+*expire_time = 0;
+}
 } else {
 *expire_time = mod_time + IPAPWD_DEFAULT_PWDLIFE;
 }
diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c b/daemons/ipa-kdb/ipa_kdb_principals.c
index f1d3e9e89c2016b8a9ebad9c0c6fd46487a33a4b..6cdfa909452a4b55912b2a5a74648abd2053482a 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -1850,6 +1850,11 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
"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);
+}
 if (kerr) {
 goto done;
 }
@@ -2105,6 +2110,12 @@ static krb5_error_code ipadb_entry_to_mods(krb5_context kcontext,
 kerr = ipadb_get_ldap_mod_time(imods,
"krbPasswordExpiration",
expire_time, mod_op);
+if (expire_time == 0) {
+kerr = ipadb_get_ldap_mod_time(imods,
+   "krbPasswordExpiration",
+   expire_time, LDAP_MOD_DELETE);
+}
+
 if (kerr) {
 goto done;
 }
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
index 5dc606d22305cf63a16feca30aab2728bb20b80d..de295e0b4cbd8eeff556d96be3db68705114b994 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
+++ 

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

2016-05-04 Thread Pavel Vomacka



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.


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


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

2016-05-04 Thread Simo Sorce
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.

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


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

2016-05-04 Thread Martin Kosek
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?

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