Before this patch, a different set of options was used when calling krb5_get_init_creds_password() for the changepw principal. Because this set of options did not contain the same FAST settings as the options for normal requests, all authentication would fail when the password of a FAST-only account would expire.
The two sets approach was cargo-cult from kinit where multiple requests could be issued using the same options set. However, in the case of krb5_child, only one request (or occasionally a well-defined second request) will be issued. Two option sets are therefore not required.
To fix this problem we removed the second option set used for changepw requests. All requests now use a single option set which is modified, if needed, for well-defined subsequent requests.
On Fri, Mar 07, 2014 at 12:23:18PM -0500, Nathaniel McCallum wrote:
Before this patch, a different set of options was used when calling krb5_get_init_creds_password() for the changepw principal. Because this set of options did not contain the same FAST settings as the options for normal requests, all authentication would fail when the password of a FAST-only account would expire.
The two sets approach was cargo-cult from kinit where multiple requests could be issued using the same options set. However, in the case of krb5_child, only one request (or occasionally a well-defined second request) will be issued. Two option sets are therefore not required.
To fix this problem we removed the second option set used for changepw requests. All requests now use a single option set which is modified, if needed, for well-defined subsequent requests.
Password changes with both expired and current password work fine with the patch. Tested with IPA and MS-AD.
The code looks good to me, there's no reason for set_changepw_options() to return anything, so making it 'void' makes sense.
I can't think of a reason to keep the two option structures separate. The way the kr->options instance is used clobbers the lifetime set with krb5_get_init_creds_opt_set_tkt_life but only when changepw principal is used.
ACK from me.
On Sun, Mar 09, 2014 at 01:04:38PM +0100, Jakub Hrozek wrote:
On Fri, Mar 07, 2014 at 12:23:18PM -0500, Nathaniel McCallum wrote:
Before this patch, a different set of options was used when calling krb5_get_init_creds_password() for the changepw principal. Because this set of options did not contain the same FAST settings as the options for normal requests, all authentication would fail when the password of a FAST-only account would expire.
The two sets approach was cargo-cult from kinit where multiple requests could be issued using the same options set. However, in the case of krb5_child, only one request (or occasionally a well-defined second request) will be issued. Two option sets are therefore not required.
To fix this problem we removed the second option set used for changepw requests. All requests now use a single option set which is modified, if needed, for well-defined subsequent requests.
Password changes with both expired and current password work fine with the patch. Tested with IPA and MS-AD.
The code looks good to me, there's no reason for set_changepw_options() to return anything, so making it 'void' makes sense.
I can't think of a reason to keep the two option structures separate. The way the kr->options instance is used clobbers the lifetime set with krb5_get_init_creds_opt_set_tkt_life but only when changepw principal is used.
ACK from me.
ACK from me as well. I checked the original tickets leading to this patch and found no reason which would force us to use a second set of options here.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Mar 10, 2014 at 10:25:56AM +0100, Sumit Bose wrote:
On Sun, Mar 09, 2014 at 01:04:38PM +0100, Jakub Hrozek wrote:
On Fri, Mar 07, 2014 at 12:23:18PM -0500, Nathaniel McCallum wrote:
Before this patch, a different set of options was used when calling krb5_get_init_creds_password() for the changepw principal. Because this set of options did not contain the same FAST settings as the options for normal requests, all authentication would fail when the password of a FAST-only account would expire.
The two sets approach was cargo-cult from kinit where multiple requests could be issued using the same options set. However, in the case of krb5_child, only one request (or occasionally a well-defined second request) will be issued. Two option sets are therefore not required.
To fix this problem we removed the second option set used for changepw requests. All requests now use a single option set which is modified, if needed, for well-defined subsequent requests.
Password changes with both expired and current password work fine with the patch. Tested with IPA and MS-AD.
The code looks good to me, there's no reason for set_changepw_options() to return anything, so making it 'void' makes sense.
I can't think of a reason to keep the two option structures separate. The way the kr->options instance is used clobbers the lifetime set with krb5_get_init_creds_opt_set_tkt_life but only when changepw principal is used.
ACK from me.
ACK from me as well. I checked the original tickets leading to this patch and found no reason which would force us to use a second set of options here.
bye, Sumit
Thanks, now that the patch has +2, pushed to master and sssd-1-11
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Nathaniel McCallum -
Sumit Bose