Hi,
these two patches aim to fix trac ticket #672. While the first patch only makes a utility function public the second adds some new layers to the LDAP access provider. I've tried to make the changes in a way to make it easy to add new rules (#670) and new expire policies (#673, #674, #690).
I would like to ask the reviewers to check if the new code is really as flexible as I think and if it enough to evaluate the shadow expire attribute here. TIA
bye, Sumit
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/25/2010 08:25 AM, Sumit Bose wrote:
Hi,
these two patches aim to fix trac ticket #672. While the first patch only makes a utility function public the second adds some new layers to the LDAP access provider. I've tried to make the changes in a way to make it easy to add new rules (#670) and new expire policies (#673, #674, #690).
I would like to ask the reviewers to check if the new code is really as flexible as I think and if it enough to evaluate the shadow expire attribute here. TIA
bye, Sumit
Patch 0001: Ack.
Patch 0002: Nack. The manpage entry for ldap_account_expire_policy should list the valid values for this option. The patch comment notes that this is only "shadow" at present.
sssm_ldap_access_init() should throw an error (or at least warn and ignore it) if we get a duplicate access rule. e.g.: ldap_access_order = filter, filter
Please rename sdap_access_decide_offline(), sdap_access_retry(), sdap_access_connect_done() and sdap_access_get_access_done() to be clear that they apply to the access_filter.
I'm not sure we want to deny on a missing password expiration attribute. Some users might be lacking this attribute with the intent that it means "no expiration". Maybe we should add a strictness option here.
next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just PAM_PERM_DENIED.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/25/2010 08:25 AM, Sumit Bose wrote:
Hi,
these two patches aim to fix trac ticket #672. While the first patch only makes a utility function public the second adds some new layers to the LDAP access provider. I've tried to make the changes in a way to make it easy to add new rules (#670) and new expire policies (#673, #674, #690).
I would like to ask the reviewers to check if the new code is really as flexible as I think and if it enough to evaluate the shadow expire attribute here. TIA
bye, Sumit
Patch 0001: Ack.
Patch 0002: Nack. The manpage entry for ldap_account_expire_policy should list the valid values for this option. The patch comment notes that this is only "shadow" at present.
done
sssm_ldap_access_init() should throw an error (or at least warn and ignore it) if we get a duplicate access rule. e.g.: ldap_access_order = filter, filter
I prefer the error, because it might not be clear which order was intended.
Please rename sdap_access_decide_offline(), sdap_access_retry(), sdap_access_connect_done() and sdap_access_get_access_done() to be clear that they apply to the access_filter.
done
I'm not sure we want to deny on a missing password expiration attribute. Some users might be lacking this attribute with the intent that it means "no expiration". Maybe we should add a strictness option here.
I checked you pam_ldap handles this. A missing attribute or a value of 0 means "no expiration".
next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just PAM_PERM_DENIED.
done
Thanks for the review, new version attached.
bye, Sumit
Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkz3poUACgkQeiVVYja6o6OzvQCfesHt9x72o974xPOquRY0NtKt 1VAAn2/yojzemyFBhnv9Zbm3HLQk6rr1 =SinO -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 12/03/2010 06:56 AM, Sumit Bose wrote:
On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote: On 11/25/2010 08:25 AM, Sumit Bose wrote:
Hi,
these two patches aim to fix trac ticket #672. While the first patch only makes a utility function public the second adds some new layers to the LDAP access provider. I've tried to make the changes in a way to make it easy to add new rules (#670) and new expire policies (#673, #674, #690).
I would like to ask the reviewers to check if the new code is really as flexible as I think and if it enough to evaluate the shadow expire attribute here. TIA
bye, Sumit
Patch 0001: Ack.
Patch 0002: Nack. The manpage entry for ldap_account_expire_policy should list the valid values for this option. The patch comment notes that this is only "shadow" at present.
done
sssm_ldap_access_init() should throw an error (or at least warn and ignore it) if we get a duplicate access rule. e.g.: ldap_access_order = filter, filter
I prefer the error, because it might not be clear which order was intended.
Please rename sdap_access_decide_offline(), sdap_access_retry(), sdap_access_connect_done() and sdap_access_get_access_done() to be clear that they apply to the access_filter.
done
I'm not sure we want to deny on a missing password expiration attribute. Some users might be lacking this attribute with the intent that it means "no expiration". Maybe we should add a strictness option here.
I checked you pam_ldap handles this. A missing attribute or a value of 0 means "no expiration".
next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just PAM_PERM_DENIED.
done
Thanks for the review, new version attached.
Nack.
Typo in sssd-ldap.5.xml (tat)
typo in ldap_init.c (dublicate)
check_list_for_duplicates() could be made more scalable by doing (in pseudocode):
diff_string_lists(list, NULL, &nodupes, NULL, NULL); /* Pass in one string, and get the list of entries that were only in that string */ if (array_len(nodupes) < array_len(list)) { /* list had duplicates */ }
Internally, this just plays some tricks with a hash addition so that if the same key is used, it will be dropped. So the resulting array returned to *_list1_only will contain no duplicates. If the number of entries in the _list1_only array is fewer than the number of entries in the input list, duplicates were dropped.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Fri, Dec 03, 2010 at 08:11:39AM -0500, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 12/03/2010 06:56 AM, Sumit Bose wrote:
On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote: On 11/25/2010 08:25 AM, Sumit Bose wrote:
Hi,
these two patches aim to fix trac ticket #672. While the first patch only makes a utility function public the second adds some new layers to the LDAP access provider. I've tried to make the changes in a way to make it easy to add new rules (#670) and new expire policies (#673, #674, #690).
I would like to ask the reviewers to check if the new code is really as flexible as I think and if it enough to evaluate the shadow expire attribute here. TIA
bye, Sumit
Patch 0001: Ack.
Patch 0002: Nack. The manpage entry for ldap_account_expire_policy should list the valid values for this option. The patch comment notes that this is only "shadow" at present.
done
sssm_ldap_access_init() should throw an error (or at least warn and ignore it) if we get a duplicate access rule. e.g.: ldap_access_order = filter, filter
I prefer the error, because it might not be clear which order was intended.
Please rename sdap_access_decide_offline(), sdap_access_retry(), sdap_access_connect_done() and sdap_access_get_access_done() to be clear that they apply to the access_filter.
done
I'm not sure we want to deny on a missing password expiration attribute. Some users might be lacking this attribute with the intent that it means "no expiration". Maybe we should add a strictness option here.
I checked you pam_ldap handles this. A missing attribute or a value of 0 means "no expiration".
next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just PAM_PERM_DENIED.
done
Thanks for the review, new version attached.
Nack.
Typo in sssd-ldap.5.xml (tat)
typo in ldap_init.c (dublicate)
fixed
check_list_for_duplicates() could be made more scalable by doing (in pseudocode):
diff_string_lists(list, NULL, &nodupes, NULL, NULL); /* Pass in one string, and get the list of entries that were only in that string */ if (array_len(nodupes) < array_len(list)) { /* list had duplicates */ }
Internally, this just plays some tricks with a hash addition so that if the same key is used, it will be dropped. So the resulting array returned to *_list1_only will contain no duplicates. If the number of entries in the _list1_only array is fewer than the number of entries in the input list, duplicates were dropped.
We discussed this on irc and agreed that the order list is too short to justify the use of a hash. Additionally it is not necessary here to get a new list without duplicates, because it is sufficient to return an error after the first duplicate is found. To avoid an unintended use of this call I moved it frim util.c to ldap_init.c
New version attached.
bye, Sumit
Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkz47IgACgkQeiVVYja6o6PovwCfW8a0+X3KKPIJ1rRMm52yov0i xzMAniR20aYjU5uJqHKlpF8AOe2Zx4eZ =RXH6 -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 12/03/2010 04:22 PM, Sumit Bose wrote:
We discussed this on irc and agreed that the order list is too short to justify the use of a hash. Additionally it is not necessary here to get a new list without duplicates, because it is sufficient to return an error after the first duplicate is found. To avoid an unintended use of this call I moved it frim util.c to ldap_init.c
New version attached.
Ack to both patches.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 12/06/2010 08:48 AM, Stephen Gallagher wrote:
On 12/03/2010 04:22 PM, Sumit Bose wrote:
We discussed this on irc and agreed that the order list is too short to justify the use of a hash. Additionally it is not necessary here to get a new list without duplicates, because it is sufficient to return an error after the first duplicate is found. To avoid an unintended use of this call I moved it frim util.c to ldap_init.c
New version attached.
Ack to both patches.
Pushed to master.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
sssd-devel@lists.fedorahosted.org
-
Stephen Gallagher -
Sumit Bose